From 9bff1e3cf80c9ad31d3fb71b00203db52052a647 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Mon, 26 Jul 2021 17:15:56 +0200 Subject: [PATCH 1/3] update OnChanOpenTry to match ICS specs --- .../27-interchain-accounts/keeper/account.go | 6 +- .../keeper/handshake.go | 36 +++++--- .../keeper/handshake_test.go | 89 +++++++++++++++++++ .../27-interchain-accounts/keeper/keeper.go | 5 ++ .../keeper/keeper_test.go | 16 ++++ .../27-interchain-accounts/types/errors.go | 19 ++-- 6 files changed, 147 insertions(+), 24 deletions(-) diff --git a/modules/apps/27-interchain-accounts/keeper/account.go b/modules/apps/27-interchain-accounts/keeper/account.go index 167e3d9e26f..3a3ec1befbd 100644 --- a/modules/apps/27-interchain-accounts/keeper/account.go +++ b/modules/apps/27-interchain-accounts/keeper/account.go @@ -41,12 +41,12 @@ func (k Keeper) InitInterchainAccount(ctx sdk.Context, connectionId, owner strin } // Register interchain account if it has not already been created -func (k Keeper) RegisterInterchainAccount(ctx sdk.Context, portId string) (types.IBCAccountI, error) { +func (k Keeper) RegisterInterchainAccount(ctx sdk.Context, portId string) error { address := k.GenerateAddress(portId) account := k.accountKeeper.GetAccount(ctx, address) if account != nil { - return nil, sdkerrors.Wrap(types.ErrAccountAlreadyExist, account.String()) + return sdkerrors.Wrap(types.ErrAccountAlreadyExist, account.String()) } interchainAccount := types.NewIBCAccount( @@ -57,7 +57,7 @@ func (k Keeper) RegisterInterchainAccount(ctx sdk.Context, portId string) (types k.accountKeeper.SetAccount(ctx, interchainAccount) _ = k.SetInterchainAccountAddress(ctx, portId, interchainAccount.Address) - return interchainAccount, nil + return nil } func (k Keeper) SetInterchainAccountAddress(ctx sdk.Context, portId string, address string) string { diff --git a/modules/apps/27-interchain-accounts/keeper/handshake.go b/modules/apps/27-interchain-accounts/keeper/handshake.go index 6a8bafe57cb..712513c2c5b 100644 --- a/modules/apps/27-interchain-accounts/keeper/handshake.go +++ b/modules/apps/27-interchain-accounts/keeper/handshake.go @@ -18,6 +18,8 @@ import ( // there must not be an active channel for the specfied port identifier, // and the interchain accounts module must be able to claim the channel // capability. +// +// Controller Chain func (k Keeper) OnChanOpenInit( ctx sdk.Context, order channeltypes.Order, @@ -50,9 +52,10 @@ func (k Keeper) OnChanOpenInit( return nil } -// register account (if it doesn't exist) -// check if counterpary version is the same -// TODO: remove ics27-1 hardcoded +// OnChanOpenTry performs basic validation of the ICA channel +// and registers a new interchain account (if it doesn't exist). +// +// Host Chain func (k Keeper) OnChanOpenTry( ctx sdk.Context, order channeltypes.Order, @@ -67,19 +70,28 @@ 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 counterpartyVersion != types.Version { + return sdkerrors.Wrapf(types.ErrInvalidVersion, "invalid counterparty version: %s, expected %s", counterpartyVersion, types.Version) + } - // TODO: Check counterparty version - // if counterpartyVersion != types.Version { - // return sdkerrors.Wrapf(porttypes.ErrInvalidPort, "invalid counterparty version: %s, expected %s", counterpartyVersion, "ics20-1") - // } - - // Claim channel capability passed back by IBC module - if err := k.ClaimCapability(ctx, chanCap, host.ChannelCapabilityPath(portID, channelID)); err != nil { - return sdkerrors.Wrap(channeltypes.ErrChannelCapabilityNotFound, err.Error()) + // Module may have already claimed capability in OnChanOpenInit in the case of crossing hellos + // (ie chainA and chainB both call ChanOpenInit before one of them calls ChanOpenTry) + // If module can already authenticate the capability then module already owns it so we don't need to claim + // Otherwise, module does not have channel capability and we must claim it from IBC + if !k.AuthenticateCapability(ctx, chanCap, host.ChannelCapabilityPath(portID, channelID)) { + // Only claim channel capability passed back by IBC module if we do not already own it + if err := k.ClaimCapability(ctx, chanCap, host.ChannelCapabilityPath(portID, channelID)); err != nil { + return err + } } // Register interchain account if it does not already exist - _, _ = k.RegisterInterchainAccount(ctx, counterparty.PortId) + if err := k.RegisterInterchainAccount(ctx, counterparty.PortId); err != nil { + return err + } return nil } diff --git a/modules/apps/27-interchain-accounts/keeper/handshake_test.go b/modules/apps/27-interchain-accounts/keeper/handshake_test.go index 7ac2b6ac0d0..667f2d10579 100644 --- a/modules/apps/27-interchain-accounts/keeper/handshake_test.go +++ b/modules/apps/27-interchain-accounts/keeper/handshake_test.go @@ -96,3 +96,92 @@ func (suite *KeeperTestSuite) TestOnChanOpenInit() { }) } } + +// ChainA is controller, ChainB is host chain +func (suite *KeeperTestSuite) TestOnChanOpenTry() { + var ( + channel *channeltypes.Channel + path *ibctesting.Path + chanCap *capabilitytypes.Capability + err error + ) + + testCases := []struct { + name string + malleate func() + expPass bool + }{ + + { + "success", func() {}, true, + }, + { + "invalid order - UNORDERED", func() { + channel.Ordering = channeltypes.UNORDERED + }, false, + }, + { + "invalid counterparty port ID", func() { + channel.Counterparty.PortId = ibctesting.MockPort + }, false, + }, + { + "invalid version", func() { + channel.Version = "version" + }, false, + }, + { + "channel is already active", func() { + suite.chainA.GetSimApp().ICAKeeper.SetActiveChannel(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) + }, false, + }, + { + "capability already claimed", func() { + err := suite.chainA.GetSimApp().ScopedICAKeeper.ClaimCapability(suite.chainA.GetContext(), chanCap, host.ChannelCapabilityPath(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)) + suite.Require().NoError(err) + }, false, + }, + } + + for _, tc := range testCases { + tc := tc + + suite.Run(tc.name, func() { + suite.SetupTest() // reset + path = NewICAPath(suite.chainA, suite.chainB) + owner := "owner" + counterpartyVersion := types.Version + suite.coordinator.SetupConnections(path) + + err := InitInterchainAccount(path.EndpointA, owner) + suite.Require().NoError(err) + + // default values + counterparty := channeltypes.NewCounterparty(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) + channel = &channeltypes.Channel{ + State: channeltypes.TRYOPEN, + Ordering: channeltypes.ORDERED, + Counterparty: counterparty, + ConnectionHops: []string{path.EndpointA.ConnectionID}, + Version: types.Version, + } + + chanCap, err = suite.chainA.App.GetScopedIBCKeeper().NewCapability(suite.chainA.GetContext(), host.ChannelCapabilityPath(portID, path.EndpointA.ChannelID)) + suite.Require().NoError(err) + + tc.malleate() // explicitly change fields in channel and testChannel + + err = suite.chainB.GetSimApp().ICAKeeper.OnChanOpenTry(suite.chainB.GetContext(), channel.Ordering, channel.GetConnectionHops(), + path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, chanCap, channel.Counterparty, channel.GetVersion(), + counterpartyVersion, + ) + + if tc.expPass { + suite.Require().NoError(err) + } else { + suite.Require().Error(err) + } + + }) + } +} diff --git a/modules/apps/27-interchain-accounts/keeper/keeper.go b/modules/apps/27-interchain-accounts/keeper/keeper.go index e8459b7cf85..781a144f6a0 100644 --- a/modules/apps/27-interchain-accounts/keeper/keeper.go +++ b/modules/apps/27-interchain-accounts/keeper/keeper.go @@ -161,3 +161,8 @@ func (k Keeper) IsActiveChannel(ctx sdk.Context, portId string) bool { _, found := k.GetActiveChannel(ctx, portId) return found } + +// AuthenticateCapability wraps the scopedKeeper's AuthenticateCapability function +func (k Keeper) AuthenticateCapability(ctx sdk.Context, cap *capabilitytypes.Capability, name string) bool { + return k.scopedKeeper.AuthenticateCapability(ctx, cap, name) +} diff --git a/modules/apps/27-interchain-accounts/keeper/keeper_test.go b/modules/apps/27-interchain-accounts/keeper/keeper_test.go index 0e53d0d9b5f..e9f693a79e9 100644 --- a/modules/apps/27-interchain-accounts/keeper/keeper_test.go +++ b/modules/apps/27-interchain-accounts/keeper/keeper_test.go @@ -6,6 +6,7 @@ import ( "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" ) @@ -35,6 +36,21 @@ func NewICAPath(chainA, chainB *ibctesting.TestChain) *ibctesting.Path { return path } +// InitInterchainAccount is a helper function for starting the channel handshake +func InitInterchainAccount(endpoint *ibctesting.Endpoint, owner string) error { + portID := endpoint.Chain.GetSimApp().ICAKeeper.GeneratePortId(owner, endpoint.ConnectionID) + channelSequence := endpoint.Chain.App.GetIBCKeeper().ChannelKeeper.GetNextChannelSequence(endpoint.Chain.GetContext()) + + if err := endpoint.Chain.GetSimApp().ICAKeeper.InitInterchainAccount(endpoint.Chain.GetContext(), endpoint.ConnectionID, owner); err != nil { + return err + } + + // update port/channel ids + endpoint.ChannelID = channeltypes.FormatChannelIdentifier(channelSequence) + endpoint.ChannelConfig.PortID = portID + return nil +} + func TestKeeperTestSuite(t *testing.T) { suite.Run(t, new(KeeperTestSuite)) } diff --git a/modules/apps/27-interchain-accounts/types/errors.go b/modules/apps/27-interchain-accounts/types/errors.go index 8854c1df4e7..6a1906ad25f 100644 --- a/modules/apps/27-interchain-accounts/types/errors.go +++ b/modules/apps/27-interchain-accounts/types/errors.go @@ -5,13 +5,14 @@ import ( ) var ( - ErrUnknownPacketData = sdkerrors.Register(ModuleName, 1, "Unknown packet data") - ErrAccountAlreadyExist = sdkerrors.Register(ModuleName, 2, "Account already exist") - ErrPortAlreadyBound = sdkerrors.Register(ModuleName, 3, "Port is already bound for address") - ErrUnsupportedChain = sdkerrors.Register(ModuleName, 4, "Unsupported chain") - ErrInvalidOutgoingData = sdkerrors.Register(ModuleName, 5, "Invalid outgoing data") - ErrInvalidRoute = sdkerrors.Register(ModuleName, 6, "Invalid route") - ErrIBCAccountNotFound = sdkerrors.Register(ModuleName, 7, "Ibc account not found") - ErrIBCAccountAlreadySet = sdkerrors.Register(ModuleName, 8, "Interchain Account is already set") - ErrActiveChannelNotFound = sdkerrors.Register(ModuleName, 9, "No active channel for this owner") + ErrUnknownPacketData = sdkerrors.Register(ModuleName, 2, "unknown packet data") + ErrAccountAlreadyExist = sdkerrors.Register(ModuleName, 3, "account already exist") + ErrPortAlreadyBound = sdkerrors.Register(ModuleName, 4, "port is already bound for address") + ErrUnsupportedChain = sdkerrors.Register(ModuleName, 5, "unsupported chain") + ErrInvalidOutgoingData = sdkerrors.Register(ModuleName, 6, "invalid outgoing data") + ErrInvalidRoute = sdkerrors.Register(ModuleName, 7, "invalid route") + ErrIBCAccountNotFound = sdkerrors.Register(ModuleName, 8, "ibc account not found") + ErrIBCAccountAlreadySet = sdkerrors.Register(ModuleName, 9, "interchain Account is already set") + ErrActiveChannelNotFound = sdkerrors.Register(ModuleName, 10, "no active channel for this owner") + ErrInvalidVersion = sdkerrors.Register(ModuleName, 11, "invalid interchain accounts version") ) From fac762c350751da88d219f16d400c27f6e93274d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Thu, 5 Aug 2021 11:20:31 +0200 Subject: [PATCH 2/3] write OnChanOpenTry and OnChanOpenAck tests --- .../27-interchain-accounts/keeper/account.go | 7 +- .../keeper/handshake.go | 17 ++-- .../keeper/handshake_test.go | 77 +++++++++++++++---- .../keeper/keeper_test.go | 8 ++ 4 files changed, 78 insertions(+), 31 deletions(-) diff --git a/modules/apps/27-interchain-accounts/keeper/account.go b/modules/apps/27-interchain-accounts/keeper/account.go index 3a3ec1befbd..51ca24429fc 100644 --- a/modules/apps/27-interchain-accounts/keeper/account.go +++ b/modules/apps/27-interchain-accounts/keeper/account.go @@ -41,12 +41,13 @@ func (k Keeper) InitInterchainAccount(ctx sdk.Context, connectionId, owner strin } // Register interchain account if it has not already been created -func (k Keeper) RegisterInterchainAccount(ctx sdk.Context, portId string) error { +func (k Keeper) RegisterInterchainAccount(ctx sdk.Context, portId string) { address := k.GenerateAddress(portId) account := k.accountKeeper.GetAccount(ctx, address) if account != nil { - return sdkerrors.Wrap(types.ErrAccountAlreadyExist, account.String()) + // account already created, return no-op + return } interchainAccount := types.NewIBCAccount( @@ -57,7 +58,7 @@ func (k Keeper) RegisterInterchainAccount(ctx sdk.Context, portId string) error k.accountKeeper.SetAccount(ctx, interchainAccount) _ = k.SetInterchainAccountAddress(ctx, portId, interchainAccount.Address) - return nil + return } func (k Keeper) SetInterchainAccountAddress(ctx sdk.Context, portId string, address string) string { diff --git a/modules/apps/27-interchain-accounts/keeper/handshake.go b/modules/apps/27-interchain-accounts/keeper/handshake.go index 712513c2c5b..36ac0389cc2 100644 --- a/modules/apps/27-interchain-accounts/keeper/handshake.go +++ b/modules/apps/27-interchain-accounts/keeper/handshake.go @@ -77,21 +77,14 @@ func (k Keeper) OnChanOpenTry( return sdkerrors.Wrapf(types.ErrInvalidVersion, "invalid counterparty version: %s, expected %s", counterpartyVersion, types.Version) } - // Module may have already claimed capability in OnChanOpenInit in the case of crossing hellos - // (ie chainA and chainB both call ChanOpenInit before one of them calls ChanOpenTry) - // If module can already authenticate the capability then module already owns it so we don't need to claim - // Otherwise, module does not have channel capability and we must claim it from IBC - if !k.AuthenticateCapability(ctx, chanCap, host.ChannelCapabilityPath(portID, channelID)) { - // Only claim channel capability passed back by IBC module if we do not already own it - if err := k.ClaimCapability(ctx, chanCap, host.ChannelCapabilityPath(portID, channelID)); err != nil { - return err - } + // TODO: update explanation + // Only claim channel capability passed back by IBC module if we do not already own it + if err := k.ClaimCapability(ctx, chanCap, host.ChannelCapabilityPath(portID, channelID)); err != nil { + return err } // Register interchain account if it does not already exist - if err := k.RegisterInterchainAccount(ctx, counterparty.PortId); err != nil { - return err - } + k.RegisterInterchainAccount(ctx, counterparty.PortId) return nil } diff --git a/modules/apps/27-interchain-accounts/keeper/handshake_test.go b/modules/apps/27-interchain-accounts/keeper/handshake_test.go index 667f2d10579..ef689f12f2c 100644 --- a/modules/apps/27-interchain-accounts/keeper/handshake_test.go +++ b/modules/apps/27-interchain-accounts/keeper/handshake_test.go @@ -100,10 +100,10 @@ func (suite *KeeperTestSuite) TestOnChanOpenInit() { // ChainA is controller, ChainB is host chain func (suite *KeeperTestSuite) TestOnChanOpenTry() { var ( - channel *channeltypes.Channel - path *ibctesting.Path - chanCap *capabilitytypes.Capability - err error + channel *channeltypes.Channel + path *ibctesting.Path + chanCap *capabilitytypes.Capability + counterpartyVersion string ) testCases := []struct { @@ -120,24 +120,19 @@ func (suite *KeeperTestSuite) TestOnChanOpenTry() { channel.Ordering = channeltypes.UNORDERED }, false, }, - { - "invalid counterparty port ID", func() { - channel.Counterparty.PortId = ibctesting.MockPort - }, false, - }, { "invalid version", func() { channel.Version = "version" }, false, }, { - "channel is already active", func() { - suite.chainA.GetSimApp().ICAKeeper.SetActiveChannel(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) + "invalid counterparty version", func() { + counterpartyVersion = "version" }, false, }, { "capability already claimed", func() { - err := suite.chainA.GetSimApp().ScopedICAKeeper.ClaimCapability(suite.chainA.GetContext(), chanCap, host.ChannelCapabilityPath(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)) + err := suite.chainB.GetSimApp().ScopedICAKeeper.ClaimCapability(suite.chainB.GetContext(), chanCap, host.ChannelCapabilityPath(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)) suite.Require().NoError(err) }, false, }, @@ -150,23 +145,23 @@ func (suite *KeeperTestSuite) TestOnChanOpenTry() { suite.SetupTest() // reset path = NewICAPath(suite.chainA, suite.chainB) owner := "owner" - counterpartyVersion := types.Version + counterpartyVersion = types.Version suite.coordinator.SetupConnections(path) err := InitInterchainAccount(path.EndpointA, owner) suite.Require().NoError(err) // default values - counterparty := channeltypes.NewCounterparty(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) + counterparty := channeltypes.NewCounterparty(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) channel = &channeltypes.Channel{ State: channeltypes.TRYOPEN, Ordering: channeltypes.ORDERED, Counterparty: counterparty, - ConnectionHops: []string{path.EndpointA.ConnectionID}, + ConnectionHops: []string{path.EndpointB.ConnectionID}, Version: types.Version, } - chanCap, err = suite.chainA.App.GetScopedIBCKeeper().NewCapability(suite.chainA.GetContext(), host.ChannelCapabilityPath(portID, path.EndpointA.ChannelID)) + chanCap, err = suite.chainB.App.GetScopedIBCKeeper().NewCapability(suite.chainB.GetContext(), host.ChannelCapabilityPath(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)) suite.Require().NoError(err) tc.malleate() // explicitly change fields in channel and testChannel @@ -185,3 +180,53 @@ func (suite *KeeperTestSuite) TestOnChanOpenTry() { }) } } + +// ChainA is controller, ChainB is host chain +func (suite *KeeperTestSuite) TestOnChanOpenAck() { + var ( + path *ibctesting.Path + counterpartyVersion string + ) + + testCases := []struct { + name string + malleate func() + expPass bool + }{ + + { + "success", func() {}, true, + }, + } + + for _, tc := range testCases { + tc := tc + + suite.Run(tc.name, func() { + suite.SetupTest() // reset + path = NewICAPath(suite.chainA, suite.chainB) + owner := "owner" + counterpartyVersion = types.Version + suite.coordinator.SetupConnections(path) + + err := InitInterchainAccount(path.EndpointA, owner) + suite.Require().NoError(err) + + err = path.EndpointB.ChanOpenTry() + suite.Require().NoError(err) + + tc.malleate() // explicitly change fields in channel and testChannel + + err = suite.chainA.GetSimApp().ICAKeeper.OnChanOpenAck(suite.chainA.GetContext(), + path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, counterpartyVersion, + ) + + if tc.expPass { + suite.Require().NoError(err) + } else { + suite.Require().Error(err) + } + + }) + } +} diff --git a/modules/apps/27-interchain-accounts/keeper/keeper_test.go b/modules/apps/27-interchain-accounts/keeper/keeper_test.go index e9f693a79e9..a0819dcdf09 100644 --- a/modules/apps/27-interchain-accounts/keeper/keeper_test.go +++ b/modules/apps/27-interchain-accounts/keeper/keeper_test.go @@ -32,6 +32,10 @@ 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.Version + path.EndpointB.ChannelConfig.Version = types.Version return path } @@ -45,6 +49,10 @@ func InitInterchainAccount(endpoint *ibctesting.Endpoint, owner string) error { return err } + // commit state changes for proof verification + endpoint.Chain.App.Commit() + endpoint.Chain.NextBlock() + // update port/channel ids endpoint.ChannelID = channeltypes.FormatChannelIdentifier(channelSequence) endpoint.ChannelConfig.PortID = portID From 05bd1075a7170fec77db0caef1c235548377b2c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Thu, 5 Aug 2021 11:24:05 +0200 Subject: [PATCH 3/3] update comment --- modules/apps/27-interchain-accounts/keeper/handshake.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/modules/apps/27-interchain-accounts/keeper/handshake.go b/modules/apps/27-interchain-accounts/keeper/handshake.go index 36ac0389cc2..43a95088f37 100644 --- a/modules/apps/27-interchain-accounts/keeper/handshake.go +++ b/modules/apps/27-interchain-accounts/keeper/handshake.go @@ -77,8 +77,8 @@ func (k Keeper) OnChanOpenTry( return sdkerrors.Wrapf(types.ErrInvalidVersion, "invalid counterparty version: %s, expected %s", counterpartyVersion, types.Version) } - // TODO: update explanation - // Only claim channel capability passed back by IBC module if we do not already own it + // On the host chain the capability may only be claimed during the OnChanOpenTry + // The capability being claimed in OpenInit is for a controller chain (the port is different) if err := k.ClaimCapability(ctx, chanCap, host.ChannelCapabilityPath(portID, channelID)); err != nil { return err }