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

Bounded for Sudo Test and Tips #11596

Closed
wants to merge 10 commits into from
Closed

Bounded for Sudo Test and Tips #11596

wants to merge 10 commits into from

Conversation

shawntabrizi
Copy link
Member

This PR removes without_storage_info from Sudo (only use in tests), and the Tips Pallet.

@github-actions github-actions bot added the A0-please_review Pull request needs code review. label Jun 5, 2022
shawntabrizi
shawntabrizi commented Jun 5, 2022
View reviewed changes
frame/tips/src/lib.rs
@@ -103,7 +108,7 @@ pub struct OpenTip<
/// scheduled.
closes: Option<BlockNumber>,
/// The members who have voted for this tip. Sorted by AccountId.
tips: Vec<(AccountId, Balance)>,
tips: BoundedVec<(AccountId, Balance), LengthBoundMaximum<Tippers>>,
Copy link
Member Author

Choose a reason for hiding this comment

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

Note that I am concerned here about having this BoundedVec controlled by Tippers, which is a constant used in a different pallet. Chances to make mistakes here could be high.

Copy link
Member

Choose a reason for hiding this comment

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

I dont see where else this is used

Copy link
Member Author

Choose a reason for hiding this comment

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

not sure what your response means. I am worried that this tips pallet has a bounded vec whose limit is the maximum length of the Tippers. Tippers are defined in the council pallet, which can change in size, potentially smaller.

I wonder if someone will catch that if they reduce the council size, it may corrupt existing state in the tips pallet.

Copy link
Member

@ggwpez ggwpez Jun 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Okay, so the tight coupling here could cause issues.
A test could help with usability, checking that LengthBoundMaximum<Tippers>::get() has a specific value, but does not solve the problem.
Then a developer would at least spot the broken test and know that something is wrong.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, it sounds to me the proper way to fix this is to run a migration whenever the council is reduced in size -- it actually makes sense to remove the vote on the tips whenever the council is reduced in size and hence someone is evicted from being a councilor; it doesn't make sense to still count their vote afterwards.

Copy link
Member

@ggwpez ggwpez Oct 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

A migration makes sense, but it is difficult to see that only because you change the DesiredMembers of the elections provider, you suddenly need a migration in the Tips pallet…
Some metadata diff tooling could notice this though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Huh, perhaps then elections provider should provide some lifecycle hook functions so that other pallets can listen in onto the council election event? Sounds like something that would fit this situation without undergoing a migration.

@shawntabrizi shawntabrizi added B7-runtimenoteworthy C1-low PR touches the given topic and has a low impact on builders. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Jun 5, 2022
@shawntabrizi shawntabrizi mentioned this pull request Aug 24, 2023
Open
40 tasks
ggwpez
ggwpez reviewed Jun 5, 2022
View reviewed changes
frame/tips/src/lib.rs Outdated Show resolved Hide resolved
frame/support/src/traits/members.rs Show resolved Hide resolved
@stale
Copy link

stale bot commented Jul 21, 2022

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Jul 21, 2022
@KiChjang KiChjang removed the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Jul 22, 2022
@KiChjang
Copy link
Contributor

Still worth having this.

@stale
Copy link

stale bot commented Aug 31, 2022

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Aug 31, 2022
@stale stale bot closed this Sep 14, 2022
@ggwpez ggwpez reopened this Sep 14, 2022
@stale stale bot removed the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Sep 14, 2022
@stale
Copy link

stale bot commented Oct 14, 2022

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. and removed A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. labels Oct 14, 2022
@stale
Copy link

stale bot commented Nov 24, 2022

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Nov 24, 2022
@stale stale bot closed this Dec 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@KiChjang KiChjang KiChjang left review comments

@ggwpez ggwpez ggwpez approved these changes

@kianenigma kianenigma Awaiting requested review from kianenigma

@athei athei Awaiting requested review from athei

@acatangiu acatangiu Awaiting requested review from acatangiu

@andresilva andresilva Awaiting requested review from andresilva

Assignees
No one assigned
Labels
A0-please_review Pull request needs code review. A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. C1-low PR touches the given topic and has a low impact on builders. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed.
Projects
Runtime / FRAME
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@shawntabrizi @KiChjang @ggwpez @hbulgarini