Default Pallet Config Trait

[kianenigma]

[kianenigma]

bot clean

[xlc]

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.

[kianenigma]

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]

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

[sam0x17]

just pushed my re-write of this using my macro_magic crate (now working on the examples/basic example).

Here is debug output from the basic example, tests on basic pass:

foreign_path: frame_system :: preludes :: testing :: TestDefaultConfig


foreign_impl:

impl DefaultConfig for TestDefaultConfig {
    type Version = ();
    type BlockWeights = ();
    type BlockLength = ();
    type DbWeight = ();
    type Index = u64;
    type BlockNumber = u64;
    type Hash = sp_core::hash::H256;
    type Hashing = sp_runtime::traits::BlakeTwo256;
    type AccountId = AccountId;
    type Lookup = IdentityLookup<AccountId>;
    type BlockHashCount = frame_support::traits::ConstU64<10>;
    type AccountData = u32;
    type OnNewAccount = ();
    type OnKilledAccount = ();
    type SystemWeightInfo = ();
    type SS58Prefix = ();
    type MaxConsumers = frame_support::traits::ConstU32<16>;
}


local_impl:

impl frame_system::Config for Test {
    type BaseCallFilter = frame_support::traits::Everything;
    type RuntimeEvent = RuntimeEvent;
    type RuntimeCall = RuntimeCall;
    type RuntimeOrigin = RuntimeOrigin;
    type OnSetCode = frame_system::DefaultSetCode<Self>;
    type PalletInfo = PalletInfo;
    type Header = Header;
    type AccountData = pallet_balances::AccountData<u64>;
}


combined_impl:

impl frame_system::Config for Test {
    type BaseCallFilter = frame_support::traits::Everything;
    type RuntimeEvent = RuntimeEvent;
    type RuntimeCall = RuntimeCall;
    type RuntimeOrigin = RuntimeOrigin;
    type OnSetCode = frame_system::DefaultSetCode<Self>;
    type PalletInfo = PalletInfo;
    type Header = Header;
    type AccountData = pallet_balances::AccountData<u64>;
    type Version = <frame_system::preludes::testing::TestDefaultConfig as frame_system::pallet::DefaultConfig>::Version;
    type BlockWeights = <frame_system::preludes::testing::TestDefaultConfig as frame_system::pallet::DefaultConfig>::BlockWeights;
    type BlockLength = <frame_system::preludes::testing::TestDefaultConfig as frame_system::pallet::DefaultConfig>::BlockLength;
    type DbWeight = <frame_system::preludes::testing::TestDefaultConfig as frame_system::pallet::DefaultConfig>::DbWeight;
    type Index = <frame_system::preludes::testing::TestDefaultConfig as frame_system::pallet::DefaultConfig>::Index;
    type BlockNumber = <frame_system::preludes::testing::TestDefaultConfig as frame_system::pallet::DefaultConfig>::BlockNumber;
    type Hash = <frame_system::preludes::testing::TestDefaultConfig as frame_system::pallet::DefaultConfig>::Hash;
    type Hashing = <frame_system::preludes::testing::TestDefaultConfig as frame_system::pallet::DefaultConfig>::Hashing;
    type AccountId = <frame_system::preludes::testing::TestDefaultConfig as frame_system::pallet::DefaultConfig>::AccountId;
    type Lookup = <frame_system::preludes::testing::TestDefaultConfig as frame_system::pallet::DefaultConfig>::Lookup;
    type BlockHashCount = <frame_system::preludes::testing::TestDefaultConfig as frame_system::pallet::DefaultConfig>::BlockHashCount;
    type OnNewAccount = <frame_system::preludes::testing::TestDefaultConfig as frame_system::pallet::DefaultConfig>::OnNewAccount;
    type OnKilledAccount = <frame_system::preludes::testing::TestDefaultConfig as frame_system::pallet::DefaultConfig>::OnKilledAccount;
    type SystemWeightInfo = <frame_system::preludes::testing::TestDefaultConfig as frame_system::pallet::DefaultConfig>::SystemWeightInfo;
    type SS58Prefix = <frame_system::preludes::testing::TestDefaultConfig as frame_system::pallet::DefaultConfig>::SS58Prefix;
    type MaxConsumers = <frame_system::preludes::testing::TestDefaultConfig as frame_system::pallet::DefaultConfig>::MaxConsumers;
}

Now just need to clean up a tiny bit more, add docs, and get everything else compiling

Also need to re-add #[pallet::no_default]

[kianenigma]

How to wrap this PR:

• [x] Finish the macro rules:
  • Only generate trait DefaultConfig if pallet::default_config is present.
  • If pallet::default_config is present, but no_default is present, either issue a warning or an error.
• [ ] Create a new pallet-example-with-default to showcase this, and write some docs in the PR description (which -- I am sorry -- I have left empty for now 🙈). This PR def. needs to be in release notes.

[sam0x17]

So dealing with a very weird issue here if anyone has any input

In master this code in the authorship pallet (and similar code in several other pallets) compiles fine:

	#[pallet::config]
	pub trait Config: frame_system::Config {
		/// Find the author of a block.
		type FindAuthor: FindAuthor<Self::AccountId>;
		/// An event handler for authored blocks.
		type EventHandler: EventHandler<Self::AccountId, Self::BlockNumber>;
	}

In this branch though, we get these errors:

error[E0220]: associated type `AccountId` not found for `Self`
  --> frame/authorship/src/lib.rs:46:37
   |
46 |         type FindAuthor: FindAuthor<Self::AccountId>;
   |                                           ^^^^^^^^^ associated type `AccountId` not found

error[E0220]: associated type `AccountId` not found for `Self`
  --> frame/authorship/src/lib.rs:48:41
   |
48 |         type EventHandler: EventHandler<Self::AccountId, Self::BlockNumber>;
   |                                               ^^^^^^^^^ associated type `AccountId` not found

error[E0220]: associated type `BlockNumber` not found for `Self`
  --> frame/authorship/src/lib.rs:48:58
   |
48 |         type EventHandler: EventHandler<Self::AccountId, Self::BlockNumber>;
   |                                                                ^^^^^^^^^^^ associated type `BlockNumber` not found

For more information about this error, try `rustc --explain E0220`.
error: could not compile `pallet-authorship` due to 3 previous errors

This is quite odd. If we check the Config trait in frame_system, both in master and in our branch, frame_system::Config has the AccountId type and the BlockNumber type, yet in our branch somehow it can't find these types even though our trait is bounded by frame_system::Config in exactly the same way as it is in master, as far as I can tell.

I double checked the macro expansion of this pallet, and the expanded code for the Config trait for the authorship pallet is exactly the following for both master and our branch:

    pub trait Config: frame_system::Config {
        /// Find the author of a block.
        type FindAuthor: FindAuthor<Self::AccountId>;
        /// An event handler for authored blocks.
        type EventHandler: EventHandler<Self::AccountId, Self::BlockNumber>;
    }

Meaning I don't see how this could be caused by macro shenanigans if the above is the fully expanded code in both scenarios.

So I think to myself, "OK, I'll make Self more explicit" so I do the following, which by the way compiles fine for master:

	#[pallet::config]
	pub trait Config: frame_system::Config {
		/// Find the author of a block.
		type FindAuthor: FindAuthor<<Self as frame_system::Config>::AccountId>;
		/// An event handler for authored blocks.
		type EventHandler: EventHandler<<Self as frame_system::Config>::AccountId, <Self as frame_system::Config>::BlockNumber>;
	}

But this just results in the this shocking error:

error[E0277]: the trait bound `Self: frame_system::Config` is not satisfied
  --> frame/authorship/src/lib.rs:46:3
   |
46 |         type FindAuthor: FindAuthor<<Self as frame_system::Config>::AccountId>;
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `frame_system::Config` is not implemented for `Self`

error[E0277]: the trait bound `Self: frame_system::Config` is not satisfied
  --> frame/authorship/src/lib.rs:48:3
   |
48 |         type EventHandler: EventHandler<<Self as frame_system::Config>::AccountId, <Self as frame_system::Config>::BlockNumber>;
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `frame_system::Config` is not implemented for `Self`

How is frame_system::Config not implemented for Self if literally the line above that says pub trait Config: frame_system::Config {? Is Self no longer the same Self somehow 🤯? What could cause this sort of thing?

I have a rust playground where I've been trying to reproduce this. The basic scenario that works in master looks like this https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=b79da3d7701113fbdd320251f801b05f and compiles fine. I can't figure out any way so far I can mess with that playground to get a similar error that might help me figure out what is going on.

Any ideas or tips would be greatly appreciated!

This applies to the current latest commit / bd68211

[kianenigma]

https://github.com/paritytech/substrate/pull/13454/files#r1117133833 is the underlying reason I believe.

[bkchr]

Yeah

diff --git a/frame/examples/basic/src/lib.rs b/frame/examples/basic/src/lib.rs
index 706a088fa97..6946449b35c 100644
--- a/frame/examples/basic/src/lib.rs
+++ b/frame/examples/basic/src/lib.rs
@@ -367,10 +367,9 @@ pub mod pallet {
        pub trait Config: pallet_balances::Config + frame_system::Config {
                // Setting a constant config parameter from the runtime
                #[pallet::constant]
-               #[pallet::no_default]
                // It is very unfortunate that we cannot have this have a default either, because it relies
                // on `<Self as pallet_balances::Config>`
-               type MagicNumber: Get<Self::Balance>;
+               type MagicNumber: Get<<Self as pallet_balances::Config>::Balance>;
 
                /// The overarching event type.
                type RuntimeEvent: From<Event<Self>> + IsType<<Self as frame_system::Config>::RuntimeEvent>;
diff --git a/frame/support/procedural/src/pallet/expand/config.rs b/frame/support/procedural/src/pallet/expand/config.rs
index baa5ae74cdd..d2f1c792f5a 100644
--- a/frame/support/procedural/src/pallet/expand/config.rs
+++ b/frame/support/procedural/src/pallet/expand/config.rs
@@ -50,9 +50,12 @@ pub fn expand_config(def: &mut Def) -> TokenStream {
                        Ok(krate) => krate,
                        Err(err) => return err.to_compile_error(),
                };
+
+               let super_traits = config_item.supertraits.iter();
+
                quote!(
                        #[#support::macro_magic::export_tokens]
-                       pub trait DefaultConfig {
+                       pub trait DefaultConfig: #( #super_traits )+* {
                                #(#trait_items)*
                        }
                )
diff --git a/frame/support/procedural/src/pallet/parse/config.rs b/frame/support/procedural/src/pallet/parse/config.rs
index b42b06d6aaa..a662b756b5b 100644
--- a/frame/support/procedural/src/pallet/parse/config.rs
+++ b/frame/support/procedural/src/pallet/parse/config.rs
@@ -58,6 +58,8 @@ pub struct ConfigDef {
        /// No, if `None`.
        /// Yes, if `Some(_)`, with the inner items that should be included.
        pub default_sub_trait: Option<Vec<syn::TraitItem>>,
+
+       pub super_traits: Vec<syn::TypeParamBound>,
 }
 
 /// Input definition for a constant in pallet config.
@@ -478,6 +480,7 @@ impl ConfigDef {
                        where_clause,
                        attr_span,
                        default_sub_trait,
+                       super_traits: item.supertraits.iter().cloned().collect(),
                })
        }
 }
diff --git a/frame/system/src/lib.rs b/frame/system/src/lib.rs
index 3f6d2274a0b..ae7745df1f7 100644
--- a/frame/system/src/lib.rs
+++ b/frame/system/src/lib.rs
@@ -209,6 +209,7 @@ pub mod pallet {
                        use macro_magic::export_tokens;
                        use sp_runtime::traits::IdentityLookup;
 
+                       #[derive(Eq, Clone, PartialEq)]
                        pub struct TestDefaultConfig {}
 
                        #[export_tokens(TestDefaultConfig)]

This fixes it

[kianenigma]

Yeah

diff --git a/frame/examples/basic/src/lib.rs b/frame/examples/basic/src/lib.rs
index 706a088fa97..6946449b35c 100644
--- a/frame/examples/basic/src/lib.rs
+++ b/frame/examples/basic/src/lib.rs
@@ -367,10 +367,9 @@ pub mod pallet {
        pub trait Config: pallet_balances::Config + frame_system::Config {
                // Setting a constant config parameter from the runtime
                #[pallet::constant]
-               #[pallet::no_default]
                // It is very unfortunate that we cannot have this have a default either, because it relies
                // on `<Self as pallet_balances::Config>`
-               type MagicNumber: Get<Self::Balance>;
+               type MagicNumber: Get<<Self as pallet_balances::Config>::Balance>;
 
                /// The overarching event type.
                type RuntimeEvent: From<Event<Self>> + IsType<<Self as frame_system::Config>::RuntimeEvent>;
diff --git a/frame/support/procedural/src/pallet/expand/config.rs b/frame/support/procedural/src/pallet/expand/config.rs
index baa5ae74cdd..d2f1c792f5a 100644
--- a/frame/support/procedural/src/pallet/expand/config.rs
+++ b/frame/support/procedural/src/pallet/expand/config.rs
@@ -50,9 +50,12 @@ pub fn expand_config(def: &mut Def) -> TokenStream {
                        Ok(krate) => krate,
                        Err(err) => return err.to_compile_error(),
                };
+
+               let super_traits = config_item.supertraits.iter();
+
                quote!(
                        #[#support::macro_magic::export_tokens]
-                       pub trait DefaultConfig {
+                       pub trait DefaultConfig: #( #super_traits )+* {
                                #(#trait_items)*
                        }
                )
diff --git a/frame/support/procedural/src/pallet/parse/config.rs b/frame/support/procedural/src/pallet/parse/config.rs
index b42b06d6aaa..a662b756b5b 100644
--- a/frame/support/procedural/src/pallet/parse/config.rs
+++ b/frame/support/procedural/src/pallet/parse/config.rs
@@ -58,6 +58,8 @@ pub struct ConfigDef {
        /// No, if `None`.
        /// Yes, if `Some(_)`, with the inner items that should be included.
        pub default_sub_trait: Option<Vec<syn::TraitItem>>,
+
+       pub super_traits: Vec<syn::TypeParamBound>,
 }
 
 /// Input definition for a constant in pallet config.
@@ -478,6 +480,7 @@ impl ConfigDef {
                        where_clause,
                        attr_span,
                        default_sub_trait,
+                       super_traits: item.supertraits.iter().cloned().collect(),
                })
        }
 }
diff --git a/frame/system/src/lib.rs b/frame/system/src/lib.rs
index 3f6d2274a0b..ae7745df1f7 100644
--- a/frame/system/src/lib.rs
+++ b/frame/system/src/lib.rs
@@ -209,6 +209,7 @@ pub mod pallet {
                        use macro_magic::export_tokens;
                        use sp_runtime::traits::IdentityLookup;
 
+                       #[derive(Eq, Clone, PartialEq)]
                        pub struct TestDefaultConfig {}
 
                        #[export_tokens(TestDefaultConfig)]

This fixes it

This is the right explanation, but the not the right solution. We don't want to express DefaultConfig: frame_system::Config because we DON'T want eg preludes::testing::Impl to also implement the pallet::DefaultConfig AND frame_system::Config, and this will become annoying to manage.

A prelude implementation, eg preludes::testing::Impl should be a standalone struct that only implements DefaultConfig of a pallet.

We could change the above assumption, and put some magic that automatically tries to fix the above, but at least for the first iteration, I am against it as it would add a lot of magic behind the scene.

First iteration, this feature is only feasible for type items that have no dependency on frame_system::Config. This will exclude anything that uses AccountId as well.

[bkchr]

We don't want to express DefaultConfig: frame_system::Config because we want eg preludes::testing::Impl to also implement the pallet::DefaultConfig AND frame_system::Config, and this will become annoying to manage.

Yeah, I just realized this now as well :see_no_evil:

[bkchr]

But what about this: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=9fe9e04140fd0316ab4dff307f9de8e7

Aka we forward the actual type that implements Config to the DefaultTrait. Then the macro can rewrite all the requirements on Self::AccountId into T::AccountId etc.

[sam0x17]

But what about this: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=9fe9e04140fd0316ab4dff307f9de8e7

I don't see any problem with this @bkchr curious what @kianenigma thinks. Alternatively we could instead just add this in a follow-up PR 🤔

[sam0x17]

With the latest push (and after much difficulty) this now actually works with the recent syn 2.x upgrade, and I ended up having to re-write most of the new config parsing which is fine because we wanted to shore that up anyway

[sam0x17]

note: just added macro_magic v0.3.3 to support custom import_tokens_attr input parsing to allow for multiple/custom arguments on #[import_tokens_attr]-style attribute macros, since this is needed to make the disambiguation_path customizable via #[derive_impl(..)] in this PR.

[sam0x17]

docs have been updated, adding UI tests and then should be good to review

[sam0x17]

bot rebase

[paritytech-processbot[bot]]

Rebased

[sam0x17]

expecting the pallet crate to contain a prelude module which in itself contains an impl for the DefaultConfig.

@KiChjang I agree we need more docs and that we need to be thorough in how we announce this to the community, but the prelude thing isn't a convention we need to expect people to follow -- yes kian decided to place the frame_system testing impl inside of frame_system::prelude::testing, but there is no requirement that other people follow this pattern, nor is prelude mentioned in derive_impl or any of the other macro implementations. You might be thinkng of how if you mark a pallet config with #[pallet::default_config], a DefaultConfig trait will be generated within the scope of the pallet module, but this isn't really a convention like you're describing, more just a thing that happens if you use these macros, and one that is documeted in the macro docs accordingly.

TLDR: there is no requirement about anything with a prelude, and DefaultConfig is generated automatically and within the pallet module if you attach #[pallet::default_config] to the pallet config.

Or did I misunderstand you?

[KiChjang]

The DefaultConfig trait is automatically generated, yes, but what about its implementors? As a dev using this pallet, I'm not as interested in the trait than I am with its implementors, because that's what I use to save myself from typing a lot of config items that already have a default.

It now looks like the prelude is the convention of where we store the implementors. This is what we need to communicate to our users, that there is a new prelude convention where we put these accessory types that are useful for configuring or interacting with the pallet.

[sam0x17]

The DefaultConfig trait is automatically generated, yes, but what about its implementors? As a dev using this pallet, I'm not as interested in the trait than I am with its implementors, because that's what I use to save myself from typing a lot of config items that already have a default.

It now looks like the prelude is the convention of where we store the implementors. This is what we need to communicate to our users, that there is a new prelude convention where we put these accessory types that are useful for configuring or interacting with the pallet.

Ah ok, yes, this makes sense 👍🏻

I will add a blurb to the PR at the top about this so it is included in the release notes

[sam0x17]

TODO:

• [ ] Update release notes to mention new convention on pallet preludes
• [x] make it clear as Whatever is optional in the docs
• [x] Re-work default_sub_trait to just be a Vec
• [x] Clean up parsing of PalletAttr so that no_default and constant are keywords again
• [x] rename foreign_path => default_impl_path
• [x] add example pallet
• [ ] Generally improve docs

[kianenigma]

What worries me about this PR is that aside from just the new concepts we're introducing, we're also introducing new conventions that aren't documented anywhere, e.g. expecting the pallet crate to contain a prelude module which in itself contains an impl for the DefaultConfig.

The documentation of pallet::default_config (or pallet::config) should cover this. Note that the prelude module and further are indeed just conventions. All of this can and will work with other names too.

[kianenigma]

TODO:

• [x] Update release notes to mention new convention on pallet preludes
• [x] make it clear as Whatever is optional in the docs
• [x] Re-work default_sub_trait to just be a Vec
• [x] Clean up parsing of PalletAttr so that no_default and constant are keywords again
• [x] rename foreign_path => default_impl_path
• [x] add example pallet
• [x] Generally improve docs

I think these really are the leftover things that I can also identify, other than the unresolved comments from me and @bkchr.

I cannot approve this @sam0x17, but you can approve on my behalf whenever you feel comfortable.

I would like to get this merged soon, and we can incrementally improve corners as we start using it. It can already greatly help https://github.com/paritytech/polkadot-sdk-docs/

[sam0x17]

example pallet is up, circling back on the other items

[kianenigma]

bot rebase

[paritytech-processbot[bot]]

Rebased

[kianenigma]

bot merge