fix: avoid panic when migrate param for newly added host #6167
Conversation
WalkthroughWalkthroughThe updates focus on enhancing the parameter migration process in the Changes
Recent Review DetailsConfiguration used: .coderabbit.yml Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
mmsqe
requested review from
colin-axner,
AdityaSripal,
damiannolan,
chatton,
crodriguezvega,
charleenfei and
DimitrisJim
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Actionable comments outside the diff hunks (1)
CHANGELOG.md (1)
Line range hint
161-161: Replace the bare URL with a markdown link for better readability and to avoid potential issues with text-based parsers.- * (apps/27-interchain-accounts) [\#6167](https://github.com/cosmos/ibc-go/pull/6167) Avoid panic when migrate param for newly added host. + * (apps/27-interchain-accounts) [[\#6167](https://github.com/cosmos/ibc-go/pull/6167)](https://github.com/cosmos/ibc-go/pull/6167) Avoid panic when migrate param for newly added host.
|
Hi @mmsqe, did you encounter a panic in the scenario you've suggested with the PR title? We normally encourage a short description when opening PRs that do not have an existing issue.
edit: The scenario here is that a chain includes ica controller but does not include host functionality, in an upgrade the ica host is enabled and subkeeper is added. It is the same release which includes the params migration. This means an entry in the |
Thanks @damiannolan, you're right, just updated the description, see if it's ok. |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
|
||
| if err := params.Validate(); err != nil { | ||
| return err | ||
| if m.keeper.legacySubspace == nil { |
There was a problem hiding this comment.
Just to clarify when icahost was added in your upgrade it was created with a nil legacy subspace? @mmsqe
There was a problem hiding this comment.
we bypass param migrate with a mockSubspace, but we could change to nil when generic fix is backported
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
does it error in that case? 🤔 An empty legacySubspace would init params as {true, []string{}} which shouldn't fail validation.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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, ¶ms)
}
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Do we want to align IfExists for controller in this PR?
There was a problem hiding this comment.
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! 🙏
| type ParamSubspace interface { | ||
| GetParamSet(ctx sdk.Context, ps paramtypes.ParamSet) |
There was a problem hiding this comment.
Sorry to be a pain but can we keep GetParamSet since it would technically be an API breaking change? Happy to just add the IfExists method as addition, but I think we should keep this in order to preserve our release guarantees when backporting to v8.3
* 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 <carlos@interchain.io> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Damian Nolan <damiannolan@gmail.com> (cherry picked from commit c4413c5)
) (#6192) * 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 <carlos@interchain.io> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Damian Nolan <damiannolan@gmail.com> (cherry picked from commit c4413c5) * fix: compiler error for NewKeeper args --------- Co-authored-by: mmsqe <mavis@crypto.com> Co-authored-by: Damian Nolan <damiannolan@gmail.com>
Description
Problem
Solution
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/).godoccomments.Files changedin the GitHub PR explorer.SonarCloud Reportin the comment section below once CI passes.Summary by CodeRabbit