-
Notifications
You must be signed in to change notification settings - Fork 12.1k
Add ERC20MultiVotes for partial delegations support #5836
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.
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
base: master
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 334e13f The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
This comment was marked as resolved.
This comment was marked as resolved.
Reviewer's GuideThis PR introduces MultiVotes as an extension to the existing Votes system to support partial delegations, integrates it into a new ERC20MultiVotes token extension with safe supply caps, and adds corresponding EIP-712 types, mocks, and comprehensive tests. Sequence diagram for multiDelegate partial delegation processsequenceDiagram
actor User
participant ERC20MultiVotes
participant MultiVotes
User->>ERC20MultiVotes: multiDelegate(delegatees[], units[])
ERC20MultiVotes->>MultiVotes: _multiDelegate(account, delegatees[], units[])
MultiVotes->>MultiVotes: Add/modify/remove delegates
MultiVotes-->>ERC20MultiVotes: Update delegation state
ERC20MultiVotes-->>User: Emit DelegateModified events
Sequence diagram for token transfer and voting units update in ERC20MultiVotessequenceDiagram
participant ERC20MultiVotes
participant MultiVotes
participant ERC20
ERC20->>ERC20MultiVotes: Transfer(from, to, value)
ERC20MultiVotes->>ERC20MultiVotes: _update(from, to, value)
ERC20MultiVotes->>MultiVotes: _transferVotingUnits(from, to, value)
MultiVotes-->>ERC20MultiVotes: Update voting units
ERC20MultiVotes-->>ERC20: Complete transfer
Class diagram for MultiVotes and ERC20MultiVotes extensionsclassDiagram
class Votes {
<<abstract>>
+delegate(address account, address delegatee)
+_delegate(address account, address delegatee)
+_setDelegate(address account, address delegatee)
+_transferVotingUnits(address from, address to, uint256 amount)
+_getVotingUnits(address account)
}
class MultiVotes {
<<abstract>>
+multiDelegates(address account, uint256 start, uint256 end)
+multiDelegate(address[] delegatees, uint256[] units)
+multiDelegateBySig(address[] delegatees, uint256[] units, uint256 nonce, uint256 expiry, uint8 v, bytes32 r, bytes32 s)
+getDelegatedUnits(address account, address delegatee)
+getFreeUnits(address account)
-_multiDelegate(account, delegatees, unitsList)
-_addDelegate(account, delegatee, units)
-_modifyDelegate(account, delegatee, units)
-_removeDelegate(account, delegatee)
-_accountHasDelegate(account, delegatee)
-_delegatesList
-_delegatesIndex
-_delegatesUnits
-_usedUnits
}
class ERC20 {
<<abstract>>
+balanceOf(address account)
+totalSupply()
}
class ERC20MultiVotes {
<<abstract>>
+_maxSupply()
+_update(address from, address to, uint256 value)
+_getVotingUnits(address account)
+numCheckpoints(address account)
+checkpoints(address account, uint32 pos)
}
Votes <|-- MultiVotes
ERC20 <|-- ERC20MultiVotes
MultiVotes <|-- ERC20MultiVotes
class IMultiVotes {
<<interface>>
+multiDelegates(address account, uint256 start, uint256 end)
+multiDelegate(address[] delegatees, uint256[] units)
+multiDelegateBySig(address[] delegatees, uint256[] units, uint256 nonce, uint256 expiry, uint8 v, bytes32 r, bytes32 s)
+getDelegatedUnits(address account, address delegatee)
+getFreeUnits(address account)
}
MultiVotes ..|> IMultiVotes
Class diagram for MultiVotesMock and ERC20MultiVotesTimestampMockclassDiagram
class MultiVotesMock {
<<abstract>>
+getTotalSupply()
+delegate(address account, address newDelegation)
+_getVotingUnits(address account)
+_mint(address account, uint256 votes)
+_burn(address account, uint256 votes)
-_votingUnits
}
class MultiVotesTimestampMock {
<<abstract>>
+clock()
+CLOCK_MODE()
}
MultiVotesMock <|-- MultiVotesTimestampMock
class ERC20MultiVotesTimestampMock {
<<abstract>>
+clock()
+CLOCK_MODE()
}
ERC20MultiVotes <|-- ERC20MultiVotesTimestampMock
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes - here's some feedback:
Blocking issues:
- Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (link)
General comments:
- The multiDelegates function walks the entire delegates list and can run out of gas for large lists—consider enforcing a maximum delegates count or adding pagination to keep iteration bounded.
- The EIP-712 struct hash for array fields in multiDelegateBySig uses abi.encodePacked, which may not match the spec for dynamic types—please switch to abi.encode or the standard TypedDataEncoder for arrays to avoid potential collisions.
- In IMultiVotes the parameter name “delegatess” is misspelled—renaming it to “delegatees” will improve consistency and reduce confusion.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The multiDelegates function walks the entire delegates list and can run out of gas for large lists—consider enforcing a maximum delegates count or adding pagination to keep iteration bounded.
- The EIP-712 struct hash for array fields in multiDelegateBySig uses abi.encodePacked, which may not match the spec for dynamic types—please switch to abi.encode or the standard TypedDataEncoder for arrays to avoid potential collisions.
- In IMultiVotes the parameter name “delegatess” is misspelled—renaming it to “delegatees” will improve consistency and reduce confusion.
## Individual Comments
### Comment 1
<location> `contracts/governance/utils/MultiVotes.sol:76` </location>
<code_context>
+ return empty;
+ }
+
+ if (end >= maxLength) {
+ end = maxLength - 1;
+ }
+ uint256 length = (end + 1) - start;
</code_context>
<issue_to_address>
Adjusting 'end' to maxLength-1 may be unintuitive for callers.
Consider reverting or returning an empty array when 'end' exceeds the list length, or document this adjustment clearly to avoid confusion.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
if (end >= maxLength) {
end = maxLength - 1;
}
uint256 length = (end + 1) - start;
address[] memory list = new address[](length);
for (uint256 i; i < length; i++) {
list[i] = _delegatesList[account][start + i];
}
return list;
=======
if (end >= maxLength) {
address[] memory empty = new address[](0);
return empty;
}
uint256 length = (end + 1) - start;
address[] memory list = new address[](length);
for (uint256 i; i < length; i++) {
list[i] = _delegatesList[account][start + i];
}
return list;
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `contracts/governance/utils/MultiVotes.sol:159` </location>
<code_context>
+ }
+ }
+
+ if (removedUnits >= givenUnits) {
+ uint256 refundedUnits;
+ refundedUnits = removedUnits - givenUnits;
+ /**
+ * Cannot Underflow: code logic assures that _usedUnits[account] is just a sum of active delegates units
+ * and that every units change of delegate on `account`, updates coherently _usedUnits
+ * so refundedUnits cannot be higher than _usedUnits[account]
+ */
+ unchecked {
+ _usedUnits[account] -= refundedUnits;
+ }
</code_context>
<issue_to_address>
Unchecked block for subtraction relies on prior logic; consider explicit assertion.
Adding an explicit assertion or revert before the unchecked subtraction would help catch any unexpected state and prevent silent underflows.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
if (removedUnits >= givenUnits) {
uint256 refundedUnits;
refundedUnits = removedUnits - givenUnits;
/**
* Cannot Underflow: code logic assures that _usedUnits[account] is just a sum of active delegates units
* and that every units change of delegate on `account`, updates coherently _usedUnits
* so refundedUnits cannot be higher than _usedUnits[account]
*/
unchecked {
_usedUnits[account] -= refundedUnits;
}
=======
if (removedUnits >= givenUnits) {
uint256 refundedUnits;
refundedUnits = removedUnits - givenUnits;
/**
* Cannot Underflow: code logic assures that _usedUnits[account] is just a sum of active delegates units
* and that every units change of delegate on `account`, updates coherently _usedUnits
* so refundedUnits cannot be higher than _usedUnits[account]
*/
require(refundedUnits <= _usedUnits[account], "MultiVotes: refundedUnits exceeds usedUnits");
unchecked {
_usedUnits[account] -= refundedUnits;
}
>>>>>>> REPLACE
</suggested_fix>
### Comment 3
<location> `contracts/governance/utils/IMultiVotes.sol:47` </location>
<code_context>
+ /**
+ * @dev Set delegates list with units assigned for each one
+ */
+ function multiDelegate(address[] calldata delegatess, uint256[] calldata units) external;
+
+ /**
</code_context>
<issue_to_address>
Typo in parameter name: 'delegatess' should be 'delegatees'.
Please update the parameter name to 'delegatees'.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
function multiDelegate(address[] calldata delegatess, uint256[] calldata units) external;
=======
function multiDelegate(address[] calldata delegatees, uint256[] calldata units) external;
>>>>>>> REPLACE
</suggested_fix>
### Comment 4
<location> `contracts/governance/utils/IMultiVotes.sol:52` </location>
<code_context>
+ /**
+ * @dev Multi delegate votes from signer to `delegatess`.
+ */
+ function multiDelegateBySig(
+ address[] calldata delegatess,
+ uint256[] calldata units,
+ uint256 nonce,
</code_context>
<issue_to_address>
Typo in parameter name: 'delegatess' should be 'delegatees'.
Please rename the parameter to 'delegatees' to correct the typo.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
/**
* @dev Set delegates list with units assigned for each one
*/
function multiDelegate(address[] calldata delegatess, uint256[] calldata units) external;
/**
* @dev Multi delegate votes from signer to `delegatess`.
*/
function multiDelegateBySig(
address[] calldata delegatess,
uint256[] calldata units,
uint256 nonce,
uint256 expiry,
uint8 v,
bytes32 r,
bytes32 s
) external;
=======
/**
* @dev Set delegates list with units assigned for each one
*/
function multiDelegate(address[] calldata delegatees, uint256[] calldata units) external;
/**
* @dev Multi delegate votes from signer to `delegatees`.
*/
function multiDelegateBySig(
address[] calldata delegatees,
uint256[] calldata units,
uint256 nonce,
uint256 expiry,
uint8 v,
bytes32 r,
bytes32 s
) external;
>>>>>>> REPLACE
</suggested_fix>
### Comment 5
<location> `test/token/ERC20/extensions/ERC20MultiVotes.test.js:47` </location>
<code_context>
+ expect(await this.token.nonces(this.holder)).to.equal(0n);
+ });
+
+ it('minting restriction', async function () {
+ const value = 2n ** 208n;
+ await expect(this.token.$_mint(this.holder, value))
+ .to.be.revertedWithCustomError(this.token, 'ERC20ExceededSafeSupply')
+ .withArgs(value, value - 1n);
+ });
+
+ it('recent checkpoints', async function () {
</code_context>
<issue_to_address>
Consider adding tests for edge cases around minting, such as minting zero tokens or minting to the zero address.
Testing these scenarios will help verify that the contract enforces correct behavior for all minting inputs.
Suggested implementation:
```javascript
it('minting zero tokens', async function () {
await expect(this.token.$_mint(this.holder, 0n))
.to.be.revertedWithCustomError(this.token, 'ERC20InvalidMintAmount')
.withArgs(0n);
});
it('minting to zero address', async function () {
const value = 1n;
await expect(this.token.$_mint(ethers.constants.AddressZero, value))
.to.be.revertedWithCustomError(this.token, 'ERC20InvalidReceiver')
.withArgs(ethers.constants.AddressZero);
});
it('recent checkpoints', async function () {
```
- If your contract does not use custom errors named `ERC20InvalidMintAmount` or `ERC20InvalidReceiver`, replace them with the actual error names or revert messages your contract emits.
- If minting zero tokens is allowed, adjust the test to expect a successful mint instead of a revert.
- Ensure `ethers.constants.AddressZero` is available in your test context (import `ethers` from `hardhat` or `ethers` package if needed).
</issue_to_address>
## Security Issues
### Issue 1
<location> `test/token/ERC20/extensions/ERC20MultiVotes.test.js:50` </location>
<issue_to_address>
**security (generic-api-key):** Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
*Source: gitleaks*
</issue_to_address>
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Fixes #4963
Implementing MultiVotes as an extension of Votes as first step towards ERC20MultiVotes
MultiVotes adds multiDelegate() to add/change/remove partial delegations, old delegate() is used as defaulted delegate, so all free voting units are assigned to defaulted.
Im currently working on that and maybe I will add ERC20MultiVotes directly trough this pull
I publish that now as a draft to gather preliminary feedback from maintainers and community to proceed in case any refactoring or improvements are advised
PR Checklist
npx changeset add
)Summary by Sourcery
Implement partial delegation support by extending Votes with MultiVotes and integrating it into ERC20 tokens via ERC20MultiVotes, alongside off-chain signature delegation, enhanced querying of delegated and free vote units, and extensive tests.
New Features:
Enhancements:
Build:
Documentation:
Tests:
Chores: