Use Trace208 in Votes to support ERC6372 clocks (#4539)

Co-authored-by: Francisco <fg@frang.io>

.changeset/wild-peas-remain.md

@@ -0,0 +1,5 @@
+---
+'openzeppelin-solidity': major
+---
+
+`Votes`: Use Trace208 for checkpoints. This enables EIP-6372 clock support for keys but reduces the max supported voting power to uint208.

.github/workflows/checks.yml

@@ -63,6 +63,7 @@ jobs:
         run: npm run test:inheritance
       - name: Check storage layout
         uses: ./.github/actions/storage-layout
+        continue-on-error: ${{ contains(github.event.pull_request.labels.*.name, 'breaking change') }}
         with:
           token: ${{ github.token }}
 

contracts/governance/extensions/GovernorVotesQuorumFraction.sol

@@ -12,9 +12,9 @@ import {Checkpoints} from "../../utils/structs/Checkpoints.sol";
  * fraction of the total supply.
  */
 abstract contract GovernorVotesQuorumFraction is GovernorVotes {
-    using Checkpoints for Checkpoints.Trace224;
+    using Checkpoints for Checkpoints.Trace208;
 
-    Checkpoints.Trace224 private _quorumNumeratorHistory;
+    Checkpoints.Trace208 private _quorumNumeratorHistory;
 
     event QuorumNumeratorUpdated(uint256 oldQuorumNumerator, uint256 newQuorumNumerator);
 
@@ -49,15 +49,15 @@ abstract contract GovernorVotesQuorumFraction is GovernorVotes {
         uint256 length = _quorumNumeratorHistory._checkpoints.length;
 
         // Optimistic search, check the latest checkpoint
-        Checkpoints.Checkpoint224 storage latest = _quorumNumeratorHistory._checkpoints[length - 1];
-        uint32 latestKey = latest._key;
-        uint224 latestValue = latest._value;
+        Checkpoints.Checkpoint208 memory latest = _quorumNumeratorHistory._checkpoints[length - 1];
+        uint48 latestKey = latest._key;
+        uint208 latestValue = latest._value;
         if (latestKey <= timepoint) {
             return latestValue;
         }
 
         // Otherwise, do the binary search
-        return _quorumNumeratorHistory.upperLookupRecent(SafeCast.toUint32(timepoint));
+        return _quorumNumeratorHistory.upperLookupRecent(SafeCast.toUint48(timepoint));
     }
 
     /**
@@ -104,7 +104,7 @@ abstract contract GovernorVotesQuorumFraction is GovernorVotes {
         }
 
         uint256 oldQuorumNumerator = quorumNumerator();
-        _quorumNumeratorHistory.push(SafeCast.toUint32(clock()), SafeCast.toUint224(newQuorumNumerator));
+        _quorumNumeratorHistory.push(clock(), SafeCast.toUint208(newQuorumNumerator));
 
         emit QuorumNumeratorUpdated(oldQuorumNumerator, newQuorumNumerator);
     }

contracts/governance/utils/Votes.sol

@@ -29,16 +29,16 @@ import {ECDSA} from "../../utils/cryptography/ECDSA.sol";
  * previous example, it would be included in {ERC721-_beforeTokenTransfer}).
  */
 abstract contract Votes is Context, EIP712, Nonces, IERC5805 {
-    using Checkpoints for Checkpoints.Trace224;
+    using Checkpoints for Checkpoints.Trace208;
 
     bytes32 private constant DELEGATION_TYPEHASH =
         keccak256("Delegation(address delegatee,uint256 nonce,uint256 expiry)");
 
     mapping(address account => address) private _delegatee;
 
-    mapping(address delegatee => Checkpoints.Trace224) private _delegateCheckpoints;
+    mapping(address delegatee => Checkpoints.Trace208) private _delegateCheckpoints;
 
-    Checkpoints.Trace224 private _totalCheckpoints;
+    Checkpoints.Trace208 private _totalCheckpoints;
 
     /**
      * @dev The clock was incorrectly modified.
@@ -90,7 +90,7 @@ abstract contract Votes is Context, EIP712, Nonces, IERC5805 {
         if (timepoint >= currentTimepoint) {
             revert ERC5805FutureLookup(timepoint, currentTimepoint);
         }
-        return _delegateCheckpoints[account].upperLookupRecent(SafeCast.toUint32(timepoint));
+        return _delegateCheckpoints[account].upperLookupRecent(SafeCast.toUint48(timepoint));
     }
 
     /**
@@ -110,7 +110,7 @@ abstract contract Votes is Context, EIP712, Nonces, IERC5805 {
         if (timepoint >= currentTimepoint) {
             revert ERC5805FutureLookup(timepoint, currentTimepoint);
         }
-        return _totalCheckpoints.upperLookupRecent(SafeCast.toUint32(timepoint));
+        return _totalCheckpoints.upperLookupRecent(SafeCast.toUint48(timepoint));
     }
 
     /**
@@ -178,10 +178,10 @@ abstract contract Votes is Context, EIP712, Nonces, IERC5805 {
      */
     function _transferVotingUnits(address from, address to, uint256 amount) internal virtual {
         if (from == address(0)) {
-            _push(_totalCheckpoints, _add, SafeCast.toUint224(amount));
+            _push(_totalCheckpoints, _add, SafeCast.toUint208(amount));
         }
         if (to == address(0)) {
-            _push(_totalCheckpoints, _subtract, SafeCast.toUint224(amount));
+            _push(_totalCheckpoints, _subtract, SafeCast.toUint208(amount));
         }
         _moveDelegateVotes(delegates(from), delegates(to), amount);
     }
@@ -195,7 +195,7 @@ abstract contract Votes is Context, EIP712, Nonces, IERC5805 {
                 (uint256 oldValue, uint256 newValue) = _push(
                     _delegateCheckpoints[from],
                     _subtract,
-                    SafeCast.toUint224(amount)
+                    SafeCast.toUint208(amount)
                 );
                 emit DelegateVotesChanged(from, oldValue, newValue);
             }
@@ -203,7 +203,7 @@ abstract contract Votes is Context, EIP712, Nonces, IERC5805 {
                 (uint256 oldValue, uint256 newValue) = _push(
                     _delegateCheckpoints[to],
                     _add,
-                    SafeCast.toUint224(amount)
+                    SafeCast.toUint208(amount)
                 );
                 emit DelegateVotesChanged(to, oldValue, newValue);
             }
@@ -223,23 +223,23 @@ abstract contract Votes is Context, EIP712, Nonces, IERC5805 {
     function _checkpoints(
         address account,
         uint32 pos
-    ) internal view virtual returns (Checkpoints.Checkpoint224 memory) {
+    ) internal view virtual returns (Checkpoints.Checkpoint208 memory) {
         return _delegateCheckpoints[account].at(pos);
     }
 
     function _push(
-        Checkpoints.Trace224 storage store,
-        function(uint224, uint224) view returns (uint224) op,
-        uint224 delta
-    ) private returns (uint224, uint224) {
-        return store.push(SafeCast.toUint32(clock()), op(store.latest(), delta));
+        Checkpoints.Trace208 storage store,
+        function(uint208, uint208) view returns (uint208) op,
+        uint208 delta
+    ) private returns (uint208, uint208) {
+        return store.push(clock(), op(store.latest(), delta));
     }
 
-    function _add(uint224 a, uint224 b) private pure returns (uint224) {
+    function _add(uint208 a, uint208 b) private pure returns (uint208) {
         return a + b;
     }
 
-    function _subtract(uint224 a, uint224 b) private pure returns (uint224) {
+    function _subtract(uint208 a, uint208 b) private pure returns (uint208) {
         return a - b;
     }
 

contracts/token/ERC20/extensions/ERC20Votes.sol

@@ -9,7 +9,7 @@ import {Checkpoints} from "../../../utils/structs/Checkpoints.sol";
 
 /**
  * @dev Extension of ERC20 to support Compound-like voting and delegation. This version is more generic than Compound's,
- * and supports token supply up to 2^224^ - 1, while COMP is limited to 2^96^ - 1.
+ * and supports token supply up to 2^208^ - 1, while COMP is limited to 2^96^ - 1.
  *
  * NOTE: This contract does not provide interface compatibility with Compound's COMP token.
  *
@@ -27,10 +27,17 @@ abstract contract ERC20Votes is ERC20, Votes {
     error ERC20ExceededSafeSupply(uint256 increasedSupply, uint256 cap);
 
     /**
-     * @dev Maximum token supply. Defaults to `type(uint224).max` (2^224^ - 1).
+     * @dev Maximum token supply. Defaults to `type(uint208).max` (2^208^ - 1).
+     *
+     * This maximum is enforced in {_update}. It limits the total supply of the token, which is otherwise a uint256,
+     * so that checkpoints can be stored in the Trace208 structure used by {{Votes}}. Increasing this value will not
+     * remove the underlying limitation, and will cause {_update} to fail because of a math overflow in
+     * {_transferVotingUnits}. An override could be used to further restrict the total supply (to a lower value) if
+     * additional logic requires it. When resolving override conflicts on this function, the minimum should be
+     * returned.
      */
-    function _maxSupply() internal view virtual returns (uint224) {
-        return type(uint224).max;
+    function _maxSupply() internal view virtual returns (uint256) {
+        return type(uint208).max;
     }
 
     /**
@@ -70,7 +77,7 @@ abstract contract ERC20Votes is ERC20, Votes {
     /**
      * @dev Get the `pos`-th checkpoint for `account`.
      */
-    function checkpoints(address account, uint32 pos) public view virtual returns (Checkpoints.Checkpoint224 memory) {
+    function checkpoints(address account, uint32 pos) public view virtual returns (Checkpoints.Checkpoint208 memory) {
         return _checkpoints(account, pos);
     }
 }

contracts/utils/structs/Checkpoints.sol

@@ -206,6 +206,193 @@ library Checkpoints {
         }
     }
 
+    struct Trace208 {
+        Checkpoint208[] _checkpoints;
+    }
+
+    struct Checkpoint208 {
+        uint48 _key;
+        uint208 _value;
+    }
+
+    /**
+     * @dev Pushes a (`key`, `value`) pair into a Trace208 so that it is stored as the checkpoint.
+     *
+     * Returns previous value and new value.
+     *
+     * IMPORTANT: Never accept `key` as a user input, since an arbitrary `type(uint48).max` key set will disable the library.
+     */
+    function push(Trace208 storage self, uint48 key, uint208 value) internal returns (uint208, uint208) {
+        return _insert(self._checkpoints, key, value);
+    }
+
+    /**
+     * @dev Returns the value in the first (oldest) checkpoint with key greater or equal than the search key, or zero if there is none.
+     */
+    function lowerLookup(Trace208 storage self, uint48 key) internal view returns (uint208) {
+        uint256 len = self._checkpoints.length;
+        uint256 pos = _lowerBinaryLookup(self._checkpoints, key, 0, len);
+        return pos == len ? 0 : _unsafeAccess(self._checkpoints, pos)._value;
+    }
+
+    /**
+     * @dev Returns the value in the last (most recent) checkpoint with key lower or equal than the search key, or zero if there is none.
+     */
+    function upperLookup(Trace208 storage self, uint48 key) internal view returns (uint208) {
+        uint256 len = self._checkpoints.length;
+        uint256 pos = _upperBinaryLookup(self._checkpoints, key, 0, len);
+        return pos == 0 ? 0 : _unsafeAccess(self._checkpoints, pos - 1)._value;
+    }
+
+    /**
+     * @dev Returns the value in the last (most recent) checkpoint with key lower or equal than the search key, or zero if there is none.
+     *
+     * NOTE: This is a variant of {upperLookup} that is optimised to find "recent" checkpoint (checkpoints with high keys).
+     */
+    function upperLookupRecent(Trace208 storage self, uint48 key) internal view returns (uint208) {
+        uint256 len = self._checkpoints.length;
+
+        uint256 low = 0;
+        uint256 high = len;
+
+        if (len > 5) {
+            uint256 mid = len - Math.sqrt(len);
+            if (key < _unsafeAccess(self._checkpoints, mid)._key) {
+                high = mid;
+            } else {
+                low = mid + 1;
+            }
+        }
+
+        uint256 pos = _upperBinaryLookup(self._checkpoints, key, low, high);
+
+        return pos == 0 ? 0 : _unsafeAccess(self._checkpoints, pos - 1)._value;
+    }
+
+    /**
+     * @dev Returns the value in the most recent checkpoint, or zero if there are no checkpoints.
+     */
+    function latest(Trace208 storage self) internal view returns (uint208) {
+        uint256 pos = self._checkpoints.length;
+        return pos == 0 ? 0 : _unsafeAccess(self._checkpoints, pos - 1)._value;
+    }
+
+    /**
+     * @dev Returns whether there is a checkpoint in the structure (i.e. it is not empty), and if so the key and value
+     * in the most recent checkpoint.
+     */
+    function latestCheckpoint(Trace208 storage self) internal view returns (bool exists, uint48 _key, uint208 _value) {
+        uint256 pos = self._checkpoints.length;
+        if (pos == 0) {
+            return (false, 0, 0);
+        } else {
+            Checkpoint208 memory ckpt = _unsafeAccess(self._checkpoints, pos - 1);
+            return (true, ckpt._key, ckpt._value);
+        }
+    }
+
+    /**
+     * @dev Returns the number of checkpoint.
+     */
+    function length(Trace208 storage self) internal view returns (uint256) {
+        return self._checkpoints.length;
+    }
+
+    /**
+     * @dev Returns checkpoint at given position.
+     */
+    function at(Trace208 storage self, uint32 pos) internal view returns (Checkpoint208 memory) {
+        return self._checkpoints[pos];
+    }
+
+    /**
+     * @dev Pushes a (`key`, `value`) pair into an ordered list of checkpoints, either by inserting a new checkpoint,
+     * or by updating the last one.
+     */
+    function _insert(Checkpoint208[] storage self, uint48 key, uint208 value) private returns (uint208, uint208) {
+        uint256 pos = self.length;
+
+        if (pos > 0) {
+            // Copying to memory is important here.
+            Checkpoint208 memory last = _unsafeAccess(self, pos - 1);
+
+            // Checkpoint keys must be non-decreasing.
+            if (last._key > key) {
+                revert CheckpointUnorderedInsertion();
+            }
+
+            // Update or push new checkpoint
+            if (last._key == key) {
+                _unsafeAccess(self, pos - 1)._value = value;
+            } else {
+                self.push(Checkpoint208({_key: key, _value: value}));
+            }
+            return (last._value, value);
+        } else {
+            self.push(Checkpoint208({_key: key, _value: value}));
+            return (0, value);
+        }
+    }
+
+    /**
+     * @dev Return the index of the last (most recent) checkpoint with key lower or equal than the search key, or `high` if there is none.
+     * `low` and `high` define a section where to do the search, with inclusive `low` and exclusive `high`.
+     *
+     * WARNING: `high` should not be greater than the array's length.
+     */
+    function _upperBinaryLookup(
+        Checkpoint208[] storage self,
+        uint48 key,
+        uint256 low,
+        uint256 high
+    ) private view returns (uint256) {
+        while (low < high) {
+            uint256 mid = Math.average(low, high);
+            if (_unsafeAccess(self, mid)._key > key) {
+                high = mid;
+            } else {
+                low = mid + 1;
+            }
+        }
+        return high;
+    }
+
+    /**
+     * @dev Return the index of the first (oldest) checkpoint with key is greater or equal than the search key, or `high` if there is none.
+     * `low` and `high` define a section where to do the search, with inclusive `low` and exclusive `high`.
+     *
+     * WARNING: `high` should not be greater than the array's length.
+     */
+    function _lowerBinaryLookup(
+        Checkpoint208[] storage self,
+        uint48 key,
+        uint256 low,
+        uint256 high
+    ) private view returns (uint256) {
+        while (low < high) {
+            uint256 mid = Math.average(low, high);
+            if (_unsafeAccess(self, mid)._key < key) {
+                low = mid + 1;
+            } else {
+                high = mid;
+            }
+        }
+        return high;
+    }
+
+    /**
+     * @dev Access an element of the array without performing bounds check. The position is assumed to be within bounds.
+     */
+    function _unsafeAccess(
+        Checkpoint208[] storage self,
+        uint256 pos
+    ) private pure returns (Checkpoint208 storage result) {
+        assembly {
+            mstore(0, self.slot)
+            result.slot := add(keccak256(0, 0x20), pos)
+        }
+    }
+
     struct Trace160 {
         Checkpoint160[] _checkpoints;
     }

scripts/generate/templates/Checkpoints.opts.js

@@ -1,5 +1,5 @@
 // OPTIONS
-const VALUE_SIZES = [224, 160];
+const VALUE_SIZES = [224, 208, 160];
 
 const defaultOpts = size => ({
   historyTypeName: `Trace${size}`,

test/token/ERC20/extensions/ERC20Votes.test.js

@@ -48,7 +48,7 @@ contract('ERC20Votes', function (accounts) {
       });
 
       it('minting restriction', async function () {
-        const value = web3.utils.toBN(1).shln(224);
+        const value = web3.utils.toBN(1).shln(208);
         await expectRevertCustomError(this.token.$_mint(holder, value), 'ERC20ExceededSafeSupply', [
           value,
           value.subn(1),

test/utils/structs/Checkpoints.t.sol

@@ -115,6 +115,114 @@ contract CheckpointsTrace224Test is Test {
     }
 }
 
+contract CheckpointsTrace208Test is Test {
+    using Checkpoints for Checkpoints.Trace208;
+
+    // Maximum gap between keys used during the fuzzing tests: the `_prepareKeys` function with make sure that
+    // key#n+1 is in the [key#n, key#n + _KEY_MAX_GAP] range.
+    uint8 internal constant _KEY_MAX_GAP = 64;
+
+    Checkpoints.Trace208 internal _ckpts;
+
+    // helpers
+    function _boundUint48(uint48 x, uint48 min, uint48 max) internal view returns (uint48) {
+        return SafeCast.toUint48(bound(uint256(x), uint256(min), uint256(max)));
+    }
+
+    function _prepareKeys(uint48[] memory keys, uint48 maxSpread) internal view {
+        uint48 lastKey = 0;
+        for (uint256 i = 0; i < keys.length; ++i) {
+            uint48 key = _boundUint48(keys[i], lastKey, lastKey + maxSpread);
+            keys[i] = key;
+            lastKey = key;
+        }
+    }
+
+    function _assertLatestCheckpoint(bool exist, uint48 key, uint208 value) internal {
+        (bool _exist, uint48 _key, uint208 _value) = _ckpts.latestCheckpoint();
+        assertEq(_exist, exist);
+        assertEq(_key, key);
+        assertEq(_value, value);
+    }
+
+    // tests
+    function testPush(uint48[] memory keys, uint208[] memory values, uint48 pastKey) public {
+        vm.assume(values.length > 0 && values.length <= keys.length);
+        _prepareKeys(keys, _KEY_MAX_GAP);
+
+        // initial state
+        assertEq(_ckpts.length(), 0);
+        assertEq(_ckpts.latest(), 0);
+        _assertLatestCheckpoint(false, 0, 0);
+
+        uint256 duplicates = 0;
+        for (uint256 i = 0; i < keys.length; ++i) {
+            uint48 key = keys[i];
+            uint208 value = values[i % values.length];
+            if (i > 0 && key == keys[i - 1]) ++duplicates;
+
+            // push
+            _ckpts.push(key, value);
+
+            // check length & latest
+            assertEq(_ckpts.length(), i + 1 - duplicates);
+            assertEq(_ckpts.latest(), value);
+            _assertLatestCheckpoint(true, key, value);
+        }
+
+        if (keys.length > 0) {
+            uint48 lastKey = keys[keys.length - 1];
+            if (lastKey > 0) {
+                pastKey = _boundUint48(pastKey, 0, lastKey - 1);
+
+                vm.expectRevert();
+                this.push(pastKey, values[keys.length % values.length]);
+            }
+        }
+    }
+
+    // used to test reverts
+    function push(uint48 key, uint208 value) external {
+        _ckpts.push(key, value);
+    }
+
+    function testLookup(uint48[] memory keys, uint208[] memory values, uint48 lookup) public {
+        vm.assume(values.length > 0 && values.length <= keys.length);
+        _prepareKeys(keys, _KEY_MAX_GAP);
+
+        uint48 lastKey = keys.length == 0 ? 0 : keys[keys.length - 1];
+        lookup = _boundUint48(lookup, 0, lastKey + _KEY_MAX_GAP);
+
+        uint208 upper = 0;
+        uint208 lower = 0;
+        uint48 lowerKey = type(uint48).max;
+        for (uint256 i = 0; i < keys.length; ++i) {
+            uint48 key = keys[i];
+            uint208 value = values[i % values.length];
+
+            // push
+            _ckpts.push(key, value);
+
+            // track expected result of lookups
+            if (key <= lookup) {
+                upper = value;
+            }
+            // find the first key that is not smaller than the lookup key
+            if (key >= lookup && (i == 0 || keys[i - 1] < lookup)) {
+                lowerKey = key;
+            }
+            if (key == lowerKey) {
+                lower = value;
+            }
+        }
+
+        // check lookup
+        assertEq(_ckpts.lowerLookup(lookup), lower);
+        assertEq(_ckpts.upperLookup(lookup), upper);
+        assertEq(_ckpts.upperLookupRecent(lookup), upper);
+    }
+}
+
 contract CheckpointsTrace160Test is Test {
     using Checkpoints for Checkpoints.Trace160;