Add remove method to Mapping
#1023
Conversation
🦑 📈 ink! Example Contracts ‒ Size Change Report 📉 🦑These are the results of building the
Link to the run | Last update: Mon Nov 29 15:58:17 CET 2021 |
Codecov Report
@@ Coverage Diff @@
## master #1023 +/- ##
==========================================
- Coverage 78.78% 75.62% -3.17%
==========================================
Files 248 248
Lines 9363 9367 +4
==========================================
- Hits 7377 7084 -293
- Misses 1986 2283 +297
Continue to review full report at Codecov.
|
crates/storage/src/lazy/mapping.rs
Outdated
| @@ -164,6 +180,45 @@ const _: () = { | |||
| mod tests { | |||
| use super::*; | |||
|
|
|||
| /// A dummy type which `REQUIRES_DEEP_CLEAN_UP` | |||
| #[derive(PartialEq, Debug, scale::Encode, scale::Decode)] | |||
| struct DeepClean<T>(T); | |||
There was a problem hiding this comment.
I wasn't able to find any of the "default" types that we implement SpreadLayout for which had REQUIRES_DEEP_CLEAN_UP set to true. Let me know if there is such a type so I can use that instead
There was a problem hiding this comment.
ink_storage::Box is an example.
Generally ink_storage::Box is the interface to work with SpreadLayout types from within PackedLayout cells.
There was a problem hiding this comment.
I tried using a Box, but it uses the REQUIRES_DEEP_CLEAN_UP of T, which are all false for primitives types as far as I could tell
There was a problem hiding this comment.
You usually use ink_storage::Box with SpreadLayout types that do nit implement PackedLayout. For example other ink_storage::collections types such as ink_storage::Vec.
HCastano
changed the title
remove method to Mappingclear_entry method to Mapping
crates/storage/src/lazy/mapping.rs
Outdated
| } | ||
|
|
||
| /// Clears the value at `key` from storage. | ||
| pub fn clear_entry<Q>(&self, key: Q) |
There was a problem hiding this comment.
| pub fn clear_entry<Q>(&self, key: Q) | |
| pub fn remove<Q>(&self, key: Q) |
Le'ts name it remove. The term entry doesn't appear anywhere else in Mapping (there's also no get_entry, no insert_entry). It can also be misleading, since we have an entry API for some data structures.
You'll have to adapt the other occurrences as well.
There was a problem hiding this comment.
I too think that remove as a name is better.
There was a problem hiding this comment.
I want to reserve remove in case we want to add an analogue to HashMap/BTreeMap::remove. Although I guess we can also use take for that. What do you guys think?
There was a problem hiding this comment.
I agree. I'm wondering, do you think you'll get away without take for the examples?
There was a problem hiding this comment.
So far I haven't needed it, but I've only gone through ERC20, ERC1155, and just started ERC721. Either way, its behaviour can be replicated by doing get followed by clear_entry (or whatever we name it)
crates/storage/src/lazy/mapping.rs
Outdated
| @@ -164,6 +180,45 @@ const _: () = { | |||
| mod tests { | |||
| use super::*; | |||
|
|
|||
| /// A dummy type which `REQUIRES_DEEP_CLEAN_UP` | |||
| #[derive(PartialEq, Debug, scale::Encode, scale::Decode)] | |||
| struct DeepClean<T>(T); | |||
There was a problem hiding this comment.
ink_storage::Box is an example.
Generally ink_storage::Box is the interface to work with SpreadLayout types from within PackedLayout cells.
Robbepop
changed the title
clear_entry method to Mappingremove method to Mapping
* Add `remove` method to `Mapping` * Avoid loading entries during clean-up if no deep clean is required * Change API to just clear storage instead of also returning value * Reduce trait bound to just `EncodeLike` * Rename `clear_entry` back to `remove` * Replace dummy `DeepClean` type with `Pack` * RustFmt
While updating one of the example contracts I realized that this would be a useful method
to have.