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

Allowing to unvote without having to switch to a new vote #218

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Allowing to unvote without having to switch to a new vote #218

wants to merge 12 commits into from
+1,683 −927

Conversation

arlai-mk
Copy link
Contributor

@arlai-mk arlai-mk commented Jan 20, 2025 •
edited
Loading

Description

Closes: #211

This PR introduces a new Unvote message allowing users to remove their votes from specific tranches by specifying lock IDs. Additionally, it includes a significant refactor of the voting system to optimize storage operations and improve code organization.

Key Changes

  1. Added new Unvote message type for explicit vote removal
  2. Moved voting logic to a new vote.rs module
  3. Refactored storage operations to batch process changes

Implementation Details

New Files

  • Created vote.rs module containing:
    • process_votes: Handles new vote application, once any previous vote has been removed
    • process_unvotes: Manages vote removal
    • validate_proposals_and_locks_for_voting: Input validation prior to voting

Major Improvements

  1. Optimized Vote Processing

    • Combined proposal power changes to reduce storage operations
    • Added batch processing for validator shares updates
    • Skips unnecessary vote removals when re-voting for same proposal

  2. Enhanced Storage Operations

    • New add_many_validator_shares_to_proposal function supporting both positive and negative changes
    • Batch updates to SCALED_PROPOSAL_SHARES_MAP and PROPOSAL_TOTAL_MAP

  3. Code Organization

    • Split voting logic into separate functions for better maintainability
    • Lock validation and skipping logic is done earlier

Minor Changes

While maintaining full functional compatibility, there are minor changes to the Vote message's response attributes: locks re-voting for the same proposal are now included in the locks_skipped attribute rather than the locks_voted attribute.

Testing Notes

  • Added tests for new unvote functionality
  • All previous unit tests still pass

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • Targeted the correct branch
  • Included the necessary unit tests
  • Added/adjusted the necessary interchain tests
  • Added a changelog entry in .changelog
  • Compiled the contracts by using make compile and included content of the artifacts directory into the PR
  • Regenerated front-end schema by using make schema and included generated files into the PR
  • Updated the relevant documentation or specification
  • Reviewed "Files changed" and left comments if necessary
  • Confirmed all CI checks have passed

@arlai-mk arlai-mk force-pushed the arlai/vote-refactor-unvote branch from 4837a83 to 9a3ac9d Compare January 20, 2025 04:39
@arlai-mk arlai-mk marked this pull request as ready for review January 20, 2025 05:21
@arlai-mk arlai-mk requested a review from a team as a code owner January 20, 2025 05:21
p-offtermatt
p-offtermatt reviewed Jan 22, 2025
View reviewed changes
contracts/hydro/src/vote.rs Outdated

let lock_entry = &lock_entries[&lock_id];

// If user didn't yet vote with the given lock in the given round and tranche, check
Copy link
Member

Choose a reason for hiding this comment

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

The comment seems outdated now. We don't have an explicit check anymore for whether the user has voted in the given round/tranche; if they already voted in this round, then implicitly their old VOTING_ALLOWED_ROUND was erased during process_unvotes

p-offtermatt
p-offtermatt reviewed Jan 22, 2025
View reviewed changes
Copy link
Member

@p-offtermatt p-offtermatt 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. Since this is a bit of a bigger change that touches core logic, I want to get a second set of eyes on it, so not approving yet.

Thanks a ton for the contribution!

@p-offtermatt
Copy link
Member

The merge conflicts with main are quite small (regenerating checksums/artifacts/schema and organizing changelog).

I don't think I have permission to fix them (since the PR is from the moonkitt-lab hydro repo), could you take care of it? The only thing to pay attention to is that the merge automatically tried to put your changelog entry into 3.0.0 (it should stay in unreleased).

Thanks!

@arlai-mk
Copy link
Contributor Author

PR is fixed and should resolve conflicts. The changelog entry is kept in unreleased.

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

@p-offtermatt p-offtermatt p-offtermatt left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow "unvoting"
2 participants
@arlai-mk @p-offtermatt