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

feat: add launch pallet removal migration #359

Merged
merged 5 commits into from
May 31, 2022
Merged

Conversation

wischli
Copy link
Contributor

@wischli wischli commented Apr 27, 2022

fixes KILTProtocol/ticket#1951

  • Removes the launch pallet
  • Adds migration to kill all storage related to the KiltLaunch storage prefix 0x37be294ab4b5aa76f1df3f80e7c180ef

Checklist:

  • I have verified that the code works
    • No panics! (checked arithmetic ops, no indexing array[3] use get(3), ...)
  • I have verified that the code is easy to understand
    • If not, I have left a well-balanced amount of inline comments
  • I have left the code in a better state
  • I have documented the changes (where applicable)

@wischli wischli changed the base branch from develop to wf-1946-polkadot-0.9.19 April 27, 2022 11:17
Base automatically changed from wf-1946-polkadot-0.9.19 to develop April 27, 2022 13:46
@wischli wischli marked this pull request as ready for review April 27, 2022 15:30
pub struct RemoveKiltLaunch<R>(PhantomData<R>);
impl<R: frame_system::Config> frame_support::traits::OnRuntimeUpgrade for RemoveKiltLaunch<R> {
fn on_runtime_upgrade() -> frame_support::weights::Weight {
let items = match frame_support::storage::unhashed::kill_prefix(&hex!("37be294ab4b5aa76f1df3f80e7c180ef"), None)
Copy link
Member

Choose a reason for hiding this comment

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

Can this be generated from the pallet name string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to do it this way but since we are removing the pallet from the runtime, we can't derive it's pallet name from the storage. You can verify the hex yourself via the Polkadot Apps
Screenshot 2022-04-28 at 10 22 03

Copy link
Member

Choose a reason for hiding this comment

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

Was just playing around, and this is what apparently works: let hash = sp_io::hashing::twox_128(b"KiltLaunch");.

No need to change it, but I think it's better for readability and for potentially fix any issues we identify in the future.

"🚀 Pre check: Launch pallet storage exists {}?",
frame_support::storage::migration::have_storage_value(
&hex!("37be294ab4b5aa76f1df3f80e7c180ef"),
// b"KiltLaunch"
Copy link
Member

Choose a reason for hiding this comment

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

Why not deriving the HEX from the name directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The hex macro requires an input of type string. I did not manage to quickly get it running with any permutation of the pallet name, e.g. hex!(b"KiltLaunch".into()). Just using b"KiltLaunch" without hex! wrapping leads to a different storage prefix.

@wischli wischli added the next release This PR is required for the next release. label May 30, 2022
Copy link
Member

@ntn-x2 ntn-x2 left a comment

Choose a reason for hiding this comment

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

LGTM! I only have one small question about the logic of the pre_upgrade() migration check.

let prefix: [u8; 16] = sp_io::hashing::twox_128(b"KiltLaunch");

assert!(
sp_io::storage::next_key(&prefix).map_or(true, |next_key| next_key.starts_with(&prefix)),
Copy link
Member

Choose a reason for hiding this comment

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

Probably not relevant, but shouldn't the assertion fail in case there is no key under the given prefix, as we do indeed expect a key?

Suggested change
sp_io::storage::next_key(&prefix).map_or(true, |next_key| next_key.starts_with(&prefix)),
sp_io::storage::next_key(&prefix).map_or(false, |next_key| next_key.starts_with(&prefix)),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! You are correct. Fixed in a4c3a5e

@wischli wischli enabled auto-merge (squash) May 31, 2022 11:55
@wischli wischli merged commit 6ce128e into develop May 31, 2022
@wischli wischli deleted the wf-1951-rm-launch-pallet branch May 31, 2022 12:07
ntn-x2 pushed a commit that referenced this pull request Jun 23, 2022
* feat: add launch pallet removal migration

* chore: rm launch pallet code

* fix: migration

* fix: clippy + suggestion

(cherry picked from commit 6ce128e)
ntn-x2 added a commit that referenced this pull request Jun 24, 2022
* Adds two more relaychain bootnodes for staging environment  (#334)

* chore: reset peregrine stg (#335)

* ci: use custom ci image (#336)

* Optimizes docker layer (#337)

* fix: add did lookup pallet to DID authorization logic + reverse lookup index (#343)

* chore: update toolchain version to nightly 1.59 (#339)

* feat: proxy type for disableling deposit claiming (#341)

* fix: rococo protocol id (#369)

* feat: generic access control (#316)

* Updates toolchain version (#345)

* refactor: enforce no runtime in pallet (#349)

* fix: features (#353)

* feat: add tips pallet (#352)

* feat: upgrade to Polkadot v0.9.19 (#357)

* chore: upgrade and clean up (#360)

* Adds the new rococo chainspec (#363)

* feat: add launch pallet removal migration (#359)

* refactor: update rilt para id from 2015 to 2108 (#364)

* fix: rilt para id (#365)

* feat: upgrade to Polkadot v0.9.23 (#366)

* use ci-linx:production base image (#368)

* feat: upgrade to Polkadot v0.9.24 (#370)

* fix: fix CI builders compilation errors and pin to a specific hash (#372)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
next release This PR is required for the next release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants