Introduce new DefaultConfigs to remove boilerplate code from SP runtimes using derive_impl

[seadanda]

It is possible to define DefaultConfigs for each pallet which can be used with derive_impl. The system parachains runtimes share a lot of copy and pasted boilerplate code which can be removed through adding more specific default configs for the various pallets used throughout these runtimes.

This is a proof of concept for frame_system (eebbcfd), which also removes some weights constants files which are unchanged across all SPs. The aim is that a DefaultConfig for all pallets used commonly in system parachains could be defined for the SystemParachainDefaultConfig struct, and then used across all testnet runtimes. This will hopefully reduce the maintenance burden of adding new configurations which are unlikely to change across most system parachains.

In this proof of concept, currently only one runtime has been updated (a21e2fb). The others can follow if it's seen to be an improvement.

If this is a worthwhile refactor then a similar module could be added in the fellows repo to define defaults for those runtimes.

[paritytech-cicd-pr]

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable-int
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6413056

[muharem]

I like the idea. I'd also love to see a short proposal on how we will structure these common implementations.

Right now, we have common crates in the Polkadot SDK and constants crates for each runtime. However, we don't have a common crate in the runtimes repo (though we probably should and move some implementations from the sdk to the runtimes). There are a few questions we need to answer. Should we have a common crate for all runtimes, or separate ones for relay and system parachains? Maybe even for the asset-hubs as well? Should we have one constants crate per runtime or better network? Should we share default configs between parachains only, or include relay chains too?

It would be helpful to have a short doc, maybe in the README, outlining where and what should be located. This would help keep everything consistent.

[muharem]

Do we really need the parameters that are going to be overloaded in the impl for Runtime?
Whenever I need to change some config parameter, if I find it in default config, after I change it, it's good to have a confidence that it wont be overloaded, and I do not need to check every runtime config.

[seadanda]

Yes I initially wanted to do this, but we need every trait member that hasn't been declared as no_default in pallet_system::Config, but then if we mark them as no_default they will have to be specified manually in testing and relays etc. We'd need the following types to be marked no_default

type SystemWeightInfo = ();
type SS58Prefix = ();
type Version = ();
type BlockWeights = ();
type DbWeight = ();
type OnSetCode = ();

I agree that it would be a better misconfiguration check if the compiler told us when something was left out unintentionally, even if the relays and tests had a few more unit types to fill in when they are configuring the pallet. I would support tagging these as no_default. WDYT @gupnik @kianenigma? Or is there a more elegant solution? For example the introduction of no_default_except_tests or something similar.

[kianenigma]

My best advice is #4689 (comment). TLDR; Provide a default that will make the compiler happy, but will force downstream users to override it. I find what is being done here to be too risky.

Option 2: remember that #[pallet::config(with_default)] is merely one way to generate trait DefaultConfig. We can use the underlying machinery and generate our own custom trait DefaultConfig and still be able to use derive_impl with it.

@gupnik how far are we from being able to do this? If it is possible, where is it documented? If not, it is worth being noted in an issue, with an evaluation of how much work it is.

I have other use cases in mind that I specualte will be useful, if we allow anyone to provide trait DefaultConfig for any pallet, and the derive_impl syntax to be able to pick any of them up.

[gupnik]

We can use the underlying machinery and generate our own custom trait DefaultConfig and still be able to use derive_impl with it

Indeed! This is the way I imagine it would look like:

pub trait CustomDefaultConfigProvider {
	type RuntimeOrigin;
	type RuntimeCall;
	type RuntimeTask;
}

pub struct CustomDefaultConfig;

#[crate::register_default_impl(CustomDefaultConfig)]
impl CustomDefaultConfigProvider for CustomDefaultConfig {
	#[inject_runtime_type]
	type RuntimeOrigin = ();
	#[inject_runtime_type]
	type RuntimeCall = ();
	#[inject_runtime_type]
	type RuntimeTask = ();
}

struct Runtime;

#[derive_impl(CustomDefaultConfig, no_aggregated_types)
impl Config for Runtime {}

Will try this out and add as an example via docify if works.

[gupnik]

Added an issue for this: #4827

[kianenigma]

Will try this out and add as an example via docify if works

Did it work?

[gupnik]

Will try this out and add as an example via docify if works

Did it work?

It did! Just need to find some time to put it into a PR :)

[kianenigma]

I hope @seadanda @muharem will be happy to progress this with the approach provided in #5246?

[seadanda]

I've used the method that @kianenigma suggested in #5246 which I must say is a very nice way to do it. Leaving something out of the implementation in the runtime leads to a reasonably clear error that points to the exact associated type required:

  error[E0277]: the trait bound `MustBeOverridden: sp_core::Get<RuntimeVersion>` is not satisfied
     --> /home/donal/remote-builds/polkadot-sdk/cumulus/parachains/runtimes/coretime/coretime-rococo/src/lib.rs:182:1
      |
  182 | #[derive_impl(default_configs::SystemParachainDefaultConfig as frame_system::DefaultConfig)]
      | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `sp_core::Get<RuntimeVersion>` is not implemented for `MustBeOverridden`
      |
      = help: the following other types implement trait `sp_core::Get<T>`:
                <MinimumPeriodTimesTwo<T> as sp_core::Get<<T as pallet_timestamp::Config>::Moment>>
                <EthereumNetwork as sp_core::Get<_I>>
                <rococo_runtime_constants::weights::BlockExecutionWeight as sp_core::Get<I>>
                <rococo_runtime_constants::weights::ExtrinsicBaseWeight as sp_core::Get<I>>
                <rococo_runtime_constants::weights::ParityDbWeight as sp_core::Get<_I>>
                <rococo_runtime_constants::weights::RocksDbWeight as sp_core::Get<_I>>
                <EpochDurationInBlocks as sp_core::Get<_I>>
                <RuntimeBlockLength as sp_core::Get<_I>>
              and 96 others
  note: required by a bound in `frame_system::Config::Version`
     --> /home/donal/remote-builds/polkadot-sdk/substrate/frame/system/src/lib.rs:558:17
      |
  558 |         type Version: Get<RuntimeVersion>;
      |                       ^^^^^^^^^^^^^^^^^^^ required by this bound in `Config::Version`
      = note: this error originates in the attribute macro `derive_impl` which comes from the expansion of the macro `frame_support::macro_magic::forward_tokens_verbatim` (in Nightly builds, run with -Z macro-backtrace for more info)

  For more information about this error, try `rustc --explain E0277`.
  error: could not compile `coretime-rococo-runtime` (lib) due to 1 previous error

This still means that the TestDefaultConfig for each pallet CAN provide defaults for these.