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

Allow pallet's info to be enumerated #10053

Merged
merged 18 commits into from
Oct 21, 2021
Merged

Conversation

gavofyork
Copy link
Member

@gavofyork gavofyork commented Oct 18, 2021

Followup to #9690, allowing the feature to actually be used.

Needed for paritytech/polkadot#4097.

Annoying this cannot be done outside of the module (e.g. in Polkadot) due to conflicting blanket impls of the trait PalletInfoAccess between pallets and tuples.

It also cannot be done by returning an impl Iterator since apparently trait functions are not allowed to do this.

@github-actions github-actions bot added the A0-please_review Pull request needs code review. label Oct 18, 2021
@gavofyork gavofyork added 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. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Oct 18, 2021
@gavofyork gavofyork added this to In progress in Runtime via automation Oct 18, 2021
@KiChjang
Copy link
Contributor

It also cannot be done by returning an impl Iterator since apparently trait functions are not allowed to do this.

I'm not quite certain what the context behind this is, but you can return a Box<dyn Iterator>, if trait objects isn't a huge issue?

@gavofyork
Copy link
Member Author

Could do that, but it's probably faster to do one single heap allocation than to do N dynamic dispatches.

frame/support/procedural/src/construct_runtime/mod.rs Outdated Show resolved Hide resolved
frame/support/src/migrations.rs Outdated Show resolved Hide resolved
@gavofyork
Copy link
Member Author

Ok - reverted back to the old form. I think this may need to be revisited in the future since the call depth of accumulate will be equal to the number of pallets.

CC @bkchr @thiolliere @KiChjang

Comment on lines +105 to +106
T2::accumulate(acc);
T1::accumulate(acc);
Copy link
Contributor

Choose a reason for hiding this comment

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

This ordering will produce vec![T2, T1], is that what we need to re-reverse the order of the pallets?

Copy link
Member Author

Choose a reason for hiding this comment

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

indeed. i'll put a comment in.

Copy link
Contributor

@KiChjang KiChjang left a comment

Choose a reason for hiding this comment

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

Don't see problems from my side after the comment.

Copy link
Contributor

@gui1117 gui1117 left a comment

Choose a reason for hiding this comment

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

looks good to me

/// Relevant for tuples.
///
/// You probably don't want this function but `infos()` instead.
fn accumulate(_accumulator: &mut Vec<PalletInfoData>) {}
Copy link
Contributor

@gui1117 gui1117 Oct 21, 2021

Choose a reason for hiding this comment

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

I usually prefer not to have default implementation when it needs to be overriden in most implementation, this can be error prone, someone can miss to override one implementation.

Runtime automation moved this from In progress to Needs Audit Oct 21, 2021
@gavofyork gavofyork merged commit 1175446 into master Oct 21, 2021
@gavofyork gavofyork deleted the gav-enumerate-pallet-info branch October 21, 2021 08:29
Runtime automation moved this from Needs Audit to Done Oct 21, 2021
@shawntabrizi shawntabrizi moved this from Done to Archive in Runtime Nov 11, 2021
grishasobol pushed a commit to gear-tech/substrate that referenced this pull request Mar 28, 2022
* Allow pallet's info to be enumerated

* Fixes

* Formatting

* Flat tuple for getting all pallet instances

* Renaming and fixing reversedness

* Formatting

* Fixes

* Back to nesting

* Back to nestingx

* Revert executive lib

* Reversions

* Reversions

* Fixes

* Fixes

* Formatting

* Fixes

* Spelling

* Comments
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
* Allow pallet's info to be enumerated

* Fixes

* Formatting

* Flat tuple for getting all pallet instances

* Renaming and fixing reversedness

* Formatting

* Fixes

* Back to nesting

* Back to nestingx

* Revert executive lib

* Reversions

* Reversions

* Fixes

* Fixes

* Formatting

* Fixes

* Spelling

* Comments
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. 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. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants