refactor: move ica connection identifiers from port to version metada…

…ta bytestring (#700) * define and generate new metadata proto type * refactor types pkg, remove version string parsers, add PortPrefix * refactor ica entrypoint and handshake to handle connection ids in metadata * fixing broken test cases * adding controller port and metadata validation, adding new testcases * updating proto doc and removing commented out code * updating testcase names, adding metadata constructor func, updating PortPrefix and removing Delimiter * adding ErrInvalidControllerPort and ErrInvalidHostPort * Apply suggestions from code review Co-authored-by: Carlos Rodriguez <crodveg@yahoo.es> * updating error msgs to use expected, got format * adding inline metadata documentation as per review, updating bz to versionBytes Co-authored-by: Carlos Rodriguez <crodveg@yahoo.es>
cosmos · Jan 13, 2022 · d882b43 · d882b43
1 parent dd9c385
commit d882b43
Show file tree

Hide file tree

Showing 24 changed files with 870 additions and 519 deletions.
diff --git a/docs/ibc/proto-docs.md b/docs/ibc/proto-docs.md
@@ -14,6 +14,9 @@
     - [HostGenesisState](#ibc.applications.interchain_accounts.v1.HostGenesisState)
     - [RegisteredInterchainAccount](#ibc.applications.interchain_accounts.v1.RegisteredInterchainAccount)
 
+- [ibc/applications/interchain_accounts/v1/metadata.proto](#ibc/applications/interchain_accounts/v1/metadata.proto)
+    - [Metadata](#ibc.applications.interchain_accounts.v1.Metadata)
+
 - [ibc/applications/interchain_accounts/v1/packet.proto](#ibc/applications/interchain_accounts/v1/packet.proto)
     - [CosmosTx](#ibc.applications.interchain_accounts.v1.CosmosTx)
     - [InterchainAccountPacketData](#ibc.applications.interchain_accounts.v1.InterchainAccountPacketData)
@@ -388,6 +391,41 @@ RegisteredInterchainAccount contains a pairing of controller port ID and associa
 
 
 
+ <!-- end messages -->
+
+ <!-- end enums -->
+
+ <!-- end HasExtensions -->
+
+ <!-- end services -->
+
+
+
+<a name="ibc/applications/interchain_accounts/v1/metadata.proto"></a>
+<p align="right"><a href="#top">Top</a></p>
+
+## ibc/applications/interchain_accounts/v1/metadata.proto
+
+
+
+<a name="ibc.applications.interchain_accounts.v1.Metadata"></a>
+
+### Metadata
+Metadata defines a set of protocol specific data encoded into the ICS27 channel version bytestring
+See ICS004: https://github.com/cosmos/ibc/tree/master/spec/core/ics-004-channel-and-packet-semantics#Versioning
+
+
+| Field | Type | Label | Description |
+| ----- | ---- | ----- | ----------- |
+| `version` | [string](#string) |  | version defines the ICS27 protocol version |
+| `controller_connection_id` | [string](#string) |  | controller_connection_id is the connection identifier associated with the controller chain |
+| `host_connection_id` | [string](#string) |  | host_connection_id is the connection identifier associated with the host chain |
+| `address` | [string](#string) |  | address defines the interchain account address to be fulfilled upon the OnChanOpenTry handshake step NOTE: the address field is empty on the OnChanOpenInit handshake step |
+
+
+
+
+
  <!-- end messages -->
 
  <!-- end enums -->

diff --git a/modules/apps/27-interchain-accounts/controller/ibc_module_test.go b/modules/apps/27-interchain-accounts/controller/ibc_module_test.go
@@ -18,15 +18,24 @@ import (
 )
 
 var (
+	// TODO: Cosmos-SDK ADR-28: Update crypto.AddressHash() when sdk uses address.Module()
+	// https://github.com/cosmos/cosmos-sdk/issues/10225
+	//
 	// TestAccAddress defines a resuable bech32 address for testing purposes
-	// TODO: update crypto.AddressHash() when sdk uses address.Module()
 	TestAccAddress = icatypes.GenerateAddress(sdk.AccAddress(crypto.AddressHash([]byte(icatypes.ModuleName))), TestPortID)
+
 	// TestOwnerAddress defines a reusable bech32 address for testing purposes
 	TestOwnerAddress = "cosmos17dtl0mjt3t77kpuhg2edqzjpszulwhgzuj9ljs"
+
 	// TestPortID defines a resuable port identifier for testing purposes
-	TestPortID, _ = icatypes.GeneratePortID(TestOwnerAddress, ibctesting.FirstConnectionID, ibctesting.FirstConnectionID)
+	TestPortID, _ = icatypes.NewControllerPortID(TestOwnerAddress)
+
 	// TestVersion defines a resuable interchainaccounts version string for testing purposes
-	TestVersion = icatypes.NewAppVersion(icatypes.VersionPrefix, TestAccAddress.String())
+	TestVersion = string(icatypes.ModuleCdc.MustMarshalJSON(&icatypes.Metadata{
+		Version:                icatypes.Version,
+		ControllerConnectionId: ibctesting.FirstConnectionID,
+		HostConnectionId:       ibctesting.FirstConnectionID,
+	}))
 )
 
 type InterchainAccountsTestSuite struct {
@@ -55,21 +64,21 @@ func NewICAPath(chainA, chainB *ibctesting.TestChain) *ibctesting.Path {
 	path.EndpointB.ChannelConfig.PortID = icatypes.PortID
 	path.EndpointA.ChannelConfig.Order = channeltypes.ORDERED
 	path.EndpointB.ChannelConfig.Order = channeltypes.ORDERED
-	path.EndpointA.ChannelConfig.Version = icatypes.VersionPrefix
+	path.EndpointA.ChannelConfig.Version = TestVersion
 	path.EndpointB.ChannelConfig.Version = TestVersion
 
 	return path
 }
 
 func InitInterchainAccount(endpoint *ibctesting.Endpoint, owner string) error {
-	portID, err := icatypes.GeneratePortID(owner, endpoint.ConnectionID, endpoint.Counterparty.ConnectionID)
+	portID, err := icatypes.NewControllerPortID(owner)
 	if err != nil {
 		return err
 	}
 
 	channelSequence := endpoint.Chain.App.GetIBCKeeper().ChannelKeeper.GetNextChannelSequence(endpoint.Chain.GetContext())
 
-	if err := endpoint.Chain.GetSimApp().ICAControllerKeeper.InitInterchainAccount(endpoint.Chain.GetContext(), endpoint.ConnectionID, endpoint.Counterparty.ConnectionID, owner); err != nil {
+	if err := endpoint.Chain.GetSimApp().ICAControllerKeeper.InitInterchainAccount(endpoint.Chain.GetContext(), endpoint.ConnectionID, owner); err != nil {
 		return err
 	}
 
@@ -150,7 +159,7 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenInit() {
 			suite.coordinator.SetupConnections(path)
 
 			// mock init interchain account
-			portID, err := icatypes.GeneratePortID(TestOwnerAddress, path.EndpointA.ConnectionID, path.EndpointB.ConnectionID)
+			portID, err := icatypes.NewControllerPortID(TestOwnerAddress)
 			suite.Require().NoError(err)
 
 			portCap := suite.chainA.GetSimApp().IBCKeeper.PortKeeper.BindPort(suite.chainA.GetContext(), portID)
@@ -166,7 +175,7 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenInit() {
 				Ordering:       channeltypes.ORDERED,
 				Counterparty:   counterparty,
 				ConnectionHops: []string{path.EndpointA.ConnectionID},
-				Version:        icatypes.VersionPrefix,
+				Version:        path.EndpointA.ChannelConfig.Version,
 			}
 
 			tc.malleate() // malleate mutates test data
@@ -219,7 +228,7 @@ func (suite *InterchainAccountsTestSuite) TestChanOpenTry() {
 	proofInit, proofHeight := path.EndpointB.Chain.QueryProof(channelKey)
 
 	// use chainA (controller) for ChanOpenTry
-	msg := channeltypes.NewMsgChannelOpenTry(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, TestVersion, channeltypes.ORDERED, []string{path.EndpointA.ConnectionID}, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, icatypes.VersionPrefix, proofInit, proofHeight, icatypes.ModuleName)
+	msg := channeltypes.NewMsgChannelOpenTry(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, TestVersion, channeltypes.ORDERED, []string{path.EndpointA.ConnectionID}, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, TestVersion, proofInit, proofHeight, icatypes.ModuleName)
 	handler := suite.chainA.GetSimApp().MsgServiceRouter().Handler(msg)
 	_, err = handler(suite.chainA.GetContext(), msg)
 

diff --git a/modules/apps/27-interchain-accounts/controller/keeper/account.go b/modules/apps/27-interchain-accounts/controller/keeper/account.go
@@ -15,8 +15,8 @@ import (
 // call 04-channel 'ChanOpenInit'. An error is returned if the port identifier is
 // already in use. Gaining access to interchain accounts whose channels have closed
 // cannot be done with this function. A regular MsgChanOpenInit must be used.
-func (k Keeper) InitInterchainAccount(ctx sdk.Context, connectionID, counterpartyConnectionID, owner string) error {
-	portID, err := icatypes.GeneratePortID(owner, connectionID, counterpartyConnectionID)
+func (k Keeper) InitInterchainAccount(ctx sdk.Context, connectionID, owner string) error {
+	portID, err := icatypes.NewControllerPortID(owner)
 	if err != nil {
 		return err
 	}
@@ -30,7 +30,19 @@ func (k Keeper) InitInterchainAccount(ctx sdk.Context, connectionID, counterpart
 		return sdkerrors.Wrap(err, "unable to bind to newly generated portID")
 	}
 
-	msg := channeltypes.NewMsgChannelOpenInit(portID, icatypes.VersionPrefix, channeltypes.ORDERED, []string{connectionID}, icatypes.PortID, icatypes.ModuleName)
+	connectionEnd, err := k.channelKeeper.GetConnection(ctx, connectionID)
+	if err != nil {
+		return err
+	}
+
+	// NOTE: An empty string is provided for accAddress, to be fulfilled upon OnChanOpenTry handshake step
+	metadata := icatypes.NewMetadata(icatypes.Version, connectionID, connectionEnd.GetCounterparty().GetConnectionID(), "")
+	versionBytes, err := icatypes.ModuleCdc.MarshalJSON(&metadata)
+	if err != nil {
+		return err
+	}
+
+	msg := channeltypes.NewMsgChannelOpenInit(portID, string(versionBytes), channeltypes.ORDERED, []string{connectionID}, icatypes.PortID, icatypes.ModuleName)
 	handler := k.msgRouter.Handler(msg)
 
 	res, err := handler(ctx, msg)

diff --git a/modules/apps/27-interchain-accounts/controller/keeper/account_test.go b/modules/apps/27-interchain-accounts/controller/keeper/account_test.go
@@ -37,7 +37,7 @@ func (suite *KeeperTestSuite) TestInitInterchainAccount() {
 		{
 			"MsgChanOpenInit fails - channel is already active",
 			func() {
-				portID, err := icatypes.GeneratePortID(owner, path.EndpointA.ConnectionID, path.EndpointB.ConnectionID)
+				portID, err := icatypes.NewControllerPortID(TestOwnerAddress)
 				suite.Require().NoError(err)
 
 				suite.chainA.GetSimApp().ICAControllerKeeper.SetActiveChannelID(suite.chainA.GetContext(), portID, path.EndpointA.ChannelID)
@@ -59,7 +59,7 @@ func (suite *KeeperTestSuite) TestInitInterchainAccount() {
 
 			tc.malleate() // malleate mutates test data
 
-			err = suite.chainA.GetSimApp().ICAControllerKeeper.InitInterchainAccount(suite.chainA.GetContext(), path.EndpointA.ConnectionID, path.EndpointB.ConnectionID, owner)
+			err = suite.chainA.GetSimApp().ICAControllerKeeper.InitInterchainAccount(suite.chainA.GetContext(), path.EndpointA.ConnectionID, owner)
 
 			if tc.expPass {
 				suite.Require().NoError(err)

diff --git a/modules/apps/27-interchain-accounts/controller/keeper/handshake.go b/modules/apps/27-interchain-accounts/controller/keeper/handshake.go
@@ -1,6 +1,8 @@
 package keeper
 
 import (
+	"strings"
+
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
 	capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types"
@@ -32,26 +34,25 @@ func (k Keeper) OnChanOpenInit(
 		return sdkerrors.Wrapf(channeltypes.ErrInvalidChannelOrdering, "expected %s channel, got %s", channeltypes.ORDERED, order)
 	}
 
-	connSequence, err := icatypes.ParseControllerConnSequence(portID)
-	if err != nil {
-		return sdkerrors.Wrapf(err, "expected format %s, got %s", icatypes.ControllerPortFormat, portID)
+	if !strings.HasPrefix(portID, icatypes.PortPrefix) {
+		return sdkerrors.Wrapf(icatypes.ErrInvalidControllerPort, "expected %s{owner-account-address}, got %s", icatypes.PortPrefix, portID)
 	}
 
-	counterpartyConnSequence, err := icatypes.ParseHostConnSequence(portID)
-	if err != nil {
-		return sdkerrors.Wrapf(err, "expected format %s, got %s", icatypes.ControllerPortFormat, portID)
+	if counterparty.PortId != icatypes.PortID {
+		return sdkerrors.Wrapf(icatypes.ErrInvalidHostPort, "expected %s, got %s", icatypes.PortID, counterparty.PortId)
 	}
 
-	if err := k.validateControllerPortParams(ctx, connectionHops, connSequence, counterpartyConnSequence); err != nil {
-		return sdkerrors.Wrapf(err, "failed to validate controller port %s", portID)
+	var metadata icatypes.Metadata
+	if err := icatypes.ModuleCdc.UnmarshalJSON([]byte(version), &metadata); err != nil {
+		return sdkerrors.Wrapf(icatypes.ErrUnknownDataType, "cannot unmarshal ICS-27 interchain accounts metadata")
 	}
 
-	if counterparty.PortId != icatypes.PortID {
-		return sdkerrors.Wrapf(porttypes.ErrInvalidPort, "expected %s, got %s", icatypes.PortID, counterparty.PortId)
+	if err := k.validateConnectionParams(ctx, connectionHops, metadata.ControllerConnectionId, metadata.HostConnectionId); err != nil {
+		return err
 	}
 
-	if version != icatypes.VersionPrefix {
-		return sdkerrors.Wrapf(icatypes.ErrInvalidVersion, "expected %s, got %s", icatypes.VersionPrefix, version)
+	if metadata.Version != icatypes.Version {
+		return sdkerrors.Wrapf(icatypes.ErrInvalidVersion, "expected %s, got %s", icatypes.Version, metadata.Version)
 	}
 
 	activeChannelID, found := k.GetActiveChannelID(ctx, portID)
@@ -71,21 +72,28 @@ func (k Keeper) OnChanOpenAck(
 	counterpartyVersion string,
 ) error {
 	if portID == icatypes.PortID {
-		return sdkerrors.Wrapf(porttypes.ErrInvalidPort, "portID cannot be host chain port ID: %s", icatypes.PortID)
+		return sdkerrors.Wrapf(icatypes.ErrInvalidControllerPort, "portID cannot be host chain port ID: %s", icatypes.PortID)
 	}
 
-	if err := icatypes.ValidateVersion(counterpartyVersion); err != nil {
-		return sdkerrors.Wrap(err, "counterparty version validation failed")
+	if !strings.HasPrefix(portID, icatypes.PortPrefix) {
+		return sdkerrors.Wrapf(icatypes.ErrInvalidControllerPort, "expected %s{owner-account-address}, got %s", icatypes.PortPrefix, portID)
 	}
 
-	k.SetActiveChannelID(ctx, portID, channelID)
+	var metadata icatypes.Metadata
+	if err := icatypes.ModuleCdc.UnmarshalJSON([]byte(counterpartyVersion), &metadata); err != nil {
+		return sdkerrors.Wrapf(icatypes.ErrUnknownDataType, "cannot unmarshal ICS-27 interchain accounts metadata")
+	}
 
-	accAddr, err := icatypes.ParseAddressFromVersion(counterpartyVersion)
-	if err != nil {
-		return sdkerrors.Wrapf(err, "expected format <app-version%saccount-address>, got %s", icatypes.Delimiter, counterpartyVersion)
+	if err := icatypes.ValidateAccountAddress(metadata.Address); err != nil {
+		return err
 	}
 
-	k.SetInterchainAccountAddress(ctx, portID, accAddr)
+	if metadata.Version != icatypes.Version {
+		return sdkerrors.Wrapf(icatypes.ErrInvalidVersion, "expected %s, got %s", icatypes.Version, metadata.Version)
+	}
+
+	k.SetActiveChannelID(ctx, portID, channelID)
+	k.SetInterchainAccountAddress(ctx, portID, metadata.Address)
 
 	return nil
 }
@@ -102,31 +110,20 @@ func (k Keeper) OnChanCloseConfirm(
 	return nil
 }
 
-// validateControllerPortParams asserts the provided connection sequence and counterparty connection sequence
-// match that of the associated connection stored in state
-func (k Keeper) validateControllerPortParams(ctx sdk.Context, connectionHops []string, connectionSeq, counterpartyConnectionSeq uint64) error {
+// validateConnectionParams asserts the provided controller and host connection identifiers match that of the associated connection stored in state
+func (k Keeper) validateConnectionParams(ctx sdk.Context, connectionHops []string, controllerConnectionID, hostConnectionID string) error {
 	connectionID := connectionHops[0]
 	connection, err := k.channelKeeper.GetConnection(ctx, connectionID)
 	if err != nil {
 		return err
 	}
 
-	connSeq, err := connectiontypes.ParseConnectionSequence(connectionID)
-	if err != nil {
-		return sdkerrors.Wrapf(err, "failed to parse connection sequence %s", connectionID)
-	}
-
-	counterpartyConnSeq, err := connectiontypes.ParseConnectionSequence(connection.GetCounterparty().GetConnectionID())
-	if err != nil {
-		return sdkerrors.Wrapf(err, "failed to parse counterparty connection sequence %s", connection.GetCounterparty().GetConnectionID())
-	}
-
-	if connSeq != connectionSeq {
-		return sdkerrors.Wrapf(connectiontypes.ErrInvalidConnection, "sequence mismatch, expected %d, got %d", connSeq, connectionSeq)
+	if controllerConnectionID != connectionID {
+		return sdkerrors.Wrapf(connectiontypes.ErrInvalidConnection, "expected %s, got %s", connectionID, controllerConnectionID)
 	}
 
-	if counterpartyConnSeq != counterpartyConnectionSeq {
-		return sdkerrors.Wrapf(connectiontypes.ErrInvalidConnection, "counterparty sequence mismatch, expected %d, got %d", counterpartyConnSeq, counterpartyConnectionSeq)
+	if hostConnectionID != connection.GetCounterparty().GetConnectionID() {
+		return sdkerrors.Wrapf(connectiontypes.ErrInvalidConnection, "expected %s, got %s", connection.GetCounterparty().GetConnectionID(), hostConnectionID)
 	}
 
 	return nil