Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(ica/host)!: migrate ICA host params to be self managed #3520

Merged
merged 75 commits into from
May 23, 2023
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
75 commits
Select commit Hold shift + click to select a range
4ac0e7b
feat: add proto msg
aleem1314 Apr 25, 2023
3e8354b
feat: add msg-server implementation
aleem1314 Apr 25, 2023
c17fdf2
wip: add migrations
aleem1314 Apr 26, 2023
dc97796
fix failing tests
aleem1314 Apr 27, 2023
2140a3e
cleanup
aleem1314 Apr 28, 2023
c9cd005
cleanup
aleem1314 May 3, 2023
98f213c
Update modules/apps/27-interchain-accounts/host/migrations/v8/migrati…
aleem1314 May 3, 2023
9c53d61
fix(transfer): 'p.AllowMessages' missing '&' in 'NewParamSetPair'
srdtrk May 15, 2023
b02d03b
fix(transfer): 'p.HostEnabled' missing '&' in 'NewParamSetPair'
srdtrk May 15, 2023
6269d16
refactor(ica/host): reduce code duplication by removing 'IsHostEnable…
srdtrk May 15, 2023
b9f0817
refactor(ica/host): moved getter and setters to 'keeper.go'
srdtrk May 15, 2023
f5c9fea
refactor(ica/host): moved getter and setters to 'keeper.go'
srdtrk May 15, 2023
d9a12a8
fix(ica/host): handling the SetParam errors now
srdtrk May 16, 2023
3c97fba
imp(ica/host): added failure cases to TestValidateParams
srdtrk May 16, 2023
a5f4b75
feat(ica/host): added codec for msg
srdtrk May 16, 2023
09da84f
fix(ica/host): added host's RegisterInterfaces to module.go
srdtrk May 16, 2023
926ffdd
fix(changelog): removed manual addition
srdtrk May 16, 2023
b8db88a
imp(ica/host): made GetParams more economical
srdtrk May 16, 2023
9c26d08
fix(fee/test): handled ica's 'SetParams' error during testing
srdtrk May 16, 2023
b6bf06c
merge: remote-tracking branch 'origin' into aleem/3504-host-params
srdtrk May 16, 2023
e982710
imp(ica/host): using ibcerrors instead of relying on sdk's govtypes
srdtrk May 16, 2023
26a44df
style(ica/host): removed '*' from Migrator's keeper
srdtrk May 16, 2023
d51de4c
revert(ica/host): reverts to the previous commit as this is more cons…
srdtrk May 16, 2023
f712f93
fix(simapp): added ParamKeyTable to the icahost subspace in app.go fo…
srdtrk May 16, 2023
b67a173
feat(ica/host/test): added migrator_test.go
srdtrk May 16, 2023
904cd3f
style(ica/host/test): ran gofumpt
srdtrk May 16, 2023
525d431
feat(ica/host): passing legacySubspace to keeper instead
srdtrk May 18, 2023
6ff8f38
fix(ica/host): removed unneeded reference to hss from 'NewAppModule' …
srdtrk May 18, 2023
1ffbda6
fix(simapp): reduced modifications to simapp to 1
srdtrk May 18, 2023
6667d99
fix(ica/host/test): updated tests
srdtrk May 18, 2023
f30f703
style(ica/host): ran gofumpt
srdtrk May 18, 2023
6fbd88c
docs(ica/host): fixed a typo
srdtrk May 18, 2023
3d75714
style(ica/host): added a space for import separation
srdtrk May 18, 2023
de7739e
style(ica/host): renamed 'migrator.go' to 'migrations.go'
srdtrk May 18, 2023
9041fa4
refactor(ica/host): removed unneeded validate function
srdtrk May 18, 2023
f09f56c
merge: remote-tracking branch 'origin' into aleem/3504-host-params
srdtrk May 18, 2023
0b59b8e
style(ica/host): storing ParamsKey using a string
srdtrk May 18, 2023
20d3800
fix(ica/host): registered the msg_server in ica's module.go
srdtrk May 19, 2023
9019c73
style(ica/host): removed unneeded fmt variable assignment
srdtrk May 19, 2023
47e72c1
imp(ica/host/test): added more test cases, and refactored the test
srdtrk May 19, 2023
0413697
style(ica/host/test): ran gofumpt
srdtrk May 19, 2023
ac4566a
imp(ica/host): 'GetParams' panics now if it can't find the params
srdtrk May 20, 2023
5b18258
imp(core): added 'ErrInvalidAuthority'
srdtrk May 20, 2023
50f2b67
imp(ica/host): uses 'ErrInvalidAuthority'
srdtrk May 20, 2023
8a3f9f2
merge: remote-tracking branch 'origin' into aleem/3504-host-params
srdtrk May 20, 2023
ddf60d4
fix(ica/test): fixed test for the ica module
srdtrk May 20, 2023
c0ce6a3
imp(ica/host/test): added a new test case
srdtrk May 20, 2023
588fcbe
imp(ica/host/test): added params test to genesis
srdtrk May 20, 2023
a87581b
style(ica/host): ran gofumpt
srdtrk May 20, 2023
7ab5513
imp(ica/host/test): added unset param test to keeper
srdtrk May 20, 2023
d4b7010
docs(ica/host): added tracker issue for removing params_legacy.go
srdtrk May 20, 2023
c51e2c6
style(ica/host): improved test case naming
srdtrk May 20, 2023
5a75a43
fix(ica/host/test): fixed a minor inaccuracy with the test in the hop…
srdtrk May 20, 2023
3d3d8aa
style(ica/host): updated err message
srdtrk May 22, 2023
289855d
imp(ica/transfer, core/errors): removed ErrInvalidAuthority
srdtrk May 22, 2023
df22e88
style(ica/host): updated nil check to be more consistent
srdtrk May 22, 2023
708d5c8
style(transfer): renamed errors to errorsmod to avoid shadowing
srdtrk May 22, 2023
72bd14d
imp(ica/host/test): made test shorter
srdtrk May 22, 2023
2b0aaae
imp(ica/host/test): removed unneeded comment
srdtrk May 22, 2023
d804902
style(ica/host/test): removed test case field names
srdtrk May 22, 2023
ba74181
refactor(ica/host, fee/test): refactored Validate out of SetParams
srdtrk May 22, 2023
c806a23
style(ica/host): ran gofumpt
srdtrk May 22, 2023
c0f74e0
imp(ica/types): switched back to 'bz == nil'
srdtrk May 22, 2023
e19914f
style(ica/host/test): looks better
srdtrk May 22, 2023
212945a
merge: remote-tracking branch 'origin' into aleem/3504-host-params
srdtrk May 22, 2023
27c9029
style(ica/host): changed panic message
srdtrk May 22, 2023
a0884ca
imp(ica/host): removed redundant Validate from msg server, handled in…
srdtrk May 22, 2023
0bc9281
style(ica/host): added code comment
srdtrk May 22, 2023
ce98d84
fix(ica/host): increase consensus version
srdtrk May 22, 2023
e84d975
style(ica/host/test): moved success case to top
srdtrk May 22, 2023
32ebf33
style(ica/host/test): improve test styling
srdtrk May 22, 2023
db33a85
merge: remote-tracking branch 'origin' into aleem/3504-host-params
srdtrk May 23, 2023
61fee14
docs(migration): added changes in app.go
srdtrk May 23, 2023
2eb440c
imp(ica/host): improved the error message
srdtrk May 23, 2023
8586e2e
merge: remote-tracking branch 'origin' into aleem/3504-host-params
srdtrk May 23, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ Ref: https://keepachangelog.com/en/1.0.0/

### State Machine Breaking

* (modules/apps/27-interchain-accounts/host) [\#3520](https://github.com/cosmos/ibc-go/pull/3520) Migrate `modules/apps/27-interchain-accounts/host` to self-managed parameters and deprecate it's usage of `x/params`.
srdtrk marked this conversation as resolved.
Show resolved Hide resolved

### Improvements

* (tests) [\#3138](https://github.com/cosmos/ibc-go/pull/3138) Support benchmarks and fuzz tests through `testing.TB`.
Expand Down
18 changes: 18 additions & 0 deletions modules/apps/27-interchain-accounts/exported/exported.go
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
package exported

import (
sdk "github.com/cosmos/cosmos-sdk/types"
paramtypes "github.com/cosmos/cosmos-sdk/x/params/types"
)

type (
ParamSet = paramtypes.ParamSet

// Subspace defines an interface that implements the legacy x/params Subspace
// type.
//
// NOTE: This is used solely for migration of x/params managed parameters.
Subspace interface {
GetParamSet(ctx sdk.Context, ps ParamSet)
}
)
29 changes: 15 additions & 14 deletions modules/apps/27-interchain-accounts/host/keeper/keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,9 @@ import (
"github.com/cosmos/cosmos-sdk/codec"
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"

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"
porttypes "github.com/cosmos/ibc-go/v7/modules/core/05-port/types"
Expand All @@ -22,9 +20,8 @@ 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

ics4Wrapper porttypes.ICS4Wrapper
channelKeeper icatypes.ChannelKeeper
Expand All @@ -34,34 +31,33 @@ 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,
ics4Wrapper porttypes.ICS4Wrapper, channelKeeper icatypes.ChannelKeeper, portKeeper icatypes.PortKeeper,
accountKeeper icatypes.AccountKeeper, scopedKeeper exported.ScopedKeeper, msgRouter icatypes.MessageRouter,
cdc codec.BinaryCodec, key storetypes.StoreKey, 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())
}

return Keeper{
storeKey: key,
cdc: cdc,
paramSpace: paramSpace,
ics4Wrapper: ics4Wrapper,
channelKeeper: channelKeeper,
portKeeper: portKeeper,
accountKeeper: accountKeeper,
scopedKeeper: scopedKeeper,
msgRouter: msgRouter,
authority: authority,
}
}

Expand Down Expand Up @@ -199,3 +195,8 @@ 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
}
29 changes: 29 additions & 0 deletions modules/apps/27-interchain-accounts/host/keeper/migrator.go
srdtrk marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
package keeper

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

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

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

// Migrate2to3 migrates the 27-interchain-accounts module state from the
// consensus version 2 to version 3. Specifically, it takes the parameters that
// are currently stored and managed by the x/params modules and stores them directly
// into the host submodule state.
func (m Migrator) Migrate2to3(ctx sdk.Context) error {
return v8.Migrate(ctx, ctx.KVStore(m.keeper.storeKey), m.legacySubspace, m.keeper.cdc)
}
34 changes: 34 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,34 @@
package keeper

import (
"context"

"cosmossdk.io/errors"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

errorsmod is the import alias we use

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree, we should avoid shadowing pkgs from std lib

srdtrk marked this conversation as resolved.
Show resolved Hide resolved
sdk "github.com/cosmos/cosmos-sdk/types"
govtypes "github.com/cosmos/cosmos-sdk/x/gov/types"
"github.com/cosmos/ibc-go/v7/modules/apps/27-interchain-accounts/host/types"
)

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}
}
Comment on lines +15 to +23
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this construction was only used for the controller submodule to avoid name collision of the SendTx and RegisterInterchainAccount functions (one function signature is legacy/deprecated now)

Maybe we should just use the keeper directly?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm kind of indifferent on this - using the unexported msgServer type does reduce the surface area for issues of other random modules doing something they shouldn't be 🤷


// 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, errors.Wrapf(govtypes.ErrInvalidSigner, "invalid authority; expected %s, got %s", m.authority, msg.Authority)
}

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

return &types.MsgUpdateParamsResponse{}, nil
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
package keeper_test

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

func (suite *KeeperTestSuite) TestUpdateParams() {
msg := types.MsgUpdateParams{}

testCases := []struct {
name string
malleate func(authority string)
expPass bool
}{
{
"invalid authority address",
func(authority string) {
msg.Authority = "authority"
msg.Params = types.DefaultParams()
},
false,
},
{
"success",
srdtrk marked this conversation as resolved.
Show resolved Hide resolved
func(authority string) {
msg.Authority = authority
msg.Params = types.DefaultParams()
},
true,
},
}

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

suite.Run(tc.name, func() {
suite.SetupTest()

ICAHostKeeper := &suite.chainA.GetSimApp().ICAHostKeeper
tc.malleate(ICAHostKeeper.GetAuthority()) // malleate mutates test data

ctx := suite.chainA.GetContext()
msgServer := keeper.NewMsgServerImpl(ICAHostKeeper)
res, err := msgServer.UpdateParams(ctx, &msg)

if tc.expPass {
suite.Require().NoError(err)
suite.Require().NotNil(res)
} else {
suite.Require().Error(err)
suite.Require().Nil(res)
}
})
}
}
41 changes: 33 additions & 8 deletions modules/apps/27-interchain-accounts/host/keeper/params.go
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Imo, it makes more sense for these functions to be in keeper.go instead.

Remove the IsHostEnabled function all together, imo, it is wrong to return false if there is a problem unmarshalling. This case should be handled in GetParams, it seems like you're re-implementing GetParams, unnecessary code duplication

Original file line number Diff line number Diff line change
Expand Up @@ -9,24 +9,49 @@ import (
// IsHostEnabled retrieves the host enabled boolean from the paramstore.
// True is returned if the host submodule is enabled.
func (k Keeper) IsHostEnabled(ctx sdk.Context) bool {
var res bool
k.paramSpace.Get(ctx, types.KeyHostEnabled, &res)
return res
var params types.Params
store := ctx.KVStore(k.storeKey)
bz := store.Get(types.ParamsKey)
if bz == nil {
return false
}

if err := k.cdc.Unmarshal(bz, &params); err != nil {
return false
}

return params.HostEnabled
}

// GetAllowMessages retrieves the host enabled msg types from the paramstore
func (k Keeper) GetAllowMessages(ctx sdk.Context) []string {
var res []string
k.paramSpace.Get(ctx, types.KeyAllowMessages, &res)
return res
var params types.Params
store := ctx.KVStore(k.storeKey)
bz := store.Get(types.ParamsKey)
if bz == nil {
return []string{}
}

k.cdc.MustUnmarshal(bz, &params)
return params.AllowMessages
}

// GetParams returns the total set of the host submodule parameters.
func (k Keeper) GetParams(ctx sdk.Context) types.Params {
return types.NewParams(k.IsHostEnabled(ctx), k.GetAllowMessages(ctx))
var params types.Params
store := ctx.KVStore(k.storeKey)
bz := store.Get(types.ParamsKey)
if bz == nil {
return 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) {
k.paramSpace.SetParamSet(ctx, &params)
store := ctx.KVStore(k.storeKey)
bz := k.cdc.MustMarshal(&params)
store.Set(types.ParamsKey, bz)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
package v8

import (
storetypes "github.com/cosmos/cosmos-sdk/store/types"

"github.com/cosmos/cosmos-sdk/codec"
sdk "github.com/cosmos/cosmos-sdk/types"

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

const (
SubModuleName = "icahost"
)

var ParamsKey = []byte{0x00}

// Migrate migrates the 27-interchain-accounts host submodule state from the consensus version 1 to
// version 2. Specifically, it takes the parameters that are currently stored
aleem1314 marked this conversation as resolved.
Show resolved Hide resolved
// and managed by the x/params modules and stores them directly into the host
// submodule state.
func Migrate(
ctx sdk.Context,
store storetypes.KVStore,
legacySubspace exported.Subspace,
cdc codec.BinaryCodec,
) error {
var currParams types.Params
legacySubspace.GetParamSet(ctx, &currParams)

if err := currParams.Validate(); err != nil {
return err
}

bz := cdc.MustMarshal(&currParams)
store.Set(ParamsKey, bz)

return nil
}
3 changes: 3 additions & 0 deletions modules/apps/27-interchain-accounts/host/types/keys.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@ import (
sdk "github.com/cosmos/cosmos-sdk/types"
)

// ParamsKey is the key to use for the storing params.
var ParamsKey = []byte{0x00}
colin-axner marked this conversation as resolved.
Show resolved Hide resolved

const (
// SubModuleName defines the interchain accounts host module name
SubModuleName = "icahost"
Expand Down
25 changes: 25 additions & 0 deletions modules/apps/27-interchain-accounts/host/types/msgs.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
package types

import sdk "github.com/cosmos/cosmos-sdk/types"

var _ sdk.Msg = (*MsgUpdateParams)(nil)

// ValidateBasic implements sdk.Msg
func (msg MsgUpdateParams) ValidateBasic() error {
_, err := sdk.AccAddressFromBech32(msg.Authority)
if err != nil {
return err
srdtrk marked this conversation as resolved.
Show resolved Hide resolved
}

return msg.Params.Validate()
}

// GetSigners implements sdk.Msg
func (msg MsgUpdateParams) GetSigners() []sdk.AccAddress {
accAddr, err := sdk.AccAddressFromBech32(msg.Authority)
if err != nil {
panic(err)
}

return []sdk.AccAddress{accAddr}
}
Loading