Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

pallet-balances: Fix inactive funds migration #12840

Merged
merged 2 commits into from
Dec 5, 2022

Conversation

bkchr
Copy link
Member

@bkchr bkchr commented Dec 4, 2022

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.

PR that added the migration: #12813

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.
@bkchr bkchr added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Dec 4, 2022
Comment on lines +25 to +27
let onchain_version = Pallet::<T, I>::on_chain_storage_version();

if onchain_version == 0 {
Copy link
Contributor

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);

Copy link
Contributor

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

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

Copy link
Member Author

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".

Copy link
Contributor

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

Copy link
Contributor

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

$( $storage_version )*

Comment on lines +25 to +27
let onchain_version = Pallet::<T, I>::on_chain_storage_version();

if onchain_version == 0 {
Copy link
Contributor

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

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

Copy link
Contributor

@joepetrowski joepetrowski left a 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.

@gavofyork gavofyork merged commit 2704ab3 into master Dec 5, 2022
@gavofyork gavofyork deleted the bkchr-fix-balances-migration branch December 5, 2022 13:57
ltfschoen pushed a commit to ltfschoen/substrate that referenced this pull request Feb 22, 2023
* 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
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
* 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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants