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

Frame remove_all with size limit. #9106

Merged
5 commits merged into from
Jun 15, 2021
Merged

Frame remove_all with size limit. #9106

5 commits merged into from
Jun 15, 2021

Conversation

cheme
Copy link
Contributor

@cheme cheme commented Jun 14, 2021

Fixes #9002.

Note that it breaks existing frame api (could have define new function with the limit but it does not seem good).

Affected APIs:

  • remove_prefix
  • clear_prefix
  • kill_prefix
  • remove_all

To fix these APIs to act the same as before, simply add an additional input parameter of None.

Also rename:

  • KillChildStorageResult -> KillStorageResult

polkadot companion: paritytech/polkadot#3257

@cheme cheme added A0-please_review Pull request needs code review. B7-runtimenoteworthy 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 14, 2021
@shawntabrizi
Copy link
Member

Note that it breaks existing frame api

The solution is simply to add None to the end right? Not actually breaking any logic silently

@cheme
Copy link
Contributor Author

cheme commented Jun 14, 2021

Note that it breaks existing frame api

The solution is simply to add None to the end right? Not actually breaking any logic silently

Yes, updating is very straight forward. Seems better to me than having two function for the same thing.

Copy link
Member

@athei athei left a comment

Choose a reason for hiding this comment

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

API changes look good to me. But we need to remove some duplication.

primitives/io/src/lib.rs Show resolved Hide resolved
primitives/state-machine/src/ext.rs Outdated Show resolved Hide resolved
@shawntabrizi
Copy link
Member

bot merge

@ghost
Copy link

ghost commented Jun 15, 2021

Trying merge.

@ghost ghost merged commit cdc55fe into paritytech:master Jun 15, 2021
@gui1117
Copy link
Contributor

gui1117 commented Jun 16, 2021

maybe it worth documenting that the function remove_all, and warn that it removes all overlay, and up to limit in the backend, people shouldn't rely on the fact that it removes exactly up to the limit.

rphmeier pushed a commit that referenced this pull request Jun 18, 2021
* remove prefixed content with limit.

* test match

* factor comment and factor ext limit removal.

* fix benchmark

Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>
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. D2-breaksapi 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.

FRAME Storage remove_all API should have a key limit
5 participants