-
Notifications
You must be signed in to change notification settings - Fork 2.6k
pallet-balances: Fix inactive funds migration #12840
Conversation
Fixes the inactive funds migration. It was missing to set the `storage_version` attribute for the `Pallet` struct. Besides that it also removes the old `StorageVersion` representation and adds support for instances of pallet-balances.
let onchain_version = Pallet::<T, I>::on_chain_storage_version(); | ||
|
||
if onchain_version == 0 { |
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.
Does the migration or the new pallet get applied first? Because in the pallet you set the storage version to 1
, so if that applies first then IIUC this migration won't actually run.
/// The current storage version.
const STORAGE_VERSION: frame_support::traits::StorageVersion =
frame_support::traits::StorageVersion::new(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.
On chain version will return 0, at this point this storage item even does not exist.
here the trait
fn on_chain_storage_version() -> StorageVersion; |
here the impl
substrate/frame/support/src/dispatch.rs
Line 2518 in a1d014d
impl<$trait_instance: $trait_name $(<I>, $instance: $instantiable)?> $crate::traits::GetStorageVersion |
and below here in the migration you can see where we updating on chain storage version
https://github.com/paritytech/substrate/pull/12840/files#diff-d3a8ee05060d4cc1bb98df686418ec5aeba54698bd793a46c9095dde576eaa77R41
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.
The storage version is only set on genesis to the "current version" (aka the version you set in the code). After genesis, you need to bump the version "manually".
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.
On chain version will return 0, at this point this storage item even does not exist.
Maybe I don't understand, because it looks like to me that it is being initialized to 1, so it would exist:
https://github.com/paritytech/substrate/pull/12840/files#diff-7f35e750129ddf17862e61be2ce506a120ac7d4de151633d2c026525130bba7dR251
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.
@joepetrowski that constant, is not persisted anywhere by frame, its only returned by on_chain_storage_version
here
substrate/frame/support/src/dispatch.rs
Line 2522 in a1d014d
$( $storage_version )* |
let onchain_version = Pallet::<T, I>::on_chain_storage_version(); | ||
|
||
if onchain_version == 0 { |
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.
On chain version will return 0, at this point this storage item even does not exist.
here the trait
fn on_chain_storage_version() -> StorageVersion; |
here the impl
substrate/frame/support/src/dispatch.rs
Line 2518 in a1d014d
impl<$trait_instance: $trait_name $(<I>, $instance: $instantiable)?> $crate::traits::GetStorageVersion |
and below here in the migration you can see where we updating on chain storage version
https://github.com/paritytech/substrate/pull/12840/files#diff-d3a8ee05060d4cc1bb98df686418ec5aeba54698bd793a46c9095dde576eaa77R41
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 still don't entirely understand the storage version macro, but the other logic in the PR looks correct to me.
* pallet-balances: Fix inactive funds migration Fixes the inactive funds migration. It was missing to set the `storage_version` attribute for the `Pallet` struct. Besides that it also removes the old `StorageVersion` representation and adds support for instances of pallet-balances. * Fix test
* pallet-balances: Fix inactive funds migration Fixes the inactive funds migration. It was missing to set the `storage_version` attribute for the `Pallet` struct. Besides that it also removes the old `StorageVersion` representation and adds support for instances of pallet-balances. * Fix test
Fixes the inactive funds migration. It was missing to set the
storage_version
attribute for thePallet
struct. Besides that it also removes the oldStorageVersion
representation and adds support for instances of pallet-balances.PR that added the migration: #12813