Storage refactoring new hope for review

[xgreenx]

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:

1. 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.

2. 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.

3. 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.

4. 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.

5. 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.

6. Sixth commit updates examples to sue a new API.

7. Seventh commit adds new UI tests and fixes the old one.

[HCastano]

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.

[paritytech-cicd-pr]

🦑 📈 ink! Example Contracts ‒ Changes Report 📉 🦑

These are the results when building the examples/* contracts from this branch with cargo-contract 1.5.0-792e4f3 and comparing them to ink! master:

	Δ Optimized Size	Δ Used Gas	Total Optimized Size	Total Used Gas
accumulator	+0.12 K		1.12 K
adder	-0.34 K		1.69 K
contract-terminate	+0.29 K		1.21 K	275_000
contract-transfer	+0.11 K		8.47 K	75_000
delegate-calls	-0.76 K	-255	2.12 K	75_987
delegator	-0.60 K	-671	5.74 K	231_467
dns	-1.01 K		7.80 K	225_000
erc1155	-1.38 K		15.77 K	450_000
erc20	-1.29 K		7.13 K	225_000
erc721	-0.88 K		10.74 K	600_000
flipper	+0.11 K		1.34 K	75_000
forward-calls	-0.49 K	-201	2.38 K	151_211
incrementer	+0.09 K		1.23 K
mother	-3.18 K		9.06 K
multisig	-2.52 K	-2_060	23.27 K	468_423
payment-channel	-2.12 K		5.82 K
rand-extension	-1.02 K		2.77 K	75_000
set-code-hash	+0.02 K		1.51 K
subber	-0.35 K		1.71 K
trait-erc20	-1.23 K		7.46 K	225_000
trait-flipper	+0.16 K		1.13 K	75_000
trait-incrementer	+0.15 K		1.27 K	150_000
updated-incrementer	-8.27 K		1.51 K
upgradeable-flipper	-0.19 K		1.29 K

Link to the run | Last update: Mon Aug 22 13:13:59 CEST 2022

[codecov-commenter]

Codecov Report

Merging #1331 (5808157) into master (078d1f5) will decrease coverage by 1.52%. The diff coverage is 78.86%.

@@            Coverage Diff             @@
##           master    #1331      +/-   ##
==========================================
- Coverage   71.80%   70.27%   -1.53%     
==========================================
  Files         177      190      +13     
  Lines        5941     5942       +1     
==========================================
- Hits         4266     4176      -90     
- Misses       1675     1766      +91     

Impacted Files	Coverage Δ
crates/env/src/backend.rs	83.33% <ø> (ø)
...odegen/src/generator/as_dependency/call_builder.rs	100.00% <ø> (ø)
...odegen/src/generator/as_dependency/contract_ref.rs	100.00% <ø> (ø)
crates/lang/codegen/src/generator/dispatch.rs	94.81% <ø> (ø)
crates/lang/codegen/src/generator/mod.rs	100.00% <ø> (ø)
crates/lang/codegen/src/generator/selector.rs	100.00% <ø> (ø)
...ng/codegen/src/generator/trait_def/call_builder.rs	98.30% <ø> (-0.25%)	:arrow_down:
.../codegen/src/generator/trait_def/call_forwarder.rs	100.00% <ø> (ø)
crates/lang/codegen/src/lib.rs	100.00% <ø> (ø)
crates/lang/ir/src/ir/trait_def/config.rs	56.25% <0.00%> (ø)
... and 61 more

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

[xgreenx]

Yeah, I also don't like it, but it seems it is a problem for all chain extensions=( Right now, we use the same version as you, but maybe you want to change it in the future

[xgreenx]

We can't use a separate struct for Key because, in that case, we can't use const KEY: Key.

It is what makes the types unique. The type declaration contains the storage key as part of the type Mapping<_, _, ManualKey<123>>.

[cmichi]

@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 https://github.com/paritytech/ink/issues/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]

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]

/tip large

[substrate-tip-bot[bot]]

@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