Client sub delegation #82

davidlaprade · 2025-01-15T19:30:46Z

Adds the ability to sub-delegate within a FlexVotingClient, i.e. to delegate one's internal voting weight to another user (who may or may not have voting weight within the client as well).

Sub delegation is an optional add-on via a new abstract contract FlexVotingDelegatable. To make this possible, shared storage vars and logic were abstracted into a new abstract contract FlexVotingBase which both FlexVotingClient and FlexVotingDelegatable inherit from. Some internal functions and variables were renamed accordingly.

Someone wishing to implement a client with sub-delegation will want a contract with (at minimum) the following inheritance structure:

contract MyClient is FlexVotingBase, FlexVotingClient, FlexVotingDelegatable {
  // ...
}

src/FlexVotingDelegatable.sol

src/FlexVotingClient.sol

src/FlexVotingDelegatable.sol

src/FlexVotingClient.sol

src/FlexVotingDelegatable.sol

src/FlexVotingBase.sol

+  function _selfDelegate() internal {
+    IVotingToken(GOVERNOR.token()).delegate(address(this));
+  }


src/FlexVotingBase.sol

+  function _applyDeltaToCheckpoint(Checkpoints.Trace208 storage _checkpoint, int256 _delta)
+    internal
+    returns (uint208 _prevTotal, uint208 _newTotal)
+  {
+    // The casting in this function is safe since:
+    // - if oldTotal + delta > int256.max it will panic and revert.
+    // - if |delta| <= oldTotal there is no risk of wrapping
+    // - if |delta| > oldTotal
+    //   * uint256(oldTotal + delta) will wrap but the wrapped value will
+    //     necessarily be greater than uint208.max, so SafeCast will revert.
+    //   * the lowest that oldTotal + delta can be is int256.min (when
+    //     oldTotal is 0 and delta is int256.min). The wrapped value of a
+    //     negative signed integer is:
+    //       wrapped(integer) = uint256.max + integer
+    //     Substituting:
+    //       wrapped(int256.min) = uint256.max + int256.min
+    //     But:
+    //       uint256.max + int256.min > uint208.max
+    //     Substituting again:
+    //       wrapped(int256.min) > uint208.max, which will revert when safecast.
+    _prevTotal = _checkpoint.latest();
+    int256 _castTotal = int256(uint256(_prevTotal));
+    _newTotal = SafeCast.toUint208(uint256(_castTotal + _delta));
+
+    uint48 _timepoint = IVotingToken(GOVERNOR.token()).clock();
+    _checkpoint.push(_timepoint, _newTotal);
+  }


src/FlexVotingBase.sol

+  function _checkpointVoteWeightOf(address _user, int256 _delta) internal virtual {
+    _applyDeltaToCheckpoint(voteWeightCheckpoints[_user], _delta);
+  }


src/FlexVotingBase.sol

+  function _checkpointTotalVoteWeight(int256 _delta) internal virtual {
+    _applyDeltaToCheckpoint(totalVoteWeightCheckpoints, _delta);
+  }


src/FlexVotingDelegatable.sol

davidlaprade · 2025-01-21T19:03:44Z

I'm not sure why scopelint check is failing on CI. It's passing locally. My working directory is clean and I'm on the latest version of scopelint.

apbendi

Not a totally full review @davidlaprade, but left some preliminary feedback. Will pick this back up quickly after your responses to continue the review cycle. Overall I like how it's put together but I feel like maybe I'm missing something so I want to get your response first (see, in particular, my question on why the mock client doesn't checkpoint the total weight in borrow. I feel like I must be confused about the model so I want to straighten that out in my head before leaving more feedback).

apbendi · 2025-04-03T19:30:41Z

test/SharedFlexVoting.t.sol

+    assertEq(flexClient.getPastTotalVoteWeight(_timepoint), uint256(_initBalance + _delta));
+  }
+
+  function testFuzz_RevertIf_negativeDeltaWraps(int256 delta, uint208 balance) public {


Two things:

This test is failing for me with the error:

[FAIL: call didn't revert at a lower depth than cheatcode call depth; counterexample: calldata=0x461c6f850000000000000000000000000000000000000000000000000000000000003200000000000000000000000000000000000000000000000000000000002ade3881 args=[12800 [1.28e4], 719206529 [7.192e8]]] testFuzz_RevertIf_negativeDeltaWraps(int256,uint208) (runs: 0, μ: 0, ~: 0)

What is this test actually...testing? I don't actually see where any of our code is under test. What am I missing?

Discussed on our call.

This failing test is fixed here in #84

Decided to just pull the fix in here via 1e1d9bb

apbendi · 2025-04-03T19:46:10Z

src/FlexVotingBase.sol

+  /// e.g. if the internal representation of balance has been scaled down.
+  function _rawBalanceOf(address _user) internal view virtual returns (uint208);
+
+  // TODO rename to avoid collision with FlexVotingDelegatable.


Flagging this: is it something we need to address in this PR? Or pull into an issue?

Created #88 and updated the comment to reference the GH issue in 345804b

apbendi · 2025-04-03T19:47:17Z

src/FlexVotingBase.sol

+    IVotingToken(GOVERNOR.token()).delegate(address(this));
+  }
+
+  function _applyDeltaToCheckpoint(Checkpoints.Trace208 storage _checkpoint, int256 _delta)


Would be nice to have natspec for this method. Alternatively we could pull out an issue to do a natspec pass/review for the whole repo.

Probably a good idea to review the whole repo. Created #86

apbendi · 2025-04-03T19:49:31Z

src/FlexVotingClient.sol

-  // conform to the EIP-6372 standard, which specifies they be uint48s.
+/// This contract extends FlexVotingBase, adding two features:
+///   (a) the ability for depositors to express voting preferences on
+///       {Governor}'s proprosals, and


nit: spelling of "proposals" is wrong

Thanks, see cb90ea6

apbendi · 2025-04-03T19:57:35Z

src/FlexVotingClient.sol

-  // https://github.com/OpenZeppelin/openzeppelin-contracts/blob/7b74442c5e87ea51dde41c7f18a209fa5154f1a4/contracts/governance/extensions/GovernorCountingFractional.sol#L37
+  /// Constant used by OZ's implementation of {GovernorCountingFractional} to
+  /// signal fractional voting.
+  /// https://github.com/OpenZeppelin/openzeppelin-contracts/blob/7b74442c5e87ea51dde41c7f18a209fa5154f1a4/contracts/governance/extensions/GovernorCountingFractional.sol#L37
  uint8 internal constant VOTE_TYPE_FRACTIONAL = 255;

  error FlexVotingClient__NoVotingWeight();
  error FlexVotingClient__AlreadyVoted();
  error FlexVotingClient__InvalidSupportValue();
  error FlexVotingClient__NoVotesExpressed();


nit: let's create a separate issue to review the repo and re-arrange declarations to our conventional ordering.

Added #87, thanks

apbendi · 2025-04-03T20:41:39Z

test/MockFlexVotingClient.sol

-    FlexVotingClient._checkpointTotalBalance(-1 * int256(uint256(_amount)));
+    int256 _delta = -1 * int256(uint256(_amount));
+    _checkpointVoteWeightOf(msg.sender, _delta);
+    _checkpointTotalVoteWeight(_delta);

    TOKEN.transfer(msg.sender, _amount); // Assumes revert on failure.
  }


arbitrary comment location

I feel like I must be missing something. Reading the implementation of this Mock, I don't should the borrow method checkpoint the total voting weight?

Discussed on our call but documenting here for posterity...

It's a little confusing, but I think the existing behavior is actually correct. borrow doesn't checkpoint the total voting weight because that storage var (i.e. totalVoteWeightCheckpoints) is really storing the the total vote weight that has been deposited into the system, not the actual vote weight of the system. We don't need to checkpoint the actual vote weight that the client has access to, that's already done for us by the governor's token.

The scenario to keep in mind is this:

user A deposits 50 GOV into client

user B deposits 100 GOV into client

user C borrows 50 GOV from client

100 GOV remains as the client's balance

proposal P is created

user B expresses his vote on P -- how much weight should his preference get?

the weight of user B's preference should NOT be:

user B's deposits / GOV.balanceOf(client @ snapshot(P))

this would be: 100 / 100 -- i.e. it would give B all of the voting weight

instead, the weight of user B's preference should be:

user B's deposits / the total amount of GOV deposited into client

i.e. 100/150, or 2/3rds of client's voting weight at the time of P

apbendi · 2025-04-03T20:49:56Z

src/FlexVotingDelegatable.sol

+///   - user B's voting weight is combined with user A's voting weight so that
+///     150 tokens are effectively cast with voting preference P on behalf of
+///     users A and B.
+abstract contract FlexVotingDelegatable is Context, FlexVotingBase {


Naming bikeshed: some googling seems to indicate "Delegatable" is not a real word but "Delegable" is. I'm not saying that definitely means we should change it, but I think we should consider it.

I'm fine with the change. I think they're equally clear and all else being equal I'd rather we use real words 😄

Done in 3e16491

apbendi · 2025-04-03T20:50:50Z

src/FlexVotingDelegatable.sol

+///
+/// This contract extends FlexVotingBase, adding the ability to subdelegate one's
+/// internal voting weight. It is meant to be inherited from in conjunction with
+/// FlexVotingClient. Doing so makes the following usecase possible:


I know we discussed this before, so can you remind me: If this contract is meant to be used with FlexVotingClient, then why not inherit from FlexVotingClient directly, instead of from the base? Why make the user figure out to inherit from both.

Ah, I found where we talked about this:

It is more modular, but it's modularity that's not really interesting, since there really isn't a usecase for FlexVotingBase + FlexVotingDelegatable without also including FlexVotingClient.

I do still kind of agree that it is more self-documenting to have MyContract is FlexVotingClient, FlexVotingDelegatable than it is to just do MyContract is FlexVotingDelegatable.

What do you think is more important in this case: simplicity (one contract to inherit from) or self-documentation?

src/FlexVotingDelegable.sol

+abstract contract FlexVotingDelegable is Context, FlexVotingBase {
+  using Checkpoints for Checkpoints.Trace208;
+
+  // @dev Emitted when an account changes its delegate.
+  event DelegateChanged(
+    address indexed delegator, address indexed fromDelegate, address indexed toDelegate
+  );
+
+  // @dev Emitted when a delegate change results in changes to a delegate's
+  // number of voting weight.
+  event DelegateWeightChanged(address indexed delegate, uint256 previousVotes, uint256 newVotes);
+
+  mapping(address account => address) private _delegatee;
+
+  // @dev Delegates votes from the sender to `delegatee`.
+  function delegate(address delegatee) public virtual {
+    address account = _msgSender();
+    _delegate(account, delegatee);
+  }
+
+  // @dev Returns the delegate that `account` has chosen. Assumes
+  // self-delegation if no delegate has been chosen.
+  function delegates(address _account) public view virtual returns (address) {
+    address _proxy = _delegatee[_account];
+    if (_proxy == address(0)) return _account;
+    return _proxy;
+  }
+
+  // @dev Delegate all of `account`'s voting units to `delegatee`.
+  //
+  // Emits events {DelegateChanged} and {DelegateWeightChanged}.
+  function _delegate(address account, address delegatee) internal virtual {
+    address oldDelegate = delegates(account);
+    _delegatee[account] = delegatee;
+
+    int256 _delta = int256(uint256(_rawBalanceOf(account)));
+    emit DelegateChanged(account, oldDelegate, delegatee);
+    _updateDelegateBalance(oldDelegate, delegatee, _delta);
+  }
+
+  function _checkpointVoteWeightOf(address _user, int256 _delta) internal virtual override {
+    address _proxy = delegates(_user);
+    _applyDeltaToCheckpoint(voteWeightCheckpoints[_proxy], _delta);
+  }
+
+  // @dev Moves delegated votes from one delegate to another.
+  function _updateDelegateBalance(address from, address to, int256 _delta) internal virtual {
+    if (from == to || _delta == 0) return;
+
+    // Decrement old delegate's weight.
+    (uint208 _oldFrom, uint208 _newFrom) =
+      _applyDeltaToCheckpoint(voteWeightCheckpoints[from], -_delta);
+    emit DelegateWeightChanged(from, _oldFrom, _newFrom);
+
+    // Increment new delegate's weight.
+    (uint208 _oldTo, uint208 _newTo) = _applyDeltaToCheckpoint(voteWeightCheckpoints[to], _delta);
+    emit DelegateWeightChanged(to, _oldTo, _newTo);
+  }
+}


src/FlexVotingDelegable.sol

+  function _checkpointVoteWeightOf(address _user, int256 _delta) internal virtual override {
+    address _proxy = delegates(_user);
+    _applyDeltaToCheckpoint(voteWeightCheckpoints[_proxy], _delta);
+  }


davidlaprade · 2025-04-15T12:38:47Z

I'm not sure why the format step keeps failing. Everything passes locally with no changes. I'm on the latest foundry and scopelint versions. Even tried it with FOUNDRY_PROFILE=ci.

github-actions · 2025-04-15T18:48:53Z

Coverage after merging client-sub-delegation into master will be

91.39%

Coverage Report

File	Stmts	Branches	Funcs	Lines	Uncovered Lines
src
FlexVotingBase.sol	100%	100%	100%	100%
FlexVotingClient.sol	100%	100%	100%	100%
FlexVotingDelegable.sol	100%	100%	100%	100%
FractionalPool.sol	79.44%	70.59%	73.33%	82.67%	110, 112, 114, 117, 192–193, 195–197, 200, 204, 240–241, 81, 83, 85–86, 88

github-advanced-security bot found potential problems Jan 15, 2025

View reviewed changes

src/FlexVotingDelegatable.sol Fixed Show fixed Hide fixed

src/FlexVotingDelegatable.sol Fixed Show fixed Hide fixed

src/FlexVotingDelegatable.sol Fixed Show fixed Hide fixed

src/FlexVotingDelegatable.sol Fixed Show fixed Hide fixed

github-advanced-security bot found potential problems Jan 20, 2025

View reviewed changes

src/FlexVotingClient.sol Fixed Show fixed Hide fixed

src/FlexVotingClient.sol Fixed Show fixed Hide fixed

davidlaprade force-pushed the client-sub-delegation branch from 3bfa073 to 8de392f Compare January 20, 2025 21:58

github-advanced-security bot found potential problems Jan 20, 2025

View reviewed changes

src/FlexVotingClient.sol Fixed Show fixed Hide fixed

src/FlexVotingDelegatable.sol Fixed Show fixed Hide fixed

github-advanced-security bot found potential problems Jan 21, 2025

View reviewed changes

src/FlexVotingClient.sol Fixed Show fixed Hide fixed

src/FlexVotingClient.sol Fixed Show fixed Hide fixed

src/FlexVotingDelegatable.sol Fixed Show fixed Hide fixed

github-advanced-security bot found potential problems Jan 21, 2025

View reviewed changes

davidlaprade marked this pull request as ready for review January 21, 2025 18:46

davidlaprade changed the title ~~[WIP] Client sub delegation~~ Client sub delegation Jan 21, 2025

davidlaprade added 16 commits January 21, 2025 13:58

Initial implementation of sub-delegation

e33de24

Add more test TODOs

4dc5df6

Am I bad person?

a7a0a26

Add more delegation tests

6bc6e89

Add more tests

94644ec

Get tests passing

6562f65

forge fmt

78254c9

Didn't need to do this after all

aaa9953

Simplify subdelegation implementations

751454b

Clean up the mess left by forge fmt

d227c10

Test changing delegates

a8160d3

Rename all the things

3ad35c0

forge fmt

da4ba56

Restructure inheritance for FlexVotingClient

05c57fb

Add contract description for FlexVotingDelegatable

3d058c7

forge fmt

83c50d8

davidlaprade force-pushed the client-sub-delegation branch from c3d4c08 to 83c50d8 Compare January 21, 2025 18:58

davidlaprade requested a review from apbendi January 21, 2025 18:58

davidlaprade mentioned this pull request Apr 1, 2025

[WIP] Multi governor client #84

Open

apbendi reviewed Apr 3, 2025

View reviewed changes

Fix spelling error

cb90ea6

rename Delegatable --> Delegable

3e16491

github-advanced-security bot found potential problems Apr 9, 2025

View reviewed changes

davidlaprade added 2 commits April 9, 2025 16:34

Fix tests

1e1d9bb

Add reference to github issue RE function naming

345804b

davidlaprade mentioned this pull request Apr 15, 2025

Troubleshoot forge fmt CI failures #89

Closed

Disable nightly warning to stop forge fmt from failing

178c0f0

davidlaprade force-pushed the client-sub-delegation branch from f4fcc00 to 178c0f0 Compare April 15, 2025 18:47

Client sub delegation #82

Are you sure you want to change the base?

Client sub delegation #82

Uh oh!

Conversation

davidlaprade commented Jan 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Check warning

Check warning

Check warning

Check warning

Uh oh!

davidlaprade commented Jan 21, 2025

Uh oh!

apbendi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Check warning

Check warning

davidlaprade commented Apr 15, 2025

Uh oh!

github-actions bot commented Apr 15, 2025

Uh oh!

Uh oh!

davidlaprade commented Jan 15, 2025 •

edited

Loading