ics20-v2: backwards compatibility for transfer rpc and packet callbacks (#6189)

* chore: adding proto files for ics20-v2

* chore: add newline

* chore: modify MsgTransfer to accept coins instead of coin

* chore: reverted unintentional comment changes

* chore: reverted unintentional comment changes

* chore: adding test for conversion fn

* chore: fix e2e linter

* chore: adding docs

* chore: addressing PR feedback

* chore: remove duplicate import

* chore: addressing PR feedback

* chore: moved extration logic into internal package

* chore: commented out failing test

* chore: adding link to issue

* chore: remove duplicate import

* chore: fix linting errors

* FungibleTokenPacketData interface methods + tests

* linter

* wip: token validation

* update trace identifier validation in Token + tests

* rm Printf

* update with pr review

* pr review

* linter

* rm unused function: linter

* wip pr feedback

* fix test

* pr review

* lintttttt

* wip: backwards compatibility for transfer rpc

* implement changes for ics20-v2 packet data for onRecvPacket, onAcknowledgePacket and onTimeoutPacket

* fix callbacks tests

* lint

---------

Co-authored-by: chatton <github.qpeyb@simplelogin.fr>
Co-authored-by: Charly <charly@interchain.berlin>

Author: 3 people

modules/apps/callbacks/ibc_middleware_test.go

@@ -13,8 +13,8 @@ import (
 	"github.com/cosmos/ibc-go/modules/apps/callbacks/testing/simapp"
 	"github.com/cosmos/ibc-go/modules/apps/callbacks/types"
 	icacontrollertypes "github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/controller/types"
-	transfertypes "github.com/cosmos/ibc-go/v8/modules/apps/transfer/types"
-	multidenom "github.com/cosmos/ibc-go/v8/modules/apps/transfer/types/v3"
+	transferv1types "github.com/cosmos/ibc-go/v8/modules/apps/transfer/types"
+	transferv3types "github.com/cosmos/ibc-go/v8/modules/apps/transfer/types/v3"
 	clienttypes "github.com/cosmos/ibc-go/v8/modules/core/02-client/types"
 	channelkeeper "github.com/cosmos/ibc-go/v8/modules/core/04-channel/keeper"
 	channeltypes "github.com/cosmos/ibc-go/v8/modules/core/04-channel/types"
@@ -94,7 +94,7 @@ func (s *CallbacksTestSuite) TestWithICS4Wrapper() {
 }
 
 func (s *CallbacksTestSuite) TestSendPacket() {
-	var packetData transfertypes.FungibleTokenPacketData
+	var packetData transferv3types.FungibleTokenPacketData
 
 	testCases := []struct {
 		name         string
@@ -165,9 +165,17 @@ func (s *CallbacksTestSuite) TestSendPacket() {
 
 			transferICS4Wrapper := GetSimApp(s.chainA).TransferKeeper.GetICS4Wrapper()
 
-			packetData = transfertypes.NewFungibleTokenPacketData(
-				ibctesting.TestCoin.GetDenom(), ibctesting.TestCoin.Amount.String(), ibctesting.TestAccAddress,
-				ibctesting.TestAccAddress, fmt.Sprintf(`{"src_callback": {"address": "%s"}}`, simapp.SuccessContract),
+			packetData = transferv3types.NewFungibleTokenPacketData(
+				[]*transferv3types.Token{
+					{
+						Denom:  ibctesting.TestCoin.GetDenom(),
+						Amount: ibctesting.TestCoin.Amount.String(),
+						Trace:  []string{},
+					},
+				},
+				ibctesting.TestAccAddress,
+				ibctesting.TestAccAddress,
+				fmt.Sprintf(`{"src_callback": {"address": "%s"}}`, simapp.SuccessContract),
 			)
 
 			chanCap := s.path.EndpointA.Chain.GetChannelCapability(s.path.EndpointA.ChannelConfig.PortID, s.path.EndpointA.ChannelID)
@@ -223,7 +231,7 @@ func (s *CallbacksTestSuite) TestOnAcknowledgementPacket() {
 	)
 
 	var (
-		packetData   transfertypes.FungibleTokenPacketData
+		packetData   transferv3types.FungibleTokenPacketData
 		packet       channeltypes.Packet
 		ack          []byte
 		ctx          sdk.Context
@@ -299,8 +307,16 @@ func (s *CallbacksTestSuite) TestOnAcknowledgementPacket() {
 			s.SetupTransferTest()
 
 			userGasLimit = 600000
-			packetData = transfertypes.NewFungibleTokenPacketData(
-				ibctesting.TestCoin.GetDenom(), ibctesting.TestCoin.Amount.String(), ibctesting.TestAccAddress, ibctesting.TestAccAddress,
+			packetData = transferv3types.NewFungibleTokenPacketData(
+				[]*transferv3types.Token{
+					{
+						Denom:  ibctesting.TestCoin.GetDenom(),
+						Amount: ibctesting.TestCoin.Amount.String(),
+						Trace:  []string{},
+					},
+				},
+				ibctesting.TestAccAddress,
+				ibctesting.TestAccAddress,
 				fmt.Sprintf(`{"src_callback": {"address":"%s", "gas_limit":"%d"}}`, simapp.SuccessContract, userGasLimit),
 			)
 
@@ -323,7 +339,7 @@ func (s *CallbacksTestSuite) TestOnAcknowledgementPacket() {
 			tc.malleate()
 
 			// callbacks module is routed as top level middleware
-			transferStack, ok := s.chainA.App.GetIBCKeeper().Router.GetRoute(transfertypes.ModuleName)
+			transferStack, ok := s.chainA.App.GetIBCKeeper().Router.GetRoute(transferv1types.ModuleName)
 			s.Require().True(ok)
 
 			onAcknowledgementPacket := func() error {
@@ -385,7 +401,7 @@ func (s *CallbacksTestSuite) TestOnTimeoutPacket() {
 	)
 
 	var (
-		packetData transfertypes.FungibleTokenPacketData
+		packetData transferv3types.FungibleTokenPacketData
 		packet     channeltypes.Packet
 		ctx        sdk.Context
 	)
@@ -408,7 +424,7 @@ func (s *CallbacksTestSuite) TestOnTimeoutPacket() {
 				packet.Data = []byte("invalid packet data")
 			},
 			noExecution,
-			ibcerrors.ErrUnknownRequest,
+			ibcerrors.ErrInvalidType,
 		},
 		{
 			"success: no-op on callback data is not valid",
@@ -462,7 +478,7 @@ func (s *CallbacksTestSuite) TestOnTimeoutPacket() {
 			// succeed on timeout
 			userGasLimit := 600_000
 			timeoutTimestamp := uint64(s.chainB.GetContext().BlockTime().UnixNano())
-			msg := transfertypes.NewMsgTransfer(
+			msg := transferv1types.NewMsgTransfer(
 				s.path.EndpointA.ChannelConfig.PortID, s.path.EndpointA.ChannelID,
 				sdk.NewCoins(ibctesting.TestCoin), s.chainA.SenderAccount.GetAddress().String(),
 				s.chainB.SenderAccount.GetAddress().String(), clienttypes.ZeroHeight(), timeoutTimestamp,
@@ -486,7 +502,7 @@ func (s *CallbacksTestSuite) TestOnTimeoutPacket() {
 			tc.malleate()
 
 			// callbacks module is routed as top level middleware
-			transferStack, ok := s.chainA.App.GetIBCKeeper().Router.GetRoute(transfertypes.ModuleName)
+			transferStack, ok := s.chainA.App.GetIBCKeeper().Router.GetRoute(transferv1types.ModuleName)
 			s.Require().True(ok)
 
 			onTimeoutPacket := func() error {
@@ -547,7 +563,7 @@ func (s *CallbacksTestSuite) TestOnRecvPacket() {
 	)
 
 	var (
-		packetData   transfertypes.FungibleTokenPacketData
+		packetData   transferv3types.FungibleTokenPacketData
 		packet       channeltypes.Packet
 		ctx          sdk.Context
 		userGasLimit uint64
@@ -624,8 +640,16 @@ func (s *CallbacksTestSuite) TestOnRecvPacket() {
 
 			// set user gas limit above panic level in mock contract keeper
 			userGasLimit = 600_000
-			packetData = transfertypes.NewFungibleTokenPacketData(
-				ibctesting.TestCoin.GetDenom(), ibctesting.TestCoin.Amount.String(), ibctesting.TestAccAddress, s.chainB.SenderAccount.GetAddress().String(),
+			packetData = transferv3types.NewFungibleTokenPacketData(
+				[]*transferv3types.Token{
+					{
+						Denom:  ibctesting.TestCoin.GetDenom(),
+						Amount: ibctesting.TestCoin.Amount.String(),
+						Trace:  []string{},
+					},
+				},
+				ibctesting.TestAccAddress,
+				s.chainB.SenderAccount.GetAddress().String(),
 				fmt.Sprintf(`{"dest_callback": {"address":"%s", "gas_limit":"%d"}}`, ibctesting.TestAccAddress, userGasLimit),
 			)
 
@@ -646,7 +670,7 @@ func (s *CallbacksTestSuite) TestOnRecvPacket() {
 			tc.malleate()
 
 			// callbacks module is routed as top level middleware
-			transferStack, ok := s.chainB.App.GetIBCKeeper().Router.GetRoute(transfertypes.ModuleName)
+			transferStack, ok := s.chainB.App.GetIBCKeeper().Router.GetRoute(transferv1types.ModuleName)
 			s.Require().True(ok)
 
 			onRecvPacket := func() ibcexported.Acknowledgement {
@@ -701,7 +725,7 @@ func (s *CallbacksTestSuite) TestOnRecvPacket() {
 
 func (s *CallbacksTestSuite) TestWriteAcknowledgement() {
 	var (
-		packetData transfertypes.FungibleTokenPacketData
+		packetData transferv3types.FungibleTokenPacketData
 		packet     channeltypes.Packet
 		ctx        sdk.Context
 		ack        ibcexported.Acknowledgement
@@ -748,8 +772,16 @@ func (s *CallbacksTestSuite) TestWriteAcknowledgement() {
 			s.SetupTransferTest()
 
 			// set user gas limit above panic level in mock contract keeper
-			packetData = transfertypes.NewFungibleTokenPacketData(
-				ibctesting.TestCoin.GetDenom(), ibctesting.TestCoin.Amount.String(), ibctesting.TestAccAddress, s.chainB.SenderAccount.GetAddress().String(),
+			packetData = transferv3types.NewFungibleTokenPacketData(
+				[]*transferv3types.Token{
+					{
+						Denom:  ibctesting.TestCoin.GetDenom(),
+						Amount: ibctesting.TestCoin.Amount.String(),
+						Trace:  []string{},
+					},
+				},
+				ibctesting.TestAccAddress,
+				s.chainB.SenderAccount.GetAddress().String(),
 				fmt.Sprintf(`{"dest_callback": {"address":"%s", "gas_limit":"600000"}}`, ibctesting.TestAccAddress),
 			)
 
@@ -945,37 +977,44 @@ func (s *CallbacksTestSuite) TestUnmarshalPacketData() {
 
 	// 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().Router.GetRoute(transfertypes.ModuleName)
+	transferStack, ok := s.chainA.App.GetIBCKeeper().Router.GetRoute(transferv1types.ModuleName)
 	s.Require().True(ok)
 
 	unmarshalerStack, ok := transferStack.(types.CallbacksCompatibleModule)
 	s.Require().True(ok)
 
-	initialPacketData := transfertypes.FungibleTokenPacketData{
+	expPacketDataICS20V1 := transferv1types.FungibleTokenPacketData{
 		Denom:    ibctesting.TestCoin.Denom,
 		Amount:   ibctesting.TestCoin.Amount.String(),
 		Sender:   ibctesting.TestAccAddress,
 		Receiver: ibctesting.TestAccAddress,
 		Memo:     fmt.Sprintf(`{"src_callback": {"address": "%s"}, "dest_callback": {"address":"%s"}}`, ibctesting.TestAccAddress, ibctesting.TestAccAddress),
 	}
-	data := initialPacketData.GetBytes()
 
-	expPacketData := multidenom.FungibleTokenPacketData{
-		Tokens: []*multidenom.Token{
+	expPacketDataICS20V2 := transferv3types.FungibleTokenPacketData{
+		Tokens: []*transferv3types.Token{
 			{
-				Denom:  initialPacketData.Denom,
-				Amount: initialPacketData.Amount,
-				Trace:  []string{""},
+				Denom:  ibctesting.TestCoin.Denom,
+				Amount: ibctesting.TestCoin.Amount.String(),
+				Trace:  nil,
 			},
 		},
-		Sender:   initialPacketData.Sender,
-		Receiver: initialPacketData.Receiver,
-		Memo:     initialPacketData.Memo,
+		Sender:   ibctesting.TestAccAddress,
+		Receiver: ibctesting.TestAccAddress,
+		Memo:     fmt.Sprintf(`{"src_callback": {"address": "%s"}, "dest_callback": {"address":"%s"}}`, ibctesting.TestAccAddress, ibctesting.TestAccAddress),
 	}
 
+	// Unmarshal ICS20 v1 packet data
+	data := expPacketDataICS20V1.GetBytes()
 	packetData, err := unmarshalerStack.UnmarshalPacketData(data)
 	s.Require().NoError(err)
-	s.Require().Equal(expPacketData, packetData)
+	s.Require().Equal(expPacketDataICS20V2, packetData)
+
+	// Unmarshal ICS20 v1 packet data
+	data = expPacketDataICS20V2.GetBytes()
+	packetData, err = unmarshalerStack.UnmarshalPacketData(data)
+	s.Require().NoError(err)
+	s.Require().Equal(expPacketDataICS20V2, packetData)
 }
 
 func (s *CallbacksTestSuite) TestGetAppVersion() {

modules/apps/transfer/ibc_module.go

@@ -207,10 +207,8 @@ func (im IBCModule) OnRecvPacket(
 	logger := im.keeper.Logger(ctx)
 	ack := channeltypes.NewResultAcknowledgement([]byte{byte(1)})
 
-	var data types.FungibleTokenPacketData
-	var ackErr error
-	if err := json.Unmarshal(packet.GetData(), &data); err != nil {
-		ackErr = errorsmod.Wrapf(ibcerrors.ErrInvalidType, "cannot unmarshal ICS-20 transfer packet data")
+	data, ackErr := im.unmarshalPacketDataBytesToICS20V2(packet.GetData())
+	if ackErr != nil {
 		logger.Error(fmt.Sprintf("%s sequence %d", ackErr.Error(), packet.Sequence))
 		ack = channeltypes.NewErrorAcknowledgement(ackErr)
 	}
@@ -228,26 +226,32 @@ func (im IBCModule) OnRecvPacket(
 		}
 	}
 
+	events := make([]sdk.Event, 0, len(data.Tokens)+1)
+	for _, token := range data.Tokens {
+		events = append(events, sdk.NewEvent(
+			types.EventTypePacket,
+			sdk.NewAttribute(types.AttributeKeySender, data.Sender),
+			sdk.NewAttribute(types.AttributeKeyReceiver, data.Receiver),
+			sdk.NewAttribute(types.AttributeKeyDenom, token.GetFullDenomPath()),
+			sdk.NewAttribute(types.AttributeKeyAmount, token.Amount),
+			sdk.NewAttribute(types.AttributeKeyMemo, data.Memo),
+		))
+	}
+
 	eventAttributes := []sdk.Attribute{
 		sdk.NewAttribute(sdk.AttributeKeyModule, types.ModuleName),
-		sdk.NewAttribute(sdk.AttributeKeySender, data.Sender),
-		sdk.NewAttribute(types.AttributeKeyReceiver, data.Receiver),
-		sdk.NewAttribute(types.AttributeKeyDenom, data.Denom),
-		sdk.NewAttribute(types.AttributeKeyAmount, data.Amount),
-		sdk.NewAttribute(types.AttributeKeyMemo, data.Memo),
 		sdk.NewAttribute(types.AttributeKeyAckSuccess, fmt.Sprintf("%t", ack.Success())),
 	}
-
 	if ackErr != nil {
 		eventAttributes = append(eventAttributes, sdk.NewAttribute(types.AttributeKeyAckError, ackErr.Error()))
 	}
 
-	ctx.EventManager().EmitEvent(
-		sdk.NewEvent(
-			types.EventTypePacket,
-			eventAttributes...,
-		),
-	)
+	events = append(events, sdk.NewEvent(
+		sdk.EventTypeMessage,
+		eventAttributes...,
+	))
+
+	ctx.EventManager().EmitEvents(events)
 
 	// NOTE: acknowledgement will be written synchronously during IBC handler execution.
 	return ack
@@ -264,27 +268,35 @@ func (im IBCModule) OnAcknowledgementPacket(
 	if err := types.ModuleCdc.UnmarshalJSON(acknowledgement, &ack); err != nil {
 		return errorsmod.Wrapf(ibcerrors.ErrUnknownRequest, "cannot unmarshal ICS-20 transfer packet acknowledgement: %v", err)
 	}
-	var data types.FungibleTokenPacketData
-	if err := json.Unmarshal(packet.GetData(), &data); err != nil {
-		return errorsmod.Wrapf(ibcerrors.ErrUnknownRequest, "cannot unmarshal ICS-20 transfer packet data: %s", err.Error())
+
+	data, err := im.unmarshalPacketDataBytesToICS20V2(packet.GetData())
+	if err != nil {
+		return err
 	}
 
 	if err := im.keeper.OnAcknowledgementPacket(ctx, packet, data, ack); err != nil {
 		return err
 	}
 
-	ctx.EventManager().EmitEvent(
-		sdk.NewEvent(
-			types.EventTypePacket,
-			sdk.NewAttribute(sdk.AttributeKeyModule, types.ModuleName),
-			sdk.NewAttribute(sdk.AttributeKeySender, data.Sender),
-			sdk.NewAttribute(types.AttributeKeyReceiver, data.Receiver),
-			sdk.NewAttribute(types.AttributeKeyDenom, data.Denom),
-			sdk.NewAttribute(types.AttributeKeyAmount, data.Amount),
-			sdk.NewAttribute(types.AttributeKeyMemo, data.Memo),
-			sdk.NewAttribute(types.AttributeKeyAck, ack.String()),
-		),
-	)
+	events := make([]sdk.Event, 0, len(data.Tokens)+1)
+	for _, token := range data.Tokens {
+		ctx.EventManager().EmitEvent(
+			sdk.NewEvent(
+				types.EventTypePacket,
+				sdk.NewAttribute(sdk.AttributeKeySender, data.Sender),
+				sdk.NewAttribute(types.AttributeKeyReceiver, data.Receiver),
+				sdk.NewAttribute(types.AttributeKeyDenom, token.GetFullDenomPath()),
+				sdk.NewAttribute(types.AttributeKeyAmount, token.Amount),
+				sdk.NewAttribute(types.AttributeKeyMemo, data.Memo),
+			),
+		)
+	}
+	events = append(events, sdk.NewEvent(
+		sdk.EventTypeMessage,
+		sdk.NewAttribute(sdk.AttributeKeyModule, types.ModuleName),
+		sdk.NewAttribute(types.AttributeKeyAck, ack.String()),
+	))
+	ctx.EventManager().EmitEvents(events)
 
 	switch resp := ack.Response.(type) {
 	case *channeltypes.Acknowledgement_Result:
@@ -312,25 +324,34 @@ func (im IBCModule) OnTimeoutPacket(
 	packet channeltypes.Packet,
 	relayer sdk.AccAddress,
 ) error {
-	var data types.FungibleTokenPacketData
-	if err := json.Unmarshal(packet.GetData(), &data); err != nil {
-		return errorsmod.Wrapf(ibcerrors.ErrUnknownRequest, "cannot unmarshal ICS-20 transfer packet data: %s", err.Error())
+	data, err := im.unmarshalPacketDataBytesToICS20V2(packet.GetData())
+	if err != nil {
+		return err
 	}
+
 	// refund tokens
 	if err := im.keeper.OnTimeoutPacket(ctx, packet, data); err != nil {
 		return err
 	}
 
-	ctx.EventManager().EmitEvent(
-		sdk.NewEvent(
-			types.EventTypeTimeout,
-			sdk.NewAttribute(sdk.AttributeKeyModule, types.ModuleName),
-			sdk.NewAttribute(types.AttributeKeyRefundReceiver, data.Sender),
-			sdk.NewAttribute(types.AttributeKeyRefundDenom, data.Denom),
-			sdk.NewAttribute(types.AttributeKeyRefundAmount, data.Amount),
-			sdk.NewAttribute(types.AttributeKeyMemo, data.Memo),
-		),
-	)
+	events := make([]sdk.Event, 0, len(data.Tokens)+1)
+	for _, token := range data.Tokens {
+		ctx.EventManager().EmitEvent(
+			sdk.NewEvent(
+				types.EventTypeTimeout,
+				sdk.NewAttribute(sdk.AttributeKeySender, data.Sender),
+				sdk.NewAttribute(types.AttributeKeyReceiver, data.Receiver),
+				sdk.NewAttribute(types.AttributeKeyDenom, token.GetFullDenomPath()),
+				sdk.NewAttribute(types.AttributeKeyAmount, token.Amount),
+				sdk.NewAttribute(types.AttributeKeyMemo, data.Memo),
+			),
+		)
+	}
+	events = append(events, sdk.NewEvent(
+		sdk.EventTypeMessage,
+		sdk.NewAttribute(sdk.AttributeKeyModule, types.ModuleName),
+	))
+	ctx.EventManager().EmitEvents(events)
 
 	return nil
 }