From 4494cc8f27dc4952b840b769a6599959a43b3e36 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 12 Mar 2021 16:10:37 +0100 Subject: [PATCH 01/42] add ERC20SnapshotEveryBlock --- .../mocks/ERC20SnapshotEveryBlockMock.sol | 25 +++ .../token/ERC20/extensions/ERC20Snapshot.sol | 119 ++------------ .../extensions/ERC20SnapshotEveryBlock.sol | 145 ++++++++++++++++++ 3 files changed, 179 insertions(+), 110 deletions(-) create mode 100644 contracts/mocks/ERC20SnapshotEveryBlockMock.sol create mode 100644 contracts/token/ERC20/extensions/ERC20SnapshotEveryBlock.sol diff --git a/contracts/mocks/ERC20SnapshotEveryBlockMock.sol b/contracts/mocks/ERC20SnapshotEveryBlockMock.sol new file mode 100644 index 00000000000..95c7b9ecaac --- /dev/null +++ b/contracts/mocks/ERC20SnapshotEveryBlockMock.sol @@ -0,0 +1,25 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.0; + +import "../token/ERC20/extensions/ERC20SnapshotEveryBlock.sol"; + + +contract ERC20SnapshotEveryBlockMock is ERC20SnapshotEveryBlock { + constructor( + string memory name, + string memory symbol, + address initialAccount, + uint256 initialBalance + ) ERC20(name, symbol) { + _mint(initialAccount, initialBalance); + } + + function mint(address account, uint256 amount) public { + _mint(account, amount); + } + + function burn(address account, uint256 amount) public { + _burn(account, amount); + } +} diff --git a/contracts/token/ERC20/extensions/ERC20Snapshot.sol b/contracts/token/ERC20/extensions/ERC20Snapshot.sol index 2246b4e3a2d..12b696fdf1d 100644 --- a/contracts/token/ERC20/extensions/ERC20Snapshot.sol +++ b/contracts/token/ERC20/extensions/ERC20Snapshot.sol @@ -2,8 +2,7 @@ pragma solidity ^0.8.0; -import "../ERC20.sol"; -import "../../../utils/Arrays.sol"; +import "./ERC20SnapshotEveryBlock.sol"; import "../../../utils/Counters.sol"; /** @@ -30,31 +29,17 @@ import "../../../utils/Counters.sol"; * only significant for the first transfer that immediately follows a snapshot for a particular account. Subsequent * transfers will have normal cost until the next snapshot, and so on. */ -abstract contract ERC20Snapshot is ERC20 { - // Inspired by Jordi Baylina's MiniMeToken to record historical balances: - // https://github.com/Giveth/minimd/blob/ea04d950eea153a04c51fa510b068b9dded390cb/contracts/MiniMeToken.sol - - using Arrays for uint256[]; +abstract contract ERC20Snapshot is ERC20SnapshotEveryBlock { using Counters for Counters.Counter; - // Snapshotted values have arrays of ids and the value corresponding to that id. These could be an array of a - // Snapshot struct, but that would impede usage of functions that work on an array. - struct Snapshots { - uint256[] ids; - uint256[] values; - } - - mapping (address => Snapshots) private _accountBalanceSnapshots; - Snapshots private _totalSupplySnapshots; - - // Snapshot ids increase monotonically, with the first value being 1. An id of 0 is invalid. - Counters.Counter private _currentSnapshotId; - /** * @dev Emitted by {_snapshot} when a snapshot identified by `id` is created. */ event Snapshot(uint256 id); + // Snapshot ids increase monotonically, with the first value being 1. An id of 0 is invalid. + Counters.Counter private _currentSnapshotId; + /** * @dev Creates a new snapshot and returns its snapshot id. * @@ -79,101 +64,15 @@ abstract contract ERC20Snapshot is ERC20 { function _snapshot() internal virtual returns (uint256) { _currentSnapshotId.increment(); - uint256 currentId = _currentSnapshotId.current(); + uint256 currentId = _getCurrentSnapshotId(); emit Snapshot(currentId); return currentId; } /** - * @dev Retrieves the balance of `account` at the time `snapshotId` was created. - */ - function balanceOfAt(address account, uint256 snapshotId) public view virtual returns (uint256) { - (bool snapshotted, uint256 value) = _valueAt(snapshotId, _accountBalanceSnapshots[account]); - - return snapshotted ? value : balanceOf(account); - } - - /** - * @dev Retrieves the total supply at the time `snapshotId` was created. + * @dev Get the current snapshotId */ - function totalSupplyAt(uint256 snapshotId) public view virtual returns(uint256) { - (bool snapshotted, uint256 value) = _valueAt(snapshotId, _totalSupplySnapshots); - - return snapshotted ? value : totalSupply(); - } - - - // Update balance and/or total supply snapshots before the values are modified. This is implemented - // in the _beforeTokenTransfer hook, which is executed for _mint, _burn, and _transfer operations. - function _beforeTokenTransfer(address from, address to, uint256 amount) internal virtual override { - super._beforeTokenTransfer(from, to, amount); - - if (from == address(0)) { - // mint - _updateAccountSnapshot(to); - _updateTotalSupplySnapshot(); - } else if (to == address(0)) { - // burn - _updateAccountSnapshot(from); - _updateTotalSupplySnapshot(); - } else { - // transfer - _updateAccountSnapshot(from); - _updateAccountSnapshot(to); - } - } - - function _valueAt(uint256 snapshotId, Snapshots storage snapshots) - private view returns (bool, uint256) - { - require(snapshotId > 0, "ERC20Snapshot: id is 0"); - // solhint-disable-next-line max-line-length - require(snapshotId <= _currentSnapshotId.current(), "ERC20Snapshot: nonexistent id"); - - // When a valid snapshot is queried, there are three possibilities: - // a) The queried value was not modified after the snapshot was taken. Therefore, a snapshot entry was never - // created for this id, and all stored snapshot ids are smaller than the requested one. The value that corresponds - // to this id is the current one. - // b) The queried value was modified after the snapshot was taken. Therefore, there will be an entry with the - // requested id, and its value is the one to return. - // c) More snapshots were created after the requested one, and the queried value was later modified. There will be - // no entry for the requested id: the value that corresponds to it is that of the smallest snapshot id that is - // larger than the requested one. - // - // In summary, we need to find an element in an array, returning the index of the smallest value that is larger if - // it is not found, unless said value doesn't exist (e.g. when all values are smaller). Arrays.findUpperBound does - // exactly this. - - uint256 index = snapshots.ids.findUpperBound(snapshotId); - - if (index == snapshots.ids.length) { - return (false, 0); - } else { - return (true, snapshots.values[index]); - } - } - - function _updateAccountSnapshot(address account) private { - _updateSnapshot(_accountBalanceSnapshots[account], balanceOf(account)); - } - - function _updateTotalSupplySnapshot() private { - _updateSnapshot(_totalSupplySnapshots, totalSupply()); - } - - function _updateSnapshot(Snapshots storage snapshots, uint256 currentValue) private { - uint256 currentId = _currentSnapshotId.current(); - if (_lastSnapshotId(snapshots.ids) < currentId) { - snapshots.ids.push(currentId); - snapshots.values.push(currentValue); - } - } - - function _lastSnapshotId(uint256[] storage ids) private view returns (uint256) { - if (ids.length == 0) { - return 0; - } else { - return ids[ids.length - 1]; - } + function _getCurrentSnapshotId() internal view virtual override returns (uint256) { + return _currentSnapshotId.current(); } } diff --git a/contracts/token/ERC20/extensions/ERC20SnapshotEveryBlock.sol b/contracts/token/ERC20/extensions/ERC20SnapshotEveryBlock.sol new file mode 100644 index 00000000000..c70146f22dd --- /dev/null +++ b/contracts/token/ERC20/extensions/ERC20SnapshotEveryBlock.sol @@ -0,0 +1,145 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.0; + +import "../ERC20.sol"; +import "../../../utils/Arrays.sol"; + +/** + * @dev This contract extends an ERC20 token with a snapshot mechanism. When a snapshot is created, the balances and + * total supply at the time are recorded for later access. + * + * This can be used to safely create mechanisms based on token balances such as trustless dividends or weighted voting. + * In naive implementations it's possible to perform a "double spend" attack by reusing the same balance from different + * accounts. By using snapshots to calculate dividends or voting power, those attacks no longer apply. It can also be + * used to create an efficient ERC20 forking mechanism. + * + * Snapshots are created by at every block. To get the total supply at a specific block, call the {totalSupplyAt} + * function with the block number. To get the balance of an account at a specific block, call the {balanceOfAt} + * function with the block number and the account address. + * + * ==== Gas Costs + * + * Snapshots are efficient. Snapshot creation is _O(1)_. Retrieval of balances or total supply from a snapshot is _O(log + * n)_ in the number of snapshots that have been created, although _n_ for a specific account will generally be much + * smaller since identical balances in subsequent snapshots are stored as a single entry. + * + * There is a constant overhead for normal ERC20 transfers due to the additional snapshot bookkeeping. This overhead is + * only significant for the first transfer that immediately follows a snapshot for a particular account. Subsequent + * transfers will have normal cost until the next snapshot, and so on. + */ +abstract contract ERC20SnapshotEveryBlock is ERC20 { + // Inspired by Jordi Baylina's MiniMeToken to record historical balances: + // https://github.com/Giveth/minimd/blob/ea04d950eea153a04c51fa510b068b9dded390cb/contracts/MiniMeToken.sol + + using Arrays for uint256[]; + + // Snapshotted values have arrays of ids and the value corresponding to that id. These could be an array of a + // Snapshot struct, but that would impede usage of functions that work on an array. + struct Snapshots { + uint256[] ids; + uint256[] values; + } + + mapping (address => Snapshots) private _accountBalanceSnapshots; + Snapshots private _totalSupplySnapshots; + + /** + * @dev Get the current snapshotId + */ + function _getCurrentSnapshotId() internal view virtual returns (uint256) { + return block.number; + } + + /** + * @dev Retrieves the balance of `account` at the time `snapshotId` was created. + */ + function balanceOfAt(address account, uint256 snapshotId) public view virtual returns (uint256) { + (bool snapshotted, uint256 value) = _valueAt(snapshotId, _accountBalanceSnapshots[account]); + + return snapshotted ? value : balanceOf(account); + } + + /** + * @dev Retrieves the total supply at the time `snapshotId` was created. + */ + function totalSupplyAt(uint256 snapshotId) public view virtual returns(uint256) { + (bool snapshotted, uint256 value) = _valueAt(snapshotId, _totalSupplySnapshots); + + return snapshotted ? value : totalSupply(); + } + + // Update balance and/or total supply snapshots before the values are modified. This is implemented + // in the _beforeTokenTransfer hook, which is executed for _mint, _burn, and _transfer operations. + function _beforeTokenTransfer(address from, address to, uint256 amount) internal virtual override { + super._beforeTokenTransfer(from, to, amount); + + if (from == address(0)) { + // mint + _updateAccountSnapshot(to); + _updateTotalSupplySnapshot(); + } else if (to == address(0)) { + // burn + _updateAccountSnapshot(from); + _updateTotalSupplySnapshot(); + } else { + // transfer + _updateAccountSnapshot(from); + _updateAccountSnapshot(to); + } + } + + function _valueAt(uint256 snapshotId, Snapshots storage snapshots) + private view returns (bool, uint256) + { + require(snapshotId > 0, "ERC20Snapshot: id is 0"); + // solhint-disable-next-line max-line-length + require(snapshotId <= _getCurrentSnapshotId(), "ERC20Snapshot: nonexistent id"); + + // When a valid snapshot is queried, there are three possibilities: + // a) The queried value was not modified after the snapshot was taken. Therefore, a snapshot entry was never + // created for this id, and all stored snapshot ids are smaller than the requested one. The value that corresponds + // to this id is the current one. + // b) The queried value was modified after the snapshot was taken. Therefore, there will be an entry with the + // requested id, and its value is the one to return. + // c) More snapshots were created after the requested one, and the queried value was later modified. There will be + // no entry for the requested id: the value that corresponds to it is that of the smallest snapshot id that is + // larger than the requested one. + // + // In summary, we need to find an element in an array, returning the index of the smallest value that is larger if + // it is not found, unless said value doesn't exist (e.g. when all values are smaller). Arrays.findUpperBound does + // exactly this. + + uint256 index = snapshots.ids.findUpperBound(snapshotId); + + if (index == snapshots.ids.length) { + return (false, 0); + } else { + return (true, snapshots.values[index]); + } + } + + function _updateAccountSnapshot(address account) private { + _updateSnapshot(_accountBalanceSnapshots[account], balanceOf(account)); + } + + function _updateTotalSupplySnapshot() private { + _updateSnapshot(_totalSupplySnapshots, totalSupply()); + } + + function _updateSnapshot(Snapshots storage snapshots, uint256 currentValue) private { + uint256 currentId = _getCurrentSnapshotId(); + if (_lastSnapshotId(snapshots.ids) < currentId) { + snapshots.ids.push(currentId); + snapshots.values.push(currentValue); + } + } + + function _lastSnapshotId(uint256[] storage ids) private view returns (uint256) { + if (ids.length == 0) { + return 0; + } else { + return ids[ids.length - 1]; + } + } +} From 53edd89547159c906fca597fc34212c6adb9a705 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 12 Mar 2021 16:49:19 +0100 Subject: [PATCH 02/42] add changelog entry --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0ff8a86584f..3c26b21ff5d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ * `IERC20Metadata`: add a new extended interface that includes the optional `name()`, `symbol()` and `decimals()` functions. ([#2561](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2561)) * `ERC777`: make reception acquirement optional in `_mint`. ([#2552](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2552)) * `ERC20Permit`: add a `_useNonce` to enable further usage of ERC712 signatures. ([#2565](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2565)) + * `ERC20SnapshotEveryBlock`: add a variation of the `ERC20Snapshot` contract that does snapshot at every block. ([#2583](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2583)) ## Unreleased From 10a5b639f6fcacd2e06086a0840f44231313daa1 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 24 Mar 2021 00:21:16 +0100 Subject: [PATCH 03/42] testing for ERC20SnapshotEveryBlock --- .../token/ERC20/extensions/ERC20Snapshot.sol | 21 +++ .../extensions/ERC20SnapshotEveryBlock.sol | 6 +- .../ERC20SnapshotEveryBlock.test.js | 136 ++++++++++++++++++ 3 files changed, 158 insertions(+), 5 deletions(-) create mode 100644 test/token/ERC20/extensions/ERC20SnapshotEveryBlock.test.js diff --git a/contracts/token/ERC20/extensions/ERC20Snapshot.sol b/contracts/token/ERC20/extensions/ERC20Snapshot.sol index 12b696fdf1d..24e7e947ab5 100644 --- a/contracts/token/ERC20/extensions/ERC20Snapshot.sol +++ b/contracts/token/ERC20/extensions/ERC20Snapshot.sol @@ -75,4 +75,25 @@ abstract contract ERC20Snapshot is ERC20SnapshotEveryBlock { function _getCurrentSnapshotId() internal view virtual override returns (uint256) { return _currentSnapshotId.current(); } + + /** + * @dev Retrieves the balance of `account` at the time `snapshotId` was created. + */ + function balanceOfAt(address account, uint256 snapshotId) public view virtual override returns (uint256) { + require(snapshotId > 0, "ERC20Snapshot: id is 0"); + // solhint-disable-next-line max-line-length + require(snapshotId <= _getCurrentSnapshotId(), "ERC20Snapshot: nonexistent id"); + return super.balanceOfAt(account, snapshotId); + } + + /** + * @dev Retrieves the total supply at the time `snapshotId` was created. + */ + function totalSupplyAt(uint256 snapshotId) public view virtual override returns(uint256) { + require(snapshotId > 0, "ERC20Snapshot: id is 0"); + // solhint-disable-next-line max-line-length + require(snapshotId <= _getCurrentSnapshotId(), "ERC20Snapshot: nonexistent id"); + return super.totalSupplyAt(snapshotId); + } + } diff --git a/contracts/token/ERC20/extensions/ERC20SnapshotEveryBlock.sol b/contracts/token/ERC20/extensions/ERC20SnapshotEveryBlock.sol index c70146f22dd..512e9db3e4e 100644 --- a/contracts/token/ERC20/extensions/ERC20SnapshotEveryBlock.sol +++ b/contracts/token/ERC20/extensions/ERC20SnapshotEveryBlock.sol @@ -48,7 +48,7 @@ abstract contract ERC20SnapshotEveryBlock is ERC20 { * @dev Get the current snapshotId */ function _getCurrentSnapshotId() internal view virtual returns (uint256) { - return block.number; + return block.number - 1; } /** @@ -92,10 +92,6 @@ abstract contract ERC20SnapshotEveryBlock is ERC20 { function _valueAt(uint256 snapshotId, Snapshots storage snapshots) private view returns (bool, uint256) { - require(snapshotId > 0, "ERC20Snapshot: id is 0"); - // solhint-disable-next-line max-line-length - require(snapshotId <= _getCurrentSnapshotId(), "ERC20Snapshot: nonexistent id"); - // When a valid snapshot is queried, there are three possibilities: // a) The queried value was not modified after the snapshot was taken. Therefore, a snapshot entry was never // created for this id, and all stored snapshot ids are smaller than the requested one. The value that corresponds diff --git a/test/token/ERC20/extensions/ERC20SnapshotEveryBlock.test.js b/test/token/ERC20/extensions/ERC20SnapshotEveryBlock.test.js new file mode 100644 index 00000000000..733e2912e2b --- /dev/null +++ b/test/token/ERC20/extensions/ERC20SnapshotEveryBlock.test.js @@ -0,0 +1,136 @@ +const { BN, time } = require('@openzeppelin/test-helpers'); +const ERC20SnapshotMock = artifacts.require('ERC20SnapshotEveryBlockMock'); + +const { expect } = require('chai'); + +function send (method, params = []) { + return new Promise(resolve => web3.currentProvider.send({ jsonrpc: '2.0', method, params }, resolve)); +} + +contract('ERC20SnapshotEveryBlock', function (accounts) { + const [ initialHolder, recipient, other ] = accounts; + + const initialSupply = new BN(100); + + const name = 'My Token'; + const symbol = 'MTKN'; + + beforeEach(async function () { + time.advanceBlock(); + this.token = await ERC20SnapshotMock.new(name, symbol, initialHolder, initialSupply); + }); + + describe('totalSupplyAt', function () { + context('with initial snapshot', function () { + context('with no supply changes after the snapshot', function () { + it('returns the current total supply', async function () { + const blockNumber = await web3.eth.getBlockNumber(); + expect(await this.token.totalSupply()).to.be.bignumber.equal(initialSupply); + expect(await this.token.totalSupplyAt(0)).to.be.bignumber.equal('0'); + expect(await this.token.totalSupplyAt(blockNumber - 1)).to.be.bignumber.equal('0'); + expect(await this.token.totalSupplyAt(blockNumber)).to.be.bignumber.equal(initialSupply); + }); + }); + + context('with supply changes', function () { + beforeEach(async function () { + this.blockNumberBefore = await web3.eth.getBlockNumber(); + await this.token.mint(other, new BN('50')); + await this.token.burn(initialHolder, new BN('20')); + this.blockNumberAfter = await web3.eth.getBlockNumber(); + }); + + it('returns the total supply before the changes', async function () { + expect(await this.token.totalSupplyAt(this.blockNumberBefore)).to.be.bignumber.equal(initialSupply); + }); + + it('snapshots return the supply after the changes', async function () { + expect(await this.token.totalSupplyAt(this.blockNumberAfter)).to.be.bignumber.equal( + await this.token.totalSupply(), + ); + }); + }); + }); + }); + + describe('balanceOfAt', function () { + context('with initial snapshot', function () { + context('with no balance changes after the snapshot', function () { + it('returns the current balance for all accounts', async function () { + const blockNumber = await web3.eth.getBlockNumber(); + expect(await this.token.balanceOf(initialHolder)).to.be.bignumber.equal(initialSupply); + expect(await this.token.balanceOfAt(initialHolder, 0)).to.be.bignumber.equal('0'); + expect(await this.token.balanceOfAt(initialHolder, blockNumber - 1)).to.be.bignumber.equal('0'); + expect(await this.token.balanceOfAt(initialHolder, blockNumber)).to.be.bignumber.equal(initialSupply); + }); + }); + + context('with balance changes', function () { + beforeEach(async function () { + this.blockNumberBefore = await web3.eth.getBlockNumber(); + await this.token.transfer(recipient, new BN('10'), { from: initialHolder }); + await this.token.mint(other, new BN('50')); + await this.token.burn(initialHolder, new BN('20')); + this.blockNumberAfter = await web3.eth.getBlockNumber(); + }); + + it('returns the balances before the changes', async function () { + expect(await this.token.balanceOfAt(initialHolder, this.blockNumberBefore)) + .to.be.bignumber.equal(initialSupply); + expect(await this.token.balanceOfAt(recipient, this.blockNumberBefore)) + .to.be.bignumber.equal('0'); + expect(await this.token.balanceOfAt(other, this.blockNumberBefore)) + .to.be.bignumber.equal('0'); + }); + + it('snapshots return the balances after the changes', async function () { + expect(await this.token.balanceOfAt(initialHolder, this.blockNumberAfter)) + .to.be.bignumber.equal(await this.token.balanceOf(initialHolder)); + expect(await this.token.balanceOfAt(recipient, this.blockNumberAfter)) + .to.be.bignumber.equal(await this.token.balanceOf(recipient)); + expect(await this.token.balanceOfAt(other, this.blockNumberAfter)) + .to.be.bignumber.equal(await this.token.balanceOf(other)); + }); + }); + + context('with multiple transfers in the same block', function () { + beforeEach(async function () { + this.blockNumberBefore = await web3.eth.getBlockNumber(); + + await send('evm_setAutomine', [false]); + const txs = Promise.all([ + this.token.transfer(recipient, new BN('10'), { from: initialHolder }), + this.token.mint(other, new BN('50')), + this.token.burn(initialHolder, new BN('20')), + ]); + + await send('evm_setIntervalMining', [1000]); + await txs; + await send('evm_setAutomine', [true]); + await send('evm_setIntervalMining', [false]); + + this.blockNumberAfter = await web3.eth.getBlockNumber(); + expect(this.blockNumberAfter).to.be.equal(this.blockNumberBefore + 1); + }); + + it('returns the balances before the changes', async function () { + expect(await this.token.balanceOfAt(initialHolder, this.blockNumberBefore)) + .to.be.bignumber.equal(initialSupply); + expect(await this.token.balanceOfAt(recipient, this.blockNumberBefore)) + .to.be.bignumber.equal('0'); + expect(await this.token.balanceOfAt(other, this.blockNumberBefore)) + .to.be.bignumber.equal('0'); + }); + + it('snapshots return the balances after the changes', async function () { + expect(await this.token.balanceOfAt(initialHolder, this.blockNumberAfter)) + .to.be.bignumber.equal(await this.token.balanceOf(initialHolder)); + expect(await this.token.balanceOfAt(recipient, this.blockNumberAfter)) + .to.be.bignumber.equal(await this.token.balanceOf(recipient)); + expect(await this.token.balanceOfAt(other, this.blockNumberAfter)) + .to.be.bignumber.equal(await this.token.balanceOf(other)); + }); + }); + }); + }); +}); From 512324ab6a948e5e9a0d4541d1635a0ae58482d0 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 24 Mar 2021 22:07:18 +0100 Subject: [PATCH 04/42] improve snapshoting --- .../token/ERC20/extensions/ERC20Snapshot.sol | 21 -- .../extensions/ERC20SnapshotEveryBlock.sol | 5 +- hardhat.config.js | 2 +- .../ERC20SnapshotEveryBlock.test.js | 269 +++++++++++------- 4 files changed, 175 insertions(+), 122 deletions(-) diff --git a/contracts/token/ERC20/extensions/ERC20Snapshot.sol b/contracts/token/ERC20/extensions/ERC20Snapshot.sol index 24e7e947ab5..12b696fdf1d 100644 --- a/contracts/token/ERC20/extensions/ERC20Snapshot.sol +++ b/contracts/token/ERC20/extensions/ERC20Snapshot.sol @@ -75,25 +75,4 @@ abstract contract ERC20Snapshot is ERC20SnapshotEveryBlock { function _getCurrentSnapshotId() internal view virtual override returns (uint256) { return _currentSnapshotId.current(); } - - /** - * @dev Retrieves the balance of `account` at the time `snapshotId` was created. - */ - function balanceOfAt(address account, uint256 snapshotId) public view virtual override returns (uint256) { - require(snapshotId > 0, "ERC20Snapshot: id is 0"); - // solhint-disable-next-line max-line-length - require(snapshotId <= _getCurrentSnapshotId(), "ERC20Snapshot: nonexistent id"); - return super.balanceOfAt(account, snapshotId); - } - - /** - * @dev Retrieves the total supply at the time `snapshotId` was created. - */ - function totalSupplyAt(uint256 snapshotId) public view virtual override returns(uint256) { - require(snapshotId > 0, "ERC20Snapshot: id is 0"); - // solhint-disable-next-line max-line-length - require(snapshotId <= _getCurrentSnapshotId(), "ERC20Snapshot: nonexistent id"); - return super.totalSupplyAt(snapshotId); - } - } diff --git a/contracts/token/ERC20/extensions/ERC20SnapshotEveryBlock.sol b/contracts/token/ERC20/extensions/ERC20SnapshotEveryBlock.sol index 512e9db3e4e..0aec337ec9e 100644 --- a/contracts/token/ERC20/extensions/ERC20SnapshotEveryBlock.sol +++ b/contracts/token/ERC20/extensions/ERC20SnapshotEveryBlock.sol @@ -48,7 +48,7 @@ abstract contract ERC20SnapshotEveryBlock is ERC20 { * @dev Get the current snapshotId */ function _getCurrentSnapshotId() internal view virtual returns (uint256) { - return block.number - 1; + return block.number; } /** @@ -92,6 +92,9 @@ abstract contract ERC20SnapshotEveryBlock is ERC20 { function _valueAt(uint256 snapshotId, Snapshots storage snapshots) private view returns (bool, uint256) { + require(snapshotId > 0, "ERC20Snapshot: id is 0"); + require(snapshotId <= _getCurrentSnapshotId(), "ERC20Snapshot: nonexistent id"); + // When a valid snapshot is queried, there are three possibilities: // a) The queried value was not modified after the snapshot was taken. Therefore, a snapshot entry was never // created for this id, and all stored snapshot ids are smaller than the requested one. The value that corresponds diff --git a/hardhat.config.js b/hardhat.config.js index fe6faf12492..f6429645db6 100644 --- a/hardhat.config.js +++ b/hardhat.config.js @@ -18,7 +18,7 @@ const enableProduction = process.env.COMPILE_MODE === 'production'; */ module.exports = { solidity: { - version: '0.8.0', + version: '0.8.3', settings: { optimizer: { enabled: enableGasReport || enableProduction, diff --git a/test/token/ERC20/extensions/ERC20SnapshotEveryBlock.test.js b/test/token/ERC20/extensions/ERC20SnapshotEveryBlock.test.js index 733e2912e2b..3c49a43523d 100644 --- a/test/token/ERC20/extensions/ERC20SnapshotEveryBlock.test.js +++ b/test/token/ERC20/extensions/ERC20SnapshotEveryBlock.test.js @@ -1,4 +1,4 @@ -const { BN, time } = require('@openzeppelin/test-helpers'); +const { BN, time, expectRevert } = require('@openzeppelin/test-helpers'); const ERC20SnapshotMock = artifacts.require('ERC20SnapshotEveryBlockMock'); const { expect } = require('chai'); @@ -7,6 +7,21 @@ function send (method, params = []) { return new Promise(resolve => web3.currentProvider.send({ jsonrpc: '2.0', method, params }, resolve)); } +async function batchInBlock (txs) { + const before = await web3.eth.getBlockNumber(); + + await send('evm_setAutomine', [false]); + const promises = Promise.all(txs.map(fn => fn())); + await send('evm_setIntervalMining', [1000]); + const receipts = await promises; + await send('evm_setAutomine', [true]); + await send('evm_setIntervalMining', [false]); + + expect(await web3.eth.getBlockNumber()).to.be.equal(before + 1); + + return receipts; +} + contract('ERC20SnapshotEveryBlock', function (accounts) { const [ initialHolder, recipient, other ] = accounts; @@ -18,118 +33,174 @@ contract('ERC20SnapshotEveryBlock', function (accounts) { beforeEach(async function () { time.advanceBlock(); this.token = await ERC20SnapshotMock.new(name, symbol, initialHolder, initialSupply); + this.initialBlock = await web3.eth.getBlockNumber(); }); describe('totalSupplyAt', function () { - context('with initial snapshot', function () { - context('with no supply changes after the snapshot', function () { - it('returns the current total supply', async function () { - const blockNumber = await web3.eth.getBlockNumber(); - expect(await this.token.totalSupply()).to.be.bignumber.equal(initialSupply); - expect(await this.token.totalSupplyAt(0)).to.be.bignumber.equal('0'); - expect(await this.token.totalSupplyAt(blockNumber - 1)).to.be.bignumber.equal('0'); - expect(await this.token.totalSupplyAt(blockNumber)).to.be.bignumber.equal(initialSupply); - }); + it('reverts with a snapshot id of 0', async function () { + await expectRevert(this.token.totalSupplyAt(0), 'ERC20Snapshot: id is 0'); + }); + + context('with no supply changes after the snapshot', function () { + it('snapshot before initial supply', async function () { + expect(await this.token.totalSupplyAt(this.initialBlock)).to.be.bignumber.equal('0'); + }); + + it('snapshot after initial supply', async function () { + await time.advanceBlock(); + expect(await this.token.totalSupplyAt(this.initialBlock + 1)).to.be.bignumber.equal(initialSupply); + }); + }); + + context('with supply changes', function () { + beforeEach(async function () { + await batchInBlock([ + () => this.token.mint(other, new BN('50')), + () => this.token.burn(initialHolder, new BN('20')), + ]); + this.operationBlockNumber = await web3.eth.getBlockNumber(); }); - context('with supply changes', function () { - beforeEach(async function () { - this.blockNumberBefore = await web3.eth.getBlockNumber(); - await this.token.mint(other, new BN('50')); - await this.token.burn(initialHolder, new BN('20')); - this.blockNumberAfter = await web3.eth.getBlockNumber(); - }); - - it('returns the total supply before the changes', async function () { - expect(await this.token.totalSupplyAt(this.blockNumberBefore)).to.be.bignumber.equal(initialSupply); - }); - - it('snapshots return the supply after the changes', async function () { - expect(await this.token.totalSupplyAt(this.blockNumberAfter)).to.be.bignumber.equal( - await this.token.totalSupply(), - ); - }); + it('returns the total supply before the changes', async function () { + expect(await this.token.totalSupplyAt(this.operationBlockNumber)) + .to.be.bignumber.equal(initialSupply); + }); + + it('snapshots return the supply after the changes', async function () { + await time.advanceBlock(); + expect(await this.token.totalSupplyAt(this.operationBlockNumber + 1)) + .to.be.bignumber.equal(initialSupply.addn(50).subn(20)); + }); + }); + + describe('with multiple operations over multiple blocks', function () { + beforeEach(async function () { + this.snapshots = [ this.initialBlock ]; + await this.token.mint(other, new BN('50')); + this.snapshots.push(await web3.eth.getBlockNumber()); + await this.token.transfer(recipient, new BN('10'), { from: initialHolder }); + this.snapshots.push(await web3.eth.getBlockNumber()); + await this.token.burn(initialHolder, new BN('20')); + this.snapshots.push(await web3.eth.getBlockNumber()); + await time.advanceBlock(); + this.snapshots.push(await web3.eth.getBlockNumber()); + }); + + it('check snapshots', async function () { + expect(await this.token.totalSupplyAt(this.snapshots[0])) + .to.be.bignumber.equal('0'); + // initial mint + expect(await this.token.totalSupplyAt(this.snapshots[1])) + .to.be.bignumber.equal(initialSupply); + // mint 50 + expect(await this.token.totalSupplyAt(this.snapshots[2])) + .to.be.bignumber.equal(initialSupply.addn(50)); + // transfer: no change + expect(await this.token.totalSupplyAt(this.snapshots[3])) + .to.be.bignumber.equal(initialSupply.addn(50)); + // burn 20 + expect(await this.token.totalSupplyAt(this.snapshots[4])) + .to.be.bignumber.equal(initialSupply.addn(50).subn(20)); }); }); }); describe('balanceOfAt', function () { - context('with initial snapshot', function () { - context('with no balance changes after the snapshot', function () { - it('returns the current balance for all accounts', async function () { - const blockNumber = await web3.eth.getBlockNumber(); - expect(await this.token.balanceOf(initialHolder)).to.be.bignumber.equal(initialSupply); - expect(await this.token.balanceOfAt(initialHolder, 0)).to.be.bignumber.equal('0'); - expect(await this.token.balanceOfAt(initialHolder, blockNumber - 1)).to.be.bignumber.equal('0'); - expect(await this.token.balanceOfAt(initialHolder, blockNumber)).to.be.bignumber.equal(initialSupply); - }); + it('reverts with a snapshot id of 0', async function () { + await expectRevert(this.token.balanceOfAt(initialHolder, 0), 'ERC20Snapshot: id is 0'); + }); + + context('with no supply changes after the snapshot', function () { + it('snapshot before initial supply', async function () { + expect(await this.token.balanceOfAt(initialHolder, this.initialBlock)) + .to.be.bignumber.equal('0'); + }); + + it('snapshot after initial supply', async function () { + await time.advanceBlock(); + expect(await this.token.balanceOfAt(initialHolder, this.initialBlock + 1)) + .to.be.bignumber.equal(initialSupply); }); + }); + + context('with balance changes', function () { + beforeEach(async function () { + await batchInBlock([ + () => this.token.transfer(recipient, new BN('10'), { from: initialHolder }), + () => this.token.mint(other, new BN('50')), + () => this.token.burn(initialHolder, new BN('20')), + ]); + this.operationBlockNumber = await web3.eth.getBlockNumber(); + }); + + it('returns the balances before the changes', async function () { + expect(await this.token.balanceOfAt(initialHolder, this.operationBlockNumber)) + .to.be.bignumber.equal(initialSupply); + expect(await this.token.balanceOfAt(recipient, this.operationBlockNumber)) + .to.be.bignumber.equal('0'); + expect(await this.token.balanceOfAt(other, this.operationBlockNumber)) + .to.be.bignumber.equal('0'); + }); + + it('snapshots return the balances after the changes', async function () { + await time.advanceBlock(); + expect(await this.token.balanceOfAt(initialHolder, this.operationBlockNumber + 1)) + .to.be.bignumber.equal(initialSupply.subn(10).subn(20)); + expect(await this.token.balanceOfAt(recipient, this.operationBlockNumber + 1)) + .to.be.bignumber.equal('10'); + expect(await this.token.balanceOfAt(other, this.operationBlockNumber + 1)) + .to.be.bignumber.equal('50'); + }); + }); - context('with balance changes', function () { - beforeEach(async function () { - this.blockNumberBefore = await web3.eth.getBlockNumber(); - await this.token.transfer(recipient, new BN('10'), { from: initialHolder }); - await this.token.mint(other, new BN('50')); - await this.token.burn(initialHolder, new BN('20')); - this.blockNumberAfter = await web3.eth.getBlockNumber(); - }); - - it('returns the balances before the changes', async function () { - expect(await this.token.balanceOfAt(initialHolder, this.blockNumberBefore)) - .to.be.bignumber.equal(initialSupply); - expect(await this.token.balanceOfAt(recipient, this.blockNumberBefore)) - .to.be.bignumber.equal('0'); - expect(await this.token.balanceOfAt(other, this.blockNumberBefore)) - .to.be.bignumber.equal('0'); - }); - - it('snapshots return the balances after the changes', async function () { - expect(await this.token.balanceOfAt(initialHolder, this.blockNumberAfter)) - .to.be.bignumber.equal(await this.token.balanceOf(initialHolder)); - expect(await this.token.balanceOfAt(recipient, this.blockNumberAfter)) - .to.be.bignumber.equal(await this.token.balanceOf(recipient)); - expect(await this.token.balanceOfAt(other, this.blockNumberAfter)) - .to.be.bignumber.equal(await this.token.balanceOf(other)); - }); + describe('with multiple operations over multiple blocks', function () { + beforeEach(async function () { + this.snapshots = [ this.initialBlock ]; + await this.token.mint(other, new BN('50')); + this.snapshots.push(await web3.eth.getBlockNumber()); + await this.token.transfer(recipient, new BN('10'), { from: initialHolder }); + this.snapshots.push(await web3.eth.getBlockNumber()); + await this.token.burn(initialHolder, new BN('20')); + this.snapshots.push(await web3.eth.getBlockNumber()); + await time.advanceBlock(); + this.snapshots.push(await web3.eth.getBlockNumber()); }); - context('with multiple transfers in the same block', function () { - beforeEach(async function () { - this.blockNumberBefore = await web3.eth.getBlockNumber(); - - await send('evm_setAutomine', [false]); - const txs = Promise.all([ - this.token.transfer(recipient, new BN('10'), { from: initialHolder }), - this.token.mint(other, new BN('50')), - this.token.burn(initialHolder, new BN('20')), - ]); - - await send('evm_setIntervalMining', [1000]); - await txs; - await send('evm_setAutomine', [true]); - await send('evm_setIntervalMining', [false]); - - this.blockNumberAfter = await web3.eth.getBlockNumber(); - expect(this.blockNumberAfter).to.be.equal(this.blockNumberBefore + 1); - }); - - it('returns the balances before the changes', async function () { - expect(await this.token.balanceOfAt(initialHolder, this.blockNumberBefore)) - .to.be.bignumber.equal(initialSupply); - expect(await this.token.balanceOfAt(recipient, this.blockNumberBefore)) - .to.be.bignumber.equal('0'); - expect(await this.token.balanceOfAt(other, this.blockNumberBefore)) - .to.be.bignumber.equal('0'); - }); - - it('snapshots return the balances after the changes', async function () { - expect(await this.token.balanceOfAt(initialHolder, this.blockNumberAfter)) - .to.be.bignumber.equal(await this.token.balanceOf(initialHolder)); - expect(await this.token.balanceOfAt(recipient, this.blockNumberAfter)) - .to.be.bignumber.equal(await this.token.balanceOf(recipient)); - expect(await this.token.balanceOfAt(other, this.blockNumberAfter)) - .to.be.bignumber.equal(await this.token.balanceOf(other)); - }); + it('check snapshots', async function () { + expect(await this.token.balanceOfAt(initialHolder, this.snapshots[0])) + .to.be.bignumber.equal('0'); + expect(await this.token.balanceOfAt(recipient, this.snapshots[0])) + .to.be.bignumber.equal('0'); + expect(await this.token.balanceOfAt(other, this.snapshots[0])) + .to.be.bignumber.equal('0'); + // initial mint + expect(await this.token.balanceOfAt(initialHolder, this.snapshots[1])) + .to.be.bignumber.equal(initialSupply); + expect(await this.token.balanceOfAt(recipient, this.snapshots[1])) + .to.be.bignumber.equal('0'); + expect(await this.token.balanceOfAt(other, this.snapshots[1])) + .to.be.bignumber.equal('0'); + // mint 50 + expect(await this.token.balanceOfAt(initialHolder, this.snapshots[2])) + .to.be.bignumber.equal(initialSupply); + expect(await this.token.balanceOfAt(recipient, this.snapshots[2])) + .to.be.bignumber.equal('0'); + expect(await this.token.balanceOfAt(other, this.snapshots[2])) + .to.be.bignumber.equal('50'); + // transfer + expect(await this.token.balanceOfAt(initialHolder, this.snapshots[3])) + .to.be.bignumber.equal(initialSupply.subn(10)); + expect(await this.token.balanceOfAt(recipient, this.snapshots[3])) + .to.be.bignumber.equal('10'); + expect(await this.token.balanceOfAt(other, this.snapshots[3])) + .to.be.bignumber.equal('50'); + // burn 20 + expect(await this.token.balanceOfAt(initialHolder, this.snapshots[4])) + .to.be.bignumber.equal(initialSupply.subn(10).subn(20)); + expect(await this.token.balanceOfAt(recipient, this.snapshots[4])) + .to.be.bignumber.equal('10'); + expect(await this.token.balanceOfAt(other, this.snapshots[4])) + .to.be.bignumber.equal('50'); }); }); }); From e87de1ebb4f3e55a78ab891670c7c2576cd63f23 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Sun, 18 Apr 2021 20:45:04 +0200 Subject: [PATCH 05/42] start work on a Comp extension to ERC20 --- contracts/mocks/ERC20CompMock.sol | 21 ++ contracts/token/ERC20/extensions/IComp.sol | 18 ++ .../ERC20/extensions/draft-ERC20Comp.sol | 101 ++++++++ .../ERC20/extensions/draft-ERC20Comp.test.js | 239 ++++++++++++++++++ 4 files changed, 379 insertions(+) create mode 100644 contracts/mocks/ERC20CompMock.sol create mode 100644 contracts/token/ERC20/extensions/IComp.sol create mode 100644 contracts/token/ERC20/extensions/draft-ERC20Comp.sol create mode 100644 test/token/ERC20/extensions/draft-ERC20Comp.test.js diff --git a/contracts/mocks/ERC20CompMock.sol b/contracts/mocks/ERC20CompMock.sol new file mode 100644 index 00000000000..309b7c66afe --- /dev/null +++ b/contracts/mocks/ERC20CompMock.sol @@ -0,0 +1,21 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.0; + + +import "../token/ERC20/extensions/draft-ERC20Comp.sol"; + +contract ERC20CompMock is ERC20Comp { + constructor ( + string memory name, + string memory symbol, + address initialAccount, + uint256 initialBalance + ) payable ERC20(name, symbol) ERC20Permit(name) { + _mint(initialAccount, initialBalance); + } + + function getChainId() external view returns (uint256) { + return block.chainid; + } +} diff --git a/contracts/token/ERC20/extensions/IComp.sol b/contracts/token/ERC20/extensions/IComp.sol new file mode 100644 index 00000000000..e028956d5bf --- /dev/null +++ b/contracts/token/ERC20/extensions/IComp.sol @@ -0,0 +1,18 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.0; + +import "../IERC20.sol"; + +interface IComp is IERC20 { + event DelegateChanged(address indexed delegator, address indexed fromDelegate, address indexed toDelegate); + event DelegateVotesChanged(address indexed delegate, uint256 previousBalance, uint256 newBalance); + + // function nonces(address owner) external view returns (uint256); + function delegates(address owner) external view returns (address); + + function delegate(address delegatee) external; + function delegateFromBySig(address delegatee, uint nonce, uint expiry, uint8 v, bytes32 r, bytes32 s) external; + function getPriorVotes(address account, uint256 blockNumber) external view returns (uint256); + function getCurrentVotes(address account) external view returns (uint256); +} diff --git a/contracts/token/ERC20/extensions/draft-ERC20Comp.sol b/contracts/token/ERC20/extensions/draft-ERC20Comp.sol new file mode 100644 index 00000000000..a7f5be313d4 --- /dev/null +++ b/contracts/token/ERC20/extensions/draft-ERC20Comp.sol @@ -0,0 +1,101 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.0; + +import "./draft-ERC20Permit.sol"; +import "./IComp.sol"; +import "../../../utils/Arrays.sol"; +import "../../../utils/cryptography/ECDSA.sol"; + +abstract contract ERC20Comp is IComp, ERC20Permit { + bytes32 private constant _DELEGATION_TYPEHASH = keccak256("Delegation(address delegatee,uint256 nonce,uint256 expiry)"); + + mapping (address => address) private _delegates; + mapping (address => uint256[]) private _checkpointBlocks; + mapping (address => mapping (uint256 => uint256)) private _checkpointWeights; + + function delegates(address account) external view override returns (address) { + return _delegates[account]; + } + + function getCurrentVotes(address account) external view override returns (uint256) { + uint256 pos = _checkpointBlocks[account].length; + return pos == 0 ? 0 : _checkpointWeights[account][pos - 1]; + } + + function getPriorVotes(address account, uint blockNumber) external view override returns (uint256) { + require(blockNumber < block.number, "ERC20Comp::getPriorVotes: not yet determined"); + uint256 pos = Arrays.findUpperBound(_checkpointBlocks[account], blockNumber); + return pos == 0 ? 0 : _checkpointWeights[account][pos - 1]; + } + + function delegate(address delegatee) public virtual override { + return _delegate(_msgSender(), delegatee); + } + + function delegateFromBySig(address delegatee, uint nonce, uint expiry, uint8 v, bytes32 r, bytes32 s) + public virtual override + { + require(block.timestamp <= expiry, "ERC20Comp::delegateBySig: signature expired"); + address signatory = ECDSA.recover( + _hashTypedDataV4(keccak256(abi.encode( + _DELEGATION_TYPEHASH, + delegatee, + nonce, + expiry + ))), + v, r, s + ); + require(nonce == _useNonce(signatory), "ERC20Comp::delegateBySig: invalid nonce"); + return _delegate(signatory, delegatee); + } + + function _delegate(address delegator, address delegatee) internal { + address currentDelegate = _delegates[delegator] == address(0) ? delegator : _delegates[delegator]; + uint256 delegatorBalance = balanceOf(delegator); + _delegates[delegator] = delegatee; + + emit DelegateChanged(delegator, currentDelegate, delegatee); + + _moveDelegates(currentDelegate, delegatee, delegatorBalance); + } + + function _moveDelegates(address srcRep, address dstRep, uint256 amount) internal { + if (srcRep != dstRep && amount > 0) { + if (srcRep != address(0)) { + uint256 srcRepNum = _checkpointBlocks[srcRep].length; + uint256 srcRepOld = srcRepNum == 0 ? 0 : _checkpointWeights[srcRep][srcRepNum - 1]; + uint256 srcRepNew = srcRepOld - amount; + _writeCheckpoint(srcRep, srcRepNum, srcRepOld, srcRepNew); + } + + if (dstRep != address(0)) { + uint256 dstRepNum = _checkpointBlocks[dstRep].length; + uint256 dstRepOld = dstRepNum == 0 ? 0 : _checkpointWeights[dstRep][dstRepNum - 1]; + uint256 dstRepNew = dstRepOld + amount; + _writeCheckpoint(dstRep, dstRepNum, dstRepOld, dstRepNew); + } + } + } + + function _writeCheckpoint(address delegatee, uint256 pos, uint256 oldWeight, uint256 newWeight) internal { + if (pos > 0 && _checkpointBlocks[delegatee][pos - 1] == block.number) { + _checkpointWeights[delegatee][pos - 1] = newWeight; + } else { + _checkpointBlocks[delegatee].push(block.number); + _checkpointWeights[delegatee][pos] = newWeight; + } + + emit DelegateVotesChanged(delegatee, oldWeight, newWeight); + } + + function _beforeTokenTransfer(address from, address to, uint256 amount) internal virtual override { + address fromDelegate = _delegates[from]; + address toDelegate = _delegates[to]; + _moveDelegates( + fromDelegate == address(0) ? from : fromDelegate, + toDelegate == address(0) ? to : toDelegate, + amount + ); + } +} diff --git a/test/token/ERC20/extensions/draft-ERC20Comp.test.js b/test/token/ERC20/extensions/draft-ERC20Comp.test.js new file mode 100644 index 00000000000..9a2aa5272f2 --- /dev/null +++ b/test/token/ERC20/extensions/draft-ERC20Comp.test.js @@ -0,0 +1,239 @@ +/* eslint-disable */ + +const { BN, constants, expectEvent, expectRevert, time } = require('@openzeppelin/test-helpers'); +const { expect } = require('chai'); +const { MAX_UINT256, ZERO_ADDRESS, ZERO_BYTES32 } = constants; + +const { fromRpcSig } = require('ethereumjs-util'); +const ethSigUtil = require('eth-sig-util'); +const Wallet = require('ethereumjs-wallet').default; + +const ERC20CompMock = artifacts.require('ERC20CompMock'); + +const { EIP712Domain, domainSeparator } = require('../../../helpers/eip712'); + +const Delegation = [ + { name: 'delegatee', type: 'address' }, + { name: 'nonce', type: 'uint256' }, + { name: 'expiry', type: 'uint256' }, +]; + +contract('ERC20Comp', function (accounts) { + const [ holder, recipient, holderDelegatee, recipientDelegatee ] = accounts; + + const name = 'My Token'; + const symbol = 'MTKN'; + const version = '1'; + + const supply = new BN(100); + + beforeEach(async function () { + this.token = await ERC20CompMock.new(name, symbol, holder, supply); + + // We get the chain id from the contract because Ganache (used for coverage) does not return the same chain id + // from within the EVM as from the JSON RPC interface. + // See https://github.com/trufflesuite/ganache-core/issues/515 + this.chainId = await this.token.getChainId(); + }); + + it('initial nonce is 0', async function () { + expect(await this.token.nonces(holder)).to.be.bignumber.equal('0'); + }); + + it('domain separator', async function () { + expect( + await this.token.DOMAIN_SEPARATOR(), + ).to.equal( + await domainSeparator(name, version, this.chainId, this.token.address), + ); + }); + + describe('delegate', function () { + it('check voting', async function () { + const blockNumber = await web3.eth.getBlockNumber(); + + expect(await this.token.getCurrentVotes(holder)).to.be.bignumber.equal(supply); + expect(await this.token.getPriorVotes(holder, blockNumber - 1)).to.be.bignumber.equal('0'); + await expectRevert(this.token.getPriorVotes(holder, blockNumber), 'ERC20Comp::getPriorVotes: not yet determined'); + + await time.advanceBlock(); + expect(await this.token.getPriorVotes(holder, blockNumber)).to.be.bignumber.equal('0'); + + await time.advanceBlock(); + expect(await this.token.getPriorVotes(holder, blockNumber + 1)).to.be.bignumber.equal(supply); + }); + + it('delegation with balance', async function () { + const { receipt } = await this.token.delegate(holderDelegatee, { from: holder }); + expectEvent(receipt, 'DelegateChanged', { + delegator: holder, + fromDelegate: holder, + toDelegate: holderDelegatee, + }); + expectEvent(receipt, 'DelegateVotesChanged', { + delegate: holder, + previousBalance: supply, + newBalance: '0', + }); + expectEvent(receipt, 'DelegateVotesChanged', { + delegate: holderDelegatee, + previousBalance: '0', + newBalance: supply, + }); + + expect(await this.token.getCurrentVotes(holder)).to.be.bignumber.equal('0'); + expect(await this.token.getCurrentVotes(holderDelegatee)).to.be.bignumber.equal(supply); + + await time.advanceBlock(); + expect(await this.token.getPriorVotes(holder, receipt.blockNumber)).to.be.bignumber.equal(supply); + expect(await this.token.getPriorVotes(holderDelegatee, receipt.blockNumber)).to.be.bignumber.equal('0'); + + await time.advanceBlock(); + expect(await this.token.getPriorVotes(holder, receipt.blockNumber + 1)).to.be.bignumber.equal('0'); + expect(await this.token.getPriorVotes(holderDelegatee, receipt.blockNumber + 1)).to.be.bignumber.equal(supply); + }); + + it('delegation without balance', async function () { + const { receipt } = await this.token.delegate(recipientDelegatee, { from: recipient }); + expectEvent(receipt, 'DelegateChanged', { + delegator: recipient, + fromDelegate: recipient, + toDelegate: recipientDelegatee, + }); + expectEvent.notEmitted(receipt, 'DelegateVotesChanged'); + }); + }); + + describe('delegateFromBySig', function () { + const delegator = Wallet.generate(); + const delegatorAddress = web3.utils.toChecksumAddress(delegator.getAddressString()); + const nonce = 0; + const maxExpiry = MAX_UINT256; + + const buildData = (chainId, verifyingContract, message) => ({ data: { + primaryType: 'Delegation', + types: { EIP712Domain, Delegation }, + domain: { name, version, chainId, verifyingContract }, + message, + }}); + + it('accept signed delegation', async function () { + const { v, r, s } = fromRpcSig(ethSigUtil.signTypedMessage( + delegator.getPrivateKey(), + buildData(this.chainId, this.token.address, { + delegatee: holderDelegatee, + nonce, + expiry: maxExpiry, + }), + )); + + await this.token.transfer(delegatorAddress, supply, { from: holder }); + const { receipt } = await this.token.delegateFromBySig(holderDelegatee, nonce, maxExpiry, v, r, s); + expectEvent(receipt, 'DelegateChanged', { + delegator: delegatorAddress, + fromDelegate: delegatorAddress, + toDelegate: holderDelegatee, + }); + expectEvent(receipt, 'DelegateVotesChanged', { + delegate: delegatorAddress, + previousBalance: supply, + newBalance: '0', + }); + expectEvent(receipt, 'DelegateVotesChanged', { + delegate: holderDelegatee, + previousBalance: '0', + newBalance: supply, + }); + + expect(await this.token.getCurrentVotes(delegatorAddress)).to.be.bignumber.equal('0'); + expect(await this.token.getCurrentVotes(holderDelegatee)).to.be.bignumber.equal(supply); + + await time.advanceBlock(); + expect(await this.token.getPriorVotes(delegatorAddress, receipt.blockNumber)).to.be.bignumber.equal(supply); + expect(await this.token.getPriorVotes(holderDelegatee, receipt.blockNumber)).to.be.bignumber.equal('0'); + + await time.advanceBlock(); + expect(await this.token.getPriorVotes(delegatorAddress, receipt.blockNumber + 1)).to.be.bignumber.equal('0'); + expect(await this.token.getPriorVotes(holderDelegatee, receipt.blockNumber + 1)).to.be.bignumber.equal(supply); + }); + + it('rejects reused signature', async function () { + const { v, r, s } = fromRpcSig(ethSigUtil.signTypedMessage( + delegator.getPrivateKey(), + buildData(this.chainId, this.token.address, { + delegatee: holderDelegatee, + nonce, + expiry: maxExpiry, + }), + )); + + await this.token.delegateFromBySig(holderDelegatee, nonce, maxExpiry, v, r, s); + + await expectRevert( + this.token.delegateFromBySig(holderDelegatee, nonce, maxExpiry, v, r, s), + 'ERC20Comp::delegateBySig: invalid nonce', + ); + }); + + it('rejects expired permit', async function () { + const expiry = (await time.latest()) - time.duration.weeks(1); + const { v, r, s } = fromRpcSig(ethSigUtil.signTypedMessage( + delegator.getPrivateKey(), + buildData(this.chainId, this.token.address, { + delegatee: holderDelegatee, + nonce, + expiry, + }), + )); + + await expectRevert( + this.token.delegateFromBySig(holderDelegatee, nonce, expiry, v, r, s), + 'ERC20Comp::delegateBySig: signature expired', + ); + }); + }); + + describe('transfers', function () { + beforeEach(async function () { + this.fromDelegatee = holder; + this.toDelegatee = recipient; + }); + + it('no delegation', async function () { + }); + + it('sender has delegatee', async function () { + this.fromDelegatee = holderDelegatee; + await this.token.delegate(holderDelegatee, { from: holder }); + }); + + it('recipient has delegatee', async function () { + this.toDelegatee = recipientDelegatee; + await this.token.delegate(recipientDelegatee, { from: recipient }); + }); + + it('sender and recipient have delegatee', async function () { + this.fromDelegatee = holderDelegatee; + this.toDelegatee = recipientDelegatee; + await this.token.delegate(holderDelegatee, { from: holder }); + await this.token.delegate(recipientDelegatee, { from: recipient }); + }); + + afterEach(async function () { + const { receipt } = await this.token.transfer(recipient, 1, { from: holder }); + expectEvent(receipt, 'Transfer', { from: holder, to: recipient, value: '1' }); + expectEvent(receipt, 'DelegateVotesChanged', { delegate: this.fromDelegatee, previousBalance: supply, newBalance: supply.subn(1) }); + expectEvent(receipt, 'DelegateVotesChanged', { delegate: this.toDelegatee, previousBalance: '0', newBalance: '1' }); + + expect(await this.token.getCurrentVotes(this.fromDelegatee)).to.be.bignumber.equal(supply.subn(1)); + expect(await this.token.getCurrentVotes(this.toDelegatee)).to.be.bignumber.equal('1'); + + // need to advance 2 blocks to see the effect of a transfer on "getPriorVotes" + time.advanceBlock(); + const blockNumber = await web3.eth.getBlockNumber(); + time.advanceBlock(); + expect(await this.token.getPriorVotes(this.fromDelegatee, blockNumber)).to.be.bignumber.equal(supply.subn(1)); + expect(await this.token.getPriorVotes(this.toDelegatee, blockNumber)).to.be.bignumber.equal('1'); + }); + }); +}); From a235d0ce96a9996e28a5aa6000c83c34f141ef02 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Sun, 18 Apr 2021 20:49:59 +0200 Subject: [PATCH 06/42] delegation query cleanup --- .../token/ERC20/extensions/draft-ERC20Comp.sol | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/contracts/token/ERC20/extensions/draft-ERC20Comp.sol b/contracts/token/ERC20/extensions/draft-ERC20Comp.sol index a7f5be313d4..0b38e3fe96d 100644 --- a/contracts/token/ERC20/extensions/draft-ERC20Comp.sol +++ b/contracts/token/ERC20/extensions/draft-ERC20Comp.sol @@ -14,8 +14,9 @@ abstract contract ERC20Comp is IComp, ERC20Permit { mapping (address => uint256[]) private _checkpointBlocks; mapping (address => mapping (uint256 => uint256)) private _checkpointWeights; - function delegates(address account) external view override returns (address) { - return _delegates[account]; + function delegates(address account) public view override returns (address) { + address delegatee = _delegates[account]; + return delegatee == address(0) ? account : delegatee; } function getCurrentVotes(address account) external view override returns (uint256) { @@ -51,7 +52,7 @@ abstract contract ERC20Comp is IComp, ERC20Permit { } function _delegate(address delegator, address delegatee) internal { - address currentDelegate = _delegates[delegator] == address(0) ? delegator : _delegates[delegator]; + address currentDelegate = delegates(delegator); uint256 delegatorBalance = balanceOf(delegator); _delegates[delegator] = delegatee; @@ -90,12 +91,6 @@ abstract contract ERC20Comp is IComp, ERC20Permit { } function _beforeTokenTransfer(address from, address to, uint256 amount) internal virtual override { - address fromDelegate = _delegates[from]; - address toDelegate = _delegates[to]; - _moveDelegates( - fromDelegate == address(0) ? from : fromDelegate, - toDelegate == address(0) ? to : toDelegate, - amount - ); + _moveDelegates(delegates(from), delegates(to), amount); } } From 4c17b9a63e79d9c113ae1e84917298e25e6e4661 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Mon, 19 Apr 2021 11:25:57 +0200 Subject: [PATCH 07/42] minor cleanup of comp test --- test/token/ERC20/extensions/draft-ERC20Comp.test.js | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/test/token/ERC20/extensions/draft-ERC20Comp.test.js b/test/token/ERC20/extensions/draft-ERC20Comp.test.js index 9a2aa5272f2..15fb8779562 100644 --- a/test/token/ERC20/extensions/draft-ERC20Comp.test.js +++ b/test/token/ERC20/extensions/draft-ERC20Comp.test.js @@ -108,7 +108,6 @@ contract('ERC20Comp', function (accounts) { const delegator = Wallet.generate(); const delegatorAddress = web3.utils.toChecksumAddress(delegator.getAddressString()); const nonce = 0; - const maxExpiry = MAX_UINT256; const buildData = (chainId, verifyingContract, message) => ({ data: { primaryType: 'Delegation', @@ -123,12 +122,12 @@ contract('ERC20Comp', function (accounts) { buildData(this.chainId, this.token.address, { delegatee: holderDelegatee, nonce, - expiry: maxExpiry, + expiry: MAX_UINT256, }), )); await this.token.transfer(delegatorAddress, supply, { from: holder }); - const { receipt } = await this.token.delegateFromBySig(holderDelegatee, nonce, maxExpiry, v, r, s); + const { receipt } = await this.token.delegateFromBySig(holderDelegatee, nonce, MAX_UINT256, v, r, s); expectEvent(receipt, 'DelegateChanged', { delegator: delegatorAddress, fromDelegate: delegatorAddress, @@ -163,14 +162,14 @@ contract('ERC20Comp', function (accounts) { buildData(this.chainId, this.token.address, { delegatee: holderDelegatee, nonce, - expiry: maxExpiry, + expiry: MAX_UINT256, }), )); - await this.token.delegateFromBySig(holderDelegatee, nonce, maxExpiry, v, r, s); + await this.token.delegateFromBySig(holderDelegatee, nonce, MAX_UINT256, v, r, s); await expectRevert( - this.token.delegateFromBySig(holderDelegatee, nonce, maxExpiry, v, r, s), + this.token.delegateFromBySig(holderDelegatee, nonce, MAX_UINT256, v, r, s), 'ERC20Comp::delegateBySig: invalid nonce', ); }); From 46aa3cc970707b6311cb6928b453ccf68c292477 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 20 Apr 2021 15:56:18 +0200 Subject: [PATCH 08/42] remove autodelegation --- .../ERC20/extensions/draft-ERC20Comp.sol | 20 +++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/contracts/token/ERC20/extensions/draft-ERC20Comp.sol b/contracts/token/ERC20/extensions/draft-ERC20Comp.sol index 0b38e3fe96d..b851454da50 100644 --- a/contracts/token/ERC20/extensions/draft-ERC20Comp.sol +++ b/contracts/token/ERC20/extensions/draft-ERC20Comp.sol @@ -14,10 +14,14 @@ abstract contract ERC20Comp is IComp, ERC20Permit { mapping (address => uint256[]) private _checkpointBlocks; mapping (address => mapping (uint256 => uint256)) private _checkpointWeights; - function delegates(address account) public view override returns (address) { - address delegatee = _delegates[account]; - return delegatee == address(0) ? account : delegatee; + function delegates(address account) public view virtual override returns (address) { + return _delegates[account]; } + // autodelegation, more expensive + // function delegates(address account) public view override returns (address) { + // address delegatee = _delegates[account]; + // return delegatee == address(0) ? account : delegatee; + // } function getCurrentVotes(address account) external view override returns (uint256) { uint256 pos = _checkpointBlocks[account].length; @@ -51,7 +55,7 @@ abstract contract ERC20Comp is IComp, ERC20Permit { return _delegate(signatory, delegatee); } - function _delegate(address delegator, address delegatee) internal { + function _delegate(address delegator, address delegatee) internal virtual { address currentDelegate = delegates(delegator); uint256 delegatorBalance = balanceOf(delegator); _delegates[delegator] = delegatee; @@ -61,25 +65,25 @@ abstract contract ERC20Comp is IComp, ERC20Permit { _moveDelegates(currentDelegate, delegatee, delegatorBalance); } - function _moveDelegates(address srcRep, address dstRep, uint256 amount) internal { + function _moveDelegates(address srcRep, address dstRep, uint256 amount) private { if (srcRep != dstRep && amount > 0) { if (srcRep != address(0)) { uint256 srcRepNum = _checkpointBlocks[srcRep].length; - uint256 srcRepOld = srcRepNum == 0 ? 0 : _checkpointWeights[srcRep][srcRepNum - 1]; + uint256 srcRepOld = srcRepNum > 0 ? _checkpointWeights[srcRep][srcRepNum - 1] : 0; uint256 srcRepNew = srcRepOld - amount; _writeCheckpoint(srcRep, srcRepNum, srcRepOld, srcRepNew); } if (dstRep != address(0)) { uint256 dstRepNum = _checkpointBlocks[dstRep].length; - uint256 dstRepOld = dstRepNum == 0 ? 0 : _checkpointWeights[dstRep][dstRepNum - 1]; + uint256 dstRepOld = dstRepNum > 0 ? _checkpointWeights[dstRep][dstRepNum - 1] : 0; uint256 dstRepNew = dstRepOld + amount; _writeCheckpoint(dstRep, dstRepNum, dstRepOld, dstRepNew); } } } - function _writeCheckpoint(address delegatee, uint256 pos, uint256 oldWeight, uint256 newWeight) internal { + function _writeCheckpoint(address delegatee, uint256 pos, uint256 oldWeight, uint256 newWeight) private { if (pos > 0 && _checkpointBlocks[delegatee][pos - 1] == block.number) { _checkpointWeights[delegatee][pos - 1] = newWeight; } else { From fabd802d309eeb543dad7918c6d4700bce680d29 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 20 Apr 2021 17:32:59 +0200 Subject: [PATCH 09/42] fix/improve testing for ERC20Comp --- .../ERC20/extensions/draft-ERC20Comp.sol | 7 +- .../ERC20/extensions/draft-ERC20Comp.test.js | 331 +++++++++++------- 2 files changed, 205 insertions(+), 133 deletions(-) diff --git a/contracts/token/ERC20/extensions/draft-ERC20Comp.sol b/contracts/token/ERC20/extensions/draft-ERC20Comp.sol index b851454da50..2342261b1a5 100644 --- a/contracts/token/ERC20/extensions/draft-ERC20Comp.sol +++ b/contracts/token/ERC20/extensions/draft-ERC20Comp.sol @@ -17,7 +17,12 @@ abstract contract ERC20Comp is IComp, ERC20Permit { function delegates(address account) public view virtual override returns (address) { return _delegates[account]; } - // autodelegation, more expensive + + /** + * Example: This enables autodelegation, makes each transfer more expensive but doesn't require user to delegate to + * themselves. Can be usefull for tokens useds exclusivelly for governance, such as voting wrappers of pre-existing + * ERC20. + */ // function delegates(address account) public view override returns (address) { // address delegatee = _delegates[account]; // return delegatee == address(0) ? account : delegatee; diff --git a/test/token/ERC20/extensions/draft-ERC20Comp.test.js b/test/token/ERC20/extensions/draft-ERC20Comp.test.js index 15fb8779562..45927b84e10 100644 --- a/test/token/ERC20/extensions/draft-ERC20Comp.test.js +++ b/test/token/ERC20/extensions/draft-ERC20Comp.test.js @@ -48,93 +48,181 @@ contract('ERC20Comp', function (accounts) { ); }); - describe('delegate', function () { - it('check voting', async function () { - const blockNumber = await web3.eth.getBlockNumber(); + describe('set delegation', function () { + describe('call', function () { + it('delegation with balance', async function () { + expect(await this.token.delegates(holder)).to.be.equal(ZERO_ADDRESS); + + const { receipt } = await this.token.delegate(holder, { from: holder }); + expectEvent(receipt, 'DelegateChanged', { + delegator: holder, + fromDelegate: ZERO_ADDRESS, + toDelegate: holder, + }); + expectEvent(receipt, 'DelegateVotesChanged', { + delegate: holder, + previousBalance: '0', + newBalance: supply, + }); + + expect(await this.token.delegates(holder)).to.be.equal(holder); + + expect(await this.token.getCurrentVotes(holder)).to.be.bignumber.equal(supply); + await time.advanceBlock(); + expect(await this.token.getPriorVotes(holder, receipt.blockNumber)).to.be.bignumber.equal('0'); + await time.advanceBlock(); + expect(await this.token.getPriorVotes(holder, receipt.blockNumber + 1)).to.be.bignumber.equal(supply); + }); - expect(await this.token.getCurrentVotes(holder)).to.be.bignumber.equal(supply); - expect(await this.token.getPriorVotes(holder, blockNumber - 1)).to.be.bignumber.equal('0'); - await expectRevert(this.token.getPriorVotes(holder, blockNumber), 'ERC20Comp::getPriorVotes: not yet determined'); + it('delegation without balance', async function () { + expect(await this.token.delegates(recipient)).to.be.equal(ZERO_ADDRESS); - await time.advanceBlock(); - expect(await this.token.getPriorVotes(holder, blockNumber)).to.be.bignumber.equal('0'); + const { receipt } = await this.token.delegate(recipient, { from: recipient }); + expectEvent(receipt, 'DelegateChanged', { + delegator: recipient, + fromDelegate: ZERO_ADDRESS, + toDelegate: recipient, + }); + expectEvent.notEmitted(receipt, 'DelegateVotesChanged'); - await time.advanceBlock(); - expect(await this.token.getPriorVotes(holder, blockNumber + 1)).to.be.bignumber.equal(supply); + expect(await this.token.delegates(recipient)).to.be.equal(recipient); + }); }); - it('delegation with balance', async function () { - const { receipt } = await this.token.delegate(holderDelegatee, { from: holder }); - expectEvent(receipt, 'DelegateChanged', { - delegator: holder, - fromDelegate: holder, - toDelegate: holderDelegatee, - }); - expectEvent(receipt, 'DelegateVotesChanged', { - delegate: holder, - previousBalance: supply, - newBalance: '0', + describe('with signature', function () { + const delegator = Wallet.generate(); + const delegatorAddress = web3.utils.toChecksumAddress(delegator.getAddressString()); + const nonce = 0; + + const buildData = (chainId, verifyingContract, message) => ({ data: { + primaryType: 'Delegation', + types: { EIP712Domain, Delegation }, + domain: { name, version, chainId, verifyingContract }, + message, + }}); + + beforeEach(async function () { + await this.token.transfer(delegatorAddress, supply, { from: holder }); }); - expectEvent(receipt, 'DelegateVotesChanged', { - delegate: holderDelegatee, - previousBalance: '0', - newBalance: supply, + + it('accept signed delegation', async function () { + const { v, r, s } = fromRpcSig(ethSigUtil.signTypedMessage( + delegator.getPrivateKey(), + buildData(this.chainId, this.token.address, { + delegatee: delegatorAddress, + nonce, + expiry: MAX_UINT256, + }), + )); + + expect(await this.token.delegates(delegatorAddress)).to.be.equal(ZERO_ADDRESS); + + const { receipt } = await this.token.delegateFromBySig(delegatorAddress, nonce, MAX_UINT256, v, r, s); + expectEvent(receipt, 'DelegateChanged', { + delegator: delegatorAddress, + fromDelegate: ZERO_ADDRESS, + toDelegate: delegatorAddress, + }); + expectEvent(receipt, 'DelegateVotesChanged', { + delegate: delegatorAddress, + previousBalance: '0', + newBalance: supply, + }); + + expect(await this.token.delegates(delegatorAddress)).to.be.equal(delegatorAddress); + + expect(await this.token.getCurrentVotes(delegatorAddress)).to.be.bignumber.equal(supply); + await time.advanceBlock(); + expect(await this.token.getPriorVotes(delegatorAddress, receipt.blockNumber)).to.be.bignumber.equal('0'); + await time.advanceBlock(); + expect(await this.token.getPriorVotes(delegatorAddress, receipt.blockNumber + 1)).to.be.bignumber.equal(supply); }); - expect(await this.token.getCurrentVotes(holder)).to.be.bignumber.equal('0'); - expect(await this.token.getCurrentVotes(holderDelegatee)).to.be.bignumber.equal(supply); + it('rejects reused signature', async function () { + const { v, r, s } = fromRpcSig(ethSigUtil.signTypedMessage( + delegator.getPrivateKey(), + buildData(this.chainId, this.token.address, { + delegatee: delegatorAddress, + nonce, + expiry: MAX_UINT256, + }), + )); + + await this.token.delegateFromBySig(delegatorAddress, nonce, MAX_UINT256, v, r, s); + + await expectRevert( + this.token.delegateFromBySig(delegatorAddress, nonce, MAX_UINT256, v, r, s), + 'ERC20Comp::delegateBySig: invalid nonce', + ); + }); - await time.advanceBlock(); - expect(await this.token.getPriorVotes(holder, receipt.blockNumber)).to.be.bignumber.equal(supply); - expect(await this.token.getPriorVotes(holderDelegatee, receipt.blockNumber)).to.be.bignumber.equal('0'); + it('rejects bad delegatee', async function () { + const { v, r, s } = fromRpcSig(ethSigUtil.signTypedMessage( + delegator.getPrivateKey(), + buildData(this.chainId, this.token.address, { + delegatee: delegatorAddress, + nonce, + expiry: MAX_UINT256, + }), + )); + + const { logs } = await this.token.delegateFromBySig(holderDelegatee, nonce, MAX_UINT256, v, r, s); + const { args } = logs.find(({ event }) => event == 'DelegateChanged'); + expect(args.delegator).to.not.be.equal(delegatorAddress); + expect(args.fromDelegate).to.be.equal(ZERO_ADDRESS); + expect(args.toDelegate).to.be.equal(holderDelegatee); + }); - await time.advanceBlock(); - expect(await this.token.getPriorVotes(holder, receipt.blockNumber + 1)).to.be.bignumber.equal('0'); - expect(await this.token.getPriorVotes(holderDelegatee, receipt.blockNumber + 1)).to.be.bignumber.equal(supply); - }); + it('rejects bad nonce', async function () { + const { v, r, s } = fromRpcSig(ethSigUtil.signTypedMessage( + delegator.getPrivateKey(), + buildData(this.chainId, this.token.address, { + delegatee: delegatorAddress, + nonce, + expiry: MAX_UINT256, + }), + )); + await expectRevert( + this.token.delegateFromBySig(delegatorAddress, nonce + 1, MAX_UINT256, v, r, s), + 'ERC20Comp::delegateBySig: invalid nonce', + ); + }); - it('delegation without balance', async function () { - const { receipt } = await this.token.delegate(recipientDelegatee, { from: recipient }); - expectEvent(receipt, 'DelegateChanged', { - delegator: recipient, - fromDelegate: recipient, - toDelegate: recipientDelegatee, + it('rejects expired permit', async function () { + const expiry = (await time.latest()) - time.duration.weeks(1); + const { v, r, s } = fromRpcSig(ethSigUtil.signTypedMessage( + delegator.getPrivateKey(), + buildData(this.chainId, this.token.address, { + delegatee: delegatorAddress, + nonce, + expiry, + }), + )); + + await expectRevert( + this.token.delegateFromBySig(delegatorAddress, nonce, expiry, v, r, s), + 'ERC20Comp::delegateBySig: signature expired', + ); }); - expectEvent.notEmitted(receipt, 'DelegateVotesChanged'); }); }); - describe('delegateFromBySig', function () { - const delegator = Wallet.generate(); - const delegatorAddress = web3.utils.toChecksumAddress(delegator.getAddressString()); - const nonce = 0; - - const buildData = (chainId, verifyingContract, message) => ({ data: { - primaryType: 'Delegation', - types: { EIP712Domain, Delegation }, - domain: { name, version, chainId, verifyingContract }, - message, - }}); - - it('accept signed delegation', async function () { - const { v, r, s } = fromRpcSig(ethSigUtil.signTypedMessage( - delegator.getPrivateKey(), - buildData(this.chainId, this.token.address, { - delegatee: holderDelegatee, - nonce, - expiry: MAX_UINT256, - }), - )); - - await this.token.transfer(delegatorAddress, supply, { from: holder }); - const { receipt } = await this.token.delegateFromBySig(holderDelegatee, nonce, MAX_UINT256, v, r, s); + describe('change delegation', function () { + beforeEach(async function () { + await this.token.delegate(holder, { from: holder }); + }); + + it('call', async function () { + expect(await this.token.delegates(holder)).to.be.equal(holder); + + const { receipt } = await this.token.delegate(holderDelegatee, { from: holder }); expectEvent(receipt, 'DelegateChanged', { - delegator: delegatorAddress, - fromDelegate: delegatorAddress, + delegator: holder, + fromDelegate: holder, toDelegate: holderDelegatee, }); expectEvent(receipt, 'DelegateVotesChanged', { - delegate: delegatorAddress, + delegate: holder, previousBalance: supply, newBalance: '0', }); @@ -144,95 +232,74 @@ contract('ERC20Comp', function (accounts) { newBalance: supply, }); - expect(await this.token.getCurrentVotes(delegatorAddress)).to.be.bignumber.equal('0'); - expect(await this.token.getCurrentVotes(holderDelegatee)).to.be.bignumber.equal(supply); + expect(await this.token.delegates(holder)).to.be.equal(holderDelegatee); + expect(await this.token.getCurrentVotes(holder)).to.be.bignumber.equal('0'); + expect(await this.token.getCurrentVotes(holderDelegatee)).to.be.bignumber.equal(supply); await time.advanceBlock(); - expect(await this.token.getPriorVotes(delegatorAddress, receipt.blockNumber)).to.be.bignumber.equal(supply); + expect(await this.token.getPriorVotes(holder, receipt.blockNumber)).to.be.bignumber.equal(supply); expect(await this.token.getPriorVotes(holderDelegatee, receipt.blockNumber)).to.be.bignumber.equal('0'); - await time.advanceBlock(); - expect(await this.token.getPriorVotes(delegatorAddress, receipt.blockNumber + 1)).to.be.bignumber.equal('0'); + expect(await this.token.getPriorVotes(holder, receipt.blockNumber + 1)).to.be.bignumber.equal('0'); expect(await this.token.getPriorVotes(holderDelegatee, receipt.blockNumber + 1)).to.be.bignumber.equal(supply); }); - - it('rejects reused signature', async function () { - const { v, r, s } = fromRpcSig(ethSigUtil.signTypedMessage( - delegator.getPrivateKey(), - buildData(this.chainId, this.token.address, { - delegatee: holderDelegatee, - nonce, - expiry: MAX_UINT256, - }), - )); - - await this.token.delegateFromBySig(holderDelegatee, nonce, MAX_UINT256, v, r, s); - - await expectRevert( - this.token.delegateFromBySig(holderDelegatee, nonce, MAX_UINT256, v, r, s), - 'ERC20Comp::delegateBySig: invalid nonce', - ); - }); - - it('rejects expired permit', async function () { - const expiry = (await time.latest()) - time.duration.weeks(1); - const { v, r, s } = fromRpcSig(ethSigUtil.signTypedMessage( - delegator.getPrivateKey(), - buildData(this.chainId, this.token.address, { - delegatee: holderDelegatee, - nonce, - expiry, - }), - )); - - await expectRevert( - this.token.delegateFromBySig(holderDelegatee, nonce, expiry, v, r, s), - 'ERC20Comp::delegateBySig: signature expired', - ); - }); }); describe('transfers', function () { - beforeEach(async function () { - this.fromDelegatee = holder; - this.toDelegatee = recipient; - }); - it('no delegation', async function () { - }); + const { receipt } = await this.token.transfer(recipient, 1, { from: holder }); + expectEvent(receipt, 'Transfer', { from: holder, to: recipient, value: '1' }); + expectEvent.notEmitted(receipt, 'DelegateVotesChanged'); - it('sender has delegatee', async function () { - this.fromDelegatee = holderDelegatee; - await this.token.delegate(holderDelegatee, { from: holder }); + this.holderVotes = '0'; + this.recipientVotes = '0'; }); - it('recipient has delegatee', async function () { - this.toDelegatee = recipientDelegatee; - await this.token.delegate(recipientDelegatee, { from: recipient }); + it('sender delegation', async function () { + await this.token.delegate(holder, { from: holder }); + + const { receipt } = await this.token.transfer(recipient, 1, { from: holder }); + expectEvent(receipt, 'Transfer', { from: holder, to: recipient, value: '1' }); + expectEvent(receipt, 'DelegateVotesChanged', { delegate: holder, previousBalance: supply, newBalance: supply.subn(1) }); + + this.holderVotes = supply.subn(1); + this.recipientVotes = '0'; }); - it('sender and recipient have delegatee', async function () { - this.fromDelegatee = holderDelegatee; - this.toDelegatee = recipientDelegatee; - await this.token.delegate(holderDelegatee, { from: holder }); - await this.token.delegate(recipientDelegatee, { from: recipient }); + it('receiver delegation', async function () { + await this.token.delegate(recipient, { from: recipient }); + + const { receipt } = await this.token.transfer(recipient, 1, { from: holder }); + expectEvent(receipt, 'Transfer', { from: holder, to: recipient, value: '1' }); + expectEvent(receipt, 'DelegateVotesChanged', { delegate: recipient, previousBalance: '0', newBalance: '1' }); + + this.holderVotes = '0'; + this.recipientVotes = '1'; }); - afterEach(async function () { + it('full delegation', async function () { + await this.token.delegate(holder, { from: holder }); + await this.token.delegate(recipient, { from: recipient }); + const { receipt } = await this.token.transfer(recipient, 1, { from: holder }); expectEvent(receipt, 'Transfer', { from: holder, to: recipient, value: '1' }); - expectEvent(receipt, 'DelegateVotesChanged', { delegate: this.fromDelegatee, previousBalance: supply, newBalance: supply.subn(1) }); - expectEvent(receipt, 'DelegateVotesChanged', { delegate: this.toDelegatee, previousBalance: '0', newBalance: '1' }); + expectEvent(receipt, 'DelegateVotesChanged', { delegate: holder, previousBalance: supply, newBalance: supply.subn(1) }); + expectEvent(receipt, 'DelegateVotesChanged', { delegate: recipient, previousBalance: '0', newBalance: '1' }); + + this.holderVotes = supply.subn(1); + this.recipientVotes = '1'; + }); - expect(await this.token.getCurrentVotes(this.fromDelegatee)).to.be.bignumber.equal(supply.subn(1)); - expect(await this.token.getCurrentVotes(this.toDelegatee)).to.be.bignumber.equal('1'); + afterEach(async function () { + expect(await this.token.getCurrentVotes(holder)).to.be.bignumber.equal(this.holderVotes); + expect(await this.token.getCurrentVotes(recipient)).to.be.bignumber.equal(this.recipientVotes); // need to advance 2 blocks to see the effect of a transfer on "getPriorVotes" time.advanceBlock(); const blockNumber = await web3.eth.getBlockNumber(); time.advanceBlock(); - expect(await this.token.getPriorVotes(this.fromDelegatee, blockNumber)).to.be.bignumber.equal(supply.subn(1)); - expect(await this.token.getPriorVotes(this.toDelegatee, blockNumber)).to.be.bignumber.equal('1'); + expect(await this.token.getPriorVotes(holder, blockNumber)).to.be.bignumber.equal(this.holderVotes); + expect(await this.token.getPriorVotes(recipient, blockNumber)).to.be.bignumber.equal(this.recipientVotes); }); }); }); From ba43a54369d7ed1c3166a331bede16cfa63dcf56 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 20 Apr 2021 17:38:40 +0200 Subject: [PATCH 10/42] rename ERC20Comp to ERC20Votes --- .../mocks/{ERC20CompMock.sol => ERC20VotesMock.sol} | 4 ++-- .../{draft-ERC20Comp.sol => draft-ERC20Votes.sol} | 8 ++++---- ...ft-ERC20Comp.test.js => draft-ERC20Votes.test.js} | 12 ++++++------ 3 files changed, 12 insertions(+), 12 deletions(-) rename contracts/mocks/{ERC20CompMock.sol => ERC20VotesMock.sol} (80%) rename contracts/token/ERC20/extensions/{draft-ERC20Comp.sol => draft-ERC20Votes.sol} (92%) rename test/token/ERC20/extensions/{draft-ERC20Comp.test.js => draft-ERC20Votes.test.js} (97%) diff --git a/contracts/mocks/ERC20CompMock.sol b/contracts/mocks/ERC20VotesMock.sol similarity index 80% rename from contracts/mocks/ERC20CompMock.sol rename to contracts/mocks/ERC20VotesMock.sol index 309b7c66afe..f7f45bf94d6 100644 --- a/contracts/mocks/ERC20CompMock.sol +++ b/contracts/mocks/ERC20VotesMock.sol @@ -3,9 +3,9 @@ pragma solidity ^0.8.0; -import "../token/ERC20/extensions/draft-ERC20Comp.sol"; +import "../token/ERC20/extensions/draft-ERC20Votes.sol"; -contract ERC20CompMock is ERC20Comp { +contract ERC20VotesMock is ERC20Votes { constructor ( string memory name, string memory symbol, diff --git a/contracts/token/ERC20/extensions/draft-ERC20Comp.sol b/contracts/token/ERC20/extensions/draft-ERC20Votes.sol similarity index 92% rename from contracts/token/ERC20/extensions/draft-ERC20Comp.sol rename to contracts/token/ERC20/extensions/draft-ERC20Votes.sol index 2342261b1a5..60ffce9f6db 100644 --- a/contracts/token/ERC20/extensions/draft-ERC20Comp.sol +++ b/contracts/token/ERC20/extensions/draft-ERC20Votes.sol @@ -7,7 +7,7 @@ import "./IComp.sol"; import "../../../utils/Arrays.sol"; import "../../../utils/cryptography/ECDSA.sol"; -abstract contract ERC20Comp is IComp, ERC20Permit { +abstract contract ERC20Votes is IComp, ERC20Permit { bytes32 private constant _DELEGATION_TYPEHASH = keccak256("Delegation(address delegatee,uint256 nonce,uint256 expiry)"); mapping (address => address) private _delegates; @@ -34,7 +34,7 @@ abstract contract ERC20Comp is IComp, ERC20Permit { } function getPriorVotes(address account, uint blockNumber) external view override returns (uint256) { - require(blockNumber < block.number, "ERC20Comp::getPriorVotes: not yet determined"); + require(blockNumber < block.number, "ERC20Votes::getPriorVotes: not yet determined"); uint256 pos = Arrays.findUpperBound(_checkpointBlocks[account], blockNumber); return pos == 0 ? 0 : _checkpointWeights[account][pos - 1]; } @@ -46,7 +46,7 @@ abstract contract ERC20Comp is IComp, ERC20Permit { function delegateFromBySig(address delegatee, uint nonce, uint expiry, uint8 v, bytes32 r, bytes32 s) public virtual override { - require(block.timestamp <= expiry, "ERC20Comp::delegateBySig: signature expired"); + require(block.timestamp <= expiry, "ERC20Votes::delegateBySig: signature expired"); address signatory = ECDSA.recover( _hashTypedDataV4(keccak256(abi.encode( _DELEGATION_TYPEHASH, @@ -56,7 +56,7 @@ abstract contract ERC20Comp is IComp, ERC20Permit { ))), v, r, s ); - require(nonce == _useNonce(signatory), "ERC20Comp::delegateBySig: invalid nonce"); + require(nonce == _useNonce(signatory), "ERC20Votes::delegateBySig: invalid nonce"); return _delegate(signatory, delegatee); } diff --git a/test/token/ERC20/extensions/draft-ERC20Comp.test.js b/test/token/ERC20/extensions/draft-ERC20Votes.test.js similarity index 97% rename from test/token/ERC20/extensions/draft-ERC20Comp.test.js rename to test/token/ERC20/extensions/draft-ERC20Votes.test.js index 45927b84e10..93d0c500304 100644 --- a/test/token/ERC20/extensions/draft-ERC20Comp.test.js +++ b/test/token/ERC20/extensions/draft-ERC20Votes.test.js @@ -8,7 +8,7 @@ const { fromRpcSig } = require('ethereumjs-util'); const ethSigUtil = require('eth-sig-util'); const Wallet = require('ethereumjs-wallet').default; -const ERC20CompMock = artifacts.require('ERC20CompMock'); +const ERC20VotesMock = artifacts.require('ERC20VotesMock'); const { EIP712Domain, domainSeparator } = require('../../../helpers/eip712'); @@ -18,7 +18,7 @@ const Delegation = [ { name: 'expiry', type: 'uint256' }, ]; -contract('ERC20Comp', function (accounts) { +contract('ERC20Votes', function (accounts) { const [ holder, recipient, holderDelegatee, recipientDelegatee ] = accounts; const name = 'My Token'; @@ -28,7 +28,7 @@ contract('ERC20Comp', function (accounts) { const supply = new BN(100); beforeEach(async function () { - this.token = await ERC20CompMock.new(name, symbol, holder, supply); + this.token = await ERC20VotesMock.new(name, symbol, holder, supply); // We get the chain id from the contract because Ganache (used for coverage) does not return the same chain id // from within the EVM as from the JSON RPC interface. @@ -152,7 +152,7 @@ contract('ERC20Comp', function (accounts) { await expectRevert( this.token.delegateFromBySig(delegatorAddress, nonce, MAX_UINT256, v, r, s), - 'ERC20Comp::delegateBySig: invalid nonce', + 'ERC20Votes::delegateBySig: invalid nonce', ); }); @@ -184,7 +184,7 @@ contract('ERC20Comp', function (accounts) { )); await expectRevert( this.token.delegateFromBySig(delegatorAddress, nonce + 1, MAX_UINT256, v, r, s), - 'ERC20Comp::delegateBySig: invalid nonce', + 'ERC20Votes::delegateBySig: invalid nonce', ); }); @@ -201,7 +201,7 @@ contract('ERC20Comp', function (accounts) { await expectRevert( this.token.delegateFromBySig(delegatorAddress, nonce, expiry, v, r, s), - 'ERC20Comp::delegateBySig: signature expired', + 'ERC20Votes::delegateBySig: signature expired', ); }); }); From c9a321d1602f561161eadf3c307636cede22d9b7 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 28 Apr 2021 11:37:39 +0200 Subject: [PATCH 11/42] gas optimization + comp compatibility and tests --- contracts/token/ERC20/extensions/IComp.sol | 12 +- .../ERC20/extensions/draft-ERC20Votes.sol | 109 ++++++++++--- .../ERC20/extensions/draft-ERC20Votes.test.js | 151 ++++++++++++++++-- 3 files changed, 236 insertions(+), 36 deletions(-) diff --git a/contracts/token/ERC20/extensions/IComp.sol b/contracts/token/ERC20/extensions/IComp.sol index e028956d5bf..ae44ef97842 100644 --- a/contracts/token/ERC20/extensions/IComp.sol +++ b/contracts/token/ERC20/extensions/IComp.sol @@ -5,14 +5,20 @@ pragma solidity ^0.8.0; import "../IERC20.sol"; interface IComp is IERC20 { + struct Checkpoint { + uint32 fromBlock; + uint224 votes; + } + event DelegateChanged(address indexed delegator, address indexed fromDelegate, address indexed toDelegate); event DelegateVotesChanged(address indexed delegate, uint256 previousBalance, uint256 newBalance); // function nonces(address owner) external view returns (uint256); function delegates(address owner) external view returns (address); - + function checkpoints(address account, uint32 pos) external view returns (Checkpoint memory); + function numCheckpoints(address account) external view returns (uint32); + function getCurrentVotes(address account) external view returns (uint256); + function getPriorVotes(address account, uint256 blockNumber) external view returns (uint256); function delegate(address delegatee) external; function delegateFromBySig(address delegatee, uint nonce, uint expiry, uint8 v, bytes32 r, bytes32 s) external; - function getPriorVotes(address account, uint256 blockNumber) external view returns (uint256); - function getCurrentVotes(address account) external view returns (uint256); } diff --git a/contracts/token/ERC20/extensions/draft-ERC20Votes.sol b/contracts/token/ERC20/extensions/draft-ERC20Votes.sol index 60ffce9f6db..828c981bd54 100644 --- a/contracts/token/ERC20/extensions/draft-ERC20Votes.sol +++ b/contracts/token/ERC20/extensions/draft-ERC20Votes.sol @@ -4,46 +4,95 @@ pragma solidity ^0.8.0; import "./draft-ERC20Permit.sol"; import "./IComp.sol"; -import "../../../utils/Arrays.sol"; +import "../../../utils/math/Math.sol"; import "../../../utils/cryptography/ECDSA.sol"; +/** + * @dev Extension of the ERC20 token contract to support Compound's voting and delegation. + * + * This extensions keeps an history (snapshots) of each account's vote power. Vote power can be delegated either + * by calling the {delegate} directly, or by providing a signature that can later be verified and processed using + * {delegateFromBySig}. Voting power, can be checked through the public accessors {getCurrentVotes} and {getPriorVotes}. + * + * By default, delegation is disabled. This makes transfers cheaper. The downside is that it requires users to delegate + * to themselves in order to activate snapshots and have their voting power snapshoted. Enabling self-delegation can + * easily be done by overloading the {delegates} function. Keep in mind however that this will significantly increass + * the base gas cost of transfers. + * + * _Available since v4.2._ + */ abstract contract ERC20Votes is IComp, ERC20Permit { bytes32 private constant _DELEGATION_TYPEHASH = keccak256("Delegation(address delegatee,uint256 nonce,uint256 expiry)"); mapping (address => address) private _delegates; - mapping (address => uint256[]) private _checkpointBlocks; - mapping (address => mapping (uint256 => uint256)) private _checkpointWeights; + mapping (address => Checkpoint[]) private _checkpoints; + /** + * @dev Get the address `account` is currently delegating to. + */ function delegates(address account) public view virtual override returns (address) { return _delegates[account]; } + function checkpoints(address account, uint32 pos) external view virtual override returns (Checkpoint memory) { + return _checkpoints[account][pos]; + } + + function numCheckpoints(address account) external view virtual override returns (uint32) { + return uint32(_checkpoints[account].length); + } + /** - * Example: This enables autodelegation, makes each transfer more expensive but doesn't require user to delegate to - * themselves. Can be usefull for tokens useds exclusivelly for governance, such as voting wrappers of pre-existing - * ERC20. + * @dev Example: This enables autodelegation, makes each transfer more expensive but doesn't require user to + * delegate to themselves. Can be usefull for tokens useds exclusivelly for governance, such as voting wrappers of + * pre-existing ERC20. */ // function delegates(address account) public view override returns (address) { // address delegatee = _delegates[account]; // return delegatee == address(0) ? account : delegatee; // } + /** + * @notice Gets the current votes balance for `account` + * @param account The address to get votes balance + * @return The number of current votes for `account` + */ function getCurrentVotes(address account) external view override returns (uint256) { - uint256 pos = _checkpointBlocks[account].length; - return pos == 0 ? 0 : _checkpointWeights[account][pos - 1]; + uint256 pos = _checkpoints[account].length; + return pos == 0 ? 0 : _checkpoints[account][pos - 1].votes; } - function getPriorVotes(address account, uint blockNumber) external view override returns (uint256) { + /** + * @notice Determine the prior number of votes for an account as of a block number + * @dev Block number must be a finalized block or else this function will revert to prevent misinformation. + * @param account The address of the account to check + * @param blockNumber The block number to get the vote balance at + * @return The number of votes the account had as of the given block + */ + function getPriorVotes(address account, uint256 blockNumber) external view override returns (uint256) { require(blockNumber < block.number, "ERC20Votes::getPriorVotes: not yet determined"); - uint256 pos = Arrays.findUpperBound(_checkpointBlocks[account], blockNumber); - return pos == 0 ? 0 : _checkpointWeights[account][pos - 1]; + uint256 pos = _findUpperBound(_checkpoints[account], blockNumber); + return pos == 0 ? 0 : _checkpoints[account][pos - 1].votes; } + /** + * @notice Delegate votes from the sender to `delegatee` + * @param delegatee The address to delegate votes to + */ function delegate(address delegatee) public virtual override { return _delegate(_msgSender(), delegatee); } - function delegateFromBySig(address delegatee, uint nonce, uint expiry, uint8 v, bytes32 r, bytes32 s) + /** + * @notice Delegates votes from signatory to `delegatee` + * @param delegatee The address to delegate votes to + * @param nonce The contract state required to match the signature + * @param expiry The time at which to expire the signature + * @param v The recovery byte of the signature + * @param r Half of the ECDSA signature pair + * @param s Half of the ECDSA signature pair + */ + function delegateFromBySig(address delegatee, uint256 nonce, uint256 expiry, uint8 v, bytes32 r, bytes32 s) public virtual override { require(block.timestamp <= expiry, "ERC20Votes::delegateBySig: signature expired"); @@ -73,15 +122,15 @@ abstract contract ERC20Votes is IComp, ERC20Permit { function _moveDelegates(address srcRep, address dstRep, uint256 amount) private { if (srcRep != dstRep && amount > 0) { if (srcRep != address(0)) { - uint256 srcRepNum = _checkpointBlocks[srcRep].length; - uint256 srcRepOld = srcRepNum > 0 ? _checkpointWeights[srcRep][srcRepNum - 1] : 0; + uint256 srcRepNum = _checkpoints[srcRep].length; + uint256 srcRepOld = srcRepNum == 0 ? 0 : _checkpoints[srcRep][srcRepNum - 1].votes; uint256 srcRepNew = srcRepOld - amount; _writeCheckpoint(srcRep, srcRepNum, srcRepOld, srcRepNew); } if (dstRep != address(0)) { - uint256 dstRepNum = _checkpointBlocks[dstRep].length; - uint256 dstRepOld = dstRepNum > 0 ? _checkpointWeights[dstRep][dstRepNum - 1] : 0; + uint256 dstRepNum = _checkpoints[dstRep].length; + uint256 dstRepOld = dstRepNum == 0 ? 0 : _checkpoints[dstRep][dstRepNum - 1].votes; uint256 dstRepNew = dstRepOld + amount; _writeCheckpoint(dstRep, dstRepNum, dstRepOld, dstRepNew); } @@ -89,16 +138,36 @@ abstract contract ERC20Votes is IComp, ERC20Permit { } function _writeCheckpoint(address delegatee, uint256 pos, uint256 oldWeight, uint256 newWeight) private { - if (pos > 0 && _checkpointBlocks[delegatee][pos - 1] == block.number) { - _checkpointWeights[delegatee][pos - 1] = newWeight; + if (pos > 0 && _checkpoints[delegatee][pos - 1].fromBlock == block.number) { + _checkpoints[delegatee][pos - 1].votes = uint224(newWeight); // TODO: test overflow ? } else { - _checkpointBlocks[delegatee].push(block.number); - _checkpointWeights[delegatee][pos] = newWeight; + _checkpoints[delegatee].push(Checkpoint({ + fromBlock: uint32(block.number), + votes: uint224(newWeight) + })); } emit DelegateVotesChanged(delegatee, oldWeight, newWeight); } + /** + * @dev Specialization of Array.findUpperBound to work on Checkpoint[].fromBlock + * See {{Array}} for more details. + */ + function _findUpperBound(Checkpoint[] storage ckpts, uint256 blockNumber) private view returns (uint256) { + uint256 high = ckpts.length; + uint256 low = 0; + while (low < high) { + uint256 mid = Math.average(low, high); + if (ckpts[mid].fromBlock > blockNumber) { + high = mid; + } else { + low = mid + 1; + } + } + return low; + } + function _beforeTokenTransfer(address from, address to, uint256 amount) internal virtual override { _moveDelegates(delegates(from), delegates(to), amount); } diff --git a/test/token/ERC20/extensions/draft-ERC20Votes.test.js b/test/token/ERC20/extensions/draft-ERC20Votes.test.js index 93d0c500304..b60ee74b5ee 100644 --- a/test/token/ERC20/extensions/draft-ERC20Votes.test.js +++ b/test/token/ERC20/extensions/draft-ERC20Votes.test.js @@ -18,14 +18,33 @@ const Delegation = [ { name: 'expiry', type: 'uint256' }, ]; +function send (method, params = []) { + return new Promise(resolve => web3.currentProvider.send({ jsonrpc: '2.0', method, params }, resolve)); +} + +async function batchInBlock (txs) { + const before = await web3.eth.getBlockNumber(); + + await send('evm_setAutomine', [false]); + const promises = Promise.all(txs.map(fn => fn())); + await send('evm_setIntervalMining', [1000]); + const receipts = await promises; + await send('evm_setAutomine', [true]); + await send('evm_setIntervalMining', [false]); + + expect(await web3.eth.getBlockNumber()).to.be.equal(before + 1); + + return receipts; +} + contract('ERC20Votes', function (accounts) { - const [ holder, recipient, holderDelegatee, recipientDelegatee ] = accounts; + const [ holder, recipient, holderDelegatee, recipientDelegatee, other1, other2 ] = accounts; const name = 'My Token'; const symbol = 'MTKN'; const version = '1'; - const supply = new BN(100); + const supply = new BN('10000000000000000000000000'); beforeEach(async function () { this.token = await ERC20VotesMock.new(name, symbol, holder, supply); @@ -68,10 +87,9 @@ contract('ERC20Votes', function (accounts) { expect(await this.token.delegates(holder)).to.be.equal(holder); expect(await this.token.getCurrentVotes(holder)).to.be.bignumber.equal(supply); + expect(await this.token.getPriorVotes(holder, receipt.blockNumber - 1)).to.be.bignumber.equal('0'); await time.advanceBlock(); - expect(await this.token.getPriorVotes(holder, receipt.blockNumber)).to.be.bignumber.equal('0'); - await time.advanceBlock(); - expect(await this.token.getPriorVotes(holder, receipt.blockNumber + 1)).to.be.bignumber.equal(supply); + expect(await this.token.getPriorVotes(holder, receipt.blockNumber)).to.be.bignumber.equal(supply); }); it('delegation without balance', async function () { @@ -132,10 +150,9 @@ contract('ERC20Votes', function (accounts) { expect(await this.token.delegates(delegatorAddress)).to.be.equal(delegatorAddress); expect(await this.token.getCurrentVotes(delegatorAddress)).to.be.bignumber.equal(supply); + expect(await this.token.getPriorVotes(delegatorAddress, receipt.blockNumber - 1)).to.be.bignumber.equal('0'); await time.advanceBlock(); - expect(await this.token.getPriorVotes(delegatorAddress, receipt.blockNumber)).to.be.bignumber.equal('0'); - await time.advanceBlock(); - expect(await this.token.getPriorVotes(delegatorAddress, receipt.blockNumber + 1)).to.be.bignumber.equal(supply); + expect(await this.token.getPriorVotes(delegatorAddress, receipt.blockNumber)).to.be.bignumber.equal(supply); }); it('rejects reused signature', async function () { @@ -236,12 +253,11 @@ contract('ERC20Votes', function (accounts) { expect(await this.token.getCurrentVotes(holder)).to.be.bignumber.equal('0'); expect(await this.token.getCurrentVotes(holderDelegatee)).to.be.bignumber.equal(supply); + expect(await this.token.getPriorVotes(holder, receipt.blockNumber - 1)).to.be.bignumber.equal(supply); + expect(await this.token.getPriorVotes(holderDelegatee, receipt.blockNumber - 1)).to.be.bignumber.equal('0'); await time.advanceBlock(); - expect(await this.token.getPriorVotes(holder, receipt.blockNumber)).to.be.bignumber.equal(supply); - expect(await this.token.getPriorVotes(holderDelegatee, receipt.blockNumber)).to.be.bignumber.equal('0'); - await time.advanceBlock(); - expect(await this.token.getPriorVotes(holder, receipt.blockNumber + 1)).to.be.bignumber.equal('0'); - expect(await this.token.getPriorVotes(holderDelegatee, receipt.blockNumber + 1)).to.be.bignumber.equal(supply); + expect(await this.token.getPriorVotes(holder, receipt.blockNumber)).to.be.bignumber.equal('0'); + expect(await this.token.getPriorVotes(holderDelegatee, receipt.blockNumber)).to.be.bignumber.equal(supply); }); }); @@ -302,4 +318,113 @@ contract('ERC20Votes', function (accounts) { expect(await this.token.getPriorVotes(recipient, blockNumber)).to.be.bignumber.equal(this.recipientVotes); }); }); + + // 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('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 + expect(await this.token.numCheckpoints(other1)).to.be.bignumber.equal('0'); + + 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'); + + const t3 = await this.token.transfer(other2, 10, { from: recipient }); + expect(await this.token.numCheckpoints(other1)).to.be.bignumber.equal('3'); + + const t4 = await this.token.transfer(recipient, 20, { from: holder }); + expect(await this.token.numCheckpoints(other1)).to.be.bignumber.equal('4'); + + expect(await this.token.checkpoints(other1, 0)).to.be.deep.equal([ t1.receipt.blockNumber.toString(), '100' ]); + expect(await this.token.checkpoints(other1, 1)).to.be.deep.equal([ t2.receipt.blockNumber.toString(), '90' ]); + expect(await this.token.checkpoints(other1, 2)).to.be.deep.equal([ t3.receipt.blockNumber.toString(), '80' ]); + expect(await this.token.checkpoints(other1, 3)).to.be.deep.equal([ t4.receipt.blockNumber.toString(), '100' ]); + }); + + it('does not add more than one checkpoint in a block', async function () { + await this.token.transfer(recipient, '100', { from: holder }); + expect(await this.token.numCheckpoints(other1)).to.be.bignumber.equal('0'); + + const [ t1, t2, t3 ] = await batchInBlock([ + () => this.token.delegate(other1, { from: recipient }), + () => this.token.transfer(other2, 10, { from: recipient }), + () => this.token.transfer(other2, 10, { from: recipient }), + ]); + expect(await this.token.numCheckpoints(other1)).to.be.bignumber.equal('1'); + expect(await this.token.checkpoints(other1, 0)).to.be.deep.equal([ t1.receipt.blockNumber.toString(), '80' ]); + // expectReve(await this.token.checkpoints(other1, 1)).to.be.deep.equal([ '0', '0' ]); // Reverts due to array overflow check + // expect(await this.token.checkpoints(other1, 2)).to.be.deep.equal([ '0', '0' ]); // Reverts due to array overflow check + + const t4 = await this.token.transfer(recipient, 20, { from: holder }); + expect(await this.token.numCheckpoints(other1)).to.be.bignumber.equal('2'); + 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::getPriorVotes: not yet determined', + ); + }); + + it('returns 0 if there are no checkpoints', async function () { + expect(await this.token.getPriorVotes(other1, 0)).to.be.bignumber.equal('0'); + }); + + it('returns the latest block if >= last checkpoint block', async function () { + const t1 = await this.token.delegate(other1, { from: holder }); + await time.advanceBlock(); + await time.advanceBlock(); + + expect(await this.token.getPriorVotes(other1, t1.receipt.blockNumber)).to.be.bignumber.equal('10000000000000000000000000'); + expect(await this.token.getPriorVotes(other1, t1.receipt.blockNumber + 1)).to.be.bignumber.equal('10000000000000000000000000'); + }); + + it('returns zero if < first checkpoint block', async function () { + await time.advanceBlock(); + const t1 = await this.token.delegate(other1, { from: holder }); + await time.advanceBlock(); + await time.advanceBlock(); + + expect(await this.token.getPriorVotes(other1, t1.receipt.blockNumber - 1)).to.be.bignumber.equal('0'); + expect(await this.token.getPriorVotes(other1, t1.receipt.blockNumber + 1)).to.be.bignumber.equal('10000000000000000000000000'); + }); + + it('generally returns the voting balance at the appropriate checkpoint', async function () { + const t1 = await this.token.delegate(other1, { from: holder }); + await time.advanceBlock(); + await time.advanceBlock(); + const t2 = await this.token.transfer(other2, 10, { from: holder }); + await time.advanceBlock(); + await time.advanceBlock(); + const t3 = await this.token.transfer(other2, 10, { from: holder }); + await time.advanceBlock(); + await time.advanceBlock(); + const t4 = await this.token.transfer(holder, 20, { from: other2 }); + await time.advanceBlock(); + await time.advanceBlock(); + + expect(await this.token.getPriorVotes(other1, t1.receipt.blockNumber - 1)).to.be.bignumber.equal('0'); + expect(await this.token.getPriorVotes(other1, t1.receipt.blockNumber)).to.be.bignumber.equal('10000000000000000000000000'); + expect(await this.token.getPriorVotes(other1, t1.receipt.blockNumber + 1)).to.be.bignumber.equal('10000000000000000000000000'); + expect(await this.token.getPriorVotes(other1, t2.receipt.blockNumber)).to.be.bignumber.equal('9999999999999999999999990'); + expect(await this.token.getPriorVotes(other1, t2.receipt.blockNumber + 1)).to.be.bignumber.equal('9999999999999999999999990'); + expect(await this.token.getPriorVotes(other1, t3.receipt.blockNumber)).to.be.bignumber.equal('9999999999999999999999980'); + expect(await this.token.getPriorVotes(other1, t3.receipt.blockNumber + 1)).to.be.bignumber.equal('9999999999999999999999980'); + expect(await this.token.getPriorVotes(other1, t4.receipt.blockNumber)).to.be.bignumber.equal('10000000000000000000000000'); + expect(await this.token.getPriorVotes(other1, t4.receipt.blockNumber + 1)).to.be.bignumber.equal('10000000000000000000000000'); + }); + }); + }); }); From 74812f9543054cf68791693ad8650a011f1ec401 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 28 Apr 2021 11:55:54 +0200 Subject: [PATCH 12/42] function ordering --- .../token/ERC20/extensions/draft-ERC20Votes.sol | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/contracts/token/ERC20/extensions/draft-ERC20Votes.sol b/contracts/token/ERC20/extensions/draft-ERC20Votes.sol index 828c981bd54..335586dec03 100644 --- a/contracts/token/ERC20/extensions/draft-ERC20Votes.sol +++ b/contracts/token/ERC20/extensions/draft-ERC20Votes.sol @@ -27,13 +27,6 @@ abstract contract ERC20Votes is IComp, ERC20Permit { mapping (address => address) private _delegates; mapping (address => Checkpoint[]) private _checkpoints; - /** - * @dev Get the address `account` is currently delegating to. - */ - function delegates(address account) public view virtual override returns (address) { - return _delegates[account]; - } - function checkpoints(address account, uint32 pos) external view virtual override returns (Checkpoint memory) { return _checkpoints[account][pos]; } @@ -42,6 +35,13 @@ abstract contract ERC20Votes is IComp, ERC20Permit { return uint32(_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 Example: This enables autodelegation, makes each transfer more expensive but doesn't require user to * delegate to themselves. Can be usefull for tokens useds exclusivelly for governance, such as voting wrappers of From 9e49d5eaf2826d169e519cbfbaa0cdafbdb8e399 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 28 Apr 2021 13:53:19 +0200 Subject: [PATCH 13/42] minor refactor --- .../ERC20/extensions/draft-ERC20Votes.sol | 35 ++++++++----------- 1 file changed, 15 insertions(+), 20 deletions(-) diff --git a/contracts/token/ERC20/extensions/draft-ERC20Votes.sol b/contracts/token/ERC20/extensions/draft-ERC20Votes.sol index 335586dec03..adb43fcccb6 100644 --- a/contracts/token/ERC20/extensions/draft-ERC20Votes.sol +++ b/contracts/token/ERC20/extensions/draft-ERC20Votes.sol @@ -71,8 +71,21 @@ abstract contract ERC20Votes is IComp, ERC20Permit { */ function getPriorVotes(address account, uint256 blockNumber) external view override returns (uint256) { require(blockNumber < block.number, "ERC20Votes::getPriorVotes: not yet determined"); - uint256 pos = _findUpperBound(_checkpoints[account], blockNumber); - return pos == 0 ? 0 : _checkpoints[account][pos - 1].votes; + + Checkpoint[] storage ckpts = _checkpoints[account]; + + uint256 high = ckpts.length; + uint256 low = 0; + while (low < high) { + uint256 mid = Math.average(low, high); + if (ckpts[mid].fromBlock > blockNumber) { + high = mid; + } else { + low = mid + 1; + } + } + + return low == 0 ? 0 : _checkpoints[account][low - 1].votes; } /** @@ -150,24 +163,6 @@ abstract contract ERC20Votes is IComp, ERC20Permit { emit DelegateVotesChanged(delegatee, oldWeight, newWeight); } - /** - * @dev Specialization of Array.findUpperBound to work on Checkpoint[].fromBlock - * See {{Array}} for more details. - */ - function _findUpperBound(Checkpoint[] storage ckpts, uint256 blockNumber) private view returns (uint256) { - uint256 high = ckpts.length; - uint256 low = 0; - while (low < high) { - uint256 mid = Math.average(low, high); - if (ckpts[mid].fromBlock > blockNumber) { - high = mid; - } else { - low = mid + 1; - } - } - return low; - } - function _beforeTokenTransfer(address from, address to, uint256 amount) internal virtual override { _moveDelegates(delegates(from), delegates(to), amount); } From fc83c27d299960edabed5177081cc77325e7f8d4 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 28 Apr 2021 13:55:36 +0200 Subject: [PATCH 14/42] minor gas reduction --- contracts/token/ERC20/extensions/draft-ERC20Votes.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/token/ERC20/extensions/draft-ERC20Votes.sol b/contracts/token/ERC20/extensions/draft-ERC20Votes.sol index adb43fcccb6..bfd989c2692 100644 --- a/contracts/token/ERC20/extensions/draft-ERC20Votes.sol +++ b/contracts/token/ERC20/extensions/draft-ERC20Votes.sol @@ -85,7 +85,7 @@ abstract contract ERC20Votes is IComp, ERC20Permit { } } - return low == 0 ? 0 : _checkpoints[account][low - 1].votes; + return low == 0 ? 0 : ckpts[low - 1].votes; } /** From 0678a9fc2ab9486c0f98d7a5f0a5181fb4bb7d82 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 28 Apr 2021 14:09:25 +0200 Subject: [PATCH 15/42] improve testing --- test/token/ERC20/extensions/draft-ERC20Votes.test.js | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/test/token/ERC20/extensions/draft-ERC20Votes.test.js b/test/token/ERC20/extensions/draft-ERC20Votes.test.js index b60ee74b5ee..7c5c6e2fe6d 100644 --- a/test/token/ERC20/extensions/draft-ERC20Votes.test.js +++ b/test/token/ERC20/extensions/draft-ERC20Votes.test.js @@ -32,7 +32,7 @@ async function batchInBlock (txs) { await send('evm_setAutomine', [true]); await send('evm_setIntervalMining', [false]); - expect(await web3.eth.getBlockNumber()).to.be.equal(before + 1); + expect(receipts.map(({ blockNumber }) => blockNumber).every((val, _, arr) => val === arr[0])); return receipts; } @@ -311,9 +311,8 @@ contract('ERC20Votes', function (accounts) { expect(await this.token.getCurrentVotes(recipient)).to.be.bignumber.equal(this.recipientVotes); // need to advance 2 blocks to see the effect of a transfer on "getPriorVotes" - time.advanceBlock(); - const blockNumber = await web3.eth.getBlockNumber(); - time.advanceBlock(); + const blockNumber = await time.latestBlock(); + await time.advanceBlock(); expect(await this.token.getPriorVotes(holder, blockNumber)).to.be.bignumber.equal(this.holderVotes); expect(await this.token.getPriorVotes(recipient, blockNumber)).to.be.bignumber.equal(this.recipientVotes); }); From 321bdf351a31348b6dec3535ff6bac7220d3ebd8 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 28 Apr 2021 16:29:39 +0200 Subject: [PATCH 16/42] remove ERC20SnapsotEveryBlock for now --- .../mocks/ERC20SnapshotEveryBlockMock.sol | 25 --- .../token/ERC20/extensions/ERC20Snapshot.sol | 122 ++++++++++- .../extensions/ERC20SnapshotEveryBlock.sol | 144 ------------ .../ERC20SnapshotEveryBlock.test.js | 207 ------------------ 4 files changed, 116 insertions(+), 382 deletions(-) delete mode 100644 contracts/mocks/ERC20SnapshotEveryBlockMock.sol delete mode 100644 contracts/token/ERC20/extensions/ERC20SnapshotEveryBlock.sol delete mode 100644 test/token/ERC20/extensions/ERC20SnapshotEveryBlock.test.js diff --git a/contracts/mocks/ERC20SnapshotEveryBlockMock.sol b/contracts/mocks/ERC20SnapshotEveryBlockMock.sol deleted file mode 100644 index 95c7b9ecaac..00000000000 --- a/contracts/mocks/ERC20SnapshotEveryBlockMock.sol +++ /dev/null @@ -1,25 +0,0 @@ -// SPDX-License-Identifier: MIT - -pragma solidity ^0.8.0; - -import "../token/ERC20/extensions/ERC20SnapshotEveryBlock.sol"; - - -contract ERC20SnapshotEveryBlockMock is ERC20SnapshotEveryBlock { - constructor( - string memory name, - string memory symbol, - address initialAccount, - uint256 initialBalance - ) ERC20(name, symbol) { - _mint(initialAccount, initialBalance); - } - - function mint(address account, uint256 amount) public { - _mint(account, amount); - } - - function burn(address account, uint256 amount) public { - _burn(account, amount); - } -} diff --git a/contracts/token/ERC20/extensions/ERC20Snapshot.sol b/contracts/token/ERC20/extensions/ERC20Snapshot.sol index 12b696fdf1d..b5d01d5ef78 100644 --- a/contracts/token/ERC20/extensions/ERC20Snapshot.sol +++ b/contracts/token/ERC20/extensions/ERC20Snapshot.sol @@ -2,7 +2,8 @@ pragma solidity ^0.8.0; -import "./ERC20SnapshotEveryBlock.sol"; +import "../ERC20.sol"; +import "../../../utils/Arrays.sol"; import "../../../utils/Counters.sol"; /** @@ -19,6 +20,10 @@ import "../../../utils/Counters.sol"; * id. To get the balance of an account at the time of a snapshot, call the {balanceOfAt} function with the snapshot id * and the account address. * + * Note: snapshot policy can be customized by overriding the {_getCurrentSnapshotId} method. For example, having it + * return `block.number` will trigger the creation of snapshot at the begining of each new block. When overridding this + * function, be carefull about the monotonicity of its result. Non-monotonic snapshot ids will break the contract. + * * ==== Gas Costs * * Snapshots are efficient. Snapshot creation is _O(1)_. Retrieval of balances or total supply from a snapshot is _O(log @@ -29,17 +34,31 @@ import "../../../utils/Counters.sol"; * only significant for the first transfer that immediately follows a snapshot for a particular account. Subsequent * transfers will have normal cost until the next snapshot, and so on. */ -abstract contract ERC20Snapshot is ERC20SnapshotEveryBlock { + +abstract contract ERC20Snapshot is ERC20 { + // Inspired by Jordi Baylina's MiniMeToken to record historical balances: + // https://github.com/Giveth/minimd/blob/ea04d950eea153a04c51fa510b068b9dded390cb/contracts/MiniMeToken.sol + + using Arrays for uint256[]; using Counters for Counters.Counter; + // Snapshotted values have arrays of ids and the value corresponding to that id. These could be an array of a + // Snapshot struct, but that would impede usage of functions that work on an array. + struct Snapshots { + uint256[] ids; + uint256[] values; + } + + // Snapshot ids increase monotonically, with the first value being 1. An id of 0 is invalid. + Counters.Counter private _currentSnapshotId; + mapping (address => Snapshots) private _accountBalanceSnapshots; + Snapshots private _totalSupplySnapshots; + /** * @dev Emitted by {_snapshot} when a snapshot identified by `id` is created. */ event Snapshot(uint256 id); - // Snapshot ids increase monotonically, with the first value being 1. An id of 0 is invalid. - Counters.Counter private _currentSnapshotId; - /** * @dev Creates a new snapshot and returns its snapshot id. * @@ -72,7 +91,98 @@ abstract contract ERC20Snapshot is ERC20SnapshotEveryBlock { /** * @dev Get the current snapshotId */ - function _getCurrentSnapshotId() internal view virtual override returns (uint256) { + function _getCurrentSnapshotId() internal view virtual returns (uint256) { return _currentSnapshotId.current(); } + + /** + * @dev Retrieves the balance of `account` at the time `snapshotId` was created. + */ + function balanceOfAt(address account, uint256 snapshotId) public view virtual returns (uint256) { + (bool snapshotted, uint256 value) = _valueAt(snapshotId, _accountBalanceSnapshots[account]); + + return snapshotted ? value : balanceOf(account); + } + + /** + * @dev Retrieves the total supply at the time `snapshotId` was created. + */ + function totalSupplyAt(uint256 snapshotId) public view virtual returns(uint256) { + (bool snapshotted, uint256 value) = _valueAt(snapshotId, _totalSupplySnapshots); + + return snapshotted ? value : totalSupply(); + } + + // Update balance and/or total supply snapshots before the values are modified. This is implemented + // in the _beforeTokenTransfer hook, which is executed for _mint, _burn, and _transfer operations. + function _beforeTokenTransfer(address from, address to, uint256 amount) internal virtual override { + super._beforeTokenTransfer(from, to, amount); + + if (from == address(0)) { + // mint + _updateAccountSnapshot(to); + _updateTotalSupplySnapshot(); + } else if (to == address(0)) { + // burn + _updateAccountSnapshot(from); + _updateTotalSupplySnapshot(); + } else { + // transfer + _updateAccountSnapshot(from); + _updateAccountSnapshot(to); + } + } + + function _valueAt(uint256 snapshotId, Snapshots storage snapshots) + private view returns (bool, uint256) + { + require(snapshotId > 0, "ERC20Snapshot: id is 0"); + require(snapshotId <= _getCurrentSnapshotId(), "ERC20Snapshot: nonexistent id"); + + // When a valid snapshot is queried, there are three possibilities: + // a) The queried value was not modified after the snapshot was taken. Therefore, a snapshot entry was never + // created for this id, and all stored snapshot ids are smaller than the requested one. The value that corresponds + // to this id is the current one. + // b) The queried value was modified after the snapshot was taken. Therefore, there will be an entry with the + // requested id, and its value is the one to return. + // c) More snapshots were created after the requested one, and the queried value was later modified. There will be + // no entry for the requested id: the value that corresponds to it is that of the smallest snapshot id that is + // larger than the requested one. + // + // In summary, we need to find an element in an array, returning the index of the smallest value that is larger if + // it is not found, unless said value doesn't exist (e.g. when all values are smaller). Arrays.findUpperBound does + // exactly this. + + uint256 index = snapshots.ids.findUpperBound(snapshotId); + + if (index == snapshots.ids.length) { + return (false, 0); + } else { + return (true, snapshots.values[index]); + } + } + + function _updateAccountSnapshot(address account) private { + _updateSnapshot(_accountBalanceSnapshots[account], balanceOf(account)); + } + + function _updateTotalSupplySnapshot() private { + _updateSnapshot(_totalSupplySnapshots, totalSupply()); + } + + function _updateSnapshot(Snapshots storage snapshots, uint256 currentValue) private { + uint256 currentId = _getCurrentSnapshotId(); + if (_lastSnapshotId(snapshots.ids) < currentId) { + snapshots.ids.push(currentId); + snapshots.values.push(currentValue); + } + } + + function _lastSnapshotId(uint256[] storage ids) private view returns (uint256) { + if (ids.length == 0) { + return 0; + } else { + return ids[ids.length - 1]; + } + } } diff --git a/contracts/token/ERC20/extensions/ERC20SnapshotEveryBlock.sol b/contracts/token/ERC20/extensions/ERC20SnapshotEveryBlock.sol deleted file mode 100644 index 0aec337ec9e..00000000000 --- a/contracts/token/ERC20/extensions/ERC20SnapshotEveryBlock.sol +++ /dev/null @@ -1,144 +0,0 @@ -// SPDX-License-Identifier: MIT - -pragma solidity ^0.8.0; - -import "../ERC20.sol"; -import "../../../utils/Arrays.sol"; - -/** - * @dev This contract extends an ERC20 token with a snapshot mechanism. When a snapshot is created, the balances and - * total supply at the time are recorded for later access. - * - * This can be used to safely create mechanisms based on token balances such as trustless dividends or weighted voting. - * In naive implementations it's possible to perform a "double spend" attack by reusing the same balance from different - * accounts. By using snapshots to calculate dividends or voting power, those attacks no longer apply. It can also be - * used to create an efficient ERC20 forking mechanism. - * - * Snapshots are created by at every block. To get the total supply at a specific block, call the {totalSupplyAt} - * function with the block number. To get the balance of an account at a specific block, call the {balanceOfAt} - * function with the block number and the account address. - * - * ==== Gas Costs - * - * Snapshots are efficient. Snapshot creation is _O(1)_. Retrieval of balances or total supply from a snapshot is _O(log - * n)_ in the number of snapshots that have been created, although _n_ for a specific account will generally be much - * smaller since identical balances in subsequent snapshots are stored as a single entry. - * - * There is a constant overhead for normal ERC20 transfers due to the additional snapshot bookkeeping. This overhead is - * only significant for the first transfer that immediately follows a snapshot for a particular account. Subsequent - * transfers will have normal cost until the next snapshot, and so on. - */ -abstract contract ERC20SnapshotEveryBlock is ERC20 { - // Inspired by Jordi Baylina's MiniMeToken to record historical balances: - // https://github.com/Giveth/minimd/blob/ea04d950eea153a04c51fa510b068b9dded390cb/contracts/MiniMeToken.sol - - using Arrays for uint256[]; - - // Snapshotted values have arrays of ids and the value corresponding to that id. These could be an array of a - // Snapshot struct, but that would impede usage of functions that work on an array. - struct Snapshots { - uint256[] ids; - uint256[] values; - } - - mapping (address => Snapshots) private _accountBalanceSnapshots; - Snapshots private _totalSupplySnapshots; - - /** - * @dev Get the current snapshotId - */ - function _getCurrentSnapshotId() internal view virtual returns (uint256) { - return block.number; - } - - /** - * @dev Retrieves the balance of `account` at the time `snapshotId` was created. - */ - function balanceOfAt(address account, uint256 snapshotId) public view virtual returns (uint256) { - (bool snapshotted, uint256 value) = _valueAt(snapshotId, _accountBalanceSnapshots[account]); - - return snapshotted ? value : balanceOf(account); - } - - /** - * @dev Retrieves the total supply at the time `snapshotId` was created. - */ - function totalSupplyAt(uint256 snapshotId) public view virtual returns(uint256) { - (bool snapshotted, uint256 value) = _valueAt(snapshotId, _totalSupplySnapshots); - - return snapshotted ? value : totalSupply(); - } - - // Update balance and/or total supply snapshots before the values are modified. This is implemented - // in the _beforeTokenTransfer hook, which is executed for _mint, _burn, and _transfer operations. - function _beforeTokenTransfer(address from, address to, uint256 amount) internal virtual override { - super._beforeTokenTransfer(from, to, amount); - - if (from == address(0)) { - // mint - _updateAccountSnapshot(to); - _updateTotalSupplySnapshot(); - } else if (to == address(0)) { - // burn - _updateAccountSnapshot(from); - _updateTotalSupplySnapshot(); - } else { - // transfer - _updateAccountSnapshot(from); - _updateAccountSnapshot(to); - } - } - - function _valueAt(uint256 snapshotId, Snapshots storage snapshots) - private view returns (bool, uint256) - { - require(snapshotId > 0, "ERC20Snapshot: id is 0"); - require(snapshotId <= _getCurrentSnapshotId(), "ERC20Snapshot: nonexistent id"); - - // When a valid snapshot is queried, there are three possibilities: - // a) The queried value was not modified after the snapshot was taken. Therefore, a snapshot entry was never - // created for this id, and all stored snapshot ids are smaller than the requested one. The value that corresponds - // to this id is the current one. - // b) The queried value was modified after the snapshot was taken. Therefore, there will be an entry with the - // requested id, and its value is the one to return. - // c) More snapshots were created after the requested one, and the queried value was later modified. There will be - // no entry for the requested id: the value that corresponds to it is that of the smallest snapshot id that is - // larger than the requested one. - // - // In summary, we need to find an element in an array, returning the index of the smallest value that is larger if - // it is not found, unless said value doesn't exist (e.g. when all values are smaller). Arrays.findUpperBound does - // exactly this. - - uint256 index = snapshots.ids.findUpperBound(snapshotId); - - if (index == snapshots.ids.length) { - return (false, 0); - } else { - return (true, snapshots.values[index]); - } - } - - function _updateAccountSnapshot(address account) private { - _updateSnapshot(_accountBalanceSnapshots[account], balanceOf(account)); - } - - function _updateTotalSupplySnapshot() private { - _updateSnapshot(_totalSupplySnapshots, totalSupply()); - } - - function _updateSnapshot(Snapshots storage snapshots, uint256 currentValue) private { - uint256 currentId = _getCurrentSnapshotId(); - if (_lastSnapshotId(snapshots.ids) < currentId) { - snapshots.ids.push(currentId); - snapshots.values.push(currentValue); - } - } - - function _lastSnapshotId(uint256[] storage ids) private view returns (uint256) { - if (ids.length == 0) { - return 0; - } else { - return ids[ids.length - 1]; - } - } -} diff --git a/test/token/ERC20/extensions/ERC20SnapshotEveryBlock.test.js b/test/token/ERC20/extensions/ERC20SnapshotEveryBlock.test.js deleted file mode 100644 index 3c49a43523d..00000000000 --- a/test/token/ERC20/extensions/ERC20SnapshotEveryBlock.test.js +++ /dev/null @@ -1,207 +0,0 @@ -const { BN, time, expectRevert } = require('@openzeppelin/test-helpers'); -const ERC20SnapshotMock = artifacts.require('ERC20SnapshotEveryBlockMock'); - -const { expect } = require('chai'); - -function send (method, params = []) { - return new Promise(resolve => web3.currentProvider.send({ jsonrpc: '2.0', method, params }, resolve)); -} - -async function batchInBlock (txs) { - const before = await web3.eth.getBlockNumber(); - - await send('evm_setAutomine', [false]); - const promises = Promise.all(txs.map(fn => fn())); - await send('evm_setIntervalMining', [1000]); - const receipts = await promises; - await send('evm_setAutomine', [true]); - await send('evm_setIntervalMining', [false]); - - expect(await web3.eth.getBlockNumber()).to.be.equal(before + 1); - - return receipts; -} - -contract('ERC20SnapshotEveryBlock', function (accounts) { - const [ initialHolder, recipient, other ] = accounts; - - const initialSupply = new BN(100); - - const name = 'My Token'; - const symbol = 'MTKN'; - - beforeEach(async function () { - time.advanceBlock(); - this.token = await ERC20SnapshotMock.new(name, symbol, initialHolder, initialSupply); - this.initialBlock = await web3.eth.getBlockNumber(); - }); - - describe('totalSupplyAt', function () { - it('reverts with a snapshot id of 0', async function () { - await expectRevert(this.token.totalSupplyAt(0), 'ERC20Snapshot: id is 0'); - }); - - context('with no supply changes after the snapshot', function () { - it('snapshot before initial supply', async function () { - expect(await this.token.totalSupplyAt(this.initialBlock)).to.be.bignumber.equal('0'); - }); - - it('snapshot after initial supply', async function () { - await time.advanceBlock(); - expect(await this.token.totalSupplyAt(this.initialBlock + 1)).to.be.bignumber.equal(initialSupply); - }); - }); - - context('with supply changes', function () { - beforeEach(async function () { - await batchInBlock([ - () => this.token.mint(other, new BN('50')), - () => this.token.burn(initialHolder, new BN('20')), - ]); - this.operationBlockNumber = await web3.eth.getBlockNumber(); - }); - - it('returns the total supply before the changes', async function () { - expect(await this.token.totalSupplyAt(this.operationBlockNumber)) - .to.be.bignumber.equal(initialSupply); - }); - - it('snapshots return the supply after the changes', async function () { - await time.advanceBlock(); - expect(await this.token.totalSupplyAt(this.operationBlockNumber + 1)) - .to.be.bignumber.equal(initialSupply.addn(50).subn(20)); - }); - }); - - describe('with multiple operations over multiple blocks', function () { - beforeEach(async function () { - this.snapshots = [ this.initialBlock ]; - await this.token.mint(other, new BN('50')); - this.snapshots.push(await web3.eth.getBlockNumber()); - await this.token.transfer(recipient, new BN('10'), { from: initialHolder }); - this.snapshots.push(await web3.eth.getBlockNumber()); - await this.token.burn(initialHolder, new BN('20')); - this.snapshots.push(await web3.eth.getBlockNumber()); - await time.advanceBlock(); - this.snapshots.push(await web3.eth.getBlockNumber()); - }); - - it('check snapshots', async function () { - expect(await this.token.totalSupplyAt(this.snapshots[0])) - .to.be.bignumber.equal('0'); - // initial mint - expect(await this.token.totalSupplyAt(this.snapshots[1])) - .to.be.bignumber.equal(initialSupply); - // mint 50 - expect(await this.token.totalSupplyAt(this.snapshots[2])) - .to.be.bignumber.equal(initialSupply.addn(50)); - // transfer: no change - expect(await this.token.totalSupplyAt(this.snapshots[3])) - .to.be.bignumber.equal(initialSupply.addn(50)); - // burn 20 - expect(await this.token.totalSupplyAt(this.snapshots[4])) - .to.be.bignumber.equal(initialSupply.addn(50).subn(20)); - }); - }); - }); - - describe('balanceOfAt', function () { - it('reverts with a snapshot id of 0', async function () { - await expectRevert(this.token.balanceOfAt(initialHolder, 0), 'ERC20Snapshot: id is 0'); - }); - - context('with no supply changes after the snapshot', function () { - it('snapshot before initial supply', async function () { - expect(await this.token.balanceOfAt(initialHolder, this.initialBlock)) - .to.be.bignumber.equal('0'); - }); - - it('snapshot after initial supply', async function () { - await time.advanceBlock(); - expect(await this.token.balanceOfAt(initialHolder, this.initialBlock + 1)) - .to.be.bignumber.equal(initialSupply); - }); - }); - - context('with balance changes', function () { - beforeEach(async function () { - await batchInBlock([ - () => this.token.transfer(recipient, new BN('10'), { from: initialHolder }), - () => this.token.mint(other, new BN('50')), - () => this.token.burn(initialHolder, new BN('20')), - ]); - this.operationBlockNumber = await web3.eth.getBlockNumber(); - }); - - it('returns the balances before the changes', async function () { - expect(await this.token.balanceOfAt(initialHolder, this.operationBlockNumber)) - .to.be.bignumber.equal(initialSupply); - expect(await this.token.balanceOfAt(recipient, this.operationBlockNumber)) - .to.be.bignumber.equal('0'); - expect(await this.token.balanceOfAt(other, this.operationBlockNumber)) - .to.be.bignumber.equal('0'); - }); - - it('snapshots return the balances after the changes', async function () { - await time.advanceBlock(); - expect(await this.token.balanceOfAt(initialHolder, this.operationBlockNumber + 1)) - .to.be.bignumber.equal(initialSupply.subn(10).subn(20)); - expect(await this.token.balanceOfAt(recipient, this.operationBlockNumber + 1)) - .to.be.bignumber.equal('10'); - expect(await this.token.balanceOfAt(other, this.operationBlockNumber + 1)) - .to.be.bignumber.equal('50'); - }); - }); - - describe('with multiple operations over multiple blocks', function () { - beforeEach(async function () { - this.snapshots = [ this.initialBlock ]; - await this.token.mint(other, new BN('50')); - this.snapshots.push(await web3.eth.getBlockNumber()); - await this.token.transfer(recipient, new BN('10'), { from: initialHolder }); - this.snapshots.push(await web3.eth.getBlockNumber()); - await this.token.burn(initialHolder, new BN('20')); - this.snapshots.push(await web3.eth.getBlockNumber()); - await time.advanceBlock(); - this.snapshots.push(await web3.eth.getBlockNumber()); - }); - - it('check snapshots', async function () { - expect(await this.token.balanceOfAt(initialHolder, this.snapshots[0])) - .to.be.bignumber.equal('0'); - expect(await this.token.balanceOfAt(recipient, this.snapshots[0])) - .to.be.bignumber.equal('0'); - expect(await this.token.balanceOfAt(other, this.snapshots[0])) - .to.be.bignumber.equal('0'); - // initial mint - expect(await this.token.balanceOfAt(initialHolder, this.snapshots[1])) - .to.be.bignumber.equal(initialSupply); - expect(await this.token.balanceOfAt(recipient, this.snapshots[1])) - .to.be.bignumber.equal('0'); - expect(await this.token.balanceOfAt(other, this.snapshots[1])) - .to.be.bignumber.equal('0'); - // mint 50 - expect(await this.token.balanceOfAt(initialHolder, this.snapshots[2])) - .to.be.bignumber.equal(initialSupply); - expect(await this.token.balanceOfAt(recipient, this.snapshots[2])) - .to.be.bignumber.equal('0'); - expect(await this.token.balanceOfAt(other, this.snapshots[2])) - .to.be.bignumber.equal('50'); - // transfer - expect(await this.token.balanceOfAt(initialHolder, this.snapshots[3])) - .to.be.bignumber.equal(initialSupply.subn(10)); - expect(await this.token.balanceOfAt(recipient, this.snapshots[3])) - .to.be.bignumber.equal('10'); - expect(await this.token.balanceOfAt(other, this.snapshots[3])) - .to.be.bignumber.equal('50'); - // burn 20 - expect(await this.token.balanceOfAt(initialHolder, this.snapshots[4])) - .to.be.bignumber.equal(initialSupply.subn(10).subn(20)); - expect(await this.token.balanceOfAt(recipient, this.snapshots[4])) - .to.be.bignumber.equal('10'); - expect(await this.token.balanceOfAt(other, this.snapshots[4])) - .to.be.bignumber.equal('50'); - }); - }); - }); -}); From cc8682f86c983c5943f8268bfbd8bc9b44d14cf4 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Mon, 3 May 2021 22:48:39 +0200 Subject: [PATCH 17/42] rename IComp to IERC20Votes --- contracts/token/ERC20/extensions/draft-ERC20Votes.sol | 4 ++-- .../ERC20/extensions/{IComp.sol => draft-IERC20Votes.sol} | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) rename contracts/token/ERC20/extensions/{IComp.sol => draft-IERC20Votes.sol} (96%) diff --git a/contracts/token/ERC20/extensions/draft-ERC20Votes.sol b/contracts/token/ERC20/extensions/draft-ERC20Votes.sol index bfd989c2692..811b9e4c5bc 100644 --- a/contracts/token/ERC20/extensions/draft-ERC20Votes.sol +++ b/contracts/token/ERC20/extensions/draft-ERC20Votes.sol @@ -3,7 +3,7 @@ pragma solidity ^0.8.0; import "./draft-ERC20Permit.sol"; -import "./IComp.sol"; +import "./draft-IERC20Votes.sol"; import "../../../utils/math/Math.sol"; import "../../../utils/cryptography/ECDSA.sol"; @@ -21,7 +21,7 @@ import "../../../utils/cryptography/ECDSA.sol"; * * _Available since v4.2._ */ -abstract contract ERC20Votes is IComp, ERC20Permit { +abstract contract ERC20Votes is IERC20Votes, ERC20Permit { bytes32 private constant _DELEGATION_TYPEHASH = keccak256("Delegation(address delegatee,uint256 nonce,uint256 expiry)"); mapping (address => address) private _delegates; diff --git a/contracts/token/ERC20/extensions/IComp.sol b/contracts/token/ERC20/extensions/draft-IERC20Votes.sol similarity index 96% rename from contracts/token/ERC20/extensions/IComp.sol rename to contracts/token/ERC20/extensions/draft-IERC20Votes.sol index ae44ef97842..6257a82bfbb 100644 --- a/contracts/token/ERC20/extensions/IComp.sol +++ b/contracts/token/ERC20/extensions/draft-IERC20Votes.sol @@ -4,7 +4,7 @@ pragma solidity ^0.8.0; import "../IERC20.sol"; -interface IComp is IERC20 { +interface IERC20Votes is IERC20 { struct Checkpoint { uint32 fromBlock; uint224 votes; From 9e166bcfb0e127520e94bca67a1100404fc4750d Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 6 May 2021 11:22:13 +0200 Subject: [PATCH 18/42] storage slot consistency --- contracts/token/ERC20/extensions/ERC20Snapshot.sol | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/contracts/token/ERC20/extensions/ERC20Snapshot.sol b/contracts/token/ERC20/extensions/ERC20Snapshot.sol index b5d01d5ef78..13b95c00d90 100644 --- a/contracts/token/ERC20/extensions/ERC20Snapshot.sol +++ b/contracts/token/ERC20/extensions/ERC20Snapshot.sol @@ -49,11 +49,12 @@ abstract contract ERC20Snapshot is ERC20 { uint256[] values; } - // Snapshot ids increase monotonically, with the first value being 1. An id of 0 is invalid. - Counters.Counter private _currentSnapshotId; mapping (address => Snapshots) private _accountBalanceSnapshots; Snapshots private _totalSupplySnapshots; + // Snapshot ids increase monotonically, with the first value being 1. An id of 0 is invalid. + Counters.Counter private _currentSnapshotId; + /** * @dev Emitted by {_snapshot} when a snapshot identified by `id` is created. */ From 2667a835223a9770e64251e7a336bd43a6052e49 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 6 May 2021 11:24:43 +0200 Subject: [PATCH 19/42] remove comment about delegation overridding --- contracts/token/ERC20/extensions/draft-ERC20Votes.sol | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/contracts/token/ERC20/extensions/draft-ERC20Votes.sol b/contracts/token/ERC20/extensions/draft-ERC20Votes.sol index 811b9e4c5bc..872d29ede6e 100644 --- a/contracts/token/ERC20/extensions/draft-ERC20Votes.sol +++ b/contracts/token/ERC20/extensions/draft-ERC20Votes.sol @@ -42,16 +42,6 @@ abstract contract ERC20Votes is IERC20Votes, ERC20Permit { return _delegates[account]; } - /** - * @dev Example: This enables autodelegation, makes each transfer more expensive but doesn't require user to - * delegate to themselves. Can be usefull for tokens useds exclusivelly for governance, such as voting wrappers of - * pre-existing ERC20. - */ - // function delegates(address account) public view override returns (address) { - // address delegatee = _delegates[account]; - // return delegatee == address(0) ? account : delegatee; - // } - /** * @notice Gets the current votes balance for `account` * @param account The address to get votes balance From 970ea15f3e52858224e075649c33379a2ebdad2c Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 6 May 2021 11:27:25 +0200 Subject: [PATCH 20/42] Apply suggestions from code review Co-authored-by: Francisco Giordano --- contracts/token/ERC20/extensions/ERC20Snapshot.sol | 7 +++++-- contracts/token/ERC20/extensions/draft-ERC20Votes.sol | 10 +++++----- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/contracts/token/ERC20/extensions/ERC20Snapshot.sol b/contracts/token/ERC20/extensions/ERC20Snapshot.sol index 13b95c00d90..349a758fa85 100644 --- a/contracts/token/ERC20/extensions/ERC20Snapshot.sol +++ b/contracts/token/ERC20/extensions/ERC20Snapshot.sol @@ -20,9 +20,12 @@ import "../../../utils/Counters.sol"; * id. To get the balance of an account at the time of a snapshot, call the {balanceOfAt} function with the snapshot id * and the account address. * - * Note: snapshot policy can be customized by overriding the {_getCurrentSnapshotId} method. For example, having it + * NOTE: Snapshot policy can be customized by overriding the {_getCurrentSnapshotId} method. For example, having it * return `block.number` will trigger the creation of snapshot at the begining of each new block. When overridding this - * function, be carefull about the monotonicity of its result. Non-monotonic snapshot ids will break the contract. + * function, be careful about the monotonicity of its result. Non-monotonic snapshot ids will break the contract. + * + * Implementing snapshots for every block using this method will incur significant gas costs. For a gas-efficient + * alternative consider {ERC20Votes}. * * ==== Gas Costs * diff --git a/contracts/token/ERC20/extensions/draft-ERC20Votes.sol b/contracts/token/ERC20/extensions/draft-ERC20Votes.sol index 872d29ede6e..2437299057c 100644 --- a/contracts/token/ERC20/extensions/draft-ERC20Votes.sol +++ b/contracts/token/ERC20/extensions/draft-ERC20Votes.sol @@ -10,13 +10,13 @@ import "../../../utils/cryptography/ECDSA.sol"; /** * @dev Extension of the ERC20 token contract to support Compound's voting and delegation. * - * This extensions keeps an history (snapshots) of each account's vote power. Vote power can be delegated either + * This extensions keeps a history (checkpoints) of each account's vote power. Vote power can be delegated either * by calling the {delegate} directly, or by providing a signature that can later be verified and processed using - * {delegateFromBySig}. Voting power, can be checked through the public accessors {getCurrentVotes} and {getPriorVotes}. + * {delegateFromBySig}. Voting power can be checked through the public accessors {getCurrentVotes} and {getPriorVotes}. * - * By default, delegation is disabled. This makes transfers cheaper. The downside is that it requires users to delegate - * to themselves in order to activate snapshots and have their voting power snapshoted. Enabling self-delegation can - * easily be done by overloading the {delegates} function. Keep in mind however that this will significantly increass + * By default, token balance does not account for voting power. This makes transfers cheaper. The downside is that it requires users to delegate + * to themselves in order to activate checkpoints and have their voting power tracked. Enabling self-delegation can + * easily be done by overriding the {delegates} function. Keep in mind however that this will significantly increase * the base gas cost of transfers. * * _Available since v4.2._ From 464b207dab3df6b72f1ed25356e3cd2cf2da7b72 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 6 May 2021 11:27:52 +0200 Subject: [PATCH 21/42] improve compound compatibility --- contracts/token/ERC20/extensions/draft-ERC20Votes.sol | 2 +- contracts/token/ERC20/extensions/draft-IERC20Votes.sol | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/contracts/token/ERC20/extensions/draft-ERC20Votes.sol b/contracts/token/ERC20/extensions/draft-ERC20Votes.sol index 872d29ede6e..07efbf26089 100644 --- a/contracts/token/ERC20/extensions/draft-ERC20Votes.sol +++ b/contracts/token/ERC20/extensions/draft-ERC20Votes.sol @@ -95,7 +95,7 @@ abstract contract ERC20Votes is IERC20Votes, ERC20Permit { * @param r Half of the ECDSA signature pair * @param s Half of the ECDSA signature pair */ - function delegateFromBySig(address delegatee, uint256 nonce, uint256 expiry, uint8 v, bytes32 r, bytes32 s) + function delegateBySig(address delegatee, uint256 nonce, uint256 expiry, uint8 v, bytes32 r, bytes32 s) public virtual override { require(block.timestamp <= expiry, "ERC20Votes::delegateBySig: signature expired"); diff --git a/contracts/token/ERC20/extensions/draft-IERC20Votes.sol b/contracts/token/ERC20/extensions/draft-IERC20Votes.sol index 6257a82bfbb..30a5f146bed 100644 --- a/contracts/token/ERC20/extensions/draft-IERC20Votes.sol +++ b/contracts/token/ERC20/extensions/draft-IERC20Votes.sol @@ -13,12 +13,11 @@ interface IERC20Votes is IERC20 { event DelegateChanged(address indexed delegator, address indexed fromDelegate, address indexed toDelegate); event DelegateVotesChanged(address indexed delegate, uint256 previousBalance, uint256 newBalance); - // function nonces(address owner) external view returns (uint256); function delegates(address owner) external view returns (address); function checkpoints(address account, uint32 pos) external view returns (Checkpoint memory); function numCheckpoints(address account) external view returns (uint32); function getCurrentVotes(address account) external view returns (uint256); function getPriorVotes(address account, uint256 blockNumber) external view returns (uint256); function delegate(address delegatee) external; - function delegateFromBySig(address delegatee, uint nonce, uint expiry, uint8 v, bytes32 r, bytes32 s) external; + function delegateBySig(address delegatee, uint nonce, uint expiry, uint8 v, bytes32 r, bytes32 s) external; } From a37d5d2fcf66d17f1d07a4c388f8186cfd9efc89 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 6 May 2021 11:32:42 +0200 Subject: [PATCH 22/42] use SafeCast and fix tests --- .../token/ERC20/extensions/draft-ERC20Votes.sol | 9 +++++---- contracts/utils/math/SafeCast.sol | 15 +++++++++++++++ .../ERC20/extensions/draft-ERC20Votes.test.js | 12 ++++++------ 3 files changed, 26 insertions(+), 10 deletions(-) diff --git a/contracts/token/ERC20/extensions/draft-ERC20Votes.sol b/contracts/token/ERC20/extensions/draft-ERC20Votes.sol index d82ed41bc73..936bc00486a 100644 --- a/contracts/token/ERC20/extensions/draft-ERC20Votes.sol +++ b/contracts/token/ERC20/extensions/draft-ERC20Votes.sol @@ -5,6 +5,7 @@ pragma solidity ^0.8.0; import "./draft-ERC20Permit.sol"; import "./draft-IERC20Votes.sol"; import "../../../utils/math/Math.sol"; +import "../../../utils/math/SafeCast.sol"; import "../../../utils/cryptography/ECDSA.sol"; /** @@ -32,7 +33,7 @@ abstract contract ERC20Votes is IERC20Votes, ERC20Permit { } function numCheckpoints(address account) external view virtual override returns (uint32) { - return uint32(_checkpoints[account].length); + return SafeCast.toUint32(_checkpoints[account].length); } /** @@ -142,11 +143,11 @@ abstract contract ERC20Votes is IERC20Votes, ERC20Permit { function _writeCheckpoint(address delegatee, uint256 pos, uint256 oldWeight, uint256 newWeight) private { if (pos > 0 && _checkpoints[delegatee][pos - 1].fromBlock == block.number) { - _checkpoints[delegatee][pos - 1].votes = uint224(newWeight); // TODO: test overflow ? + _checkpoints[delegatee][pos - 1].votes = SafeCast.toUint224(newWeight); } else { _checkpoints[delegatee].push(Checkpoint({ - fromBlock: uint32(block.number), - votes: uint224(newWeight) + fromBlock: SafeCast.toUint32(block.number), + votes: SafeCast.toUint224(newWeight) })); } diff --git a/contracts/utils/math/SafeCast.sol b/contracts/utils/math/SafeCast.sol index 5415462c19a..e217ca69ea0 100644 --- a/contracts/utils/math/SafeCast.sol +++ b/contracts/utils/math/SafeCast.sol @@ -18,6 +18,21 @@ pragma solidity ^0.8.0; * all math on `uint256` and `int256` and then downcasting. */ library SafeCast { + /** + * @dev Returns the downcasted uint224 from uint256, reverting on + * overflow (when the input is greater than largest uint224). + * + * Counterpart to Solidity's `uint224` operator. + * + * Requirements: + * + * - input must fit into 224 bits + */ + function toUint224(uint256 value) internal pure returns (uint224) { + require(value < 2**224, "SafeCast: value doesn\'t fit in 224 bits"); + return uint224(value); + } + /** * @dev Returns the downcasted uint128 from uint256, reverting on * overflow (when the input is greater than largest uint128). diff --git a/test/token/ERC20/extensions/draft-ERC20Votes.test.js b/test/token/ERC20/extensions/draft-ERC20Votes.test.js index 7c5c6e2fe6d..f6863d1dcf2 100644 --- a/test/token/ERC20/extensions/draft-ERC20Votes.test.js +++ b/test/token/ERC20/extensions/draft-ERC20Votes.test.js @@ -135,7 +135,7 @@ contract('ERC20Votes', function (accounts) { expect(await this.token.delegates(delegatorAddress)).to.be.equal(ZERO_ADDRESS); - const { receipt } = await this.token.delegateFromBySig(delegatorAddress, nonce, MAX_UINT256, v, r, s); + const { receipt } = await this.token.delegateBySig(delegatorAddress, nonce, MAX_UINT256, v, r, s); expectEvent(receipt, 'DelegateChanged', { delegator: delegatorAddress, fromDelegate: ZERO_ADDRESS, @@ -165,10 +165,10 @@ contract('ERC20Votes', function (accounts) { }), )); - await this.token.delegateFromBySig(delegatorAddress, nonce, MAX_UINT256, v, r, s); + await this.token.delegateBySig(delegatorAddress, nonce, MAX_UINT256, v, r, s); await expectRevert( - this.token.delegateFromBySig(delegatorAddress, nonce, MAX_UINT256, v, r, s), + this.token.delegateBySig(delegatorAddress, nonce, MAX_UINT256, v, r, s), 'ERC20Votes::delegateBySig: invalid nonce', ); }); @@ -183,7 +183,7 @@ contract('ERC20Votes', function (accounts) { }), )); - const { logs } = await this.token.delegateFromBySig(holderDelegatee, nonce, MAX_UINT256, v, r, s); + const { logs } = await this.token.delegateBySig(holderDelegatee, nonce, MAX_UINT256, v, r, s); const { args } = logs.find(({ event }) => event == 'DelegateChanged'); expect(args.delegator).to.not.be.equal(delegatorAddress); expect(args.fromDelegate).to.be.equal(ZERO_ADDRESS); @@ -200,7 +200,7 @@ contract('ERC20Votes', function (accounts) { }), )); await expectRevert( - this.token.delegateFromBySig(delegatorAddress, nonce + 1, MAX_UINT256, v, r, s), + this.token.delegateBySig(delegatorAddress, nonce + 1, MAX_UINT256, v, r, s), 'ERC20Votes::delegateBySig: invalid nonce', ); }); @@ -217,7 +217,7 @@ contract('ERC20Votes', function (accounts) { )); await expectRevert( - this.token.delegateFromBySig(delegatorAddress, nonce, expiry, v, r, s), + this.token.delegateBySig(delegatorAddress, nonce, expiry, v, r, s), 'ERC20Votes::delegateBySig: signature expired', ); }); From de5c951123db4aef6ce42f8d704c0688c318ee39 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 6 May 2021 11:53:12 +0200 Subject: [PATCH 23/42] update changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 549d196ab80..c8c2d1efd0f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,7 @@ ## Unreleased - * `ERC20SnapshotEveryBlock`: add a variation of the `ERC20Snapshot` contract that does snapshot at every block. ([#2583](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2583)) + * `ERC20Votes`: add a new extension of the `ERC20` token with support for voting snapshots and delegation. This extension is compatible with Compound's `Comp` token interface. ([#2632](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2632)) ## Unreleased From cdb3484988e36232f59ddd2576e11761d65122c9 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 6 May 2021 12:58:08 +0200 Subject: [PATCH 24/42] extend ERC20Votes testing --- test/token/ERC20/extensions/draft-ERC20Votes.test.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test/token/ERC20/extensions/draft-ERC20Votes.test.js b/test/token/ERC20/extensions/draft-ERC20Votes.test.js index f6863d1dcf2..d49af45b07f 100644 --- a/test/token/ERC20/extensions/draft-ERC20Votes.test.js +++ b/test/token/ERC20/extensions/draft-ERC20Votes.test.js @@ -347,6 +347,12 @@ contract('ERC20Votes', function (accounts) { expect(await this.token.checkpoints(other1, 1)).to.be.deep.equal([ t2.receipt.blockNumber.toString(), '90' ]); expect(await this.token.checkpoints(other1, 2)).to.be.deep.equal([ t3.receipt.blockNumber.toString(), '80' ]); expect(await this.token.checkpoints(other1, 3)).to.be.deep.equal([ t4.receipt.blockNumber.toString(), '100' ]); + + await time.advanceBlock(); + expect(await this.token.getPriorVotes(other1, t1.receipt.blockNumber)).to.be.bignumber.equal('100'); + expect(await this.token.getPriorVotes(other1, t2.receipt.blockNumber)).to.be.bignumber.equal('90'); + expect(await this.token.getPriorVotes(other1, t3.receipt.blockNumber)).to.be.bignumber.equal('80'); + expect(await this.token.getPriorVotes(other1, t4.receipt.blockNumber)).to.be.bignumber.equal('100'); }); it('does not add more than one checkpoint in a block', async function () { From 4158e7ff4957338df0557b85f6dbc9564d29c4f8 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 11 May 2021 17:46:35 +0200 Subject: [PATCH 25/42] comment getPriorVotes functionnality --- .../token/ERC20/extensions/draft-ERC20Votes.sol | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/contracts/token/ERC20/extensions/draft-ERC20Votes.sol b/contracts/token/ERC20/extensions/draft-ERC20Votes.sol index 936bc00486a..0ff30fee69d 100644 --- a/contracts/token/ERC20/extensions/draft-ERC20Votes.sol +++ b/contracts/token/ERC20/extensions/draft-ERC20Votes.sol @@ -65,6 +65,16 @@ abstract contract ERC20Votes is IERC20Votes, ERC20Permit { Checkpoint[] storage ckpts = _checkpoints[account]; + // Property: low and high converge on the first (earliest) checkpoint that is AFTER (or equal) `blockNumber`. + // - If all checkpoints are before `blockNumber`, low and high converge toward `ckpts.length` + // - If all checkpoints are after `blockNumber`, low and high will converge toward `0` + // - If there are no checkpoints, low = high = 0 = ckpts.length, and the 2 properties above do hold + // At each iteration: + // - If checkpoints[mid].fromBlock is equal or after `blockNumber`, we look in [low, mid] (mid is a candidate) + // - If checkpoints[mid].fromBlock is before `blockNumber`, we look in [mid+1, high] (mid is not a candidate) + // Once we have found the first checkpoint AFTER (or equal) `blockNumber`, we get the value at the beginning of + // `blockNumber` by reading the checkpoint just before that. If there is no checkpoint before, then we return 0 + // (no value). uint256 high = ckpts.length; uint256 low = 0; while (low < high) { @@ -76,7 +86,7 @@ abstract contract ERC20Votes is IERC20Votes, ERC20Permit { } } - return low == 0 ? 0 : ckpts[low - 1].votes; + return high == 0 ? 0 : ckpts[high - 1].votes; } /** From 829fdf1b45684a0bb60f104015c802c808fa5326 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 11 May 2021 17:58:17 +0200 Subject: [PATCH 26/42] documentation for ERC20Votes --- contracts/token/ERC20/README.adoc | 4 +++ .../ERC20/extensions/draft-ERC20Votes.sol | 36 +++++++++---------- 2 files changed, 20 insertions(+), 20 deletions(-) diff --git a/contracts/token/ERC20/README.adoc b/contracts/token/ERC20/README.adoc index 16f38b75754..d203daf2fd0 100644 --- a/contracts/token/ERC20/README.adoc +++ b/contracts/token/ERC20/README.adoc @@ -21,6 +21,7 @@ Additionally there are multiple custom extensions, including: * {ERC20Snapshot}: efficient storage of past token balances to be later queried at any point in time. * {ERC20Permit}: gasless approval of tokens (standardized as ERC2612). * {ERC20FlashMint}: token level support for flash loans through the minting and burning of ephemeral tokens (standardized as ERC3156). +* {ERC20Votes}: support for voting and vote delegation (compatible with Compound's token). Finally, there are some utilities to interact with ERC20 contracts in various ways. @@ -31,6 +32,7 @@ The following related EIPs are in draft status. - {ERC20Permit} - {ERC20FlashMint} +- {ERC20Votes} NOTE: This core set of contracts is designed to be unopinionated, allowing developers to access the internal functions in ERC20 (such as <>) and expose them as external functions in the way they prefer. On the other hand, xref:ROOT:erc20.adoc#Presets[ERC20 Presets] (such as {ERC20PresetMinterPauser}) are designed using opinionated patterns to provide developers with ready to use, deployable contracts. @@ -60,6 +62,8 @@ The following EIPs are still in Draft status. Due to their nature as drafts, the {{ERC20FlashMint}} +{{ERC20Votes}} + == Presets These contracts are preconfigured combinations of the above features. They can be used through inheritance or as models to copy and paste their source code. diff --git a/contracts/token/ERC20/extensions/draft-ERC20Votes.sol b/contracts/token/ERC20/extensions/draft-ERC20Votes.sol index 0ff30fee69d..aa6e4f5f7a7 100644 --- a/contracts/token/ERC20/extensions/draft-ERC20Votes.sol +++ b/contracts/token/ERC20/extensions/draft-ERC20Votes.sol @@ -28,25 +28,29 @@ abstract contract ERC20Votes is IERC20Votes, ERC20Permit { mapping (address => address) private _delegates; mapping (address => Checkpoint[]) private _checkpoints; + /** + * @dev Get the `pos`-th checkpoint for `account`. + */ function checkpoints(address account, uint32 pos) external view virtual override returns (Checkpoint memory) { return _checkpoints[account][pos]; } + /** + * @dev Get number of checkpoints for `account`. + */ function numCheckpoints(address account) external view virtual override returns (uint32) { return SafeCast.toUint32(_checkpoints[account].length); } /** - * @dev Get the address `account` is currently delegating to. - */ + * @dev Get the address `account` is currently delegating to. + */ function delegates(address account) public view virtual override returns (address) { return _delegates[account]; } /** - * @notice Gets the current votes balance for `account` - * @param account The address to get votes balance - * @return The number of current votes for `account` + * @dev Gets the current votes balance for `account` */ function getCurrentVotes(address account) external view override returns (uint256) { uint256 pos = _checkpoints[account].length; @@ -54,11 +58,7 @@ abstract contract ERC20Votes is IERC20Votes, ERC20Permit { } /** - * @notice Determine the prior number of votes for an account as of a block number - * @dev Block number must be a finalized block or else this function will revert to prevent misinformation. - * @param account The address of the account to check - * @param blockNumber The block number to get the vote balance at - * @return The number of votes the account had as of the given block + * @dev Determine the number of votes for `account` at the begining of `blockNumber`. */ function getPriorVotes(address account, uint256 blockNumber) external view override returns (uint256) { require(blockNumber < block.number, "ERC20Votes::getPriorVotes: not yet determined"); @@ -90,21 +90,14 @@ abstract contract ERC20Votes is IERC20Votes, ERC20Permit { } /** - * @notice Delegate votes from the sender to `delegatee` - * @param delegatee The address to delegate votes to - */ + * @dev Delegate votes from the sender to `delegatee`. + */ function delegate(address delegatee) public virtual override { return _delegate(_msgSender(), delegatee); } /** - * @notice Delegates votes from signatory to `delegatee` - * @param delegatee The address to delegate votes to - * @param nonce The contract state required to match the signature - * @param expiry The time at which to expire the signature - * @param v The recovery byte of the signature - * @param r Half of the ECDSA signature pair - * @param s Half of the ECDSA signature pair + * @dev Delegates votes from signatory to `delegatee` */ function delegateBySig(address delegatee, uint256 nonce, uint256 expiry, uint8 v, bytes32 r, bytes32 s) public virtual override @@ -123,6 +116,9 @@ abstract contract ERC20Votes is IERC20Votes, ERC20Permit { return _delegate(signatory, delegatee); } + /** + * @dev Change delegation for `delegator` to `delegatee`. + */ function _delegate(address delegator, address delegatee) internal virtual { address currentDelegate = delegates(delegator); uint256 delegatorBalance = balanceOf(delegator); From 53439893c70a137e70cccec6402dfd9901bf2cc7 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 11 May 2021 19:05:16 +0200 Subject: [PATCH 27/42] try to fix batchInBlock --- test/token/ERC20/extensions/draft-ERC20Votes.test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/token/ERC20/extensions/draft-ERC20Votes.test.js b/test/token/ERC20/extensions/draft-ERC20Votes.test.js index d49af45b07f..f44b3534abd 100644 --- a/test/token/ERC20/extensions/draft-ERC20Votes.test.js +++ b/test/token/ERC20/extensions/draft-ERC20Votes.test.js @@ -29,10 +29,10 @@ async function batchInBlock (txs) { const promises = Promise.all(txs.map(fn => fn())); await send('evm_setIntervalMining', [1000]); const receipts = await promises; - await send('evm_setAutomine', [true]); await send('evm_setIntervalMining', [false]); + await send('evm_setAutomine', [true]); - expect(receipts.map(({ blockNumber }) => blockNumber).every((val, _, arr) => val === arr[0])); + expect(receipts.map(({ receipt }) => receipt.blockNumber).every((val, _, arr) => val === arr[0])); return receipts; } From ad792d65881bb693e4d91dfa9a07466c72c24e45 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 11 May 2021 19:21:52 +0200 Subject: [PATCH 28/42] try to fix batchInBlock --- test/token/ERC20/extensions/draft-ERC20Votes.test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/token/ERC20/extensions/draft-ERC20Votes.test.js b/test/token/ERC20/extensions/draft-ERC20Votes.test.js index f44b3534abd..19b32d00916 100644 --- a/test/token/ERC20/extensions/draft-ERC20Votes.test.js +++ b/test/token/ERC20/extensions/draft-ERC20Votes.test.js @@ -27,9 +27,9 @@ async function batchInBlock (txs) { await send('evm_setAutomine', [false]); const promises = Promise.all(txs.map(fn => fn())); - await send('evm_setIntervalMining', [1000]); + await (new Promise(resolve => setTimeout(resolve, 1000))); + await send('evm_mine'); const receipts = await promises; - await send('evm_setIntervalMining', [false]); await send('evm_setAutomine', [true]); expect(receipts.map(({ receipt }) => receipt.blockNumber).every((val, _, arr) => val === arr[0])); From 7fd286d836a3b5077e23f141401b3043ce6863e8 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Tue, 11 May 2021 16:22:25 -0300 Subject: [PATCH 29/42] remove setTimeout from tests --- test/token/ERC20/extensions/draft-ERC20Votes.test.js | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/test/token/ERC20/extensions/draft-ERC20Votes.test.js b/test/token/ERC20/extensions/draft-ERC20Votes.test.js index 19b32d00916..cf6d1d35104 100644 --- a/test/token/ERC20/extensions/draft-ERC20Votes.test.js +++ b/test/token/ERC20/extensions/draft-ERC20Votes.test.js @@ -4,6 +4,7 @@ const { BN, constants, expectEvent, expectRevert, time } = require('@openzeppeli const { expect } = require('chai'); const { MAX_UINT256, ZERO_ADDRESS, ZERO_BYTES32 } = constants; +const events = require('events'); const { fromRpcSig } = require('ethereumjs-util'); const ethSigUtil = require('eth-sig-util'); const Wallet = require('ethereumjs-wallet').default; @@ -23,13 +24,11 @@ function send (method, params = []) { } async function batchInBlock (txs) { - const before = await web3.eth.getBlockNumber(); - await send('evm_setAutomine', [false]); - const promises = Promise.all(txs.map(fn => fn())); - await (new Promise(resolve => setTimeout(resolve, 1000))); + const promises = txs.map(fn => fn()); + await Promise.all(promises.map(p => events.once(p, 'transactionHash'))); await send('evm_mine'); - const receipts = await promises; + const receipts = await Promise.all(promises); await send('evm_setAutomine', [true]); expect(receipts.map(({ receipt }) => receipt.blockNumber).every((val, _, arr) => val === arr[0])); From a716d7816db4bd05fb12617dcaca413014a6d372 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Tue, 11 May 2021 16:46:47 -0300 Subject: [PATCH 30/42] reenable automine in finally block --- .../ERC20/extensions/draft-ERC20Votes.test.js | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/test/token/ERC20/extensions/draft-ERC20Votes.test.js b/test/token/ERC20/extensions/draft-ERC20Votes.test.js index cf6d1d35104..3b9f5b1c53c 100644 --- a/test/token/ERC20/extensions/draft-ERC20Votes.test.js +++ b/test/token/ERC20/extensions/draft-ERC20Votes.test.js @@ -25,15 +25,19 @@ function send (method, params = []) { async function batchInBlock (txs) { await send('evm_setAutomine', [false]); - const promises = txs.map(fn => fn()); - await Promise.all(promises.map(p => events.once(p, 'transactionHash'))); - await send('evm_mine'); - const receipts = await Promise.all(promises); - await send('evm_setAutomine', [true]); - expect(receipts.map(({ receipt }) => receipt.blockNumber).every((val, _, arr) => val === arr[0])); + try { + const promises = txs.map(fn => fn()); + await Promise.all(promises.map(p => events.once(p, 'transactionHash'))); + await send('evm_mine'); + const receipts = await Promise.all(promises); - return receipts; + expect(receipts.map(({ receipt }) => receipt.blockNumber).every((val, _, arr) => val === arr[0])); + + return receipts; + } finally { + await send('evm_setAutomine', [true]); + } } contract('ERC20Votes', function (accounts) { From 72a1d09530e23232d17c69f7febe748c27b3b019 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Tue, 11 May 2021 16:47:15 -0300 Subject: [PATCH 31/42] use Set to count distinct mined blocks --- test/token/ERC20/extensions/draft-ERC20Votes.test.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/token/ERC20/extensions/draft-ERC20Votes.test.js b/test/token/ERC20/extensions/draft-ERC20Votes.test.js index 3b9f5b1c53c..980042d1d79 100644 --- a/test/token/ERC20/extensions/draft-ERC20Votes.test.js +++ b/test/token/ERC20/extensions/draft-ERC20Votes.test.js @@ -32,7 +32,8 @@ async function batchInBlock (txs) { await send('evm_mine'); const receipts = await Promise.all(promises); - expect(receipts.map(({ receipt }) => receipt.blockNumber).every((val, _, arr) => val === arr[0])); + const minedBlocks = new Set(receipts.map(({ receipt }) => receipt.blockNumber)); + expect(minedBlocks.size).to.equal(1); return receipts; } finally { From 690f43d8464cf1efdaec00b4e2a4e39a33aff03d Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Tue, 11 May 2021 22:50:17 -0300 Subject: [PATCH 32/42] use built in provider.send function --- test/token/ERC20/extensions/draft-ERC20Votes.test.js | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/test/token/ERC20/extensions/draft-ERC20Votes.test.js b/test/token/ERC20/extensions/draft-ERC20Votes.test.js index 980042d1d79..5bc83acd6d9 100644 --- a/test/token/ERC20/extensions/draft-ERC20Votes.test.js +++ b/test/token/ERC20/extensions/draft-ERC20Votes.test.js @@ -19,17 +19,13 @@ const Delegation = [ { name: 'expiry', type: 'uint256' }, ]; -function send (method, params = []) { - return new Promise(resolve => web3.currentProvider.send({ jsonrpc: '2.0', method, params }, resolve)); -} - async function batchInBlock (txs) { - await send('evm_setAutomine', [false]); + await network.provider.send('evm_setAutomine', [false]); try { const promises = txs.map(fn => fn()); await Promise.all(promises.map(p => events.once(p, 'transactionHash'))); - await send('evm_mine'); + await network.provider.send('evm_mine'); const receipts = await Promise.all(promises); const minedBlocks = new Set(receipts.map(({ receipt }) => receipt.blockNumber)); @@ -37,7 +33,7 @@ async function batchInBlock (txs) { return receipts; } finally { - await send('evm_setAutomine', [true]); + await network.provider.send('evm_setAutomine', [true]); } } From 5060eb913c7ce8c6aff712d8b57517bb9c30d48e Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Tue, 11 May 2021 22:51:04 -0300 Subject: [PATCH 33/42] fix tests --- .../ERC20/extensions/draft-ERC20Votes.test.js | 20 ++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/test/token/ERC20/extensions/draft-ERC20Votes.test.js b/test/token/ERC20/extensions/draft-ERC20Votes.test.js index 5bc83acd6d9..87dd522ca8d 100644 --- a/test/token/ERC20/extensions/draft-ERC20Votes.test.js +++ b/test/token/ERC20/extensions/draft-ERC20Votes.test.js @@ -4,11 +4,13 @@ const { BN, constants, expectEvent, expectRevert, time } = require('@openzeppeli const { expect } = require('chai'); const { MAX_UINT256, ZERO_ADDRESS, ZERO_BYTES32 } = constants; -const events = require('events'); const { fromRpcSig } = require('ethereumjs-util'); const ethSigUtil = require('eth-sig-util'); const Wallet = require('ethereumjs-wallet').default; +const { promisify } = require('util'); +const queue = promisify(setImmediate); + const ERC20VotesMock = artifacts.require('ERC20VotesMock'); const { EIP712Domain, domainSeparator } = require('../../../helpers/eip712'); @@ -19,12 +21,20 @@ const Delegation = [ { name: 'expiry', type: 'uint256' }, ]; +async function countPendingTransactions() { + return parseInt( + await network.provider.send('eth_getBlockTransactionCountByNumber', ['pending']) + ); +} + async function batchInBlock (txs) { await network.provider.send('evm_setAutomine', [false]); try { const promises = txs.map(fn => fn()); - await Promise.all(promises.map(p => events.once(p, 'transactionHash'))); + do { + await queue(); + } while (txs.length > await countPendingTransactions()); await network.provider.send('evm_mine'); const receipts = await Promise.all(promises); @@ -360,9 +370,9 @@ contract('ERC20Votes', function (accounts) { expect(await this.token.numCheckpoints(other1)).to.be.bignumber.equal('0'); const [ t1, t2, t3 ] = await batchInBlock([ - () => this.token.delegate(other1, { from: recipient }), - () => this.token.transfer(other2, 10, { from: recipient }), - () => this.token.transfer(other2, 10, { from: recipient }), + () => this.token.delegate(other1, { from: recipient, gas: 100000 }), + () => this.token.transfer(other2, 10, { from: recipient, gas: 100000 }), + () => this.token.transfer(other2, 10, { from: recipient, gas: 100000 }), ]); expect(await this.token.numCheckpoints(other1)).to.be.bignumber.equal('1'); expect(await this.token.checkpoints(other1, 0)).to.be.deep.equal([ t1.receipt.blockNumber.toString(), '80' ]); From da59c90963a164179539f4319b94e8ed279cdcec Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Tue, 11 May 2021 22:57:01 -0300 Subject: [PATCH 34/42] docs wording and whitespace --- .../token/ERC20/extensions/draft-ERC20Votes.sol | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/contracts/token/ERC20/extensions/draft-ERC20Votes.sol b/contracts/token/ERC20/extensions/draft-ERC20Votes.sol index aa6e4f5f7a7..094dfcded07 100644 --- a/contracts/token/ERC20/extensions/draft-ERC20Votes.sol +++ b/contracts/token/ERC20/extensions/draft-ERC20Votes.sol @@ -12,13 +12,13 @@ import "../../../utils/cryptography/ECDSA.sol"; * @dev Extension of the ERC20 token contract to support Compound's voting and delegation. * * This extensions keeps a history (checkpoints) of each account's vote power. Vote power can be delegated either - * by calling the {delegate} directly, or by providing a signature that can later be verified and processed using - * {delegateFromBySig}. Voting power can be checked through the public accessors {getCurrentVotes} and {getPriorVotes}. + * by calling the {delegate} function directly, or by providing a signature to be used with {delegateBySig}. Voting + * power can be queried through the public accessors {getCurrentVotes} and {getPriorVotes}. * - * By default, token balance does not account for voting power. This makes transfers cheaper. The downside is that it requires users to delegate - * to themselves in order to activate checkpoints and have their voting power tracked. Enabling self-delegation can - * easily be done by overriding the {delegates} function. Keep in mind however that this will significantly increase - * the base gas cost of transfers. + * By default, token balance does not account for voting power. This makes transfers cheaper. The downside is that it + * requires users to delegate to themselves in order to activate checkpoints and have their voting power tracked. + * Enabling self-delegation can easily be done by overriding the {delegates} function. Keep in mind however that this + * will significantly increase the base gas cost of transfers. * * _Available since v4.2._ */ From 8fbd9c44ccaf87a519f057eecd9d8f5a361c87ed Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Tue, 11 May 2021 23:08:33 -0300 Subject: [PATCH 35/42] whitespace --- contracts/token/ERC20/extensions/draft-ERC20Votes.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/token/ERC20/extensions/draft-ERC20Votes.sol b/contracts/token/ERC20/extensions/draft-ERC20Votes.sol index 094dfcded07..650fcaeef6b 100644 --- a/contracts/token/ERC20/extensions/draft-ERC20Votes.sol +++ b/contracts/token/ERC20/extensions/draft-ERC20Votes.sol @@ -100,7 +100,7 @@ abstract contract ERC20Votes is IERC20Votes, ERC20Permit { * @dev Delegates votes from signatory to `delegatee` */ function delegateBySig(address delegatee, uint256 nonce, uint256 expiry, uint8 v, bytes32 r, bytes32 s) - public virtual override + public virtual override { require(block.timestamp <= expiry, "ERC20Votes::delegateBySig: signature expired"); address signatory = ECDSA.recover( From 017fcd9bc723e64f86529c0fbab532de24bf98d0 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Tue, 11 May 2021 23:11:24 -0300 Subject: [PATCH 36/42] rename signatory -> signer --- contracts/token/ERC20/extensions/draft-ERC20Votes.sol | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/contracts/token/ERC20/extensions/draft-ERC20Votes.sol b/contracts/token/ERC20/extensions/draft-ERC20Votes.sol index 650fcaeef6b..97fe74cd8fd 100644 --- a/contracts/token/ERC20/extensions/draft-ERC20Votes.sol +++ b/contracts/token/ERC20/extensions/draft-ERC20Votes.sol @@ -97,13 +97,13 @@ abstract contract ERC20Votes is IERC20Votes, ERC20Permit { } /** - * @dev Delegates votes from signatory to `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::delegateBySig: signature expired"); - address signatory = ECDSA.recover( + address signer = ECDSA.recover( _hashTypedDataV4(keccak256(abi.encode( _DELEGATION_TYPEHASH, delegatee, @@ -112,8 +112,8 @@ abstract contract ERC20Votes is IERC20Votes, ERC20Permit { ))), v, r, s ); - require(nonce == _useNonce(signatory), "ERC20Votes::delegateBySig: invalid nonce"); - return _delegate(signatory, delegatee); + require(nonce == _useNonce(signer), "ERC20Votes::delegateBySig: invalid nonce"); + return _delegate(signer, delegatee); } /** From 5cd61c09990234c093771d0f2b918856c411ed97 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Tue, 11 May 2021 23:49:07 -0300 Subject: [PATCH 37/42] edit comment before binary search --- .../ERC20/extensions/draft-ERC20Votes.sol | 21 ++++++++++--------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/contracts/token/ERC20/extensions/draft-ERC20Votes.sol b/contracts/token/ERC20/extensions/draft-ERC20Votes.sol index 97fe74cd8fd..17d4a912764 100644 --- a/contracts/token/ERC20/extensions/draft-ERC20Votes.sol +++ b/contracts/token/ERC20/extensions/draft-ERC20Votes.sol @@ -65,16 +65,17 @@ abstract contract ERC20Votes is IERC20Votes, ERC20Permit { Checkpoint[] storage ckpts = _checkpoints[account]; - // Property: low and high converge on the first (earliest) checkpoint that is AFTER (or equal) `blockNumber`. - // - If all checkpoints are before `blockNumber`, low and high converge toward `ckpts.length` - // - If all checkpoints are after `blockNumber`, low and high will converge toward `0` - // - If there are no checkpoints, low = high = 0 = ckpts.length, and the 2 properties above do hold - // At each iteration: - // - If checkpoints[mid].fromBlock is equal or after `blockNumber`, we look in [low, mid] (mid is a candidate) - // - If checkpoints[mid].fromBlock is before `blockNumber`, we look in [mid+1, high] (mid is not a candidate) - // Once we have found the first checkpoint AFTER (or equal) `blockNumber`, we get the value at the beginning of - // `blockNumber` by reading the checkpoint just before that. If there is no checkpoint before, then we return 0 - // (no value). + // We run a binary search to look for the earliest checkpoint taken after `blockNumber`. + // + // During the loop, the index of the wanted checkpoint remains in the range [low, 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 `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 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 the earliest checkpoint after `blockNumber`, but it + // works out the same. uint256 high = ckpts.length; uint256 low = 0; while (low < high) { From 88aa1bb5bd7fff1d97e3204e3aa34ea35d12435a Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Tue, 11 May 2021 23:51:58 -0300 Subject: [PATCH 38/42] typo --- contracts/token/ERC20/extensions/draft-ERC20Votes.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/token/ERC20/extensions/draft-ERC20Votes.sol b/contracts/token/ERC20/extensions/draft-ERC20Votes.sol index 17d4a912764..7172bf4ffee 100644 --- a/contracts/token/ERC20/extensions/draft-ERC20Votes.sol +++ b/contracts/token/ERC20/extensions/draft-ERC20Votes.sol @@ -72,7 +72,7 @@ abstract contract ERC20Votes is IERC20Votes, ERC20Permit { // - If the middle checkpoint is after `blockNumber`, we look in [low, mid) // - If the middle checkpoint is before `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 we're looking too far in the past and the result is 0). + // 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 the earliest checkpoint after `blockNumber`, but it // works out the same. From ea6601016b93c3679d817370e42f7f981e7a0c3a Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Wed, 12 May 2021 00:34:41 -0300 Subject: [PATCH 39/42] wording --- contracts/token/ERC20/extensions/draft-ERC20Votes.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/token/ERC20/extensions/draft-ERC20Votes.sol b/contracts/token/ERC20/extensions/draft-ERC20Votes.sol index 7172bf4ffee..7b0ac2ecb4b 100644 --- a/contracts/token/ERC20/extensions/draft-ERC20Votes.sol +++ b/contracts/token/ERC20/extensions/draft-ERC20Votes.sol @@ -74,8 +74,8 @@ abstract contract ERC20Votes is IERC20Votes, ERC20Permit { // 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 the earliest checkpoint after `blockNumber`, but it - // works out the same. + // past the end of the array, so we technically don't find a checkpoint after `blockNumber`, but it works out + // the same. uint256 high = ckpts.length; uint256 low = 0; while (low < high) { From 63dc2477499771f118359b2ceb2de5de99698aa4 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 12 May 2021 08:53:58 +0200 Subject: [PATCH 40/42] add uint224 check on ERC20Votes total supply --- .../ERC20/extensions/draft-ERC20Votes.sol | 33 +++++++++++-------- .../ERC20/extensions/draft-ERC20Votes.test.js | 8 +++++ 2 files changed, 27 insertions(+), 14 deletions(-) diff --git a/contracts/token/ERC20/extensions/draft-ERC20Votes.sol b/contracts/token/ERC20/extensions/draft-ERC20Votes.sol index 7b0ac2ecb4b..787fa2c0273 100644 --- a/contracts/token/ERC20/extensions/draft-ERC20Votes.sol +++ b/contracts/token/ERC20/extensions/draft-ERC20Votes.sol @@ -127,23 +127,23 @@ abstract contract ERC20Votes is IERC20Votes, ERC20Permit { emit DelegateChanged(delegator, currentDelegate, delegatee); - _moveDelegates(currentDelegate, delegatee, delegatorBalance); + _moveVotingPower(currentDelegate, delegatee, delegatorBalance); } - function _moveDelegates(address srcRep, address dstRep, uint256 amount) private { - if (srcRep != dstRep && amount > 0) { - if (srcRep != address(0)) { - uint256 srcRepNum = _checkpoints[srcRep].length; - uint256 srcRepOld = srcRepNum == 0 ? 0 : _checkpoints[srcRep][srcRepNum - 1].votes; - uint256 srcRepNew = srcRepOld - amount; - _writeCheckpoint(srcRep, srcRepNum, srcRepOld, srcRepNew); + function _moveVotingPower(address src, address dst, uint256 amount) private { + if (src != dst && amount > 0) { + if (src != address(0)) { + uint256 srcNum = _checkpoints[src].length; + uint256 srcOld = srcNum == 0 ? 0 : _checkpoints[src][srcNum - 1].votes; + uint256 srcNew = srcOld - amount; + _writeCheckpoint(src, srcNum, srcOld, srcNew); } - if (dstRep != address(0)) { - uint256 dstRepNum = _checkpoints[dstRep].length; - uint256 dstRepOld = dstRepNum == 0 ? 0 : _checkpoints[dstRep][dstRepNum - 1].votes; - uint256 dstRepNew = dstRepOld + amount; - _writeCheckpoint(dstRep, dstRepNum, dstRepOld, dstRepNew); + if (dst != address(0)) { + uint256 dstNum = _checkpoints[dst].length; + uint256 dstOld = dstNum == 0 ? 0 : _checkpoints[dst][dstNum - 1].votes; + uint256 dstNew = dstOld + amount; + _writeCheckpoint(dst, dstNum, dstOld, dstNew); } } } @@ -161,7 +161,12 @@ abstract contract ERC20Votes is IERC20Votes, ERC20Permit { emit DelegateVotesChanged(delegatee, oldWeight, newWeight); } + function _mint(address account, uint256 amount) internal virtual override { + super._mint(account, amount); + require(totalSupply() <= type(uint224).max, "ERC20Votes: total supply exceeds 2**224"); + } + function _beforeTokenTransfer(address from, address to, uint256 amount) internal virtual override { - _moveDelegates(delegates(from), delegates(to), amount); + _moveVotingPower(delegates(from), delegates(to), amount); } } diff --git a/test/token/ERC20/extensions/draft-ERC20Votes.test.js b/test/token/ERC20/extensions/draft-ERC20Votes.test.js index 87dd522ca8d..f6b6f6af4a6 100644 --- a/test/token/ERC20/extensions/draft-ERC20Votes.test.js +++ b/test/token/ERC20/extensions/draft-ERC20Votes.test.js @@ -77,6 +77,14 @@ contract('ERC20Votes', function (accounts) { ); }); + it('minting restriction', async function () { + const amount = new BN('2').pow(new BN('224')); + await expectRevert( + ERC20VotesMock.new(name, symbol, holder, amount), + 'ERC20Votes: total supply exceeds 2**224', + ); + }); + describe('set delegation', function () { describe('call', function () { it('delegation with balance', async function () { From 55ecf72cd78c6ac856e06b64da3bd3ce18d73491 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 12 May 2021 12:22:15 +0200 Subject: [PATCH 41/42] Rename vars for clarity --- .../token/ERC20/extensions/draft-ERC20Votes.sol | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/contracts/token/ERC20/extensions/draft-ERC20Votes.sol b/contracts/token/ERC20/extensions/draft-ERC20Votes.sol index 787fa2c0273..d4b3a094ee3 100644 --- a/contracts/token/ERC20/extensions/draft-ERC20Votes.sol +++ b/contracts/token/ERC20/extensions/draft-ERC20Votes.sol @@ -133,17 +133,17 @@ abstract contract ERC20Votes is IERC20Votes, ERC20Permit { function _moveVotingPower(address src, address dst, uint256 amount) private { if (src != dst && amount > 0) { if (src != address(0)) { - uint256 srcNum = _checkpoints[src].length; - uint256 srcOld = srcNum == 0 ? 0 : _checkpoints[src][srcNum - 1].votes; - uint256 srcNew = srcOld - amount; - _writeCheckpoint(src, srcNum, srcOld, srcNew); + uint256 srcCkptLen = _checkpoints[src].length; + uint256 srcCkptOld = srcCkptLen == 0 ? 0 : _checkpoints[src][srcCkptLen - 1].votes; + uint256 srcCkptNew = srcCkptOld - amount; + _writeCheckpoint(src, srcCkptLen, srcCkptOld, srcCkptNew); } if (dst != address(0)) { - uint256 dstNum = _checkpoints[dst].length; - uint256 dstOld = dstNum == 0 ? 0 : _checkpoints[dst][dstNum - 1].votes; - uint256 dstNew = dstOld + amount; - _writeCheckpoint(dst, dstNum, dstOld, dstNew); + uint256 dstCkptLen = _checkpoints[dst].length; + uint256 dstCkptOld = dstCkptLen == 0 ? 0 : _checkpoints[dst][dstCkptLen - 1].votes; + uint256 dstCkptNew = dstCkptOld + amount; + _writeCheckpoint(dst, dstCkptLen, dstCkptOld, dstCkptNew); } } } From 861f07fcefbb785667097c22427668d72035898a Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 12 May 2021 12:52:36 +0200 Subject: [PATCH 42/42] document batchInBlock --- .../ERC20/extensions/draft-ERC20Votes.test.js | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/test/token/ERC20/extensions/draft-ERC20Votes.test.js b/test/token/ERC20/extensions/draft-ERC20Votes.test.js index f6b6f6af4a6..6daf35f3699 100644 --- a/test/token/ERC20/extensions/draft-ERC20Votes.test.js +++ b/test/token/ERC20/extensions/draft-ERC20Votes.test.js @@ -28,21 +28,26 @@ async function countPendingTransactions() { } async function batchInBlock (txs) { - await network.provider.send('evm_setAutomine', [false]); - try { + // disable auto-mining + await network.provider.send('evm_setAutomine', [false]); + // send all transactions const promises = txs.map(fn => fn()); - do { + // wait for node to have all pending transactions + while (txs.length > await countPendingTransactions()) { await queue(); - } while (txs.length > await countPendingTransactions()); + } + // mine one block await network.provider.send('evm_mine'); + // fetch receipts const receipts = await Promise.all(promises); - + // Sanity check, all tx should be in the same block const minedBlocks = new Set(receipts.map(({ receipt }) => receipt.blockNumber)); expect(minedBlocks.size).to.equal(1); return receipts; } finally { + // enable auto-mining await network.provider.send('evm_setAutomine', [true]); } }