add frame_system::DefaultConfig to individual pallet DefaultConfigs

[kianenigma]

step in paritytech/polkadot-sdk#171. It solves the issue explained in the next section, and it makes pallet-proxy and pallet-multisig use derive_impl for frame-system and pallet-balances.

Overview

This changes the DeriveImpl experimental line of work as follows. Before this PR, the generated trait always looked like this:

trait DefaultConfig {
  Item1;
  Item2;
  ...
}

After this PR, if the trait Config is itself bound by frame_system::Config, then it generates the following:

trait DefaultConfig: frame_system::DefaultConfig {
  Item1;
  Item2;
  ...
}

This requires you to implement for frame_system::DefaultConfig eg. TestDefaultConfig, but luckily this can in itself be done by derive_impl. For example, in pallet-example-default-config, we have:

pub struct TestDefaultConfig;

#[derive_impl(frame_system::config_preludes::TestDefaultConfig as frame_system::DefaultConfig)]
impl frame_system::DefaultConfig for TestDefaultConfig {}

#[frame_support::register_default_impl(TestDefaultConfig)]
impl DefaultConfig for TestDefaultConfig {
	...
}

This allows types that are reliant on frame_system::Config to also possibly have defaults, as long as they are present in frame_system::DefaultConfig.

Something like type Foo: Get<<Self as frame_system::Config>::RuntimeCall> still can (and for the foreseeable future will) not have a default, as it is not present in frame_system::DefaultConfig.

Next Steps / Questions

Currently, the code for impl DefaultConfig for SomeStruct is pure Rust. So, we don't have any way to inject impl frame_system::DefaultConfig for TestDefaultConfig {} into it. For now, I am happy to keep it as is, but possibly prior to stabilization, we can have something like #[pallet::default_config_impl] struct TestDefaultConfig and this should:

1. auto-inject register_default_impl
2. possibly auto-inject impl frame_system::DefaultConfig for _

Moreover, the current implementation indeed relies on the name frame_system::DefaultConfig, which is fine for now.

[bkchr]

Not sure why you not try out what I had outlined here: #13454 (comment)

This should also give you access to the types from frame-system.

[kianenigma]

Not sure why you not try out what I had outlined here: #13454 (comment)

This should also give you access to the types from frame-system.

I had actually overlooked it, thanks for the follow-up. I will look into it and revise.

[kianenigma]

@bkchr the only issue I see with your approach is what I have highlighted here: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=34052108e3149aedb7f80e367eac85c7

• Solution1: mandate fully-qualified syntax eg <Self as frame_system::Config>::AccountId to be used. Then replace it with T::AccountId.
• Solution2: We know all other Self types, so whatever is not in Self must have come from <Self as frame_system::Config>::AccountId and must be replaced with T::AccountId.
• Solution3: screw all of this, let the user write DefaultConfig manually :))

I will probably try Solution 2 for now, but it is note worthy that this will become a mishmash as soon as you start bounding Config to outer traits, which I have to check if we already allow in FRAME or not. If not, then the approaches here are reasonable, if we allow that, I need to think about it more.

UPDATE: I am not quite sure if 2 works given that we can have some type alias like BalanceOf. Only 1 will work, assuming you don't use such type aliases.

[bkchr]

@bkchr the only issue I see with your approach is what I have highlighted here: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=34052108e3149aedb7f80e367eac85c7

@kianenigma the link doesn't work.

[bkchr]

• Solution2: We know all other Self types, so whatever is not in Self must have come from <Self as frame_system::Config>::AccountId and must be replaced with T::AccountId.

If I understand this correctly, it sounds like the correct solution. Basically you are saying, if we see that Self::Whatever the Whatever is declared in the trait itself, then we don't need to replace Self with T?

[gupnik]

@bkchr I was looking into your suggestion, specially around:

trait DefaultSomethingConfig<T: SomethingConfig> {
    type DoSomething: DoSomething<T::AccountId>;
    type Whatever;
}

However, we would need the generic to be bound by DefaultSystemConfig to use the correct types, something like:

trait DefaultSystemConfig {
    type AccountId = u64;
}

trait DefaultSomethingConfig<T: DefaultSystemConfig> {
    type DoSomething: DoSomething<T::AccountId>;
    type Whatever;
}

Thus, we would need a separate expansion for DefaultSystemConfig and other default configs would assume this to be present. I am not completely convinced that it would be a good solution, but let me know your thoughts.

CC: @kianenigma @sam0x17

[kianenigma]

However, we would need the generic to be bound by DefaultSystemConfig to use the correct types, something like:

Not suer why this is the case? Ideally, we want the DefaultConfig of any pallet to be generic over <T: system::Config>, not <T: system::DefaultConfig>, as the former has all the types we need, while the latter is only a subset.

Perhaps it would be useful to clean up the rust-playground link with some better names (I find this SomethingSomething examples a bit confusing) and showcase exactly what you mean.

[gupnik]

@kianenigma Please take a look at this one: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=09bd639ef9fa8e32f8bb552273517c2e

[bkchr]

@kianenigma Please take a look at this one: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=09bd639ef9fa8e32f8bb552273517c2e

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=b247134b042de6ed1adaa772fd79b477

Question is, how often do we see this? But this is the proper solution using Rust.

[gupnik]

Question is, how often do we see this? But this is the proper solution using Rust.

IMO, this would actually happen for all types that are referenced in the super trait and the current trait. The reason being that the type in the actual config would be different than DefaultConfig. However, we are specifying the generic bound as the config (and not DefaultConfig), causing the conflict.

[bkchr]

IMO, this would actually happen for all types that are referenced in the super trait and the current trait.

That was not my question. My question was on how many types we have where this is required. For sure for stuff like Balance this will probably happen quite often, but it also doesn't look that ugly/is that special.

[gupnik]

IMO, this would actually happen for all types that are referenced in the super trait and the current trait.

That was not my question. My question was on how many types we have where this is required. For sure for stuff like Balance this will probably happen quite often, but it also doesn't look that ugly/is that special.

I can think of AccountId, AssetId and AssetBalance. Probably a few others as well?

[bkchr]

Yeah for sure, but what would be the other way to solve this?

[gupnik]

bot fmt

[command-bot]

@gupnik https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3181922 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/substrate/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 23-10a722c3-39bb-46e2-97b1-ae1eb743cfc9 to cancel this command or bot cancel to cancel all commands in this pull request.

[command-bot]

@gupnik Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3181922 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3181922/artifacts/download.

[gupnik]

@bkchr @kianenigma @sam0x17 This is ready for review.

[bkchr]

@bkchr @kianenigma @sam0x17 This is ready for review.

Another point this pr is missing is when a pallet has not only the system pallet as supertrait, but also another pallet. So, we should be generic over the supertraits.

[kianenigma]

With the way this PR is working, there seems to be no path to have defaults for things like RuntimeEvent. Is this correct, or do you have a plan around those?

[gupnik]

With the way this PR is working, there seems to be no path to have defaults for things like RuntimeEvent. Is this correct, or do you have a plan around those?

I was hoping to pick up this suggestion from @sam0x17 as a follow-up - #14437 (comment)

[kianenigma]

With the way this PR is working, there seems to be no path to have defaults for things like RuntimeEvent. Is this correct, or do you have a plan around those?

I was hoping to pick up this suggestion from @sam0x17 as a follow-up - #14437 (comment)

I don't think that this will work all the way though, even with pallet::no_bound. With this macro, we will be able to add items to systemDefaultConfig without any bounds, such as:

// frame-system
trait DefaultConfig {
    type Block: Bound1 + Bound2 + ...
    type RuntimeEvent; // no bound, because we marked it as such.
}

But then what? still, somewhere, in frame/system/src/lib.rs we need to provide an

// still in frame-system
impl frame_system::DefaultConfig for PoorTypeThatCannotStillProvideADefaultForRuntimeEvent {
    type RuntimeEvent = ???
}

Which as I joked in the name, still cannot provide a sensible default for RuntimeEvent, even if it has no bounds.

The only solution to fix this, as far as I can see, is to make something be generic over <X: frame_system::Config> (or as a super-trait, as opposed to frame_system::DefaultConfig)

[gupnik]

The only solution to fix this, as far as I can see, is to make something be generic over <X: frame_system::Config> (or as a super-trait, as opposed to frame_system::DefaultConfig)

Thanks, I do see your point but I am not completely sure if this approach would work as well. Even if DefaultConfig is defined as follows:

trait DefaultConfig<T: frame_system::Config> {
   type RuntimeEvent;
}

this would still require someone to specify RuntimeEvent at the impl site?

My idea with #[verbatim] was to allow defaults that are replaced with the same names at the impl site, assuming that such a type exists in scope. So, you would have

trait DefaultConfig {
    type RuntimeEvent; // no bound, because we marked it as such.
}

struct TestDefaultConfig;
impl DefaultConfig for TestDefaultConfig {
   #[verbatim] // add an attribute here to hint derive_impl
   type RuntimeEvent = ();
}

and later derive_impl replaces it as

impl frame_system::Config for Runtime {
   type RuntimeEvent = RuntimeEvent;
}

Note that no_bound and verbatim are entirely separate macros here. The latter is actually used when providing the default implementation. I am just worried that we are making too many assumptions here, but that might be acceptable?

This verbatim attribute while implementing the DefaultConfig might actually be extended to provide other types that are assumed to be present at the impl site. For example,

struct TestDefaultConfig;
impl DefaultConfig for TestDefaultConfig {
   #[verbatim(TestBlock)] 
   type Block = ();
}

becomes

impl frame_system::Config for Runtime {
   type Block = TestBlock;
}

and assumes that TestBlock exists.

I still need to see this in actual code to make sure that it works, but just wanted to put it down here to see if this makes sense at all.

[sam0x17]

I am actually writing the docs right now for supertrait but I think it might handle these scenarios (will have to double check). It handles generics perfectly and you can make use of Self:: types etc. i.e. https://github.com/sam0x17/supertrait/blob/main/tests/tests.rs#L231-L279 so I'll try to have this published by end of weekend

[gupnik]

I still need to see this in actual code to make sure that it works, but just wanted to put it down here to see if this makes sense at all.

#14682 describes the POC for this idea. @kianenigma @bkchr @sam0x17 Would love to get some feedback on it.

[sam0x17]

This looks reasonable to me. I think this should migrate over painlessly to supertrait, which I'm planning to release on crates.io tonight and should handle some of these edge cases, especially around generics, more gracefully

[kianenigma]

Can't approve this myself, but I had another look and happy to see this merged :)

[kianenigma]

I think this should migrate over painlessly to supertrait, which I'm planning to release on crates.io tonight and should handle some of these edge cases, especially around generics, more gracefully

I'm not yet clear on what issue we exactly solve with supertrait? I would appreciate it if you write down the issues that we have now, in an issue (pun unintended), and how we intend to solve them, if there are any.

To me it seems like with a combination of this and #14682 we will get all the features that we want.

[gupnik]

bot merge