paranoia commit, revert

Author: davidlaprade

src/FlexVotingClient.sol

@@ -1,6 +1,8 @@
 // SPDX-License-Identifier: MIT
 pragma solidity ^0.8.20;
 
+import "forge-std/console2.sol";
+
 import {SafeCast} from "@openzeppelin/contracts/utils/math/SafeCast.sol";
 import {Checkpoints} from "@openzeppelin/contracts/utils/structs/Checkpoints.sol";
 import {IFractionalGovernor} from "src/interfaces/IFractionalGovernor.sol";
@@ -69,6 +71,7 @@ abstract contract FlexVotingClient {
     uint128 abstainVotes;
   }
 
+  /// TODO need to check that this still works
   /// @dev Map proposalId to an address to whether they have voted on this proposal.
   mapping(uint256 => mapping(address => bool)) private proposalVotersHasVoted;
 
@@ -112,6 +115,7 @@ abstract contract FlexVotingClient {
     return "rolled-up vote from governance token holders";
   }
 
+  // TODO rename to avoid collision with FlexVotingDelegatable.
   /// @dev Delegates the present contract's voting rights with `GOVERNOR` to itself.
   function _selfDelegate() internal {
     IVotingToken(GOVERNOR.token()).delegate(address(this));
@@ -202,13 +206,11 @@ abstract contract FlexVotingClient {
     );
   }
 
-  /// @dev Checkpoints the _user's current raw balance.
-  function _checkpointRawBalanceOf(address _user) internal {
-    balanceCheckpoints[_user].push(IVotingToken(GOVERNOR.token()).clock(), _rawBalanceOf(_user));
-  }
+  function _applyDeltaToCheckpoint(
+    Checkpoints.Trace208 storage _checkpoint,
+    int256 _delta
+  ) internal returns (uint208 _prevTotal, uint208 _newTotal) {
 
-  /// @dev Checkpoints the total balance after applying `_delta`.
-  function _checkpointTotalBalance(int256 _delta) internal {
     // The casting in this function is safe since:
     // - if oldTotal + delta > int256.max it will panic and revert.
     // - if |delta| <= oldTotal
@@ -226,12 +228,27 @@ abstract contract FlexVotingClient {
     //       uint256.max + int256.min > uint208.max
     //     Substituting again:
     //       wrapped(int256.min) > uint208.max, which will revert when safecast.
-    uint256 _oldTotal = uint256(totalBalanceCheckpoints.latest());
-    uint256 _newTotal = uint256(int256(_oldTotal) + _delta);
+    _prevTotal = _checkpoint.latest();
+    int256 _castTotal = int256(uint256(_prevTotal));
+    console2.log("david applyDelta", _castTotal);
+    console2.log("david applyDelta", _delta);
+    _newTotal = SafeCast.toUint208(uint256(_castTotal + _delta));
+
+    uint48 _timepoint = IVotingToken(GOVERNOR.token()).clock();
+    _checkpoint.push(_timepoint, _newTotal);
+  }
 
-    totalBalanceCheckpoints.push(
-      IVotingToken(GOVERNOR.token()).clock(), SafeCast.toUint208(_newTotal)
-    );
+  /// @dev Checkpoints the _user's current raw balance.
+  function _checkpointRawBalanceOf(
+    address _user,
+    int256 _delta
+  ) internal virtual returns (uint208 _prevTotal, uint208 _newTotal) {
+    (_prevTotal, _newTotal) = _applyDeltaToCheckpoint(balanceCheckpoints[_user], _delta);
+  }
+
+  /// @dev Checkpoints the total balance after applying `_delta`.
+  function _checkpointTotalBalance(int256 _delta) internal virtual {
+    _applyDeltaToCheckpoint(totalBalanceCheckpoints, _delta);
   }
 
   /// @notice Returns the `_user`'s raw balance at `_timepoint`.

src/FlexVotingDelegatable.sol

@@ -1,6 +1,8 @@
 // SPDX-License-Identifier: MIT
 pragma solidity ^0.8.20;
 
+import "forge-std/console2.sol";
+
 import {SafeCast} from "@openzeppelin/contracts/utils/math/SafeCast.sol";
 import {Context} from "@openzeppelin/contracts/utils/Context.sol";
 import {Checkpoints} from "@openzeppelin/contracts/utils/structs/Checkpoints.sol";
@@ -43,39 +45,34 @@ abstract contract FlexVotingDelegatable is Context, FlexVotingClient {
     address oldDelegate = delegates(account);
     _delegatee[account] = delegatee;
 
+    // TODO There is a bug here: if you didn't have a delegate before and you
+    // implicitly self-delegated, then _updateDelegateBalance will attempt to
+    // subtract from the old delegate's balance, which will be zero. This will
+    int256 _delta = int256(uint256(_rawBalanceOf(account)));
     emit DelegateChanged(account, oldDelegate, delegatee);
-    _updateDelegateBalance(oldDelegate, delegatee, _rawBalanceOf(account));
+    _updateDelegateBalance(oldDelegate, delegatee, _delta);
   }
 
-  // @dev Moves delegated votes from one delegate to another.
-  function _updateDelegateBalance(address from, address to, uint208 amount) internal virtual {
-    if (from == to || amount == 0) return;
-
-    if (from != address(0)) {
-      (uint256 oldValue, uint256 newValue) =
-        _push(FlexVotingClient.balanceCheckpoints[from], _subtract, amount);
-      emit DelegateWeightChanged(from, oldValue, newValue);
-    }
-    if (to != address(0)) {
-      (uint256 oldValue, uint256 newValue) =
-        _push(FlexVotingClient.balanceCheckpoints[to], _add, amount);
-      emit DelegateWeightChanged(to, oldValue, newValue);
-    }
+  function _checkpointRawBalanceOf(
+    address _user,
+    int256 _delta
+  ) internal virtual override returns (uint208 _prevTotal, uint208 _newTotal) {
+    address _proxy = delegates(_user);
+    // TODO it's a bit confusing to call "checkpoint" on a user but then get the
+    // user's delegate's voting weight back.
+    (_prevTotal, _newTotal) = _applyDeltaToCheckpoint(balanceCheckpoints[_proxy], _delta);
   }
 
-  function _push(
-    Checkpoints.Trace208 storage store,
-    function(uint208, uint208) view returns (uint208) fn,
-    uint208 delta
-  ) private returns (uint208 oldValue, uint208 newValue) {
-    return store.push(IVotingToken(GOVERNOR.token()).clock(), fn(store.latest(), 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;
 
-  function _add(uint208 a, uint208 b) private pure returns (uint208) {
-    return a + b;
-  }
+    // Decrement old delegate's weight.
+    (uint208 _oldFrom, uint208 _newFrom) = _checkpointRawBalanceOf(from, -_delta);
+    emit DelegateWeightChanged(from, _oldFrom, _newFrom);
 
-  function _subtract(uint208 a, uint208 b) private pure returns (uint208) {
-    return a - b;
+    // Increment new delegate's weight.
+    (uint208 _oldTo, uint208 _newTo) = _checkpointRawBalanceOf(to, _delta);
+    emit DelegateWeightChanged(to, _oldTo, _newTo);
   }
 }