Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add _mint(to, quantity) function #235

Merged
merged 3 commits into from
Apr 16, 2022
Merged

Add _mint(to, quantity) function #235

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c832dc5
Added slim _mint + tests
Vectorized Apr 12, 2022
d8e7b90
Update GasReporterMock to use slim _mint
Vectorized Apr 15, 2022
da726e4
Refactor _safeMint and _mint
Vectorized Apr 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 46 additions & 22 deletions contracts/ERC721A.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,6 +329,9 @@ contract ERC721A is Context, ERC165, IERC721, IERC721Metadata {
return _startTokenId() <= tokenId && tokenId < _currentIndex && !_ownerships[tokenId].burned;
}

/**
* @dev Equivalent to `_safeMint(to, quantity, '')`.
*/
function _safeMint(address to, uint256 quantity) internal {
_safeMint(to, quantity, '');
}
Expand All @@ -338,7 +341,8 @@ contract ERC721A is Context, ERC165, IERC721, IERC721Metadata {
*
* Requirements:
*
* - If `to` refers to a smart contract, it must implement {IERC721Receiver-onERC721Received}, which is called for each safe transfer.
* - If `to` refers to a smart contract, it must implement
* {IERC721Receiver-onERC721Received}, which is called for each safe transfer.
* - `quantity` must be greater than 0.
*
* Emits a {Transfer} event.
Expand All @@ -347,25 +351,6 @@ contract ERC721A is Context, ERC165, IERC721, IERC721Metadata {
address to,
uint256 quantity,
bytes memory _data
) internal {
_mint(to, quantity, _data, true);
}

/**
* @dev Mints `quantity` tokens and transfers them to `to`.
*
* Requirements:
*
* - `to` cannot be the zero address.
* - `quantity` must be greater than 0.
*
* Emits a {Transfer} event.
*/
function _mint(
address to,
uint256 quantity,
bytes memory _data,
bool safe
) internal {
uint256 startTokenId = _currentIndex;
if (to == address(0)) revert MintToZeroAddress();
Expand All @@ -386,7 +371,7 @@ contract ERC721A is Context, ERC165, IERC721, IERC721Metadata {
uint256 updatedIndex = startTokenId;
uint256 end = updatedIndex + quantity;

if (safe && to.isContract()) {
if (to.isContract()) {
do {
emit Transfer(address(0), to, updatedIndex);
if (!_checkContractOnERC721Received(address(0), to, updatedIndex++, _data)) {
Expand All @@ -405,6 +390,45 @@ contract ERC721A is Context, ERC165, IERC721, IERC721Metadata {
_afterTokenTransfers(address(0), to, startTokenId, quantity);
}

/**
* @dev Mints `quantity` tokens and transfers them to `to`.
*
* Requirements:
*
* - `to` cannot be the zero address.
* - `quantity` must be greater than 0.
*
* Emits a {Transfer} event.
*/
function _mint(address to, uint256 quantity) internal {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we just rename the existing mint function to _safeMint and remove the safe parameter? This will be backwards incompatible, but will make the code cleaner since I don't see ppl calling _mint(to, qty,'', false) going forward

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree to remove the safe parameter and rename _safeMint.
For backward compatibility, we could add back _mint(to, qty, data, safe) and then call _mint and _safeMint based on the safe parameter, but it cost maintainability.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m just worried about the customers calling the ERC721A helpdesk if we break backwards compatibility. Lemme think for a while.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well we've broken it a couple times already. I'm ok cutting a v4 of ERC721A for a change like that

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Outside rn, brb in a few hours probably

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code is neater, about 50 gas saved. Let’s go.

uint256 startTokenId = _currentIndex;
if (to == address(0)) revert MintToZeroAddress();
if (quantity == 0) revert MintZeroQuantity();

_beforeTokenTransfers(address(0), to, startTokenId, quantity);

// Overflows are incredibly unrealistic.
// balance or numberMinted overflow if current value of either + quantity > 1.8e19 (2**64) - 1
// updatedIndex overflows if _currentIndex + quantity > 1.2e77 (2**256) - 1
unchecked {
_addressData[to].balance += uint64(quantity);
_addressData[to].numberMinted += uint64(quantity);

_ownerships[startTokenId].addr = to;
_ownerships[startTokenId].startTimestamp = uint64(block.timestamp);

uint256 updatedIndex = startTokenId;
uint256 end = updatedIndex + quantity;

do {
emit Transfer(address(0), to, updatedIndex++);
} while (updatedIndex != end);

_currentIndex = updatedIndex;
}
_afterTokenTransfers(address(0), to, startTokenId, quantity);
}

/**
* @dev Transfers `tokenId` from `from` to `to`.
*
Expand Down Expand Up @@ -466,7 +490,7 @@ contract ERC721A is Context, ERC165, IERC721, IERC721Metadata {
}

/**
* @dev This is equivalent to _burn(tokenId, false)
* @dev Equivalent to `_burn(tokenId, false)`.
*/
function _burn(uint256 tokenId) internal virtual {
_burn(tokenId, false);
Expand Down
4 changes: 2 additions & 2 deletions contracts/mocks/ERC721AGasReporterMock.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,14 @@ contract ERC721AGasReporterMock is ERC721A {
}

function mintOne(address to) public {
_mint(to, 1, '', false);
_mint(to, 1);
}

function safeMintTen(address to) public {
_safeMint(to, 10);
}

function mintTen(address to) public {
_mint(to, 10, '', false);
_mint(to, 10);
}
}
15 changes: 3 additions & 12 deletions contracts/mocks/ERC721AMock.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,21 +36,12 @@ contract ERC721AMock is ERC721A {
_safeMint(to, quantity);
}

function safeMint(
address to,
uint256 quantity,
bytes memory _data
) public {
function safeMint(address to, uint256 quantity, bytes memory _data) public {
_safeMint(to, quantity, _data);
}

function mint(
address to,
uint256 quantity,
bytes memory _data,
bool safe
) public {
_mint(to, quantity, _data, safe);
function mint(address to, uint256 quantity) public {
_mint(to, quantity);
}

function burn(uint256 tokenId, bool approvalCheck) public {
Expand Down
12 changes: 5 additions & 7 deletions test/ERC721A.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,10 +318,8 @@ const createTestSuite = ({ contract, constructorArgs }) =>
});

describe('mint', function () {
const data = '0x42';

it('successfully mints a single token', async function () {
const mintTx = await this.erc721a.mint(this.receiver.address, 1, data, false);
const mintTx = await this.erc721a.mint(this.receiver.address, 1);
await expect(mintTx)
.to.emit(this.erc721a, 'Transfer')
.withArgs(ZERO_ADDRESS, this.receiver.address, this.startTokenId);
Expand All @@ -330,7 +328,7 @@ const createTestSuite = ({ contract, constructorArgs }) =>
});

it('successfully mints multiple tokens', async function () {
const mintTx = await this.erc721a.mint(this.receiver.address, 5, data, false);
const mintTx = await this.erc721a.mint(this.receiver.address, 5);
for (let tokenId = this.startTokenId; tokenId < 5 + this.startTokenId; tokenId++) {
await expect(mintTx)
.to.emit(this.erc721a, 'Transfer')
Expand All @@ -342,16 +340,16 @@ const createTestSuite = ({ contract, constructorArgs }) =>

it('does not revert for non-receivers', async function () {
const nonReceiver = this.erc721a;
await this.erc721a.mint(nonReceiver.address, 1, data, false);
await this.erc721a.mint(nonReceiver.address, 1);
expect(await this.erc721a.ownerOf(this.startTokenId)).to.equal(nonReceiver.address);
});

it('rejects mints to the zero address', async function () {
await expect(this.erc721a.mint(ZERO_ADDRESS, 1, data, false)).to.be.revertedWith('MintToZeroAddress');
await expect(this.erc721a.mint(ZERO_ADDRESS, 1)).to.be.revertedWith('MintToZeroAddress');
});

it('requires quantity to be greater than 0', async function () {
await expect(this.erc721a.mint(this.owner.address, 0, data, false)).to.be.revertedWith('MintZeroQuantity');
await expect(this.erc721a.mint(this.owner.address, 0)).to.be.revertedWith('MintZeroQuantity');
});
});
});
Expand Down