Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Enact ImOnline migration #7403

Closed
wants to merge 2 commits into from
Closed

Enact ImOnline migration #7403

wants to merge 2 commits into from

Conversation

liamaharon
Copy link
Contributor

@liamaharon liamaharon commented Jun 20, 2023 •
edited
Loading

Need to set the onchain version for the pallet as part of fixing try-on-runtime-upgrade Kusama & Westend checks.

2023-06-20 13:39:37.179 ERROR main runtime::frame-support: ImOnline: On chain storage version StorageVersion(0) doesn't match current storage version StorageVersion(1).

todo:

  • check if rococo and polkadot also need this migration

@liamaharon liamaharon requested a review from bkchr June 20, 2023 11:44
@paritytech-ci paritytech-ci requested review from a team June 20, 2023 11:44
@liamaharon liamaharon added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit. E0-runtime_migration PR introduces code that might require downstream chains to run a runtime upgrade. labels Jun 20, 2023
@liamaharon liamaharon mentioned this pull request Jun 20, 2023
Merged
2 tasks
@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable
Logs: https://gitlab.parity.io/parity/mirrors/polkadot/-/jobs/3032433

kianenigma
kianenigma approved these changes Jun 20, 2023
View reviewed changes
Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

Can't we have a blanket storage version setter that receive the list (or tuple) of all pallets and sets the correct version for all of them, and we run that once?

Or even run that always?

@paritytech-ci paritytech-ci requested a review from a team June 20, 2023 12:23
ggwpez
ggwpez approved these changes Jun 20, 2023
View reviewed changes
Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

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

+1 for a generic wrapper

@liamaharon liamaharon changed the title Set ImOnline onchain version Enact ImOnline migration Jun 20, 2023
@liamaharon liamaharon marked this pull request as draft June 20, 2023 12:51
@liamaharon
Copy link
Contributor Author

I don't think we should not use a generic wrapper because it would risk us bumping the on-chain version to the current version if someone forgot to include the migration in the Migrations tuple, like I was right about to do in this PR :)

#7309 added a migration for ImOnline. Adding it to Unreleased is currently in review currently here: #7387

Closing in favour of that PR.

@liamaharon liamaharon closed this Jun 20, 2023
@liamaharon liamaharon deleted the liam-set-imonline-version branch June 20, 2023 12:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ggwpez ggwpez ggwpez approved these changes

@kianenigma kianenigma kianenigma approved these changes

@bkchr bkchr Awaiting requested review from bkchr

Assignees
No one assigned
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit. E0-runtime_migration PR introduces code that might require downstream chains to run a runtime upgrade.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@liamaharon @paritytech-cicd-pr @kianenigma @ggwpez