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 <[email protected]> Co-authored-by: DimitrisJim <[email protected]>
cosmos · Mar 12, 2024 · 1ebede6 · 1ebede6
1 parent 9aa7151
commit 1ebede6
Show file tree

Hide file tree

Showing 7 changed files with 53 additions and 37 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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)