Implement iter for doublemap #5504
Conversation
|
@thiolliere Not building... |
gui1117
added
A3-in_progress
gui1117
added
A0-please_review
|
Fixed |
bkchr
left a comment
bkchr
left a comment
There was a problem hiding this comment.
Some nitpicks, otherwise looks good
|
👍 nitpicks addressed |
|
|
||
| fn key_before_prefix(mut prefix: Vec<u8>) -> Vec<u8> { | ||
| let last = prefix.iter_mut().last().unwrap(); | ||
| assert!(*last != 0, "mock function not implemented for this prefix"); |
There was a problem hiding this comment.
You are aware that rust panics automatically with debug_assertions enabled? (we have this in CI)
There was a problem hiding this comment.
And if you wanted to keep it more explicit, *last = last.checked_sub(1).unwrap() would be the way to go.
But using unwrap_or_else(|| unimplemented!()) is really weird.
There was a problem hiding this comment.
You are aware that rust panics automatically with
debug_assertionsenabled? (we have this in CI)
I though CI was running in release mode, and so operation was just silently wrapping around.
But I guess best was: *last = last.checked_sub(1).expect("not implementation for mock") indeed
There was a problem hiding this comment.
Yes in release, but with debug-assertions enabled: https://github.com/paritytech/substrate/blob/master/.gitlab-ci.yml#L191
| _phantom: Default::default(), | ||
| drain: false, | ||
| closure: |raw_key_without_prefix, mut raw_value| { | ||
| let mut k1_k2_material = G::Hasher1::reverse(raw_key_without_prefix); |
There was a problem hiding this comment.
actually this make the assumption that reversing will return the part for k1 and the remainings input. but nothing prevent a hasher to implement ReversibleStorageHasher in a different way. I must update the doc of reversible hasher or introduce new function
There was a problem hiding this comment.
I assume you will push it to this pr?
There was a problem hiding this comment.
Yes added to the PR, or there is the branch gui-reversible-doc-and-constraint if wanted
Context
StorageDoubleMap is missing a function iter to be able to iterate on all values.
Done:
IterableDoubleMap:
fn iterandfn drainare renamed iter_prefix and drain_prefix (I used iter_prefix because this is how function working with k1 are called in StorageDoubleMap)fn iterandfn drainare introduced, they iterate over the whole storage and decode K1 and K2 and ValueStorageDoubleMap:
fn iter_prefixis renamediter_prefix_valuesas it doesn't decode the keys, this name is consistent withStoragePrefixedMap::iter_values