Skip to content

Refactor ERC721 to use _update pattern #4419

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

Closed
frangio wants to merge 8 commits into from
Closed

Refactor ERC721 to use _update pattern #4419

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2c9f7bb
Refactor ERC721 to use _update pattern
frangio Jul 4, 2023
930ee64
Update contracts/token/ERC721/ERC721.sol
frangio Jul 7, 2023
14ffe4d
Apply suggestions
ernestognw Jul 7, 2023
63717fc
Rename updateBalance to increaseBalance
ernestognw Jul 7, 2023
5f05fc9
Fix tests (omg hopefully)
ernestognw Jul 7, 2023
38c0bf0
Add changeset
ernestognw Jul 7, 2023
ca05bc5
Use _increaseBalance in _update
ernestognw Jul 7, 2023
cdacda1
Rollback _increaseBalance in _update
ernestognw Jul 7, 2023
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
Prev Previous commit
Next Next commit
Fix tests (omg hopefully)
  • Loading branch information
@ernestognw
ernestognw committed Jul 7, 2023
commit 5f05fc9ba126740fc5caf1a3fc363c56aa3f0a5c
2 changes: 1 addition & 1 deletion contracts/token/ERC721/ERC721.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,7 +328,7 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er
}

/**
* @dev Updates the ownership of toke with id `tokenId` from `from` to `to`.
* @dev Updates the ownership of token with id `tokenId` from `from` to `to`.
* It clears the approval for the token when is transferred.
*
* Requirements:
Expand Down
2 changes: 1 addition & 1 deletion contracts/token/ERC721/extensions/ERC721Consecutive.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ abstract contract ERC721Consecutive is IERC2309, ERC721 {
* during construction, and burning of tokens that have been sequentially minted are complete.
*/
function _update(address from, address to, uint256 tokenId) internal virtual override {
if (to == address(0) && address(this).code.length == 0) {
if (from == address(0) && address(this).code.length == 0) {
revert ERC721ForbiddenMint();
}

Expand Down
8 changes: 4 additions & 4 deletions contracts/token/ERC721/extensions/ERC721Enumerable.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,8 @@ abstract contract ERC721Enumerable is ERC721, IERC721Enumerable {
* @dev See {ERC721-_beforeTokenTransfer}.
*/
function _update(address from, address to, uint256 tokenId) internal virtual override {
super._update(from, to, tokenId);

if (from == address(0)) {
_addTokenToAllTokensEnumeration(tokenId);
} else if (from != to) {
Expand All @@ -87,8 +89,6 @@ abstract contract ERC721Enumerable is ERC721, IERC721Enumerable {
} else if (to != from) {
_addTokenToOwnerEnumeration(to, tokenId);
}

super._update(from, to, tokenId);
}

/**
Expand All @@ -97,7 +97,7 @@ abstract contract ERC721Enumerable is ERC721, IERC721Enumerable {
* @param tokenId uint256 ID of the token to be added to the tokens list of the given address
*/
function _addTokenToOwnerEnumeration(address to, uint256 tokenId) private {
uint256 length = balanceOf(to);
uint256 length = balanceOf(to) - 1; // Balance is already increased by super._update
_ownedTokens[to][length] = tokenId;
_ownedTokensIndex[tokenId] = length;
}
Expand All @@ -123,7 +123,7 @@ abstract contract ERC721Enumerable is ERC721, IERC721Enumerable {
// To prevent a gap in from's tokens array, we store the last token in the index of the token to delete, and
// then delete the last slot (swap and pop).

uint256 lastTokenIndex = balanceOf(from) - 1;
uint256 lastTokenIndex = balanceOf(from); // Balance is already decreased by super._update
uint256 tokenIndex = _ownedTokensIndex[tokenId];

// When the token to delete is the last token, the swap operation is unnecessary
Expand Down
6 changes: 1 addition & 5 deletions contracts/token/ERC721/extensions/ERC721Pausable.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,7 @@ abstract contract ERC721Pausable is ERC721, Pausable {
*
* - the contract must not be paused.
*/
function _update(
address from,
address to,
uint256 tokenId
) internal virtual override {
function _update(address from, address to, uint256 tokenId) internal virtual override {
_requireNotPaused();
Copy link
Member

@ernestognw ernestognw Jul 6, 2023

Choose a reason for hiding this comment

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

I think it's a good moment to use whenNotPaused instead as suggested here

Copy link
Contributor Author

@frangio frangio Jul 7, 2023

Choose a reason for hiding this comment

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

Personally I prefer the function call. They are functionally equivalent but the function call is visible inside the function, I just find it more explicit. I don't know if everyone feels the same though.

Copy link
Member

@ernestognw ernestognw Jul 7, 2023

Choose a reason for hiding this comment

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

I prefer whenNotPaused precisely because it's less explicit (== less pervasive?).

I'm fine with the function call if we agree to keep consistency between this and ERC1155Pausable where we use the modifier instead.

super._update(from, to, tokenId);
}
Expand Down
10 changes: 8 additions & 2 deletions test/token/ERC721/ERC721.behavior.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -705,7 +705,11 @@ function shouldBehaveLikeERC721(owner, newOwner, approved, anotherApproved, oper
});

it('reverts when adding a token id that already exists', async function () {
await expectRevertCustomError(this.token.$_mint(owner, firstTokenId), 'ERC721IncorrectOwner', [ZERO_ADDRESS, firstTokenId, owner]);
await expectRevertCustomError(this.token.$_mint(owner, firstTokenId), 'ERC721IncorrectOwner', [
ZERO_ADDRESS,
firstTokenId,
owner,
]);
});
});
});
Expand Down Expand Up @@ -738,7 +742,9 @@ function shouldBehaveLikeERC721(owner, newOwner, approved, anotherApproved, oper
});

it('reverts when burning a token id that has been deleted', async function () {
await expectRevertCustomError(this.token.$_burn(owner, firstTokenId), 'ERC721NonexistentToken', [firstTokenId]);
await expectRevertCustomError(this.token.$_burn(owner, firstTokenId), 'ERC721NonexistentToken', [
firstTokenId,
]);
});
});
});
Expand Down
17 changes: 10 additions & 7 deletions test/token/ERC721/extensions/ERC721Burnable.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,18 +63,21 @@ contract('ERC721Burnable', function (accounts) {

describe('when there is no previous approval burned', function () {
it('reverts', async function () {
await expectRevertCustomError(this.token.burn(owner, tokenId, { from: another }), 'ERC721InsufficientApproval', [
another,
tokenId,
]);
await expectRevertCustomError(
this.token.burn(owner, tokenId, { from: another }),
'ERC721InsufficientApproval',
[another, tokenId],
);
});
});

describe('when the given token ID was not tracked by this contract', function () {
it('reverts', async function () {
await expectRevertCustomError(this.token.burn(owner, unknownTokenId, { from: owner }), 'ERC721NonexistentToken', [
unknownTokenId,
]);
await expectRevertCustomError(
this.token.burn(owner, unknownTokenId, { from: owner }),
'ERC721NonexistentToken',
[unknownTokenId],
);
});
});
});
Expand Down
14 changes: 5 additions & 9 deletions test/token/ERC721/extensions/ERC721Consecutive.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,11 @@ contract('ERC721Consecutive', function (accounts) {

expect(await this.token.$_exists(tokenId)).to.be.equal(true);

await expectRevertCustomError(this.token.$_mint(user1, tokenId), 'ERC721InvalidSender', [ZERO_ADDRESS]);
await expectRevertCustomError(this.token.$_mint(user1, tokenId), 'ERC721IncorrectOwner', [
ZERO_ADDRESS,
tokenId,
await this.token.ownerOf(tokenId),
]);
});
});

Expand Down Expand Up @@ -201,14 +205,6 @@ contract('ERC721Consecutive', function (accounts) {
);
});

it('cannot use single minting during construction', async function () {
await expectRevertCustomError(
ERC721ConsecutiveNoConstructorMintMock.new(name, symbol),
'ERC721ForbiddenMint',
[],
);
});

it('consecutive mint not compatible with enumerability', async function () {
await expectRevertCustomError(
ERC721ConsecutiveEnumerableMock.new(
Expand Down