Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Implement iter_keys function for all types of storage maps #9238

Merged
5 commits merged into from
Jul 1, 2021

Conversation

KiChjang
Copy link
Contributor

Fixes #9118.

@github-actions github-actions bot added the A0-please_review Pull request needs code review. label Jun 30, 2021
@KiChjang KiChjang added B3-apinoteworthy C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Jun 30, 2021
Copy link
Contributor

@coriolinus coriolinus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I'm not convinced that drain_keys and friends are really worth it; it seems like they're justified only if decoding the values takes non-trivial time. If you do choose to keep them, it would be good to test them, not just the iter_keys variants.

Still, the code overall looks good.

frame/support/src/storage/generator/double_map.rs Outdated Show resolved Hide resolved
frame/support/src/storage/generator/double_map.rs Outdated Show resolved Hide resolved
frame/support/src/storage/generator/double_map.rs Outdated Show resolved Hide resolved
frame/support/src/storage/mod.rs Outdated Show resolved Hide resolved
prefix: Vec<u8>,
previous_key: Vec<u8>,
/// If true then value are removed while iterating
drain: bool,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a normal way of doing this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do similar for PrefixIterator, that would allow use to create drain_keys easily if we want.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually spend a few minutes trying to see if we can merge this with PrefixIterator, the impls are pretty similar, only the decoding closure is different.

Copy link
Member

@shawntabrizi shawntabrizi Jul 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kianenigma seems hard to have the output be only keys or also keys and values. (i.e. making an API that makes sense and works in both scenarios?)

Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@shawntabrizi
Copy link
Member

bot merge

@ghost
Copy link

ghost commented Jul 1, 2021

Waiting for commit status.

@ghost ghost merged commit 57e0bb3 into master Jul 1, 2021
@ghost ghost deleted the kckyeung/storage-iter-keys branch July 1, 2021 17:20
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add iter_keys Function for Iterable Storage Maps
5 participants