Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Improve BoundedVec API #8707

Merged
4 commits merged into from
May 4, 2021
Merged

Improve BoundedVec API #8707

4 commits merged into from
May 4, 2021

Conversation

shawntabrizi
Copy link
Member

This adds some useful APIs for making the conversion from Vec to BoundedVec more seamless.

@shawntabrizi shawntabrizi requested a review from kianenigma May 1, 2021 00:38
@github-actions github-actions bot added the A0-please_review Pull request needs code review. label May 1, 2021
@shawntabrizi shawntabrizi 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. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels May 1, 2021
Copy link
Contributor

@kianenigma kianenigma 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 adding derefMut can be dangerous and add footguns.

@@ -188,11 +201,26 @@ impl<T: BoundedVecValue, S: Get<u32>> sp_std::ops::Deref for BoundedVec<T, S> {
}
}

impl<T: BoundedVecValue, S: Get<u32>> sp_std::ops::DerefMut for BoundedVec<T, S> {
Copy link
Contributor

Choose a reason for hiding this comment

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

This allows you to call methods like extend and push without checking, not a good idea.

overall, bounded vec should only coerce to vec when methods are safe regarding the bound (cannot expand and re allocate). This is why I only did deref initially.

Same comments apply to indexMut and AsRefMut: if you can expand the size with them, we can't add them and instead it should be a checked method on the type itself, or just manually convert to vec and try and convert it back.

Copy link
Member

Choose a reason for hiding this comment

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

I'm going with DerefMut, this shouldn't be implelemented as said by @kianenigma

However, IndexMut and AsMut are safe as they don't support extending the vec. Both just give you multiple access to the vec elements.

Copy link
Contributor

@gui1117 gui1117 left a comment

Choose a reason for hiding this comment

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

once we remove deref mut, then it is good to me

frame/support/src/storage/bounded_vec.rs Outdated Show resolved Hide resolved
Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>
@shawntabrizi shawntabrizi requested a review from kianenigma May 3, 2021 22:43
frame/support/src/storage/bounded_vec.rs Outdated Show resolved Hide resolved
frame/support/src/storage/bounded_vec.rs Outdated Show resolved Hide resolved
@bkchr
Copy link
Member

bkchr commented May 4, 2021

bot merge

@ghost
Copy link

ghost commented May 4, 2021

Waiting for commit status.

@ghost ghost merged commit cbd09f6 into master May 4, 2021
@ghost ghost deleted the shawntabrizi-bounded-vec-api branch May 4, 2021 09:17
nazar-pc pushed a commit to autonomys/substrate that referenced this pull request Aug 8, 2021
* improve bounded vec api

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

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

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

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

Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>
Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. 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. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants