Distinct handling for N fields + 1 hasher vs N fields + N hashers #458
Conversation
codegen/src/api/storage.rs
Outdated
|
|
||
| let key_impl = if hashers.len() == fields.len() { | ||
| // If the number of hashers matches the number of fields, we're dealing with a | ||
| // StorageNMap, and each field should be hashed separately according to the |
There was a problem hiding this comment.
Comment not strictly accurate, as it would be possible (although useless) to have a single tuple key e.g. (KeyType,) for a regular StorageMap
There was a problem hiding this comment.
I tweaked the comment to "something shaped like a StorageNMap"; does that make more sense?
I guess in the 1-key version it doesn't really matter what branch we follow as both have the same effect anyway.
| Ok(()) | ||
| } | ||
|
|
||
| // This fails until the fix in https://github.com/paritytech/subxt/pull/458 is introduced. |
There was a problem hiding this comment.
Can remove this self referential comment now?
There was a problem hiding this comment.
I wanted it there more as a record for why the slightly random looking test exists for future onlookers :)
| // something shaped like a StorageNMap, and each field should be hashed separately | ||
| // according to the corresponding hasher. | ||
| let keys = (0..tuple.fields().len()) | ||
| .into_iter() |
There was a problem hiding this comment.
Don't think you need this, range should be an iterator already
There was a problem hiding this comment.
I always forget this (because they probably shouldn't be :D)
codegen/src/api/storage.rs
Outdated
| // If the number of hashers matches the number of fields, we're dealing with | ||
| // something shaped like a StorageNMap, and each field should be hashed separately | ||
| // according to the corresponding hasher. | ||
| let keys = (0..tuple.fields().len()) |
There was a problem hiding this comment.
I'd have probably gone with hashers.into_iter().enumerate().take(fields.len()), but that's just how my brain works with iterators, this is fine.
I'd change tuple.fields().len() to fields.len() though.
There was a problem hiding this comment.
Good point; I think I'd have gone for the same sort of thing if I was approaching it fresh :)
There was a problem hiding this comment.
Yes that's much more intuitive, I am responsible for this hacky approach 🙈
codegen/src/api/storage.rs
Outdated
| // StorageMap. | ||
| let hasher = hashers.get(0).expect("checked for 1 hasher"); | ||
| let items = | ||
| (0..tuple.fields().len()).into_iter().map(|field_idx| { |
There was a problem hiding this comment.
Ditto re fields.len() and into_iterator().
| Ok(()) | ||
| } | ||
|
|
||
| // This fails until the fix in https://github.com/paritytech/subxt/pull/458 is introduced. |
There was a problem hiding this comment.
| // This fails until the fix in https://github.com/paritytech/subxt/pull/458 is introduced. | |
| // See https://github.com/paritytech/subxt/pull/458 for the motivation. |
…maybe?
There was a problem hiding this comment.
I'm easy either way :)
subxt/tests/integration/storage.rs
Outdated
| let ctx = test_context().await; | ||
|
|
||
| // Boilerplate; we create a new asset class with ID 99, and then | ||
| // we "approveTransfer" of some of this asset class. This Gives us an |
There was a problem hiding this comment.
| // we "approveTransfer" of some of this asset class. This Gives us an | |
| // we "approveTransfer" of some of this asset class. This gives us an |
Tests still needed before this merges (https://github.com/paritytech/substrate/blob/f318350b6410ef47e438aae24c0b4779373d0a7b/frame/assets/src/lib.rs#L274 is example of StorageNMap item).I added a few general storage tests, and a specific one that is fixed by this PR and broken before it.
Closes #446