Implement a CountedStorageMap

[gui1117]

This PR introduces a new Storage abstraction: CountedStorageMap

The CountedStorageMap is a new type with a similar API as StorageMap which internally use a StorageMap and StorageValue<u32> and keeps count of the number of items in the map by using the value as a counter.

[gui1117]

if we want to support limit, then we need sp-io to return the number of deleted key in the overlay, once merged I will open an issue, and we can work on it when we need it

[emostov]

What is limit?

[gui1117]

for other storage the remove_all method allow to remove only up to a certain number of key in the backend: https://substrate.dev/rustdocs/latest/frame_support/storage/types/struct.StorageMap.html#method.remove_all

Here it is not possible without changing sp-io. Because we need to count the value removed in the overlay. And https://substrate.dev/rustdocs/latest/sp_io/storage/fn.clear_prefix.html doesn't give us this information

[gui1117]

would StorageCountedMap be a better name than CountedStorageMap, this makes all storages type start with Storage ?

[shawntabrizi]

would StorageCountedMap be a better name than CountedStorageMap, this makes all storages type start with Storage ?

I think CountedStorageMap is more natural, but I dont really care one way or another.

[shawntabrizi]

@jacogr i think this is a change to the metadata

[shawntabrizi]

AFAICT this looks good to me.

However, I wonder how scalable this overall approach is.

I can imagine we will continue to want to add new and different kinds of storage types to make runtime development easier and more ergonomic.

Do you see that writing all this code every time is sustainable?

[shawntabrizi]

Note also that we will already need to extend this API for all of our needs.

See here in Assets we have a StorageNMap:

https://github.com/paritytech/substrate/blob/master/frame/assets/src/lib.rs#L245

	#[pallet::storage]
	/// Approved balance transfers. First balance is the amount approved for transfer. Second
	/// is the amount of `T::Currency` reserved for storing this.
	/// First key is the asset ID, second key is the owner and third key is the delegate.
	pub(super) type Approvals<T: Config<I>, I: 'static = ()> = StorageNMap<
		_,
		(
			NMapKey<Blake2_128Concat, T::AssetId>,
			NMapKey<Blake2_128Concat, T::AccountId>, // owner
			NMapKey<Blake2_128Concat, T::AccountId>, // delegate
		),
		Approval<T::Balance, DepositBalanceOf<T, I>>,
		OptionQuery,
		GetDefault,
		ConstU32<300_000>,
	>;

We are not only interested in the total number of values for this whole map, but also just the number of values for the map under a specific user.

So something like CountedStorageNMap would need N - 1 counters... Does that change the way you approach this problem?

[KiChjang]

I honestly think #8605 is a better approach here, but it looks like there is already a pressing need for counters? If so, the question then becomes maintaining a sensible API that's compatible with the storage mutation hooks. I agree that adding additional storage item types don't seem to be a scalable solution, and we may end up with a system that is overly complex.

I think all storage map types should simply support the basic CRUD operations, and if there is a need to add additional functionality to them, creating extension traits or functions sounds like the best way forward to me.

[jacogr]

You will need to bump the metadata version - this is smack-bang in the middle of the enum which means V13 parsers cannot parse this at all.

[gui1117]

This PR doesn't change the metadata... The countedstoragemap declare a storage map entry and a storage value entry in the metadata. Maybe we can enhance the metadata later in order to be able to declare the relationship between those 2 storages usage. About the hook on storage map i agree it would be better, it might be a bit more difficult to optimize. E.g. we don't want to read if a previous key exists before each insert when we know a previous exist or not like in a mutate operation. But it should be doable.

[kianenigma]

LGTM except a few suggestions, there should be a follow up that migrates the staking counters to this PR.

[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.

[gui1117]

I slightly refactored the metadata for storages, instead of having the trait StorageEntryMetadata which gives the name, the type, etc.. I did StorageEntryMetadataBuilder which directly build the metadata of the storage. This is good because in case of counted storage map it allows to generate both inner storage metadata (the map and the value holding the counter), without any additional code from the macro.
So the macro only needs to care about a few less stuff.

EDIT: if people want they can do a final review, but I think it is good to merge, and as behavior hasn't change, the current review are valid.

[gui1117]

bot merge

[ghost]

Trying merge.