From c4413c5877f9ef883494da1721cb18caaba7f7f5 Mon Sep 17 00:00:00 2001 From: mmsqe Date: Mon, 22 Apr 2024 19:50:20 +0800 Subject: [PATCH] fix: avoid panic when migrate param for newly added host (#6167) * fix: avoid panic when migrate param for newly added host * keep default params * Apply suggestions from code review * allow use default params when set nil legacySubspace * Update CHANGELOG.md Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Update CHANGELOG.md * cleanup * refactor: rm setter in icahost migrator and adjust test case * chore: update changelog * Apply suggestions from code review * Apply suggestions from code review * Apply suggestions from code review --------- Co-authored-by: Carlos Rodriguez Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Damian Nolan --- CHANGELOG.md | 2 ++ .../controller/keeper/migrations.go | 7 +++--- .../controller/keeper/migrations_test.go | 17 ++++++++++++++ .../host/keeper/migrations.go | 7 +++--- .../host/keeper/migrations_test.go | 22 +++++++++++++++++++ .../types/expected_keepers.go | 1 + 6 files changed, 50 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3d94e357b2d..7d88d921e18 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -60,6 +60,8 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Bug Fixes +* (apps/27-interchain-accounts) [\#6167](https://github.com/cosmos/ibc-go/pull/6167) Fixed an edge case bug where migrating params for a pre-existing ica module which implemented controller functionality only could panic when migrating params for newly added host, and align controller param migration with host. + ## [v8.2.0](https://github.com/cosmos/ibc-go/releases/tag/v8.2.0) - 2024-04-05 ### Dependencies diff --git a/modules/apps/27-interchain-accounts/controller/keeper/migrations.go b/modules/apps/27-interchain-accounts/controller/keeper/migrations.go index 705d93cb4b4..77f62a73c5d 100644 --- a/modules/apps/27-interchain-accounts/controller/keeper/migrations.go +++ b/modules/apps/27-interchain-accounts/controller/keeper/migrations.go @@ -55,9 +55,10 @@ func (m Migrator) AssertChannelCapabilityMigrations(ctx sdk.Context) error { // 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, ¶ms) - + params := controllertypes.DefaultParams() + if m.keeper.legacySubspace != nil { + m.keeper.legacySubspace.GetParamSetIfExists(ctx, ¶ms) + } m.keeper.SetParams(ctx, params) m.keeper.Logger(ctx).Info("successfully migrated ica/controller submodule to self-manage params") } diff --git a/modules/apps/27-interchain-accounts/controller/keeper/migrations_test.go b/modules/apps/27-interchain-accounts/controller/keeper/migrations_test.go index da5be84b6f5..f0011439d14 100644 --- a/modules/apps/27-interchain-accounts/controller/keeper/migrations_test.go +++ b/modules/apps/27-interchain-accounts/controller/keeper/migrations_test.go @@ -92,6 +92,23 @@ func (suite *KeeperTestSuite) TestMigratorMigrateParams() { }, icacontrollertypes.DefaultParams(), }, + { + "success: no legacy params pre-migration", + func() { + suite.chainA.GetSimApp().ICAControllerKeeper = icacontrollerkeeper.NewKeeper( + suite.chainA.Codec, + suite.chainA.GetSimApp().GetKey(icacontrollertypes.StoreKey), + nil, // assign a nil legacy param subspace + suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper, + suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper, + suite.chainA.GetSimApp().IBCKeeper.PortKeeper, + suite.chainA.GetSimApp().ScopedICAControllerKeeper, + suite.chainA.GetSimApp().MsgServiceRouter(), + suite.chainA.GetSimApp().ICAControllerKeeper.GetAuthority(), + ) + }, + icacontrollertypes.DefaultParams(), + }, } for _, tc := range testCases { diff --git a/modules/apps/27-interchain-accounts/host/keeper/migrations.go b/modules/apps/27-interchain-accounts/host/keeper/migrations.go index 894018606c7..992027d92f1 100644 --- a/modules/apps/27-interchain-accounts/host/keeper/migrations.go +++ b/modules/apps/27-interchain-accounts/host/keeper/migrations.go @@ -21,9 +21,10 @@ func NewMigrator(k *Keeper) Migrator { // MigrateParams migrates the host submodule's parameters from the x/params to self store. func (m Migrator) MigrateParams(ctx sdk.Context) error { if m.keeper != nil { - var params types.Params - m.keeper.legacySubspace.GetParamSet(ctx, ¶ms) - + params := types.DefaultParams() + if m.keeper.legacySubspace != nil { + m.keeper.legacySubspace.GetParamSetIfExists(ctx, ¶ms) + } if err := params.Validate(); err != nil { return err } diff --git a/modules/apps/27-interchain-accounts/host/keeper/migrations_test.go b/modules/apps/27-interchain-accounts/host/keeper/migrations_test.go index e47e40f100f..1f15f947aab 100644 --- a/modules/apps/27-interchain-accounts/host/keeper/migrations_test.go +++ b/modules/apps/27-interchain-accounts/host/keeper/migrations_test.go @@ -3,6 +3,9 @@ package keeper_test import ( "fmt" + authtypes "github.com/cosmos/cosmos-sdk/x/auth/types" + govtypes "github.com/cosmos/cosmos-sdk/x/gov/types" + icahostkeeper "github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/host/keeper" icahosttypes "github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/host/types" ) @@ -22,6 +25,25 @@ func (suite *KeeperTestSuite) TestMigratorMigrateParams() { }, icahosttypes.DefaultParams(), }, + { + "success: no legacy params pre-migration", + func() { + suite.chainA.GetSimApp().ICAHostKeeper = icahostkeeper.NewKeeper( + suite.chainA.Codec, + suite.chainA.GetSimApp().GetKey(icahosttypes.StoreKey), + nil, // assign a nil legacy param subspace + suite.chainA.GetSimApp().IBCFeeKeeper, + suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper, + suite.chainA.GetSimApp().IBCKeeper.PortKeeper, + suite.chainA.GetSimApp().AccountKeeper, + suite.chainA.GetSimApp().ScopedICAHostKeeper, + suite.chainA.GetSimApp().MsgServiceRouter(), + suite.chainA.GetSimApp().GRPCQueryRouter(), + authtypes.NewModuleAddress(govtypes.ModuleName).String(), + ) + }, + icahosttypes.DefaultParams(), + }, } for _, tc := range testCases { diff --git a/modules/apps/27-interchain-accounts/types/expected_keepers.go b/modules/apps/27-interchain-accounts/types/expected_keepers.go index de2eb9de245..7ea7bc795d7 100644 --- a/modules/apps/27-interchain-accounts/types/expected_keepers.go +++ b/modules/apps/27-interchain-accounts/types/expected_keepers.go @@ -37,4 +37,5 @@ type PortKeeper interface { // ParamSubspace defines the expected Subspace interface for module parameters. type ParamSubspace interface { GetParamSet(ctx sdk.Context, ps paramtypes.ParamSet) + GetParamSetIfExists(ctx sdk.Context, ps paramtypes.ParamSet) }