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

[FRAME Core] Default Pallet Config Trait / derive_impl #13454

Merged
merged 124 commits into from
May 30, 2023

Conversation

kianenigma
Copy link
Contributor

@kianenigma kianenigma commented Feb 23, 2023

WARNING

This feature is not extensively used and tested in Substrate crates yet. Its usage in tests is encouraged (whilst reporting any issues you encounter), but not in production yet.

See paritytech/polkadot-sdk#171 to track the progress.

At a Glance

  • In short, this PR adds the ability for a pallet to define one or more defaults for its trait Config). This is dine via #[pallet::config(with_default)].
  • Then, when writing impl Config for Runtime, one of these defaults can be used to elide any missing type item. This is done via #[derive_impl(path_to_default_impl)]
  • At the moment, this is limited to type items that do not rely on frame_system::Config. Such types must be marked by #[pallet::no_default].
  • RuntimeEvent is one such type and is automatically excluded.

End-to-End Example

See ./frame/example/default-config for the live version of this example.

Details

derive_impl

This PR adds several attribute proc macros to frame_support, most notably #[derive_impl(..)] and #[register_default_impl(..)], which in tandem make it possible to create trait impls that provide sane defaults and (via macros) dynamically inject the non-colliding trait items from these impls into downstream trait impls that want to use the sane defaults but potentially override some of them.

This is a general-purpose behavior that can be used for a number of things, though the specific purpose of this PR is to use this for implementing "default" pallet test config traits, as outlined below.

Changes/Additions

  • Adds a new optional option to #[pallet::config]: #[pallet::config(with_default)]. When enabled, this option will spit out a pub trait DefaultConfig in the pallet containing trait items from the pallet's Config that are not marked with #[pallet::no_default] (a new attribute).
  • Adds a new pallet attribute #[pallet::no_default], used in tandem with #[pallet::config(with_default)] as described above.
  • Default configs implemented for frame_system
  • Example default-config-example pallet added demonstrating how to use this
  • Numerous UI and unit tests covering all of this
  • Documentation covering these additions

@kianenigma
Copy link
Contributor Author

bot clean

@xlc
Copy link
Contributor

xlc commented Feb 24, 2023

For Get types, we could make it fn block_number() { 100 } and use the default trait impl feature of Rust.
The only downside is that will be a big breakinig change.

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.

Looks very cool on first glance!

frame/balances/src/lib.rs Outdated Show resolved Hide resolved
frame/examples/basic/src/tests.rs Outdated Show resolved Hide resolved
frame/support/procedural/src/derive_impl.rs Outdated Show resolved Hide resolved
frame/support/procedural/src/derive_impl.rs Outdated Show resolved Hide resolved
frame/system/src/lib.rs Outdated Show resolved Hide resolved
@kianenigma
Copy link
Contributor Author

For Get types, we could make it fn block_number() { 100 } and use the default trait impl feature of Rust. The only downside is that will be a big breakinig change.

Yeah we've discussed this as well many times, but never got to a census about it :( but as you said, it will be a big change. This is all backwards compatible. Even this line of work can be partially set aside once we have default associated type in Rust.

@ggwpez
Copy link
Member

ggwpez commented Mar 1, 2023

@thiolliere I bet you are interested in this 😄
It aims to give default Config trait impls.

@kianenigma kianenigma requested a review from KiChjang May 29, 2023 09:20
@kianenigma
Copy link
Contributor Author

bot rebase

@paritytech-processbot
Copy link

Rebased

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.

LGTM after addressing my comment

@kianenigma
Copy link
Contributor Author

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 06d8934 into master May 30, 2023
@paritytech-processbot paritytech-processbot bot deleted the kiz-simpkified-runtime branch May 30, 2023 08:06
@kianenigma kianenigma changed the title Default Pallet Config Trait / derive_impl [FRAME Core] Default Pallet Config Trait / derive_impl May 30, 2023
brunopgalvao added a commit that referenced this pull request Jul 16, 2023
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
* first draft, probably won't work

* first draft, probably won't work

* good progress..

* good milestone, still a lot to do.

* EVERYTHING WORKS

* Update frame/support/procedural/src/derive_impl.rs

Co-authored-by: Oliver Tale-Yazdi <[email protected]>

* Update frame/support/procedural/src/derive_impl.rs

Co-authored-by: Oliver Tale-Yazdi <[email protected]>

* clean up + cargo fmt

* import tokens WIP

* export_tokens working with impl Trait

* WIP / notes

* use macro_magic 0.2.0's export_tokens to access foreign items

* token importing working properly using macro_magic 0.2.5

* combine_impls almost working

* successfully get foreign path via macro_magic 0.2.6

* combine_impls using implementing_type generics

* working + clean up

* more clean up

* decrease rightwards drift and add docs to combine_impls

* add support for macros to impl_item_ident in case we hit that

* add docs for impl_item_ident method

* fix no_std issues

* re-export of macro_magic working in pallets 🎉

* clean up + fully resolve no_std issue with macro_magic with v0.2.11

* remove trait item code for different trait item types since this
is now handled directly by combine_impls

* clean up

* remove dev comments

* only generate default trait if #[pallet::default_trait] is attached

* authorship and most other pallets now compiling

* compiling 🎉

* add check for more than two pallet attributes on Config trait

* remove unused import in nomination-pool

* clean up debug code

* upgrade to macro_magic v0.2.12

* add neater #[register_default_config(SomeIdent)] macro

* really just a thin wrapper around #[export_tokens]

* upgrade to macro_magic 0.3.1

* rewrite parsing to be compatible with syn 2.x, compiling 🎉

* remove unused keywords

* macro stubs for the new pallet:: macros, preliminary docs

* upgrade to macro_magic v0.3.2

* rename register_default_config => register_default_impl

* bump to macro_magic v0.3.3

* custom disambiguation_path working as 2nd arg to derive_impl

* overhaul docs

* fixes, ident-style paths shortcut working

* remove ident-style shortcut because it makes testing difficult

* add passing UI tests for derive_impl

* switch to `ForeignPath as DisambiguationPath` syntax + update docs

* add UI test for bad foreign path

* add UI test for bad disambiguation path

* add UI test for missing disambiguation path

* add UI test for attached to non impl

* fix derive_impl_attr_args_parsing test

* move tests to bottom

* fix nightly issue

* add doc notes on importing/re-exporting

* remove explicit use of macro_magic::use_attr

Co-authored-by: Bastian Köcher <[email protected]>

* use explicit macro_magic::use_attr

Co-authored-by: Bastian Köcher <[email protected]>

* remove unneeded {}

Co-authored-by: Bastian Köcher <[email protected]>

* remove unneeded collect

Co-authored-by: Bastian Köcher <[email protected]>

* add docs for TestDefaultConfig

* remove unneeded `#[export_tokens]` on `DefaultConfig`

* add docs for auto-generated `DefaultConfig`

* no need to clone

Co-authored-by: Bastian Köcher <[email protected]>

* clean up combine_impls + compiling again

* remove unused dependency

* simplify struct definition

Co-authored-by: Bastian Köcher <[email protected]>

* fix register_default_impl docs

* reduce rightward drift / refactor

Co-authored-by: Keith Yeung <[email protected]>

* fix derive_impl after keith's changes

* simplify disambiguation_path calculation

Co-authored-by: Keith Yeung <[email protected]>

* compiling again

* simplify parsing of trait item

Co-authored-by: Keith Yeung <[email protected]>

* rename preludes => prelude

Co-authored-by: Keith Yeung <[email protected]>

* fix other places where we used preludes instead of prelude

* fix indents

* simplify PalletAttr parsing

Co-authored-by: Keith Yeung <[email protected]>

* go back to having no_default and constant as keywords

* make it more clear that disambiguation_path is optional

* make default_trait_items just a Vec instead of Option<Vec>

* rename foreign_path => default_impl_path within substrate

* fix docs

* Change {} to ;

Co-authored-by: Bastian Köcher <[email protected]>

* highlight full end-to-end example with link

* add pallet-default-config-example, start by copying dev mode code

* update dev-mode specific docs

* use Person and Points instead of Dummy and Bar

* add docs to example pallet

* revert changes to pallets other than the default config example

* fix outdated references to basic example pallet

* re-order docs to be a bit more clear

* better errors for extra attributes

* add UI tests for duplicate/extra attributes on trait items

* change `#[pallet::default_config]` to option on `#[pallet::config()]`

* update UI tests
* add UI test covering missing `#[pallet::config(with_default)]` when
  `#[pallet::no_default]` is used

* add note about new optional conventions

* improve docs about `DefaultConfig` and link to these from a few places

* fix doc comment

* fix old comment referencing `pallet::default_config`

* use u32 instead of u64 for block number

Co-authored-by: Kian Paimani <[email protected]>

* use () instead of u32 for `AccountData`

Co-authored-by: Kian Paimani <[email protected]>

* use ConstU32<10> for BlockHashCount instead of ConstU64<10>

Co-authored-by: Kian Paimani <[email protected]>

* people are not dummies

Co-authored-by: Liam Aharon <[email protected]>

* fix wording

Co-authored-by: Just van Stam <[email protected]>

* Person => People and compiling again

* add docs for `prelude` module in frame_system

* update Cargo.lock

* cleaner example

* tweaks

* update docs more

* update docs more

* update docs more

* update docs more

* fix ui tests

* err

* Update frame/support/test/tests/pallet_ui.rs

* update ui tests

---------

Co-authored-by: Oliver Tale-Yazdi <[email protected]>
Co-authored-by: Sam Johnson <[email protected]>
Co-authored-by: parity-processbot <>
Co-authored-by: Bastian Köcher <[email protected]>
Co-authored-by: Keith Yeung <[email protected]>
Co-authored-by: Liam Aharon <[email protected]>
Co-authored-by: Just van Stam <[email protected]>
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. 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
Status: Done
Status: Done
Development

Successfully merging this pull request may close these issues.

8 participants