feat(payout): add withdraw function (Only withdraw with latest main)#674
feat(payout): add withdraw function (Only withdraw with latest main)#674NicoSerranoP wants to merge 1 commit into
main)#674Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds a withdraw function to the Tally contract that allows the owner to withdraw all remaining funds to a designated custodian address after the deposit window has closed. The implementation includes proper access controls and modifier checks.
- Introduces a custodian field in the StrategyInit struct and Tally contract
- Adds a withdraw function with owner-only and post-deposit-window restrictions
- Updates deployment configuration and tests to support the new custodian functionality
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/contracts/contracts/interfaces/IPayoutStrategy.sol | Adds custodian field to StrategyInit struct and withdraw function to interface |
| packages/contracts/contracts/maci/Tally.sol | Implements withdraw function and custodian storage variable |
| packages/contracts/tests/Tally.test.ts | Adds comprehensive test coverage for withdraw functionality |
| packages/contracts/tasks/deploy/poll/01-poll.ts | Updates deployment task to include custodian parameter |
| packages/contracts/deploy-config-example.json | Adds custodian configuration field across all network examples |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| uint256 totalFunds = token.balanceOf(address(this)); | ||
|
|
||
| token.safeTransfer(custodian, totalFunds); |
There was a problem hiding this comment.
The withdraw function doesn't update the contract's internal state after transferring funds. Consider setting totalAmount to 0 or adding an event to track the withdrawal.
| interface IPayoutStrategy { | ||
| /// @notice Strategy initialization params | ||
| struct StrategyInit { | ||
| /// @notice The custodian address who should receive leftover funds if voting period is over |
There was a problem hiding this comment.
The comment mentions 'voting period' but the actual implementation uses 'deposit window'. Update to 'if deposit window is over' for consistency with the contract logic.
| /// @notice The custodian address who should receive leftover funds if voting period is over | |
| /// @notice The custodian address who should receive leftover funds if deposit window is over |
| } | ||
|
|
||
| /// @inheritdoc IPayoutStrategy | ||
| function withdraw() public override isInitialized onlyOwner afterDepositWindow { |
There was a problem hiding this comment.
The withdraw function lacks input validation for the custodian address. Consider adding a check to ensure custodian is not the zero address before allowing withdrawal.
| function withdraw() public override isInitialized onlyOwner afterDepositWindow { | |
| function withdraw() public override isInitialized onlyOwner afterDepositWindow { | |
| require(custodian != address(0), "Tally: custodian address is zero"); |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| uint256 public voiceCreditFactor; | ||
|
|
||
| /// @notice The custodian address who should receive leftover funds if tallying and cooldown period are over | ||
| address public custodian; |
Check failure
Code scanning / Slither
Uninitialized state variables High
| uint256 public voiceCreditFactor; | ||
|
|
||
| /// @notice The custodian address who should receive leftover funds if tallying and cooldown period are over | ||
| address public custodian; |
Check warning
Code scanning / Slither
State variables that could be declared constant Warning
Description
This is a clone of #673
Additionals
withdrawuses theafterDepositWindowmodifier because there should not be a cool down period anymore according to latest mainclaimstill callableafterDepositWindowaccording to latest mainConfirmation
Important
We do not accept minor grammatical fixes (e.g., correcting typos, rewording sentences) unless they significantly improve clarity in technical documentation. These contributions, while appreciated, are not a priority for merging. If there is a grammatical error feel free to message the team.