From d4b06c8d6aa2352e1adbd90aae9a868276469916 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?colin=20axn=C3=A9r?=
 <25233464+colin-axner@users.noreply.github.com>
Date: Thu, 23 May 2024 13:04:48 +0200
Subject: [PATCH] imp: self review comments for ics20-v2 (#6360)

* refactor: address various self review comments

* revert: unnecessary change

* lint
---
 CHANGELOG.md                              |  1 +
 docs/docs/05-migrations/13-v8-to-v9.md    |  1 +
 e2e/testsuite/testsuite.go                |  2 +-
 modules/apps/transfer/types/keys.go       |  4 +-
 modules/apps/transfer/types/token_test.go | 82 +++++++++++++++++++++++
 5 files changed, 87 insertions(+), 3 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index d9299281e19..00a5a5ba760 100644
--- a/CHANGELOG.md
+++ b/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
 
diff --git a/docs/docs/05-migrations/13-v8-to-v9.md b/docs/docs/05-migrations/13-v8-to-v9.md
index afbf6a0e98c..bd67a09515d 100644
--- a/docs/docs/05-migrations/13-v8-to-v9.md
+++ b/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
diff --git a/e2e/testsuite/testsuite.go b/e2e/testsuite/testsuite.go
index 221c523f186..4552dea20ee 100644
--- a/e2e/testsuite/testsuite.go
+++ b/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 {
diff --git a/modules/apps/transfer/types/keys.go b/modules/apps/transfer/types/keys.go
index 5e1fa0d70e6..f8faa1336fe 100644
--- a/modules/apps/transfer/types/keys.go
+++ b/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.
diff --git a/modules/apps/transfer/types/token_test.go b/modules/apps/transfer/types/token_test.go
index afc008b6ed1..027405b582e 100644
--- a/modules/apps/transfer/types/token_test.go
+++ b/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())
+	}
+}