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.

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

Use the _update mechanism in ERC721 #4377

Merged
merged 53 commits into from
Aug 9, 2023
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
53 commits
Select commit Hold shift + click to select a range
a3526ac
Rebase ERC721._update on top of next-v5
Amxx Apr 27, 2023
1ed8f9e
use __unsafe_increaseBalance to react to batch minting
Amxx Jun 21, 2023
7ec3435
Apply suggestions from code review
Amxx Jun 21, 2023
e2fdbac
fix lint
Amxx Jun 21, 2023
e9f03bd
Exclude address(0) in ERC721._isApprovedOrOwner
frangio Jun 30, 2023
78c280b
Merge branch 'master' into refactor/erc721-update-fnPointer
Amxx Jun 30, 2023
1cc7f54
Merge remote-tracking branch 'upstream' into refactor/erc721-update-f…
Amxx Jul 3, 2023
c7303ec
fix lint
Amxx Jul 3, 2023
54cb3ca
Merge branch 'master' into refactor/erc721-update-fnPointer
Amxx Jul 3, 2023
562ddf5
implement hybrid _update
Amxx Jul 5, 2023
0bb98cb
Merge branch 'master' into feature/Governor-storage
Amxx Jul 7, 2023
5ab254c
lint
Amxx Jul 7, 2023
bd0c52e
refactor constraint into an optionalChecks bitmap
Amxx Jul 11, 2023
1a95520
replace constraints with a simple operator check
Amxx Jul 11, 2023
7e9d024
Apply suggestions from code review
Amxx Jul 12, 2023
16f2f15
remove _isApproedOrOwner in favor of _isApproved + refactor _checkOnE…
Amxx Jul 12, 2023
2558c8f
change _increaseBalance type to uint128
Amxx Jul 12, 2023
de570d0
allow using approve/_approve to clean approval
Amxx Jul 12, 2023
7121ff7
Merge branch 'erc721-approve-0' into refactor/erc721-update-fnPointer
Amxx Jul 12, 2023
b973d98
changesets
Amxx Jul 12, 2023
e4b0e72
use whenNotPaused in ERC721Pausable
Amxx Jul 12, 2023
4516803
make the safe function without a data field non virtual
Amxx Jul 12, 2023
7c3f161
Update .changeset/eighty-lemons-shake.md
frangio Jul 12, 2023
9ba0120
Format _increaseBalance NatSpec
ernestognw Jul 13, 2023
1081508
Lint
ernestognw Jul 13, 2023
fb4d951
Apply suggestions from code review
Amxx Jul 13, 2023
d7a6aaf
remove _exists
Amxx Jul 13, 2023
4c25b48
Merge branch 'refactor/erc721-update-fnPointer' of https://github.com…
Amxx Jul 13, 2023
20048ca
Changes suggested in the PR discussions
Amxx Jul 13, 2023
e996ba4
add ERC721 specific details in the 'How to upgrade from 4.x' section …
Amxx Jul 13, 2023
b29e573
rename from → previousOwner
Amxx Jul 13, 2023
328b16b
Authorised → Authorized
Amxx Jul 13, 2023
08da709
refactor _checkAuhtorized
Amxx Jul 13, 2023
12f63b3
add test
Amxx Jul 13, 2023
81aca96
Update CHANGELOG.md
frangio Jul 13, 2023
d037530
Apply suggestions from code review
frangio Jul 13, 2023
5ce49a4
remove unnecessary solhint annotation
frangio Jul 13, 2023
a023cad
wrap long line
frangio Jul 13, 2023
caabbf3
improve warnings and notes
frangio Jul 13, 2023
ca32b45
fix _safeTransfer docs
frangio Jul 13, 2023
b982e2a
Update ERC721.behavior.js
Amxx Jul 14, 2023
f404802
Update ERC721.sol
Amxx Jul 14, 2023
20bb47f
Update contracts/token/ERC721/ERC721.sol
Amxx Jul 14, 2023
a475ffa
Update ERC721.sol
Amxx Jul 14, 2023
e26d5c0
Update IERC721.sol
Amxx Jul 14, 2023
2897abc
Update ERC721.sol
Amxx Jul 14, 2023
52923d1
coverage for internal _transfer and _safeTransfer
Amxx Aug 4, 2023
42e17ee
mint
Amxx Aug 4, 2023
c2e1a55
fix comments _isApproved -> _isAuthorized
frangio Aug 9, 2023
a036284
extend warning for _isAuthorized
frangio Aug 9, 2023
1e4f353
add comment to _approve
frangio Aug 9, 2023
7b26030
Update contracts/token/ERC721/ERC721.sol
frangio Aug 9, 2023
7249414
Update contracts/token/ERC721/ERC721.sol
frangio Aug 9, 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
28 changes: 11 additions & 17 deletions contracts/mocks/token/ERC721ConsecutiveEnumerableMock.sol
Original file line number Diff line number Diff line change
Expand Up @@ -28,25 +28,19 @@ contract ERC721ConsecutiveEnumerableMock is ERC721Consecutive, ERC721Enumerable
return super._ownerOf(tokenId);
}

function _mint(address to, uint256 tokenId) internal virtual override(ERC721, ERC721Consecutive) {
super._mint(to, tokenId);
}

function _beforeTokenTransfer(
address from,
function _update(
address to,
uint256 firstTokenId,
uint256 batchSize
) internal virtual override(ERC721, ERC721Enumerable) {
super._beforeTokenTransfer(from, to, firstTokenId, batchSize);
uint256 tokenId,
function(address, address, uint256) view constraints
) internal virtual override(ERC721Consecutive, ERC721Enumerable) returns (address) {
return super._update(to, tokenId, constraints);
}

function _afterTokenTransfer(
address from,
address to,
uint256 firstTokenId,
uint256 batchSize
) internal virtual override(ERC721, ERC721Consecutive) {
super._afterTokenTransfer(from, to, firstTokenId, batchSize);
// solhint-disable-next-line func-name-mixedcase
Amxx marked this conversation as resolved.
Show resolved Hide resolved
Amxx marked this conversation as resolved.
Show resolved Hide resolved
function __unsafe_increaseBalance(
address account,
uint256 amount
) internal virtual override(ERC721, ERC721Enumerable) {
super.__unsafe_increaseBalance(account, amount);
}
}
25 changes: 8 additions & 17 deletions contracts/mocks/token/ERC721ConsecutiveMock.sol
Original file line number Diff line number Diff line change
Expand Up @@ -41,26 +41,17 @@ contract ERC721ConsecutiveMock is ERC721Consecutive, ERC721Pausable, ERC721Votes
return super._ownerOf(tokenId);
}

function _mint(address to, uint256 tokenId) internal virtual override(ERC721, ERC721Consecutive) {
super._mint(to, tokenId);
}

function _beforeTokenTransfer(
address from,
function _update(
address to,
uint256 firstTokenId,
uint256 batchSize
) internal virtual override(ERC721, ERC721Pausable) {
super._beforeTokenTransfer(from, to, firstTokenId, batchSize);
uint256 tokenId,
function(address, address, uint256) view constraints
) internal virtual override(ERC721Consecutive, ERC721Pausable, ERC721Votes) returns (address) {
return super._update(to, tokenId, constraints);
}

function _afterTokenTransfer(
address from,
address to,
uint256 firstTokenId,
uint256 batchSize
) internal virtual override(ERC721, ERC721Votes, ERC721Consecutive) {
super._afterTokenTransfer(from, to, firstTokenId, batchSize);
// solhint-disable-next-line func-name-mixedcase
ernestognw marked this conversation as resolved.
Show resolved Hide resolved
function __unsafe_increaseBalance(address account, uint256 amount) internal virtual override(ERC721, ERC721Votes) {
super.__unsafe_increaseBalance(account, amount);
}
}

Expand Down
194 changes: 82 additions & 112 deletions contracts/token/ERC721/ERC721.sol
frangio marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -151,12 +151,14 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er
/**
* @dev See {IERC721-transferFrom}.
*/
function transferFrom(address from, address to, uint256 tokenId) public virtual {
if (!_isApprovedOrOwner(_msgSender(), tokenId)) {
revert ERC721InsufficientApproval(_msgSender(), tokenId);
function transferFrom(address from, address to, uint256 tokenId) public virtual override {
Amxx marked this conversation as resolved.
Show resolved Hide resolved
if (to == address(0)) {
revert ERC721InvalidReceiver(address(0));
}
address owner = _update(to, tokenId, _constraintApprovedOrOwner);
if (owner != from) {
revert ERC721IncorrectOwner(from, tokenId, owner);
frangio marked this conversation as resolved.
Show resolved Hide resolved
}

_transfer(from, to, tokenId);
}

/**
Expand Down Expand Up @@ -257,6 +259,46 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er
}
}

/**
* @dev Transfers `tokenId` from its current owner to `to`, or alternatively mints (or burns) if the current owner
* (or `to`) is the zero address.
Amxx marked this conversation as resolved.
Show resolved Hide resolved
*
* The `constraints` argument is used to specify constraints, and eventually revert. For example this can be used
* to ensure that the current owner is what was expected.

* All customizations to transfers, mints, and burns should be done by overriding this function.
*
* Emits a {Transfer} event.
*/
function _update(
address to,
uint256 tokenId,
function(address, address, uint256) view constraints
) internal virtual returns (address) {
address from = _ownerOf(tokenId);

constraints(from, to, tokenId);

if (from != address(0)) {
delete _tokenApprovals[tokenId];
unchecked {
frangio marked this conversation as resolved.
Show resolved Hide resolved
_balances[from] -= 1;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
_balances[from] -= 1;
--_balances[from];

Copy link
Member

Choose a reason for hiding this comment

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

Alternatively:

Suggested change
_balances[from] -= 1;
_balances[from]--;

Copy link
Contributor

Choose a reason for hiding this comment

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

I personally prefer -= 1 and += 1. @Amxx what is your preference here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

+= 1 / -= 1 is what we currently have, and I kept it. If I was to write the contract from scratch today, I would use -- and ++, but I don't have a strong preference.

Copy link
Member

Choose a reason for hiding this comment

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

We agreed to keep it as it is because it's not a highly meaningful change. We might reconsider once this PR is done so we can correctly measure the gas savings.

Keeping this convo open and closing the other one.

}
}

if (to != address(0)) {
unchecked {
_balances[to] += 1;
ernestognw marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not increaseBalance(…) btw? It may be logical again to use a function that can be overridden (then a similar point with a decrease). But of course it can cost extra gas if it doesn't inline

Copy link
Contributor

@frangio frangio Aug 31, 2023

Choose a reason for hiding this comment

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

If we use _increaseBalance here, there is another risk regarding overrides, which is that an extension overriding both _update and _increaseBalance may end up executing its logic twice. See ERC721Votes for example, which overrides both in order to use _transferVotingUnits.

To avoid that we could add a _decreaseBalance function, but we wanted to reduce the number of needed overrides, not increase it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe then give the function a name that more specifically reflects its purpose?

Copy link
Contributor

Choose a reason for hiding this comment

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

Which one do you mean? _update?

Copy link
Contributor

Choose a reason for hiding this comment

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

Which one do you mean? _update?

_increaseBalance
Maybe like _increaseBalanceUnsafe, _increaseBalanceDirectly, _increaseBalanceManually or smth

}
}

_owners[tokenId] = to;

emit Transfer(from, to, tokenId);

return from;
}

/**
* @dev Mints `tokenId` and transfers it to `to`.
*
Expand All @@ -269,34 +311,11 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er
*
* Emits a {Transfer} event.
*/
function _mint(address to, uint256 tokenId) internal virtual {
function _mint(address to, uint256 tokenId) internal {
if (to == address(0)) {
revert ERC721InvalidReceiver(address(0));
}
if (_exists(tokenId)) {
ernestognw marked this conversation as resolved.
Show resolved Hide resolved
revert ERC721InvalidSender(address(0));
}

_beforeTokenTransfer(address(0), to, tokenId, 1);

// Check that tokenId was not minted by `_beforeTokenTransfer` hook
if (_exists(tokenId)) {
revert ERC721InvalidSender(address(0));
}

unchecked {
// Will not overflow unless all 2**256 token ids are minted to the same owner.
// Given that tokens are minted one by one, it is impossible in practice that
// this ever happens. Might change if we allow batch minting.
// The ERC fails to describe this case.
_balances[to] += 1;
}

_owners[tokenId] = to;

emit Transfer(address(0), to, tokenId);

_afterTokenTransfer(address(0), to, tokenId, 1);
_update(to, tokenId, _constraintNotMinted);
}

/**
Expand All @@ -310,26 +329,8 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er
*
* Emits a {Transfer} event.
*/
function _burn(uint256 tokenId) internal virtual {
address owner = ownerOf(tokenId);

_beforeTokenTransfer(owner, address(0), tokenId, 1);

// Update ownership in case tokenId was transferred by `_beforeTokenTransfer` hook
owner = ownerOf(tokenId);

// Clear approvals
delete _tokenApprovals[tokenId];

// Decrease balance with checked arithmetic, because an `ownerOf` override may
// invalidate the assumption that `_balances[from] >= 1`.
_balances[owner] -= 1;

delete _owners[tokenId];

emit Transfer(owner, address(0), tokenId);

_afterTokenTransfer(owner, address(0), tokenId, 1);
function _burn(uint256 tokenId) internal {
_update(address(0), tokenId, _constraintMinted);
}

/**
Expand All @@ -343,41 +344,14 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er
*
* Emits a {Transfer} event.
*/
function _transfer(address from, address to, uint256 tokenId) internal virtual {
address owner = ownerOf(tokenId);
if (owner != from) {
revert ERC721IncorrectOwner(from, tokenId, owner);
}
function _transfer(address from, address to, uint256 tokenId) internal {
Amxx marked this conversation as resolved.
Show resolved Hide resolved
if (to == address(0)) {
revert ERC721InvalidReceiver(address(0));
}

_beforeTokenTransfer(from, to, tokenId, 1);

// Check that tokenId was not transferred by `_beforeTokenTransfer` hook
owner = ownerOf(tokenId);
address owner = _update(to, tokenId, _constraintMinted);
if (owner != from) {
revert ERC721IncorrectOwner(from, tokenId, owner);
}
frangio marked this conversation as resolved.
Show resolved Hide resolved

// Clear approvals from the previous owner
delete _tokenApprovals[tokenId];

// Decrease balance with checked arithmetic, because an `ownerOf` override may
// invalidate the assumption that `_balances[from] >= 1`.
_balances[from] -= 1;

unchecked {
// `_balances[to]` could overflow in the conditions described in `_mint`. That would require
// all 2**256 token ids to be minted, which in practice is impossible.
_balances[to] += 1;
}

_owners[tokenId] = to;

emit Transfer(from, to, tokenId);

_afterTokenTransfer(from, to, tokenId, 1);
}

/**
Expand Down Expand Up @@ -412,6 +386,34 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er
}
}

/**
* @dev Constraint: revert if token is already minted
*/
function _constraintNotMinted(address from, address, uint256) internal pure {
if (from != address(0)) {
revert ERC721InvalidSender(address(0));
}
}

/**
* @dev Constraint: revert if token is not yet minted
*/
function _constraintMinted(address from, address, uint256 tokenId) internal pure {
if (from == address(0)) {
revert ERC721NonexistentToken(tokenId);
}
}

/**
* @dev Constraint: check that the caller is the current owner, or approved by the current owner
*/
function _constraintApprovedOrOwner(address owner, address, uint256 tokenId) internal view {
address spender = _msgSender();
if (spender != owner && !isApprovedForAll(owner, spender) && getApproved(tokenId) != spender) {
revert ERC721InsufficientApproval(_msgSender(), tokenId);
}
frangio marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* @dev Private function to invoke {IERC721Receiver-onERC721Received} on a target address.
* The call is not executed if the target address is not a contract.
Expand Down Expand Up @@ -446,38 +448,6 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er
}
}

/**
* @dev Hook that is called before any token transfer. This includes minting and burning. If {ERC721Consecutive} is
* used, the hook may be called as part of a consecutive (batch) mint, as indicated by `batchSize` greater than 1.
*
* Calling conditions:
*
* - When `from` and `to` are both non-zero, ``from``'s tokens will be transferred to `to`.
* - When `from` is zero, the tokens will be minted for `to`.
* - When `to` is zero, ``from``'s tokens will be burned.
* - `from` and `to` are never both zero.
* - `batchSize` is non-zero.
*
* To learn more about hooks, head to xref:ROOT:extending-contracts.adoc#using-hooks[Using Hooks].
*/
function _beforeTokenTransfer(address from, address to, uint256 firstTokenId, uint256 batchSize) internal virtual {}

/**
* @dev Hook that is called after any token transfer. This includes minting and burning. If {ERC721Consecutive} is
* used, the hook may be called as part of a consecutive (batch) mint, as indicated by `batchSize` greater than 1.
*
* Calling conditions:
*
* - When `from` and `to` are both non-zero, ``from``'s tokens were transferred to `to`.
* - When `from` is zero, the tokens were minted for `to`.
* - When `to` is zero, ``from``'s tokens were burned.
* - `from` and `to` are never both zero.
* - `batchSize` is non-zero.
*
* To learn more about hooks, head to xref:ROOT:extending-contracts.adoc#using-hooks[Using Hooks].
*/
function _afterTokenTransfer(address from, address to, uint256 firstTokenId, uint256 batchSize) internal virtual {}

/**
* @dev Unsafe write access to the balances, used by extensions that "mint" tokens using an {ownerOf} override.
*
Expand All @@ -486,7 +456,7 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er
* that `ownerOf(tokenId)` is `a`.
*/
// solhint-disable-next-line func-name-mixedcase
function __unsafe_increaseBalance(address account, uint256 value) internal {
function __unsafe_increaseBalance(address account, uint256 value) internal virtual {
_balances[account] += value;
Amxx marked this conversation as resolved.
Show resolved Hide resolved
}
}
Loading