[FRAME Core] tracking issue for full unleash of `DefaultConfig` for FRAME

kianenigma commented 1 year ago

https://github.com/paritytech/substrate/pull/13454 will bring in the basic functionality needed to start using this feature. See this example to learn more.

This will mark the first version of this work complete, but a lot has to be done in order for it to be fully complete.

Improve Error Handling, Bugs

[ ] Improve error handling. There seems to be an error in the parse/config.rs file that allows #[pallet::whatever] to be parsed and ignored in #[pallet::config]. This has to be fixed.
[x] Properly hide __export_tokens_tt_test_default_config and similar from docs.
[ ] Handle pallet instancing.

Full usage in Substrate Pallets

[ ] migrate the docs for derive_impl to use real code. As all of this functionality can work on normal traits, and as per paritytech/substrate#14115, we should strive to make the docs of derive_impl be real rust code. We might even want to remove ./frame/example/default-config in favor this.
Provide testing defaults for common pallets
- [x] System: already provided
- [x] Balances https://github.com/paritytech/substrate/pull/14453
- [x] Assets https://github.com/paritytech/polkadot-sdk/pull/3637
Then, we should start migrating existing pallets and their test setup to use the unified testing prelude of system, balances and so on. Here's the tracking issue: https://github.com/paritytech/polkadot-sdk/issues/3237

Full Usage in `node-template-runtime` and `parachain-template-runtime`

[x] We should provide runtime templates that utilize this, evangelising its usage. This requires us to carefully consider what is a reasonable default for a node template in order to use. We should provide possibly two new preludes:

Full usage in `westend` -> `kusama` -> `polkadot` runtimes.

[x] Once we have are more and more confident in both the core functionality, and our skills to define sensible defaults, we can slowly start migrating perhaps some of the configs define in Polkadot to use this feature.

For security reasons we should be super caution here. Adding #[derive_impl] will open the door for upstream changes manipulating the configurations used in the Polkadot runtime. Perhaps we should only ever do this once we have more tooling that could notify us of unexpected upstream changes to config_preludes.

I would personally find it reasonable to never use this feature in production runtimes for this reason.

V2

At the moment, the system that is introduced is has some limitations. Most notably:

[x] it cannot provide default for types that are reliant on types that are themselves reliant on frame_system::Config. For example, a type Admin: Get<Self::AccountId> cannot have a default. Is is mainly because trait DefaultConfig is not in itself trait DefaultConfig: frame_system::DefaultConfig. Moreover, sadly, because the definition of BalanceOf<T> (somewhat needlessly) also relies on frame_system::Config::AccountId, anything akin to Get<BalanceOf<Self>> can also not have a sensible default. https://github.com/paritytech/substrate/pull/14453

I believe this is father easy to fix, by standardizing frame_system::DefaultConfig and making trait DefaultConfig: frame_system::DefaultConfig.

[x] If the above is done, we are only left with types that cannot really have a default in any pallet. Such types are: PalletInfo, RuntimeEvent, RuntimeCall and such. Such types can also be mocked but they require more coordination with construct_runtime!. A new attribute can mark these types simply as #[derive_from_runtime] or similar, which would mean they are expected to be found in the existing scope with the same name. https://github.com/paritytech/substrate/pull/14682

Other approaches could be explored for the last step, as I am not quite sure what is the best way forward.

Improvements

[x] Remove as frame_system::DefaultConfig from the required syntax https://github.com/paritytech/polkadot-sdk/pull/3505

bkchr commented 1 year ago

it cannot provide default for types that are reliant on types that are themselves reliant on frame_system::Config. For example, a type Admin: Get<Self::AccountId> cannot have a default. Is is mainly because trait DefaultConfig is not in itself trait DefaultConfig: frame_system::DefaultConfig. Moreover, sadly, because the definition of BalanceOf<T> (somewhat needlessly) also relies on frame_system::Config::AccountId, anything akin to Get<BalanceOf<Self>> can also not have a sensible default.

Personally I would say that we do this before we do all the other things, otherwise you will start changing all this code twice. I already left a comment here on how it could work: https://github.com/paritytech/substrate/pull/13454#issuecomment-1495855632

In general to this feature, I had some idea recently which could improve the versioning around our crates. Currently when we are adding a new type to a Config trait it requires that we bump the major version of the crate. If we have the system outlined here working and it is being used everywhere. (I would even go that far and say it is fine to use it in production runtimes, at least having the attribute above each Config implementation) Then we could introduce some kind of "versioning" of these Config traits. Then every time you would add a new config type, you would bump this version in your pallet. We would then only require to bump the minor version. Downstream users would then at some point bump the pallet minor version. It would still compile, because the attribute above the impl Config would put there some safe/secure default value. But it would also generate a warning that tells the user that it put there some new type. It could even show some more context to the new type added. The user can accept the new safe default value by bumping the "trait version to use" or they just add the type on their own and bump the trait version in use. Not sure if this idea is something we should really do, but I wanted to leave it here :P