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

Improve storage_alias and make UnlockAndUnreserveAllFunds independent of the pallet #14773

Merged
merged 8 commits into from
Aug 16, 2023
Merged

Improve storage_alias and make UnlockAndUnreserveAllFunds independent of the pallet #14773

merged 8 commits into from
Aug 16, 2023
+1,109 −922

Conversation

bkchr
Copy link
Member

@bkchr bkchr commented Aug 15, 2023

This pull request is doing two things:

  1. It improve storage_alias by making it more generic over the prefix to use. Currently storage_alias tries to guess based on the generics if the user wants to use the pallet name or a fixed identifier as prefix. This pull requests changes the macro to take an additional attribute that tells it what prefix type the user wants. Besides that a third prefix type is added, a type that implements Get<&'static str> can also be provided as prefix. See the docs in the code for more information on how this works.

  2. pallet_democracy::UnlockAndUnreserveAllFunds is made independent of the presence of the pallet-democracy pallet. This means that the migration can be used without the pallet still being used in the runtime. The user only needs to implement the UnlockConfig for some type that is then passed to UnlockAndUnreserveAllFunds to forward certain information that are required by the migration.

Besides this the pull request also puts some tests in frame-support into their own file to clean up the lib.rs.

@bkchr bkchr added A0-please_review Pull request needs code review. 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 B1-note_worthy Changes should be noted in the release notes labels Aug 15, 2023
@bkchr bkchr requested review from a team August 15, 2023 15:02
@bkchr bkchr added the T1-runtime This PR/Issue is related to the topic “runtime”. label Aug 15, 2023
ggwpez
ggwpez approved these changes Aug 15, 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.

Just some nits

frame/support/procedural/src/storage_alias.rs Outdated
///
/// If generics are passed, this is assumed to be a pallet and the pallet name should be used.
/// Otherwise use the verbatim passed name as prefix.
Backwards,
Copy link
Member

Choose a reason for hiding this comment

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

Legacy or Compatibility? Otherwise it sounds like it interprets something backwards.

frame/support/src/tests/storage_alias.rs Show resolved Hide resolved
frame/support/src/tests/storage_alias.rs

#[docify::export]
#[test]
fn storage_alias_guess() {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe one test that the backwards compatibility works?

sam0x17
sam0x17 approved these changes Aug 16, 2023
View reviewed changes
Copy link
Contributor

@sam0x17 sam0x17 left a comment

Choose a reason for hiding this comment

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

found a bunch of minor grammar/wording nits. Might need to re-wrap a few of the doc paragraphs if you accept them

frame/democracy/src/migrations/unlock_and_unreserve_all_funds.rs Outdated Show resolved Hide resolved
frame/democracy/src/migrations/unlock_and_unreserve_all_funds.rs Outdated Show resolved Hide resolved
frame/democracy/src/migrations/unlock_and_unreserve_all_funds.rs Outdated Show resolved Hide resolved
frame/democracy/src/migrations/unlock_and_unreserve_all_funds.rs Outdated Show resolved Hide resolved
frame/support/procedural/src/storage_alias.rs Outdated Show resolved Hide resolved
frame/support/src/lib.rs Outdated Show resolved Hide resolved
frame/support/src/lib.rs Outdated Show resolved Hide resolved
frame/support/src/lib.rs Outdated Show resolved Hide resolved
frame/support/src/lib.rs Outdated Show resolved Hide resolved
frame/support/src/lib.rs Outdated Show resolved Hide resolved
bkchr
bkchr commented Aug 16, 2023
View reviewed changes
frame/support/src/lib.rs Outdated Show resolved Hide resolved
@bkchr
Copy link
Member Author

bkchr commented Aug 16, 2023

bot fmt

@command-bot
Copy link

command-bot bot commented Aug 16, 2023 •
edited
Loading

@bkchr https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3395811 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/substrate/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 32-876d6c0b-ef05-4d17-aa17-7892c304e213 to cancel this command or bot cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented Aug 16, 2023

@bkchr Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3395811 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3395811/artifacts/download.

@bkchr
Copy link
Member Author

bkchr commented Aug 16, 2023

bot merge

@paritytech-processbot
Copy link

Waiting for commit status.

liamaharon
liamaharon approved these changes Aug 16, 2023
View reviewed changes
Copy link
Contributor

@liamaharon liamaharon left a comment

Choose a reason for hiding this comment

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

Perfect, thank you!

I have implemented and tested this on my Gov V1 unlock PR (commit with changes:
paritytech/polkadot@5e440c1) and it works great.

Additional benefit: we can wipe the pallet storage in the same migration we unlock the funds.

@liamaharon liamaharon mentioned this pull request Aug 16, 2023
Merged
11 tasks
@liamaharon liamaharon changed the title Improve storage_alias and make UnlockAndUnreserveAllFunds indepenend of the pallet Improve storage_alias and make UnlockAndUnreserveAllFunds independent of the pallet Aug 16, 2023
@liamaharon liamaharon mentioned this pull request Aug 16, 2023
Closed
@paritytech-processbot paritytech-processbot bot merged commit 28a3084 into master Aug 16, 2023
6 checks passed
@paritytech-processbot paritytech-processbot bot deleted the bkchr-more-generic-storage-alias branch August 16, 2023 08:15
@liamaharon liamaharon mentioned this pull request Aug 16, 2023
Merged
Ank4n pushed a commit that referenced this pull request Aug 20, 2023
@bkchr @sam0x17 @Ank4n
Improve storage_alias and make UnlockAndUnreserveAllFunds indepen…
c40cf54 
…dent of the pallet (#14773)

* Make `storage_alias` more generic over the `prefix`

* Make `UnlockAndUnreserveAllFunds` indepenend from the pallet

* FMT

* Fix error reporting

* Rename prefix type

* Add test

* Apply suggestions from code review

Co-authored-by: Sam Johnson <[email protected]>

* ".git/.scripts/commands/fmt/fmt.sh"

---------

Co-authored-by: Sam Johnson <[email protected]>
Co-authored-by: command-bot <>
@crystalin crystalin mentioned this pull request Oct 20, 2023
Closed
@JesseAbram JesseAbram mentioned this pull request Oct 26, 2023
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sam0x17 sam0x17 sam0x17 approved these changes

@ggwpez ggwpez ggwpez approved these changes

@wentelteefje wentelteefje wentelteefje approved these changes

@liamaharon liamaharon liamaharon approved these changes

@gupnik gupnik gupnik approved these changes

Assignees
No one assigned
Labels
A0-please_review Pull request needs code review. B1-note_worthy Changes should be noted in the 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 T1-runtime This PR/Issue is related to the topic “runtime”.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@bkchr @sam0x17 @ggwpez @wentelteefje @liamaharon @gupnik