Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Update storage docs for ink! v4 #137
Changes from 43 commits
33cbdf7
bac0ba0
fc0a0b9
e83dcc3
daf100a
a985a0b
bbf7610
ee9fa22
1ef577b
270f826
07398fb
16d86db
037c35b
009a0d1
fc05550
c873ff0
d39bf54
a2d4880
c5ee9db
14d4b8a
d23196d
26ffbb3
b75865e
601cb69
278b18d
37dd49a
855c005
3889490
55ca0a9
0d06b29
3be2041
de68341
ed67c09
6a86095
3282161
a551e16
8dbce95
055a411
5685d84
76bc8ae
852607a
d1bf3a8
9832d47
54954a5
e19ead9
69544a3
a4edead
125b4e0
de865b3
475082c
0c59044
9448a2e
7c0826f
ad310b3
213762d
c61c021
9b07b1f
f8fca53
8f6fbe4
9464011
78a0e29
4cdf361
a5998c4
12f2884
f6fb885
3447c94
f1d2eac
53acaea
e02fab3
ac6d467
3418267
8e0891d
6942184
04fd497
e22dab2
ce6bb9f
0eb238a
5481b4d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to highlight how the developer can implement this trait.
Storable
.Encode
andDecode
.Also, maybe it would be nice to add an example for each case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll look into that 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this already well covered by the example Artem is working on? I'd prefer to keep that section here short, and instead link his example (and extend his example if anything is missing). WDYT?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a sentence to mention manual implementation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Long line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd also link to the
storage_item
docs hereThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Link to those docs pls
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this example, very nice=)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also add a quick mention of why this is bad? A new dev may not make the connection between code size and cost/performance
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, maybe it is better not to mention it here or create a separate section to describe how iterable storage could be implemented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, we don't have
ink_prelude
anymore. You should useink::prelude::vec::Vec
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see the point, but I don't think this is something that fits into the documentation. I just saw fit to mention that as a warning here (it might not be obvious to anyone that loading from storage causes an API call each time and this can get costly).
There was a problem hiding this comment.
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 ofblake2_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)"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not totally true. All types from the
ink::prelude
are packed, which means we need fully load them into memory. The purpose of theMapping
is to allow storing a lot of values=) And because we don't load all entries of the mapping into memory, we can't provide some advanced API.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I see your point. But currently, we only provide a friction of the API in comparison with the stdlib HashMap. I'll try to rephrase this sentence :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually a very good point that was missing before. I added it as well 💯