feat(ica/controller)!: migrate ica/controller parameters to be self m…

…anaged (#3590)

* added the rpc endpoint and its messages

* ran make protogen command

* implemented sdk.Msg for MsgUpdateParams in types/msgs.go

* added NewMsgUpdateParams method in msgs.go

* added some functions in keeper and msg_Server

* changed ibc_middleware file

* changed the codec file

* used ibcerrors in msg_server

* made changes in all the files

* style(ica/controller): ran gofumpt

* imp(ica/controller): chnaged ParamsKey to string

* imp(ica/controller): removed validate function as the parameter is just a bool

* fix(ica/controller): fixed get/set functions based on new changes

* fix(ica/host): fixed errors caused by the recent changes

* style(ica/controller): ran gofumpt

* fix(ica/host): fixed golanci-lint errors

* docs(ica/host.proto): fixed comment on proto

* fix(ica/controller): naively removed legacySubspace from App

* feat(ica/controller): added legacy subspace to keeper instead

* fix(simapp): reduced the number of modifications due to recent changes

* style(ica/controller): ran gofumpt

* fix(ica/controller): fixed merge conflict

* style(ica/controller): made panic message more consistent

* fix(ica): increased the migration version

* fix(ica): increased the migration version

* feat(ica/controller.test): added keeper params test

* fix(ica.test): fixed test panic

* feat(ica/controller.test): added test cases for MsgUpdateParams validation

* feat(ica/controller.test): added test cases for UpdateParams rpc handler

* imp(ica/controller.test): improved test cases for UpdateParams rpc handler

* style(ica/controller.test): ran gofumpt

* feat(ica/controller.test): added a new test called TestUnsetParams

* feat(ica/controller.test): added a new test case to TestMsgUpdateParamsGetSigners

* style(ica/controller.test): ran gofumpt

* docs(ica/controller): added tracker issue

* style(simapp): improved styling

* docs: added migration info

* docs(ica/controller): added godoc to UpdateParams

* docs(ica/controller): fixed godocs

* imp(ica/controller): improved err message

* imp(ica): combined the two param migrations

* style(ica/controller): changed the ordering of imports

* feat(ica/controller.test): added a new test called TestGetAuthority to keeper tests

* fix(ica/controller.test): fixed a repeated test case

* style(ica/controller): used GetAuthority instead of k.authority

* fix(ica/controller): fixed error type

* style(ica/controller.proto): added missing space

* docs(ica/controller): fixed godoc typo

* style(ica/controller): used controllertypes alias

* add comment

* rename test

* error message formatting

* fix: ran proto-gen

* chore: added 'option (cosmos.msg.v1.signer) = authority;' to proto

* imp: ran proto-gen

* fix(ica/controller): fixed migrations not checking nil keeper

* style: removed nolint interfacer comment

---------

Co-authored-by: srdtrk <srdtrk@hotmail.com>
Co-authored-by: Carlos Rodriguez <carlos@interchain.io>

docs/apps/interchain-accounts/parameters.md

@@ -8,7 +8,7 @@ The Interchain Accounts module contains the following on-chain parameters, logic
 
 ## Controller Submodule Parameters
 
-| Key                    | Type | Default Value |
+| Name                   | Type | Default Value |
 |------------------------|------|---------------|
 | `ControllerEnabled`    | bool | `true`        |
 
@@ -24,7 +24,7 @@ The `ControllerEnabled` parameter controls a chains ability to service ICS-27 co
 
 ## Host Submodule Parameters
 
-| Key                    | Type     | Default Value |
+| Name                   | Type     | Default Value |
 |------------------------|----------|---------------|
 | `HostEnabled`          | bool     | `true`        |
 | `AllowMessages`        | []string | `["*"]`       |

docs/migrations/v7-to-v8.md

@@ -16,7 +16,7 @@ There are four sections based on the four potential user groups of this document
 
 TODO: https://github.com/cosmos/ibc-go/pull/3505 (extra parameter added to transfer's `GenesisState`)
 
-- You should pass the `authority` to the icahost keeper. ([#3520](https://github.com/cosmos/ibc-go/pull/3520)) See [diff](https://github.com/cosmos/ibc-go/pull/3520/files#diff-d18972debee5e64f16e40807b2ae112ddbe609504a93ea5e1c80a5d489c3a08a).
+- You must pass the `authority` to the icahost keeper. ([#3520](https://github.com/cosmos/ibc-go/pull/3520)) See [diff](https://github.com/cosmos/ibc-go/pull/3520/files#diff-d18972debee5e64f16e40807b2ae112ddbe609504a93ea5e1c80a5d489c3a08a).
 
 ```diff
 // app.go
@@ -31,7 +31,22 @@ TODO: https://github.com/cosmos/ibc-go/pull/3505 (extra parameter added to trans
 	)
 ```
 
-- You should pass the `authority` to the ibctransfer keeper. ([#3520](https://github.com/cosmos/ibc-go/pull/3520)) See [diff](https://github.com/cosmos/ibc-go/pull/3520/files#diff-d18972debee5e64f16e40807b2ae112ddbe609504a93ea5e1c80a5d489c3a08a). ([#3553](https://github.com/cosmos/ibc-go/pull/3553))
+- You must pass the `authority` to the icacontroller keeper. ([#3590](https://github.com/cosmos/ibc-go/pull/3590)) See [diff](https://github.com/cosmos/ibc-go/pull/3590/files#diff-d18972debee5e64f16e40807b2ae112ddbe609504a93ea5e1c80a5d489c3a08a).
+
+```diff
+// app.go
+
+	// ICA Controller keeper
+	app.ICAControllerKeeper = icacontrollerkeeper.NewKeeper(
+		appCodec, keys[icacontrollertypes.StoreKey], app.GetSubspace(icacontrollertypes.SubModuleName),
+		app.IBCFeeKeeper, // use ics29 fee as ics4Wrapper in middleware stack
+		app.IBCKeeper.ChannelKeeper, &app.IBCKeeper.PortKeeper,
+		scopedICAControllerKeeper, app.MsgServiceRouter(),
++		authtypes.NewModuleAddress(govtypes.ModuleName).String(),
+	)
+```
+
+- You must pass the `authority` to the ibctransfer keeper. ([#3553](https://github.com/cosmos/ibc-go/pull/3553)) See [diff](https://github.com/cosmos/ibc-go/pull/3553/files#diff-d18972debee5e64f16e40807b2ae112ddbe609504a93ea5e1c80a5d489c3a08a).
 
 ```diff
 // app.go

modules/apps/27-interchain-accounts/controller/ibc_middleware.go

@@ -3,8 +3,8 @@ package controller
 import (
 	errorsmod "cosmossdk.io/errors"
 	sdk "github.com/cosmos/cosmos-sdk/types"
-	capabilitytypes "github.com/cosmos/ibc-go/modules/capability/types"
 
+	capabilitytypes "github.com/cosmos/ibc-go/modules/capability/types"
 	"github.com/cosmos/ibc-go/v7/modules/apps/27-interchain-accounts/controller/keeper"
 	"github.com/cosmos/ibc-go/v7/modules/apps/27-interchain-accounts/controller/types"
 	icatypes "github.com/cosmos/ibc-go/v7/modules/apps/27-interchain-accounts/types"
@@ -49,7 +49,7 @@ func (im IBCMiddleware) OnChanOpenInit(
 	counterparty channeltypes.Counterparty,
 	version string,
 ) (string, error) {
-	if !im.keeper.IsControllerEnabled(ctx) {
+	if !im.keeper.GetParams(ctx).ControllerEnabled {
 		return "", types.ErrControllerSubModuleDisabled
 	}
 
@@ -101,7 +101,7 @@ func (im IBCMiddleware) OnChanOpenAck(
 	counterpartyChannelID string,
 	counterpartyVersion string,
 ) error {
-	if !im.keeper.IsControllerEnabled(ctx) {
+	if !im.keeper.GetParams(ctx).ControllerEnabled {
 		return types.ErrControllerSubModuleDisabled
 	}
 
@@ -182,7 +182,7 @@ func (im IBCMiddleware) OnAcknowledgementPacket(
 	acknowledgement []byte,
 	relayer sdk.AccAddress,
 ) error {
-	if !im.keeper.IsControllerEnabled(ctx) {
+	if !im.keeper.GetParams(ctx).ControllerEnabled {
 		return types.ErrControllerSubModuleDisabled
 	}
 
@@ -205,7 +205,7 @@ func (im IBCMiddleware) OnTimeoutPacket(
 	packet channeltypes.Packet,
 	relayer sdk.AccAddress,
 ) error {
-	if !im.keeper.IsControllerEnabled(ctx) {
+	if !im.keeper.GetParams(ctx).ControllerEnabled {
 		return types.ErrControllerSubModuleDisabled
 	}
 

modules/apps/27-interchain-accounts/controller/keeper/keeper.go

@@ -11,8 +11,8 @@ import (
 	storetypes "github.com/cosmos/cosmos-sdk/store/types"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	paramtypes "github.com/cosmos/cosmos-sdk/x/params/types"
-	capabilitytypes "github.com/cosmos/ibc-go/modules/capability/types"
 
+	capabilitytypes "github.com/cosmos/ibc-go/modules/capability/types"
 	"github.com/cosmos/ibc-go/v7/modules/apps/27-interchain-accounts/controller/types"
 	genesistypes "github.com/cosmos/ibc-go/v7/modules/apps/27-interchain-accounts/genesis/types"
 	icatypes "github.com/cosmos/ibc-go/v7/modules/apps/27-interchain-accounts/types"
@@ -24,39 +24,43 @@ import (
 
 // Keeper defines the IBC interchain accounts controller keeper
 type Keeper struct {
-	storeKey   storetypes.StoreKey
-	cdc        codec.BinaryCodec
-	paramSpace paramtypes.Subspace
-
-	ics4Wrapper   porttypes.ICS4Wrapper
-	channelKeeper icatypes.ChannelKeeper
-	portKeeper    icatypes.PortKeeper
+	storeKey       storetypes.StoreKey
+	cdc            codec.BinaryCodec
+	legacySubspace paramtypes.Subspace
+	ics4Wrapper    porttypes.ICS4Wrapper
+	channelKeeper  icatypes.ChannelKeeper
+	portKeeper     icatypes.PortKeeper
 
 	scopedKeeper exported.ScopedKeeper
 
 	msgRouter icatypes.MessageRouter
+
+	// the address capable of executing a MsgUpdateParams message. Typically, this
+	// should be the x/gov module account.
+	authority string
 }
 
 // NewKeeper creates a new interchain accounts controller Keeper instance
 func NewKeeper(
-	cdc codec.BinaryCodec, key storetypes.StoreKey, paramSpace paramtypes.Subspace,
+	cdc codec.BinaryCodec, key storetypes.StoreKey, legacySubspace paramtypes.Subspace,
 	ics4Wrapper porttypes.ICS4Wrapper, channelKeeper icatypes.ChannelKeeper, portKeeper icatypes.PortKeeper,
-	scopedKeeper exported.ScopedKeeper, msgRouter icatypes.MessageRouter,
+	scopedKeeper exported.ScopedKeeper, msgRouter icatypes.MessageRouter, authority string,
 ) Keeper {
 	// set KeyTable if it has not already been set
-	if !paramSpace.HasKeyTable() {
-		paramSpace = paramSpace.WithKeyTable(types.ParamKeyTable())
+	if !legacySubspace.HasKeyTable() {
+		legacySubspace = legacySubspace.WithKeyTable(types.ParamKeyTable())
 	}
 
 	return Keeper{
-		storeKey:      key,
-		cdc:           cdc,
-		paramSpace:    paramSpace,
-		ics4Wrapper:   ics4Wrapper,
-		channelKeeper: channelKeeper,
-		portKeeper:    portKeeper,
-		scopedKeeper:  scopedKeeper,
-		msgRouter:     msgRouter,
+		storeKey:       key,
+		cdc:            cdc,
+		legacySubspace: legacySubspace,
+		ics4Wrapper:    ics4Wrapper,
+		channelKeeper:  channelKeeper,
+		portKeeper:     portKeeper,
+		scopedKeeper:   scopedKeeper,
+		msgRouter:      msgRouter,
+		authority:      authority,
 	}
 }
 
@@ -265,3 +269,28 @@ func (k Keeper) DeleteMiddlewareEnabled(ctx sdk.Context, portID, connectionID st
 	store := ctx.KVStore(k.storeKey)
 	store.Delete(icatypes.KeyIsMiddlewareEnabled(portID, connectionID))
 }
+
+// GetAuthority returns the ica/controller submodule's authority.
+func (k Keeper) GetAuthority() string {
+	return k.authority
+}
+
+// GetParams returns the current ica/controller submodule parameters.
+func (k Keeper) GetParams(ctx sdk.Context) types.Params {
+	store := ctx.KVStore(k.storeKey)
+	bz := store.Get([]byte(types.ParamsKey))
+	if bz == nil { // only panic on unset params and not on empty params
+		panic("ica/controller params are not set in store")
+	}
+
+	var params types.Params
+	k.cdc.MustUnmarshal(bz, &params)
+	return params
+}
+
+// SetParams sets the ica/controller submodule parameters.
+func (k Keeper) SetParams(ctx sdk.Context, params types.Params) {
+	store := ctx.KVStore(k.storeKey)
+	bz := k.cdc.MustMarshal(&params)
+	store.Set([]byte(types.ParamsKey), bz)
+}

modules/apps/27-interchain-accounts/controller/keeper/keeper_test.go

@@ -5,6 +5,10 @@ import (
 
 	"github.com/stretchr/testify/suite"
 
+	authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
+	govtypes "github.com/cosmos/cosmos-sdk/x/gov/types"
+
+	"github.com/cosmos/ibc-go/v7/modules/apps/27-interchain-accounts/controller/types"
 	genesistypes "github.com/cosmos/ibc-go/v7/modules/apps/27-interchain-accounts/genesis/types"
 	icatypes "github.com/cosmos/ibc-go/v7/modules/apps/27-interchain-accounts/types"
 	channeltypes "github.com/cosmos/ibc-go/v7/modules/core/04-channel/types"
@@ -238,3 +242,53 @@ func (suite *KeeperTestSuite) TestSetInterchainAccountAddress() {
 	suite.Require().True(found)
 	suite.Require().Equal(expectedAccAddr, retrievedAddr)
 }
+
+func (suite *KeeperTestSuite) TestSetAndGetParams() {
+	testCases := []struct {
+		name    string
+		input   types.Params
+		expPass bool
+	}{
+		// it is not possible to set invalid booleans
+		{"success: set params false", types.NewParams(false), true},
+		{"success: set params true", types.NewParams(true), true},
+	}
+
+	for _, tc := range testCases {
+		tc := tc
+		suite.Run(tc.name, func() {
+			suite.SetupTest() // reset
+			ctx := suite.chainA.GetContext()
+			if tc.expPass {
+				suite.chainA.GetSimApp().ICAControllerKeeper.SetParams(ctx, tc.input)
+				expected := tc.input
+				p := suite.chainA.GetSimApp().ICAControllerKeeper.GetParams(ctx)
+				suite.Require().Equal(expected, p)
+			} else { // currently not possible to set invalid params
+				suite.Require().Panics(func() {
+					suite.chainA.GetSimApp().ICAControllerKeeper.SetParams(ctx, tc.input)
+				})
+			}
+		})
+	}
+}
+
+func (suite *KeeperTestSuite) TestUnsetParams() {
+	suite.SetupTest()
+
+	ctx := suite.chainA.GetContext()
+	store := suite.chainA.GetContext().KVStore(suite.chainA.GetSimApp().GetKey(types.SubModuleName))
+	store.Delete([]byte(types.ParamsKey))
+
+	suite.Require().Panics(func() {
+		suite.chainA.GetSimApp().ICAControllerKeeper.GetParams(ctx)
+	})
+}
+
+func (suite *KeeperTestSuite) TestGetAuthority() {
+	suite.SetupTest()
+
+	authority := suite.chainA.GetSimApp().ICAControllerKeeper.GetAuthority()
+	expectedAuth := authtypes.NewModuleAddress(govtypes.ModuleName).String()
+	suite.Require().Equal(expectedAuth, authority)
+}

modules/apps/27-interchain-accounts/controller/keeper/migrations.go

@@ -5,8 +5,8 @@ import (
 
 	errorsmod "cosmossdk.io/errors"
 	sdk "github.com/cosmos/cosmos-sdk/types"
-	capabilitytypes "github.com/cosmos/ibc-go/modules/capability/types"
 
+	capabilitytypes "github.com/cosmos/ibc-go/modules/capability/types"
 	controllertypes "github.com/cosmos/ibc-go/v7/modules/apps/27-interchain-accounts/controller/types"
 	icatypes "github.com/cosmos/ibc-go/v7/modules/apps/27-interchain-accounts/types"
 	host "github.com/cosmos/ibc-go/v7/modules/core/24-host"
@@ -17,9 +17,11 @@ type Migrator struct {
 	keeper *Keeper
 }
 
-// NewMigrator returns a new Migrator.
-func NewMigrator(keeper *Keeper) Migrator {
-	return Migrator{keeper: keeper}
+// NewMigrator returns Migrator instance for the state migration.
+func NewMigrator(k *Keeper) Migrator {
+	return Migrator{
+		keeper: k,
+	}
 }
 
 // AssertChannelCapabilityMigrations checks that all channel capabilities generated using the interchain accounts controller port prefix
@@ -48,3 +50,14 @@ func (m Migrator) AssertChannelCapabilityMigrations(ctx sdk.Context) error {
 	}
 	return nil
 }
+
+// MigrateParams migrates the controller submodule's parameters from the x/params to self store.
+func (m Migrator) MigrateParams(ctx sdk.Context) error {
+	if m.keeper != nil {
+		var params controllertypes.Params
+		m.keeper.legacySubspace.GetParamSet(ctx, &params)
+
+		m.keeper.SetParams(ctx, params)
+	}
+	return nil
+}

modules/apps/27-interchain-accounts/controller/keeper/migrations_test.go

@@ -3,7 +3,8 @@ package keeper_test
 import (
 	"fmt"
 
-	"github.com/cosmos/ibc-go/v7/modules/apps/27-interchain-accounts/controller/keeper"
+	icacontrollerkeeper "github.com/cosmos/ibc-go/v7/modules/apps/27-interchain-accounts/controller/keeper"
+	icacontrollertypes "github.com/cosmos/ibc-go/v7/modules/apps/27-interchain-accounts/controller/types"
 	icatypes "github.com/cosmos/ibc-go/v7/modules/apps/27-interchain-accounts/types"
 	channeltypes "github.com/cosmos/ibc-go/v7/modules/core/04-channel/types"
 	ibctesting "github.com/cosmos/ibc-go/v7/testing"
@@ -54,7 +55,7 @@ func (suite *KeeperTestSuite) TestAssertChannelCapabilityMigrations() {
 
 			tc.malleate()
 
-			migrator := keeper.NewMigrator(&suite.chainA.GetSimApp().ICAControllerKeeper)
+			migrator := icacontrollerkeeper.NewMigrator(&suite.chainA.GetSimApp().ICAControllerKeeper)
 			err = migrator.AssertChannelCapabilityMigrations(suite.chainA.GetContext())
 
 			if tc.expPass {
@@ -73,3 +74,36 @@ func (suite *KeeperTestSuite) TestAssertChannelCapabilityMigrations() {
 		})
 	}
 }
+
+func (suite *KeeperTestSuite) TestMigratorMigrateParams() {
+	testCases := []struct {
+		msg            string
+		malleate       func()
+		expectedParams icacontrollertypes.Params
+	}{
+		{
+			"success: default params",
+			func() {
+				params := icacontrollertypes.DefaultParams()
+				subspace := suite.chainA.GetSimApp().GetSubspace(icacontrollertypes.SubModuleName) // get subspace
+				subspace.SetParamSet(suite.chainA.GetContext(), &params)                           // set params
+			},
+			icacontrollertypes.DefaultParams(),
+		},
+	}
+
+	for _, tc := range testCases {
+		suite.Run(fmt.Sprintf("case %s", tc.msg), func() {
+			suite.SetupTest() // reset
+
+			tc.malleate() // explicitly set params
+
+			migrator := icacontrollerkeeper.NewMigrator(&suite.chainA.GetSimApp().ICAControllerKeeper)
+			err := migrator.MigrateParams(suite.chainA.GetContext())
+			suite.Require().NoError(err)
+
+			params := suite.chainA.GetSimApp().ICAControllerKeeper.GetParams(suite.chainA.GetContext())
+			suite.Require().Equal(tc.expectedParams, params)
+		})
+	}
+}

modules/apps/27-interchain-accounts/controller/keeper/msg_server.go

@@ -8,6 +8,7 @@ import (
 
 	"github.com/cosmos/ibc-go/v7/modules/apps/27-interchain-accounts/controller/types"
 	icatypes "github.com/cosmos/ibc-go/v7/modules/apps/27-interchain-accounts/types"
+	ibcerrors "github.com/cosmos/ibc-go/v7/modules/core/errors"
 )
 
 var _ types.MsgServer = (*msgServer)(nil)
@@ -70,3 +71,15 @@ func (s msgServer) SendTx(goCtx context.Context, msg *types.MsgSendTx) (*types.M
 
 	return &types.MsgSendTxResponse{Sequence: seq}, nil
 }
+
+// UpdateParams defines an rpc handler method for MsgUpdateParams. Updates the ica/controller submodule's parameters.
+func (k Keeper) UpdateParams(goCtx context.Context, msg *types.MsgUpdateParams) (*types.MsgUpdateParamsResponse, error) {
+	if k.GetAuthority() != msg.Authority {
+		return nil, errorsmod.Wrapf(ibcerrors.ErrUnauthorized, "expected %s, got %s", k.GetAuthority(), msg.Authority)
+	}
+
+	ctx := sdk.UnwrapSDKContext(goCtx)
+	k.SetParams(ctx, msg.Params)
+
+	return &types.MsgUpdateParamsResponse{}, nil
+}