Refactor forwarding messages for Transfer and Packet (#6655)

* feat(transfer): add unwind, refactor proto structure. gen-all * tests(transfer/types): fix test failures in types tests. * tests(transfer/keeper): fix test failures in keeper tests. * cli(transfer): fix cli usage. pending flag for unwind. * tests(callbacks): fix failing tests in callbacks. * tests(transfer/internal): fix failures in internal package. * tests(transfer): fix test failures in top level tranfer package. * tests(ica/host/keeper): fix repr of msg transfer in ica host msg execution. * lint(all): lint this bad boy * chore(transfer/types): amend validation for MsgTransfer's ShouldBeForwarded, add tests for ForwardedPacketData, minor nits. * nit(self): only pass relevant fields to create packet data; minor comment improvement. * Apply suggestions from code review Co-authored-by: Carlos Rodriguez <[email protected]> * chore(merge): fix merge issues. * chore(proto): mention optional nature of fields. * memo: do not drop it * validation: drop requirement on memo being empty on msg transfer. --------- Co-authored-by: Carlos Rodriguez <[email protected]>
cosmos · Jun 20, 2024 · 87d1f91 · 87d1f91
1 parent 1ca1f2f
commit 87d1f91
Show file tree

Hide file tree

Showing 25 changed files with 613 additions and 276 deletions.
diff --git a/modules/apps/27-interchain-accounts/host/keeper/relay_test.go b/modules/apps/27-interchain-accounts/host/keeper/relay_test.go
@@ -798,7 +798,7 @@ func (suite *KeeperTestSuite) TestJSONOnRecvPacket() {
 							"timeout_height": { "revision_number": 1, "revision_height": 100 },
 							"timeout_timestamp": 0,
 							"memo": "",
-							"forwarding": { "hops": [], "memo": "" }
+							"forwarding": { "hops": [], "unwind": false }
 						}
 					]
 				}`)

diff --git a/modules/apps/callbacks/ibc_middleware_test.go b/modules/apps/callbacks/ibc_middleware_test.go
@@ -24,7 +24,7 @@ import (
 	ibcmock "github.com/cosmos/ibc-go/v8/testing/mock"
 )
 
-var emptyForwarding = transfertypes.Forwarding{}
+var emptyForwardingPacketData = transfertypes.ForwardingPacketData{}
 
 func (s *CallbacksTestSuite) TestNewIBCMiddleware() {
 	testCases := []struct {
@@ -188,7 +188,7 @@ func (s *CallbacksTestSuite) TestSendPacket() {
 				ibctesting.TestAccAddress,
 				ibctesting.TestAccAddress,
 				fmt.Sprintf(`{"src_callback": {"address": "%s"}}`, simapp.SuccessContract),
-				emptyForwarding,
+				emptyForwardingPacketData,
 			)
 
 			chanCap := s.path.EndpointA.Chain.GetChannelCapability(s.path.EndpointA.ChannelConfig.PortID, s.path.EndpointA.ChannelID)
@@ -330,7 +330,7 @@ func (s *CallbacksTestSuite) TestOnAcknowledgementPacket() {
 				ibctesting.TestAccAddress,
 				ibctesting.TestAccAddress,
 				fmt.Sprintf(`{"src_callback": {"address":"%s", "gas_limit":"%d"}}`, simapp.SuccessContract, userGasLimit),
-				emptyForwarding,
+				emptyForwardingPacketData,
 			)
 
 			packet = channeltypes.Packet{
@@ -496,7 +496,7 @@ func (s *CallbacksTestSuite) TestOnTimeoutPacket() {
 				sdk.NewCoins(ibctesting.TestCoin), s.chainA.SenderAccount.GetAddress().String(),
 				s.chainB.SenderAccount.GetAddress().String(), clienttypes.ZeroHeight(), timeoutTimestamp,
 				fmt.Sprintf(`{"src_callback": {"address":"%s", "gas_limit":"%d"}}`, ibctesting.TestAccAddress, userGasLimit), // set user gas limit above panic level in mock contract keeper
-				emptyForwarding,
+				transfertypes.Forwarding{},
 			)
 
 			res, err := s.chainA.SendMsgs(msg)
@@ -664,7 +664,7 @@ func (s *CallbacksTestSuite) TestOnRecvPacket() {
 				ibctesting.TestAccAddress,
 				s.chainB.SenderAccount.GetAddress().String(),
 				fmt.Sprintf(`{"dest_callback": {"address":"%s", "gas_limit":"%d"}}`, ibctesting.TestAccAddress, userGasLimit),
-				emptyForwarding,
+				emptyForwardingPacketData,
 			)
 
 			packet = channeltypes.Packet{
@@ -796,7 +796,7 @@ func (s *CallbacksTestSuite) TestWriteAcknowledgement() {
 				ibctesting.TestAccAddress,
 				s.chainB.SenderAccount.GetAddress().String(),
 				fmt.Sprintf(`{"dest_callback": {"address":"%s", "gas_limit":"600000"}}`, ibctesting.TestAccAddress),
-				emptyForwarding,
+				emptyForwardingPacketData,
 			)
 
 			packet = channeltypes.Packet{
@@ -1020,7 +1020,7 @@ func (s *CallbacksTestSuite) TestUnmarshalPacketDataV1() {
 		Sender:     ibctesting.TestAccAddress,
 		Receiver:   ibctesting.TestAccAddress,
 		Memo:       fmt.Sprintf(`{"src_callback": {"address": "%s"}, "dest_callback": {"address":"%s"}}`, ibctesting.TestAccAddress, ibctesting.TestAccAddress),
-		Forwarding: emptyForwarding,
+		Forwarding: emptyForwardingPacketData,
 	}
 
 	portID := s.path.EndpointA.ChannelConfig.PortID

diff --git a/modules/apps/transfer/client/cli/tx.go b/modules/apps/transfer/client/cli/tx.go
@@ -96,12 +96,6 @@ using the {packet-timeout-timestamp} flag. If no timeout value is set then a def
 				return err
 			}
 
-			// If parsed, set and replace memo.
-			if len(forwarding.Hops) > 0 {
-				forwarding.Memo = memo
-				memo = ""
-			}
-
 			// NOTE: relative timeouts using block height are not supported.
 			// if the timeouts are not absolute, CLI users rely solely on local clock time in order to calculate relative timestamps.
 			if !absoluteTimeouts {
@@ -164,6 +158,6 @@ func parseForwarding(cmd *cobra.Command) (types.Forwarding, error) {
 		hops = append(hops, hop)
 	}
 
-	forwarding := types.NewForwarding("", hops...)
-	return forwarding, nil
+	// TODO(jim): Add flag for unwind value
+	return types.NewForwarding(false, hops...), nil
 }
diff --git a/modules/apps/transfer/ibc_module_test.go b/modules/apps/transfer/ibc_module_test.go
@@ -303,7 +303,7 @@ func (suite *TransferTestSuite) TestOnRecvPacket() {
 					suite.chainA.SenderAccount.GetAddress().String(),
 					suite.chainB.SenderAccount.GetAddress().String(),
 					"",
-					types.NewForwarding("", types.Hop{PortId: "transfer", ChannelId: "channel-0"}),
+					types.NewForwardingPacketData("", types.Hop{PortId: "transfer", ChannelId: "channel-0"}),
 				)
 				packet.Data = packetData.GetBytes()
 
@@ -368,7 +368,7 @@ func (suite *TransferTestSuite) TestOnRecvPacket() {
 				suite.chainA.SenderAccount.GetAddress().String(),
 				suite.chainB.SenderAccount.GetAddress().String(),
 				"",
-				types.Forwarding{},
+				types.ForwardingPacketData{},
 			)
 
 			tokensBz, err := json.Marshal(packetData.Tokens)

diff --git a/modules/apps/transfer/internal/convert/convert.go b/modules/apps/transfer/internal/convert/convert.go
@@ -24,6 +24,6 @@ func PacketDataV1ToV2(packetData types.FungibleTokenPacketData) (types.FungibleT
 		Sender:     packetData.Sender,
 		Receiver:   packetData.Receiver,
 		Memo:       packetData.Memo,
-		Forwarding: types.Forwarding{},
+		Forwarding: types.ForwardingPacketData{},
 	}, nil
 }
diff --git a/modules/apps/transfer/internal/events/events.go b/modules/apps/transfer/internal/events/events.go
@@ -34,7 +34,7 @@ func EmitTransferEvent(ctx sdk.Context, sender, receiver string, tokens types.To
 // EmitOnRecvPacketEvent emits a fungible token packet event in the OnRecvPacket callback
 func EmitOnRecvPacketEvent(ctx sdk.Context, packetData types.FungibleTokenPacketDataV2, ack channeltypes.Acknowledgement, ackErr error) {
 	tokensStr := mustMarshalType[types.Tokens](packetData.Tokens)
-	forwardingStr := mustMarshalType[types.Forwarding](packetData.Forwarding)
+	forwardingStr := mustMarshalType[types.ForwardingPacketData](packetData.Forwarding)
 
 	eventAttributes := []sdk.Attribute{
 		sdk.NewAttribute(types.AttributeKeySender, packetData.Sender),
@@ -64,7 +64,7 @@ func EmitOnRecvPacketEvent(ctx sdk.Context, packetData types.FungibleTokenPacket
 // EmitOnAcknowledgementPacketEvent emits a fungible token packet event in the OnAcknowledgementPacket callback
 func EmitOnAcknowledgementPacketEvent(ctx sdk.Context, packetData types.FungibleTokenPacketDataV2, ack channeltypes.Acknowledgement) {
 	tokensStr := mustMarshalType[types.Tokens](packetData.Tokens)
-	forwardingStr := mustMarshalType[types.Forwarding](packetData.Forwarding)
+	forwardingStr := mustMarshalType[types.ForwardingPacketData](packetData.Forwarding)
 
 	ctx.EventManager().EmitEvents(sdk.Events{
 		sdk.NewEvent(
@@ -103,7 +103,7 @@ func EmitOnAcknowledgementPacketEvent(ctx sdk.Context, packetData types.Fungible
 // EmitOnTimeoutEvent emits a fungible token packet event in the OnTimeoutPacket callback
 func EmitOnTimeoutEvent(ctx sdk.Context, packetData types.FungibleTokenPacketDataV2) {
 	tokensStr := mustMarshalType[types.Tokens](packetData.Tokens)
-	forwardingStr := mustMarshalType[types.Forwarding](packetData.Forwarding)
+	forwardingStr := mustMarshalType[types.ForwardingPacketData](packetData.Forwarding)
 
 	ctx.EventManager().EmitEvents(sdk.Events{
 		sdk.NewEvent(

diff --git a/modules/apps/transfer/internal/packet_test.go b/modules/apps/transfer/internal/packet_test.go
@@ -10,7 +10,7 @@ import (
 	"github.com/cosmos/ibc-go/v8/modules/apps/transfer/types"
 )
 
-var emptyForwarding = types.Forwarding{}
+var emptyForwardingPacketData = types.ForwardingPacketData{}
 
 func TestUnmarshalPacketData(t *testing.T) {
 	var (
@@ -37,7 +37,7 @@ func TestUnmarshalPacketData(t *testing.T) {
 							Denom:  types.NewDenom("atom", types.NewTrace("transfer", "channel-0")),
 							Amount: "1000",
 						},
-					}, "sender", "receiver", "", emptyForwarding)
+					}, "sender", "receiver", "", emptyForwardingPacketData)
 
 				packetDataBz = packetData.GetBytes()
 				version = types.V2
@@ -94,7 +94,7 @@ func TestPacketV1ToPacketV2(t *testing.T) {
 						Denom:  types.NewDenom("atom", types.NewTrace("transfer", "channel-0")),
 						Amount: "1000",
 					},
-				}, sender, receiver, "", emptyForwarding),
+				}, sender, receiver, "", emptyForwardingPacketData),
 			nil,
 		},
 		{
@@ -106,7 +106,7 @@ func TestPacketV1ToPacketV2(t *testing.T) {
 						Denom:  types.NewDenom("atom"),
 						Amount: "1000",
 					},
-				}, sender, receiver, "", emptyForwarding),
+				}, sender, receiver, "", emptyForwardingPacketData),
 			nil,
 		},
 		{
@@ -118,7 +118,7 @@ func TestPacketV1ToPacketV2(t *testing.T) {
 						Denom:  types.NewDenom("atom/withslash", types.NewTrace("transfer", "channel-0")),
 						Amount: "1000",
 					},
-				}, sender, receiver, "", emptyForwarding),
+				}, sender, receiver, "", emptyForwardingPacketData),
 			nil,
 		},
 		{
@@ -130,7 +130,7 @@ func TestPacketV1ToPacketV2(t *testing.T) {
 						Denom:  types.NewDenom("atom/", types.NewTrace("transfer", "channel-0")),
 						Amount: "1000",
 					},
-				}, sender, receiver, "", emptyForwarding),
+				}, sender, receiver, "", emptyForwardingPacketData),
 			nil,
 		},
 		{
@@ -142,7 +142,7 @@ func TestPacketV1ToPacketV2(t *testing.T) {
 						Denom:  types.NewDenom("atom/pool", types.NewTrace("transfer", "channel-0"), types.NewTrace("transfer", "channel-1")),
 						Amount: "1000",
 					},
-				}, sender, receiver, "", emptyForwarding),
+				}, sender, receiver, "", emptyForwardingPacketData),
 			nil,
 		},
 		{
@@ -154,7 +154,7 @@ func TestPacketV1ToPacketV2(t *testing.T) {
 						Denom:  types.NewDenom("atom", types.NewTrace("transfer", "channel-0"), types.NewTrace("transfer", "channel-1"), types.NewTrace("transfer-custom", "channel-2")),
 						Amount: "1000",
 					},
-				}, sender, receiver, "", emptyForwarding),
+				}, sender, receiver, "", emptyForwardingPacketData),
 			nil,
 		},
 		{
@@ -166,7 +166,7 @@ func TestPacketV1ToPacketV2(t *testing.T) {
 						Denom:  types.NewDenom("atom/pool", types.NewTrace("transfer", "channel-0"), types.NewTrace("transfer", "channel-1"), types.NewTrace("transfer-custom", "channel-2")),
 						Amount: "1000",
 					},
-				}, sender, receiver, "", emptyForwarding),
+				}, sender, receiver, "", emptyForwardingPacketData),
 			nil,
 		},
 		{

diff --git a/modules/apps/transfer/keeper/export_test.go b/modules/apps/transfer/keeper/export_test.go
@@ -34,6 +34,6 @@ func (k Keeper) TokenFromCoin(ctx sdk.Context, coin sdk.Coin) (types.Token, erro
 }
 
 // CreatePacketDataBytesFromVersion is a wrapper around createPacketDataBytesFromVersion for testing purposes
-func CreatePacketDataBytesFromVersion(appVersion, sender, receiver, memo string, tokens types.Tokens, forwarding types.Forwarding) []byte {
-	return createPacketDataBytesFromVersion(appVersion, sender, receiver, memo, tokens, forwarding)
+func CreatePacketDataBytesFromVersion(appVersion, sender, receiver, memo string, tokens types.Tokens, hops []types.Hop) []byte {
+	return createPacketDataBytesFromVersion(appVersion, sender, receiver, memo, tokens, hops)
 }
diff --git a/modules/apps/transfer/keeper/forwarding.go b/modules/apps/transfer/keeper/forwarding.go
@@ -14,13 +14,9 @@ import (
 
 // forwardPacket forwards a fungible FungibleTokenPacketDataV2 to the next hop in the forwarding path.
 func (k Keeper) forwardPacket(ctx sdk.Context, data types.FungibleTokenPacketDataV2, packet channeltypes.Packet, receivedCoins sdk.Coins) error {
-	var memo string
-
 	var nextForwardingPath types.Forwarding
-	if len(data.Forwarding.Hops) == 1 {
-		memo = data.Forwarding.Memo
-	} else {
-		nextForwardingPath = types.NewForwarding(data.Forwarding.Memo, data.Forwarding.Hops[1:]...)
+	if len(data.Forwarding.Hops) > 1 {
+		nextForwardingPath = types.NewForwarding(false, data.Forwarding.Hops[1:]...)
 	}
 
 	// sending from the forward escrow address to the original receiver address.
@@ -34,7 +30,7 @@ func (k Keeper) forwardPacket(ctx sdk.Context, data types.FungibleTokenPacketDat
 		data.Receiver,
 		clienttypes.ZeroHeight(),
 		packet.TimeoutTimestamp,
-		memo,
+		data.Forwarding.DestinationMemo,
 		nextForwardingPath,
 	)
 

diff --git a/modules/apps/transfer/keeper/mbt_relay_test.go b/modules/apps/transfer/keeper/mbt_relay_test.go
@@ -159,7 +159,7 @@ func FungibleTokenPacketFromTla(packet TlaFungibleTokenPacket) FungibleTokenPack
 			AddressFromString(packet.Data.Sender),
 			AddressFromString(packet.Data.Receiver),
 			"",
-			types.Forwarding{},
+			types.ForwardingPacketData{},
 		),
 	}
 }

diff --git a/modules/apps/transfer/keeper/relay.go b/modules/apps/transfer/keeper/relay.go
@@ -140,7 +140,7 @@ func (k Keeper) sendTransfer(
 		tokens = append(tokens, token)
 	}
 
-	packetDataBytes := createPacketDataBytesFromVersion(appVersion, sender.String(), receiver, memo, tokens, forwarding)
+	packetDataBytes := createPacketDataBytesFromVersion(appVersion, sender.String(), receiver, memo, tokens, forwarding.Hops)
 
 	sequence, err := k.ics4Wrapper.SendPacket(ctx, channelCap, sourcePort, sourceChannel, timeoutHeight, timeoutTimestamp, packetDataBytes)
 	if err != nil {
@@ -431,7 +431,7 @@ func (k Keeper) tokenFromCoin(ctx sdk.Context, coin sdk.Coin) (types.Token, erro
 }
 
 // createPacketDataBytesFromVersion creates the packet data bytes to be sent based on the application version.
-func createPacketDataBytesFromVersion(appVersion, sender, receiver, memo string, tokens types.Tokens, forwarding types.Forwarding) []byte {
+func createPacketDataBytesFromVersion(appVersion, sender, receiver, memo string, tokens types.Tokens, hops []types.Hop) []byte {
 	var packetDataBytes []byte
 	switch appVersion {
 	case types.V1:
@@ -444,7 +444,14 @@ func createPacketDataBytesFromVersion(appVersion, sender, receiver, memo string,
 		packetData := types.NewFungibleTokenPacketData(token.Denom.Path(), token.Amount, sender, receiver, memo)
 		packetDataBytes = packetData.GetBytes()
 	case types.V2:
-		packetData := types.NewFungibleTokenPacketDataV2(tokens, sender, receiver, memo, forwarding)
+		// If forwarding is needed, move memo to forwarding packet data and set packet.Memo to empty string.
+		var forwardingPacketData types.ForwardingPacketData
+		if len(hops) > 0 {
+			forwardingPacketData = types.NewForwardingPacketData(memo, hops...)
+			memo = ""
+		}
+
+		packetData := types.NewFungibleTokenPacketDataV2(tokens, sender, receiver, memo, forwardingPacketData)
 		packetDataBytes = packetData.GetBytes()
 	default:
 		panic(fmt.Errorf("app version must be one of %s", types.SupportedVersions))