From 62894b1de5fb5709cac920de1d470ed42d738cc2 Mon Sep 17 00:00:00 2001 From: RenanSouza2 Date: Sat, 8 Apr 2023 15:52:25 -0300 Subject: [PATCH 01/13] mid commit --- .../GovernorVotesQuorumFraction.sol | 11 +- contracts/governance/utils/Votes.sol | 14 +- .../token/ERC20/extensions/ERC20Votes.sol | 2 +- contracts/utils/Checkpoints.sol | 196 ------------------ package.json | 2 +- scripts/generate/templates/Checkpoints.js | 4 - 6 files changed, 16 insertions(+), 213 deletions(-) diff --git a/contracts/governance/extensions/GovernorVotesQuorumFraction.sol b/contracts/governance/extensions/GovernorVotesQuorumFraction.sol index 8efefce3959..80c189f30fb 100644 --- a/contracts/governance/extensions/GovernorVotesQuorumFraction.sol +++ b/contracts/governance/extensions/GovernorVotesQuorumFraction.sol @@ -14,10 +14,10 @@ import "../../utils/math/SafeCast.sol"; * _Available since v4.3._ */ abstract contract GovernorVotesQuorumFraction is GovernorVotes { - using Checkpoints for Checkpoints.History; + using Checkpoints for Checkpoints.Trace224; uint256 private _quorumNumerator; // DEPRECATED - Checkpoints.History private _quorumNumeratorHistory; + Checkpoints.Trace224 private _quorumNumeratorHistory; event QuorumNumeratorUpdated(uint256 oldQuorumNumerator, uint256 newQuorumNumerator); @@ -50,13 +50,14 @@ abstract contract GovernorVotesQuorumFraction is GovernorVotes { } // Optimistic search, check the latest checkpoint - Checkpoints.Checkpoint memory latest = _quorumNumeratorHistory._checkpoints[length - 1]; - if (latest._blockNumber <= blockNumber) { + Checkpoints.Checkpoint224 memory latest = _quorumNumeratorHistory._checkpoints[length - 1]; + if (latest._key <= blockNumber) { return latest._value; } // Otherwise, do the binary search - return _quorumNumeratorHistory.getAtBlock(blockNumber); + uint32 key = SafeCast.toUint32(blockNumber); + return _quorumNumeratorHistory.lowerLookup(key); } /** diff --git a/contracts/governance/utils/Votes.sol b/contracts/governance/utils/Votes.sol index ccac72bce2a..dd81762c402 100644 --- a/contracts/governance/utils/Votes.sol +++ b/contracts/governance/utils/Votes.sol @@ -30,14 +30,14 @@ import "../../utils/math/SafeCast.sol"; * _Available since v4.5._ */ abstract contract Votes is IVotes, Context, EIP712, Nonces { - using Checkpoints for Checkpoints.History; + using Checkpoints for Checkpoints.Trace224; bytes32 private constant _DELEGATION_TYPEHASH = keccak256("Delegation(address delegatee,uint256 nonce,uint256 expiry)"); mapping(address => address) private _delegation; - mapping(address => Checkpoints.History) private _delegateCheckpoints; - Checkpoints.History private _totalCheckpoints; + mapping(address => Checkpoints.Trace224) private _delegateCheckpoints; + Checkpoints.Trace224 private _totalCheckpoints; /** * @dev Returns the current amount of votes that `account` has. @@ -54,7 +54,8 @@ abstract contract Votes is IVotes, Context, EIP712, Nonces { * - `blockNumber` must have been already mined */ function getPastVotes(address account, uint256 blockNumber) public view virtual override returns (uint256) { - return _delegateCheckpoints[account].getAtProbablyRecentBlock(blockNumber); + uint32 key = SafeCast.toUint32(blockNumber); + return _delegateCheckpoints[account].upperLookup(key); } /** @@ -69,7 +70,8 @@ abstract contract Votes is IVotes, Context, EIP712, Nonces { * - `blockNumber` must have been already mined */ function getPastTotalSupply(uint256 blockNumber) public view virtual override returns (uint256) { - return _totalCheckpoints.getAtProbablyRecentBlock(blockNumber); + uint32 key = SafeCast.toUint32(blockNumber); + return _totalCheckpoints.upperLookup(key); } /** @@ -169,7 +171,7 @@ abstract contract Votes is IVotes, Context, EIP712, Nonces { /** * @dev Get the `pos`-th checkpoint for `account`. */ - function _checkpoints(address account, uint32 pos) internal view virtual returns (Checkpoints.Checkpoint memory) { + function _checkpoints(address account, uint32 pos) internal view virtual returns (Checkpoints.Checkpoint224 memory) { return _delegateCheckpoints[account].getAtPosition(pos); } diff --git a/contracts/token/ERC20/extensions/ERC20Votes.sol b/contracts/token/ERC20/extensions/ERC20Votes.sol index 225d08c443e..fe25b9bb506 100644 --- a/contracts/token/ERC20/extensions/ERC20Votes.sol +++ b/contracts/token/ERC20/extensions/ERC20Votes.sol @@ -53,7 +53,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.Checkpoint memory) { + function checkpoints(address account, uint32 pos) public view virtual returns (Checkpoints.Checkpoint224 memory) { return _checkpoints(account, pos); } diff --git a/contracts/utils/Checkpoints.sol b/contracts/utils/Checkpoints.sol index 76203edd2ce..ada1bbedcb7 100644 --- a/contracts/utils/Checkpoints.sol +++ b/contracts/utils/Checkpoints.sol @@ -17,202 +17,6 @@ import "./math/SafeCast.sol"; * _Available since v4.5._ */ library Checkpoints { - struct History { - Checkpoint[] _checkpoints; - } - - struct Checkpoint { - uint32 _blockNumber; - uint224 _value; - } - - /** - * @dev Returns the value at a given block number. If a checkpoint is not available at that block, the closest one - * before it is returned, or zero otherwise. Because the number returned corresponds to that at the end of the - * block, the requested block number must be in the past, excluding the current block. - */ - function getAtBlock(History storage self, uint256 blockNumber) internal view returns (uint256) { - require(blockNumber < block.number, "Checkpoints: block not yet mined"); - uint32 key = SafeCast.toUint32(blockNumber); - - 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 at a given block number. If a checkpoint is not available at that block, the closest one - * before it is returned, or zero otherwise. Similar to {upperLookup} but optimized for the case when the searched - * checkpoint is probably "recent", defined as being among the last sqrt(N) checkpoints where N is the number of - * checkpoints. - */ - function getAtProbablyRecentBlock(History storage self, uint256 blockNumber) internal view returns (uint256) { - require(blockNumber < block.number, "Checkpoints: block not yet mined"); - uint32 key = SafeCast.toUint32(blockNumber); - - 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)._blockNumber) { - 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 checkpoint at given position. - */ - function getAtPosition(History storage self, uint32 pos) internal view returns (Checkpoint memory) { - return self._checkpoints[pos]; - } - - /** - * @dev Pushes a value onto a History so that it is stored as the checkpoint for the current block. - * - * Returns previous value and new value. - */ - function push(History storage self, uint256 value) internal returns (uint256, uint256) { - return _insert(self._checkpoints, SafeCast.toUint32(block.number), SafeCast.toUint224(value)); - } - - /** - * @dev Pushes a value onto a History, by updating the latest value using binary operation `op`. The new value will - * be set to `op(latest, delta)`. - * - * Returns previous value and new value. - */ - function push( - History storage self, - function(uint256, uint256) view returns (uint256) op, - uint256 delta - ) internal returns (uint256, uint256) { - return push(self, op(latest(self), delta)); - } - - /** - * @dev Returns the value in the most recent checkpoint, or zero if there are no checkpoints. - */ - function latest(History storage self) internal view returns (uint224) { - 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( - History storage self - ) internal view returns (bool exists, uint32 _blockNumber, uint224 _value) { - uint256 pos = self._checkpoints.length; - if (pos == 0) { - return (false, 0, 0); - } else { - Checkpoint memory ckpt = _unsafeAccess(self._checkpoints, pos - 1); - return (true, ckpt._blockNumber, ckpt._value); - } - } - - /** - * @dev Returns the number of checkpoint. - */ - function length(History storage self) internal view returns (uint256) { - return self._checkpoints.length; - } - - /** - * @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(Checkpoint[] storage self, uint32 key, uint224 value) private returns (uint224, uint224) { - uint256 pos = self.length; - - if (pos > 0) { - // Copying to memory is important here. - Checkpoint memory last = _unsafeAccess(self, pos - 1); - - // Checkpoint keys must be non-decreasing. - require(last._blockNumber <= key, "Checkpoint: decreasing keys"); - - // Update or push new checkpoint - if (last._blockNumber == key) { - _unsafeAccess(self, pos - 1)._value = value; - } else { - self.push(Checkpoint({_blockNumber: key, _value: value})); - } - return (last._value, value); - } else { - self.push(Checkpoint({_blockNumber: key, _value: value})); - return (0, value); - } - } - - /** - * @dev Return the index of the oldest checkpoint whose key is greater 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( - Checkpoint[] storage self, - uint32 key, - uint256 low, - uint256 high - ) private view returns (uint256) { - while (low < high) { - uint256 mid = Math.average(low, high); - if (_unsafeAccess(self, mid)._blockNumber > key) { - high = mid; - } else { - low = mid + 1; - } - } - return high; - } - - /** - * @dev Return the index of the oldest checkpoint whose 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( - Checkpoint[] storage self, - uint32 key, - uint256 low, - uint256 high - ) private view returns (uint256) { - while (low < high) { - uint256 mid = Math.average(low, high); - if (_unsafeAccess(self, mid)._blockNumber < 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(Checkpoint[] storage self, uint256 pos) private pure returns (Checkpoint storage result) { - assembly { - mstore(0, self.slot) - result.slot := add(keccak256(0, 0x20), pos) - } - } - struct Trace224 { Checkpoint224[] _checkpoints; } diff --git a/package.json b/package.json index 72978ffec05..5b0eb34fe95 100644 --- a/package.json +++ b/package.json @@ -22,7 +22,7 @@ "clean": "hardhat clean && rimraf build contracts/build", "prepare": "scripts/prepare.sh", "prepack": "scripts/prepack.sh", - "generate": "scripts/generate/run.js", + "generate": "node scripts/generate/run.js", "release": "scripts/release/release.sh", "version": "scripts/release/version.sh", "test": "hardhat test", diff --git a/scripts/generate/templates/Checkpoints.js b/scripts/generate/templates/Checkpoints.js index c4673d245f9..99f7aaa92c3 100644 --- a/scripts/generate/templates/Checkpoints.js +++ b/scripts/generate/templates/Checkpoints.js @@ -295,10 +295,6 @@ module.exports = format( header.trimEnd(), 'library Checkpoints {', [ - // Legacy types & functions - types(LEGACY_OPTS), - legacyOperations(LEGACY_OPTS), - common(LEGACY_OPTS), // New flavors ...OPTS.flatMap(opts => [types(opts), operations(opts), common(opts)]), ], From 3b8610f90457d877e96a6ce31d53de88aa1b8bee Mon Sep 17 00:00:00 2001 From: RenanSouza2 Date: Sat, 8 Apr 2023 16:35:30 -0300 Subject: [PATCH 02/13] proposal 1 --- .../GovernorVotesQuorumFraction.sol | 4 +- contracts/utils/Checkpoints.sol | 32 ++++++++++ scripts/generate/templates/Checkpoints.js | 59 +++---------------- 3 files changed, 41 insertions(+), 54 deletions(-) diff --git a/contracts/governance/extensions/GovernorVotesQuorumFraction.sol b/contracts/governance/extensions/GovernorVotesQuorumFraction.sol index 80c189f30fb..d84ef60319b 100644 --- a/contracts/governance/extensions/GovernorVotesQuorumFraction.sol +++ b/contracts/governance/extensions/GovernorVotesQuorumFraction.sol @@ -108,12 +108,12 @@ abstract contract GovernorVotesQuorumFraction is GovernorVotes { // Make sure we keep track of the original numerator in contracts upgraded from a version without checkpoints. if (oldQuorumNumerator != 0 && _quorumNumeratorHistory._checkpoints.length == 0) { _quorumNumeratorHistory._checkpoints.push( - Checkpoints.Checkpoint({_blockNumber: 0, _value: SafeCast.toUint224(oldQuorumNumerator)}) + Checkpoints.Checkpoint224({_key: 0, _value: SafeCast.toUint224(oldQuorumNumerator)}) ); } // Set new quorum for future proposals - _quorumNumeratorHistory.push(newQuorumNumerator); + _quorumNumeratorHistory.push(SafeCast.toUint224(newQuorumNumerator)); emit QuorumNumeratorUpdated(oldQuorumNumerator, newQuorumNumerator); } diff --git a/contracts/utils/Checkpoints.sol b/contracts/utils/Checkpoints.sol index ada1bbedcb7..fdc27504eed 100644 --- a/contracts/utils/Checkpoints.sol +++ b/contracts/utils/Checkpoints.sol @@ -17,6 +17,38 @@ import "./math/SafeCast.sol"; * _Available since v4.5._ */ library Checkpoints { + /** + * @dev Returns checkpoint at given position. + */ + function getAtPosition(Trace224 storage self, uint32 pos) internal view returns (Checkpoint224 memory) { + return self._checkpoints[pos]; + } + + /** + * @dev Pushes a value onto a History so that it is stored as the checkpoint for the current block. + * + * Returns previous value and new value. + */ + function push(Trace224 storage self, uint256 value) internal returns (uint256, uint256) { + return _insert(self._checkpoints, SafeCast.toUint32(block.number), SafeCast.toUint224(value)); + } + + /** + * @dev Pushes a value onto a History, by updating the latest value using binary operation `op`. The new value will + * be set to `op(latest, delta)`. + * + * Returns previous value and new value. + */ + function push( + Trace224 storage self, + function(uint256, uint256) view returns (uint256) op, + uint256 delta + ) internal returns (uint256, uint256) { + uint32 key = SafeCast.toUint32(block.number); + uint224 value = SafeCast.toUint224(op(latest(self), delta)); + return push(self, key, value); + } + struct Trace224 { Checkpoint224[] _checkpoints; } diff --git a/scripts/generate/templates/Checkpoints.js b/scripts/generate/templates/Checkpoints.js index 99f7aaa92c3..79b2f354421 100644 --- a/scripts/generate/templates/Checkpoints.js +++ b/scripts/generate/templates/Checkpoints.js @@ -65,53 +65,10 @@ function upperLookup(${opts.historyTypeName} storage self, ${opts.keyTypeName} k `; const legacyOperations = opts => `\ -/** - * @dev Returns the value at a given block number. If a checkpoint is not available at that block, the closest one - * before it is returned, or zero otherwise. Because the number returned corresponds to that at the end of the - * block, the requested block number must be in the past, excluding the current block. - */ -function getAtBlock(${opts.historyTypeName} storage self, uint256 blockNumber) internal view returns (uint256) { - require(blockNumber < block.number, "Checkpoints: block not yet mined"); - uint32 key = SafeCast.toUint32(blockNumber); - - uint256 len = self.${opts.checkpointFieldName}.length; - uint256 pos = _upperBinaryLookup(self.${opts.checkpointFieldName}, key, 0, len); - return pos == 0 ? 0 : _unsafeAccess(self.${opts.checkpointFieldName}, pos - 1).${opts.valueFieldName}; -} - -/** - * @dev Returns the value at a given block number. If a checkpoint is not available at that block, the closest one - * before it is returned, or zero otherwise. Similar to {upperLookup} but optimized for the case when the searched - * checkpoint is probably "recent", defined as being among the last sqrt(N) checkpoints where N is the number of - * checkpoints. - */ -function getAtProbablyRecentBlock(${opts.historyTypeName} storage self, uint256 blockNumber) internal view returns (uint256) { - require(blockNumber < block.number, "Checkpoints: block not yet mined"); - uint32 key = SafeCast.toUint32(blockNumber); - - uint256 len = self.${opts.checkpointFieldName}.length; - - uint256 low = 0; - uint256 high = len; - - if (len > 5) { - uint256 mid = len - Math.sqrt(len); - if (key < _unsafeAccess(self.${opts.checkpointFieldName}, mid)._blockNumber) { - high = mid; - } else { - low = mid + 1; - } - } - - uint256 pos = _upperBinaryLookup(self.${opts.checkpointFieldName}, key, low, high); - - return pos == 0 ? 0 : _unsafeAccess(self.${opts.checkpointFieldName}, pos - 1).${opts.valueFieldName}; -} - /** * @dev Returns checkpoint at given position. */ -function getAtPosition(History storage self, uint32 pos) internal view returns (Checkpoint memory) { +function getAtPosition(${opts.historyTypeName} storage self, uint32 pos) internal view returns (${opts.checkpointTypeName} memory) { return self._checkpoints[pos]; } @@ -135,7 +92,9 @@ function push( function(uint256, uint256) view returns (uint256) op, uint256 delta ) internal returns (uint256, uint256) { - return push(self, op(latest(self), delta)); + uint32 key = SafeCast.toUint32(block.number); + ${opts.valueTypeName} value = SafeCast.to${opts.valueTypeNameCap}(op(latest(self), delta)); + return push(self, key, value); } `; @@ -278,23 +237,19 @@ const defaultOpts = size => ({ keyTypeName: `uint${256 - size}`, keyFieldName: '_key', valueTypeName: `uint${size}`, + valueTypeNameCap: `Uint${size}`, valueFieldName: '_value', }); const OPTS = VALUE_SIZES.map(size => defaultOpts(size)); -const LEGACY_OPTS = { - ...defaultOpts(224), - historyTypeName: 'History', - checkpointTypeName: 'Checkpoint', - keyFieldName: '_blockNumber', -}; - // GENERATE module.exports = format( header.trimEnd(), 'library Checkpoints {', [ + // Legacy function + legacyOperations(defaultOpts(224)), // New flavors ...OPTS.flatMap(opts => [types(opts), operations(opts), common(opts)]), ], From 578bc8bf72ff5007462945269a707f1e06cf7231 Mon Sep 17 00:00:00 2001 From: RenanSouza2 Date: Sat, 15 Apr 2023 10:58:30 -0300 Subject: [PATCH 03/13] Checkpoint tests some fixes --- test/utils/Checkpoints.test.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/utils/Checkpoints.test.js b/test/utils/Checkpoints.test.js index d43d469cc17..013a690b139 100644 --- a/test/utils/Checkpoints.test.js +++ b/test/utils/Checkpoints.test.js @@ -14,14 +14,14 @@ contract('Checkpoints', function () { this.mock = await $Checkpoints.new(); }); - describe('History checkpoints', function () { - const latest = (self, ...args) => self.methods['$latest_Checkpoints_History(uint256)'](0, ...args); + describe.only('History checkpoints', function () { + const latest = (self, ...args) => self.methods['$latest_Checkpoints_Trace224(uint256)'](0, ...args); const latestCheckpoint = (self, ...args) => - self.methods['$latestCheckpoint_Checkpoints_History(uint256)'](0, ...args); + self.methods['$latestCheckpoint_Checkpoints_Trace224(uint256)'](0, ...args); const push = (self, ...args) => self.methods['$push(uint256,uint256)'](0, ...args); const getAtBlock = (self, ...args) => self.methods['$getAtBlock(uint256,uint256)'](0, ...args); const getAtRecentBlock = (self, ...args) => self.methods['$getAtProbablyRecentBlock(uint256,uint256)'](0, ...args); - const getLength = (self, ...args) => self.methods['$length_Checkpoints_History(uint256)'](0, ...args); + const getLength = (self, ...args) => self.methods['$length_Checkpoints_Trace224(uint256)'](0, ...args); describe('without checkpoints', function () { it('returns zero as latest value', async function () { From d53e29d1e519db342f9585113ea12c6b3bfd9dfa Mon Sep 17 00:00:00 2001 From: RenanSouza2 Date: Sat, 15 Apr 2023 15:10:04 -0300 Subject: [PATCH 04/13] returns zero as past value --- test/utils/Checkpoints.test.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/utils/Checkpoints.test.js b/test/utils/Checkpoints.test.js index 013a690b139..634dd8f9a0e 100644 --- a/test/utils/Checkpoints.test.js +++ b/test/utils/Checkpoints.test.js @@ -19,7 +19,7 @@ contract('Checkpoints', function () { const latestCheckpoint = (self, ...args) => self.methods['$latestCheckpoint_Checkpoints_Trace224(uint256)'](0, ...args); const push = (self, ...args) => self.methods['$push(uint256,uint256)'](0, ...args); - const getAtBlock = (self, ...args) => self.methods['$getAtBlock(uint256,uint256)'](0, ...args); + const getAtBlock = (self, ...args) => self.methods['$upperLookup(uint256,uint32)'](0, ...args); const getAtRecentBlock = (self, ...args) => self.methods['$getAtProbablyRecentBlock(uint256,uint256)'](0, ...args); const getLength = (self, ...args) => self.methods['$length_Checkpoints_Trace224(uint256)'](0, ...args); @@ -36,7 +36,6 @@ contract('Checkpoints', function () { it('returns zero as past value', async function () { await time.advanceBlock(); expect(await getAtBlock(this.mock, (await web3.eth.getBlockNumber()) - 1)).to.be.bignumber.equal('0'); - expect(await getAtRecentBlock(this.mock, (await web3.eth.getBlockNumber()) - 1)).to.be.bignumber.equal('0'); }); }); From d89cf696e9e29b847a2625f8d7248878ef8daa40 Mon Sep 17 00:00:00 2001 From: RenanSouza2 Date: Sat, 15 Apr 2023 15:17:07 -0300 Subject: [PATCH 05/13] more tests --- test/utils/Checkpoints.test.js | 46 ++++++++-------------------------- 1 file changed, 10 insertions(+), 36 deletions(-) diff --git a/test/utils/Checkpoints.test.js b/test/utils/Checkpoints.test.js index 634dd8f9a0e..97bf079bd3c 100644 --- a/test/utils/Checkpoints.test.js +++ b/test/utils/Checkpoints.test.js @@ -20,7 +20,6 @@ contract('Checkpoints', function () { self.methods['$latestCheckpoint_Checkpoints_Trace224(uint256)'](0, ...args); const push = (self, ...args) => self.methods['$push(uint256,uint256)'](0, ...args); const getAtBlock = (self, ...args) => self.methods['$upperLookup(uint256,uint32)'](0, ...args); - const getAtRecentBlock = (self, ...args) => self.methods['$getAtProbablyRecentBlock(uint256,uint256)'](0, ...args); const getLength = (self, ...args) => self.methods['$length_Checkpoints_Trace224(uint256)'](0, ...args); describe('without checkpoints', function () { @@ -58,30 +57,17 @@ contract('Checkpoints', function () { expect(ckpt[2]).to.be.bignumber.equal(web3.utils.toBN('3')); }); - for (const getAtBlockVariant of [getAtBlock, getAtRecentBlock]) { - describe(`lookup: ${getAtBlockVariant}`, function () { - it('returns past values', async function () { - expect(await getAtBlockVariant(this.mock, this.tx1.receipt.blockNumber - 1)).to.be.bignumber.equal('0'); - expect(await getAtBlockVariant(this.mock, this.tx1.receipt.blockNumber)).to.be.bignumber.equal('1'); - expect(await getAtBlockVariant(this.mock, this.tx2.receipt.blockNumber)).to.be.bignumber.equal('2'); - // Block with no new checkpoints - expect(await getAtBlockVariant(this.mock, this.tx2.receipt.blockNumber + 1)).to.be.bignumber.equal('2'); - expect(await getAtBlockVariant(this.mock, this.tx3.receipt.blockNumber)).to.be.bignumber.equal('3'); - expect(await getAtBlockVariant(this.mock, this.tx3.receipt.blockNumber + 1)).to.be.bignumber.equal('3'); - }); - it('reverts if block number >= current block', async function () { - await expectRevert( - getAtBlockVariant(this.mock, await web3.eth.getBlockNumber()), - 'Checkpoints: block not yet mined', - ); - - await expectRevert( - getAtBlockVariant(this.mock, (await web3.eth.getBlockNumber()) + 1), - 'Checkpoints: block not yet mined', - ); - }); + describe(`lookup: getAtBlock`, function () { + it('returns past values', async function () { + expect(await getAtBlock(this.mock, this.tx1.receipt.blockNumber - 1)).to.be.bignumber.equal('0'); + expect(await getAtBlock(this.mock, this.tx1.receipt.blockNumber)).to.be.bignumber.equal('1'); + expect(await getAtBlock(this.mock, this.tx2.receipt.blockNumber)).to.be.bignumber.equal('2'); + // Block with no new checkpoints + expect(await getAtBlock(this.mock, this.tx2.receipt.blockNumber + 1)).to.be.bignumber.equal('2'); + expect(await getAtBlock(this.mock, this.tx3.receipt.blockNumber)).to.be.bignumber.equal('3'); + expect(await getAtBlock(this.mock, this.tx3.receipt.blockNumber + 1)).to.be.bignumber.equal('3'); }); - } + }); it('multiple checkpoints in the same block', async function () { const lengthBefore = await getLength(this.mock); @@ -95,18 +81,6 @@ contract('Checkpoints', function () { expect(await getLength(this.mock)).to.be.bignumber.equal(lengthBefore.addn(1)); expect(await latest(this.mock)).to.be.bignumber.equal('10'); }); - - it('more than 5 checkpoints', async function () { - for (let i = 4; i <= 6; i++) { - await push(this.mock, i); - } - expect(await getLength(this.mock)).to.be.bignumber.equal('6'); - const block = await web3.eth.getBlockNumber(); - // recent - expect(await getAtRecentBlock(this.mock, block - 1)).to.be.bignumber.equal('5'); - // non-recent - expect(await getAtRecentBlock(this.mock, block - 9)).to.be.bignumber.equal('0'); - }); }); }); From 83f4f643427bc42908c6dad273972a5b38c8a82b Mon Sep 17 00:00:00 2001 From: RenanSouza2 Date: Sat, 15 Apr 2023 15:35:08 -0300 Subject: [PATCH 06/13] Votes --- contracts/governance/utils/Votes.sol | 2 ++ test/governance/utils/Votes.behavior.js | 2 +- test/governance/utils/Votes.test.js | 4 ++-- test/utils/Checkpoints.test.js | 2 +- 4 files changed, 6 insertions(+), 4 deletions(-) diff --git a/contracts/governance/utils/Votes.sol b/contracts/governance/utils/Votes.sol index dd81762c402..e6ca8c19dce 100644 --- a/contracts/governance/utils/Votes.sol +++ b/contracts/governance/utils/Votes.sol @@ -54,6 +54,7 @@ abstract contract Votes is IVotes, Context, EIP712, Nonces { * - `blockNumber` must have been already mined */ function getPastVotes(address account, uint256 blockNumber) public view virtual override returns (uint256) { + require(blockNumber < block.number, 'Votes: block not yet mined'); uint32 key = SafeCast.toUint32(blockNumber); return _delegateCheckpoints[account].upperLookup(key); } @@ -70,6 +71,7 @@ abstract contract Votes is IVotes, Context, EIP712, Nonces { * - `blockNumber` must have been already mined */ function getPastTotalSupply(uint256 blockNumber) public view virtual override returns (uint256) { + require(blockNumber < block.number, 'Votes: block not yet mined'); uint32 key = SafeCast.toUint32(blockNumber); return _totalCheckpoints.upperLookup(key); } diff --git a/test/governance/utils/Votes.behavior.js b/test/governance/utils/Votes.behavior.js index 7cc8f133eb9..70de6e540d2 100644 --- a/test/governance/utils/Votes.behavior.js +++ b/test/governance/utils/Votes.behavior.js @@ -281,7 +281,7 @@ function shouldBehaveLikeVotes(accounts, tokens, fungible = true) { expect(await this.votes.getPastTotalSupply(blockNumber + 9)).to.be.bignumber.equal(weight[2]); expect(await this.votes.getPastTotalSupply(blockNumber + 10)).to.be.bignumber.equal(weight[2]); expect(await this.votes.getPastTotalSupply(blockNumber + 11)).to.be.bignumber.equal('0'); - await expectRevert(this.votes.getPastTotalSupply(blockNumber + 12), 'Checkpoints: block not yet mined'); + await expectRevert(this.votes.getPastTotalSupply(blockNumber + 12), 'Votes: block not yet mined'); }); }); diff --git a/test/governance/utils/Votes.test.js b/test/governance/utils/Votes.test.js index c0c574df4e0..06e3eb2b24c 100644 --- a/test/governance/utils/Votes.test.js +++ b/test/governance/utils/Votes.test.js @@ -37,7 +37,7 @@ contract('Votes', function (accounts) { it('reverts if block number >= current block', async function () { await expectRevert( this.votes.getPastTotalSupply(this.txs.at(-1).receipt.blockNumber + 1), - 'Checkpoints: block not yet mined', + 'Votes: block not yet mined', ); }); @@ -76,7 +76,7 @@ contract('Votes', function (accounts) { }); }); - describe('performs voting workflow', function () { + describe.only('performs voting workflow', function () { beforeEach(async function () { this.chainId = await getChainId(); }); diff --git a/test/utils/Checkpoints.test.js b/test/utils/Checkpoints.test.js index 97bf079bd3c..9d35e85f5b4 100644 --- a/test/utils/Checkpoints.test.js +++ b/test/utils/Checkpoints.test.js @@ -14,7 +14,7 @@ contract('Checkpoints', function () { this.mock = await $Checkpoints.new(); }); - describe.only('History checkpoints', function () { + describe('History checkpoints', function () { const latest = (self, ...args) => self.methods['$latest_Checkpoints_Trace224(uint256)'](0, ...args); const latestCheckpoint = (self, ...args) => self.methods['$latestCheckpoint_Checkpoints_Trace224(uint256)'](0, ...args); From bf2282842b63fb8196c49e5569fb10c946d294dd Mon Sep 17 00:00:00 2001 From: RenanSouza2 Date: Sat, 15 Apr 2023 16:01:24 -0300 Subject: [PATCH 07/13] ERC20Votes --- test/governance/utils/Votes.test.js | 2 +- test/token/ERC20/extensions/ERC20Votes.test.js | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/test/governance/utils/Votes.test.js b/test/governance/utils/Votes.test.js index 06e3eb2b24c..e27a8ccf1c0 100644 --- a/test/governance/utils/Votes.test.js +++ b/test/governance/utils/Votes.test.js @@ -76,7 +76,7 @@ contract('Votes', function (accounts) { }); }); - describe.only('performs voting workflow', function () { + describe('performs voting workflow', function () { beforeEach(async function () { this.chainId = await getChainId(); }); diff --git a/test/token/ERC20/extensions/ERC20Votes.test.js b/test/token/ERC20/extensions/ERC20Votes.test.js index 8d010ec6bc9..d3e237283ab 100644 --- a/test/token/ERC20/extensions/ERC20Votes.test.js +++ b/test/token/ERC20/extensions/ERC20Votes.test.js @@ -422,7 +422,7 @@ contract('ERC20Votes', function (accounts) { describe('getPastVotes', function () { it('reverts if block number >= current block', async function () { - await expectRevert(this.token.getPastVotes(other1, 5e10), 'Checkpoints: block not yet mined'); + await expectRevert(this.token.getPastVotes(other1, 5e10), 'Votes: block not yet mined'); }); it('returns 0 if there are no checkpoints', async function () { @@ -503,7 +503,7 @@ contract('ERC20Votes', function (accounts) { }); it('reverts if block number >= current block', async function () { - await expectRevert(this.token.getPastTotalSupply(5e10), 'Checkpoints: block not yet mined'); + await expectRevert(this.token.getPastTotalSupply(5e10), 'Votes: block not yet mined'); }); it('returns 0 if there are no checkpoints', async function () { From 9b13eb313b5a8d6b74e995b1078cc3c8b334aee4 Mon Sep 17 00:00:00 2001 From: RenanSouza2 Date: Sat, 15 Apr 2023 16:12:33 -0300 Subject: [PATCH 08/13] ERC20VotesComp --- test/token/ERC20/extensions/ERC20VotesComp.test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/token/ERC20/extensions/ERC20VotesComp.test.js b/test/token/ERC20/extensions/ERC20VotesComp.test.js index ef4a22be225..c7ef1cad19a 100644 --- a/test/token/ERC20/extensions/ERC20VotesComp.test.js +++ b/test/token/ERC20/extensions/ERC20VotesComp.test.js @@ -389,7 +389,7 @@ contract('ERC20VotesComp', function (accounts) { describe('getPriorVotes', function () { it('reverts if block number >= current block', async function () { - await expectRevert(this.token.getPriorVotes(other1, 5e10), 'Checkpoints: block not yet mined'); + await expectRevert(this.token.getPriorVotes(other1, 5e10), 'Votes: block not yet mined'); }); it('returns 0 if there are no checkpoints', async function () { @@ -470,7 +470,7 @@ contract('ERC20VotesComp', function (accounts) { }); it('reverts if block number >= current block', async function () { - await expectRevert(this.token.getPastTotalSupply(5e10), 'Checkpoints: block not yet mined'); + await expectRevert(this.token.getPastTotalSupply(5e10), 'Votes: block not yet mined'); }); it('returns 0 if there are no checkpoints', async function () { From a91837cb63e2cabff2c002587e550d0b21ea67aa Mon Sep 17 00:00:00 2001 From: RenanSouza2 Date: Sat, 15 Apr 2023 16:47:41 -0300 Subject: [PATCH 09/13] Tested --- contracts/governance/extensions/GovernorVotesQuorumFraction.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/governance/extensions/GovernorVotesQuorumFraction.sol b/contracts/governance/extensions/GovernorVotesQuorumFraction.sol index d84ef60319b..f41b73dd710 100644 --- a/contracts/governance/extensions/GovernorVotesQuorumFraction.sol +++ b/contracts/governance/extensions/GovernorVotesQuorumFraction.sol @@ -57,7 +57,7 @@ abstract contract GovernorVotesQuorumFraction is GovernorVotes { // Otherwise, do the binary search uint32 key = SafeCast.toUint32(blockNumber); - return _quorumNumeratorHistory.lowerLookup(key); + return _quorumNumeratorHistory.upperLookup(key); } /** From 73ce43726ac08465af5a08a515d1406e0d043603 Mon Sep 17 00:00:00 2001 From: RenanSouza2 Date: Sun, 16 Apr 2023 09:37:02 -0300 Subject: [PATCH 10/13] compiled --- .../GovernorVotesQuorumFraction.sol | 9 +++--- contracts/governance/utils/Votes.sol | 31 +++++++++++------- .../token/ERC20/extensions/ERC20Votes.sol | 7 ---- contracts/utils/Checkpoints.sol | 32 ------------------- scripts/generate/templates/Checkpoints.js | 2 -- 5 files changed, 25 insertions(+), 56 deletions(-) diff --git a/contracts/governance/extensions/GovernorVotesQuorumFraction.sol b/contracts/governance/extensions/GovernorVotesQuorumFraction.sol index f41b73dd710..6c57f2d395e 100644 --- a/contracts/governance/extensions/GovernorVotesQuorumFraction.sol +++ b/contracts/governance/extensions/GovernorVotesQuorumFraction.sol @@ -107,13 +107,14 @@ abstract contract GovernorVotesQuorumFraction is GovernorVotes { // Make sure we keep track of the original numerator in contracts upgraded from a version without checkpoints. if (oldQuorumNumerator != 0 && _quorumNumeratorHistory._checkpoints.length == 0) { - _quorumNumeratorHistory._checkpoints.push( - Checkpoints.Checkpoint224({_key: 0, _value: SafeCast.toUint224(oldQuorumNumerator)}) - ); + _quorumNumeratorHistory.push(0, SafeCast.toUint224(oldQuorumNumerator)); } // Set new quorum for future proposals - _quorumNumeratorHistory.push(SafeCast.toUint224(newQuorumNumerator)); + _quorumNumeratorHistory.push( + SafeCast.toUint32(block.number), + SafeCast.toUint224(newQuorumNumerator) + ); emit QuorumNumeratorUpdated(oldQuorumNumerator, newQuorumNumerator); } diff --git a/contracts/governance/utils/Votes.sol b/contracts/governance/utils/Votes.sol index e6ca8c19dce..8ecba74495a 100644 --- a/contracts/governance/utils/Votes.sol +++ b/contracts/governance/utils/Votes.sol @@ -139,10 +139,18 @@ abstract contract Votes is IVotes, Context, EIP712, Nonces { */ function _transferVotingUnits(address from, address to, uint256 amount) internal virtual { if (from == address(0)) { - _totalCheckpoints.push(_add, amount); + uint224 latest = _totalCheckpoints.latest(); + _totalCheckpoints.push( + SafeCast.toUint32(block.timestamp), + latest + SafeCast.toUint32(amount) + ); } if (to == address(0)) { - _totalCheckpoints.push(_subtract, amount); + uint224 latest = _totalCheckpoints.latest(); + _totalCheckpoints.push( + SafeCast.toUint32(block.timestamp), + latest - SafeCast.toUint32(amount) + ); } _moveDelegateVotes(delegates(from), delegates(to), amount); } @@ -153,11 +161,19 @@ abstract contract Votes is IVotes, Context, EIP712, Nonces { function _moveDelegateVotes(address from, address to, uint256 amount) private { if (from != to && amount > 0) { if (from != address(0)) { - (uint256 oldValue, uint256 newValue) = _delegateCheckpoints[from].push(_subtract, amount); + uint224 latest = _totalCheckpoints.latest(); + (uint256 oldValue, uint256 newValue) = _delegateCheckpoints[from].push( + SafeCast.toUint32(block.timestamp), + latest - SafeCast.toUint32(amount) + ); emit DelegateVotesChanged(from, oldValue, newValue); } if (to != address(0)) { - (uint256 oldValue, uint256 newValue) = _delegateCheckpoints[to].push(_add, amount); + uint224 latest = _totalCheckpoints.latest(); + (uint256 oldValue, uint256 newValue) = _delegateCheckpoints[to].push( + SafeCast.toUint32(block.timestamp), + latest + SafeCast.toUint32(amount) + ); emit DelegateVotesChanged(to, oldValue, newValue); } } @@ -170,13 +186,6 @@ abstract contract Votes is IVotes, Context, EIP712, Nonces { return SafeCast.toUint32(_delegateCheckpoints[account].length()); } - /** - * @dev Get the `pos`-th checkpoint for `account`. - */ - function _checkpoints(address account, uint32 pos) internal view virtual returns (Checkpoints.Checkpoint224 memory) { - return _delegateCheckpoints[account].getAtPosition(pos); - } - function _add(uint256 a, uint256 b) private pure returns (uint256) { return a + b; } diff --git a/contracts/token/ERC20/extensions/ERC20Votes.sol b/contracts/token/ERC20/extensions/ERC20Votes.sol index fe25b9bb506..a2b52a16359 100644 --- a/contracts/token/ERC20/extensions/ERC20Votes.sol +++ b/contracts/token/ERC20/extensions/ERC20Votes.sol @@ -50,13 +50,6 @@ abstract contract ERC20Votes is ERC20, Votes { return _numCheckpoints(account); } - /** - * @dev Get the `pos`-th checkpoint for `account`. - */ - function checkpoints(address account, uint32 pos) public view virtual returns (Checkpoints.Checkpoint224 memory) { - return _checkpoints(account, pos); - } - /** * @dev Returns the balance of `account`. */ diff --git a/contracts/utils/Checkpoints.sol b/contracts/utils/Checkpoints.sol index fdc27504eed..ada1bbedcb7 100644 --- a/contracts/utils/Checkpoints.sol +++ b/contracts/utils/Checkpoints.sol @@ -17,38 +17,6 @@ import "./math/SafeCast.sol"; * _Available since v4.5._ */ library Checkpoints { - /** - * @dev Returns checkpoint at given position. - */ - function getAtPosition(Trace224 storage self, uint32 pos) internal view returns (Checkpoint224 memory) { - return self._checkpoints[pos]; - } - - /** - * @dev Pushes a value onto a History so that it is stored as the checkpoint for the current block. - * - * Returns previous value and new value. - */ - function push(Trace224 storage self, uint256 value) internal returns (uint256, uint256) { - return _insert(self._checkpoints, SafeCast.toUint32(block.number), SafeCast.toUint224(value)); - } - - /** - * @dev Pushes a value onto a History, by updating the latest value using binary operation `op`. The new value will - * be set to `op(latest, delta)`. - * - * Returns previous value and new value. - */ - function push( - Trace224 storage self, - function(uint256, uint256) view returns (uint256) op, - uint256 delta - ) internal returns (uint256, uint256) { - uint32 key = SafeCast.toUint32(block.number); - uint224 value = SafeCast.toUint224(op(latest(self), delta)); - return push(self, key, value); - } - struct Trace224 { Checkpoint224[] _checkpoints; } diff --git a/scripts/generate/templates/Checkpoints.js b/scripts/generate/templates/Checkpoints.js index 79b2f354421..bbee1f2c00d 100644 --- a/scripts/generate/templates/Checkpoints.js +++ b/scripts/generate/templates/Checkpoints.js @@ -248,8 +248,6 @@ module.exports = format( header.trimEnd(), 'library Checkpoints {', [ - // Legacy function - legacyOperations(defaultOpts(224)), // New flavors ...OPTS.flatMap(opts => [types(opts), operations(opts), common(opts)]), ], From 8b9029b31d183a0c72d991db00024f97059c858b Mon Sep 17 00:00:00 2001 From: RenanSouza2 Date: Sun, 16 Apr 2023 09:42:57 -0300 Subject: [PATCH 11/13] lint --- .../GovernorVotesQuorumFraction.sol | 7 +--- contracts/governance/utils/Votes.sol | 18 +++------ scripts/generate/templates/Checkpoints.js | 38 +------------------ 3 files changed, 9 insertions(+), 54 deletions(-) diff --git a/contracts/governance/extensions/GovernorVotesQuorumFraction.sol b/contracts/governance/extensions/GovernorVotesQuorumFraction.sol index 6c57f2d395e..0c4aea4e083 100644 --- a/contracts/governance/extensions/GovernorVotesQuorumFraction.sol +++ b/contracts/governance/extensions/GovernorVotesQuorumFraction.sol @@ -56,7 +56,7 @@ abstract contract GovernorVotesQuorumFraction is GovernorVotes { } // Otherwise, do the binary search - uint32 key = SafeCast.toUint32(blockNumber); + uint32 key = SafeCast.toUint32(blockNumber); return _quorumNumeratorHistory.upperLookup(key); } @@ -111,10 +111,7 @@ abstract contract GovernorVotesQuorumFraction is GovernorVotes { } // Set new quorum for future proposals - _quorumNumeratorHistory.push( - SafeCast.toUint32(block.number), - SafeCast.toUint224(newQuorumNumerator) - ); + _quorumNumeratorHistory.push(SafeCast.toUint32(block.number), SafeCast.toUint224(newQuorumNumerator)); emit QuorumNumeratorUpdated(oldQuorumNumerator, newQuorumNumerator); } diff --git a/contracts/governance/utils/Votes.sol b/contracts/governance/utils/Votes.sol index 8ecba74495a..c4b3bd4059f 100644 --- a/contracts/governance/utils/Votes.sol +++ b/contracts/governance/utils/Votes.sol @@ -54,7 +54,7 @@ abstract contract Votes is IVotes, Context, EIP712, Nonces { * - `blockNumber` must have been already mined */ function getPastVotes(address account, uint256 blockNumber) public view virtual override returns (uint256) { - require(blockNumber < block.number, 'Votes: block not yet mined'); + require(blockNumber < block.number, "Votes: block not yet mined"); uint32 key = SafeCast.toUint32(blockNumber); return _delegateCheckpoints[account].upperLookup(key); } @@ -71,7 +71,7 @@ abstract contract Votes is IVotes, Context, EIP712, Nonces { * - `blockNumber` must have been already mined */ function getPastTotalSupply(uint256 blockNumber) public view virtual override returns (uint256) { - require(blockNumber < block.number, 'Votes: block not yet mined'); + require(blockNumber < block.number, "Votes: block not yet mined"); uint32 key = SafeCast.toUint32(blockNumber); return _totalCheckpoints.upperLookup(key); } @@ -140,17 +140,11 @@ abstract contract Votes is IVotes, Context, EIP712, Nonces { function _transferVotingUnits(address from, address to, uint256 amount) internal virtual { if (from == address(0)) { uint224 latest = _totalCheckpoints.latest(); - _totalCheckpoints.push( - SafeCast.toUint32(block.timestamp), - latest + SafeCast.toUint32(amount) - ); + _totalCheckpoints.push(SafeCast.toUint32(block.timestamp), latest + SafeCast.toUint32(amount)); } if (to == address(0)) { uint224 latest = _totalCheckpoints.latest(); - _totalCheckpoints.push( - SafeCast.toUint32(block.timestamp), - latest - SafeCast.toUint32(amount) - ); + _totalCheckpoints.push(SafeCast.toUint32(block.timestamp), latest - SafeCast.toUint32(amount)); } _moveDelegateVotes(delegates(from), delegates(to), amount); } @@ -163,7 +157,7 @@ abstract contract Votes is IVotes, Context, EIP712, Nonces { if (from != address(0)) { uint224 latest = _totalCheckpoints.latest(); (uint256 oldValue, uint256 newValue) = _delegateCheckpoints[from].push( - SafeCast.toUint32(block.timestamp), + SafeCast.toUint32(block.timestamp), latest - SafeCast.toUint32(amount) ); emit DelegateVotesChanged(from, oldValue, newValue); @@ -171,7 +165,7 @@ abstract contract Votes is IVotes, Context, EIP712, Nonces { if (to != address(0)) { uint224 latest = _totalCheckpoints.latest(); (uint256 oldValue, uint256 newValue) = _delegateCheckpoints[to].push( - SafeCast.toUint32(block.timestamp), + SafeCast.toUint32(block.timestamp), latest + SafeCast.toUint32(amount) ); emit DelegateVotesChanged(to, oldValue, newValue); diff --git a/scripts/generate/templates/Checkpoints.js b/scripts/generate/templates/Checkpoints.js index bbee1f2c00d..bb65b653a5f 100644 --- a/scripts/generate/templates/Checkpoints.js +++ b/scripts/generate/templates/Checkpoints.js @@ -62,43 +62,7 @@ function upperLookup(${opts.historyTypeName} storage self, ${opts.keyTypeName} k uint256 pos = _upperBinaryLookup(self.${opts.checkpointFieldName}, key, 0, len); return pos == 0 ? 0 : _unsafeAccess(self.${opts.checkpointFieldName}, pos - 1).${opts.valueFieldName}; } -`; - -const legacyOperations = opts => `\ -/** - * @dev Returns checkpoint at given position. - */ -function getAtPosition(${opts.historyTypeName} storage self, uint32 pos) internal view returns (${opts.checkpointTypeName} memory) { - return self._checkpoints[pos]; -} - -/** - * @dev Pushes a value onto a History so that it is stored as the checkpoint for the current block. - * - * Returns previous value and new value. - */ -function push(${opts.historyTypeName} storage self, uint256 value) internal returns (uint256, uint256) { - return _insert(self.${opts.checkpointFieldName}, SafeCast.toUint32(block.number), SafeCast.toUint224(value)); -} - -/** - * @dev Pushes a value onto a History, by updating the latest value using binary operation \`op\`. The new value will - * be set to \`op(latest, delta)\`. - * - * Returns previous value and new value. - */ -function push( - ${opts.historyTypeName} storage self, - function(uint256, uint256) view returns (uint256) op, - uint256 delta -) internal returns (uint256, uint256) { - uint32 key = SafeCast.toUint32(block.number); - ${opts.valueTypeName} value = SafeCast.to${opts.valueTypeNameCap}(op(latest(self), delta)); - return push(self, key, value); -} -`; -const common = opts => `\ /** * @dev Returns the value in the most recent checkpoint, or zero if there are no checkpoints. */ @@ -249,7 +213,7 @@ module.exports = format( 'library Checkpoints {', [ // New flavors - ...OPTS.flatMap(opts => [types(opts), operations(opts), common(opts)]), + ...OPTS.flatMap(opts => [types(opts), operations(opts)]), ], '}', ); From 16ae23b7fd5013b47f15f717735c89138752b580 Mon Sep 17 00:00:00 2001 From: RenanSouza2 Date: Mon, 17 Apr 2023 01:48:57 -0300 Subject: [PATCH 12/13] some fixes --- contracts/governance/utils/Votes.sol | 23 ++++-- .../token/ERC20/extensions/ERC20Votes.sol | 7 ++ .../token/ERC20/extensions/ERC20Votes.test.js | 12 +-- test/utils/Checkpoints.test.js | 74 +------------------ 4 files changed, 29 insertions(+), 87 deletions(-) diff --git a/contracts/governance/utils/Votes.sol b/contracts/governance/utils/Votes.sol index c4b3bd4059f..a6657b361eb 100644 --- a/contracts/governance/utils/Votes.sol +++ b/contracts/governance/utils/Votes.sol @@ -140,11 +140,11 @@ abstract contract Votes is IVotes, Context, EIP712, Nonces { function _transferVotingUnits(address from, address to, uint256 amount) internal virtual { if (from == address(0)) { uint224 latest = _totalCheckpoints.latest(); - _totalCheckpoints.push(SafeCast.toUint32(block.timestamp), latest + SafeCast.toUint32(amount)); + _totalCheckpoints.push(SafeCast.toUint32(block.number), latest + SafeCast.toUint224(amount)); } if (to == address(0)) { uint224 latest = _totalCheckpoints.latest(); - _totalCheckpoints.push(SafeCast.toUint32(block.timestamp), latest - SafeCast.toUint32(amount)); + _totalCheckpoints.push(SafeCast.toUint32(block.number), latest - SafeCast.toUint224(amount)); } _moveDelegateVotes(delegates(from), delegates(to), amount); } @@ -155,18 +155,18 @@ abstract contract Votes is IVotes, Context, EIP712, Nonces { function _moveDelegateVotes(address from, address to, uint256 amount) private { if (from != to && amount > 0) { if (from != address(0)) { - uint224 latest = _totalCheckpoints.latest(); + uint224 latest = _delegateCheckpoints[from].latest(); (uint256 oldValue, uint256 newValue) = _delegateCheckpoints[from].push( - SafeCast.toUint32(block.timestamp), - latest - SafeCast.toUint32(amount) + SafeCast.toUint32(block.number), + latest - SafeCast.toUint224(amount) ); emit DelegateVotesChanged(from, oldValue, newValue); } if (to != address(0)) { - uint224 latest = _totalCheckpoints.latest(); + uint224 latest = _delegateCheckpoints[to].latest(); (uint256 oldValue, uint256 newValue) = _delegateCheckpoints[to].push( - SafeCast.toUint32(block.timestamp), - latest + SafeCast.toUint32(amount) + SafeCast.toUint32(block.number), + latest + SafeCast.toUint224(amount) ); emit DelegateVotesChanged(to, oldValue, newValue); } @@ -180,6 +180,13 @@ abstract contract Votes is IVotes, Context, EIP712, Nonces { return SafeCast.toUint32(_delegateCheckpoints[account].length()); } + /** + * @dev Get the `pos`-th checkpoint for `account`. + */ + function _checkpoints(address account, uint32 pos) internal view virtual returns (Checkpoints.Checkpoint224 memory) { + return _delegateCheckpoints[account]._checkpoints[pos]; + } + function _add(uint256 a, uint256 b) private pure returns (uint256) { return a + b; } diff --git a/contracts/token/ERC20/extensions/ERC20Votes.sol b/contracts/token/ERC20/extensions/ERC20Votes.sol index a2b52a16359..fe25b9bb506 100644 --- a/contracts/token/ERC20/extensions/ERC20Votes.sol +++ b/contracts/token/ERC20/extensions/ERC20Votes.sol @@ -50,6 +50,13 @@ abstract contract ERC20Votes is ERC20, Votes { return _numCheckpoints(account); } + /** + * @dev Get the `pos`-th checkpoint for `account`. + */ + function checkpoints(address account, uint32 pos) public view virtual returns (Checkpoints.Checkpoint224 memory) { + return _checkpoints(account, pos); + } + /** * @dev Returns the balance of `account`. */ diff --git a/test/token/ERC20/extensions/ERC20Votes.test.js b/test/token/ERC20/extensions/ERC20Votes.test.js index d3e237283ab..dd0619109e9 100644 --- a/test/token/ERC20/extensions/ERC20Votes.test.js +++ b/test/token/ERC20/extensions/ERC20Votes.test.js @@ -61,7 +61,7 @@ contract('ERC20Votes', function (accounts) { expect(await this.token.getPastVotes(holder, block - 6)).to.be.bignumber.equal('0'); }); - describe('set delegation', function () { + describe.only('set delegation', function () { describe('call', function () { it('delegation with balance', async function () { await this.token.$_mint(holder, supply); @@ -231,7 +231,7 @@ contract('ERC20Votes', function (accounts) { }); }); - describe('change delegation', function () { + describe.only('change delegation', function () { beforeEach(async function () { await this.token.$_mint(holder, supply); await this.token.delegate(holder, { from: holder }); @@ -269,7 +269,7 @@ contract('ERC20Votes', function (accounts) { }); }); - describe('transfers', function () { + describe.only('transfers', function () { beforeEach(async function () { await this.token.$_mint(holder, supply); }); @@ -360,7 +360,7 @@ contract('ERC20Votes', function (accounts) { }); // The following tests are a adaptation of https://github.com/compound-finance/compound-protocol/blob/master/tests/Governance/CompTest.js. - describe('Compound test suite', function () { + describe.only('Compound test suite', function () { beforeEach(async function () { await this.token.$_mint(holder, supply); }); @@ -497,7 +497,7 @@ contract('ERC20Votes', function (accounts) { }); }); - describe('getPastTotalSupply', function () { + describe.only('getPastTotalSupply', function () { beforeEach(async function () { await this.token.delegate(holder, { from: holder }); }); @@ -574,7 +574,7 @@ contract('ERC20Votes', function (accounts) { }); }); - describe('Voting workflow', function () { + describe.only('Voting workflow', function () { beforeEach(async function () { this.name = name; this.votes = this.token; diff --git a/test/utils/Checkpoints.test.js b/test/utils/Checkpoints.test.js index 9d35e85f5b4..47b5153410d 100644 --- a/test/utils/Checkpoints.test.js +++ b/test/utils/Checkpoints.test.js @@ -1,9 +1,7 @@ -const { expectRevert, time } = require('@openzeppelin/test-helpers'); +const { expectRevert } = require('@openzeppelin/test-helpers'); const { expect } = require('chai'); -const { batchInBlock } = require('../helpers/txpool'); - const $Checkpoints = artifacts.require('$Checkpoints'); const first = array => (array.length ? array[0] : undefined); @@ -14,76 +12,6 @@ contract('Checkpoints', function () { this.mock = await $Checkpoints.new(); }); - describe('History checkpoints', function () { - const latest = (self, ...args) => self.methods['$latest_Checkpoints_Trace224(uint256)'](0, ...args); - const latestCheckpoint = (self, ...args) => - self.methods['$latestCheckpoint_Checkpoints_Trace224(uint256)'](0, ...args); - const push = (self, ...args) => self.methods['$push(uint256,uint256)'](0, ...args); - const getAtBlock = (self, ...args) => self.methods['$upperLookup(uint256,uint32)'](0, ...args); - const getLength = (self, ...args) => self.methods['$length_Checkpoints_Trace224(uint256)'](0, ...args); - - describe('without checkpoints', function () { - it('returns zero as latest value', async function () { - expect(await latest(this.mock)).to.be.bignumber.equal('0'); - - const ckpt = await latestCheckpoint(this.mock); - expect(ckpt[0]).to.be.equal(false); - expect(ckpt[1]).to.be.bignumber.equal('0'); - expect(ckpt[2]).to.be.bignumber.equal('0'); - }); - - it('returns zero as past value', async function () { - await time.advanceBlock(); - expect(await getAtBlock(this.mock, (await web3.eth.getBlockNumber()) - 1)).to.be.bignumber.equal('0'); - }); - }); - - describe('with checkpoints', function () { - beforeEach('pushing checkpoints', async function () { - this.tx1 = await push(this.mock, 1); - this.tx2 = await push(this.mock, 2); - await time.advanceBlock(); - this.tx3 = await push(this.mock, 3); - await time.advanceBlock(); - await time.advanceBlock(); - }); - - it('returns latest value', async function () { - expect(await latest(this.mock)).to.be.bignumber.equal('3'); - - const ckpt = await latestCheckpoint(this.mock); - expect(ckpt[0]).to.be.equal(true); - expect(ckpt[1]).to.be.bignumber.equal(web3.utils.toBN(this.tx3.receipt.blockNumber)); - expect(ckpt[2]).to.be.bignumber.equal(web3.utils.toBN('3')); - }); - - describe(`lookup: getAtBlock`, function () { - it('returns past values', async function () { - expect(await getAtBlock(this.mock, this.tx1.receipt.blockNumber - 1)).to.be.bignumber.equal('0'); - expect(await getAtBlock(this.mock, this.tx1.receipt.blockNumber)).to.be.bignumber.equal('1'); - expect(await getAtBlock(this.mock, this.tx2.receipt.blockNumber)).to.be.bignumber.equal('2'); - // Block with no new checkpoints - expect(await getAtBlock(this.mock, this.tx2.receipt.blockNumber + 1)).to.be.bignumber.equal('2'); - expect(await getAtBlock(this.mock, this.tx3.receipt.blockNumber)).to.be.bignumber.equal('3'); - expect(await getAtBlock(this.mock, this.tx3.receipt.blockNumber + 1)).to.be.bignumber.equal('3'); - }); - }); - - it('multiple checkpoints in the same block', async function () { - const lengthBefore = await getLength(this.mock); - - await batchInBlock([ - () => push(this.mock, 8, { gas: 100000 }), - () => push(this.mock, 9, { gas: 100000 }), - () => push(this.mock, 10, { gas: 100000 }), - ]); - - expect(await getLength(this.mock)).to.be.bignumber.equal(lengthBefore.addn(1)); - expect(await latest(this.mock)).to.be.bignumber.equal('10'); - }); - }); - }); - for (const length of [160, 224]) { describe(`Trace${length}`, function () { const latest = (self, ...args) => self.methods[`$latest_Checkpoints_Trace${length}(uint256)`](0, ...args); From da3ac5394243f97dccfd4a5db9b6457a0a40e16c Mon Sep 17 00:00:00 2001 From: RenanSouza2 Date: Mon, 17 Apr 2023 01:57:57 -0300 Subject: [PATCH 13/13] enable tests --- test/token/ERC20/extensions/ERC20Votes.test.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/test/token/ERC20/extensions/ERC20Votes.test.js b/test/token/ERC20/extensions/ERC20Votes.test.js index dd0619109e9..d3e237283ab 100644 --- a/test/token/ERC20/extensions/ERC20Votes.test.js +++ b/test/token/ERC20/extensions/ERC20Votes.test.js @@ -61,7 +61,7 @@ contract('ERC20Votes', function (accounts) { expect(await this.token.getPastVotes(holder, block - 6)).to.be.bignumber.equal('0'); }); - describe.only('set delegation', function () { + describe('set delegation', function () { describe('call', function () { it('delegation with balance', async function () { await this.token.$_mint(holder, supply); @@ -231,7 +231,7 @@ contract('ERC20Votes', function (accounts) { }); }); - describe.only('change delegation', function () { + describe('change delegation', function () { beforeEach(async function () { await this.token.$_mint(holder, supply); await this.token.delegate(holder, { from: holder }); @@ -269,7 +269,7 @@ contract('ERC20Votes', function (accounts) { }); }); - describe.only('transfers', function () { + describe('transfers', function () { beforeEach(async function () { await this.token.$_mint(holder, supply); }); @@ -360,7 +360,7 @@ contract('ERC20Votes', function (accounts) { }); // The following tests are a adaptation of https://github.com/compound-finance/compound-protocol/blob/master/tests/Governance/CompTest.js. - describe.only('Compound test suite', function () { + describe('Compound test suite', function () { beforeEach(async function () { await this.token.$_mint(holder, supply); }); @@ -497,7 +497,7 @@ contract('ERC20Votes', function (accounts) { }); }); - describe.only('getPastTotalSupply', function () { + describe('getPastTotalSupply', function () { beforeEach(async function () { await this.token.delegate(holder, { from: holder }); }); @@ -574,7 +574,7 @@ contract('ERC20Votes', function (accounts) { }); }); - describe.only('Voting workflow', function () { + describe('Voting workflow', function () { beforeEach(async function () { this.name = name; this.votes = this.token;