This repository has been archived by the owner on Nov 15, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Remove xcm on_runtime_upgrade pallet hook #7235
Merged
Merged
Changes from 1 commit
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
518f7df
move migration stuffs
pgherveou 35c0800
Apply suggestions from code review
pgherveou 4e3f15f
Merge branch 'master' into pg/move-xcm-upgrade
pgherveou c4fb47b
Merge branch 'master' into pg/move-xcm-upgrade
pgherveou bc41403
Fix test
pgherveou 011f017
fix
pgherveou a8772b8
lint
pgherveou f07d01a
fix lint
pgherveou fa9ad9a
rm extra space
pgherveou 6fad87b
fix lint
pgherveou 92a2d9f
Merge branch 'master' into pg/move-xcm-upgrade
pgherveou f279726
PR review
pgherveou bd6615b
fixes
pgherveou ed26d11
use saturating_accrue in fn
pgherveou 6f58baf
fix test
pgherveou File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not 100% sure if the migration has already happened in all of the runtimes, but removing this here without adding the migration back to the runtimes would prevent any migration from happening. Have you checked what the onchain storage version is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's hard to check for all of them. Every migration should be tested with try-runtime which will yell at you if there is a storage mismatch for any pallet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a list of runtimes we can check off before merging this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I initially did that in this PR, but after reviews decided to only apply it to the chain I was upgrading (contracts-rococo)
paritytech/cumulus#2570, (see also other earlier commit where I apply it to all runtimes paritytech/cumulus@d890570)
The reasoning is that adding a migration that have already been applied, adds some unnecessary bloat to the wasm runtime, so the best practice should be to test with try-runtime and see what needs to be included.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the basic ones that we care about the most would do, e.g. polkadot, kusama, westend, rococo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
following up, here since @vstam1 is trying to close all pending issues before the repo migration.
This logic is moved to the Migration Struct that should be included in the
frame_executive::Executive
when a migration needs to be passed.My understanding is that
on_runtime_upgrade
hooks are deprecated and the migration logic should move out of the pallet into these Migration structs so that they can be controlled and configured by runtime authors.This is why I opened this PR in the first place, so that
pallet_xcm
adheres to this new conventions and integrate better with tools like try-runtimeThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but what we're saying here in this comment is whether or not the existing runtimes that we care about is already on storage version 1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this should impact whether or not we merge this PR?
The code here was moved to MigrateToV1.
If one runtime is not on v1 then it should run the upgrade by passing MigrateToV1 to frame_executive
if it's already on V1, then it should not use MigrateToV1.
In both scenarios dry-running the migration with try-runtime will tell you if something is missing or superfluous.
Feel free to close to unblock the repo migration if I got that wrong