Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add method removeVoteForTrack to ConvictionVoting precompile #2201

Merged
merged 6 commits into from Apr 11, 2023
Merged

Add method removeVoteForTrack to ConvictionVoting precompile #2201

merged 6 commits into from Apr 11, 2023

Conversation

4meta5
Copy link
Contributor

@4meta5 4meta5 commented Mar 31, 2023 •
edited

Allows users of the ConvictionVoting precompile to specify class: Some(trackId) for removeVote extrinsic. Previously, the removeVote precompile method assumed class: None, which led to unexpected errors. The new precompile method removeVoteForTrack allows users to specify trackId for substrate removeVote extrinsic.

@4meta5 4meta5 added A0-pleasereview Pull request needs code review. B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes D3-trivial PR contains trivial changes in a runtime directory that do not require an audit breaking Needs to be mentioned in breaking changes labels Mar 31, 2023
@4meta5 4meta5 requested a review from notlesh March 31, 2023 23:29
@4meta5 4meta5 added A3-inprogress Pull request is in progress. No review needed at this stage. and removed A0-pleasereview Pull request needs code review. labels Mar 31, 2023
@4meta5 4meta5 removed the request for review from notlesh March 31, 2023 23:33
@4meta5
Copy link
Contributor Author

4meta5 commented Mar 31, 2023 •
edited

This PR needs some work before it is ready for review.

  • Need to refactor common code between removeVote and removeSomeVote into a helper function. They are very similar.

  • Also need to add a new log for removeSomeVote. Every log has a specific structure so we can't make SELECTOR_LOG_VOTE_REMOVED take 2 different structures with/without trackId (as is currently).

@4meta5 4meta5 added A0-pleasereview Pull request needs code review. and removed A3-inprogress Pull request is in progress. No review needed at this stage. labels Apr 1, 2023
@4meta5 4meta5 requested a review from notlesh April 1, 2023 20:18
@4meta5 4meta5 added A3-inprogress Pull request is in progress. No review needed at this stage. and removed A0-pleasereview Pull request needs code review. labels Apr 1, 2023
@4meta5 4meta5 added A0-pleasereview Pull request needs code review. and removed A3-inprogress Pull request is in progress. No review needed at this stage. labels Apr 1, 2023
@4meta5 4meta5 changed the title [MOON-2307] Precompile method conviction_voting::removeSomeVote [MOON-2307] Precompile method conviction_voting::removeVoteForTrack Apr 1, 2023
@crystalin crystalin added D9-needsaudit👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited and removed D3-trivial PR contains trivial changes in a runtime directory that do not require an audit labels Apr 11, 2023
@librelois librelois mentioned this pull request Apr 11, 2023
Closed
20 tasks
notlesh
notlesh reviewed Apr 11, 2023
View reviewed changes
precompiles/conviction-voting/ConvictionVoting.sol
Comment on lines 80 to +87
/// @param pollIndex Index of the poll
function removeVote(uint32 pollIndex) external;

/// @dev Remove vote in poll for track
/// @custom:selector cc3aee1a
/// @param pollIndex Index of the poll
/// @param trackId Id of the track
function removeVoteForTrack(uint32 pollIndex, uint16 trackId) external;
Copy link
Contributor

Choose a reason for hiding this comment

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

A note about when we need to specify track could be useful. Alternatively, maybe removing removeVote() altogether would make this more concise.

notlesh
notlesh approved these changes Apr 11, 2023
View reviewed changes
Copy link
Contributor

@notlesh notlesh left a comment

Choose a reason for hiding this comment

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

LGTM, seems thorough. A test to cover abstaining could be useful.

@crystalin crystalin merged commit b3f9d9f into master Apr 11, 2023
18 checks passed
@crystalin crystalin deleted the amar-fix-remove-vote branch April 11, 2023 19:33
@crystalin crystalin added not-breaking Does not need to be mentioned in breaking changes and removed breaking Needs to be mentioned in breaking changes labels Apr 12, 2023
@crystalin crystalin changed the title [MOON-2307] Precompile method conviction_voting::removeVoteForTrack Add method removeVoteForTrack to ConvictionVoting precompile Apr 13, 2023
@notlesh notlesh added D1-audited👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited and removed D9-needsaudit👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Apr 26, 2023
imstar15 pushed a commit to OAK-Foundation/moonbeam that referenced this pull request May 16, 2023
@4meta5 @crystalin @imstar15
[MOON-2307] Precompile method conviction_voting::removeVoteForTrack (m…
8929b3d 
…oonbeam-foundation#2201)

* add remove some vote to conviction voting precompile

* fix and add pallet unit test for log

* rename fix logs

* Adds bunch of ts-tests

* Adds test for expired proposals

---------

Co-authored-by: crystalin <alan.sapede@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@notlesh notlesh notlesh approved these changes

Assignees
No one assigned
Labels
A0-pleasereview Pull request needs code review. B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes D1-audited👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited not-breaking Does not need to be mentioned in breaking changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@4meta5 @notlesh @crystalin