Simple Mapping type improvements #979

Robbepop · 2021-10-23T09:43:42Z

This fork of the above PR improves the Mapping type even further which reduces the ERC-20 example smart contract Wasm file size to 10.8kB so far.

Implementation Notes

The ink! codegen does not automatically generate SpreadAllocate implementations of its #[ink(storage)] struct and at this point it is even undecided if that's a good idea. Therefore we currently are required to implement SpreadAllocate manually in the ERC-20 example contract.
The ERC-20 new constructor as well as the new_init method seem a bit verbose but we are working on some improvements to make this less of a deal.
The EncodeLike trait usage in the Mapping type allows us to use &(&K1, &K2) in the Mapping::{get, insert} methods instead of just &(K1, K2) which saves us a lot of Wasm file size overhead with respect to big value types such as Balance and especially AccountId.
To avoid dealing with big value types helper methods have been implemented in the ERC-20 example contract such as allowance_impl and balance_of_impl that take and forward their arguments by reference instead of by value.

This reverts commit cede033. Using `RefCell`/`UnsafeCell` doesn't reduce the contract size more than what we have now, and it introduced `unsafe` code. We believe the limiting factor here is the `Key` type definition anyways.

ascjones

LGTM.

As you say the constructor could do with some improvement, but size improvements good.

ascjones · 2021-10-28T08:43:36Z

examples/erc20/lib.rs

+        /// # Note
+        ///
+        /// Prefer to call this method over `balance_of` since this
+        /// works using references which are more efficient in Wasm.


I wonder whether that's another ra lint we could add in cargo-contract, to prefer references as keys for a Mapping.

HCastano

Just curious, have you been able to successfully deploy the ERC-20 contract to a local chain?

examples/erc20/lib.rs

HCastano · 2021-10-29T13:50:39Z

examples/erc20/lib.rs

+
+        /// Returns the amount which `spender` is still allowed to withdraw from `owner`.
+        ///
+        /// Returns `0` if no allowance has been set `0`.


Suggested change

/// Returns `0` if no allowance has been set `0`.

/// Returns `0` if no allowance has been set.

HCastano · 2021-10-29T13:51:23Z

examples/erc20/lib.rs

+        /// works using references which are more efficient in Wasm.
+        #[inline]
+        fn balance_of_impl(&self, owner: &AccountId) -> Balance {
+            self.balances.get(owner).unwrap_or_default()
        }

        /// Returns the amount which `spender` is still allowed to withdraw from `owner`.
        ///
        /// Returns `0` if no allowance has been set `0`.


Suggested change

/// Returns `0` if no allowance has been set `0`.

/// Returns `0` if no allowance has been set.

examples/erc20/lib.rs

HCastano · 2021-10-29T16:44:54Z

crates/storage/src/collections/mapping/mod.rs

+    ///
+    /// This key is a combination of the `Mapping`'s internal `offset_key`
+    /// and the user provided `key`.
+    fn storage_key<Q>(&self, key: Q) -> Key


Suggested change

fn storage_key<Q>(&self, key: Q) -> Key

#[inline]

fn storage_key<Q>(&self, key: Q) -> Key

have you benchmarked that this yields an improvement?

I think we don't need to specify the inline attribute. Because the compiler will decide it based on the optimization flag(in our case it reduces the size of the code). So compiler better knows what better to inline to reduce the size of the contract=)
https://nnethercote.github.io/perf-book/inlining.html

Just tried now, and there are no changes to the Wasm size. However, there are also no changes if I remove the inline's from insert() and get(), so maybe we should get rid of those as well?

Not sure if the compiler also takes into account that passing large types (> 64bit) is very expensive in wasm. Ideally, Q would be a reference.

HCastano · 2021-11-04T18:18:00Z

crates/storage/src/collections/mapping/mod.rs

-        Q: scale::Encode,
-        V: core::borrow::Borrow<R>,
-        R: PackedLayout,
+        Q: scale::EncodeLike<K>,


Can I think of EncodeLike sort-of like EncodeLike = Borrow + Encode?

EncodeLike is a purely marker trait coming from the parity-scale-codec crate. Please go to the crates docs to read more about its general use.

I read the docs, and I think my question is still valid 😄

I think the semantics of a type being able to be SCALE encoded as another type (EncodeLike) vs. being able to be borrowed as another type (Borrow) are slightly different, which is why I asked

HCastano · 2021-11-04T18:37:13Z

examples/erc20/lib.rs

@@ -55,24 +60,37 @@ mod erc20 {
    /// The ERC-20 result type.
    pub type Result<T> = core::result::Result<T, Error>;

+    impl SpreadAllocate for Erc20 {


Should we add a note here that this is intended to only be temporary and will (hopefully) be replaced by #995?

I think it is better to merge this PR after merging the one PR that improves the SpreadLayout traits.

examples/erc20/lib.rs

* Add `Mapping` storage collection * Implement `insert` and `get` for `Mapping` * Implement `SpreadLayout` for `Mapping` * Fix typo * Add some basic tests * Fix some documentation formatting * Use `PackedLayout` as trait bound instead of `Encode/Decode` * Avoid using low level `ink_env` functions when interacting with storage * RustFmt * Appease Clippy * Only use single `PhantomData` field * Change `get` API to take reference to `key` * Implement `TypeInfo` and `StorageLayout` for `Mapping` * Properly gate `TypeInfo` and `StorageLayout` impls behind `std` * Replace `HashMap` with `Mapping` in ERC-20 example * Return `Option` from `Mapping::get` * Update ERC-20 to handle `Option` returns * Change `get` and `key` to use `Borrow`-ed values * Add `Debug` and `Default` implementations * Proper spelling * Change `insert` to only accept borrowed K,V pairs * Update ERC-20 example accordingly * Make more explicit what each `key` is referring to * Try using a `RefCell` instead of passing `Key` around * Try using `UnsafeCell` instead * Revert "Try using a `RefCell` instead of passing `Key` around" This reverts commit cede033. Using `RefCell`/`UnsafeCell` doesn't reduce the contract size more than what we have now, and it introduced `unsafe` code. We believe the limiting factor here is the `Key` type definition anyways. * Clean up some of the documentation * Simple Mapping type improvements (#979) * Add `Mapping` storage collection * Implement `insert` and `get` for `Mapping` * Implement `SpreadLayout` for `Mapping` * Fix typo * Add some basic tests * Fix some documentation formatting * Use `PackedLayout` as trait bound instead of `Encode/Decode` * Avoid using low level `ink_env` functions when interacting with storage * RustFmt * Appease Clippy * Only use single `PhantomData` field * Change `get` API to take reference to `key` * Implement `TypeInfo` and `StorageLayout` for `Mapping` * Properly gate `TypeInfo` and `StorageLayout` impls behind `std` * Replace `HashMap` with `Mapping` in ERC-20 example * Return `Option` from `Mapping::get` * Update ERC-20 to handle `Option` returns * Change `get` and `key` to use `Borrow`-ed values * Add `Debug` and `Default` implementations * Proper spelling * Change `insert` to only accept borrowed K,V pairs * Update ERC-20 example accordingly * Make more explicit what each `key` is referring to * Try using a `RefCell` instead of passing `Key` around * Try using `UnsafeCell` instead * Revert "Try using a `RefCell` instead of passing `Key` around" This reverts commit cede033. Using `RefCell`/`UnsafeCell` doesn't reduce the contract size more than what we have now, and it introduced `unsafe` code. We believe the limiting factor here is the `Key` type definition anyways. * Clean up some of the documentation * adjust the Mapping type for the new SpreadAllocate trait * adjust ERC-20 example for changes in Mapping type * remove commented out code * add doc comment to new_init * make it possible to use references in more cases with Mapping * use references in more cases for ERC-20 example contract * remove unnecessary references in Mapping methods * refactor/improve pull_packed_root_opt utility method slightly * fix ERC-20 example contract The problem with *self.total_supply is that it may implicitly read from storage in case it has not yet read a value from storage whereas Lazy::set just writes the value to the Lazy instance. Co-authored-by: Hernando Castano <hernando@hcastano.com> Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com> * Use new `initialize_contract()` function * Derive `SpreadAllocate` for `ink(storage)` structs * Stop manually implementing SpreadAllocate for ERC-20 * Stop implementing `SpreadAllocate` in the storage codegen * Derive `SpreadAllocate` manually for ERC-20 * RustFmt example * Move `Mapping` from `collections` to `lazy` * Remove extra `0` in docs Co-authored-by: Robin Freyler <robin.freyler@gmail.com>

forgetso · 2021-11-18T17:40:31Z

Is there a plan for a function like get_mut as part of Mapping? How do we update a key value pair - just insert again?

xgreenx · 2021-11-18T18:17:43Z

Is there a plan for a function like get_mut as part of Mapping? How do we update a key value pair - just insert again?

Yes, insert again=)

Robbepop · 2021-11-18T18:51:22Z

Is there a plan for a function like get_mut as part of Mapping? How do we update a key value pair - just insert again?

A get_mut function won't be part of the simple Mapping type but it will be available on types that build upon the Mapping type internally and provide a more high-level API.

forgetso · 2021-11-19T15:49:27Z

Thanks for the info. Is iter deliberately not implemented for Mapping? I started converting my contract to Mapping but I have methods that previously used hashmap.keys(). Now I can't seem to create an iterator for Mapping to implement a keys style function.

    impl ProviderMapping {
        fn keys(&self) -> Vec<AccountId> {
            let mut keys: Vec<AccountId> = Vec::new();
            for (key, value) in &self.providers.iter() {
                keys.push(key);
            }
            keys
        }

for (key, value) in &self.providers.iter() {
   |                                                 ^^^^ method not found in `Mapping<ink_env::AccountId, Provider>`

Do I need to implement iter for my custom Mapping struct or should I wait for the higher level API that will build on Mapping?

Robbepop · 2021-11-19T16:09:15Z

If you come from the Solidity world then Mapping is literally just what Solidity provides to you with its own builtin mapping types which are ... just mappings and no real collections that do have knowledge about their contained elements.
So it is not the job of the Mapping type to provide collection APIs such as iter. This is a very common misunderstanding unfortunately.
If what you need is a collection then you can either build one yourself using the Mapping type internally OR use ink!'s ink_storage::HashMap.

forgetso · 2021-11-19T16:37:06Z

Cool this is my first time coding smart contracts and I'm coming from a python/JS background, never written in Rust/C. I'll try creating an iterable Mapping as per this example and compare contract size to Hashmap. Thanks for the clarification, you probably saved me a few more hours or days of searching.

xgreenx · 2021-11-19T18:07:08Z

Cool this is my first time coding smart contracts and I'm coming from a python/JS background, never written in Rust/C. I'll try creating an iterable Mapping as per this example and compare contract size to Hashmap. Thanks for the clarification, you probably saved me a few more hours or days of searching.

Better to avoid iteration through the map in the smart contracts because it can contain a lot of entries. If you need to resolve some local issue(for example, in your algorithm, you need properties of the map to solve some issues. But it is needed only during function execution and after that, you don't need to store that map into storage), you can use ink_prelude::collections::HashMap. Or if you know that count of entries is not big, then you can store ink_prelude::collections::HashMap into the storage(when you will load/save that map you will load/save the whole map with all entries).

forgetso · 2021-11-24T15:10:12Z

I have reworked my contract using Mapping instead of Hashmap, which has reduced the size from 59.8K to 45K when building on Supercolony's reworked-common-part-of-message branch. However, I also removed any get functions and replaced them with Vectors that contain known keys for Mappings - so that will have helped.

One of the issues I ran into during this process is that enum is not supported by SpreadAllocate, which seems to be required as part of the new contract instantiation process.

I had to modify ink to allow enums to implement SpreadAllocate. Hopefully this will be allowed in ink in future.

I also tried and failed to generalise my types that build on Mapping, as mentioned here.

xgreenx · 2021-11-29T13:37:31Z

I have reworked my contract using Mapping instead of Hashmap, which has reduced the size from 59.8K to 45K when building on Supercolony's reworked-common-part-of-message branch. However, I also removed any get functions and replaced them with Vectors that contain known keys for Mappings - so that will have helped.

One of the issues I ran into during this process is that enum is not supported by SpreadAllocate, which seems to be required as part of the new contract instantiation process.

I had to modify ink to allow enums to implement SpreadAllocate. Hopefully this will be allowed in ink in future.

I also tried and failed to generalise my types that build on Mapping, as mentioned here.

I tried your contract with a new version and it takes Original wasm size: 81.7K, Optimized: 32.4K(32355).

* Add `Mapping` storage collection * Implement `insert` and `get` for `Mapping` * Implement `SpreadLayout` for `Mapping` * Fix typo * Add some basic tests * Fix some documentation formatting * Use `PackedLayout` as trait bound instead of `Encode/Decode` * Avoid using low level `ink_env` functions when interacting with storage * RustFmt * Appease Clippy * Only use single `PhantomData` field * Change `get` API to take reference to `key` * Implement `TypeInfo` and `StorageLayout` for `Mapping` * Properly gate `TypeInfo` and `StorageLayout` impls behind `std` * Replace `HashMap` with `Mapping` in ERC-20 example * Return `Option` from `Mapping::get` * Update ERC-20 to handle `Option` returns * Change `get` and `key` to use `Borrow`-ed values * Add `Debug` and `Default` implementations * Proper spelling * Change `insert` to only accept borrowed K,V pairs * Update ERC-20 example accordingly * Make more explicit what each `key` is referring to * Try using a `RefCell` instead of passing `Key` around * Try using `UnsafeCell` instead * Revert "Try using a `RefCell` instead of passing `Key` around" This reverts commit cede033. Using `RefCell`/`UnsafeCell` doesn't reduce the contract size more than what we have now, and it introduced `unsafe` code. We believe the limiting factor here is the `Key` type definition anyways. * Clean up some of the documentation * Simple Mapping type improvements (use-ink#979) * Add `Mapping` storage collection * Implement `insert` and `get` for `Mapping` * Implement `SpreadLayout` for `Mapping` * Fix typo * Add some basic tests * Fix some documentation formatting * Use `PackedLayout` as trait bound instead of `Encode/Decode` * Avoid using low level `ink_env` functions when interacting with storage * RustFmt * Appease Clippy * Only use single `PhantomData` field * Change `get` API to take reference to `key` * Implement `TypeInfo` and `StorageLayout` for `Mapping` * Properly gate `TypeInfo` and `StorageLayout` impls behind `std` * Replace `HashMap` with `Mapping` in ERC-20 example * Return `Option` from `Mapping::get` * Update ERC-20 to handle `Option` returns * Change `get` and `key` to use `Borrow`-ed values * Add `Debug` and `Default` implementations * Proper spelling * Change `insert` to only accept borrowed K,V pairs * Update ERC-20 example accordingly * Make more explicit what each `key` is referring to * Try using a `RefCell` instead of passing `Key` around * Try using `UnsafeCell` instead * Revert "Try using a `RefCell` instead of passing `Key` around" This reverts commit cede033. Using `RefCell`/`UnsafeCell` doesn't reduce the contract size more than what we have now, and it introduced `unsafe` code. We believe the limiting factor here is the `Key` type definition anyways. * Clean up some of the documentation * adjust the Mapping type for the new SpreadAllocate trait * adjust ERC-20 example for changes in Mapping type * remove commented out code * add doc comment to new_init * make it possible to use references in more cases with Mapping * use references in more cases for ERC-20 example contract * remove unnecessary references in Mapping methods * refactor/improve pull_packed_root_opt utility method slightly * fix ERC-20 example contract The problem with *self.total_supply is that it may implicitly read from storage in case it has not yet read a value from storage whereas Lazy::set just writes the value to the Lazy instance. Co-authored-by: Hernando Castano <hernando@hcastano.com> Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com> * Use new `initialize_contract()` function * Derive `SpreadAllocate` for `ink(storage)` structs * Stop manually implementing SpreadAllocate for ERC-20 * Stop implementing `SpreadAllocate` in the storage codegen * Derive `SpreadAllocate` manually for ERC-20 * RustFmt example * Move `Mapping` from `collections` to `lazy` * Remove extra `0` in docs Co-authored-by: Robin Freyler <robin.freyler@gmail.com>

HCastano and others added 30 commits October 23, 2021 11:00

Add Mapping storage collection

70172ac

Implement insert and get for Mapping

0ce829f

Implement SpreadLayout for Mapping

e7e7f5d

Fix typo

1fead2f

Add some basic tests

62a6f01

Fix some documentation formatting

f25b560

Use PackedLayout as trait bound instead of Encode/Decode

9622ad5

Avoid using low level ink_env functions when interacting with storage

73d4ab9

RustFmt

92a4ce6

Appease Clippy

e1650c2

Only use single PhantomData field

60c3370

Change get API to take reference to key

cfd4b03

Implement TypeInfo and StorageLayout for Mapping

60bd5cd

Properly gate TypeInfo and StorageLayout impls behind std

e173a4d

Replace HashMap with Mapping in ERC-20 example

99cad62

Return Option from Mapping::get

047ae35

Update ERC-20 to handle Option returns

b880b5a

Change get and key to use Borrow-ed values

20395ab

Add Debug and Default implementations

955896b

Proper spelling

de7e67f

Change insert to only accept borrowed K,V pairs

623ba34

Update ERC-20 example accordingly

9d12813

Make more explicit what each key is referring to

387b752

Try using a RefCell instead of passing Key around

5ce455c

Try using UnsafeCell instead

e272f3d

Revert "Try using a RefCell instead of passing Key around"

b0ecb48

This reverts commit cede033. Using `RefCell`/`UnsafeCell` doesn't reduce the contract size more than what we have now, and it introduced `unsafe` code. We believe the limiting factor here is the `Key` type definition anyways.

Clean up some of the documentation

ebfaea5

adjust the Mapping type for the new SpreadAllocate trait

e652165

adjust ERC-20 example for changes in Mapping type

91f65f1

remove commented out code

8ee0882

Robbepop requested review from cmichi, ascjones and HCastano October 23, 2021 15:01

ascjones approved these changes Oct 28, 2021

View reviewed changes

HCastano reviewed Oct 29, 2021

View reviewed changes

Merge branch 'master' into hc-rf-mapping-type

8da218f

HCastano changed the base branch from master to hc-simple-mapping-primitive November 4, 2021 17:38

Merge branch 'hc-simple-mapping-primitive' into hc-rf-mapping-type

9ee7ee6

HCastano approved these changes Nov 4, 2021

View reviewed changes

HCastano changed the title ~~[WIP] Simple Mapping type improvements~~ Simple Mapping type improvements Nov 4, 2021

HCastano mentioned this pull request Nov 4, 2021

Wrap Comments #1003

Closed

xgreenx mentioned this pull request Nov 9, 2021

Refactor all contract to use Mapping Supercolony-net/openbrush-contracts#52

Closed

HCastano merged commit b954162 into hc-simple-mapping-primitive Nov 9, 2021

HCastano deleted the hc-rf-mapping-type branch November 9, 2021 15:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simple Mapping type improvements #979

Simple Mapping type improvements #979

Robbepop commented Oct 23, 2021 •

edited

Loading

ascjones left a comment

ascjones Oct 28, 2021

HCastano left a comment

HCastano Oct 29, 2021

HCastano Oct 29, 2021

HCastano Oct 29, 2021

Robbepop Oct 29, 2021

xgreenx Nov 4, 2021

HCastano Nov 4, 2021

athei Nov 4, 2021

HCastano Nov 4, 2021

Robbepop Nov 4, 2021

HCastano Nov 5, 2021

HCastano Nov 4, 2021

Robbepop Nov 4, 2021

forgetso commented Nov 18, 2021

xgreenx commented Nov 18, 2021

Robbepop commented Nov 18, 2021

forgetso commented Nov 19, 2021 •

edited

Loading

Robbepop commented Nov 19, 2021

forgetso commented Nov 19, 2021 •

edited

Loading

xgreenx commented Nov 19, 2021

forgetso commented Nov 24, 2021 •

edited

Loading

xgreenx commented Nov 29, 2021

	/// Returns `0` if no allowance has been set `0`.
	/// Returns `0` if no allowance has been set.

	fn storage_key<Q>(&self, key: Q) -> Key
	#[inline]
	fn storage_key<Q>(&self, key: Q) -> Key

Simple Mapping type improvements #979

Simple Mapping type improvements #979

Conversation

Robbepop commented Oct 23, 2021 • edited Loading

Implementation Notes

ascjones left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HCastano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

forgetso commented Nov 18, 2021

xgreenx commented Nov 18, 2021

Robbepop commented Nov 18, 2021

forgetso commented Nov 19, 2021 • edited Loading

Robbepop commented Nov 19, 2021

forgetso commented Nov 19, 2021 • edited Loading

xgreenx commented Nov 19, 2021

forgetso commented Nov 24, 2021 • edited Loading

xgreenx commented Nov 29, 2021

Robbepop commented Oct 23, 2021 •

edited

Loading

forgetso commented Nov 19, 2021 •

edited

Loading

forgetso commented Nov 19, 2021 •

edited

Loading

forgetso commented Nov 24, 2021 •

edited

Loading