Allow to specify pallet prefix for some storages.

[gui1117]

related #8044 (comment)

currently the pallet prefix used by storages (storage store their values after the prefix: concat(twox128(pallet_prefix), twox128(storage_prefix)) where storage_prefix is the name given to the storage type in the macro, and pallet_prefix the name given to the pallet in construct_runtime).

Some cases seems to require to give a specific pallet_prefix as in #8044 (comment)
For example the pallet Asset could want its storage Balance to be stored at

concat!(twox128("SomeSpecificName"), twox182("Balance"))

instead of

concat!(twox128($NameOfPalletAssetInConstructRuntime), twox128("Balance"))

If we want to override the pallet_prefix, it is not very difficult, we need to do a special case when expanding the prefix in frame/support/procedural/src/pallet/expand/storage.rs

If we want to override only one storage. it requires changing the metadata because currently the storage metadata of a pallet specify only one prefix for all of its storage: https://substrate.dev/rustdocs/v3.0.0/frame_metadata/struct.StorageMetadata.html

Note that I'm not convinced the usecase is legit.

[bkchr]

where storage_prefix is the name of the storage

By that you mean the name of the storage item?

[gui1117]

where storage_prefix is the name of the storage

By that you mean the name of the storage item?

I rephrased, but what I meant is that storage_prefix is the name given to the type alias in the macro. (e.g. "Foo" for storage type Foo<T> = StorageValue<_, u32>.

[bkchr]

And you want to support to replace Foo with something else?

(Sorry for dumb questions 😅)

[gui1117]

No the request was to be able to specify the pallet_prefix for some or all storages.

For example the pallet Asset want its storage Balance to be stored at concat!(twox128("SomeSpecificName"), twox182("Balance")) instead of concat!(twox128($NameOfPalletAsset), twox128("Balance")).

The argument is in order not to break third party tools.

Maybe there is also usecase for share storages between pallets but I think this is another issue.

note that I'm not personally convinced the usecase is legit

[bkchr]

Okay :)

Yeah I think the whole time we wanted to have the flexibility that the storage prefix is set by the runtime developer and we now reached it. I'm also not convinced of bringing back support for a fixed storage prefix.

[kianenigma]

As I said in our Frame steering call: I think a pallet itself should not dictate where it is stored. This is quite counter intuitive: As the author of the top level runtime amalgamation file, you expect to have control over these details.

In other words, I am against adding a hack that brings us back to the old scheme where the prefix was defined inside of the pallet, wihtin delc_storage!.

We can and should introduce the functionality in construct_runtime! instead. Instead of assuming the pallet alias name (e.g. System in System: frame_system::{Pallet, Origin, Config, ...}) is the prefix, we should make it explicit:

System: frame_system::{Pallet, Origin, Config, Prefix = "System"}

And the default would be the pallet name.

Then, for example in shawn's case we can do:

System: frame_system::{Pallet, Origin, Config, Prefix = "System"}
Accounts: frame_accounts{Pallet, ..., Prefix = "System"} 

This will be enough to override the prefix of all storage items in one pallet in an ergonomic way. But for a single storage item, I don't think we can, and I don't see a use case for it either.

I wonder, if we add this, can't we have technically also have instanciable pallets? what would break if I do:

System: frame_system::{Pallet, Origin, Config, Prefix = "System"}
System2: frame_system::{Pallet, ..., Prefix = "System2"} 

[bkchr]

Then, for example in shawn's case we can do:

System: frame_system::{Pallet, Origin, Config, Prefix = "System"}
Accounts: frame_accounts{Pallet, ..., Prefix = "System"} 

This will be enough to override the prefix of all storage items in one pallet in an ergonomic way. But for a single storage item, I don't think we can, and I don't see a use case for it either.

While this could be done, why is it that important to set the prefix? The point of the prefix is to make pallets not collide in storage. If we open the door to change the prefix, it means that the runtime author needs to check if there are two storage items with the same name in both pallets. So, this just brings more problems IMHO than it could solve.

I wonder, if we add this, can't we have technically also have instanciable pallets? what would break if I do:

System: frame_system::{Pallet, Origin, Config, Prefix = "System"}
System2: frame_system::{Pallet, ..., Prefix = "System2"} 

Stuff like PalletInfo expects that there is only one "Rust instance" per pallet/pallet instance.

[kianenigma]

While this could be done, why is it that important to set the prefix? The point of the prefix is to make pallets not collide in storage. If we open the door to change the prefix, it means that the runtime author needs to check if there are two storage items with the same name in both pallets. So, this just brings more problems IMHO than it could solve.

But how else would you allow the example of system and account pallets both having their prefix in System? It either need to hardcoded in the account pallet that it should use the system prefix, or what I proposed.

Stuff like PalletInfo expects that there is only one "Rust instance" per pallet/pallet instance.

👍

[bkchr]

But how else would you allow the example of system and account pallets both having their prefix in System? It either need to hardcoded in the account pallet that it should use the system prefix, or what I proposed.

I'm not quite sure why both need to use the same prefix?^^ I did not follow @shawntabrizi pr yet that closely.

[kianenigma]

Regardless of Shawn's PR, looking at the description of this issue, you have the same situation:

concat!(twox128("SomeSpecificName"), twox182("Balance"))

instead of

concat!(twox128($NameOfPalletAssetInConstructRuntime), twox128("Balance"))

And then my question to you is: Where would you define SomeSpecificName? I think doing it inside the pallet is horrible. Allowing it to be overriden in construct_runtime! is much less horrible.

NOTE: although I am ofc being opinionated here. I can see that maybe we can do something like:

type StorageThing<T> = StorageValue<_, u32, _, $HardcodedPrefix>

or something like this. But this looks like a black box ot the runtime author and they have no control over this prefix anymore, which I think is bad.

[bkchr]

Honestly, I don't even understand why you want to change the storage location of some single item? :D

[kianenigma]

Yeah I am not a fan of that either :D but you can answer the first part of my previous comment

[bkchr]

And then my question to you is: Where would you define SomeSpecificName?

You mean this?

Yeah, I'm for defining this in the runtime itself. However, we already do this with the name of the pallet xD

[shawntabrizi]

I think the point should be:

by default, we do all the sensible things like use the pallet name as the prefix of a storage item...

but it should be that this is extensible, so that with some additional code we can support custom prefix.

For example, in my migration of accounts from System Pallet to Accounts Pallet, i might want to avoid a migration.

I should be able to do to:

	/// The full account information for a particular account ID.
	#[pallet::storage]
	#[pallet::storage::prefix("System")]  // <--- this
	#[pallet::getter(fn account)]
	pub type Account<T: Config> = StorageMap<
		_,
		Blake2_128Concat,
		T::AccountId,
		AccountInfo<T::Index, T::AccountData>,
		ValueQuery,
	>;

I at least think this would be cool and useful in multiple ways.

[bkchr]

Why avoid a migration? This just sounds like we want to go some lazy way here 🙈

[kianenigma]

@shawntabrizi did you see my comment here: #8391 (comment)?

I am strongly in favour of doing this override in construct_runtime!, not inside the pallet.

[gui1117]

@shawntabrizi did you see my comment here: #8391 (comment)?

I am strongly in favour of doing this override in construct_runtime!, not inside the pallet.

I think Shawn would prefer to override for a single storage.

Something we can do is:

	pub trait Config {
		...
		type PalletPrefixForAccount: Get<'static str>;
	}

	#[pallet::storage]
	#[pallet::storage::prefix(T::PalletPrefixForAccount)]
	pub type Account<T: Config> = StorageMap<
		_,
		...
	>;

Thus the prefix is configured when implementing the runtime.

[kianenigma]

That seems like a good place to meet in the middle.

[bkchr]

For example, in my migration of accounts from System Pallet to Accounts Pallet, i might want to avoid a migration.

I'm still not convinced by this. This is again some "Polkadot centric" migration affecting Substrate. Everyone wanting to use this pallet (so probably everyone) will need to specify this prefix, which is essentially not a user experience.

[shawntabrizi]

yeah we can probably avoid this for now, but i think there may still exist reasons to have such features.

[bkchr]

Okay :)

Yeah in general the feature could probably be useful for someone .

[stale]

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.