Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Range with no prefix support #433

Merged
merged 6 commits into from
Sep 20, 2021
Merged

Range with no prefix support #433

merged 6 commits into from
Sep 20, 2021

Conversation

maurolacy
Copy link
Contributor

@maurolacy maurolacy commented Sep 18, 2021

Adds a NoPrefix associated type, to use for range() short-cuts in Map along with the EmptyPrefix marker. Just force the empty prefix in Map::range.

This type was missing from the impl. Adding it basically allows the added test case, where we iterate over a triple key without a prefix. Removed it altogether for simplicity.

Can be reviewed / merged independently of #432. Already integrated in #432 branch.

Copy link
Member

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

I see three useful no_prefix functions.

But since no prefix is always () / vec![], let's remove the argument, the extra trait type and just hard code it those 3 places.

packages/storage-plus/src/keys.rs Outdated Show resolved Hide resolved
packages/storage-plus/src/map.rs Outdated Show resolved Hide resolved
packages/storage-plus/src/map.rs Show resolved Hide resolved
packages/storage-plus/src/indexes.rs Outdated Show resolved Hide resolved
packages/storage-plus/src/indexes.rs Outdated Show resolved Hide resolved
Copy link
Member

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

Looks quite nice. Except for removing all the #[cfg(feature = "iterator")] everywhere.

Not sure how we can disable iterator (default) in cosmwasm-storage unless it is explicitly set. We should try to at least build the code without iterator enabled and ensure this works.

packages/storage-plus/src/indexed_map.rs Show resolved Hide resolved
packages/storage-plus/src/iter_helpers.rs Outdated Show resolved Hide resolved
packages/storage-plus/src/indexed_map.rs Outdated Show resolved Hide resolved
@ethanfrey
Copy link
Member

Please rebase on #438 (maybe after merge?) and fix any CI failures (due to iterator) before merge.

@maurolacy
Copy link
Contributor Author

Please rebase on #438 (maybe after merge?) and fix any CI failures (due to iterator) before merge.

Done.

Copy link
Member

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

lgtm

@ethanfrey ethanfrey merged commit 32e1e28 into main Sep 20, 2021
@ethanfrey ethanfrey deleted the range-no-prefix branch September 20, 2021 06:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants