From 2a160ed1edaf1a9aac3552b58068c678d8a85a47 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 12:11:23 +0200 Subject: [PATCH] ICA OnChanOpenTry update + tests (#299) * update OnChanOpenTry to match ICS specs * write OnChanOpenTry and OnChanOpenAck tests * update comment --- .../27-interchain-accounts/keeper/account.go | 7 +- .../keeper/handshake.go | 27 ++-- .../keeper/handshake_test.go | 134 ++++++++++++++++++ .../27-interchain-accounts/keeper/keeper.go | 5 + .../keeper/keeper_test.go | 24 ++++ .../27-interchain-accounts/types/errors.go | 19 +-- 6 files changed, 193 insertions(+), 23 deletions(-) diff --git a/modules/apps/27-interchain-accounts/keeper/account.go b/modules/apps/27-interchain-accounts/keeper/account.go index 6bad42a1133..99bc84ce9c9 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) (types.InterchainAccountI, error) { +func (k Keeper) RegisterInterchainAccount(ctx sdk.Context, portId string) { address := k.GenerateAddress(portId) account := k.accountKeeper.GetAccount(ctx, address) if account != nil { - return nil, sdkerrors.Wrap(types.ErrAccountAlreadyExist, account.String()) + // account already created, return no-op + return } interchainAccount := types.NewInterchainAccount( @@ -57,7 +58,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 } 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..43a95088f37 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,21 @@ 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 + // 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 sdkerrors.Wrap(channeltypes.ErrChannelCapabilityNotFound, err.Error()) + return err } // Register interchain account if it does not already exist - _, _ = k.RegisterInterchainAccount(ctx, counterparty.PortId) + 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 7ac2b6ac0d0..ef689f12f2c 100644 --- a/modules/apps/27-interchain-accounts/keeper/handshake_test.go +++ b/modules/apps/27-interchain-accounts/keeper/handshake_test.go @@ -96,3 +96,137 @@ 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 + counterpartyVersion string + ) + + testCases := []struct { + name string + malleate func() + expPass bool + }{ + + { + "success", func() {}, true, + }, + { + "invalid order - UNORDERED", func() { + channel.Ordering = channeltypes.UNORDERED + }, false, + }, + { + "invalid version", func() { + channel.Version = "version" + }, false, + }, + { + "invalid counterparty version", func() { + counterpartyVersion = "version" + }, false, + }, + { + "capability already claimed", func() { + err := suite.chainB.GetSimApp().ScopedICAKeeper.ClaimCapability(suite.chainB.GetContext(), chanCap, host.ChannelCapabilityPath(path.EndpointB.ChannelConfig.PortID, path.EndpointB.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.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) + channel = &channeltypes.Channel{ + State: channeltypes.TRYOPEN, + Ordering: channeltypes.ORDERED, + Counterparty: counterparty, + ConnectionHops: []string{path.EndpointB.ConnectionID}, + Version: types.Version, + } + + 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 + + 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) + } + + }) + } +} + +// 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.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..a0819dcdf09 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" ) @@ -31,10 +32,33 @@ 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 } +// 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 + } + + // 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 + 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 f05d8bc0a3d..40578c46cee 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") - ErrInterchainAccountNotFound = sdkerrors.Register(ModuleName, 7, "Interchain account not found") - ErrInterchainAccountAlreadySet = 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") + ErrInterchainAccountNotFound = sdkerrors.Register(ModuleName, 8, "Interchain Account not found") + ErrInterchainAccountAlreadySet = 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") )