construct_runtime should be order independent

[gilescope]

Description of bug

construct_runtime!(
pub enum Runtime where ...
{
    System: frame_system::{Pallet, Call, Config, Storage, Event<T>} = 0,
    Timestamp: pallet_timestamp::{Pallet, Call, Storage, Inherent} = 2,
    ParachainInfo: parachain_info::{Pallet, Storage, Config} = 1,
}

should generate identical code to:

construct_runtime!(
pub enum Runtime where ...
{
    System: frame_system::{Pallet, Call, Config, Storage, Event<T>} = 0,
    ParachainInfo: parachain_info::{Pallet, Storage, Config} = 1,
    Timestamp: pallet_timestamp::{Pallet, Call, Storage, Inherent} = 2,
}

I know there's special carve outs for the ordering of System in the generated code, but the above two inputs to the construct runtime should produce identical outputs.

(Found out the hard way that they are not currently producing the same code: polkadot-js/apps#9077 )

The current differences seem to be AllPalletsWithinSystem ordering, enum Call ordering, get_module_names and the results of the metadata call order.

[gilescope]

Either that or we have it as a hard fail if the pallets are not defined in numerical order? - yes that might be nicer given that non-explicit indices are assigned based on the last index that has been come across.

[gilescope]

People seem to like that they don't have to specify the pallets in strict numerical order so we should sort the pallets by index before we generate the runtime.

[xlc]

We really need a policy requires all the breaking change proposal to be sufficiently communicated & discussed first to minimize potential impacts to downstream projects. cc @bkchr @gavofyork

The pallet order determines the hook order. So this is a hard to notice breaking change that will break invariants some chain may have.

https://github.com/AcalaNetwork/Acala/blob/1cf443e85bbad490e5b0a3d4fe266aae027acb99/runtime/acala/src/lib.rs#L1746-L1747

[bkchr]

We really need a policy requires all the breaking change proposal to be sufficiently communicated & discussed first to minimize potential impacts to downstream projects. cc @bkchr @gavofyork

Yes, I know that these are breaking changes and I'm also against them. There is no real justifications to do this. Polkadot-js could also just order them by index, but in general I don't see any reason why that is even important.

The general idea to have more control over the order etc makes sense nevertheless and there are already some ideas around some new construct_runtime! syntax where ideas like this could be included: paritytech/polkadot-sdk#232

Please post a gist of this issue in the linked issue or we create a totally new issue construct_runtime v2 or something like this.

[gilescope]

Jan pointed out that this could be implemented by making it a compile error if an attribute was not added to contstruct_runtime! that forced runtimes to choose from #[legacy_mixed_ordering] or #[strict_pallet_index_ordering].

[gilescope]

Just had another dev hit this. It's not fixed.

[bkchr]

Please post a gist of this issue in the linked issue or we create a totally new issue construct_runtime v2 or something like this.

Can you please just do what I said?