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

Update storage docs for ink! v4 #137

Merged
merged 78 commits into from
Feb 2, 2023
Merged

Conversation

xermicus
Copy link
Contributor

@xermicus xermicus commented Jan 25, 2023

Update the storage docs for our upcoming version. The docs should give smart contract developers a better conceptual understanding and hint at some implications for contract development.

@xgreenx you're welcome to have a look as well

xermicus and others added 14 commits January 23, 2023 11:30
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Signed-off-by: xermicus <cyrill@parity.io>
Signed-off-by: xermicus <cyrill@parity.io>
Signed-off-by: xermicus <cyrill@parity.io>
Signed-off-by: xermicus <cyrill@parity.io>
Signed-off-by: xermicus <cyrill@parity.io>
Signed-off-by: xermicus <cyrill@parity.io>
Signed-off-by: xermicus <cyrill@parity.io>
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
@xermicus xermicus requested a review from athei January 25, 2023 18:13
@xermicus xermicus changed the title Update storage docs Update storage docs for ink! v4 Jan 25, 2023
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Copy link

@ivan770 ivan770 left a comment

Choose a reason for hiding this comment

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

Hi! There is a similar documentation for the new ink! storage organization available. It goes into details on how the pre-ink! 4 storage worked, comparing the changes with the new version. Perhaps we could merge some parts of it into this PR?


:::

## Manual vs. Automatic Key generation
Copy link

Choose a reason for hiding this comment

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

There is a slightly more detailed explanation on how compile-time storage keys work at the mentioned document


Beware, this page is no longer up to date for 4.0!
:::
## Using custom types on storage
Copy link

Choose a reason for hiding this comment

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

Perhaps we could merge the "Pre-ink! v4.0.0-beta storage" chapter here, to provide information about how new traits differ from the old traits?

similar to traditional hash tables and comparable to the `mapping` type Solidity offers.
As a core ingredient to the ink! language, its main advantage is being simple and
lightweight. However, it does not provide any high-level functionality, such as iteration
or automatic clean-up. Smart contract authors will need to implement any high level
Copy link
Collaborator

Choose a reason for hiding this comment

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

How do you mean "automatic clean-up" here? I'm asking because we remove the storage entry on calls of Mapping::remove and Mapping::take, which is my understanding of an automatic clean-up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm yes true, I overlooked this as I just took it on from the existing version. Need to go over this again

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It also now came to my attention that here we provide iteration as something that can be implement but later I say it is not advisable 🐒

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made an effort to document how our storage works to a full extend, please have a look again


Using `ManualKey` instead of `AutoKey` might be especially desireable for upgradable
contracts, as using `AutoKey` might result in a different storage key for the same field
in a newer version of the contract. Which will break the contract after an upgrade!
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just took a look and we actually don't use ManualKey in our ink/examples/upgradeable-contracts/ yet.

@xermicus
Copy link
Contributor Author

xermicus commented Jan 26, 2023

Hi! There is a similar documentation for the new ink! storage organization available. It goes into details on how the pre-ink! 4 storage worked, comparing the changes with the new version. Perhaps we could merge some parts of it into this PR?

Hi @ivan770

I am aware of this example being worked on, however I thought that the documentation should mainly describe how the storage works in the current version. My plan was to link this example somewhere once it gets merged. What do you think?

xermicus and others added 2 commits January 26, 2023 11:35
Co-authored-by: Michael Müller <mich@elmueller.net>
…yout.md

Co-authored-by: Michael Müller <mich@elmueller.net>
…yout.md

Co-authored-by: Michael Müller <mich@elmueller.net>
xermicus and others added 8 commits January 31, 2023 23:46
…-metadata.md

Co-authored-by: Michael Müller <mich@elmueller.net>
…yout.md

Co-authored-by: Michael Müller <mich@elmueller.net>
…-metadata.md

Co-authored-by: Michael Müller <mich@elmueller.net>
…yout.md

Co-authored-by: Michael Müller <mich@elmueller.net>
…-metadata.md

Co-authored-by: Michael Müller <mich@elmueller.net>
…yout.md

Co-authored-by: Michael Müller <mich@elmueller.net>
…-metadata.md

Co-authored-by: Michael Müller <mich@elmueller.net>
…-metadata.md

Co-authored-by: Michael Müller <mich@elmueller.net>
Copy link
Contributor

@HCastano HCastano left a comment

Choose a reason for hiding this comment

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

I think one more pass tomorrow should be it 💪

Comment on lines 10 to 15
Any custom type wanting to be compatible with ink! storage must implement the
[`Storable`](https://docs.rs/ink_storage_traits/4.0.0-beta/ink_storage_traits/trait.Storable.html)
trait, so it can be SCALE
[`encoded`](https://docs.rs/parity-scale-codec/3.2.2/parity_scale_codec/trait.Encode.html)
and
[`decoded`](https://docs.rs/parity-scale-codec/3.2.2/parity_scale_codec/trait.Decode.html).
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd leave the simple derive example for now and do a follow-up with the manual implementation. This PR is already big enough as is


A basic example of a custom struct is shown below:
Even better: there is a macro `#[ink::storage_item]`, which derives all necessary traits for you. If there is no need to implement any special behaviour, the above code example
Copy link
Contributor

Choose a reason for hiding this comment

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

Long line

```

### Calcualte the `PrefixedStorageKey` from the child trie ID
A [`PrefixedStorageKey`](https://docs.rs/sp-storage/10.0.0/sp_storage/struct.PrefixedStorageKey.html)
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we linked to the docs earlier we can get rid of this link

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch

We observe that the storage layout is represented as a tree, where tangible storage values end up
inside a `leaf`. Because of `Packed` encoding, leafs can share the same storage key, and
in order to reach them you'd need fetch and decode the whole storage cell under this key.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe not for this PR, but I'd love to see a little ASCII diagram showing the storage layout. E.g

        Root
        /   \
    Struct    Leaf
    /   \
 Leaf   Leaf

I'd find it helpful for better visualizing the storage

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea! I think our illustrator is already working on some kind of storage layout visualization

Comment on lines +48 to +53
"root": {
"layout": {
"leaf": {
"key": "0xb1f4904e",
"ty": 2
}
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 in how much detail we want to talk about this in the docs, but there are more keys than just root and leaf available to describe items in storage. E.g we can have a root key that contains an array or enum layout

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm yeah, I think it is a good point but I failed to bring up an example covering everything that is also small and easy to digest...

Comment on lines 39 to 44
On the other hand, this can get problematic if we're storing a large `ink::prelude::vec::Vec`
in the contract storage but provide messages that do not need to read and write from this
`ink::prelude::vec::Vec`. In that scenario, each and every contract message bears runtime
overhead by dealing with that `ink::prelude::vec::Vec`, regardless whether they access it or
not. This results in extra gas costs. To solve this problem we need to turn our storage
into a non-packed layout somehow.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd only use the full import path once and then refer to it as Vec in later parts where its clear from the context that we're referring to the same Vec

xermicus and others added 2 commits February 1, 2023 10:19
Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>
Copy link
Collaborator

@xgreenx xgreenx left a comment

Choose a reason for hiding this comment

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

Looks good to me, thank you for hard work!=)

I only found several [nit]s. Others are covered by @HCastano comments=)

Comment on lines 10 to 15
Any custom type wanting to be compatible with ink! storage must implement the
[`Storable`](https://docs.rs/ink_storage_traits/4.0.0-beta/ink_storage_traits/trait.Storable.html)
trait, so it can be SCALE
[`encoded`](https://docs.rs/parity-scale-codec/3.2.2/parity_scale_codec/trait.Encode.html)
and
[`decoded`](https://docs.rs/parity-scale-codec/3.2.2/parity_scale_codec/trait.Decode.html).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I didn't mean to describe it fully here, only to mention that it has 3 variants of the implementation=)

I guess in the future we need to add something like "More detailed about it [here](And here link to Artem's&Ivan's documentation)"

Comment on lines 121 to 123
Circumventing this restriction by storing populated keys inside an `ink_prelude::Vec` might
not always be advisable: As accessing a storage cell is relatively expensive, this might
result in very high gas costs for large mappings.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't like it here, because it is a big topic. If you use Vec, and you have a lot of keys, it will break your contract, it will not fit into the decode buffer. We don't need to propose a solution to the iterator problem.

Also, we plan to support iterating over elements of Mapping, because of blake2_128_concat, in the future.

I would prefer to keep only the line "you can't iterate over the mapping". Or "you can't iterate over the mapping, but it is possible to implement this behaviour in another way [](link to the article/section/discussion about it)"

Signed-off-by: xermicus <cyrill@parity.io>
Signed-off-by: xermicus <cyrill@parity.io>
or to serve as the base key for calculating the actual keys needed to access values in
non-`Packed` fields (such as `Mapping`s).
Layouts under a root key can contain `leaf`s directly, but also `struct`, `enum` or `array`
layout, which are used
Copy link
Collaborator

@cmichi cmichi Feb 1, 2023

Choose a reason for hiding this comment

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

The sentence cut-off here is strange.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This wasn't supposed to go in 🙈

Signed-off-by: xermicus <cyrill@parity.io>

A basic example of a custom struct is shown below:
Even better: there is a macro `#[ink::storage_item]`, which derives all necessary traits for
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please add some notes about Packed/Non-Packed values because it is important to understand the context when this is useful to use?

Copy link
Contributor Author

@xermicus xermicus Feb 1, 2023

Choose a reason for hiding this comment

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

I'm not sure I understand, there's a whole chapter Packed vs Non-Packed layout in the storage layout docs explaining just that?

Copy link
Contributor

Choose a reason for hiding this comment

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

True, so just add a few sentences that this attribute is preferred to be used for non-packed values:

From docs

If the type is non-packed it is best to rely on automatic storage key calculation via ink::storage_item

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I see, thanks. Unfortunately the macro docs do not say why that it is the case, so I don't know what to write here. However I added this to be a link to the docs of the macro

Copy link
Collaborator

Choose a reason for hiding this comment

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

The #[ink::storage_item] macro is responsible for the storage key calculation for the non-Packed type with AutoKey. Without it, the key for non-Packed fields will be zero. I would say that the usage of this macro is mandatory if you don't plan to use ManualKey.

If you want to have your own implementation of the ink! storage traits for the non-Packed type, it still needs to use this macro only for the key calculation with #[ink::storage_item(derive = false)].

If your type can be Packed, then you need to avoid usage of this macro and derive scale::Encode and scale::Decode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @xgreenx, I added it as a note

Signed-off-by: xermicus <cyrill@parity.io>
Copy link
Contributor

@HCastano HCastano left a comment

Choose a reason for hiding this comment

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

Thanks! This is really useful work 😃

@xermicus xermicus merged commit 3211276 into use-ink:master Feb 2, 2023
@xermicus xermicus deleted the cl/storage-docs branch February 2, 2023 11:15
@athei
Copy link
Collaborator

athei commented Feb 2, 2023

Absolute legend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants