Change construct_runtime! macro to use pallet names instead of pallet paths for enum variant identifiers

[KiChjang]

Currently, during macro expansion of construct_runtime!, the enums that we generate uses the pallet path rather than the pallet name as a component for the enum variants:

construct_runtime!(
	pub enum Runtime where
		Block = Block,
		NodeBlock = Block,
		UncheckedExtrinsic = UncheckedExtrinsic
	{
		Foo: bar::{Pallet, Call, Storage, Event, Origin},
	}
);

// This would generate the following Event enum for example:

pub enum Event {
	bar(bar::Event)
}

Ideally, we would use the pallet name as specified in the construct_runtime! macro so that we generate the following instead:

pub enum Event {
	Foo(bar::Event)
}

In other words, audit calls to PalletPath::mod_name, and see if they can be replaced by the pallet name. mod_name should ideally not exist.

Files: frame/support/procedural/src/construct_runtime/mod.rs , frame/support/procedural/src/construct_runtime/expand/event.rs, frame/support/procedural/src/construct_runtime/expand/event.rs, frame/support/procedural/src/construct_runtime/expand/origin.rs

[shawntabrizi]

What does this solve?

What will this break?

[gui1117]

(edited: I wrongly answered for another issue)

What does this solve?

this would make names of field and variants more consistent and better IMO, path can lead to weird names, not unique names, and currently we concatenate the instance in order to be unique which is not very good.

What will this break?

the names fields in genesis config, the name of variant in event, the name of variant in origin
EDIT: and maybe metadata, I have to see the PR

[KiChjang]

Currently, we use the module path as the enum variant names for a couple of the enums that we generate during macro expansion of construct_runtime, namely OriginCaller, Event and a few others.

This is undesirable fort the reasons Gui mentioned, and to further expand on why this naming scheme is not unique: suppose I have two pallets that are both named foo but located in different paths, one is a separate crate called foo and one is a module in the current crate (so crate::foo). We currently allow them both to be imported via their unique paths foo and crate::foo like so:

construct_runtime! {
    pub enum Runtime where
        // where clause goes here
    {
        Pallet1: foo::{Pallet, Event},
        Pallet2: crate::foo::{Pallet, Event},
    }
}

We then use their respective paths to construct enum variant names, but with a caveat: we drop the self, super and crate semi-keywords when creating the variant names, so as a result these two modules would in fact have the same variant name foo and would clash.

We could solve this easily by including crate, self and super as part of the variant name, but it doesn't quite make much sense: you'd see a variant name Event::crate_foo and Event::foo, and at first glance it's not quite clear what the difference is between the two. Moreover, we have a real use case where the same pallet is declared twice during runtime construction, and it's the Collective pallet. Using the current naming scheme, we'd end up with something like Event::pallet_collective_Instance1 and Event::pallet_collective_Instance2, which isn't really very descriptive about what they really are.

If the objective here is to use a relatively unique name to name the variants, it would make much more sense to just use an identifier that is already unique -- the pallet name. Not only is it unique, it's also pretty self-documenting: using pallet-collective as an example, I don't need to wonder what Instance1 or Instance2 is about, because it'll just use the pallet name as defined in construct_runtime!, so instead you'll see Event::Council and Event::TechnicalCommittee.

In terms of breakage, when FRAME devs upgrade their runtime, if their code makes references to the Event, OriginCaller and GenesisConfig as generated by construct_runtime!, then they'll have to ensure that any enum variant (field name in the case of GenesisConfig) they reference uses the new naming scheme instead of the old one. It should be trivial to fix, and I don't expect any major issues arising from changing the naming schemes for these enum variants/fields.