From 005c2cc65edabefb94e3df7ad00069045bb43981 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 30 Aug 2023 19:25:17 +0200 Subject: [PATCH] Use Trace208 in Votes to support ERC6372 clocks (#4539) Co-authored-by: Francisco --- .changeset/wild-peas-remain.md | 5 + .github/workflows/checks.yml | 1 + .../GovernorVotesQuorumFraction.sol | 14 +- contracts/governance/utils/Votes.sol | 34 ++-- .../token/ERC20/extensions/ERC20Votes.sol | 17 +- contracts/utils/structs/Checkpoints.sol | 187 ++++++++++++++++++ .../generate/templates/Checkpoints.opts.js | 2 +- .../token/ERC20/extensions/ERC20Votes.test.js | 2 +- test/utils/structs/Checkpoints.t.sol | 108 ++++++++++ 9 files changed, 339 insertions(+), 31 deletions(-) create mode 100644 .changeset/wild-peas-remain.md diff --git a/.changeset/wild-peas-remain.md b/.changeset/wild-peas-remain.md new file mode 100644 index 00000000000..83b4bb30765 --- /dev/null +++ b/.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. diff --git a/.github/workflows/checks.yml b/.github/workflows/checks.yml index 122d3956413..199d9e36f1f 100644 --- a/.github/workflows/checks.yml +++ b/.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 }} diff --git a/contracts/governance/extensions/GovernorVotesQuorumFraction.sol b/contracts/governance/extensions/GovernorVotesQuorumFraction.sol index 774bff6b1b9..5346fe4e989 100644 --- a/contracts/governance/extensions/GovernorVotesQuorumFraction.sol +++ b/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); } diff --git a/contracts/governance/utils/Votes.sol b/contracts/governance/utils/Votes.sol index 11476eb554c..d9f5e01bb9f 100644 --- a/contracts/governance/utils/Votes.sol +++ b/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; } diff --git a/contracts/token/ERC20/extensions/ERC20Votes.sol b/contracts/token/ERC20/extensions/ERC20Votes.sol index a4ded6b2467..0ec1e6059d5 100644 --- a/contracts/token/ERC20/extensions/ERC20Votes.sol +++ 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); } } diff --git a/contracts/utils/structs/Checkpoints.sol b/contracts/utils/structs/Checkpoints.sol index 383f01af8fa..d0579f8998c 100644 --- a/contracts/utils/structs/Checkpoints.sol +++ b/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; } diff --git a/scripts/generate/templates/Checkpoints.opts.js b/scripts/generate/templates/Checkpoints.opts.js index b8be23104d5..08b7b910b84 100644 --- a/scripts/generate/templates/Checkpoints.opts.js +++ b/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}`, diff --git a/test/token/ERC20/extensions/ERC20Votes.test.js b/test/token/ERC20/extensions/ERC20Votes.test.js index a6abb5f9fac..faf1a15adb4 100644 --- a/test/token/ERC20/extensions/ERC20Votes.test.js +++ b/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), diff --git a/test/utils/structs/Checkpoints.t.sol b/test/utils/structs/Checkpoints.t.sol index afda2423ed3..7bdbcfddfbc 100644 --- a/test/utils/structs/Checkpoints.t.sol +++ b/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;