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.

Sign up for GitHub

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

fix: avoid panic when migrate param for newly added host #6167

Merged
merged 17 commits into from
Apr 22, 2024
Merged

fix: avoid panic when migrate param for newly added host #6167

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
3d09829
fix: avoid panic when migrate param for newly added host
mmsqe Apr 16, 2024
0f52582
keep default params
mmsqe Apr 16, 2024
ef89bbe
Apply suggestions from code review
mmsqe Apr 16, 2024
c140b93
Merge remote-tracking branch 'origin/main' into host_params
mmsqe Apr 16, 2024
eb28d2c
allow use default params when set nil legacySubspace
mmsqe Apr 16, 2024
f33b221
Merge remote-tracking branch 'origin/main' into host_params
mmsqe Apr 17, 2024
215785d
Update CHANGELOG.md
Apr 18, 2024
3ffc86c
Update CHANGELOG.md
Apr 18, 2024
338df0b
cleanup
mmsqe Apr 17, 2024
debe264
Merge remote-tracking branch 'origin/main' into host_params
mmsqe Apr 18, 2024
8cb18ce
refactor: rm setter in icahost migrator and adjust test case
damiannolan Apr 18, 2024
5c1b9ec
chore: update changelog
damiannolan Apr 18, 2024
4142567
Merge branch 'main' into host_params
damiannolan Apr 18, 2024
0c5c440
Apply suggestions from code review
mmsqe Apr 22, 2024
1ba7d37
Merge remote-tracking branch 'origin/main' into host_params
mmsqe Apr 22, 2024
7f45153
Apply suggestions from code review
mmsqe Apr 22, 2024
c2d0397
Apply suggestions from code review
mmsqe Apr 22, 2024
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

## [v8.2.0](https://github.com/cosmos/ibc-go/releases/tag/v8.2.0) - 2024-04-05

### Dependencies
Expand Down
11 changes: 7 additions & 4 deletions modules/apps/27-interchain-accounts/host/keeper/migrations.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,13 @@ func NewMigrator(k *Keeper) Migrator {
func (m Migrator) MigrateParams(ctx sdk.Context) error {
if m.keeper != nil {
var params types.Params
m.keeper.legacySubspace.GetParamSet(ctx, &params)

if err := params.Validate(); err != nil {
return err
if m.keeper.legacySubspace == nil {
Copy link
Member

Choose a reason for hiding this comment

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

Just to clarify when icahost was added in your upgrade it was created with a nil legacy subspace? @mmsqe

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we bypass param migrate with a mockSubspace, but we could change to nil when generic fix is backported

Copy link
Member

Choose a reason for hiding this comment

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

Okay, thank you! In that case this should be fine.

I'm curious about a case in which an actual params subspace is created and passed but which does not have any params stored in the param set from a previous version. As genesis would already have been run for the ica module but not setup default params I would imagine it to be empty. I assume in that case params.Validate() should error below, and this would technically misconfiguration by the chain's app wiring.

Copy link
Contributor

@DimitrisJim DimitrisJim Apr 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

does it error in that case? 🤔 An empty legacySubspace would init params as {true, []string{}} which shouldn't fail validation.

Copy link
Member

Choose a reason for hiding this comment

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

Aha, yea I believe you're right @DimitrisJim. With RegisterParamSet(&Params{}), right?

That's probably fine behaviour imo - given this would probably be the result of misconfig anyways - a param's change would need to be done for allowing msgs in []string then. This is a pretty edge case scenario either way. Most chains deploy host first anyways, and controller later

Copy link
Member

Choose a reason for hiding this comment

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

Wondering if this migration code would be safer and cover edge cases such as described above (this would at least init params with all msgs allowed instead of the empty string slice, its a bit more explicit than relying on a side effect of RegisterParamSet):

if m.keeper != nil {
        params := types.DefaultParams()
	if m.keeper.legacySubspace != nil {
		m.keeper.legacySubspace.GetParamSetIfExists(ctx, &params)
	}

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

	m.keeper.SetParams(ctx, params)
	m.keeper.Logger(ctx).Info("successfully migrated ica/host submodule to self-manage params")
}

Any thoughts? Happy to stick with the current changes tho if agreed.

Copy link
Contributor

Choose a reason for hiding this comment

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

reads clearer to me! Would we want to use IfExists variant though? I'd assume we'd want to panic if we try to get something and it doesn't exist.

One sidenote here, should we be doing same sanity checks for controller?

Copy link
Contributor Author

@mmsqe mmsqe Apr 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Do we want to align IfExists for controller in this PR?

Copy link
Member

Choose a reason for hiding this comment

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

I'd assume we'd want to panic if we try to get something and it doesn't exist.

I'm not sure, and I don't feel particularly strongly about this - if it doesn't exist we've at least assigned safe defaults via types.DefaultParams rather than panicking. Do you have any feedback here @mmsqe, as someone maintaining a chain?

One sidenote here, should we be doing same sanity checks for controller?

@mmsqe yes I think it would be nice to align the controller migration code as well, thank you! 🙏

params = types.DefaultParams()
} else {
m.keeper.legacySubspace.GetParamSet(ctx, &params)
if err := params.Validate(); err != nil {
return err
}
}
m.keeper.SetParams(ctx, params)
m.keeper.Logger(ctx).Info("successfully migrated ica/host submodule to self-manage params")
Expand Down
22 changes: 22 additions & 0 deletions modules/apps/27-interchain-accounts/host/keeper/migrations_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand All @@ -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 {
Expand Down
Loading