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

Make pallet macro generate accessor to PalletInfo information on pallet placeholder #8630

Merged
6 commits merged into from
Apr 19, 2021

Conversation

gui1117
Copy link
Contributor

@gui1117 gui1117 commented Apr 16, 2021

paritytech/polkadot-sdk#324

PalletInfoAccess trait is introduced, and implemented by pallet macro on Pallet.

Controversial

  • Maybe we can make it more explicit by only generating them when user give the attribute: #[implement_pallet_info_access)] or something similar.
  • (We can also implement fn index and fn name directly on the struct to avoid having to import the trait in scope, but this makes generation less expectable and can break user code if they implement a function with such a name already)

@github-actions github-actions bot added the A0-please_review Pull request needs code review. label Apr 16, 2021
@gui1117 gui1117 requested a review from gavofyork April 16, 2021 14:33
@gui1117 gui1117 changed the title Make pallet macro generates accessor to PalletInfo information on pallet placeholder Make pallet macro generate accessor to PalletInfo information on pallet placeholder Apr 16, 2021
@gui1117 gui1117 added 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 labels Apr 16, 2021
@bkchr
Copy link
Member

bkchr commented Apr 16, 2021

Why not make it a trait?

@gui1117
Copy link
Contributor Author

gui1117 commented Apr 16, 2021

Why not make it a trait?

I thought maybe ppl would be annoyed to have to import the trait in scope.
But yes a trait seems better indeed, I'm doing so

@gui1117 gui1117 added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A0-please_review Pull request needs code review. labels Apr 16, 2021
@gui1117 gui1117 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 Apr 16, 2021
@gui1117
Copy link
Contributor Author

gui1117 commented Apr 16, 2021

now implemented with trait.

@bkchr
Copy link
Member

bkchr commented Apr 17, 2021

We could put the trait into the prelude

/// Provides information about the pallet setup in the runtime.
///
/// Access the information provided by [`PalletInfo`] for a specific pallet.
pub trait PalletInfoAccess {
Copy link
Member

Choose a reason for hiding this comment

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

Is PalletInfo already taken? Access is weird.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PalletInfo is the trait giving the info for all pallets https://substrate.dev/rustdocs/v3.0.0/frame_support/traits/trait.PalletInfo.html
It is implemented by an associated type on frame-system https://substrate.dev/rustdocs/v3.0.0/frame_system/pallet/trait.Config.html#associatedtype.PalletInfo

Ideally PalletInfo could be renamed PalletsInfo as it contains info for all pallets, and thus we could use PalletInfo here.

Anyway those 2 traits are very related, PalletInfoAccess is implementing accessor to the info of the pallet. info of the pallet are retrieved from PalletInfo which contains info for all pallets (but which is cumbersome to use directly).

Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally PalletInfo could be renamed PalletsInfo as it contains info for all pallets, and thus we could use PalletInfo here.

Totally agree with this, maybe even in this PR? the naming is weird, but the functionality is great, working with the old PalletInfo was not simple.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At the same time I prefer not to break, as PalletInfoAccess is also an OK name to me.
Maybe we can write it somewhere for when we want to break frame stuff.

Copy link
Contributor

Choose a reason for hiding this comment

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

sure, lets stay backwards compat.

@gui1117
Copy link
Contributor Author

gui1117 commented Apr 19, 2021

bot merge

@ghost
Copy link

ghost commented Apr 19, 2021

Error: Approval criteria was not satisfied.

The following errors might have affected the outcome of this attempt:

  • 'Benchmarking and Weights' does not match any projects in substrate's Process.json

Merge failed. Check out the criteria for merge.

@gui1117
Copy link
Contributor Author

gui1117 commented Apr 19, 2021

bot merge

@ghost
Copy link

ghost commented Apr 19, 2021

Trying merge.

@ghost ghost merged commit 3a19eb6 into master Apr 19, 2021
@ghost ghost deleted the gui-pallet-accessor branch April 19, 2021 10:09
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. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants