Add migration logs to pallet v2 #8243
Conversation
kianenigma
added
A0-please_review
| @@ -66,10 +68,16 @@ impl HooksDef { | |||
| return Err(syn::Error::new(item_trait.span(), msg)); | |||
| } | |||
|
|
|||
| let has_runtime_upgrade = item.items.iter().any(|i| match i { | |||
| syn::ImplItem::Method(method) => method.sig.ident == "on_runtime_upgrade", | |||
There was a problem hiding this comment.
Happy to write a test for this, if someone can point out where, or find another way to avoid hardcoding the name here.
| @@ -1329,21 +1329,12 @@ macro_rules! decl_module { | |||
| { | |||
| fn on_runtime_upgrade() -> $return { | |||
| $crate::sp_tracing::enter_span!($crate::sp_tracing::trace_span!("on_runtime_upgrade")); | |||
| let result: $return = (|| { $( $impl )* })(); | |||
There was a problem hiding this comment.
here I just re-ordered stuff: first log that the migration is about to happen, and then do it.
| @@ -25,6 +25,29 @@ pub fn expand_hooks(def: &mut Def) -> proc_macro2::TokenStream { | |||
| let pallet_ident = &def.pallet_struct.pallet; | |||
| let where_clause = &def.hooks.where_clause; | |||
| let frame_system = &def.frame_system; | |||
| let has_runtime_upgrade = def.hooks.has_runtime_upgrade; | |||
There was a problem hiding this comment.
Is this true when a pallet has a OnRuntimeUpgrade hook or only when the storage version is lower than the pallet version?
There was a problem hiding this comment.
only when you have a function with name on_runime_upgrade in your #[pallet_hooks].
Perhaps I can name it has_custom_, because even if you don't have this, the storage version is upgraded.
There was a problem hiding this comment.
So once we add the hook we will get
"
⚠️ running migration for {} and setting new storage version to {:?}",
forever?
Even if the current upgrade did not require a migration because no changes to the data structures were made?
There was a problem hiding this comment.
Yes, if you don't have the migration you should not have fn on_runtime_upgrade in your code, as it can be accidentally re-applied.
You might say I am opinionated here, but this is exactly the scenario of how we almost burned polkadot's balance structure once, by applying a migration twice, due to forgetting to remove it from on_runtime_upgrade. While we have now storage versions to also prevent that, my opinion is due to such incidents and catching them early.
There was a problem hiding this comment.
Is this true when a pallet has a OnRuntimeUpgrade hook or only when the storage version is lower than the pallet version?
it is true when a pallet has a OnRuntimeUpgrade implementation (even if it is {}).
EDIT: It was an answer to the first message
There was a problem hiding this comment.
Yeah I'm unsure how to resolve the conflict between keeping very long migration functions around vs not supporting upgrades from old versions. My current preferred compromise is providing this sort of full migration in the migration mod or crate and then calling whatever the pallet author thinks is best in on_runtime_upgrade. Then downstream users still have access to all the migrations, but you don't have to keep them around in on_runtime_upgrade.
There was a problem hiding this comment.
if version <= 10 { panic!("version too old not supported") }
if version <= 11 { /* migrate from 10 to 11 */ }
if version <= 12 { /* migrate from 11 to 12 */ }
if version <= 13 { /* migrate from 12 to 13 */ }
This is actually what I was planning to do. Apart from being annoying its the only thing that I can come up with that "just works". Maybe we can build some macros or functions around this pattern to make it more bearable. Because for users this is the best case.
There was a problem hiding this comment.
For now I will change the log to something that reflects it, so that we can move on:
⚠️ Pallet-xxx is declaring internal migrations (which may or may not execute) and updates storage version to xxx
This is because what you are doing, is neat, but what I am building is mostly for pallets that make it into polkadot and current workflow there is that before each release we want to delete all the old migrations, and such warnings can help with that.
There was a problem hiding this comment.
I understand. I am fine with that. Contracts should be easily upgradeable for all runtimes. Therefore I need to go down this route.
There was a problem hiding this comment.
updated, then can you approve? :D
|
bot merge |
|
Waiting for commit status. |
ghost
merged commit 
Missing from https://github.com/paritytech/substrate/pull/8123/files