BoundedVec + Shims for Append/DecodeLength #8556

kianenigma · 2021-04-07T10:54:21Z

frame/support/src/storage/mod.rs

gavofyork · 2021-04-07T12:25:35Z

frame/support/src/storage/mod.rs

+impl<T: ValueType, S: Get<usize>, V: generator::StorageValue<BoundedVec<T, S>>> TryAppend<T, S>
+	for V
+{
+	fn try_append<LikeT: EncodeLike<T>>(item: LikeT) -> Result<(), ()> {


yeah, though eventually we'll probably want this to become a DispatchError to avoid a lot of .map_err(|()| Error::<T>::ItemOverflow)s.

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.

apopiak

cool addition!

frame/support/src/storage/mod.rs

…nded-vec

frame/support/src/storage/mod.rs

…nded-vec

frame/example/src/lib.rs

frame/support/src/storage/mod.rs

coriolinus · 2021-04-12T08:02:36Z

frame/support/src/storage/mod.rs

+/// As the name suggests, the length of the queue is always bounded. All internal operations ensure
+/// this bound is respected.
+#[derive(Encode, Decode, crate::DefaultNoBound, crate::CloneNoBound, crate::RuntimeDebugNoBound)]
+pub struct BoundedVec<T: Value, S: Get<u32>>(Vec<T>, sp_std::marker::PhantomData<S>);


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?

Actually it probably can, I just didn't think about it :D Will try

because we don't want to raise the min supported rust version to 1.51?

I am pretty sure we always require latest stable version and don't care much about older versions.

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<_> for const MAX_PROPOSAL: _.

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.

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.

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?

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.

frame/support/src/storage/mod.rs

gui1117 · 2021-04-12T10:35:52Z

frame/support/src/storage/mod.rs

+/// Storage value that is *maybe* capable of [`StorageAppend`].
+pub trait TryAppendValue<T: Value, S: Get<u32>> {
+	fn try_append<LikeT: EncodeLike<T>>(item: LikeT) -> Result<(), ()>;
+}


why not creating TryStorageappend trait, and create the function fn try_append directly in StorageMap, StorageValue and StorageDoubleMap traits, with a where clause, similarly to fn 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"

Sounds good, so far I tried to NOT add anything extra to the bast traits StorageValue and generator::StorageValue, but yeah this will enable us to not need to import the extra trait everywhere.

gui1117 · 2021-04-13T11:05:48Z

having fn try_append inside the trait StorageMap/StorageValue/StorageDoubeMap, similarly to fn append can be good but is not necessary to me (in the past I liked to have everything in those traits so that people don't need to import to much trait in scope. but with frameV2 we don't need to import any trait to access method on storage, thus I'm fine anyway).

But I would like that we implement try_append on the structs StorageValue, StorageMap, and StorageDoubleMap (in frame/support/src/storage/types/) so that people can use these method without importing any trait.

(Apart from that we can improve to not require the value to be Eq, PartialEq, and Default, but this can be done later when needed.)

But otherwise it looks good to me

* Example with balances * Fix tests * Make it indexable * fix * Fix tests * fix test * Fix collective as well * Fix test

Co-authored-by: Peter Goodspeed-Niklaus <coriolinus@users.noreply.github.com>

kianenigma · 2021-04-14T05:49:15Z

having fn try_append inside the trait StorageMap/StorageValue/StorageDoubeMap, similarly to fn append can be good but is not necessary to me (in the past I liked to have everything in those traits so that people don't need to import to much trait in scope. but with frameV2 we don't need to import any trait to access method on storage, thus I'm fine anyway).

For now I kept them in separate traits that get a blanket implementation. If and when try_append becomes more useful, and we are sure that it should be a fundamental part of storage, we can mix the traits.

But I would like that we implement try_append on the structs StorageValue, StorageMap, and StorageDoubleMap (in frame/support/src/storage/types/) so that people can use these method without importing any trait.

Done.

(Apart from that we can improve to not require the value to be Eq, PartialEq, and Default, but this can be done later when needed.)

Also done.

I also merged the upstream PR, so now this PR contains examples for balances and proposals.
nonetheless, this is all fine since the encoding of Vec<T> and BoundedVec<T, _> is the same.
I repotted everything related to bounded_vec to a new file, but nothing changes since previous reviews.

gui1117

looks good to me

frame/support/src/storage/bounded_vec.rs

Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>

apopiak

some nits

frame/collective/src/lib.rs

frame/support/src/storage/bounded_vec.rs

…o kiz-bounded-vec

shawntabrizi

looks like a great start :)

Need an issue to migrate existing vecs into the boundedvec

…nded-vec

kianenigma · 2021-04-16T05:39:24Z

bot merge

ghost · 2021-04-16T05:39:31Z

Waiting for commit status.

paritytech/substrate#8556

prototype for shawn

6440f6c

shawntabrizi reviewed Apr 7, 2021

View reviewed changes

frame/support/src/storage/mod.rs Outdated Show resolved Hide resolved

gavofyork reviewed Apr 7, 2021

View reviewed changes

Clean and document it

8c60a17

kianenigma marked this pull request as ready for review April 8, 2021 08:15

kianenigma requested review from gavofyork and shawntabrizi April 8, 2021 08:15

github-actions bot added the A0-please_review Pull request needs code review. label Apr 8, 2021

kianenigma added B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Apr 8, 2021

Add more docs

9fb6bb3

apopiak reviewed Apr 8, 2021

View reviewed changes

frame/support/src/storage/mod.rs Outdated Show resolved Hide resolved

Move imports

33f7d93

shawntabrizi added the D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. label Apr 8, 2021

kianenigma added 2 commits April 9, 2021 09:57

Merge branch 'master' of github.com:paritytech/substrate into kiz-bou…

0b3c08c

…nded-vec

Some changes for easier compat.

aa9b839

This was referenced Apr 9, 2021

BoundedVec for AccountLocks #8580

Merged

pallet-simple-staking paritytech/statemint#28

Merged

bkchr reviewed Apr 9, 2021

View reviewed changes

frame/support/src/storage/mod.rs Outdated Show resolved Hide resolved

Merge branch 'master' of github.com:paritytech/substrate into kiz-bou…

373ff23

…nded-vec

coriolinus reviewed Apr 12, 2021

View reviewed changes

frame/example/src/lib.rs Outdated Show resolved Hide resolved

kianenigma added 2 commits April 12, 2021 09:47

revert exmaple pallet

d2eed35

rename

cdeb274

coriolinus reviewed Apr 12, 2021

View reviewed changes

frame/support/src/storage/mod.rs Outdated Show resolved Hide resolved

coriolinus reviewed Apr 12, 2021

View reviewed changes

frame/support/src/storage/mod.rs Outdated Show resolved Hide resolved

gui1117 reviewed Apr 12, 2021

View reviewed changes

kianenigma and others added 2 commits April 13, 2021 12:48

BoundedVec for AccountLocks (#8580)

b3e876c

* Example with balances * Fix tests * Make it indexable * fix * Fix tests * fix test * Fix collective as well * Fix test

Update frame/support/src/storage/mod.rs

66490c3

Co-authored-by: Peter Goodspeed-Niklaus <coriolinus@users.noreply.github.com>

kianenigma added 4 commits April 14, 2021 07:33

Repot and add for value

8d4ce50

Add for map and double map

c9e4356

Master.into()

2bbdacb

Final touches.

ca43313

gui1117 approved these changes Apr 14, 2021

View reviewed changes

frame/support/src/storage/bounded_vec.rs Outdated Show resolved Hide resolved

Update frame/support/src/storage/bounded_vec.rs

07417e5

Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>

apopiak approved these changes Apr 14, 2021

View reviewed changes

frame/collective/src/lib.rs Show resolved Hide resolved

frame/support/src/storage/bounded_vec.rs Outdated Show resolved Hide resolved

frame/support/src/storage/bounded_vec.rs Show resolved Hide resolved

bkchr reviewed Apr 14, 2021

View reviewed changes

frame/support/src/storage/bounded_vec.rs Show resolved Hide resolved

kianenigma added 3 commits April 15, 2021 09:07

Add a few more tests

007047f

Add import

678bb20

Merge branch 'kiz-bounded-vec' of github.com:paritytech/substrate int…

c402aa1

…o kiz-bounded-vec

shawntabrizi approved these changes Apr 15, 2021

View reviewed changes

Merge branch 'master' of github.com:paritytech/substrate into kiz-bou…

19b3102

…nded-vec

ghost merged commit 8c4e296 into master Apr 16, 2021

ghost deleted the kiz-bounded-vec branch April 16, 2021 06:06

shawntabrizi mentioned this pull request Aug 24, 2023

[FRAME Core] Remove without_storage_info on pallets paritytech/polkadot-sdk#323

Open

40 tasks

jacogr mentioned this pull request Apr 20, 2021

Add support for BoundedVec polkadot-js/api#3415

Merged

shawntabrizi mentioned this pull request Apr 21, 2021

PoV Benchmarking Tracking Issue paritytech/polkadot-sdk#398

Open

9 tasks

arjanz added a commit to polkascan/py-scale-codec that referenced this pull request May 3, 2021

Added BoundedVec support

bb18a33

paritytech/substrate#8556

jakoblell added D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited and removed D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. labels May 31, 2021

rajesh-nodle mentioned this pull request Jun 7, 2022

Looking for some support on - Remove without_storage_info on pallets & migration to BoundedVec ... #11615

Closed

2 tasks

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BoundedVec + Shims for Append/DecodeLength #8556

BoundedVec + Shims for Append/DecodeLength #8556

kianenigma commented Apr 7, 2021 •

edited

Loading

gavofyork Apr 7, 2021

kianenigma Apr 8, 2021

apopiak left a comment

coriolinus Apr 12, 2021

kianenigma Apr 12, 2021

kianenigma Apr 12, 2021 •

edited

Loading

coriolinus Apr 12, 2021

bkchr Apr 12, 2021

kianenigma Apr 13, 2021

bkchr Apr 14, 2021

gui1117 Apr 12, 2021

kianenigma Apr 13, 2021

gui1117 commented Apr 13, 2021

kianenigma commented Apr 14, 2021 •

edited

Loading

gui1117 left a comment

apopiak left a comment

shawntabrizi left a comment

kianenigma commented Apr 16, 2021

ghost commented Apr 16, 2021

BoundedVec + Shims for Append/DecodeLength #8556

BoundedVec + Shims for Append/DecodeLength #8556

Conversation

kianenigma commented Apr 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apopiak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kianenigma Apr 12, 2021 • edited Loading

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

gui1117 commented Apr 13, 2021

kianenigma commented Apr 14, 2021 • edited Loading

gui1117 left a comment

Choose a reason for hiding this comment

apopiak left a comment

Choose a reason for hiding this comment

shawntabrizi left a comment

Choose a reason for hiding this comment

kianenigma commented Apr 16, 2021

ghost commented Apr 16, 2021

kianenigma commented Apr 7, 2021 •

edited

Loading

kianenigma Apr 12, 2021 •

edited

Loading

kianenigma commented Apr 14, 2021 •

edited

Loading