From 450535ede0a553b98c9d8285bfb7bfc9d7999576 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 24 Mar 2021 22:07:18 +0100 Subject: [PATCH] improve snapshoting --- .../token/ERC20/extensions/ERC20Snapshot.sol | 21 --- .../extensions/ERC20SnapshotEveryBlock.sol | 6 +- hardhat.config.js | 2 +- .../ERC20SnapshotEveryBlock.test.js | 133 ++++++++---------- 4 files changed, 67 insertions(+), 95 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..5fb028f93f3 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,10 @@ abstract contract ERC20SnapshotEveryBlock is ERC20 { function _valueAt(uint256 snapshotId, Snapshots storage snapshots) private view returns (bool, uint256) { + require(snapshotId > 0, "ERC20Snapshot: id is 0"); + // snapshotId == _getCurrentSnapshotId() + 1 <=> before the next one <=> current value + require(snapshotId <= _getCurrentSnapshotId() + 1, "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..65203043a79 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,117 +33,91 @@ 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 () { + it('reverts with a snapshot id of 0', async function () { + await expectRevert(this.token.totalSupplyAt(0), 'ERC20Snapshot: id is 0'); + }); + 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('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 () { + expect(await this.token.totalSupplyAt(this.initialBlock + 1)).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(); + 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.blockNumberBefore)).to.be.bignumber.equal(initialSupply); + expect(await this.token.totalSupplyAt(this.operationBlockNumber)) + .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(), - ); + expect(await this.token.totalSupplyAt(this.operationBlockNumber + 1)) + .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); - }); - }); - - 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('reverts with a snapshot id of 0', async function () { + await expectRevert(this.token.balanceOfAt(initialHolder, 0), 'ERC20Snapshot: id is 0'); + }); - 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)) + context('with initial snapshot', function () { + 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('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('snapshot after initial supply', async function () { + expect(await this.token.balanceOfAt(initialHolder, this.initialBlock + 1)) + .to.be.bignumber.equal(initialSupply); }); }); - context('with multiple transfers in the same block', function () { + context('with balance changes', 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 batchInBlock([ + () => 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); + this.operationBlockNumber = await web3.eth.getBlockNumber(); }); it('returns the balances before the changes', async function () { - expect(await this.token.balanceOfAt(initialHolder, this.blockNumberBefore)) + expect(await this.token.balanceOfAt(initialHolder, this.operationBlockNumber)) .to.be.bignumber.equal(initialSupply); - expect(await this.token.balanceOfAt(recipient, this.blockNumberBefore)) + expect(await this.token.balanceOfAt(recipient, this.operationBlockNumber)) .to.be.bignumber.equal('0'); - expect(await this.token.balanceOfAt(other, this.blockNumberBefore)) + expect(await this.token.balanceOfAt(other, this.operationBlockNumber)) .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)); + 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'); }); }); });