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

Implement a CountedStorageMap #9089

Closed
wants to merge 11 commits into from
Closed

Implement a CountedStorageMap #9089

wants to merge 11 commits into from

Conversation

shawntabrizi
Copy link
Member

This PR introduces a new Storage abstraction: CountedStorageMap

The CountedStorageMap is a simple wrapper around a StorageMap and StorageValue<u32> which keeps count of the number of items in the map by using the value as a counter.

@github-actions github-actions bot added the A0-please_review Pull request needs code review. label Jun 12, 2021
@shawntabrizi shawntabrizi added B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Jun 12, 2021
@shawntabrizi
Copy link
Member Author

Few things I need help improving here:

  • Is it possible to get rid of the Key and Value generics in CountedStorageMap? The information is redundant in the fact that we wrap StorageMap which ultimately defines the key and value.
  • Does it make sense to create a match arm for the #[pallet::storage] macro for this, so we dont need to define the other two storage items, but just define one object?
  • How can I prevent users from calling the Map / Value without using the wrapper? It does not seem I can keep things private.

gui1117
gui1117 reviewed Jun 13, 2021
View reviewed changes
frame/support/src/storage/counted_map.rs Outdated Show resolved Hide resolved
frame/support/src/storage/counted_map.rs Outdated Show resolved Hide resolved
@gui1117
Copy link
Contributor

gui1117 commented Jun 13, 2021

about raised issue, the whole thing is that we would want to get rid of the generator (which were only useful for decl_storage), in order to implement the traits StorageMap directly.

some idea were given in another context here #8605 (comment)

xlc
xlc reviewed Jun 13, 2021
View reviewed changes
frame/support/src/storage/counted_map.rs Outdated Show resolved Hide resolved
frame/support/src/storage/counted_map.rs Outdated Show resolved Hide resolved
frame/support/src/storage/counted_map.rs Outdated Show resolved Hide resolved
frame/support/src/storage/counted_map.rs Outdated Show resolved Hide resolved
@gui1117 @coriolinus @xlc
Apply suggestions from code review
9ce58da 
Co-authored-by: Peter Goodspeed-Niklaus <coriolinus@users.noreply.github.com>
Co-authored-by: Xiliang Chen <xlchen1291@gmail.com>
@gui1117 gui1117 added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A0-please_review Pull request needs code review. labels Jun 14, 2021
@emostov emostov mentioned this pull request Jun 15, 2021
Merged
1 task
@gui1117 gui1117 added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. A7-needspolkadotpr labels Jun 16, 2021
gui1117
gui1117 reviewed Jun 16, 2021
View reviewed changes
frame/support/src/storage/types/counted_map.rs
}

/// Remove all value of the storage.
pub fn remove_all() {
Copy link
Contributor

@gui1117 gui1117 Jun 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

if we want to support limit, then we need sp-io to return the number of deleted key in the overlay, once merged I will open an issue, and we can work on it when we need it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is this not already covered here?

#9106

Copy link
Contributor

@gui1117 gui1117 Jun 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

the new API doesn't say about how many value are removed from the overlay, without this information it is not possible to update the counter efficiently

@gui1117 gui1117 added B3-apinoteworthy and removed B0-silent Changes should not be mentioned in any release notes labels Jun 16, 2021
@gui1117
Copy link
Contributor

gui1117 commented Jun 16, 2021 •
edited
Loading

superseeded by #9125 for proper review approvals

@gui1117 gui1117 closed this Jun 16, 2021
@gui1117 gui1117 deleted the shawntabrizi-counted-map branch June 16, 2021 10:34
@louismerlin louismerlin removed the D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. label Oct 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@xlc xlc xlc left review comments

@KiChjang KiChjang KiChjang left review comments

@coriolinus coriolinus coriolinus left review comments

@gui1117 gui1117 gui1117 left review comments

Assignees
No one assigned
Labels
A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@shawntabrizi @gui1117 @xlc @KiChjang @coriolinus @louismerlin