From 8d56b94ecea2740dc40de663d2ea6ae36a0d6e34 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Wed, 7 Sep 2022 20:09:09 +0200 Subject: [PATCH] chore: adding assertion of channel capabilities migration (#2140) * wip initial commit * draft migration completed * removing unnecessary storekey arg * additional cleanup * adding updates to migrations and tests additional assertions * updating and moving migrations code * adding additional checks to tests * cleaning up tests * cleaning up tests * updating inline doc comments, checking err return * using InitMemStore in favour of InitializeCapability, adjusting tests * adding assertion of channel capabilities migration * adapting migration code to use get/authenticate capability * adding changelog * updating tests * set middleware enabled for existing channels * assert middleware enabled in tests * updating changelog with middleware enabled flag --- CHANGELOG.md | 1 + .../controller/keeper/migrations.go | 41 ++++++++++++ .../controller/keeper/migrations_test.go | 63 +++++++++++++++++++ modules/apps/27-interchain-accounts/module.go | 7 ++- 4 files changed, 111 insertions(+), 1 deletion(-) create mode 100644 modules/apps/27-interchain-accounts/controller/keeper/migrations.go create mode 100644 modules/apps/27-interchain-accounts/controller/keeper/migrations_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index a84e94470a0..dd3ab8a620c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -94,6 +94,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (apps/27-interchain-accounts) [\#2146](https://github.com/cosmos/ibc-go/pull/2146) ICS27 controller now claims the channel capability passed via ibc core, and passes `nil` to the underlying app callback. The channel capability arg in `SendTx` is now ignored and looked up internally. * (apps/27-interchain-accounts) [\#2134](https://github.com/cosmos/ibc-go/pull/2134) Adding upgrade handler to ICS27 `controller` submodule for migration of channel capabilities. This upgrade handler migrates ownership of channel capabilities from the underlying application to the ICS27 `controller` submodule. * (apps/27-interchain-accounts) [\#2157](https://github.com/cosmos/ibc-go/pull/2157) Adding `IsMiddlewareEnabled` functionality to enforce calls to ICS27 msg server to *not* route to the underlying application. +* (apps/27-interchain-accounts) [\#2140](https://github.com/cosmos/ibc-go/pull/2140) Adding migration handler to ICS27 `controller` submodule to assert ownership of channel capabilities and set middleware enabled flag for existing channels. The ICS27 module consensus version has been bumped from 1 to 2. ### Features diff --git a/modules/apps/27-interchain-accounts/controller/keeper/migrations.go b/modules/apps/27-interchain-accounts/controller/keeper/migrations.go new file mode 100644 index 00000000000..ffe86e9f6e7 --- /dev/null +++ b/modules/apps/27-interchain-accounts/controller/keeper/migrations.go @@ -0,0 +1,41 @@ +package keeper + +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/cosmos/ibc-go/v5/modules/core/23-commitment/types" + host "github.com/cosmos/ibc-go/v5/modules/core/24-host" +) + +// Migrator is a struct for handling in-place store migrations. +type Migrator struct { + keeper *Keeper +} + +// NewMigrator returns a new Migrator. +func NewMigrator(keeper *Keeper) Migrator { + return Migrator{keeper: keeper} +} + +// AssertChannelCapabilityMigrations checks that all channel capabilities generated using the interchain accounts controller port prefix +// are owned by the controller submodule and ibc. +func (m Migrator) AssertChannelCapabilityMigrations(ctx sdk.Context) error { + for _, ch := range m.keeper.GetAllActiveChannels(ctx) { + name := host.ChannelCapabilityPath(ch.PortId, ch.ChannelId) + cap, found := m.keeper.scopedKeeper.GetCapability(ctx, name) + if !found { + return sdkerrors.Wrapf(capabilitytypes.ErrCapabilityNotFound, "failed to find capability: %s", name) + } + + isAuthenticated := m.keeper.scopedKeeper.AuthenticateCapability(ctx, cap, name) + if !isAuthenticated { + return sdkerrors.Wrapf(capabilitytypes.ErrCapabilityNotOwned, "expected capability owner: %s", types.SubModuleName) + } + + m.keeper.SetMiddlewareEnabled(ctx, ch.PortId, ch.ChannelId) + } + + return nil +} diff --git a/modules/apps/27-interchain-accounts/controller/keeper/migrations_test.go b/modules/apps/27-interchain-accounts/controller/keeper/migrations_test.go new file mode 100644 index 00000000000..be20f12cfae --- /dev/null +++ b/modules/apps/27-interchain-accounts/controller/keeper/migrations_test.go @@ -0,0 +1,63 @@ +package keeper_test + +import ( + "github.com/cosmos/ibc-go/v5/modules/apps/27-interchain-accounts/controller/keeper" + ibctesting "github.com/cosmos/ibc-go/v5/testing" +) + +func (suite *KeeperTestSuite) TestAssertChannelCapabilityMigrations() { + testCases := []struct { + name string + malleate func() + expPass bool + }{ + { + "success", + func() {}, + true, + }, + { + "capability not found", + func() { + suite.chainA.GetSimApp().ICAControllerKeeper.SetActiveChannelID( + suite.chainA.GetContext(), + ibctesting.FirstConnectionID, + ibctesting.MockPort, + ibctesting.FirstChannelID, + ) + }, + false, + }, + } + + for _, tc := range testCases { + suite.Run(tc.name, func() { + suite.SetupTest() + + path := NewICAPath(suite.chainA, suite.chainB) + suite.coordinator.SetupConnections(path) + + err := SetupICAPath(path, ibctesting.TestAccAddress) + suite.Require().NoError(err) + + tc.malleate() + + migrator := keeper.NewMigrator(&suite.chainA.GetSimApp().ICAControllerKeeper) + err = migrator.AssertChannelCapabilityMigrations(suite.chainA.GetContext()) + + if tc.expPass { + suite.Require().NoError(err) + + isMiddlewareEnabled := suite.chainA.GetSimApp().ICAControllerKeeper.IsMiddlewareEnabled( + suite.chainA.GetContext(), + path.EndpointA.ChannelConfig.PortID, + path.EndpointA.ChannelID, + ) + + suite.Require().True(isMiddlewareEnabled) + } else { + suite.Require().Error(err) + } + }) + } +} diff --git a/modules/apps/27-interchain-accounts/module.go b/modules/apps/27-interchain-accounts/module.go index 52adcd0efca..d534247c805 100644 --- a/modules/apps/27-interchain-accounts/module.go +++ b/modules/apps/27-interchain-accounts/module.go @@ -153,6 +153,11 @@ func (am AppModule) RegisterServices(cfg module.Configurator) { controllertypes.RegisterMsgServer(cfg.MsgServer(), am.controllerKeeper) controllertypes.RegisterQueryServer(cfg.QueryServer(), am.controllerKeeper) hosttypes.RegisterQueryServer(cfg.QueryServer(), am.hostKeeper) + + m := controllerkeeper.NewMigrator(am.controllerKeeper) + if err := cfg.RegisterMigration(types.ModuleName, 1, m.AssertChannelCapabilityMigrations); err != nil { + panic(fmt.Sprintf("failed to migrate interchainaccounts app from version 1 to 2: %v", err)) + } } // InitGenesis performs genesis initialization for the interchain accounts module. @@ -193,7 +198,7 @@ func (am AppModule) ExportGenesis(ctx sdk.Context, cdc codec.JSONCodec) json.Raw } // ConsensusVersion implements AppModule/ConsensusVersion. -func (AppModule) ConsensusVersion() uint64 { return 1 } +func (AppModule) ConsensusVersion() uint64 { return 2 } // BeginBlock implements the AppModule interface func (am AppModule) BeginBlock(ctx sdk.Context, req abci.RequestBeginBlock) {