Skip to content
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: on-chain storage version for pallets where it is missing #458

Merged
merged 6 commits into from
Jan 30, 2023

Conversation

ntn-x2
Copy link
Member

@ntn-x2 ntn-x2 commented Jan 26, 2023

A while ago, we have migrated to using the StorageVersion way of declaring the storage version for our pallets. Nevertheless, we were not aware that such a change would only be written to storage either explicitly in a storage migration for live chains, or at genesis for new chains.

This means that we have few places where the declared storage version, which is a const inside each pallet, does not reflect what is written on chain in the relative entry.

This means that, for the future, every time we add a new pallet we must also include the storage migration to write the value of the storage version on chain.

Peregrine

The attestation and publicCredentials pallet expose a StorageVersion of 1, but the on-chain value is 0.

Spiritnet

It has the same issue as Peregrine, with the addition of the web3Names pallet as well.

This PR then exposes a pre-runtime hook that logs with a warning all pallets that suffer from this inconsistency, and a post-runtime check that verifies that everything has been fixed.

Note

The migration that updates the storage version has to be run last, so that pre- and post- runtime hooks don't trigger any unexpected behaviour.

How to test

Compile the kilt-parachain binary with cargo build -p kilt-parachain --features try-runtime.

For Peregrine, run:

./target/debug/kilt-parachain try-runtime \
--runtime target/debug/wbuild/peregrine-runtime/peregrine_runtime.compact.compressed.wasm \
-lruntime=info \
on-runtime-upgrade --checks \
live --uri wss://peregrine.kilt.io:443/parachain-public-ws

For Spiritnet, run:

./target/debug/kilt-parachain try-runtime \
--runtime target/debug/wbuild/spiritnet-runtime/spiritnet_runtime.compact.compressed.wasm \
-lruntime=info \
on-runtime-upgrade --checks \
live --uri wss://spiritnet.kilt.io:443

@ntn-x2 ntn-x2 self-assigned this Jan 26, 2023
}
}

#[cfg(feature = "try-runtime")]
Copy link
Member Author

Choose a reason for hiding this comment

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

Did not find a better way to make the dependencies that are only used with try-runtime optional.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you not make the pre_ and post_ implementation optional?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes but you can't import crates only for that. The point of this solution is that the crates for the pallets are imported only when the feature is enabled.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hence the dependency for those can be made optional.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah you are referring to the where clause

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes!

@ntn-x2 ntn-x2 marked this pull request as ready for review January 26, 2023 12:50
@ntn-x2 ntn-x2 requested review from weichweich and trusch January 26, 2023 12:50
@ntn-x2 ntn-x2 changed the title fix: fix on-chain storage version for pallets where it is missing fix: on-chain storage version for pallets where it is missing Jan 26, 2023
@ntn-x2 ntn-x2 merged commit eff278b into develop Jan 30, 2023
@ntn-x2 ntn-x2 deleted the aa/fix-ctype-storage-version branch January 30, 2023 14:02
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.

2 participants