Implement StorageNMap

[KiChjang]

Partial #8291.

Things left to be done:

• Support StorageNMap in #[pallet::storage] and decl_storage! macros
• Make use of it in pallets that wanted a StorageTripleMap
• (Optional, needs discussion) Deprecate StorageMap and StorageDoubleMap and make them use StorageNMap underneath

[shawntabrizi]

@KiChjang I see this is a draft. If there are outstanding things that need to be done, make a checklist of those things?

[KiChjang]

@shawntabrizi There are some really tough problems with regards to supporting iteration of prefixes in the StorageNMap. Specifically, I'm really not sure how to tell Rust to return the suffix key types, so I think I'd probably not support the iter_prefix and the drain_prefix methods.

Other than that, what's mostly left to do is trying to get it to actually work in pallets, so that people can start using it.

[shawntabrizi]

It feels to me without the ability to do iteration and the lingering (), this is probably no the correct approach.

@bkchr are there better ways to approach this?

[KiChjang]

@shawntabrizi Just to clarify, iteration itself is supported, the only problem is supporting the iteration of prefixes.

EDIT: That means there is a very crude way of supporting the iteration of prefixes using filter or filter_map and filtering out keys that matches a certain prefix.

I'm pausing development for now until there's a consensus on what to do next.

[bkchr]

If you use tuples as keys, you should be able to remove this extra ().

What is the exact problem with prefix iteration? That it is hard to declare a prefix of v for a key (v, x)?

[KiChjang]

@bkchr With the current design, this is how a iter_prefix method declaration can be written as follows:

fn iter_prefix<KG, KArg>(key: (KArg, KG::NextKey)) -> ??? // what should even go in here?

The problem I see here is two-fold:

1. How do I make sure that the KG parameter is a proper prefix of K in the StorageNMap?
2. How do I write the rest of the key suffixes out as the return type?

[gui1117]

for prefix iterator maybe we can create a new trait which enforce the correctness of the prefix. an example can be:

trait KeyPrefix<T>: KeyGenerator {}

// To be implemented with macro
impl<A: KeyGenerator, B: KeyGenerator, C: KeyGenerator> KeyPrefix<(A, B, C)> for (A, B) {}
impl<A: KeyGenerator, B: KeyGenerator, C: KeyGenerator, D: KeyGenerator> KeyPrefix<(A, B, C, D)> for (A, B) {}
impl<A: KeyGenerator, B: KeyGenerator, C: KeyGenerator, D: KeyGenerator, E: KeyGenerator> KeyPrefix<(A, B, C, D, E)> for (A, B) {}

impl<A: KeyGenerator, B: KeyGenerator, C: KeyGenerator, D: KeyGenerator> KeyPrefix<(A, B, C, D)> for (A, B, C) {}
impl<A: KeyGenerator, B: KeyGenerator, C: KeyGenerator, D: KeyGenerator, E: KeyGenerator> KeyPrefix<(A, B, C, D, E)> for (A, B, C) {}

[bkchr]

for prefix iterator maybe we can create a new trait which enforce the correctness of the prefix. an example can be:

trait KeyPrefix<T>: KeyGenerator {}

// To be implemented with macro
impl<A: KeyGenerator, B: KeyGenerator, C: KeyGenerator> KeyPrefix<(A, B, C)> for (A, B) {}
impl<A: KeyGenerator, B: KeyGenerator, C: KeyGenerator, D: KeyGenerator> KeyPrefix<(A, B, C, D)> for (A, B) {}
impl<A: KeyGenerator, B: KeyGenerator, C: KeyGenerator, D: KeyGenerator, E: KeyGenerator> KeyPrefix<(A, B, C, D, E)> for (A, B) {}

impl<A: KeyGenerator, B: KeyGenerator, C: KeyGenerator, D: KeyGenerator> KeyPrefix<(A, B, C, D)> for (A, B, C) {}
impl<A: KeyGenerator, B: KeyGenerator, C: KeyGenerator, D: KeyGenerator, E: KeyGenerator> KeyPrefix<(A, B, C, D, E)> for (A, B, C) {}

Yeah nice! I had the same idea, basically it should be done in a similar way as it is done with Borrow.

So,

fn iterate_prefix<P>(prefix: P) where Key: KeyPrefix<P>

[bkchr]

So, I would actually do it the other way around:

impl<A, B, C> KeyPrefix<(A, B)> for (A, B, C) {}

And we will probably require some methods on KeyPrefix (maybe we should use IsKeyPrefix as name), but you should see this when implementing.

[KiChjang]

So the suggestions above would only solve (1), but I suppose I can easily extend it to contain an associated type KeySuffix, which will solve (2) as such:

fn iter_prefix<P>(prefix: P) -> PrefixIterator<(<Key as KeyPrefix<P>>::KeySuffix, V)> where Key: KeyPrefix<P>

Now here comes the fun part: what do I do about final_hash? This function is used only in one place, and that's in migrate_keys where we need to migrate the keys from an old hasher to a new hasher. Using impl_for_tuple causes us to lose the ability to extract each individual key out to be paired with the corresponding new hasher, so I don't have a good idea about what to do there.

[bkchr]

migeate_keys can be more manual.

Meaning you take the old hashers as tuple as you take them for the struct itself. The user would need to make sure that they pass the same number of arguments of hashers as the struct itself.

[KiChjang]

migeate_keys can be more manual.

Meaning you take the old hashers as tuple as you take them for the struct itself. The user would need to make sure that they pass the same number of arguments of hashers as the struct itself.

I thought about this before, but StorageHasher functions are defined as "class functions" on the hasher structs, so I can't just hasher.hash() something, I need to have its type and do Hasher::hash().

I of course could add a hash(&self, x: &[u8]) method for these structs, inside of which would just contain Self::hash(x), but I'm not sure if that's a good idea.

[bkchr]

If migrate keys is a problem, you could obmit it for now and make the rest working for now.

[gui1117]

looks good to me, polkadot js seems to support it polkadot-js/api#3465
but I don't know how much we should advertise the metadata change.
But at some point as long as we don't use any storage n map in polkadot the metadata are entirely compatible, except the version has bump to v13.

note the metadata will also change in #8615, I don't know if worth doing both at the same time.

maybe @joepetrowski can say more ?

[joepetrowski]

Might be a breaking change to some of the paths in Sidecar and tx construction in txwrapper-core. Deferring to @TarikGul if those will need a major version change.

Will this or #8615 change transaction encoding? Then we would need to bump the version.

[kianenigma]

Will this or #8615 change transaction encoding? Then we would need to bump the version.

Will not affect tx-version.

[kianenigma]

I see some leftover discussions, but probably all can be done separately.

As sanity, I'd be interested in seeing a a test that demonstrates a double map and an NMap(2) which same key types and hashers, generate exactly identical keys, and are thus exactly identical.

And for all the rest looks broadly good to me! Welcome :)

[bkchr]

Some nitpicks.

I also did not review anything, but I also don't want to block anything here.

[gui1117]

it seems follow up pieces are:

• As sanity, I'd be interested in seeing a a test that demonstrates a double map and an NMap(2) which same key types and hashers, generate exactly identical keys, and are thus exactly identical.

• have a procedural macro for impl_key_prefix_for!((A, B, C), (A, B), C); ....

• update generate_storage_alias to support nmpa as well

• maybe making StorageMap and StorageDoubleMap using StorageNMap under the hood.

• maybe consider using storage n map everywhere instead of storage map and storage double map. or the other way around: Implement StorageNMap #8635 (review)

[gui1117]

bot merge

[ghost]

Trying merge.