Skip to content
Merged
Changes from 2 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
34 changes: 19 additions & 15 deletions contracts/token/ERC721/ERC721.sol
Original file line number Diff line number Diff line change
Expand Up @@ -179,8 +179,10 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er
/**
* @dev Returns the owner of the `tokenId`. Does NOT revert if token doesn't exist
*
* WARNING: Any override of this function that allocates token to accounts following a custom logic should go in
* pair with a call to {_increaseBalance} so that balances and ownerships remain consistent with one another.
* IMPORTANT: Any overrides to this function that add ownership of tokens not tracked by the
* core ERC721 logic MUST be matched with the use of {_increaseBalance} to keep balances
* consistent with ownership. The invariant to preserve is that for any address `a` the value returned by
* `balanceOf(a)` must be equal to the number of tokens such that `_ownerOf(tokenId)` is `a`.
*/
function _ownerOf(uint256 tokenId) internal view virtual returns (address) {
return _owners[tokenId];
Expand All @@ -197,20 +199,17 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er
* @dev Returns whether `spender` is allowed to manage `owner`'s tokens, or `tokenId` in
* particular (ignoring whether it is owned by `owner`).
*
* WARNING:
* - ownership is not checked.
* - spender = address(0) will lead to this function returning true in unexpected situations.
* WARNING: This function doesn't check that `owner` is the actual owner of the specified `tokenId`. Moreover, it returns true if `owner == spender` in order to skip the check where needed. Consider checking for cases where `spender == address(0)` since they could lead to unexpected behavior.
*/
function _isApproved(address owner, address spender, uint256 tokenId) internal view virtual returns (bool) {
return owner == spender || isApprovedForAll(owner, spender) || getApproved(tokenId) == spender;
}

/**
* @dev Checks whether `spender` was approved by `owner` to operate on `tokenId` or is the owner of `tokenId`.
* Reverts if `spender` is not approved nor owner of `tokenId`.
* @dev Checks if `spender` can operate on `tokenId`, assuming the provided `owner` is the actual owner.
* Reverts if `spender` has not approval for all assets of the provided `owner` nor the actual owner approved the `spender` for the specific `tokenId`.
*
* WARNING:
* - ownership is not checked.
* WARNING: This function relies on {_isApproved}, so it doesn't check whether `owner` is the actual owner of `tokenId` and will skip the check if `spender == owner`
*/
function _checkApproved(address owner, address spender, uint256 tokenId) internal view virtual {
if (!_isApproved(owner, spender, tokenId)) {
Expand Down Expand Up @@ -279,7 +278,7 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er
}

/**
* @dev Safely mints `tokenId` and transfers it to `to`.
* @dev Mints `tokenId`, transfers it to `to` and checks for `to` acceptance.
*
* Requirements:
*
Expand Down Expand Up @@ -343,8 +342,8 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er
}

/**
* @dev Safely transfers `tokenId` token from `from` to `to`, checking first that contract recipients
* are aware of the ERC721 protocol to prevent tokens from being forever locked.
* @dev Safely transfers `tokenId` token from `from` to `to`, checking that contract recipients
* are aware of the ERC721 standard to prevent tokens from being forever locked.
*
* `data` is additional data, it has no specified format and it is sent in call to `to`.
*
Expand All @@ -361,8 +360,7 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er
* Emits a {Transfer} event.
*/
function _safeTransfer(address from, address to, uint256 tokenId) internal {
_transfer(from, to, tokenId);
_checkOnERC721Received(from, to, tokenId, "");
_safeTransfer(from, to, tokenId, "");
}

/**
Expand Down Expand Up @@ -391,6 +389,10 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er
/**
* @dev Approve `operator` to operate on all of `owner` tokens
*
* Requirements:
* - operator can't be the address zero.
* - owner can't approve himself.
Copy link
Member

Choose a reason for hiding this comment

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

This requirement is not present in the function. I think we'd like to remove the requirement because approving to self is arguably a no-op.

We also need to update in IERC721-setApprovalForAll NatSpec

*
* Emits an {ApprovalForAll} event.
*/
function _setApprovalForAll(address owner, address operator, bool approved) internal virtual {
Expand Down Expand Up @@ -449,6 +451,8 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er
* remain consistent with one another.
*/
function _increaseBalance(address account, uint128 value) internal virtual {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be moved next to _update IMO.

_balances[account] += value;
unchecked {
_balances[account] += value;
}
}
}