From bcee04857318c26a480a9e027550deaec956fca0 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 12:08:21 +0200 Subject: [PATCH 1/3] refactor: address various self review comments --- CHANGELOG.md | 1 + docs/docs/05-migrations/13-v8-to-v9.md | 1 + e2e/tests/wasm/grandpa_test.go | 4 +- e2e/testsuite/testsuite.go | 2 +- modules/apps/transfer/types/keys.go | 4 +- modules/apps/transfer/types/token_test.go | 83 +++++++++++++++++++++++ 6 files changed, 90 insertions(+), 5 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/tests/wasm/grandpa_test.go b/e2e/tests/wasm/grandpa_test.go index 448cd840e31..69b4ff22937 100644 --- a/e2e/tests/wasm/grandpa_test.go +++ b/e2e/tests/wasm/grandpa_test.go @@ -140,7 +140,7 @@ func (s *GrandpaTestSuite) TestMsgTransfer_Succeeds_GrandpaContract() { s.Require().NoError(err) // Create a new channel & get channels from each chain - err = r.CreateChannel(ctx, eRep, pathName, ibc.DefaultChannelOpts()) + err = r.CreateChannel(ctx, eRep, pathName, s.TransferChannelOptions()) s.Require().NoError(err) err = testutil.WaitForBlocks(ctx, 1, cosmosChain, polkadotChain) s.Require().NoError(err) @@ -293,7 +293,7 @@ func (s *GrandpaTestSuite) TestMsgTransfer_TimesOut_GrandpaContract() { s.Require().NoError(err) // Create a new channel & get channels from each chain - err = r.CreateChannel(ctx, eRep, pathName, ibc.DefaultChannelOpts()) + err = r.CreateChannel(ctx, eRep, pathName, s.TransferChannelOptions()) s.Require().NoError(err) err = testutil.WaitForBlocks(ctx, 1, cosmosChain, polkadotChain) s.Require().NoError(err) 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..e284a3e57f7 100644 --- a/modules/apps/transfer/types/token_test.go +++ b/modules/apps/transfer/types/token_test.go @@ -149,3 +149,86 @@ 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()) + } + +} From 19f306ca7c3d047ab735b4beb2600a60377cbdda 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 12:11:50 +0200 Subject: [PATCH 2/3] revert: unnecessary change --- e2e/tests/wasm/grandpa_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/e2e/tests/wasm/grandpa_test.go b/e2e/tests/wasm/grandpa_test.go index 69b4ff22937..448cd840e31 100644 --- a/e2e/tests/wasm/grandpa_test.go +++ b/e2e/tests/wasm/grandpa_test.go @@ -140,7 +140,7 @@ func (s *GrandpaTestSuite) TestMsgTransfer_Succeeds_GrandpaContract() { s.Require().NoError(err) // Create a new channel & get channels from each chain - err = r.CreateChannel(ctx, eRep, pathName, s.TransferChannelOptions()) + err = r.CreateChannel(ctx, eRep, pathName, ibc.DefaultChannelOpts()) s.Require().NoError(err) err = testutil.WaitForBlocks(ctx, 1, cosmosChain, polkadotChain) s.Require().NoError(err) @@ -293,7 +293,7 @@ func (s *GrandpaTestSuite) TestMsgTransfer_TimesOut_GrandpaContract() { s.Require().NoError(err) // Create a new channel & get channels from each chain - err = r.CreateChannel(ctx, eRep, pathName, s.TransferChannelOptions()) + err = r.CreateChannel(ctx, eRep, pathName, ibc.DefaultChannelOpts()) s.Require().NoError(err) err = testutil.WaitForBlocks(ctx, 1, cosmosChain, polkadotChain) s.Require().NoError(err) From 690179388cf5959aeb61d363ca8c938554193c8b 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 12:41:26 +0200 Subject: [PATCH 3/3] lint --- modules/apps/transfer/types/token_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/modules/apps/transfer/types/token_test.go b/modules/apps/transfer/types/token_test.go index e284a3e57f7..027405b582e 100644 --- a/modules/apps/transfer/types/token_test.go +++ b/modules/apps/transfer/types/token_test.go @@ -230,5 +230,4 @@ func TestTokens_String(t *testing.T) { for _, tt := range cases { require.Equal(t, tt.expected, tt.input.String()) } - }