Automatic pallet parts in construct_runtime

[gui1117]

Fix #8084

can also superseed #8743 (considering that the exclude_parts and use_parts` doesn't require giving the generics).

for pallet declared with the pallet attribute macro, the pallet parts can now be automatically derived.
Also it is possible to exclude some parts manually.

the syntax can be discussed, it is currently with 2 new keywords exclude_parts and use_parts

        Test4_Instance1: test4::<Instance1> exclude_parts { Call, Origin } = 3,
        Test4: test4 exclude_parts { Storage },

        Test4_Instance1: test4::<Instance1> use_parts { Call, Origin } = 3,
        Test4: test4 use_parts { Storage },

        Test4_Instance1: test4::<Instance1>  = 3,
        Test4: test4,

If any part specified in use_parts or exclude_parts are not a part generated by the pallet (like if user write exclude_parts { Call } and the pallet has no Call. Then an error message is generated, telling that only available parts can be excluded or used.

(I'll do remove the parts in pallets mock in another PR)

How does it work:

pallet macro generates a macro tt_default_parts.
construct_runtime will call the macro tt_default_parts via tt_call in order to retrieve the pallet parts.

More precisely the macro call:

construct_runtime!(
	pub enum Runtime where
		Block = Block,
		NodeBlock = node_primitives::Block,
		UncheckedExtrinsic = UncheckedExtrinsic
	{
		System: frame_system,
	}
);

will generate a macro call:

frame_support::tt_call! {
	macro = [{ frame_system::tt_default_parts }] // this fetches the pallet parts as parser tokens...
	~~> frame_support::match_and_insert! { // ...and feeds the tokens as an input to the `match_and_insert!` macro as an argument
	// the token to expand inside
	target = [{
		frame_support::construct_runtime!(
			pub enum Runtime where
				Block = Block,
				NodeBlock = node_primitives::Block,
				UncheckedExtrinsic = UncheckedExtrinsic
			{
				System: frame_system,
			}
		);
	}]
	// some token to know where to expand the parts. name is unique so this pattern uniquely identifies where to expand the parts.
	pattern = [{ System: frame_system }]
	// the `tokens = [{ ::{Pallet, Call, ...} }]` argument is returned by tt_default_parts and is piped into this macro by tt_call
}

This will expand to:

frame_support::construct_runtime!(
	pub enum Runtime where
		Block = Block,
		NodeBlock = node_primitives::Block,
		UncheckedExtrinsic = UncheckedExtrinsic
	{
		System: frame_system::{Pallet, Call, ...},
	}
);

And this contains all the parts information and so will finish the expansion.

TODO

• update doc

[KiChjang]

In terms of syntax, I was thinking more of a rustic convention by using *, so you'd have System: frame_system::*, and perhaps excludes could be done with the !, so you can have System: frame_system::{*, !Pallet, !Call}. This is still a foreign syntax to most Rustaceans, but hopefully it's close enough so that they can decipher the meaning just by looking at it.

[KiChjang]

Holy crud, there's definitely a macro-ception going on in this PR, I think I need some strong caffeine for this. One thing that's immediately apparent from this complexity is the maintainability of it. Could just be an issue with documentation, but I think what's really missing now is some sort of flowchart that explains how everything gets strung together in the end.

In addition, compilation times of pallets will definitely increase as a result, and while I think we're not hitting any limits yet (recursion or otherwise), we may need to keep an eye on it sooner or later.

[gilescope]

❤️ This looks a lot cleaner.

[gilescope]

I'm not sure about excluded. Better to be explicit with your includes if you want to exclude something - it's a bit more rusty.

[KiChjang]

The <T> generic here still makes me a bit sad, but that's really what #8743 aims to solve.

[gui1117]

yes but I expect nobody to use this syntax anymore once all pallet use pallet macro.
maybe we can explicit whitelist syntax like use_parts as well.

[gui1117]

about syntax I'm usually a bit skeptical of using rusty syntax for stuff which are not rust syntax, it sometimes lead to more confusion about the true possible syntax than it helps.
So I think exclude_parts { Call } is a bit more simple than a definition of parts like ::{*, !Call}.

I'm not sure about excluded. Better to be explicit with your includes if you want to exclude something - it's a bit more rusty.

In this example I like to exclude because:

• the most usual scenario is to not exclude anything.
• I expect people to usually exclude very few parts, and probably only Call, ValidateUnsigned and Inherent. So syntax is shorter.
• Because the usual behavior is to include everything, having the emphasize on what is excluded makes sense to me.

That said if we introduce new parts in the future all those points might not stand anymore. So maybe we can do use_parts { Call, Storage, ... } instead, and that will only use the specified parts.

One thing that's immediately apparent from this complexity is the maintainability of it.

Yes agree, I'll improve documentation, with good dev documentation I think this complexity is not so difficult to maintain.

[gui1117]

at some point we can have both exclude_parts and use_parts keywords, and people can choose whether they prefer to whitelist or blacklist.

[gui1117]

looks good to me

[shawntabrizi]

Suggested change

	/// Test4_Instance1: test4::<Instance1> exclude_parts { Call, Origin },
	/// Test4_Instance1: test4::<Instance1>::{ !Call, !Origin },

Would such a syntax be possible?

[gui1117]

the thing is I want to be backward compatible.

In case somebody writes: Test: test::{Config}, does that mean the pallet has a config part with no generic and user want to set it. or does it mean that the pallet has a config part which is maybe generic or maybe not and user want to set it.
In the first case we don't need to retrieve the parts from the pallet, which is needed for pallets with decl_module.
In the second case we need to retrieve the parts from the pallet, which is a breaking change for pallets with decl_module which don't support it.

So the way the PR implements it is:

• either part are manually declared. So no need to retrieve the part from the pallet
• or the part are not declared and just whitelisted/blacklisted. So we will call into the pallet to get the parts.

Maybe we can improve the syntax but we can't mix them together.

[gui1117]

a way to implemented could to have something like:
Test: test $some_keyword_or_any_token_to_indicate_that_parts_should_be_retrieved_from_pallet {Pallet, !Call}

But also I don't know I implemented this PR so that when you blacklist/whitelist a part, it must be an existing part of the pallet.
Like you can't do use_parts { Call } if the pallet has no call.

[shawntabrizi]

Suggested change

	Pallet: pallet exclude_parts { Pallet } use_parts { Pallet },
	Pallet: pallet::{ !Pallet, Pallet },

[gui1117]

bot merge

[paritytech-processbot]

Waiting for commit status.

[nuke-web3]

This is indeed cleaner in the construct_runtime macro, but it leads understandably to some confusion:

Other than just commenting and making note on the devhub, what can we do to help make this more idiomatic an less "magic" assignment?

[shawntabrizi]

@nukemandan those configuration types and the things in the construct_runtime! are not the same.

Each pallet has a set of objects generated for each of them depending on which macros you use and what you generate.

For example:

• enum Event: https://github.com/paritytech/substrate/blob/master/frame/sudo/src/lib.rs#L252
• enum Call: https://github.com/paritytech/substrate/blob/master/frame/sudo/src/lib.rs#L128
• type Origin: https://github.com/paritytech/substrate/blob/master/frame/collective/src/lib.rs#L241 & https://github.com/paritytech/substrate/blob/master/frame/collective/src/lib.rs#L130

Whereas those are configuration types needed for frame_system, and are all described here:

https://github.com/paritytech/substrate/blob/master/frame/system/src/lib.rs#L162

Obviously they are related, but nothing about what the construct_runtime! does and hiding this will make any of that stuff more clear I think...

[gui1117]

Yes this PR is only about construct_runtime declaration, in the past it was mandatory to declare each "parts" to use for a pallet. For example: Config part will put the pallet genesis config into the aggregated runtime genesis config, etc..
Now user can skip this explicit declaration and use the default: which is: all parts available for the pallet (i.e. if the pallet has some genesis config then automatically use the Config part).
Also user can choose to use a subset of the available parts by using: use_parts orexcept_parts syntax

[kianenigma]

fwiw I do agree that these types being magically generated is a bit confusing, and I strongly think they should be prefixed with 'Outer', e.g. OuterCall.

[KiChjang]

Disagree on the "Outer" prefix, because you need to understand what "inner" is, and it's not self-evident what they are.

Stepping back a bit, I think this is a problem that existed even before the implicit part declarations. Before this PR, it was not clear what the Event, Call and Origin types were referring to either, nor where they are defined. So to expose these types more explicitly is in fact a new feature request, and it's not quite clear to me yet how exactly we can do so, or whether it's worth putting the effort in exposing them.

[bkchr]

The key point here are missing docs. We just need more and better docs. I just searched the devhub and couldn't find an article about construct_runtime!. This should be added.

[bkchr]

polkadot-developers/substrate-docs#502

[kianenigma]

Disagree on the "Outer" prefix, because you need to understand what "inner" is, and it's not self-evident what they are.

how is naming both the outer call and inner call Call helping with understanding? This historical bad choice of name (IMO) is the very reason why it is not self-evident, and we should start fixing it from somewhere. The fact that the inner calls are still confusing is not a justification to not make the outer ones a bit more understandable.

For example, Type Call in pallet system is documented to be the overarching call inside the pallet. While when you look at the top level runtime type Call = Call; is not a good demonstration of this. Type Call = OverArchingCall or type Call = OuterCall is better in my opinion.

[KiChjang]

how is naming both the outer call and inner call Call helping with understanding?

I think construct_runtime should only expose the outer Call enum as an abstraction. The inner calls are implementation details that need not be exposed, because it seems to me that the only time where one would access those inner calls would be in BaseCallFilter. For all other intents and purposes, there's not much need to further surface the inner workings between an outer and an inner call to the user.