Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Simplify Acknowledgement structure #7555

Merged
merged 4 commits into from
Nov 13, 2024
Merged

Simplify Acknowledgement structure #7555

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e5bce0b
simplify ack to array of byte arrays
AdityaSripal Nov 12, 2024
7a67e72
simplify test
AdityaSripal Nov 12, 2024
602b07a
fix merge
AdityaSripal Nov 12, 2024
11501cf
lint
AdityaSripal Nov 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 18 additions & 19 deletions modules/core/04-channel/v2/keeper/msg_server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package keeper

import (
"context"
"slices"
"time"

errorsmod "cosmossdk.io/errors"
Expand Down Expand Up @@ -130,9 +129,10 @@ func (k *Keeper) RecvPacket(ctx context.Context, msg *types.MsgRecvPacket) (*typ

// build up the recv results for each application callback.
ack := types.Acknowledgement{
AcknowledgementResults: []types.AcknowledgementResult{},
AppAcknowledgements: [][]byte{},
}

var isAsync bool
for _, pd := range msg.Packet.Payloads {
// Cache context so that we may discard state changes from callback if the acknowledgement is unsuccessful.
cacheCtx, writeFn = sdkCtx.CacheContext()
Expand All @@ -147,23 +147,26 @@ func (k *Keeper) RecvPacket(ctx context.Context, msg *types.MsgRecvPacket) (*typ
sdkCtx.EventManager().EmitEvents(internalerrors.ConvertToErrorEvents(cacheCtx.EventManager().Events()))
}

ack.AcknowledgementResults = append(ack.AcknowledgementResults, types.AcknowledgementResult{
AppName: pd.DestinationPort,
RecvPacketResult: res,
})
if res.Status == types.PacketStatus_Async {
// Set packet acknowledgement to async if any of the acknowledgements are async.
isAsync = true
// Return error if there is more than 1 payload
// TODO: Handle case where there are multiple payloads
if len(msg.Packet.Payloads) > 1 {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently just error for multipayload in async ack case

return nil, errorsmod.Wrapf(types.ErrInvalidPacket, "packet with multiple payloads cannot have async acknowledgement")
}
}

// append app acknowledgement to the overall acknowledgement
ack.AppAcknowledgements = append(ack.AppAcknowledgements, res.Acknowledgement)
}

// note this should never happen as the payload would have had to be empty.
if len(ack.AcknowledgementResults) == 0 {
if len(ack.AppAcknowledgements) == 0 {
sdkCtx.Logger().Error("receive packet failed", "source-channel", msg.Packet.SourceChannel, "error", errorsmod.Wrap(err, "invalid acknowledgement results"))
return &types.MsgRecvPacketResponse{Result: types.FAILURE}, errorsmod.Wrapf(err, "receive packet failed source-channel %s invalid acknowledgement results", msg.Packet.SourceChannel)
}

// NOTE: TBD how we will handle async acknowledgements with more than one payload.
isAsync := slices.ContainsFunc(ack.AcknowledgementResults, func(ackResult types.AcknowledgementResult) bool {
return ackResult.RecvPacketResult.Status == types.PacketStatus_Async
})

if !isAsync {
// Set packet acknowledgement only if the acknowledgement is not async.
// NOTE: IBC applications modules may call the WriteAcknowledgement asynchronously if the
Expand Down Expand Up @@ -203,14 +206,10 @@ func (k *Keeper) Acknowledgement(ctx context.Context, msg *types.MsgAcknowledgem
return nil, errorsmod.Wrap(err, "acknowledge packet verification failed")
}

recvResults := make(map[string]types.RecvPacketResult)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This puts the list into a map which will remove any subsequent acknowledgements from the same app. This is an error since we want to support sending multiple packets with the same portIDs.

Moreover it is unnecessary, the acknowledgement list is meant to match 1-1 to the list of payloads so we can associate each individual acknowledgement to the correct payload by looking at the portIDs in the matching payload in the payload list

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch, and yeah makes sense that we can just use the index.

for _, r := range msg.Acknowledgement.AcknowledgementResults {
recvResults[r.AppName] = r.RecvPacketResult
}

for _, pd := range msg.Packet.Payloads {
for i, pd := range msg.Packet.Payloads {
cbs := k.Router.Route(pd.SourcePort)
err := cbs.OnAcknowledgementPacket(ctx, msg.Packet.SourceChannel, msg.Packet.DestinationChannel, pd, recvResults[pd.DestinationPort].Acknowledgement, relayer)
ack := msg.Acknowledgement.AppAcknowledgements[i]
err := cbs.OnAcknowledgementPacket(ctx, msg.Packet.SourceChannel, msg.Packet.DestinationChannel, pd, ack, relayer)
Comment on lines +211 to +212
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See here: We just match using the index

if err != nil {
return nil, errorsmod.Wrapf(err, "failed OnAcknowledgementPacket for source port %s, source channel %s, destination channel %s", pd.SourcePort, msg.Packet.SourceChannel, msg.Packet.DestinationChannel)
}
Expand Down
77 changes: 32 additions & 45 deletions modules/core/04-channel/v2/keeper/msg_server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,59 +227,52 @@ func (suite *KeeperTestSuite) TestMsgSendPacket() {

func (suite *KeeperTestSuite) TestMsgRecvPacket() {
var (
path *ibctesting.Path
packet types.Packet
expectedAck types.Acknowledgement
path *ibctesting.Path
packet types.Packet
expRecvRes types.RecvPacketResult
)

testCases := []struct {
name string
malleate func()
expError error
name string
malleate func()
expError error
expAckWritten bool
}{
{
name: "success",
malleate: func() {},
expError: nil,
name: "success",
malleate: func() {},
expError: nil,
expAckWritten: true,
},
{
name: "success: failed recv result",
malleate: func() {
failedRecvResult := types.RecvPacketResult{
expRecvRes = types.RecvPacketResult{
Status: types.PacketStatus_Failure,
Acknowledgement: mock.MockFailPacketData,
}

// a failed ack should be returned by the application.
expectedAck.AcknowledgementResults[0].RecvPacketResult = failedRecvResult

path.EndpointB.Chain.GetSimApp().MockModuleV2B.IBCApp.OnRecvPacket = func(ctx context.Context, sourceChannel string, destinationChannel string, data types.Payload, relayer sdk.AccAddress) types.RecvPacketResult {
return failedRecvResult
}
},
expError: nil,
expAckWritten: true,
},
{
name: "success: async recv result",
malleate: func() {
asyncResult := types.RecvPacketResult{
expRecvRes = types.RecvPacketResult{
Status: types.PacketStatus_Async,
Acknowledgement: nil,
}

// an async ack should be returned by the application.
expectedAck.AcknowledgementResults[0].RecvPacketResult = asyncResult

path.EndpointB.Chain.GetSimApp().MockModuleV2B.IBCApp.OnRecvPacket = func(ctx context.Context, sourceChannel string, destinationChannel string, data types.Payload, relayer sdk.AccAddress) types.RecvPacketResult {
return asyncResult
}
},
expError: nil,
expAckWritten: false,
},
{
name: "success: NoOp",
malleate: func() {
suite.chainB.App.GetIBCKeeper().ChannelKeeperV2.SetPacketReceipt(suite.chainB.GetContext(), packet.DestinationChannel, packet.Sequence)
expectedAck = types.Acknowledgement{}
},
expError: nil,
expAckWritten: false,
},
{
name: "failure: counterparty not found",
Expand Down Expand Up @@ -314,18 +307,19 @@ func (suite *KeeperTestSuite) TestMsgRecvPacket() {
packet, err = path.EndpointA.MsgSendPacket(timeoutTimestamp, mockv2.NewMockPayload(mockv2.ModuleNameA, mockv2.ModuleNameB))
suite.Require().NoError(err)

// default expected ack is a single successful recv result for moduleB.
expectedAck = types.Acknowledgement{
AcknowledgementResults: []types.AcknowledgementResult{
{
AppName: mockv2.ModuleNameB,
RecvPacketResult: mockv2.MockRecvPacketResult,
},
},
}
// default expected receive result is a single successful recv result for moduleB.
expRecvRes = mockv2.MockRecvPacketResult

tc.malleate()

// expectedAck is derived from the expected recv result.
expectedAck := types.Acknowledgement{AppAcknowledgements: [][]byte{expRecvRes.Acknowledgement}}

// modify the callback to return the expected recv result.
path.EndpointB.Chain.GetSimApp().MockModuleV2B.IBCApp.OnRecvPacket = func(ctx context.Context, sourceChannel string, destinationChannel string, data types.Payload, relayer sdk.AccAddress) types.RecvPacketResult {
return expRecvRes
}

err = path.EndpointB.MsgRecvPacket(packet)

ck := path.EndpointB.Chain.GetSimApp().IBCKeeper.ChannelKeeperV2
Expand All @@ -340,7 +334,7 @@ func (suite *KeeperTestSuite) TestMsgRecvPacket() {

ackWritten := ck.HasPacketAcknowledgement(path.EndpointB.Chain.GetContext(), packet.DestinationChannel, packet.Sequence)

if len(expectedAck.AcknowledgementResults) == 0 || expectedAck.AcknowledgementResults[0].RecvPacketResult.Status == types.PacketStatus_Async {
if !tc.expAckWritten {
// ack should not be written for async app or if the packet receipt was already present.
suite.Require().False(ackWritten)
} else { // successful or failed acknowledgement
Expand Down Expand Up @@ -415,7 +409,7 @@ func (suite *KeeperTestSuite) TestMsgAcknowledgement() {
{
name: "failure: failed membership verification",
malleate: func() {
ack.AcknowledgementResults[0].RecvPacketResult.Acknowledgement = mock.MockFailPacketData
ack.AppAcknowledgements[0] = mock.MockFailPacketData
},
expError: errors.New("failed packet acknowledgement verification"),
},
Expand All @@ -438,14 +432,7 @@ func (suite *KeeperTestSuite) TestMsgAcknowledgement() {
suite.Require().NoError(err)

// Construct expected acknowledgement
ack = types.Acknowledgement{
AcknowledgementResults: []types.AcknowledgementResult{
{
AppName: mockv2.ModuleNameB,
RecvPacketResult: mockv2.MockRecvPacketResult,
},
},
}
ack = types.Acknowledgement{AppAcknowledgements: [][]byte{mockv2.MockRecvPacketResult.Acknowledgement}}

tc.malleate()

Expand Down
4 changes: 2 additions & 2 deletions modules/core/04-channel/v2/keeper/packet.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,8 +191,8 @@ func (k Keeper) WriteAcknowledgement(

// TODO: Validate Acknowledgment more thoroughly here after Issue #7472: https://github.com/cosmos/ibc-go/issues/7472

if len(ack.AcknowledgementResults) != len(packet.Payloads) {
return errorsmod.Wrapf(types.ErrInvalidAcknowledgement, "length of acknowledgement results %d does not match length of payload %d", len(ack.AcknowledgementResults), len(packet.Payloads))
if len(ack.AppAcknowledgements) != len(packet.Payloads) {
return errorsmod.Wrapf(types.ErrInvalidAcknowledgement, "length of app acknowledgement %d does not match length of app payload %d", len(ack.AppAcknowledgements), len(packet.Payloads))
}

// set the acknowledgement so that it can be verified on the other side
Expand Down
16 changes: 4 additions & 12 deletions modules/core/04-channel/v2/keeper/packet_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,7 @@ func (suite *KeeperTestSuite) TestWriteAcknowledgement() {
"failure: empty ack",
func() {
ack = types.Acknowledgement{
AcknowledgementResults: []types.AcknowledgementResult{},
AppAcknowledgements: [][]byte{},
}
},
types.ErrInvalidAcknowledgement,
Expand Down Expand Up @@ -307,12 +307,7 @@ func (suite *KeeperTestSuite) TestWriteAcknowledgement() {

// create standard ack that can be malleated
ack = types.Acknowledgement{
AcknowledgementResults: []types.AcknowledgementResult{
{
AppName: mockv2.ModuleNameB,
RecvPacketResult: mockv2.MockRecvPacketResult,
},
},
AppAcknowledgements: [][]byte{mockv2.MockRecvPacketResult.Acknowledgement},
}

suite.chainB.App.GetIBCKeeper().ChannelKeeperV2.SetPacketReceipt(suite.chainB.GetContext(), packet.DestinationChannel, packet.Sequence)
Expand Down Expand Up @@ -340,10 +335,7 @@ func (suite *KeeperTestSuite) TestAcknowledgePacket() {
packet types.Packet
err error
ack = types.Acknowledgement{
AcknowledgementResults: []types.AcknowledgementResult{{
AppName: mockv2.ModuleNameB,
RecvPacketResult: mockv2.MockRecvPacketResult,
}},
AppAcknowledgements: [][]byte{mockv2.MockRecvPacketResult.Acknowledgement},
}
freezeClient bool
)
Expand Down Expand Up @@ -397,7 +389,7 @@ func (suite *KeeperTestSuite) TestAcknowledgePacket() {
{
"failure: verify membership fails",
func() {
ack.AcknowledgementResults[0].RecvPacketResult = mockv2.MockFailRecvPacketResult
ack.AppAcknowledgements[0] = mockv2.MockFailRecvPacketResult.Acknowledgement
},
commitmenttypes.ErrInvalidProof,
},
Expand Down
4 changes: 2 additions & 2 deletions modules/core/04-channel/v2/types/commitment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,8 @@ func hashPayload(data Payload) []byte {
// CommitAcknowledgement returns the hash of the acknowledgement data.
func CommitAcknowledgement(acknowledgement Acknowledgement) []byte {
var buf []byte
for _, ack := range acknowledgement.GetAcknowledgementResults() {
hash := sha256.Sum256(ack.RecvPacketResult.GetAcknowledgement())
for _, ack := range acknowledgement.GetAppAcknowledgements() {
hash := sha256.Sum256(ack)
buf = append(buf, hash[:]...)
}

Expand Down
Loading
Loading