-
Notifications
You must be signed in to change notification settings - Fork 404
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(contracts): migrate CodeInfoOf<T>
from Twox64Concat
hashing to Identity
as defined in pallet-contracts
#1200
Conversation
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.
LGTM so far
primitives/src/migrations/mod.rs
Outdated
impl<T: pallet_contracts::Config, const V: u16> OnRuntimeUpgrade for ForceContractsVersion<T, V> { | ||
fn on_runtime_upgrade() -> Weight { | ||
StorageVersion::new(V).put::<pallet_contracts::Pallet<T>>(); | ||
<T as frame_system::Config>::DbWeight::get().reads_writes(1, 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.
Not a big deal, but isn't this a single write only?
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.
you are right, I'l change this
runtime/shiden/src/lib.rs
Outdated
// [contracts fix] Part of shiden-121 (added after v5.33.0 release) | ||
astar_primitives::migrations::ForceContractsVersion<Runtime, 14>, | ||
pallet_contracts::Migration<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.
If I understood correctly, this will downgrade storage version by 1, to ensure we bump it back to the correct one with the new migration, right?
Might be an overkill, but I'd add another dummy struct here that checks in try-runtime hooks that version was bumped back up to the expected one. Just a suggestion.
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, that's correct.
I'll add that.
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.
LGTM!
Minimum allowed line rate 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.
LGTM
Pull Request Summary
The release v5.33.0 included a faulty pallet-contracts migration v12 where
CodeInfoOf<T>
storage item had incorrect alias.This PR includes a new migration to fix this issue. Also in order to run the migration properly we need to bring down pallet's storage version to 14.
TODO
Check list