From 7de03ee63e90e277cea6627edc0daf0632e8fcf7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Thu, 16 Sep 2021 16:15:49 +0200 Subject: [PATCH 01/17] initial draft for ica middleware --- .../keeper/handshake.go | 2 +- .../27-interchain-accounts/keeper/keeper.go | 5 +- .../27-interchain-accounts/keeper/relay.go | 34 ++------- modules/apps/27-interchain-accounts/module.go | 73 +++++++++++++------ .../apps/27-interchain-accounts/types/hook.go | 30 -------- 5 files changed, 58 insertions(+), 86 deletions(-) delete mode 100644 modules/apps/27-interchain-accounts/types/hook.go diff --git a/modules/apps/27-interchain-accounts/keeper/handshake.go b/modules/apps/27-interchain-accounts/keeper/handshake.go index e4677b72188..19505415954 100644 --- a/modules/apps/27-interchain-accounts/keeper/handshake.go +++ b/modules/apps/27-interchain-accounts/keeper/handshake.go @@ -47,7 +47,7 @@ func (k Keeper) OnChanOpenInit( // 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()) + return sdkerrors.Wrap(err, "interchain accounts moudle could not claim capability passed back by the IBC module") } return nil diff --git a/modules/apps/27-interchain-accounts/keeper/keeper.go b/modules/apps/27-interchain-accounts/keeper/keeper.go index d45fd9037aa..df5c2fc8523 100644 --- a/modules/apps/27-interchain-accounts/keeper/keeper.go +++ b/modules/apps/27-interchain-accounts/keeper/keeper.go @@ -20,8 +20,6 @@ type Keeper struct { storeKey sdk.StoreKey cdc codec.BinaryCodec - hook types.IBCAccountHooks - channelKeeper types.ChannelKeeper portKeeper types.PortKeeper accountKeeper types.AccountKeeper @@ -35,7 +33,7 @@ type Keeper struct { func NewKeeper( cdc codec.BinaryCodec, key sdk.StoreKey, channelKeeper types.ChannelKeeper, portKeeper types.PortKeeper, - accountKeeper types.AccountKeeper, scopedKeeper capabilitykeeper.ScopedKeeper, msgRouter *baseapp.MsgServiceRouter, hook types.IBCAccountHooks, + accountKeeper types.AccountKeeper, scopedKeeper capabilitykeeper.ScopedKeeper, msgRouter *baseapp.MsgServiceRouter, ) Keeper { return Keeper{ storeKey: key, @@ -45,7 +43,6 @@ func NewKeeper( accountKeeper: accountKeeper, scopedKeeper: scopedKeeper, msgRouter: msgRouter, - hook: hook, } } diff --git a/modules/apps/27-interchain-accounts/keeper/relay.go b/modules/apps/27-interchain-accounts/keeper/relay.go index baf6009b2e1..38c65f03955 100644 --- a/modules/apps/27-interchain-accounts/keeper/relay.go +++ b/modules/apps/27-interchain-accounts/keeper/relay.go @@ -91,7 +91,12 @@ func (k Keeper) createOutgoingPacket( timeoutTimestamp, ) - return k.ComputeVirtualTxHash(packetData.Data, packet.Sequence), k.channelKeeper.SendPacket(ctx, channelCap, packet) + // TODO use ics4 wrapper to send packet + if err := k.channelKeeper.SendPacket(ctx, channelCap, packet); err != nil { + return nil, err + } + + return k.ComputeVirtualTxHash(packetData.Data, packet.Sequence), nil } func (k Keeper) DeserializeTx(_ sdk.Context, txBytes []byte) ([]sdk.Msg, error) { @@ -222,30 +227,3 @@ func (k Keeper) OnRecvPacket(ctx sdk.Context, packet channeltypes.Packet) error return types.ErrUnknownPacketData } } - -func (k Keeper) OnAcknowledgementPacket(ctx sdk.Context, packet channeltypes.Packet, data types.IBCAccountPacketData, ack channeltypes.Acknowledgement) error { - switch ack.Response.(type) { - case *channeltypes.Acknowledgement_Error: - if k.hook != nil { - k.hook.OnTxFailed(ctx, packet.SourcePort, packet.SourceChannel, k.ComputeVirtualTxHash(data.Data, packet.Sequence), data.Data) - } - return nil - case *channeltypes.Acknowledgement_Result: - if k.hook != nil { - k.hook.OnTxSucceeded(ctx, packet.SourcePort, packet.SourceChannel, k.ComputeVirtualTxHash(data.Data, packet.Sequence), data.Data) - } - return nil - default: - // the acknowledgement succeeded on the receiving chain so nothing - // needs to be executed and no error needs to be returned - return nil - } -} - -func (k Keeper) OnTimeoutPacket(ctx sdk.Context, packet channeltypes.Packet, data types.IBCAccountPacketData) error { - if k.hook != nil { - k.hook.OnTxFailed(ctx, packet.SourcePort, packet.SourceChannel, k.ComputeVirtualTxHash(data.Data, packet.Sequence), data.Data) - } - - return nil -} diff --git a/modules/apps/27-interchain-accounts/module.go b/modules/apps/27-interchain-accounts/module.go index 740eea4ad6b..97799ce0323 100644 --- a/modules/apps/27-interchain-accounts/module.go +++ b/modules/apps/27-interchain-accounts/module.go @@ -13,6 +13,7 @@ import ( sdk "github.com/cosmos/cosmos-sdk/types" sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" "github.com/cosmos/cosmos-sdk/types/module" + capabilitykeeper "github.com/cosmos/cosmos-sdk/x/capability/keeper" capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types" "github.com/grpc-ecosystem/grpc-gateway/runtime" abci "github.com/tendermint/tendermint/abci/types" @@ -74,7 +75,9 @@ func (AppModuleBasic) RegisterGRPCGatewayRoutes(clientCtx client.Context, mux *r type AppModule struct { AppModuleBasic - keeper keeper.Keeper + keeper keeper.Keeper + scopedKeeper capabilitykeeper.ScopedKeeper + app porttypes.IBCModule } func NewAppModule(k keeper.Keeper) AppModule { @@ -136,7 +139,13 @@ func (am AppModule) EndBlock(ctx sdk.Context, req abci.RequestEndBlock) []abci.V return []abci.ValidatorUpdate{} } -// Implement IBCModule callbacks +// OnChanOpenInit implements the IBCModule callbacks. Interchain Accounts is +// implemented to act as middleware for connected authentication modules on +// the controller side. The connected modules may not change the portID or +// version. They will be allowed to perform custom logic without changing +// the parameters stored within a channel struct. +// +// Controller Chain func (am AppModule) OnChanOpenInit( ctx sdk.Context, order channeltypes.Order, @@ -147,9 +156,24 @@ func (am AppModule) OnChanOpenInit( counterparty channeltypes.Counterparty, version string, ) error { - return am.keeper.OnChanOpenInit(ctx, order, connectionHops, portID, channelID, chanCap, counterparty, version) + if err := am.keeper.OnChanOpenInit(ctx, order, connectionHops, portID, channelID, chanCap, counterparty, version); err != nil { + return err + } + + // issue capability for connected authentication module + appCap, err := am.scopedKeeper.NewCapability(ctx, types.AppCapabilityName(portID, channelID)) + if err != nil { + return sdkerrors.Wrap(err, "could not create capability for underlying application") + } + + // call underlying app's OnChanOpenInit callback with the appVersion + return am.app.OnChanOpenInit(ctx, order, connectionHops, portID, channelID, + appCap, counterparty, version) } +// OnChanOpenTry implements the IBCModule callbacks. +// +// Host Chain func (am AppModule) OnChanOpenTry( ctx sdk.Context, order channeltypes.Order, @@ -164,15 +188,26 @@ func (am AppModule) OnChanOpenTry( return am.keeper.OnChanOpenTry(ctx, order, connectionHops, portID, channelID, chanCap, counterparty, version, counterpartyVersion) } +// OnChanOpenAck implements the IBCModule callbacks. +// +// Controller Chain func (am AppModule) OnChanOpenAck( ctx sdk.Context, portID, channelID string, counterpartyVersion string, ) error { - return am.keeper.OnChanOpenAck(ctx, portID, channelID, counterpartyVersion) + if err := am.keeper.OnChanOpenAck(ctx, portID, channelID, counterpartyVersion); err != nil { + return err + } + + // call underlying app's OnChanOpenAck callback with the counterparty app version. + return am.app.OnChanOpenAck(ctx, portID, channelID, counterpartyVersion) } +// OnChanOpenAck implements the IBCModule callbacks. +// +// Host Chain func (am AppModule) OnChanOpenConfirm( ctx sdk.Context, portID, @@ -181,6 +216,7 @@ func (am AppModule) OnChanOpenConfirm( return am.keeper.OnChanOpenConfirm(ctx, portID, channelID) } +// OnChanCloseInit implements the IBCModule callbacks. func (am AppModule) OnChanCloseInit( ctx sdk.Context, portID, @@ -190,6 +226,7 @@ func (am AppModule) OnChanCloseInit( return nil } +// OnChanCloseConfirm implements the IBCModule callbacks. func (am AppModule) OnChanCloseConfirm( ctx sdk.Context, portID, @@ -198,6 +235,7 @@ func (am AppModule) OnChanCloseConfirm( return sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "user cannot close channel") } +// OnRecvPacket implements the IBCModule callbacks. func (am AppModule) OnRecvPacket( ctx sdk.Context, packet channeltypes.Packet, @@ -223,36 +261,25 @@ func (am AppModule) OnRecvPacket( return ack } +// OnAcknowledgementPacket implements the IBCModule callbacks. func (am AppModule) OnAcknowledgementPacket( ctx sdk.Context, packet channeltypes.Packet, acknowledgement []byte, - _ sdk.AccAddress, + relayer sdk.AccAddress, ) error { - var ack channeltypes.Acknowledgement - - if err := types.ModuleCdc.UnmarshalJSON(acknowledgement, &ack); err != nil { - return sdkerrors.Wrapf(sdkerrors.ErrUnknownRequest, "cannot unmarshal ICS-27 interchain account packet acknowledgment: %v", err) - } - var data types.IBCAccountPacketData - if err := types.ModuleCdc.UnmarshalJSON(packet.GetData(), &data); err != nil { - return sdkerrors.Wrapf(sdkerrors.ErrUnknownRequest, "cannot unmarshal ICS-27 interchain account packet data: %s", err.Error()) - } - - if err := am.keeper.OnAcknowledgementPacket(ctx, packet, data, ack); err != nil { - return err - } - - return nil + // call underlying app's OnAcknowledgementPacket callback. + return am.app.OnAcknowledgementPacket(ctx, packet, acknowledgement, relayer) } +// OnTimeoutPacket implements the IBCModule callbacks. func (am AppModule) OnTimeoutPacket( ctx sdk.Context, packet channeltypes.Packet, - _ sdk.AccAddress, + relayer sdk.AccAddress, ) error { - // TODO - return nil + // call underlying app's OnTimeoutPacket callback + return am.app.OnTimeoutPacket(ctx, packet, relayer) } func (am AppModule) NegotiateAppVersion(ctx sdk.Context, order channeltypes.Order, connectionID, portID string, counterparty channeltypes.Counterparty, proposedVersion string) (string, error) { diff --git a/modules/apps/27-interchain-accounts/types/hook.go b/modules/apps/27-interchain-accounts/types/hook.go deleted file mode 100644 index 0218cc815cb..00000000000 --- a/modules/apps/27-interchain-accounts/types/hook.go +++ /dev/null @@ -1,30 +0,0 @@ -package types - -import sdk "github.com/cosmos/cosmos-sdk/types" - -type IBCAccountHooks interface { - OnTxSucceeded(ctx sdk.Context, sourcePort, sourceChannel string, txHash []byte, txBytes []byte) - OnTxFailed(ctx sdk.Context, sourcePort, sourceChannel string, txHash []byte, txBytes []byte) -} - -type MultiIBCAccountHooks []IBCAccountHooks - -var ( - _ IBCAccountHooks = MultiIBCAccountHooks{} -) - -func NewMultiIBCAccountHooks(hooks ...IBCAccountHooks) MultiIBCAccountHooks { - return hooks -} - -func (h MultiIBCAccountHooks) OnTxSucceeded(ctx sdk.Context, sourcePort, sourceChannel string, txHash []byte, txBytes []byte) { - for i := range h { - h[i].OnTxSucceeded(ctx, sourcePort, sourceChannel, txHash, txBytes) - } -} - -func (h MultiIBCAccountHooks) OnTxFailed(ctx sdk.Context, sourcePort, sourceChannel string, txHash []byte, txBytes []byte) { - for i := range h { - h[i].OnTxFailed(ctx, sourcePort, sourceChannel, txHash, txBytes) - } -} From 9f0f49556d3709b629552e0e18f710bb7cbc43db Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Fri, 17 Sep 2021 12:02:52 +0200 Subject: [PATCH 02/17] add capability handling, fix build and tests --- .../27-interchain-accounts/keeper/relay.go | 17 +++++++++++------ .../keeper/relay_test.go | 15 ++++++++++++++- modules/apps/27-interchain-accounts/module.go | 7 +++++-- .../27-interchain-accounts/types/errors.go | 1 + .../types/expected_keepers.go | 2 +- .../apps/27-interchain-accounts/types/keys.go | 7 +++++++ testing/simapp/app.go | 19 ++++++------------- 7 files changed, 45 insertions(+), 23 deletions(-) diff --git a/modules/apps/27-interchain-accounts/keeper/relay.go b/modules/apps/27-interchain-accounts/keeper/relay.go index 38c65f03955..0e61c67510b 100644 --- a/modules/apps/27-interchain-accounts/keeper/relay.go +++ b/modules/apps/27-interchain-accounts/keeper/relay.go @@ -5,16 +5,18 @@ import ( sdk "github.com/cosmos/cosmos-sdk/types" sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" + capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types" + "github.com/tendermint/tendermint/crypto/tmhash" + "github.com/cosmos/ibc-go/modules/apps/27-interchain-accounts/types" clienttypes "github.com/cosmos/ibc-go/modules/core/02-client/types" channeltypes "github.com/cosmos/ibc-go/modules/core/04-channel/types" host "github.com/cosmos/ibc-go/modules/core/24-host" - "github.com/tendermint/tendermint/crypto/tmhash" ) -// TODO: implement middleware functionality, this will allow us to use capabilities to -// manage helper module access to owner addresses they do not have capabilities for -func (k Keeper) TrySendTx(ctx sdk.Context, portID string, data interface{}) ([]byte, error) { +// TrySendTx takes in a transaction from a base application and attempts to send the packet +// if the base application has the capability to send on the provided portID +func (k Keeper) TrySendTx(ctx sdk.Context, appCap *capabilitytypes.Capability, portID string, data interface{}) ([]byte, error) { // Check for the active channel activeChannelId, found := k.GetActiveChannel(ctx, portID) if !found { @@ -23,7 +25,11 @@ func (k Keeper) TrySendTx(ctx sdk.Context, portID string, data interface{}) ([]b sourceChannelEnd, found := k.channelKeeper.GetChannel(ctx, portID, activeChannelId) if !found { - return []byte{}, sdkerrors.Wrap(channeltypes.ErrChannelNotFound, activeChannelId) + return nil, sdkerrors.Wrap(channeltypes.ErrChannelNotFound, activeChannelId) + } + + if !k.AuthenticateCapability(ctx, appCap, types.AppCapabilityName(portID, activeChannelId)) { + return nil, sdkerrors.Wrapf(types.ErrAppCapabilityNotFound, "could not authenticate provided capability for portID %s and channelID %s", portID, activeChannelId) } destinationPort := sourceChannelEnd.GetCounterparty().GetPortID() @@ -91,7 +97,6 @@ func (k Keeper) createOutgoingPacket( timeoutTimestamp, ) - // TODO use ics4 wrapper to send packet if err := k.channelKeeper.SendPacket(ctx, channelCap, packet); err != nil { return nil, err } diff --git a/modules/apps/27-interchain-accounts/keeper/relay_test.go b/modules/apps/27-interchain-accounts/keeper/relay_test.go index 66c3b22d51a..b9a960f6e6c 100644 --- a/modules/apps/27-interchain-accounts/keeper/relay_test.go +++ b/modules/apps/27-interchain-accounts/keeper/relay_test.go @@ -3,7 +3,9 @@ package keeper_test import ( sdk "github.com/cosmos/cosmos-sdk/types" banktypes "github.com/cosmos/cosmos-sdk/x/bank/types" + capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types" + "github.com/cosmos/ibc-go/modules/apps/27-interchain-accounts/types" ibctesting "github.com/cosmos/ibc-go/testing" ) @@ -12,6 +14,7 @@ func (suite *KeeperTestSuite) TestTrySendTx() { path *ibctesting.Path msg interface{} portID string + appCap *capabilitytypes.Capability ) testCases := []struct { @@ -56,6 +59,15 @@ func (suite *KeeperTestSuite) TestTrySendTx() { suite.chainA.GetSimApp().ICAKeeper.SetActiveChannel(suite.chainA.GetContext(), portID, "channel-100") }, false, }, + { + "could not authenticate app capability", func() { + amount, _ := sdk.ParseCoinsNormalized("100stake") + interchainAccountAddr, _ := suite.chainB.GetSimApp().ICAKeeper.GetInterchainAccountAddress(suite.chainB.GetContext(), path.EndpointA.ChannelConfig.PortID) + msg = &banktypes.MsgSend{FromAddress: interchainAccountAddr, ToAddress: suite.chainB.SenderAccount.GetAddress().String(), Amount: amount} + appCap = nil + }, false, + }, + { "data is nil", func() { msg = nil @@ -80,8 +92,9 @@ func (suite *KeeperTestSuite) TestTrySendTx() { err := suite.SetupICAPath(path, owner) suite.Require().NoError(err) portID = path.EndpointA.ChannelConfig.PortID + appCap, _ = suite.chainA.GetSimApp().ScopedICAKeeper.GetCapability(suite.chainA.GetContext(), types.AppCapabilityName(portID, path.EndpointA.ChannelID)) tc.malleate() - _, err = suite.chainA.GetSimApp().ICAKeeper.TrySendTx(suite.chainA.GetContext(), portID, msg) + _, err = suite.chainA.GetSimApp().ICAKeeper.TrySendTx(suite.chainA.GetContext(), appCap, portID, msg) if tc.expPass { suite.Require().NoError(err) diff --git a/modules/apps/27-interchain-accounts/module.go b/modules/apps/27-interchain-accounts/module.go index 97799ce0323..900f07052cc 100644 --- a/modules/apps/27-interchain-accounts/module.go +++ b/modules/apps/27-interchain-accounts/module.go @@ -80,9 +80,12 @@ type AppModule struct { app porttypes.IBCModule } -func NewAppModule(k keeper.Keeper) AppModule { +// NewAppModule creates an interchain accounts app module. +func NewAppModule(k keeper.Keeper, scopedKeeper capabilitykeeper.ScopedKeeper, app porttypes.IBCModule) AppModule { return AppModule{ - keeper: k, + keeper: k, + scopedKeeper: scopedKeeper, + app: app, } } diff --git a/modules/apps/27-interchain-accounts/types/errors.go b/modules/apps/27-interchain-accounts/types/errors.go index 32425a81071..cfb2a41b136 100644 --- a/modules/apps/27-interchain-accounts/types/errors.go +++ b/modules/apps/27-interchain-accounts/types/errors.go @@ -16,4 +16,5 @@ var ( ErrActiveChannelNotFound = sdkerrors.Register(ModuleName, 10, "no active channel for this owner") ErrInvalidVersion = sdkerrors.Register(ModuleName, 11, "invalid interchain accounts version") ErrInvalidOwnerAddress = sdkerrors.Register(ModuleName, 12, "invalid owner address") + ErrAppCapabilityNotFound = sdkerrors.Register(ModuleName, 13, "app capability not found") ) diff --git a/modules/apps/27-interchain-accounts/types/expected_keepers.go b/modules/apps/27-interchain-accounts/types/expected_keepers.go index 448a0b3fcaa..a67ba3f75f0 100644 --- a/modules/apps/27-interchain-accounts/types/expected_keepers.go +++ b/modules/apps/27-interchain-accounts/types/expected_keepers.go @@ -4,6 +4,7 @@ import ( sdk "github.com/cosmos/cosmos-sdk/types" authtypes "github.com/cosmos/cosmos-sdk/x/auth/types" capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types" + connectiontypes "github.com/cosmos/ibc-go/modules/core/03-connection/types" channeltypes "github.com/cosmos/ibc-go/modules/core/04-channel/types" ibcexported "github.com/cosmos/ibc-go/modules/core/exported" @@ -26,7 +27,6 @@ type ChannelKeeper interface { GetChannel(ctx sdk.Context, srcPort, srcChan string) (channel channeltypes.Channel, found bool) GetNextSequenceSend(ctx sdk.Context, portID, channelID string) (uint64, bool) SendPacket(ctx sdk.Context, channelCap *capabilitytypes.Capability, packet ibcexported.PacketI) error - ChanCloseInit(ctx sdk.Context, portID, channelID string, chanCap *capabilitytypes.Capability) error ChanOpenInit(ctx sdk.Context, order channeltypes.Order, connectionHops []string, portID string, portCap *capabilitytypes.Capability, counterparty channeltypes.Counterparty, version string) (string, *capabilitytypes.Capability, error) } diff --git a/modules/apps/27-interchain-accounts/types/keys.go b/modules/apps/27-interchain-accounts/types/keys.go index 909813ec46d..96187e0cf9b 100644 --- a/modules/apps/27-interchain-accounts/types/keys.go +++ b/modules/apps/27-interchain-accounts/types/keys.go @@ -22,6 +22,8 @@ const ( // MemStoreKey defines the in-memory store key MemStoreKey = "mem_capability" + + KeyAppCapabilityPrefix = "appCapabilities" ) func KeyActiveChannel(portId string) []byte { @@ -39,3 +41,8 @@ var ( func GetIdentifier(portID, channelID string) string { return fmt.Sprintf("%s/%s/", portID, channelID) } + +// TODO: remove once generic function is added to 24-host +func AppCapabilityName(channelID, portID string) string { + return fmt.Sprintf("%s/%s/%s", KeyAppCapabilityPrefix, channelID, portID) +} diff --git a/testing/simapp/app.go b/testing/simapp/app.go index 6592c6ed424..2843a010247 100644 --- a/testing/simapp/app.go +++ b/testing/simapp/app.go @@ -331,16 +331,16 @@ func NewSimApp( ) transferModule := transfer.NewAppModule(app.TransferKeeper) + // NOTE: the IBC mock keeper and application module is used only for testing core IBC. Do + // note replicate if you do not need to test core IBC or light clients. + mockModule := ibcmock.NewAppModule(scopedIBCMockKeeper, &app.IBCKeeper.PortKeeper) + app.ICAKeeper = icakeeper.NewKeeper( appCodec, keys[icatypes.StoreKey], app.IBCKeeper.ChannelKeeper, &app.IBCKeeper.PortKeeper, - app.AccountKeeper, scopedICAKeeper, app.MsgServiceRouter(), app, + app.AccountKeeper, scopedICAKeeper, app.MsgServiceRouter(), ) - icaModule := ica.NewAppModule(app.ICAKeeper) - - // NOTE: the IBC mock keeper and application module is used only for testing core IBC. Do - // note replicate if you do not need to test core IBC or light clients. - mockModule := ibcmock.NewAppModule(scopedIBCMockKeeper, &app.IBCKeeper.PortKeeper) + icaModule := ica.NewAppModule(app.ICAKeeper, scopedICAKeeper, mockModule) // Create static IBC router, add transfer route, then set and seal it ibcRouter := porttypes.NewRouter() @@ -675,10 +675,3 @@ func initParamsKeeper(appCodec codec.BinaryCodec, legacyAmino *codec.LegacyAmino return paramsKeeper } - -// Interchain Accounts code -func (*SimApp) OnTxSucceeded(ctx sdk.Context, sourcePort, sourceChannel string, txHash []byte, txBytes []byte) { -} - -func (*SimApp) OnTxFailed(ctx sdk.Context, sourcePort, sourceChannel string, txHash []byte, txBytes []byte) { -} From 0eb681af8ba5868a844aab03130f3fc54d004f2f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Wed, 29 Sep 2021 12:36:29 +0200 Subject: [PATCH 03/17] split module.go into ibc_module.go --- .../apps/27-interchain-accounts/ibc_module.go | 179 ++++++++++++++++++ modules/apps/27-interchain-accounts/module.go | 165 +--------------- 2 files changed, 184 insertions(+), 160 deletions(-) create mode 100644 modules/apps/27-interchain-accounts/ibc_module.go diff --git a/modules/apps/27-interchain-accounts/ibc_module.go b/modules/apps/27-interchain-accounts/ibc_module.go new file mode 100644 index 00000000000..21aa421af48 --- /dev/null +++ b/modules/apps/27-interchain-accounts/ibc_module.go @@ -0,0 +1,179 @@ +package interchain_accounts + +import ( + "fmt" + + sdk "github.com/cosmos/cosmos-sdk/types" + sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" + capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types" + + "github.com/cosmos/ibc-go/v2/modules/apps/27-interchain-accounts/keeper" + "github.com/cosmos/ibc-go/v2/modules/apps/27-interchain-accounts/types" + channeltypes "github.com/cosmos/ibc-go/v2/modules/core/04-channel/types" + porttypes "github.com/cosmos/ibc-go/v2/modules/core/05-port/types" + ibcexported "github.com/cosmos/ibc-go/v2/modules/core/exported" +) + +// IBCModule implements the ICS26 callbacks for interchain accounts given +// the interchain account keeper and the underlying application. +type IBCModule struct { + keeper keeper.Keeper + app porttypes.IBCModule +} + +// NewIBCModule creates a new IBCModule given the keeper and underlying application +func NewIBCModule(k keeper.Keeper, app porttypes.IBCModule) IBCModule { + return IBCModule{ + keeper: k, + app: app, + } +} + +// OnChanOpenInit implements the IBCModule callbacks. Interchain Accounts is +// implemented to act as middleware for connected authentication modules on +// the controller side. The connected modules may not change the portID or +// version. They will be allowed to perform custom logic without changing +// the parameters stored within a channel struct. +// +// Controller Chain +func (im IBCModule) OnChanOpenInit( + ctx sdk.Context, + order channeltypes.Order, + connectionHops []string, + portID string, + channelID string, + chanCap *capabilitytypes.Capability, + counterparty channeltypes.Counterparty, + version string, +) error { + if err := im.keeper.OnChanOpenInit(ctx, order, connectionHops, portID, channelID, chanCap, counterparty, version); err != nil { + return err + } + + // call underlying app's OnChanOpenInit callback with the appVersion + return im.app.OnChanOpenInit(ctx, order, connectionHops, portID, channelID, + chanCap, counterparty, version) +} + +// OnChanOpenTry implements the IBCModule callbacks. +// +// Host Chain +func (im IBCModule) OnChanOpenTry( + ctx sdk.Context, + order channeltypes.Order, + connectionHops []string, + portID, + channelID string, + chanCap *capabilitytypes.Capability, + counterparty channeltypes.Counterparty, + version, + counterpartyVersion string, +) error { + return im.keeper.OnChanOpenTry(ctx, order, connectionHops, portID, channelID, chanCap, counterparty, version, counterpartyVersion) +} + +// OnChanOpenAck implements the IBCModule callbacks. +// +// Controller Chain +func (im IBCModule) OnChanOpenAck( + ctx sdk.Context, + portID, + channelID string, + counterpartyVersion string, +) error { + if err := im.keeper.OnChanOpenAck(ctx, portID, channelID, counterpartyVersion); err != nil { + return err + } + + // call underlying app's OnChanOpenAck callback with the counterparty app version. + return im.app.OnChanOpenAck(ctx, portID, channelID, counterpartyVersion) +} + +// OnChanOpenAck implements the IBCModule callbacks. +// +// Host Chain +func (im IBCModule) OnChanOpenConfirm( + ctx sdk.Context, + portID, + channelID string, +) error { + return im.keeper.OnChanOpenConfirm(ctx, portID, channelID) +} + +// OnChanCloseInit implements the IBCModule callbacks. +func (im IBCModule) OnChanCloseInit( + ctx sdk.Context, + portID, + channelID string, +) error { + // Disallow user-initiated channel closing for interchain account channels + return sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "user cannot close channel") +} + +// OnChanCloseConfirm implements the IBCModule callbacks. +func (im IBCModule) OnChanCloseConfirm( + ctx sdk.Context, + portID, + channelID string, +) error { + return nil +} + +// OnRecvPacket implements the IBCModule callbacks. +func (im IBCModule) OnRecvPacket( + ctx sdk.Context, + packet channeltypes.Packet, + _ sdk.AccAddress, +) ibcexported.Acknowledgement { + ack := channeltypes.NewResultAcknowledgement([]byte{byte(1)}) + + var data types.IBCAccountPacketData + if err := types.ModuleCdc.UnmarshalJSON(packet.GetData(), &data); err != nil { + ack = channeltypes.NewErrorAcknowledgement(fmt.Sprintf("cannot unmarshal ICS-27 interchain account packet data: %s", err.Error())) + } + + // only attempt the application logic if the packet data + // was successfully decoded + if ack.Success() { + err := im.keeper.OnRecvPacket(ctx, packet) + if err != nil { + ack = channeltypes.NewErrorAcknowledgement(err.Error()) + } + } + + // NOTE: acknowledgement will be written synchronously during IBC handler execution. + return ack +} + +// OnAcknowledgementPacket implements the IBCModule callbacks. +func (im IBCModule) OnAcknowledgementPacket( + ctx sdk.Context, + packet channeltypes.Packet, + acknowledgement []byte, + relayer sdk.AccAddress, +) error { + // call underlying app's OnAcknowledgementPacket callback. + return im.app.OnAcknowledgementPacket(ctx, packet, acknowledgement, relayer) +} + +// OnTimeoutPacket implements the IBCModule callbacks. +func (im IBCModule) OnTimeoutPacket( + ctx sdk.Context, + packet channeltypes.Packet, + relayer sdk.AccAddress, +) error { + // call underlying app's OnTimeoutPacket callback + return im.app.OnTimeoutPacket(ctx, packet, relayer) +} + +// NegotiateAppVersion implements the IBCModule interface +func (im IBCModule) NegotiateAppVersion( + ctx sdk.Context, + order channeltypes.Order, + connectionID string, + portID string, + counterparty channeltypes.Counterparty, + proposedVersion string, +) (string, error) { + return im.keeper.NegotiateAppVersion(ctx, order, connectionID, portID, counterparty, proposedVersion) +} diff --git a/modules/apps/27-interchain-accounts/module.go b/modules/apps/27-interchain-accounts/module.go index f77afd642a9..40aa710dbfe 100644 --- a/modules/apps/27-interchain-accounts/module.go +++ b/modules/apps/27-interchain-accounts/module.go @@ -2,7 +2,6 @@ package interchain_accounts import ( "encoding/json" - "fmt" "github.com/gorilla/mux" "github.com/spf13/cobra" @@ -11,25 +10,26 @@ import ( "github.com/cosmos/cosmos-sdk/codec" codectypes "github.com/cosmos/cosmos-sdk/codec/types" sdk "github.com/cosmos/cosmos-sdk/types" - sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" "github.com/cosmos/cosmos-sdk/types/module" capabilitykeeper "github.com/cosmos/cosmos-sdk/x/capability/keeper" - capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types" "github.com/grpc-ecosystem/grpc-gateway/runtime" abci "github.com/tendermint/tendermint/abci/types" "github.com/cosmos/ibc-go/v2/modules/apps/27-interchain-accounts/client/cli" "github.com/cosmos/ibc-go/v2/modules/apps/27-interchain-accounts/keeper" "github.com/cosmos/ibc-go/v2/modules/apps/27-interchain-accounts/types" - channeltypes "github.com/cosmos/ibc-go/v2/modules/core/04-channel/types" porttypes "github.com/cosmos/ibc-go/v2/modules/core/05-port/types" - ibcexported "github.com/cosmos/ibc-go/v2/modules/core/exported" ) var ( _ module.AppModule = AppModule{} _ porttypes.IBCModule = AppModule{} _ module.AppModuleBasic = AppModuleBasic{} + + // Middleware must implement types.ChannelKeeper and types.PortKeeper expected interfaces + // so that it can wrap IBC channel and port logic for underlying application. + _ types.ChannelKeeper = Keeper{} + _ types.PortKeeper = Keeper{} ) type AppModuleBasic struct{} @@ -141,158 +141,3 @@ func (am AppModule) BeginBlock(ctx sdk.Context, req abci.RequestBeginBlock) { func (am AppModule) EndBlock(ctx sdk.Context, req abci.RequestEndBlock) []abci.ValidatorUpdate { return []abci.ValidatorUpdate{} } - -// OnChanOpenInit implements the IBCModule callbacks. Interchain Accounts is -// implemented to act as middleware for connected authentication modules on -// the controller side. The connected modules may not change the portID or -// version. They will be allowed to perform custom logic without changing -// the parameters stored within a channel struct. -// -// Controller Chain -func (am AppModule) OnChanOpenInit( - ctx sdk.Context, - order channeltypes.Order, - connectionHops []string, - portID string, - channelID string, - chanCap *capabilitytypes.Capability, - counterparty channeltypes.Counterparty, - version string, -) error { - if err := am.keeper.OnChanOpenInit(ctx, order, connectionHops, portID, channelID, chanCap, counterparty, version); err != nil { - return err - } - - // issue capability for connected authentication module - appCap, err := am.scopedKeeper.NewCapability(ctx, types.AppCapabilityName(portID, channelID)) - if err != nil { - return sdkerrors.Wrap(err, "could not create capability for underlying application") - } - - // call underlying app's OnChanOpenInit callback with the appVersion - return am.app.OnChanOpenInit(ctx, order, connectionHops, portID, channelID, - appCap, counterparty, version) -} - -// OnChanOpenTry implements the IBCModule callbacks. -// -// Host Chain -func (am AppModule) OnChanOpenTry( - ctx sdk.Context, - order channeltypes.Order, - connectionHops []string, - portID, - channelID string, - chanCap *capabilitytypes.Capability, - counterparty channeltypes.Counterparty, - version, - counterpartyVersion string, -) error { - return am.keeper.OnChanOpenTry(ctx, order, connectionHops, portID, channelID, chanCap, counterparty, version, counterpartyVersion) -} - -// OnChanOpenAck implements the IBCModule callbacks. -// -// Controller Chain -func (am AppModule) OnChanOpenAck( - ctx sdk.Context, - portID, - channelID string, - counterpartyVersion string, -) error { - if err := am.keeper.OnChanOpenAck(ctx, portID, channelID, counterpartyVersion); err != nil { - return err - } - - // call underlying app's OnChanOpenAck callback with the counterparty app version. - return am.app.OnChanOpenAck(ctx, portID, channelID, counterpartyVersion) -} - -// OnChanOpenAck implements the IBCModule callbacks. -// -// Host Chain -func (am AppModule) OnChanOpenConfirm( - ctx sdk.Context, - portID, - channelID string, -) error { - return am.keeper.OnChanOpenConfirm(ctx, portID, channelID) -} - -// OnChanCloseInit implements the IBCModule callbacks. -func (am AppModule) OnChanCloseInit( - ctx sdk.Context, - portID, - channelID string, -) error { - // Disallow user-initiated channel closing for interchain account channels - return sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "user cannot close channel") -} - -// OnChanCloseConfirm implements the IBCModule callbacks. -func (am AppModule) OnChanCloseConfirm( - ctx sdk.Context, - portID, - channelID string, -) error { - return nil -} - -// OnRecvPacket implements the IBCModule callbacks. -func (am AppModule) OnRecvPacket( - ctx sdk.Context, - packet channeltypes.Packet, - _ sdk.AccAddress, -) ibcexported.Acknowledgement { - ack := channeltypes.NewResultAcknowledgement([]byte{byte(1)}) - - var data types.IBCAccountPacketData - if err := types.ModuleCdc.UnmarshalJSON(packet.GetData(), &data); err != nil { - ack = channeltypes.NewErrorAcknowledgement(fmt.Sprintf("cannot unmarshal ICS-27 interchain account packet data: %s", err.Error())) - } - - // only attempt the application logic if the packet data - // was successfully decoded - if ack.Success() { - err := am.keeper.OnRecvPacket(ctx, packet) - if err != nil { - ack = channeltypes.NewErrorAcknowledgement(err.Error()) - } - } - - // NOTE: acknowledgement will be written synchronously during IBC handler execution. - return ack -} - -// OnAcknowledgementPacket implements the IBCModule callbacks. -func (am AppModule) OnAcknowledgementPacket( - ctx sdk.Context, - packet channeltypes.Packet, - acknowledgement []byte, - relayer sdk.AccAddress, -) error { - // call underlying app's OnAcknowledgementPacket callback. - return am.app.OnAcknowledgementPacket(ctx, packet, acknowledgement, relayer) -} - -// OnTimeoutPacket implements the IBCModule callbacks. -func (am AppModule) OnTimeoutPacket( - ctx sdk.Context, - packet channeltypes.Packet, - relayer sdk.AccAddress, -) error { - // call underlying app's OnTimeoutPacket callback - return am.app.OnTimeoutPacket(ctx, packet, relayer) -} - -// 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) { - return am.keeper.NegotiateAppVersion(ctx, order, connectionID, portID, counterparty, proposedVersion) -} From 44045358f63987ab7cc5a2c48d257e76248d52f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Wed, 29 Sep 2021 16:39:00 +0200 Subject: [PATCH 04/17] add middleware handshake tests --- .../27-interchain-accounts/ibc_module_test.go | 354 +++++++++++++----- .../keeper/handshake.go | 5 - 2 files changed, 258 insertions(+), 101 deletions(-) diff --git a/modules/apps/27-interchain-accounts/ibc_module_test.go b/modules/apps/27-interchain-accounts/ibc_module_test.go index 3be90ae572c..788f6678781 100644 --- a/modules/apps/27-interchain-accounts/ibc_module_test.go +++ b/modules/apps/27-interchain-accounts/ibc_module_test.go @@ -45,20 +45,6 @@ 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)) - - // modify mock module (ica authentication module) to no-op on OpenChanInit - // instead of attempting to claim channel capability - mockModuleA := suite.chainA.GetSimApp().GetMockModule() - mockModuleB := suite.chainB.GetSimApp().GetMockModule() - onChanOpenInit := func(ctx sdk.Context, order channeltypes.Order, connectionHops []string, - portID, channelID string, chanCap *capabilitytypes.Capability, - counterparty channeltypes.Counterparty, version string, - ) error { - // do not claim channel capability - return nil - } - mockModuleA.IBCApp.OnChanOpenInit = onChanOpenInit - mockModuleB.IBCApp.OnChanOpenInit = onChanOpenInit } func NewICAPath(chainA, chainB *ibctesting.TestChain) *ibctesting.Path { @@ -116,118 +102,294 @@ func SetupICAPath(path *ibctesting.Path, owner string) error { } func (suite *InterchainAccountsTestSuite) TestOnChanOpenInit() { - suite.SetupTest() // reset - path := NewICAPath(suite.chainA, suite.chainB) - suite.coordinator.SetupConnections(path) + var ( + channel *channeltypes.Channel + ) + testCases := []struct { + name string + malleate func() + expPass bool + }{ + { + "success", func() {}, true, + }, + { + "ICA OnChanOpenInit fails - UNORDERED channel", func() { + channel.Ordering = channeltypes.UNORDERED + }, false, + }, + { + "ICA auth module callback fails", func() { + mockModule := suite.chainA.GetSimApp().GetMockModule() + mockModule.IBCApp.OnChanOpenInit = func(ctx sdk.Context, order channeltypes.Order, connectionHops []string, + portID, channelID string, chanCap *capabilitytypes.Capability, + counterparty channeltypes.Counterparty, version string, + ) error { + return fmt.Errorf("mock ica auth fails") + } + + }, false, + }, + } - // mock init interchain account - 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)) - path.EndpointA.ChannelConfig.PortID = portID - - // default values - counterparty := channeltypes.NewCounterparty(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) - channel := &channeltypes.Channel{ - State: channeltypes.INIT, - Ordering: channeltypes.ORDERED, - Counterparty: counterparty, - ConnectionHops: []string{path.EndpointA.ConnectionID}, - Version: types.VersionPrefix, + for _, tc := range testCases { + tc := tc + + suite.Run(tc.name, func() { + suite.SetupTest() // reset + path := NewICAPath(suite.chainA, suite.chainB) + suite.coordinator.SetupConnections(path) + + // mock init interchain account + 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)) + path.EndpointA.ChannelConfig.PortID = portID + + // default values + counterparty := channeltypes.NewCounterparty(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) + channel = &channeltypes.Channel{ + State: channeltypes.INIT, + Ordering: channeltypes.ORDERED, + Counterparty: counterparty, + ConnectionHops: []string{path.EndpointA.ConnectionID}, + Version: types.VersionPrefix, + } + + tc.malleate() + + module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID) + suite.Require().NoError(err) + + chanCap, err := suite.chainA.App.GetScopedIBCKeeper().NewCapability(suite.chainA.GetContext(), host.ChannelCapabilityPath(ibctesting.TransferPort, path.EndpointA.ChannelID)) + suite.Require().NoError(err) + + cbs, ok := suite.chainA.App.GetIBCKeeper().Router.GetRoute(module) + suite.Require().True(ok) + + err = cbs.OnChanOpenInit(suite.chainA.GetContext(), channel.Ordering, channel.GetConnectionHops(), + path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, chanCap, channel.Counterparty, channel.GetVersion(), + ) + + if tc.expPass { + suite.Require().NoError(err) + } else { + suite.Require().Error(err) + } + + }) } - module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), types.PortID) - suite.Require().NoError(err) +} - chanCap, err := suite.chainA.App.GetScopedIBCKeeper().NewCapability(suite.chainA.GetContext(), host.ChannelCapabilityPath(ibctesting.TransferPort, path.EndpointA.ChannelID)) - suite.Require().NoError(err) +func (suite *InterchainAccountsTestSuite) TestOnChanOpenTry() { + var ( + channel *channeltypes.Channel + ) + testCases := []struct { + name string + malleate func() + expPass bool + }{ + + { + "success", func() {}, true, + }, + { + "success: ICA auth module callback returns error", func() { + // mock module callback should not be called on host side + mockModule := suite.chainB.GetSimApp().GetMockModule() + mockModule.IBCApp.OnChanOpenTry = func(ctx sdk.Context, order channeltypes.Order, connectionHops []string, + portID, channelID string, chanCap *capabilitytypes.Capability, + counterparty channeltypes.Counterparty, version, counterpartyVersion string, + ) error { + return fmt.Errorf("mock ica auth fails") + } + + }, true, + }, + { + "ICA callback fails - invalid version", func() { + channel.Version = types.VersionPrefix + }, false, + }, + } - cbs, ok := suite.chainA.App.GetIBCKeeper().Router.GetRoute(module) - suite.Require().True(ok) + for _, tc := range testCases { + tc := tc - err = cbs.OnChanOpenInit(suite.chainA.GetContext(), channel.Ordering, channel.GetConnectionHops(), - path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, chanCap, channel.Counterparty, channel.GetVersion(), - ) - suite.Require().NoError(err) -} + suite.Run(tc.name, func() { + suite.SetupTest() // reset + path := NewICAPath(suite.chainA, suite.chainB) + counterpartyVersion := TestVersion + suite.coordinator.SetupConnections(path) -func (suite *InterchainAccountsTestSuite) TestOnChanOpenTry() { - suite.SetupTest() // reset - path := NewICAPath(suite.chainA, suite.chainB) - counterpartyVersion := types.VersionPrefix - suite.coordinator.SetupConnections(path) + err := InitInterchainAccount(path.EndpointA, TestOwnerAddress) + suite.Require().NoError(err) - err := InitInterchainAccount(path.EndpointA, TestOwnerAddress) - 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: TestVersion, + } - // 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.VersionPrefix, - } + tc.malleate() - module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), types.PortID) - suite.Require().NoError(err) + module, _, err := suite.chainB.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID) + suite.Require().NoError(err) - chanCap, err := suite.chainA.App.GetScopedIBCKeeper().NewCapability(suite.chainA.GetContext(), host.ChannelCapabilityPath(ibctesting.TransferPort, path.EndpointA.ChannelID)) - suite.Require().NoError(err) + chanCap, err := suite.chainB.App.GetScopedIBCKeeper().NewCapability(suite.chainB.GetContext(), host.ChannelCapabilityPath(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)) + suite.Require().NoError(err) - cbs, ok := suite.chainA.App.GetIBCKeeper().Router.GetRoute(module) - suite.Require().True(ok) + cbs, ok := suite.chainB.App.GetIBCKeeper().Router.GetRoute(module) + suite.Require().True(ok) + + err = cbs.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) + } + + }) + } - err = cbs.OnChanOpenTry(suite.chainA.GetContext(), channel.Ordering, channel.GetConnectionHops(), - path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, chanCap, channel.Counterparty, channel.GetVersion(), counterpartyVersion, - ) } func (suite *InterchainAccountsTestSuite) TestOnChanOpenAck() { - suite.SetupTest() // reset - path := NewICAPath(suite.chainA, suite.chainB) - counterpartyVersion := types.VersionPrefix - suite.coordinator.SetupConnections(path) + var ( + counterpartyVersion string + ) + testCases := []struct { + name string + malleate func() + expPass bool + }{ + { + "success", func() {}, true, + }, + { + "ICA OnChanOpenACK fails - invalid version", func() { + counterpartyVersion = "invalid|version" + }, false, + }, + { + "ICA auth module callback fails", func() { + mockModule := suite.chainA.GetSimApp().GetMockModule() + mockModule.IBCApp.OnChanOpenAck = func( + ctx sdk.Context, portID, channelID string, counterpartyVersion string, + ) error { + return fmt.Errorf("mock ica auth fails") + } + }, false, + }, + } - err := InitInterchainAccount(path.EndpointA, TestOwnerAddress) - suite.Require().NoError(err) + for _, tc := range testCases { + tc := tc - err = path.EndpointB.ChanOpenTry() - suite.Require().NoError(err) + suite.Run(tc.name, func() { - module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), types.PortID) - suite.Require().NoError(err) + suite.SetupTest() // reset + path := NewICAPath(suite.chainA, suite.chainB) + counterpartyVersion = types.VersionPrefix + suite.coordinator.SetupConnections(path) - cbs, ok := suite.chainA.App.GetIBCKeeper().Router.GetRoute(module) - suite.Require().True(ok) + err := InitInterchainAccount(path.EndpointA, TestOwnerAddress) + suite.Require().NoError(err) + + err = path.EndpointB.ChanOpenTry() + suite.Require().NoError(err) + + tc.malleate() + + module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), types.PortID) + suite.Require().NoError(err) + + cbs, ok := suite.chainA.App.GetIBCKeeper().Router.GetRoute(module) + suite.Require().True(ok) + + err = cbs.OnChanOpenAck(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, counterpartyVersion) + + if tc.expPass { + suite.Require().NoError(err) + } else { + suite.Require().Error(err) + } + + }) + } - err = cbs.OnChanOpenAck(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, counterpartyVersion) - suite.Require().NoError(err) } func (suite *InterchainAccountsTestSuite) TestOnChanOpenConfirm() { - path := NewICAPath(suite.chainA, suite.chainB) - suite.coordinator.SetupConnections(path) + testCases := []struct { + name string + malleate func() + expPass bool + }{ + + { + "success", func() {}, true, + }, + { + "success: ICA auth module callback returns error", func() { + // mock module callback should not be called on host side + mockModule := suite.chainB.GetSimApp().GetMockModule() + mockModule.IBCApp.OnChanOpenConfirm = func( + ctx sdk.Context, portID, channelID string, + ) error { + return fmt.Errorf("mock ica auth fails") + } + + }, true, + }, + } - err := InitInterchainAccount(path.EndpointA, TestOwnerAddress) - suite.Require().NoError(err) + for _, tc := range testCases { + tc := tc - err = path.EndpointB.ChanOpenTry() - suite.Require().NoError(err) + suite.Run(tc.name, func() { + suite.SetupTest() + path := NewICAPath(suite.chainA, suite.chainB) + suite.coordinator.SetupConnections(path) - err = path.EndpointA.ChanOpenAck() - suite.Require().NoError(err) + err := InitInterchainAccount(path.EndpointA, TestOwnerAddress) + suite.Require().NoError(err) - module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), types.PortID) - suite.Require().NoError(err) + err = path.EndpointB.ChanOpenTry() + suite.Require().NoError(err) - cbs, ok := suite.chainA.App.GetIBCKeeper().Router.GetRoute(module) - suite.Require().True(ok) + err = path.EndpointA.ChanOpenAck() + suite.Require().NoError(err) + + tc.malleate() + + module, _, err := suite.chainB.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID) + suite.Require().NoError(err) + + cbs, ok := suite.chainB.App.GetIBCKeeper().Router.GetRoute(module) + suite.Require().True(ok) + + err = cbs.OnChanOpenConfirm(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) + + if tc.expPass { + suite.Require().NoError(err) + } else { + suite.Require().Error(err) + } + + }) + } - err = cbs.OnChanOpenConfirm(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) - suite.Require().NoError(err) } func (suite *InterchainAccountsTestSuite) TestNegotiateAppVersion() { diff --git a/modules/apps/27-interchain-accounts/keeper/handshake.go b/modules/apps/27-interchain-accounts/keeper/handshake.go index 9b75327e3b7..3a7cce677e0 100644 --- a/modules/apps/27-interchain-accounts/keeper/handshake.go +++ b/modules/apps/27-interchain-accounts/keeper/handshake.go @@ -46,11 +46,6 @@ func (k Keeper) OnChanOpenInit( return sdkerrors.Wrapf(porttypes.ErrInvalidPort, "existing active channel (%s) for portID (%s)", existingChannelID, portID) } - // Claim channel capability passed back by IBC module - if err := k.ClaimCapability(ctx, chanCap, host.ChannelCapabilityPath(portID, channelID)); err != nil { - return sdkerrors.Wrap(err, "interchain accounts moudle could not claim capability passed back by the IBC module") - } - return nil } From eb071fb181b185a8314ed2e6f034d12492cac517 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Tue, 5 Oct 2021 11:59:27 +0200 Subject: [PATCH 05/17] fix app.go wiring and various tests --- .../keeper/handshake_test.go | 6 --- .../27-interchain-accounts/keeper/relay.go | 17 ++------ .../keeper/relay_test.go | 42 +++++++++---------- testing/simapp/app.go | 7 +++- 4 files changed, 31 insertions(+), 41 deletions(-) diff --git a/modules/apps/27-interchain-accounts/keeper/handshake_test.go b/modules/apps/27-interchain-accounts/keeper/handshake_test.go index ba2a1eea9ca..58273df928b 100644 --- a/modules/apps/27-interchain-accounts/keeper/handshake_test.go +++ b/modules/apps/27-interchain-accounts/keeper/handshake_test.go @@ -45,12 +45,6 @@ func (suite *KeeperTestSuite) TestOnChanOpenInit() { 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 { diff --git a/modules/apps/27-interchain-accounts/keeper/relay.go b/modules/apps/27-interchain-accounts/keeper/relay.go index 3dbac6ce352..2fb345d02d6 100644 --- a/modules/apps/27-interchain-accounts/keeper/relay.go +++ b/modules/apps/27-interchain-accounts/keeper/relay.go @@ -11,12 +11,11 @@ import ( "github.com/cosmos/ibc-go/v2/modules/apps/27-interchain-accounts/types" clienttypes "github.com/cosmos/ibc-go/v2/modules/core/02-client/types" channeltypes "github.com/cosmos/ibc-go/v2/modules/core/04-channel/types" - host "github.com/cosmos/ibc-go/v2/modules/core/24-host" ) // TrySendTx takes in a transaction from a base application and attempts to send the packet // if the base application has the capability to send on the provided portID -func (k Keeper) TrySendTx(ctx sdk.Context, appCap *capabilitytypes.Capability, portID string, data interface{}) ([]byte, error) { +func (k Keeper) TrySendTx(ctx sdk.Context, chanCap *capabilitytypes.Capability, portID string, data interface{}) ([]byte, error) { // Check for the active channel activeChannelId, found := k.GetActiveChannel(ctx, portID) if !found { @@ -28,14 +27,10 @@ func (k Keeper) TrySendTx(ctx sdk.Context, appCap *capabilitytypes.Capability, p return nil, sdkerrors.Wrap(channeltypes.ErrChannelNotFound, activeChannelId) } - // if !k.AuthenticateCapability(ctx, appCap, types.AppCapabilityName(portID, activeChannelId)) { - // return nil, sdkerrors.Wrapf(types.ErrAppCapabilityNotFound, "could not authenticate provided capability for portID %s and channelID %s", portID, activeChannelId) - // } - destinationPort := sourceChannelEnd.GetCounterparty().GetPortID() destinationChannel := sourceChannelEnd.GetCounterparty().GetChannelID() - return k.createOutgoingPacket(ctx, portID, activeChannelId, destinationPort, destinationChannel, data) + return k.createOutgoingPacket(ctx, portID, activeChannelId, destinationPort, destinationChannel, chanCap, data) } func (k Keeper) createOutgoingPacket( @@ -44,6 +39,7 @@ func (k Keeper) createOutgoingPacket( sourceChannel, destinationPort, destinationChannel string, + chanCap *capabilitytypes.Capability, data interface{}, ) ([]byte, error) { if data == nil { @@ -55,11 +51,6 @@ func (k Keeper) createOutgoingPacket( return []byte{}, sdkerrors.Wrap(err, "invalid packet data or codec") } - channelCap, ok := k.scopedKeeper.GetCapability(ctx, host.ChannelCapabilityPath(sourcePort, sourceChannel)) - if !ok { - return []byte{}, sdkerrors.Wrap(channeltypes.ErrChannelCapabilityNotFound, "module does not own channel capability") - } - // get the next sequence sequence, found := k.channelKeeper.GetNextSequenceSend(ctx, sourcePort, sourceChannel) if !found { @@ -86,7 +77,7 @@ func (k Keeper) createOutgoingPacket( timeoutTimestamp, ) - if err := k.channelKeeper.SendPacket(ctx, channelCap, packet); err != nil { + if err := k.channelKeeper.SendPacket(ctx, chanCap, packet); err != nil { return nil, err } diff --git a/modules/apps/27-interchain-accounts/keeper/relay_test.go b/modules/apps/27-interchain-accounts/keeper/relay_test.go index 23b86a7fd36..90a207d0fa0 100644 --- a/modules/apps/27-interchain-accounts/keeper/relay_test.go +++ b/modules/apps/27-interchain-accounts/keeper/relay_test.go @@ -5,18 +5,21 @@ import ( sdk "github.com/cosmos/cosmos-sdk/types" banktypes "github.com/cosmos/cosmos-sdk/x/bank/types" + capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types" "github.com/cosmos/ibc-go/v2/modules/apps/27-interchain-accounts/types" clienttypes "github.com/cosmos/ibc-go/v2/modules/core/02-client/types" channeltypes "github.com/cosmos/ibc-go/v2/modules/core/04-channel/types" + host "github.com/cosmos/ibc-go/v2/modules/core/24-host" ibctesting "github.com/cosmos/ibc-go/v2/testing" ) func (suite *KeeperTestSuite) TestTrySendTx() { var ( - path *ibctesting.Path - msg interface{} - portID string + path *ibctesting.Path + msg interface{} + portID string + chanCap *capabilitytypes.Capability ) testCases := []struct { @@ -26,9 +29,6 @@ func (suite *KeeperTestSuite) TestTrySendTx() { }{ { "success", func() { - amount, _ := sdk.ParseCoinsNormalized("100stake") - interchainAccountAddr, _ := suite.chainB.GetSimApp().ICAKeeper.GetInterchainAccountAddress(suite.chainB.GetContext(), path.EndpointA.ChannelConfig.PortID) - msg = &banktypes.MsgSend{FromAddress: interchainAccountAddr, ToAddress: suite.chainB.SenderAccount.GetAddress().String(), Amount: amount} }, true, }, { @@ -47,28 +47,14 @@ func (suite *KeeperTestSuite) TestTrySendTx() { }, { "active channel not found", func() { - amount, _ := sdk.ParseCoinsNormalized("100stake") - interchainAccountAddr, _ := suite.chainB.GetSimApp().ICAKeeper.GetInterchainAccountAddress(suite.chainB.GetContext(), path.EndpointA.ChannelConfig.PortID) - msg = &banktypes.MsgSend{FromAddress: interchainAccountAddr, ToAddress: suite.chainB.SenderAccount.GetAddress().String(), Amount: amount} portID = "incorrect portID" }, false, }, { "channel does not exist", func() { - amount, _ := sdk.ParseCoinsNormalized("100stake") - interchainAccountAddr, _ := suite.chainB.GetSimApp().ICAKeeper.GetInterchainAccountAddress(suite.chainB.GetContext(), path.EndpointA.ChannelConfig.PortID) - msg = &banktypes.MsgSend{FromAddress: interchainAccountAddr, ToAddress: suite.chainB.SenderAccount.GetAddress().String(), Amount: amount} suite.chainA.GetSimApp().ICAKeeper.SetActiveChannel(suite.chainA.GetContext(), portID, "channel-100") }, false, }, - /* { - "could not authenticate app capability", func() { - amount, _ := sdk.ParseCoinsNormalized("100stake") - interchainAccountAddr, _ := suite.chainB.GetSimApp().ICAKeeper.GetInterchainAccountAddress(suite.chainB.GetContext(), path.EndpointA.ChannelConfig.PortID) - msg = &banktypes.MsgSend{FromAddress: interchainAccountAddr, ToAddress: suite.chainB.SenderAccount.GetAddress().String(), Amount: amount} - appCap = nil - }, false, - },*/ { "data is nil", func() { msg = nil @@ -79,6 +65,11 @@ func (suite *KeeperTestSuite) TestTrySendTx() { msg = "not an sdk message" }, false, }, + { + "invalid channel capability provided", func() { + chanCap = nil + }, false, + }, } for _, tc := range testCases { @@ -92,11 +83,20 @@ func (suite *KeeperTestSuite) TestTrySendTx() { err := suite.SetupICAPath(path, TestOwnerAddress) suite.Require().NoError(err) + // default setup portID = path.EndpointA.ChannelConfig.PortID + amount, _ := sdk.ParseCoinsNormalized("100stake") + interchainAccountAddr, _ := suite.chainB.GetSimApp().ICAKeeper.GetInterchainAccountAddress(suite.chainB.GetContext(), path.EndpointA.ChannelConfig.PortID) + msg = &banktypes.MsgSend{FromAddress: interchainAccountAddr, ToAddress: suite.chainB.SenderAccount.GetAddress().String(), Amount: amount} + + var ok bool + chanCap, ok = suite.chainA.GetSimApp().ScopedICAMockKeeper.GetCapability(path.EndpointA.Chain.GetContext(), host.ChannelCapabilityPath(portID, path.EndpointA.ChannelID)) + suite.Require().True(ok) + tc.malleate() - _, err = suite.chainA.GetSimApp().ICAKeeper.TrySendTx(suite.chainA.GetContext(), nil, portID, msg) + _, err = suite.chainA.GetSimApp().ICAKeeper.TrySendTx(suite.chainA.GetContext(), chanCap, portID, msg) if tc.expPass { suite.Require().NoError(err) diff --git a/testing/simapp/app.go b/testing/simapp/app.go index bafe3a6b1cd..6a68904ca19 100644 --- a/testing/simapp/app.go +++ b/testing/simapp/app.go @@ -196,6 +196,7 @@ type SimApp struct { ScopedTransferKeeper capabilitykeeper.ScopedKeeper ScopedICAKeeper capabilitykeeper.ScopedKeeper ScopedIBCMockKeeper capabilitykeeper.ScopedKeeper + ScopedICAMockKeeper capabilitykeeper.ScopedKeeper // the module manager mm *module.Manager @@ -267,6 +268,7 @@ func NewSimApp( // NOTE: the IBC mock keeper and application module is used only for testing core IBC. Do // note replicate if you do not need to test core IBC or light clients. scopedIBCMockKeeper := app.CapabilityKeeper.ScopeToModule(ibcmock.ModuleName) + scopedICAMockKeeper := app.CapabilityKeeper.ScopeToModule(ibcmock.ModuleName + icatypes.ModuleName) // seal capability keeper after scoping modules app.CapabilityKeeper.Seal() @@ -335,6 +337,7 @@ func NewSimApp( // NOTE: the IBC mock keeper and application module is used only for testing core IBC. Do // note replicate if you do not need to test core IBC or light clients. mockModule := ibcmock.NewAppModule(scopedIBCMockKeeper, &app.IBCKeeper.PortKeeper) + icaMockModule := ibcmock.NewAppModule(scopedICAMockKeeper, &app.IBCKeeper.PortKeeper) app.ICAKeeper = icakeeper.NewKeeper( appCodec, keys[icatypes.StoreKey], @@ -342,12 +345,13 @@ func NewSimApp( app.AccountKeeper, scopedICAKeeper, app.MsgServiceRouter(), ) icaModule := ica.NewAppModule(app.ICAKeeper) - icaIBCModule := ica.NewIBCModule(app.ICAKeeper, mockModule) + icaIBCModule := ica.NewIBCModule(app.ICAKeeper, icaMockModule) // Create static IBC router, add transfer route, then set and seal it ibcRouter := porttypes.NewRouter() ibcRouter.AddRoute(ibctransfertypes.ModuleName, transferModule) ibcRouter.AddRoute(icatypes.ModuleName, icaIBCModule) + ibcRouter.AddRoute(ibcmock.ModuleName+icatypes.ModuleName, icaIBCModule) // ica + mock stack route to ica ibcRouter.AddRoute(ibcmock.ModuleName, mockModule) app.IBCKeeper.SetRouter(ibcRouter) @@ -484,6 +488,7 @@ func NewSimApp( // NOTE: the IBC mock keeper and application module is used only for testing core IBC. Do // note replicate if you do not need to test core IBC or light clients. app.ScopedIBCMockKeeper = scopedIBCMockKeeper + app.ScopedICAMockKeeper = scopedICAMockKeeper return app } From a6066dabf82dcdea378d7c5a00f825657ae34974 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Tue, 12 Oct 2021 16:50:50 +0200 Subject: [PATCH 06/17] godoc self nits --- modules/apps/27-interchain-accounts/ibc_module.go | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/modules/apps/27-interchain-accounts/ibc_module.go b/modules/apps/27-interchain-accounts/ibc_module.go index 26ac40631a6..77dbc11e458 100644 --- a/modules/apps/27-interchain-accounts/ibc_module.go +++ b/modules/apps/27-interchain-accounts/ibc_module.go @@ -72,7 +72,11 @@ func (im IBCModule) OnChanOpenTry( return im.keeper.OnChanOpenTry(ctx, order, connectionHops, portID, channelID, chanCap, counterparty, version, counterpartyVersion) } -// OnChanOpenAck implements the IBCModule interface +// OnChanOpenAck implements the IBCModule interface. Interchain Accounts is +// implemented to act as middleware for connected authentication modules on +// the controller side. The connected modules may not change the portID or +// version. They will be allowed to perform custom logic without changing +// the parameters stored within a channel struct. // // Controller Chain func (im IBCModule) OnChanOpenAck( @@ -120,6 +124,8 @@ func (im IBCModule) OnChanCloseConfirm( } // OnRecvPacket implements the IBCModule interface +// +// Host Chain func (im IBCModule) OnRecvPacket( ctx sdk.Context, packet channeltypes.Packet, @@ -145,7 +151,9 @@ func (im IBCModule) OnRecvPacket( return ack } -// OnAcknowledgementPacket implements the IBCModule interface. +// OnAcknowledgementPacket implements the IBCModule interface +// +// Controller Chain func (im IBCModule) OnAcknowledgementPacket( ctx sdk.Context, packet channeltypes.Packet, @@ -157,6 +165,8 @@ func (im IBCModule) OnAcknowledgementPacket( } // OnTimeoutPacket implements the IBCModule interface +// +// Controller Chain func (im IBCModule) OnTimeoutPacket( ctx sdk.Context, packet channeltypes.Packet, From 70bf29b918eb0c608a14a5ce5240700b1d0c5297 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Tue, 12 Oct 2021 17:03:31 +0200 Subject: [PATCH 07/17] remove unnecessary error --- modules/apps/27-interchain-accounts/types/errors.go | 1 - 1 file changed, 1 deletion(-) diff --git a/modules/apps/27-interchain-accounts/types/errors.go b/modules/apps/27-interchain-accounts/types/errors.go index ed7a3831405..77787281d1a 100644 --- a/modules/apps/27-interchain-accounts/types/errors.go +++ b/modules/apps/27-interchain-accounts/types/errors.go @@ -17,5 +17,4 @@ var ( ErrInvalidVersion = sdkerrors.Register(ModuleName, 11, "invalid interchain accounts version") ErrInvalidAccountAddress = sdkerrors.Register(ModuleName, 12, "invalid account address") ErrUnsupported = sdkerrors.Register(ModuleName, 13, "interchain account does not support this action") - ErrAppCapabilityNotFound = sdkerrors.Register(ModuleName, 14, "app capability not found") ) From 205d6f0b66c09a9faf3e264785b1084e78525784 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Mon, 1 Nov 2021 14:54:54 +0100 Subject: [PATCH 08/17] update comment --- modules/apps/27-interchain-accounts/keeper/relay.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/apps/27-interchain-accounts/keeper/relay.go b/modules/apps/27-interchain-accounts/keeper/relay.go index 96fb231a9eb..cde05de66fa 100644 --- a/modules/apps/27-interchain-accounts/keeper/relay.go +++ b/modules/apps/27-interchain-accounts/keeper/relay.go @@ -10,7 +10,7 @@ import ( channeltypes "github.com/cosmos/ibc-go/v2/modules/core/04-channel/types" ) -// TrySendTx takes in a transaction from a base application and attempts to send the packet +// TrySendTx takes in a transaction from a authentication module and attempts to send the packet // if the base application has the capability to send on the provided portID func (k Keeper) TrySendTx(ctx sdk.Context, chanCap *capabilitytypes.Capability, portID string, icaPacketData types.InterchainAccountPacketData) (uint64, error) { // Check for the active channel From 7baafe0aa259dde0dd7e09870b2b198cb2c48d53 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Thu, 4 Nov 2021 11:31:32 +0100 Subject: [PATCH 09/17] fix testing build --- .../27-interchain-accounts/ibc_module_test.go | 18 +++++++++++++----- testing/simapp/app.go | 19 ++++++++++++++++--- testing/values.go | 3 +++ 3 files changed, 32 insertions(+), 8 deletions(-) diff --git a/modules/apps/27-interchain-accounts/ibc_module_test.go b/modules/apps/27-interchain-accounts/ibc_module_test.go index 27da35a290d..6077bc476f6 100644 --- a/modules/apps/27-interchain-accounts/ibc_module_test.go +++ b/modules/apps/27-interchain-accounts/ibc_module_test.go @@ -120,8 +120,10 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenInit() { }, { "ICA auth module callback fails", func() { - mockModule := suite.chainA.GetSimApp().GetMockModule() - mockModule.IBCApp.OnChanOpenInit = func(ctx sdk.Context, order channeltypes.Order, connectionHops []string, + icaAuthModule, found := suite.chainA.GetSimApp().GetMockModule(ibctesting.ICAAuthModuleName) + suite.Require().True(found) + + icaAuthModule.IBCApp.OnChanOpenInit = func(ctx sdk.Context, order channeltypes.Order, connectionHops []string, portID, channelID string, chanCap *capabilitytypes.Capability, counterparty channeltypes.Counterparty, version string, ) error { @@ -199,7 +201,9 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenTry() { { "success: ICA auth module callback returns error", func() { // mock module callback should not be called on host side - mockModule := suite.chainB.GetSimApp().GetMockModule() + mockModule, found := suite.chainB.GetSimApp().GetMockModule(ibctesting.ICAAuthModuleName) + suite.Require().True(found) + mockModule.IBCApp.OnChanOpenTry = func(ctx sdk.Context, order channeltypes.Order, connectionHops []string, portID, channelID string, chanCap *capabilitytypes.Capability, counterparty channeltypes.Counterparty, version, counterpartyVersion string, @@ -283,7 +287,9 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenAck() { }, { "ICA auth module callback fails", func() { - mockModule := suite.chainA.GetSimApp().GetMockModule() + mockModule, found := suite.chainA.GetSimApp().GetMockModule(ibctesting.ICAAuthModuleName) + suite.Require().True(found) + mockModule.IBCApp.OnChanOpenAck = func( ctx sdk.Context, portID, channelID string, counterpartyVersion string, ) error { @@ -343,7 +349,9 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenConfirm() { { "success: ICA auth module callback returns error", func() { // mock module callback should not be called on host side - mockModule := suite.chainB.GetSimApp().GetMockModule() + mockModule, found := suite.chainB.GetSimApp().GetMockModule(ibctesting.ICAAuthModuleName) + suite.Require().True(found) + mockModule.IBCApp.OnChanOpenConfirm = func( ctx sdk.Context, portID, channelID string, ) error { diff --git a/testing/simapp/app.go b/testing/simapp/app.go index 4f43d90d9ac..bdb515f7632 100644 --- a/testing/simapp/app.go +++ b/testing/simapp/app.go @@ -109,6 +109,9 @@ var ( // DefaultNodeHome default home directories for the application daemon DefaultNodeHome string + // ICAAuthModuleName is the module name used in routing to the ICA Auth Module + ICAAuthModuleName = ibcmock.ModuleName + icatypes.ModuleName + // ModuleBasics defines the module BasicManager is in charge of setting up basic, // non-dependant module elements, such as codec registration // and genesis verification. @@ -354,7 +357,7 @@ func NewSimApp( // Create static IBC router, add transfer route, then set and seal it ibcRouter := porttypes.NewRouter() ibcRouter.AddRoute(icatypes.ModuleName, icaIBCModule). - AddRoute(ibcmock.ModuleName+icatypes.ModuleName, icaIBCModule). // ica + mock stack route to ica + AddRoute(ICAAuthModuleName, icaIBCModule). // ica + mock stack route to ica AddRoute(ibctransfertypes.ModuleName, transferIBCModule). AddRoute(ibcmock.ModuleName, mockIBCModule) app.IBCKeeper.SetRouter(ibcRouter) @@ -608,8 +611,18 @@ func (app *SimApp) GetScopedIBCKeeper() capabilitykeeper.ScopedKeeper { } // GetMockModule returns the mock module in the testing application -func (app *SimApp) GetMockModule() ibcmock.AppModule { - return app.mm.Modules[ibcmock.ModuleName].(ibcmock.AppModule) +func (app *SimApp) GetMockModule(moduleName string) (ibcmock.IBCModule, bool) { + ibcModule, found := app.IBCKeeper.Router.GetRoute(moduleName) + if !found { + return ibcmock.IBCModule{}, false + } + + mockModule, ok := ibcModule.(ibcmock.IBCModule) + if !ok { + return ibcmock.IBCModule{}, false + } + + return mockModule, true } // GetTxConfig implements the TestingApp interface. diff --git a/testing/values.go b/testing/values.go index 9994040423e..df16c40ae46 100644 --- a/testing/values.go +++ b/testing/values.go @@ -16,6 +16,7 @@ import ( commitmenttypes "github.com/cosmos/ibc-go/v2/modules/core/23-commitment/types" ibctmtypes "github.com/cosmos/ibc-go/v2/modules/light-clients/07-tendermint/types" "github.com/cosmos/ibc-go/v2/testing/mock" + "github.com/cosmos/ibc-go/v2/testing/simapp" ) const ( @@ -58,6 +59,8 @@ var ( MockRecvCanaryCapabilityName = mock.MockRecvCanaryCapabilityName prefix = commitmenttypes.NewMerklePrefix([]byte("ibc")) + + ICAAuthModuleName = simapp.ICAAuthModuleName ) func GetMockRecvCanaryCapabilityName(packet channeltypes.Packet) string { From b166880a015b088636946fa9932474155c71d120 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Thu, 4 Nov 2021 11:35:40 +0100 Subject: [PATCH 10/17] split channel keeper interface splits ChannelKeeper interface into ics4Wrapper and core ChannelKeeper --- modules/apps/27-interchain-accounts/keeper/keeper.go | 4 +++- modules/apps/27-interchain-accounts/keeper/relay.go | 2 +- .../apps/27-interchain-accounts/types/expected_keepers.go | 6 +++++- testing/simapp/app.go | 1 + 4 files changed, 10 insertions(+), 3 deletions(-) diff --git a/modules/apps/27-interchain-accounts/keeper/keeper.go b/modules/apps/27-interchain-accounts/keeper/keeper.go index 467fbc519dc..c730882207d 100644 --- a/modules/apps/27-interchain-accounts/keeper/keeper.go +++ b/modules/apps/27-interchain-accounts/keeper/keeper.go @@ -22,6 +22,7 @@ type Keeper struct { storeKey sdk.StoreKey cdc codec.BinaryCodec + ics4Wrapper types.ICS4Wrapper channelKeeper types.ChannelKeeper portKeeper types.PortKeeper accountKeeper types.AccountKeeper @@ -34,7 +35,7 @@ type Keeper struct { // NewKeeper creates a new interchain account Keeper instance func NewKeeper( cdc codec.BinaryCodec, key sdk.StoreKey, - channelKeeper types.ChannelKeeper, portKeeper types.PortKeeper, + ics4Wrapper types.ICS4Wrapper, channelKeeper types.ChannelKeeper, portKeeper types.PortKeeper, accountKeeper types.AccountKeeper, scopedKeeper capabilitykeeper.ScopedKeeper, msgRouter *baseapp.MsgServiceRouter, ) Keeper { @@ -46,6 +47,7 @@ func NewKeeper( return Keeper{ storeKey: key, cdc: cdc, + ics4Wrapper: ics4Wrapper, channelKeeper: channelKeeper, portKeeper: portKeeper, accountKeeper: accountKeeper, diff --git a/modules/apps/27-interchain-accounts/keeper/relay.go b/modules/apps/27-interchain-accounts/keeper/relay.go index 67af6315d73..fc033f5fb9e 100644 --- a/modules/apps/27-interchain-accounts/keeper/relay.go +++ b/modules/apps/27-interchain-accounts/keeper/relay.go @@ -64,7 +64,7 @@ func (k Keeper) createOutgoingPacket( timeoutTimestamp, ) - if err := k.channelKeeper.SendPacket(ctx, chanCap, packet); err != nil { + if err := k.ics4Wrapper.SendPacket(ctx, chanCap, packet); err != nil { return 0, err } diff --git a/modules/apps/27-interchain-accounts/types/expected_keepers.go b/modules/apps/27-interchain-accounts/types/expected_keepers.go index 2b08a9bcfe9..558a38c4033 100644 --- a/modules/apps/27-interchain-accounts/types/expected_keepers.go +++ b/modules/apps/27-interchain-accounts/types/expected_keepers.go @@ -18,11 +18,15 @@ type AccountKeeper interface { GetModuleAddress(name string) sdk.AccAddress } +// ICS4Wrapper defines the expected ICS4Wrapper for middleware +type ICS4Wrapper interface { + SendPacket(ctx sdk.Context, channelCap *capabilitytypes.Capability, packet ibcexported.PacketI) error +} + // ChannelKeeper defines the expected IBC channel keeper type ChannelKeeper interface { GetChannel(ctx sdk.Context, srcPort, srcChan string) (channel channeltypes.Channel, found bool) GetNextSequenceSend(ctx sdk.Context, portID, channelID string) (uint64, bool) - SendPacket(ctx sdk.Context, channelCap *capabilitytypes.Capability, packet ibcexported.PacketI) error CounterpartyHops(ctx sdk.Context, channel channeltypes.Channel) ([]string, bool) } diff --git a/testing/simapp/app.go b/testing/simapp/app.go index bdb515f7632..1cd7a17c9b5 100644 --- a/testing/simapp/app.go +++ b/testing/simapp/app.go @@ -345,6 +345,7 @@ func NewSimApp( app.ICAKeeper = icakeeper.NewKeeper( appCodec, keys[icatypes.StoreKey], + app.IBCKeeper.ChannelKeeper, // may be replaced with middleware such as ics29 fee app.IBCKeeper.ChannelKeeper, &app.IBCKeeper.PortKeeper, app.AccountKeeper, scopedICAKeeper, app.MsgServiceRouter(), ) From 45bee5115954ad22bf47f783c6175bfdf68e40b0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Thu, 4 Nov 2021 12:01:42 +0100 Subject: [PATCH 11/17] fix tests --- .../27-interchain-accounts/ibc_module_test.go | 35 ++++++++----------- testing/simapp/app.go | 28 +++++---------- testing/values.go | 3 -- 3 files changed, 23 insertions(+), 43 deletions(-) diff --git a/modules/apps/27-interchain-accounts/ibc_module_test.go b/modules/apps/27-interchain-accounts/ibc_module_test.go index 6077bc476f6..9fe11ca36d4 100644 --- a/modules/apps/27-interchain-accounts/ibc_module_test.go +++ b/modules/apps/27-interchain-accounts/ibc_module_test.go @@ -120,16 +120,12 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenInit() { }, { "ICA auth module callback fails", func() { - icaAuthModule, found := suite.chainA.GetSimApp().GetMockModule(ibctesting.ICAAuthModuleName) - suite.Require().True(found) - - icaAuthModule.IBCApp.OnChanOpenInit = func(ctx sdk.Context, order channeltypes.Order, connectionHops []string, + suite.chainA.GetSimApp().ICAAuthModule.IBCApp.OnChanOpenInit = func(ctx sdk.Context, order channeltypes.Order, connectionHops []string, portID, channelID string, chanCap *capabilitytypes.Capability, counterparty channeltypes.Counterparty, version string, ) error { return fmt.Errorf("mock ica auth fails") } - }, false, }, } @@ -147,7 +143,9 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenInit() { 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)) + path.EndpointA.ChannelConfig.PortID = portID + path.EndpointA.ChannelID = ibctesting.FirstChannelID // default values counterparty := channeltypes.NewCounterparty(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) @@ -161,6 +159,9 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenInit() { tc.malleate() + // ensure channel on chainA is set in state + suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper.SetChannel(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, *channel) + module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID) suite.Require().NoError(err) @@ -201,10 +202,7 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenTry() { { "success: ICA auth module callback returns error", func() { // mock module callback should not be called on host side - mockModule, found := suite.chainB.GetSimApp().GetMockModule(ibctesting.ICAAuthModuleName) - suite.Require().True(found) - - mockModule.IBCApp.OnChanOpenTry = func(ctx sdk.Context, order channeltypes.Order, connectionHops []string, + suite.chainB.GetSimApp().ICAAuthModule.IBCApp.OnChanOpenTry = func(ctx sdk.Context, order channeltypes.Order, connectionHops []string, portID, channelID string, chanCap *capabilitytypes.Capability, counterparty channeltypes.Counterparty, version, counterpartyVersion string, ) error { @@ -226,11 +224,12 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenTry() { suite.Run(tc.name, func() { suite.SetupTest() // reset path := NewICAPath(suite.chainA, suite.chainB) - counterpartyVersion := TestVersion + counterpartyVersion := types.VersionPrefix suite.coordinator.SetupConnections(path) err := InitInterchainAccount(path.EndpointA, TestOwnerAddress) suite.Require().NoError(err) + path.EndpointB.ChannelID = ibctesting.FirstChannelID // default values counterparty := channeltypes.NewCounterparty(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) @@ -244,6 +243,9 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenTry() { tc.malleate() + // ensure channel on chainB is set in state + suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.SetChannel(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, *channel) + module, _, err := suite.chainB.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID) suite.Require().NoError(err) @@ -287,10 +289,7 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenAck() { }, { "ICA auth module callback fails", func() { - mockModule, found := suite.chainA.GetSimApp().GetMockModule(ibctesting.ICAAuthModuleName) - suite.Require().True(found) - - mockModule.IBCApp.OnChanOpenAck = func( + suite.chainA.GetSimApp().ICAAuthModule.IBCApp.OnChanOpenAck = func( ctx sdk.Context, portID, channelID string, counterpartyVersion string, ) error { return fmt.Errorf("mock ica auth fails") @@ -303,10 +302,9 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenAck() { tc := tc suite.Run(tc.name, func() { - suite.SetupTest() // reset path := NewICAPath(suite.chainA, suite.chainB) - counterpartyVersion = types.VersionPrefix + counterpartyVersion = TestVersion suite.coordinator.SetupConnections(path) err := InitInterchainAccount(path.EndpointA, TestOwnerAddress) @@ -349,10 +347,7 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenConfirm() { { "success: ICA auth module callback returns error", func() { // mock module callback should not be called on host side - mockModule, found := suite.chainB.GetSimApp().GetMockModule(ibctesting.ICAAuthModuleName) - suite.Require().True(found) - - mockModule.IBCApp.OnChanOpenConfirm = func( + suite.chainB.GetSimApp().ICAAuthModule.IBCApp.OnChanOpenConfirm = func( ctx sdk.Context, portID, channelID string, ) error { return fmt.Errorf("mock ica auth fails") diff --git a/testing/simapp/app.go b/testing/simapp/app.go index 1cd7a17c9b5..4a7b73e9169 100644 --- a/testing/simapp/app.go +++ b/testing/simapp/app.go @@ -109,9 +109,6 @@ var ( // DefaultNodeHome default home directories for the application daemon DefaultNodeHome string - // ICAAuthModuleName is the module name used in routing to the ICA Auth Module - ICAAuthModuleName = ibcmock.ModuleName + icatypes.ModuleName - // ModuleBasics defines the module BasicManager is in charge of setting up basic, // non-dependant module elements, such as codec registration // and genesis verification. @@ -201,6 +198,10 @@ type SimApp struct { ScopedIBCMockKeeper capabilitykeeper.ScopedKeeper ScopedICAMockKeeper capabilitykeeper.ScopedKeeper + // make IBC modules public for test purposes + // these modules are never directly routed to by the IBC Router + ICAAuthModule ibcmock.IBCModule + // the module manager mm *module.Manager @@ -352,13 +353,15 @@ func NewSimApp( icaModule := ica.NewAppModule(app.ICAKeeper) // initialize ICA module with mock module as the authentication module on the controller side - icaAuthModule := ibcmock.NewIBCModule(&ibcmock.MockIBCApp{}, scopedIBCMockKeeper) + icaAuthModule := ibcmock.NewIBCModule(&ibcmock.MockIBCApp{}, scopedICAMockKeeper) + app.ICAAuthModule = icaAuthModule + icaIBCModule := ica.NewIBCModule(app.ICAKeeper, icaAuthModule) // Create static IBC router, add transfer route, then set and seal it ibcRouter := porttypes.NewRouter() ibcRouter.AddRoute(icatypes.ModuleName, icaIBCModule). - AddRoute(ICAAuthModuleName, icaIBCModule). // ica + mock stack route to ica + AddRoute(ibcmock.ModuleName+icatypes.ModuleName, icaIBCModule). // ica with mock auth module stack route to ica (top level of middleware stack) AddRoute(ibctransfertypes.ModuleName, transferIBCModule). AddRoute(ibcmock.ModuleName, mockIBCModule) app.IBCKeeper.SetRouter(ibcRouter) @@ -611,21 +614,6 @@ func (app *SimApp) GetScopedIBCKeeper() capabilitykeeper.ScopedKeeper { return app.ScopedIBCKeeper } -// GetMockModule returns the mock module in the testing application -func (app *SimApp) GetMockModule(moduleName string) (ibcmock.IBCModule, bool) { - ibcModule, found := app.IBCKeeper.Router.GetRoute(moduleName) - if !found { - return ibcmock.IBCModule{}, false - } - - mockModule, ok := ibcModule.(ibcmock.IBCModule) - if !ok { - return ibcmock.IBCModule{}, false - } - - return mockModule, true -} - // GetTxConfig implements the TestingApp interface. func (app *SimApp) GetTxConfig() client.TxConfig { return MakeTestEncodingConfig().TxConfig diff --git a/testing/values.go b/testing/values.go index df16c40ae46..9994040423e 100644 --- a/testing/values.go +++ b/testing/values.go @@ -16,7 +16,6 @@ import ( commitmenttypes "github.com/cosmos/ibc-go/v2/modules/core/23-commitment/types" ibctmtypes "github.com/cosmos/ibc-go/v2/modules/light-clients/07-tendermint/types" "github.com/cosmos/ibc-go/v2/testing/mock" - "github.com/cosmos/ibc-go/v2/testing/simapp" ) const ( @@ -59,8 +58,6 @@ var ( MockRecvCanaryCapabilityName = mock.MockRecvCanaryCapabilityName prefix = commitmenttypes.NewMerklePrefix([]byte("ibc")) - - ICAAuthModuleName = simapp.ICAAuthModuleName ) func GetMockRecvCanaryCapabilityName(packet channeltypes.Packet) string { From 816293bf32976726d57cc095b4a728f27968fad4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Thu, 4 Nov 2021 12:10:12 +0100 Subject: [PATCH 12/17] remove comments --- modules/apps/27-interchain-accounts/module.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/modules/apps/27-interchain-accounts/module.go b/modules/apps/27-interchain-accounts/module.go index edeb433640f..8e83463aef9 100644 --- a/modules/apps/27-interchain-accounts/module.go +++ b/modules/apps/27-interchain-accounts/module.go @@ -24,11 +24,6 @@ var ( _ module.AppModule = AppModule{} _ porttypes.IBCModule = IBCModule{} _ module.AppModuleBasic = AppModuleBasic{} - - // Middleware must implement types.ChannelKeeper and types.PortKeeper expected interfaces - // so that it can wrap IBC channel and port logic for underlying application. -// _ types.ChannelKeeper = Keeper{} -// _ types.PortKeeper = Keeper{} ) type AppModuleBasic struct{} From 9a0e7de87db4cf03522d128afcba2dde9e00bf0b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Thu, 4 Nov 2021 12:21:17 +0100 Subject: [PATCH 13/17] add callback for timeouts --- modules/apps/27-interchain-accounts/ibc_module.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/modules/apps/27-interchain-accounts/ibc_module.go b/modules/apps/27-interchain-accounts/ibc_module.go index 77dbc11e458..105b2068ded 100644 --- a/modules/apps/27-interchain-accounts/ibc_module.go +++ b/modules/apps/27-interchain-accounts/ibc_module.go @@ -172,7 +172,11 @@ func (im IBCModule) OnTimeoutPacket( packet channeltypes.Packet, relayer sdk.AccAddress, ) error { - return im.keeper.OnTimeoutPacket(ctx, packet) + if err := im.keeper.OnTimeoutPacket(ctx, packet); err != nil { + return err + } + + return im.app.OnTimeoutPacket(ctx, packet, relayer) } // NegotiateAppVersion implements the IBCModule interface From b2db520e522116e66f8875cf31a252f187774453 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?colin=20axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Fri, 5 Nov 2021 10:42:51 +0100 Subject: [PATCH 14/17] Apply suggestions from code review Co-authored-by: Sean King Co-authored-by: Damian Nolan --- modules/apps/27-interchain-accounts/ibc_module.go | 2 +- modules/apps/27-interchain-accounts/keeper/relay.go | 2 +- testing/simapp/app.go | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/modules/apps/27-interchain-accounts/ibc_module.go b/modules/apps/27-interchain-accounts/ibc_module.go index 105b2068ded..618f0e40686 100644 --- a/modules/apps/27-interchain-accounts/ibc_module.go +++ b/modules/apps/27-interchain-accounts/ibc_module.go @@ -31,7 +31,7 @@ func NewIBCModule(k keeper.Keeper, app porttypes.IBCModule) IBCModule { // OnChanOpenInit implements the IBCModule interface. Interchain Accounts is // implemented to act as middleware for connected authentication modules on -// the controller side. The connected modules may not change the portID or +// the controller side. The connected modules may not change the controller side portID or // version. They will be allowed to perform custom logic without changing // the parameters stored within a channel struct. // diff --git a/modules/apps/27-interchain-accounts/keeper/relay.go b/modules/apps/27-interchain-accounts/keeper/relay.go index fc033f5fb9e..681433e03ee 100644 --- a/modules/apps/27-interchain-accounts/keeper/relay.go +++ b/modules/apps/27-interchain-accounts/keeper/relay.go @@ -10,7 +10,7 @@ import ( channeltypes "github.com/cosmos/ibc-go/v2/modules/core/04-channel/types" ) -// TrySendTx takes in a transaction from a authentication module and attempts to send the packet +// TrySendTx takes in a transaction from an authentication module and attempts to send the packet // if the base application has the capability to send on the provided portID func (k Keeper) TrySendTx(ctx sdk.Context, chanCap *capabilitytypes.Capability, portID string, icaPacketData types.InterchainAccountPacketData) (uint64, error) { // Check for the active channel diff --git a/testing/simapp/app.go b/testing/simapp/app.go index 4a7b73e9169..3050e6436d6 100644 --- a/testing/simapp/app.go +++ b/testing/simapp/app.go @@ -270,7 +270,7 @@ func NewSimApp( scopedICAKeeper := app.CapabilityKeeper.ScopeToModule(icatypes.ModuleName) // NOTE: the IBC mock keeper and application module is used only for testing core IBC. Do - // note replicate if you do not need to test core IBC or light clients. + // not replicate if you do not need to test core IBC or light clients. scopedIBCMockKeeper := app.CapabilityKeeper.ScopeToModule(ibcmock.ModuleName) scopedICAMockKeeper := app.CapabilityKeeper.ScopeToModule(ibcmock.ModuleName + icatypes.ModuleName) @@ -340,7 +340,7 @@ func NewSimApp( transferIBCModule := transfer.NewIBCModule(app.TransferKeeper) // NOTE: the IBC mock keeper and application module is used only for testing core IBC. Do - // note replicate if you do not need to test core IBC or light clients. + // not replicate if you do not need to test core IBC or light clients. mockModule := ibcmock.NewAppModule(scopedIBCMockKeeper, &app.IBCKeeper.PortKeeper) mockIBCModule := ibcmock.NewIBCModule(&ibcmock.MockIBCApp{}, scopedIBCMockKeeper) From c4a4aeb3903a9b4efd3d045c47fe9f4ae80966a6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Fri, 5 Nov 2021 10:53:06 +0100 Subject: [PATCH 15/17] fix test and update testing README apply test fix pointed out by Sean. Update testing README to reflect how to test with the mock module for middleware --- .../27-interchain-accounts/ibc_module_test.go | 2 +- testing/README.md | 16 +++++++++++++++- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/modules/apps/27-interchain-accounts/ibc_module_test.go b/modules/apps/27-interchain-accounts/ibc_module_test.go index 9fe11ca36d4..006afa70c6c 100644 --- a/modules/apps/27-interchain-accounts/ibc_module_test.go +++ b/modules/apps/27-interchain-accounts/ibc_module_test.go @@ -165,7 +165,7 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenInit() { module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID) suite.Require().NoError(err) - chanCap, err := suite.chainA.App.GetScopedIBCKeeper().NewCapability(suite.chainA.GetContext(), host.ChannelCapabilityPath(ibctesting.TransferPort, path.EndpointA.ChannelID)) + chanCap, err := suite.chainA.App.GetScopedIBCKeeper().NewCapability(suite.chainA.GetContext(), host.ChannelCapabilityPath(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)) suite.Require().NoError(err) cbs, ok := suite.chainA.App.GetIBCKeeper().Router.GetRoute(module) diff --git a/testing/README.md b/testing/README.md index f7a71bb1348..189f8e8c6cc 100644 --- a/testing/README.md +++ b/testing/README.md @@ -294,7 +294,7 @@ When writing IBC applications acting as middleware, it might be desirable to tes This can be done by wiring a middleware stack in the app.go file using existing applications as middleware and IBC base applications. The mock module may also be leveraged to act as a base application in the instance that such an application is not available for testing or causes dependency concerns. -The mock module contains a `MockIBCApp`. This struct contains a function field for every IBC App Module callback. +The mock IBC module contains a `MockIBCApp`. This struct contains a function field for every IBC App Module callback. Each of these functions can be individually set to mock expected behaviour of a base application. For example, if one wanted to test that the base application cannot affect the outcome of the `OnChanOpenTry` callback, the mock module base application callback could be updated as such: @@ -303,3 +303,17 @@ For example, if one wanted to test that the base application cannot affect the o return fmt.Errorf("mock base app must not be called for OnChanOpenTry") } ``` + +Using a mock module as a base application in a middleware stack may require adding the module to your `SimApp`. +This is because IBC will route to the top level IBC module of a middleware stack, so a module which never +sits at the top of middleware stack will need to be accessed via a public field in `SimApp` + +This might look like: +```go + suite.chainA.GetSimApp().ICAAuthModule.IBCApp.OnChanOpenInit = func(ctx sdk.Context, order channeltypes.Order, connectionHops []string, + portID, channelID string, chanCap *capabilitytypes.Capability, + counterparty channeltypes.Counterparty, version string, + ) error { + return fmt.Errorf("mock ica auth fails") + } +``` From 46f30de7985b01c88be6f71a351c770b915efe28 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Fri, 5 Nov 2021 11:09:47 +0100 Subject: [PATCH 16/17] add OnRecvPacket test Add test cases for OnRecvPacket, reused structure in relay_test.go --- .../apps/27-interchain-accounts/ibc_module.go | 8 -- .../27-interchain-accounts/ibc_module_test.go | 87 +++++++++++++++++++ 2 files changed, 87 insertions(+), 8 deletions(-) diff --git a/modules/apps/27-interchain-accounts/ibc_module.go b/modules/apps/27-interchain-accounts/ibc_module.go index 105b2068ded..88600bc4029 100644 --- a/modules/apps/27-interchain-accounts/ibc_module.go +++ b/modules/apps/27-interchain-accounts/ibc_module.go @@ -1,14 +1,11 @@ package interchain_accounts import ( - "fmt" - sdk "github.com/cosmos/cosmos-sdk/types" sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types" "github.com/cosmos/ibc-go/v2/modules/apps/27-interchain-accounts/keeper" - "github.com/cosmos/ibc-go/v2/modules/apps/27-interchain-accounts/types" channeltypes "github.com/cosmos/ibc-go/v2/modules/core/04-channel/types" porttypes "github.com/cosmos/ibc-go/v2/modules/core/05-port/types" ibcexported "github.com/cosmos/ibc-go/v2/modules/core/exported" @@ -133,11 +130,6 @@ func (im IBCModule) OnRecvPacket( ) ibcexported.Acknowledgement { ack := channeltypes.NewResultAcknowledgement([]byte{byte(1)}) - var data types.InterchainAccountPacketData - if err := types.ModuleCdc.UnmarshalJSON(packet.GetData(), &data); err != nil { - ack = channeltypes.NewErrorAcknowledgement(fmt.Sprintf("cannot unmarshal ICS-27 interchain account packet data: %s", err.Error())) - } - // only attempt the application logic if the packet data // was successfully decoded if ack.Success() { diff --git a/modules/apps/27-interchain-accounts/ibc_module_test.go b/modules/apps/27-interchain-accounts/ibc_module_test.go index 006afa70c6c..fa1c3756e6d 100644 --- a/modules/apps/27-interchain-accounts/ibc_module_test.go +++ b/modules/apps/27-interchain-accounts/ibc_module_test.go @@ -5,13 +5,16 @@ import ( "testing" sdk "github.com/cosmos/cosmos-sdk/types" + banktypes "github.com/cosmos/cosmos-sdk/x/bank/types" capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types" "github.com/stretchr/testify/suite" "github.com/tendermint/tendermint/crypto" "github.com/cosmos/ibc-go/v2/modules/apps/27-interchain-accounts/types" + clienttypes "github.com/cosmos/ibc-go/v2/modules/core/02-client/types" channeltypes "github.com/cosmos/ibc-go/v2/modules/core/04-channel/types" host "github.com/cosmos/ibc-go/v2/modules/core/24-host" + "github.com/cosmos/ibc-go/v2/modules/core/exported" ibctesting "github.com/cosmos/ibc-go/v2/testing" ) @@ -395,6 +398,90 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenConfirm() { } +func (suite *InterchainAccountsTestSuite) TestOnRecvPacket() { + var ( + packetData []byte + ) + testCases := []struct { + name string + malleate func() + expAckSuccess bool + }{ + { + "success", func() {}, true, + }, + { + "success with ICA auth module callback failure", func() { + suite.chainB.GetSimApp().ICAAuthModule.IBCApp.OnRecvPacket = func( + ctx sdk.Context, packet channeltypes.Packet, relayer sdk.AccAddress, + ) exported.Acknowledgement { + return channeltypes.NewErrorAcknowledgement("failed OnRecvPacket mock callback") + } + }, true, + }, + { + "ICA OnRecvPacket fails - cannot unmarshal packet data", func() { + packetData = []byte("invalid data") + }, false, + }, + } + + for _, tc := range testCases { + tc := tc + + suite.Run(tc.name, func() { + suite.SetupTest() // reset + + path := NewICAPath(suite.chainA, suite.chainB) + suite.coordinator.SetupConnections(path) + err := SetupICAPath(path, TestOwnerAddress) + suite.Require().NoError(err) + + // send 100stake to interchain account wallet + amount, _ := sdk.ParseCoinsNormalized("100stake") + interchainAccountAddr, _ := suite.chainB.GetSimApp().ICAKeeper.GetInterchainAccountAddress(suite.chainB.GetContext(), path.EndpointA.ChannelConfig.PortID) + bankMsg := &banktypes.MsgSend{FromAddress: suite.chainB.SenderAccount.GetAddress().String(), ToAddress: interchainAccountAddr, Amount: amount} + + _, err = suite.chainB.SendMsgs(bankMsg) + suite.Require().NoError(err) + + // build packet data + msg := &banktypes.MsgSend{ + FromAddress: interchainAccountAddr, + ToAddress: suite.chainB.SenderAccount.GetAddress().String(), + Amount: amount, + } + data, err := types.SerializeCosmosTx(suite.chainA.Codec, []sdk.Msg{msg}) + suite.Require().NoError(err) + + icaPacketData := types.InterchainAccountPacketData{ + Type: types.EXECUTE_TX, + Data: data, + } + packetData = icaPacketData.GetBytes() + + // malleate packetData for test cases + tc.malleate() + + seq := uint64(1) + packet := channeltypes.NewPacket(packetData, seq, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, clienttypes.NewHeight(0, 100), 0) + + tc.malleate() + + module, _, err := suite.chainB.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID) + suite.Require().NoError(err) + + cbs, ok := suite.chainB.App.GetIBCKeeper().Router.GetRoute(module) + suite.Require().True(ok) + + ack := cbs.OnRecvPacket(suite.chainB.GetContext(), packet, nil) + suite.Require().Equal(tc.expAckSuccess, ack.Success()) + + }) + } + +} + func (suite *InterchainAccountsTestSuite) TestNegotiateAppVersion() { suite.SetupTest() path := NewICAPath(suite.chainA, suite.chainB) From 62dd3d3ac186d2f6002b3dbac62bc4fdeec78404 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Fri, 5 Nov 2021 12:03:05 +0100 Subject: [PATCH 17/17] add failing test case for NegotiateAppVersion --- .../27-interchain-accounts/ibc_module_test.go | 69 ++++++++++++++----- 1 file changed, 50 insertions(+), 19 deletions(-) diff --git a/modules/apps/27-interchain-accounts/ibc_module_test.go b/modules/apps/27-interchain-accounts/ibc_module_test.go index fa1c3756e6d..ca010b6ede1 100644 --- a/modules/apps/27-interchain-accounts/ibc_module_test.go +++ b/modules/apps/27-interchain-accounts/ibc_module_test.go @@ -183,10 +183,8 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenInit() { } else { suite.Require().Error(err) } - }) } - } func (suite *InterchainAccountsTestSuite) TestOnChanOpenTry() { @@ -483,26 +481,59 @@ func (suite *InterchainAccountsTestSuite) TestOnRecvPacket() { } func (suite *InterchainAccountsTestSuite) TestNegotiateAppVersion() { - suite.SetupTest() - path := NewICAPath(suite.chainA, suite.chainB) - suite.coordinator.SetupConnections(path) + var ( + proposedVersion string + ) + testCases := []struct { + name string + malleate func() + expPass bool + }{ + { + "success", func() {}, true, + }, + { + "invalid proposed version", func() { + proposedVersion = "invalid version" + }, false, + }, + } - module, _, err := suite.chainA.GetSimApp().GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), types.PortID) - suite.Require().NoError(err) + for _, tc := range testCases { + tc := tc - cbs, ok := suite.chainA.GetSimApp().GetIBCKeeper().Router.GetRoute(module) - suite.Require().True(ok) + suite.Run(tc.name, func() { + suite.SetupTest() + path := NewICAPath(suite.chainA, suite.chainB) + suite.coordinator.SetupConnections(path) - counterpartyPortID, err := types.GeneratePortID(TestOwnerAddress, path.EndpointA.ConnectionID, path.EndpointB.ConnectionID) - suite.Require().NoError(err) + module, _, err := suite.chainA.GetSimApp().GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), types.PortID) + suite.Require().NoError(err) - counterparty := &channeltypes.Counterparty{ - PortId: counterpartyPortID, - ChannelId: path.EndpointB.ChannelID, - } + 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, + } + + proposedVersion = types.VersionPrefix - 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) + tc.malleate() + + version, err := cbs.NegotiateAppVersion(suite.chainA.GetContext(), channeltypes.ORDERED, path.EndpointA.ConnectionID, types.PortID, *counterparty, proposedVersion) + if tc.expPass { + suite.Require().NoError(err) + suite.Require().NoError(types.ValidateVersion(version)) + suite.Require().Equal(TestVersion, version) + } else { + suite.Require().Error(err) + suite.Require().Empty(version) + } + }) + } }