From ba80d01d78e469b907b76f5737020d8a5c81d5db Mon Sep 17 00:00:00 2001 From: jacopo <39241410+jjranalli@users.noreply.github.com> Date: Tue, 20 Aug 2024 20:40:41 +0200 Subject: [PATCH 01/13] Add `batchTransferFrom` and `ERC721ABatchTransferable` extension (#458) * added comments on transfer hooks * added sort * added clearApprovalsAndEmitTransferEvent * added tokenBatchTransfer hooks * added _batchTransferFrom and safe variants * added ERC721ABatchTransferable extension and interface * formatting * added interface and ERC721ABatchTransferableMock * added ERC721ABatchTransferable tests (wip) * added approvalCheck * fixed duplicate call * comment * fixed next initialized * refactored lastInitPackedOwnership to use prevPackedOwnership * comments * ensured correctness of nextInitialized in slots of transferred token Ids * renamed variables * reverted to leave nextInitialized unchanged * comment * replace sort -> insertion sort * bump: prettier-plugin-solidity * prettier * added prettier-ignore * fixed nextTokenId in last array element * tests wip * refactor * updated BatchTransferable mock and extension * updated tests * add approval tests * lint * lint fix * restore original .prettierrc * fix * comments and refactor * added _batchBurn * added ERC721ABatchBurnable extension, interfaces and mock * fixed _batchBurn * fixed update of last tokenId + 1 * batchBurnable tests wip * refactor * fix * add auto-clearing of consecutive ids and set `nextInitialized` to false * batchTransfer tests refactor * tests wip * tests wip * comments * added extraData logic to batch mocks * updated batch tests * refactored ERC721A to use _updateTokenId * wip * comment * Add ERC721ABatchBurnableMock (#450) * change tokenIds in ascending order in test * removal of unneeded internal functions * prettier * removed batch transfer logic * changed _updateTokenId * fixed mock * fixed extension and mock * fixed tests and cleaned unused functions in mock * removed _updateTokenId * minor gas optimizations * comment * optimize: avoid potential double read from storage * removed bulkBurn from mock * optimization: reset _packedOwnerships for initialized sequential IDs * added tests for sequential ID clearing * added test for tokenIds in strictly ascending order * comment * optimize: keep track of prevTokenOwner to bypass duplicated logic * revert: resetting _packedOwnerships in initialized sequential IDs * cleanup * optimize: avoid potential double read from storage * refactor _batchTransfer logic * optimized and stacked not too deep * optimize: removed unneeded exists() via getApproved * removed unneeded functions and batchBurn --- .prettierrc | 2 +- contracts/ERC721A.sol | 211 +++++++ contracts/IERC721A.sol | 5 + .../extensions/ERC721ABatchTransferable.sol | 40 ++ .../extensions/IERC721ABatchTransferable.sol | 62 ++ .../interfaces/IERC721ABatchTransferable.sol | 7 + .../mocks/ERC721ABatchTransferableMock.sol | 69 +++ .../ERC721ABatchTransferable.test.js | 529 ++++++++++++++++++ 8 files changed, 924 insertions(+), 1 deletion(-) create mode 100644 contracts/extensions/ERC721ABatchTransferable.sol create mode 100644 contracts/extensions/IERC721ABatchTransferable.sol create mode 100644 contracts/interfaces/IERC721ABatchTransferable.sol create mode 100644 contracts/mocks/ERC721ABatchTransferableMock.sol create mode 100644 test/extensions/ERC721ABatchTransferable.test.js diff --git a/.prettierrc b/.prettierrc index 7ec948a64..3dff7f42f 100644 --- a/.prettierrc +++ b/.prettierrc @@ -10,4 +10,4 @@ } } ] -} \ No newline at end of file +} diff --git a/contracts/ERC721A.sol b/contracts/ERC721A.sol index c4bcfa9d5..f4341b9e7 100644 --- a/contracts/ERC721A.sol +++ b/contracts/ERC721A.sol @@ -712,6 +712,208 @@ contract ERC721A is IERC721A { } } + /** + * @dev Equivalent to `_batchTransferFrom(from, to, tokenIds, false)`. + */ + function _batchTransferFrom( + address from, + address to, + uint256[] memory tokenIds + ) internal virtual { + _batchTransferFrom(from, to, tokenIds, false); + } + + /** + * @dev Transfers `tokenIds` in batch from `from` to `to`. + * + * Requirements: + * + * - `from` cannot be the zero address. + * - `to` cannot be the zero address. + * - `tokenIds` tokens must be owned by `from`. + * - `tokenIds` must be strictly ascending. + * - If the caller is not `from`, it must be approved to move these tokens + * by either {approve} or {setApprovalForAll}. + * + * Emits a {Transfer} event for each transfer. + */ + function _batchTransferFrom( + address from, + address to, + uint256[] memory tokenIds, + bool approvalCheck + ) internal virtual { + // We can use unchecked as the length of `tokenIds` is bounded + // to a small number by the max block gas limit. + unchecked { + // Mask `from` and `to` to the lower 160 bits, in case the upper bits somehow aren't clean. + from = address(uint160(uint256(uint160(from)) & _BITMASK_ADDRESS)); + if (uint256(uint160(to)) & _BITMASK_ADDRESS == 0) revert TransferToZeroAddress(); + + // Disable `approvalCheck` if sender is either the owner or an approved operator for all tokens + approvalCheck = from != _msgSenderERC721A() && !isApprovedForAll(from, _msgSenderERC721A()); + + uint256 n = tokenIds.length; + + // Increment and decrement the balances. + _packedAddressData[from] -= n; + _packedAddressData[to] += n; + + // The next `tokenId` to be minted (i.e. `_nextTokenId()`). + uint256 stop = _currentIndex; + + // For checking if the `tokenIds` are strictly ascending. + uint256 prevTokenId; + + uint256 tokenId; + uint256 currTokenId; + uint256 prevOwnershipPacked; + uint256 lastOwnershipPacked; + for (uint256 i; i != n; ) { + tokenId = tokenIds[i]; + + // Revert `tokenId` is out of bounds. + if (_or(tokenId < _startTokenId(), stop <= tokenId)) revert OwnerQueryForNonexistentToken(); + + // Revert if `tokenIds` is not strictly ascending. + if (i != 0) + if (tokenId <= prevTokenId) revert TokenIdsNotStrictlyAscending(); + + // Scan backwards for an initialized packed ownership slot. + // ERC721A's invariant guarantees that there will always be an initialized slot as long as + // the start of the backwards scan falls within `[_startTokenId() .. _nextTokenId())`. + for (uint256 j = tokenId; (prevOwnershipPacked = _packedOwnerships[j]) == 0; ) --j; + + // If the initialized slot is burned, revert. + if (prevOwnershipPacked & _BITMASK_BURNED != 0) revert OwnerQueryForNonexistentToken(); + + // Check ownership of `tokenId`. + if (address(uint160(prevOwnershipPacked)) != from) revert TransferFromIncorrectOwner(); + + currTokenId = tokenId; + uint256 offset; + do { + address approvedAddress = _tokenApprovals[currTokenId].value; + + // Revert if the sender is not authorized to transfer the token. + if (approvalCheck) { + if (_msgSenderERC721A() != approvedAddress) revert TransferCallerNotOwnerNorApproved(); + } + + // Call the hook. + _beforeTokenTransfers(from, to, currTokenId, 1); + + if (approvedAddress != address(0)) delete _tokenApprovals[currTokenId]; + + // Emit the `Transfer` event. + emit Transfer(from, to, currTokenId); + // Call the hook. + _afterTokenTransfers(from, to, currTokenId, 1); + // Increment `offset` and update `currTokenId`. + currTokenId = tokenId + (++offset); + } while ( + // Neither out of bounds, nor at the end of `tokenIds`. + !_or(currTokenId == stop, i + offset == n) && + // Token ID is sequential. + tokenIds[i + offset] == currTokenId && + // The packed ownership slot is not initialized. + (lastOwnershipPacked = _packedOwnerships[currTokenId]) == 0 + ); + + // Updates tokenId: + // - `address` to the next owner. + // - `startTimestamp` to the timestamp of transfering. + // - `burned` to `false`. + // - `nextInitialized` to `false`. + _packedOwnerships[tokenId] = _packOwnershipData(to, _nextExtraData(from, to, prevOwnershipPacked)); + + // If the slot after the mini batch is neither out of bounds, nor initialized. + // If `lastOwnershipPacked == 0` we didn't break the loop due to an initialized slot. + if (currTokenId != stop) + if (lastOwnershipPacked == 0) + if (_packedOwnerships[currTokenId] == 0) _packedOwnerships[currTokenId] = prevOwnershipPacked; + + // Advance `i` by `offset`, the number of tokens transferred in the mini batch. + i += offset; + + // Set the `prevTokenId` for checking that the `tokenIds` is strictly ascending. + prevTokenId = currTokenId - 1; + } + } + } + + /** + * @dev Equivalent to `_safeBatchTransferFrom(from, to, tokenIds, false)`. + */ + function _safeBatchTransferFrom( + address from, + address to, + uint256[] memory tokenIds + ) internal virtual { + _safeBatchTransferFrom(from, to, tokenIds, false); + } + + /** + * @dev Equivalent to `_safeBatchTransferFrom(from, to, tokenIds, '', approvalCheck)`. + */ + function _safeBatchTransferFrom( + address from, + address to, + uint256[] memory tokenIds, + bool approvalCheck + ) internal virtual { + _safeBatchTransferFrom(from, to, tokenIds, '', approvalCheck); + } + + /** + * @dev Equivalent to `_safeBatchTransferFrom(from, to, tokenIds, _data, false)`. + */ + function _safeBatchTransferFrom( + address from, + address to, + uint256[] memory tokenIds, + bytes memory _data + ) internal virtual { + _safeBatchTransferFrom(from, to, tokenIds, _data, false); + } + + /** + * @dev Safely transfers `tokenIds` in batch from `from` to `to`. + * + * Requirements: + * + * - `from` cannot be the zero address. + * - `to` cannot be the zero address. + * - `tokenIds` tokens must be owned by `from`. + * - If the caller is not `from`, it must be approved to move these tokens + * by either {approve} or {setApprovalForAll}. + * - If `to` refers to a smart contract, it must implement + * {IERC721Receiver-onERC721Received}, which is called for each transferred token. + * + * Emits a {Transfer} event for each transfer. + */ + function _safeBatchTransferFrom( + address from, + address to, + uint256[] memory tokenIds, + bytes memory _data, + bool approvalCheck + ) internal virtual { + _batchTransferFrom(from, to, tokenIds, approvalCheck); + + uint256 tokenId; + uint256 n = tokenIds.length; + unchecked { + for (uint256 i; i < n; ++i) { + tokenId = tokenIds[i]; + if (to.code.length != 0) + if (!_checkContractOnERC721Received(from, to, tokenId, _data)) { + _revert(TransferToNonERC721ReceiverImplementer.selector); + } + } + } + } + /** * @dev Hook that is called before a set of serially-ordered token IDs * are about to be transferred. This includes minting. @@ -1311,4 +1513,13 @@ contract ERC721A is IERC721A { revert(0x00, 0x04) } } + + /** + * @dev Branchless or. + */ + function _or(bool a, bool b) private pure returns (bool c) { + assembly { + c := or(a, b) + } + } } diff --git a/contracts/IERC721A.sol b/contracts/IERC721A.sol index ebde3074b..e7815e1f1 100644 --- a/contracts/IERC721A.sol +++ b/contracts/IERC721A.sol @@ -74,6 +74,11 @@ interface IERC721A { */ error OwnershipNotInitializedForExtraData(); + /** + * The `tokenIds` must be strictly ascending. + */ + error TokenIdsNotStrictlyAscending(); + /** * `_sequentialUpTo()` must be greater than `_startTokenId()`. */ diff --git a/contracts/extensions/ERC721ABatchTransferable.sol b/contracts/extensions/ERC721ABatchTransferable.sol new file mode 100644 index 000000000..d4caa4a0a --- /dev/null +++ b/contracts/extensions/ERC721ABatchTransferable.sol @@ -0,0 +1,40 @@ +// SPDX-License-Identifier: MIT +// ERC721A Contracts v4.2.3 +// Creator: Chiru Labs + +pragma solidity ^0.8.4; + +import '../ERC721A.sol'; +import './IERC721ABatchTransferable.sol'; + +/** + * @title ERC721ABatchTransferable. + * + * @dev ERC721A token optimized for batch transfers. + */ +abstract contract ERC721ABatchTransferable is ERC721A, IERC721ABatchTransferable { + function batchTransferFrom( + address from, + address to, + uint256[] memory tokenIds + ) public payable virtual override { + _batchTransferFrom(from, to, tokenIds, true); + } + + function safeBatchTransferFrom( + address from, + address to, + uint256[] memory tokenIds + ) public payable virtual override { + _safeBatchTransferFrom(from, to, tokenIds, true); + } + + function safeBatchTransferFrom( + address from, + address to, + uint256[] memory tokenIds, + bytes memory _data + ) public payable virtual override { + _safeBatchTransferFrom(from, to, tokenIds, _data, true); + } +} diff --git a/contracts/extensions/IERC721ABatchTransferable.sol b/contracts/extensions/IERC721ABatchTransferable.sol new file mode 100644 index 000000000..0b984e16b --- /dev/null +++ b/contracts/extensions/IERC721ABatchTransferable.sol @@ -0,0 +1,62 @@ +// SPDX-License-Identifier: MIT +// ERC721A Contracts v4.2.3 +// Creator: Chiru Labs + +pragma solidity ^0.8.4; + +import '../IERC721A.sol'; + +/** + * @dev Interface of ERC721ABatchTransferable. + */ +interface IERC721ABatchTransferable is IERC721A { + /** + * @dev Transfers `tokenIds` in batch from `from` to `to`. See {ERC721A-_batchTransferFrom}. + * + * Requirements: + * + * - `from` cannot be the zero address. + * - `to` cannot be the zero address. + * - `tokenIds` tokens must be owned by `from`. + * - If the caller is not `from`, it must be approved to move these tokens + * by either {approve} or {setApprovalForAll}. + * + * Emits a {Transfer} event for each transfer. + */ + function batchTransferFrom( + address from, + address to, + uint256[] memory tokenIds + ) external payable; + + /** + * @dev Equivalent to `safeBatchTransferFrom(from, to, tokenIds, '')`. + */ + function safeBatchTransferFrom( + address from, + address to, + uint256[] memory tokenIds + ) external payable; + + /** + * @dev Safely transfers `tokenIds` in batch from `from` to `to`. See {ERC721A-_safeBatchTransferFrom}. + * + * Requirements: + * + * - `from` cannot be the zero address. + * - `to` cannot be the zero address. + * - `tokenIds` tokens must be owned by `from`. + * - If the caller is not `from`, it must be approved to move these tokens + * by either {approve} or {setApprovalForAll}. + * - If `to` refers to a smart contract, it must implement + * {IERC721Receiver-onERC721Received}, which is called for each transferred token. + * + * Emits a {Transfer} event for each transfer. + */ + function safeBatchTransferFrom( + address from, + address to, + uint256[] memory tokenIds, + bytes memory _data + ) external payable; +} diff --git a/contracts/interfaces/IERC721ABatchTransferable.sol b/contracts/interfaces/IERC721ABatchTransferable.sol new file mode 100644 index 000000000..bde71ccae --- /dev/null +++ b/contracts/interfaces/IERC721ABatchTransferable.sol @@ -0,0 +1,7 @@ +// SPDX-License-Identifier: MIT +// ERC721A Contracts v4.2.3 +// Creator: Chiru Labs + +pragma solidity ^0.8.4; + +import '../extensions/IERC721ABatchTransferable.sol'; diff --git a/contracts/mocks/ERC721ABatchTransferableMock.sol b/contracts/mocks/ERC721ABatchTransferableMock.sol new file mode 100644 index 000000000..d0aaea814 --- /dev/null +++ b/contracts/mocks/ERC721ABatchTransferableMock.sol @@ -0,0 +1,69 @@ +// SPDX-License-Identifier: MIT +// ERC721A Contracts v4.2.3 +// Creators: Chiru Labs + +pragma solidity ^0.8.4; + +import '../extensions/ERC721ABatchTransferable.sol'; + +contract ERC721ABatchTransferableMock is ERC721ABatchTransferable { + constructor(string memory name_, string memory symbol_) ERC721A(name_, symbol_) {} + + function exists(uint256 tokenId) public view returns (bool) { + return _exists(tokenId); + } + + function safeMint(address to, uint256 quantity) public { + _safeMint(to, quantity); + } + + function getOwnershipAt(uint256 index) public view returns (TokenOwnership memory) { + return _ownershipAt(index); + } + + function totalMinted() public view returns (uint256) { + return _totalMinted(); + } + + function totalBurned() public view returns (uint256) { + return _totalBurned(); + } + + function numberBurned(address owner) public view returns (uint256) { + return _numberBurned(owner); + } + + function burn(uint256 tokenId) public { + _burn(tokenId, true); + } + + function initializeOwnershipAt(uint256 index) public { + _initializeOwnershipAt(index); + } + + function _extraData( + address, + address, + uint24 previousExtraData + ) internal view virtual override returns (uint24) { + return previousExtraData; + } + + function setExtraDataAt(uint256 index, uint24 extraData) public { + _setExtraDataAt(index, extraData); + } + + function batchTransferFromUnoptimized( + address from, + address to, + uint256[] memory tokenIds + ) public { + unchecked { + uint256 tokenId; + for (uint256 i; i < tokenIds.length; ++i) { + tokenId = tokenIds[i]; + transferFrom(from, to, tokenId); + } + } + } +} diff --git a/test/extensions/ERC721ABatchTransferable.test.js b/test/extensions/ERC721ABatchTransferable.test.js new file mode 100644 index 000000000..20382f7f2 --- /dev/null +++ b/test/extensions/ERC721ABatchTransferable.test.js @@ -0,0 +1,529 @@ +const { deployContract, getBlockTimestamp, mineBlockTimestamp, offsettedIndex } = require('../helpers.js'); +const { expect } = require('chai'); +const { constants } = require('@openzeppelin/test-helpers'); +const { ZERO_ADDRESS } = constants; + +const RECEIVER_MAGIC_VALUE = '0x150b7a02'; + +const createTestSuite = ({ contract, constructorArgs }) => + function () { + let offsetted; + + context(`${contract}`, function () { + beforeEach(async function () { + this.erc721aBatchTransferable = await deployContract(contract, constructorArgs); + this.receiver = await deployContract('ERC721ReceiverMock', [ + RECEIVER_MAGIC_VALUE, + this.erc721aBatchTransferable.address, + ]); + this.startTokenId = this.erc721aBatchTransferable.startTokenId + ? (await this.erc721aBatchTransferable.startTokenId()).toNumber() + : 0; + + offsetted = (...arr) => offsettedIndex(this.startTokenId, arr); + offsetted(0); + }); + + beforeEach(async function () { + const [owner, addr1, addr2, addr3, addr4, addr5] = await ethers.getSigners(); + this.owner = owner; + this.addr1 = addr1; + this.addr2 = addr2; + this.addr3 = addr3; + this.addr4 = addr4; + this.addr5 = addr5; + + this.addr1.expected = { + mintCount: 3, + tokens: offsetted(2, 4, 5), + }; + + this.addr2.expected = { + mintCount: 20, + tokens: offsetted(0, 1, 3, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22), + }; + + this.addr3.expected = { + mintCount: 7, + tokens: offsetted(23, 24, 25, 26, 27, 28, 29), + }; + + this.numTotalTokens = + this.addr1.expected.mintCount + this.addr2.expected.mintCount + this.addr3.expected.mintCount; + + await this.erc721aBatchTransferable['safeMint(address,uint256)'](this.addr2.address, 2); + await this.erc721aBatchTransferable['safeMint(address,uint256)'](this.addr1.address, 1); + await this.erc721aBatchTransferable['safeMint(address,uint256)'](this.addr2.address, 1); + await this.erc721aBatchTransferable['safeMint(address,uint256)'](this.addr1.address, 2); + await this.erc721aBatchTransferable['safeMint(address,uint256)'](this.addr2.address, 17); + await this.erc721aBatchTransferable['safeMint(address,uint256)'](this.addr3.address, 7); + }); + + context('test batch transfer functionality', function () { + const testSuccessfulBatchTransfer = function (transferFn, transferToContract = true) { + describe('successful transfers', async function () { + beforeEach(async function () { + const sender = this.addr2; + this.tokenIds = this.addr2.expected.tokens; + this.from = sender.address; + this.to = transferToContract ? this.receiver : this.addr4; + this.approvedIds = [this.tokenIds[2], this.tokenIds[3]]; + this.initializedToken = 8; + this.uninitializedToken = 10; + + this.approvedIds.forEach(async (tokenId) => { + await this.erc721aBatchTransferable.connect(sender).approve(this.to.address, tokenId); + }); + + // Manually initialize `this.initializedToken` + await this.erc721aBatchTransferable.initializeOwnershipAt(this.initializedToken); + + const ownershipBefore = await this.erc721aBatchTransferable.getOwnershipAt(3); + this.timestampBefore = parseInt(ownershipBefore.startTimestamp); + this.timestampToMine = (await getBlockTimestamp()) + 12345; + await mineBlockTimestamp(this.timestampToMine); + this.timestampMined = await getBlockTimestamp(); + + // prettier-ignore + this.transferTx = await this.erc721aBatchTransferable + .connect(sender)[transferFn](this.from, this.to.address, this.tokenIds); + + const ownershipAfter = await this.erc721aBatchTransferable.getOwnershipAt(3); + this.timestampAfter = parseInt(ownershipAfter.startTimestamp); + + // Transfer part of uninitialized tokens + this.tokensToTransferAlt = [25, 26, 27]; + // prettier-ignore + this.transferTxAlt = await this.erc721aBatchTransferable.connect(this.addr3)[transferFn]( + this.addr3.address, this.addr5.address, this.tokensToTransferAlt + ); + }); + + it('emits Transfers event', async function () { + for (let i = 0; i < this.tokenIds.length; i++) { + const tokenId = this.tokenIds[i]; + await expect(this.transferTx) + .to.emit(this.erc721aBatchTransferable, 'Transfer') + .withArgs(this.from, this.to.address, tokenId); + } + }); + + it('adjusts owners balances', async function () { + expect(await this.erc721aBatchTransferable.balanceOf(this.from)).to.be.equal(0); + expect(await this.erc721aBatchTransferable.balanceOf(this.to.address)).to.be.equal( + this.addr2.expected.mintCount + ); + expect(await this.erc721aBatchTransferable.balanceOf(this.addr3.address)).to.be.equal( + this.addr3.expected.tokens.length - this.tokensToTransferAlt.length + ); + expect(await this.erc721aBatchTransferable.balanceOf(this.addr5.address)).to.be.equal( + this.tokensToTransferAlt.length + ); + }); + + it('clears the approval for the token IDs', async function () { + this.approvedIds.forEach(async (tokenId) => { + expect(await this.erc721aBatchTransferable.getApproved(tokenId)).to.be.equal(ZERO_ADDRESS); + }); + }); + + it('startTimestamp updated correctly', async function () { + expect(this.timestampBefore).to.be.lt(this.timestampToMine); + expect(this.timestampAfter).to.be.gte(this.timestampToMine); + expect(this.timestampAfter).to.be.lt(this.timestampToMine + 10); + expect(this.timestampToMine).to.be.eq(this.timestampMined); + }); + + it('with transfer of the given token IDs to the given address', async function () { + for (let i = 0; i < this.tokenIds.length; i++) { + const tokenId = this.tokenIds[i]; + expect(await this.erc721aBatchTransferable.ownerOf(tokenId)).to.be.equal(this.to.address); + } + + // Initialized tokens were updated + expect((await this.erc721aBatchTransferable.getOwnershipAt(3))[0]).to.be.equal(this.to.address); + + // // Initialized tokens in a consecutive transfer are cleared + // expect((await this.erc721aBatchTransferable.getOwnershipAt(8))[0]).to.be.equal( + // transferFn !== 'batchTransferFromUnoptimized' ? ZERO_ADDRESS : this.to.address + // ); + + // Uninitialized tokens are left uninitialized + expect((await this.erc721aBatchTransferable.getOwnershipAt(7))[0]).to.be.equal( + transferFn !== 'batchTransferFromUnoptimized' ? ZERO_ADDRESS : this.to.address + ); + + // Other tokens in between are left unchanged + for (let i = 0; i < this.addr1.expected.tokens.length; i++) { + const tokenId = this.addr1.expected.tokens[i]; + expect(await this.erc721aBatchTransferable.ownerOf(tokenId)).to.be.equal(this.addr1.address); + } + }); + + it('with transfers of uninitialized token IDs to the given address', async function () { + const allTokensInitiallyOwned = this.addr3.expected.tokens; + allTokensInitiallyOwned.splice(2, this.tokensToTransferAlt.length); + + for (let i = 0; i < this.tokensToTransferAlt.length; i++) { + const tokenId = this.tokensToTransferAlt[i]; + expect(await this.erc721aBatchTransferable.ownerOf(tokenId)).to.be.equal(this.addr5.address); + } + + for (let i = 0; i < allTokensInitiallyOwned.length; i++) { + const tokenId = allTokensInitiallyOwned[i]; + expect(await this.erc721aBatchTransferable.ownerOf(tokenId)).to.be.equal(this.addr3.address); + } + + // Ownership of tokens was updated + expect((await this.erc721aBatchTransferable.getOwnershipAt(this.tokensToTransferAlt[0]))[0]).to.be.equal( + this.addr5.address + ); + expect((await this.erc721aBatchTransferable.getOwnershipAt(allTokensInitiallyOwned[2]))[0]).to.be.equal( + this.addr3.address + ); + + // Uninitialized tokens are left uninitialized + expect( + (await this.erc721aBatchTransferable.getOwnershipAt(this.tokensToTransferAlt[0] - 1))[0] + ).to.be.equal(ZERO_ADDRESS); + expect((await this.erc721aBatchTransferable.getOwnershipAt(allTokensInitiallyOwned[3]))[0]).to.be.equal( + ZERO_ADDRESS + ); + expect((await this.erc721aBatchTransferable.getOwnershipAt(this.tokensToTransferAlt[1]))[0]).to.be.equal( + transferFn !== 'batchTransferFromUnoptimized' ? ZERO_ADDRESS : this.addr5.address + ); + expect((await this.erc721aBatchTransferable.getOwnershipAt(this.tokensToTransferAlt[2]))[0]).to.be.equal( + transferFn !== 'batchTransferFromUnoptimized' ? ZERO_ADDRESS : this.addr5.address + ); + }); + }); + + describe('ownership correctly set', async function () { + beforeEach(async function () { + const sender = this.addr2; + this.from = sender.address; + this.to = transferToContract ? this.receiver : this.addr4; + this.initializedToken = 8; + this.uninitializedToken = 10; + + // Manually initialize some tokens of addr2 + await this.erc721aBatchTransferable.initializeOwnershipAt(this.initializedToken); + }); + + it('with tokens transferred and cleared', async function () { + const initializedToken = 15; + + expect((await this.erc721aBatchTransferable.getOwnershipAt(initializedToken - 1))[0]).to.be.equal( + ZERO_ADDRESS + ); + expect((await this.erc721aBatchTransferable.getOwnershipAt(initializedToken))[0]).to.be.equal( + ZERO_ADDRESS + ); + + // Initialize token + await this.erc721aBatchTransferable.initializeOwnershipAt(initializedToken); + expect((await this.erc721aBatchTransferable.getOwnershipAt(initializedToken - 1))[0]).to.be.equal( + ZERO_ADDRESS + ); + expect((await this.erc721aBatchTransferable.getOwnershipAt(initializedToken))[0]).to.be.equal( + this.addr2.address + ); + + // Transfer tokens + // prettier-ignore + await this.erc721aBatchTransferable + .connect(this.addr2)[transferFn]( + this.from, this.to.address, [initializedToken - 1, initializedToken] + ); + expect((await this.erc721aBatchTransferable.getOwnershipAt(initializedToken - 1))[0]).to.be.equal( + this.to.address + ); + + // Initialized tokens in a consecutive transfer are cleared + // expect((await this.erc721aBatchTransferable.getOwnershipAt(initializedToken))[0]).to.be.equal( + // transferFn !== 'batchTransferFromUnoptimized' ? ZERO_ADDRESS : this.to.address + // ); + }); + + it('with tokens transferred and updated', async function () { + const initializedToken = 15; + const extraData = 123; + + expect((await this.erc721aBatchTransferable.getOwnershipAt(initializedToken - 1))[0]).to.be.equal( + ZERO_ADDRESS + ); + expect((await this.erc721aBatchTransferable.getOwnershipAt(initializedToken))[0]).to.be.equal( + ZERO_ADDRESS + ); + + // Initialize token + await this.erc721aBatchTransferable.initializeOwnershipAt(initializedToken); + expect((await this.erc721aBatchTransferable.getOwnershipAt(initializedToken - 1))[0]).to.be.equal( + ZERO_ADDRESS + ); + expect((await this.erc721aBatchTransferable.getOwnershipAt(initializedToken))[0]).to.be.equal( + this.addr2.address + ); + + // Set extra data + await this.erc721aBatchTransferable.setExtraDataAt(initializedToken, extraData); + expect((await this.erc721aBatchTransferable.getOwnershipAt(initializedToken))[3]).to.be.equal(extraData); + + // Transfer tokens + // prettier-ignore + await this.erc721aBatchTransferable + .connect(this.addr2)[transferFn]( + this.from, this.to.address, [initializedToken - 1, initializedToken] + ); + expect((await this.erc721aBatchTransferable.getOwnershipAt(initializedToken - 1))[0]).to.be.equal( + this.to.address + ); + + // Initialized tokens in a consecutive transfer are updated when nextData is not 0 + expect((await this.erc721aBatchTransferable.getOwnershipAt(initializedToken))[0]).to.be.equal( + this.to.address + ); + expect((await this.erc721aBatchTransferable.getOwnershipAt(initializedToken))[3]).to.be.equal(extraData); + }); + + it('with first token transferred', async function () { + expect(await this.erc721aBatchTransferable.ownerOf(0)).to.be.equal(this.from); + expect(await this.erc721aBatchTransferable.ownerOf(1)).to.be.equal(this.from); + expect((await this.erc721aBatchTransferable.getOwnershipAt(0))[0]).to.be.equal(this.from); + expect((await this.erc721aBatchTransferable.getOwnershipAt(1))[0]).to.be.equal(ZERO_ADDRESS); + + // prettier-ignore + await this.erc721aBatchTransferable + .connect(this.addr2)[transferFn](this.from, this.to.address, [0]); + + expect(await this.erc721aBatchTransferable.ownerOf(0)).to.be.equal(this.to.address); + expect(await this.erc721aBatchTransferable.ownerOf(1)).to.be.equal(this.from); + expect((await this.erc721aBatchTransferable.getOwnershipAt(0))[0]).to.be.equal(this.to.address); + expect((await this.erc721aBatchTransferable.getOwnershipAt(1))[0]).to.be.equal(this.from); + }); + + it('with last token transferred', async function () { + await expect(this.erc721aBatchTransferable.ownerOf(this.numTotalTokens)).to.be.revertedWith( + 'OwnerQueryForNonexistentToken' + ); + + // prettier-ignore + await this.erc721aBatchTransferable + .connect(this.addr3)[transferFn]( + this.addr3.address, this.to.address, [offsetted(this.numTotalTokens - 1 + )]); + + expect(await this.erc721aBatchTransferable.ownerOf(offsetted(this.numTotalTokens - 1))).to.be.equal( + this.to.address + ); + await expect(this.erc721aBatchTransferable.ownerOf(this.numTotalTokens)).to.be.revertedWith( + 'OwnerQueryForNonexistentToken' + ); + }); + + it('with initialized token transferred', async function () { + expect(await this.erc721aBatchTransferable.ownerOf(this.initializedToken)).to.be.equal(this.from); + expect(await this.erc721aBatchTransferable.ownerOf(this.initializedToken + 1)).to.be.equal(this.from); + expect((await this.erc721aBatchTransferable.getOwnershipAt(this.initializedToken))[0]).to.be.equal( + this.from + ); + expect((await this.erc721aBatchTransferable.getOwnershipAt(this.initializedToken + 1))[0]).to.be.equal( + ZERO_ADDRESS + ); + + // prettier-ignore + await this.erc721aBatchTransferable + .connect(this.addr2)[transferFn](this.from, this.to.address, [this.initializedToken]); + + expect(await this.erc721aBatchTransferable.ownerOf(this.initializedToken)).to.be.equal(this.to.address); + expect(await this.erc721aBatchTransferable.ownerOf(this.initializedToken + 1)).to.be.equal(this.from); + expect((await this.erc721aBatchTransferable.getOwnershipAt(this.initializedToken))[0]).to.be.equal( + this.to.address + ); + expect((await this.erc721aBatchTransferable.getOwnershipAt(this.initializedToken + 1))[0]).to.be.equal( + this.from + ); + }); + + it('with uninitialized token transferred', async function () { + expect(await this.erc721aBatchTransferable.ownerOf(this.uninitializedToken)).to.be.equal(this.from); + expect(await this.erc721aBatchTransferable.ownerOf(this.uninitializedToken + 1)).to.be.equal(this.from); + expect((await this.erc721aBatchTransferable.getOwnershipAt(this.uninitializedToken))[0]).to.be.equal( + ZERO_ADDRESS + ); + expect((await this.erc721aBatchTransferable.getOwnershipAt(this.uninitializedToken + 1))[0]).to.be.equal( + ZERO_ADDRESS + ); + + // prettier-ignore + await this.erc721aBatchTransferable + .connect(this.addr2)[transferFn](this.from, this.to.address, [this.uninitializedToken]); + + expect(await this.erc721aBatchTransferable.ownerOf(this.uninitializedToken)).to.be.equal(this.to.address); + expect(await this.erc721aBatchTransferable.ownerOf(this.uninitializedToken + 1)).to.be.equal(this.from); + expect((await this.erc721aBatchTransferable.getOwnershipAt(this.uninitializedToken))[0]).to.be.equal( + this.to.address + ); + expect((await this.erc721aBatchTransferable.getOwnershipAt(this.uninitializedToken + 1))[0]).to.be.equal( + this.from + ); + }); + }); + }; + + const testUnsuccessfulBatchTransfer = function (transferFn) { + describe('unsuccessful transfers', function () { + beforeEach(function () { + this.tokenIds = this.addr2.expected.tokens.slice(0, 2); + this.sender = this.addr1; + }); + + it('rejects unapproved transfer', async function () { + // prettier-ignore + await expect( + this.erc721aBatchTransferable + .connect(this.sender)[transferFn]( + this.addr2.address, this.sender.address, this.tokenIds + ) + ).to.be.revertedWith('TransferCallerNotOwnerNorApproved'); + }); + + it('rejects transfer from incorrect owner', async function () { + await this.erc721aBatchTransferable.connect(this.addr2).setApprovalForAll(this.sender.address, true); + // prettier-ignore + await expect( + this.erc721aBatchTransferable + .connect(this.sender)[transferFn]( + this.addr3.address, this.sender.address, this.tokenIds + ) + ).to.be.revertedWith('TransferFromIncorrectOwner'); + }); + + it('rejects transfer to zero address', async function () { + await this.erc721aBatchTransferable.connect(this.addr2).setApprovalForAll(this.sender.address, true); + // prettier-ignore + await expect( + this.erc721aBatchTransferable + .connect(this.sender)[transferFn]( + this.addr2.address, ZERO_ADDRESS, this.tokenIds + ) + ).to.be.revertedWith('TransferToZeroAddress'); + }); + }); + }; + + const testApproveBatchTransfer = function (transferFn) { + describe('approvals correctly set', async function () { + beforeEach(function () { + this.tokenIds = this.addr1.expected.tokens.slice(0, 2); + }); + + it('approval allows batch transfers', async function () { + // prettier-ignore + await expect( + this.erc721aBatchTransferable + .connect(this.addr3)[transferFn]( + this.addr1.address, this.addr3.address, this.tokenIds + ) + ).to.be.revertedWith('TransferCallerNotOwnerNorApproved'); + + for (let i = 0; i < this.tokenIds.length; i++) { + const tokenId = this.tokenIds[i]; + await this.erc721aBatchTransferable.connect(this.addr1).approve(this.addr3.address, tokenId); + } + + // prettier-ignore + await this.erc721aBatchTransferable + .connect(this.addr3)[transferFn]( + this.addr1.address, this.addr3.address, this.tokenIds + ); + // prettier-ignore + await expect( + this.erc721aBatchTransferable + .connect(this.addr1)[transferFn]( + this.addr3.address, this.addr1.address, this.tokenIds + ) + ).to.be.revertedWith('TransferCallerNotOwnerNorApproved'); + }); + + it('self-approval is cleared on batch transfers', async function () { + for (let i = 0; i < this.tokenIds.length; i++) { + const tokenId = this.tokenIds[i]; + await this.erc721aBatchTransferable.connect(this.addr1).approve(this.addr1.address, tokenId); + expect(await this.erc721aBatchTransferable.getApproved(tokenId)).to.equal(this.addr1.address); + } + + // prettier-ignore + await this.erc721aBatchTransferable + .connect(this.addr1)[transferFn]( + this.addr1.address, this.addr2.address, this.tokenIds + ); + for (let i = 0; i < this.tokenIds.length; i++) { + const tokenId = this.tokenIds[i]; + expect(await this.erc721aBatchTransferable.getApproved(tokenId)).to.not.equal(this.addr1.address); + } + }); + + it('approval for all allows batch transfers', async function () { + await this.erc721aBatchTransferable.connect(this.addr1).setApprovalForAll(this.addr3.address, true); + + // prettier-ignore + await this.erc721aBatchTransferable + .connect(this.addr3)[transferFn]( + this.addr1.address, this.addr3.address, this.tokenIds + ); + }); + }); + }; + + context('successful transfers', function () { + context('batchTransferFrom', function (fn = 'batchTransferFrom') { + describe('to contract', function () { + testSuccessfulBatchTransfer(fn); + testUnsuccessfulBatchTransfer(fn); + testApproveBatchTransfer(fn); + }); + + describe('to EOA', function () { + testSuccessfulBatchTransfer(fn, false); + testUnsuccessfulBatchTransfer(fn, false); + testApproveBatchTransfer(fn, false); + }); + }); + context('safeBatchTransferFrom', function (fn = 'safeBatchTransferFrom(address,address,uint256[])') { + describe('to contract', function () { + testSuccessfulBatchTransfer(fn); + testUnsuccessfulBatchTransfer(fn); + testApproveBatchTransfer(fn); + }); + + describe('to EOA', function () { + testSuccessfulBatchTransfer(fn, false); + testUnsuccessfulBatchTransfer(fn, false); + testApproveBatchTransfer(fn, false); + }); + }); + + // Use to compare gas usage and verify expected behaviour with respect to normal transfers + context('batchTransferFromUnoptimized', function (fn = 'batchTransferFromUnoptimized') { + describe('to contract', function () { + testSuccessfulBatchTransfer(fn); + testUnsuccessfulBatchTransfer(fn); + testApproveBatchTransfer(fn); + }); + + describe('to EOA', function () { + testSuccessfulBatchTransfer(fn, false); + testUnsuccessfulBatchTransfer(fn, false); + testApproveBatchTransfer(fn, false); + }); + }); + }); + }); + }); + }; + +describe( + 'ERC721ABatchTransferable', + createTestSuite({ contract: 'ERC721ABatchTransferableMock', constructorArgs: ['Azuki', 'AZUKI'] }) +); From 7b3d048a5b352919f2b93d6700e6e87c92df0f7b Mon Sep 17 00:00:00 2001 From: Vectorized Date: Wed, 21 Aug 2024 01:50:29 +0000 Subject: [PATCH 02/13] Tidy, optimize --- contracts/ERC721A.sol | 310 +++++++++--------- .../extensions/ERC721ABatchTransferable.sol | 6 +- contracts/mocks/ERC721AGasReporterMock.sol | 26 ++ package-lock.json | 2 +- test/GasUsage.test.js | 18 + 5 files changed, 194 insertions(+), 168 deletions(-) diff --git a/contracts/ERC721A.sol b/contracts/ERC721A.sol index f4341b9e7..8e001b714 100644 --- a/contracts/ERC721A.sol +++ b/contracts/ERC721A.sol @@ -378,7 +378,7 @@ contract ERC721A is IERC721A { * @dev Initializes the ownership slot minted at `index` for efficiency purposes. */ function _initializeOwnershipAt(uint256 index) internal virtual { - if (_packedOwnerships[index] == 0) { + if (_packedOwnerships[index] == uint256(0)) { _packedOwnerships[index] = _packedOwnershipOf(index); } } @@ -396,7 +396,7 @@ contract ERC721A is IERC721A { } // If the data at the starting slot does not exist, start the scan. - if (packed == 0) { + if (packed == uint256(0)) { if (tokenId >= _currentIndex) _revert(OwnerQueryForNonexistentToken.selector); // Invariant: // There will always be an initialized ownership slot @@ -411,8 +411,8 @@ contract ERC721A is IERC721A { unchecked { packed = _packedOwnerships[--tokenId]; } - if (packed == 0) continue; - if (packed & _BITMASK_BURNED == 0) return packed; + if (packed == uint256(0)) continue; + if (packed & _BITMASK_BURNED == uint256(0)) return packed; // Otherwise, the token is burned, and we must revert. // This handles the case of batch burned tokens, where only the burned bit // of the starting slot is set, and remaining slots are left uninitialized. @@ -423,7 +423,7 @@ contract ERC721A is IERC721A { // This is possible because we have already achieved the target condition. // This saves 2143 gas on transfers of initialized tokens. // If the token is not burned, return `packed`. Otherwise, revert. - if (packed & _BITMASK_BURNED == 0) return packed; + if (packed & _BITMASK_BURNED == uint256(0)) return packed; } _revert(OwnerQueryForNonexistentToken.selector); } @@ -527,8 +527,8 @@ contract ERC721A is IERC721A { if (tokenId < _currentIndex) { uint256 packed; - while ((packed = _packedOwnerships[tokenId]) == 0) --tokenId; - result = packed & _BITMASK_BURNED == 0; + while ((packed = _packedOwnerships[tokenId]) == uint256(0)) --tokenId; + result = packed & _BITMASK_BURNED == uint256(0); } } } @@ -548,33 +548,28 @@ contract ERC721A is IERC721A { * @dev Returns whether `msgSender` is equal to `approvedAddress` or `owner`. */ function _isSenderApprovedOrOwner( - address approvedAddress, - address owner, - address msgSender + uint256 approvedAddressValue, + uint256 ownerMasked, + uint256 msgSenderMasked ) private pure returns (bool result) { assembly { - // Mask `owner` to the lower 160 bits, in case the upper bits somehow aren't clean. - owner := and(owner, _BITMASK_ADDRESS) - // Mask `msgSender` to the lower 160 bits, in case the upper bits somehow aren't clean. - msgSender := and(msgSender, _BITMASK_ADDRESS) - // `msgSender == owner || msgSender == approvedAddress`. - result := or(eq(msgSender, owner), eq(msgSender, approvedAddress)) + result := or(eq(msgSenderMasked, ownerMasked), eq(msgSenderMasked, approvedAddressValue)) } } /** - * @dev Returns the storage slot and value for the approved address of `tokenId`. + * @dev Returns the storage slot and value for the approved address of `tokenId` casted to a uint256. */ - function _getApprovedSlotAndAddress(uint256 tokenId) + function _getApprovedSlotAndValue(uint256 tokenId) private view - returns (uint256 approvedAddressSlot, address approvedAddress) + returns (uint256 approvedAddressSlot, uint256 approvedAddressValue) { TokenApprovalRef storage tokenApproval = _tokenApprovals[tokenId]; - // The following is equivalent to `approvedAddress = _tokenApprovals[tokenId].value`. + // The following is equivalent to `approvedAddressValue = uint160(_tokenApprovals[tokenId].value)`. assembly { approvedAddressSlot := tokenApproval.slot - approvedAddress := sload(approvedAddressSlot) + approvedAddressValue := sload(approvedAddressSlot) } } @@ -601,25 +596,21 @@ contract ERC721A is IERC721A { uint256 tokenId ) public payable virtual override { uint256 prevOwnershipPacked = _packedOwnershipOf(tokenId); + uint256 fromMasked = uint160(from); - // Mask `from` to the lower 160 bits, in case the upper bits somehow aren't clean. - from = address(uint160(uint256(uint160(from)) & _BITMASK_ADDRESS)); + if (uint160(prevOwnershipPacked) != fromMasked) _revert(TransferFromIncorrectOwner.selector); - if (address(uint160(prevOwnershipPacked)) != from) _revert(TransferFromIncorrectOwner.selector); - - (uint256 approvedAddressSlot, address approvedAddress) = _getApprovedSlotAndAddress(tokenId); + (uint256 approvedAddressSlot, uint256 approvedAddressValue) = _getApprovedSlotAndValue(tokenId); // The nested ifs save around 20+ gas over a compound boolean condition. - if (!_isSenderApprovedOrOwner(approvedAddress, from, _msgSenderERC721A())) + if (!_isSenderApprovedOrOwner(approvedAddressValue, fromMasked, uint160(_msgSenderERC721A()))) if (!isApprovedForAll(from, _msgSenderERC721A())) _revert(TransferCallerNotOwnerNorApproved.selector); _beforeTokenTransfers(from, to, tokenId, 1); - // Clear approvals from the previous owner. assembly { - if approvedAddress { - // This is equivalent to `delete _tokenApprovals[tokenId]`. - sstore(approvedAddressSlot, 0) + if approvedAddressValue { + sstore(approvedAddressSlot, 0) // Equivalent to `delete _tokenApprovals[tokenId]`. } } @@ -642,10 +633,10 @@ contract ERC721A is IERC721A { ); // If the next slot may not have been initialized (i.e. `nextInitialized == false`) . - if (prevOwnershipPacked & _BITMASK_NEXT_INITIALIZED == 0) { + if (prevOwnershipPacked & _BITMASK_NEXT_INITIALIZED == uint256(0)) { uint256 nextTokenId = tokenId + 1; // If the next slot's address is zero and not burned (i.e. packed value is zero). - if (_packedOwnerships[nextTokenId] == 0) { + if (_packedOwnerships[nextTokenId] == uint256(0)) { // If the next slot is within bounds. if (nextTokenId != _currentIndex) { // Initialize the next slot to maintain correctness for `ownerOf(tokenId + 1)`. @@ -655,20 +646,20 @@ contract ERC721A is IERC721A { } } - // Mask `to` to the lower 160 bits, in case the upper bits somehow aren't clean. - uint256 toMasked = uint256(uint160(to)) & _BITMASK_ADDRESS; + // Mask to the lower 160 bits, in case the upper bits somehow aren't clean. + uint256 toMasked = uint160(to); assembly { // Emit the `Transfer` event. log4( 0, // Start of data (0, since no data). 0, // End of data (0, since no data). _TRANSFER_EVENT_SIGNATURE, // Signature. - from, // `from`. + fromMasked, // `from`. toMasked, // `to`. tokenId // `tokenId`. ) } - if (toMasked == 0) _revert(TransferToZeroAddress.selector); + if (toMasked == uint256(0)) _revert(TransferToZeroAddress.selector); _afterTokenTransfers(from, to, tokenId, 1); } @@ -713,14 +704,14 @@ contract ERC721A is IERC721A { } /** - * @dev Equivalent to `_batchTransferFrom(from, to, tokenIds, false)`. + * @dev Equivalent to `_batchTransferFrom(from, to, tokenIds)`. */ function _batchTransferFrom( address from, address to, uint256[] memory tokenIds ) internal virtual { - _batchTransferFrom(from, to, tokenIds, false); + _batchTransferFrom(address(0), from, to, tokenIds); } /** @@ -732,141 +723,122 @@ contract ERC721A is IERC721A { * - `to` cannot be the zero address. * - `tokenIds` tokens must be owned by `from`. * - `tokenIds` must be strictly ascending. - * - If the caller is not `from`, it must be approved to move these tokens + * - If `by` is not `from`, it must be approved to move these tokens * by either {approve} or {setApprovalForAll}. * + * `by` is the address that to check token approval for. + * If token approval check is not needed, pass in `address(0)` for `by`. + * * Emits a {Transfer} event for each transfer. */ function _batchTransferFrom( + address by, address from, address to, - uint256[] memory tokenIds, - bool approvalCheck + uint256[] memory tokenIds ) internal virtual { - // We can use unchecked as the length of `tokenIds` is bounded - // to a small number by the max block gas limit. + uint256 fromMasked = uint160(from); + uint256 toMasked = uint160(to); + uint256 byMasked = uint160(by); + // Disallow transfer to zero address. + if (toMasked == uint256(0)) _revert(TransferToZeroAddress.selector); + // Early return if `tokenIds` is empty. + if (tokenIds.length == uint256(0)) return; + // Whether we need to check the individual token approvals. + bool approvalCheck = byMasked != uint256(0) && byMasked != fromMasked && !isApprovedForAll(from, by); + // The next `tokenId` to be minted (i.e. `_nextTokenId()`). + uint256 end = _currentIndex; + // Pointer to start and end (exclusive) of `tokenIds`. + (uint256 i, uint256 e) = _mdata(tokenIds); + // Ensure that `tokenIds` is strictly ascending, and perform the before hooks before any state changes. unchecked { - // Mask `from` and `to` to the lower 160 bits, in case the upper bits somehow aren't clean. - from = address(uint160(uint256(uint160(from)) & _BITMASK_ADDRESS)); - if (uint256(uint160(to)) & _BITMASK_ADDRESS == 0) revert TransferToZeroAddress(); - - // Disable `approvalCheck` if sender is either the owner or an approved operator for all tokens - approvalCheck = from != _msgSenderERC721A() && !isApprovedForAll(from, _msgSenderERC721A()); - + uint256 tokenId = _mload(i); // For checking if the `tokenIds` are strictly ascending. + // Revert if the minimum of the `tokenIds` is out of bounds. + // This is equivalent to `tokenId < _startTokenId() || end <= tokenId`. + if (end - _startTokenId() <= tokenId - _startTokenId()) _revert(OwnerQueryForNonexistentToken.selector); + _beforeTokenTransfers(from, to, tokenId, 1); // Perform the before hook. uint256 n = tokenIds.length; - + if (n >= 2) { + uint256 j = i + 0x20; + do { + uint256 next = _mload(j); + if (next <= tokenId) _revert(TokenIdsNotStrictlyAscending.selector); + _beforeTokenTransfers(from, to, tokenId = next, 1); // Perform the before hook. + } while ((j += 0x20) != e); + // Revert if the maximum of the `tokenIds` is out of bounds. + if (end <= tokenId) _revert(OwnerQueryForNonexistentToken.selector); + } // Increment and decrement the balances. _packedAddressData[from] -= n; _packedAddressData[to] += n; - - // The next `tokenId` to be minted (i.e. `_nextTokenId()`). - uint256 stop = _currentIndex; - - // For checking if the `tokenIds` are strictly ascending. - uint256 prevTokenId; - - uint256 tokenId; - uint256 currTokenId; - uint256 prevOwnershipPacked; - uint256 lastOwnershipPacked; - for (uint256 i; i != n; ) { - tokenId = tokenIds[i]; - - // Revert `tokenId` is out of bounds. - if (_or(tokenId < _startTokenId(), stop <= tokenId)) revert OwnerQueryForNonexistentToken(); - - // Revert if `tokenIds` is not strictly ascending. - if (i != 0) - if (tokenId <= prevTokenId) revert TokenIdsNotStrictlyAscending(); - + } + uint256 prevOwnershipPacked; + do { + uint256 tokenId = _mload(i); + unchecked { // Scan backwards for an initialized packed ownership slot. // ERC721A's invariant guarantees that there will always be an initialized slot as long as // the start of the backwards scan falls within `[_startTokenId() .. _nextTokenId())`. - for (uint256 j = tokenId; (prevOwnershipPacked = _packedOwnerships[j]) == 0; ) --j; - + for (uint256 j = tokenId; (prevOwnershipPacked = _packedOwnerships[j]) == uint256(0); ) --j; // If the initialized slot is burned, revert. - if (prevOwnershipPacked & _BITMASK_BURNED != 0) revert OwnerQueryForNonexistentToken(); - - // Check ownership of `tokenId`. - if (address(uint160(prevOwnershipPacked)) != from) revert TransferFromIncorrectOwner(); - - currTokenId = tokenId; - uint256 offset; - do { - address approvedAddress = _tokenApprovals[currTokenId].value; - - // Revert if the sender is not authorized to transfer the token. - if (approvalCheck) { - if (_msgSenderERC721A() != approvedAddress) revert TransferCallerNotOwnerNorApproved(); - } - - // Call the hook. - _beforeTokenTransfers(from, to, currTokenId, 1); - - if (approvedAddress != address(0)) delete _tokenApprovals[currTokenId]; - - // Emit the `Transfer` event. - emit Transfer(from, to, currTokenId); - // Call the hook. - _afterTokenTransfers(from, to, currTokenId, 1); - // Increment `offset` and update `currTokenId`. - currTokenId = tokenId + (++offset); - } while ( - // Neither out of bounds, nor at the end of `tokenIds`. - !_or(currTokenId == stop, i + offset == n) && - // Token ID is sequential. - tokenIds[i + offset] == currTokenId && - // The packed ownership slot is not initialized. - (lastOwnershipPacked = _packedOwnerships[currTokenId]) == 0 - ); - + if (prevOwnershipPacked & _BITMASK_BURNED != 0) _revert(OwnerQueryForNonexistentToken.selector); + // Check that `tokenId` is owned by `from`. + if (uint160(prevOwnershipPacked) != fromMasked) _revert(TransferFromIncorrectOwner.selector); // Updates tokenId: // - `address` to the next owner. - // - `startTimestamp` to the timestamp of transfering. + // - `startTimestamp` to the timestamp of transferring. // - `burned` to `false`. // - `nextInitialized` to `false`. _packedOwnerships[tokenId] = _packOwnershipData(to, _nextExtraData(from, to, prevOwnershipPacked)); - - // If the slot after the mini batch is neither out of bounds, nor initialized. - // If `lastOwnershipPacked == 0` we didn't break the loop due to an initialized slot. - if (currTokenId != stop) - if (lastOwnershipPacked == 0) - if (_packedOwnerships[currTokenId] == 0) _packedOwnerships[currTokenId] = prevOwnershipPacked; - - // Advance `i` by `offset`, the number of tokens transferred in the mini batch. - i += offset; - - // Set the `prevTokenId` for checking that the `tokenIds` is strictly ascending. - prevTokenId = currTokenId - 1; + do { + (uint256 approvedAddressSlot, uint256 approvedAddressValue) = _getApprovedSlotAndValue(tokenId); + // Revert if the sender is not authorized to transfer the token. + if (approvalCheck) + if (byMasked != approvedAddressValue) _revert(TransferCallerNotOwnerNorApproved.selector); + assembly { + if approvedAddressValue { + sstore(approvedAddressSlot, 0) // Equivalent to `delete _tokenApprovals[tokenId]`. + } + // Emit the `Transfer` event. + log4(0, 0, _TRANSFER_EVENT_SIGNATURE, fromMasked, toMasked, tokenId) + } + _afterTokenTransfers(from, to, tokenId, 1); // Perform the after hook. + if (_mload(i += 0x20) != ++tokenId) break; // Break if `tokenId` is not sequential. + if (i == e) break; // Break if at the end of `tokenIds`. + // Break if the packed ownership slot is initialized. + } while (_packedOwnerships[tokenId] == uint256(0)); } - } + // If the slot after the mini batch is neither out of bounds, nor initialized. + if (tokenId != end) + if (_packedOwnerships[tokenId] == uint256(0)) _packedOwnerships[tokenId] = prevOwnershipPacked; + } while (i != e); } /** - * @dev Equivalent to `_safeBatchTransferFrom(from, to, tokenIds, false)`. + * @dev Equivalent to `_safeBatchTransferFrom(from, to, tokenIds)`. */ function _safeBatchTransferFrom( address from, address to, uint256[] memory tokenIds ) internal virtual { - _safeBatchTransferFrom(from, to, tokenIds, false); + _safeBatchTransferFrom(address(0), from, to, tokenIds); } /** - * @dev Equivalent to `_safeBatchTransferFrom(from, to, tokenIds, '', approvalCheck)`. + * @dev Equivalent to `_safeBatchTransferFrom(by, from, to, tokenIds, '')`. */ function _safeBatchTransferFrom( + address by, address from, address to, - uint256[] memory tokenIds, - bool approvalCheck + uint256[] memory tokenIds ) internal virtual { - _safeBatchTransferFrom(from, to, tokenIds, '', approvalCheck); + _safeBatchTransferFrom(by, from, to, tokenIds, ''); } /** - * @dev Equivalent to `_safeBatchTransferFrom(from, to, tokenIds, _data, false)`. + * @dev Equivalent to `_safeBatchTransferFrom(address(0), from, to, tokenIds, _data)`. */ function _safeBatchTransferFrom( address from, @@ -874,7 +846,7 @@ contract ERC721A is IERC721A { uint256[] memory tokenIds, bytes memory _data ) internal virtual { - _safeBatchTransferFrom(from, to, tokenIds, _data, false); + _safeBatchTransferFrom(address(0), from, to, tokenIds, _data); } /** @@ -885,31 +857,32 @@ contract ERC721A is IERC721A { * - `from` cannot be the zero address. * - `to` cannot be the zero address. * - `tokenIds` tokens must be owned by `from`. - * - If the caller is not `from`, it must be approved to move these tokens + * - If `by` is not `from`, it must be approved to move these tokens * by either {approve} or {setApprovalForAll}. * - If `to` refers to a smart contract, it must implement * {IERC721Receiver-onERC721Received}, which is called for each transferred token. * + * `by` is the address that to check token approval for. + * If token approval check is not needed, pass in `address(0)` for `by`. + * * Emits a {Transfer} event for each transfer. */ function _safeBatchTransferFrom( + address by, address from, address to, uint256[] memory tokenIds, - bytes memory _data, - bool approvalCheck + bytes memory _data ) internal virtual { - _batchTransferFrom(from, to, tokenIds, approvalCheck); + _batchTransferFrom(by, from, to, tokenIds); - uint256 tokenId; - uint256 n = tokenIds.length; unchecked { - for (uint256 i; i < n; ++i) { - tokenId = tokenIds[i]; - if (to.code.length != 0) - if (!_checkContractOnERC721Received(from, to, tokenId, _data)) { + if (to.code.length != 0) { + for ((uint256 i, uint256 e) = _mdata(tokenIds); i != e; i += 0x20) { + if (!_checkContractOnERC721Received(from, to, _mload(i), _data)) { _revert(TransferToNonERC721ReceiverImplementer.selector); } + } } } } @@ -981,7 +954,7 @@ contract ERC721A is IERC721A { ) { return retval == ERC721A__IERC721Receiver(to).onERC721Received.selector; } catch (bytes memory reason) { - if (reason.length == 0) { + if (reason.length == uint256(0)) { _revert(TransferToNonERC721ReceiverImplementer.selector); } assembly { @@ -1006,7 +979,7 @@ contract ERC721A is IERC721A { */ function _mint(address to, uint256 quantity) internal virtual { uint256 startTokenId = _currentIndex; - if (quantity == 0) _revert(MintZeroQuantity.selector); + if (quantity == uint256(0)) _revert(MintZeroQuantity.selector); _beforeTokenTransfers(address(0), to, startTokenId, quantity); @@ -1031,10 +1004,10 @@ contract ERC721A is IERC721A { // We can directly add to the `balance` and `numberMinted`. _packedAddressData[to] += quantity * ((1 << _BITPOS_NUMBER_MINTED) | 1); - // Mask `to` to the lower 160 bits, in case the upper bits somehow aren't clean. - uint256 toMasked = uint256(uint160(to)) & _BITMASK_ADDRESS; + // Mask to the lower 160 bits, in case the upper bits somehow aren't clean. + uint256 toMasked = uint160(to); - if (toMasked == 0) _revert(MintToZeroAddress.selector); + if (toMasked == uint256(0)) _revert(MintToZeroAddress.selector); uint256 end = startTokenId + quantity; uint256 tokenId = startTokenId; @@ -1086,7 +1059,7 @@ contract ERC721A is IERC721A { function _mintERC2309(address to, uint256 quantity) internal virtual { uint256 startTokenId = _currentIndex; if (to == address(0)) _revert(MintToZeroAddress.selector); - if (quantity == 0) _revert(MintZeroQuantity.selector); + if (quantity == uint256(0)) _revert(MintZeroQuantity.selector); if (quantity > _MAX_MINT_ERC2309_QUANTITY_LIMIT) _revert(MintERC2309QuantityExceedsLimit.selector); _beforeTokenTransfers(address(0), to, startTokenId, quantity); @@ -1203,10 +1176,10 @@ contract ERC721A is IERC721A { // We can directly add to the `balance` and `numberMinted`. _packedAddressData[to] += (1 << _BITPOS_NUMBER_MINTED) | 1; - // Mask `to` to the lower 160 bits, in case the upper bits somehow aren't clean. - uint256 toMasked = uint256(uint160(to)) & _BITMASK_ADDRESS; + // Mask to the lower 160 bits, in case the upper bits somehow aren't clean. + uint256 toMasked = uint160(to); - if (toMasked == 0) _revert(MintToZeroAddress.selector); + if (toMasked == uint256(0)) _revert(MintToZeroAddress.selector); assembly { // Emit the `Transfer` event. @@ -1332,23 +1305,22 @@ contract ERC721A is IERC721A { function _burn(uint256 tokenId, bool approvalCheck) internal virtual { uint256 prevOwnershipPacked = _packedOwnershipOf(tokenId); - address from = address(uint160(prevOwnershipPacked)); + uint256 fromMasked = uint160(prevOwnershipPacked); + address from = address(uint160(fromMasked)); - (uint256 approvedAddressSlot, address approvedAddress) = _getApprovedSlotAndAddress(tokenId); + (uint256 approvedAddressSlot, uint256 approvedAddressValue) = _getApprovedSlotAndValue(tokenId); if (approvalCheck) { // The nested ifs save around 20+ gas over a compound boolean condition. - if (!_isSenderApprovedOrOwner(approvedAddress, from, _msgSenderERC721A())) + if (!_isSenderApprovedOrOwner(approvedAddressValue, fromMasked, uint160(_msgSenderERC721A()))) if (!isApprovedForAll(from, _msgSenderERC721A())) _revert(TransferCallerNotOwnerNorApproved.selector); } _beforeTokenTransfers(from, address(0), tokenId, 1); - // Clear approvals from the previous owner. assembly { - if approvedAddress { - // This is equivalent to `delete _tokenApprovals[tokenId]`. - sstore(approvedAddressSlot, 0) + if approvedAddressValue { + sstore(approvedAddressSlot, 0) // Equivalent to `delete _tokenApprovals[tokenId]`. } } @@ -1375,10 +1347,10 @@ contract ERC721A is IERC721A { ); // If the next slot may not have been initialized (i.e. `nextInitialized == false`) . - if (prevOwnershipPacked & _BITMASK_NEXT_INITIALIZED == 0) { + if (prevOwnershipPacked & _BITMASK_NEXT_INITIALIZED == uint256(0)) { uint256 nextTokenId = tokenId + 1; // If the next slot's address is zero and not burned (i.e. packed value is zero). - if (_packedOwnerships[nextTokenId] == 0) { + if (_packedOwnerships[nextTokenId] == uint256(0)) { // If the next slot is within bounds. if (nextTokenId != _currentIndex) { // Initialize the next slot to maintain correctness for `ownerOf(tokenId + 1)`. @@ -1406,7 +1378,7 @@ contract ERC721A is IERC721A { */ function _setExtraDataAt(uint256 index, uint24 extraData) internal virtual { uint256 packed = _packedOwnerships[index]; - if (packed == 0) _revert(OwnershipNotInitializedForExtraData.selector); + if (packed == uint256(0)) _revert(OwnershipNotInitializedForExtraData.selector); uint256 extraDataCasted; // Cast `extraData` with assembly to avoid redundant masking. assembly { @@ -1515,11 +1487,21 @@ contract ERC721A is IERC721A { } /** - * @dev Branchless or. + * @dev Returns a memory pointer to the start of `a`'s data. + */ + function _mdata(uint256[] memory a) private pure returns (uint256 start, uint256 end) { + assembly { + start := add(a, 0x20) + end := add(start, shl(5, mload(a))) + } + } + + /** + * @dev Returns the uint256 at `p` in memory. */ - function _or(bool a, bool b) private pure returns (bool c) { + function _mload(uint256 p) private pure returns (uint256 result) { assembly { - c := or(a, b) + result := mload(p) } } } diff --git a/contracts/extensions/ERC721ABatchTransferable.sol b/contracts/extensions/ERC721ABatchTransferable.sol index d4caa4a0a..b1d66227b 100644 --- a/contracts/extensions/ERC721ABatchTransferable.sol +++ b/contracts/extensions/ERC721ABatchTransferable.sol @@ -18,7 +18,7 @@ abstract contract ERC721ABatchTransferable is ERC721A, IERC721ABatchTransferable address to, uint256[] memory tokenIds ) public payable virtual override { - _batchTransferFrom(from, to, tokenIds, true); + _batchTransferFrom(_msgSenderERC721A(), from, to, tokenIds); } function safeBatchTransferFrom( @@ -26,7 +26,7 @@ abstract contract ERC721ABatchTransferable is ERC721A, IERC721ABatchTransferable address to, uint256[] memory tokenIds ) public payable virtual override { - _safeBatchTransferFrom(from, to, tokenIds, true); + _safeBatchTransferFrom(_msgSenderERC721A(), from, to, tokenIds, ''); } function safeBatchTransferFrom( @@ -35,6 +35,6 @@ abstract contract ERC721ABatchTransferable is ERC721A, IERC721ABatchTransferable uint256[] memory tokenIds, bytes memory _data ) public payable virtual override { - _safeBatchTransferFrom(from, to, tokenIds, _data, true); + _safeBatchTransferFrom(_msgSenderERC721A(), from, to, tokenIds, _data); } } diff --git a/contracts/mocks/ERC721AGasReporterMock.sol b/contracts/mocks/ERC721AGasReporterMock.sol index 847a7773d..a5eecac52 100644 --- a/contracts/mocks/ERC721AGasReporterMock.sol +++ b/contracts/mocks/ERC721AGasReporterMock.sol @@ -25,6 +25,14 @@ contract ERC721AGasReporterMock is ERC721A { _mint(to, 10); } + function safeMintHundred(address to) public { + _safeMint(to, 100); + } + + function mintHundred(address to) public { + _mint(to, 100); + } + function transferTenAsc(address to) public { unchecked { transferFrom(msg.sender, to, 0); @@ -69,4 +77,22 @@ contract ERC721AGasReporterMock is ERC721A { transferFrom(msg.sender, to, 9); } } + + function batchTransferHundredUnoptimized(address to) public { + unchecked { + for (uint256 i; i != 100; ++i) { + transferFrom(msg.sender, to, i); + } + } + } + + function batchTransferHundredOptimized(address to) public { + unchecked { + uint256[] memory tokenIds = new uint256[](100); + for (uint256 i; i != 100; ++i) { + tokenIds[i] = i; + } + _batchTransferFrom(msg.sender, msg.sender, to, tokenIds); + } + } } diff --git a/package-lock.json b/package-lock.json index 9ef9f6f98..bde6c1457 100644 --- a/package-lock.json +++ b/package-lock.json @@ -48430,4 +48430,4 @@ } } } -} \ No newline at end of file +} diff --git a/test/GasUsage.test.js b/test/GasUsage.test.js index 380ff9583..7b6e435c2 100644 --- a/test/GasUsage.test.js +++ b/test/GasUsage.test.js @@ -67,6 +67,24 @@ describe('ERC721A Gas Usage', function () { it('transferTen average order', async function () { await this.erc721a.connect(this.owner).transferTenAvg(this.addr1.address); }); + + it('transferTen average order', async function () { + await this.erc721a.connect(this.owner).transferTenAvg(this.addr1.address); + }); + }); + + context('batchTransferFromHundred', function () { + beforeEach(async function () { + await this.erc721a.mintHundred(this.owner.address); + }); + + it('batchTransferFromHundred unoptimized', async function () { + await this.erc721a.connect(this.owner).batchTransferHundredUnoptimized(this.addr1.address); + }); + + it('batchTransferFromHundred optimized', async function () { + await this.erc721a.connect(this.owner).batchTransferHundredOptimized(this.addr1.address); + }); }); it('mintOneERC2309', async function () { From 66350ea95a940ba087dd845ae3873ee354139a74 Mon Sep 17 00:00:00 2001 From: Vectorized Date: Wed, 21 Aug 2024 02:23:31 +0000 Subject: [PATCH 03/13] Tidy --- contracts/ERC721A.sol | 63 +++++++++++++++++++++++-------------------- 1 file changed, 34 insertions(+), 29 deletions(-) diff --git a/contracts/ERC721A.sol b/contracts/ERC721A.sol index 8e001b714..f27fe4cbc 100644 --- a/contracts/ERC721A.sol +++ b/contracts/ERC721A.sol @@ -742,26 +742,27 @@ contract ERC721A is IERC721A { uint256 byMasked = uint160(by); // Disallow transfer to zero address. if (toMasked == uint256(0)) _revert(TransferToZeroAddress.selector); - // Early return if `tokenIds` is empty. - if (tokenIds.length == uint256(0)) return; // Whether we need to check the individual token approvals. bool approvalCheck = byMasked != uint256(0) && byMasked != fromMasked && !isApprovedForAll(from, by); + uint256 n = tokenIds.length; + // Early return if `tokenIds` is empty. + if (n == uint256(0)) return; // The next `tokenId` to be minted (i.e. `_nextTokenId()`). uint256 end = _currentIndex; // Pointer to start and end (exclusive) of `tokenIds`. - (uint256 i, uint256 e) = _mdata(tokenIds); + (uint256 i, uint256 e) = _mdataERC721A(tokenIds); // Ensure that `tokenIds` is strictly ascending, and perform the before hooks before any state changes. unchecked { - uint256 tokenId = _mload(i); // For checking if the `tokenIds` are strictly ascending. + uint256 tokenId = _mloadERC721A(i); // For checking if the `tokenIds` are strictly ascending. // Revert if the minimum of the `tokenIds` is out of bounds. // This is equivalent to `tokenId < _startTokenId() || end <= tokenId`. if (end - _startTokenId() <= tokenId - _startTokenId()) _revert(OwnerQueryForNonexistentToken.selector); _beforeTokenTransfers(from, to, tokenId, 1); // Perform the before hook. - uint256 n = tokenIds.length; + if (n >= 2) { uint256 j = i + 0x20; do { - uint256 next = _mload(j); + uint256 next = _mloadERC721A(j); if (next <= tokenId) _revert(TokenIdsNotStrictlyAscending.selector); _beforeTokenTransfers(from, to, tokenId = next, 1); // Perform the before hook. } while ((j += 0x20) != e); @@ -774,7 +775,7 @@ contract ERC721A is IERC721A { } uint256 prevOwnershipPacked; do { - uint256 tokenId = _mload(i); + uint256 tokenId = _mloadERC721A(i); unchecked { // Scan backwards for an initialized packed ownership slot. // ERC721A's invariant guarantees that there will always be an initialized slot as long as @@ -803,7 +804,7 @@ contract ERC721A is IERC721A { log4(0, 0, _TRANSFER_EVENT_SIGNATURE, fromMasked, toMasked, tokenId) } _afterTokenTransfers(from, to, tokenId, 1); // Perform the after hook. - if (_mload(i += 0x20) != ++tokenId) break; // Break if `tokenId` is not sequential. + if (_mloadERC721A(i += 0x20) != ++tokenId) break; // Break if `tokenId` is not sequential. if (i == e) break; // Break if at the end of `tokenIds`. // Break if the packed ownership slot is initialized. } while (_packedOwnerships[tokenId] == uint256(0)); @@ -878,8 +879,8 @@ contract ERC721A is IERC721A { unchecked { if (to.code.length != 0) { - for ((uint256 i, uint256 e) = _mdata(tokenIds); i != e; i += 0x20) { - if (!_checkContractOnERC721Received(from, to, _mload(i), _data)) { + for ((uint256 i, uint256 e) = _mdataERC721A(tokenIds); i != e; i += 0x20) { + if (!_checkContractOnERC721Received(from, to, _mloadERC721A(i), _data)) { _revert(TransferToNonERC721ReceiverImplementer.selector); } } @@ -1421,6 +1422,29 @@ contract ERC721A is IERC721A { return uint256(_extraData(from, to, extraData)) << _BITPOS_EXTRA_DATA; } + // ============================================================= + // PRIVATE HELPERS + // ============================================================= + + /** + * @dev Returns a memory pointer to the start of `a`'s data. + */ + function _mdataERC721A(uint256[] memory a) private pure returns (uint256 start, uint256 end) { + assembly { + start := add(a, 0x20) + end := add(start, shl(5, mload(a))) + } + } + + /** + * @dev Returns the uint256 at `p` in memory. + */ + function _mloadERC721A(uint256 p) private pure returns (uint256 result) { + assembly { + result := mload(p) + } + } + // ============================================================= // OTHER OPERATIONS // ============================================================= @@ -1485,23 +1509,4 @@ contract ERC721A is IERC721A { revert(0x00, 0x04) } } - - /** - * @dev Returns a memory pointer to the start of `a`'s data. - */ - function _mdata(uint256[] memory a) private pure returns (uint256 start, uint256 end) { - assembly { - start := add(a, 0x20) - end := add(start, shl(5, mload(a))) - } - } - - /** - * @dev Returns the uint256 at `p` in memory. - */ - function _mload(uint256 p) private pure returns (uint256 result) { - assembly { - result := mload(p) - } - } } From d1e624cdd2c691f9df97330084a0d16c7d725a7a Mon Sep 17 00:00:00 2001 From: Vectorized Date: Wed, 21 Aug 2024 02:55:51 +0000 Subject: [PATCH 04/13] Add test --- .../mocks/ERC721ABatchTransferableMock.sol | 9 +++ .../ERC721ABatchTransferable.test.js | 58 ++++++++----------- 2 files changed, 32 insertions(+), 35 deletions(-) diff --git a/contracts/mocks/ERC721ABatchTransferableMock.sol b/contracts/mocks/ERC721ABatchTransferableMock.sol index d0aaea814..764ccfb82 100644 --- a/contracts/mocks/ERC721ABatchTransferableMock.sol +++ b/contracts/mocks/ERC721ABatchTransferableMock.sol @@ -66,4 +66,13 @@ contract ERC721ABatchTransferableMock is ERC721ABatchTransferable { } } } + + function directBatchTransferFrom( + address by, + address from, + address to, + uint256[] memory tokenIds + ) public { + _batchTransferFrom(by, from, to, tokenIds); + } } diff --git a/test/extensions/ERC721ABatchTransferable.test.js b/test/extensions/ERC721ABatchTransferable.test.js index 20382f7f2..ca9548221 100644 --- a/test/extensions/ERC721ABatchTransferable.test.js +++ b/test/extensions/ERC721ABatchTransferable.test.js @@ -84,7 +84,6 @@ const createTestSuite = ({ contract, constructorArgs }) => await mineBlockTimestamp(this.timestampToMine); this.timestampMined = await getBlockTimestamp(); - // prettier-ignore this.transferTx = await this.erc721aBatchTransferable .connect(sender)[transferFn](this.from, this.to.address, this.tokenIds); @@ -93,7 +92,6 @@ const createTestSuite = ({ contract, constructorArgs }) => // Transfer part of uninitialized tokens this.tokensToTransferAlt = [25, 26, 27]; - // prettier-ignore this.transferTxAlt = await this.erc721aBatchTransferable.connect(this.addr3)[transferFn]( this.addr3.address, this.addr5.address, this.tokensToTransferAlt ); @@ -143,11 +141,6 @@ const createTestSuite = ({ contract, constructorArgs }) => // Initialized tokens were updated expect((await this.erc721aBatchTransferable.getOwnershipAt(3))[0]).to.be.equal(this.to.address); - // // Initialized tokens in a consecutive transfer are cleared - // expect((await this.erc721aBatchTransferable.getOwnershipAt(8))[0]).to.be.equal( - // transferFn !== 'batchTransferFromUnoptimized' ? ZERO_ADDRESS : this.to.address - // ); - // Uninitialized tokens are left uninitialized expect((await this.erc721aBatchTransferable.getOwnershipAt(7))[0]).to.be.equal( transferFn !== 'batchTransferFromUnoptimized' ? ZERO_ADDRESS : this.to.address @@ -230,7 +223,6 @@ const createTestSuite = ({ contract, constructorArgs }) => ); // Transfer tokens - // prettier-ignore await this.erc721aBatchTransferable .connect(this.addr2)[transferFn]( this.from, this.to.address, [initializedToken - 1, initializedToken] @@ -238,11 +230,6 @@ const createTestSuite = ({ contract, constructorArgs }) => expect((await this.erc721aBatchTransferable.getOwnershipAt(initializedToken - 1))[0]).to.be.equal( this.to.address ); - - // Initialized tokens in a consecutive transfer are cleared - // expect((await this.erc721aBatchTransferable.getOwnershipAt(initializedToken))[0]).to.be.equal( - // transferFn !== 'batchTransferFromUnoptimized' ? ZERO_ADDRESS : this.to.address - // ); }); it('with tokens transferred and updated', async function () { @@ -270,7 +257,6 @@ const createTestSuite = ({ contract, constructorArgs }) => expect((await this.erc721aBatchTransferable.getOwnershipAt(initializedToken))[3]).to.be.equal(extraData); // Transfer tokens - // prettier-ignore await this.erc721aBatchTransferable .connect(this.addr2)[transferFn]( this.from, this.to.address, [initializedToken - 1, initializedToken] @@ -292,7 +278,6 @@ const createTestSuite = ({ contract, constructorArgs }) => expect((await this.erc721aBatchTransferable.getOwnershipAt(0))[0]).to.be.equal(this.from); expect((await this.erc721aBatchTransferable.getOwnershipAt(1))[0]).to.be.equal(ZERO_ADDRESS); - // prettier-ignore await this.erc721aBatchTransferable .connect(this.addr2)[transferFn](this.from, this.to.address, [0]); @@ -307,7 +292,6 @@ const createTestSuite = ({ contract, constructorArgs }) => 'OwnerQueryForNonexistentToken' ); - // prettier-ignore await this.erc721aBatchTransferable .connect(this.addr3)[transferFn]( this.addr3.address, this.to.address, [offsetted(this.numTotalTokens - 1 @@ -331,7 +315,6 @@ const createTestSuite = ({ contract, constructorArgs }) => ZERO_ADDRESS ); - // prettier-ignore await this.erc721aBatchTransferable .connect(this.addr2)[transferFn](this.from, this.to.address, [this.initializedToken]); @@ -355,7 +338,6 @@ const createTestSuite = ({ contract, constructorArgs }) => ZERO_ADDRESS ); - // prettier-ignore await this.erc721aBatchTransferable .connect(this.addr2)[transferFn](this.from, this.to.address, [this.uninitializedToken]); @@ -379,29 +361,40 @@ const createTestSuite = ({ contract, constructorArgs }) => }); it('rejects unapproved transfer', async function () { - // prettier-ignore await expect( - this.erc721aBatchTransferable - .connect(this.sender)[transferFn]( - this.addr2.address, this.sender.address, this.tokenIds - ) - ).to.be.revertedWith('TransferCallerNotOwnerNorApproved'); + this.erc721aBatchTransferable + .connect(this.sender)[transferFn]( + this.addr2.address, this.sender.address, this.tokenIds + ) + ).to.be.revertedWith('TransferCallerNotOwnerNorApproved'); }); it('rejects transfer from incorrect owner', async function () { await this.erc721aBatchTransferable.connect(this.addr2).setApprovalForAll(this.sender.address, true); - // prettier-ignore await expect( - this.erc721aBatchTransferable - .connect(this.sender)[transferFn]( - this.addr3.address, this.sender.address, this.tokenIds + this.erc721aBatchTransferable + .connect(this.sender)[transferFn]( + this.addr3.address, this.sender.address, this.tokenIds + ) + ).to.be.revertedWith('TransferFromIncorrectOwner'); + }); + + it('rejects transfer from zero address', async function () { + await this.erc721aBatchTransferable.connect(this.addr2).setApprovalForAll(this.sender.address, true); + await expect( + this.erc721aBatchTransferable + .connect(this.sender)['directBatchTransferFrom']( + ZERO_ADDRESS, ZERO_ADDRESS, this.sender.address, this.tokenIds ) - ).to.be.revertedWith('TransferFromIncorrectOwner'); + ).to.be.revertedWith('TransferFromIncorrectOwner'); + this.erc721aBatchTransferable + .connect(this.sender)['directBatchTransferFrom']( + ZERO_ADDRESS, this.addr2.address, this.sender.address, this.tokenIds + ); }); it('rejects transfer to zero address', async function () { await this.erc721aBatchTransferable.connect(this.addr2).setApprovalForAll(this.sender.address, true); - // prettier-ignore await expect( this.erc721aBatchTransferable .connect(this.sender)[transferFn]( @@ -419,7 +412,6 @@ const createTestSuite = ({ contract, constructorArgs }) => }); it('approval allows batch transfers', async function () { - // prettier-ignore await expect( this.erc721aBatchTransferable .connect(this.addr3)[transferFn]( @@ -432,12 +424,10 @@ const createTestSuite = ({ contract, constructorArgs }) => await this.erc721aBatchTransferable.connect(this.addr1).approve(this.addr3.address, tokenId); } - // prettier-ignore await this.erc721aBatchTransferable .connect(this.addr3)[transferFn]( this.addr1.address, this.addr3.address, this.tokenIds ); - // prettier-ignore await expect( this.erc721aBatchTransferable .connect(this.addr1)[transferFn]( @@ -453,7 +443,6 @@ const createTestSuite = ({ contract, constructorArgs }) => expect(await this.erc721aBatchTransferable.getApproved(tokenId)).to.equal(this.addr1.address); } - // prettier-ignore await this.erc721aBatchTransferable .connect(this.addr1)[transferFn]( this.addr1.address, this.addr2.address, this.tokenIds @@ -467,7 +456,6 @@ const createTestSuite = ({ contract, constructorArgs }) => it('approval for all allows batch transfers', async function () { await this.erc721aBatchTransferable.connect(this.addr1).setApprovalForAll(this.addr3.address, true); - // prettier-ignore await this.erc721aBatchTransferable .connect(this.addr3)[transferFn]( this.addr1.address, this.addr3.address, this.tokenIds From 1f4ff52556c8c3e42a37449500d7bf97dbcbd09f Mon Sep 17 00:00:00 2001 From: Vectorized Date: Wed, 21 Aug 2024 03:03:02 +0000 Subject: [PATCH 05/13] Tidy --- contracts/ERC721A.sol | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/contracts/ERC721A.sol b/contracts/ERC721A.sol index f27fe4cbc..5d6de3a66 100644 --- a/contracts/ERC721A.sol +++ b/contracts/ERC721A.sol @@ -743,7 +743,8 @@ contract ERC721A is IERC721A { // Disallow transfer to zero address. if (toMasked == uint256(0)) _revert(TransferToZeroAddress.selector); // Whether we need to check the individual token approvals. - bool approvalCheck = byMasked != uint256(0) && byMasked != fromMasked && !isApprovedForAll(from, by); + bool approvalCheck; + if (!_orERC721A(byMasked == uint256(0), byMasked == fromMasked)) approvalCheck = !isApprovedForAll(from, by); uint256 n = tokenIds.length; // Early return if `tokenIds` is empty. if (n == uint256(0)) return; @@ -755,10 +756,8 @@ contract ERC721A is IERC721A { unchecked { uint256 tokenId = _mloadERC721A(i); // For checking if the `tokenIds` are strictly ascending. // Revert if the minimum of the `tokenIds` is out of bounds. - // This is equivalent to `tokenId < _startTokenId() || end <= tokenId`. - if (end - _startTokenId() <= tokenId - _startTokenId()) _revert(OwnerQueryForNonexistentToken.selector); + if (_orERC721A(tokenId < _startTokenId(), end <= tokenId)) _revert(OwnerQueryForNonexistentToken.selector); _beforeTokenTransfers(from, to, tokenId, 1); // Perform the before hook. - if (n >= 2) { uint256 j = i + 0x20; do { @@ -789,7 +788,7 @@ contract ERC721A is IERC721A { // - `address` to the next owner. // - `startTimestamp` to the timestamp of transferring. // - `burned` to `false`. - // - `nextInitialized` to `false`. + // - `nextInitialized` to `false`, as it is optional. _packedOwnerships[tokenId] = _packOwnershipData(to, _nextExtraData(from, to, prevOwnershipPacked)); do { (uint256 approvedAddressSlot, uint256 approvedAddressValue) = _getApprovedSlotAndValue(tokenId); @@ -1445,6 +1444,15 @@ contract ERC721A is IERC721A { } } + /** + * @dev Branchless boolean or. + */ + function _orERC721A(bool a, bool b) private pure returns (bool result) { + assembly { + result := or(iszero(iszero(a)), iszero(iszero(b))) + } + } + // ============================================================= // OTHER OPERATIONS // ============================================================= From 64fcf3a04c70468d172a3e8fcf75436fcf3ed6d1 Mon Sep 17 00:00:00 2001 From: Vectorized Date: Wed, 21 Aug 2024 03:13:44 +0000 Subject: [PATCH 06/13] Tidy --- contracts/ERC721A.sol | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/contracts/ERC721A.sol b/contracts/ERC721A.sol index 5d6de3a66..0114e115d 100644 --- a/contracts/ERC721A.sol +++ b/contracts/ERC721A.sol @@ -737,14 +737,15 @@ contract ERC721A is IERC721A { address to, uint256[] memory tokenIds ) internal virtual { + uint256 byMasked = uint160(by); uint256 fromMasked = uint160(from); uint256 toMasked = uint160(to); - uint256 byMasked = uint160(by); // Disallow transfer to zero address. if (toMasked == uint256(0)) _revert(TransferToZeroAddress.selector); // Whether we need to check the individual token approvals. bool approvalCheck; if (!_orERC721A(byMasked == uint256(0), byMasked == fromMasked)) approvalCheck = !isApprovedForAll(from, by); + uint256 n = tokenIds.length; // Early return if `tokenIds` is empty. if (n == uint256(0)) return; @@ -754,16 +755,16 @@ contract ERC721A is IERC721A { (uint256 i, uint256 e) = _mdataERC721A(tokenIds); // Ensure that `tokenIds` is strictly ascending, and perform the before hooks before any state changes. unchecked { - uint256 tokenId = _mloadERC721A(i); // For checking if the `tokenIds` are strictly ascending. + uint256 tokenId = _mloadERC721A(i); // Revert if the minimum of the `tokenIds` is out of bounds. if (_orERC721A(tokenId < _startTokenId(), end <= tokenId)) _revert(OwnerQueryForNonexistentToken.selector); - _beforeTokenTransfers(from, to, tokenId, 1); // Perform the before hook. + _beforeTokenTransfers(from, to, tokenId, 1); if (n >= 2) { uint256 j = i + 0x20; do { uint256 next = _mloadERC721A(j); if (next <= tokenId) _revert(TokenIdsNotStrictlyAscending.selector); - _beforeTokenTransfers(from, to, tokenId = next, 1); // Perform the before hook. + _beforeTokenTransfers(from, to, tokenId = next, 1); } while ((j += 0x20) != e); // Revert if the maximum of the `tokenIds` is out of bounds. if (end <= tokenId) _revert(OwnerQueryForNonexistentToken.selector); @@ -802,13 +803,12 @@ contract ERC721A is IERC721A { // Emit the `Transfer` event. log4(0, 0, _TRANSFER_EVENT_SIGNATURE, fromMasked, toMasked, tokenId) } - _afterTokenTransfers(from, to, tokenId, 1); // Perform the after hook. - if (_mloadERC721A(i += 0x20) != ++tokenId) break; // Break if `tokenId` is not sequential. - if (i == e) break; // Break if at the end of `tokenIds`. - // Break if the packed ownership slot is initialized. + _afterTokenTransfers(from, to, tokenId, 1); + if (_mloadERC721A(i += 0x20) != ++tokenId) break; + if (i == e) break; } while (_packedOwnerships[tokenId] == uint256(0)); } - // If the slot after the mini batch is neither out of bounds, nor initialized. + // Initialize the next slot if needed. if (tokenId != end) if (_packedOwnerships[tokenId] == uint256(0)) _packedOwnerships[tokenId] = prevOwnershipPacked; } while (i != e); From 401a6acd40f7515bb23776b5ddaa10b59a1b5cba Mon Sep 17 00:00:00 2001 From: Vectorized Date: Wed, 21 Aug 2024 04:27:57 +0000 Subject: [PATCH 07/13] Tidy, optimize --- contracts/ERC721A.sol | 175 +++++++++++++++++++++++++++++++++--------- 1 file changed, 137 insertions(+), 38 deletions(-) diff --git a/contracts/ERC721A.sol b/contracts/ERC721A.sol index 0114e115d..7b0a6cb1f 100644 --- a/contracts/ERC721A.sol +++ b/contracts/ERC721A.sol @@ -743,8 +743,7 @@ contract ERC721A is IERC721A { // Disallow transfer to zero address. if (toMasked == uint256(0)) _revert(TransferToZeroAddress.selector); // Whether we need to check the individual token approvals. - bool approvalCheck; - if (!_orERC721A(byMasked == uint256(0), byMasked == fromMasked)) approvalCheck = !isApprovedForAll(from, by); + bool mayTransfer = _orERC721A(byMasked == uint256(0), byMasked == fromMasked) || isApprovedForAll(from, by); uint256 n = tokenIds.length; // Early return if `tokenIds` is empty. @@ -753,30 +752,19 @@ contract ERC721A is IERC721A { uint256 end = _currentIndex; // Pointer to start and end (exclusive) of `tokenIds`. (uint256 i, uint256 e) = _mdataERC721A(tokenIds); - // Ensure that `tokenIds` is strictly ascending, and perform the before hooks before any state changes. - unchecked { - uint256 tokenId = _mloadERC721A(i); - // Revert if the minimum of the `tokenIds` is out of bounds. - if (_orERC721A(tokenId < _startTokenId(), end <= tokenId)) _revert(OwnerQueryForNonexistentToken.selector); - _beforeTokenTransfers(from, to, tokenId, 1); - if (n >= 2) { - uint256 j = i + 0x20; - do { - uint256 next = _mloadERC721A(j); - if (next <= tokenId) _revert(TokenIdsNotStrictlyAscending.selector); - _beforeTokenTransfers(from, to, tokenId = next, 1); - } while ((j += 0x20) != e); - // Revert if the maximum of the `tokenIds` is out of bounds. - if (end <= tokenId) _revert(OwnerQueryForNonexistentToken.selector); - } - // Increment and decrement the balances. - _packedAddressData[from] -= n; - _packedAddressData[to] += n; - } + + uint256 prevTokenId; uint256 prevOwnershipPacked; - do { - uint256 tokenId = _mloadERC721A(i); - unchecked { + unchecked { + do { + uint256 tokenId = _mloadERC721A(i); + uint256 miniBatchStart = tokenId; + // Revert `tokenId` is out of bounds. + if (_orERC721A(tokenId < _startTokenId(), end <= tokenId)) + _revert(OwnerQueryForNonexistentToken.selector); + // Revert if `tokenIds` is not strictly ascending. + if (prevOwnershipPacked != 0) + if (tokenId <= prevTokenId) _revert(TokenIdsNotStrictlyAscending.selector); // Scan backwards for an initialized packed ownership slot. // ERC721A's invariant guarantees that there will always be an initialized slot as long as // the start of the backwards scan falls within `[_startTokenId() .. _nextTokenId())`. @@ -785,16 +773,12 @@ contract ERC721A is IERC721A { if (prevOwnershipPacked & _BITMASK_BURNED != 0) _revert(OwnerQueryForNonexistentToken.selector); // Check that `tokenId` is owned by `from`. if (uint160(prevOwnershipPacked) != fromMasked) _revert(TransferFromIncorrectOwner.selector); - // Updates tokenId: - // - `address` to the next owner. - // - `startTimestamp` to the timestamp of transferring. - // - `burned` to `false`. - // - `nextInitialized` to `false`, as it is optional. - _packedOwnerships[tokenId] = _packOwnershipData(to, _nextExtraData(from, to, prevOwnershipPacked)); + do { (uint256 approvedAddressSlot, uint256 approvedAddressValue) = _getApprovedSlotAndValue(tokenId); + _beforeTokenTransfers(address(uint160(fromMasked)), address(uint160(toMasked)), tokenId, 1); // Revert if the sender is not authorized to transfer the token. - if (approvalCheck) + if (!mayTransfer) if (byMasked != approvedAddressValue) _revert(TransferCallerNotOwnerNorApproved.selector); assembly { if approvedAddressValue { @@ -803,15 +787,33 @@ contract ERC721A is IERC721A { // Emit the `Transfer` event. log4(0, 0, _TRANSFER_EVENT_SIGNATURE, fromMasked, toMasked, tokenId) } - _afterTokenTransfers(from, to, tokenId, 1); + if (_mloadERC721A(i += 0x20) != ++tokenId) break; if (i == e) break; } while (_packedOwnerships[tokenId] == uint256(0)); - } - // Initialize the next slot if needed. - if (tokenId != end) - if (_packedOwnerships[tokenId] == uint256(0)) _packedOwnerships[tokenId] = prevOwnershipPacked; - } while (i != e); + + // Updates tokenId: + // - `address` to the next owner. + // - `startTimestamp` to the timestamp of transferring. + // - `burned` to `false`. + // - `nextInitialized` to `false`, as it is optional. + _packedOwnerships[miniBatchStart] = _packOwnershipData( + address(uint160(toMasked)), + _nextExtraData(address(uint160(fromMasked)), address(uint160(toMasked)), prevOwnershipPacked) + ); + uint256 l = tokenId - miniBatchStart; + // Update the address data. + _packedAddressData[address(uint160(fromMasked))] -= l; + _packedAddressData[address(uint160(toMasked))] += l; + // Initialize the next slot if needed. + if (tokenId != end) + if (_packedOwnerships[tokenId] == uint256(0)) _packedOwnerships[tokenId] = prevOwnershipPacked; + // Perform the after hook for the batch. + _afterTokenTransfers(address(uint160(fromMasked)), address(uint160(toMasked)), miniBatchStart, l); + // Set the `prevTokenId` for checking that the `tokenIds` is strictly ascending. + prevTokenId = tokenId - 1; + } while (i != e); + } } /** @@ -1369,6 +1371,103 @@ contract ERC721A is IERC721A { } } + /** + * @dev Equivalent to `_batchBurn(address(0), tokenIds)`. + */ + function _batchBurn(uint256[] memory tokenIds) internal virtual { + _batchBurn(address(0), tokenIds); + } + + /** + * @dev Destroys `tokenIds`. + * Approvals are not cleared when tokenIds are burned. + * + * Requirements: + * + * - `tokenIds` must exist. + * - `tokenIds` must be strictly ascending. + * - `by` must be approved to burn these tokens by either {approve} or {setApprovalForAll}. + * + * Emits a {Transfer} event for each token burned. + */ + function _batchBurn(address by, uint256[] memory tokenIds) internal virtual { + uint256 n = tokenIds.length; + // Early return if `tokenIds` is empty. + if (n == uint256(0)) return; + // The next `tokenId` to be minted (i.e. `_nextTokenId()`). + uint256 end = _currentIndex; + // Pointer to start and end (exclusive) of `tokenIds`. + (uint256 i, uint256 e) = _mdataERC721A(tokenIds); + + uint256 prevOwnershipPacked; + address prevTokenOwner; + uint256 prevTokenId; + bool mayBurn; + unchecked { + do { + uint256 tokenId = _mloadERC721A(i); + uint256 miniBatchStart = tokenId; + // Revert `tokenId` is out of bounds. + if (_orERC721A(tokenId < _startTokenId(), end <= tokenId)) + _revert(OwnerQueryForNonexistentToken.selector); + // Revert if `tokenIds` is not strictly ascending. + if (prevOwnershipPacked != 0) + if (tokenId <= prevTokenId) _revert(TokenIdsNotStrictlyAscending.selector); + // Scan backwards for an initialized packed ownership slot. + // ERC721A's invariant guarantees that there will always be an initialized slot as long as + // the start of the backwards scan falls within `[_startTokenId() .. _nextTokenId())`. + for (uint256 j = tokenId; (prevOwnershipPacked = _packedOwnerships[j]) == uint256(0); ) --j; + // If the initialized slot is burned, revert. + if (prevOwnershipPacked & _BITMASK_BURNED != 0) _revert(OwnerQueryForNonexistentToken.selector); + + address tokenOwner = address(uint160(prevOwnershipPacked)); + if (tokenOwner != prevTokenOwner) { + prevTokenOwner = tokenOwner; + mayBurn = _orERC721A(by == address(0), tokenOwner == by) || isApprovedForAll(tokenOwner, by); + } + + do { + (uint256 approvedAddressSlot, uint256 approvedAddressValue) = _getApprovedSlotAndValue(tokenId); + _beforeTokenTransfers(tokenOwner, address(0), tokenId, 1); + // Revert if the sender is not authorized to transfer the token. + if (!mayBurn) + if (uint160(by) != approvedAddressValue) _revert(TransferCallerNotOwnerNorApproved.selector); + assembly { + if approvedAddressValue { + sstore(approvedAddressSlot, 0) // Equivalent to `delete _tokenApprovals[tokenId]`. + } + // Emit the `Transfer` event. + log4(0, 0, _TRANSFER_EVENT_SIGNATURE, and(_BITMASK_ADDRESS, tokenOwner), 0, tokenId) + } + if (_mloadERC721A(i += 0x20) != ++tokenId) break; + if (i == e) break; + } while (_packedOwnerships[tokenId] == uint256(0)); + + // Updates tokenId: + // - `address` to the same `tokenOwner`. + // - `startTimestamp` to the timestamp of transferring. + // - `burned` to `true`. + // - `nextInitialized` to `false`, as it is optional. + _packedOwnerships[miniBatchStart] = _packOwnershipData( + tokenOwner, + _BITMASK_BURNED | _nextExtraData(tokenOwner, address(0), prevOwnershipPacked) + ); + uint256 l = tokenId - miniBatchStart; + // Update the address data. + _packedAddressData[tokenOwner] += (l << _BITPOS_NUMBER_BURNED) - l; + // Initialize the next slot if needed. + if (tokenId != end) + if (_packedOwnerships[tokenId] == uint256(0)) _packedOwnerships[tokenId] = prevOwnershipPacked; + // Perform the after hook for the batch. + _afterTokenTransfers(tokenOwner, address(0), miniBatchStart, l); + // Set the `prevTokenId` for checking that the `tokenIds` is strictly ascending. + prevTokenId = tokenId - 1; + } while (i != e); + // Increment the overall burn counter. + _burnCounter += n; + } + } + // ============================================================= // EXTRA DATA OPERATIONS // ============================================================= From 230bad1968a3da5794e63a6ea5f1353bf5aeb64e Mon Sep 17 00:00:00 2001 From: Vectorized Date: Wed, 21 Aug 2024 04:33:37 +0000 Subject: [PATCH 08/13] Combine batch burnable and transferable --- contracts/extensions/ERC721ABatchBurnable.sol | 19 + .../extensions/IERC721ABatchBurnable.sol | 14 + .../interfaces/IERC721ABatchBurnable.sol | 7 + contracts/mocks/ERC721ABatchBurnableMock.sol | 50 +++ test/extensions/ERC721ABatchBurnable.test.js | 370 ++++++++++++++++++ 5 files changed, 460 insertions(+) create mode 100644 contracts/extensions/ERC721ABatchBurnable.sol create mode 100644 contracts/extensions/IERC721ABatchBurnable.sol create mode 100644 contracts/interfaces/IERC721ABatchBurnable.sol create mode 100644 contracts/mocks/ERC721ABatchBurnableMock.sol create mode 100644 test/extensions/ERC721ABatchBurnable.test.js diff --git a/contracts/extensions/ERC721ABatchBurnable.sol b/contracts/extensions/ERC721ABatchBurnable.sol new file mode 100644 index 000000000..4f3f8f145 --- /dev/null +++ b/contracts/extensions/ERC721ABatchBurnable.sol @@ -0,0 +1,19 @@ +// SPDX-License-Identifier: MIT +// ERC721A Contracts v4.2.3 +// Creator: Chiru Labs + +pragma solidity ^0.8.4; + +import './ERC721ABurnable.sol'; +import './IERC721ABatchBurnable.sol'; + +/** + * @title ERC721ABatchBurnable. + * + * @dev ERC721A token optimized for batch burns. + */ +abstract contract ERC721ABatchBurnable is ERC721ABurnable, IERC721ABatchBurnable { + function batchBurn(uint256[] memory tokenIds) public virtual override { + _batchBurn(_msgSenderERC721A(), tokenIds); + } +} diff --git a/contracts/extensions/IERC721ABatchBurnable.sol b/contracts/extensions/IERC721ABatchBurnable.sol new file mode 100644 index 000000000..2c7840ad4 --- /dev/null +++ b/contracts/extensions/IERC721ABatchBurnable.sol @@ -0,0 +1,14 @@ +// SPDX-License-Identifier: MIT +// ERC721A Contracts v4.2.3 +// Creator: Chiru Labs + +pragma solidity ^0.8.4; + +import './IERC721ABurnable.sol'; + +/** + * @dev Interface of ERC721ABatchBurnable. + */ +interface IERC721ABatchBurnable is IERC721ABurnable { + function batchBurn(uint256[] memory tokenIds) external; +} diff --git a/contracts/interfaces/IERC721ABatchBurnable.sol b/contracts/interfaces/IERC721ABatchBurnable.sol new file mode 100644 index 000000000..3074153eb --- /dev/null +++ b/contracts/interfaces/IERC721ABatchBurnable.sol @@ -0,0 +1,7 @@ +// SPDX-License-Identifier: MIT +// ERC721A Contracts v4.2.3 +// Creator: Chiru Labs + +pragma solidity ^0.8.4; + +import '../extensions/IERC721ABatchBurnable.sol'; diff --git a/contracts/mocks/ERC721ABatchBurnableMock.sol b/contracts/mocks/ERC721ABatchBurnableMock.sol new file mode 100644 index 000000000..058355ab3 --- /dev/null +++ b/contracts/mocks/ERC721ABatchBurnableMock.sol @@ -0,0 +1,50 @@ +// SPDX-License-Identifier: MIT +// ERC721A Contracts v4.2.3 +// Creators: Chiru Labs + +pragma solidity ^0.8.4; + +import '../extensions/ERC721ABatchBurnable.sol'; +import './DirectBurnBitSetterHelper.sol'; + +contract ERC721ABatchBurnableMock is ERC721ABatchBurnable, DirectBurnBitSetterHelper { + constructor(string memory name_, string memory symbol_) ERC721A(name_, symbol_) {} + + function exists(uint256 tokenId) public view returns (bool) { + return _exists(tokenId); + } + + function safeMint(address to, uint256 quantity) public { + _safeMint(to, quantity); + } + + function getOwnershipAt(uint256 index) public view returns (TokenOwnership memory) { + return _ownershipAt(index); + } + + function totalMinted() public view returns (uint256) { + return _totalMinted(); + } + + function totalBurned() public view returns (uint256) { + return _totalBurned(); + } + + function numberBurned(address owner) public view returns (uint256) { + return _numberBurned(owner); + } + + function initializeOwnershipAt(uint256 index) public { + _initializeOwnershipAt(index); + } + + function batchBurnUnoptimized(uint256[] memory tokenIds) public { + unchecked { + uint256 tokenId; + for (uint256 i; i < tokenIds.length; ++i) { + tokenId = tokenIds[i]; + _burn(tokenId); + } + } + } +} diff --git a/test/extensions/ERC721ABatchBurnable.test.js b/test/extensions/ERC721ABatchBurnable.test.js new file mode 100644 index 000000000..8c3fbe975 --- /dev/null +++ b/test/extensions/ERC721ABatchBurnable.test.js @@ -0,0 +1,370 @@ +const { deployContract, getBlockTimestamp, mineBlockTimestamp, offsettedIndex } = require('../helpers.js'); +const { expect } = require('chai'); +const { constants } = require('@openzeppelin/test-helpers'); +const { ZERO_ADDRESS } = constants; + +const createTestSuite = ({ contract, constructorArgs }) => + function () { + let offsetted; + + context(`${contract}`, function () { + beforeEach(async function () { + this.erc721aBatchBurnable = await deployContract(contract, constructorArgs); + + this.startTokenId = 0; + + offsetted = (...arr) => offsettedIndex(this.startTokenId, arr); + }); + + beforeEach(async function () { + const [owner, addr1, addr2, spender] = await ethers.getSigners(); + this.owner = owner; + this.addr1 = addr1; + this.addr2 = addr2; + this.spender = spender; + this.numTestTokens = 20; + this.totalTokens = 40; + this.totalBurned = 6; + this.burnedTokenIds1 = [2, 3, 4]; + this.burnedTokenIds2 = [7, 9, 10]; + this.notBurnedTokenId1 = 1; + this.notBurnedTokenId2 = 5; + this.notBurnedTokenId3 = 6; + this.notBurnedTokenId4 = 8; + this.initializedToken = 12; + this.uninitializedToken = 13; + + await this.erc721aBatchBurnable['safeMint(address,uint256)'](this.addr1.address, this.numTestTokens); + await this.erc721aBatchBurnable['safeMint(address,uint256)'](this.addr2.address, this.numTestTokens); + // Manually initialize token IDs + await this.erc721aBatchBurnable.initializeOwnershipAt(3); + await this.erc721aBatchBurnable.initializeOwnershipAt(this.initializedToken); + + await this.erc721aBatchBurnable + .connect(this.addr1) + .batchBurn([...this.burnedTokenIds1, ...this.burnedTokenIds2]); + }); + + context('totalSupply()', function () { + it('has the expected value', async function () { + expect(await this.erc721aBatchBurnable.totalSupply()).to.equal(this.totalTokens - this.totalBurned); + }); + + it('is reduced by burns', async function () { + const supplyBefore = await this.erc721aBatchBurnable.totalSupply(); + + await this.erc721aBatchBurnable + .connect(this.addr1) + .batchBurn(offsetted(this.notBurnedTokenId3, this.notBurnedTokenId4)); + + const supplyNow = await this.erc721aBatchBurnable.totalSupply(); + expect(supplyNow).to.equal(supplyBefore - 2); + }); + }); + + it('changes numberBurned', async function () { + expect(await this.erc721aBatchBurnable.numberBurned(this.addr1.address)).to.equal(this.totalBurned); + await this.erc721aBatchBurnable.connect(this.addr1).batchBurn([this.notBurnedTokenId4]); + expect(await this.erc721aBatchBurnable.numberBurned(this.addr1.address)).to.equal(this.totalBurned + 1); + }); + + it('changes totalBurned', async function () { + const totalBurnedBefore = (await this.erc721aBatchBurnable.totalBurned()).toNumber(); + expect(totalBurnedBefore).to.equal(this.totalBurned); + + await this.erc721aBatchBurnable + .connect(this.addr1) + .batchBurn(offsetted(this.notBurnedTokenId3, this.notBurnedTokenId4)); + + const totalBurnedNow = (await this.erc721aBatchBurnable.totalBurned()).toNumber(); + expect(totalBurnedNow).to.equal(totalBurnedBefore + 2); + }); + + it('changes exists', async function () { + for (let i = 0; i < 3; ++i) { + expect(await this.erc721aBatchBurnable.exists(this.burnedTokenIds1[i])).to.be.false; + expect(await this.erc721aBatchBurnable.exists(this.burnedTokenIds2[i])).to.be.false; + } + + expect(await this.erc721aBatchBurnable.exists(this.notBurnedTokenId1)).to.be.true; + expect(await this.erc721aBatchBurnable.exists(this.notBurnedTokenId2)).to.be.true; + expect(await this.erc721aBatchBurnable.exists(this.notBurnedTokenId3)).to.be.true; + expect(await this.erc721aBatchBurnable.exists(this.notBurnedTokenId4)).to.be.true; + + await this.erc721aBatchBurnable + .connect(this.addr1) + .batchBurn(offsetted(this.notBurnedTokenId3, this.notBurnedTokenId4)); + expect(await this.erc721aBatchBurnable.exists(this.notBurnedTokenId3)).to.be.false; + expect(await this.erc721aBatchBurnable.exists(this.notBurnedTokenId4)).to.be.false; + expect(await this.erc721aBatchBurnable.exists(this.totalTokens)).to.be.false; + }); + + it('cannot burn a non-existing token', async function () { + const query = this.erc721aBatchBurnable + .connect(this.addr1) + .batchBurn([this.notBurnedTokenId4, this.totalTokens]); + await expect(query).to.be.revertedWith('OwnerQueryForNonexistentToken'); + }); + + it('can only burn tokenIds when provided in ascending order', async function () { + const query = this.erc721aBatchBurnable + .connect(this.addr1) + .batchBurn([this.notBurnedTokenId3, this.notBurnedTokenId2, this.notBurnedTokenId1]); + await expect(query).to.be.revertedWith('TokenIdsNotStrictlyAscending'); + }); + + it('cannot burn a burned token', async function () { + const query = this.erc721aBatchBurnable.connect(this.addr1).batchBurn(this.burnedTokenIds1); + await expect(query).to.be.revertedWith('OwnerQueryForNonexistentToken'); + }); + + it('cannot burn with wrong caller or spender', async function () { + const tokenIdsToBurn = [this.notBurnedTokenId1, this.notBurnedTokenId2]; + + // sanity check + await this.erc721aBatchBurnable.connect(this.addr1).approve(ZERO_ADDRESS, tokenIdsToBurn[0]); + await this.erc721aBatchBurnable.connect(this.addr1).approve(ZERO_ADDRESS, tokenIdsToBurn[1]); + await this.erc721aBatchBurnable.connect(this.addr1).setApprovalForAll(this.spender.address, false); + + const query = this.erc721aBatchBurnable.connect(this.spender).batchBurn(tokenIdsToBurn); + await expect(query).to.be.revertedWith('TransferCallerNotOwnerNorApproved'); + }); + + it('cannot burn sequential ID with wrong owner', async function () { + const tokenIdsToBurn = [this.notBurnedTokenId2, this.notBurnedTokenId3]; + + await this.erc721aBatchBurnable.connect(this.addr1).approve(this.spender.address, tokenIdsToBurn[0]); + + const query1 = this.erc721aBatchBurnable.connect(this.spender).batchBurn(tokenIdsToBurn); + await expect(query1).to.be.revertedWith('TransferCallerNotOwnerNorApproved'); + const query2 = this.erc721aBatchBurnable.connect(this.addr1).batchBurn([19, 20]); + await expect(query2).to.be.revertedWith('TransferCallerNotOwnerNorApproved'); + }); + + it('spender can burn with specific approved tokenId', async function () { + const tokenIdsToBurn = [this.notBurnedTokenId1, this.notBurnedTokenId2]; + + await this.erc721aBatchBurnable.connect(this.addr1).approve(this.spender.address, tokenIdsToBurn[0]); + await this.erc721aBatchBurnable.connect(this.addr1).approve(this.spender.address, tokenIdsToBurn[1]); + await this.erc721aBatchBurnable.connect(this.spender).batchBurn(tokenIdsToBurn); + expect(await this.erc721aBatchBurnable.exists(tokenIdsToBurn[0])).to.be.false; + expect(await this.erc721aBatchBurnable.exists(tokenIdsToBurn[1])).to.be.false; + }); + + it('spender can burn with one-time approval', async function () { + const tokenIdsToBurn = [this.notBurnedTokenId1, this.notBurnedTokenId2]; + + await this.erc721aBatchBurnable.connect(this.addr1).setApprovalForAll(this.spender.address, true); + await this.erc721aBatchBurnable.connect(this.spender).batchBurn(tokenIdsToBurn); + expect(await this.erc721aBatchBurnable.exists(tokenIdsToBurn[0])).to.be.false; + expect(await this.erc721aBatchBurnable.exists(tokenIdsToBurn[1])).to.be.false; + }); + + it('cannot transfer a burned token', async function () { + const query = this.erc721aBatchBurnable + .connect(this.addr1) + .transferFrom(this.addr1.address, this.addr2.address, this.burnedTokenIds1[0]); + await expect(query).to.be.revertedWith('OwnerQueryForNonexistentToken'); + }); + + it('can burn tokens with different owners', async function () { + const tokenIdsToBurn = [this.notBurnedTokenId1, this.notBurnedTokenId2, this.notBurnedTokenId3]; + + await this.erc721aBatchBurnable.connect(this.addr1).setApprovalForAll(this.spender.address, true); + await this.erc721aBatchBurnable + .connect(this.addr1) + .transferFrom(this.addr1.address, this.spender.address, this.notBurnedTokenId2); + + await this.erc721aBatchBurnable + .connect(this.addr1) + .transferFrom(this.addr1.address, this.addr2.address, this.notBurnedTokenId3); + await this.erc721aBatchBurnable.connect(this.addr2).approve(this.spender.address, this.notBurnedTokenId3); + + const totalBurnedBefore = (await this.erc721aBatchBurnable.totalBurned()).toNumber(); + await this.erc721aBatchBurnable.connect(this.spender).batchBurn(tokenIdsToBurn); + + expect(await this.erc721aBatchBurnable.exists(this.notBurnedTokenId1)).to.be.false; + expect(await this.erc721aBatchBurnable.exists(this.notBurnedTokenId2)).to.be.false; + expect(await this.erc721aBatchBurnable.exists(this.notBurnedTokenId3)).to.be.false; + expect((await this.erc721aBatchBurnable.totalBurned()).toNumber() - totalBurnedBefore).to.equal(3); + }); + + it('does not affect _totalMinted', async function () { + const tokenIdsToBurn = [this.notBurnedTokenId1, this.notBurnedTokenId2]; + const totalMintedBefore = await this.erc721aBatchBurnable.totalMinted(); + expect(totalMintedBefore).to.equal(this.totalTokens); + await this.erc721aBatchBurnable.connect(this.addr1).batchBurn(tokenIdsToBurn); + expect(await this.erc721aBatchBurnable.totalMinted()).to.equal(totalMintedBefore); + }); + + it('adjusts owners balances', async function () { + expect(await this.erc721aBatchBurnable.balanceOf(this.addr1.address)).to.be.equal( + this.numTestTokens - this.totalBurned + ); + }); + + it('startTimestamp updated correctly', async function () { + const tokenIdsToBurn = [this.notBurnedTokenId1]; + const ownershipBefore = await this.erc721aBatchBurnable.getOwnershipAt(tokenIdsToBurn[0]); + const timestampBefore = parseInt(ownershipBefore.startTimestamp); + const timestampToMine = (await getBlockTimestamp()) + 12345; + await mineBlockTimestamp(timestampToMine); + const timestampMined = await getBlockTimestamp(); + await this.erc721aBatchBurnable.connect(this.addr1).batchBurn(tokenIdsToBurn); + const ownershipAfter = await this.erc721aBatchBurnable.getOwnershipAt(tokenIdsToBurn[0]); + const timestampAfter = parseInt(ownershipAfter.startTimestamp); + expect(timestampBefore).to.be.lt(timestampToMine); + expect(timestampAfter).to.be.gte(timestampToMine); + expect(timestampAfter).to.be.lt(timestampToMine + 10); + expect(timestampToMine).to.be.eq(timestampMined); + }); + + describe('ownerships correctly set', async function () { + it('with tokens burned', async function () { + await this.erc721aBatchBurnable.connect(this.addr1).batchBurn([this.notBurnedTokenId1]); + + for (let i = 0; i < this.numTestTokens; ++i) { + const initializedTokens = [0, 2, 3, 5, 7, 8, 9, 11, 12, this.notBurnedTokenId1]; + + expect((await this.erc721aBatchBurnable.getOwnershipAt(i))[0]).to.be.equal( + initializedTokens.includes(i) ? this.addr1.address : ZERO_ADDRESS + ); + } + }); + + // it('with tokens burned and cleared', async function () { + // const initializedToken = 15; + + // expect((await this.erc721aBatchBurnable.getOwnershipAt(initializedToken - 1))[0]) + // .to.be.equal(ZERO_ADDRESS); + // expect((await this.erc721aBatchBurnable.getOwnershipAt(initializedToken))[0]).to.be.equal(ZERO_ADDRESS); + + // // Initialize token + // await this.erc721aBatchBurnable.initializeOwnershipAt(initializedToken); + // expect((await this.erc721aBatchBurnable.getOwnershipAt(initializedToken - 1))[0]) + // .to.be.equal(ZERO_ADDRESS); + // expect((await this.erc721aBatchBurnable.getOwnershipAt(initializedToken))[0]) + // .to.be.equal(this.addr1.address); + + // // Burn tokens + // await this.erc721aBatchBurnable.connect(this.addr1).batchBurn([initializedToken - 1, initializedToken]); + // expect((await this.erc721aBatchBurnable.getOwnershipAt(initializedToken - 1))[0]).to.be.equal( + // this.addr1.address + // ); + + // // Initialized tokens in a consecutive burn are cleared + // expect((await this.erc721aBatchBurnable.getOwnershipAt(3))[0]).to.be.equal(ZERO_ADDRESS); + // expect((await this.erc721aBatchBurnable.getOwnershipAt(initializedToken))[0]).to.be.equal(ZERO_ADDRESS); + // }); + + it('with token before previously burnt token transferred and burned', async function () { + await this.erc721aBatchBurnable + .connect(this.addr1) + .transferFrom(this.addr1.address, this.addr2.address, this.notBurnedTokenId1); + expect(await this.erc721aBatchBurnable.ownerOf(this.notBurnedTokenId1)).to.be.equal(this.addr2.address); + await this.erc721aBatchBurnable.connect(this.addr2).batchBurn([this.notBurnedTokenId1]); + for (let i = 0; i < this.numTestTokens; ++i) { + if (i == this.notBurnedTokenId1 || this.burnedTokenIds1.includes(i) || this.burnedTokenIds2.includes(i)) { + await expect(this.erc721aBatchBurnable.ownerOf(i)).to.be.revertedWith('OwnerQueryForNonexistentToken'); + } else { + expect(await this.erc721aBatchBurnable.ownerOf(i)).to.be.equal(this.addr1.address); + } + } + }); + + it('with token after previously burnt token transferred and burned', async function () { + const tokenIdsToBurn = [this.notBurnedTokenId1, this.notBurnedTokenId3]; + await this.erc721aBatchBurnable + .connect(this.addr1) + .transferFrom(this.addr1.address, this.addr2.address, tokenIdsToBurn[0]); + await this.erc721aBatchBurnable + .connect(this.addr1) + .transferFrom(this.addr1.address, this.addr2.address, tokenIdsToBurn[1]); + expect(await this.erc721aBatchBurnable.ownerOf(tokenIdsToBurn[0])).to.be.equal(this.addr2.address); + expect(await this.erc721aBatchBurnable.ownerOf(tokenIdsToBurn[1])).to.be.equal(this.addr2.address); + await this.erc721aBatchBurnable.connect(this.addr2).batchBurn(tokenIdsToBurn); + for (let i = 0; i < this.numTestTokens; ++i) { + if (tokenIdsToBurn.includes(i) || this.burnedTokenIds1.includes(i) || this.burnedTokenIds2.includes(i)) { + await expect(this.erc721aBatchBurnable.ownerOf(i)).to.be.revertedWith('OwnerQueryForNonexistentToken'); + } else { + expect(await this.erc721aBatchBurnable.ownerOf(i)).to.be.equal(this.addr1.address); + } + } + }); + + it('with first token burned', async function () { + await this.erc721aBatchBurnable.connect(this.addr1).batchBurn([0]); + for (let i = 0; i < this.numTestTokens; ++i) { + if (i == 0 || this.burnedTokenIds1.includes(i) || this.burnedTokenIds2.includes(i)) { + await expect(this.erc721aBatchBurnable.ownerOf(i)).to.be.revertedWith('OwnerQueryForNonexistentToken'); + } else { + expect(await this.erc721aBatchBurnable.ownerOf(i)).to.be.equal(this.addr1.address); + } + } + }); + + it('with last token burned', async function () { + await expect(this.erc721aBatchBurnable.ownerOf(offsetted(this.totalTokens))).to.be.revertedWith( + 'OwnerQueryForNonexistentToken' + ); + await this.erc721aBatchBurnable.connect(this.addr2).batchBurn([offsetted(this.totalTokens - 1)]); + await expect(this.erc721aBatchBurnable.ownerOf(offsetted(this.totalTokens - 1))).to.be.revertedWith( + 'OwnerQueryForNonexistentToken' + ); + }); + + it('with initialized token transferred', async function () { + expect(await this.erc721aBatchBurnable.ownerOf(this.initializedToken)).to.be.equal(this.addr1.address); + expect(await this.erc721aBatchBurnable.ownerOf(this.initializedToken + 1)).to.be.equal(this.addr1.address); + expect((await this.erc721aBatchBurnable.getOwnershipAt(this.initializedToken))[0]).to.be.equal( + this.addr1.address + ); + expect((await this.erc721aBatchBurnable.getOwnershipAt(this.initializedToken + 1))[0]).to.be.equal( + ZERO_ADDRESS + ); + + await this.erc721aBatchBurnable.connect(this.addr1).batchBurn([this.initializedToken]); + + await expect(this.erc721aBatchBurnable.ownerOf(this.initializedToken)).to.be.revertedWith( + 'OwnerQueryForNonexistentToken' + ); + expect(await this.erc721aBatchBurnable.ownerOf(this.initializedToken + 1)).to.be.equal(this.addr1.address); + expect((await this.erc721aBatchBurnable.getOwnershipAt(this.initializedToken))[0]).to.be.equal( + this.addr1.address + ); + expect((await this.erc721aBatchBurnable.getOwnershipAt(this.initializedToken + 1))[0]).to.be.equal( + this.addr1.address + ); + }); + + it('with uninitialized token transferred', async function () { + expect(await this.erc721aBatchBurnable.ownerOf(this.uninitializedToken)).to.be.equal(this.addr1.address); + expect(await this.erc721aBatchBurnable.ownerOf(this.uninitializedToken + 1)).to.be.equal(this.addr1.address); + expect((await this.erc721aBatchBurnable.getOwnershipAt(this.uninitializedToken))[0]).to.be.equal( + ZERO_ADDRESS + ); + expect((await this.erc721aBatchBurnable.getOwnershipAt(this.uninitializedToken + 1))[0]).to.be.equal( + ZERO_ADDRESS + ); + + await this.erc721aBatchBurnable.connect(this.addr1).batchBurn([this.uninitializedToken]); + + await expect(this.erc721aBatchBurnable.ownerOf(this.uninitializedToken)).to.be.revertedWith( + 'OwnerQueryForNonexistentToken' + ); + expect(await this.erc721aBatchBurnable.ownerOf(this.uninitializedToken + 1)).to.be.equal(this.addr1.address); + expect((await this.erc721aBatchBurnable.getOwnershipAt(this.uninitializedToken))[0]).to.be.equal( + this.addr1.address + ); + expect((await this.erc721aBatchBurnable.getOwnershipAt(this.uninitializedToken + 1))[0]).to.be.equal( + this.addr1.address + ); + }); + }); + }); + }; + +describe( + 'ERC721ABatchBurnable', + createTestSuite({ contract: 'ERC721ABatchBurnableMock', constructorArgs: ['Azuki', 'AZUKI'] }) +); From e28d30c658b7e723d42af8b3f27fcf6ed4d14e95 Mon Sep 17 00:00:00 2001 From: Vectorized Date: Wed, 21 Aug 2024 04:36:06 +0000 Subject: [PATCH 09/13] Edit comment --- contracts/ERC721A.sol | 3 +++ 1 file changed, 3 insertions(+) diff --git a/contracts/ERC721A.sol b/contracts/ERC721A.sol index 7b0a6cb1f..cf597b022 100644 --- a/contracts/ERC721A.sol +++ b/contracts/ERC721A.sol @@ -1388,6 +1388,9 @@ contract ERC721A is IERC721A { * - `tokenIds` must be strictly ascending. * - `by` must be approved to burn these tokens by either {approve} or {setApprovalForAll}. * + * `by` is the address that to check token approval for. + * If token approval check is not needed, pass in `address(0)` for `by`. + * * Emits a {Transfer} event for each token burned. */ function _batchBurn(address by, uint256[] memory tokenIds) internal virtual { From 2ed86e8f785e4f2cebd1179ef5ec19176a0eb497 Mon Sep 17 00:00:00 2001 From: Vectorized Date: Fri, 23 Aug 2024 07:06:07 +0000 Subject: [PATCH 10/13] Tidy tests --- test/extensions/ERC721ABatchBurnable.test.js | 25 -------------------- 1 file changed, 25 deletions(-) diff --git a/test/extensions/ERC721ABatchBurnable.test.js b/test/extensions/ERC721ABatchBurnable.test.js index 8c3fbe975..dad2a5d7b 100644 --- a/test/extensions/ERC721ABatchBurnable.test.js +++ b/test/extensions/ERC721ABatchBurnable.test.js @@ -232,31 +232,6 @@ const createTestSuite = ({ contract, constructorArgs }) => } }); - // it('with tokens burned and cleared', async function () { - // const initializedToken = 15; - - // expect((await this.erc721aBatchBurnable.getOwnershipAt(initializedToken - 1))[0]) - // .to.be.equal(ZERO_ADDRESS); - // expect((await this.erc721aBatchBurnable.getOwnershipAt(initializedToken))[0]).to.be.equal(ZERO_ADDRESS); - - // // Initialize token - // await this.erc721aBatchBurnable.initializeOwnershipAt(initializedToken); - // expect((await this.erc721aBatchBurnable.getOwnershipAt(initializedToken - 1))[0]) - // .to.be.equal(ZERO_ADDRESS); - // expect((await this.erc721aBatchBurnable.getOwnershipAt(initializedToken))[0]) - // .to.be.equal(this.addr1.address); - - // // Burn tokens - // await this.erc721aBatchBurnable.connect(this.addr1).batchBurn([initializedToken - 1, initializedToken]); - // expect((await this.erc721aBatchBurnable.getOwnershipAt(initializedToken - 1))[0]).to.be.equal( - // this.addr1.address - // ); - - // // Initialized tokens in a consecutive burn are cleared - // expect((await this.erc721aBatchBurnable.getOwnershipAt(3))[0]).to.be.equal(ZERO_ADDRESS); - // expect((await this.erc721aBatchBurnable.getOwnershipAt(initializedToken))[0]).to.be.equal(ZERO_ADDRESS); - // }); - it('with token before previously burnt token transferred and burned', async function () { await this.erc721aBatchBurnable .connect(this.addr1) From a0a200a23642b49e85345b3c7c7a44c0fc9d7442 Mon Sep 17 00:00:00 2001 From: Vectorized Date: Fri, 23 Aug 2024 07:33:04 +0000 Subject: [PATCH 11/13] Tidy --- contracts/ERC721A.sol | 51 +++++++++++++++++++++++-------------------- 1 file changed, 27 insertions(+), 24 deletions(-) diff --git a/contracts/ERC721A.sol b/contracts/ERC721A.sol index cf597b022..e292cc7a6 100644 --- a/contracts/ERC721A.sol +++ b/contracts/ERC721A.sol @@ -745,19 +745,18 @@ contract ERC721A is IERC721A { // Whether we need to check the individual token approvals. bool mayTransfer = _orERC721A(byMasked == uint256(0), byMasked == fromMasked) || isApprovedForAll(from, by); - uint256 n = tokenIds.length; // Early return if `tokenIds` is empty. - if (n == uint256(0)) return; + if (tokenIds.length == uint256(0)) return; // The next `tokenId` to be minted (i.e. `_nextTokenId()`). uint256 end = _currentIndex; // Pointer to start and end (exclusive) of `tokenIds`. - (uint256 i, uint256 e) = _mdataERC721A(tokenIds); + (uint256 ptr, uint256 ptrEnd) = _mdataERC721A(tokenIds); uint256 prevTokenId; uint256 prevOwnershipPacked; unchecked { do { - uint256 tokenId = _mloadERC721A(i); + uint256 tokenId = _mloadERC721A(ptr); uint256 miniBatchStart = tokenId; // Revert `tokenId` is out of bounds. if (_orERC721A(tokenId < _startTokenId(), end <= tokenId)) @@ -788,8 +787,8 @@ contract ERC721A is IERC721A { log4(0, 0, _TRANSFER_EVENT_SIGNATURE, fromMasked, toMasked, tokenId) } - if (_mloadERC721A(i += 0x20) != ++tokenId) break; - if (i == e) break; + if (_mloadERC721A(ptr += 0x20) != ++tokenId) break; + if (ptr == ptrEnd) break; } while (_packedOwnerships[tokenId] == uint256(0)); // Updates tokenId: @@ -801,18 +800,23 @@ contract ERC721A is IERC721A { address(uint160(toMasked)), _nextExtraData(address(uint160(fromMasked)), address(uint160(toMasked)), prevOwnershipPacked) ); - uint256 l = tokenId - miniBatchStart; + uint256 miniBatchLength = tokenId - miniBatchStart; // Update the address data. - _packedAddressData[address(uint160(fromMasked))] -= l; - _packedAddressData[address(uint160(toMasked))] += l; + _packedAddressData[address(uint160(fromMasked))] -= miniBatchLength; + _packedAddressData[address(uint160(toMasked))] += miniBatchLength; // Initialize the next slot if needed. if (tokenId != end) if (_packedOwnerships[tokenId] == uint256(0)) _packedOwnerships[tokenId] = prevOwnershipPacked; // Perform the after hook for the batch. - _afterTokenTransfers(address(uint160(fromMasked)), address(uint160(toMasked)), miniBatchStart, l); + _afterTokenTransfers( + address(uint160(fromMasked)), + address(uint160(toMasked)), + miniBatchStart, + miniBatchLength + ); // Set the `prevTokenId` for checking that the `tokenIds` is strictly ascending. prevTokenId = tokenId - 1; - } while (i != e); + } while (ptr != ptrEnd); } } @@ -880,8 +884,8 @@ contract ERC721A is IERC721A { unchecked { if (to.code.length != 0) { - for ((uint256 i, uint256 e) = _mdataERC721A(tokenIds); i != e; i += 0x20) { - if (!_checkContractOnERC721Received(from, to, _mloadERC721A(i), _data)) { + for ((uint256 ptr, uint256 ptrEnd) = _mdataERC721A(tokenIds); ptr != ptrEnd; ptr += 0x20) { + if (!_checkContractOnERC721Received(from, to, _mloadERC721A(ptr), _data)) { _revert(TransferToNonERC721ReceiverImplementer.selector); } } @@ -1394,13 +1398,12 @@ contract ERC721A is IERC721A { * Emits a {Transfer} event for each token burned. */ function _batchBurn(address by, uint256[] memory tokenIds) internal virtual { - uint256 n = tokenIds.length; // Early return if `tokenIds` is empty. - if (n == uint256(0)) return; + if (tokenIds.length == uint256(0)) return; // The next `tokenId` to be minted (i.e. `_nextTokenId()`). uint256 end = _currentIndex; // Pointer to start and end (exclusive) of `tokenIds`. - (uint256 i, uint256 e) = _mdataERC721A(tokenIds); + (uint256 ptr, uint256 ptrEnd) = _mdataERC721A(tokenIds); uint256 prevOwnershipPacked; address prevTokenOwner; @@ -1408,7 +1411,7 @@ contract ERC721A is IERC721A { bool mayBurn; unchecked { do { - uint256 tokenId = _mloadERC721A(i); + uint256 tokenId = _mloadERC721A(ptr); uint256 miniBatchStart = tokenId; // Revert `tokenId` is out of bounds. if (_orERC721A(tokenId < _startTokenId(), end <= tokenId)) @@ -1442,8 +1445,8 @@ contract ERC721A is IERC721A { // Emit the `Transfer` event. log4(0, 0, _TRANSFER_EVENT_SIGNATURE, and(_BITMASK_ADDRESS, tokenOwner), 0, tokenId) } - if (_mloadERC721A(i += 0x20) != ++tokenId) break; - if (i == e) break; + if (_mloadERC721A(ptr += 0x20) != ++tokenId) break; + if (ptr == ptrEnd) break; } while (_packedOwnerships[tokenId] == uint256(0)); // Updates tokenId: @@ -1455,19 +1458,19 @@ contract ERC721A is IERC721A { tokenOwner, _BITMASK_BURNED | _nextExtraData(tokenOwner, address(0), prevOwnershipPacked) ); - uint256 l = tokenId - miniBatchStart; + uint256 miniBatchLength = tokenId - miniBatchStart; // Update the address data. - _packedAddressData[tokenOwner] += (l << _BITPOS_NUMBER_BURNED) - l; + _packedAddressData[tokenOwner] += (miniBatchLength << _BITPOS_NUMBER_BURNED) - miniBatchLength; // Initialize the next slot if needed. if (tokenId != end) if (_packedOwnerships[tokenId] == uint256(0)) _packedOwnerships[tokenId] = prevOwnershipPacked; // Perform the after hook for the batch. - _afterTokenTransfers(tokenOwner, address(0), miniBatchStart, l); + _afterTokenTransfers(tokenOwner, address(0), miniBatchStart, miniBatchLength); // Set the `prevTokenId` for checking that the `tokenIds` is strictly ascending. prevTokenId = tokenId - 1; - } while (i != e); + } while (ptr != ptrEnd); // Increment the overall burn counter. - _burnCounter += n; + _burnCounter += tokenIds.length; } } From c48aa1433ea202f4929eb17370645e4e54224001 Mon Sep 17 00:00:00 2001 From: Vectorized Date: Tue, 27 Aug 2024 04:15:25 +0000 Subject: [PATCH 12/13] Edit comment --- contracts/ERC721A.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/ERC721A.sol b/contracts/ERC721A.sol index e292cc7a6..30e0cb0ca 100644 --- a/contracts/ERC721A.sol +++ b/contracts/ERC721A.sol @@ -742,7 +742,7 @@ contract ERC721A is IERC721A { uint256 toMasked = uint160(to); // Disallow transfer to zero address. if (toMasked == uint256(0)) _revert(TransferToZeroAddress.selector); - // Whether we need to check the individual token approvals. + // Whether `by` may transfer the tokens. bool mayTransfer = _orERC721A(byMasked == uint256(0), byMasked == fromMasked) || isApprovedForAll(from, by); // Early return if `tokenIds` is empty. From ab31bd7328b3fd972ce79155a5f100f712d68286 Mon Sep 17 00:00:00 2001 From: Vectorized Date: Tue, 27 Aug 2024 06:13:18 +0000 Subject: [PATCH 13/13] Remove unnecessary internal functions, max out test coverage --- contracts/ERC721A.sol | 42 ------------------- contracts/mocks/ERC721ABatchBurnableMock.sol | 10 ----- .../mocks/ERC721ABatchTransferableMock.sol | 34 +++++---------- test/GasUsage.test.js | 16 +++++++ test/extensions/ERC721ABatchBurnable.test.js | 4 ++ .../ERC721ABatchTransferable.test.js | 27 ++++++++++-- 6 files changed, 53 insertions(+), 80 deletions(-) diff --git a/contracts/ERC721A.sol b/contracts/ERC721A.sol index 30e0cb0ca..b4ad398cd 100644 --- a/contracts/ERC721A.sol +++ b/contracts/ERC721A.sol @@ -820,41 +820,6 @@ contract ERC721A is IERC721A { } } - /** - * @dev Equivalent to `_safeBatchTransferFrom(from, to, tokenIds)`. - */ - function _safeBatchTransferFrom( - address from, - address to, - uint256[] memory tokenIds - ) internal virtual { - _safeBatchTransferFrom(address(0), from, to, tokenIds); - } - - /** - * @dev Equivalent to `_safeBatchTransferFrom(by, from, to, tokenIds, '')`. - */ - function _safeBatchTransferFrom( - address by, - address from, - address to, - uint256[] memory tokenIds - ) internal virtual { - _safeBatchTransferFrom(by, from, to, tokenIds, ''); - } - - /** - * @dev Equivalent to `_safeBatchTransferFrom(address(0), from, to, tokenIds, _data)`. - */ - function _safeBatchTransferFrom( - address from, - address to, - uint256[] memory tokenIds, - bytes memory _data - ) internal virtual { - _safeBatchTransferFrom(address(0), from, to, tokenIds, _data); - } - /** * @dev Safely transfers `tokenIds` in batch from `from` to `to`. * @@ -1375,13 +1340,6 @@ contract ERC721A is IERC721A { } } - /** - * @dev Equivalent to `_batchBurn(address(0), tokenIds)`. - */ - function _batchBurn(uint256[] memory tokenIds) internal virtual { - _batchBurn(address(0), tokenIds); - } - /** * @dev Destroys `tokenIds`. * Approvals are not cleared when tokenIds are burned. diff --git a/contracts/mocks/ERC721ABatchBurnableMock.sol b/contracts/mocks/ERC721ABatchBurnableMock.sol index 058355ab3..80ead601f 100644 --- a/contracts/mocks/ERC721ABatchBurnableMock.sol +++ b/contracts/mocks/ERC721ABatchBurnableMock.sol @@ -37,14 +37,4 @@ contract ERC721ABatchBurnableMock is ERC721ABatchBurnable, DirectBurnBitSetterHe function initializeOwnershipAt(uint256 index) public { _initializeOwnershipAt(index); } - - function batchBurnUnoptimized(uint256[] memory tokenIds) public { - unchecked { - uint256 tokenId; - for (uint256 i; i < tokenIds.length; ++i) { - tokenId = tokenIds[i]; - _burn(tokenId); - } - } - } } diff --git a/contracts/mocks/ERC721ABatchTransferableMock.sol b/contracts/mocks/ERC721ABatchTransferableMock.sol index 764ccfb82..619c0b93e 100644 --- a/contracts/mocks/ERC721ABatchTransferableMock.sol +++ b/contracts/mocks/ERC721ABatchTransferableMock.sol @@ -9,10 +9,6 @@ import '../extensions/ERC721ABatchTransferable.sol'; contract ERC721ABatchTransferableMock is ERC721ABatchTransferable { constructor(string memory name_, string memory symbol_) ERC721A(name_, symbol_) {} - function exists(uint256 tokenId) public view returns (bool) { - return _exists(tokenId); - } - function safeMint(address to, uint256 quantity) public { _safeMint(to, quantity); } @@ -21,22 +17,6 @@ contract ERC721ABatchTransferableMock is ERC721ABatchTransferable { return _ownershipAt(index); } - function totalMinted() public view returns (uint256) { - return _totalMinted(); - } - - function totalBurned() public view returns (uint256) { - return _totalBurned(); - } - - function numberBurned(address owner) public view returns (uint256) { - return _numberBurned(owner); - } - - function burn(uint256 tokenId) public { - _burn(tokenId, true); - } - function initializeOwnershipAt(uint256 index) public { _initializeOwnershipAt(index); } @@ -59,10 +39,8 @@ contract ERC721ABatchTransferableMock is ERC721ABatchTransferable { uint256[] memory tokenIds ) public { unchecked { - uint256 tokenId; - for (uint256 i; i < tokenIds.length; ++i) { - tokenId = tokenIds[i]; - transferFrom(from, to, tokenId); + for (uint256 i; i != tokenIds.length; ++i) { + transferFrom(from, to, tokenIds[i]); } } } @@ -75,4 +53,12 @@ contract ERC721ABatchTransferableMock is ERC721ABatchTransferable { ) public { _batchTransferFrom(by, from, to, tokenIds); } + + function directBatchTransferFrom( + address from, + address to, + uint256[] memory tokenIds + ) public { + _batchTransferFrom(from, to, tokenIds); + } } diff --git a/test/GasUsage.test.js b/test/GasUsage.test.js index 7b6e435c2..c1e9cfcc3 100644 --- a/test/GasUsage.test.js +++ b/test/GasUsage.test.js @@ -40,6 +40,22 @@ describe('ERC721A Gas Usage', function () { }); }); + context('mintHundred', function () { + it('runs mintHundred 2 times', async function () { + for (let i = 0; i < 2; i++) { + await this.erc721a.mintHundred(this.addr1.address); + } + }); + }); + + context('safeMintHundred', function () { + it('runs safeMintHundred 2 times', async function () { + for (let i = 0; i < 2; i++) { + await this.erc721a.safeMintHundred(this.addr1.address); + } + }); + }); + context('transferFrom', function () { beforeEach(async function () { await this.erc721a.mintTen(this.owner.address); diff --git a/test/extensions/ERC721ABatchBurnable.test.js b/test/extensions/ERC721ABatchBurnable.test.js index dad2a5d7b..61bb31b9e 100644 --- a/test/extensions/ERC721ABatchBurnable.test.js +++ b/test/extensions/ERC721ABatchBurnable.test.js @@ -62,6 +62,10 @@ const createTestSuite = ({ contract, constructorArgs }) => }); }); + it('batch burn nothing', async function () { + await this.erc721aBatchBurnable.connect(this.addr1).batchBurn([]); + }); + it('changes numberBurned', async function () { expect(await this.erc721aBatchBurnable.numberBurned(this.addr1.address)).to.equal(this.totalBurned); await this.erc721aBatchBurnable.connect(this.addr1).batchBurn([this.notBurnedTokenId4]); diff --git a/test/extensions/ERC721ABatchTransferable.test.js b/test/extensions/ERC721ABatchTransferable.test.js index ca9548221..994024e92 100644 --- a/test/extensions/ERC721ABatchTransferable.test.js +++ b/test/extensions/ERC721ABatchTransferable.test.js @@ -59,6 +59,20 @@ const createTestSuite = ({ contract, constructorArgs }) => await this.erc721aBatchTransferable['safeMint(address,uint256)'](this.addr3.address, 7); }); + it('test safe batch transfer with data', async function () { + const transferFn = 'safeBatchTransferFrom(address,address,uint256[],bytes)'; + const tokensToTransfer = this.addr1.expected.tokens; + await expect(this.erc721aBatchTransferable.connect(this.addr1)[transferFn]( + this.addr1.address, this.receiver.address, tokensToTransfer, '0x01' + )).to.be.revertedWith('reverted in the receiver contract!'); + }); + + it('batch transfer nothing', async function () { + const transferFn = 'safeBatchTransferFrom(address,address,uint256[])'; + await this.erc721aBatchTransferable.connect(this.addr1)[transferFn]( + this.addr1.address, this.receiver.address, []); + }); + context('test batch transfer functionality', function () { const testSuccessfulBatchTransfer = function (transferFn, transferToContract = true) { describe('successful transfers', async function () { @@ -93,8 +107,8 @@ const createTestSuite = ({ contract, constructorArgs }) => // Transfer part of uninitialized tokens this.tokensToTransferAlt = [25, 26, 27]; this.transferTxAlt = await this.erc721aBatchTransferable.connect(this.addr3)[transferFn]( - this.addr3.address, this.addr5.address, this.tokensToTransferAlt - ); + this.addr3.address, this.addr5.address, this.tokensToTransferAlt + ); }); it('emits Transfers event', async function () { @@ -383,14 +397,18 @@ const createTestSuite = ({ contract, constructorArgs }) => await this.erc721aBatchTransferable.connect(this.addr2).setApprovalForAll(this.sender.address, true); await expect( this.erc721aBatchTransferable - .connect(this.sender)['directBatchTransferFrom']( + .connect(this.sender)['directBatchTransferFrom(address,address,address,uint256[])']( ZERO_ADDRESS, ZERO_ADDRESS, this.sender.address, this.tokenIds ) ).to.be.revertedWith('TransferFromIncorrectOwner'); this.erc721aBatchTransferable - .connect(this.sender)['directBatchTransferFrom']( + .connect(this.sender)['directBatchTransferFrom(address,address,address,uint256[])']( ZERO_ADDRESS, this.addr2.address, this.sender.address, this.tokenIds ); + this.erc721aBatchTransferable + .connect(this.addr2)['directBatchTransferFrom(address,address,uint256[])']( + this.sender.address, this.addr2.address, this.tokenIds + ); }); it('rejects transfer to zero address', async function () { @@ -478,6 +496,7 @@ const createTestSuite = ({ contract, constructorArgs }) => testApproveBatchTransfer(fn, false); }); }); + context('safeBatchTransferFrom', function (fn = 'safeBatchTransferFrom(address,address,uint256[])') { describe('to contract', function () { testSuccessfulBatchTransfer(fn);