From 31133eb0cd4c6b543b10da24bae2aa5822926cad Mon Sep 17 00:00:00 2001 From: chatton Date: Wed, 22 May 2024 10:00:01 +0100 Subject: [PATCH 1/9] chore: specifically unmarshal v1 or v2 without brute force --- modules/apps/transfer/ibc_module.go | 54 ++++++++++++++++++----------- 1 file changed, 34 insertions(+), 20 deletions(-) diff --git a/modules/apps/transfer/ibc_module.go b/modules/apps/transfer/ibc_module.go index 62a5b975506..ee197dbd27a 100644 --- a/modules/apps/transfer/ibc_module.go +++ b/modules/apps/transfer/ibc_module.go @@ -174,25 +174,23 @@ func (IBCModule) OnChanCloseConfirm( return nil } -func (IBCModule) unmarshalPacketDataBytesToICS20V2(bz []byte) (types.FungibleTokenPacketDataV2, error) { - // TODO: remove support for this function parsing v1 packet data - // TODO: explicit check for packet data type against app version - - var datav1 types.FungibleTokenPacketData - if err := json.Unmarshal(bz, &datav1); err == nil { - if len(datav1.Denom) != 0 { - return convertinternal.PacketDataV1ToV2(datav1), nil +func (IBCModule) unmarshalPacketDataBytesToICS20V2(bz []byte, ics20Version string) (types.FungibleTokenPacketDataV2, error) { + switch ics20Version { + case types.V1: + var datav1 types.FungibleTokenPacketData + if err := json.Unmarshal(bz, &datav1); err != nil { + return types.FungibleTokenPacketDataV2{}, err } - } - - var data types.FungibleTokenPacketDataV2 - if err := json.Unmarshal(bz, &data); err == nil { - if len(data.Tokens) != 0 { - return data, nil + return convertinternal.PacketDataV1ToV2(datav1), nil + case types.V2: + var data types.FungibleTokenPacketDataV2 + if err := json.Unmarshal(bz, &data); err != nil { + return types.FungibleTokenPacketDataV2{}, err } + return data, nil + default: + return types.FungibleTokenPacketDataV2{}, errorsmod.Wrapf(types.ErrInvalidVersion, "invalid ICS-20 version: %s", ics20Version) } - - return types.FungibleTokenPacketDataV2{}, errorsmod.Wrapf(ibcerrors.ErrInvalidType, "cannot unmarshal ICS-20 transfer packet data") } // OnRecvPacket implements the IBCModule interface. A successful acknowledgement @@ -205,7 +203,12 @@ func (im IBCModule) OnRecvPacket( ) ibcexported.Acknowledgement { ack := channeltypes.NewResultAcknowledgement([]byte{byte(1)}) - data, ackErr := im.unmarshalPacketDataBytesToICS20V2(packet.GetData()) + ics20Version, ok := im.keeper.GetICS4Wrapper().GetAppVersion(ctx, packet.SourcePort, packet.SourceChannel) + if !ok { + return channeltypes.NewErrorAcknowledgement(fmt.Errorf("could not retrieve app version for channel (%s, %s)", packet.SourcePort, packet.SourceChannel)) + } + + data, ackErr := im.unmarshalPacketDataBytesToICS20V2(packet.GetData(), ics20Version) if ackErr != nil { im.keeper.Logger(ctx).Error(fmt.Sprintf("%s sequence %d", ackErr.Error(), packet.Sequence)) ack = channeltypes.NewErrorAcknowledgement(ackErr) @@ -267,7 +270,12 @@ func (im IBCModule) OnAcknowledgementPacket( return errorsmod.Wrapf(ibcerrors.ErrUnknownRequest, "cannot unmarshal ICS-20 transfer packet acknowledgement: %v", err) } - data, err := im.unmarshalPacketDataBytesToICS20V2(packet.GetData()) + ics20Version, ok := im.keeper.GetICS4Wrapper().GetAppVersion(ctx, packet.SourcePort, packet.SourceChannel) + if !ok { + return errorsmod.Wrapf(ibcerrors.ErrInvalidVersion, "could not retrieve app version for channel (%s, %s)", packet.SourcePort, packet.SourceChannel) + } + + data, err := im.unmarshalPacketDataBytesToICS20V2(packet.GetData(), ics20Version) if err != nil { return err } @@ -322,7 +330,12 @@ func (im IBCModule) OnTimeoutPacket( packet channeltypes.Packet, relayer sdk.AccAddress, ) error { - data, err := im.unmarshalPacketDataBytesToICS20V2(packet.GetData()) + ics20Version, ok := im.keeper.GetICS4Wrapper().GetAppVersion(ctx, packet.SourcePort, packet.SourceChannel) + if !ok { + return errorsmod.Wrapf(ibcerrors.ErrInvalidVersion, "could not retrieve app version for channel (%s, %s)", packet.SourcePort, packet.SourceChannel) + } + + data, err := im.unmarshalPacketDataBytesToICS20V2(packet.GetData(), ics20Version) if err != nil { return err } @@ -397,7 +410,8 @@ func (IBCModule) OnChanUpgradeOpen(ctx sdk.Context, portID, channelID string, pr // into a FungibleTokenPacketData. This function implements the optional // PacketDataUnmarshaler interface required for ADR 008 support. func (im IBCModule) UnmarshalPacketData(bz []byte) (interface{}, error) { - ftpd, err := im.unmarshalPacketDataBytesToICS20V2(bz) + // TODO: implement correctly after https://github.com/cosmos/ibc-go/pull/6341 is merged + ftpd, err := im.unmarshalPacketDataBytesToICS20V2(bz, types.V2) if err != nil { return nil, err } From de8521a72494ff0dd41279110af73a3c94337ca8 Mon Sep 17 00:00:00 2001 From: chatton Date: Wed, 22 May 2024 12:34:26 +0100 Subject: [PATCH 2/9] chore: fix TestPacketDataUnmarshalerInterface test in transfer module --- modules/apps/callbacks/callbacks_test.go | 2 +- modules/apps/transfer/ibc_module.go | 16 +++++++-- modules/apps/transfer/ibc_module_test.go | 44 +++++++++++++++++++++--- 3 files changed, 54 insertions(+), 8 deletions(-) diff --git a/modules/apps/callbacks/callbacks_test.go b/modules/apps/callbacks/callbacks_test.go index ef88db93483..9f00d9503d2 100644 --- a/modules/apps/callbacks/callbacks_test.go +++ b/modules/apps/callbacks/callbacks_test.go @@ -306,7 +306,7 @@ func GetExpectedEvent( ctx = ctx.WithGasMeter(gasMeter) // Mock packet. - packet := channeltypes.NewPacket(data, 0, srcPortID, "", "", "", clienttypes.ZeroHeight(), 0) + packet := channeltypes.NewPacket(data, 0, srcPortID, eventChannelID, "", "", clienttypes.ZeroHeight(), 0) if callbackType == types.CallbackTypeReceivePacket { callbackData, err = types.GetDestCallbackData(ctx, packetDataUnmarshaler, packet, maxCallbackGas) } else { diff --git a/modules/apps/transfer/ibc_module.go b/modules/apps/transfer/ibc_module.go index 2ebd7cb64b8..1cd0c602afc 100644 --- a/modules/apps/transfer/ibc_module.go +++ b/modules/apps/transfer/ibc_module.go @@ -179,15 +179,25 @@ func (IBCModule) unmarshalPacketDataBytesToICS20V2(bz []byte, ics20Version strin case types.V1: var datav1 types.FungibleTokenPacketData if err := json.Unmarshal(bz, &datav1); err != nil { + return types.FungibleTokenPacketDataV2{}, errorsmod.Wrap(err, "cannot unmarshal ICS20-V2 transfer packet data") + } + + if err := datav1.ValidateBasic(); err != nil { return types.FungibleTokenPacketDataV2{}, err } + return convertinternal.PacketDataV1ToV2(datav1), nil case types.V2: - var data types.FungibleTokenPacketDataV2 - if err := json.Unmarshal(bz, &data); err != nil { + var datav2 types.FungibleTokenPacketDataV2 + if err := json.Unmarshal(bz, &datav2); err != nil { + return types.FungibleTokenPacketDataV2{}, errorsmod.Wrap(err, "cannot unmarshal ICS20-V2 transfer packet data") + } + + if err := datav2.ValidateBasic(); err != nil { return types.FungibleTokenPacketDataV2{}, err } - return data, nil + + return datav2, nil default: return types.FungibleTokenPacketDataV2{}, errorsmod.Wrapf(types.ErrInvalidVersion, "invalid ICS-20 version: %s", ics20Version) } diff --git a/modules/apps/transfer/ibc_module_test.go b/modules/apps/transfer/ibc_module_test.go index 46a2302ff8e..ddf43d332a1 100644 --- a/modules/apps/transfer/ibc_module_test.go +++ b/modules/apps/transfer/ibc_module_test.go @@ -496,6 +496,7 @@ func (suite *TransferTestSuite) TestPacketDataUnmarshalerInterface() { sender = sdk.AccAddress(secp256k1.GenPrivKey().PubKey().Address()).String() receiver = sdk.AccAddress(secp256k1.GenPrivKey().PubKey().Address()).String() + path *ibctesting.Path data []byte initialPacketData interface{} ) @@ -508,6 +509,7 @@ func (suite *TransferTestSuite) TestPacketDataUnmarshalerInterface() { { "success: valid packet data single denom -> multidenom conversion with memo", func() { + path.EndpointA.ChannelConfig.Version = types.V1 initialPacketData = types.FungibleTokenPacketData{ Denom: ibctesting.TestCoin.Denom, Amount: ibctesting.TestCoin.Amount.String(), @@ -523,6 +525,7 @@ func (suite *TransferTestSuite) TestPacketDataUnmarshalerInterface() { { "success: valid packet data single denom -> multidenom conversion without memo", func() { + path.EndpointA.ChannelConfig.Version = types.V1 initialPacketData = types.FungibleTokenPacketData{ Denom: ibctesting.TestCoin.Denom, Amount: ibctesting.TestCoin.Amount.String(), @@ -538,6 +541,7 @@ func (suite *TransferTestSuite) TestPacketDataUnmarshalerInterface() { { "success: valid packet data single denom with trace -> multidenom conversion with trace", func() { + path.EndpointA.ChannelConfig.Version = types.V1 initialPacketData = types.FungibleTokenPacketData{ Denom: "transfer/channel-0/atom", Amount: ibctesting.TestCoin.Amount.String(), @@ -571,14 +575,15 @@ func (suite *TransferTestSuite) TestPacketDataUnmarshalerInterface() { nil, }, { - "success: valid packet data multidenom without memo", + "success: valid packet data multidenom nil trace", func() { + path.EndpointA.ChannelConfig.Version = types.V2 initialPacketData = types.FungibleTokenPacketDataV2{ Tokens: []types.Token{ { Denom: ibctesting.TestCoin.Denom, Amount: ibctesting.TestCoin.Amount.String(), - Trace: []string{""}, + Trace: nil, }, }, Sender: sender, @@ -590,21 +595,52 @@ func (suite *TransferTestSuite) TestPacketDataUnmarshalerInterface() { }, nil, }, + { + "failure: valid packet data multidenom without memo", + func() { + path.EndpointA.ChannelConfig.Version = types.V2 + initialPacketData = types.FungibleTokenPacketDataV2{ + Tokens: []types.Token{ + { + Denom: ibctesting.TestCoin.Denom, + Amount: ibctesting.TestCoin.Amount.String(), + Trace: []string{""}, + }, + }, + Sender: sender, + Receiver: receiver, + Memo: "", + } + + data = initialPacketData.(types.FungibleTokenPacketDataV2).GetBytes() + }, + errors.New("trace info must come in pairs of port and channel identifiers"), + }, { "failure: invalid packet data", func() { data = []byte("invalid packet data") }, - errors.New("cannot unmarshal ICS-20 transfer packet data"), + errors.New("cannot unmarshal ICS20-V2 transfer packet data"), }, } for _, tc := range testCases { tc := tc suite.Run(tc.name, func() { + path = ibctesting.NewTransferPath(suite.chainA, suite.chainB) + tc.malleate() - packetData, err := transfer.IBCModule{}.UnmarshalPacketData(suite.chainA.GetContext(), "", "", data) + path.Setup() + + transferStack, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.Route(types.ModuleName) + suite.Require().True(ok) + + unmarshalerStack, ok := transferStack.(porttypes.PacketDataUnmarshaler) + suite.Require().True(ok) + + packetData, err := unmarshalerStack.UnmarshalPacketData(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, data) expPass := tc.expError == nil if expPass { From f90da0ddfdd0d7b4efc78f7bf796eb7681b8e223 Mon Sep 17 00:00:00 2001 From: DimitrisJim Date: Wed, 22 May 2024 15:55:03 +0300 Subject: [PATCH 3/9] Pass dest values OnRecv, refactor GetExpectedEvents --- modules/apps/callbacks/callbacks_test.go | 4 ++-- modules/apps/transfer/ibc_module.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/modules/apps/callbacks/callbacks_test.go b/modules/apps/callbacks/callbacks_test.go index 9f00d9503d2..f9874896a22 100644 --- a/modules/apps/callbacks/callbacks_test.go +++ b/modules/apps/callbacks/callbacks_test.go @@ -305,11 +305,11 @@ func GetExpectedEvent( gasMeter := storetypes.NewGasMeter(remainingGas) ctx = ctx.WithGasMeter(gasMeter) - // Mock packet. - packet := channeltypes.NewPacket(data, 0, srcPortID, eventChannelID, "", "", clienttypes.ZeroHeight(), 0) if callbackType == types.CallbackTypeReceivePacket { + packet := channeltypes.NewPacket(data, seq, "", "", eventPortID, eventChannelID, clienttypes.ZeroHeight(), 0) callbackData, err = types.GetDestCallbackData(ctx, packetDataUnmarshaler, packet, maxCallbackGas) } else { + packet := channeltypes.NewPacket(data, seq, eventPortID, eventChannelID, "", "", clienttypes.ZeroHeight(), 0) callbackData, err = types.GetSourceCallbackData(ctx, packetDataUnmarshaler, packet, maxCallbackGas) } if err != nil { diff --git a/modules/apps/transfer/ibc_module.go b/modules/apps/transfer/ibc_module.go index 1cd0c602afc..093f2302e7b 100644 --- a/modules/apps/transfer/ibc_module.go +++ b/modules/apps/transfer/ibc_module.go @@ -213,7 +213,7 @@ func (im IBCModule) OnRecvPacket( ) ibcexported.Acknowledgement { ack := channeltypes.NewResultAcknowledgement([]byte{byte(1)}) - ics20Version, ok := im.keeper.GetICS4Wrapper().GetAppVersion(ctx, packet.SourcePort, packet.SourceChannel) + ics20Version, ok := im.keeper.GetICS4Wrapper().GetAppVersion(ctx, packet.DestinationPort, packet.DestinationChannel) if !ok { return channeltypes.NewErrorAcknowledgement(fmt.Errorf("could not retrieve app version for channel (%s, %s)", packet.SourcePort, packet.SourceChannel)) } From 7263c2a04b17937ff81c0d0c42f4574c377e396b Mon Sep 17 00:00:00 2001 From: chatton Date: Wed, 22 May 2024 15:55:01 +0100 Subject: [PATCH 4/9] chore: fixing TestGetCallbackData test --- modules/apps/callbacks/ibc_middleware_test.go | 43 ++++++++++-- .../apps/callbacks/types/callbacks_test.go | 69 ++++++++++++++++--- modules/apps/callbacks/types/types_test.go | 11 ++- modules/apps/transfer/ibc_module.go | 7 +- 4 files changed, 110 insertions(+), 20 deletions(-) diff --git a/modules/apps/callbacks/ibc_middleware_test.go b/modules/apps/callbacks/ibc_middleware_test.go index 0b487d93611..7c394bcaa9f 100644 --- a/modules/apps/callbacks/ibc_middleware_test.go +++ b/modules/apps/callbacks/ibc_middleware_test.go @@ -971,8 +971,13 @@ func (s *CallbacksTestSuite) TestProcessCallback() { } } -func (s *CallbacksTestSuite) TestUnmarshalPacketData() { +func (s *CallbacksTestSuite) TestUnmarshalPacketDataV1() { s.setupChains() + s.path.EndpointA.ChannelConfig.PortID = ibctesting.TransferPort + s.path.EndpointB.ChannelConfig.PortID = ibctesting.TransferPort + s.path.EndpointA.ChannelConfig.Version = transfertypes.V1 + s.path.EndpointB.ChannelConfig.Version = transfertypes.V1 + s.path.Setup() // We will pass the function call down the transfer stack to the transfer module // transfer stack UnmarshalPacketData call order: callbacks -> fee -> transfer @@ -1006,15 +1011,43 @@ func (s *CallbacksTestSuite) TestUnmarshalPacketData() { portID := s.path.EndpointA.ChannelConfig.PortID channelID := s.path.EndpointA.ChannelID - // Unmarshal ICS20 v1 packet data + // Unmarshal ICS20 v1 packet data into v2 packet data data := expPacketDataICS20V1.GetBytes() packetData, err := unmarshalerStack.UnmarshalPacketData(s.chainA.GetContext(), portID, channelID, data) s.Require().NoError(err) s.Require().Equal(expPacketDataICS20V2, packetData) +} + +func (s *CallbacksTestSuite) TestUnmarshalPacketDataV2() { + s.SetupTransferTest() + + // We will pass the function call down the transfer stack to the transfer module + // transfer stack UnmarshalPacketData call order: callbacks -> fee -> transfer + transferStack, ok := s.chainA.App.GetIBCKeeper().PortKeeper.Route(transfertypes.ModuleName) + s.Require().True(ok) - // Unmarshal ICS20 v1 packet data - data = expPacketDataICS20V2.GetBytes() - packetData, err = unmarshalerStack.UnmarshalPacketData(s.chainA.GetContext(), portID, channelID, data) + unmarshalerStack, ok := transferStack.(types.CallbacksCompatibleModule) + s.Require().True(ok) + + expPacketDataICS20V2 := transfertypes.FungibleTokenPacketDataV2{ + Tokens: []transfertypes.Token{ + { + Denom: ibctesting.TestCoin.Denom, + Amount: ibctesting.TestCoin.Amount.String(), + Trace: nil, + }, + }, + Sender: ibctesting.TestAccAddress, + Receiver: ibctesting.TestAccAddress, + Memo: fmt.Sprintf(`{"src_callback": {"address": "%s"}, "dest_callback": {"address":"%s"}}`, ibctesting.TestAccAddress, ibctesting.TestAccAddress), + } + + portID := s.path.EndpointA.ChannelConfig.PortID + channelID := s.path.EndpointA.ChannelID + + // Unmarshal ICS20 v2 packet data + data := expPacketDataICS20V2.GetBytes() + packetData, err := unmarshalerStack.UnmarshalPacketData(s.chainA.GetContext(), portID, channelID, data) s.Require().NoError(err) s.Require().Equal(expPacketDataICS20V2, packetData) } diff --git a/modules/apps/callbacks/types/callbacks_test.go b/modules/apps/callbacks/types/callbacks_test.go index 754118dda90..8af7aa944b5 100644 --- a/modules/apps/callbacks/types/callbacks_test.go +++ b/modules/apps/callbacks/types/callbacks_test.go @@ -2,6 +2,7 @@ package types_test import ( "fmt" + porttypes "github.com/cosmos/ibc-go/v8/modules/core/05-port/types" storetypes "cosmossdk.io/store/types" @@ -14,7 +15,6 @@ import ( transfertypes "github.com/cosmos/ibc-go/v8/modules/apps/transfer/types" clienttypes "github.com/cosmos/ibc-go/v8/modules/core/02-client/types" channeltypes "github.com/cosmos/ibc-go/v8/modules/core/04-channel/types" - porttypes "github.com/cosmos/ibc-go/v8/modules/core/05-port/types" ibctesting "github.com/cosmos/ibc-go/v8/testing" ibcmock "github.com/cosmos/ibc-go/v8/testing/mock" ) @@ -39,6 +39,9 @@ func (s *CallbacksTypesTestSuite) TestGetCallbackData() { { "success: source callback", func() { + s.path.EndpointA.ChannelConfig.Version = transfertypes.V1 + s.path.EndpointB.ChannelConfig.Version = transfertypes.V1 + remainingGas = 2_000_000 expPacketData := transfertypes.FungibleTokenPacketData{ Denom: ibctesting.TestCoin.Denom, @@ -60,6 +63,9 @@ func (s *CallbacksTypesTestSuite) TestGetCallbackData() { { "success: destination callback", func() { + s.path.EndpointA.ChannelConfig.Version = transfertypes.V1 + s.path.EndpointB.ChannelConfig.Version = transfertypes.V1 + callbackKey = types.DestinationCallbackKey remainingGas = 2_000_000 expPacketData := transfertypes.FungibleTokenPacketData{ @@ -82,6 +88,9 @@ func (s *CallbacksTypesTestSuite) TestGetCallbackData() { { "success: destination callback with 0 user defined gas limit", func() { + s.path.EndpointA.ChannelConfig.Version = transfertypes.V1 + s.path.EndpointB.ChannelConfig.Version = transfertypes.V1 + callbackKey = types.DestinationCallbackKey remainingGas = 2_000_000 expPacketData := transfertypes.FungibleTokenPacketData{ @@ -104,6 +113,9 @@ func (s *CallbacksTypesTestSuite) TestGetCallbackData() { { "success: source callback with gas limit < remaining gas < max gas", func() { + s.path.EndpointA.ChannelConfig.Version = transfertypes.V1 + s.path.EndpointB.ChannelConfig.Version = transfertypes.V1 + expPacketData := transfertypes.FungibleTokenPacketData{ Denom: ibctesting.TestCoin.Denom, Amount: ibctesting.TestCoin.Amount.String(), @@ -126,6 +138,9 @@ func (s *CallbacksTypesTestSuite) TestGetCallbackData() { { "success: source callback with remaining gas < gas limit < max gas", func() { + s.path.EndpointA.ChannelConfig.Version = transfertypes.V1 + s.path.EndpointB.ChannelConfig.Version = transfertypes.V1 + remainingGas = 100_000 expPacketData := transfertypes.FungibleTokenPacketData{ Denom: ibctesting.TestCoin.Denom, @@ -139,7 +154,7 @@ func (s *CallbacksTypesTestSuite) TestGetCallbackData() { types.CallbackData{ CallbackAddress: sender, SenderAddress: sender, - ExecutionGasLimit: 100_000, + ExecutionGasLimit: 97_715, CommitGasLimit: 200_000, }, nil, @@ -147,6 +162,9 @@ func (s *CallbacksTypesTestSuite) TestGetCallbackData() { { "success: source callback with remaining gas < max gas < gas limit", func() { + s.path.EndpointA.ChannelConfig.Version = transfertypes.V1 + s.path.EndpointB.ChannelConfig.Version = transfertypes.V1 + remainingGas = 100_000 expPacketData := transfertypes.FungibleTokenPacketData{ Denom: ibctesting.TestCoin.Denom, @@ -160,7 +178,7 @@ func (s *CallbacksTypesTestSuite) TestGetCallbackData() { types.CallbackData{ CallbackAddress: sender, SenderAddress: sender, - ExecutionGasLimit: 100_000, + ExecutionGasLimit: 97_715, CommitGasLimit: 1_000_000, }, nil, @@ -168,6 +186,9 @@ func (s *CallbacksTypesTestSuite) TestGetCallbackData() { { "success: destination callback with remaining gas < max gas < gas limit", func() { + s.path.EndpointA.ChannelConfig.Version = transfertypes.V1 + s.path.EndpointB.ChannelConfig.Version = transfertypes.V1 + callbackKey = types.DestinationCallbackKey remainingGas = 100_000 expPacketData := transfertypes.FungibleTokenPacketData{ @@ -182,7 +203,7 @@ func (s *CallbacksTypesTestSuite) TestGetCallbackData() { types.CallbackData{ CallbackAddress: sender, SenderAddress: "", - ExecutionGasLimit: 100_000, + ExecutionGasLimit: 97_715, CommitGasLimit: 1_000_000, }, nil, @@ -190,6 +211,9 @@ func (s *CallbacksTypesTestSuite) TestGetCallbackData() { { "success: source callback with max gas < remaining gas < gas limit", func() { + s.path.EndpointA.ChannelConfig.Version = transfertypes.V1 + s.path.EndpointB.ChannelConfig.Version = transfertypes.V1 + remainingGas = 2_000_000 expPacketData := transfertypes.FungibleTokenPacketData{ Denom: ibctesting.TestCoin.Denom, @@ -228,6 +252,9 @@ func (s *CallbacksTypesTestSuite) TestGetCallbackData() { { "failure: empty memo", func() { + s.path.EndpointA.ChannelConfig.Version = transfertypes.V1 + s.path.EndpointB.ChannelConfig.Version = transfertypes.V1 + expPacketData := transfertypes.FungibleTokenPacketData{ Denom: ibctesting.TestCoin.Denom, Amount: ibctesting.TestCoin.Amount.String(), @@ -243,6 +270,9 @@ func (s *CallbacksTypesTestSuite) TestGetCallbackData() { { "failure: empty address", func() { + s.path.EndpointA.ChannelConfig.Version = transfertypes.V1 + s.path.EndpointB.ChannelConfig.Version = transfertypes.V1 + expPacketData := transfertypes.FungibleTokenPacketData{ Denom: ibctesting.TestCoin.Denom, Amount: ibctesting.TestCoin.Amount.String(), @@ -258,6 +288,9 @@ func (s *CallbacksTypesTestSuite) TestGetCallbackData() { { "failure: space address", func() { + s.path.EndpointA.ChannelConfig.Version = transfertypes.V1 + s.path.EndpointB.ChannelConfig.Version = transfertypes.V1 + expPacketData := transfertypes.FungibleTokenPacketData{ Denom: ibctesting.TestCoin.Denom, Amount: ibctesting.TestCoin.Amount.String(), @@ -275,22 +308,38 @@ func (s *CallbacksTypesTestSuite) TestGetCallbackData() { for _, tc := range testCases { tc := tc s.Run(tc.name, func() { + s.SetupTest() + callbackKey = types.SourceCallbackKey - packetDataUnmarshaler = transfer.IBCModule{} + s.path.EndpointA.ChannelConfig.Version = transfertypes.V2 + s.path.EndpointA.ChannelConfig.PortID = transfertypes.ModuleName + s.path.EndpointB.ChannelConfig.Version = transfertypes.V2 + s.path.EndpointB.ChannelConfig.PortID = transfertypes.ModuleName - tc.malleate() + transferStack, ok := s.chainA.App.GetIBCKeeper().PortKeeper.Route(transfertypes.ModuleName) + s.Require().True(ok) - // Set up gas meter for context. - gasMeter := storetypes.NewGasMeter(remainingGas) - ctx := s.chain.GetContext().WithGasMeter(gasMeter) + packetDataUnmarshaler, ok = transferStack.(types.CallbacksCompatibleModule) + s.Require().True(ok) + + tc.malleate() - packet := channeltypes.NewPacket(packetData, 0, ibcmock.PortID, "", "", "", clienttypes.ZeroHeight(), 0) + s.path.Setup() var ( callbackData types.CallbackData err error ) + + // Set up gas meter for context. + gasMeter := storetypes.NewGasMeter(remainingGas) + + // we can use the context of chainA in both cases because we're doing direct function calls + // to GetDestCallbackData and GetSourceCallbackData on a single chain. + ctx := s.chainA.GetContext().WithGasMeter(gasMeter) + + packet := channeltypes.NewPacket(packetData, 0, transfertypes.PortID, s.path.EndpointA.ChannelID, transfertypes.PortID, s.path.EndpointB.ChannelID, clienttypes.ZeroHeight(), 0) if callbackKey == types.DestinationCallbackKey { callbackData, err = types.GetDestCallbackData(ctx, packetDataUnmarshaler, packet, uint64(1_000_000)) } else { diff --git a/modules/apps/callbacks/types/types_test.go b/modules/apps/callbacks/types/types_test.go index 7bb61f95162..5843f972249 100644 --- a/modules/apps/callbacks/types/types_test.go +++ b/modules/apps/callbacks/types/types_test.go @@ -15,12 +15,19 @@ type CallbacksTypesTestSuite struct { coord *ibctesting.Coordinator chain *ibctesting.TestChain + + chainA, chainB *ibctesting.TestChain + + path *ibctesting.Path } // SetupTest creates a coordinator with 1 test chain. -func (s *CallbacksTypesTestSuite) SetupSuite() { - s.coord = ibctesting.NewCoordinator(s.T(), 1) +func (s *CallbacksTypesTestSuite) SetupTest() { + s.coord = ibctesting.NewCoordinator(s.T(), 3) s.chain = s.coord.GetChain(ibctesting.GetChainID(1)) + s.chainA = s.coord.GetChain(ibctesting.GetChainID(2)) + s.chainB = s.coord.GetChain(ibctesting.GetChainID(3)) + s.path = ibctesting.NewPath(s.chainA, s.chainB) } func TestCallbacksTypesTestSuite(t *testing.T) { diff --git a/modules/apps/transfer/ibc_module.go b/modules/apps/transfer/ibc_module.go index 093f2302e7b..2e72a42857b 100644 --- a/modules/apps/transfer/ibc_module.go +++ b/modules/apps/transfer/ibc_module.go @@ -179,7 +179,7 @@ func (IBCModule) unmarshalPacketDataBytesToICS20V2(bz []byte, ics20Version strin case types.V1: var datav1 types.FungibleTokenPacketData if err := json.Unmarshal(bz, &datav1); err != nil { - return types.FungibleTokenPacketDataV2{}, errorsmod.Wrap(err, "cannot unmarshal ICS20-V2 transfer packet data") + return types.FungibleTokenPacketDataV2{}, errorsmod.Wrapf(ibcerrors.ErrInvalidType, "cannot unmarshal ICS20-V2 transfer packet data: %s", err.Error()) } if err := datav1.ValidateBasic(); err != nil { @@ -190,7 +190,7 @@ func (IBCModule) unmarshalPacketDataBytesToICS20V2(bz []byte, ics20Version strin case types.V2: var datav2 types.FungibleTokenPacketDataV2 if err := json.Unmarshal(bz, &datav2); err != nil { - return types.FungibleTokenPacketDataV2{}, errorsmod.Wrap(err, "cannot unmarshal ICS20-V2 transfer packet data") + return types.FungibleTokenPacketDataV2{}, errorsmod.Wrapf(ibcerrors.ErrInvalidType, "cannot unmarshal ICS20-V2 transfer packet data: %s", err.Error()) } if err := datav2.ValidateBasic(); err != nil { @@ -215,11 +215,12 @@ func (im IBCModule) OnRecvPacket( ics20Version, ok := im.keeper.GetICS4Wrapper().GetAppVersion(ctx, packet.DestinationPort, packet.DestinationChannel) if !ok { - return channeltypes.NewErrorAcknowledgement(fmt.Errorf("could not retrieve app version for channel (%s, %s)", packet.SourcePort, packet.SourceChannel)) + return channeltypes.NewErrorAcknowledgement(errorsmod.Wrapf(ibcerrors.ErrInvalidVersion, "could not retrieve app version for channel (%s, %s)", packet.SourcePort, packet.SourceChannel)) } data, ackErr := im.unmarshalPacketDataBytesToICS20V2(packet.GetData(), ics20Version) if ackErr != nil { + ackErr = errorsmod.Wrapf(ibcerrors.ErrInvalidType, ackErr.Error()) im.keeper.Logger(ctx).Error(fmt.Sprintf("%s sequence %d", ackErr.Error(), packet.Sequence)) ack = channeltypes.NewErrorAcknowledgement(ackErr) } From 4f8a28381cc19ad5e9441fbcc1effceccc3436d8 Mon Sep 17 00:00:00 2001 From: chatton Date: Wed, 22 May 2024 16:07:33 +0100 Subject: [PATCH 5/9] chore: fixed remaining tests in callbacks module --- .../apps/callbacks/types/callbacks_test.go | 43 +++++++++++++++---- 1 file changed, 34 insertions(+), 9 deletions(-) diff --git a/modules/apps/callbacks/types/callbacks_test.go b/modules/apps/callbacks/types/callbacks_test.go index 8af7aa944b5..052ece95ee8 100644 --- a/modules/apps/callbacks/types/callbacks_test.go +++ b/modules/apps/callbacks/types/callbacks_test.go @@ -2,7 +2,7 @@ package types_test import ( "fmt" - porttypes "github.com/cosmos/ibc-go/v8/modules/core/05-port/types" + storetypes "cosmossdk.io/store/types" @@ -11,10 +11,10 @@ import ( "github.com/cometbft/cometbft/crypto/secp256k1" "github.com/cosmos/ibc-go/modules/apps/callbacks/types" - "github.com/cosmos/ibc-go/v8/modules/apps/transfer" transfertypes "github.com/cosmos/ibc-go/v8/modules/apps/transfer/types" clienttypes "github.com/cosmos/ibc-go/v8/modules/core/02-client/types" channeltypes "github.com/cosmos/ibc-go/v8/modules/core/04-channel/types" + porttypes "github.com/cosmos/ibc-go/v8/modules/core/05-port/types" ibctesting "github.com/cosmos/ibc-go/v8/testing" ibcmock "github.com/cosmos/ibc-go/v8/testing/mock" ) @@ -361,6 +361,8 @@ func (s *CallbacksTypesTestSuite) TestGetCallbackData() { } func (s *CallbacksTypesTestSuite) TestGetSourceCallbackDataTransfer() { + s.SetupTest() + sender := sdk.AccAddress(secp256k1.GenPrivKey().PubKey().Address()).String() receiver := sdk.AccAddress(secp256k1.GenPrivKey().PubKey().Address()).String() @@ -380,19 +382,32 @@ func (s *CallbacksTypesTestSuite) TestGetSourceCallbackDataTransfer() { CommitGasLimit: 1_000_000, } - packetUnmarshaler := transfer.IBCModule{} + s.path.EndpointA.ChannelConfig.Version = transfertypes.V1 + s.path.EndpointA.ChannelConfig.PortID = transfertypes.ModuleName + s.path.EndpointB.ChannelConfig.Version = transfertypes.V1 + s.path.EndpointB.ChannelConfig.PortID = transfertypes.ModuleName + + transferStack, ok := s.chainA.App.GetIBCKeeper().PortKeeper.Route(transfertypes.ModuleName) + s.Require().True(ok) + + packetUnmarshaler, ok := transferStack.(types.CallbacksCompatibleModule) + s.Require().True(ok) + + s.path.Setup() // Set up gas meter for context. gasMeter := storetypes.NewGasMeter(2_000_000) - ctx := s.chain.GetContext().WithGasMeter(gasMeter) + ctx := s.chainA.GetContext().WithGasMeter(gasMeter) - packet := channeltypes.NewPacket(packetDataBytes, 0, ibcmock.PortID, "", "", "", clienttypes.ZeroHeight(), 0) + packet := channeltypes.NewPacket(packetDataBytes, 0, transfertypes.PortID, s.path.EndpointA.ChannelID, transfertypes.PortID, s.path.EndpointB.ChannelID, clienttypes.ZeroHeight(), 0) callbackData, err := types.GetSourceCallbackData(ctx, packetUnmarshaler, packet, 1_000_000) s.Require().NoError(err) s.Require().Equal(expCallbackData, callbackData) } func (s *CallbacksTypesTestSuite) TestGetDestCallbackDataTransfer() { + s.SetupTest() + sender := sdk.AccAddress(secp256k1.GenPrivKey().PubKey().Address()).String() receiver := sdk.AccAddress(secp256k1.GenPrivKey().PubKey().Address()).String() @@ -412,12 +427,22 @@ func (s *CallbacksTypesTestSuite) TestGetDestCallbackDataTransfer() { CommitGasLimit: 1_000_000, } - packetUnmarshaler := transfer.IBCModule{} + s.path.EndpointA.ChannelConfig.Version = transfertypes.V1 + s.path.EndpointA.ChannelConfig.PortID = transfertypes.ModuleName + s.path.EndpointB.ChannelConfig.Version = transfertypes.V1 + s.path.EndpointB.ChannelConfig.PortID = transfertypes.ModuleName - gasMeter := storetypes.NewGasMeter(2_000_000) - ctx := s.chain.GetContext().WithGasMeter(gasMeter) + transferStack, ok := s.chainA.App.GetIBCKeeper().PortKeeper.Route(transfertypes.ModuleName) + s.Require().True(ok) - packet := channeltypes.NewPacket(packetDataBytes, 0, ibcmock.PortID, "", "", "", clienttypes.ZeroHeight(), 0) + packetUnmarshaler, ok := transferStack.(types.CallbacksCompatibleModule) + s.Require().True(ok) + + s.path.Setup() + + gasMeter := storetypes.NewGasMeter(2_000_000) + ctx := s.chainA.GetContext().WithGasMeter(gasMeter) + packet := channeltypes.NewPacket(packetDataBytes, 0, transfertypes.PortID, s.path.EndpointA.ChannelID, transfertypes.PortID, s.path.EndpointB.ChannelID, clienttypes.ZeroHeight(), 0) callbackData, err := types.GetDestCallbackData(ctx, packetUnmarshaler, packet, 1_000_000) s.Require().NoError(err) s.Require().Equal(expCallbackData, callbackData) From 6668c93a2b84d03ee5b9bfcd23676d408983a5c5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Wed, 22 May 2024 17:42:06 +0200 Subject: [PATCH 6/9] test: simplify callbacks test, revert back to previous behaviour --- .../apps/callbacks/types/callbacks_test.go | 129 ++++-------------- modules/apps/callbacks/types/export_test.go | 8 ++ 2 files changed, 31 insertions(+), 106 deletions(-) diff --git a/modules/apps/callbacks/types/callbacks_test.go b/modules/apps/callbacks/types/callbacks_test.go index 052ece95ee8..be4098f9bc9 100644 --- a/modules/apps/callbacks/types/callbacks_test.go +++ b/modules/apps/callbacks/types/callbacks_test.go @@ -3,7 +3,6 @@ package types_test import ( "fmt" - storetypes "cosmossdk.io/store/types" sdk "github.com/cosmos/cosmos-sdk/types" @@ -14,19 +13,17 @@ import ( transfertypes "github.com/cosmos/ibc-go/v8/modules/apps/transfer/types" clienttypes "github.com/cosmos/ibc-go/v8/modules/core/02-client/types" channeltypes "github.com/cosmos/ibc-go/v8/modules/core/04-channel/types" - porttypes "github.com/cosmos/ibc-go/v8/modules/core/05-port/types" ibctesting "github.com/cosmos/ibc-go/v8/testing" ibcmock "github.com/cosmos/ibc-go/v8/testing/mock" ) func (s *CallbacksTypesTestSuite) TestGetCallbackData() { var ( - sender = sdk.AccAddress(secp256k1.GenPrivKey().PubKey().Address()).String() - receiver = sdk.AccAddress(secp256k1.GenPrivKey().PubKey().Address()).String() - packetDataUnmarshaler porttypes.PacketDataUnmarshaler - packetData []byte - remainingGas uint64 - callbackKey string + sender = sdk.AccAddress(secp256k1.GenPrivKey().PubKey().Address()).String() + receiver = sdk.AccAddress(secp256k1.GenPrivKey().PubKey().Address()).String() + packetData interface{} + remainingGas uint64 + callbackKey string ) // max gas is 1_000_000 @@ -39,18 +36,14 @@ func (s *CallbacksTypesTestSuite) TestGetCallbackData() { { "success: source callback", func() { - s.path.EndpointA.ChannelConfig.Version = transfertypes.V1 - s.path.EndpointB.ChannelConfig.Version = transfertypes.V1 - remainingGas = 2_000_000 - expPacketData := transfertypes.FungibleTokenPacketData{ + packetData = transfertypes.FungibleTokenPacketData{ Denom: ibctesting.TestCoin.Denom, Amount: ibctesting.TestCoin.Amount.String(), Sender: sender, Receiver: receiver, Memo: fmt.Sprintf(`{"src_callback": {"address": "%s"}}`, sender), } - packetData = expPacketData.GetBytes() }, types.CallbackData{ CallbackAddress: sender, @@ -63,19 +56,16 @@ func (s *CallbacksTypesTestSuite) TestGetCallbackData() { { "success: destination callback", func() { - s.path.EndpointA.ChannelConfig.Version = transfertypes.V1 - s.path.EndpointB.ChannelConfig.Version = transfertypes.V1 - callbackKey = types.DestinationCallbackKey + remainingGas = 2_000_000 - expPacketData := transfertypes.FungibleTokenPacketData{ + packetData = transfertypes.FungibleTokenPacketData{ Denom: ibctesting.TestCoin.Denom, Amount: ibctesting.TestCoin.Amount.String(), Sender: sender, Receiver: receiver, Memo: fmt.Sprintf(`{"dest_callback": {"address": "%s"}}`, sender), } - packetData = expPacketData.GetBytes() }, types.CallbackData{ CallbackAddress: sender, @@ -88,19 +78,16 @@ func (s *CallbacksTypesTestSuite) TestGetCallbackData() { { "success: destination callback with 0 user defined gas limit", func() { - s.path.EndpointA.ChannelConfig.Version = transfertypes.V1 - s.path.EndpointB.ChannelConfig.Version = transfertypes.V1 - callbackKey = types.DestinationCallbackKey + remainingGas = 2_000_000 - expPacketData := transfertypes.FungibleTokenPacketData{ + packetData = transfertypes.FungibleTokenPacketData{ Denom: ibctesting.TestCoin.Denom, Amount: ibctesting.TestCoin.Amount.String(), Sender: sender, Receiver: receiver, Memo: fmt.Sprintf(`{"dest_callback": {"address": "%s", "gas_limit":"0"}}`, sender), } - packetData = expPacketData.GetBytes() }, types.CallbackData{ CallbackAddress: sender, @@ -113,17 +100,13 @@ func (s *CallbacksTypesTestSuite) TestGetCallbackData() { { "success: source callback with gas limit < remaining gas < max gas", func() { - s.path.EndpointA.ChannelConfig.Version = transfertypes.V1 - s.path.EndpointB.ChannelConfig.Version = transfertypes.V1 - - expPacketData := transfertypes.FungibleTokenPacketData{ + packetData = transfertypes.FungibleTokenPacketData{ Denom: ibctesting.TestCoin.Denom, Amount: ibctesting.TestCoin.Amount.String(), Sender: sender, Receiver: receiver, Memo: fmt.Sprintf(`{"src_callback": {"address": "%s", "gas_limit": "50000"}}`, sender), } - packetData = expPacketData.GetBytes() remainingGas = 100_000 }, @@ -138,23 +121,19 @@ func (s *CallbacksTypesTestSuite) TestGetCallbackData() { { "success: source callback with remaining gas < gas limit < max gas", func() { - s.path.EndpointA.ChannelConfig.Version = transfertypes.V1 - s.path.EndpointB.ChannelConfig.Version = transfertypes.V1 - remainingGas = 100_000 - expPacketData := transfertypes.FungibleTokenPacketData{ + packetData = transfertypes.FungibleTokenPacketData{ Denom: ibctesting.TestCoin.Denom, Amount: ibctesting.TestCoin.Amount.String(), Sender: sender, Receiver: receiver, Memo: fmt.Sprintf(`{"src_callback": {"address": "%s", "gas_limit": "200000"}}`, sender), } - packetData = expPacketData.GetBytes() }, types.CallbackData{ CallbackAddress: sender, SenderAddress: sender, - ExecutionGasLimit: 97_715, + ExecutionGasLimit: 100_000, CommitGasLimit: 200_000, }, nil, @@ -162,23 +141,19 @@ func (s *CallbacksTypesTestSuite) TestGetCallbackData() { { "success: source callback with remaining gas < max gas < gas limit", func() { - s.path.EndpointA.ChannelConfig.Version = transfertypes.V1 - s.path.EndpointB.ChannelConfig.Version = transfertypes.V1 - remainingGas = 100_000 - expPacketData := transfertypes.FungibleTokenPacketData{ + packetData = transfertypes.FungibleTokenPacketData{ Denom: ibctesting.TestCoin.Denom, Amount: ibctesting.TestCoin.Amount.String(), Sender: sender, Receiver: receiver, Memo: fmt.Sprintf(`{"src_callback": {"address": "%s", "gas_limit": "2000000"}}`, sender), } - packetData = expPacketData.GetBytes() }, types.CallbackData{ CallbackAddress: sender, SenderAddress: sender, - ExecutionGasLimit: 97_715, + ExecutionGasLimit: 100_000, CommitGasLimit: 1_000_000, }, nil, @@ -186,24 +161,21 @@ func (s *CallbacksTypesTestSuite) TestGetCallbackData() { { "success: destination callback with remaining gas < max gas < gas limit", func() { - s.path.EndpointA.ChannelConfig.Version = transfertypes.V1 - s.path.EndpointB.ChannelConfig.Version = transfertypes.V1 - callbackKey = types.DestinationCallbackKey + remainingGas = 100_000 - expPacketData := transfertypes.FungibleTokenPacketData{ + packetData = transfertypes.FungibleTokenPacketData{ Denom: ibctesting.TestCoin.Denom, Amount: ibctesting.TestCoin.Amount.String(), Sender: sender, Receiver: receiver, Memo: fmt.Sprintf(`{"dest_callback": {"address": "%s", "gas_limit": "2000000"}}`, sender), } - packetData = expPacketData.GetBytes() }, types.CallbackData{ CallbackAddress: sender, SenderAddress: "", - ExecutionGasLimit: 97_715, + ExecutionGasLimit: 100_000, CommitGasLimit: 1_000_000, }, nil, @@ -211,18 +183,14 @@ func (s *CallbacksTypesTestSuite) TestGetCallbackData() { { "success: source callback with max gas < remaining gas < gas limit", func() { - s.path.EndpointA.ChannelConfig.Version = transfertypes.V1 - s.path.EndpointB.ChannelConfig.Version = transfertypes.V1 - remainingGas = 2_000_000 - expPacketData := transfertypes.FungibleTokenPacketData{ + packetData = transfertypes.FungibleTokenPacketData{ Denom: ibctesting.TestCoin.Denom, Amount: ibctesting.TestCoin.Amount.String(), Sender: sender, Receiver: receiver, Memo: fmt.Sprintf(`{"src_callback": {"address": "%s", "gas_limit": "3000000"}}`, sender), } - packetData = expPacketData.GetBytes() }, types.CallbackData{ CallbackAddress: sender, @@ -232,19 +200,10 @@ func (s *CallbacksTypesTestSuite) TestGetCallbackData() { }, nil, }, - { - "failure: invalid packet data", - func() { - packetData = []byte("invalid packet data") - }, - types.CallbackData{}, - types.ErrCannotUnmarshalPacketData, - }, { "failure: packet data does not implement PacketDataProvider", func() { packetData = ibcmock.MockPacketData - packetDataUnmarshaler = ibcmock.IBCModule{} }, types.CallbackData{}, types.ErrNotPacketDataProvider, @@ -252,17 +211,13 @@ func (s *CallbacksTypesTestSuite) TestGetCallbackData() { { "failure: empty memo", func() { - s.path.EndpointA.ChannelConfig.Version = transfertypes.V1 - s.path.EndpointB.ChannelConfig.Version = transfertypes.V1 - - expPacketData := transfertypes.FungibleTokenPacketData{ + packetData = transfertypes.FungibleTokenPacketData{ Denom: ibctesting.TestCoin.Denom, Amount: ibctesting.TestCoin.Amount.String(), Sender: sender, Receiver: receiver, Memo: "", } - packetData = expPacketData.GetBytes() }, types.CallbackData{}, types.ErrCallbackKeyNotFound, @@ -270,17 +225,13 @@ func (s *CallbacksTypesTestSuite) TestGetCallbackData() { { "failure: empty address", func() { - s.path.EndpointA.ChannelConfig.Version = transfertypes.V1 - s.path.EndpointB.ChannelConfig.Version = transfertypes.V1 - - expPacketData := transfertypes.FungibleTokenPacketData{ + packetData = transfertypes.FungibleTokenPacketData{ Denom: ibctesting.TestCoin.Denom, Amount: ibctesting.TestCoin.Amount.String(), Sender: sender, Receiver: receiver, Memo: `{"src_callback": {"address": ""}}`, } - packetData = expPacketData.GetBytes() }, types.CallbackData{}, types.ErrCallbackAddressNotFound, @@ -288,17 +239,13 @@ func (s *CallbacksTypesTestSuite) TestGetCallbackData() { { "failure: space address", func() { - s.path.EndpointA.ChannelConfig.Version = transfertypes.V1 - s.path.EndpointB.ChannelConfig.Version = transfertypes.V1 - - expPacketData := transfertypes.FungibleTokenPacketData{ + packetData = transfertypes.FungibleTokenPacketData{ Denom: ibctesting.TestCoin.Denom, Amount: ibctesting.TestCoin.Amount.String(), Sender: sender, Receiver: receiver, Memo: `{"src_callback": {"address": " "}}`, } - packetData = expPacketData.GetBytes() }, types.CallbackData{}, types.ErrCallbackAddressNotFound, @@ -312,39 +259,9 @@ func (s *CallbacksTypesTestSuite) TestGetCallbackData() { callbackKey = types.SourceCallbackKey - s.path.EndpointA.ChannelConfig.Version = transfertypes.V2 - s.path.EndpointA.ChannelConfig.PortID = transfertypes.ModuleName - s.path.EndpointB.ChannelConfig.Version = transfertypes.V2 - s.path.EndpointB.ChannelConfig.PortID = transfertypes.ModuleName - - transferStack, ok := s.chainA.App.GetIBCKeeper().PortKeeper.Route(transfertypes.ModuleName) - s.Require().True(ok) - - packetDataUnmarshaler, ok = transferStack.(types.CallbacksCompatibleModule) - s.Require().True(ok) - tc.malleate() - s.path.Setup() - - var ( - callbackData types.CallbackData - err error - ) - - // Set up gas meter for context. - gasMeter := storetypes.NewGasMeter(remainingGas) - - // we can use the context of chainA in both cases because we're doing direct function calls - // to GetDestCallbackData and GetSourceCallbackData on a single chain. - ctx := s.chainA.GetContext().WithGasMeter(gasMeter) - - packet := channeltypes.NewPacket(packetData, 0, transfertypes.PortID, s.path.EndpointA.ChannelID, transfertypes.PortID, s.path.EndpointB.ChannelID, clienttypes.ZeroHeight(), 0) - if callbackKey == types.DestinationCallbackKey { - callbackData, err = types.GetDestCallbackData(ctx, packetDataUnmarshaler, packet, uint64(1_000_000)) - } else { - callbackData, err = types.GetSourceCallbackData(ctx, packetDataUnmarshaler, packet, uint64(1_000_000)) - } + callbackData, err := types.GetCallbackData(packetData, transfertypes.PortID, remainingGas, uint64(1_000_000), callbackKey) expPass := tc.expError == nil if expPass { diff --git a/modules/apps/callbacks/types/export_test.go b/modules/apps/callbacks/types/export_test.go index 5b0a32508e3..4cc85624827 100644 --- a/modules/apps/callbacks/types/export_test.go +++ b/modules/apps/callbacks/types/export_test.go @@ -4,6 +4,14 @@ package types This file is to allow for unexported functions to be accessible to the testing package. */ +// GetCallbackData is a wrapper around getCallbackData to allow the function to be directly called in tests. +func GetCallbackData( + packetData interface{}, srcPortID string, remainingGas, + maxGas uint64, callbackKey string, +) (CallbackData, error) { + return getCallbackData(packetData, srcPortID, remainingGas, maxGas, callbackKey) +} + // GetCallbackAddress is a wrapper around getCallbackAddress to allow the function to be directly called in tests. func GetCallbackAddress(callbackData map[string]interface{}) string { return getCallbackAddress(callbackData) From 7409af2a834014bb25a669aa0c4e6abc4a8c9db8 Mon Sep 17 00:00:00 2001 From: chatton Date: Wed, 22 May 2024 17:00:02 +0100 Subject: [PATCH 7/9] chore: fix test case name --- modules/apps/transfer/ibc_module_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/apps/transfer/ibc_module_test.go b/modules/apps/transfer/ibc_module_test.go index ddf43d332a1..e52b04fc7da 100644 --- a/modules/apps/transfer/ibc_module_test.go +++ b/modules/apps/transfer/ibc_module_test.go @@ -596,7 +596,7 @@ func (suite *TransferTestSuite) TestPacketDataUnmarshalerInterface() { nil, }, { - "failure: valid packet data multidenom without memo", + "failure: invalid token trace", func() { path.EndpointA.ChannelConfig.Version = types.V2 initialPacketData = types.FungibleTokenPacketDataV2{ From efe253235e092608451cb298657e415bfa2315b0 Mon Sep 17 00:00:00 2001 From: chatton Date: Thu, 23 May 2024 08:59:42 +0100 Subject: [PATCH 8/9] chore: addressing PR feedback --- modules/apps/transfer/ibc_module.go | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/modules/apps/transfer/ibc_module.go b/modules/apps/transfer/ibc_module.go index 2e72a42857b..1c3b8e1210e 100644 --- a/modules/apps/transfer/ibc_module.go +++ b/modules/apps/transfer/ibc_module.go @@ -199,7 +199,7 @@ func (IBCModule) unmarshalPacketDataBytesToICS20V2(bz []byte, ics20Version strin return datav2, nil default: - return types.FungibleTokenPacketDataV2{}, errorsmod.Wrapf(types.ErrInvalidVersion, "invalid ICS-20 version: %s", ics20Version) + return types.FungibleTokenPacketDataV2{}, errorsmod.Wrap(types.ErrInvalidVersion, ics20Version) } } @@ -213,9 +213,9 @@ func (im IBCModule) OnRecvPacket( ) ibcexported.Acknowledgement { ack := channeltypes.NewResultAcknowledgement([]byte{byte(1)}) - ics20Version, ok := im.keeper.GetICS4Wrapper().GetAppVersion(ctx, packet.DestinationPort, packet.DestinationChannel) - if !ok { - return channeltypes.NewErrorAcknowledgement(errorsmod.Wrapf(ibcerrors.ErrInvalidVersion, "could not retrieve app version for channel (%s, %s)", packet.SourcePort, packet.SourceChannel)) + ics20Version, found := im.keeper.GetICS4Wrapper().GetAppVersion(ctx, packet.DestinationPort, packet.DestinationChannel) + if !found { + return channeltypes.NewErrorAcknowledgement(errorsmod.Wrapf(ibcerrors.ErrNotFound, "app version not found for port %s and channel %s", packet.DestinationPort, packet.DestinationChannel)) } data, ackErr := im.unmarshalPacketDataBytesToICS20V2(packet.GetData(), ics20Version) @@ -275,9 +275,9 @@ func (im IBCModule) OnAcknowledgementPacket( return errorsmod.Wrapf(ibcerrors.ErrUnknownRequest, "cannot unmarshal ICS-20 transfer packet acknowledgement: %v", err) } - ics20Version, ok := im.keeper.GetICS4Wrapper().GetAppVersion(ctx, packet.SourcePort, packet.SourceChannel) - if !ok { - return errorsmod.Wrapf(ibcerrors.ErrInvalidVersion, "could not retrieve app version for channel (%s, %s)", packet.SourcePort, packet.SourceChannel) + ics20Version, found := im.keeper.GetICS4Wrapper().GetAppVersion(ctx, packet.SourcePort, packet.SourceChannel) + if !found { + return errorsmod.Wrapf(ibcerrors.ErrNotFound, "app version not found for port %s and channel %s", packet.SourcePort, packet.SourceChannel) } data, err := im.unmarshalPacketDataBytesToICS20V2(packet.GetData(), ics20Version) @@ -330,9 +330,9 @@ func (im IBCModule) OnTimeoutPacket( packet channeltypes.Packet, relayer sdk.AccAddress, ) error { - ics20Version, ok := im.keeper.GetICS4Wrapper().GetAppVersion(ctx, packet.SourcePort, packet.SourceChannel) - if !ok { - return errorsmod.Wrapf(ibcerrors.ErrInvalidVersion, "could not retrieve app version for channel (%s, %s)", packet.SourcePort, packet.SourceChannel) + ics20Version, found := im.keeper.GetICS4Wrapper().GetAppVersion(ctx, packet.SourcePort, packet.SourceChannel) + if !found { + return errorsmod.Wrapf(ibcerrors.ErrNotFound, "app version not found for port %s and channel %s", packet.SourcePort, packet.SourceChannel) } data, err := im.unmarshalPacketDataBytesToICS20V2(packet.GetData(), ics20Version) @@ -405,9 +405,9 @@ func (IBCModule) OnChanUpgradeOpen(ctx sdk.Context, portID, channelID string, pr // into a FungibleTokenPacketData. This function implements the optional // PacketDataUnmarshaler interface required for ADR 008 support. func (im IBCModule) UnmarshalPacketData(ctx sdk.Context, portID, channelID string, bz []byte) (interface{}, error) { - ics20Version, ok := im.keeper.GetICS4Wrapper().GetAppVersion(ctx, portID, channelID) - if !ok { - return nil, errorsmod.Wrapf(ibcerrors.ErrInvalidVersion, "could not retrieve app version for channel (%s, %s)", portID, channelID) + ics20Version, found := im.keeper.GetICS4Wrapper().GetAppVersion(ctx, portID, channelID) + if !found { + return nil, errorsmod.Wrapf(ibcerrors.ErrNotFound, "app version not found for port %s and channel %s", portID, channelID) } ftpd, err := im.unmarshalPacketDataBytesToICS20V2(bz, ics20Version) From 53066f46f82b876a8375c6582322674ef6735cd7 Mon Sep 17 00:00:00 2001 From: chatton Date: Thu, 23 May 2024 09:01:57 +0100 Subject: [PATCH 9/9] chore: added docstring for unmarshalPacketDataBytesToICS20V2 --- modules/apps/transfer/ibc_module.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/modules/apps/transfer/ibc_module.go b/modules/apps/transfer/ibc_module.go index 1c3b8e1210e..c87591e6c9f 100644 --- a/modules/apps/transfer/ibc_module.go +++ b/modules/apps/transfer/ibc_module.go @@ -174,6 +174,8 @@ func (IBCModule) OnChanCloseConfirm( return nil } +// unmarshalPacketDataBytesToICS20V2 attempts to unmarshal the provided packet data bytes into a FungibleTokenPacketDataV2. +// The version of ics20 should be provided and should be either ics20-1 or ics20-2. func (IBCModule) unmarshalPacketDataBytesToICS20V2(bz []byte, ics20Version string) (types.FungibleTokenPacketDataV2, error) { switch ics20Version { case types.V1: