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

RemovePallet migration tool#6977

Closed
liamaharon wants to merge 40 commits intomasterfrom
liam-remove-kusama-gov-v1-storage
Closed

RemovePallet migration tool#6977
liamaharon wants to merge 40 commits intomasterfrom
liam-remove-kusama-gov-v1-storage

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

@liamaharon liamaharon requested a review from kianenigma March 29, 2023 09:05
@paritytech-ci paritytech-ci requested review from a team March 29, 2023 09:06
liamaharon
liamaharon commented Mar 29, 2023
View reviewed changes
@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
kianenigma
kianenigma reviewed Mar 29, 2023
View reviewed changes
@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
kianenigma
kianenigma reviewed Mar 29, 2023
View reviewed changes
@paritytech-ci paritytech-ci requested a review from a team March 29, 2023 09:13
kianenigma
kianenigma reviewed Mar 29, 2023
View reviewed changes
@paritytech-ci paritytech-ci requested a review from a team March 29, 2023 09:14
kianenigma
kianenigma reviewed Mar 29, 2023
View reviewed changes
@paritytech-ci paritytech-ci requested a review from a team March 29, 2023 09:14
kianenigma
kianenigma reviewed Mar 29, 2023
View reviewed changes
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!

@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
Copy link
Contributor Author

liamaharon commented Apr 1, 2023

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
kianenigma
kianenigma reviewed Apr 5, 2023
View reviewed changes
@paritytech-ci paritytech-ci requested a review from a team April 5, 2023 09:14
kianenigma
kianenigma reviewed Apr 5, 2023
View reviewed changes
kianenigma
kianenigma previously approved these changes Apr 5, 2023
View reviewed changes
@kianenigma
Copy link
Contributor

kianenigma commented Apr 5, 2023

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

paritytech-cicd-pr commented Apr 5, 2023

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
/// (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

@ggwpez ggwpez Apr 5, 2023

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

@liamaharon liamaharon Apr 5, 2023

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

@bkchr bkchr Apr 5, 2023

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

liamaharon commented Apr 5, 2023

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

3 more reviewers

@bkchr bkchr bkchr approved these changes

@ggwpez ggwpez ggwpez requested changes

@kianenigma kianenigma kianenigma left review comments

Reviewers whose approvals may not affect merge requirements

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