From 34148288c85e6eb7f9e323adcfde3a3a96ae0f10 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Mon, 13 Jun 2022 19:35:02 +0200 Subject: [PATCH 01/11] store history of values for quorum fraction --- .../extensions/GovernorVotesQuorumFraction.sol | 14 +++++++++----- ...test.js => GovernorVotesQuorumFraction.test.js} | 7 +++++++ 2 files changed, 16 insertions(+), 5 deletions(-) rename test/governance/extensions/{GovernorWeightQuorumFraction.test.js => GovernorVotesQuorumFraction.test.js} (95%) diff --git a/contracts/governance/extensions/GovernorVotesQuorumFraction.sol b/contracts/governance/extensions/GovernorVotesQuorumFraction.sol index 40f912cbf7b..6946ba25311 100644 --- a/contracts/governance/extensions/GovernorVotesQuorumFraction.sol +++ b/contracts/governance/extensions/GovernorVotesQuorumFraction.sol @@ -4,6 +4,7 @@ pragma solidity ^0.8.0; import "./GovernorVotes.sol"; +import "../../utils/Checkpoints.sol"; /** * @dev Extension of {Governor} for voting weight extraction from an {ERC20Votes} token and a quorum expressed as a @@ -12,7 +13,10 @@ import "./GovernorVotes.sol"; * _Available since v4.3._ */ abstract contract GovernorVotesQuorumFraction is GovernorVotes { - uint256 private _quorumNumerator; + using Checkpoints for Checkpoints.History; + + uint256 private _quorumNumerator; // DEPRECATED + Checkpoints.History private _quorumNumeratorHistory; event QuorumNumeratorUpdated(uint256 oldQuorumNumerator, uint256 newQuorumNumerator); @@ -31,7 +35,7 @@ abstract contract GovernorVotesQuorumFraction is GovernorVotes { * @dev Returns the current quorum numerator. See {quorumDenominator}. */ function quorumNumerator() public view virtual returns (uint256) { - return _quorumNumerator; + return _quorumNumeratorHistory.latest(); } /** @@ -45,7 +49,7 @@ abstract contract GovernorVotesQuorumFraction is GovernorVotes { * @dev Returns the quorum for a block number, in terms of number of votes: `supply * numerator / denominator`. */ function quorum(uint256 blockNumber) public view virtual override returns (uint256) { - return (token.getPastTotalSupply(blockNumber) * quorumNumerator()) / quorumDenominator(); + return (token.getPastTotalSupply(blockNumber) * _quorumNumeratorHistory.getAtBlock(blockNumber)) / quorumDenominator(); } /** @@ -77,8 +81,8 @@ abstract contract GovernorVotesQuorumFraction is GovernorVotes { "GovernorVotesQuorumFraction: quorumNumerator over quorumDenominator" ); - uint256 oldQuorumNumerator = _quorumNumerator; - _quorumNumerator = newQuorumNumerator; + uint256 oldQuorumNumerator = _quorumNumeratorHistory.latest(); + _quorumNumeratorHistory.push(newQuorumNumerator); emit QuorumNumeratorUpdated(oldQuorumNumerator, newQuorumNumerator); } diff --git a/test/governance/extensions/GovernorWeightQuorumFraction.test.js b/test/governance/extensions/GovernorVotesQuorumFraction.test.js similarity index 95% rename from test/governance/extensions/GovernorWeightQuorumFraction.test.js rename to test/governance/extensions/GovernorVotesQuorumFraction.test.js index ea2b55e8df1..9e6b71bc05c 100644 --- a/test/governance/extensions/GovernorWeightQuorumFraction.test.js +++ b/test/governance/extensions/GovernorVotesQuorumFraction.test.js @@ -104,6 +104,13 @@ contract('GovernorVotesQuorumFraction', function (accounts) { expect(await this.mock.quorumNumerator()).to.be.bignumber.equal(newRatio); expect(await this.mock.quorumDenominator()).to.be.bignumber.equal('100'); + + // it takes one block for the new quorum to take effect + expect(await time.latestBlock().then(blockNumber => this.mock.quorum(blockNumber.subn(1)))) + .to.be.bignumber.equal(tokenSupply.mul(ratio).divn(100)); + + await time.advanceBlock(); + expect(await time.latestBlock().then(blockNumber => this.mock.quorum(blockNumber.subn(1)))) .to.be.bignumber.equal(tokenSupply.mul(newRatio).divn(100)); }); From 2e614559dfd5080d567a9171d7c87ba5bea2751d Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Mon, 13 Jun 2022 20:39:13 +0200 Subject: [PATCH 02/11] revert to old slot if history is empty --- .../extensions/GovernorVotesQuorumFraction.sol | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/contracts/governance/extensions/GovernorVotesQuorumFraction.sol b/contracts/governance/extensions/GovernorVotesQuorumFraction.sol index 6946ba25311..eba86bf3aec 100644 --- a/contracts/governance/extensions/GovernorVotesQuorumFraction.sol +++ b/contracts/governance/extensions/GovernorVotesQuorumFraction.sol @@ -35,7 +35,16 @@ abstract contract GovernorVotesQuorumFraction is GovernorVotes { * @dev Returns the current quorum numerator. See {quorumDenominator}. */ function quorumNumerator() public view virtual returns (uint256) { - return _quorumNumeratorHistory.latest(); + return quorumNumerator(block.number - 1); + } + + /** + * @dev Returns the quorum numerator at a specific block number. See {quorumDenominator}. + */ + function quorumNumerator(uint256 blockNumber) public view virtual returns (uint256) { + return _quorumNumeratorHistory._checkpoints.length == 0 + ? _quorumNumerator + : _quorumNumeratorHistory.getAtBlock(blockNumber); } /** @@ -49,7 +58,7 @@ abstract contract GovernorVotesQuorumFraction is GovernorVotes { * @dev Returns the quorum for a block number, in terms of number of votes: `supply * numerator / denominator`. */ function quorum(uint256 blockNumber) public view virtual override returns (uint256) { - return (token.getPastTotalSupply(blockNumber) * _quorumNumeratorHistory.getAtBlock(blockNumber)) / quorumDenominator(); + return (token.getPastTotalSupply(blockNumber) * quorumNumerator(blockNumber)) / quorumDenominator(); } /** From 8d566653107a04c8250ffb348e2373ed9f22928f Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Mon, 13 Jun 2022 20:40:57 +0200 Subject: [PATCH 03/11] fix --- 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 eba86bf3aec..fbe9290db92 100644 --- a/contracts/governance/extensions/GovernorVotesQuorumFraction.sol +++ b/contracts/governance/extensions/GovernorVotesQuorumFraction.sol @@ -90,7 +90,7 @@ abstract contract GovernorVotesQuorumFraction is GovernorVotes { "GovernorVotesQuorumFraction: quorumNumerator over quorumDenominator" ); - uint256 oldQuorumNumerator = _quorumNumeratorHistory.latest(); + uint256 oldQuorumNumerator = quorumNumerator(); _quorumNumeratorHistory.push(newQuorumNumerator); emit QuorumNumeratorUpdated(oldQuorumNumerator, newQuorumNumerator); From 01affe84f34580ef6b7a75ae54b0df613453404d Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Mon, 13 Jun 2022 20:48:25 +0200 Subject: [PATCH 04/11] optimisation --- .../governance/extensions/GovernorVotesQuorumFraction.sol | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/contracts/governance/extensions/GovernorVotesQuorumFraction.sol b/contracts/governance/extensions/GovernorVotesQuorumFraction.sol index fbe9290db92..0ebfac3a7f8 100644 --- a/contracts/governance/extensions/GovernorVotesQuorumFraction.sol +++ b/contracts/governance/extensions/GovernorVotesQuorumFraction.sol @@ -35,7 +35,9 @@ abstract contract GovernorVotesQuorumFraction is GovernorVotes { * @dev Returns the current quorum numerator. See {quorumDenominator}. */ function quorumNumerator() public view virtual returns (uint256) { - return quorumNumerator(block.number - 1); + return _quorumNumeratorHistory._checkpoints.length == 0 + ? _quorumNumerator + : _quorumNumeratorHistory.latest(); } /** From 659c25366611b6a4d466bb9a1f9b72562889c9f2 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Mon, 18 Jul 2022 17:26:50 +0200 Subject: [PATCH 05/11] track old quorum in case of upgrade --- .../extensions/GovernorVotesQuorumFraction.sol | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/contracts/governance/extensions/GovernorVotesQuorumFraction.sol b/contracts/governance/extensions/GovernorVotesQuorumFraction.sol index 0ebfac3a7f8..202624ba719 100644 --- a/contracts/governance/extensions/GovernorVotesQuorumFraction.sol +++ b/contracts/governance/extensions/GovernorVotesQuorumFraction.sol @@ -93,6 +93,15 @@ abstract contract GovernorVotesQuorumFraction is GovernorVotes { ); uint256 oldQuorumNumerator = quorumNumerator(); + + // make sure we keep track of the old numerator (applicable to upgrade only) + if (_quorumNumeratorHistory._checkpoints.length == 0) { + _quorumNumeratorHistory._checkpoints.push( + Checkpoint({ _blockNumber: 0, _value: oldQuorumNumerator }) + ); + } + + // set new quorum for upcomming proposals _quorumNumeratorHistory.push(newQuorumNumerator); emit QuorumNumeratorUpdated(oldQuorumNumerator, newQuorumNumerator); From 6ab5423d9278f7d75de3c24799c05b0b84ffcc08 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Mon, 18 Jul 2022 17:53:31 -0300 Subject: [PATCH 06/11] fix identifier --- 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 202624ba719..c50a035f14b 100644 --- a/contracts/governance/extensions/GovernorVotesQuorumFraction.sol +++ b/contracts/governance/extensions/GovernorVotesQuorumFraction.sol @@ -97,7 +97,7 @@ abstract contract GovernorVotesQuorumFraction is GovernorVotes { // make sure we keep track of the old numerator (applicable to upgrade only) if (_quorumNumeratorHistory._checkpoints.length == 0) { _quorumNumeratorHistory._checkpoints.push( - Checkpoint({ _blockNumber: 0, _value: oldQuorumNumerator }) + Checkpoints({ _blockNumber: 0, _value: oldQuorumNumerator }) ); } From 19da84a7f0ff198546cfecdb27bb4acde3651d53 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Mon, 18 Jul 2022 18:16:13 -0300 Subject: [PATCH 07/11] fix creation of Checkpoint struct --- .../governance/extensions/GovernorVotesQuorumFraction.sol | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/contracts/governance/extensions/GovernorVotesQuorumFraction.sol b/contracts/governance/extensions/GovernorVotesQuorumFraction.sol index c50a035f14b..544f2c2bf7f 100644 --- a/contracts/governance/extensions/GovernorVotesQuorumFraction.sol +++ b/contracts/governance/extensions/GovernorVotesQuorumFraction.sol @@ -5,6 +5,7 @@ pragma solidity ^0.8.0; import "./GovernorVotes.sol"; import "../../utils/Checkpoints.sol"; +import "../../utils/math/SafeCast.sol"; /** * @dev Extension of {Governor} for voting weight extraction from an {ERC20Votes} token and a quorum expressed as a @@ -97,7 +98,7 @@ abstract contract GovernorVotesQuorumFraction is GovernorVotes { // make sure we keep track of the old numerator (applicable to upgrade only) if (_quorumNumeratorHistory._checkpoints.length == 0) { _quorumNumeratorHistory._checkpoints.push( - Checkpoints({ _blockNumber: 0, _value: oldQuorumNumerator }) + Checkpoints.Checkpoint({_blockNumber: 0, _value: SafeCast.toUint224(oldQuorumNumerator)}) ); } From 4240cc0f7c028102ddab9264d5873b23dec8c96f Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Mon, 18 Jul 2022 18:18:31 -0300 Subject: [PATCH 08/11] add check oldQuorumNumerator != 0 to avoid double checkpoint in constructor --- .../governance/extensions/GovernorVotesQuorumFraction.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/governance/extensions/GovernorVotesQuorumFraction.sol b/contracts/governance/extensions/GovernorVotesQuorumFraction.sol index 544f2c2bf7f..36f5a8f1891 100644 --- a/contracts/governance/extensions/GovernorVotesQuorumFraction.sol +++ b/contracts/governance/extensions/GovernorVotesQuorumFraction.sol @@ -95,8 +95,8 @@ abstract contract GovernorVotesQuorumFraction is GovernorVotes { uint256 oldQuorumNumerator = quorumNumerator(); - // make sure we keep track of the old numerator (applicable to upgrade only) - if (_quorumNumeratorHistory._checkpoints.length == 0) { + // 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)}) ); From 63c19ab86b45a598580f0c693b1632e38d644260 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Mon, 18 Jul 2022 18:20:44 -0300 Subject: [PATCH 09/11] lint --- .../extensions/GovernorVotesQuorumFraction.sol | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/contracts/governance/extensions/GovernorVotesQuorumFraction.sol b/contracts/governance/extensions/GovernorVotesQuorumFraction.sol index 36f5a8f1891..30840f8d9db 100644 --- a/contracts/governance/extensions/GovernorVotesQuorumFraction.sol +++ b/contracts/governance/extensions/GovernorVotesQuorumFraction.sol @@ -36,18 +36,17 @@ abstract contract GovernorVotesQuorumFraction is GovernorVotes { * @dev Returns the current quorum numerator. See {quorumDenominator}. */ function quorumNumerator() public view virtual returns (uint256) { - return _quorumNumeratorHistory._checkpoints.length == 0 - ? _quorumNumerator - : _quorumNumeratorHistory.latest(); + return _quorumNumeratorHistory._checkpoints.length == 0 ? _quorumNumerator : _quorumNumeratorHistory.latest(); } /** * @dev Returns the quorum numerator at a specific block number. See {quorumDenominator}. */ function quorumNumerator(uint256 blockNumber) public view virtual returns (uint256) { - return _quorumNumeratorHistory._checkpoints.length == 0 - ? _quorumNumerator - : _quorumNumeratorHistory.getAtBlock(blockNumber); + return + _quorumNumeratorHistory._checkpoints.length == 0 + ? _quorumNumerator + : _quorumNumeratorHistory.getAtBlock(blockNumber); } /** From 1a34eb7db7b1245f93a1ec91e23c44374fadff19 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Mon, 18 Jul 2022 18:37:08 -0300 Subject: [PATCH 10/11] rephrase comment --- 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 30840f8d9db..121995827d1 100644 --- a/contracts/governance/extensions/GovernorVotesQuorumFraction.sol +++ b/contracts/governance/extensions/GovernorVotesQuorumFraction.sol @@ -101,7 +101,7 @@ abstract contract GovernorVotesQuorumFraction is GovernorVotes { ); } - // set new quorum for upcomming proposals + // Set new quorum for future proposals _quorumNumeratorHistory.push(newQuorumNumerator); emit QuorumNumeratorUpdated(oldQuorumNumerator, newQuorumNumerator); From 08e9bdf5e5d7064cb4822367e54e68ffb2e3fd34 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 27 Jul 2022 10:21:01 +0200 Subject: [PATCH 11/11] optimistic quorum Numerator search --- .../extensions/GovernorVotesQuorumFraction.sol | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/contracts/governance/extensions/GovernorVotesQuorumFraction.sol b/contracts/governance/extensions/GovernorVotesQuorumFraction.sol index 121995827d1..e5f0888c043 100644 --- a/contracts/governance/extensions/GovernorVotesQuorumFraction.sol +++ b/contracts/governance/extensions/GovernorVotesQuorumFraction.sol @@ -43,10 +43,20 @@ abstract contract GovernorVotesQuorumFraction is GovernorVotes { * @dev Returns the quorum numerator at a specific block number. See {quorumDenominator}. */ function quorumNumerator(uint256 blockNumber) public view virtual returns (uint256) { - return - _quorumNumeratorHistory._checkpoints.length == 0 - ? _quorumNumerator - : _quorumNumeratorHistory.getAtBlock(blockNumber); + // If history is empty, fallback to old storage + uint256 length = _quorumNumeratorHistory._checkpoints.length; + if (length == 0) { + return _quorumNumerator; + } + + // Optimistic search, check the latest checkpoint + Checkpoints.Checkpoint memory latest = _quorumNumeratorHistory._checkpoints[length - 1]; + if (latest._blockNumber <= blockNumber) { + return latest._value; + } + + // Otherwize, do the binary search + return _quorumNumeratorHistory.getAtBlock(blockNumber); } /**