From e2d2ebc8fcb8d5582b262dd94f432a40df43b114 Mon Sep 17 00:00:00 2001 From: JulissaDantes Date: Tue, 29 Nov 2022 10:25:52 -0400 Subject: [PATCH] Make ERC20Votes independent from ERC20Permit (#3816) Co-authored-by: Francisco --- CHANGELOG.md | 2 + contracts/governance/utils/Votes.sol | 40 ++- contracts/mocks/ERC20VotesCompMock.sol | 2 +- contracts/mocks/ERC20VotesMock.sol | 2 +- contracts/mocks/ERC721VotesMock.sol | 2 +- contracts/mocks/NoncesImpl.sol | 11 + contracts/mocks/VotesMock.sol | 2 +- .../token/ERC20/extensions/ERC20Permit.sol | 23 +- .../token/ERC20/extensions/ERC20Votes.sol | 251 ++---------------- contracts/utils/Checkpoints.sol | 7 + contracts/utils/Nonces.sol | 31 +++ scripts/generate/templates/Checkpoints.js | 7 + test/governance/utils/Votes.behavior.js | 29 +- test/governance/utils/Votes.test.js | 8 +- .../token/ERC20/extensions/ERC20Votes.test.js | 46 ++-- .../ERC20/extensions/ERC20VotesComp.test.js | 35 ++- .../ERC721/extensions/ERC721Votes.test.js | 51 ++-- test/utils/Nonces.test.js | 25 ++ 18 files changed, 231 insertions(+), 343 deletions(-) create mode 100644 contracts/mocks/NoncesImpl.sol create mode 100644 contracts/utils/Nonces.sol create mode 100644 test/utils/Nonces.test.js diff --git a/CHANGELOG.md b/CHANGELOG.md index 61e6d6c23..bf92d9b43 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,8 @@ ## Unreleased (Breaking) * `TimelockController`: Changed the role architecture to use `DEFAULT_ADMIN_ROLE` as the admin for all roles, instead of the bespoke `TIMELOCK_ADMIN_ROLE` that was used previously. This aligns with the general recommendation for `AccessControl` and makes the addition of new roles easier. Accordingly, the `admin` parameter and timelock will now be granted `DEFAULT_ADMIN_ROLE` instead of `TIMELOCK_ADMIN_ROLE`. ([#3799](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3799)) + * `ERC20Votes`: Changed internal vote accounting to reusable `Votes` module previously used by `ERC721Votes`. Removed implicit `ERC20Permit` inheritance. [#3816](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3816) + * `Nonces`: Added a new contract to keep track of user nonces. Used for signatures in `ERC20Permit`, `ERC20Votes`, and `ERC721Votes`. ([#3816](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3816)) * Removed presets in favor of [OpenZeppelin Contracts Wizard](https://wizard.openzeppelin.com/). * `TransparentUpgradeableProxy`: Removed `admin` and `implementation` getters, which were only callable by the proxy owner and thus not very useful. ([#3820](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3820)) * `ProxyAdmin`: Removed `getProxyAdmin` and `getProxyImplementation` getters. ([#3820](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3820)) diff --git a/contracts/governance/utils/Votes.sol b/contracts/governance/utils/Votes.sol index f763589c6..dfcf1e610 100644 --- a/contracts/governance/utils/Votes.sol +++ b/contracts/governance/utils/Votes.sol @@ -3,10 +3,11 @@ pragma solidity ^0.8.0; import "../../utils/Context.sol"; -import "../../utils/Counters.sol"; +import "../../utils/Nonces.sol"; import "../../utils/Checkpoints.sol"; import "../../utils/cryptography/EIP712.sol"; import "./IVotes.sol"; +import "../../utils/math/SafeCast.sol"; /** * @dev This is a base abstract contract that tracks voting units, which are a measure of voting power that can be @@ -28,9 +29,8 @@ import "./IVotes.sol"; * * _Available since v4.5._ */ -abstract contract Votes is IVotes, Context, EIP712 { +abstract contract Votes is IVotes, Context, EIP712, Nonces { using Checkpoints for Checkpoints.History; - using Counters for Counters.Counter; bytes32 private constant _DELEGATION_TYPEHASH = keccak256("Delegation(address delegatee,uint256 nonce,uint256 expiry)"); @@ -39,8 +39,6 @@ abstract contract Votes is IVotes, Context, EIP712 { mapping(address => Checkpoints.History) private _delegateCheckpoints; Checkpoints.History private _totalCheckpoints; - mapping(address => Counters.Counter) private _nonces; - /** * @dev Returns the current amount of votes that `account` has. */ @@ -170,6 +168,20 @@ abstract contract Votes is IVotes, Context, EIP712 { } } + /** + * @dev Get number of checkpoints for `account`. + */ + function _numCheckpoints(address account) internal view virtual returns (uint32) { + 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.Checkpoint memory) { + return _delegateCheckpoints[account].getAtPosition(pos); + } + function _add(uint256 a, uint256 b) private pure returns (uint256) { return a + b; } @@ -178,24 +190,6 @@ abstract contract Votes is IVotes, Context, EIP712 { return a - b; } - /** - * @dev Consumes a nonce. - * - * Returns the current value and increments nonce. - */ - function _useNonce(address owner) internal virtual returns (uint256 current) { - Counters.Counter storage nonce = _nonces[owner]; - current = nonce.current(); - nonce.increment(); - } - - /** - * @dev Returns an address nonce. - */ - function nonces(address owner) public view virtual returns (uint256) { - return _nonces[owner].current(); - } - /** * @dev Returns the contract's {EIP712} domain separator. */ diff --git a/contracts/mocks/ERC20VotesCompMock.sol b/contracts/mocks/ERC20VotesCompMock.sol index 171071fd5..5a19ae81a 100644 --- a/contracts/mocks/ERC20VotesCompMock.sol +++ b/contracts/mocks/ERC20VotesCompMock.sol @@ -5,7 +5,7 @@ pragma solidity ^0.8.0; import "../token/ERC20/extensions/ERC20VotesComp.sol"; contract ERC20VotesCompMock is ERC20VotesComp { - constructor(string memory name, string memory symbol) ERC20(name, symbol) ERC20Permit(name) {} + constructor(string memory name, string memory symbol) ERC20(name, symbol) EIP712(name, "1") {} function mint(address account, uint256 amount) public { _mint(account, amount); diff --git a/contracts/mocks/ERC20VotesMock.sol b/contracts/mocks/ERC20VotesMock.sol index 0975e8b9f..654a089ac 100644 --- a/contracts/mocks/ERC20VotesMock.sol +++ b/contracts/mocks/ERC20VotesMock.sol @@ -5,7 +5,7 @@ pragma solidity ^0.8.0; import "../token/ERC20/extensions/ERC20Votes.sol"; contract ERC20VotesMock is ERC20Votes { - constructor(string memory name, string memory symbol) ERC20(name, symbol) ERC20Permit(name) {} + constructor(string memory name, string memory symbol) ERC20(name, symbol) EIP712(name, "1") {} function mint(address account, uint256 amount) public { _mint(account, amount); diff --git a/contracts/mocks/ERC721VotesMock.sol b/contracts/mocks/ERC721VotesMock.sol index acb51ebfb..2d9447428 100644 --- a/contracts/mocks/ERC721VotesMock.sol +++ b/contracts/mocks/ERC721VotesMock.sol @@ -15,7 +15,7 @@ contract ERC721VotesMock is ERC721Votes { _mint(account, tokenId); } - function burn(uint256 tokenId) public { + function burn(address, uint256 tokenId) public { _burn(tokenId); } diff --git a/contracts/mocks/NoncesImpl.sol b/contracts/mocks/NoncesImpl.sol new file mode 100644 index 000000000..41b913c81 --- /dev/null +++ b/contracts/mocks/NoncesImpl.sol @@ -0,0 +1,11 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.0; + +import "../utils/Nonces.sol"; + +contract NoncesImpl is Nonces { + function useNonce(address owner) public { + super._useNonce(owner); + } +} diff --git a/contracts/mocks/VotesMock.sol b/contracts/mocks/VotesMock.sol index f888490da..f080bfb72 100644 --- a/contracts/mocks/VotesMock.sol +++ b/contracts/mocks/VotesMock.sol @@ -28,7 +28,7 @@ contract VotesMock is Votes { _transferVotingUnits(address(0), account, 1); } - function burn(uint256 voteId) external { + function burn(address, uint256 voteId) external { address owner = _owners[voteId]; _balances[owner] -= 1; _transferVotingUnits(owner, address(0), 1); diff --git a/contracts/token/ERC20/extensions/ERC20Permit.sol b/contracts/token/ERC20/extensions/ERC20Permit.sol index a357199b1..70b48a8d4 100644 --- a/contracts/token/ERC20/extensions/ERC20Permit.sol +++ b/contracts/token/ERC20/extensions/ERC20Permit.sol @@ -7,7 +7,7 @@ import "./IERC20Permit.sol"; import "../ERC20.sol"; import "../../../utils/cryptography/ECDSA.sol"; import "../../../utils/cryptography/EIP712.sol"; -import "../../../utils/Counters.sol"; +import "../../../utils/Nonces.sol"; /** * @dev Implementation of the ERC20 Permit extension allowing approvals to be made via signatures, as defined in @@ -19,11 +19,7 @@ import "../../../utils/Counters.sol"; * * _Available since v3.4._ */ -abstract contract ERC20Permit is ERC20, IERC20Permit, EIP712 { - using Counters for Counters.Counter; - - mapping(address => Counters.Counter) private _nonces; - +abstract contract ERC20Permit is ERC20, IERC20Permit, EIP712, Nonces { // solhint-disable-next-line var-name-mixedcase bytes32 private constant _PERMIT_TYPEHASH = keccak256("Permit(address owner,address spender,uint256 value,uint256 nonce,uint256 deadline)"); @@ -70,8 +66,8 @@ abstract contract ERC20Permit is ERC20, IERC20Permit, EIP712 { /** * @dev See {IERC20Permit-nonces}. */ - function nonces(address owner) public view virtual override returns (uint256) { - return _nonces[owner].current(); + function nonces(address owner) public view virtual override(IERC20Permit, Nonces) returns (uint256) { + return super.nonces(owner); } /** @@ -81,15 +77,4 @@ abstract contract ERC20Permit is ERC20, IERC20Permit, EIP712 { function DOMAIN_SEPARATOR() external view override returns (bytes32) { return _domainSeparatorV4(); } - - /** - * @dev "Consume a nonce": return the current value and increment. - * - * _Available since v4.1._ - */ - function _useNonce(address owner) internal virtual returns (uint256 current) { - Counters.Counter storage nonce = _nonces[owner]; - current = nonce.current(); - nonce.increment(); - } } diff --git a/contracts/token/ERC20/extensions/ERC20Votes.sol b/contracts/token/ERC20/extensions/ERC20Votes.sol index bf319c26f..e2a070921 100644 --- a/contracts/token/ERC20/extensions/ERC20Votes.sol +++ b/contracts/token/ERC20/extensions/ERC20Votes.sol @@ -3,11 +3,9 @@ pragma solidity ^0.8.0; -import "./ERC20Permit.sol"; -import "../../../utils/math/Math.sol"; -import "../../../governance/utils/IVotes.sol"; +import "../ERC20.sol"; +import "../../../governance/utils/Votes.sol"; import "../../../utils/math/SafeCast.sol"; -import "../../../utils/cryptography/ECDSA.sol"; /** * @dev Extension of ERC20 to support Compound-like voting and delegation. This version is more generic than Compound's, @@ -24,148 +22,7 @@ import "../../../utils/cryptography/ECDSA.sol"; * * _Available since v4.2._ */ -abstract contract ERC20Votes is IVotes, ERC20Permit { - struct Checkpoint { - uint32 fromBlock; - uint224 votes; - } - - bytes32 private constant _DELEGATION_TYPEHASH = - keccak256("Delegation(address delegatee,uint256 nonce,uint256 expiry)"); - - mapping(address => address) private _delegates; - mapping(address => Checkpoint[]) private _checkpoints; - Checkpoint[] private _totalSupplyCheckpoints; - - /** - * @dev Get the `pos`-th checkpoint for `account`. - */ - function checkpoints(address account, uint32 pos) public view virtual returns (Checkpoint memory) { - return _checkpoints[account][pos]; - } - - /** - * @dev Get number of checkpoints for `account`. - */ - function numCheckpoints(address account) public view virtual returns (uint32) { - return SafeCast.toUint32(_checkpoints[account].length); - } - - /** - * @dev Get the address `account` is currently delegating to. - */ - function delegates(address account) public view virtual override returns (address) { - return _delegates[account]; - } - - /** - * @dev Gets the current votes balance for `account` - */ - function getVotes(address account) public view virtual override returns (uint256) { - uint256 pos = _checkpoints[account].length; - unchecked { - return pos == 0 ? 0 : _checkpoints[account][pos - 1].votes; - } - } - - /** - * @dev Retrieve the number of votes for `account` at the end of `blockNumber`. - * - * Requirements: - * - * - `blockNumber` must have been already mined - */ - function getPastVotes(address account, uint256 blockNumber) public view virtual override returns (uint256) { - require(blockNumber < block.number, "ERC20Votes: block not yet mined"); - return _checkpointsLookup(_checkpoints[account], blockNumber); - } - - /** - * @dev Retrieve the `totalSupply` at the end of `blockNumber`. Note, this value is the sum of all balances. - * It is NOT the sum of all the delegated votes! - * - * Requirements: - * - * - `blockNumber` must have been already mined - */ - function getPastTotalSupply(uint256 blockNumber) public view virtual override returns (uint256) { - require(blockNumber < block.number, "ERC20Votes: block not yet mined"); - return _checkpointsLookup(_totalSupplyCheckpoints, blockNumber); - } - - /** - * @dev Lookup a value in a list of (sorted) checkpoints. - */ - function _checkpointsLookup(Checkpoint[] storage ckpts, uint256 blockNumber) private view returns (uint256) { - // We run a binary search to look for the earliest checkpoint taken after `blockNumber`. - // - // Initially we check if the block is recent to narrow the search range. - // During the loop, the index of the wanted checkpoint remains in the range [low-1, high). - // With each iteration, either `low` or `high` is moved towards the middle of the range to maintain the invariant. - // - If the middle checkpoint is after `blockNumber`, we look in [low, mid) - // - If the middle checkpoint is before or equal to `blockNumber`, we look in [mid+1, high) - // Once we reach a single value (when low == high), we've found the right checkpoint at the index high-1, if not - // out of bounds (in which case we're looking too far in the past and the result is 0). - // Note that if the latest checkpoint available is exactly for `blockNumber`, we end up with an index that is - // past the end of the array, so we technically don't find a checkpoint after `blockNumber`, but it works out - // the same. - uint256 length = ckpts.length; - - uint256 low = 0; - uint256 high = length; - - if (length > 5) { - uint256 mid = length - Math.sqrt(length); - if (_unsafeAccess(ckpts, mid).fromBlock > blockNumber) { - high = mid; - } else { - low = mid + 1; - } - } - - while (low < high) { - uint256 mid = Math.average(low, high); - if (_unsafeAccess(ckpts, mid).fromBlock > blockNumber) { - high = mid; - } else { - low = mid + 1; - } - } - - unchecked { - return high == 0 ? 0 : _unsafeAccess(ckpts, high - 1).votes; - } - } - - /** - * @dev Delegate votes from the sender to `delegatee`. - */ - function delegate(address delegatee) public virtual override { - _delegate(_msgSender(), delegatee); - } - - /** - * @dev Delegates votes from signer to `delegatee` - */ - function delegateBySig( - address delegatee, - uint256 nonce, - uint256 expiry, - uint8 v, - bytes32 r, - bytes32 s - ) public virtual override { - require(block.timestamp <= expiry, "ERC20Votes: signature expired"); - address signer = ECDSA.recover( - _hashTypedDataV4(keccak256(abi.encode(_DELEGATION_TYPEHASH, delegatee, nonce, expiry))), - v, - r, - s - ); - require(nonce == _useNonce(signer), "ERC20Votes: invalid nonce"); - _delegate(signer, delegatee); - } - +abstract contract ERC20Votes is ERC20, Votes { /** * @dev Maximum token supply. Defaults to `type(uint224).max` (2^224^ - 1). */ @@ -173,25 +30,6 @@ abstract contract ERC20Votes is IVotes, ERC20Permit { return type(uint224).max; } - /** - * @dev Snapshots the totalSupply after it has been increased. - */ - function _mint(address account, uint256 amount) internal virtual override { - super._mint(account, amount); - require(totalSupply() <= _maxSupply(), "ERC20Votes: total supply risks overflowing votes"); - - _writeCheckpoint(_totalSupplyCheckpoints, _add, amount); - } - - /** - * @dev Snapshots the totalSupply after it has been decreased. - */ - function _burn(address account, uint256 amount) internal virtual override { - super._burn(account, amount); - - _writeCheckpoint(_totalSupplyCheckpoints, _subtract, amount); - } - /** * @dev Move voting power when tokens are transferred. * @@ -202,79 +40,36 @@ abstract contract ERC20Votes is IVotes, ERC20Permit { address to, uint256 amount ) internal virtual override { + _transferVotingUnits(from, to, amount); super._afterTokenTransfer(from, to, amount); - - _moveVotingPower(delegates(from), delegates(to), amount); } /** - * @dev Change delegation for `delegator` to `delegatee`. - * - * Emits events {IVotes-DelegateChanged} and {IVotes-DelegateVotesChanged}. + * @dev Get number of checkpoints for `account`. */ - function _delegate(address delegator, address delegatee) internal virtual { - address currentDelegate = delegates(delegator); - uint256 delegatorBalance = balanceOf(delegator); - _delegates[delegator] = delegatee; - - emit DelegateChanged(delegator, currentDelegate, delegatee); - - _moveVotingPower(currentDelegate, delegatee, delegatorBalance); + function numCheckpoints(address account) public view virtual returns (uint32) { + return _numCheckpoints(account); } - function _moveVotingPower( - address src, - address dst, - uint256 amount - ) private { - if (src != dst && amount > 0) { - if (src != address(0)) { - (uint256 oldWeight, uint256 newWeight) = _writeCheckpoint(_checkpoints[src], _subtract, amount); - emit DelegateVotesChanged(src, oldWeight, newWeight); - } - - if (dst != address(0)) { - (uint256 oldWeight, uint256 newWeight) = _writeCheckpoint(_checkpoints[dst], _add, amount); - emit DelegateVotesChanged(dst, oldWeight, newWeight); - } - } + /** + * @dev Get the `pos`-th checkpoint for `account`. + */ + function checkpoints(address account, uint32 pos) public view virtual returns (Checkpoints.Checkpoint memory) { + return _checkpoints(account, pos); } - function _writeCheckpoint( - Checkpoint[] storage ckpts, - function(uint256, uint256) view returns (uint256) op, - uint256 delta - ) private returns (uint256 oldWeight, uint256 newWeight) { - uint256 pos = ckpts.length; - - unchecked { - Checkpoint memory oldCkpt = pos == 0 ? Checkpoint(0, 0) : _unsafeAccess(ckpts, pos - 1); - - oldWeight = oldCkpt.votes; - newWeight = op(oldWeight, delta); - - if (pos > 0 && oldCkpt.fromBlock == block.number) { - _unsafeAccess(ckpts, pos - 1).votes = SafeCast.toUint224(newWeight); - } else { - ckpts.push( - Checkpoint({fromBlock: SafeCast.toUint32(block.number), votes: SafeCast.toUint224(newWeight)}) - ); - } - } + /** + * @dev Returns the balance of `account`. + */ + function _getVotingUnits(address account) internal view virtual override returns (uint256) { + return balanceOf(account); } - function _add(uint256 a, uint256 b) private pure returns (uint256) { - return a + b; - } - - function _subtract(uint256 a, uint256 b) private pure returns (uint256) { - return a - b; - } - - function _unsafeAccess(Checkpoint[] storage ckpts, uint256 pos) private pure returns (Checkpoint storage result) { - assembly { - mstore(0, ckpts.slot) - result.slot := add(keccak256(0, 0x20), pos) - } + /** + * @dev Snapshots the totalSupply after it has been increased. + */ + function _mint(address account, uint256 amount) internal virtual override { + super._mint(account, amount); + require(totalSupply() <= _maxSupply(), "ERC20Votes: total supply risks overflowing votes"); } } diff --git a/contracts/utils/Checkpoints.sol b/contracts/utils/Checkpoints.sol index 81a8b5d5a..da9523b5e 100644 --- a/contracts/utils/Checkpoints.sol +++ b/contracts/utils/Checkpoints.sol @@ -68,6 +68,13 @@ library Checkpoints { 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. * diff --git a/contracts/utils/Nonces.sol b/contracts/utils/Nonces.sol new file mode 100644 index 000000000..ce65909ff --- /dev/null +++ b/contracts/utils/Nonces.sol @@ -0,0 +1,31 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.0; + +import "./Counters.sol"; + +/** + * @dev Provides tracking nonces for addresses. Nonces will only increment. + */ +abstract contract Nonces { + using Counters for Counters.Counter; + + mapping(address => Counters.Counter) private _nonces; + + /** + * @dev Returns an address nonce. + */ + function nonces(address owner) public view virtual returns (uint256) { + return _nonces[owner].current(); + } + + /** + * @dev Consumes a nonce. + * + * Returns the current value and increments nonce. + */ + function _useNonce(address owner) internal virtual returns (uint256 current) { + Counters.Counter storage nonce = _nonces[owner]; + current = nonce.current(); + nonce.increment(); + } +} diff --git a/scripts/generate/templates/Checkpoints.js b/scripts/generate/templates/Checkpoints.js index b78ed5ceb..ed8940b75 100644 --- a/scripts/generate/templates/Checkpoints.js +++ b/scripts/generate/templates/Checkpoints.js @@ -107,6 +107,13 @@ function getAtProbablyRecentBlock(${opts.historyTypeName} storage self, uint256 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) { + return self._checkpoints[pos]; +} + /** * @dev Pushes a value onto a History so that it is stored as the checkpoint for the current block. * diff --git a/test/governance/utils/Votes.behavior.js b/test/governance/utils/Votes.behavior.js index aee227bdb..40fa4c152 100644 --- a/test/governance/utils/Votes.behavior.js +++ b/test/governance/utils/Votes.behavior.js @@ -7,6 +7,7 @@ const ethSigUtil = require('eth-sig-util'); const Wallet = require('ethereumjs-wallet').default; const { EIP712Domain, domainSeparator } = require('../../helpers/eip712'); +const { web3 } = require('hardhat'); const Delegation = [ { name: 'delegatee', type: 'address' }, @@ -45,7 +46,7 @@ function shouldBehaveLikeVotes () { }); beforeEach(async function () { - await this.votes.mint(delegatorAddress, this.NFT0); + await this.votes.mint(delegatorAddress, this.token0); }); it('accept signed delegation', async function () { @@ -151,7 +152,7 @@ function shouldBehaveLikeVotes () { describe('set delegation', function () { describe('call', function () { it('delegation with tokens', async function () { - await this.votes.mint(this.account1, this.NFT0); + await this.votes.mint(this.account1, this.token0); expect(await this.votes.delegates(this.account1)).to.be.equal(ZERO_ADDRESS); const { receipt } = await this.votes.delegate(this.account1, { from: this.account1 }); @@ -192,7 +193,7 @@ function shouldBehaveLikeVotes () { describe('change delegation', function () { beforeEach(async function () { - await this.votes.mint(this.account1, this.NFT0); + await this.votes.mint(this.account1, this.token0); await this.votes.delegate(this.account1, { from: this.account1 }); }); @@ -245,7 +246,7 @@ function shouldBehaveLikeVotes () { }); it('returns the latest block if >= last checkpoint block', async function () { - const t1 = await this.votes.mint(this.account1, this.NFT0); + const t1 = await this.votes.mint(this.account1, this.token0); await time.advanceBlock(); await time.advanceBlock(); @@ -255,7 +256,7 @@ function shouldBehaveLikeVotes () { it('returns zero if < first checkpoint block', async function () { await time.advanceBlock(); - const t2 = await this.votes.mint(this.account1, this.NFT1); + const t2 = await this.votes.mint(this.account1, this.token1); await time.advanceBlock(); await time.advanceBlock(); @@ -264,19 +265,19 @@ function shouldBehaveLikeVotes () { }); it('generally returns the voting balance at the appropriate checkpoint', async function () { - const t1 = await this.votes.mint(this.account1, this.NFT1); + const t1 = await this.votes.mint(this.account1, this.token1); await time.advanceBlock(); await time.advanceBlock(); - const t2 = await this.votes.burn(this.NFT1); + const t2 = await this.votes.burn(this.account1, this.token1); await time.advanceBlock(); await time.advanceBlock(); - const t3 = await this.votes.mint(this.account1, this.NFT2); + const t3 = await this.votes.mint(this.account1, this.token2); await time.advanceBlock(); await time.advanceBlock(); - const t4 = await this.votes.burn(this.NFT2); + const t4 = await this.votes.burn(this.account1, this.token2); await time.advanceBlock(); await time.advanceBlock(); - const t5 = await this.votes.mint(this.account1, this.NFT3); + const t5 = await this.votes.mint(this.account1, this.token3); await time.advanceBlock(); await time.advanceBlock(); @@ -298,10 +299,10 @@ function shouldBehaveLikeVotes () { // https://github.com/compound-finance/compound-protocol/blob/master/tests/Governance/CompTest.js. describe('Compound test suite', function () { beforeEach(async function () { - await this.votes.mint(this.account1, this.NFT0); - await this.votes.mint(this.account1, this.NFT1); - await this.votes.mint(this.account1, this.NFT2); - await this.votes.mint(this.account1, this.NFT3); + await this.votes.mint(this.account1, this.token0); + await this.votes.mint(this.account1, this.token1); + await this.votes.mint(this.account1, this.token2); + await this.votes.mint(this.account1, this.token3); }); describe('getPastVotes', function () { diff --git a/test/governance/utils/Votes.test.js b/test/governance/utils/Votes.test.js index 32b7d1dca..8d5de78f7 100644 --- a/test/governance/utils/Votes.test.js +++ b/test/governance/utils/Votes.test.js @@ -50,10 +50,10 @@ contract('Votes', function (accounts) { this.account1 = account1; this.account2 = account2; this.account1Delegatee = account2; - this.NFT0 = new BN('10000000000000000000000000'); - this.NFT1 = new BN('10'); - this.NFT2 = new BN('20'); - this.NFT3 = new BN('30'); + this.token0 = new BN('10000000000000000000000000'); + this.token1 = new BN('10'); + this.token2 = new BN('20'); + this.token3 = new BN('30'); }); shouldBehaveLikeVotes(); diff --git a/test/token/ERC20/extensions/ERC20Votes.test.js b/test/token/ERC20/extensions/ERC20Votes.test.js index 9d3160bd1..0d3ab74aa 100644 --- a/test/token/ERC20/extensions/ERC20Votes.test.js +++ b/test/token/ERC20/extensions/ERC20Votes.test.js @@ -2,8 +2,9 @@ const { BN, constants, expectEvent, expectRevert, time } = require('@openzeppelin/test-helpers'); const { expect } = require('chai'); -const { MAX_UINT256, ZERO_ADDRESS, ZERO_BYTES32 } = constants; +const { MAX_UINT256, ZERO_ADDRESS } = constants; +const { shouldBehaveLikeVotes } = require('../../../governance/utils/Votes.behavior'); const { fromRpcSig } = require('ethereumjs-util'); const ethSigUtil = require('eth-sig-util'); const Wallet = require('ethereumjs-wallet').default; @@ -52,7 +53,7 @@ contract('ERC20Votes', function (accounts) { const amount = new BN('2').pow(new BN('224')); await expectRevert( this.token.mint(holder, amount), - 'ERC20Votes: total supply risks overflowing votes', + "SafeCast: value doesn't fit in 224 bits", ); }); @@ -62,7 +63,6 @@ contract('ERC20Votes', function (accounts) { await this.token.mint(holder, 1); } const block = await web3.eth.getBlockNumber(); - expect(await this.token.numCheckpoints(holder)).to.be.bignumber.equal('6'); // recent expect(await this.token.getPastVotes(holder, block - 1)).to.be.bignumber.equal('5'); // non-recent @@ -172,7 +172,7 @@ contract('ERC20Votes', function (accounts) { await expectRevert( this.token.delegateBySig(delegatorAddress, nonce, MAX_UINT256, v, r, s), - 'ERC20Votes: invalid nonce', + 'Votes: invalid nonce', ); }); @@ -204,7 +204,7 @@ contract('ERC20Votes', function (accounts) { )); await expectRevert( this.token.delegateBySig(delegatorAddress, nonce + 1, MAX_UINT256, v, r, s), - 'ERC20Votes: invalid nonce', + 'Votes: invalid nonce', ); }); @@ -221,7 +221,7 @@ contract('ERC20Votes', function (accounts) { await expectRevert( this.token.delegateBySig(delegatorAddress, nonce, expiry, v, r, s), - 'ERC20Votes: signature expired', + 'Votes: signature expired', ); }); }); @@ -341,12 +341,6 @@ contract('ERC20Votes', function (accounts) { await this.token.mint(holder, supply); }); - describe('balanceOf', function () { - it('grants to initial account', async function () { - expect(await this.token.balanceOf(holder)).to.be.bignumber.equal('10000000000000000000000000'); - }); - }); - describe('numCheckpoints', function () { it('returns the number of checkpoints for a delegate', async function () { await this.token.transfer(recipient, '100', { from: holder }); //give an account a few tokens for readability @@ -354,7 +348,7 @@ contract('ERC20Votes', function (accounts) { const t1 = await this.token.delegate(other1, { from: recipient }); expect(await this.token.numCheckpoints(other1)).to.be.bignumber.equal('1'); - + const t2 = await this.token.transfer(other2, 10, { from: recipient }); expect(await this.token.numCheckpoints(other1)).to.be.bignumber.equal('2'); @@ -396,11 +390,17 @@ contract('ERC20Votes', function (accounts) { }); }); + describe('balanceOf', function () { + it('grants to initial account', async function () { + expect(await this.token.balanceOf(holder)).to.be.bignumber.equal('10000000000000000000000000'); + }); + }); + describe('getPastVotes', function () { it('reverts if block number >= current block', async function () { await expectRevert( this.token.getPastVotes(other1, 5e10), - 'ERC20Votes: block not yet mined', + 'Checkpoints: block not yet mined', ); }); @@ -462,7 +462,7 @@ contract('ERC20Votes', function (accounts) { it('reverts if block number >= current block', async function () { await expectRevert( this.token.getPastTotalSupply(5e10), - 'ERC20Votes: block not yet mined', + 'Votes: block not yet mined', ); }); @@ -515,4 +515,20 @@ contract('ERC20Votes', function (accounts) { expect(await this.token.getPastTotalSupply(t4.receipt.blockNumber + 1)).to.be.bignumber.equal('10000000000000000000000000'); }); }); + + describe('Voting workflow', function () { + beforeEach(async function () { + this.account1 = holder; + this.account1Delegatee = holderDelegatee; + this.account2 = recipient; + this.name = 'My Token'; + this.votes = this.token + this.token0 = 1; + this.token1 = 1; + this.token2 = 1; + this.token3 = 1; + }); + + shouldBehaveLikeVotes(); + }); }); diff --git a/test/token/ERC20/extensions/ERC20VotesComp.test.js b/test/token/ERC20/extensions/ERC20VotesComp.test.js index b70c6d167..de2090b67 100644 --- a/test/token/ERC20/extensions/ERC20VotesComp.test.js +++ b/test/token/ERC20/extensions/ERC20VotesComp.test.js @@ -2,15 +2,16 @@ const { BN, constants, expectEvent, expectRevert, time } = require('@openzeppelin/test-helpers'); const { expect } = require('chai'); -const { MAX_UINT256, ZERO_ADDRESS, ZERO_BYTES32 } = constants; +const { MAX_UINT256, ZERO_ADDRESS } = constants; +const { batchInBlock } = require('../../../helpers/txpool'); +const { shouldBehaveLikeVotes } = require('../../../governance/utils/Votes.behavior'); const { fromRpcSig } = require('ethereumjs-util'); const ethSigUtil = require('eth-sig-util'); const Wallet = require('ethereumjs-wallet').default; const ERC20VotesCompMock = artifacts.require('ERC20VotesCompMock'); -const { batchInBlock } = require('../../../helpers/txpool'); const { EIP712Domain, domainSeparator } = require('../../../helpers/eip712'); const Delegation = [ @@ -52,7 +53,7 @@ contract('ERC20VotesComp', function (accounts) { const amount = new BN('2').pow(new BN('96')); await expectRevert( this.token.mint(holder, amount), - 'ERC20Votes: total supply risks overflowing votes', + "ERC20Votes: total supply risks overflowing votes", ); }); @@ -159,7 +160,7 @@ contract('ERC20VotesComp', function (accounts) { await expectRevert( this.token.delegateBySig(delegatorAddress, nonce, MAX_UINT256, v, r, s), - 'ERC20Votes: invalid nonce', + 'Votes: invalid nonce', ); }); @@ -191,7 +192,7 @@ contract('ERC20VotesComp', function (accounts) { )); await expectRevert( this.token.delegateBySig(delegatorAddress, nonce + 1, MAX_UINT256, v, r, s), - 'ERC20Votes: invalid nonce', + 'Votes: invalid nonce', ); }); @@ -208,7 +209,7 @@ contract('ERC20VotesComp', function (accounts) { await expectRevert( this.token.delegateBySig(delegatorAddress, nonce, expiry, v, r, s), - 'ERC20Votes: signature expired', + 'Votes: signature expired', ); }); }); @@ -373,12 +374,12 @@ contract('ERC20VotesComp', function (accounts) { expect(await this.token.checkpoints(other1, 1)).to.be.deep.equal([ t4.receipt.blockNumber.toString(), '100' ]); }); }); - + describe('getPriorVotes', function () { it('reverts if block number >= current block', async function () { await expectRevert( this.token.getPriorVotes(other1, 5e10), - 'ERC20Votes: block not yet mined', + 'Checkpoints: block not yet mined', ); }); @@ -440,7 +441,7 @@ contract('ERC20VotesComp', function (accounts) { it('reverts if block number >= current block', async function () { await expectRevert( this.token.getPastTotalSupply(5e10), - 'ERC20Votes: block not yet mined', + 'Votes: block not yet mined', ); }); @@ -493,4 +494,20 @@ contract('ERC20VotesComp', function (accounts) { expect(await this.token.getPastTotalSupply(t4.receipt.blockNumber + 1)).to.be.bignumber.equal('10000000000000000000000000'); }); }); + + describe('Voting workflow', function () { + beforeEach(async function () { + this.account1 = holder; + this.account1Delegatee = holderDelegatee; + this.account2 = recipient; + this.name = 'My Token'; + this.votes = this.token + this.token0 = 1; + this.token1 = 1; + this.token2 = 1; + this.token3 = 1; + }); + + shouldBehaveLikeVotes(); + }); }); diff --git a/test/token/ERC721/extensions/ERC721Votes.test.js b/test/token/ERC721/extensions/ERC721Votes.test.js index 6f001f20b..c6a9bbb32 100644 --- a/test/token/ERC721/extensions/ERC721Votes.test.js +++ b/test/token/ERC721/extensions/ERC721Votes.test.js @@ -3,9 +3,6 @@ const { BN, expectEvent, time } = require('@openzeppelin/test-helpers'); const { expect } = require('chai'); -const { promisify } = require('util'); -const queue = promisify(setImmediate); - const ERC721VotesMock = artifacts.require('ERC721VotesMock'); const { shouldBehaveLikeVotes } = require('../../../governance/utils/Votes.behavior'); @@ -23,18 +20,18 @@ contract('ERC721Votes', function (accounts) { // See https://github.com/trufflesuite/ganache-core/issues/515 this.chainId = await this.votes.getChainId(); - this.NFT0 = new BN('10000000000000000000000000'); - this.NFT1 = new BN('10'); - this.NFT2 = new BN('20'); - this.NFT3 = new BN('30'); + this.token0 = new BN('10000000000000000000000000'); + this.token1 = new BN('10'); + this.token2 = new BN('20'); + this.token3 = new BN('30'); }); describe('balanceOf', function () { beforeEach(async function () { - await this.votes.mint(account1, this.NFT0); - await this.votes.mint(account1, this.NFT1); - await this.votes.mint(account1, this.NFT2); - await this.votes.mint(account1, this.NFT3); + await this.votes.mint(account1, this.token0); + await this.votes.mint(account1, this.token1); + await this.votes.mint(account1, this.token2); + await this.votes.mint(account1, this.token3); }); it('grants to initial account', async function () { @@ -44,12 +41,12 @@ contract('ERC721Votes', function (accounts) { describe('transfers', function () { beforeEach(async function () { - await this.votes.mint(account1, this.NFT0); + await this.votes.mint(account1, this.token0); }); it('no delegation', async function () { - const { receipt } = await this.votes.transferFrom(account1, account2, this.NFT0, { from: account1 }); - expectEvent(receipt, 'Transfer', { from: account1, to: account2, tokenId: this.NFT0 }); + const { receipt } = await this.votes.transferFrom(account1, account2, this.token0, { from: account1 }); + expectEvent(receipt, 'Transfer', { from: account1, to: account2, tokenId: this.token0 }); expectEvent.notEmitted(receipt, 'DelegateVotesChanged'); this.account1Votes = '0'; @@ -59,8 +56,8 @@ contract('ERC721Votes', function (accounts) { it('sender delegation', async function () { await this.votes.delegate(account1, { from: account1 }); - const { receipt } = await this.votes.transferFrom(account1, account2, this.NFT0, { from: account1 }); - expectEvent(receipt, 'Transfer', { from: account1, to: account2, tokenId: this.NFT0 }); + const { receipt } = await this.votes.transferFrom(account1, account2, this.token0, { from: account1 }); + expectEvent(receipt, 'Transfer', { from: account1, to: account2, tokenId: this.token0 }); expectEvent(receipt, 'DelegateVotesChanged', { delegate: account1, previousBalance: '1', newBalance: '0' }); const { logIndex: transferLogIndex } = receipt.logs.find(({ event }) => event == 'Transfer'); @@ -73,8 +70,8 @@ contract('ERC721Votes', function (accounts) { it('receiver delegation', async function () { await this.votes.delegate(account2, { from: account2 }); - const { receipt } = await this.votes.transferFrom(account1, account2, this.NFT0, { from: account1 }); - expectEvent(receipt, 'Transfer', { from: account1, to: account2, tokenId: this.NFT0 }); + const { receipt } = await this.votes.transferFrom(account1, account2, this.token0, { from: account1 }); + expectEvent(receipt, 'Transfer', { from: account1, to: account2, tokenId: this.token0 }); expectEvent(receipt, 'DelegateVotesChanged', { delegate: account2, previousBalance: '0', newBalance: '1' }); const { logIndex: transferLogIndex } = receipt.logs.find(({ event }) => event == 'Transfer'); @@ -88,8 +85,8 @@ contract('ERC721Votes', function (accounts) { await this.votes.delegate(account1, { from: account1 }); await this.votes.delegate(account2, { from: account2 }); - const { receipt } = await this.votes.transferFrom(account1, account2, this.NFT0, { from: account1 }); - expectEvent(receipt, 'Transfer', { from: account1, to: account2, tokenId: this.NFT0 }); + const { receipt } = await this.votes.transferFrom(account1, account2, this.token0, { from: account1 }); + expectEvent(receipt, 'Transfer', { from: account1, to: account2, tokenId: this.token0 }); expectEvent(receipt, 'DelegateVotesChanged', { delegate: account1, previousBalance: '1', newBalance: '0'}); expectEvent(receipt, 'DelegateVotesChanged', { delegate: account2, previousBalance: '0', newBalance: '1' }); @@ -103,7 +100,7 @@ contract('ERC721Votes', function (accounts) { it('returns the same total supply on transfers', async function () { await this.votes.delegate(account1, { from: account1 }); - const { receipt } = await this.votes.transferFrom(account1, account2, this.NFT0, { from: account1 }); + const { receipt } = await this.votes.transferFrom(account1, account2, this.token0, { from: account1 }); await time.advanceBlock(); await time.advanceBlock(); @@ -116,22 +113,22 @@ contract('ERC721Votes', function (accounts) { }); it('generally returns the voting balance at the appropriate checkpoint', async function () { - await this.votes.mint(account1, this.NFT1); - await this.votes.mint(account1, this.NFT2); - await this.votes.mint(account1, this.NFT3); + await this.votes.mint(account1, this.token1); + await this.votes.mint(account1, this.token2); + await this.votes.mint(account1, this.token3); const total = await this.votes.balanceOf(account1); const t1 = await this.votes.delegate(other1, { from: account1 }); await time.advanceBlock(); await time.advanceBlock(); - const t2 = await this.votes.transferFrom(account1, other2, this.NFT0, { from: account1 }); + const t2 = await this.votes.transferFrom(account1, other2, this.token0, { from: account1 }); await time.advanceBlock(); await time.advanceBlock(); - const t3 = await this.votes.transferFrom(account1, other2, this.NFT2, { from: account1 }); + const t3 = await this.votes.transferFrom(account1, other2, this.token2, { from: account1 }); await time.advanceBlock(); await time.advanceBlock(); - const t4 = await this.votes.transferFrom(other2, account1, this.NFT2, { from: other2 }); + const t4 = await this.votes.transferFrom(other2, account1, this.token2, { from: other2 }); await time.advanceBlock(); await time.advanceBlock(); diff --git a/test/utils/Nonces.test.js b/test/utils/Nonces.test.js new file mode 100644 index 000000000..890bdb4b7 --- /dev/null +++ b/test/utils/Nonces.test.js @@ -0,0 +1,25 @@ +require('@openzeppelin/test-helpers'); + +const NoncesImpl = artifacts.require('NoncesImpl'); + +contract('Nonces', function (accounts) { + const [ sender, other ] = accounts; + + beforeEach(async function () { + this.nonces = await NoncesImpl.new(); + }); + + it('gets a nonce', async function () { + expect(await this.nonces.nonces(sender)).to.be.bignumber.equal('0'); + }); + + it('increment a nonce', async function () { + await this.nonces.useNonce(sender); + expect(await this.nonces.nonces(sender)).to.be.bignumber.equal('1'); + }); + + it('nonce is specific to address argument', async function () { + await this.nonces.useNonce(sender); + expect(await this.nonces.nonces(other)).to.be.bignumber.equal('0'); + }); +});