From 41768528cf9f7d27af625635912e49837fec8fd8 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 28 Jul 2022 11:46:53 +0200 Subject: [PATCH 01/46] move mock --- contracts/mocks/{CheckpointsImpl.sol => CheckpointsMock.sol} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename contracts/mocks/{CheckpointsImpl.sol => CheckpointsMock.sol} (100%) diff --git a/contracts/mocks/CheckpointsImpl.sol b/contracts/mocks/CheckpointsMock.sol similarity index 100% rename from contracts/mocks/CheckpointsImpl.sol rename to contracts/mocks/CheckpointsMock.sol From 63edb741cc3cdf49babcf9d774656787b538498e Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 28 Jul 2022 15:42:59 +0200 Subject: [PATCH 02/46] Expand Checkpoint library --- .../GovernorVotesQuorumFraction.sol | 6 +- contracts/mocks/CheckpointsMock.sol | 68 +++++- contracts/utils/Checkpoints.sol | 200 +++++++++++++++--- scripts/generate/run.js | 2 + scripts/generate/templates/Checkpoints.js | 167 +++++++++++++++ scripts/generate/templates/CheckpointsMock.js | 73 +++++++ scripts/generate/templates/SafeCast.js | 4 +- scripts/generate/templates/SafeCastMock.js | 4 +- test/utils/Checkpoints.test.js | 168 ++++++++++----- 9 files changed, 602 insertions(+), 90 deletions(-) create mode 100644 scripts/generate/templates/Checkpoints.js create mode 100755 scripts/generate/templates/CheckpointsMock.js diff --git a/contracts/governance/extensions/GovernorVotesQuorumFraction.sol b/contracts/governance/extensions/GovernorVotesQuorumFraction.sol index e5f0888c043..7779811de8a 100644 --- a/contracts/governance/extensions/GovernorVotesQuorumFraction.sol +++ b/contracts/governance/extensions/GovernorVotesQuorumFraction.sol @@ -50,8 +50,8 @@ abstract contract GovernorVotesQuorumFraction is GovernorVotes { } // Optimistic search, check the latest checkpoint - Checkpoints.Checkpoint memory latest = _quorumNumeratorHistory._checkpoints[length - 1]; - if (latest._blockNumber <= blockNumber) { + Checkpoints.Checkpoint224 memory latest = _quorumNumeratorHistory._checkpoints[length - 1]; + if (latest._key <= blockNumber) { return latest._value; } @@ -107,7 +107,7 @@ abstract contract GovernorVotesQuorumFraction is GovernorVotes { // Make sure we keep track of the original numerator in contracts upgraded from a version without checkpoints. if (oldQuorumNumerator != 0 && _quorumNumeratorHistory._checkpoints.length == 0) { _quorumNumeratorHistory._checkpoints.push( - Checkpoints.Checkpoint({_blockNumber: 0, _value: SafeCast.toUint224(oldQuorumNumerator)}) + Checkpoints.Checkpoint224({_key: 0, _value: SafeCast.toUint224(oldQuorumNumerator)}) ); } diff --git a/contracts/mocks/CheckpointsMock.sol b/contracts/mocks/CheckpointsMock.sol index 14681ca40b9..f59a11e994d 100644 --- a/contracts/mocks/CheckpointsMock.sol +++ b/contracts/mocks/CheckpointsMock.sol @@ -4,7 +4,7 @@ pragma solidity ^0.8.0; import "../utils/Checkpoints.sol"; -contract CheckpointsImpl { +contract CheckpointsMock { using Checkpoints for Checkpoints.History; Checkpoints.History private _totalCheckpoints; @@ -13,15 +13,75 @@ contract CheckpointsImpl { return _totalCheckpoints.latest(); } + function push(uint256 value) public returns (uint256, uint256) { + return _totalCheckpoints.push(value); + } + function getAtBlock(uint256 blockNumber) public view returns (uint256) { return _totalCheckpoints.getAtBlock(blockNumber); } - function push(uint256 value) public returns (uint256, uint256) { - return _totalCheckpoints.push(value); + function length() public view returns (uint256) { + return _totalCheckpoints._checkpoints.length; + } +} + +contract Checkpoints224Mock { + using Checkpoints for Checkpoints.Checkpoint224[]; + + Checkpoints.Checkpoint224[] private _totalCheckpoints; + + function latest() public view returns (uint224) { + return _totalCheckpoints.latest(); + } + + function push(uint32 key, uint224 value) public returns (uint224, uint224) { + return _totalCheckpoints.push(key, value); + } + + function lowerLookup(uint32 key) public view returns (uint224) { + return _totalCheckpoints.lowerLookup(key); + } + + function upperLookup(uint32 key) public view returns (uint224) { + return _totalCheckpoints.upperLookup(key); + } + + function upperLookupExpEnd(uint32 key) public view returns (uint224) { + return _totalCheckpoints.upperLookupExpEnd(key); } function length() public view returns (uint256) { - return _totalCheckpoints._checkpoints.length; + return _totalCheckpoints.length; + } +} + +contract Checkpoints160Mock { + using Checkpoints for Checkpoints.Checkpoint160[]; + + Checkpoints.Checkpoint160[] private _totalCheckpoints; + + function latest() public view returns (uint160) { + return _totalCheckpoints.latest(); + } + + function push(uint96 key, uint160 value) public returns (uint160, uint160) { + return _totalCheckpoints.push(key, value); + } + + function lowerLookup(uint96 key) public view returns (uint160) { + return _totalCheckpoints.lowerLookup(key); + } + + function upperLookup(uint96 key) public view returns (uint160) { + return _totalCheckpoints.upperLookup(key); + } + + function upperLookupExpEnd(uint96 key) public view returns (uint224) { + return _totalCheckpoints.upperLookupExpEnd(key); + } + + function length() public view returns (uint256) { + return _totalCheckpoints.length; } } diff --git a/contracts/utils/Checkpoints.sol b/contracts/utils/Checkpoints.sol index 606098bcc27..936b53ed9d5 100644 --- a/contracts/utils/Checkpoints.sol +++ b/contracts/utils/Checkpoints.sol @@ -1,5 +1,6 @@ // SPDX-License-Identifier: MIT // OpenZeppelin Contracts (last updated v4.5.0) (utils/Checkpoints.sol) + pragma solidity ^0.8.0; import "./math/Math.sol"; @@ -15,21 +16,187 @@ import "./math/SafeCast.sol"; * _Available since v4.5._ */ library Checkpoints { - struct Checkpoint { - uint32 _blockNumber; + struct Checkpoint224 { + uint32 _key; uint224 _value; } + function latest(Checkpoint224[] storage self) internal view returns (uint224) { + uint256 pos = self.length; + return pos == 0 ? 0 : self[pos - 1]._value; + } + + function push( + Checkpoint224[] storage self, + uint32 key, + uint224 value + ) internal returns (uint224, uint224) { + uint256 pos = self.length; + uint224 old = latest(self); + if (pos > 0 && self[pos - 1]._key == key) { + self[pos - 1]._value = value; + } else { + self.push(Checkpoint224({_key: key, _value: value})); + } + return (old, value); + } + + function lowerLookup(Checkpoint224[] storage self, uint32 key) internal view returns (uint224) { + uint256 length = self.length; + uint256 pos = _lowerDichotomicLookup(self, key, 0, length); + return pos == length ? 0 : self[pos]._value; + } + + function upperLookup(Checkpoint224[] storage self, uint32 key) internal view returns (uint224) { + uint256 length = self.length; + uint256 pos = _upperDichotomicLookup(self, key, 0, length); + return pos == 0 ? 0 : self[pos - 1]._value; + } + + function upperLookupExpEnd(Checkpoint224[] storage self, uint32 key) internal view returns (uint224) { + uint256 length = self.length; + uint256 offset = 1; + + while (offset <= length && self[length - offset]._key > key) { + offset <<= 1; + } + + uint256 low = offset < length ? length - offset : 0; + uint256 high = length - (offset >> 1); + uint256 pos = _upperDichotomicLookup(self, key, low, high); + + return pos == 0 ? 0 : self[pos - 1]._value; + } + + function _upperDichotomicLookup( + Checkpoint224[] storage self, + uint32 key, + uint256 low, + uint256 high + ) private view returns (uint256) { + while (low < high) { + uint256 mid = Math.average(low, high); + if (self[mid]._key > key) { + high = mid; + } else { + low = mid + 1; + } + } + return high; + } + + function _lowerDichotomicLookup( + Checkpoint224[] storage self, + uint32 key, + uint256 low, + uint256 high + ) private view returns (uint256) { + while (low < high) { + uint256 mid = Math.average(low, high); + if (self[mid]._key < key) { + low = mid + 1; + } else { + high = mid; + } + } + return high; + } + + struct Checkpoint160 { + uint96 _key; + uint160 _value; + } + + function latest(Checkpoint160[] storage self) internal view returns (uint160) { + uint256 pos = self.length; + return pos == 0 ? 0 : self[pos - 1]._value; + } + + function push( + Checkpoint160[] storage self, + uint96 key, + uint160 value + ) internal returns (uint160, uint160) { + uint256 pos = self.length; + uint160 old = latest(self); + if (pos > 0 && self[pos - 1]._key == key) { + self[pos - 1]._value = value; + } else { + self.push(Checkpoint160({_key: key, _value: value})); + } + return (old, value); + } + + function lowerLookup(Checkpoint160[] storage self, uint96 key) internal view returns (uint160) { + uint256 length = self.length; + uint256 pos = _lowerDichotomicLookup(self, key, 0, length); + return pos == length ? 0 : self[pos]._value; + } + + function upperLookup(Checkpoint160[] storage self, uint96 key) internal view returns (uint160) { + uint256 length = self.length; + uint256 pos = _upperDichotomicLookup(self, key, 0, length); + return pos == 0 ? 0 : self[pos - 1]._value; + } + + function upperLookupExpEnd(Checkpoint160[] storage self, uint96 key) internal view returns (uint224) { + uint256 length = self.length; + uint256 offset = 1; + + while (offset <= length && self[length - offset]._key > key) { + offset <<= 1; + } + + uint256 low = offset < length ? length - offset : 0; + uint256 high = length - (offset >> 1); + uint256 pos = _upperDichotomicLookup(self, key, low, high); + + return pos == 0 ? 0 : self[pos - 1]._value; + } + + function _upperDichotomicLookup( + Checkpoint160[] storage self, + uint96 key, + uint256 low, + uint256 high + ) private view returns (uint256) { + while (low < high) { + uint256 mid = Math.average(low, high); + if (self[mid]._key > key) { + high = mid; + } else { + low = mid + 1; + } + } + return high; + } + + function _lowerDichotomicLookup( + Checkpoint160[] storage self, + uint96 key, + uint256 low, + uint256 high + ) private view returns (uint256) { + while (low < high) { + uint256 mid = Math.average(low, high); + if (self[mid]._key < key) { + low = mid + 1; + } else { + high = mid; + } + } + return high; + } + struct History { - Checkpoint[] _checkpoints; + Checkpoint224[] _checkpoints; } /** * @dev Returns the value in the latest checkpoint, or zero if there are no checkpoints. */ function latest(History storage self) internal view returns (uint256) { - uint256 pos = self._checkpoints.length; - return pos == 0 ? 0 : self._checkpoints[pos - 1]._value; + return latest(self._checkpoints); } /** @@ -39,17 +206,7 @@ library Checkpoints { function getAtBlock(History storage self, uint256 blockNumber) internal view returns (uint256) { require(blockNumber < block.number, "Checkpoints: block not yet mined"); - uint256 high = self._checkpoints.length; - uint256 low = 0; - while (low < high) { - uint256 mid = Math.average(low, high); - if (self._checkpoints[mid]._blockNumber > blockNumber) { - high = mid; - } else { - low = mid + 1; - } - } - return high == 0 ? 0 : self._checkpoints[high - 1]._value; + return upperLookupExpEnd(self._checkpoints, SafeCast.toUint32(blockNumber)); } /** @@ -58,16 +215,7 @@ library Checkpoints { * Returns previous value and new value. */ function push(History storage self, uint256 value) internal returns (uint256, uint256) { - uint256 pos = self._checkpoints.length; - uint256 old = latest(self); - if (pos > 0 && self._checkpoints[pos - 1]._blockNumber == block.number) { - self._checkpoints[pos - 1]._value = SafeCast.toUint224(value); - } else { - self._checkpoints.push( - Checkpoint({_blockNumber: SafeCast.toUint32(block.number), _value: SafeCast.toUint224(value)}) - ); - } - return (old, value); + return push(self._checkpoints, SafeCast.toUint32(block.number), SafeCast.toUint224(value)); } /** diff --git a/scripts/generate/run.js b/scripts/generate/run.js index 0072653d0c8..bd4fd34e6b9 100755 --- a/scripts/generate/run.js +++ b/scripts/generate/run.js @@ -14,6 +14,8 @@ function getVersion (path) { } for (const [ file, template ] of Object.entries({ + 'utils/Checkpoints.sol': './templates/Checkpoints', + 'mocks/CheckpointsMock.sol': './templates/CheckpointsMock', 'utils/math/SafeCast.sol': './templates/SafeCast', 'mocks/SafeCastMock.sol': './templates/SafeCastMock', })) { diff --git a/scripts/generate/templates/Checkpoints.js b/scripts/generate/templates/Checkpoints.js new file mode 100644 index 00000000000..30126481ef2 --- /dev/null +++ b/scripts/generate/templates/Checkpoints.js @@ -0,0 +1,167 @@ +const format = require('../format-lines'); + +const LENGTHS = [ 224, 160 ]; + +const header = `\ +pragma solidity ^0.8.0; + +import "./math/Math.sol"; +import "./math/SafeCast.sol"; + +/** + * @dev This library defines the \`History\` struct, for checkpointing values as they change at different points in + * time, and later looking up past values by block number. See {Votes} as an example. + * + * To create a history of checkpoints define a variable type \`Checkpoints.History\` in your contract, and store a new + * checkpoint for the current transaction block using the {push} function. + * + * _Available since v4.5._ + */ +`; + +const legacy = () => `\ +struct History { + Checkpoint224[] _checkpoints; +} + +/** + * @dev Returns the value in the latest checkpoint, or zero if there are no checkpoints. + */ +function latest(History storage self) internal view returns (uint256) { + return latest(self._checkpoints); +} + +/** + * @dev Returns the value at a given block number. If a checkpoint is not available at that block, the closest one + * before it is returned, or zero otherwise. + */ +function getAtBlock(History storage self, uint256 blockNumber) internal view returns (uint256) { + require(blockNumber < block.number, "Checkpoints: block not yet mined"); + + return upperLookupExpEnd(self._checkpoints, SafeCast.toUint32(blockNumber)); +} + +/** + * @dev Pushes a value onto a History so that it is stored as the checkpoint for the current block. + * + * Returns previous value and new value. + */ +function push(History storage self, uint256 value) internal returns (uint256, uint256) { + return push(self._checkpoints, SafeCast.toUint32(block.number), SafeCast.toUint224(value)); +} + +/** + * @dev Pushes a value onto a History, by updating the latest value using binary operation \`op\`. The new value will + * be set to \`op(latest, delta)\`. + * + * Returns previous value and new value. + */ +function push( + History storage self, + function(uint256, uint256) view returns (uint256) op, + uint256 delta +) internal returns (uint256, uint256) { + return push(self, op(latest(self), delta)); +} +`; + +/* eslint-disable max-len */ +const checkpoint = length => `\ +struct Checkpoint${length} { + uint${256 - length} _key; + uint${length} _value; +} + +function latest(Checkpoint${length}[] storage self) internal view returns (uint${length}) { + uint256 pos = self.length; + return pos == 0 ? 0 : self[pos - 1]._value; +} + +function push( + Checkpoint${length}[] storage self, + uint${256 - length} key, + uint${length} value +) internal returns (uint${length}, uint${length}) { + uint256 pos = self.length; + uint${length} old = latest(self); + if (pos > 0 && self[pos - 1]._key == key) { + self[pos - 1]._value = value; + } else { + self.push(Checkpoint${length}({_key: key, _value: value})); + } + return (old, value); +} + +function lowerLookup(Checkpoint${length}[] storage self, uint${256 - length} key) internal view returns (uint${length}) { + uint256 length = self.length; + uint256 pos = _lowerDichotomicLookup(self, key, 0, length); + return pos == length ? 0 : self[pos]._value; +} + +function upperLookup(Checkpoint${length}[] storage self, uint${256 - length} key) internal view returns (uint${length}) { + uint256 length = self.length; + uint256 pos = _upperDichotomicLookup(self, key, 0, length); + return pos == 0 ? 0 : self[pos - 1]._value; +} + +function upperLookupExpEnd(Checkpoint${length}[] storage self, uint${256 - length} key) internal view returns (uint224) { + uint256 length = self.length; + uint256 offset = 1; + + while (offset <= length && self[length - offset]._key > key) { + offset <<= 1; + } + + uint256 low = offset < length ? length - offset : 0; + uint256 high = length - (offset >> 1); + uint256 pos = _upperDichotomicLookup(self, key, low, high); + + return pos == 0 ? 0 : self[pos - 1]._value; +} + +function _upperDichotomicLookup( + Checkpoint${length}[] storage self, + uint${256 - length} key, + uint256 low, + uint256 high +) private view returns (uint256) { + while (low < high) { + uint256 mid = Math.average(low, high); + if (self[mid]._key > key) { + high = mid; + } else { + low = mid + 1; + } + } + return high; +} + +function _lowerDichotomicLookup( + Checkpoint${length}[] storage self, + uint${256 - length} key, + uint256 low, + uint256 high +) private view returns (uint256) { + while (low < high) { + uint256 mid = Math.average(low, high); + if (self[mid]._key < key) { + low = mid + 1; + } else { + high = mid; + } + } + return high; +} +`; +/* eslint-enable max-len */ + +// GENERATE +module.exports = format( + header.trimEnd(), + 'library Checkpoints {', + [ + ...LENGTHS.map(checkpoint), + legacy().trimEnd(), + ], + '}', +); diff --git a/scripts/generate/templates/CheckpointsMock.js b/scripts/generate/templates/CheckpointsMock.js new file mode 100755 index 00000000000..72079293429 --- /dev/null +++ b/scripts/generate/templates/CheckpointsMock.js @@ -0,0 +1,73 @@ +const format = require('../format-lines'); + +const LENGTHS = [ 224, 160 ]; + +const header = `\ +pragma solidity ^0.8.0; + +import "../utils/Checkpoints.sol"; +`; + +const legacy = () => `\ +contract CheckpointsMock { + using Checkpoints for Checkpoints.History; + + Checkpoints.History private _totalCheckpoints; + + function latest() public view returns (uint256) { + return _totalCheckpoints.latest(); + } + + function push(uint256 value) public returns (uint256, uint256) { + return _totalCheckpoints.push(value); + } + + function getAtBlock(uint256 blockNumber) public view returns (uint256) { + return _totalCheckpoints.getAtBlock(blockNumber); + } + + function length() public view returns (uint256) { + return _totalCheckpoints._checkpoints.length; + } +} +`; + +const checkpoint = length => `\ +contract Checkpoints${length}Mock { + using Checkpoints for Checkpoints.Checkpoint${length}[]; + + Checkpoints.Checkpoint${length}[] private _totalCheckpoints; + + function latest() public view returns (uint${length}) { + return _totalCheckpoints.latest(); + } + + function push(uint${256 - length} key, uint${length} value) public returns (uint${length}, uint${length}) { + return _totalCheckpoints.push(key, value); + } + + function lowerLookup(uint${256 - length} key) public view returns (uint${length}) { + return _totalCheckpoints.lowerLookup(key); + } + + function upperLookup(uint${256 - length} key) public view returns (uint${length}) { + return _totalCheckpoints.upperLookup(key); + } + + function upperLookupExpEnd(uint${256 - length} key) public view returns (uint224) { + return _totalCheckpoints.upperLookupExpEnd(key); + } + + function length() public view returns (uint256) { + return _totalCheckpoints.length; + } +} +`; + +// GENERATE +module.exports = format( + header, + legacy(), + ...LENGTHS.map(checkpoint), + // ].flatMap(fn => fn.split('\n')).slice(0, -1), +); diff --git a/scripts/generate/templates/SafeCast.js b/scripts/generate/templates/SafeCast.js index 4792d410b2d..87799e80e4b 100755 --- a/scripts/generate/templates/SafeCast.js +++ b/scripts/generate/templates/SafeCast.js @@ -159,9 +159,9 @@ module.exports = format( header.trimEnd(), 'library SafeCast {', [ - ...LENGTHS.map(size => toUintDownCast(size)), + ...LENGTHS.map(toUintDownCast), toUint(256), - ...LENGTHS.map(size => toIntDownCast(size)), + ...LENGTHS.map(toIntDownCast), toInt(256).trimEnd(), ], '}', diff --git a/scripts/generate/templates/SafeCastMock.js b/scripts/generate/templates/SafeCastMock.js index 9bb64d2c766..196d9b4f100 100755 --- a/scripts/generate/templates/SafeCastMock.js +++ b/scripts/generate/templates/SafeCastMock.js @@ -42,9 +42,9 @@ module.exports = format( 'using SafeCast for int256;', '', toUint(256), - ...LENGTHS.map(size => toUintDownCast(size)), + ...LENGTHS.map(toUintDownCast), toInt(256), - ...LENGTHS.map(size => toIntDownCast(size)), + ...LENGTHS.map(toIntDownCast), ].flatMap(fn => fn.split('\n')).slice(0, -1), '}', ); diff --git a/test/utils/Checkpoints.test.js b/test/utils/Checkpoints.test.js index 9938dc35b04..de10b388088 100644 --- a/test/utils/Checkpoints.test.js +++ b/test/utils/Checkpoints.test.js @@ -4,71 +4,133 @@ const { expect } = require('chai'); const { batchInBlock } = require('../helpers/txpool'); -const CheckpointsImpl = artifacts.require('CheckpointsImpl'); +const CheckpointsMock = artifacts.require('CheckpointsMock'); + +const first = (...args) => args.length ? args[0] : undefined; +const last = (...args) => args.length ? args[args.length - 1] : undefined; contract('Checkpoints', function (accounts) { - beforeEach(async function () { - this.checkpoint = await CheckpointsImpl.new(); - }); + describe('History checkpoints', function () { + beforeEach(async function () { + this.checkpoint = await CheckpointsMock.new(); + }); - describe('without checkpoints', function () { - it('returns zero as latest value', async function () { - expect(await this.checkpoint.latest()).to.be.bignumber.equal('0'); + describe('without checkpoints', function () { + it('returns zero as latest value', async function () { + expect(await this.checkpoint.latest()).to.be.bignumber.equal('0'); + }); + + it('returns zero as past value', async function () { + await time.advanceBlock(); + expect(await this.checkpoint.getAtBlock(await web3.eth.getBlockNumber() - 1)).to.be.bignumber.equal('0'); + }); }); - it('returns zero as past value', async function () { - await time.advanceBlock(); - expect(await this.checkpoint.getAtBlock(await web3.eth.getBlockNumber() - 1)).to.be.bignumber.equal('0'); + describe('with checkpoints', function () { + beforeEach('pushing checkpoints', async function () { + this.tx1 = await this.checkpoint.push(1); + this.tx2 = await this.checkpoint.push(2); + await time.advanceBlock(); + this.tx3 = await this.checkpoint.push(3); + await time.advanceBlock(); + await time.advanceBlock(); + }); + + it('returns latest value', async function () { + expect(await this.checkpoint.latest()).to.be.bignumber.equal('3'); + }); + + it('returns past values', async function () { + expect(await this.checkpoint.getAtBlock(this.tx1.receipt.blockNumber - 1)).to.be.bignumber.equal('0'); + expect(await this.checkpoint.getAtBlock(this.tx1.receipt.blockNumber)).to.be.bignumber.equal('1'); + expect(await this.checkpoint.getAtBlock(this.tx2.receipt.blockNumber)).to.be.bignumber.equal('2'); + // Block with no new checkpoints + expect(await this.checkpoint.getAtBlock(this.tx2.receipt.blockNumber + 1)).to.be.bignumber.equal('2'); + expect(await this.checkpoint.getAtBlock(this.tx3.receipt.blockNumber)).to.be.bignumber.equal('3'); + expect(await this.checkpoint.getAtBlock(this.tx3.receipt.blockNumber + 1)).to.be.bignumber.equal('3'); + }); + + it('reverts if block number >= current block', async function () { + await expectRevert( + this.checkpoint.getAtBlock(await web3.eth.getBlockNumber()), + 'Checkpoints: block not yet mined', + ); + + await expectRevert( + this.checkpoint.getAtBlock(await web3.eth.getBlockNumber() + 1), + 'Checkpoints: block not yet mined', + ); + }); + + it('multiple checkpoints in the same block', async function () { + const lengthBefore = await this.checkpoint.length(); + await batchInBlock([ + () => this.checkpoint.push(8, { gas: 100000 }), + () => this.checkpoint.push(9, { gas: 100000 }), + () => this.checkpoint.push(10, { gas: 100000 }), + ]); + const lengthAfter = await this.checkpoint.length(); + + expect(lengthAfter.toNumber()).to.be.equal(lengthBefore.toNumber() + 1); + expect(await this.checkpoint.latest()).to.be.bignumber.equal('10'); + }); }); }); - describe('with checkpoints', function () { - beforeEach('pushing checkpoints', async function () { - this.tx1 = await this.checkpoint.push(1); - this.tx2 = await this.checkpoint.push(2); - await time.advanceBlock(); - this.tx3 = await this.checkpoint.push(3); - await time.advanceBlock(); - await time.advanceBlock(); - }); + for (const length of [160, 224]) { + describe(`Checkpoint${length}[]`, function () { + beforeEach(async function () { + this.contract = await artifacts.require(`Checkpoints${length}Mock`).new(); + }); - it('returns latest value', async function () { - expect(await this.checkpoint.latest()).to.be.bignumber.equal('3'); - }); + describe('without checkpoints', function () { + it('returns zero as latest value', async function () { + expect(await this.contract.latest()).to.be.bignumber.equal('0'); + }); - it('returns past values', async function () { - expect(await this.checkpoint.getAtBlock(this.tx1.receipt.blockNumber - 1)).to.be.bignumber.equal('0'); - expect(await this.checkpoint.getAtBlock(this.tx1.receipt.blockNumber)).to.be.bignumber.equal('1'); - expect(await this.checkpoint.getAtBlock(this.tx2.receipt.blockNumber)).to.be.bignumber.equal('2'); - // Block with no new checkpoints - expect(await this.checkpoint.getAtBlock(this.tx2.receipt.blockNumber + 1)).to.be.bignumber.equal('2'); - expect(await this.checkpoint.getAtBlock(this.tx3.receipt.blockNumber)).to.be.bignumber.equal('3'); - expect(await this.checkpoint.getAtBlock(this.tx3.receipt.blockNumber + 1)).to.be.bignumber.equal('3'); - }); + it('lookup returns 0', async function () { + expect(await this.contract.lowerLookup(0)).to.be.bignumber.equal('0'); + expect(await this.contract.upperLookup(0)).to.be.bignumber.equal('0'); + expect(await this.contract.upperLookupExpEnd(0)).to.be.bignumber.equal('0'); + }); + }); - it('reverts if block number >= current block', async function () { - await expectRevert( - this.checkpoint.getAtBlock(await web3.eth.getBlockNumber()), - 'Checkpoints: block not yet mined', - ); + describe('with checkpoints', function () { + beforeEach('pushing checkpoints', async function () { + this.checkpoints = [ + { key: 2, value: '2' }, + { key: 3, value: '3' }, + { key: 5, value: '5' }, + { key: 7, value: '8' }, + { key: 11, value: '13' }, + ]; + for (const { key, value } of this.checkpoints) { + await this.contract.push(key, value); + } + }); - await expectRevert( - this.checkpoint.getAtBlock(await web3.eth.getBlockNumber() + 1), - 'Checkpoints: block not yet mined', - ); - }); + it('returns latest value', async function () { + expect(await this.contract.latest()) + .to.be.bignumber.equal(last(...this.checkpoints).value); + }); - it('multiple checkpoints in the same block', async function () { - const lengthBefore = await this.checkpoint.length(); - await batchInBlock([ - () => this.checkpoint.push(8, { gas: 100000 }), - () => this.checkpoint.push(9, { gas: 100000 }), - () => this.checkpoint.push(10, { gas: 100000 }), - ]); - const lengthAfter = await this.checkpoint.length(); - - expect(lengthAfter.toNumber()).to.be.equal(lengthBefore.toNumber() + 1); - expect(await this.checkpoint.latest()).to.be.bignumber.equal('10'); + it('lower lookup', async function () { + for (let i = 0; i < 14; ++i) { + const value = first(...this.checkpoints.filter(x => i <= x.key))?.value || '0'; + + expect(await this.contract.lowerLookup(i)).to.be.bignumber.equal(value); + } + }); + + it('upper lookup', async function () { + for (let i = 0; i < 14; ++i) { + const value = last(...this.checkpoints.filter(x => i >= x.key))?.value || '0'; + + expect(await this.contract.upperLookup(i)).to.be.bignumber.equal(value); + expect(await this.contract.upperLookupExpEnd(i)).to.be.bignumber.equal(value); + } + }); + }); }); - }); + } }); From da0ea3162a5595fd5720fea1538c099a1827ba71 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 28 Jul 2022 16:43:50 +0200 Subject: [PATCH 03/46] =?UTF-8?q?rename=20upperLookupExpEnd=20=E2=86=92=20?= =?UTF-8?q?upperLookupRecent?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- contracts/mocks/CheckpointsMock.sol | 8 ++++---- contracts/utils/Checkpoints.sol | 6 +++--- scripts/generate/templates/Checkpoints.js | 4 ++-- scripts/generate/templates/CheckpointsMock.js | 4 ++-- test/utils/Checkpoints.test.js | 4 ++-- 5 files changed, 13 insertions(+), 13 deletions(-) diff --git a/contracts/mocks/CheckpointsMock.sol b/contracts/mocks/CheckpointsMock.sol index f59a11e994d..1969f92cb70 100644 --- a/contracts/mocks/CheckpointsMock.sol +++ b/contracts/mocks/CheckpointsMock.sol @@ -47,8 +47,8 @@ contract Checkpoints224Mock { return _totalCheckpoints.upperLookup(key); } - function upperLookupExpEnd(uint32 key) public view returns (uint224) { - return _totalCheckpoints.upperLookupExpEnd(key); + function upperLookupRecent(uint32 key) public view returns (uint224) { + return _totalCheckpoints.upperLookupRecent(key); } function length() public view returns (uint256) { @@ -77,8 +77,8 @@ contract Checkpoints160Mock { return _totalCheckpoints.upperLookup(key); } - function upperLookupExpEnd(uint96 key) public view returns (uint224) { - return _totalCheckpoints.upperLookupExpEnd(key); + function upperLookupRecent(uint96 key) public view returns (uint224) { + return _totalCheckpoints.upperLookupRecent(key); } function length() public view returns (uint256) { diff --git a/contracts/utils/Checkpoints.sol b/contracts/utils/Checkpoints.sol index 936b53ed9d5..fa3edbf4746 100644 --- a/contracts/utils/Checkpoints.sol +++ b/contracts/utils/Checkpoints.sol @@ -53,7 +53,7 @@ library Checkpoints { return pos == 0 ? 0 : self[pos - 1]._value; } - function upperLookupExpEnd(Checkpoint224[] storage self, uint32 key) internal view returns (uint224) { + function upperLookupRecent(Checkpoint224[] storage self, uint32 key) internal view returns (uint224) { uint256 length = self.length; uint256 offset = 1; @@ -139,7 +139,7 @@ library Checkpoints { return pos == 0 ? 0 : self[pos - 1]._value; } - function upperLookupExpEnd(Checkpoint160[] storage self, uint96 key) internal view returns (uint224) { + function upperLookupRecent(Checkpoint160[] storage self, uint96 key) internal view returns (uint224) { uint256 length = self.length; uint256 offset = 1; @@ -206,7 +206,7 @@ library Checkpoints { function getAtBlock(History storage self, uint256 blockNumber) internal view returns (uint256) { require(blockNumber < block.number, "Checkpoints: block not yet mined"); - return upperLookupExpEnd(self._checkpoints, SafeCast.toUint32(blockNumber)); + return upperLookupRecent(self._checkpoints, SafeCast.toUint32(blockNumber)); } /** diff --git a/scripts/generate/templates/Checkpoints.js b/scripts/generate/templates/Checkpoints.js index 30126481ef2..924cc4e111c 100644 --- a/scripts/generate/templates/Checkpoints.js +++ b/scripts/generate/templates/Checkpoints.js @@ -38,7 +38,7 @@ function latest(History storage self) internal view returns (uint256) { function getAtBlock(History storage self, uint256 blockNumber) internal view returns (uint256) { require(blockNumber < block.number, "Checkpoints: block not yet mined"); - return upperLookupExpEnd(self._checkpoints, SafeCast.toUint32(blockNumber)); + return upperLookupRecent(self._checkpoints, SafeCast.toUint32(blockNumber)); } /** @@ -104,7 +104,7 @@ function upperLookup(Checkpoint${length}[] storage self, uint${256 - length} key return pos == 0 ? 0 : self[pos - 1]._value; } -function upperLookupExpEnd(Checkpoint${length}[] storage self, uint${256 - length} key) internal view returns (uint224) { +function upperLookupRecent(Checkpoint${length}[] storage self, uint${256 - length} key) internal view returns (uint224) { uint256 length = self.length; uint256 offset = 1; diff --git a/scripts/generate/templates/CheckpointsMock.js b/scripts/generate/templates/CheckpointsMock.js index 72079293429..78a72c5885c 100755 --- a/scripts/generate/templates/CheckpointsMock.js +++ b/scripts/generate/templates/CheckpointsMock.js @@ -54,8 +54,8 @@ contract Checkpoints${length}Mock { return _totalCheckpoints.upperLookup(key); } - function upperLookupExpEnd(uint${256 - length} key) public view returns (uint224) { - return _totalCheckpoints.upperLookupExpEnd(key); + function upperLookupRecent(uint${256 - length} key) public view returns (uint224) { + return _totalCheckpoints.upperLookupRecent(key); } function length() public view returns (uint256) { diff --git a/test/utils/Checkpoints.test.js b/test/utils/Checkpoints.test.js index de10b388088..794f1d7af45 100644 --- a/test/utils/Checkpoints.test.js +++ b/test/utils/Checkpoints.test.js @@ -91,7 +91,7 @@ contract('Checkpoints', function (accounts) { it('lookup returns 0', async function () { expect(await this.contract.lowerLookup(0)).to.be.bignumber.equal('0'); expect(await this.contract.upperLookup(0)).to.be.bignumber.equal('0'); - expect(await this.contract.upperLookupExpEnd(0)).to.be.bignumber.equal('0'); + expect(await this.contract.upperLookupRecent(0)).to.be.bignumber.equal('0'); }); }); @@ -127,7 +127,7 @@ contract('Checkpoints', function (accounts) { const value = last(...this.checkpoints.filter(x => i >= x.key))?.value || '0'; expect(await this.contract.upperLookup(i)).to.be.bignumber.equal(value); - expect(await this.contract.upperLookupExpEnd(i)).to.be.bignumber.equal(value); + expect(await this.contract.upperLookupRecent(i)).to.be.bignumber.equal(value); } }); }); From ed81388619b8df687b80f09861b2902b8f84640e Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 28 Jul 2022 16:59:42 +0200 Subject: [PATCH 04/46] sanity check when pushing checkpoints --- contracts/utils/Checkpoints.sol | 40 ++++++++++++++++++----- scripts/generate/templates/Checkpoints.js | 20 +++++++++--- 2 files changed, 48 insertions(+), 12 deletions(-) diff --git a/contracts/utils/Checkpoints.sol b/contracts/utils/Checkpoints.sol index fa3edbf4746..30e4a477c87 100644 --- a/contracts/utils/Checkpoints.sol +++ b/contracts/utils/Checkpoints.sol @@ -32,13 +32,25 @@ library Checkpoints { uint224 value ) internal returns (uint224, uint224) { uint256 pos = self.length; - uint224 old = latest(self); - if (pos > 0 && self[pos - 1]._key == key) { - self[pos - 1]._value = value; + + if (pos > 0) { + // Use of memory is important here. + Checkpoint224 memory last = self[pos - 1]; + + // Checkpoints keys must be increassing. + require(last._key <= key, "Checkpoint: invalid key"); + + // Update or push new checkpoint + if (last._key == key) { + self[pos - 1]._value = value; + } else { + self.push(Checkpoint224({_key: key, _value: value})); + } + return (last._value, value); } else { self.push(Checkpoint224({_key: key, _value: value})); + return (0, value); } - return (old, value); } function lowerLookup(Checkpoint224[] storage self, uint32 key) internal view returns (uint224) { @@ -118,13 +130,25 @@ library Checkpoints { uint160 value ) internal returns (uint160, uint160) { uint256 pos = self.length; - uint160 old = latest(self); - if (pos > 0 && self[pos - 1]._key == key) { - self[pos - 1]._value = value; + + if (pos > 0) { + // Use of memory is important here. + Checkpoint160 memory last = self[pos - 1]; + + // Checkpoints keys must be increassing. + require(last._key <= key, "Checkpoint: invalid key"); + + // Update or push new checkpoint + if (last._key == key) { + self[pos - 1]._value = value; + } else { + self.push(Checkpoint160({_key: key, _value: value})); + } + return (last._value, value); } else { self.push(Checkpoint160({_key: key, _value: value})); + return (0, value); } - return (old, value); } function lowerLookup(Checkpoint160[] storage self, uint96 key) internal view returns (uint160) { diff --git a/scripts/generate/templates/Checkpoints.js b/scripts/generate/templates/Checkpoints.js index 924cc4e111c..860f766133a 100644 --- a/scripts/generate/templates/Checkpoints.js +++ b/scripts/generate/templates/Checkpoints.js @@ -83,13 +83,25 @@ function push( uint${length} value ) internal returns (uint${length}, uint${length}) { uint256 pos = self.length; - uint${length} old = latest(self); - if (pos > 0 && self[pos - 1]._key == key) { - self[pos - 1]._value = value; + + if (pos > 0) { + // Use of memory is important here. + Checkpoint${length} memory last = self[pos - 1]; + + // Checkpoints keys must be increassing. + require(last._key <= key, "Checkpoint: invalid key"); + + // Update or push new checkpoint + if (last._key == key) { + self[pos - 1]._value = value; + } else { + self.push(Checkpoint${length}({_key: key, _value: value})); + } + return (last._value, value); } else { self.push(Checkpoint${length}({_key: key, _value: value})); + return (0, value); } - return (old, value); } function lowerLookup(Checkpoint${length}[] storage self, uint${256 - length} key) internal view returns (uint${length}) { From b91832d78add5ecf64c83172077eb21b751085fd Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 29 Jul 2022 08:35:22 +0200 Subject: [PATCH 05/46] Skip redundant optimisation in VotesQuorumFraction --- .../governance/extensions/GovernorVotesQuorumFraction.sol | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/contracts/governance/extensions/GovernorVotesQuorumFraction.sol b/contracts/governance/extensions/GovernorVotesQuorumFraction.sol index 7779811de8a..b4938ec5773 100644 --- a/contracts/governance/extensions/GovernorVotesQuorumFraction.sol +++ b/contracts/governance/extensions/GovernorVotesQuorumFraction.sol @@ -49,13 +49,7 @@ abstract contract GovernorVotesQuorumFraction is GovernorVotes { return _quorumNumerator; } - // Optimistic search, check the latest checkpoint - Checkpoints.Checkpoint224 memory latest = _quorumNumeratorHistory._checkpoints[length - 1]; - if (latest._key <= blockNumber) { - return latest._value; - } - - // Otherwize, do the binary search + // Otherwize, do the binary search (recent optimized) return _quorumNumeratorHistory.getAtBlock(blockNumber); } From 2a6adb3e7a8334656af0cd321d15e438cec4f9ce Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 29 Jul 2022 09:33:05 +0200 Subject: [PATCH 06/46] reset previous history lookup algorithm and reenable quorum checks --- .../governance/extensions/GovernorVotesQuorumFraction.sol | 8 +++++++- contracts/utils/Checkpoints.sol | 2 +- scripts/generate/templates/Checkpoints.js | 2 +- 3 files changed, 9 insertions(+), 3 deletions(-) diff --git a/contracts/governance/extensions/GovernorVotesQuorumFraction.sol b/contracts/governance/extensions/GovernorVotesQuorumFraction.sol index b4938ec5773..7779811de8a 100644 --- a/contracts/governance/extensions/GovernorVotesQuorumFraction.sol +++ b/contracts/governance/extensions/GovernorVotesQuorumFraction.sol @@ -49,7 +49,13 @@ abstract contract GovernorVotesQuorumFraction is GovernorVotes { return _quorumNumerator; } - // Otherwize, do the binary search (recent optimized) + // Optimistic search, check the latest checkpoint + Checkpoints.Checkpoint224 memory latest = _quorumNumeratorHistory._checkpoints[length - 1]; + if (latest._key <= blockNumber) { + return latest._value; + } + + // Otherwize, do the binary search return _quorumNumeratorHistory.getAtBlock(blockNumber); } diff --git a/contracts/utils/Checkpoints.sol b/contracts/utils/Checkpoints.sol index 30e4a477c87..c0f6c82b3c3 100644 --- a/contracts/utils/Checkpoints.sol +++ b/contracts/utils/Checkpoints.sol @@ -230,7 +230,7 @@ library Checkpoints { function getAtBlock(History storage self, uint256 blockNumber) internal view returns (uint256) { require(blockNumber < block.number, "Checkpoints: block not yet mined"); - return upperLookupRecent(self._checkpoints, SafeCast.toUint32(blockNumber)); + return upperLookup(self._checkpoints, SafeCast.toUint32(blockNumber)); } /** diff --git a/scripts/generate/templates/Checkpoints.js b/scripts/generate/templates/Checkpoints.js index 860f766133a..dbe33610575 100644 --- a/scripts/generate/templates/Checkpoints.js +++ b/scripts/generate/templates/Checkpoints.js @@ -38,7 +38,7 @@ function latest(History storage self) internal view returns (uint256) { function getAtBlock(History storage self, uint256 blockNumber) internal view returns (uint256) { require(blockNumber < block.number, "Checkpoints: block not yet mined"); - return upperLookupRecent(self._checkpoints, SafeCast.toUint32(blockNumber)); + return upperLookup(self._checkpoints, SafeCast.toUint32(blockNumber)); } /** From 28f6e6fb3ff40cb98841090d1d74c4d30e34f7dd Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 29 Jul 2022 10:03:40 +0200 Subject: [PATCH 07/46] Use unsafeAccess to save gas --- contracts/utils/Arrays.sol | 15 ++++++- contracts/utils/Checkpoints.sol | 52 ++++++++++++++--------- scripts/generate/templates/Checkpoints.js | 25 +++++++---- 3 files changed, 62 insertions(+), 30 deletions(-) diff --git a/contracts/utils/Arrays.sol b/contracts/utils/Arrays.sol index 0783614cd3e..3cfa44f5ffa 100644 --- a/contracts/utils/Arrays.sol +++ b/contracts/utils/Arrays.sol @@ -31,7 +31,7 @@ library Arrays { // Note that mid will always be strictly less than high (i.e. it will be a valid array index) // because Math.average rounds down (it does integer division with truncation). - if (array[mid] > element) { + if (unsafeAccess(array, mid) > element) { high = mid; } else { low = mid + 1; @@ -39,10 +39,21 @@ library Arrays { } // At this point `low` is the exclusive upper bound. We will return the inclusive upper bound. - if (low > 0 && array[low - 1] == element) { + if (low > 0 && unsafeAccess(array, low - 1) == element) { return low - 1; } else { return low; } } + + /** + * @dev Access an array in an "unsafe" way. Skips solidity "index-out-of-range" check. + * @notice WARNING: only use if you are certain pos is lower then the array length. + */ + function unsafeAccess(uint256[] storage arr, uint256 pos) internal view returns (uint256 result) { + assembly { + mstore(0, arr.slot) + result := sload(add(keccak256(0, 0x20), pos)) + } + } } diff --git a/contracts/utils/Checkpoints.sol b/contracts/utils/Checkpoints.sol index c0f6c82b3c3..495be349c3b 100644 --- a/contracts/utils/Checkpoints.sol +++ b/contracts/utils/Checkpoints.sol @@ -23,7 +23,7 @@ library Checkpoints { function latest(Checkpoint224[] storage self) internal view returns (uint224) { uint256 pos = self.length; - return pos == 0 ? 0 : self[pos - 1]._value; + return pos == 0 ? 0 : _unsafeAccess(self, pos - 1)._value; } function push( @@ -35,14 +35,14 @@ library Checkpoints { if (pos > 0) { // Use of memory is important here. - Checkpoint224 memory last = self[pos - 1]; + Checkpoint224 memory last = _unsafeAccess(self, pos - 1); // Checkpoints keys must be increassing. require(last._key <= key, "Checkpoint: invalid key"); // Update or push new checkpoint if (last._key == key) { - self[pos - 1]._value = value; + _unsafeAccess(self, pos - 1)._value = value; } else { self.push(Checkpoint224({_key: key, _value: value})); } @@ -56,20 +56,20 @@ library Checkpoints { function lowerLookup(Checkpoint224[] storage self, uint32 key) internal view returns (uint224) { uint256 length = self.length; uint256 pos = _lowerDichotomicLookup(self, key, 0, length); - return pos == length ? 0 : self[pos]._value; + return pos == length ? 0 : _unsafeAccess(self, pos)._value; } function upperLookup(Checkpoint224[] storage self, uint32 key) internal view returns (uint224) { uint256 length = self.length; uint256 pos = _upperDichotomicLookup(self, key, 0, length); - return pos == 0 ? 0 : self[pos - 1]._value; + return pos == 0 ? 0 : _unsafeAccess(self, pos - 1)._value; } function upperLookupRecent(Checkpoint224[] storage self, uint32 key) internal view returns (uint224) { uint256 length = self.length; uint256 offset = 1; - while (offset <= length && self[length - offset]._key > key) { + while (offset <= length && _unsafeAccess(self, length - offset)._key > key) { offset <<= 1; } @@ -77,7 +77,7 @@ library Checkpoints { uint256 high = length - (offset >> 1); uint256 pos = _upperDichotomicLookup(self, key, low, high); - return pos == 0 ? 0 : self[pos - 1]._value; + return pos == 0 ? 0 : _unsafeAccess(self, pos - 1)._value; } function _upperDichotomicLookup( @@ -88,7 +88,7 @@ library Checkpoints { ) private view returns (uint256) { while (low < high) { uint256 mid = Math.average(low, high); - if (self[mid]._key > key) { + if (_unsafeAccess(self, mid)._key > key) { high = mid; } else { low = mid + 1; @@ -105,7 +105,7 @@ library Checkpoints { ) private view returns (uint256) { while (low < high) { uint256 mid = Math.average(low, high); - if (self[mid]._key < key) { + if (_unsafeAccess(self, mid)._key < key) { low = mid + 1; } else { high = mid; @@ -114,6 +114,13 @@ library Checkpoints { return high; } + function _unsafeAccess(Checkpoint224[] storage self, uint256 pos) private view returns (Checkpoint224 storage result) { + assembly { + mstore(0, self.slot) + result.slot := add(keccak256(0, 0x20), pos) + } + } + struct Checkpoint160 { uint96 _key; uint160 _value; @@ -121,7 +128,7 @@ library Checkpoints { function latest(Checkpoint160[] storage self) internal view returns (uint160) { uint256 pos = self.length; - return pos == 0 ? 0 : self[pos - 1]._value; + return pos == 0 ? 0 : _unsafeAccess(self, pos - 1)._value; } function push( @@ -133,14 +140,14 @@ library Checkpoints { if (pos > 0) { // Use of memory is important here. - Checkpoint160 memory last = self[pos - 1]; + Checkpoint160 memory last = _unsafeAccess(self, pos - 1); // Checkpoints keys must be increassing. require(last._key <= key, "Checkpoint: invalid key"); // Update or push new checkpoint if (last._key == key) { - self[pos - 1]._value = value; + _unsafeAccess(self, pos - 1)._value = value; } else { self.push(Checkpoint160({_key: key, _value: value})); } @@ -154,20 +161,20 @@ library Checkpoints { function lowerLookup(Checkpoint160[] storage self, uint96 key) internal view returns (uint160) { uint256 length = self.length; uint256 pos = _lowerDichotomicLookup(self, key, 0, length); - return pos == length ? 0 : self[pos]._value; + return pos == length ? 0 : _unsafeAccess(self, pos)._value; } function upperLookup(Checkpoint160[] storage self, uint96 key) internal view returns (uint160) { uint256 length = self.length; uint256 pos = _upperDichotomicLookup(self, key, 0, length); - return pos == 0 ? 0 : self[pos - 1]._value; + return pos == 0 ? 0 : _unsafeAccess(self, pos - 1)._value; } function upperLookupRecent(Checkpoint160[] storage self, uint96 key) internal view returns (uint224) { uint256 length = self.length; uint256 offset = 1; - while (offset <= length && self[length - offset]._key > key) { + while (offset <= length && _unsafeAccess(self, length - offset)._key > key) { offset <<= 1; } @@ -175,7 +182,7 @@ library Checkpoints { uint256 high = length - (offset >> 1); uint256 pos = _upperDichotomicLookup(self, key, low, high); - return pos == 0 ? 0 : self[pos - 1]._value; + return pos == 0 ? 0 : _unsafeAccess(self, pos - 1)._value; } function _upperDichotomicLookup( @@ -186,7 +193,7 @@ library Checkpoints { ) private view returns (uint256) { while (low < high) { uint256 mid = Math.average(low, high); - if (self[mid]._key > key) { + if (_unsafeAccess(self, mid)._key > key) { high = mid; } else { low = mid + 1; @@ -203,7 +210,7 @@ library Checkpoints { ) private view returns (uint256) { while (low < high) { uint256 mid = Math.average(low, high); - if (self[mid]._key < key) { + if (_unsafeAccess(self, mid)._key < key) { low = mid + 1; } else { high = mid; @@ -212,6 +219,13 @@ library Checkpoints { return high; } + function _unsafeAccess(Checkpoint160[] storage self, uint256 pos) private view returns (Checkpoint160 storage result) { + assembly { + mstore(0, self.slot) + result.slot := add(keccak256(0, 0x20), pos) + } + } + struct History { Checkpoint224[] _checkpoints; } @@ -230,7 +244,7 @@ library Checkpoints { function getAtBlock(History storage self, uint256 blockNumber) internal view returns (uint256) { require(blockNumber < block.number, "Checkpoints: block not yet mined"); - return upperLookup(self._checkpoints, SafeCast.toUint32(blockNumber)); + return upperLookupRecent(self._checkpoints, SafeCast.toUint32(blockNumber)); } /** diff --git a/scripts/generate/templates/Checkpoints.js b/scripts/generate/templates/Checkpoints.js index dbe33610575..46404ddeaaa 100644 --- a/scripts/generate/templates/Checkpoints.js +++ b/scripts/generate/templates/Checkpoints.js @@ -74,7 +74,7 @@ struct Checkpoint${length} { function latest(Checkpoint${length}[] storage self) internal view returns (uint${length}) { uint256 pos = self.length; - return pos == 0 ? 0 : self[pos - 1]._value; + return pos == 0 ? 0 : _unsafeAccess(self, pos - 1)._value; } function push( @@ -86,14 +86,14 @@ function push( if (pos > 0) { // Use of memory is important here. - Checkpoint${length} memory last = self[pos - 1]; + Checkpoint${length} memory last = _unsafeAccess(self, pos - 1); // Checkpoints keys must be increassing. require(last._key <= key, "Checkpoint: invalid key"); // Update or push new checkpoint if (last._key == key) { - self[pos - 1]._value = value; + _unsafeAccess(self, pos - 1)._value = value; } else { self.push(Checkpoint${length}({_key: key, _value: value})); } @@ -107,20 +107,20 @@ function push( function lowerLookup(Checkpoint${length}[] storage self, uint${256 - length} key) internal view returns (uint${length}) { uint256 length = self.length; uint256 pos = _lowerDichotomicLookup(self, key, 0, length); - return pos == length ? 0 : self[pos]._value; + return pos == length ? 0 : _unsafeAccess(self, pos)._value; } function upperLookup(Checkpoint${length}[] storage self, uint${256 - length} key) internal view returns (uint${length}) { uint256 length = self.length; uint256 pos = _upperDichotomicLookup(self, key, 0, length); - return pos == 0 ? 0 : self[pos - 1]._value; + return pos == 0 ? 0 : _unsafeAccess(self, pos - 1)._value; } function upperLookupRecent(Checkpoint${length}[] storage self, uint${256 - length} key) internal view returns (uint224) { uint256 length = self.length; uint256 offset = 1; - while (offset <= length && self[length - offset]._key > key) { + while (offset <= length && _unsafeAccess(self, length - offset)._key > key) { offset <<= 1; } @@ -128,7 +128,7 @@ function upperLookupRecent(Checkpoint${length}[] storage self, uint${256 - lengt uint256 high = length - (offset >> 1); uint256 pos = _upperDichotomicLookup(self, key, low, high); - return pos == 0 ? 0 : self[pos - 1]._value; + return pos == 0 ? 0 : _unsafeAccess(self, pos - 1)._value; } function _upperDichotomicLookup( @@ -139,7 +139,7 @@ function _upperDichotomicLookup( ) private view returns (uint256) { while (low < high) { uint256 mid = Math.average(low, high); - if (self[mid]._key > key) { + if (_unsafeAccess(self, mid)._key > key) { high = mid; } else { low = mid + 1; @@ -156,7 +156,7 @@ function _lowerDichotomicLookup( ) private view returns (uint256) { while (low < high) { uint256 mid = Math.average(low, high); - if (self[mid]._key < key) { + if (_unsafeAccess(self, mid)._key < key) { low = mid + 1; } else { high = mid; @@ -164,6 +164,13 @@ function _lowerDichotomicLookup( } return high; } + +function _unsafeAccess(Checkpoint${length}[] storage self, uint256 pos) private view returns (Checkpoint${length} storage result) { + assembly { + mstore(0, self.slot) + result.slot := add(keccak256(0, 0x20), pos) + } +} `; /* eslint-enable max-len */ From 616c51b78ec16eb14d25e35ac3f81e00aad06b99 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 29 Jul 2022 10:22:46 +0200 Subject: [PATCH 08/46] Regenerate contracts --- contracts/utils/Checkpoints.sol | 14 +++++++++++--- scripts/generate/templates/Checkpoints.js | 6 +++++- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/contracts/utils/Checkpoints.sol b/contracts/utils/Checkpoints.sol index 495be349c3b..502fab73259 100644 --- a/contracts/utils/Checkpoints.sol +++ b/contracts/utils/Checkpoints.sol @@ -114,7 +114,11 @@ library Checkpoints { return high; } - function _unsafeAccess(Checkpoint224[] storage self, uint256 pos) private view returns (Checkpoint224 storage result) { + function _unsafeAccess(Checkpoint224[] storage self, uint256 pos) + private + view + returns (Checkpoint224 storage result) + { assembly { mstore(0, self.slot) result.slot := add(keccak256(0, 0x20), pos) @@ -219,7 +223,11 @@ library Checkpoints { return high; } - function _unsafeAccess(Checkpoint160[] storage self, uint256 pos) private view returns (Checkpoint160 storage result) { + function _unsafeAccess(Checkpoint160[] storage self, uint256 pos) + private + view + returns (Checkpoint160 storage result) + { assembly { mstore(0, self.slot) result.slot := add(keccak256(0, 0x20), pos) @@ -244,7 +252,7 @@ library Checkpoints { function getAtBlock(History storage self, uint256 blockNumber) internal view returns (uint256) { require(blockNumber < block.number, "Checkpoints: block not yet mined"); - return upperLookupRecent(self._checkpoints, SafeCast.toUint32(blockNumber)); + return upperLookup(self._checkpoints, SafeCast.toUint32(blockNumber)); } /** diff --git a/scripts/generate/templates/Checkpoints.js b/scripts/generate/templates/Checkpoints.js index 46404ddeaaa..bdc488bdb0b 100644 --- a/scripts/generate/templates/Checkpoints.js +++ b/scripts/generate/templates/Checkpoints.js @@ -165,7 +165,11 @@ function _lowerDichotomicLookup( return high; } -function _unsafeAccess(Checkpoint${length}[] storage self, uint256 pos) private view returns (Checkpoint${length} storage result) { +function _unsafeAccess(Checkpoint${length}[] storage self, uint256 pos) + private + view + returns (Checkpoint${length} storage result) +{ assembly { mstore(0, self.slot) result.slot := add(keccak256(0, 0x20), pos) From 3fb185bfbb681389a8101b6e3acf290e7c6baeeb Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 29 Jul 2022 10:50:08 +0200 Subject: [PATCH 09/46] Add changelog entry --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1afa0ddf822..10f0293636f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,9 @@ * `ReentrancyGuard`: Reduce code size impact of the modifier by using internal functions. ([#3515](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3515)) * `SafeCast`: optimize downcasting of signed integers. ([#3565](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3565)) * `ERC20FlashMint`: add an internal `_flashFee` function for overriding. ([#3551](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3551)) + * `Checkpoints`: Use provedural generation to support multiple key/value lengths. ([#3589](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3589)) + * `Checkpoints`: Add new lookup mechanisms. ([#3589](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3589)) + * `Array`: Add a `unsafeAccess` function that bypasses solidity's "out-of-array" check when reading from an array. ([#3589](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3589)) ### Compatibility Note From ce0c7a6d95ca201b9055674b077c7c8c4acbb46e Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 29 Jul 2022 18:06:43 +0200 Subject: [PATCH 10/46] use StorageSlot for unsafeAccess returns --- contracts/utils/Arrays.sol | 40 ++++++++++++++++++++++++++++++++++---- 1 file changed, 36 insertions(+), 4 deletions(-) diff --git a/contracts/utils/Arrays.sol b/contracts/utils/Arrays.sol index 3cfa44f5ffa..9874c115e8e 100644 --- a/contracts/utils/Arrays.sol +++ b/contracts/utils/Arrays.sol @@ -3,6 +3,7 @@ pragma solidity ^0.8.0; +import "./StorageSlot.sol"; import "./math/Math.sol"; /** @@ -31,7 +32,7 @@ library Arrays { // Note that mid will always be strictly less than high (i.e. it will be a valid array index) // because Math.average rounds down (it does integer division with truncation). - if (unsafeAccess(array, mid) > element) { + if (unsafeAccess(array, mid).value > element) { high = mid; } else { low = mid + 1; @@ -39,7 +40,7 @@ library Arrays { } // At this point `low` is the exclusive upper bound. We will return the inclusive upper bound. - if (low > 0 && unsafeAccess(array, low - 1) == element) { + if (low > 0 && unsafeAccess(array, low - 1).value == element) { return low - 1; } else { return low; @@ -50,10 +51,41 @@ library Arrays { * @dev Access an array in an "unsafe" way. Skips solidity "index-out-of-range" check. * @notice WARNING: only use if you are certain pos is lower then the array length. */ - function unsafeAccess(uint256[] storage arr, uint256 pos) internal view returns (uint256 result) { + function unsafeAccess(address[] storage arr, uint256 pos) internal pure returns (StorageSlot.AddressSlot storage) { + bytes32 slot; + /// @solidity memory-safe-assembly assembly { mstore(0, arr.slot) - result := sload(add(keccak256(0, 0x20), pos)) + slot := add(keccak256(0, 0x20), pos) } + return StorageSlot.getAddressSlot(slot); + } + + /** + * @dev Access an array in an "unsafe" way. Skips solidity "index-out-of-range" check. + * @notice WARNING: only use if you are certain pos is lower then the array length. + */ + function unsafeAccess(bytes32[] storage arr, uint256 pos) internal pure returns (StorageSlot.Bytes32Slot storage) { + bytes32 slot; + /// @solidity memory-safe-assembly + assembly { + mstore(0, arr.slot) + slot := add(keccak256(0, 0x20), pos) + } + return StorageSlot.getBytes32Slot(slot); + } + + /** + * @dev Access an array in an "unsafe" way. Skips solidity "index-out-of-range" check. + * @notice WARNING: only use if you are certain pos is lower then the array length. + */ + function unsafeAccess(uint256[] storage arr, uint256 pos) internal pure returns (StorageSlot.Uint256Slot storage) { + bytes32 slot; + /// @solidity memory-safe-assembly + assembly { + mstore(0, arr.slot) + slot := add(keccak256(0, 0x20), pos) + } + return StorageSlot.getUint256Slot(slot); } } From 33741e702cd6281209e702d61b01a365ee1473eb Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 29 Jul 2022 18:09:29 +0200 Subject: [PATCH 11/46] minor rewrite for readability --- contracts/utils/Arrays.sol | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/contracts/utils/Arrays.sol b/contracts/utils/Arrays.sol index 9874c115e8e..2c7ab89e6c9 100644 --- a/contracts/utils/Arrays.sol +++ b/contracts/utils/Arrays.sol @@ -10,6 +10,8 @@ import "./math/Math.sol"; * @dev Collection of functions related to array types. */ library Arrays { + using StorageSlot for bytes32; + /** * @dev Searches a sorted `array` and returns the first index that contains * a value greater or equal to `element`. If no such index exists (i.e. all @@ -58,7 +60,7 @@ library Arrays { mstore(0, arr.slot) slot := add(keccak256(0, 0x20), pos) } - return StorageSlot.getAddressSlot(slot); + return slot.getAddressSlot(); } /** @@ -72,7 +74,7 @@ library Arrays { mstore(0, arr.slot) slot := add(keccak256(0, 0x20), pos) } - return StorageSlot.getBytes32Slot(slot); + return slot.getBytes32Slot(); } /** @@ -86,6 +88,6 @@ library Arrays { mstore(0, arr.slot) slot := add(keccak256(0, 0x20), pos) } - return StorageSlot.getUint256Slot(slot); + return slot.getUint256Slot(); } } From 8425e4a39d7fdabbfedcd5be8fea06e733c38227 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 29 Jul 2022 18:13:00 +0200 Subject: [PATCH 12/46] fix typo --- contracts/utils/Arrays.sol | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/contracts/utils/Arrays.sol b/contracts/utils/Arrays.sol index 2c7ab89e6c9..5b2eccf9ea1 100644 --- a/contracts/utils/Arrays.sol +++ b/contracts/utils/Arrays.sol @@ -51,7 +51,7 @@ library Arrays { /** * @dev Access an array in an "unsafe" way. Skips solidity "index-out-of-range" check. - * @notice WARNING: only use if you are certain pos is lower then the array length. + * @notice WARNING: only use if you are certain pos is lower than the array length. */ function unsafeAccess(address[] storage arr, uint256 pos) internal pure returns (StorageSlot.AddressSlot storage) { bytes32 slot; @@ -65,7 +65,7 @@ library Arrays { /** * @dev Access an array in an "unsafe" way. Skips solidity "index-out-of-range" check. - * @notice WARNING: only use if you are certain pos is lower then the array length. + * @notice WARNING: only use if you are certain pos is lower than the array length. */ function unsafeAccess(bytes32[] storage arr, uint256 pos) internal pure returns (StorageSlot.Bytes32Slot storage) { bytes32 slot; @@ -79,7 +79,7 @@ library Arrays { /** * @dev Access an array in an "unsafe" way. Skips solidity "index-out-of-range" check. - * @notice WARNING: only use if you are certain pos is lower then the array length. + * @notice WARNING: only use if you are certain pos is lower than the array length. */ function unsafeAccess(uint256[] storage arr, uint256 pos) internal pure returns (StorageSlot.Uint256Slot storage) { bytes32 slot; From 74fef959ffed0b037f535f3e9314fa1b3c6bd164 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 29 Jul 2022 18:34:38 +0200 Subject: [PATCH 13/46] testing unsafeAccess --- contracts/mocks/ArraysImpl.sol | 19 ------------- contracts/mocks/ArraysMock.sol | 51 ++++++++++++++++++++++++++++++++++ test/utils/Arrays.test.js | 26 +++++++++++++---- 3 files changed, 72 insertions(+), 24 deletions(-) delete mode 100644 contracts/mocks/ArraysImpl.sol create mode 100644 contracts/mocks/ArraysMock.sol diff --git a/contracts/mocks/ArraysImpl.sol b/contracts/mocks/ArraysImpl.sol deleted file mode 100644 index f720524b808..00000000000 --- a/contracts/mocks/ArraysImpl.sol +++ /dev/null @@ -1,19 +0,0 @@ -// SPDX-License-Identifier: MIT - -pragma solidity ^0.8.0; - -import "../utils/Arrays.sol"; - -contract ArraysImpl { - using Arrays for uint256[]; - - uint256[] private _array; - - constructor(uint256[] memory array) { - _array = array; - } - - function findUpperBound(uint256 element) external view returns (uint256) { - return _array.findUpperBound(element); - } -} diff --git a/contracts/mocks/ArraysMock.sol b/contracts/mocks/ArraysMock.sol new file mode 100644 index 00000000000..7dd02367b1d --- /dev/null +++ b/contracts/mocks/ArraysMock.sol @@ -0,0 +1,51 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.0; + +import "../utils/Arrays.sol"; + +contract Uint256ArraysMock { + using Arrays for uint256[]; + + uint256[] private _array; + + constructor(uint256[] memory array) { + _array = array; + } + + function findUpperBound(uint256 element) external view returns (uint256) { + return _array.findUpperBound(element); + } + + function unsafeAccess(uint256 pos) external view returns (uint256) { + return _array.unsafeAccess(pos).value; + } +} + +contract AddressArraysMock { + using Arrays for address[]; + + address[] private _array; + + constructor(address[] memory array) { + _array = array; + } + + function unsafeAccess(uint256 pos) external view returns (address) { + return _array.unsafeAccess(pos).value; + } +} + +contract Bytes32ArraysMock { + using Arrays for bytes32[]; + + bytes32[] private _array; + + constructor(bytes32[] memory array) { + _array = array; + } + + function unsafeAccess(uint256 pos) external view returns (bytes32) { + return _array.unsafeAccess(pos).value; + } +} \ No newline at end of file diff --git a/test/utils/Arrays.test.js b/test/utils/Arrays.test.js index 67128fac26f..e2a66553cf7 100644 --- a/test/utils/Arrays.test.js +++ b/test/utils/Arrays.test.js @@ -2,7 +2,7 @@ require('@openzeppelin/test-helpers'); const { expect } = require('chai'); -const ArraysImpl = artifacts.require('ArraysImpl'); +const Uint256ArraysMock = artifacts.require('Uint256ArraysMock'); contract('Arrays', function (accounts) { describe('findUpperBound', function () { @@ -10,7 +10,7 @@ contract('Arrays', function (accounts) { const EVEN_ELEMENTS_ARRAY = [11, 12, 13, 14, 15, 16, 17, 18, 19, 20]; beforeEach(async function () { - this.arrays = await ArraysImpl.new(EVEN_ELEMENTS_ARRAY); + this.arrays = await Uint256ArraysMock.new(EVEN_ELEMENTS_ARRAY); }); it('returns correct index for the basic case', async function () { @@ -38,7 +38,7 @@ contract('Arrays', function (accounts) { const ODD_ELEMENTS_ARRAY = [11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21]; beforeEach(async function () { - this.arrays = await ArraysImpl.new(ODD_ELEMENTS_ARRAY); + this.arrays = await Uint256ArraysMock.new(ODD_ELEMENTS_ARRAY); }); it('returns correct index for the basic case', async function () { @@ -66,7 +66,7 @@ contract('Arrays', function (accounts) { const WITH_GAP_ARRAY = [11, 12, 13, 14, 15, 20, 21, 22, 23, 24]; beforeEach(async function () { - this.arrays = await ArraysImpl.new(WITH_GAP_ARRAY); + this.arrays = await Uint256ArraysMock.new(WITH_GAP_ARRAY); }); it('returns index of first element in next filled range', async function () { @@ -76,7 +76,7 @@ contract('Arrays', function (accounts) { context('Empty array', function () { beforeEach(async function () { - this.arrays = await ArraysImpl.new([]); + this.arrays = await Uint256ArraysMock.new([]); }); it('always returns 0 for empty array', async function () { @@ -84,4 +84,20 @@ contract('Arrays', function (accounts) { }); }); }); + + describe('unsafeAccess', function () { + for (const { type, artifact, elements } of [ + { type: 'address', artifact: artifacts.require('AddressArraysMock'), elements: Array(10).fill().map(() => web3.utils.randomHex(20)) }, + { type: 'bytes32', artifact: artifacts.require('Bytes32ArraysMock'), elements: Array(10).fill().map(() => web3.utils.randomHex(32)) }, + { type: 'uint256', artifact: artifacts.require('Uint256ArraysMock'), elements: Array(10).fill().map(() => web3.utils.randomHex(32)) }, + ]) { + it(type, async function () { + const contract = await artifact.new(elements); + + for (const i in elements) { + expect(await contract.unsafeAccess(i)).to.be.bignumber.equal(elements[i]); + } + }); + } + }); }); From df3a9b1d9b7b6c42c4e5ce4dacd18d38c68ccf07 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 29 Jul 2022 18:36:10 +0200 Subject: [PATCH 14/46] fix lint --- contracts/mocks/ArraysMock.sol | 2 +- test/utils/Arrays.test.js | 8 +++++--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/contracts/mocks/ArraysMock.sol b/contracts/mocks/ArraysMock.sol index 7dd02367b1d..2ea17a09fde 100644 --- a/contracts/mocks/ArraysMock.sol +++ b/contracts/mocks/ArraysMock.sol @@ -48,4 +48,4 @@ contract Bytes32ArraysMock { function unsafeAccess(uint256 pos) external view returns (bytes32) { return _array.unsafeAccess(pos).value; } -} \ No newline at end of file +} diff --git a/test/utils/Arrays.test.js b/test/utils/Arrays.test.js index e2a66553cf7..8c287ccfff7 100644 --- a/test/utils/Arrays.test.js +++ b/test/utils/Arrays.test.js @@ -2,6 +2,8 @@ require('@openzeppelin/test-helpers'); const { expect } = require('chai'); +const AddressArraysMock = artifacts.require('AddressArraysMock'); +const Bytes32ArraysMock = artifacts.require('Bytes32ArraysMock'); const Uint256ArraysMock = artifacts.require('Uint256ArraysMock'); contract('Arrays', function (accounts) { @@ -87,9 +89,9 @@ contract('Arrays', function (accounts) { describe('unsafeAccess', function () { for (const { type, artifact, elements } of [ - { type: 'address', artifact: artifacts.require('AddressArraysMock'), elements: Array(10).fill().map(() => web3.utils.randomHex(20)) }, - { type: 'bytes32', artifact: artifacts.require('Bytes32ArraysMock'), elements: Array(10).fill().map(() => web3.utils.randomHex(32)) }, - { type: 'uint256', artifact: artifacts.require('Uint256ArraysMock'), elements: Array(10).fill().map(() => web3.utils.randomHex(32)) }, + { type: 'address', artifact: AddressArraysMock, elements: Array(10).fill().map(() => web3.utils.randomHex(20)) }, + { type: 'bytes32', artifact: Bytes32ArraysMock, elements: Array(10).fill().map(() => web3.utils.randomHex(32)) }, + { type: 'uint256', artifact: Uint256ArraysMock, elements: Array(10).fill().map(() => web3.utils.randomHex(32)) }, ]) { it(type, async function () { const contract = await artifact.new(elements); From e182730d05d95355916c17bccdb257ed4a95faac Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 29 Jul 2022 18:38:44 +0200 Subject: [PATCH 15/46] Update CHANGELOG.md --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 10f0293636f..68dd9cb732c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,7 +14,7 @@ * `ERC20FlashMint`: add an internal `_flashFee` function for overriding. ([#3551](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3551)) * `Checkpoints`: Use provedural generation to support multiple key/value lengths. ([#3589](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3589)) * `Checkpoints`: Add new lookup mechanisms. ([#3589](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3589)) - * `Array`: Add a `unsafeAccess` function that bypasses solidity's "out-of-array" check when reading from an array. ([#3589](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3589)) + * `Array`: Add `unsafeAccess` functions that allow reading and writting to a element in a storage array while bypassing solidity's "out-of-array" check. ([#3589](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3589)) ### Compatibility Note From 03835b3ed78cc2b001d742bfb2917e92f1a4a9e9 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 10 Aug 2022 16:48:16 +0200 Subject: [PATCH 16/46] Rebase ERC721Consecutive work on Checkpoints --- contracts/mocks/ERC721ConsecutiveMock.sol | 118 ++++++++++++++++++ contracts/token/ERC721/ERC721.sol | 35 +++++- .../ERC721/extensions/ERC721Consecutive.sol | 98 +++++++++++++++ .../ERC721/extensions/ERC721Enumerable.sol | 42 +++++++ .../ERC721/extensions/ERC721Pausable.sol | 11 ++ .../ERC721/extensions/draft-ERC721Votes.sol | 15 +++ .../ERC721PresetMinterPauserAutoId.sol | 9 ++ .../extensions/ERC721Consecutive.test.js | 109 ++++++++++++++++ 8 files changed, 435 insertions(+), 2 deletions(-) create mode 100644 contracts/mocks/ERC721ConsecutiveMock.sol create mode 100644 contracts/token/ERC721/extensions/ERC721Consecutive.sol create mode 100644 test/token/ERC721/extensions/ERC721Consecutive.test.js diff --git a/contracts/mocks/ERC721ConsecutiveMock.sol b/contracts/mocks/ERC721ConsecutiveMock.sol new file mode 100644 index 00000000000..c4845e22bbc --- /dev/null +++ b/contracts/mocks/ERC721ConsecutiveMock.sol @@ -0,0 +1,118 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.0; + +import "../token/ERC721/extensions/ERC721Burnable.sol"; +import "../token/ERC721/extensions/ERC721Consecutive.sol"; +import "../token/ERC721/extensions/ERC721Enumerable.sol"; +import "../token/ERC721/extensions/ERC721Pausable.sol"; +import "../token/ERC721/extensions/draft-ERC721Votes.sol"; + +/* solhint-disable-next-line contract-name-camelcase */ +abstract contract __VotesDelegationInConstructor is Votes { + constructor(address[] memory accounts) { + for (uint256 i; i < accounts.length; ++i) { + _delegate(accounts[i], accounts[i]); + } + } +} + +/** + * @title ERC721ConsecutiveMock + */ +contract ERC721ConsecutiveMock is + __VotesDelegationInConstructor, + ERC721Burnable, + ERC721Consecutive, + ERC721Enumerable, + ERC721Pausable, + ERC721Votes +{ + constructor( + string memory name, + string memory symbol, + address[] memory receivers, + uint96[] memory amounts + ) + __VotesDelegationInConstructor(receivers) + ERC721(name, symbol) + ERC721Consecutive(receivers, amounts) + EIP712(name, "1") + {} + + function pause() external { + _pause(); + } + + function unpause() external { + _unpause(); + } + + function supportsInterface(bytes4 interfaceId) + public + view + virtual + override(ERC721, ERC721Enumerable) + returns (bool) + { + return super.supportsInterface(interfaceId); + } + + function exists(uint256 tokenId) public view returns (bool) { + return _exists(tokenId); + } + + function mint(address to, uint256 tokenId) public { + _mint(to, tokenId); + } + + function safeMint(address to, uint256 tokenId) public { + _safeMint(to, tokenId); + } + + function _ownerOf(uint256 tokenId) internal view virtual override(ERC721, ERC721Consecutive) returns (address) { + return super._ownerOf(tokenId); + } + + function _burn(uint256 tokenId) internal virtual override(ERC721, ERC721Consecutive) { + super._burn(tokenId); + } + + function _mint(address to, uint256 tokenId) internal virtual override(ERC721, ERC721Consecutive) { + super._mint(to, tokenId); + } + + function _beforeTokenTransfer( + address from, + address to, + uint256 tokenId + ) internal virtual override(ERC721, ERC721Enumerable, ERC721Pausable) { + super._beforeTokenTransfer(from, to, tokenId); + } + + function _afterTokenTransfer( + address from, + address to, + uint256 tokenId + ) internal virtual override(ERC721, ERC721Votes) { + super._afterTokenTransfer(from, to, tokenId); + } + + function _beforeConsecutiveTokenTransfer( + address from, + address to, + uint256 first, + uint256 last + ) internal virtual override(ERC721, ERC721Enumerable, ERC721Pausable) { + super._beforeConsecutiveTokenTransfer(from, to, first, last); + } + + function _afterConsecutiveTokenTransfer( + address from, + address to, + uint256 first, + uint256 last + ) internal virtual override(ERC721, ERC721Votes) { + super._afterConsecutiveTokenTransfer(from, to, first, last); + } +} diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index e33d2c79459..77601ce8e05 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -68,7 +68,7 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata { * @dev See {IERC721-ownerOf}. */ function ownerOf(uint256 tokenId) public view virtual override returns (address) { - address owner = _owners[tokenId]; + address owner = _ownerOf(tokenId); require(owner != address(0), "ERC721: invalid token ID"); return owner; } @@ -210,6 +210,13 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata { require(_checkOnERC721Received(from, to, tokenId, data), "ERC721: transfer to non ERC721Receiver implementer"); } + /** + * @dev Returns the owner of the `tokenId`. Does NOT revert if token doesn't exist + */ + function _ownerOf(uint256 tokenId) internal view virtual returns (address) { + return _owners[tokenId]; + } + /** * @dev Returns whether `tokenId` exists. * @@ -219,7 +226,7 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata { * and stop existing when they are burned (`_burn`). */ function _exists(uint256 tokenId) internal view virtual returns (bool) { - return _owners[tokenId] != address(0); + return _ownerOf(tokenId) != address(0); } /** @@ -452,4 +459,28 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata { address to, uint256 tokenId ) internal virtual {} + + /** + * TODO + */ + function _beforeConsecutiveTokenTransfer( + address from, + address to, + uint256 first, + uint256 last + ) internal virtual { + if (from != address(0)) { + _balances[from] -= last - first + 1; + } + if (to != address(0)) { + _balances[to] += last - first + 1; + } + } + + function _afterConsecutiveTokenTransfer( + address from, + address to, + uint256 first, + uint256 last + ) internal virtual {} } diff --git a/contracts/token/ERC721/extensions/ERC721Consecutive.sol b/contracts/token/ERC721/extensions/ERC721Consecutive.sol new file mode 100644 index 00000000000..8894692cfd5 --- /dev/null +++ b/contracts/token/ERC721/extensions/ERC721Consecutive.sol @@ -0,0 +1,98 @@ +// SPDX-License-Identifier: MIT +// OpenZeppelin Contracts v4.4.1 (token/ERC721/extensions/ERC721Burnable.sol) + +pragma solidity ^0.8.0; + +import "../ERC721.sol"; +import "../../../utils/Checkpoints.sol"; +import "../../../utils/math/SafeCast.sol"; +import "../../../utils/structs/BitMaps.sol"; + +/** + * @title ERC721 Cheap sequential minting + */ +abstract contract ERC721Consecutive is ERC721 { + using BitMaps for BitMaps.BitMap; + using Checkpoints for Checkpoints.Checkpoint160[]; + + Checkpoints.Checkpoint160[] private _sequentialOwnership; + BitMaps.BitMap private _sequentialBurn; + + event ConsecutiveTransfer( + uint256 indexed fromTokenId, + uint256 toTokenId, + address indexed fromAddress, + address indexed toAddress + ); + + constructor(address[] memory receivers, uint96[] memory amounts) { + // Check input length + uint256 length = receivers.length; + require(length == amounts.length); + + // For each batch of token + for (uint256 i = 0; i < length; ++i) { + _mintConsecutive(receivers[i], amounts[i]); + } + } + + function _ownerOf(uint256 tokenId) internal view virtual override returns (address) { + address owner = super._ownerOf(tokenId); + + // If token is owned by the core, or beyound consecutive range, return base value + if (owner != address(0) || tokenId > type(uint96).max) { + return owner; + } + + // Otherwize, check the token was not burned, and fetch ownership from the anchors + // Note: no need for safe cast, we know that tokenId <= type(uint96).max + return + _sequentialBurn.get(tokenId) + ? address(0) + : address(_sequentialOwnership.lowerLookup(uint96(tokenId))); + } + + function _mintConsecutive(address to, uint96 batchSize) internal virtual { + require(!Address.isContract(address(this)), "ERC721Consecutive: batch minting restricted to constructor"); + + require(to != address(0), "ERC721Consecutive: mint to the zero address"); + require(batchSize > 0, "ERC721Consecutive: empty batch"); + require(batchSize < 5000, "ERC721Consecutive: batches too large for indexing"); + + uint96 first = _totalConsecutiveSupply(); + uint96 last = first + batchSize - 1; + + // hook before + _beforeConsecutiveTokenTransfer(address(0), to, first, last); + + // push an ownership checkpoint & emit event + _sequentialOwnership.push(SafeCast.toUint96(last), uint160(to)); + emit ConsecutiveTransfer(first, last, address(0), to); + + // hook after + _afterConsecutiveTokenTransfer(address(0), to, first, last); + } + + function _mint(address to, uint256 tokenId) internal virtual override { + // During construction, minting should only be performed using the batch mechanism. + // This is necessary because interleaving mint and batchmint would cause issues. + require(Address.isContract(address(this)), "ERC721Consecutive: cant mint durring construction"); + + super._mint(to, tokenId); + if (_sequentialBurn.get(tokenId)) { + _sequentialBurn.unset(tokenId); + } + } + + function _burn(uint256 tokenId) internal virtual override { + super._burn(tokenId); + if (tokenId <= _totalConsecutiveSupply()) { + _sequentialBurn.set(tokenId); + } + } + + function _totalConsecutiveSupply() private view returns (uint96) { + uint256 length = _sequentialOwnership.length; + return length == 0 ? 0 : _sequentialOwnership[length - 1]._key + 1; + } +} diff --git a/contracts/token/ERC721/extensions/ERC721Enumerable.sol b/contracts/token/ERC721/extensions/ERC721Enumerable.sol index 46afd5d0b0c..338d39edf7b 100644 --- a/contracts/token/ERC721/extensions/ERC721Enumerable.sol +++ b/contracts/token/ERC721/extensions/ERC721Enumerable.sol @@ -88,6 +88,48 @@ abstract contract ERC721Enumerable is ERC721, IERC721Enumerable { } } + /** + * @dev Hook that is called before any batch token transfer. For now this is limited + * to batch minting by the {ERC721Consecutive} extension. + * + * Calling conditions: + * + * - When `from` and `to` are both non-zero, ``from``'s `tokenId` will be + * transferred to `to`. + * - When `from` is zero, `tokenId` will be minted for `to`. + * - When `to` is zero, ``from``'s `tokenId` will be burned. + * - `from` cannot be the zero address. + * - `to` cannot be the zero address. + * + * To learn more about hooks, head to xref:ROOT:extending-contracts.adoc#using-hooks[Using Hooks]. + */ + function _beforeConsecutiveTokenTransfer( + address from, + address to, + uint256 first, + uint256 last + ) internal virtual override { + require(from == address(0) && to != address(0), "ERC721Enumerable: only batch minting is supported"); + + // Before balance is updated (that is part of the super call) + uint256 length = ERC721.balanceOf(to); + + // Do the super call + super._beforeConsecutiveTokenTransfer(from, to, first, last); + + // Add to enumerability + for (uint256 tokenId = first; tokenId <= last; ++tokenId) { + // Add to all tokens + _addTokenToAllTokensEnumeration(tokenId); + + // Add to owner tokens + _ownedTokens[to][length] = tokenId; + _ownedTokensIndex[tokenId] = length; + + ++length; + } + } + /** * @dev Private function to add a token to this extension's ownership-tracking data structures. * @param to address representing the new owner of the given token ID diff --git a/contracts/token/ERC721/extensions/ERC721Pausable.sol b/contracts/token/ERC721/extensions/ERC721Pausable.sol index fbf8b638236..eb8bf2f611a 100644 --- a/contracts/token/ERC721/extensions/ERC721Pausable.sol +++ b/contracts/token/ERC721/extensions/ERC721Pausable.sol @@ -30,4 +30,15 @@ abstract contract ERC721Pausable is ERC721, Pausable { require(!paused(), "ERC721Pausable: token transfer while paused"); } + + function _beforeConsecutiveTokenTransfer( + address from, + address to, + uint256 first, + uint256 last + ) internal virtual override { + super._beforeConsecutiveTokenTransfer(from, to, first, last); + + require(!paused(), "ERC721Pausable: token transfer while paused"); + } } diff --git a/contracts/token/ERC721/extensions/draft-ERC721Votes.sol b/contracts/token/ERC721/extensions/draft-ERC721Votes.sol index b4ec91eab5e..5af0f958f04 100644 --- a/contracts/token/ERC721/extensions/draft-ERC721Votes.sol +++ b/contracts/token/ERC721/extensions/draft-ERC721Votes.sol @@ -31,6 +31,21 @@ abstract contract ERC721Votes is ERC721, Votes { super._afterTokenTransfer(from, to, tokenId); } + /** + * @dev Adjusts votes when a batch of tokens is transferred. + * + * Emits a {Votes-DelegateVotesChanged} event. + */ + function _afterConsecutiveTokenTransfer( + address from, + address to, + uint256 first, + uint256 last + ) internal virtual override { + _transferVotingUnits(from, to, last - first + 1); + super._afterConsecutiveTokenTransfer(from, to, first, last); + } + /** * @dev Returns the balance of `account`. */ diff --git a/contracts/token/ERC721/presets/ERC721PresetMinterPauserAutoId.sol b/contracts/token/ERC721/presets/ERC721PresetMinterPauserAutoId.sol index 11b9787800d..11b97564e7c 100644 --- a/contracts/token/ERC721/presets/ERC721PresetMinterPauserAutoId.sol +++ b/contracts/token/ERC721/presets/ERC721PresetMinterPauserAutoId.sol @@ -124,6 +124,15 @@ contract ERC721PresetMinterPauserAutoId is super._beforeTokenTransfer(from, to, tokenId); } + function _beforeConsecutiveTokenTransfer( + address from, + address to, + uint256 first, + uint256 last + ) internal virtual override(ERC721, ERC721Enumerable, ERC721Pausable) { + super._beforeConsecutiveTokenTransfer(from, to, first, last); + } + /** * @dev See {IERC165-supportsInterface}. */ diff --git a/test/token/ERC721/extensions/ERC721Consecutive.test.js b/test/token/ERC721/extensions/ERC721Consecutive.test.js new file mode 100644 index 00000000000..6d9736f13a2 --- /dev/null +++ b/test/token/ERC721/extensions/ERC721Consecutive.test.js @@ -0,0 +1,109 @@ +const { constants, expectEvent, expectRevert } = require('@openzeppelin/test-helpers'); +const { expect } = require('chai'); + +const ERC721ConsecutiveMock = artifacts.require('ERC721ConsecutiveMock'); + +contract('ERC721Consecutive', function (accounts) { + const [ user1, user2, receiver ] = accounts; + + const name = 'Non Fungible Token'; + const symbol = 'NFT'; + const batches = [ + { receiver: user1, amount: 3 }, + { receiver: user2, amount: 5 }, + { receiver: user1, amount: 7 }, + ]; + + beforeEach(async function () { + this.token = await ERC721ConsecutiveMock.new( + name, + symbol, + batches.map(({ receiver }) => receiver), + batches.map(({ amount }) => amount), + ); + }); + + describe('batch are minted', function () { + it('events are emitted at construction', async function () { + let first = 0; + + for (const batch of batches) { + await expectEvent.inTransaction(this.token.transactionHash, this.token, 'ConsecutiveTransfer', { + fromTokenId: web3.utils.toBN(first), + toTokenId: web3.utils.toBN(first + batch.amount - 1), + fromAddress: constants.ZERO_ADDRESS, + toAddress: batch.receiver, + }); + + first += batch.amount; + } + }); + + it('ownership is set', async function () { + const owners = batches.flatMap(({ receiver, amount }) => Array(amount).fill(receiver)); + + for (const tokenId in owners) { + expect(await this.token.ownerOf(tokenId)) + .to.be.equal(owners[tokenId]); + } + }); + + it('balance & voting poser are set', async function () { + for (const account of accounts) { + const balance = batches + .filter(({ receiver }) => receiver === account) + .map(({ amount }) => amount) + .reduce((a, b) => a + b, 0); + + expect(await this.token.balanceOf(account)) + .to.be.bignumber.equal(web3.utils.toBN(balance)); + + expect(await this.token.getVotes(account)) + .to.be.bignumber.equal(web3.utils.toBN(balance)); + } + }); + + it('enumerability correctly set', async function () { + const owners = batches.flatMap(({ receiver, amount }) => Array(amount).fill(receiver)); + + expect(await this.token.totalSupply()) + .to.be.bignumber.equal(web3.utils.toBN(owners.length)); + + for (const tokenId in owners) { + expect(await this.token.tokenByIndex(tokenId)) + .to.be.bignumber.equal(web3.utils.toBN(tokenId)); + } + + for (const account of accounts) { + const owned = Object.entries(owners) + .filter(([ _, owner ]) => owner === account) + .map(([ tokenId, _ ]) => tokenId); + + for (const i in owned) { + expect(await this.token.tokenOfOwnerByIndex(account, i).then(x => x.toString())) + .to.be.bignumber.equal(web3.utils.toBN(owned[i])); + } + } + }); + }); + + describe('ERC721 behavior', function () { + it('core takes over ownership on transfer', async function () { + await this.token.transferFrom(user1, receiver, 1, { from: user1 }); + + expect(await this.token.ownerOf(1)).to.be.equal(receiver); + }); + + it('tokens can be burned and re-minted', async function () { + const receipt1 = await this.token.burn(1, { from: user1 }); + expectEvent(receipt1, 'Transfer', { from: user1, to: constants.ZERO_ADDRESS, tokenId: '1' }); + + await expectRevert(this.token.ownerOf(1), 'ERC721: invalid token ID'); + + const receipt2 = await this.token.mint(user2, 1); + expectEvent(receipt2, 'Transfer', { from: constants.ZERO_ADDRESS, to: user2, tokenId: '1' }); + + expect(await this.token.ownerOf(1)).to.be.equal(user2); + }); + }); +}); From 0a5d397862b1fb945009ad0ede6891eae75ddbb4 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 10 Aug 2022 17:44:57 +0200 Subject: [PATCH 17/46] improvement & cleanup --- contracts/mocks/ERC721ConsecutiveMock.sol | 15 ++++--- contracts/token/ERC721/ERC721.sol | 16 ++++--- .../ERC721/extensions/ERC721Consecutive.sol | 43 +++++++++++-------- .../extensions/ERC721Consecutive.test.js | 17 +++++++- 4 files changed, 62 insertions(+), 29 deletions(-) diff --git a/contracts/mocks/ERC721ConsecutiveMock.sol b/contracts/mocks/ERC721ConsecutiveMock.sol index c4845e22bbc..41e76b29bea 100644 --- a/contracts/mocks/ERC721ConsecutiveMock.sol +++ b/contracts/mocks/ERC721ConsecutiveMock.sol @@ -33,12 +33,11 @@ contract ERC721ConsecutiveMock is string memory symbol, address[] memory receivers, uint96[] memory amounts - ) - __VotesDelegationInConstructor(receivers) - ERC721(name, symbol) - ERC721Consecutive(receivers, amounts) - EIP712(name, "1") - {} + ) __VotesDelegationInConstructor(receivers) ERC721(name, symbol) EIP712(name, "1") { + for (uint256 i = 0; i < receivers.length; ++i) { + _mintConsecutive(receivers[i], amounts[i]); + } + } function pause() external { _pause(); @@ -66,6 +65,10 @@ contract ERC721ConsecutiveMock is _mint(to, tokenId); } + function mintConsecutive(address to, uint96 amount) public { + _mintConsecutive(to, amount); + } + function safeMint(address to, uint256 tokenId) public { _safeMint(to, tokenId); } diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index 77601ce8e05..33df114d7e4 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -424,8 +424,7 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata { } /** - * @dev Hook that is called before any token transfer. This includes minting - * and burning. + * @dev Hook that is called before any (single) token transfer. This includes minting and burning. * * Calling conditions: * @@ -444,8 +443,7 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata { ) internal virtual {} /** - * @dev Hook that is called after any transfer of tokens. This includes - * minting and burning. + * @dev Hook that is called after any (single) transfer of tokens. This includes minting and burning. * * Calling conditions: * @@ -461,7 +459,11 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata { ) internal virtual {} /** - * TODO + * @dev Hook that is called before consecutive token transfers. + * Calling conditions are similar to {_beforeTokenTransfer}. + * + * The default implementation include balances updates that extensions such as {ERC721Consecutive} cannot performe + * directly. */ function _beforeConsecutiveTokenTransfer( address from, @@ -477,6 +479,10 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata { } } + /** + * @dev Hook that is called after consecutive token transfers. + * Calling conditions are similar to {_afterTokenTransfer}. + */ function _afterConsecutiveTokenTransfer( address from, address to, diff --git a/contracts/token/ERC721/extensions/ERC721Consecutive.sol b/contracts/token/ERC721/extensions/ERC721Consecutive.sol index 8894692cfd5..04913d51dd6 100644 --- a/contracts/token/ERC721/extensions/ERC721Consecutive.sol +++ b/contracts/token/ERC721/extensions/ERC721Consecutive.sol @@ -25,17 +25,10 @@ abstract contract ERC721Consecutive is ERC721 { address indexed toAddress ); - constructor(address[] memory receivers, uint96[] memory amounts) { - // Check input length - uint256 length = receivers.length; - require(length == amounts.length); - - // For each batch of token - for (uint256 i = 0; i < length; ++i) { - _mintConsecutive(receivers[i], amounts[i]); - } - } - + /** + * @dev See {ERC721-_ownerOf}. Override version that checks the sequential ownership structure for tokens that have + * been minted as part of a batch, and not yet transfered. + */ function _ownerOf(uint256 tokenId) internal view virtual override returns (address) { address owner = super._ownerOf(tokenId); @@ -46,12 +39,20 @@ abstract contract ERC721Consecutive is ERC721 { // Otherwize, check the token was not burned, and fetch ownership from the anchors // Note: no need for safe cast, we know that tokenId <= type(uint96).max - return - _sequentialBurn.get(tokenId) - ? address(0) - : address(_sequentialOwnership.lowerLookup(uint96(tokenId))); + return _sequentialBurn.get(tokenId) ? address(0) : address(_sequentialOwnership.lowerLookup(uint96(tokenId))); } + /** + * @dev Mint a batch of token of length `batchSize` for `to`. + * + * WARNING: Consecutive mint is only available during construction. ERC721 requires that any minting done after + * construction emits a Transfer event, which is not the case of mints performed using this function. + * + * WARNING: Consecutive mint is limited to batches of 5000 tokens. Further minting is possible from a contract's + * point of view but would cause indexing issues for off-chain services. + * + * Emits a {ConsecutiveTransfer} event. + */ function _mintConsecutive(address to, uint96 batchSize) internal virtual { require(!Address.isContract(address(this)), "ERC721Consecutive: batch minting restricted to constructor"); @@ -73,9 +74,13 @@ abstract contract ERC721Consecutive is ERC721 { _afterConsecutiveTokenTransfer(address(0), to, first, last); } + /** + * @dev See {ERC721-_mint}. Override version that restricts normal minting to after construction. + * + * Warning: Using {ERC721Consecutive} prevents using {_mint} during construction in favor of {_mintConsecutive}. + * After construction, {_mintConsecutive} is no longer available and {_mint} becomes available. + */ function _mint(address to, uint256 tokenId) internal virtual override { - // During construction, minting should only be performed using the batch mechanism. - // This is necessary because interleaving mint and batchmint would cause issues. require(Address.isContract(address(this)), "ERC721Consecutive: cant mint durring construction"); super._mint(to, tokenId); @@ -84,6 +89,10 @@ abstract contract ERC721Consecutive is ERC721 { } } + /** + * @dev See {ERC721-_mint}. Override needed to avoid looking up in the sequential ownership structure for burnt + * token. + */ function _burn(uint256 tokenId) internal virtual override { super._burn(tokenId); if (tokenId <= _totalConsecutiveSupply()) { diff --git a/test/token/ERC721/extensions/ERC721Consecutive.test.js b/test/token/ERC721/extensions/ERC721Consecutive.test.js index 6d9736f13a2..bbb065f0be4 100644 --- a/test/token/ERC721/extensions/ERC721Consecutive.test.js +++ b/test/token/ERC721/extensions/ERC721Consecutive.test.js @@ -23,7 +23,7 @@ contract('ERC721Consecutive', function (accounts) { ); }); - describe('batch are minted', function () { + describe('minting during construction', function () { it('events are emitted at construction', async function () { let first = 0; @@ -87,6 +87,21 @@ contract('ERC721Consecutive', function (accounts) { }); }); + describe('minting after construction', function () { + it('consecutive minting is not possible after construction', async function () { + await expectRevert( + this.token.mintConsecutive(user1, 10), + 'ERC721Consecutive: batch minting restricted to constructor', + ); + }); + + it('simple minting is possible after construction', async function () { + const tokenId = batches.reduce((acc, { amount }) => acc + amount, 0) + 1; + const receipt = await this.token.mint(user1, tokenId); + expectEvent(receipt, 'Transfer', { from: constants.ZERO_ADDRESS, to: user1, tokenId: tokenId.toString() }); + }); + }); + describe('ERC721 behavior', function () { it('core takes over ownership on transfer', async function () { await this.token.transferFrom(user1, receiver, 1, { from: user1 }); From fb48fcfbc78a4c85aa4f59e410f1e02238566e2f Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 10 Aug 2022 17:54:14 +0200 Subject: [PATCH 18/46] documentation --- contracts/token/ERC721/README.adoc | 3 +++ .../token/ERC721/extensions/ERC721Consecutive.sol | 12 ++++++++++-- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/contracts/token/ERC721/README.adoc b/contracts/token/ERC721/README.adoc index 92959576bc1..b3377afeff3 100644 --- a/contracts/token/ERC721/README.adoc +++ b/contracts/token/ERC721/README.adoc @@ -22,6 +22,7 @@ OpenZeppelin Contracts provides implementations of all four interfaces: Additionally there are a few of other extensions: +* {ERC721Consecutive}: An implementation of https://eips.ethereum.org/EIPS/eip-2309[ERC2309] for minting batchs of tokens during construction, in accordance with ERC721. * {ERC721URIStorage}: A more flexible but more expensive way of storing metadata. * {ERC721Votes}: Support for voting and vote delegation. * {ERC721Royalty}: A way to signal royalty information following ERC2981. @@ -50,6 +51,8 @@ NOTE: This core set of contracts is designed to be unopinionated, allowing devel {{ERC721Burnable}} +{{ERC721Consecutive}} + {{ERC721URIStorage}} {{ERC721Votes}} diff --git a/contracts/token/ERC721/extensions/ERC721Consecutive.sol b/contracts/token/ERC721/extensions/ERC721Consecutive.sol index 04913d51dd6..02a0cac2ed8 100644 --- a/contracts/token/ERC721/extensions/ERC721Consecutive.sol +++ b/contracts/token/ERC721/extensions/ERC721Consecutive.sol @@ -1,5 +1,4 @@ // SPDX-License-Identifier: MIT -// OpenZeppelin Contracts v4.4.1 (token/ERC721/extensions/ERC721Burnable.sol) pragma solidity ^0.8.0; @@ -9,7 +8,16 @@ import "../../../utils/math/SafeCast.sol"; import "../../../utils/structs/BitMaps.sol"; /** - * @title ERC721 Cheap sequential minting + * * @dev Implementation of the ERC2309 "Consecutive Transfer Extension" as defined in + * https://eips.ethereum.org/EIPS/eip-2309[EIP-2309]. + * + * This extension allows the minting of large batches of tokens duting the contract construction. These batches are + * limited to 5000 tokens at a time to accomodate off-chain indexers. + * + * Using this extensions removes the ability to mint single token buring the contract construction. This ability is + * regained after construction. During construction, only batch minting is allowed. + * + * _Available since v4.8._ */ abstract contract ERC721Consecutive is ERC721 { using BitMaps for BitMaps.BitMap; From dac9814fad9098795fdc28ed85714e71e254beb7 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 30 Aug 2022 21:40:47 +0200 Subject: [PATCH 19/46] fix merge --- .../extensions/GovernorVotesQuorumFraction.sol | 6 +++--- contracts/token/ERC721/ERC721.sol | 2 +- .../token/ERC721/extensions/ERC721Consecutive.sol | 14 +++++++------- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/contracts/governance/extensions/GovernorVotesQuorumFraction.sol b/contracts/governance/extensions/GovernorVotesQuorumFraction.sol index fe18d699be5..1f19fc33ea6 100644 --- a/contracts/governance/extensions/GovernorVotesQuorumFraction.sol +++ b/contracts/governance/extensions/GovernorVotesQuorumFraction.sol @@ -50,8 +50,8 @@ abstract contract GovernorVotesQuorumFraction is GovernorVotes { } // Optimistic search, check the latest checkpoint - Checkpoints.Checkpoint224 memory latest = _quorumNumeratorHistory._checkpoints[length - 1]; - if (latest._key <= blockNumber) { + Checkpoints.Checkpoint memory latest = _quorumNumeratorHistory._checkpoints[length - 1]; + if (latest._blockNumber <= blockNumber) { return latest._value; } @@ -107,7 +107,7 @@ abstract contract GovernorVotesQuorumFraction is GovernorVotes { // Make sure we keep track of the original numerator in contracts upgraded from a version without checkpoints. if (oldQuorumNumerator != 0 && _quorumNumeratorHistory._checkpoints.length == 0) { _quorumNumeratorHistory._checkpoints.push( - Checkpoints.Checkpoint224({_key: 0, _value: SafeCast.toUint224(oldQuorumNumerator)}) + Checkpoints.Checkpoint({_blockNumber: 0, _value: SafeCast.toUint224(oldQuorumNumerator)}) ); } diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index 847f55f6114..ea3ad58b5bf 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -489,7 +489,7 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata { * @dev Hook that is called before consecutive token transfers. * Calling conditions are similar to {_beforeTokenTransfer}. * - * The default implementation include balances updates that extensions such as {ERC721Consecutive} cannot performe + * The default implementation include balances updates that extensions such as {ERC721Consecutive} cannot perform * directly. */ function _beforeConsecutiveTokenTransfer( diff --git a/contracts/token/ERC721/extensions/ERC721Consecutive.sol b/contracts/token/ERC721/extensions/ERC721Consecutive.sol index 02a0cac2ed8..459cc24e765 100644 --- a/contracts/token/ERC721/extensions/ERC721Consecutive.sol +++ b/contracts/token/ERC721/extensions/ERC721Consecutive.sol @@ -11,10 +11,10 @@ import "../../../utils/structs/BitMaps.sol"; * * @dev Implementation of the ERC2309 "Consecutive Transfer Extension" as defined in * https://eips.ethereum.org/EIPS/eip-2309[EIP-2309]. * - * This extension allows the minting of large batches of tokens duting the contract construction. These batches are - * limited to 5000 tokens at a time to accomodate off-chain indexers. + * This extension allows the minting of large batches of tokens during the contract construction. These batches are + * limited to 5000 tokens at a time to accommodate off-chain indexers. * - * Using this extensions removes the ability to mint single token buring the contract construction. This ability is + * Using this extensions removes the ability to mint single token during the contract construction. This ability is * regained after construction. During construction, only batch minting is allowed. * * _Available since v4.8._ @@ -35,17 +35,17 @@ abstract contract ERC721Consecutive is ERC721 { /** * @dev See {ERC721-_ownerOf}. Override version that checks the sequential ownership structure for tokens that have - * been minted as part of a batch, and not yet transfered. + * been minted as part of a batch, and not yet transferred. */ function _ownerOf(uint256 tokenId) internal view virtual override returns (address) { address owner = super._ownerOf(tokenId); - // If token is owned by the core, or beyound consecutive range, return base value + // If token is owned by the core, or beyond consecutive range, return base value if (owner != address(0) || tokenId > type(uint96).max) { return owner; } - // Otherwize, check the token was not burned, and fetch ownership from the anchors + // Otherwise, check the token was not burned, and fetch ownership from the anchors // Note: no need for safe cast, we know that tokenId <= type(uint96).max return _sequentialBurn.get(tokenId) ? address(0) : address(_sequentialOwnership.lowerLookup(uint96(tokenId))); } @@ -89,7 +89,7 @@ abstract contract ERC721Consecutive is ERC721 { * After construction, {_mintConsecutive} is no longer available and {_mint} becomes available. */ function _mint(address to, uint256 tokenId) internal virtual override { - require(Address.isContract(address(this)), "ERC721Consecutive: cant mint durring construction"); + require(Address.isContract(address(this)), "ERC721Consecutive: can't mint during construction"); super._mint(to, tokenId); if (_sequentialBurn.get(tokenId)) { From 76f914b17aedd5c5a9c8fddd86c70e9959a5026b Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 31 Aug 2022 10:26:15 +0200 Subject: [PATCH 20/46] simplify mocks --- contracts/mocks/ERC721ConsecutiveMock.sol | 13 ++----------- .../token/ERC721/extensions/ERC721Consecutive.sol | 8 ++++---- 2 files changed, 6 insertions(+), 15 deletions(-) diff --git a/contracts/mocks/ERC721ConsecutiveMock.sol b/contracts/mocks/ERC721ConsecutiveMock.sol index 41e76b29bea..b3214c2432e 100644 --- a/contracts/mocks/ERC721ConsecutiveMock.sol +++ b/contracts/mocks/ERC721ConsecutiveMock.sol @@ -8,20 +8,10 @@ import "../token/ERC721/extensions/ERC721Enumerable.sol"; import "../token/ERC721/extensions/ERC721Pausable.sol"; import "../token/ERC721/extensions/draft-ERC721Votes.sol"; -/* solhint-disable-next-line contract-name-camelcase */ -abstract contract __VotesDelegationInConstructor is Votes { - constructor(address[] memory accounts) { - for (uint256 i; i < accounts.length; ++i) { - _delegate(accounts[i], accounts[i]); - } - } -} - /** * @title ERC721ConsecutiveMock */ contract ERC721ConsecutiveMock is - __VotesDelegationInConstructor, ERC721Burnable, ERC721Consecutive, ERC721Enumerable, @@ -33,8 +23,9 @@ contract ERC721ConsecutiveMock is string memory symbol, address[] memory receivers, uint96[] memory amounts - ) __VotesDelegationInConstructor(receivers) ERC721(name, symbol) EIP712(name, "1") { + ) ERC721(name, symbol) EIP712(name, "1") { for (uint256 i = 0; i < receivers.length; ++i) { + _delegate(receivers[i], receivers[i]); _mintConsecutive(receivers[i], amounts[i]); } } diff --git a/contracts/token/ERC721/extensions/ERC721Consecutive.sol b/contracts/token/ERC721/extensions/ERC721Consecutive.sol index 459cc24e765..eecf019892c 100644 --- a/contracts/token/ERC721/extensions/ERC721Consecutive.sol +++ b/contracts/token/ERC721/extensions/ERC721Consecutive.sol @@ -21,9 +21,9 @@ import "../../../utils/structs/BitMaps.sol"; */ abstract contract ERC721Consecutive is ERC721 { using BitMaps for BitMaps.BitMap; - using Checkpoints for Checkpoints.Checkpoint160[]; + using Checkpoints for Checkpoints.Trace160; - Checkpoints.Checkpoint160[] private _sequentialOwnership; + Checkpoints.Trace160 private _sequentialOwnership; BitMaps.BitMap private _sequentialBurn; event ConsecutiveTransfer( @@ -109,7 +109,7 @@ abstract contract ERC721Consecutive is ERC721 { } function _totalConsecutiveSupply() private view returns (uint96) { - uint256 length = _sequentialOwnership.length; - return length == 0 ? 0 : _sequentialOwnership[length - 1]._key + 1; + uint256 length = _sequentialOwnership._checkpoints.length; + return length == 0 ? 0 : _sequentialOwnership._checkpoints[length - 1]._key + 1; } } From 70b0ba594a7683b3eeea8301165e116e72df5129 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 31 Aug 2022 10:32:38 +0200 Subject: [PATCH 21/46] don't revert on no-op --- contracts/token/ERC721/extensions/ERC721Consecutive.sol | 5 +++-- test/token/ERC721/extensions/ERC721Consecutive.test.js | 6 +++++- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/contracts/token/ERC721/extensions/ERC721Consecutive.sol b/contracts/token/ERC721/extensions/ERC721Consecutive.sol index eecf019892c..7ca59a8ea97 100644 --- a/contracts/token/ERC721/extensions/ERC721Consecutive.sol +++ b/contracts/token/ERC721/extensions/ERC721Consecutive.sol @@ -62,10 +62,11 @@ abstract contract ERC721Consecutive is ERC721 { * Emits a {ConsecutiveTransfer} event. */ function _mintConsecutive(address to, uint96 batchSize) internal virtual { - require(!Address.isContract(address(this)), "ERC721Consecutive: batch minting restricted to constructor"); + // minting a batch of size 0 is a no-op + if (batchSize == 0) return; + require(!Address.isContract(address(this)), "ERC721Consecutive: batch minting restricted to constructor"); require(to != address(0), "ERC721Consecutive: mint to the zero address"); - require(batchSize > 0, "ERC721Consecutive: empty batch"); require(batchSize < 5000, "ERC721Consecutive: batches too large for indexing"); uint96 first = _totalConsecutiveSupply(); diff --git a/test/token/ERC721/extensions/ERC721Consecutive.test.js b/test/token/ERC721/extensions/ERC721Consecutive.test.js index bbb065f0be4..cec8cdfe37e 100644 --- a/test/token/ERC721/extensions/ERC721Consecutive.test.js +++ b/test/token/ERC721/extensions/ERC721Consecutive.test.js @@ -4,13 +4,15 @@ const { expect } = require('chai'); const ERC721ConsecutiveMock = artifacts.require('ERC721ConsecutiveMock'); contract('ERC721Consecutive', function (accounts) { - const [ user1, user2, receiver ] = accounts; + const [ user1, user2, user3, receiver ] = accounts; const name = 'Non Fungible Token'; const symbol = 'NFT'; const batches = [ + { receiver: user1, amount: 0 }, { receiver: user1, amount: 3 }, { receiver: user2, amount: 5 }, + { receiver: user3, amount: 0 }, { receiver: user1, amount: 7 }, ]; @@ -28,6 +30,8 @@ contract('ERC721Consecutive', function (accounts) { let first = 0; for (const batch of batches) { + if (batch.amount == 0) continue; + await expectEvent.inTransaction(this.token.transactionHash, this.token, 'ConsecutiveTransfer', { fromTokenId: web3.utils.toBN(first), toTokenId: web3.utils.toBN(first + batch.amount - 1), From d3c7bd6f2e3acbe673b6a6e8892cdf7af8b098cb Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 1 Sep 2022 09:56:21 +0200 Subject: [PATCH 22/46] Apply suggestions from code review Co-authored-by: Francisco --- contracts/token/ERC721/ERC721.sol | 2 ++ contracts/token/ERC721/extensions/ERC721Consecutive.sol | 5 ++++- test/token/ERC721/extensions/ERC721Consecutive.test.js | 2 +- 3 files changed, 7 insertions(+), 2 deletions(-) diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index ea3ad58b5bf..07699094b3a 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -452,6 +452,7 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata { /** * @dev Hook that is called before any (single) token transfer. This includes minting and burning. + * See {_beforeConsecutiveTokenTransfer}. * * Calling conditions: * @@ -471,6 +472,7 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata { /** * @dev Hook that is called after any (single) transfer of tokens. This includes minting and burning. + * See {_afterConsecutiveTokenTransfer}. * * Calling conditions: * diff --git a/contracts/token/ERC721/extensions/ERC721Consecutive.sol b/contracts/token/ERC721/extensions/ERC721Consecutive.sol index 7ca59a8ea97..1b4481cf8fc 100644 --- a/contracts/token/ERC721/extensions/ERC721Consecutive.sol +++ b/contracts/token/ERC721/extensions/ERC721Consecutive.sol @@ -8,7 +8,7 @@ import "../../../utils/math/SafeCast.sol"; import "../../../utils/structs/BitMaps.sol"; /** - * * @dev Implementation of the ERC2309 "Consecutive Transfer Extension" as defined in + * @dev Implementation of the ERC2309 "Consecutive Transfer Extension" as defined in * https://eips.ethereum.org/EIPS/eip-2309[EIP-2309]. * * This extension allows the minting of large batches of tokens during the contract construction. These batches are @@ -17,6 +17,8 @@ import "../../../utils/structs/BitMaps.sol"; * Using this extensions removes the ability to mint single token during the contract construction. This ability is * regained after construction. During construction, only batch minting is allowed. * + * IMPORTANT: This extension bypasses the hooks `_beforeTokenTransfer` and `_afterTokenTransfer` + * * _Available since v4.8._ */ abstract contract ERC721Consecutive is ERC721 { @@ -103,6 +105,7 @@ abstract contract ERC721Consecutive is ERC721 { * token. */ function _burn(uint256 tokenId) internal virtual override { + // Invoke the core burn functionality to adjust balances, invoke hooks, etc. super._burn(tokenId); if (tokenId <= _totalConsecutiveSupply()) { _sequentialBurn.set(tokenId); diff --git a/test/token/ERC721/extensions/ERC721Consecutive.test.js b/test/token/ERC721/extensions/ERC721Consecutive.test.js index cec8cdfe37e..9490559d991 100644 --- a/test/token/ERC721/extensions/ERC721Consecutive.test.js +++ b/test/token/ERC721/extensions/ERC721Consecutive.test.js @@ -52,7 +52,7 @@ contract('ERC721Consecutive', function (accounts) { } }); - it('balance & voting poser are set', async function () { + it('balance & voting power are set', async function () { for (const account of accounts) { const balance = batches .filter(({ receiver }) => receiver === account) From 7f625ef23cc11c58045a2e4161dfcee400e34ea2 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 1 Sep 2022 10:06:14 +0200 Subject: [PATCH 23/46] Change params of _xxxxConsecutiveTokenTransfer hooks --- contracts/mocks/ERC721ConsecutiveMock.sol | 8 ++++---- contracts/token/ERC721/ERC721.sol | 16 ++++++++-------- .../ERC721/extensions/ERC721Consecutive.sol | 4 ++-- .../token/ERC721/extensions/ERC721Enumerable.sol | 6 +++--- .../token/ERC721/extensions/ERC721Pausable.sol | 4 ++-- .../ERC721/extensions/draft-ERC721Votes.sol | 6 +++--- .../presets/ERC721PresetMinterPauserAutoId.sol | 4 ++-- 7 files changed, 24 insertions(+), 24 deletions(-) diff --git a/contracts/mocks/ERC721ConsecutiveMock.sol b/contracts/mocks/ERC721ConsecutiveMock.sol index b3214c2432e..b6a138b9eee 100644 --- a/contracts/mocks/ERC721ConsecutiveMock.sol +++ b/contracts/mocks/ERC721ConsecutiveMock.sol @@ -96,17 +96,17 @@ contract ERC721ConsecutiveMock is address from, address to, uint256 first, - uint256 last + uint96 size ) internal virtual override(ERC721, ERC721Enumerable, ERC721Pausable) { - super._beforeConsecutiveTokenTransfer(from, to, first, last); + super._beforeConsecutiveTokenTransfer(from, to, first, size); } function _afterConsecutiveTokenTransfer( address from, address to, uint256 first, - uint256 last + uint96 size ) internal virtual override(ERC721, ERC721Votes) { - super._afterConsecutiveTokenTransfer(from, to, first, last); + super._afterConsecutiveTokenTransfer(from, to, first, size); } } diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index ea3ad58b5bf..28b7e04aecc 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -495,14 +495,14 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata { function _beforeConsecutiveTokenTransfer( address from, address to, - uint256 first, - uint256 last + uint256 /*first*/, + uint96 size ) internal virtual { if (from != address(0)) { - _balances[from] -= last - first + 1; + _balances[from] -= size; } if (to != address(0)) { - _balances[to] += last - first + 1; + _balances[to] += size; } } @@ -511,9 +511,9 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata { * Calling conditions are similar to {_afterTokenTransfer}. */ function _afterConsecutiveTokenTransfer( - address from, - address to, - uint256 first, - uint256 last + address /*from*/, + address /*to*/, + uint256 /*first*/, + uint96 /*size*/ ) internal virtual {} } diff --git a/contracts/token/ERC721/extensions/ERC721Consecutive.sol b/contracts/token/ERC721/extensions/ERC721Consecutive.sol index 7ca59a8ea97..eb016d95211 100644 --- a/contracts/token/ERC721/extensions/ERC721Consecutive.sol +++ b/contracts/token/ERC721/extensions/ERC721Consecutive.sol @@ -73,14 +73,14 @@ abstract contract ERC721Consecutive is ERC721 { uint96 last = first + batchSize - 1; // hook before - _beforeConsecutiveTokenTransfer(address(0), to, first, last); + _beforeConsecutiveTokenTransfer(address(0), to, first, batchSize); // push an ownership checkpoint & emit event _sequentialOwnership.push(SafeCast.toUint96(last), uint160(to)); emit ConsecutiveTransfer(first, last, address(0), to); // hook after - _afterConsecutiveTokenTransfer(address(0), to, first, last); + _afterConsecutiveTokenTransfer(address(0), to, first, batchSize); } /** diff --git a/contracts/token/ERC721/extensions/ERC721Enumerable.sol b/contracts/token/ERC721/extensions/ERC721Enumerable.sol index 8a57f2c4e0d..ab7fe98d003 100644 --- a/contracts/token/ERC721/extensions/ERC721Enumerable.sol +++ b/contracts/token/ERC721/extensions/ERC721Enumerable.sol @@ -106,7 +106,7 @@ abstract contract ERC721Enumerable is ERC721, IERC721Enumerable { address from, address to, uint256 first, - uint256 last + uint96 size ) internal virtual override { require(from == address(0) && to != address(0), "ERC721Enumerable: only batch minting is supported"); @@ -114,10 +114,10 @@ abstract contract ERC721Enumerable is ERC721, IERC721Enumerable { uint256 length = ERC721.balanceOf(to); // Do the super call - super._beforeConsecutiveTokenTransfer(from, to, first, last); + super._beforeConsecutiveTokenTransfer(from, to, first, size); // Add to enumerability - for (uint256 tokenId = first; tokenId <= last; ++tokenId) { + for (uint256 tokenId = first; tokenId < first + size; ++tokenId) { // Add to all tokens _addTokenToAllTokensEnumeration(tokenId); diff --git a/contracts/token/ERC721/extensions/ERC721Pausable.sol b/contracts/token/ERC721/extensions/ERC721Pausable.sol index eb8bf2f611a..22163b0f7c9 100644 --- a/contracts/token/ERC721/extensions/ERC721Pausable.sol +++ b/contracts/token/ERC721/extensions/ERC721Pausable.sol @@ -35,9 +35,9 @@ abstract contract ERC721Pausable is ERC721, Pausable { address from, address to, uint256 first, - uint256 last + uint96 size ) internal virtual override { - super._beforeConsecutiveTokenTransfer(from, to, first, last); + super._beforeConsecutiveTokenTransfer(from, to, first, size); require(!paused(), "ERC721Pausable: token transfer while paused"); } diff --git a/contracts/token/ERC721/extensions/draft-ERC721Votes.sol b/contracts/token/ERC721/extensions/draft-ERC721Votes.sol index 5af0f958f04..1d23c78549d 100644 --- a/contracts/token/ERC721/extensions/draft-ERC721Votes.sol +++ b/contracts/token/ERC721/extensions/draft-ERC721Votes.sol @@ -40,10 +40,10 @@ abstract contract ERC721Votes is ERC721, Votes { address from, address to, uint256 first, - uint256 last + uint96 size ) internal virtual override { - _transferVotingUnits(from, to, last - first + 1); - super._afterConsecutiveTokenTransfer(from, to, first, last); + _transferVotingUnits(from, to, size); + super._afterConsecutiveTokenTransfer(from, to, first, size); } /** diff --git a/contracts/token/ERC721/presets/ERC721PresetMinterPauserAutoId.sol b/contracts/token/ERC721/presets/ERC721PresetMinterPauserAutoId.sol index 11b97564e7c..65ffef44b9b 100644 --- a/contracts/token/ERC721/presets/ERC721PresetMinterPauserAutoId.sol +++ b/contracts/token/ERC721/presets/ERC721PresetMinterPauserAutoId.sol @@ -128,9 +128,9 @@ contract ERC721PresetMinterPauserAutoId is address from, address to, uint256 first, - uint256 last + uint96 size ) internal virtual override(ERC721, ERC721Enumerable, ERC721Pausable) { - super._beforeConsecutiveTokenTransfer(from, to, first, last); + super._beforeConsecutiveTokenTransfer(from, to, first, size); } /** From ceeb03853fe09da0934e9a7c411df1988674a2dc Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 1 Sep 2022 10:09:29 +0200 Subject: [PATCH 24/46] address comments from the PR --- .../ERC721/extensions/ERC721Consecutive.sol | 31 ++++++++++--------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/contracts/token/ERC721/extensions/ERC721Consecutive.sol b/contracts/token/ERC721/extensions/ERC721Consecutive.sol index ecdacecd0ea..bcb056e5c24 100644 --- a/contracts/token/ERC721/extensions/ERC721Consecutive.sol +++ b/contracts/token/ERC721/extensions/ERC721Consecutive.sol @@ -63,26 +63,29 @@ abstract contract ERC721Consecutive is ERC721 { * * Emits a {ConsecutiveTransfer} event. */ - function _mintConsecutive(address to, uint96 batchSize) internal virtual { + function _mintConsecutive(address to, uint96 batchSize) internal virtual returns (uint96) { + uint96 first = _totalConsecutiveSupply(); + // minting a batch of size 0 is a no-op - if (batchSize == 0) return; + if (batchSize > 0) { + require(!Address.isContract(address(this)), "ERC721Consecutive: batch minting restricted to constructor"); + require(to != address(0), "ERC721Consecutive: mint to the zero address"); + require(batchSize <= 5000, "ERC721Consecutive: batches too large for indexing"); - require(!Address.isContract(address(this)), "ERC721Consecutive: batch minting restricted to constructor"); - require(to != address(0), "ERC721Consecutive: mint to the zero address"); - require(batchSize < 5000, "ERC721Consecutive: batches too large for indexing"); - uint96 first = _totalConsecutiveSupply(); - uint96 last = first + batchSize - 1; + // hook before + _beforeConsecutiveTokenTransfer(address(0), to, first, batchSize); - // hook before - _beforeConsecutiveTokenTransfer(address(0), to, first, batchSize); + // push an ownership checkpoint & emit event + uint96 last = first + batchSize - 1; + _sequentialOwnership.push(last, uint160(to)); + emit ConsecutiveTransfer(first, last, address(0), to); - // push an ownership checkpoint & emit event - _sequentialOwnership.push(SafeCast.toUint96(last), uint160(to)); - emit ConsecutiveTransfer(first, last, address(0), to); + // hook after + _afterConsecutiveTokenTransfer(address(0), to, first, batchSize); + } - // hook after - _afterConsecutiveTokenTransfer(address(0), to, first, batchSize); + return first; } /** From bb2155259efac6fa18225ac340b845931c255513 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 1 Sep 2022 11:27:24 +0200 Subject: [PATCH 25/46] more testing --- contracts/mocks/ERC721ConsecutiveMock.sol | 12 ++- .../extensions/ERC721Consecutive.test.js | 76 +++++++++++++++---- 2 files changed, 71 insertions(+), 17 deletions(-) diff --git a/contracts/mocks/ERC721ConsecutiveMock.sol b/contracts/mocks/ERC721ConsecutiveMock.sol index b6a138b9eee..d77a45c3ed5 100644 --- a/contracts/mocks/ERC721ConsecutiveMock.sol +++ b/contracts/mocks/ERC721ConsecutiveMock.sol @@ -21,11 +21,15 @@ contract ERC721ConsecutiveMock is constructor( string memory name, string memory symbol, + address[] memory delegates, address[] memory receivers, uint96[] memory amounts ) ERC721(name, symbol) EIP712(name, "1") { + for (uint256 i = 0; i < delegates.length; ++i) { + _delegate(delegates[i], delegates[i]); + } + for (uint256 i = 0; i < receivers.length; ++i) { - _delegate(receivers[i], receivers[i]); _mintConsecutive(receivers[i], amounts[i]); } } @@ -110,3 +114,9 @@ contract ERC721ConsecutiveMock is super._afterConsecutiveTokenTransfer(from, to, first, size); } } + +contract ERC721ConsecutiveNoConstructorMintMock is ERC721Consecutive { + constructor(string memory name, string memory symbol) ERC721(name, symbol) { + _mint(msg.sender, 0); + } +} \ No newline at end of file diff --git a/test/token/ERC721/extensions/ERC721Consecutive.test.js b/test/token/ERC721/extensions/ERC721Consecutive.test.js index 9490559d991..b9d4246c7bb 100644 --- a/test/token/ERC721/extensions/ERC721Consecutive.test.js +++ b/test/token/ERC721/extensions/ERC721Consecutive.test.js @@ -2,6 +2,7 @@ const { constants, expectEvent, expectRevert } = require('@openzeppelin/test-hel const { expect } = require('chai'); const ERC721ConsecutiveMock = artifacts.require('ERC721ConsecutiveMock'); +const ERC721ConsecutiveNoConstructorMintMock = artifacts.require('ERC721ConsecutiveNoConstructorMintMock'); contract('ERC721Consecutive', function (accounts) { const [ user1, user2, user3, receiver ] = accounts; @@ -15,11 +16,13 @@ contract('ERC721Consecutive', function (accounts) { { receiver: user3, amount: 0 }, { receiver: user1, amount: 7 }, ]; + const delegates = [ user1, user3 ]; beforeEach(async function () { this.token = await ERC721ConsecutiveMock.new( name, symbol, + delegates, batches.map(({ receiver }) => receiver), batches.map(({ amount }) => amount), ); @@ -30,15 +33,17 @@ contract('ERC721Consecutive', function (accounts) { let first = 0; for (const batch of batches) { - if (batch.amount == 0) continue; - - await expectEvent.inTransaction(this.token.transactionHash, this.token, 'ConsecutiveTransfer', { - fromTokenId: web3.utils.toBN(first), - toTokenId: web3.utils.toBN(first + batch.amount - 1), - fromAddress: constants.ZERO_ADDRESS, - toAddress: batch.receiver, - }); - + if (batch.amount > 0) { + // an event is emmited + await expectEvent.inConstruction(this.token, 'ConsecutiveTransfer', { + fromTokenId: web3.utils.toBN(first), + toTokenId: web3.utils.toBN(first + batch.amount - 1), + fromAddress: constants.ZERO_ADDRESS, + toAddress: batch.receiver, + }); + } else { + // expectEvent.notEmitted.inConstruction only looks at event name, and doesn't check the parameters + } first += batch.amount; } }); @@ -62,6 +67,15 @@ contract('ERC721Consecutive', function (accounts) { expect(await this.token.balanceOf(account)) .to.be.bignumber.equal(web3.utils.toBN(balance)); + // If not delegated at construction, check before + do delegation + if (!delegates.includes(account)) { + expect(await this.token.getVotes(account)) + .to.be.bignumber.equal(web3.utils.toBN(0)); + + await this.token.delegate(account, { from: account }); + } + + // At this point all accounts should have delegated expect(await this.token.getVotes(account)) .to.be.bignumber.equal(web3.utils.toBN(balance)); } @@ -89,6 +103,13 @@ contract('ERC721Consecutive', function (accounts) { } } }); + + it('cannot use single minting during construction', async function () { + await expectRevert( + ERC721ConsecutiveNoConstructorMintMock.new(name, symbol), + 'ERC721Consecutive: can\'t mint during construction', + ); + }); }); describe('minting after construction', function () { @@ -100,9 +121,26 @@ contract('ERC721Consecutive', function (accounts) { }); it('simple minting is possible after construction', async function () { - const tokenId = batches.reduce((acc, { amount }) => acc + amount, 0) + 1; - const receipt = await this.token.mint(user1, tokenId); - expectEvent(receipt, 'Transfer', { from: constants.ZERO_ADDRESS, to: user1, tokenId: tokenId.toString() }); + const tokenId = batches.reduce((acc, { amount }) => acc + amount, 0); + + expect(await this.token.exists(tokenId)).to.be.equal(false); + + expectEvent( + await this.token.mint(user1, tokenId), + 'Transfer', + { from: constants.ZERO_ADDRESS, to: user1, tokenId: tokenId.toString() }, + ); + }); + + it('cannot mint a token that has been batched minted', async function () { + const tokenId = batches.reduce((acc, { amount }) => acc + amount, 0) - 1; + + expect(await this.token.exists(tokenId)).to.be.equal(true); + + await expectRevert( + this.token.mint(user1, tokenId), + 'ERC721: token already minted', + ); }); }); @@ -114,13 +152,19 @@ contract('ERC721Consecutive', function (accounts) { }); it('tokens can be burned and re-minted', async function () { - const receipt1 = await this.token.burn(1, { from: user1 }); - expectEvent(receipt1, 'Transfer', { from: user1, to: constants.ZERO_ADDRESS, tokenId: '1' }); + expectEvent( + await this.token.burn(1, { from: user1 }), + 'Transfer', + { from: user1, to: constants.ZERO_ADDRESS, tokenId: '1' }, + ); await expectRevert(this.token.ownerOf(1), 'ERC721: invalid token ID'); - const receipt2 = await this.token.mint(user2, 1); - expectEvent(receipt2, 'Transfer', { from: constants.ZERO_ADDRESS, to: user2, tokenId: '1' }); + expectEvent( + await this.token.mint(user2, 1), + 'Transfer', + { from: constants.ZERO_ADDRESS, to: user2, tokenId: '1' }, + ); expect(await this.token.ownerOf(1)).to.be.equal(user2); }); From cb8dfcf6a10c72bb7c38c39fc04dd6939f2d8343 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 1 Sep 2022 19:35:26 +0200 Subject: [PATCH 26/46] fix lint --- contracts/mocks/ERC721ConsecutiveMock.sol | 10 ++-------- contracts/token/ERC721/ERC721.sol | 8 ++++---- .../token/ERC721/extensions/ERC721Consecutive.sol | 1 - 3 files changed, 6 insertions(+), 13 deletions(-) diff --git a/contracts/mocks/ERC721ConsecutiveMock.sol b/contracts/mocks/ERC721ConsecutiveMock.sol index d77a45c3ed5..174df641355 100644 --- a/contracts/mocks/ERC721ConsecutiveMock.sol +++ b/contracts/mocks/ERC721ConsecutiveMock.sol @@ -11,13 +11,7 @@ import "../token/ERC721/extensions/draft-ERC721Votes.sol"; /** * @title ERC721ConsecutiveMock */ -contract ERC721ConsecutiveMock is - ERC721Burnable, - ERC721Consecutive, - ERC721Enumerable, - ERC721Pausable, - ERC721Votes -{ +contract ERC721ConsecutiveMock is ERC721Burnable, ERC721Consecutive, ERC721Enumerable, ERC721Pausable, ERC721Votes { constructor( string memory name, string memory symbol, @@ -119,4 +113,4 @@ contract ERC721ConsecutiveNoConstructorMintMock is ERC721Consecutive { constructor(string memory name, string memory symbol) ERC721(name, symbol) { _mint(msg.sender, 0); } -} \ No newline at end of file +} diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index 78d27689137..a8e5aa4abc9 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -497,7 +497,7 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata { function _beforeConsecutiveTokenTransfer( address from, address to, - uint256 /*first*/, + uint256, /*first*/ uint96 size ) internal virtual { if (from != address(0)) { @@ -513,9 +513,9 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata { * Calling conditions are similar to {_afterTokenTransfer}. */ function _afterConsecutiveTokenTransfer( - address /*from*/, - address /*to*/, - uint256 /*first*/, + address, /*from*/ + address, /*to*/ + uint256, /*first*/ uint96 /*size*/ ) internal virtual {} } diff --git a/contracts/token/ERC721/extensions/ERC721Consecutive.sol b/contracts/token/ERC721/extensions/ERC721Consecutive.sol index bcb056e5c24..0aa075b1fac 100644 --- a/contracts/token/ERC721/extensions/ERC721Consecutive.sol +++ b/contracts/token/ERC721/extensions/ERC721Consecutive.sol @@ -72,7 +72,6 @@ abstract contract ERC721Consecutive is ERC721 { require(to != address(0), "ERC721Consecutive: mint to the zero address"); require(batchSize <= 5000, "ERC721Consecutive: batches too large for indexing"); - // hook before _beforeConsecutiveTokenTransfer(address(0), to, first, batchSize); From 8d198ee1316d1142abdd70a79c57f88563575bc8 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 1 Sep 2022 19:36:59 +0200 Subject: [PATCH 27/46] spelling --- test/token/ERC721/extensions/ERC721Consecutive.test.js | 1 - 1 file changed, 1 deletion(-) diff --git a/test/token/ERC721/extensions/ERC721Consecutive.test.js b/test/token/ERC721/extensions/ERC721Consecutive.test.js index b9d4246c7bb..1caf21c7c8a 100644 --- a/test/token/ERC721/extensions/ERC721Consecutive.test.js +++ b/test/token/ERC721/extensions/ERC721Consecutive.test.js @@ -34,7 +34,6 @@ contract('ERC721Consecutive', function (accounts) { for (const batch of batches) { if (batch.amount > 0) { - // an event is emmited await expectEvent.inConstruction(this.token, 'ConsecutiveTransfer', { fromTokenId: web3.utils.toBN(first), toTokenId: web3.utils.toBN(first + batch.amount - 1), From 7060d601539fc835716fae1cb8df17860eb9d050 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 2 Sep 2022 17:21:11 +0200 Subject: [PATCH 28/46] use hooks for explicit burning + docs --- .../ERC721/extensions/ERC721Consecutive.sol | 30 ++++++++++++------- 1 file changed, 19 insertions(+), 11 deletions(-) diff --git a/contracts/token/ERC721/extensions/ERC721Consecutive.sol b/contracts/token/ERC721/extensions/ERC721Consecutive.sol index 0aa075b1fac..bbe9935e8db 100644 --- a/contracts/token/ERC721/extensions/ERC721Consecutive.sol +++ b/contracts/token/ERC721/extensions/ERC721Consecutive.sol @@ -17,7 +17,13 @@ import "../../../utils/structs/BitMaps.sol"; * Using this extensions removes the ability to mint single token during the contract construction. This ability is * regained after construction. During construction, only batch minting is allowed. * - * IMPORTANT: This extension bypasses the hooks `_beforeTokenTransfer` and `_afterTokenTransfer` + * IMPORTANT: This extension bypasses the hooks {_beforeTokenTransfer} and {_afterTokenTransfer} for token minted in + * batch. When using this extensions, you should consider the {_beforeConsecutiveTokenTransfer} and + * {_afterConsecutiveTokenTransfer} hooks in addition to {_beforeTokenTransfer} and {_afterTokenTransfer}. + * + * IMPORTANT: When overriding {_afterTokenTransfer}, be carefull about call ordering. {ownerOf} may return invalid + * values during the {_afterTokenTransfer} execution if the super call is not called first. To be safe, execute the + * super call before your custom logic. * * _Available since v4.8._ */ @@ -95,23 +101,25 @@ abstract contract ERC721Consecutive is ERC721 { */ function _mint(address to, uint256 tokenId) internal virtual override { require(Address.isContract(address(this)), "ERC721Consecutive: can't mint during construction"); - super._mint(to, tokenId); - if (_sequentialBurn.get(tokenId)) { - _sequentialBurn.unset(tokenId); - } } /** - * @dev See {ERC721-_mint}. Override needed to avoid looking up in the sequential ownership structure for burnt - * token. + * @dev See {ERC721-_afterTokenTransfer}. Burning of token that have been sequentially minted must be explicit. */ - function _burn(uint256 tokenId) internal virtual override { - // Invoke the core burn functionality to adjust balances, invoke hooks, etc. - super._burn(tokenId); - if (tokenId <= _totalConsecutiveSupply()) { + function _afterTokenTransfer( + address from, + address to, + uint256 tokenId + ) internal virtual override { + if ( + to == address(0) && // if we burn + tokenId <= _totalConsecutiveSupply() && // and the tokenId was sequentialy minted + !_sequentialBurn.get(tokenId) // and the token was never marked as burnt + ) { _sequentialBurn.set(tokenId); } + super._afterTokenTransfer(from, to, tokenId); } function _totalConsecutiveSupply() private view returns (uint96) { From 0b071127cfbb19a2c4c7f88da39304c091b494e4 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 2 Sep 2022 17:25:09 +0200 Subject: [PATCH 29/46] add changelog entry --- CHANGELOG.md | 1 + contracts/token/ERC721/extensions/ERC721Consecutive.sol | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 403579e4795..1184043fa2d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ * `ERC721`: optimize burn by making approval clearing implicit instead of emitting an event. ([#3538](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3538)) * `ERC721`: Fix balance accounting when a custom `_beforeTokenTransfer` hook results in a transfer of the token under consideration. ([#3611](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3611)) * `ERC721`: use unchecked arithmetic for balance updates. ([#3524](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3524)) + * `ERC721Consecutive`: Implementation of EIP-2309 that allows batch minting of ERC721 tokens during construction. ([#3311](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3311)) * `ReentrancyGuard`: Reduce code size impact of the modifier by using internal functions. ([#3515](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3515)) * `SafeCast`: optimize downcasting of signed integers. ([#3565](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3565)) * `VestingWallet`: remove unused library `Math.sol`. ([#3605](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3605)) diff --git a/contracts/token/ERC721/extensions/ERC721Consecutive.sol b/contracts/token/ERC721/extensions/ERC721Consecutive.sol index bbe9935e8db..d27dcc31ec9 100644 --- a/contracts/token/ERC721/extensions/ERC721Consecutive.sol +++ b/contracts/token/ERC721/extensions/ERC721Consecutive.sol @@ -21,7 +21,7 @@ import "../../../utils/structs/BitMaps.sol"; * batch. When using this extensions, you should consider the {_beforeConsecutiveTokenTransfer} and * {_afterConsecutiveTokenTransfer} hooks in addition to {_beforeTokenTransfer} and {_afterTokenTransfer}. * - * IMPORTANT: When overriding {_afterTokenTransfer}, be carefull about call ordering. {ownerOf} may return invalid + * IMPORTANT: When overriding {_afterTokenTransfer}, be careful about call ordering. {ownerOf} may return invalid * values during the {_afterTokenTransfer} execution if the super call is not called first. To be safe, execute the * super call before your custom logic. * @@ -114,7 +114,7 @@ abstract contract ERC721Consecutive is ERC721 { ) internal virtual override { if ( to == address(0) && // if we burn - tokenId <= _totalConsecutiveSupply() && // and the tokenId was sequentialy minted + tokenId <= _totalConsecutiveSupply() && // and the tokenId was minted is a batch !_sequentialBurn.get(tokenId) // and the token was never marked as burnt ) { _sequentialBurn.set(tokenId); From 7b30f04f07da4a303ca5b0e70faf040f07a0759e Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 2 Sep 2022 17:27:54 +0200 Subject: [PATCH 30/46] fix mock --- contracts/mocks/ERC721ConsecutiveMock.sol | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/contracts/mocks/ERC721ConsecutiveMock.sol b/contracts/mocks/ERC721ConsecutiveMock.sol index 174df641355..08be76958cd 100644 --- a/contracts/mocks/ERC721ConsecutiveMock.sol +++ b/contracts/mocks/ERC721ConsecutiveMock.sol @@ -66,10 +66,6 @@ contract ERC721ConsecutiveMock is ERC721Burnable, ERC721Consecutive, ERC721Enume return super._ownerOf(tokenId); } - function _burn(uint256 tokenId) internal virtual override(ERC721, ERC721Consecutive) { - super._burn(tokenId); - } - function _mint(address to, uint256 tokenId) internal virtual override(ERC721, ERC721Consecutive) { super._mint(to, tokenId); } @@ -86,7 +82,7 @@ contract ERC721ConsecutiveMock is ERC721Burnable, ERC721Consecutive, ERC721Enume address from, address to, uint256 tokenId - ) internal virtual override(ERC721, ERC721Votes) { + ) internal virtual override(ERC721, ERC721Votes, ERC721Consecutive) { super._afterTokenTransfer(from, to, tokenId); } From a204f8e83647ff23994e64cdf23d2f2a2306695c Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Sun, 4 Sep 2022 22:41:20 +0200 Subject: [PATCH 31/46] disable enumerability of consecutivelly minted tokens --- contracts/mocks/ERC721ConsecutiveMock.sol | 72 +++++++++++++++---- .../ERC721/extensions/ERC721Enumerable.sol | 20 +----- .../extensions/ERC721Consecutive.test.js | 41 +++++------ 3 files changed, 77 insertions(+), 56 deletions(-) diff --git a/contracts/mocks/ERC721ConsecutiveMock.sol b/contracts/mocks/ERC721ConsecutiveMock.sol index 08be76958cd..11c93d188fb 100644 --- a/contracts/mocks/ERC721ConsecutiveMock.sol +++ b/contracts/mocks/ERC721ConsecutiveMock.sol @@ -11,7 +11,7 @@ import "../token/ERC721/extensions/draft-ERC721Votes.sol"; /** * @title ERC721ConsecutiveMock */ -contract ERC721ConsecutiveMock is ERC721Burnable, ERC721Consecutive, ERC721Enumerable, ERC721Pausable, ERC721Votes { +contract ERC721ConsecutiveMock is ERC721Burnable, ERC721Consecutive, ERC721Pausable, ERC721Votes { constructor( string memory name, string memory symbol, @@ -36,16 +36,6 @@ contract ERC721ConsecutiveMock is ERC721Burnable, ERC721Consecutive, ERC721Enume _unpause(); } - function supportsInterface(bytes4 interfaceId) - public - view - virtual - override(ERC721, ERC721Enumerable) - returns (bool) - { - return super.supportsInterface(interfaceId); - } - function exists(uint256 tokenId) public view returns (bool) { return _exists(tokenId); } @@ -74,7 +64,7 @@ contract ERC721ConsecutiveMock is ERC721Burnable, ERC721Consecutive, ERC721Enume address from, address to, uint256 tokenId - ) internal virtual override(ERC721, ERC721Enumerable, ERC721Pausable) { + ) internal virtual override(ERC721, ERC721Pausable) { super._beforeTokenTransfer(from, to, tokenId); } @@ -91,7 +81,7 @@ contract ERC721ConsecutiveMock is ERC721Burnable, ERC721Consecutive, ERC721Enume address to, uint256 first, uint96 size - ) internal virtual override(ERC721, ERC721Enumerable, ERC721Pausable) { + ) internal virtual override(ERC721, ERC721Pausable) { super._beforeConsecutiveTokenTransfer(from, to, first, size); } @@ -105,6 +95,62 @@ contract ERC721ConsecutiveMock is ERC721Burnable, ERC721Consecutive, ERC721Enume } } +contract ERC721ConsecutiveEnumerableMock is ERC721Consecutive, ERC721Enumerable { + constructor( + string memory name, + string memory symbol, + address[] memory receivers, + uint96[] memory amounts + ) ERC721(name, symbol) { + for (uint256 i = 0; i < receivers.length; ++i) { + _mintConsecutive(receivers[i], amounts[i]); + } + } + + function supportsInterface(bytes4 interfaceId) + public + view + virtual + override(ERC721, ERC721Enumerable) + returns (bool) + { + return super.supportsInterface(interfaceId); + } + + function _ownerOf(uint256 tokenId) internal view virtual override(ERC721, ERC721Consecutive) returns (address) { + return super._ownerOf(tokenId); + } + + function _mint(address to, uint256 tokenId) internal virtual override(ERC721, ERC721Consecutive) { + super._mint(to, tokenId); + } + + function _beforeTokenTransfer( + address from, + address to, + uint256 tokenId + ) internal virtual override(ERC721, ERC721Enumerable) { + super._beforeTokenTransfer(from, to, tokenId); + } + + function _afterTokenTransfer( + address from, + address to, + uint256 tokenId + ) internal virtual override(ERC721, ERC721Consecutive) { + super._afterTokenTransfer(from, to, tokenId); + } + + function _beforeConsecutiveTokenTransfer( + address from, + address to, + uint256 first, + uint96 size + ) internal virtual override(ERC721, ERC721Enumerable) { + super._beforeConsecutiveTokenTransfer(from, to, first, size); + } +} + contract ERC721ConsecutiveNoConstructorMintMock is ERC721Consecutive { constructor(string memory name, string memory symbol) ERC721(name, symbol) { _mint(msg.sender, 0); diff --git a/contracts/token/ERC721/extensions/ERC721Enumerable.sol b/contracts/token/ERC721/extensions/ERC721Enumerable.sol index ab7fe98d003..0ea1de93a74 100644 --- a/contracts/token/ERC721/extensions/ERC721Enumerable.sol +++ b/contracts/token/ERC721/extensions/ERC721Enumerable.sol @@ -108,25 +108,7 @@ abstract contract ERC721Enumerable is ERC721, IERC721Enumerable { uint256 first, uint96 size ) internal virtual override { - require(from == address(0) && to != address(0), "ERC721Enumerable: only batch minting is supported"); - - // Before balance is updated (that is part of the super call) - uint256 length = ERC721.balanceOf(to); - - // Do the super call - super._beforeConsecutiveTokenTransfer(from, to, first, size); - - // Add to enumerability - for (uint256 tokenId = first; tokenId < first + size; ++tokenId) { - // Add to all tokens - _addTokenToAllTokensEnumeration(tokenId); - - // Add to owner tokens - _ownedTokens[to][length] = tokenId; - _ownedTokensIndex[tokenId] = length; - - ++length; - } + revert("ERC721Enumerable: consecutive transfers not supported"); } /** diff --git a/test/token/ERC721/extensions/ERC721Consecutive.test.js b/test/token/ERC721/extensions/ERC721Consecutive.test.js index 1caf21c7c8a..15bee3511ae 100644 --- a/test/token/ERC721/extensions/ERC721Consecutive.test.js +++ b/test/token/ERC721/extensions/ERC721Consecutive.test.js @@ -2,6 +2,7 @@ const { constants, expectEvent, expectRevert } = require('@openzeppelin/test-hel const { expect } = require('chai'); const ERC721ConsecutiveMock = artifacts.require('ERC721ConsecutiveMock'); +const ERC721ConsecutiveEnumerableMock = artifacts.require('ERC721ConsecutiveEnumerableMock'); const ERC721ConsecutiveNoConstructorMintMock = artifacts.require('ERC721ConsecutiveNoConstructorMintMock'); contract('ERC721Consecutive', function (accounts) { @@ -80,35 +81,27 @@ contract('ERC721Consecutive', function (accounts) { } }); - it('enumerability correctly set', async function () { - const owners = batches.flatMap(({ receiver, amount }) => Array(amount).fill(receiver)); - - expect(await this.token.totalSupply()) - .to.be.bignumber.equal(web3.utils.toBN(owners.length)); - - for (const tokenId in owners) { - expect(await this.token.tokenByIndex(tokenId)) - .to.be.bignumber.equal(web3.utils.toBN(tokenId)); - } - - for (const account of accounts) { - const owned = Object.entries(owners) - .filter(([ _, owner ]) => owner === account) - .map(([ tokenId, _ ]) => tokenId); - - for (const i in owned) { - expect(await this.token.tokenOfOwnerByIndex(account, i).then(x => x.toString())) - .to.be.bignumber.equal(web3.utils.toBN(owned[i])); - } - } - }); - it('cannot use single minting during construction', async function () { await expectRevert( - ERC721ConsecutiveNoConstructorMintMock.new(name, symbol), + ERC721ConsecutiveNoConstructorMintMock.new( + name, + symbol, + ), 'ERC721Consecutive: can\'t mint during construction', ); }); + + it('consecutive mint not compatible with enumerability', async function () { + await expectRevert( + ERC721ConsecutiveEnumerableMock.new( + name, + symbol, + batches.map(({ receiver }) => receiver), + batches.map(({ amount }) => amount), + ), + 'ERC721Enumerable: consecutive transfers not supported' + ); + }); }); describe('minting after construction', function () { From 2ef65e0060678512654bca60f6348c33f1b8d0bf Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Sun, 4 Sep 2022 22:51:13 +0200 Subject: [PATCH 32/46] fix lint --- test/token/ERC721/extensions/ERC721Consecutive.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/token/ERC721/extensions/ERC721Consecutive.test.js b/test/token/ERC721/extensions/ERC721Consecutive.test.js index 15bee3511ae..fb29fd7a927 100644 --- a/test/token/ERC721/extensions/ERC721Consecutive.test.js +++ b/test/token/ERC721/extensions/ERC721Consecutive.test.js @@ -99,7 +99,7 @@ contract('ERC721Consecutive', function (accounts) { batches.map(({ receiver }) => receiver), batches.map(({ amount }) => amount), ), - 'ERC721Enumerable: consecutive transfers not supported' + 'ERC721Enumerable: consecutive transfers not supported', ); }); }); From dfc692d4d93f71eea44d55358654bb4a2d2627d8 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Sun, 4 Sep 2022 23:38:26 +0200 Subject: [PATCH 33/46] extend Checkpoint library --- contracts/mocks/CheckpointsMock.sol | 36 ++++--- .../ERC721/extensions/ERC721Consecutive.sol | 9 +- contracts/utils/Checkpoints.sol | 99 ++++++++++++++----- scripts/generate/templates/Checkpoints.js | 47 +++++---- scripts/generate/templates/CheckpointsMock.js | 24 +++-- test/utils/Checkpoints.test.js | 31 ++++-- 6 files changed, 174 insertions(+), 72 deletions(-) diff --git a/contracts/mocks/CheckpointsMock.sol b/contracts/mocks/CheckpointsMock.sol index 591d2fc3029..07d06679dea 100644 --- a/contracts/mocks/CheckpointsMock.sol +++ b/contracts/mocks/CheckpointsMock.sol @@ -14,6 +14,14 @@ contract CheckpointsMock { return _totalCheckpoints.latest(); } + function latestCheckpoint() public view returns (uint256, uint256) { + return _totalCheckpoints.latestCheckpoint(); + } + + function length() public view returns (uint256) { + return _totalCheckpoints.length(); + } + function push(uint256 value) public returns (uint256, uint256) { return _totalCheckpoints.push(value); } @@ -25,10 +33,6 @@ contract CheckpointsMock { function getAtRecentBlock(uint256 blockNumber) public view returns (uint256) { return _totalCheckpoints.getAtRecentBlock(blockNumber); } - - function length() public view returns (uint256) { - return _totalCheckpoints._checkpoints.length; - } } contract Checkpoints224Mock { @@ -40,6 +44,14 @@ contract Checkpoints224Mock { return _totalCheckpoints.latest(); } + function latestCheckpoint() public view returns (uint32, uint224) { + return _totalCheckpoints.latestCheckpoint(); + } + + function length() public view returns (uint256) { + return _totalCheckpoints.length(); + } + function push(uint32 key, uint224 value) public returns (uint224, uint224) { return _totalCheckpoints.push(key, value); } @@ -55,10 +67,6 @@ contract Checkpoints224Mock { function upperLookupRecent(uint32 key) public view returns (uint224) { return _totalCheckpoints.upperLookupRecent(key); } - - function length() public view returns (uint256) { - return _totalCheckpoints._checkpoints.length; - } } contract Checkpoints160Mock { @@ -70,6 +78,14 @@ contract Checkpoints160Mock { return _totalCheckpoints.latest(); } + function latestCheckpoint() public view returns (uint96, uint160) { + return _totalCheckpoints.latestCheckpoint(); + } + + function length() public view returns (uint256) { + return _totalCheckpoints.length(); + } + function push(uint96 key, uint160 value) public returns (uint160, uint160) { return _totalCheckpoints.push(key, value); } @@ -85,8 +101,4 @@ contract Checkpoints160Mock { function upperLookupRecent(uint96 key) public view returns (uint224) { return _totalCheckpoints.upperLookupRecent(key); } - - function length() public view returns (uint256) { - return _totalCheckpoints._checkpoints.length; - } } diff --git a/contracts/token/ERC721/extensions/ERC721Consecutive.sol b/contracts/token/ERC721/extensions/ERC721Consecutive.sol index d27dcc31ec9..9b4a7398a25 100644 --- a/contracts/token/ERC721/extensions/ERC721Consecutive.sol +++ b/contracts/token/ERC721/extensions/ERC721Consecutive.sol @@ -123,7 +123,12 @@ abstract contract ERC721Consecutive is ERC721 { } function _totalConsecutiveSupply() private view returns (uint96) { - uint256 length = _sequentialOwnership._checkpoints.length; - return length == 0 ? 0 : _sequentialOwnership._checkpoints[length - 1]._key + 1; + uint256 length = _sequentialOwnership.length(); + if (length == 0) { + return 0; + } else { + (uint96 latestId, ) = _sequentialOwnership.latestCheckpoint(); + return latestId + 1; + } } } diff --git a/contracts/utils/Checkpoints.sol b/contracts/utils/Checkpoints.sol index 793ea3bd160..91c32098a1c 100644 --- a/contracts/utils/Checkpoints.sol +++ b/contracts/utils/Checkpoints.sol @@ -26,14 +26,6 @@ library Checkpoints { uint224 _value; } - /** - * @dev Returns the value in the latest checkpoint, or zero if there are no checkpoints. - */ - function latest(History storage self) internal view returns (uint256) { - uint256 pos = self._checkpoints.length; - return pos == 0 ? 0 : _unsafeAccess(self._checkpoints, pos - 1)._value; - } - /** * @dev Returns the value at a given block number. If a checkpoint is not available at that block, the closest one * before it is returned, or zero otherwise. @@ -93,6 +85,31 @@ library Checkpoints { return push(self, op(latest(self), delta)); } + /** + * @dev Returns the value in the most recent checkpoint, or zero if there are no checkpoints. + */ + function latest(History storage self) internal view returns (uint224) { + uint256 pos = self._checkpoints.length; + return pos == 0 ? 0 : _unsafeAccess(self._checkpoints, pos - 1)._value; + } + + /** + * @dev Returns the key and value in the most recent checkpoint. Revert if there are no checkpoints. + */ + function latestCheckpoint(History storage self) internal view returns (uint32, uint224) { + uint256 pos = self._checkpoints.length; + require(pos > 0, "Checkpoint: empty"); + Checkpoint memory ckpt = _unsafeAccess(self._checkpoints, pos - 1); + return (ckpt._blockNumber, ckpt._value); + } + + /** + * @dev Returns the number of checkpoint. + */ + function length(History storage self) internal view returns (uint256) { + return self._checkpoints.length; + } + /** * @dev Pushes a (`key`, `value`) pair into an ordered list of checkpoints, either by inserting a new checkpoint, * or by updating the last one. @@ -186,14 +203,6 @@ library Checkpoints { uint224 _value; } - /** - * @dev Returns the value in the most recent checkpoint, or zero if there are no checkpoints. - */ - function latest(Trace224 storage self) internal view returns (uint224) { - uint256 pos = self._checkpoints.length; - return pos == 0 ? 0 : _unsafeAccess(self._checkpoints, pos - 1)._value; - } - /** * @dev Pushes a (`key`, `value`) pair into a Trace224 so that it is stored as the checkpoint. * @@ -244,6 +253,31 @@ library Checkpoints { return pos == 0 ? 0 : _unsafeAccess(self._checkpoints, pos - 1)._value; } + /** + * @dev Returns the value in the most recent checkpoint, or zero if there are no checkpoints. + */ + function latest(Trace224 storage self) internal view returns (uint224) { + uint256 pos = self._checkpoints.length; + return pos == 0 ? 0 : _unsafeAccess(self._checkpoints, pos - 1)._value; + } + + /** + * @dev Returns the key and value in the most recent checkpoint. Revert if there are no checkpoints. + */ + function latestCheckpoint(Trace224 storage self) internal view returns (uint32, uint224) { + uint256 pos = self._checkpoints.length; + require(pos > 0, "Checkpoint: empty"); + Checkpoint224 memory ckpt = _unsafeAccess(self._checkpoints, pos - 1); + return (ckpt._key, ckpt._value); + } + + /** + * @dev Returns the number of checkpoint. + */ + function length(Trace224 storage self) internal view returns (uint256) { + return self._checkpoints.length; + } + /** * @dev Pushes a (`key`, `value`) pair into an ordered list of checkpoints, either by inserting a new checkpoint, * or by updating the last one. @@ -341,14 +375,6 @@ library Checkpoints { uint160 _value; } - /** - * @dev Returns the value in the most recent checkpoint, or zero if there are no checkpoints. - */ - function latest(Trace160 storage self) internal view returns (uint160) { - uint256 pos = self._checkpoints.length; - return pos == 0 ? 0 : _unsafeAccess(self._checkpoints, pos - 1)._value; - } - /** * @dev Pushes a (`key`, `value`) pair into a Trace160 so that it is stored as the checkpoint. * @@ -399,6 +425,31 @@ library Checkpoints { return pos == 0 ? 0 : _unsafeAccess(self._checkpoints, pos - 1)._value; } + /** + * @dev Returns the value in the most recent checkpoint, or zero if there are no checkpoints. + */ + function latest(Trace160 storage self) internal view returns (uint160) { + uint256 pos = self._checkpoints.length; + return pos == 0 ? 0 : _unsafeAccess(self._checkpoints, pos - 1)._value; + } + + /** + * @dev Returns the key and value in the most recent checkpoint. Revert if there are no checkpoints. + */ + function latestCheckpoint(Trace160 storage self) internal view returns (uint96, uint160) { + uint256 pos = self._checkpoints.length; + require(pos > 0, "Checkpoint: empty"); + Checkpoint160 memory ckpt = _unsafeAccess(self._checkpoints, pos - 1); + return (ckpt._key, ckpt._value); + } + + /** + * @dev Returns the number of checkpoint. + */ + function length(Trace160 storage self) internal view returns (uint256) { + return self._checkpoints.length; + } + /** * @dev Pushes a (`key`, `value`) pair into an ordered list of checkpoints, either by inserting a new checkpoint, * or by updating the last one. diff --git a/scripts/generate/templates/Checkpoints.js b/scripts/generate/templates/Checkpoints.js index e368299f377..4f62d3f8fa0 100644 --- a/scripts/generate/templates/Checkpoints.js +++ b/scripts/generate/templates/Checkpoints.js @@ -32,14 +32,6 @@ struct ${opts.checkpointTypeName} { /* eslint-disable max-len */ const operations = opts => `\ -/** - * @dev Returns the value in the most recent checkpoint, or zero if there are no checkpoints. - */ -function latest(${opts.historyTypeName} storage self) internal view returns (${opts.valueTypeName}) { - uint256 pos = self.${opts.checkpointFieldName}.length; - return pos == 0 ? 0 : _unsafeAccess(self.${opts.checkpointFieldName}, pos - 1).${opts.valueFieldName}; -} - /** * @dev Pushes a (\`key\`, \`value\`) pair into a ${opts.historyTypeName} so that it is stored as the checkpoint. * @@ -92,14 +84,6 @@ function upperLookupRecent(${opts.historyTypeName} storage self, ${opts.keyTypeN `; const legacyOperations = opts => `\ -/** - * @dev Returns the value in the latest checkpoint, or zero if there are no checkpoints. - */ -function latest(${opts.historyTypeName} storage self) internal view returns (uint256) { - uint256 pos = self.${opts.checkpointFieldName}.length; - return pos == 0 ? 0 : _unsafeAccess(self.${opts.checkpointFieldName}, pos - 1).${opts.valueFieldName}; -} - /** * @dev Returns the value at a given block number. If a checkpoint is not available at that block, the closest one * before it is returned, or zero otherwise. @@ -160,7 +144,32 @@ function push( } `; -const helpers = opts => `\ +const common = opts => `\ +/** + * @dev Returns the value in the most recent checkpoint, or zero if there are no checkpoints. + */ +function latest(${opts.historyTypeName} storage self) internal view returns (${opts.valueTypeName}) { + uint256 pos = self.${opts.checkpointFieldName}.length; + return pos == 0 ? 0 : _unsafeAccess(self.${opts.checkpointFieldName}, pos - 1).${opts.valueFieldName}; +} + +/** + * @dev Returns the key and value in the most recent checkpoint. Revert if there are no checkpoints. + */ +function latestCheckpoint(${opts.historyTypeName} storage self) internal view returns (${opts.keyTypeName}, ${opts.valueTypeName}) { + uint256 pos = self.${opts.checkpointFieldName}.length; + require(pos > 0, "Checkpoint: empty"); + ${opts.checkpointTypeName} memory ckpt = _unsafeAccess(self.${opts.checkpointFieldName}, pos - 1); + return (ckpt.${opts.keyFieldName}, ckpt.${opts.valueFieldName}); +} + +/** + * @dev Returns the number of checkpoint. + */ +function length(${opts.historyTypeName} storage self) internal view returns (uint256) { + return self.${opts.checkpointFieldName}.length; +} + /** * @dev Pushes a (\`key\`, \`value\`) pair into an ordered list of checkpoints, either by inserting a new checkpoint, * or by updating the last one. @@ -279,12 +288,12 @@ module.exports = format( // Legacy types & functions types(LEGACY_OPTS), legacyOperations(LEGACY_OPTS), - helpers(LEGACY_OPTS), + common(LEGACY_OPTS), // New flavors ...OPTS.flatMap(opts => [ types(opts), operations(opts), - helpers(opts), + common(opts), ]), ], '}', diff --git a/scripts/generate/templates/CheckpointsMock.js b/scripts/generate/templates/CheckpointsMock.js index 6ce8e534b0b..e5646de23d8 100755 --- a/scripts/generate/templates/CheckpointsMock.js +++ b/scripts/generate/templates/CheckpointsMock.js @@ -18,6 +18,14 @@ contract CheckpointsMock { return _totalCheckpoints.latest(); } + function latestCheckpoint() public view returns (uint256, uint256) { + return _totalCheckpoints.latestCheckpoint(); + } + + function length() public view returns (uint256) { + return _totalCheckpoints.length(); + } + function push(uint256 value) public returns (uint256, uint256) { return _totalCheckpoints.push(value); } @@ -29,10 +37,6 @@ contract CheckpointsMock { function getAtRecentBlock(uint256 blockNumber) public view returns (uint256) { return _totalCheckpoints.getAtRecentBlock(blockNumber); } - - function length() public view returns (uint256) { - return _totalCheckpoints._checkpoints.length; - } } `; @@ -46,6 +50,14 @@ contract Checkpoints${length}Mock { return _totalCheckpoints.latest(); } + function latestCheckpoint() public view returns (uint${256 - length}, uint${length}) { + return _totalCheckpoints.latestCheckpoint(); + } + + function length() public view returns (uint256) { + return _totalCheckpoints.length(); + } + function push(uint${256 - length} key, uint${length} value) public returns (uint${length}, uint${length}) { return _totalCheckpoints.push(key, value); } @@ -61,10 +73,6 @@ contract Checkpoints${length}Mock { function upperLookupRecent(uint${256 - length} key) public view returns (uint224) { return _totalCheckpoints.upperLookupRecent(key); } - - function length() public view returns (uint256) { - return _totalCheckpoints._checkpoints.length; - } } `; diff --git a/test/utils/Checkpoints.test.js b/test/utils/Checkpoints.test.js index af8a1a13bb9..c08e96aaa5b 100644 --- a/test/utils/Checkpoints.test.js +++ b/test/utils/Checkpoints.test.js @@ -18,6 +18,11 @@ contract('Checkpoints', function (accounts) { describe('without checkpoints', function () { it('returns zero as latest value', async function () { expect(await this.checkpoint.latest()).to.be.bignumber.equal('0'); + + await expectRevert( + this.checkpoint.latestCheckpoint(), + 'Checkpoint: empty', + ); }); it('returns zero as past value', async function () { @@ -39,6 +44,10 @@ contract('Checkpoints', function (accounts) { it('returns latest value', async function () { expect(await this.checkpoint.latest()).to.be.bignumber.equal('3'); + + const ckpt = await this.checkpoint.latestCheckpoint(); + expect(ckpt[0]).to.be.bignumber.equal(web3.utils.toBN(this.tx3.receipt.blockNumber)); + expect(ckpt[1]).to.be.bignumber.equal(web3.utils.toBN('3')); }); for (const fn of [ 'getAtBlock(uint256)', 'getAtRecentBlock(uint256)' ]) { @@ -90,6 +99,11 @@ contract('Checkpoints', function (accounts) { describe('without checkpoints', function () { it('returns zero as latest value', async function () { expect(await this.contract.latest()).to.be.bignumber.equal('0'); + + await expectRevert( + this.contract.latestCheckpoint(), + 'Checkpoint: empty', + ); }); it('lookup returns 0', async function () { @@ -102,11 +116,11 @@ contract('Checkpoints', function (accounts) { describe('with checkpoints', function () { beforeEach('pushing checkpoints', async function () { this.checkpoints = [ - { key: 2, value: '17' }, - { key: 3, value: '42' }, - { key: 5, value: '101' }, - { key: 7, value: '23' }, - { key: 11, value: '99' }, + { key: '2', value: '17' }, + { key: '3', value: '42' }, + { key: '5', value: '101' }, + { key: '7', value: '23' }, + { key: '11', value: '99' }, ]; for (const { key, value } of this.checkpoints) { await this.contract.push(key, value); @@ -114,8 +128,11 @@ contract('Checkpoints', function (accounts) { }); it('returns latest value', async function () { - expect(await this.contract.latest()) - .to.be.bignumber.equal(last(this.checkpoints).value); + expect(await this.contract.latest()).to.be.bignumber.equal(last(this.checkpoints).value); + + const ckpt = await this.contract.latestCheckpoint(); + expect(ckpt[0]).to.be.bignumber.equal(last(this.checkpoints).key); + expect(ckpt[1]).to.be.bignumber.equal(last(this.checkpoints).value); }); it('cannot push values in the past', async function () { From ed7668d85593d57655f89f256643b299c8d4ec48 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Sun, 4 Sep 2022 23:40:38 +0200 Subject: [PATCH 34/46] fix lint --- contracts/token/ERC721/extensions/ERC721Enumerable.sol | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/contracts/token/ERC721/extensions/ERC721Enumerable.sol b/contracts/token/ERC721/extensions/ERC721Enumerable.sol index 0ea1de93a74..c766133410d 100644 --- a/contracts/token/ERC721/extensions/ERC721Enumerable.sol +++ b/contracts/token/ERC721/extensions/ERC721Enumerable.sol @@ -103,10 +103,10 @@ abstract contract ERC721Enumerable is ERC721, IERC721Enumerable { * To learn more about hooks, head to xref:ROOT:extending-contracts.adoc#using-hooks[Using Hooks]. */ function _beforeConsecutiveTokenTransfer( - address from, - address to, - uint256 first, - uint96 size + address, + address, + uint256, + uint96 ) internal virtual override { revert("ERC721Enumerable: consecutive transfers not supported"); } From 031278c36b16033c598e032b9a4ee1699d1c0d9a Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Mon, 5 Sep 2022 16:04:53 +0200 Subject: [PATCH 35/46] improve latestCheckpoint() function --- contracts/mocks/CheckpointsMock.sol | 30 ++++++++- .../ERC721/extensions/ERC721Consecutive.sol | 9 +-- contracts/utils/Checkpoints.sol | 66 ++++++++++++++----- scripts/generate/templates/Checkpoints.js | 22 +++++-- scripts/generate/templates/CheckpointsMock.js | 4 +- test/utils/Checkpoints.test.js | 26 ++++---- 6 files changed, 113 insertions(+), 44 deletions(-) diff --git a/contracts/mocks/CheckpointsMock.sol b/contracts/mocks/CheckpointsMock.sol index 07d06679dea..e333532bad8 100644 --- a/contracts/mocks/CheckpointsMock.sol +++ b/contracts/mocks/CheckpointsMock.sol @@ -14,7 +14,15 @@ contract CheckpointsMock { return _totalCheckpoints.latest(); } - function latestCheckpoint() public view returns (uint256, uint256) { + function latestCheckpoint() + public + view + returns ( + bool, + uint256, + uint256 + ) + { return _totalCheckpoints.latestCheckpoint(); } @@ -44,7 +52,15 @@ contract Checkpoints224Mock { return _totalCheckpoints.latest(); } - function latestCheckpoint() public view returns (uint32, uint224) { + function latestCheckpoint() + public + view + returns ( + bool, + uint32, + uint224 + ) + { return _totalCheckpoints.latestCheckpoint(); } @@ -78,7 +94,15 @@ contract Checkpoints160Mock { return _totalCheckpoints.latest(); } - function latestCheckpoint() public view returns (uint96, uint160) { + function latestCheckpoint() + public + view + returns ( + bool, + uint96, + uint160 + ) + { return _totalCheckpoints.latestCheckpoint(); } diff --git a/contracts/token/ERC721/extensions/ERC721Consecutive.sol b/contracts/token/ERC721/extensions/ERC721Consecutive.sol index 9b4a7398a25..45807ac56da 100644 --- a/contracts/token/ERC721/extensions/ERC721Consecutive.sol +++ b/contracts/token/ERC721/extensions/ERC721Consecutive.sol @@ -123,12 +123,7 @@ abstract contract ERC721Consecutive is ERC721 { } function _totalConsecutiveSupply() private view returns (uint96) { - uint256 length = _sequentialOwnership.length(); - if (length == 0) { - return 0; - } else { - (uint96 latestId, ) = _sequentialOwnership.latestCheckpoint(); - return latestId + 1; - } + (bool exist, uint96 latestId, ) = _sequentialOwnership.latestCheckpoint(); + return exist ? latestId + 1 : 0; } } diff --git a/contracts/utils/Checkpoints.sol b/contracts/utils/Checkpoints.sol index 91c32098a1c..03ec149d8ba 100644 --- a/contracts/utils/Checkpoints.sol +++ b/contracts/utils/Checkpoints.sol @@ -94,13 +94,25 @@ library Checkpoints { } /** - * @dev Returns the key and value in the most recent checkpoint. Revert if there are no checkpoints. + * @dev Returns weither there is a checkpoint if the structure (structure is not empty), and if so the key and value + * in the most recent checkpoint. */ - function latestCheckpoint(History storage self) internal view returns (uint32, uint224) { + function latestCheckpoint(History storage self) + internal + view + returns ( + bool exist, + uint32 _blockNumber, + uint224 _value + ) + { uint256 pos = self._checkpoints.length; - require(pos > 0, "Checkpoint: empty"); - Checkpoint memory ckpt = _unsafeAccess(self._checkpoints, pos - 1); - return (ckpt._blockNumber, ckpt._value); + if (pos == 0) { + return (false, 0, 0); + } else { + Checkpoint memory ckpt = _unsafeAccess(self._checkpoints, pos - 1); + return (true, ckpt._blockNumber, ckpt._value); + } } /** @@ -262,13 +274,25 @@ library Checkpoints { } /** - * @dev Returns the key and value in the most recent checkpoint. Revert if there are no checkpoints. + * @dev Returns weither there is a checkpoint if the structure (structure is not empty), and if so the key and value + * in the most recent checkpoint. */ - function latestCheckpoint(Trace224 storage self) internal view returns (uint32, uint224) { + function latestCheckpoint(Trace224 storage self) + internal + view + returns ( + bool exist, + uint32 _key, + uint224 _value + ) + { uint256 pos = self._checkpoints.length; - require(pos > 0, "Checkpoint: empty"); - Checkpoint224 memory ckpt = _unsafeAccess(self._checkpoints, pos - 1); - return (ckpt._key, ckpt._value); + if (pos == 0) { + return (false, 0, 0); + } else { + Checkpoint224 memory ckpt = _unsafeAccess(self._checkpoints, pos - 1); + return (true, ckpt._key, ckpt._value); + } } /** @@ -434,13 +458,25 @@ library Checkpoints { } /** - * @dev Returns the key and value in the most recent checkpoint. Revert if there are no checkpoints. + * @dev Returns weither there is a checkpoint if the structure (structure is not empty), and if so the key and value + * in the most recent checkpoint. */ - function latestCheckpoint(Trace160 storage self) internal view returns (uint96, uint160) { + function latestCheckpoint(Trace160 storage self) + internal + view + returns ( + bool exist, + uint96 _key, + uint160 _value + ) + { uint256 pos = self._checkpoints.length; - require(pos > 0, "Checkpoint: empty"); - Checkpoint160 memory ckpt = _unsafeAccess(self._checkpoints, pos - 1); - return (ckpt._key, ckpt._value); + if (pos == 0) { + return (false, 0, 0); + } else { + Checkpoint160 memory ckpt = _unsafeAccess(self._checkpoints, pos - 1); + return (true, ckpt._key, ckpt._value); + } } /** diff --git a/scripts/generate/templates/Checkpoints.js b/scripts/generate/templates/Checkpoints.js index 4f62d3f8fa0..2532c14845d 100644 --- a/scripts/generate/templates/Checkpoints.js +++ b/scripts/generate/templates/Checkpoints.js @@ -154,13 +154,25 @@ function latest(${opts.historyTypeName} storage self) internal view returns (${o } /** - * @dev Returns the key and value in the most recent checkpoint. Revert if there are no checkpoints. + * @dev Returns weither there is a checkpoint if the structure (structure is not empty), and if so the key and value + * in the most recent checkpoint. */ -function latestCheckpoint(${opts.historyTypeName} storage self) internal view returns (${opts.keyTypeName}, ${opts.valueTypeName}) { +function latestCheckpoint(${opts.historyTypeName} storage self) + internal + view + returns ( + bool exist, + ${opts.keyTypeName} ${opts.keyFieldName}, + ${opts.valueTypeName} ${opts.valueFieldName} + ) +{ uint256 pos = self.${opts.checkpointFieldName}.length; - require(pos > 0, "Checkpoint: empty"); - ${opts.checkpointTypeName} memory ckpt = _unsafeAccess(self.${opts.checkpointFieldName}, pos - 1); - return (ckpt.${opts.keyFieldName}, ckpt.${opts.valueFieldName}); + if (pos == 0) { + return (false, 0, 0); + } else { + ${opts.checkpointTypeName} memory ckpt = _unsafeAccess(self.${opts.checkpointFieldName}, pos - 1); + return (true, ckpt.${opts.keyFieldName}, ckpt.${opts.valueFieldName}); + } } /** diff --git a/scripts/generate/templates/CheckpointsMock.js b/scripts/generate/templates/CheckpointsMock.js index e5646de23d8..755effbd378 100755 --- a/scripts/generate/templates/CheckpointsMock.js +++ b/scripts/generate/templates/CheckpointsMock.js @@ -18,7 +18,7 @@ contract CheckpointsMock { return _totalCheckpoints.latest(); } - function latestCheckpoint() public view returns (uint256, uint256) { + function latestCheckpoint() public view returns (bool, uint256, uint256) { return _totalCheckpoints.latestCheckpoint(); } @@ -50,7 +50,7 @@ contract Checkpoints${length}Mock { return _totalCheckpoints.latest(); } - function latestCheckpoint() public view returns (uint${256 - length}, uint${length}) { + function latestCheckpoint() public view returns (bool, uint${256 - length}, uint${length}) { return _totalCheckpoints.latestCheckpoint(); } diff --git a/test/utils/Checkpoints.test.js b/test/utils/Checkpoints.test.js index c08e96aaa5b..627a101fcb9 100644 --- a/test/utils/Checkpoints.test.js +++ b/test/utils/Checkpoints.test.js @@ -19,10 +19,10 @@ contract('Checkpoints', function (accounts) { it('returns zero as latest value', async function () { expect(await this.checkpoint.latest()).to.be.bignumber.equal('0'); - await expectRevert( - this.checkpoint.latestCheckpoint(), - 'Checkpoint: empty', - ); + const ckpt = await this.checkpoint.latestCheckpoint(); + expect(ckpt[0]).to.be.equal(false); + expect(ckpt[1]).to.be.bignumber.equal('0'); + expect(ckpt[2]).to.be.bignumber.equal('0'); }); it('returns zero as past value', async function () { @@ -46,8 +46,9 @@ contract('Checkpoints', function (accounts) { expect(await this.checkpoint.latest()).to.be.bignumber.equal('3'); const ckpt = await this.checkpoint.latestCheckpoint(); - expect(ckpt[0]).to.be.bignumber.equal(web3.utils.toBN(this.tx3.receipt.blockNumber)); - expect(ckpt[1]).to.be.bignumber.equal(web3.utils.toBN('3')); + expect(ckpt[0]).to.be.equal(true); + expect(ckpt[1]).to.be.bignumber.equal(web3.utils.toBN(this.tx3.receipt.blockNumber)); + expect(ckpt[2]).to.be.bignumber.equal(web3.utils.toBN('3')); }); for (const fn of [ 'getAtBlock(uint256)', 'getAtRecentBlock(uint256)' ]) { @@ -100,10 +101,10 @@ contract('Checkpoints', function (accounts) { it('returns zero as latest value', async function () { expect(await this.contract.latest()).to.be.bignumber.equal('0'); - await expectRevert( - this.contract.latestCheckpoint(), - 'Checkpoint: empty', - ); + const ckpt = await this.contract.latestCheckpoint(); + expect(ckpt[0]).to.be.equal(false); + expect(ckpt[1]).to.be.bignumber.equal('0'); + expect(ckpt[2]).to.be.bignumber.equal('0'); }); it('lookup returns 0', async function () { @@ -131,8 +132,9 @@ contract('Checkpoints', function (accounts) { expect(await this.contract.latest()).to.be.bignumber.equal(last(this.checkpoints).value); const ckpt = await this.contract.latestCheckpoint(); - expect(ckpt[0]).to.be.bignumber.equal(last(this.checkpoints).key); - expect(ckpt[1]).to.be.bignumber.equal(last(this.checkpoints).value); + expect(ckpt[0]).to.be.equal(true); + expect(ckpt[1]).to.be.bignumber.equal(last(this.checkpoints).key); + expect(ckpt[2]).to.be.bignumber.equal(last(this.checkpoints).value); }); it('cannot push values in the past', async function () { From 828b91066a083e50edcfcc3fe26145c28d4b22e5 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Mon, 5 Sep 2022 16:14:37 +0200 Subject: [PATCH 36/46] Add an IERC2309 interface --- contracts/interfaces/IERC2309.sol | 20 +++++++++++++++++++ .../ERC721/extensions/ERC721Consecutive.sol | 10 ++-------- 2 files changed, 22 insertions(+), 8 deletions(-) create mode 100644 contracts/interfaces/IERC2309.sol diff --git a/contracts/interfaces/IERC2309.sol b/contracts/interfaces/IERC2309.sol new file mode 100644 index 00000000000..9b2b42b1f77 --- /dev/null +++ b/contracts/interfaces/IERC2309.sol @@ -0,0 +1,20 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.0; + +/** + * @dev ERC-2309: ERC-721 Consecutive Transfer Extension. + * + * _Available since v4.8._ + */ +interface IERC2309 { + /** + * @dev Emitted when the tokens from `fromTokenId` to `toTokenId` are transferred from `fromAddress` to `toAddress`. + */ + event ConsecutiveTransfer( + uint256 indexed fromTokenId, + uint256 toTokenId, + address indexed fromAddress, + address indexed toAddress + ); +} diff --git a/contracts/token/ERC721/extensions/ERC721Consecutive.sol b/contracts/token/ERC721/extensions/ERC721Consecutive.sol index 45807ac56da..5b12626fca1 100644 --- a/contracts/token/ERC721/extensions/ERC721Consecutive.sol +++ b/contracts/token/ERC721/extensions/ERC721Consecutive.sol @@ -3,6 +3,7 @@ pragma solidity ^0.8.0; import "../ERC721.sol"; +import "../../../interfaces/IERC2309.sol"; import "../../../utils/Checkpoints.sol"; import "../../../utils/math/SafeCast.sol"; import "../../../utils/structs/BitMaps.sol"; @@ -27,20 +28,13 @@ import "../../../utils/structs/BitMaps.sol"; * * _Available since v4.8._ */ -abstract contract ERC721Consecutive is ERC721 { +abstract contract ERC721Consecutive is ERC721, IERC2309 { using BitMaps for BitMaps.BitMap; using Checkpoints for Checkpoints.Trace160; Checkpoints.Trace160 private _sequentialOwnership; BitMaps.BitMap private _sequentialBurn; - event ConsecutiveTransfer( - uint256 indexed fromTokenId, - uint256 toTokenId, - address indexed fromAddress, - address indexed toAddress - ); - /** * @dev See {ERC721-_ownerOf}. Override version that checks the sequential ownership structure for tokens that have * been minted as part of a batch, and not yet transferred. From b8c48c85abc5870812845fb2389337eef9aa1a22 Mon Sep 17 00:00:00 2001 From: Francisco Date: Mon, 5 Sep 2022 11:43:10 -0300 Subject: [PATCH 37/46] add compatibility note --- CHANGELOG.md | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 67c5a15a92f..0a64da414a3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,7 +8,6 @@ * `Address`: optimize `functionCall` functions by checking contract size only if there is no returned data. ([#3469](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3469)) * `GovernorCompatibilityBravo`: remove unused `using` statements. ([#3506](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3506)) * `ERC20`: optimize `_transfer`, `_mint` and `_burn` by using `unchecked` arithmetic when possible. ([#3513](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3513)) - * `ERC20Votes`, `ERC721Votes`: optimize `getPastVotes` for looking up recent checkpoints. ([#3673](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3673)) * `ERC20FlashMint`: add an internal `_flashFee` function for overriding. ([#3551](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3551)) * `ERC4626`: use the same `decimals()` as the underlying asset by default (if available). ([#3639](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3639)) * `ERC4626`: add internal `_initialConvertToShares` and `_initialConvertToAssets` functions to customize empty vaults behavior. ([#3639](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3639)) @@ -19,17 +18,18 @@ * `ERC721Consecutive`: Implementation of EIP-2309 that allows batch minting of ERC721 tokens during construction. ([#3311](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3311)) * `ReentrancyGuard`: Reduce code size impact of the modifier by using internal functions. ([#3515](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3515)) * `SafeCast`: optimize downcasting of signed integers. ([#3565](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3565)) + * `VestingWallet`: remove unused library `Math.sol`. ([#3605](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3605)) * `ECDSA`: Remove redundant check on the `v` value. ([#3591](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3591)) * `VestingWallet`: add `releasable` getters. ([#3580](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3580)) - * `VestingWallet`: remove unused library `Math.sol`. ([#3605](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3605)) - * `VestingWallet`: make constructor payable. ([#3665](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3665)) * `Create2`: optimize address computation by using assembly instead of `abi.encodePacked`. ([#3600](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3600)) - * `Clones`: optimized the assembly to use only the scratch space during deployments, and optimized `predictDeterministicAddress` to use fewer operations. ([#3640](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3640)) + * `Clones`: optimized the assembly to use only the scratch space during deployments, and optimized `predictDeterministicAddress` to use lesser operations. ([#3640](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3640)) * `Checkpoints`: Use procedural generation to support multiple key/value lengths. ([#3589](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3589)) * `Checkpoints`: Add new lookup mechanisms. ([#3589](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3589)) - * `Arrays`: Add `unsafeAccess` functions that allow reading and writing to an element in a storage array bypassing Solidity's "out-of-bounds" check. ([#3589](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3589)) - * `Strings`: optimize `toString`. ([#3573](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3573)) - * `Ownable2Step`: extension of `Ownable` that makes the ownership transfers a two step process. ([#3620](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3620)) + * `Array`: Add `unsafeAccess` functions that allow reading and writing to an element in a storage array bypassing Solidity's "out-of-bounds" check. ([#3589](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3589)) + +### Compatibility note + + * `ERC721Consecutive`: This new extension slightly changes the internal workings of `ERC721`, so custom extensions to ERC721 should be reviewed to ensure they remain correct. The new internal functions that should be considered are `_ownerOf`, `_beforeConsecutiveTokenTransfer`, and `_afterConsecutiveTokenTransfer`, and the existing internal functions that should be reviewed are `_exists`, `_beforeTokenTransfer`, and `_afterTokenTransfer`. ### Breaking changes @@ -120,7 +120,7 @@ ERC-721 integrators that interpret contract state from events should make sure t ### Breaking changes * `Governor`: Adds internal virtual `_getVotes` method that must be implemented; this is a breaking change for existing concrete extensions to `Governor`. To fix this on an existing voting module extension, rename `getVotes` to `_getVotes` and add a `bytes memory` argument. ([#3043](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3043)) -* `Governor`: Adds `params` parameter to internal virtual `_countVote` method; this is a breaking change for existing concrete extensions to `Governor`. To fix this on an existing counting module extension, add a `bytes memory` argument to `_countVote`. ([#3043](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3043)) +* `Governor`: Adds `params` parameter to internal virtual `_countVote ` method; this is a breaking change for existing concrete extensions to `Governor`. To fix this on an existing counting module extension, add a `bytes memory` argument to `_countVote`. ([#3043](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3043)) * `Governor`: Does not emit `VoteCast` event when params data is non-empty; instead emits `VoteCastWithParams` event. To fix this on an integration that consumes the `VoteCast` event, also fetch/monitor `VoteCastWithParams` events. ([#3043](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3043)) * `Votes`: The internal virtual function `_getVotingUnits` was made `view` (which was accidentally missing). Any overrides should now be updated so they are `view` as well. @@ -143,7 +143,7 @@ ERC-721 integrators that interpret contract state from events should make sure t * `ERC777`: do not update allowance on `transferFrom` when allowance is `type(uint256).max`. ([#3085](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3085)) * `ERC777`: add a `_spendAllowance` internal function. ([#3170](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3170)) * `SignedMath`: a new signed version of the Math library with `max`, `min`, and `average`. ([#2686](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2686)) - * `SignedMath`: add an `abs(int256)` method that returns the unsigned absolute value of a signed value. ([#2984](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2984)) + * `SignedMath`: add a `abs(int256)` method that returns the unsigned absolute value of a signed value. ([#2984](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2984)) * `ERC1967Upgrade`: Refactor the secure upgrade to use `ERC1822` instead of the previous rollback mechanism. This reduces code complexity and attack surface with similar security guarantees. ([#3021](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3021)) * `UUPSUpgradeable`: Add `ERC1822` compliance to support the updated secure upgrade mechanism. ([#3021](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3021)) * Some more functions have been made virtual to customize them via overrides. In many cases this will not imply that other functions in the contract will automatically adapt to the overridden definitions. People who wish to override should consult the source code to understand the impact and if they need to override any additional functions to achieve the desired behavior. @@ -247,7 +247,7 @@ It is no longer possible to call an `initializer`-protected function from within * `SignatureChecker`: add a signature verification library that supports both EOA and ERC1271 compliant contracts as signers. ([#2532](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2532)) * `Multicall`: add abstract contract with `multicall(bytes[] calldata data)` function to bundle multiple calls together ([#2608](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2608)) * `ECDSA`: add support for ERC2098 short-signatures. ([#2582](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2582)) - * `AccessControl`: add an `onlyRole` modifier to restrict specific function to callers bearing a specific role. ([#2609](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2609)) + * `AccessControl`: add a `onlyRole` modifier to restrict specific function to callers bearing a specific role. ([#2609](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2609)) * `StorageSlot`: add a library for reading and writing primitive types to specific storage slots. ([#2542](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2542)) * UUPS Proxies: add `UUPSUpgradeable` to implement the UUPS proxy pattern together with `EIP1967Proxy`. ([#2542](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2542)) From f6d6c6e21d9856cf1cdeeaf20608e6b01f26a096 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Mon, 5 Sep 2022 11:44:31 -0300 Subject: [PATCH 38/46] Revert "add compatibility note" This reverts commit b8c48c85abc5870812845fb2389337eef9aa1a22. --- CHANGELOG.md | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0a64da414a3..67c5a15a92f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ * `Address`: optimize `functionCall` functions by checking contract size only if there is no returned data. ([#3469](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3469)) * `GovernorCompatibilityBravo`: remove unused `using` statements. ([#3506](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3506)) * `ERC20`: optimize `_transfer`, `_mint` and `_burn` by using `unchecked` arithmetic when possible. ([#3513](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3513)) + * `ERC20Votes`, `ERC721Votes`: optimize `getPastVotes` for looking up recent checkpoints. ([#3673](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3673)) * `ERC20FlashMint`: add an internal `_flashFee` function for overriding. ([#3551](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3551)) * `ERC4626`: use the same `decimals()` as the underlying asset by default (if available). ([#3639](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3639)) * `ERC4626`: add internal `_initialConvertToShares` and `_initialConvertToAssets` functions to customize empty vaults behavior. ([#3639](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3639)) @@ -18,18 +19,17 @@ * `ERC721Consecutive`: Implementation of EIP-2309 that allows batch minting of ERC721 tokens during construction. ([#3311](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3311)) * `ReentrancyGuard`: Reduce code size impact of the modifier by using internal functions. ([#3515](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3515)) * `SafeCast`: optimize downcasting of signed integers. ([#3565](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3565)) - * `VestingWallet`: remove unused library `Math.sol`. ([#3605](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3605)) * `ECDSA`: Remove redundant check on the `v` value. ([#3591](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3591)) * `VestingWallet`: add `releasable` getters. ([#3580](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3580)) + * `VestingWallet`: remove unused library `Math.sol`. ([#3605](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3605)) + * `VestingWallet`: make constructor payable. ([#3665](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3665)) * `Create2`: optimize address computation by using assembly instead of `abi.encodePacked`. ([#3600](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3600)) - * `Clones`: optimized the assembly to use only the scratch space during deployments, and optimized `predictDeterministicAddress` to use lesser operations. ([#3640](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3640)) + * `Clones`: optimized the assembly to use only the scratch space during deployments, and optimized `predictDeterministicAddress` to use fewer operations. ([#3640](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3640)) * `Checkpoints`: Use procedural generation to support multiple key/value lengths. ([#3589](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3589)) * `Checkpoints`: Add new lookup mechanisms. ([#3589](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3589)) - * `Array`: Add `unsafeAccess` functions that allow reading and writing to an element in a storage array bypassing Solidity's "out-of-bounds" check. ([#3589](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3589)) - -### Compatibility note - - * `ERC721Consecutive`: This new extension slightly changes the internal workings of `ERC721`, so custom extensions to ERC721 should be reviewed to ensure they remain correct. The new internal functions that should be considered are `_ownerOf`, `_beforeConsecutiveTokenTransfer`, and `_afterConsecutiveTokenTransfer`, and the existing internal functions that should be reviewed are `_exists`, `_beforeTokenTransfer`, and `_afterTokenTransfer`. + * `Arrays`: Add `unsafeAccess` functions that allow reading and writing to an element in a storage array bypassing Solidity's "out-of-bounds" check. ([#3589](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3589)) + * `Strings`: optimize `toString`. ([#3573](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3573)) + * `Ownable2Step`: extension of `Ownable` that makes the ownership transfers a two step process. ([#3620](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3620)) ### Breaking changes @@ -120,7 +120,7 @@ ERC-721 integrators that interpret contract state from events should make sure t ### Breaking changes * `Governor`: Adds internal virtual `_getVotes` method that must be implemented; this is a breaking change for existing concrete extensions to `Governor`. To fix this on an existing voting module extension, rename `getVotes` to `_getVotes` and add a `bytes memory` argument. ([#3043](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3043)) -* `Governor`: Adds `params` parameter to internal virtual `_countVote ` method; this is a breaking change for existing concrete extensions to `Governor`. To fix this on an existing counting module extension, add a `bytes memory` argument to `_countVote`. ([#3043](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3043)) +* `Governor`: Adds `params` parameter to internal virtual `_countVote` method; this is a breaking change for existing concrete extensions to `Governor`. To fix this on an existing counting module extension, add a `bytes memory` argument to `_countVote`. ([#3043](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3043)) * `Governor`: Does not emit `VoteCast` event when params data is non-empty; instead emits `VoteCastWithParams` event. To fix this on an integration that consumes the `VoteCast` event, also fetch/monitor `VoteCastWithParams` events. ([#3043](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3043)) * `Votes`: The internal virtual function `_getVotingUnits` was made `view` (which was accidentally missing). Any overrides should now be updated so they are `view` as well. @@ -143,7 +143,7 @@ ERC-721 integrators that interpret contract state from events should make sure t * `ERC777`: do not update allowance on `transferFrom` when allowance is `type(uint256).max`. ([#3085](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3085)) * `ERC777`: add a `_spendAllowance` internal function. ([#3170](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3170)) * `SignedMath`: a new signed version of the Math library with `max`, `min`, and `average`. ([#2686](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2686)) - * `SignedMath`: add a `abs(int256)` method that returns the unsigned absolute value of a signed value. ([#2984](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2984)) + * `SignedMath`: add an `abs(int256)` method that returns the unsigned absolute value of a signed value. ([#2984](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2984)) * `ERC1967Upgrade`: Refactor the secure upgrade to use `ERC1822` instead of the previous rollback mechanism. This reduces code complexity and attack surface with similar security guarantees. ([#3021](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3021)) * `UUPSUpgradeable`: Add `ERC1822` compliance to support the updated secure upgrade mechanism. ([#3021](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3021)) * Some more functions have been made virtual to customize them via overrides. In many cases this will not imply that other functions in the contract will automatically adapt to the overridden definitions. People who wish to override should consult the source code to understand the impact and if they need to override any additional functions to achieve the desired behavior. @@ -247,7 +247,7 @@ It is no longer possible to call an `initializer`-protected function from within * `SignatureChecker`: add a signature verification library that supports both EOA and ERC1271 compliant contracts as signers. ([#2532](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2532)) * `Multicall`: add abstract contract with `multicall(bytes[] calldata data)` function to bundle multiple calls together ([#2608](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2608)) * `ECDSA`: add support for ERC2098 short-signatures. ([#2582](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2582)) - * `AccessControl`: add a `onlyRole` modifier to restrict specific function to callers bearing a specific role. ([#2609](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2609)) + * `AccessControl`: add an `onlyRole` modifier to restrict specific function to callers bearing a specific role. ([#2609](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2609)) * `StorageSlot`: add a library for reading and writing primitive types to specific storage slots. ([#2542](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2542)) * UUPS Proxies: add `UUPSUpgradeable` to implement the UUPS proxy pattern together with `EIP1967Proxy`. ([#2542](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2542)) From b34f0c9c330279f6816a481e873555836e514f86 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Mon, 5 Sep 2022 11:47:00 -0300 Subject: [PATCH 39/46] add compatibility note --- CHANGELOG.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 67c5a15a92f..29c4f40e017 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,10 +44,12 @@ +import "@openzeppelin/contracts/utils/cryptography/EIP712.sol"; ``` -### Compatibility Note +### ERC-721 Compatibility Note ERC-721 integrators that interpret contract state from events should make sure that they implement the clearing of approval that is implicit in every transfer according to the EIP. Previous versions of OpenZeppelin Contracts emitted an explicit `Approval` event even though it was not required by the specification, and this is no longer the case. +With the new `ERC721Consecutive` extension, the internal workings of `ERC721` are slightly changed. Custom extensions to ERC721 should be reviewed to ensure they remain correct. The new internal functions that should be considered are `_ownerOf`, `_beforeConsecutiveTokenTransfer`, and `_afterConsecutiveTokenTransfer`, and the existing internal functions that should be reviewed are `_exists`, `_beforeTokenTransfer`, and `_afterTokenTransfer`. + ## 4.7.3 ### Breaking changes From 1ecf1b8041015d23b13a7befafaa17b23f768ce8 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Mon, 5 Sep 2022 11:54:04 -0300 Subject: [PATCH 40/46] fix typo weither -> wether --- contracts/governance/IGovernor.sol | 2 +- contracts/utils/Checkpoints.sol | 6 +++--- scripts/generate/templates/Checkpoints.js | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/contracts/governance/IGovernor.sol b/contracts/governance/IGovernor.sol index 829d516c577..3c331e6a796 100644 --- a/contracts/governance/IGovernor.sol +++ b/contracts/governance/IGovernor.sol @@ -184,7 +184,7 @@ abstract contract IGovernor is IERC165 { /** * @notice module:voting - * @dev Returns weither `account` has cast a vote on `proposalId`. + * @dev Returns wether `account` has cast a vote on `proposalId`. */ function hasVoted(uint256 proposalId, address account) public view virtual returns (bool); diff --git a/contracts/utils/Checkpoints.sol b/contracts/utils/Checkpoints.sol index 68e255c4c9b..a024afc23c8 100644 --- a/contracts/utils/Checkpoints.sol +++ b/contracts/utils/Checkpoints.sol @@ -100,7 +100,7 @@ library Checkpoints { } /** - * @dev Returns weither there is a checkpoint if the structure (structure is not empty), and if so the key and value + * @dev Returns wether there is a checkpoint if the structure (structure is not empty), and if so the key and value * in the most recent checkpoint. */ function latestCheckpoint(History storage self) @@ -261,7 +261,7 @@ library Checkpoints { } /** - * @dev Returns weither there is a checkpoint if the structure (structure is not empty), and if so the key and value + * @dev Returns wether there is a checkpoint if the structure (structure is not empty), and if so the key and value * in the most recent checkpoint. */ function latestCheckpoint(Trace224 storage self) @@ -426,7 +426,7 @@ library Checkpoints { } /** - * @dev Returns weither there is a checkpoint if the structure (structure is not empty), and if so the key and value + * @dev Returns wether there is a checkpoint if the structure (structure is not empty), and if so the key and value * in the most recent checkpoint. */ function latestCheckpoint(Trace160 storage self) diff --git a/scripts/generate/templates/Checkpoints.js b/scripts/generate/templates/Checkpoints.js index 7504d9fc457..22c0b15e768 100644 --- a/scripts/generate/templates/Checkpoints.js +++ b/scripts/generate/templates/Checkpoints.js @@ -141,7 +141,7 @@ function latest(${opts.historyTypeName} storage self) internal view returns (${o } /** - * @dev Returns weither there is a checkpoint if the structure (structure is not empty), and if so the key and value + * @dev Returns wether there is a checkpoint if the structure (structure is not empty), and if so the key and value * in the most recent checkpoint. */ function latestCheckpoint(${opts.historyTypeName} storage self) From 86d8ef088677286ba6b4c14d6adc25edba06b421 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Mon, 5 Sep 2022 11:55:10 -0300 Subject: [PATCH 41/46] fix typos --- contracts/governance/IGovernor.sol | 2 +- contracts/utils/Checkpoints.sol | 6 +++--- scripts/generate/templates/Checkpoints.js | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/contracts/governance/IGovernor.sol b/contracts/governance/IGovernor.sol index 3c331e6a796..a93231ca987 100644 --- a/contracts/governance/IGovernor.sol +++ b/contracts/governance/IGovernor.sol @@ -184,7 +184,7 @@ abstract contract IGovernor is IERC165 { /** * @notice module:voting - * @dev Returns wether `account` has cast a vote on `proposalId`. + * @dev Returns whether `account` has cast a vote on `proposalId`. */ function hasVoted(uint256 proposalId, address account) public view virtual returns (bool); diff --git a/contracts/utils/Checkpoints.sol b/contracts/utils/Checkpoints.sol index a024afc23c8..7b75cdd00bf 100644 --- a/contracts/utils/Checkpoints.sol +++ b/contracts/utils/Checkpoints.sol @@ -100,7 +100,7 @@ library Checkpoints { } /** - * @dev Returns wether there is a checkpoint if the structure (structure is not empty), and if so the key and value + * @dev Returns whether there is a checkpoint in the structure (i.e. it is not empty), and if so the key and value * in the most recent checkpoint. */ function latestCheckpoint(History storage self) @@ -261,7 +261,7 @@ library Checkpoints { } /** - * @dev Returns wether there is a checkpoint if the structure (structure is not empty), and if so the key and value + * @dev Returns whether there is a checkpoint in the structure (i.e. it is not empty), and if so the key and value * in the most recent checkpoint. */ function latestCheckpoint(Trace224 storage self) @@ -426,7 +426,7 @@ library Checkpoints { } /** - * @dev Returns wether there is a checkpoint if the structure (structure is not empty), and if so the key and value + * @dev Returns whether there is a checkpoint in the structure (i.e. it is not empty), and if so the key and value * in the most recent checkpoint. */ function latestCheckpoint(Trace160 storage self) diff --git a/scripts/generate/templates/Checkpoints.js b/scripts/generate/templates/Checkpoints.js index 22c0b15e768..46d43f92b99 100644 --- a/scripts/generate/templates/Checkpoints.js +++ b/scripts/generate/templates/Checkpoints.js @@ -141,7 +141,7 @@ function latest(${opts.historyTypeName} storage self) internal view returns (${o } /** - * @dev Returns wether there is a checkpoint if the structure (structure is not empty), and if so the key and value + * @dev Returns whether there is a checkpoint in the structure (i.e. it is not empty), and if so the key and value * in the most recent checkpoint. */ function latestCheckpoint(${opts.historyTypeName} storage self) From b5903520aa935c6703d9d6bdeb15c65ae2c60b0e Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Mon, 5 Sep 2022 11:58:03 -0300 Subject: [PATCH 42/46] grammar --- contracts/token/ERC721/extensions/ERC721Consecutive.sol | 4 ++-- contracts/utils/Checkpoints.sol | 6 +++--- scripts/generate/templates/Checkpoints.js | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/contracts/token/ERC721/extensions/ERC721Consecutive.sol b/contracts/token/ERC721/extensions/ERC721Consecutive.sol index 5b12626fca1..8ad8bcdbc0d 100644 --- a/contracts/token/ERC721/extensions/ERC721Consecutive.sol +++ b/contracts/token/ERC721/extensions/ERC721Consecutive.sol @@ -117,7 +117,7 @@ abstract contract ERC721Consecutive is ERC721, IERC2309 { } function _totalConsecutiveSupply() private view returns (uint96) { - (bool exist, uint96 latestId, ) = _sequentialOwnership.latestCheckpoint(); - return exist ? latestId + 1 : 0; + (bool exists, uint96 latestId, ) = _sequentialOwnership.latestCheckpoint(); + return exists ? latestId + 1 : 0; } } diff --git a/contracts/utils/Checkpoints.sol b/contracts/utils/Checkpoints.sol index 7b75cdd00bf..c8a062ae7d1 100644 --- a/contracts/utils/Checkpoints.sol +++ b/contracts/utils/Checkpoints.sol @@ -107,7 +107,7 @@ library Checkpoints { internal view returns ( - bool exist, + bool exists, uint32 _blockNumber, uint224 _value ) @@ -268,7 +268,7 @@ library Checkpoints { internal view returns ( - bool exist, + bool exists, uint32 _key, uint224 _value ) @@ -433,7 +433,7 @@ library Checkpoints { internal view returns ( - bool exist, + bool exists, uint96 _key, uint160 _value ) diff --git a/scripts/generate/templates/Checkpoints.js b/scripts/generate/templates/Checkpoints.js index 46d43f92b99..61c398d8fd9 100644 --- a/scripts/generate/templates/Checkpoints.js +++ b/scripts/generate/templates/Checkpoints.js @@ -148,7 +148,7 @@ function latestCheckpoint(${opts.historyTypeName} storage self) internal view returns ( - bool exist, + bool exists, ${opts.keyTypeName} ${opts.keyFieldName}, ${opts.valueTypeName} ${opts.valueFieldName} ) From d23313d75b9d9d52fe69b675fbd49d35bbe70883 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Mon, 5 Sep 2022 17:00:25 -0300 Subject: [PATCH 43/46] reorder inheritance --- contracts/token/ERC721/extensions/ERC721Consecutive.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/token/ERC721/extensions/ERC721Consecutive.sol b/contracts/token/ERC721/extensions/ERC721Consecutive.sol index 8ad8bcdbc0d..d6b61bea2ee 100644 --- a/contracts/token/ERC721/extensions/ERC721Consecutive.sol +++ b/contracts/token/ERC721/extensions/ERC721Consecutive.sol @@ -28,7 +28,7 @@ import "../../../utils/structs/BitMaps.sol"; * * _Available since v4.8._ */ -abstract contract ERC721Consecutive is ERC721, IERC2309 { +abstract contract ERC721Consecutive is IERC2309, ERC721 { using BitMaps for BitMaps.BitMap; using Checkpoints for Checkpoints.Trace160; From e35e4f603c3ebcb3b49943e06f09ff10fdfcea2b Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Mon, 5 Sep 2022 17:01:51 -0300 Subject: [PATCH 44/46] grammar --- contracts/token/ERC721/extensions/ERC721Consecutive.sol | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/contracts/token/ERC721/extensions/ERC721Consecutive.sol b/contracts/token/ERC721/extensions/ERC721Consecutive.sol index d6b61bea2ee..e543b1e3d5a 100644 --- a/contracts/token/ERC721/extensions/ERC721Consecutive.sol +++ b/contracts/token/ERC721/extensions/ERC721Consecutive.sol @@ -15,11 +15,11 @@ import "../../../utils/structs/BitMaps.sol"; * This extension allows the minting of large batches of tokens during the contract construction. These batches are * limited to 5000 tokens at a time to accommodate off-chain indexers. * - * Using this extensions removes the ability to mint single token during the contract construction. This ability is + * Using this extension removes the ability to mint single tokens during the contract construction. This ability is * regained after construction. During construction, only batch minting is allowed. * - * IMPORTANT: This extension bypasses the hooks {_beforeTokenTransfer} and {_afterTokenTransfer} for token minted in - * batch. When using this extensions, you should consider the {_beforeConsecutiveTokenTransfer} and + * IMPORTANT: This extension bypasses the hooks {_beforeTokenTransfer} and {_afterTokenTransfer} for tokens minted in + * batch. When using this extension, you should consider the {_beforeConsecutiveTokenTransfer} and * {_afterConsecutiveTokenTransfer} hooks in addition to {_beforeTokenTransfer} and {_afterTokenTransfer}. * * IMPORTANT: When overriding {_afterTokenTransfer}, be careful about call ordering. {ownerOf} may return invalid From 3c85862be8e1686aabec4d5c880ca20006b845cb Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Mon, 5 Sep 2022 17:27:27 -0300 Subject: [PATCH 45/46] grammar --- contracts/token/ERC721/extensions/ERC721Consecutive.sol | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/contracts/token/ERC721/extensions/ERC721Consecutive.sol b/contracts/token/ERC721/extensions/ERC721Consecutive.sol index e543b1e3d5a..538809b80df 100644 --- a/contracts/token/ERC721/extensions/ERC721Consecutive.sol +++ b/contracts/token/ERC721/extensions/ERC721Consecutive.sol @@ -36,7 +36,7 @@ abstract contract ERC721Consecutive is IERC2309, ERC721 { BitMaps.BitMap private _sequentialBurn; /** - * @dev See {ERC721-_ownerOf}. Override version that checks the sequential ownership structure for tokens that have + * @dev See {ERC721-_ownerOf}. Override that checks the sequential ownership structure for tokens that have * been minted as part of a batch, and not yet transferred. */ function _ownerOf(uint256 tokenId) internal view virtual override returns (address) { @@ -53,10 +53,10 @@ abstract contract ERC721Consecutive is IERC2309, ERC721 { } /** - * @dev Mint a batch of token of length `batchSize` for `to`. + * @dev Mint a batch of tokens of length `batchSize` for `to`. * * WARNING: Consecutive mint is only available during construction. ERC721 requires that any minting done after - * construction emits a Transfer event, which is not the case of mints performed using this function. + * construction emits a `Transfer` event, which is not the case of mints performed using this function. * * WARNING: Consecutive mint is limited to batches of 5000 tokens. Further minting is possible from a contract's * point of view but would cause indexing issues for off-chain services. From b7b94348dc04753171a18867715ac1edd1b49f80 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Mon, 5 Sep 2022 17:27:43 -0300 Subject: [PATCH 46/46] test batch of 5001 not allowed --- .../ERC721/extensions/ERC721Consecutive.sol | 2 +- .../extensions/ERC721Consecutive.test.js | 241 ++++++++++-------- 2 files changed, 135 insertions(+), 108 deletions(-) diff --git a/contracts/token/ERC721/extensions/ERC721Consecutive.sol b/contracts/token/ERC721/extensions/ERC721Consecutive.sol index 538809b80df..15ecdd98650 100644 --- a/contracts/token/ERC721/extensions/ERC721Consecutive.sol +++ b/contracts/token/ERC721/extensions/ERC721Consecutive.sol @@ -70,7 +70,7 @@ abstract contract ERC721Consecutive is IERC2309, ERC721 { if (batchSize > 0) { require(!Address.isContract(address(this)), "ERC721Consecutive: batch minting restricted to constructor"); require(to != address(0), "ERC721Consecutive: mint to the zero address"); - require(batchSize <= 5000, "ERC721Consecutive: batches too large for indexing"); + require(batchSize <= 5000, "ERC721Consecutive: batch too large"); // hook before _beforeConsecutiveTokenTransfer(address(0), to, first, batchSize); diff --git a/test/token/ERC721/extensions/ERC721Consecutive.test.js b/test/token/ERC721/extensions/ERC721Consecutive.test.js index fb29fd7a927..de3af0a077a 100644 --- a/test/token/ERC721/extensions/ERC721Consecutive.test.js +++ b/test/token/ERC721/extensions/ERC721Consecutive.test.js @@ -19,146 +19,173 @@ contract('ERC721Consecutive', function (accounts) { ]; const delegates = [ user1, user3 ]; - beforeEach(async function () { - this.token = await ERC721ConsecutiveMock.new( - name, - symbol, - delegates, - batches.map(({ receiver }) => receiver), - batches.map(({ amount }) => amount), - ); - }); + describe('with valid batches', function () { + beforeEach(async function () { + this.token = await ERC721ConsecutiveMock.new( + name, + symbol, + delegates, + batches.map(({ receiver }) => receiver), + batches.map(({ amount }) => amount), + ); + }); - describe('minting during construction', function () { - it('events are emitted at construction', async function () { - let first = 0; - - for (const batch of batches) { - if (batch.amount > 0) { - await expectEvent.inConstruction(this.token, 'ConsecutiveTransfer', { - fromTokenId: web3.utils.toBN(first), - toTokenId: web3.utils.toBN(first + batch.amount - 1), - fromAddress: constants.ZERO_ADDRESS, - toAddress: batch.receiver, - }); - } else { - // expectEvent.notEmitted.inConstruction only looks at event name, and doesn't check the parameters + describe('minting during construction', function () { + it('events are emitted at construction', async function () { + let first = 0; + + for (const batch of batches) { + if (batch.amount > 0) { + await expectEvent.inConstruction(this.token, 'ConsecutiveTransfer', { + fromTokenId: web3.utils.toBN(first), + toTokenId: web3.utils.toBN(first + batch.amount - 1), + fromAddress: constants.ZERO_ADDRESS, + toAddress: batch.receiver, + }); + } else { + // expectEvent.notEmitted.inConstruction only looks at event name, and doesn't check the parameters + } + first += batch.amount; } - first += batch.amount; - } + }); + + it('ownership is set', async function () { + const owners = batches.flatMap(({ receiver, amount }) => Array(amount).fill(receiver)); + + for (const tokenId in owners) { + expect(await this.token.ownerOf(tokenId)) + .to.be.equal(owners[tokenId]); + } + }); + + it('balance & voting power are set', async function () { + for (const account of accounts) { + const balance = batches + .filter(({ receiver }) => receiver === account) + .map(({ amount }) => amount) + .reduce((a, b) => a + b, 0); + + expect(await this.token.balanceOf(account)) + .to.be.bignumber.equal(web3.utils.toBN(balance)); + + // If not delegated at construction, check before + do delegation + if (!delegates.includes(account)) { + expect(await this.token.getVotes(account)) + .to.be.bignumber.equal(web3.utils.toBN(0)); + + await this.token.delegate(account, { from: account }); + } + + // At this point all accounts should have delegated + expect(await this.token.getVotes(account)) + .to.be.bignumber.equal(web3.utils.toBN(balance)); + } + }); }); - it('ownership is set', async function () { - const owners = batches.flatMap(({ receiver, amount }) => Array(amount).fill(receiver)); + describe('minting after construction', function () { + it('consecutive minting is not possible after construction', async function () { + await expectRevert( + this.token.mintConsecutive(user1, 10), + 'ERC721Consecutive: batch minting restricted to constructor', + ); + }); + + it('simple minting is possible after construction', async function () { + const tokenId = batches.reduce((acc, { amount }) => acc + amount, 0); + + expect(await this.token.exists(tokenId)).to.be.equal(false); + + expectEvent( + await this.token.mint(user1, tokenId), + 'Transfer', + { from: constants.ZERO_ADDRESS, to: user1, tokenId: tokenId.toString() }, + ); + }); + + it('cannot mint a token that has been batched minted', async function () { + const tokenId = batches.reduce((acc, { amount }) => acc + amount, 0) - 1; - for (const tokenId in owners) { - expect(await this.token.ownerOf(tokenId)) - .to.be.equal(owners[tokenId]); - } + expect(await this.token.exists(tokenId)).to.be.equal(true); + + await expectRevert( + this.token.mint(user1, tokenId), + 'ERC721: token already minted', + ); + }); }); - it('balance & voting power are set', async function () { - for (const account of accounts) { - const balance = batches - .filter(({ receiver }) => receiver === account) - .map(({ amount }) => amount) - .reduce((a, b) => a + b, 0); + describe('ERC721 behavior', function () { + it('core takes over ownership on transfer', async function () { + await this.token.transferFrom(user1, receiver, 1, { from: user1 }); - expect(await this.token.balanceOf(account)) - .to.be.bignumber.equal(web3.utils.toBN(balance)); + expect(await this.token.ownerOf(1)).to.be.equal(receiver); + }); - // If not delegated at construction, check before + do delegation - if (!delegates.includes(account)) { - expect(await this.token.getVotes(account)) - .to.be.bignumber.equal(web3.utils.toBN(0)); + it('tokens can be burned and re-minted', async function () { + expectEvent( + await this.token.burn(1, { from: user1 }), + 'Transfer', + { from: user1, to: constants.ZERO_ADDRESS, tokenId: '1' }, + ); - await this.token.delegate(account, { from: account }); - } + await expectRevert(this.token.ownerOf(1), 'ERC721: invalid token ID'); + + expectEvent( + await this.token.mint(user2, 1), + 'Transfer', + { from: constants.ZERO_ADDRESS, to: user2, tokenId: '1' }, + ); - // At this point all accounts should have delegated - expect(await this.token.getVotes(account)) - .to.be.bignumber.equal(web3.utils.toBN(balance)); - } + expect(await this.token.ownerOf(1)).to.be.equal(user2); + }); }); + }); - it('cannot use single minting during construction', async function () { + describe('invalid use', function () { + it('cannot mint a batch larger than 5000', async function () { await expectRevert( - ERC721ConsecutiveNoConstructorMintMock.new( + ERC721ConsecutiveMock.new( name, symbol, + [], + [user1], + ['5001'], ), - 'ERC721Consecutive: can\'t mint during construction', + 'ERC721Consecutive: batch too large', ); }); - it('consecutive mint not compatible with enumerability', async function () { + it('cannot use single minting during construction', async function () { await expectRevert( - ERC721ConsecutiveEnumerableMock.new( + ERC721ConsecutiveNoConstructorMintMock.new( name, symbol, - batches.map(({ receiver }) => receiver), - batches.map(({ amount }) => amount), ), - 'ERC721Enumerable: consecutive transfers not supported', + 'ERC721Consecutive: can\'t mint during construction', ); }); - }); - describe('minting after construction', function () { - it('consecutive minting is not possible after construction', async function () { + it('cannot use single minting during construction', async function () { await expectRevert( - this.token.mintConsecutive(user1, 10), - 'ERC721Consecutive: batch minting restricted to constructor', - ); - }); - - it('simple minting is possible after construction', async function () { - const tokenId = batches.reduce((acc, { amount }) => acc + amount, 0); - - expect(await this.token.exists(tokenId)).to.be.equal(false); - - expectEvent( - await this.token.mint(user1, tokenId), - 'Transfer', - { from: constants.ZERO_ADDRESS, to: user1, tokenId: tokenId.toString() }, + ERC721ConsecutiveNoConstructorMintMock.new( + name, + symbol, + ), + 'ERC721Consecutive: can\'t mint during construction', ); }); - it('cannot mint a token that has been batched minted', async function () { - const tokenId = batches.reduce((acc, { amount }) => acc + amount, 0) - 1; - - expect(await this.token.exists(tokenId)).to.be.equal(true); - + it('consecutive mint not compatible with enumerability', async function () { await expectRevert( - this.token.mint(user1, tokenId), - 'ERC721: token already minted', - ); - }); - }); - - describe('ERC721 behavior', function () { - it('core takes over ownership on transfer', async function () { - await this.token.transferFrom(user1, receiver, 1, { from: user1 }); - - expect(await this.token.ownerOf(1)).to.be.equal(receiver); - }); - - it('tokens can be burned and re-minted', async function () { - expectEvent( - await this.token.burn(1, { from: user1 }), - 'Transfer', - { from: user1, to: constants.ZERO_ADDRESS, tokenId: '1' }, - ); - - await expectRevert(this.token.ownerOf(1), 'ERC721: invalid token ID'); - - expectEvent( - await this.token.mint(user2, 1), - 'Transfer', - { from: constants.ZERO_ADDRESS, to: user2, tokenId: '1' }, + ERC721ConsecutiveEnumerableMock.new( + name, + symbol, + batches.map(({ receiver }) => receiver), + batches.map(({ amount }) => amount), + ), + 'ERC721Enumerable: consecutive transfers not supported', ); - - expect(await this.token.ownerOf(1)).to.be.equal(user2); }); }); });