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

Migrate pallet-balances to pallet attribute macro #7936

Merged
32 commits merged into from
Feb 10, 2021
Merged

Conversation

ascjones
Copy link
Contributor

@ascjones ascjones commented Jan 20, 2021

Part of #7882.

Converts the Balances pallet to the new pallet attribute macro introduced in #6877.

Following the upgrade guidelines here: https://crates.parity.io/frame_support/attr.pallet.html#upgrade-guidelines.

⚠️ Breaking Change ⚠️

From https://crates.parity.io/frame_support/attr.pallet.html#checking-upgrade-guidelines

storages now use PalletInfo for module_prefix instead of the one given to decl_storage: Thus any use of this pallet in construct_runtime! should be careful to update name in order not to break storage or to upgrade storage (moreover for instantiable pallet). If pallet is published, make sure to warn about this breaking change.

So users of the Balances pallet must be careful about the name they used in construct_runtime!. Hence the runtime-migration label, which might not be needed depending on the configuration of the Balances pallet.

polkadot companion: paritytech/polkadot#2331

@github-actions github-actions bot added the A3-in_progress Pull request is in progress. No review needed at this stage. label Jan 20, 2021
@ascjones ascjones changed the title Migrate frame-system to pallet attribute macro Migrate pallet-balances to pallet attribute macro Jan 20, 2021
@ascjones ascjones added B3-apinoteworthy C1-low PR touches the given topic and has a low impact on builders. labels Jan 20, 2021
@ascjones ascjones marked this pull request as ready for review January 20, 2021 15:30
@github-actions github-actions bot added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Jan 20, 2021
@ascjones ascjones requested a review from gui1117 January 20, 2021 15:31
@ascjones ascjones requested a review from athei as a code owner January 20, 2021 16:30
frame/system/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Member

@athei athei left a comment

Choose a reason for hiding this comment

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

LGTM. We need a polkadot companion, though.

frame/balances/src/lib.rs Show resolved Hide resolved
frame/balances/src/lib.rs Outdated Show resolved Hide resolved
frame/balances/src/lib.rs Outdated Show resolved Hide resolved
frame/balances/src/lib.rs Outdated Show resolved Hide resolved
@ascjones
Copy link
Contributor Author

ascjones commented Jan 27, 2021

Companion PR needs approving for CI to pass: paritytech/polkadot#2331 Done ✔️

@ascjones
Copy link
Contributor Author

Just waiting for a second approval, @thiolliere @dvdplm

Copy link
Contributor

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

LGTM but I'd feel more comfortable if @thiolliere gave thumbs up as well, this being a pretty important pallet...

@gui1117
Copy link
Contributor

gui1117 commented Jan 29, 2021

i haven't review yet, but would have prefered to fix #7949 first, as with the incoming next release of substrate, user pallets can start failing due to their pallet info being (), and I can expect some of them to get very confused/lost.
I know none of substrate pallets would but still.

@ascjones
Copy link
Contributor Author

ascjones commented Jan 29, 2021

Ok we will wait until #7949 is fully complete before merging (is there an appropriate label for that?). I'll migrate some more tests this afternoon.

@gui1117
Copy link
Contributor

gui1117 commented Jan 29, 2021

on-ice can be appopriate

type AccountStore: StoredMap<Self::AccountId, AccountData<Self::Balance>>;
#[pallet::pallet]
#[pallet::generate_store(pub(super) trait Store)]
pub struct Pallet<T, I=()>(PhantomData<(T, I)>);
Copy link
Member

Choose a reason for hiding this comment

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

@thiolliere this feels a bit boilerplatey. can we have the macro automatically do the PhantomData dance if we just use a _ here?

Suggested change
pub struct Pallet<T, I=()>(PhantomData<(T, I)>);
pub struct Pallet<T, I=()>(_);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On the other hand it would go against the idea of the new syntax being valid Rust.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, here is the PR #8091

Copy link
Contributor

Choose a reason for hiding this comment

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

On the other hand it would go against the idea of the new syntax being valid Rust.

it is true, but at least this syntax is accepted by syn crate thus we have a relatively good syntax parser.
Hopefully, the expansion is documented enough not to confuse too much.
And it is same logic as in #[pallet::storage]: _ is a type to be replaced by macro.

@ascjones
Copy link
Contributor Author

ascjones commented Feb 9, 2021

#7949 now done, so ready for a look @thiolliere

@ascjones ascjones removed the A1-onice label Feb 9, 2021
@gui1117
Copy link
Contributor

gui1117 commented Feb 10, 2021

bot merge

@ghost
Copy link

ghost commented Feb 10, 2021

Trying merge.

@ghost ghost merged commit 22441aa into master Feb 10, 2021
@ghost ghost deleted the aj-migrate-balances branch February 10, 2021 12:33
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants