Iterate following discussion with Tally

- use public facing functions for override instead of params
- rename some fields/variables
- cleanup/implification

contracts/governance/Governor.sol

@@ -731,7 +731,7 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72
      *
      * If requirements are not met, reverts with a {GovernorUnexpectedProposalState} error.
      */
-    function _validateStateBitmap(uint256 proposalId, bytes32 allowedStates) private view returns (ProposalState) {
+    function _validateStateBitmap(uint256 proposalId, bytes32 allowedStates) internal view returns (ProposalState) {
         ProposalState currentState = state(proposalId);
         if (_encodeStateBitmap(currentState) & allowedStates == bytes32(0)) {
             revert GovernorUnexpectedProposalState(proposalId, currentState, allowedStates);

contracts/governance/extensions/GovernorOverrideDelegateVote.sol

@@ -2,18 +2,19 @@
 
 pragma solidity ^0.8.20;
 
-import {GovernorVotes} from "./GovernorVotes.sol";
-import {IVotes} from "../utils/IVotes.sol";
-import {IERC5805} from "../../interfaces/IERC5805.sol";
+import {SignatureChecker} from "../../utils/cryptography/SignatureChecker.sol";
 import {SafeCast} from "../../utils/math/SafeCast.sol";
-import {Time} from "../../utils/types/Time.sol";
 import {VotesOverridable} from "../utils/VotesOverridable.sol";
+import {GovernorVotes} from "./GovernorVotes.sol";
 
 /**
  * @dev Extension of {Governor} which enables delegatees to override the vote of their delegates. This module requires a
  * token token that inherits `VotesOverridable`.
  */
 abstract contract GovernorOverrideDelegateVote is GovernorVotes {
+    bytes32 public constant OVERRIDE_TYPEHASH =
+        keccak256("Override(uint256 proposalId,uint8 support,address voter,uint256 nonce)");
+
     /**
      * @dev Supported vote types. Matches Governor Bravo ordering.
      */
@@ -24,37 +25,33 @@ abstract contract GovernorOverrideDelegateVote is GovernorVotes {
     }
 
     struct VoteReceipt {
-        uint8 support;
+        uint8 casted; // 0 if vote was not casted. Otherwise: support + 1
         bool hasOverriden;
-        uint208 overrideWeight;
+        uint208 overridenWeight;
     }
 
     struct ProposalVote {
-        uint256 againstVotes;
-        uint256 forVotes;
-        uint256 abstainVotes;
+        uint256[3] votes;
         mapping(address voter => VoteReceipt) voteReceipt;
     }
 
     error GovernorAlreadyCastVoteOverride(address account);
 
     mapping(uint256 proposalId => ProposalVote) private _proposalVotes;
 
-    constructor(VotesOverridable tokenAddress) GovernorVotes(tokenAddress) {}
-
     /**
      * @dev See {IGovernor-COUNTING_MODE}.
      */
     // solhint-disable-next-line func-name-mixedcase
     function COUNTING_MODE() public pure virtual override returns (string memory) {
-        return "support=bravo,override&quorum=for,abstain&params=override";
+        return "support=bravo,override&quorum=for,abstain&overridable=true";
     }
 
     /**
      * @dev See {IGovernor-hasVoted}.
      */
     function hasVoted(uint256 proposalId, address account) public view virtual override returns (bool) {
-        return _proposalVotes[proposalId].voteReceipt[account].support != 0;
+        return _proposalVotes[proposalId].voteReceipt[account].casted != 0;
     }
 
     /**
@@ -70,112 +67,112 @@ abstract contract GovernorOverrideDelegateVote is GovernorVotes {
     function proposalVotes(
         uint256 proposalId
     ) public view virtual returns (uint256 againstVotes, uint256 forVotes, uint256 abstainVotes) {
-        ProposalVote storage proposalVote = _proposalVotes[proposalId];
-        return (proposalVote.againstVotes, proposalVote.forVotes, proposalVote.abstainVotes);
+        uint256[3] storage votes = _proposalVotes[proposalId].votes;
+        return (votes[uint8(VoteType.Against)], votes[uint8(VoteType.For)], votes[uint8(VoteType.Abstain)]);
     }
 
     /**
      * @dev See {Governor-_quorumReached}.
      */
     function _quorumReached(uint256 proposalId) internal view virtual override returns (bool) {
-        ProposalVote storage proposalVote = _proposalVotes[proposalId];
-
-        return quorum(proposalSnapshot(proposalId)) <= proposalVote.forVotes + proposalVote.abstainVotes;
+        uint256[3] storage votes = _proposalVotes[proposalId].votes;
+        return quorum(proposalSnapshot(proposalId)) <= votes[uint8(VoteType.For)] + votes[uint8(VoteType.Abstain)];
     }
 
     /**
      * @dev See {Governor-_voteSucceeded}. In this module, the forVotes must be strictly over the againstVotes.
      */
     function _voteSucceeded(uint256 proposalId) internal view virtual override returns (bool) {
-        ProposalVote storage proposalVote = _proposalVotes[proposalId];
-
-        return proposalVote.forVotes > proposalVote.againstVotes;
+        uint256[3] storage votes = _proposalVotes[proposalId].votes;
+        return votes[uint8(VoteType.For)] > votes[uint8(VoteType.Against)];
     }
 
+    /**
+     * @dev See {Governor-_countVote}. In this module, the support follows the `VoteType` enum (from Governor Bravo).
+     *
+     * NOTE: called by {Governor-_castVote} which emits the {IGovernor-VoteCast} (or {IGovernor-VoteCastWithParams})
+     * event.
+     */
     function _countVote(
         uint256 proposalId,
         address account,
         uint8 support,
         uint256 totalWeight,
-        bytes memory params
+        bytes memory /*params*/
     ) internal virtual override returns (uint256) {
-        if (keccak256(params) == keccak256(hex"23b70c8d0000000000000000000000000000000000000000")) {
-            return _countVotesOverride(proposalId, account, support, params);
-        }
-
         ProposalVote storage proposalVote = _proposalVotes[proposalId];
 
-        totalWeight -= proposalVote.voteReceipt[account].overrideWeight;
+        if (support > uint8(VoteType.Abstain)) {
+            revert GovernorInvalidVoteType();
+        }
 
-        if (proposalVote.voteReceipt[account].support != 0) {
+        if (proposalVote.voteReceipt[account].casted == 0) {
             revert GovernorAlreadyCastVote(account);
         }
-        // Support tracks support and if a user voted. Store support + 1.
-        proposalVote.voteReceipt[account].support = support + 1;
 
-        _tallyVote(proposalVote, support, _add, totalWeight);
+        totalWeight -= proposalVote.voteReceipt[account].overridenWeight;
+        proposalVote.votes[support] += totalWeight;
+        proposalVote.voteReceipt[account].casted = support + 1;
 
         return totalWeight;
     }
 
-    function _countVotesOverride(
-        uint256 proposalId,
-        address account,
-        uint8 support,
-        bytes memory params
-    ) private returns (uint256) {
+    function _overrideVote(uint256 proposalId, address account, uint8 support) internal virtual returns (uint256) {
+        if (support > uint8(VoteType.Abstain)) {
+            revert GovernorInvalidVoteType();
+        }
+
         ProposalVote storage proposalVote = _proposalVotes[proposalId];
-        uint256 proposalSnapshot = proposalSnapshot(proposalId);
-        address delegate = VotesOverridable(address(token())).getPastDelegate(account, proposalSnapshot);
 
         if (proposalVote.voteReceipt[account].hasOverriden) {
             revert GovernorAlreadyCastVoteOverride(account);
         }
-        proposalVote.voteReceipt[account].hasOverriden = true;
 
-        uint256 overrideWeight = VotesOverridable(address(token())).getPastBalanceOf(account, proposalSnapshot);
-        _tallyVote(proposalVote, support, _add, overrideWeight);
-
-        // Account for the delegate's vote
-        VoteReceipt memory delegateVoteReceipt = proposalVote.voteReceipt[delegate];
-        if (delegateVoteReceipt.support != 0) {
-            uint8 correctedSupport = delegateVoteReceipt.support - 1;
-            // If delegate has voted, remove the delegatee's vote weight from their support
-            _tallyVote(proposalVote, correctedSupport, _subtract, overrideWeight);
+        uint256 proposalSnapshot = proposalSnapshot(proposalId);
+        uint256 overridenWeight = VotesOverridable(address(token())).getPastBalanceOf(account, proposalSnapshot);
+        address delegate = VotesOverridable(address(token())).getPastDelegate(account, proposalSnapshot);
+        uint8 delegateCasted = proposalVote.voteReceipt[delegate].casted;
 
-            // Write delegate into the params for event
-            assembly {
-                mstore(add(params, 0x20), or(mload(add(params, 0x20)), shl(64, delegate)))
-            }
+        proposalVote.voteReceipt[account].hasOverriden = true;
+        proposalVote.votes[support] += overridenWeight;
+        if (delegateCasted == 0) {
+            proposalVote.voteReceipt[delegate].overridenWeight += SafeCast.toUint208(overridenWeight);
+            // TODO: emit event VoteCast ?
         } else {
-            // Only write override weight if they have not voted yet
-            proposalVote.voteReceipt[delegate].overrideWeight += uint208(overrideWeight);
+            proposalVote.votes[delegateCasted - 1] -= overridenWeight;
+            // TODO: emit event VoteCastOverride ?
         }
-        return overrideWeight;
-    }
 
-    function _tallyVote(
-        ProposalVote storage proposalVote,
-        uint8 support,
-        function(uint256, uint256) view returns (uint256) op,
-        uint256 delta
-    ) private {
-        if (support == uint8(VoteType.Against)) {
-            proposalVote.againstVotes = op(proposalVote.againstVotes, delta);
-        } else if (support == uint8(VoteType.For)) {
-            proposalVote.forVotes = op(proposalVote.forVotes, delta);
-        } else if (support == uint8(VoteType.Abstain)) {
-            proposalVote.abstainVotes = op(proposalVote.abstainVotes, delta);
-        } else {
-            revert GovernorInvalidVoteType();
-        }
+        return overridenWeight;
     }
 
-    function _add(uint256 a, uint256 b) private pure returns (uint256) {
-        return a + b;
+    function overrideVote(uint256 proposalId, uint8 support) public virtual returns (uint256) {
+        _validateStateBitmap(proposalId, _encodeStateBitmap(ProposalState.Active));
+
+        address voter = _msgSender();
+        return _overrideVote(proposalId, voter, support);
     }
 
-    function _subtract(uint256 a, uint256 b) private pure returns (uint256) {
-        return a - b;
+    function overrideVoteBySig(
+        uint256 proposalId,
+        uint8 support,
+        address voter,
+        bytes memory signature
+    ) public virtual returns (uint256) {
+        _validateStateBitmap(proposalId, _encodeStateBitmap(ProposalState.Active));
+
+        if (
+            !SignatureChecker.isValidSignatureNow(
+                voter,
+                _hashTypedDataV4(
+                    keccak256(abi.encode(OVERRIDE_TYPEHASH, proposalId, support, voter, _useNonce(voter)))
+                ),
+                signature
+            )
+        ) {
+            revert GovernorInvalidSignature(voter);
+        }
+
+        return _overrideVote(proposalId, voter, support);
     }
 }

contracts/governance/utils/VotesOverridable.sol

@@ -10,19 +10,13 @@ import {SafeCast} from "../../utils/math/SafeCast.sol";
  * to use the `GovernorOverrideDelegateVote` extension.
  */
 abstract contract VotesOverridable is Votes {
+    using SafeCast for uint256;
+    using Checkpoints for Checkpoints.Trace160;
     using Checkpoints for Checkpoints.Trace208;
 
-    error VotesOverridableFutureLookup(uint256 timepoint, uint256 currentTimepoint);
-
-    mapping(address delegatee => Checkpoints.Trace208) private _delegateCheckpoints;
+    mapping(address delegatee => Checkpoints.Trace160) private _delegateCheckpoints;
     mapping(address account => Checkpoints.Trace208) private _balanceOfCheckpoints;
 
-    function _delegate(address account, address delegatee) internal virtual override {
-        super._delegate(account, delegatee);
-
-        _delegateCheckpoints[account].push(clock(), uint160(delegatee));
-    }
-
     /**
      * @dev Returns the delegate of an `account` at a specific moment in the past. If the `clock()` is
      * configured to use block numbers, this will return the value at the end of the corresponding block.
@@ -31,29 +25,12 @@ abstract contract VotesOverridable is Votes {
      *
      * - `timepoint` must be in the past. If operating using block numbers, the block must be already mined.
      */
-    function getPastDelegate(address account, uint256 timepoint) public view returns (address) {
+    function getPastDelegate(address account, uint256 timepoint) public view virtual returns (address) {
         uint48 currentTimepoint = clock();
         if (timepoint >= currentTimepoint) {
-            revert VotesOverridableFutureLookup(timepoint, currentTimepoint);
-        }
-        return address(uint160(_delegateCheckpoints[account].upperLookupRecent(SafeCast.toUint48(timepoint))));
-    }
-
-    /**
-     * @dev Extend functionality of the function by checkpointing balances.
-     */
-    function _transferVotingUnits(address from, address to, uint256 amount) internal virtual override {
-        super._transferVotingUnits(from, to, amount);
-        if (from != to) {
-            if (from != address(0)) {
-                Checkpoints.Trace208 storage store = _balanceOfCheckpoints[from];
-                store.push(clock(), uint208(_getVotingUnits(from)));
-            }
-            if (to != address(0)) {
-                Checkpoints.Trace208 storage store = _balanceOfCheckpoints[to];
-                store.push(clock(), uint208(_getVotingUnits(to)));
-            }
+            revert ERC5805FutureLookup(timepoint, currentTimepoint);
         }
+        return address(_delegateCheckpoints[account].upperLookupRecent(timepoint.toUint48()));
     }
 
     /**
@@ -64,12 +41,32 @@ abstract contract VotesOverridable is Votes {
      *
      * - `timepoint` must be in the past. If operating using block numbers, the block must be already mined.
      */
-    function getPastBalanceOf(address account, uint256 timepoint) public view returns (uint256) {
+    function getPastBalanceOf(address account, uint256 timepoint) public view virtual returns (uint256) {
         uint48 currentTimepoint = clock();
         if (timepoint >= currentTimepoint) {
             // Note this ERC is not relevant to the specific error. Should probably be a different error.
-            revert VotesOverridableFutureLookup(timepoint, currentTimepoint);
+            revert ERC5805FutureLookup(timepoint, currentTimepoint);
+        }
+        return _balanceOfCheckpoints[account].upperLookupRecent(timepoint.toUint48());
+    }
+
+    /// @inheritdoc Votes
+    function _delegate(address account, address delegatee) internal virtual override {
+        super._delegate(account, delegatee);
+
+        _delegateCheckpoints[account].push(clock(), uint160(delegatee));
+    }
+
+    /// @inheritdoc Votes
+    function _transferVotingUnits(address from, address to, uint256 amount) internal virtual override {
+        super._transferVotingUnits(from, to, amount);
+        if (from != to) {
+            if (from != address(0)) {
+                _balanceOfCheckpoints[from].push(clock(), _getVotingUnits(from).toUint208());
+            }
+            if (to != address(0)) {
+                _balanceOfCheckpoints[to].push(clock(), _getVotingUnits(to).toUint208());
+            }
         }
-        return _balanceOfCheckpoints[account].upperLookupRecent(SafeCast.toUint48(timepoint));
     }
 }