Default PalletInfo impl for () can cause storage collisions

[ascjones]

Discovered while working on #7936.

Usually construct_runtime! generates a PalletInfo impl which returns the unique pallet name to use as the storage prefix. However, manually configured test runtimes use type PalletInfo = () in their system::Config implementation. The PalletInfo impl for () returns "test" from name(), which can cause storage collisions where two pallets have a storage item with the same name.

In my case it was because both System and Balances have an Account storage item - the temporary fix was applied here: c147734.

Chatted with @thiolliere, and we have agreed that we need to remove the footgun of the PalletInfo impl for (). Potential solutions:

• Make construct_runtime! macro work for test ruuntimes (@thiolliere already has some code in this direction)
• Create impl_pallet_info! for test runtimes to generate code similar to construct_runtime
• Make the default impl for () return some guaranteed unique and consistent pallet prefix, maybe using something in std::any.
• Allow standalone pallets to somehow provide an implementation of PalletInfo that is only used if the System does not provide one, though this might be tricky because of instantiable pallets (@thiolliere to elaborate)

Update:

We are in the process of updating all test runtimes to use construct_runtime! directly. Here is a list, please put your name against any that you are working on to avoid duplicating work. It's just a raw list of all frame modules, so feel free to amend where necessary.

• assets Use construct_runtime for some more pallet #7974
• atomic-swap Use construct_runtime for some more pallet #7974
• aura Use construct_runtime for some more pallet #7974
• authority-discovery Use construct_runtime for some more pallet #7974
• authorship Use construct_runtime for some more pallet #7974
• babe Migrate some more pallets to construct_runtime #7975
• balances Migrate some more pallets to construct_runtime #7975
• benchmarking @thiolliere
• bounties @ascjones Migrate more pallet tests to construct_runtime  #8051
• collective
• contracts @ascjones Migrate more pallet tests to construct_runtime  #8051
• democracy @ascjones Migrate more pallet tests to construct_runtime  #8051
• elections
• elections-phragmen
• example @thiolliere
• example-offchain-worker @thiolliere
• example-parallel @thiolliere
• executive
• grandpa @thiolliere
• identity @thiolliere
• im-online @thiolliere
• indices Migrate some more pallets to construct_runtime #7975
• lottery @thiolliere
• membership @thiolliere
• merkle-mountain-range @thiolliere
• metadata @thiolliere
• multisig @thiolliere
• nicks @thiolliere
• node-authorization @thiolliere
• offences @thiolliere
• proxy @thiolliere
• randomness-collective-flip
• recovery
• scheduler
• scored-pool
• session
• society
• staking
• sudo
• support
• system @thiolliere
• timestamp
• tips
• transaction-payment
• treasury
• utility
• vesting
• test-utils

[gui1117]

I think ppl should already use construct_runtime in their tests: staking, executive and few other do that.
We can also introduce some helper type in system IMO as done in #7950

[xlc]

Maybe someone can fix paritytech/polkadot-sdk#367 so we don't need to remember all the details to generate a test runtime?

[gui1117]

maybe we can do a mix of them:
improving implementation of () for PalletInfo with std::any but at the same deprecate it with a clear message:
The implementation () for PalletInfo use trait Any and can be inconsistent between tests use construct_runtime or impl_pallet_info instead

[bkchr]

I think we should fix it properly directly.

As I have implemented it, we didn't use it, so there wasn't any problem with this common prefix 😁

[gui1117]

Does an implementation of PalletInfo for () using std::any::type_name or Any as Debug is a proper fix.
IMHO I'm not sure std::any::type_name is always consistent between testing environment.
And same for Any as Debug.

Thus the proper fix is deprecated or remove implementation of PalletInfo for (), while providing a macro to implement it easily and encourage ppl to make use of construct_runtime

[ascjones]

while providing a macro to implement it easily and encourage ppl to make use of construct_runtime

Should we only encourage use of construct_runtime in the deprecation message, rather than another macro to remember to add in addition to impl_outer_event and impl_outer_origin

[bkchr]

I like the idea of encouraging people to use construct_runtime in tests as well.

[shawntabrizi]

I think we should remove the impl of () for PalletInfo. If not, why?

[ascjones]

We should, once all PalletInfo = () occurrences are gone. There's still a few there if you do a search. I will remove more today.