Skip to content

Conversation

@shunsukew
Copy link
Contributor

@shunsukew shunsukew commented Oct 26, 2022

Pull Request Summary
In Shibuya and Shiden networks, pallet-contracts storage migration was missing. paritytech/substrate#12083
As a result of the commit history & current storage value investigation, we found out only Migration v8 needs to get executed with polkadot-v0.9.30 uplifts. https://github.com/paritytech/substrate/blob/a3ed0119c45cdd0d571ad34e5b3ee7518c8cef8d/frame/contracts/src/migration.rs#L58

This PR make sure pallet contracts StorageVersion updated to 7 (now it is 0) before pallet contracts Migration, and only pallet contracts Migration v8 get executed with runtime upgrade.

Check list

  • added or updated unit tests
  • updated Astar official documentation
  • added OnRuntimeUpgrade hook for precompile revert code registration
  • updated spec version
  • updated semver

@shunsukew shunsukew marked this pull request as ready for review October 30, 2022 08:30
@shunsukew shunsukew requested review from 0x7CFE, Dinonard and akru and removed request for Dinonard October 30, 2022 08:30
@shunsukew shunsukew changed the title Enable pallet contracts runtime upgrade migration Enable pallet contracts storage migration Oct 30, 2022
"pallet-xc-asset-config/runtime-benchmarks",
]
try-runtime = [
"log",
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't log 3rd party product? It shouldn't have any deps with try-runtime.

Copy link
Contributor Author

@shunsukew shunsukew Nov 1, 2022

Choose a reason for hiding this comment

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

I just would like to show storage version in pre and post runtime upgrade.

log::info!("Pre upgrade StorageVersion: {:?}", version);

Is there any reason? even though it won't be included in node binary and wasm.

Copy link
Contributor

Choose a reason for hiding this comment

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

It will, this is just related to features. You'd have to make it optinal or a dev dependency for it to be excluded.

Copy link
Contributor Author

@shunsukew shunsukew Nov 1, 2022

Choose a reason for hiding this comment

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

log dependency is already optional

log = { version = "0.4.17", optional = true }

Copy link
Contributor

Choose a reason for hiding this comment

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

Must have missed it. 👍

pub use sp_runtime::BuildStorage;

mod chain_extensions;
mod migration;
Copy link
Contributor

Choose a reason for hiding this comment

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

Imo, no need for this. It's few lines of code. Just putting this where we usually place custom OnRuntimeUpgrade hooks is fine. We'll have to delete this anyhow later on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's preference matter. I thought deleting a file can remove unnecessary stuffs later on, and it's clear to understand. Anywau, I'll stop using separated file.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's fine if it's your personal preference, no need to change.

@shunsukew shunsukew requested a review from Dinonard November 2, 2022 04:37
#[cfg(feature = "try-runtime")]
fn pre_upgrade() -> Result<Vec<u8>, &'static str> {
let version = StorageVersion::get::<pallet_contracts::Pallet<T>>();
log::info!("Pre upgrade StorageVersion: {:?}", version);
Copy link
Contributor

Choose a reason for hiding this comment

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

In general, I'd advise using asserts in pre/post upgrade hooks. That way you'll get a clear fail when running it.

@shunsukew shunsukew merged commit 830df53 into master Nov 2, 2022
@shunsukew shunsukew deleted the enable-pallet-contracts-migration branch November 2, 2022 06:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants