Skip to content

Commit

Permalink
remove fn pointers for constraints in ERC721._update
Browse files Browse the repository at this point in the history
  • Loading branch information
@Amxx
Amxx committed Jul 5, 2023
1 parent 54cb3ca commit e782f4e
Show file tree
Hide file tree
Showing 10 changed files with 67 additions and 91 deletions.
5 changes: 2 additions & 3 deletions contracts/mocks/token/ERC721ConsecutiveEnumerableMock.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,9 @@ contract ERC721ConsecutiveEnumerableMock is ERC721Consecutive, ERC721Enumerable

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

// solhint-disable-next-line func-name-mixedcase
Expand Down
5 changes: 2 additions & 3 deletions contracts/mocks/token/ERC721ConsecutiveMock.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,9 @@ contract ERC721ConsecutiveMock is ERC721Consecutive, ERC721Pausable, ERC721Votes

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

// solhint-disable-next-line func-name-mixedcase
Expand Down
96 changes: 50 additions & 46 deletions contracts/token/ERC721/ERC721.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -155,9 +155,16 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er
if (to == address(0)) {
revert ERC721InvalidReceiver(address(0));
}
address owner = _update(to, tokenId, _constraintApprovedOrOwner);
if (owner != from) {
revert ERC721IncorrectOwner(from, tokenId, owner);

_checkApproved(from, _msgSender(), tokenId);
address previousOwner = _update(to, tokenId);

if (previousOwner != from) {
if (previousOwner == address(0)) {
revert ERC721NonexistentToken(tokenId);
} else {
revert ERC721IncorrectOwner(from, tokenId, previousOwner);
}
}
}

Expand Down Expand Up @@ -223,17 +230,42 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er
}

/**
* @dev Returns whether `spender` is allowed to manage `tokenId`.
* @dev Returns whether `spender` is allowed to manage `tokenId` owned by `owner`.
*
* Requirements:
*
* - `tokenId` must exist.
*/
function _isApprovedOrOwner(address spender, uint256 tokenId) internal view virtual returns (bool) {
address owner = ownerOf(tokenId);
function _isApprovedOrOwner(address owner, address spender, uint256 tokenId) internal view virtual returns (bool) {
return (spender == owner || isApprovedForAll(owner, spender) || getApproved(tokenId) == spender);
}

/**
* @dev Alternative version of {_isApprovedOrOwner} that will fetch the owner from storage.
* This is not virtual, overrides should be performed on the other version of {_isApprovedOrOwner}
*/
function _isApprovedOrOwner(address spender, uint256 tokenId) internal view returns (bool) {
return _isApprovedOrOwner(_ownerOf(tokenId), spender, tokenId);
}

/**
* @dev Check that spender is approved or owner, and revert if that is not the case.
*
* NOTE: If the approval is correct, the owner is not checked. If the `owner` parameter is not directly comming
* from a call to `_ownerOf`, it should be checked. {_update} returns the previousOwner, which should match the
* `owner` for this function. See {transferFrom}.
*/
function _checkApproved(address owner, address spender, uint256 tokenId) internal view virtual {
if (!_isApprovedOrOwner(owner, spender, tokenId)) {
address actualOwner = _ownerOf(tokenId);
if (owner == actualOwner) {
revert ERC721InsufficientApproval(spender, tokenId);
} else {
revert ERC721IncorrectOwner(owner, tokenId, actualOwner);
}
}
}

/**
* @dev Safely mints `tokenId` and transfers it to `to`.
*
Expand Down Expand Up @@ -270,15 +302,9 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er
*
* Emits a {Transfer} event.
*/
function _update(
address to,
uint256 tokenId,
function(address, address, uint256) view constraints
) internal virtual returns (address) {
function _update(address to, uint256 tokenId) internal virtual returns (address previousOwner) {
address from = _ownerOf(tokenId);

constraints(from, to, tokenId);

if (from != address(0)) {
delete _tokenApprovals[tokenId];
unchecked {
Expand Down Expand Up @@ -315,7 +341,10 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er
if (to == address(0)) {
revert ERC721InvalidReceiver(address(0));
}
_update(to, tokenId, _constraintNotMinted);
address previousOwner = _update(to, tokenId);
if (previousOwner != address(0)) {
revert ERC721InvalidSender(address(0));
}
}

/**
Expand All @@ -330,7 +359,10 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er
* Emits a {Transfer} event.
*/
function _burn(uint256 tokenId) internal {
_update(address(0), tokenId, _constraintMinted);
address previousOwner = _update(address(0), tokenId);
if (previousOwner == address(0)) {
revert ERC721NonexistentToken(tokenId);
}
}

/**
Expand All @@ -348,9 +380,9 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er
if (to == address(0)) {
revert ERC721InvalidReceiver(address(0));
}
address owner = _update(to, tokenId, _constraintMinted);
if (owner != from) {
revert ERC721IncorrectOwner(from, tokenId, owner);
address previousOwner = _update(to, tokenId);
if (previousOwner != from) {
revert ERC721IncorrectOwner(from, tokenId, previousOwner);
}
}

Expand Down Expand Up @@ -386,34 +418,6 @@ 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);
}
}

/**
* @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
4 changes: 1 addition & 3 deletions contracts/token/ERC721/extensions/ERC721Burnable.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,7 @@ abstract contract ERC721Burnable is Context, ERC721 {
* - The caller must own `tokenId` or be an approved operator.
*/
function burn(uint256 tokenId) public virtual {
if (!_isApprovedOrOwner(_msgSender(), tokenId)) {
revert ERC721InsufficientApproval(_msgSender(), tokenId);
}
_checkApproved(_ownerOf(tokenId), _msgSender(), tokenId);
_burn(tokenId);
}
}
8 changes: 2 additions & 6 deletions contracts/token/ERC721/extensions/ERC721Consecutive.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -140,12 +140,8 @@ abstract contract ERC721Consecutive is IERC2309, ERC721 {
* Warning: Using {ERC721Consecutive} prevents minting during construction in favor of {_mintConsecutive}.
* After construction, {_mintConsecutive} is no longer available and minting through {_update} becomes available.
*/
function _update(
address to,
uint256 tokenId,
function(address, address, uint256) view constraints
) internal virtual override returns (address) {
address from = super._update(to, tokenId, constraints);
function _update(address to, uint256 tokenId) internal virtual override returns (address) {
address from = super._update(to, tokenId);

// only mint after construction
if (from == address(0) && address(this).code.length == 0) {
Expand Down
8 changes: 2 additions & 6 deletions contracts/token/ERC721/extensions/ERC721Enumerable.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,12 +76,8 @@ abstract contract ERC721Enumerable is ERC721, IERC721Enumerable {
/**
* @dev See {ERC721-_update}.
*/
function _update(
address to,
uint256 tokenId,
function(address, address, uint256) view constraints
) internal virtual override returns (address) {
address from = super._update(to, tokenId, constraints);
function _update(address to, uint256 tokenId) internal virtual override returns (address) {
address from = super._update(to, tokenId);

if (from == address(0)) {
_addTokenToAllTokensEnumeration(tokenId);
Expand Down
8 changes: 2 additions & 6 deletions contracts/token/ERC721/extensions/ERC721Pausable.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,8 @@ abstract contract ERC721Pausable is ERC721, Pausable {
*
* - the contract must not be paused.
*/
function _update(
address to,
uint256 tokenId,
function(address, address, uint256) view constraints
) internal virtual override returns (address) {
function _update(address to, uint256 tokenId) internal virtual override returns (address) {
_requireNotPaused();
return super._update(to, tokenId, constraints);
return super._update(to, tokenId);
}
}
8 changes: 2 additions & 6 deletions contracts/token/ERC721/extensions/ERC721Royalty.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,8 @@ abstract contract ERC721Royalty is ERC2981, ERC721 {
/**
* @dev See {ERC721-_update}. This override additionally clears the royalty information for the token.
*/
function _update(
address to,
uint256 tokenId,
function(address, address, uint256) view constraints
) internal virtual override returns (address) {
address from = super._update(to, tokenId, constraints);
function _update(address to, uint256 tokenId) internal virtual override returns (address) {
address from = super._update(to, tokenId);

if (to == address(0)) {
_resetTokenRoyalty(tokenId);
Expand Down
8 changes: 2 additions & 6 deletions contracts/token/ERC721/extensions/ERC721URIStorage.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,12 +68,8 @@ abstract contract ERC721URIStorage is IERC4906, ERC721 {
* token-specific URI was set for the token, and if so, it deletes the token URI from
* the storage mapping.
*/
function _update(
address to,
uint256 tokenId,
function(address, address, uint256) view constraints
) internal virtual override returns (address) {
address from = super._update(to, tokenId, constraints);
function _update(address to, uint256 tokenId) internal virtual override returns (address) {
address from = super._update(to, tokenId);

if (to == address(0) && bytes(_tokenURIs[tokenId]).length != 0) {
delete _tokenURIs[tokenId];
Expand Down
8 changes: 2 additions & 6 deletions contracts/token/ERC721/extensions/ERC721Votes.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,8 @@ abstract contract ERC721Votes is ERC721, Votes {
*
* Emits a {IVotes-DelegateVotesChanged} event.
*/
function _update(
address to,
uint256 tokenId,
function(address, address, uint256) view constraints
) internal virtual override returns (address) {
address from = super._update(to, tokenId, constraints);
function _update(address to, uint256 tokenId) internal virtual override returns (address) {
address from = super._update(to, tokenId);
_transferVotingUnits(from, to, 1);
return from;
}
Expand Down

0 comments on commit e782f4e

Please sign in to comment.