CountedNMap implementation

[Mr-Leshiy]

add CountedStorageNMap implementation which is similar as CountedStorageMap for StorageMap but for the StorageNMap https://github.com/paritytech/substrate/issues/10602

[Mr-Leshiy]

@bkchr , @shawntabrizi don't want to be tactless, but can we proceed with this PR, it will be helpful to solve this issue https://github.com/paritytech/polkadot-sdk/issues/316. Also after if this PR will be approved and merged I will prepare the same update for the StorageNMap.

[kianenigma]

This PR seems valuable, yet hat slipped through the crack. Partially due to lack of labeling and project assignment from our side. Hopefully will get some review soon. Thanks @Mr-Leshiy!

[KiChjang]

I'm honestly interested to know why can't we just have a CountedStorageNMap instead? The CountedStorageDoubleMap can just be a type alias to a CountedStorageNMap that way (and the same can also be said for CountedStorageMap).

[stale[bot]]

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.

[Mr-Leshiy]

@KiChjang , so sorry for the delay.

I'm honestly interested to know why can't we just have a CountedStorageNMap instead? The CountedStorageDoubleMap can just be a type alias to a CountedStorageNMap that way (and the same can also be said for CountedStorageMap).

For my understanding it is impossible to implement in such way, because all these types of storages have different type of interfaces and also have different type of implementation. So in this case it is impossible to unify them and hide under the same implementation and interface.

[KiChjang]

I'm sorry but what you said doesn't expose or explain what exactly the impossibility is. Why is it not as simple as this:

type CountedStorageDoubleMap<P, H1, K1, H2, K2, V, Q, O, M> = CountedStorageNMap<P, (NMapKey<H1, K1>, NMapKey<H2, K2>), V, Q, O, M>;

What exactly is different about the interfaces?

[Mr-Leshiy]

@KiChjang , as a reference we can consider the following function from the original double map and from the original nmap contains_key.

DoubleMap function signature contains_key:

contains_key<KArg1, KArg2>(k1: KArg1, k2: KArg2) -> bool

Nmap function signature contains_key:

contains_key<KArg: EncodeLikeTuple<Key::KArg> + TupleToEncodedIter>(key: KArg) -> bool

[KiChjang]

Okay, then we don't do a type alias, but call into the NMap methods underneath, i.e.

fn contains_key<KArg1, KArg2>(k1: KArg1, k2: KArg2) -> bool {
    NMap::contains_key(((k1, Hasher1::hasher()), (k2, Hasher2::hasher())))
}

The point here is to reduce the amount of code that we have to maintain by using NMap behind the scenes.

[Mr-Leshiy]

@KiChjang , But how it will helps if I will replace an underneath implementation from the native DoubleMap storage to the NMap ? And if so I am coming with the general question why we have to different implementations of DoubleMap and NMap ?

[KiChjang]

DoubleMap is a relic of the past when we didn't have NMap. Since we're going to support 2 keys in a counted map, it would then make a lot more sense now to not support legacy code and see if we can all use NMap underneath.

[Mr-Leshiy]

@KiChjang I gotcha, sorry that was so annoying with questions and doubts, but I was not totally understood what do you mean and why need to change it 😊 So, what do you think if I will change this implementation just for the NMap, so will have a CountedNMap ? And later will figure out how to extend it to be compatible with the DoubleMap interface ?

[KiChjang]

That sounds like a workable plan to me. It may also be easier if you attempt to convert the existing StorageDoubleMap to use StorageNMap underneath first, but having a CountedNMap implementation right now is also valuable as well.

[stale[bot]]

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.

[Mr-Leshiy]

In progress...

[Mr-Leshiy]

@KiChjang Hello, I returning back to you ! Hope you are well ))) I have updated implementation and added CountedStorageNMap implementation, also removed CountedStorageDoubleMap. If we will successfully proceed with this one and it will be needed I will add CountedStorageDoubleMap implementation. :)

[KiChjang]

Everything looks great so far!

Hmm... since we still have StorageDoubleMap nowadays, I think we do have to add a CountedStorageDoubleMap after all, since that is what people would expect, but we can then have it actually use a CountedStorageNMap underneath.

[Mr-Leshiy]

@KiChjang , totally agree with you. But I would prefer to make StorageDoubleMap in the separate PR. But if you think that ti should be done here, I can do it, but will need some time for it :)

[KiChjang]

@KiChjang , totally agree with you. But I would prefer to make StorageDoubleMap in the separate PR. But if you think that ti should be done here, I can do it, but will need some time for it :)

Totally fine as well. That also reminds me -- CountedStorageMap itself can use StorageNMap underneath too, no reason to have separate implementations of it.

[stale[bot]]

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.

[Mr-Leshiy]

Still in progress ...

[stale[bot]]

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.

[Mr-Leshiy]

@KiChjang I have updated and seems fixed with your suggestion. I have tried your solution but unfortunately it does not work. I have implemented it as follows.

impl<'a, T: EncodeLike<U> + EncodeLikeTuple<U>, U: Encode> EncodeLikeTuple<U>
	for codec::Ref<'a, T, U>
{
}
impl<'a, T: EncodeLike<U> + EncodeLikeTuple<U>, U: Encode> crate::storage::private::Sealed
	for codec::Ref<'a, T, U>
{
}

I have tried different approaches but seems it would be the best one. What do you think ?

[KiChjang]

Made a small relocation change, but otherwise everything LGTM.

[ruseinov]

Before this can be merged we'll likely also need to add storage_alias macro support for this map along with some tests, similar to how it's done here: https://github.com/paritytech/substrate/pull/13366 Either that or a new issue, though it might be a nice precedent for posterity to have it all in one PR.

[stale[bot]]

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.

[Mr-Leshiy]

Still in progress

[stale[bot]]

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.

[Mr-Leshiy]

Still in progress

[juangirini]

@Mr-Leshiy is there any progress on this?

[gui1117]

for convenience and boilerplate when declaring a storage in a pallet we usually declare like this:

	#[pallet::storage]
	pub(super) type Dummy2<T: Config> = CountedStorageNMap<_, T::Balance>;

And the macro will create the type to replace the first type parameter _.

So this need to be handle in the pallet macro to be usable in a pallet.