Simple Delegation #34

ujenjt · 2024-01-25T14:18:51Z

The Simple Delegation update allows LDO token holders to delegate their voting power to other addresses and delegates to participate in on-chain voting on behalf of their delegated voters.

Spec

…ride for holders

apps/voting/contracts/Voting.sol

krogla · 2024-01-25T17:01:18Z

...I would replace voteFor with a method like voteByManager, where I wouldn't pass a list of addresses, but simply check if the msg.sender address has any under its management, and if so, vote on their behalf in a loop. Of course, it would require adding chunks, but then it seems we could simplify the logic of checking whether the manager's vote matches the delegate's vote.

apps/voting/contracts/Voting.sol

zuzueeka · 2024-01-30T21:24:53Z

apps/voting/contracts/Voting.sol

        }
    }

    /**
    * @dev Internal function to cast a vote or object to.
      @dev It assumes that voter can support or object to the vote
    */
-    function _vote(uint256 _voteId, bool _supports, address _voter) internal {
+    function _vote(uint256 _voteId, bool _supports, address _voter, bool _isManager) internal {
        Vote storage vote_ = votes[_voteId];

        // This could re-enter, though we can assume the governance token is not malicious
        uint256 voterStake = token.balanceOfAt(_voter, vote_.snapshotBlock);


LDO balance calculation can be transferred after checking that the tokenholder has already voted (to line 499)

token.balanceOfAt is called twice and this is quite complex function inside MiniMe token, for example take a look into relatively good case second call is cost us additional 2% of gas consumption. Take a look here: https://dashboard.tenderly.co/tx/mainnet/0xc20bba17d0809089e00ffb7e69b870f5659ae0ddc296ecf7906e8419e3b56a75/gas-usage

I am suggest to consider possibility of moving _hasVotingPower check into the _vote

Addressed in 5d01f37.

We've decided to move token.balanceOfAt call up and convert votingPower to _vote's argument.

apps/voting/contracts/Voting.sol

…ddressList management

…check from _vote to voteFor

… voting state

* test: add delegation state management unit tests * test: add delegation scenario tests (#43) * test: add delegation scenario tests * test: remove block with exception because it is pass in coverage * test: add delegation voting unit tests * test: remove outdated delegation tests --------- Co-authored-by: BATMAH69 <dev-www@yandex.ru>

ujenjt · 2024-04-14T13:39:40Z

apps/voting/contracts/Voting.sol

@@ -24,6 +24,7 @@ contract Voting is IForwarder, AragonApp {

    uint64 public constant PCT_BASE = 10 ** 18; // 0% = 0; 1% = 10^16; 100% = 10^18

+    uint256 private constant UINT_96_MAX = 0xFFFFFFFFFFFFFFFFFFFFFFFF;


Why not uint96(-1) to not manually count the number of Fs

You're right; this way of declaring the number could seem confusing. I'll fix it.

Fixed in 5ef1358.

ujenjt · 2024-04-14T13:55:15Z

apps/voting/contracts/Voting.sol

+    /**
+    * @notice Unassign `_delegate` from the sender
+    */
+    function resetDelegate() external {


The naming is a bit misleading here - it's easy to think that reset is like set second time - it's better to call it removeDelegate or unassignDelegate

Good catch, thank you. I'll come up with better names for these functions.

Fixed in 94e3276.

ujenjt · 2024-04-14T13:56:19Z

apps/voting/contracts/Voting.sol

@@ -73,6 +96,9 @@ contract Voting is IForwarder, AragonApp {
    event ChangeMinQuorum(uint64 minAcceptQuorumPct);
    event ChangeVoteTime(uint64 voteTime);
    event ChangeObjectionPhaseTime(uint64 objectionPhaseTime);
+    event SetDelegate(address indexed voter, address indexed delegate);
+    event ResetDelegate(address indexed voter, address indexed delegate);


the same as fro method ResetDelegate it's also better to rename the argument delegate to removedDelegated or so

imo "reset" is a bad verb here since it means "set again" which is not what's really meant here. a better verb would be "remove"

Thanks for pointing that out; I'll come up with more appropriate names.

Fixed in 94e3276.

apps/voting/contracts/Voting.sol

skozin

no real blockers but few suggestions/nitpicks

skozin · 2024-04-14T19:34:22Z

apps/voting/contracts/Voting.sol

@@ -73,6 +96,9 @@ contract Voting is IForwarder, AragonApp {
    event ChangeMinQuorum(uint64 minAcceptQuorumPct);
    event ChangeVoteTime(uint64 voteTime);
    event ChangeObjectionPhaseTime(uint64 objectionPhaseTime);
+    event SetDelegate(address indexed voter, address indexed delegate);
+    event ResetDelegate(address indexed voter, address indexed delegate);


imo "reset" is a bad verb here since it means "set again" which is not what's really meant here. a better verb would be "remove"

apps/voting/contracts/Voting.sol

skozin · 2024-04-14T19:46:06Z

apps/voting/contracts/Voting.sol

+     * @dev Internal function to cast a vote or object to.
+     * @dev It assumes that voter can support or object to the vote
+     * @param _voteId The identifier of the vote
+     * @param _supports Whether the voter supports the vote or not


the "or not" part is excessive

Good catch, thanks!

Fixed in c4d9e99.

skozin · 2024-04-14T19:54:19Z

apps/voting/contracts/Voting.sol

+
+        uint96 voterIndex = delegates[_voter].voterIndex;
+        assert(delegatedVoters[_delegate].addresses[voterIndex] == _voter);
+        address lastVoter = delegatedVoters[_delegate].addresses[delegatedVotersCount - 1];


this line can be moved inside the if block: the code will read better and consume less gas when removing the last delegated address (incl. the only address which might be a common scenario)

Agree with that; thanks for pointing it out.

Fixed in 5edff75.

skozin · 2024-04-14T19:58:02Z

apps/voting/contracts/Voting.sol

+        require(delegatedVotersCount <= UINT_96_MAX, ERROR_MAX_DELEGATED_VOTERS_REACHED);
+
+        delegatedVoters[_delegate].addresses.push(_voter);
+        delegates[_voter] = Delegate(_delegate, uint96(delegatedVotersCount));


there's a difference in semantics between _addDelegatedAddressFor and _removeDelegatedAddressFor: the former updates the delegates mapping but the latter doesn't (so it must be updated by the fn caller). this difference is not reflected in the fn names which can be misleading

I think we can consider moving mapping update to the _removeDelegatedAddressFor, it will make these functions more similar, and it's not a big deal in terms of gas cost (+100).

Fixed in e826ff0.

skozin · 2024-04-14T20:09:01Z

apps/voting/contracts/Voting.sol

+            delegatedVoters[_delegate].addresses[voterIndex] = lastVoter;
+            delegates[lastVoter].voterIndex = voterIndex;
+        }
+        delegatedVoters[_delegate].addresses.length--;


using .pop() instead of .length-- would read better imo

That's right, but unfortunately, Solidity v0.4.24 doesn't have .pop() support.

skozin · 2024-04-14T20:11:24Z

apps/voting/contracts/Voting.sol

+    function attemptVoteForMultiple(uint256 _voteId, bool _supports, address[] _voters) public voteExists(_voteId) {
+        Vote storage vote_ = votes[_voteId];
+        require(_isValidPhaseToVote(vote_, _supports), ERROR_CAN_NOT_VOTE);
+        bool hasManagedToVote = false;


votedForAtLeastOne would be a clearer name imo

Fixed in c035a74.

ujenjt · 2024-04-14T20:54:27Z

apps/voting/contracts/Voting.sol

+     * @param _supports Whether the delegate supports the vote
+     * @param _voter address of the voter
+     */
+    function attemptVoteFor(uint256 _voteId, bool _supports, address _voter) external voteExists(_voteId) {


voteExists calls twice - not critical, but extra unnecessary gas spendings

Missed that, thanks!

Fixed in 6c2840a.

ujenjt · 2024-04-14T20:59:30Z

apps/voting/contracts/Voting.sol

-        require(!_supports || _getVotePhase(votes[_voteId]) == VotePhase.Main, ERROR_CAN_NOT_VOTE);
-        _vote(_voteId, _supports, msg.sender);
+    function vote(uint256 _voteId, bool _supports, bool /* _executesIfDecided_deprecated */) external voteExists(_voteId) {
+        Vote storage vote_ = votes[_voteId];


Vote storage vote_ = votes[_voteId]; duplicated line here and in the _vote

After doing some tests with contracts built for cancun hardfork, we've decided not to introduce vote_ as an argument for _vote function. The gas cost decrease with such an approach is 72 per voter (7200 gas for 100 voters, respectively), which seems insufficient for the change that is decreasing the code readability.

ujenjt · 2024-04-14T21:01:50Z

apps/voting/contracts/Voting.sol

+     * @param _voters list of voters
+     */
+    function attemptVoteForMultiple(uint256 _voteId, bool _supports, address[] _voters) public voteExists(_voteId) {
+        Vote storage vote_ = votes[_voteId];


Vote storage vote_ = votes[_voteId]; duplicated line here and in the _vote
It's not a lot of gas but extra 100 gas

See #34 (comment)

ujenjt · 2024-04-14T21:44:09Z

apps/voting/contracts/Voting.sol

+    * @param _voter address of the voter
+    * @return True if _delegate is a current delegate for the _voter, false otherwise
+    */
+    function _isDelegateFor(address _delegate, address _voter) internal view returns (bool) {


I would suggest to rename _isDelegateForVoter or _isDelegated. Just because typical function named somethingFor implies that first arg is something for what this function is being applyed

Addressed in d33e9aa.

ujenjt · 2024-04-14T21:51:53Z

apps/voting/contracts/Voting.sol

@@ -479,6 +750,7 @@ contract Voting is IForwarder, AragonApp {

    /**
    * @dev Internal function to check if a vote is still open for both support and objection
+    * @param vote_ The queried vote
    * @return True if less than voteTime has passed since the vote start
    */
    function _isVoteOpen(Vote storage vote_) internal view returns (bool) {


!vote.executed is redundant check, since after introduction of second phase a vote couldn't be executed earlier than voteTime has been passing

@ujenjt This check might seem redundant, but it's not. There is an edge case where the vote can be open and executed at the same time. If you look at this test, you'll see that calling the unsafelyChangeVoteTime might break the vote's state. If we remove the !vote.executed check, with the usage of unsafelyChangeVoteTime, it will be possible to vote for the already executed vote.

ujenjt · 2024-04-14T22:08:46Z

apps/voting/contracts/Voting.sol

+        require(prevDelegate != address(0), ERROR_DELEGATE_NOT_SET);
+
+        _removeDelegatedAddressFor(prevDelegate, msg.sender);
+        delete delegates[msg.sender];


clearing of delegates mapping happening after the event ResetDelegate has been emited which is a bit confusing

Thanks for pointing this out. As mentioned earlier, I'll do some refactoring to ensure that ResetDelegate will be emitted after the deletion.

Fixed in e826ff0.

ujenjt · 2024-04-14T22:21:22Z

apps/voting/contracts/Voting.sol

+     * @param _voters the list of voters
+     * @param _voteId Vote identifier
+     */
+    function getVotersStateAtVote(uint256 _voteId, address[] _voters) external view voteExists(_voteId) returns (VoterState[] memory voterStatesList) {


This function is batch accessor for the getVoterState - it's better to name it getVoterStateMultiple for example

Addressed in cd8697b.

ujenjt · 2024-04-14T22:48:23Z

apps/voting/contracts/Voting.sol

@@ -73,6 +96,9 @@ contract Voting is IForwarder, AragonApp {
    event ChangeMinQuorum(uint64 minAcceptQuorumPct);
    event ChangeVoteTime(uint64 voteTime);
    event ChangeObjectionPhaseTime(uint64 objectionPhaseTime);
+    event SetDelegate(address indexed voter, address indexed delegate);
+    event ResetDelegate(address indexed voter, address indexed delegate);
+    event CastVoteAsDelegate(uint256 indexed voteId, address indexed delegate, address indexed voter, bool supports, uint256 stake);


Maybe let's optimize CastVoteAsDelegate event and cut off supports and stake fields? it's additional 2*256 units of gas! Or maybe get rid of indexed params?

See this comment — #34 (comment)

ujenjt · 2024-04-14T22:51:09Z

apps/voting/contracts/Voting.sol

        } else {
            vote_.nay = vote_.nay.add(voterStake);
-            vote_.voters[_voter] = VoterState.Nay;
+            vote_.voters[_voter] = _isDelegate ? VoterState.DelegateNay : VoterState.Nay;
        }

        emit CastVote(_voteId, _voter, _supports, voterStake);

        if (_getVotePhase(vote_) == VotePhase.Objection) {
            emit CastObjection(_voteId, _voter, voterStake);


Do we still need a separate event for it? Maybe let's consider to add phase parameter to CastVote event? It depends on frontend / subgraph / tooling tho

We've decided to keep these two events as they were since changing them may result in bad backward compatibility.

…ve duplicated tests

ujenjt · 2024-04-17T20:42:03Z

apps/voting/contracts/Voting.sol

+     */
+    function _removeDelegatedAddressFor(address _delegate, address _voter) internal {
+        uint256 delegatedVotersCount = delegatedVoters[_delegate].addresses.length;
+        require(delegatedVotersCount > 0, ERROR_DELEGATE_NOT_SET);


Excess check - never triggers

next assert actually checking more strict condition

Good one, thank you!

Fixed in 8ec459c.

ujenjt · 2024-04-17T20:49:48Z

apps/voting/contracts/Voting.sol

+     * @param _voter address of the voter
+     */
+    function _removeDelegatedAddressFor(address _delegate, address _voter) internal {
+        uint256 delegatedVotersCount = delegatedVoters[_delegate].addresses.length;


better to change to lastVoterIndex = delegatedVoters[_delegate].addresses.length - 1 and simplify the code later

delegatedVoters[_delegate] might be extracted to the separate variable to save some gas

Nice findings, thanks 🙏

Fixed in 8ec459c.

ujenjt · 2024-04-17T21:06:07Z

apps/voting/contracts/Voting.sol

+            delegates[lastVoter].voterIndex = voterIndex;
+        }
+        delegatedVoters[_delegate].addresses.length--;
+        emit ResetDelegate(_voter, _delegate);


technically at this line delegate is not completely reset, since it still in the delegates mapping, I suggest to move events out of this func

Maybe make ordering of voting and delegate the same?
function _removeDelegatedAddressFor(address _delegate, address _voter)
emit ResetDelegate(_voter, _delegate)

technically at this line delegate is not completely reset, since it still in the delegates mapping, I suggest to move events out of this func

Fixed in 8ec459c.

ujenjt added 2 commits January 25, 2024 14:13

chore: add voting manager feature for holders

8b8ef36

chore: Prohibit managers from overwriting holder's votes, retain over…

c1fa2f1

…ride for holders

skozin reviewed Jan 25, 2024

View reviewed changes

apps/voting/contracts/Voting.sol Outdated Show resolved Hide resolved

apps/voting/contracts/Voting.sol Outdated Show resolved Hide resolved

krogla reviewed Jan 25, 2024

View reviewed changes

apps/voting/contracts/Voting.sol Outdated Show resolved Hide resolved

krogla reviewed Jan 25, 2024

View reviewed changes

apps/voting/contracts/Voting.sol Outdated Show resolved Hide resolved

folkyatina reviewed Jan 26, 2024

View reviewed changes

apps/voting/contracts/Voting.sol Outdated Show resolved Hide resolved

Psirex reviewed Jan 26, 2024

View reviewed changes

apps/voting/contracts/Voting.sol Outdated Show resolved Hide resolved

zuzueeka reviewed Jan 30, 2024

View reviewed changes

apps/voting/contracts/Voting.sol Outdated Show resolved Hide resolved

katamarinaki added 20 commits February 7, 2024 16:11

fix: internal function types

52a6524

refactor: rename holder -> voter, manager -> delegate

7ed76a8

refactor: update delegation related naming in tests

740bb2b

refactor: extract vote phase check into separate fn

739ae34

refactor: add CastVoteAsDelegate event

7392eb4

refactor: introduce Delegate struct and use it to simplify DelegatedA…

978b4eb

…ddressList management

feat: add balance check to setDelegate

e8c7965

refactor: remove reverts from voteForMultiple and _vote, move revert …

baa1a71

…check from _vote to voteFor

fix: update voterIndex of swapped address during voter deletion

1d98abf

refactor: remove voterIndex from DelegateSet event

81a75b7

refactor: remove MAX_VOTERS_PER_DELEGATE limitation

b040619

refactor: add offset and count params to getDelegatedVoters

337b031

refactor: remove unused constant

f98f68d

refactor: add revert if error inside loop in voteForMultiple

4d6f150

refactor: add getter for delegated voters with their voting power and…

7bf4014

… voting state

refactor: optimize delegated voter removal

c3f8577

refactor: add max uint96 check for delegated voters array length

5970417

refactor: remove unused variables from Voting contract

3138944

refactor: rebuild invariant checks for voteForMultiple, refactor voteFor

42bb8f3

refactor: update delegation getters and setters

569a600

fix: audit fixes

08d43e3

bulbozaur previously approved these changes Mar 21, 2024

View reviewed changes

fix: remove inconsistent balance check

22fe318

katamarinaki dismissed bulbozaur’s stale review via 22fe318 March 22, 2024 13:01

katamarinaki and others added 6 commits March 27, 2024 23:30

feat: add holesky support for deployment

e7faa43

refactor: rename _vote param to vote_

1096e16

refactor: comment out deprecated arguments names

f2f1eed

docs: add missing natspec comments

e3cef8c

test: remove outdated test

42f8dc2

ujenjt commented Apr 14, 2024

View reviewed changes

apps/voting/contracts/Voting.sol Show resolved Hide resolved

ujenjt commented Apr 14, 2024

View reviewed changes

apps/voting/contracts/Voting.sol Show resolved Hide resolved

skozin reviewed Apr 14, 2024

View reviewed changes

ujenjt commented Apr 14, 2024

View reviewed changes

test: move out tests for delegation to the diiferent test file & remo…

54caa64

…ve duplicated tests

ujenjt commented Apr 17, 2024

View reviewed changes

katamarinaki mentioned this pull request May 22, 2024

Simple Delegation Refactor #44

Open

		@@ -24,6 +24,7 @@ contract Voting is IForwarder, AragonApp {

		uint64 public constant PCT_BASE = 10 ** 18; // 0% = 0; 1% = 10^16; 100% = 10^18

		uint256 private constant UINT_96_MAX = 0xFFFFFFFFFFFFFFFFFFFFFFFF;

Simple Delegation #34

Are you sure you want to change the base?

Simple Delegation #34

Conversation

ujenjt commented Jan 25, 2024 • edited by katamarinaki

krogla commented Jan 25, 2024

zuzueeka Jan 30, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katamarinaki Apr 22, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katamarinaki Apr 18, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skozin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katamarinaki Apr 18, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ujenjt Apr 14, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katamarinaki Apr 22, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katamarinaki Apr 22, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ujenjt commented Jan 25, 2024 •

edited by katamarinaki

zuzueeka Jan 30, 2024 •

edited

katamarinaki Apr 22, 2024 •

edited

katamarinaki Apr 18, 2024 •

edited

katamarinaki Apr 18, 2024 •

edited

ujenjt Apr 14, 2024 •

edited

katamarinaki Apr 22, 2024 •

edited

katamarinaki Apr 22, 2024 •

edited