Move PalletVersion away from the crate version

[bkchr]

Before this pr, PalletVersion was referring to the crate version that
hosted the pallet. This pr introduces a custom package.metadata.frame
section in the Cargo.toml that can contain a pallet-version key
value pair. While the value is expected to be a valid u16. If this
key/value pair isn't given, the version is set to 1.

It also changes the PalletVersion declaration. We now only have one
u16 that represents the version. Not a major/minor/patch version. As
the old PalletVersion was starting with the u16 major, decoding the
old values will work.

polkadot companion: paritytech/polkadot#3526

[shawntabrizi]

As the old PalletVersion was starting with the u16 major, decoding the old values will work.

nice

[athei]

Why is this useful and to which version (crate or pallet) do I refer to in my CHANGELOG.md from now on?

[gui1117]

code looks good to me,

I don't have strong opinion between storage-version and pallet-version.

• storage-version makes it more close to its current usage: we bump it when storage is breaking and do some conditional with it in storage migrations
• pallet-version makes it more close to its original goal (as I understand it): having a stored number which can identifier clearly the pallet. So we can easily checkout the pallet code. But if we don't bump this number when the pallet behaviour changes, then the name is more misleading than helpful.

Also I agree 0 could make more sense when no version is defined https://github.com/paritytech/substrate/pull/9165/files#r655954312

[apopiak]

Just FYI that this XCM PR currently assumes (major, minor, patch) for pallet versions:
polkadot-fellows/xcm-format#22

[bkchr]

Just FYI that this XCM PR currently assumes (major, minor, patch) for pallet versions:
paritytech/xcm-format#22

Hmm good point. Yeah, I don't know. I'm still seeing the "need" for this, but currently no one bumps any versions anyway... So, yeah... It is hard.

[bkchr]

Or we just keep the PalletVersion with major, minor and patch and make these values set able through the Cargo.toml as I have done it here already in the pr?

CC @shawntabrizi

[bkchr]

@kianenigma @thiolliere @athei what do you think?

[kianenigma]

TBH I am only interested in this for storage migrations (see my linked issue), so that we can get rid of all the transient storage items. For that, a single version is enough. If the triplet is also useful in certain cases, we can

version = "x.y.z" // This is written as 'PalletVersion', basically the old system

[package.metadata.frame]  
storage-version = "v" // This is written as `StorageVersion`

version is only about crates.io and semver. storage-version is only about the version of storage, and with each change, it should be bumped. In an ideal world, any storage-version bump must have a major version update as well. But the opposite is not true.

[shawntabrizi]

I think this should be named storage-version and contain just a single value, similar to spec_version and stuff

storage is either backwards compatible or not. afaik, there is not a need for minor and patch version numbers

[athei]

I agree with shawn and kian. We only need one number because this version seems to be about storage only (for everything else we have the crate version). We should name it storage-version and it should be a single number.

[apopiak]

Agree as well. Just linked the XCM PR to make sure that we adjust that once we change this.

[bkchr]

Okay I have rewritten the entire thing.

• PalletVersion was removed and replaced by StorageVersion
• We don't have any magical macro anymore that pulls data out of the Cargo.toml. I decided for making the storage version explicit as a const in the top of the crate. However, we still need to tell the pallet macros about it.
• The pallet author is also now required to set the correct storage version after a migration.
• I added a migration for migrating from PalletVersion to StorageVersion.

Outdated

[gui1117]

needs to fix the on genesis for decl_storage macro: #9165 (comment)

But otherwise it is good to me

[gui1117]

Note for other about migration:

The storage version is now stored under a different prefix than the old pallet version. To upgrade you can use migrate_from_pallet_version_to_storage_version like in https://github.com/paritytech/polkadot/pull/3526/files
Basically adding the struct:

/// Migrate from `PalletVersion` to the new `StorageVersion`
pub struct MigratePalletVersionToStorageVersion;

impl OnRuntimeUpgrade for MigratePalletVersionToStorageVersion {
	fn on_runtime_upgrade() -> frame_support::weights::Weight {
		frame_support::migrations::migrate_from_pallet_version_to_storage_version::<AllPalletsWithSystem>(
				&RocksDbWeight::get()
		)
	}
}

And add this struct to the generic CustomOnRuntimeUpgrade of Executive

Warning: The custom runtime upgrade happens before the pallet's runtime upgrede (defined inside hooks). Thus any hook who rely on the pallet version won't be triggered and migration must be done manually. in substrate repo, pallet-contracts is the only pallet which look into the stored pallet version.

[bkchr]

@thiolliere ty

[bkchr]

bot merge

[ghost]

Waiting for commit status.