From 1ebede6c7d4d88f99b9046fa9e40db6571b46b81 Mon Sep 17 00:00:00 2001 From: GnaD <89174180+GNaD13@users.noreply.github.com> Date: Tue, 12 Mar 2024 19:40:12 +0700 Subject: [PATCH] imp: set `host_connection_id` in version metadata on `ChanOpenTry` (#5533) * remove host connection ID in default metadata * allow empty host connection id in validateConnectionParams * add host connection id to the metadata on host open try * nit * add test init with empty host connection ID * add test host * correct test * nit * lint * remove assert * TestOnChanOpenInit * OnChanOpenTry * TestOnChanUpgradeInit * TestOnChanUpgradeTry * TestOnChanUpgradeTry * remove check * test * test * nit * nit * add check version * NewDefaultMetadataString * e2e * change func name and var place * callbacks * revert icatypes.NewDefaultMetadataString * add back NewDefaultMetadataString with hostConnectionID * nit * nit * nit * remove unused code * revert * address self-review comments * add import back * some more refactoring * more cleanup * remove unused test * change set version func * lint * still set host connection ID in our controller implementation * remove empty lines * remove unnecessary line * add changelog --------- Co-authored-by: Carlos Rodriguez Co-authored-by: DimitrisJim --- CHANGELOG.md | 2 + .../controller/ibc_middleware_test.go | 13 ++--- .../controller/keeper/handshake_test.go | 14 ++++-- .../migrations/v6/migrations_test.go | 6 ++- .../host/keeper/handshake.go | 3 ++ .../host/keeper/handshake_test.go | 48 ++++++++++++------- .../27-interchain-accounts/types/metadata.go | 4 +- 7 files changed, 53 insertions(+), 37 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7803cb99797..a3d37512f5b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,6 +44,8 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Improvements +* (apps/27-interchain-accounts) [\#5533](https://github.com/cosmos/ibc-go/pull/5533) ICA host sets the host connection ID on `OnChanOpenTry`, so that ICA controller implementations are not obliged to set the value on `OnChanOpenInit` if they are not able. + ### Features ### Bug Fixes diff --git a/modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go b/modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go index bdceec9393d..c8ef37eecdf 100644 --- a/modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go +++ b/modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go @@ -31,13 +31,7 @@ var ( TestPortID, _ = icatypes.NewControllerPortID(TestOwnerAddress) // TestVersion defines a reusable interchainaccounts version string for testing purposes - TestVersion = string(icatypes.ModuleCdc.MustMarshalJSON(&icatypes.Metadata{ - Version: icatypes.Version, - ControllerConnectionId: ibctesting.FirstConnectionID, - HostConnectionId: ibctesting.FirstConnectionID, - Encoding: icatypes.EncodingProtobuf, - TxType: icatypes.TxTypeSDKMultiMsg, - })) + TestVersion = icatypes.NewDefaultMetadataString(ibctesting.FirstConnectionID, ibctesting.FirstConnectionID) ) type InterchainAccountsTestSuite struct { @@ -244,7 +238,7 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenInit() { ) if tc.expPass { - suite.Require().Equal(icatypes.NewDefaultMetadataString(path.EndpointA.ConnectionID, path.EndpointB.ConnectionID), version) + suite.Require().Equal(TestVersion, version) suite.Require().NoError(err) } else { suite.Require().Error(err) @@ -842,8 +836,7 @@ func (suite *InterchainAccountsTestSuite) TestOnChanUpgradeInit() { err := RegisterInterchainAccount(path.EndpointA, TestOwnerAddress) suite.Require().NoError(err) - metadata := icatypes.NewDefaultMetadata(path.EndpointA.ConnectionID, path.EndpointB.ConnectionID) - version = string(icatypes.ModuleCdc.MustMarshalJSON(&metadata)) + version = icatypes.NewDefaultMetadataString(path.EndpointA.ConnectionID, path.EndpointB.ConnectionID) tc.malleate() // malleate mutates test data diff --git a/modules/apps/27-interchain-accounts/controller/keeper/handshake_test.go b/modules/apps/27-interchain-accounts/controller/keeper/handshake_test.go index 83340686207..ec8a671b307 100644 --- a/modules/apps/27-interchain-accounts/controller/keeper/handshake_test.go +++ b/modules/apps/27-interchain-accounts/controller/keeper/handshake_test.go @@ -16,10 +16,11 @@ const ( func (suite *KeeperTestSuite) TestOnChanOpenInit() { var ( - channel *channeltypes.Channel - path *ibctesting.Path - chanCap *capabilitytypes.Capability - metadata icatypes.Metadata + channel *channeltypes.Channel + path *ibctesting.Path + chanCap *capabilitytypes.Capability + metadata icatypes.Metadata + expectedVersion string ) testCases := []struct { @@ -54,6 +55,7 @@ func (suite *KeeperTestSuite) TestOnChanOpenInit() { "success: empty channel version returns default metadata JSON string", func() { channel.Version = "" + expectedVersion = icatypes.NewDefaultMetadataString(path.EndpointA.ConnectionID, path.EndpointB.ConnectionID) }, nil, }, @@ -275,6 +277,8 @@ func (suite *KeeperTestSuite) TestOnChanOpenInit() { versionBytes, err := icatypes.ModuleCdc.MarshalJSON(&metadata) suite.Require().NoError(err) + expectedVersion = string(versionBytes) + counterparty := channeltypes.NewCounterparty(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) channel = &channeltypes.Channel{ State: channeltypes.INIT, @@ -296,7 +300,7 @@ func (suite *KeeperTestSuite) TestOnChanOpenInit() { expPass := tc.expError == nil if expPass { suite.Require().NoError(err) - suite.Require().Equal(string(versionBytes), version) + suite.Require().Equal(expectedVersion, version) } else { suite.Require().Error(err) suite.Require().ErrorIs(err, tc.expError) diff --git a/modules/apps/27-interchain-accounts/controller/migrations/v6/migrations_test.go b/modules/apps/27-interchain-accounts/controller/migrations/v6/migrations_test.go index 813436cd3ed..dcbe067ceaf 100644 --- a/modules/apps/27-interchain-accounts/controller/migrations/v6/migrations_test.go +++ b/modules/apps/27-interchain-accounts/controller/migrations/v6/migrations_test.go @@ -28,6 +28,8 @@ type MigrationsTestSuite struct { } func (suite *MigrationsTestSuite) SetupTest() { + version := icatypes.NewDefaultMetadataString(ibctesting.FirstConnectionID, ibctesting.FirstConnectionID) + suite.coordinator = ibctesting.NewCoordinator(suite.T(), 2) suite.chainA = suite.coordinator.GetChain(ibctesting.GetChainID(1)) @@ -38,8 +40,8 @@ func (suite *MigrationsTestSuite) SetupTest() { suite.path.EndpointB.ChannelConfig.PortID = icatypes.HostPortID suite.path.EndpointA.ChannelConfig.Order = channeltypes.ORDERED suite.path.EndpointB.ChannelConfig.Order = channeltypes.ORDERED - suite.path.EndpointA.ChannelConfig.Version = icatypes.NewDefaultMetadataString(ibctesting.FirstConnectionID, ibctesting.FirstConnectionID) - suite.path.EndpointB.ChannelConfig.Version = icatypes.NewDefaultMetadataString(ibctesting.FirstConnectionID, ibctesting.FirstConnectionID) + suite.path.EndpointA.ChannelConfig.Version = version + suite.path.EndpointB.ChannelConfig.Version = version } func (suite *MigrationsTestSuite) SetupPath() error { diff --git a/modules/apps/27-interchain-accounts/host/keeper/handshake.go b/modules/apps/27-interchain-accounts/host/keeper/handshake.go index 46e9e91fa8c..a9a62c55e75 100644 --- a/modules/apps/27-interchain-accounts/host/keeper/handshake.go +++ b/modules/apps/27-interchain-accounts/host/keeper/handshake.go @@ -39,6 +39,9 @@ func (k Keeper) OnChanOpenTry( return "", err } + // set here the HostConnectionId in case the controller did not set it + metadata.HostConnectionId = connectionHops[0] + if err = icatypes.ValidateHostMetadata(ctx, k.channelKeeper, connectionHops, metadata); err != nil { return "", err } diff --git a/modules/apps/27-interchain-accounts/host/keeper/handshake_test.go b/modules/apps/27-interchain-accounts/host/keeper/handshake_test.go index d8ce6db3ab4..90a5cb74874 100644 --- a/modules/apps/27-interchain-accounts/host/keeper/handshake_test.go +++ b/modules/apps/27-interchain-accounts/host/keeper/handshake_test.go @@ -70,7 +70,8 @@ func (suite *KeeperTestSuite) TestOnChanOpenTry() { suite.Require().NoError(err) suite.openAndCloseChannel(path) - }, true, + }, + true, }, { "success - reopening account with new address", @@ -90,7 +91,20 @@ func (suite *KeeperTestSuite) TestOnChanOpenTry() { // assert interchain account address mapping was deleted _, found := suite.chainB.GetSimApp().ICAHostKeeper.GetInterchainAccountAddress(suite.chainB.GetContext(), path.EndpointB.ConnectionID, path.EndpointA.ChannelConfig.PortID) suite.Require().False(found) - }, true, + }, + true, + }, + { + "success - empty host connection ID", + func() { + metadata.HostConnectionId = "" + + versionBytes, err := icatypes.ModuleCdc.MarshalJSON(&metadata) + suite.Require().NoError(err) + + path.EndpointA.ChannelConfig.Version = string(versionBytes) + }, + true, }, { "success - previous metadata is different", @@ -128,7 +142,8 @@ func (suite *KeeperTestSuite) TestOnChanOpenTry() { acc := suite.chainB.GetSimApp().AccountKeeper.GetAccount(suite.chainB.GetContext(), sdk.MustAccAddressFromBech32(addr)) suite.chainB.GetSimApp().AccountKeeper.RemoveAccount(suite.chainB.GetContext(), acc) - }, false, + }, + false, }, { "reopening account fails - existing account is not interchain account type", @@ -152,7 +167,8 @@ func (suite *KeeperTestSuite) TestOnChanOpenTry() { // overwrite existing account with only base account type, not intercahin account type suite.chainB.GetSimApp().AccountKeeper.SetAccount(suite.chainB.GetContext(), icaAcc.BaseAccount) - }, false, + }, + false, }, { "account already exists", @@ -222,18 +238,6 @@ func (suite *KeeperTestSuite) TestOnChanOpenTry() { }, false, }, - { - "invalid host connection ID", - func() { - metadata.HostConnectionId = "invalid-connnection-id" - - versionBytes, err := icatypes.ModuleCdc.MarshalJSON(&metadata) - suite.Require().NoError(err) - - path.EndpointA.ChannelConfig.Version = string(versionBytes) - }, - false, - }, { "invalid counterparty version", func() { @@ -264,7 +268,8 @@ func (suite *KeeperTestSuite) TestOnChanOpenTry() { // set the active channelID in state suite.chainB.GetSimApp().ICAHostKeeper.SetActiveChannelID(suite.chainB.GetContext(), ibctesting.FirstConnectionID, path.EndpointA.ChannelConfig.PortID, path.EndpointB.ChannelID) - }, false, + }, + false, }, { "channel is already active (FLUSHING state)", @@ -306,6 +311,8 @@ func (suite *KeeperTestSuite) TestOnChanOpenTry() { versionBytes, err := icatypes.ModuleCdc.MarshalJSON(&metadata) suite.Require().NoError(err) + expectedMetadata := metadata + counterparty := channeltypes.NewCounterparty(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) channel = &channeltypes.Channel{ State: channeltypes.TRYOPEN, @@ -336,6 +343,12 @@ func (suite *KeeperTestSuite) TestOnChanOpenTry() { // Check if account is created interchainAccount := suite.chainB.GetSimApp().AccountKeeper.GetAccount(suite.chainB.GetContext(), interchainAccAddr) suite.Require().Equal(interchainAccount.GetAddress().String(), storedAddr) + + expectedMetadata.Address = storedAddr + expectedVersionBytes, err := icatypes.ModuleCdc.MarshalJSON(&expectedMetadata) + suite.Require().NoError(err) + + suite.Require().Equal(string(expectedVersionBytes), version) } else { suite.Require().Error(err) suite.Require().Equal("", version) @@ -577,7 +590,6 @@ func (suite *KeeperTestSuite) TestOnChanUpgradeTry() { metadata = icatypes.NewDefaultMetadata(path.EndpointA.ConnectionID, path.EndpointB.ConnectionID) // use the same address as the previous metadata. metadata.Address = currentMetadata.Address - // this is the actual change to the version. metadata.Encoding = icatypes.EncodingProto3JSON diff --git a/modules/apps/27-interchain-accounts/types/metadata.go b/modules/apps/27-interchain-accounts/types/metadata.go index 8cf3772bf94..0508466aad5 100644 --- a/modules/apps/27-interchain-accounts/types/metadata.go +++ b/modules/apps/27-interchain-accounts/types/metadata.go @@ -33,7 +33,7 @@ func NewMetadata(version, controllerConnectionID, hostConnectionID, accAddress, } // NewDefaultMetadata creates and returns a new ICS27 Metadata instance containing the default ICS27 Metadata values -// with the provided controller and host connection identifiers +// with the provided controller and host connection identifiers. The host connection identifier may be an empty string. func NewDefaultMetadata(controllerConnectionID, hostConnectionID string) Metadata { metadata := Metadata{ ControllerConnectionId: controllerConnectionID, @@ -47,7 +47,7 @@ func NewDefaultMetadata(controllerConnectionID, hostConnectionID string) Metadat } // NewDefaultMetadataString creates and returns a new JSON encoded version string containing the default ICS27 Metadata values -// with the provided controller and host connection identifiers +// with the provided controller and host connection identifiers. The host connection identifier may be an empty string. func NewDefaultMetadataString(controllerConnectionID, hostConnectionID string) string { metadata := NewDefaultMetadata(controllerConnectionID, hostConnectionID)