-
Notifications
You must be signed in to change notification settings - Fork 12.1k
Use the _update mechanism in ERC721 #4377
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
Changes from 8 commits
a3526ac
1ed8f9e
7ec3435
e2fdbac
e9f03bd
78c280b
1cc7f54
c7303ec
54cb3ca
562ddf5
0bb98cb
5ab254c
bd0c52e
1a95520
7e9d024
16f2f15
2558c8f
de570d0
7121ff7
b973d98
e4b0e72
4516803
7c3f161
9ba0120
1081508
fb4d951
d7a6aaf
4c25b48
20048ca
e996ba4
b29e573
328b16b
08da709
12f63b3
81aca96
d037530
5ce49a4
a023cad
caabbf3
ca32b45
b982e2a
f404802
20bb47f
a475ffa
e26d5c0
2897abc
52923d1
42e17ee
c2e1a55
a036284
1e4f353
7b26030
7249414
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
frangio marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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); | ||||||||||
} | ||||||||||
|
||||||||||
/** | ||||||||||
|
@@ -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; | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Alternatively:
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I personally prefer There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we use To avoid that we could add a There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Which one do you mean? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||||||
} | ||||||||||
} | ||||||||||
|
||||||||||
_owners[tokenId] = to; | ||||||||||
|
||||||||||
emit Transfer(from, to, tokenId); | ||||||||||
|
||||||||||
return from; | ||||||||||
} | ||||||||||
|
||||||||||
/** | ||||||||||
* @dev Mints `tokenId` and transfers it to `to`. | ||||||||||
* | ||||||||||
|
@@ -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); | ||||||||||
} | ||||||||||
|
||||||||||
/** | ||||||||||
|
@@ -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); | ||||||||||
} | ||||||||||
|
||||||||||
/** | ||||||||||
|
@@ -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); | ||||||||||
} | ||||||||||
|
||||||||||
/** | ||||||||||
|
@@ -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. | ||||||||||
|
@@ -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. | ||||||||||
* | ||||||||||
|
@@ -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
|
||||||||||
} | ||||||||||
} |
Uh oh!
There was an error while loading. Please reload this page.