imp: self review comments for ics20-v2 (#6360)

* refactor: address various self review comments

* revert: unnecessary change

* lint

CHANGELOG.md

@@ -53,6 +53,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 * (core) [\#6138](https://github.com/cosmos/ibc-go/pull/6138) Remove `Router` reference from IBC core keeper and use instead the router on the existing `PortKeeper` reference.
 * (core/04-channel) [\#6023](https://github.com/cosmos/ibc-go/pull/6023) Remove emission of non-hexlified event attributes `packet_data` and `packet_ack`.
 * (core) [\#6320](https://github.com/cosmos/ibc-go/pull/6320) Remove unnecessary `Proof` interface from `exported` package.
+* (core/05-port) [\#6341](https://github.com/cosmos/ibc-go/pull/6341) Modify `UnmarshalPacketData` interface to take in the context, portID, and channelID. This allows for packet data's to be unmarshaled based on the channel version. 
 
 ### State Machine Breaking
 

docs/docs/05-migrations/13-v8-to-v9.md

@@ -66,6 +66,7 @@ func AssertEvents(
 
 ### IBC core
 
+- `UnmarshalPacketData` now takes in the context, portID, and channelID. This allows the packet data to be unmarshaled based on the channel version.
 - `Router` reference has been removed from IBC core keeper: [#6138](https://github.com/cosmos/ibc-go/pull/6138)
 
 ### ICS27 - Interchain Accounts

e2e/testsuite/testsuite.go

@@ -154,8 +154,8 @@ func (s *E2ETestSuite) ConfigureRelayer(ctx context.Context, chainA, chainB ibc.
 	pathName := s.generatePathName()
 
 	channelOptions := ibc.DefaultChannelOpts()
-	// TODO: better way to do this.
 	// For now, set the version to the latest transfer module version
+	// DefaultChannelOpts uses V1 at the moment
 	channelOptions.Version = transfertypes.V2
 
 	if channelOpts != nil {

modules/apps/transfer/types/keys.go

@@ -38,8 +38,8 @@ const (
 	// V1 defines first version of the IBC transfer module
 	V1 = "ics20-1"
 
-	// V2 defines the current version the IBC transfer
-	// module supports
+	// V2 defines the transfer version which introduces multidenom support
+	// through the FungibleTokenPacketDataV2. It is the latest version.
 	V2 = "ics20-2"
 
 	// escrowAddressVersion should remain as ics20-1 to avoid the address changing.

modules/apps/transfer/types/token_test.go

@@ -149,3 +149,85 @@ func TestValidate(t *testing.T) {
 		})
 	}
 }
+
+func TestTokens_String(t *testing.T) {
+	cases := []struct {
+		name     string
+		input    Tokens
+		expected string
+	}{
+		{
+			"empty tokens",
+			Tokens{},
+			"",
+		},
+		{
+			"single token, no trace",
+			Tokens{
+				Token{
+					Denom:  "tree",
+					Amount: "1",
+					Trace:  []string{},
+				},
+			},
+			`denom:"tree" amount:"1" `,
+		},
+		{
+			"single token with trace",
+			Tokens{
+				Token{
+					Denom:  "tree",
+					Amount: "1",
+					Trace:  []string{"portid/channelid"},
+				},
+			},
+			`denom:"tree" amount:"1" trace:"portid/channelid" `,
+		},
+		{
+			"multiple tokens, no trace",
+			Tokens{
+				Token{
+					Denom:  "tree",
+					Amount: "1",
+					Trace:  []string{},
+				},
+				Token{
+					Denom:  "gas",
+					Amount: "2",
+					Trace:  []string{},
+				},
+				Token{
+					Denom:  "mineral",
+					Amount: "3",
+					Trace:  []string{},
+				},
+			},
+			`denom:"tree" amount:"1" ,denom:"gas" amount:"2" ,denom:"mineral" amount:"3" `,
+		},
+		{
+			"multiple tokens, trace and no trace",
+			Tokens{
+				Token{
+					Denom:  "tree",
+					Amount: "1",
+					Trace:  []string{},
+				},
+				Token{
+					Denom:  "gas",
+					Amount: "2",
+					Trace:  []string{"portid/channelid"},
+				},
+				Token{
+					Denom:  "mineral",
+					Amount: "3",
+					Trace:  []string{"portid/channelid", "transfer/channel-52"},
+				},
+			},
+			`denom:"tree" amount:"1" ,denom:"gas" amount:"2" trace:"portid/channelid" ,denom:"mineral" amount:"3" trace:"portid/channelid" trace:"transfer/channel-52" `,
+		},
+	}
+
+	for _, tt := range cases {
+		require.Equal(t, tt.expected, tt.input.String())
+	}
+}