Storage refactoring new hope for review #1331

xgreenx · 2022-07-20T17:56:27Z

We decided to abandon old PR because it contains a lot of commits and comments and is hard to review.

The whole change was split into seven commits with their descriptions. In most cases, it is crate-based splitting. But some crates united together because they related to the same area.

You could check first the commit six with updated examples. It will help to understand how the usage changed. But after better to go in order of commits.
Commits:

First commit imports new unstable functions with transparent hashing.
Updated the API to work with the generic key K: Encode instead of the old Key.
Also, the change contains optimization to reduce the size of contracts. In most cases, it is #[inline(always)]; but return_value also got small optimization; removed usage of extract_from_slice where it is not needed.
Second commit:
- primitives crate:
  Removed old 32 bytes Key. Replaced it with u32. Added KeyComposer, it is a helper struct that does all manipulation with the storage key. It allows concat two storage keys into one, compute storage key for fields based on the filed, struct, enum, variants names.
  Removed all tests and benches. Didn't add it for new primitives because the key is standard u32, and all keys are calculated during compilation.
- storage crate:
  Removed SpreadLayout, PackedLayout, SpreadAllocate, PackedAllocate, and all related helper functions.
  Removed Packed struct cause it is the default behavior for storage right now. Added Lazy struct that allows get/set value from/into the storage. It is similar to Mapping but works with one storage key and one value.
  Introduced new main traits to work with storage in storage/src/traits/storage.rs.
  Also added a new OnCallInitializer trait to improve the flow with upgradable contracts and support initialization on demand by default. Added pull_or_init macro that inits the storage struct if it is impossible to load it. It also can be used as optimization for contracts without an explicit constructor.
  Replaced implementation of old traits for main primitives with a new one. Added blanket implementation of new traits for structures that are Packed by default. It reduces the amount of code and adds support of generic structures but adds problems with tuples(now tuples implement new traits only if inner items are Packed).
  Introduced AutoKey and ManualKey that allows specifying which key the user wants to use. Added support of it into all traits and structures.
  Refactored Mapping to follow new rules.
Third commit:
- metadata crate:
  Updated storage layout in the metadata. Introduces the concept of roots and leafs. Root defines the storage key for all sub-tree until there is another Root. Leafs are common types that are part of the sub-tree and serialized/deserialized together into one storage key.
  Replaced 32 bytes storage key with u32.
  Added validation that all root storage keys don't overlap. Maybe better to add that error or reuse that validator on the cargo-contract side to show a more user-friendly error.
  RootLayout and validator are used in codegen(next commit). The contract is wrapped into RootLayout, and we do validation for that tree.
  Metadata now contains name for each struct/enum/variant/field. It is useful information because now the storage key is calculated based on the name of struct/enum/variant/field.
- storage crate:
  Added a new helper crate storage/codegen. It contains some useful functional that is used in ink_storage_derive and in ink_lang_codegen crates. It has a method that returns all types of the struct/enum/unit and a method that finds "salt" in the generics of the struct/enum. It uses the magic constant "KeyHolder"(about that you can read in the issue) to find salt, so I tried to have only one place where we are using that constant.
- storage derive crate:
  Replaced derive implementation of old trait with a new one. You can check the tests to see what the implementation looks like. Storable recursively calls encode and decode for all fields. KeyHolder return the key of the salt. Item uses AutoKey or a key specified by the user. I want to highlight that PreferredKey only is used with the AutoItem trait. If PreferredKey is AutoKey, then AutoItem<AutoGenerated> select auto-generated key, otherwise preferred. So AutoItem trait decides which key to use. It is why derive macro only set PreferredKey.
  Updated drive for StorageLayout, now it uses u32 and passes the name of the struct into metadata.
Fourth commit removed initialize_contract and related to initialization stuff. Now the codegen uses pull_or_init in the call. Updated execute_constructor to work with new storage methods.
Allowed usage of generics during the declaration of the primary contract storage. Users can specify the default storage key with KeyHolder via generic.
Added parser for storage_item macro with its config.
Fifth commit removed the old codegen related to spread and packed layout. If some object implements Decode and Encode, it is Packed, and it uses the blanket implementation of new traits.
In dispatch, codegen started to use a new method to work with storage.
In metadata codegen added usage of new RootLayout. We wrap the contract into that layout because the contract has its storage key for all inner fields by default. Also added a run of validation logic during metadata generation.
Added storage_item macro. It transforms all types from autokey into manual key(if types support it). It calculates the storage key based on the name(it uses the KeyComposer::compute_key function from the primitives crate). Also, macro generates an additional Check structure that includes all raw fields. It helps show the user correct errors in typos, wrong types, etc.
Sixth commit updates examples to sue a new API.
Seventh commit adds new UI tests and fixes the old one.

polkadot address: 1nNaTpU9GHFvF7ZrSMu2CudQjXftR8Aqx58oMDgcuoH8dKe

Updated the API to work with the generic key `K: Encode` instead of the old `Key`. Also, the change contains optimization to reduce the size of contracts. In most cases, it is `#[inline(always)]`; but `return_value` also got small optimization; removed usage of `extract_from_slice` where it is not needed.

Removed old 32 bytes `Key`. Replaced it with `u32`. Added `KeyComposer`, it is a helper struct that does all manipulation with the storage key. It allows concat two storage keys into one, compute storage key for fields based on the filed, struct, enum, variants names. Removed all tests and benches. Didn't add it for new primitives because the key is standard `u32` and all keys are calculated during compilation. storage crate: Removed `SpreadLayout`, `PackedLayout`, `SpreadAllocate`, `PackedAllocate`, and all related helper functions. Removed `Packed` struct cause it is the default behavior for storage right now. Added `Lazy` struct that allows `get`/`set` value from/into the storage. It is similar to `Mapping` but works with one storage key and one value. Introduced new main traits to work with storage in `storage/src/traits/storage.rs`. Also added a new `OnCallInitializer` trait to improve the flow with upgradable contracts and support initialization on demand by default. Added `pull_or_init` macro that inits the storage struct if it is impossible to load it. It also can be used as optimization for contracts without an explicit constructor. Replaced implementation of old traits for main primitives with a new one. Added blanket implementation of new traits for structures that are `Packed` by default. It reduces the amount of code and adds support of generic structures but adds problems with tuples(now tuples implement new traits only if inner items are `Packed`). Introduced `AutoKey` and `ManualKey` that allows specifying which key the user wants to use. Added support of it into all traits and structures. Refactored `Mapping` to follow new rules.

Updated storage layout in the metadata. Introduces the concept of roots and leafs. Root defines the storage key for all sub-tree until there will be another Root. Leafs are common types that are part of the sub-tree and serialized/deserialized together into one storage key. Replaced 32 bytes storage key with `u32`. Added validation that all root storage keys don't overlap. Maybe better to add that error or reuse that validator on the `cargo-contract` side to show a more user-friendly error. `RootLayout` and validator are used in codegen(next commit). The contract is wrapped into `RootLayout`, and we do validation for that tree. Metadata now contains name for each struct/enum/variant/field. It is useful information because now the storage key is calculated based on the name of struct/enum/variant/field. storage crate: Added a new helper crate `storage/codegen`. It contains some useful functional that is used in `ink_storage_derive` and in `ink_lang_codegen` crates. It has a method that returns all types of the struct/enum/unit and a method that finds "salt" in the generics of the struct/enum. It uses magic constant "KeyHolder"(about that you can read in issue) to find salt, so I tried to have only one place where we are using that constant. Replaced derive implementation of old trait with new one. You can check the tests to see how the implementation looks like. `Storable` recursively call `encode` and `decode` for all fields. `KeyHolder` return key of the salt. `Item` uses `AutoKey` or key specified by the user. I want to highlight that `PreferredKey` only is used with the `AutoItem` trait. If `PreferredKey` is `AutoKey`, then `AutoItem<AutoGenerated>` select auto-generated key, otherwise preferred. So `AutoItem` trait decides that key to use. It is why derive macro only set `PreferredKey`. Updated drive for `StorageLayout`, now it uses `u32` and pass name of the struct into metadata.

…w the codegen uses `pull_or_init` in the `call`. Updated `execute_constructor` to work with new storage methods. Allowed usage of generics during the declaration of the primary contract storage. Users can specify the default storage key with `KeyHolder` via generic. Added parser for `storage_item` macro with its config.

…object implements `Decode` and `Encode`, it is `Packed`, and it uses the blanket implementation of new traits. In dispatch, codegen started to use a new method to work with storage. In metadata codegen added usage of new `RootLayout`. We wrap the contract into that layout because the contract has its storage key for all inner fields by default. Also added a run of validation logic during metadata generation. Added `storage_item` macro. It transforms all types from autokey into manual key(if types support it). It calculates the storage key based on the name(it uses the `KeyComposer::compute_key` function from the primitives crate). Also, macro generates an additional `Check` structure that includes all raw fields. It helps show correct errors to the user in case of typos, wrong types, etc.

Simplified delegate call example very well causes of new `OnCallInitializer` trait and support of manual specifying key.

Can't highlight something unusual here=)

HCastano

Yeah this is a lot easier to review!

So far I've only looked at the 1st and 6th commits (and took a peek at the expanded code), but will work through it this week

examples/upgradeable-contracts/delegate-calls/upgradeable-flipper/upgradeable.rs

crates/env/src/engine/on_chain/buffer.rs

crates/env/src/engine/on_chain/impls.rs

HCastano · 2022-07-25T21:52:00Z

crates/env/src/engine/on_chain/impls.rs

-        let mut scope = self.scoped_buffer();
-        let enc_return_value = scope.take_encoded(return_value);
-        ext::return_value(flags, enc_return_value);
+        let mut scope = EncodeScope::from(&mut self.buffer[..]);


Can we not use the append_encoded and take_appended methods from ScopedBuffer instead of using EncodeScope?

I didn't get your question=) We want to use EncodeScope because it implements scale::Output. The optimization is to do nothing with a buffer during result returning(It is the end of the execution, and we don't worry about memory validity after usage).

crates/env/src/engine/on_chain/ext.rs

crates/storage/src/traits/mod.rs

crates/storage/src/traits/impls/storage.rs

crates/storage/src/traits/storage.rs

HCastano · 2022-07-26T19:05:25Z

crates/storage/src/traits/storage.rs

+
+/// Automatically returns the type that should be used for storing the value.
+///
+/// Trait is used be codegen to use the right storage type.


"... used by codegen ..."

What does "correct" mean here?

Updated:

The trait is used by codegen to determine which storage key the type should have.

I think we should find a better name for AutoItem, something that reflects more what it's about. Maybe StorageType?

Regarding

The trait is used by codegen to determine which storage key the type should have.

The word type here is confusing to me. We don't derive the key from the type, no? If I look at fn convert_into_storage_field the type is not part of the key.

The word type here is confusing to me. We don't derive the key from the type, no? If I look at fn convert_into_storage_field the type is not part of the key.

The type defines which storage key we should use=)

// Here, the storage key is `123` let storage: Contract<ManualKey<123>> = Default::default(); // Here, the storage key is `234` let storage: Contract<ManualKey<234>> = Default::default(); // Here, the storage key is a hash of `234` and `123` let storage: Contract<ManualKey<234, ManualKey<123> /* It is Salt or ParentStorageKey */ >> = Default::default();

I think we should find a better name for AutoItem, something that reflects more what it's about. Maybe StorageType?

StorageType is the previous name of the Item=) And previously, it was AutoStorageType. I'm okay with StorageType but it doesn't highlight that it is related to automatically type determination. Item helps to determine the type too, but it also contains PreferredKey

What do you think about?:

pub trait Item<AutoGenerated: StorageKey> { type Type: Storable; type PreferredKey: StorageKey; } pub trait StorageType<AutoGenerated: StorageKey> { type Type: Storable; }

or maybe simplify to Generated

crates/storage/src/traits/storage.rs

crates/primitives/src/key.rs

HCastano · 2022-07-27T16:39:16Z

crates/primitives/src/key.rs

-                bytes[offset + 2],
-                bytes[offset + 3]
-            )?;
+use ink_prelude::vec;


We should still have some tests here imo

crates/storage/src/traits/impls/storage.rs

crates/storage/src/traits/impls/mod.rs

crates/storage/src/test_utils.rs

crates/storage/src/traits/layout/impls.rs

crates/storage/src/traits/mod.rs

crates/storage/src/traits/pull_or_init.rs

HCastano · 2022-07-28T17:48:56Z

crates/storage/src/traits/pull_or_init.rs

+pub trait PullOrInitFallback<T: Storable> {
+    #[allow(dead_code)]
+    fn pull_or_init(key: &Key) -> T {
+        pull_storage(key)
+    }
+}
+impl<T: Storable> PullOrInitFallback<T> for PullOrInit<T> {}


Is this trait really necessary? Can you not implement this on PullOrInit directly? It looks like in the codegen you go through the PullOrInit struct anyways

That trait provides a default implementation of the pull_or_init method if the contract doesn't implement the OnCallInitializer trait. The same if don't for is_result_type(IsResultTypeFallback).

It is a way how to get autoref specialization without specialization feature)

HCastano · 2022-07-28T17:50:25Z

crates/storage/src/traits/pull_or_init.rs

+/// Pulls the struct from the storage or creates and new one and inits it.
+#[macro_export]
+#[doc(hidden)]
+macro_rules! pull_or_init {
+    ( $T:ty, $key:expr $(,)? ) => {{
+        #[allow(unused_imports)]
+        use $crate::traits::pull_or_init::PullOrInitFallback as _;
+
+        $crate::traits::pull_or_init::PullOrInit::<$T>::pull_or_init(&$key)
+    }};
+}


I don't think this macro is really necessary. It's only used once it the codegen, and a couple of times in the tests

It is a separate macro with its module because of tests. But tests check that everything works as expected=)

crates/storage/src/traits/mod.rs

crates/storage/src/lazy/mod.rs

HCastano · 2022-08-02T18:58:50Z

crates/storage/src/lazy/mapping.rs

 /// }
 ///
 /// impl MyContract {
 ///     #[ink(constructor)]
 ///     pub fn new() -> Self {
-///         ink_lang::utils::initialize_contract(Self::new_init)
+///         let mut instance = Self::default();
+///         instance.new_init();


Similar thing here, I'd take the code in new_init and just fold it here. But again, if we just want to show how to use ManualKey do we even need the rest of the example?

crates/storage/src/lazy/mapping.rs

crates/storage/src/lazy/mod.rs

crates/storage/src/lazy/mapping.rs

HCastano · 2022-08-02T19:36:27Z

crates/storage/src/lazy/mapping.rs

@@ -154,7 +166,8 @@ where
    where
        Q: scale::EncodeLike<K>,
    {
-        pull_packed_root_opt(&self.storage_key(&key))
+        ink_env::get_contract_storage::<(&Key, Q), V>(&(&KeyType::KEY, key))
+            .unwrap_or_else(|error| panic!("failed to get value in mapping: {:?}", error))


Suggested change

.unwrap_or_else(|error| panic!("failed to get value in mapping: {:?}", error))

.unwrap_or_else(|error| panic!("Failed to get value in Mapping: {:?}", error))

I wonder if we can test this somehow. Maybe by clearing the underlying contract storage manually and then trying to pull. Really this shouldn't happen unless a storage migration messes up your storage layout

xgreenx

Thank you @HCastano for the review=) I hope I didn't miss any of your comments.

I applied most of your suggestions locally, updated/added comments and tests. There are several major questions regarding naming let's discuss them and after I will push all changes.

Hernando suggested renaming the KeyHolder trait into the StorageKey trait. During the previous review iteration, we decided to use the naming KeyHolder and we tried to remove all "Storage" prefixes for all entities in the ink_storage. The KeyHolder trait means that the type occupies some storage key. So StorageKey also fits well.
The trait Item<X: KeyHolder> is used to manipulate the type's storage key. By default, the type will use the storage key passed as X. I called it Salt because in the implementation underhood it is used as a salt to generate the final storage key. But it is not accurate naming for public trait and may be better to name it "Key: StorageKey" or "KeyHolder: StorageKey", "ParentKey: StorageKey" or maybe another name(remember we can select a good name for bound too).
Packed vs non-Packed: With the introduction of blanket implementation of the Packed trait, it simplified the implementation of Storable and other traits for primitives. We can simplify the previous structure as Hernando suggested. I used the old structure because we didn't decide if we wanted to have two macros, ink::packed and ink::non_packed. With two macros, maybe we don't want to have blanket implementation. Do we want to discuss it or are we okay with the current implementation and I will simplify the structure more?
All functions from ink_env use scale::Decode and scale::Encode. ink_env doesn't know about ink_storage::traits::Storable. So here I created EncodeWrapper and DecodeWrapper to transform Storable into scale::Encode or scale::Decode. And it is a hack=) I tried not to break the abstraction that all traits related to storage in the ink_storage crate. What do you think, can we move the Storable definition to ink_env and use Storable for ink_env::set_contract_storage and get_contract_storage instead of scale::Decode and scale::Encode? And where we should define derive macro for Storable in that case?

crates/env/src/engine/on_chain/buffer.rs

xgreenx · 2022-08-09T09:45:43Z

crates/env/src/engine/on_chain/impls.rs

-        let mut scope = self.scoped_buffer();
-        let enc_return_value = scope.take_encoded(return_value);
-        ext::return_value(flags, enc_return_value);
+        let mut scope = EncodeScope::from(&mut self.buffer[..]);


I didn't get your question=) We want to use EncodeScope because it implements scale::Output. The optimization is to do nothing with a buffer during result returning(It is the end of the execution, and we don't worry about memory validity after usage).

crates/storage/src/lazy/mod.rs

crates/storage/src/test_utils.rs

crates/storage/src/traits/storage.rs

xgreenx · 2022-08-09T16:20:18Z

crates/storage/src/traits/storage.rs

+///
+/// The trait is automatically implemented for [`Packed`](crate::traits::Packed) types
+/// via blank implementation.
+pub trait Item<Salt: KeyHolder> {


It was initially called Salt because it is only one part of the storage key, and in most cases, it is salt for the hash function=)

But Key or KeyHolder for the generic sounds better. Let's discuss about it in general

crates/storage/src/traits/storage.rs

xgreenx · 2022-08-09T16:26:45Z

crates/storage/src/traits/storage.rs

+
+/// Automatically returns the type that should be used for storing the value.
+///
+/// Trait is used be codegen to use the right storage type.


Updated:

The trait is used by codegen to determine which storage key the type should have.

crates/storage/src/traits/storage.rs

cmichi · 2022-08-10T16:11:15Z

examples/upgradeable-contracts/set-code-hash/updated-incrementer/Cargo.toml

@@ -10,7 +10,7 @@ publish = false
 [dependencies]
 ink_primitives = { path = "../../../../crates/primitives", default-features = false }
 ink_metadata = { path = "../../../../crates/metadata", default-features = false, features = ["derive"], optional = true }
-ink_env = { path = "../../../../crates/env", default-features = false, features = ["ink-debug"] }


I think this feature removal is unintentional.

I removed it to track the actual size of the contract

HCastano · 2022-08-11T22:25:05Z

1. Hernando suggested renaming the `KeyHolder` trait into the `StorageKey` trait. During the previous review iteration, we decided to use the naming `KeyHolder` and we tried to remove all "Storage" prefixes for all entities in the `ink_storage`. The `KeyHolder` trait means that the type occupies some storage key. So `StorageKey` also fits well.

I don't mind having the Storage prefix in this case. It makes things clearer imo.

2. The trait `Item<X: KeyHolder>` is used to manipulate the type's storage key. By default, the type will use the storage key passed as `X`. I called it `Salt` because in the implementation underhood it is used as a salt to generate the final storage key. But it is not accurate naming for public trait and may be better to name it "Key: StorageKey" or "KeyHolder: StorageKey", "ParentKey: StorageKey" or maybe another name(remember we can select a good name for bound too).

So a big part of the confusion for me is that the Salt is only used after a few layers
of trait implementations, and only really populated in one specific case.

It's not used on Item directly, and not even on AutoItem directly. Instead we use the
Salt when implemeting KeyHolder for ManualKey, and then ManualKey gets used in
the implementation of AutoItem (which internally uses Item).

I think there's room for simplification around the traits and implementations
(Get rid of AutoItem? Roll the salting into KeyHolder? Implement ManualKeys later?),
but I'm a bit stuck on figuring out what the right move is here.

3. `Packed` vs non-`Packed`: With the introduction of blanket implementation of the `Packed` trait, it simplified the implementation of `Storable` and other traits for primitives. We can simplify the previous structure as Hernando suggested. I used the old structure because we didn't decide if we wanted to have two macros, `ink::packed` and `ink::non_packed`. With two macros, maybe we don't want to have blanket implementation. Do we want to discuss it or are we okay with the current implementation and I will simplify the structure more?

I'd stick with the current implementation. We can always have a follow up PR if we decide there
is a better approach.

4. All functions from `ink_env` use `scale::Decode` and `scale::Encode`. `ink_env` doesn't know about `ink_storage::traits::Storable`. So [here](https://github.com/paritytech/ink/pull/1331#discussion_r932509661) I created `EncodeWrapper` and `DecodeWrapper` to transform `Storable` into `scale::Encode` or `scale::Decode`. And it is a hack=) I tried not to break the abstraction that all traits related to storage in the `ink_storage` crate. What do you think, can we move the `Storable` definition to `ink_env` and use `Storable` for `ink_env::set_contract_storage` and `get_contract_storage` instead of `scale::Decode` and `scale::Encode`? And where we should define derive macro for `Storable` in that case?

Haven't gotten around to this yet, I'll need to think about it.

# Conflicts: # crates/lang/macro/Cargo.toml

crates/lang/codegen/src/generator/metadata.rs

crates/lang/codegen/src/generator/storage_item.rs

crates/storage/src/traits/mod.rs

crates/storage/src/pull_or_init.rs

crates/lang/codegen/src/generator/storage_item.rs

Removed `delegate-call` example with `OnCallInitializer`

…ork-new-hope # Conflicts: # .gitlab-ci.yml

…ork-new-hope

ascjones

LGTM!

HCastano

Green, thanks so much for this 💚! It's definitely been quite an effort on both sides, and
it's about time we get this into the hands of users. There's inevitably gonna be
follow up PRs as we discover bugs, edge cases, and quirks. Hopefully we catch
all the major ones before this hits a mainnet deployment 😅

Green, can you put down either a Polkadot or Kusama address in the PR description so
we can submit a tip for you?

cmichi · 2022-09-05T21:16:12Z

@xgreenx As Hernando wrote above: we want to give you a tip for the huge amount of work you've put into this PR (and the previous one from which this one was created + the smaller ones which were split off). In total you worked on this project for over six months, starting with the discussions in #1134, specifying it out. And then over four months ago you created the first PR and met with a number of people from the core team in person two times to discuss it.

Thank you so much! We're very happy to have you be part of our ecosystem!

Could you edit your description of this PR and add either one of those?

polkadot address: <SS58 Address>

or

kusama address: <SS58 Address>

xgreenx · 2022-09-06T08:59:12Z

It's definitely been quite an effort on both sides, and
it's about time we get this into the hands of users.

Yea, we finally did it!=)

There's inevitably gonna be follow up PRs as we discover bugs,
edge cases, and quirks. Hopefully we catch all the major ones
before this hits a mainnet deployment 😅

I hope the number of problems will be as few as possible=) But definitely, we will have follow-up PRs to improve the feature, documentation, etc.

Thank you so much! We're very happy to have you be part of our ecosystem!

💚💚💚

cmichi · 2022-09-07T18:25:44Z

/tip large

substrate-tip-bot · 2022-09-07T18:25:48Z

@cmichi A large tip was successfully submitted for xgreenx (1nNaTpU9GHFvF7ZrSMu2CudQjXftR8Aqx58oMDgcuoH8dKe on polkadot).

https://polkadot.js.org/apps/?rpc=wss%3A%2F%2Frpc.polkadot.io#/treasury/tips

* Import new unstable functions with transparent hashing. Updated the API to work with the generic key `K: Encode` instead of the old `Key`. Also, the change contains optimization to reduce the size of contracts. In most cases, it is `#[inline(always)]`; but `return_value` also got small optimization; removed usage of `extract_from_slice` where it is not needed. * primitives crate: Removed old 32 bytes `Key`. Replaced it with `u32`. Added `KeyComposer`, it is a helper struct that does all manipulation with the storage key. It allows concat two storage keys into one, compute storage key for fields based on the filed, struct, enum, variants names. Removed all tests and benches. Didn't add it for new primitives because the key is standard `u32` and all keys are calculated during compilation. storage crate: Removed `SpreadLayout`, `PackedLayout`, `SpreadAllocate`, `PackedAllocate`, and all related helper functions. Removed `Packed` struct cause it is the default behavior for storage right now. Added `Lazy` struct that allows `get`/`set` value from/into the storage. It is similar to `Mapping` but works with one storage key and one value. Introduced new main traits to work with storage in `storage/src/traits/storage.rs`. Also added a new `OnCallInitializer` trait to improve the flow with upgradable contracts and support initialization on demand by default. Added `pull_or_init` macro that inits the storage struct if it is impossible to load it. It also can be used as optimization for contracts without an explicit constructor. Replaced implementation of old traits for main primitives with a new one. Added blanket implementation of new traits for structures that are `Packed` by default. It reduces the amount of code and adds support of generic structures but adds problems with tuples(now tuples implement new traits only if inner items are `Packed`). Introduced `AutoKey` and `ManualKey` that allows specifying which key the user wants to use. Added support of it into all traits and structures. Refactored `Mapping` to follow new rules. * metadata crate: Updated storage layout in the metadata. Introduces the concept of roots and leafs. Root defines the storage key for all sub-tree until there will be another Root. Leafs are common types that are part of the sub-tree and serialized/deserialized together into one storage key. Replaced 32 bytes storage key with `u32`. Added validation that all root storage keys don't overlap. Maybe better to add that error or reuse that validator on the `cargo-contract` side to show a more user-friendly error. `RootLayout` and validator are used in codegen(next commit). The contract is wrapped into `RootLayout`, and we do validation for that tree. Metadata now contains name for each struct/enum/variant/field. It is useful information because now the storage key is calculated based on the name of struct/enum/variant/field. storage crate: Added a new helper crate `storage/codegen`. It contains some useful functional that is used in `ink_storage_derive` and in `ink_lang_codegen` crates. It has a method that returns all types of the struct/enum/unit and a method that finds "salt" in the generics of the struct/enum. It uses magic constant "KeyHolder"(about that you can read in issue) to find salt, so I tried to have only one place where we are using that constant. Replaced derive implementation of old trait with new one. You can check the tests to see how the implementation looks like. `Storable` recursively call `encode` and `decode` for all fields. `KeyHolder` return key of the salt. `Item` uses `AutoKey` or key specified by the user. I want to highlight that `PreferredKey` only is used with the `AutoItem` trait. If `PreferredKey` is `AutoKey`, then `AutoItem<AutoGenerated>` select auto-generated key, otherwise preferred. So `AutoItem` trait decides that key to use. It is why derive macro only set `PreferredKey`. Updated drive for `StorageLayout`, now it uses `u32` and pass name of the struct into metadata. * Removed `initialize_contract` and related to initialization stuff. Now the codegen uses `pull_or_init` in the `call`. Updated `execute_constructor` to work with new storage methods. Allowed usage of generics during the declaration of the primary contract storage. Users can specify the default storage key with `KeyHolder` via generic. Added parser for `storage_item` macro with its config. * Removed the old codegen related to spread and packed layout. If some object implements `Decode` and `Encode`, it is `Packed`, and it uses the blanket implementation of new traits. In dispatch, codegen started to use a new method to work with storage. In metadata codegen added usage of new `RootLayout`. We wrap the contract into that layout because the contract has its storage key for all inner fields by default. Also added a run of validation logic during metadata generation. Added `storage_item` macro. It transforms all types from autokey into manual key(if types support it). It calculates the storage key based on the name(it uses the `KeyComposer::compute_key` function from the primitives crate). Also, macro generates an additional `Check` structure that includes all raw fields. It helps show correct errors to the user in case of typos, wrong types, etc. * Updated all examples to use a new API. Simplified delegate call example very well causes of new `OnCallInitializer` trait and support of manual specifying key. * UI tests for a new codegen. Can't highlight something unusual here=) * Apply all suggestion from the review * Make CI happy * Fix tests * Fix tests * Fix tests * Fix tests * Apply suggestions: - In most cases it is comments=) - Moved `pull_or_init` on one level upper. - Put the tests into the `impls/mod.rs` * Fix doc * Add comment to autoref specialisation * Suggestion from the review * Revert back u8 * Remove unwrap * Collapse if let * Fixed overflow for enums * Fixing comments * Renamed `Item` to `StorableHint` and `AutoItem` to `AutoStorableHint` * Fix test * Renamed key_holder. Add UI test for double storage_item. Applied suggestion from the review. * Nightly fmt * Remove `Packed` path * Fix doc test * Apply suggestions from hte review * Fixed build * Fix build * Removed `initialize_contract` from linting and deleted all tests * Fix doc link * Fix mapping example * Applied suggestion. Removed `delegate-call` example with `OnCallInitializer` * Removed `delegate-calls` from the CI. Replaced it with `set-code-hash` * fix test * fix test * Fix CI to use stable for contract build * Fix CI to use stable for examples

xgreenx requested review from cmichi, ascjones and HCastano as code owners July 20, 2022 17:56

xgreenx force-pushed the feature/storage-rework-new-hope branch 3 times, most recently from aa358eb to 3d969bf Compare July 20, 2022 18:39

xgreenx added 7 commits July 21, 2022 11:54

Updated all examples to use a new API.

d6db8de

Simplified delegate call example very well causes of new `OnCallInitializer` trait and support of manual specifying key.

UI tests for a new codegen.

f967986

Can't highlight something unusual here=)

xgreenx force-pushed the feature/storage-rework-new-hope branch from 3d969bf to f967986 Compare July 21, 2022 13:46

xgreenx mentioned this pull request Jul 21, 2022

Storage rework #1217

Closed

HCastano added the A-ink_storage [ink_storage] Work Item label Jul 21, 2022

HCastano reviewed Jul 25, 2022

View reviewed changes

HCastano reviewed Jul 26, 2022

View reviewed changes

xgreenx mentioned this pull request Jul 27, 2022

Delivery report for milestone one of the third grant of OpenBrush(6 milestone) w3f/Grant-Milestone-Delivery#515

Merged

5 tasks

HCastano reviewed Jul 27, 2022

View reviewed changes

cmichi mentioned this pull request Jul 28, 2022

ink! 4.0 #1343

Closed

cmichi assigned xgreenx Jul 28, 2022

HCastano reviewed Jul 28, 2022

View reviewed changes

HCastano reviewed Aug 2, 2022

View reviewed changes

xgreenx commented Aug 9, 2022

View reviewed changes

cmichi reviewed Aug 10, 2022

View reviewed changes

xgreenx added 2 commits August 15, 2022 17:41

Merge branch 'master' into feature/storage-rework-new-hope

fc3fcb0

Apply all suggestion from the review

8f511ec

xgreenx requested a review from a team as a code owner August 15, 2022 16:43

xgreenx added 3 commits August 26, 2022 11:39

Fix doc link

cad9234

Merge branch 'master' into feature/storage-rework-new-hope

7371905

# Conflicts: # crates/lang/macro/Cargo.toml

Fix mapping example

477e4cf

HCastano reviewed Aug 31, 2022

View reviewed changes

xgreenx added 8 commits August 31, 2022 21:57

Applied suggestion.

235d649

Removed `delegate-call` example with `OnCallInitializer`

Removed delegate-calls from the CI. Replaced it with set-code-hash

8b9ccbc

Merge remote-tracking branch 'parity/master' into feature/storage-rew…

a42bc9d

…ork-new-hope # Conflicts: # .gitlab-ci.yml

fix test

be1563e

fix test

300b139

Fix CI to use stable for contract build

7b8733d

Fix CI to use stable for examples

3e3fbd7

Merge remote-tracking branch 'parity/master' into feature/storage-rew…

5808157

…ork-new-hope

cmichi approved these changes Sep 5, 2022

View reviewed changes

ascjones approved these changes Sep 5, 2022

View reviewed changes

HCastano approved these changes Sep 5, 2022

View reviewed changes

xermicus approved these changes Sep 5, 2022

View reviewed changes

cmichi merged commit 5fc2d2a into use-ink:master Sep 5, 2022

xgreenx mentioned this pull request Sep 7, 2022

Proposal to initialize the contract on demand #1388

Open

ascjones mentioned this pull request Sep 8, 2022

Combine storage traits and derives into single crate #1389

Merged

This was referenced Sep 20, 2022

Release v4.0.0 alpha.2 #1408

Merged

Release v4.0.0-alpha.3 #1409

Merged

HCastano mentioned this pull request Oct 19, 2022

Macro based storage rework #1134

Closed

HCastano mentioned this pull request Nov 8, 2022

Simple struct requires SpreadLayout trait implementation #1440

Closed

ascjones mentioned this pull request Feb 15, 2023

Release v4.0.0 #1653

Merged

HCastano mentioned this pull request Feb 22, 2023

Research and implement new layout metadata format #767

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Storage refactoring new hope for review #1331

Storage refactoring new hope for review #1331

xgreenx commented Jul 20, 2022 •

edited

Loading

HCastano left a comment

HCastano Jul 25, 2022

xgreenx Aug 9, 2022

HCastano Jul 26, 2022

xgreenx Aug 9, 2022

cmichi Aug 12, 2022

xgreenx Aug 12, 2022

xgreenx Aug 12, 2022

HCastano Jul 27, 2022

HCastano Jul 28, 2022

xgreenx Aug 9, 2022

HCastano Jul 28, 2022

xgreenx Aug 9, 2022

HCastano Aug 2, 2022

HCastano Aug 2, 2022

xgreenx left a comment

xgreenx Aug 9, 2022

xgreenx Aug 9, 2022

xgreenx Aug 9, 2022

cmichi Aug 10, 2022

xgreenx Aug 12, 2022

HCastano commented Aug 11, 2022

ascjones left a comment

HCastano left a comment •

edited

Loading

cmichi commented Sep 5, 2022

xgreenx commented Sep 6, 2022

cmichi commented Sep 7, 2022

substrate-tip-bot bot commented Sep 7, 2022

	.unwrap_or_else(\|error\| panic!("failed to get value in mapping: {:?}", error))
	.unwrap_or_else(\|error\| panic!("Failed to get value in Mapping: {:?}", error))

Storage refactoring new hope for review #1331

Storage refactoring new hope for review #1331

Conversation

xgreenx commented Jul 20, 2022 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xgreenx 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

HCastano commented Aug 11, 2022

ascjones left a comment

Choose a reason for hiding this comment

HCastano left a comment • edited Loading

Choose a reason for hiding this comment

cmichi commented Sep 5, 2022

xgreenx commented Sep 6, 2022

cmichi commented Sep 7, 2022

substrate-tip-bot bot commented Sep 7, 2022

xgreenx commented Jul 20, 2022 •

edited

Loading

HCastano left a comment •

edited

Loading