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

Simple Mapping type improvements #979

Merged
merged 38 commits into from
Nov 9, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
70172ac
Add `Mapping` storage collection
HCastano Sep 29, 2021
0ce829f
Implement `insert` and `get` for `Mapping`
HCastano Sep 29, 2021
e7e7f5d
Implement `SpreadLayout` for `Mapping`
HCastano Sep 29, 2021
1fead2f
Fix typo
HCastano Sep 29, 2021
62a6f01
Add some basic tests
HCastano Sep 29, 2021
f25b560
Fix some documentation formatting
HCastano Sep 29, 2021
9622ad5
Use `PackedLayout` as trait bound instead of `Encode/Decode`
HCastano Sep 30, 2021
73d4ab9
Avoid using low level `ink_env` functions when interacting with storage
HCastano Sep 30, 2021
92a4ce6
RustFmt
HCastano Sep 30, 2021
e1650c2
Appease Clippy
HCastano Sep 30, 2021
60c3370
Only use single `PhantomData` field
HCastano Sep 30, 2021
cfd4b03
Change `get` API to take reference to `key`
HCastano Sep 30, 2021
60bd5cd
Implement `TypeInfo` and `StorageLayout` for `Mapping`
HCastano Sep 30, 2021
e173a4d
Properly gate `TypeInfo` and `StorageLayout` impls behind `std`
HCastano Sep 30, 2021
99cad62
Replace `HashMap` with `Mapping` in ERC-20 example
HCastano Sep 30, 2021
047ae35
Return `Option` from `Mapping::get`
HCastano Oct 1, 2021
b880b5a
Update ERC-20 to handle `Option` returns
HCastano Oct 1, 2021
20395ab
Change `get` and `key` to use `Borrow`-ed values
HCastano Oct 11, 2021
955896b
Add `Debug` and `Default` implementations
HCastano Oct 11, 2021
de7e67f
Proper spelling
HCastano Oct 11, 2021
623ba34
Change `insert` to only accept borrowed K,V pairs
HCastano Oct 13, 2021
9d12813
Update ERC-20 example accordingly
HCastano Oct 13, 2021
387b752
Make more explicit what each `key` is referring to
HCastano Oct 13, 2021
5ce455c
Try using a `RefCell` instead of passing `Key` around
HCastano Oct 13, 2021
e272f3d
Try using `UnsafeCell` instead
HCastano Oct 13, 2021
b0ecb48
Revert "Try using a `RefCell` instead of passing `Key` around"
HCastano Oct 14, 2021
ebfaea5
Clean up some of the documentation
HCastano Oct 19, 2021
e652165
adjust the Mapping type for the new SpreadAllocate trait
Robbepop Oct 23, 2021
91f65f1
adjust ERC-20 example for changes in Mapping type
Robbepop Oct 23, 2021
8ee0882
remove commented out code
Robbepop Oct 23, 2021
8fcdbdf
add doc comment to new_init
Robbepop Oct 23, 2021
89f5799
make it possible to use references in more cases with Mapping
Robbepop Oct 23, 2021
53e63b4
use references in more cases for ERC-20 example contract
Robbepop Oct 23, 2021
cf823d1
remove unnecessary references in Mapping methods
Robbepop Oct 23, 2021
674a04d
refactor/improve pull_packed_root_opt utility method slightly
Robbepop Oct 23, 2021
46286d4
fix ERC-20 example contract
Robbepop Oct 23, 2021
8da218f
Merge branch 'master' into hc-rf-mapping-type
HCastano Nov 4, 2021
9ee7ee6
Merge branch 'hc-simple-mapping-primitive' into hc-rf-mapping-type
HCastano Nov 4, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 43 additions & 35 deletions crates/storage/src/collections/mapping/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,21 +14,20 @@

//! A simple mapping to contract storage.
//!
//! This mapping doesn't actually "own" any data. Instead it is just a simple wrapper around the
//! contract storage facilities.
//! # Note
//!
//! This mapping doesn't actually "own" any data.
//! Instead it is just a simple wrapper around the contract storage facilities.

use crate::traits::{
clear_spread_root,
pull_packed_root_opt,
pull_spread_root,
push_packed_root,
push_spread_root,
ExtKeyPtr,
KeyPtr,
PackedLayout,
SpreadAllocate,
SpreadLayout,
};

use core::marker::PhantomData;

use ink_env::hash::{
Expand All @@ -42,7 +41,7 @@ use ink_primitives::Key;
#[derive(Default)]
pub struct Mapping<K, V> {
offset_key: Key,
_marker: PhantomData<(K, V)>,
_marker: PhantomData<fn() -> (K, V)>,
}

impl<K, V> core::fmt::Debug for Mapping<K, V> {
Expand All @@ -53,52 +52,49 @@ impl<K, V> core::fmt::Debug for Mapping<K, V> {
}
}

impl<K, V> Mapping<K, V>
where
K: PackedLayout,
V: PackedLayout,
{
impl<K, V> Mapping<K, V> {
/// Creates a new empty `Mapping`.
///
/// TODO [#961]: Ideally we improve how this is initialized by extending the
/// `SpreadLayout`/`PackedLayout` traits for non-caching data structures.
pub fn new(offset_key: Key) -> Self {
fn new(offset_key: Key) -> Self {
Self {
offset_key,
_marker: Default::default(),
}
}
}

impl<K, V> Mapping<K, V>
where
K: PackedLayout,
V: PackedLayout,
{
/// Insert the given `value` to the contract storage.
pub fn insert<Q, R>(&mut self, key: &Q, value: &R)
#[inline]
pub fn insert<Q, R>(&mut self, key: Q, value: &R)
where
K: core::borrow::Borrow<Q>,
Q: scale::Encode,
V: core::borrow::Borrow<R>,
R: PackedLayout,
Q: scale::EncodeLike<K>,
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

R: scale::EncodeLike<V> + PackedLayout,
{
push_packed_root(value, &self.storage_key(key));
}

/// Get the `value` at `key` from the contract storage.
///
/// Returns `None` if no `value` exists at the given `key`.
pub fn get<Q>(&self, key: &Q) -> Option<V>
#[inline]
pub fn get<Q>(&self, key: Q) -> Option<V>
where
K: core::borrow::Borrow<Q>,
Q: scale::Encode,
Q: scale::EncodeLike<K>,
{
pull_packed_root_opt(&self.storage_key(key))
}

/// Returns a `Key` pointer used internally by the storage API.
///
/// 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
/// 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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

have you benchmarked that this yields an improvement?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

where
K: core::borrow::Borrow<Q>,
Q: scale::Encode,
Q: scale::EncodeLike<K>,
{
let encodedable_key = (&self.offset_key, key);
let mut output = <Blake2x256 as HashOutput>::Type::default();
Expand All @@ -113,20 +109,32 @@ impl<K, V> SpreadLayout for Mapping<K, V> {

#[inline]
fn pull_spread(ptr: &mut KeyPtr) -> Self {
let root_key = ExtKeyPtr::next_for::<Self>(ptr);
pull_spread_root::<Self>(root_key)
// Note: There is no need to pull anything from the storage for the
// mapping type since it initializes itself entirely by the
// given key pointer.
Self::new(*ExtKeyPtr::next_for::<Self>(ptr))
}

#[inline]
fn push_spread(&self, ptr: &mut KeyPtr) {
let root_key = ExtKeyPtr::next_for::<Self>(ptr);
push_spread_root::<Self>(self, root_key);
// Note: The mapping type does not store any state in its associated
// storage region, therefore only the pointer has to be incremented.
ptr.advance_by(Self::FOOTPRINT);
}

#[inline]
fn clear_spread(&self, ptr: &mut KeyPtr) {
let root_key = ExtKeyPtr::next_for::<Self>(ptr);
clear_spread_root::<Self>(self, root_key);
// Note: The mapping type is not aware of its elements, therefore
// it is not possible to clean up after itself.
ptr.advance_by(Self::FOOTPRINT);
}
}

impl<K, V> SpreadAllocate for Mapping<K, V> {
#[inline]
fn allocate_spread(ptr: &mut KeyPtr) -> Self {
// Note: The mapping type initializes itself entirely by the key pointer.
Self::new(*ExtKeyPtr::next_for::<Self>(ptr))
}
}

Expand Down
24 changes: 13 additions & 11 deletions crates/storage/src/traits/optspec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -102,17 +102,19 @@ pub fn pull_packed_root_opt<T>(root_key: &Key) -> Option<T>
where
T: PackedLayout,
{
match ink_env::get_contract_storage::<T>(root_key)
.expect("decoding does not match expected type")
{
Some(mut value) => {
// In case the contract storage is occupied we handle
// the Option<T> as if it was a T.
ink_env::get_contract_storage::<T>(root_key)
.unwrap_or_else(|error| {
panic!(
"failed to pull packed from root key {}: {:?}",
root_key, error
)
})
.map(|mut value| {
// In case the contract storage is occupied at the root key
// we handle the Option<T> as if it was a T.
<T as PackedLayout>::pull_packed(&mut value, root_key);
Some(value)
}
None => None,
}
value
})
}

pub fn push_packed_root_opt<T>(entity: Option<&T>, root_key: &Key)
Expand All @@ -128,7 +130,7 @@ where
super::push_packed_root(value, root_key)
}
None => {
// Clear the associated storage cell.
// Clear the associated storage cell since the entity is `None`.
ink_env::clear_contract_storage(root_key);
}
}
Expand Down
92 changes: 69 additions & 23 deletions examples/erc20/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,14 @@ use ink_lang as ink;

#[ink::contract]
mod erc20 {
use ink_primitives::{
Key,
KeyPtr,
};
use ink_storage::{
collections::mapping::Mapping,
lazy::Lazy,
traits::SpreadAllocate,
};

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

impl SpreadAllocate for Erc20 {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

fn allocate_spread(ptr: &mut KeyPtr) -> Self {
Self {
total_supply: SpreadAllocate::allocate_spread(ptr),
balances: SpreadAllocate::allocate_spread(ptr),
allowances: SpreadAllocate::allocate_spread(ptr),
}
}
}

impl Erc20 {
/// Creates a new ERC-20 contract with the specified initial supply.
#[ink(constructor)]
pub fn new(initial_supply: Balance) -> Self {
let root_key = Key::from([0x00; 32]);
let mut key_ptr = KeyPtr::from(root_key);
let mut instance = Self::allocate_spread(&mut key_ptr);
instance.new_init(initial_supply);
instance
}

/// Default initializes the ERC-20 contract with the specified initial supply.
fn new_init(&mut self, initial_supply: Balance) {
let caller = Self::env().caller();
let mut balances = Mapping::new([1u8; 32].into());
balances.insert(&caller, &initial_supply);
let instance = Self {
total_supply: Lazy::new(initial_supply),
balances,
allowances: Mapping::new([1u8; 32].into()),
};
self.balances.insert(&caller, &initial_supply);
Lazy::set(&mut self.total_supply, initial_supply);
Self::env().emit_event(Transfer {
from: None,
to: Some(caller),
value: initial_supply,
});
instance
}

/// Returns the total token supply.
Expand All @@ -86,15 +104,41 @@ mod erc20 {
/// Returns `0` if the account is non-existent.
#[ink(message)]
pub fn balance_of(&self, owner: AccountId) -> Balance {
self.balances.get(&owner).unwrap_or_default() // .copied().unwrap_or(0)
self.balance_of_impl(&owner)
}

/// Returns the account balance for the specified `owner`.
///
/// Returns `0` if the account is non-existent.
///
/// # Note
///
/// Prefer to call this method over `balance_of` since this
/// works using references which are more efficient in Wasm.
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

#[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`.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

#[ink(message)]
pub fn allowance(&self, owner: AccountId, spender: AccountId) -> Balance {
self.allowances.get(&(owner, spender)).unwrap_or_default() //.copied().unwrap_or(0)
self.allowance_impl(&owner, &spender)
}

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

Choose a reason for hiding this comment

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

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

///
/// # Note
///
/// Prefer to call this method over `allowance` since this
/// works using references which are more efficient in Wasm.
#[inline]
fn allowance_impl(&self, owner: &AccountId, spender: &AccountId) -> Balance {
self.allowances.get((owner, spender)).unwrap_or_default()
}

/// Transfers `value` amount of tokens from the caller's account to account `to`.
Expand All @@ -108,7 +152,7 @@ mod erc20 {
#[ink(message)]
pub fn transfer(&mut self, to: AccountId, value: Balance) -> Result<()> {
let from = self.env().caller();
self.transfer_from_to(from, to, value)
self.transfer_from_to(&from, &to, value)
}

/// Allows `spender` to withdraw from the caller's account multiple times, up to
Expand All @@ -120,7 +164,7 @@ mod erc20 {
#[ink(message)]
pub fn approve(&mut self, spender: AccountId, value: Balance) -> Result<()> {
let owner = self.env().caller();
self.allowances.insert(&(owner, spender), &value);
self.allowances.insert((&owner, &spender), &value);
self.env().emit_event(Approval {
owner,
spender,
Expand Down Expand Up @@ -151,12 +195,13 @@ mod erc20 {
value: Balance,
) -> Result<()> {
let caller = self.env().caller();
let allowance = self.allowance(from, caller);
let allowance = self.allowance_impl(&from, &caller);
if allowance < value {
return Err(Error::InsufficientAllowance)
}
self.transfer_from_to(from, to, value)?;
self.allowances.insert(&(from, caller), &(allowance - value));
self.transfer_from_to(&from, &to, value)?;
self.allowances
.insert((&from, &caller), &(allowance - value));
Ok(())
}

Expand All @@ -170,20 +215,21 @@ mod erc20 {
/// the caller's account balance.
fn transfer_from_to(
&mut self,
from: AccountId,
to: AccountId,
from: &AccountId,
to: &AccountId,
value: Balance,
) -> Result<()> {
let from_balance = self.balance_of(from);
let from_balance = self.balance_of_impl(from);
if from_balance < value {
return Err(Error::InsufficientBalance)
}
self.balances.insert(&from, &(from_balance - value));
let to_balance = self.balance_of(to);
self.balances.insert(&to, &(to_balance + value));

self.balances.insert(from, &(from_balance - value));
let to_balance = self.balance_of_impl(to);
self.balances.insert(to, &(to_balance + value));
self.env().emit_event(Transfer {
from: Some(from),
to: Some(to),
from: Some(*from),
to: Some(*to),
value,
});
Ok(())
Expand Down