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

Upgraded ICS to 4.0 #1153

Merged
merged 4 commits into from
Mar 21, 2024
Merged

Upgraded ICS to 4.0 #1153

merged 4 commits into from
Mar 21, 2024

Conversation

sampocs
Copy link
Collaborator

@sampocs sampocs commented Mar 21, 2024

Context

Upgraded ICS to v4

Brief Changelog

  • Changed ICS version and import paths
  • Added migrations to upgrade handler
  • Fixed unit tests

@sampocs sampocs requested a review from asalzmann March 21, 2024 02:01
@github-actions github-actions bot added C:app-wiring C:CLI C:stakeibc C:test dependencies Pull requests that update a dependency file labels Mar 21, 2024
Copy link
Contributor

@asalzmann asalzmann left a comment

Choose a reason for hiding this comment

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

LGTM


// Migrates ICS Params to add the new RetryDelayParam
func MigrateICSParams(ctx sdk.Context, ck ccvconsumerkeeper.Keeper) {
params := ck.GetConsumerParams(ctx)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice - so this didn't work pre-SDK 47, and now it does?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

that's my guess. Although, I think it might be up to the specific module. For instance, Stride as a chain is on SDK 47 but I don't think stakeibc ever upgraded the param store to the new format (low confidence though)

Either way, we can figure out if this works right away through the dockernet IG tests

@sampocs sampocs merged commit 27c8a71 into main Mar 21, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:app-wiring C:CLI C:stakeibc C:test dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants