Skip to content

Commit

Permalink
feat(ica/host)!: migrate ica/host to params to be self managed (#3520)
Browse files Browse the repository at this point in the history
* feat: add proto msg

* feat: add msg-server implementation

* wip: add migrations

* fix failing tests

* cleanup

* cleanup

* Update modules/apps/27-interchain-accounts/host/migrations/v8/migrations.go

* fix(transfer): 'p.AllowMessages' missing '&' in 'NewParamSetPair'

* fix(transfer): 'p.HostEnabled' missing '&' in 'NewParamSetPair'

* refactor(ica/host): reduce code duplication by removing 'IsHostEnabled' and 'GetAllowMessages' functions

* refactor(ica/host): moved getter and setters to 'keeper.go'

* refactor(ica/host): moved getter and setters to 'keeper.go'

* fix(ica/host): handling the SetParam errors now

* imp(ica/host): added failure cases to TestValidateParams

* feat(ica/host): added codec for msg

* fix(ica/host): added host's RegisterInterfaces to module.go

* fix(changelog): removed manual addition

* imp(ica/host): made GetParams more economical

* fix(fee/test): handled ica's 'SetParams' error during testing

* imp(ica/host): using ibcerrors instead of relying on sdk's govtypes

* style(ica/host): removed '*' from Migrator's keeper

* revert(ica/host): reverts to the previous commit as this is more consistent with what is done in module.go

* fix(simapp): added ParamKeyTable to the icahost subspace in app.go for migration

* feat(ica/host/test): added migrator_test.go

* style(ica/host/test): ran gofumpt

* feat(ica/host): passing legacySubspace to keeper instead

* fix(ica/host): removed unneeded reference to hss from 'NewAppModule' function

* fix(simapp): reduced modifications to simapp to 1

* fix(ica/host/test): updated tests

* style(ica/host): ran gofumpt

* docs(ica/host): fixed a typo

* style(ica/host): added a space for import separation

* style(ica/host): renamed 'migrator.go' to 'migrations.go'

* refactor(ica/host): removed unneeded validate function

* style(ica/host): storing ParamsKey using a string

* fix(ica/host): registered the msg_server in ica's module.go

* style(ica/host): removed unneeded fmt variable assignment

* imp(ica/host/test): added more test cases, and refactored the test

* style(ica/host/test): ran gofumpt

* imp(ica/host): 'GetParams' panics now if it can't find the params

* imp(core): added 'ErrInvalidAuthority'

* imp(ica/host): uses 'ErrInvalidAuthority'

* fix(ica/test): fixed test for the ica module

* imp(ica/host/test): added a new test case

* imp(ica/host/test): added params test to genesis

* style(ica/host): ran gofumpt

* imp(ica/host/test): added unset param test to keeper

* docs(ica/host): added tracker issue for removing params_legacy.go

* style(ica/host): improved test case naming

* fix(ica/host/test): fixed a minor inaccuracy with the test in the hopes of increasing codecov

* style(ica/host): updated err message

* imp(ica/transfer, core/errors): removed ErrInvalidAuthority

* style(ica/host): updated nil check to be more consistent

* style(transfer): renamed errors to errorsmod to avoid shadowing

* imp(ica/host/test): made test shorter

* imp(ica/host/test): removed unneeded comment

* style(ica/host/test): removed test case field names

* refactor(ica/host, fee/test): refactored Validate out of SetParams

* style(ica/host): ran gofumpt

* imp(ica/types): switched back to 'bz == nil'

* style(ica/host/test): looks better

* style(ica/host): changed panic message

* imp(ica/host): removed redundant Validate from msg server, handled in ValidateBasic

* style(ica/host): added code comment

* fix(ica/host): increase consensus version

* style(ica/host/test): moved success case to top

* style(ica/host/test): improve test styling

* docs(migration): added changes in app.go

* imp(ica/host): improved the error message

---------

Co-authored-by: srdtrk <[email protected]>
Co-authored-by: srdtrk <[email protected]>
  • Loading branch information
3 people authored May 23, 2023
1 parent ba62612 commit df918d2
Show file tree
Hide file tree
Showing 24 changed files with 1,157 additions and 111 deletions.
15 changes: 15 additions & 0 deletions docs/migrations/v7-to-v8.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,21 @@ 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).

```diff
// app.go

// ICA Host keeper
app.ICAHostKeeper = icahostkeeper.NewKeeper(
appCodec, keys[icahosttypes.StoreKey], app.GetSubspace(icahosttypes.SubModuleName),
app.IBCFeeKeeper, // use ics29 fee as ics4Wrapper in middleware stack
app.IBCKeeper.ChannelKeeper, &app.IBCKeeper.PortKeeper,
app.AccountKeeper, scopedICAHostKeeper, app.MsgServiceRouter(),
+ authtypes.NewModuleAddress(govtypes.ModuleName).String(),
)
```

## IBC Apps

TODO: https://github.com/cosmos/ibc-go/pull/3303
Expand Down
6 changes: 3 additions & 3 deletions modules/apps/27-interchain-accounts/host/ibc_module.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ func (im IBCModule) OnChanOpenTry(
counterparty channeltypes.Counterparty,
counterpartyVersion string,
) (string, error) {
if !im.keeper.IsHostEnabled(ctx) {
if !im.keeper.GetParams(ctx).HostEnabled {
return "", types.ErrHostSubModuleDisabled
}

Expand All @@ -76,7 +76,7 @@ func (im IBCModule) OnChanOpenConfirm(
portID,
channelID string,
) error {
if !im.keeper.IsHostEnabled(ctx) {
if !im.keeper.GetParams(ctx).HostEnabled {
return types.ErrHostSubModuleDisabled
}

Expand Down Expand Up @@ -109,7 +109,7 @@ func (im IBCModule) OnRecvPacket(
_ sdk.AccAddress,
) ibcexported.Acknowledgement {
logger := im.keeper.Logger(ctx)
if !im.keeper.IsHostEnabled(ctx) {
if !im.keeper.GetParams(ctx).HostEnabled {
logger.Info("host submodule is disabled")
return channeltypes.NewErrorAcknowledgement(types.ErrHostSubModuleDisabled)
}
Expand Down
3 changes: 3 additions & 0 deletions modules/apps/27-interchain-accounts/host/keeper/genesis.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@ func InitGenesis(ctx sdk.Context, keeper Keeper, state genesistypes.HostGenesisS
keeper.SetInterchainAccountAddress(ctx, acc.ConnectionId, acc.PortId, acc.AccountAddress)
}

if err := state.Params.Validate(); err != nil {
panic(fmt.Sprintf("could not set ica host params at genesis: %v", err))
}
keeper.SetParams(ctx, state.Params)
}

Expand Down
62 changes: 61 additions & 1 deletion modules/apps/27-interchain-accounts/host/keeper/genesis_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,11 +41,71 @@ func (suite *KeeperTestSuite) TestInitGenesis() {
suite.Require().True(found)
suite.Require().Equal(interchainAccAddr.String(), accountAdrr)

expParams := types.NewParams(false, nil)
expParams := genesisState.GetParams()
params := suite.chainA.GetSimApp().ICAHostKeeper.GetParams(suite.chainA.GetContext())
suite.Require().Equal(expParams, params)
}

func (suite *KeeperTestSuite) TestGenesisParams() {
testCases := []struct {
name string
input types.Params
expPass bool
}{
{"success: set default params", types.DefaultParams(), true},
{"success: non-default params", types.NewParams(!types.DefaultHostEnabled, []string{"/cosmos.staking.v1beta1.MsgDelegate"}), true},
{"success: set empty byte for allow messages", types.NewParams(true, nil), true},
{"failure: set empty string for allow messages", types.NewParams(true, []string{""}), false},
{"failure: set space string for allow messages", types.NewParams(true, []string{" "}), false},
}

for _, tc := range testCases {
tc := tc

suite.Run(tc.name, func() {
suite.SetupTest() // reset
interchainAccAddr := icatypes.GenerateAddress(suite.chainB.GetContext(), ibctesting.FirstConnectionID, TestPortID)
genesisState := genesistypes.HostGenesisState{
ActiveChannels: []genesistypes.ActiveChannel{
{
ConnectionId: ibctesting.FirstConnectionID,
PortId: TestPortID,
ChannelId: ibctesting.FirstChannelID,
},
},
InterchainAccounts: []genesistypes.RegisteredInterchainAccount{
{
ConnectionId: ibctesting.FirstConnectionID,
PortId: TestPortID,
AccountAddress: interchainAccAddr.String(),
},
},
Port: icatypes.HostPortID,
Params: tc.input,
}
if tc.expPass {
keeper.InitGenesis(suite.chainA.GetContext(), suite.chainA.GetSimApp().ICAHostKeeper, genesisState)

channelID, found := suite.chainA.GetSimApp().ICAHostKeeper.GetActiveChannelID(suite.chainA.GetContext(), ibctesting.FirstConnectionID, TestPortID)
suite.Require().True(found)
suite.Require().Equal(ibctesting.FirstChannelID, channelID)

accountAdrr, found := suite.chainA.GetSimApp().ICAHostKeeper.GetInterchainAccountAddress(suite.chainA.GetContext(), ibctesting.FirstConnectionID, TestPortID)
suite.Require().True(found)
suite.Require().Equal(interchainAccAddr.String(), accountAdrr)

expParams := tc.input
params := suite.chainA.GetSimApp().ICAHostKeeper.GetParams(suite.chainA.GetContext())
suite.Require().Equal(expParams, params)
} else {
suite.Require().Panics(func() {
keeper.InitGenesis(suite.chainA.GetContext(), suite.chainA.GetSimApp().ICAHostKeeper, genesisState)
})
}
})
}
}

func (suite *KeeperTestSuite) TestExportGenesis() {
suite.SetupTest()

Expand Down
61 changes: 46 additions & 15 deletions modules/apps/27-interchain-accounts/host/keeper/keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,9 @@ import (

// Keeper defines the IBC interchain accounts host keeper
type Keeper struct {
storeKey storetypes.StoreKey
cdc codec.BinaryCodec
paramSpace paramtypes.Subspace
storeKey storetypes.StoreKey
cdc codec.BinaryCodec
legacySubspace paramtypes.Subspace

ics4Wrapper porttypes.ICS4Wrapper
channelKeeper icatypes.ChannelKeeper
Expand All @@ -34,34 +34,40 @@ type Keeper struct {
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 host 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,
accountKeeper icatypes.AccountKeeper, scopedKeeper exported.ScopedKeeper, msgRouter icatypes.MessageRouter,
authority string,
) Keeper {
// ensure ibc interchain accounts module account is set
if addr := accountKeeper.GetModuleAddress(icatypes.ModuleName); addr == nil {
panic("the Interchain Accounts module account has not been set")
}

// 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,
accountKeeper: accountKeeper,
scopedKeeper: scopedKeeper,
msgRouter: msgRouter,
storeKey: key,
cdc: cdc,
legacySubspace: legacySubspace,
ics4Wrapper: ics4Wrapper,
channelKeeper: channelKeeper,
portKeeper: portKeeper,
accountKeeper: accountKeeper,
scopedKeeper: scopedKeeper,
msgRouter: msgRouter,
authority: authority,
}
}

Expand Down Expand Up @@ -199,3 +205,28 @@ func (k Keeper) SetInterchainAccountAddress(ctx sdk.Context, connectionID, portI
store := ctx.KVStore(k.storeKey)
store.Set(icatypes.KeyOwnerAccount(portID, connectionID), []byte(address))
}

// GetAuthority returns the 27-interchain-accounts host submodule's authority.
func (k Keeper) GetAuthority() string {
return k.authority
}

// GetParams returns the total set of the host 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/host params are not set in store")
}

var params types.Params
k.cdc.MustUnmarshal(bz, &params)
return params
}

// SetParams sets the total set of the host 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)
}
50 changes: 50 additions & 0 deletions modules/apps/27-interchain-accounts/host/keeper/keeper_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"github.com/stretchr/testify/suite"

genesistypes "github.com/cosmos/ibc-go/v7/modules/apps/27-interchain-accounts/genesis/types"
"github.com/cosmos/ibc-go/v7/modules/apps/27-interchain-accounts/host/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"
Expand Down Expand Up @@ -218,3 +219,52 @@ func (suite *KeeperTestSuite) TestSetInterchainAccountAddress() {
suite.Require().True(found)
suite.Require().Equal(expectedAccAddr, retrievedAddr)
}

func (suite *KeeperTestSuite) TestParams() {
expParams := types.DefaultParams()

params := suite.chainA.GetSimApp().ICAHostKeeper.GetParams(suite.chainA.GetContext())
suite.Require().Equal(expParams, params)

testCases := []struct {
name string
input types.Params
expPass bool
}{
{"success: set default params", types.DefaultParams(), true},
{"success: non-default params", types.NewParams(!types.DefaultHostEnabled, []string{"/cosmos.staking.v1beta1.MsgDelegate"}), true},
{"success: set empty byte for allow messages", types.NewParams(true, nil), true},
{"failure: set empty string for allow messages", types.NewParams(true, []string{""}), false},
{"failure: set space string for allow messages", types.NewParams(true, []string{" "}), false},
}

for _, tc := range testCases {
tc := tc

suite.Run(tc.name, func() {
suite.SetupTest() // reset
ctx := suite.chainA.GetContext()
err := tc.input.Validate()
suite.chainA.GetSimApp().ICAHostKeeper.SetParams(ctx, tc.input)
if tc.expPass {
suite.Require().NoError(err)
expected := tc.input
p := suite.chainA.GetSimApp().ICAHostKeeper.GetParams(ctx)
suite.Require().Equal(expected, p)
} else {
suite.Require().Error(err)
}
})
}
}

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().ICAHostKeeper.GetParams(ctx)
})
}
31 changes: 31 additions & 0 deletions modules/apps/27-interchain-accounts/host/keeper/migrations.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
package keeper

import (
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/ibc-go/v7/modules/apps/27-interchain-accounts/host/types"
)

// Migrator is a struct for handling in-place state migrations.
type Migrator struct {
keeper *Keeper
}

// NewMigrator returns Migrator instance for the state migration.
func NewMigrator(k *Keeper) Migrator {
return Migrator{
keeper: k,
}
}

// MigrateParams migrates the host submodule's parameters from the x/params to self store.
func (m Migrator) MigrateParams(ctx sdk.Context) error {
var params types.Params
m.keeper.legacySubspace.GetParamSet(ctx, &params)

if err := params.Validate(); err != nil {
return err
}
m.keeper.SetParams(ctx, params)

return nil
}
41 changes: 41 additions & 0 deletions modules/apps/27-interchain-accounts/host/keeper/migrations_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
package keeper_test

import (
"fmt"

icahostkeeper "github.com/cosmos/ibc-go/v7/modules/apps/27-interchain-accounts/host/keeper"
icahosttypes "github.com/cosmos/ibc-go/v7/modules/apps/27-interchain-accounts/host/types"
)

func (suite *KeeperTestSuite) TestMigratorMigrateParams() {
testCases := []struct {
msg string
malleate func()
expectedParams icahosttypes.Params
}{
{
"success: default params",
func() {
params := icahosttypes.DefaultParams()
subspace := suite.chainA.GetSimApp().GetSubspace(icahosttypes.SubModuleName) // get subspace
subspace.SetParamSet(suite.chainA.GetContext(), &params) // set params
},
icahosttypes.DefaultParams(),
},
}

for _, tc := range testCases {
suite.Run(fmt.Sprintf("case %s", tc.msg), func() {
suite.SetupTest() // reset

tc.malleate() // explicitly set params

migrator := icahostkeeper.NewMigrator(&suite.chainA.GetSimApp().ICAHostKeeper)
err := migrator.MigrateParams(suite.chainA.GetContext())
suite.Require().NoError(err)

params := suite.chainA.GetSimApp().ICAHostKeeper.GetParams(suite.chainA.GetContext())
suite.Require().Equal(tc.expectedParams, params)
})
}
}
35 changes: 35 additions & 0 deletions modules/apps/27-interchain-accounts/host/keeper/msg_server.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
package keeper

import (
"context"

errorsmod "cosmossdk.io/errors"
sdk "github.com/cosmos/cosmos-sdk/types"

"github.com/cosmos/ibc-go/v7/modules/apps/27-interchain-accounts/host/types"
ibcerrors "github.com/cosmos/ibc-go/v7/modules/core/errors"
)

var _ types.MsgServer = (*msgServer)(nil)

type msgServer struct {
*Keeper
}

// NewMsgServerImpl returns an implementation of the ICS27 host MsgServer interface
// for the provided Keeper.
func NewMsgServerImpl(keeper *Keeper) types.MsgServer {
return &msgServer{Keeper: keeper}
}

// UpdateParams updates the host submodule's params.
func (m msgServer) UpdateParams(goCtx context.Context, msg *types.MsgUpdateParams) (*types.MsgUpdateParamsResponse, error) {
if m.authority != msg.Authority {
return nil, errorsmod.Wrapf(ibcerrors.ErrUnauthorized, "expected %s, got %s", m.authority, msg.Authority)
}

ctx := sdk.UnwrapSDKContext(goCtx)
m.SetParams(ctx, msg.Params)

return &types.MsgUpdateParamsResponse{}, nil
}
Loading

0 comments on commit df918d2

Please sign in to comment.