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 53 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
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
15 changes: 10 additions & 5 deletions modules/apps/27-interchain-accounts/host/ibc_module_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,8 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenTry() {
},
{
"host submodule disabled", func() {
suite.chainB.GetSimApp().ICAHostKeeper.SetParams(suite.chainB.GetContext(), types.NewParams(false, []string{}))
err := suite.chainB.GetSimApp().ICAHostKeeper.SetParams(suite.chainB.GetContext(), types.NewParams(false, []string{}))
suite.Require().NoError(err)
}, false,
},
{
Expand Down Expand Up @@ -273,7 +274,8 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenConfirm() {
},
{
"host submodule disabled", func() {
suite.chainB.GetSimApp().ICAHostKeeper.SetParams(suite.chainB.GetContext(), types.NewParams(false, []string{}))
err := suite.chainB.GetSimApp().ICAHostKeeper.SetParams(suite.chainB.GetContext(), types.NewParams(false, []string{}))
suite.Require().NoError(err)
}, false,
},
{
Expand Down Expand Up @@ -399,7 +401,8 @@ func (suite *InterchainAccountsTestSuite) TestOnRecvPacket() {
},
{
"host submodule disabled", func() {
suite.chainB.GetSimApp().ICAHostKeeper.SetParams(suite.chainB.GetContext(), types.NewParams(false, []string{}))
err := suite.chainB.GetSimApp().ICAHostKeeper.SetParams(suite.chainB.GetContext(), types.NewParams(false, []string{}))
suite.Require().NoError(err)
}, false,
},
{
Expand Down Expand Up @@ -464,7 +467,8 @@ func (suite *InterchainAccountsTestSuite) TestOnRecvPacket() {
expectedAck := channeltypes.NewResultAcknowledgement(expectedTxResponse)

params := types.NewParams(true, []string{sdk.MsgTypeURL(msg)})
suite.chainB.GetSimApp().ICAHostKeeper.SetParams(suite.chainB.GetContext(), params)
err = suite.chainB.GetSimApp().ICAHostKeeper.SetParams(suite.chainB.GetContext(), params)
suite.Require().NoError(err)

// malleate packetData for test cases
tc.malleate()
Expand Down Expand Up @@ -661,7 +665,8 @@ func (suite *InterchainAccountsTestSuite) TestControlAccountAfterChannelClose()
}

params := types.NewParams(true, []string{sdk.MsgTypeURL(msg)})
suite.chainB.GetSimApp().ICAHostKeeper.SetParams(suite.chainB.GetContext(), params)
err = suite.chainB.GetSimApp().ICAHostKeeper.SetParams(suite.chainB.GetContext(), params)
suite.Require().NoError(err)

//nolint: staticcheck // SA1019: ibctesting.FirstConnectionID is deprecated: use path.EndpointA.ConnectionID instead. (staticcheck)
_, err = suite.chainA.GetSimApp().ICAControllerKeeper.SendTx(suite.chainA.GetContext(), nil, ibctesting.FirstConnectionID, path.EndpointA.ChannelConfig.PortID, icaPacketData, ^uint64(0))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,9 @@ func InitGenesis(ctx sdk.Context, keeper Keeper, state genesistypes.HostGenesisS
keeper.SetInterchainAccountAddress(ctx, acc.ConnectionId, acc.PortId, acc.AccountAddress)
}

keeper.SetParams(ctx, state.Params)
if err := keeper.SetParams(ctx, state.Params); err != nil {
panic(fmt.Sprintf("could not set ica host params at genesis: %v", err))
}
srdtrk marked this conversation as resolved.
Show resolved Hide resolved
}

// ExportGenesis returns the interchain accounts host exported genesis
Expand Down
60 changes: 60 additions & 0 deletions modules/apps/27-interchain-accounts/host/keeper/genesis_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,66 @@ func (suite *KeeperTestSuite) TestInitGenesis() {
suite.Require().Equal(expParams, params)
}

func (suite *KeeperTestSuite) TestGenesisParams() {
testCases := []struct {
name string
input types.Params
expPass bool
}{
{name: "success: set default params", input: types.DefaultParams(), expPass: true},
{name: "success: non-default params", input: types.NewParams(!types.DefaultHostEnabled, []string{"/cosmos.staking.v1beta1.MsgDelegate"}), expPass: true},
{name: "success: set empty allowMsg", input: types.NewParams(true, nil), expPass: true},
srdtrk marked this conversation as resolved.
Show resolved Hide resolved
{name: "failure: set empty string", input: types.NewParams(true, []string{""}), expPass: false},
srdtrk marked this conversation as resolved.
Show resolved Hide resolved
{name: "failure: set space string", input: types.NewParams(true, []string{" "}), expPass: false},
srdtrk marked this conversation as resolved.
Show resolved Hide resolved
}

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
66 changes: 51 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,33 @@ 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 {
panic("ica/host params have not been set")
}

srdtrk marked this conversation as resolved.
Show resolved Hide resolved
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) error {
if err := params.Validate(); err != nil {
return err
}
srdtrk marked this conversation as resolved.
Show resolved Hide resolved
srdtrk marked this conversation as resolved.
Show resolved Hide resolved

store := ctx.KVStore(k.storeKey)
bz := k.cdc.MustMarshal(&params)
store.Set([]byte(types.ParamsKey), bz)
return nil
}
61 changes: 61 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 @@ -3,12 +3,19 @@ package keeper_test
import (
"testing"

dbm "github.com/cometbft/cometbft-db"
"github.com/cometbft/cometbft/libs/log"
tmproto "github.com/cometbft/cometbft/proto/tendermint/types"
"github.com/cosmos/cosmos-sdk/baseapp"
simtestutil "github.com/cosmos/cosmos-sdk/testutil/sims"
"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"
"github.com/cosmos/ibc-go/v7/testing/simapp"
)

var (
Expand Down Expand Up @@ -218,3 +225,57 @@ 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
}{
{name: "success: set default params", input: types.DefaultParams(), expPass: true},
{name: "success: non-default params", input: types.NewParams(!types.DefaultHostEnabled, []string{"/cosmos.staking.v1beta1.MsgDelegate"}), expPass: true},
{name: "success: set empty allowMsg", input: types.NewParams(true, nil), expPass: true},
{name: "failure: set empty string", input: types.NewParams(true, []string{""}), expPass: false},
{name: "failure: set space string", input: types.NewParams(true, []string{" "}), expPass: false},
}

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

suite.Run(tc.name, func() {
suite.SetupTest() // reset
ctx := suite.chainA.GetContext()
err := 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() {
// this state should not be possible outside of tests
chainID := "testchain"
header := tmproto.Header{
ChainID: chainID,
Height: 1,
Time: suite.coordinator.CurrentTime.UTC(),
}

app := simapp.NewSimApp(log.NewNopLogger(), dbm.NewMemDB(), nil, true, map[int64]bool{}, simapp.DefaultNodeHome, 5, simapp.MakeTestEncodingConfig(), simtestutil.EmptyAppOptions{}, baseapp.SetChainID(chainID))
ctx := app.GetBaseApp().NewContext(true, header)
srdtrk marked this conversation as resolved.
Show resolved Hide resolved

suite.Require().Panics(func() {
app.ICAHostKeeper.GetParams(ctx)
})
}
26 changes: 26 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,26 @@
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)

return m.keeper.SetParams(ctx, params)
}
Loading