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

RemovePallet migration tool #6977

Closed
wants to merge 40 commits into from
Closed

RemovePallet migration tool #6977

wants to merge 40 commits into from

Conversation

liamaharon
Copy link
Contributor

@liamaharon liamaharon commented Mar 29, 2023 •
edited
Loading

  • Creates RemovePallet which can be used to remove all storage items for a given pallet on the next runtime upgrade.
  • Uses RemovePallet to delete storage for Kusama pallets that were removed from the runtime in Introduce OpenGov into Polkadot #6701

Related: paritytech/polkadot-sdk#485

@paritytech-ci paritytech-ci requested review from a team March 29, 2023 09:06
liamaharon
liamaharon commented Mar 29, 2023
View reviewed changes
runtime/kusama/src/lib.rs Outdated Show resolved Hide resolved
@liamaharon liamaharon 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. labels Mar 29, 2023
@ggwpez ggwpez mentioned this pull request Apr 24, 2023
Closed
12 tasks
@liamaharon liamaharon added the D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit. label Mar 29, 2023
@paritytech-ci paritytech-ci requested a review from a team March 29, 2023 09:12
@paritytech-ci paritytech-ci requested a review from a team March 29, 2023 09:13
@paritytech-ci paritytech-ci requested a review from a team March 29, 2023 09:14
@paritytech-ci paritytech-ci requested a review from a team March 29, 2023 09:14
ggwpez
ggwpez approved these changes Mar 31, 2023
View reviewed changes
Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

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

Thanks!

runtime/common/src/remove_pallet.rs Outdated Show resolved Hide resolved
@liamaharon @ggwpez
Update runtime/common/src/remove_pallet.rs
7766cb1 
Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@liamaharon liamaharon changed the title Remove Kusama Gov V1 storage RemovePallet tool, Remove Kusama Gov V1 storage Mar 31, 2023
@xlc
Copy link
Contributor

xlc commented Mar 31, 2023

how about locked/reserved funds?

@liamaharon
Merge branch 'liam-remove-kusama-gov-v1-storage' of github.com:parity…
39ce205 
…tech/polkadot into liam-remove-kusama-gov-v1-storage
@liamaharon
Copy link
Contributor Author

how about locked/reserved funds?

This tool is specifically for removing storage. Something for migrating/recovering locked/reserved funds I think should be part of a seperate work effort. Sounds like deserving of a new issue to me.

@xlc xlc mentioned this pull request Apr 1, 2023
Closed
@paritytech-ci paritytech-ci requested a review from a team April 5, 2023 09:14
kianenigma
kianenigma previously approved these changes Apr 5, 2023
View reviewed changes
@kianenigma
Copy link
Contributor

how about locked/reserved funds?

I thought the PR that kickstarted Gov V2 already did that.

If not, it will be much harder to do once we remove the storage. We should also do all the unreserve/unlocks in this PR.

@kianenigma kianenigma dismissed their stale review April 5, 2023 09:17

unlock/undreserve

@paritytech-ci paritytech-ci requested a review from a team April 5, 2023 09:17
@xlc
Copy link
Contributor

xlc commented Apr 5, 2023

We have not yet disabled gov1 and the migration must happen after it is disabled before it is removed

ggwpez
ggwpez suggested changes Apr 5, 2023
View reviewed changes
Copy link
Member

@ggwpez ggwpez left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Yea going to block so we dont accidentally merge it until gov1 is removed.

Or rather: Just dont use it on Kusama. We can already merge the tooling.

@liamaharon liamaharon changed the title RemovePallet tool, Remove Kusama Gov V1 storage RemovePallet migration tool Apr 5, 2023
liamaharon and others added 5 commits April 5, 2023 21:09
@liamaharon @kianenigma
Update runtime/common/src/remove_pallet.rs
f935984 
Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
@liamaharon @kianenigma
Update runtime/common/src/remove_pallet.rs
e0632be 
Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
@liamaharon
Merge branch 'master' of github.com:paritytech/polkadot into liam-rem…
fb39cbe 
…ove-kusama-gov-v1-storage
@liamaharon
remove kusama gov v1 storage deletion
299ce74
@liamaharon
fix universal alias function signature
21baa56
@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable
Logs: https://gitlab.parity.io/parity/mirrors/polkadot/-/jobs/2645642

ggwpez
ggwpez reviewed Apr 5, 2023
View reviewed changes
runtime/common/src/remove_pallet.rs
@@ -77,8 +84,12 @@ use sp_std::{marker::PhantomData, vec::Vec};
/// a multi-block scheduler currently under development which will allow for removal of storage
/// items (and performing other heavy migrations) over multiple blocks.
/// (https://github.com/paritytech/substrate/issues/13690)
pub struct RemovePallet<P: Get<&'static str>>(PhantomData<P>);
impl<P: Get<&'static str>, DbWeight: Get<DbWeight>> frame_support::traits::OnRuntimeUpgrade for RemovePallet<P> {
pub struct RemovePallet<P: Get<&'static str>, DbWeight: Get<RuntimeDbWeight>>(
Copy link
Member

Choose a reason for hiding this comment

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

We could also move it to Substrate… 🤷‍♂️
Not sure if it is worth it right now though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ggwpez good point, I think we may as well do it now if it would need to happen later anyway.

Is there an existing package in substrate you think this would fit well into?

Copy link
Member

Choose a reason for hiding this comment

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

Probably frame-support.

@liamaharon
Copy link
Contributor Author

Closing this PR, will reopen another in substrate adding RemovePallet there.

@liamaharon liamaharon closed this Apr 5, 2023
@liamaharon liamaharon mentioned this pull request Apr 6, 2023
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bkchr bkchr bkchr approved these changes

@ggwpez ggwpez ggwpez requested changes

@kianenigma kianenigma kianenigma left review comments

Assignees
No one assigned
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. E0-runtime_migration PR introduces code that might require downstream chains to run a runtime upgrade.
Projects
Runtime / FRAME
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@liamaharon @kianenigma @xlc @paritytech-cicd-pr @bkchr @ggwpez