Skip to content

Pallet-pools: replace Vec storage by BoundedVec#979

Merged
branan merged 2 commits intoparachainfrom
pools-bounded-vec
Sep 22, 2022
Merged

Pallet-pools: replace Vec storage by BoundedVec#979
branan merged 2 commits intoparachainfrom
pools-bounded-vec

Conversation

@lemunozm
Copy link
Contributor

@lemunozm lemunozm commented Sep 19, 2022 •
edited
Loading

Description

Replace Vec storage with BoundedVec in pallet-pools to be prepared for Weights V2

Related to: #897

Changes and Descriptions

Modify Vec types found in the pallet-pools storages.

Type of change

  • Bug fix (non-breaking change which fixes an issue). In this case, a future issue.

How Has This Been Tested?

cargo test -p pallet-pools
cargo test --workspace --release --features test-benchmarks,try-runtime
cargo run --features runtime-benchmarks benchmark pallet --pallet="pallet-pools" --chain="altair-dev" --extrinsic="*"

Checklist:

  • I have performed a self-review of my code
  • My changes generate no new warnings
  • New and existing unit tests pass locally with my changes
  • I rebased on the latest parachain branch

@lemunozm lemunozm self-assigned this Sep 19, 2022
@lemunozm lemunozm added I3-annoyance The code behaves as expected, but "expected" is an issue. D2-notify Pull request can be merged and notification about changes should be documented. crcl-runtime Circle runtime related. I7-optimisation Enhancement that provides better perfromance or smaller footprint. Q1-easy Can be done by primarily duplicating and adapting code. P0-someday-maybe Issue might be worth doing someday. and removed I3-annoyance The code behaves as expected, but "expected" is an issue. labels Sep 19, 2022
@lemunozm lemunozm requested review from branan, gruberb, hieronx and mustermeiszer and removed request for branan and gruberb September 19, 2022 12:38
lemunozm
lemunozm commented Sep 19, 2022
View reviewed changes
@lemunozm lemunozm added P2-nice-to-have Issue is worth doing. and removed P0-someday-maybe Issue might be worth doing someday. labels Sep 19, 2022
@lemunozm lemunozm mentioned this pull request Sep 19, 2022
Closed
4 tasks
NunoAlexandre
NunoAlexandre previously approved these changes Sep 19, 2022
View reviewed changes
Copy link
Contributor

@NunoAlexandre NunoAlexandre 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 but it should be approved by a pallet-pools pool owner ☑️

mustermeiszer
mustermeiszer reviewed Sep 19, 2022
View reviewed changes
Copy link
Collaborator

@mustermeiszer mustermeiszer left a comment

Choose a reason for hiding this comment

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

This looks fine.

The reason why I am not approving is that I am not sure of the bounds of vectors are checked when being deserialized from rpc.

Can anybody confirm that an extrinsic with an BoundedVec<T, S> will fail to be called when the input for the vector exceeds S?

@lemunozm lemunozm changed the title replace Vec storage by BoundedVec Pallet-pools: replace Vec storage by BoundedVec Sep 19, 2022
branan
branan suggested changes Sep 19, 2022
View reviewed changes
Copy link
Contributor

@branan branan left a comment

Choose a reason for hiding this comment

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

Very small change requested for cleanup/readability. Otherwise this LGTM.

@lemunozm
Copy link
Contributor Author

lemunozm commented Sep 19, 2022

Can anybody confirm that an extrinsic with an BoundedVec<T, S> will fail to be called when the input for the vector exceeds S?

Yes, it fails: https://paritytech.github.io/substrate/master/src/sp_core/bounded/bounded_vec.rs.html#81

@branan
Copy link
Contributor

branan commented Sep 19, 2022

@mustermeiszer BoundedVec does check its bounds on decode - https://paritytech.github.io/substrate/master/src/sp_core/bounded/bounded_vec.rs.html#279-291

mustermeiszer
mustermeiszer previously approved these changes Sep 19, 2022
View reviewed changes
Copy link
Collaborator

@mustermeiszer mustermeiszer left a comment

Choose a reason for hiding this comment

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

LGTM! Although, if you wanna adjust according to Branans proposal of creating a type for the changed structs that would be great.

@lemunozm lemunozm requested a review from branan September 20, 2022 07:14
@lemunozm lemunozm enabled auto-merge (squash) September 20, 2022 07:14
branan
branan previously approved these changes Sep 22, 2022
View reviewed changes
@branan
Copy link
Contributor

branan commented Sep 22, 2022

Removed my request for changes, so I believe once this is rebased it can be approved by anyone to get it merged now

branan
branan reviewed Sep 22, 2022
View reviewed changes
@branan branan disabled auto-merge September 22, 2022 21:34
branan
branan approved these changes Sep 22, 2022
View reviewed changes
Copy link
Contributor

@branan branan left a comment

Choose a reason for hiding this comment

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

I turned off the auto-merge because I had a nitpick, but I'm otherwise happy with this 👍

@branan
Copy link
Contributor

branan commented Sep 22, 2022

Meh, changed my mind. This has been lingering long enough, and it's easy enough to fix that type the next time we touch this. As long as clippy isn't complaining about type complexity yet it's not the end of the world

@branan branan merged commit ff695c7 into parachain Sep 22, 2022
@NunoAlexandre NunoAlexandre deleted the pools-bounded-vec branch September 23, 2022 07:21
@lemunozm
Copy link
Contributor Author

lemunozm commented Sep 23, 2022

I've seen late your last comment. Thanks for the review anyway!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gruberb gruberb Awaiting requested review from gruberb

@hieronx hieronx Awaiting requested review from hieronx

@mustermeiszer mustermeiszer Awaiting requested review from mustermeiszer

2 more reviewers

@branan branan branan approved these changes

@NunoAlexandre NunoAlexandre NunoAlexandre left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@lemunozm lemunozm

Labels

crcl-runtime Circle runtime related. D2-notify Pull request can be merged and notification about changes should be documented. I7-optimisation Enhancement that provides better perfromance or smaller footprint. P2-nice-to-have Issue is worth doing. Q1-easy Can be done by primarily duplicating and adapting code.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@lemunozm @branan @NunoAlexandre @mustermeiszer