-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Remove xcm on_runtime_upgrade pallet hook #7235
Conversation
Co-authored-by: Bastian Köcher <git@kchr.de>
@@ -685,12 +685,6 @@ pub mod pallet { | |||
} | |||
weight_used | |||
} | |||
fn on_runtime_upgrade() -> Weight { |
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-runtime
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.
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
@@ -685,12 +685,6 @@ pub mod pallet { | |||
} | |||
weight_used | |||
} | |||
fn on_runtime_upgrade() -> Weight { |
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?
if StorageVersion::get::<Pallet<T>>() != 0 { | ||
log::warn!("skipping v1, should be removed"); | ||
return weight | ||
} |
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.
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_upgrade
hook logic into migration.rs