This repository has been archived by the owner on Nov 15, 2023. It is now read-only.
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.
BoundedVec + Shims for Append/DecodeLength #8556
BoundedVec + Shims for Append/DecodeLength #8556
Changes from 9 commits
6440f6c
8c60a17
9fb6bb3
33f7d93
0b3c08c
aa9b839
373ff23
d2eed35
cdeb274
b3e876c
66490c3
8d4ce50
c9e4356
2bbdacb
ca43313
07417e5
007047f
678bb20
c402aa1
19b3102
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.
I guess that this is using a type number with
Get<u32>
instead of a const generic because we don't want to raise the min supported rust version to 1.51?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.
Actually it probably can, I just didn't think about it :D Will try
I am pretty sure we always require latest stable version and don't care much about older versions.
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.
errr -- no.
I don't think we can make this configurable to the end-user, as it is now, with const generics. See how it is done in #8580.
We can, only if we swap things like
type MaxProposal: Get<_>
forconst MAX_PROPOSAL: _
.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.
Right, ok. I think that in the future it may be worth a large-scale refactor to use actual consts and const generics instead of faking it with type parameters, but that may have to wait for Frame 3.0.
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.
We would already have switched to associated consts if it would be beneficial. The main advantage of using the trait is that we are much more flexible in tests. Overwriting constants would be a mess. Now we can just use thread local variables to overwrite the values.
And than there are also test runtimes where the trait enables us to read variables from the storage and thus, make it really flexible.
We would loose all this flexibility when using const generics.
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, tests are also an issue with regard to const. Either of you know what kind of shenanigans we'd need to override consts? if possible at all?
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.
We would need to make tests more generic, add new traits that provide the consts we want to override and implement X times, everytime with the const value we want. You see where this is going. This would not be easy and would just require tons of boilerplate without any advantage.
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.
why not creating
TryStorageappend
trait, and create the functionfn try_append
directly in StorageMap, StorageValue and StorageDoubleMap traits, with a where clause, similarly tofn append
.Also if you want those function to be accessible on storages without having to import trait in scope, then it should also be written in "frame/support/src/storage/types/value.rs"
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.
Sounds good, so far I tried to NOT add anything extra to the bast traits
StorageValue
andgenerator::StorageValue
, but yeah this will enable us to not need to import the extra trait everywhere.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, though eventually we'll probably want this to become a
DispatchError
to avoid a lot of.map_err(|()| Error::<T>::ItemOverflow)
s.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.
Not sure which variant it should be?
Other
or a new (general)Overflow
would be okay, but the benefit of.map_err(|()| Error::<T>::ItemOverflow)
is actually that we know from which pallet it is coming from.