Remove xcm on_runtime_upgrade pallet hook #7235
Conversation
bkchr
added
the
E0-runtime_migration
Co-authored-by: Bastian Köcher <git@kchr.de>
pgherveou
added
A0-please_review
| @@ -685,12 +685,6 @@ pub mod pallet { | |||
| } | |||
| weight_used | |||
| } | |||
| fn on_runtime_upgrade() -> Weight { | |||
There was a problem hiding this comment.
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.
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.
Is there a list of runtimes we can check off before merging this?
There was a problem hiding this comment.
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.
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.
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-runtime
There was a problem hiding this comment.
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.
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
| @@ -685,12 +685,6 @@ pub mod pallet { | |||
| } | |||
| weight_used | |||
| } | |||
| fn on_runtime_upgrade() -> Weight { | |||
There was a problem hiding this comment.
Is there a list of runtimes we can check off before merging this?
| if StorageVersion::get::<Pallet<T>>() != 0 { | ||
| log::warn!("skipping v1, should be removed"); | ||
| return weight | ||
| } |
There was a problem hiding this comment.
this will work much better with try-runtime-cli. thanks. 👌
|
bot merge |
* move migration stuffs * Apply suggestions from code review Co-authored-by: Bastian Köcher <git@kchr.de> * Fix test * fix * lint * fix lint * rm extra space * fix lint * PR review * fixes * use saturating_accrue in fn * fix test --------- Co-authored-by: Bastian Köcher <git@kchr.de>
Move the pallet
on_runtime_upgradehook logic into migration.rs