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 1 commit
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
32 changes: 32 additions & 0 deletions contracts/ERC721A.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -405,6 +405,38 @@ contract ERC721A is Context, ERC165, IERC721, IERC721Metadata {
_afterTokenTransfers(address(0), to, startTokenId, quantity);
}

/**
* @dev Equivalent to _mint(to, quantity, '', false).
*/
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
4 changes: 4 additions & 0 deletions contracts/mocks/ERC721AMock.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,10 @@ contract ERC721AMock is ERC721A {
_mint(to, quantity, _data, safe);
}

function slimMint(address to, uint256 quantity) public {
_mint(to, quantity);
}

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

describe('mint', function () {
const data = '0x42';
const subTests = function (description, isSlim) {
describe(description, async function () {
const mint = async function (to, quantity) {
if (isSlim) {
return await this.erc721a.slimMint(to, quantity);
} else {
return await this.erc721a.mint(to, quantity, '0x42', false);
}
};

it('successfully mints a single token', async function () {
const mintTx = await mint.call(this, this.receiver.address, 1);
await expect(mintTx)
.to.emit(this.erc721a, 'Transfer')
.withArgs(ZERO_ADDRESS, this.receiver.address, this.startTokenId);
await expect(mintTx).to.not.emit(this.receiver, 'Received');
expect(await this.erc721a.ownerOf(this.startTokenId))
.to.equal(this.receiver.address);
});

it('successfully mints a single token', async function () {
const mintTx = await this.erc721a.mint(this.receiver.address, 1, data, false);
await expect(mintTx)
.to.emit(this.erc721a, 'Transfer')
.withArgs(ZERO_ADDRESS, this.receiver.address, this.startTokenId);
await expect(mintTx).to.not.emit(this.receiver, 'Received');
expect(await this.erc721a.ownerOf(this.startTokenId)).to.equal(this.receiver.address);
});
it('successfully mints multiple tokens', async function () {
const mintTx = await mint.call(this, this.receiver.address, 5);
for (let tokenId = this.startTokenId; tokenId < 5 + this.startTokenId; tokenId++) {
await expect(mintTx)
.to.emit(this.erc721a, 'Transfer')
.withArgs(ZERO_ADDRESS, this.receiver.address, tokenId);
await expect(mintTx).to.not.emit(this.receiver, 'Received');
expect(await this.erc721a.ownerOf(tokenId))
.to.equal(this.receiver.address);
}
});

it('successfully mints multiple tokens', async function () {
const mintTx = await this.erc721a.mint(this.receiver.address, 5, data, false);
for (let tokenId = this.startTokenId; tokenId < 5 + this.startTokenId; tokenId++) {
await expect(mintTx)
.to.emit(this.erc721a, 'Transfer')
.withArgs(ZERO_ADDRESS, this.receiver.address, tokenId);
await expect(mintTx).to.not.emit(this.receiver, 'Received');
expect(await this.erc721a.ownerOf(tokenId)).to.equal(this.receiver.address);
}
});
it('does not revert for non-receivers', async function () {
const nonReceiver = this.erc721a;
await mint.call(this, nonReceiver.address, 1);
expect(await this.erc721a.ownerOf(this.startTokenId))
.to.equal(nonReceiver.address);
});

it('does not revert for non-receivers', async function () {
const nonReceiver = this.erc721a;
await this.erc721a.mint(nonReceiver.address, 1, data, false);
expect(await this.erc721a.ownerOf(this.startTokenId)).to.equal(nonReceiver.address);
});
it('rejects mints to the zero address', async function () {
await expect(mint.call(this, ZERO_ADDRESS, 1))
.to.be.revertedWith('MintToZeroAddress');
});

it('rejects mints to the zero address', async function () {
await expect(this.erc721a.mint(ZERO_ADDRESS, 1, data, false)).to.be.revertedWith('MintToZeroAddress');
});
it('requires quantity to be greater than 0', async function () {
await expect(mint.call(this, this.owner.address, 0))
.to.be.revertedWith('MintZeroQuantity');
});
});
};

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');
});
subTests('_mint(address,uint256,bytes,bool)', false);

subTests('_mint(address,uint256)', true);
});
});
});
Expand Down