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

feat(contracts): payout strategy #353

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat(contracts): payout strategy #353

wants to merge 1 commit into from

Conversation

0xmad
Copy link
Collaborator

@0xmad 0xmad commented Sep 23, 2024 •
edited
Loading

  • Use tally as payout strategy
  • Add deposit/claim/withdraw functions
  • Add tests
  • Add pause for tally in poll deploy script
  • Update deploy config
  • Add tally initialization
  • Add batch method for rewards calculation

Next things to do:

  1. Subgraph integration

Related to #263

@0xmad 0xmad added the enhancement New feature or request label Sep 23, 2024
@0xmad 0xmad self-assigned this Sep 23, 2024
@vercel Vercel
Copy link

vercel bot commented Sep 23, 2024 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
maci-platform ❌ Failed (Inspect) Oct 25, 2024 3:27pm

@0xmad 0xmad force-pushed the feature/payout-strategy branch 2 times, most recently from 539d403 to f8b6297 Compare September 23, 2024 20:52
github-advanced-security[bot]
github-advanced-security bot found potential problems Sep 26, 2024
View reviewed changes
packages/contracts/contracts/maci/Tally.sol Dismissed Show dismissed Hide dismissed
packages/contracts/contracts/maci/Tally.sol Fixed Show fixed Hide fixed
packages/contracts/contracts/maci/Tally.sol Fixed Show fixed Hide fixed
packages/contracts/contracts/maci/Tally.sol Fixed Show fixed Hide fixed
packages/contracts/contracts/maci/Tally.sol Fixed Show fixed Hide fixed
packages/contracts/contracts/maci/Tally.sol Dismissed Show dismissed Hide dismissed
packages/contracts/contracts/maci/Tally.sol Dismissed Show dismissed Hide dismissed
packages/contracts/contracts/maci/Tally.sol Dismissed Show dismissed Hide dismissed
packages/contracts/contracts/maci/Tally.sol Fixed Show fixed Hide fixed
packages/contracts/contracts/maci/TallyFactory.sol Fixed Show fixed Hide fixed
github-advanced-security[bot]
github-advanced-security bot found potential problems Oct 3, 2024
View reviewed changes
packages/contracts/contracts/maci/Tally.sol Fixed Show fixed Hide fixed
packages/contracts/contracts/maci/Tally.sol Dismissed Show dismissed Hide dismissed
@0xmad 0xmad mentioned this pull request Oct 21, 2024
Merged
6 tasks
@0xmad 0xmad mentioned this pull request Oct 23, 2024
Merged
3 tasks
github-advanced-security[bot]
github-advanced-security bot found potential problems Oct 24, 2024
View reviewed changes
packages/contracts/contracts/maci/Tally.sol Dismissed Show dismissed Hide dismissed
packages/contracts/contracts/maci/Tally.sol Dismissed Show dismissed Hide dismissed
packages/contracts/contracts/maci/Tally.sol Dismissed Show dismissed Hide dismissed
packages/contracts/contracts/maci/Tally.sol Dismissed Show dismissed Hide dismissed
ctrlc03
ctrlc03 approved these changes Oct 25, 2024
View reviewed changes
Copy link
Collaborator

@ctrlc03 ctrlc03 left a comment

Choose a reason for hiding this comment

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

Thanks so much! just one comment but otherwise lgtm

packages/contracts/contracts/maci/Tally.sol
/// @param voiceCreditsPerOptions The voice credit options received for recipient
function getAllocatedAmounts(
uint256[] calldata voiceCreditsPerOptions
) public view afterTallying returns (uint256[] memory amounts) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

don't think there's need to have afterTallying here, when called by other functions they also have the same modifier.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a public function so it can be used directly. And if there is no results it won't show the correct result and revert with no reason.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ballots tallied not necessarily means that the results have been added (sure in our version with our scripts it does cause the scripts do both operations in once, but they are not necessarily done together, nor would be true in the case of multiple batches of results and the call made before the second batch is added). Either way not a problem, just a small suggestion

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So I'd keep this one. There is also a check if we don't have full results uploaded.

@0xmad 0xmad disabled auto-merge October 25, 2024 14:55
@0xmad
feat(contracts): payout strategy
b028697 
- [x] Use tally as payout strategy
- [x] Add deposit/claim/withdraw functions
- [x] Add tests
- [x] Add pause for tally in poll deploy script
- [x] Update deploy config
- [x] Add tally initialization
- [x] Add batch method for rewards calculation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kittybest kittybest kittybest left review comments

@ctrlc03 ctrlc03 ctrlc03 approved these changes

@crisgarner crisgarner Awaiting requested review from crisgarner crisgarner is a code owner

Assignees

@0xmad 0xmad

Labels
enhancement New feature or request
Projects
MACI team sprint board
Status: In Progress
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@0xmad @kittybest @ctrlc03