feat: ica app version negotiation (#410)

* adding NegotiateAppVersion implementation and tests * updating GenerateAddress to return sdk.AccAddress, fixing tests * updating ica handshake flow to parse address from version string, fixing associated tests * updating module_tests * derive ica addresses from module account addr * removing unused keys * adding improved version validation, updating tests * removing redundant local var - owner * updating Delimiter godoc * updating validation logic * adding test cases for ValidateVersion * adding additional validation testcase, updating godocs * updating Version -> VersionPrefix, error msgs, validation tests * updating NewAppVersion func sig and usage * updating NewAppVersion args and returning more appropriate errors for handshake * Update modules/apps/27-interchain-accounts/keeper/handshake.go Co-authored-by: Sean King <[email protected]> * updating ValidateVersion godoc Co-authored-by: Sean King <[email protected]>
cosmos · Sep 20, 2021 · e7cf4d3 · e7cf4d3
1 parent 3517def
commit e7cf4d3
Show file tree

Hide file tree

Showing 14 changed files with 364 additions and 99 deletions.
diff --git a/modules/apps/27-interchain-accounts/keeper/account.go b/modules/apps/27-interchain-accounts/keeper/account.go
@@ -31,7 +31,7 @@ func (k Keeper) InitInterchainAccount(ctx sdk.Context, connectionID, counterpart
 		return sdkerrors.Wrap(err, "unable to bind to newly generated portID")
 	}
 
-	msg := channeltypes.NewMsgChannelOpenInit(portID, types.Version, channeltypes.ORDERED, []string{connectionID}, types.PortID, types.ModuleName)
+	msg := channeltypes.NewMsgChannelOpenInit(portID, types.VersionPrefix, channeltypes.ORDERED, []string{connectionID}, types.PortID, types.ModuleName)
 	handler := k.msgRouter.Handler(msg)
 	if _, err := handler(ctx, msg); err != nil {
 		return err
@@ -40,17 +40,15 @@ func (k Keeper) InitInterchainAccount(ctx sdk.Context, connectionID, counterpart
 	return nil
 }
 
-// Register interchain account if it has not already been created
-func (k Keeper) RegisterInterchainAccount(ctx sdk.Context, portID string) {
-	address := types.GenerateAddress(portID)
-
-	account := k.accountKeeper.GetAccount(ctx, address)
-	if account != nil {
+// RegisterInterchainAccount attempts to create a new account using the provided address and stores it in state keyed by the provided port identifier
+// If an account for the provided address already exists this function returns early (no-op)
+func (k Keeper) RegisterInterchainAccount(ctx sdk.Context, accAddr sdk.AccAddress, portID string) {
+	if acc := k.accountKeeper.GetAccount(ctx, accAddr); acc != nil {
 		return
 	}
 
 	interchainAccount := types.NewInterchainAccount(
-		authtypes.NewBaseAccountWithAddress(address),
+		authtypes.NewBaseAccountWithAddress(accAddr),
 		portID,
 	)
 

diff --git a/modules/apps/27-interchain-accounts/keeper/account_test.go b/modules/apps/27-interchain-accounts/keeper/account_test.go
@@ -44,8 +44,8 @@ func (suite *KeeperTestSuite) TestInitInterchainAccount() {
 		tc := tc
 
 		suite.Run(tc.name, func() {
-			suite.SetupTest() // reset
-			owner = "owner"   // must be explicitly changed
+			suite.SetupTest()        // reset
+			owner = TestOwnerAddress // must be explicitly changed
 			path = NewICAPath(suite.chainA, suite.chainB)
 			suite.coordinator.SetupConnections(path)
 

diff --git a/modules/apps/27-interchain-accounts/keeper/handshake.go b/modules/apps/27-interchain-accounts/keeper/handshake.go
@@ -36,8 +36,9 @@ func (k Keeper) OnChanOpenInit(
 	if counterparty.PortId != types.PortID {
 		return sdkerrors.Wrapf(porttypes.ErrInvalidPort, "counterparty port-id must be '%s', (%s != %s)", types.PortID, counterparty.PortId, types.PortID)
 	}
-	if version != types.Version {
-		return sdkerrors.Wrapf(channeltypes.ErrInvalidChannelVersion, "channel version must be '%s' (%s != %s)", types.Version, version, types.Version)
+
+	if err := types.ValidateVersion(version); err != nil {
+		return sdkerrors.Wrap(err, "version validation failed")
 	}
 
 	existingChannelID, found := k.GetActiveChannel(ctx, portID)
@@ -71,11 +72,13 @@ func (k Keeper) OnChanOpenTry(
 	if order != channeltypes.ORDERED {
 		return sdkerrors.Wrapf(channeltypes.ErrInvalidChannelOrdering, "invalid channel ordering: %s, expected %s", order.String(), channeltypes.ORDERED.String())
 	}
-	if version != types.Version {
-		return sdkerrors.Wrapf(types.ErrInvalidVersion, "got: %s, expected %s", version, types.Version)
+
+	if err := types.ValidateVersion(version); err != nil {
+		return sdkerrors.Wrap(err, "version validation failed")
 	}
-	if counterpartyVersion != types.Version {
-		return sdkerrors.Wrapf(types.ErrInvalidVersion, "invalid counterparty version: %s, expected %s", counterpartyVersion, types.Version)
+
+	if err := types.ValidateVersion(counterpartyVersion); err != nil {
+		return sdkerrors.Wrap(err, "counterparty version validation failed")
 	}
 
 	// On the host chain the capability may only be claimed during the OnChanOpenTry
@@ -84,23 +87,38 @@ func (k Keeper) OnChanOpenTry(
 		return err
 	}
 
+        // Check to ensure that the version string contains the expected address generated from the Counterparty portID 
+	accAddr := types.GenerateAddress(counterparty.PortId)
+	parsedAddr := types.ParseAddressFromVersion(version)
+	if parsedAddr != accAddr.String() {
+		return sdkerrors.Wrapf(types.ErrInvalidAccountAddress, "version contains invalid account address: expected %s, got %s", parsedAddr, accAddr)
+	}
+
 	// Register interchain account if it does not already exist
-	k.RegisterInterchainAccount(ctx, counterparty.PortId)
+	k.RegisterInterchainAccount(ctx, accAddr, counterparty.PortId)
+
 	return nil
 }
 
+// OnChanOpenAck sets the active channel for the interchain account/owner pair
+// and stores the associated interchain account address in state keyed by it's corresponding port identifier
+//
+// Controller Chain
 func (k Keeper) OnChanOpenAck(
 	ctx sdk.Context,
 	portID,
 	channelID string,
 	counterpartyVersion string,
 ) error {
-	if counterpartyVersion != types.Version {
-		return sdkerrors.Wrapf(types.ErrInvalidVersion, "invalid counterparty version: %s, expected %s", counterpartyVersion, types.Version)
+	if err := types.ValidateVersion(counterpartyVersion); err != nil {
+		return sdkerrors.Wrap(err, "counterparty version validation failed")
 	}
 
 	k.SetActiveChannel(ctx, portID, channelID)
 
+	accAddr := types.ParseAddressFromVersion(counterpartyVersion)
+	k.SetInterchainAccountAddress(ctx, portID, accAddr)
+
 	return nil
 }
 

diff --git a/modules/apps/27-interchain-accounts/keeper/handshake_test.go b/modules/apps/27-interchain-accounts/keeper/handshake_test.go
@@ -62,7 +62,7 @@ func (suite *KeeperTestSuite) TestOnChanOpenInit() {
 			suite.coordinator.SetupConnections(path)
 
 			// mock init interchain account
-			portID, err := types.GeneratePortID("owner", path.EndpointA.ConnectionID, path.EndpointB.ConnectionID)
+			portID, err := types.GeneratePortID(TestOwnerAddress, path.EndpointA.ConnectionID, path.EndpointB.ConnectionID)
 			suite.Require().NoError(err)
 			portCap := suite.chainA.GetSimApp().IBCKeeper.PortKeeper.BindPort(suite.chainA.GetContext(), portID)
 			suite.chainA.GetSimApp().ICAKeeper.ClaimCapability(suite.chainA.GetContext(), portCap, host.PortPath(portID))
@@ -75,7 +75,7 @@ func (suite *KeeperTestSuite) TestOnChanOpenInit() {
 				Ordering:       channeltypes.ORDERED,
 				Counterparty:   counterparty,
 				ConnectionHops: []string{path.EndpointA.ConnectionID},
-				Version:        types.Version,
+				Version:        types.VersionPrefix,
 			}
 
 			chanCap, err = suite.chainA.App.GetScopedIBCKeeper().NewCapability(suite.chainA.GetContext(), host.ChannelCapabilityPath(portID, path.EndpointA.ChannelID))
@@ -136,6 +136,11 @@ func (suite *KeeperTestSuite) TestOnChanOpenTry() {
 				suite.Require().NoError(err)
 			}, false,
 		},
+		{
+			"invalid account address", func() {
+				channel.Counterparty.PortId = "invalid-port-id"
+			}, false,
+		},
 	}
 
 	for _, tc := range testCases {
@@ -144,11 +149,10 @@ func (suite *KeeperTestSuite) TestOnChanOpenTry() {
 		suite.Run(tc.name, func() {
 			suite.SetupTest() // reset
 			path = NewICAPath(suite.chainA, suite.chainB)
-			owner := "owner"
-			counterpartyVersion = types.Version
+			counterpartyVersion = types.VersionPrefix
 			suite.coordinator.SetupConnections(path)
 
-			err := InitInterchainAccount(path.EndpointA, owner)
+			err := InitInterchainAccount(path.EndpointA, TestOwnerAddress)
 			suite.Require().NoError(err)
 
 			// default values
@@ -158,7 +162,7 @@ func (suite *KeeperTestSuite) TestOnChanOpenTry() {
 				Ordering:       channeltypes.ORDERED,
 				Counterparty:   counterparty,
 				ConnectionHops: []string{path.EndpointB.ConnectionID},
-				Version:        types.Version,
+				Version:        TestVersion,
 			}
 
 			chanCap, err = suite.chainB.App.GetScopedIBCKeeper().NewCapability(suite.chainB.GetContext(), host.ChannelCapabilityPath(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID))
@@ -211,11 +215,10 @@ func (suite *KeeperTestSuite) TestOnChanOpenAck() {
 		suite.Run(tc.name, func() {
 			suite.SetupTest() // reset
 			path = NewICAPath(suite.chainA, suite.chainB)
-			owner := "owner"
-			counterpartyVersion = types.Version
+			counterpartyVersion = TestVersion
 			suite.coordinator.SetupConnections(path)
 
-			err := InitInterchainAccount(path.EndpointA, owner)
+			err := InitInterchainAccount(path.EndpointA, TestOwnerAddress)
 			suite.Require().NoError(err)
 
 			err = path.EndpointB.ChanOpenTry()
@@ -264,10 +267,9 @@ func (suite *KeeperTestSuite) TestOnChanOpenConfirm() {
 		suite.Run(tc.name, func() {
 			suite.SetupTest() // reset
 			path = NewICAPath(suite.chainA, suite.chainB)
-			owner := "owner"
 			suite.coordinator.SetupConnections(path)
 
-			err := InitInterchainAccount(path.EndpointA, owner)
+			err := InitInterchainAccount(path.EndpointA, TestOwnerAddress)
 			suite.Require().NoError(err)
 
 			err = path.EndpointB.ChanOpenTry()

diff --git a/modules/apps/27-interchain-accounts/keeper/keeper_test.go b/modules/apps/27-interchain-accounts/keeper/keeper_test.go
@@ -1,6 +1,7 @@
 package keeper_test
 
 import (
+	"fmt"
 	"testing"
 
 	authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
@@ -11,6 +12,15 @@ import (
 	ibctesting "github.com/cosmos/ibc-go/testing"
 )
 
+var (
+	// TestOwnerAddress defines a reusable bech32 address for testing purposes
+	TestOwnerAddress = "cosmos17dtl0mjt3t77kpuhg2edqzjpszulwhgzuj9ljs"
+	// TestPortID defines a resuable port identifier for testing purposes
+	TestPortID = fmt.Sprintf("ics-27-0-0-%s", TestOwnerAddress)
+	// TestVersion defines a resuable interchainaccounts version string for testing purposes
+	TestVersion = types.NewAppVersion(types.VersionPrefix, types.GenerateAddress(TestPortID).String())
+)
+
 type KeeperTestSuite struct {
 	suite.Suite
 
@@ -35,8 +45,8 @@ func NewICAPath(chainA, chainB *ibctesting.TestChain) *ibctesting.Path {
 	path.EndpointB.ChannelConfig.PortID = types.PortID
 	path.EndpointA.ChannelConfig.Order = channeltypes.ORDERED
 	path.EndpointB.ChannelConfig.Order = channeltypes.ORDERED
-	path.EndpointA.ChannelConfig.Version = types.Version
-	path.EndpointB.ChannelConfig.Version = types.Version
+	path.EndpointA.ChannelConfig.Version = types.VersionPrefix
+	path.EndpointB.ChannelConfig.Version = TestVersion
 
 	return path
 }
@@ -104,7 +114,7 @@ func (suite *KeeperTestSuite) TestGetInterchainAccountAddress() {
 	path := NewICAPath(suite.chainA, suite.chainB)
 	suite.coordinator.SetupConnections(path)
 
-	err := SetupICAPath(path, "testing")
+	err := SetupICAPath(path, TestOwnerAddress)
 	suite.Require().NoError(err)
 
 	counterpartyPortID := path.EndpointA.ChannelConfig.PortID

diff --git a/modules/apps/27-interchain-accounts/keeper/relay_test.go b/modules/apps/27-interchain-accounts/keeper/relay_test.go
@@ -74,7 +74,7 @@ func (suite *KeeperTestSuite) TestTrySendTx() {
 		suite.Run(tc.name, func() {
 			suite.SetupTest() // reset
 			path = NewICAPath(suite.chainA, suite.chainB)
-			owner := "owner"
+			owner := TestOwnerAddress
 			suite.coordinator.SetupConnections(path)
 
 			err := suite.SetupICAPath(path, owner)

diff --git a/modules/apps/27-interchain-accounts/module.go b/modules/apps/27-interchain-accounts/module.go
@@ -255,6 +255,18 @@ func (am AppModule) OnTimeoutPacket(
 	return nil
 }
 
-func (am AppModule) NegotiateAppVersion(ctx sdk.Context, order channeltypes.Order, connectionID, portID string, counterparty channeltypes.Counterparty, proposedVersion string) (string, error) {
-	return "", nil
+// NegotiateAppVersion implements the IBCModule interface
+func (am AppModule) NegotiateAppVersion(
+	ctx sdk.Context,
+	order channeltypes.Order,
+	connectionID string,
+	portID string,
+	counterparty channeltypes.Counterparty,
+	proposedVersion string,
+) (string, error) {
+	if proposedVersion != types.VersionPrefix {
+		return "", sdkerrors.Wrapf(types.ErrInvalidVersion, "failed to negotiate app version: expected %s, got %s", types.VersionPrefix, proposedVersion)
+	}
+
+	return types.NewAppVersion(types.VersionPrefix, types.GenerateAddress(counterparty.PortId).String()), nil
 }
diff --git a/modules/apps/27-interchain-accounts/module_test.go b/modules/apps/27-interchain-accounts/module_test.go
@@ -0,0 +1,80 @@
+package interchain_accounts_test
+
+import (
+	"fmt"
+	"testing"
+
+	"github.com/stretchr/testify/suite"
+
+	"github.com/cosmos/ibc-go/modules/apps/27-interchain-accounts/types"
+	channeltypes "github.com/cosmos/ibc-go/modules/core/04-channel/types"
+	ibctesting "github.com/cosmos/ibc-go/testing"
+)
+
+var (
+	// TestOwnerAddress defines a reusable bech32 address for testing purposes
+	TestOwnerAddress = "cosmos17dtl0mjt3t77kpuhg2edqzjpszulwhgzuj9ljs"
+	// TestPortID defines a resuable port identifier for testing purposes
+	TestPortID = fmt.Sprintf("ics-27-0-0-%s", TestOwnerAddress)
+	// TestVersion defines a resuable interchainaccounts version string for testing purposes
+	TestVersion = types.NewAppVersion(types.VersionPrefix, types.GenerateAddress(TestPortID).String())
+)
+
+type InterchainAccountsTestSuite struct {
+	suite.Suite
+
+	coordinator *ibctesting.Coordinator
+
+	// testing chains used for convenience and readability
+	chainA *ibctesting.TestChain
+	chainB *ibctesting.TestChain
+	chainC *ibctesting.TestChain
+}
+
+func (suite *InterchainAccountsTestSuite) SetupTest() {
+	suite.coordinator = ibctesting.NewCoordinator(suite.T(), 3)
+	suite.chainA = suite.coordinator.GetChain(ibctesting.GetChainID(0))
+	suite.chainB = suite.coordinator.GetChain(ibctesting.GetChainID(1))
+	suite.chainC = suite.coordinator.GetChain(ibctesting.GetChainID(2))
+}
+
+func NewICAPath(chainA, chainB *ibctesting.TestChain) *ibctesting.Path {
+	path := ibctesting.NewPath(chainA, chainB)
+	path.EndpointA.ChannelConfig.PortID = types.PortID
+	path.EndpointB.ChannelConfig.PortID = types.PortID
+	path.EndpointA.ChannelConfig.Order = channeltypes.ORDERED
+	path.EndpointB.ChannelConfig.Order = channeltypes.ORDERED
+	path.EndpointA.ChannelConfig.Version = types.VersionPrefix
+	path.EndpointB.ChannelConfig.Version = TestVersion
+
+	return path
+}
+
+func TestICATestSuite(t *testing.T) {
+	suite.Run(t, new(InterchainAccountsTestSuite))
+}
+
+func (suite *InterchainAccountsTestSuite) TestNegotiateAppVersion() {
+	suite.SetupTest()
+	path := NewICAPath(suite.chainA, suite.chainB)
+	suite.coordinator.SetupConnections(path)
+
+	module, _, err := suite.chainA.GetSimApp().GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), types.PortID)
+	suite.Require().NoError(err)
+
+	cbs, ok := suite.chainA.GetSimApp().GetIBCKeeper().Router.GetRoute(module)
+	suite.Require().True(ok)
+
+	counterpartyPortID, err := types.GeneratePortID(TestOwnerAddress, path.EndpointA.ConnectionID, path.EndpointB.ConnectionID)
+	suite.Require().NoError(err)
+
+	counterparty := &channeltypes.Counterparty{
+		PortId:    counterpartyPortID,
+		ChannelId: path.EndpointB.ChannelID,
+	}
+
+	version, err := cbs.NegotiateAppVersion(suite.chainA.GetContext(), channeltypes.ORDERED, path.EndpointA.ConnectionID, types.PortID, *counterparty, types.VersionPrefix)
+	suite.Require().NoError(err)
+	suite.Require().NoError(types.ValidateVersion(version))
+	suite.Require().Equal(TestVersion, version)
+}