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

Deprecated ERC721._burn(address, uint256) #1550

Merged
merged 2 commits into from Dec 11, 2018
Merged
Changes from 1 commit
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -12,6 +12,6 @@ contract ERC721Mock is ERC721 {
}

function burn(uint256 tokenId) public {
_burn(ownerOf(tokenId), tokenId);
_burn(tokenId);
}
}
@@ -13,7 +13,7 @@ contract ERC721PausableMock is ERC721Pausable, PauserRoleMock {
}

function burn(uint256 tokenId) public {
super._burn(ownerOf(tokenId), tokenId);
super._burn(tokenId);
}

function exists(uint256 tokenId) public view returns (bool) {
@@ -198,7 +198,7 @@ contract ERC721 is ERC165, IERC721 {
* @dev Internal function to mint a new token
* Reverts if the given token ID already exists
* @param to The address that will own the minted token
* @param tokenId uint256 ID of the token to be minted by the msg.sender
* @param tokenId uint256 ID of the token to be minted
*/
function _mint(address to, uint256 tokenId) internal {
require(to != address(0));
@@ -209,14 +209,24 @@ contract ERC721 is ERC165, IERC721 {
/**
* @dev Internal function to burn a specific token
* Reverts if the token does not exist
* @param tokenId uint256 ID of the token being burned by the msg.sender
* Deprecated, use _burn(uint256) instead.
* @param tokenId uint256 ID of the token being burned
This conversation was marked as resolved by nventuro

This comment has been minimized.

Copy link
@frangio

frangio Dec 11, 2018

Member

Can you add the missing @param owner here? 😬

This comment has been minimized.

Copy link
@nventuro

nventuro Dec 11, 2018

Author Member

😬

*/
function _burn(address owner, uint256 tokenId) internal {
_clearApproval(tokenId);
_removeTokenFrom(owner, tokenId);
emit Transfer(owner, address(0), tokenId);
}

/**
* @dev Internal function to burn a specific token
* Reverts if the token does not exist
* @param tokenId uint256 ID of the token being burned
*/
function _burn(uint256 tokenId) internal {
_burn(ownerOf(tokenId), tokenId);
This conversation was marked as resolved by frangio

This comment has been minimized.

Copy link
@frangio

frangio Dec 11, 2018

Member

If we want this to result in a performance improvement (one less SLOAD!) it should be the other way around: the old _burn(owner, tokenId) should be implemented in terms of the new one.

This comment has been minimized.

Copy link
@frangio

frangio Dec 11, 2018

Member

Oh wait, we do need to call _removeTokenFrom, right?

This comment has been minimized.

Copy link
@nventuro

nventuro Dec 11, 2018

Author Member

Yes, we do need to know the address, what we don't need is to provide it (since it will always be ownerOf(tokenId)). The actuall access control check happens in ERC721Burnable, and is

require(_isApprovedOrOwner(msg.sender, tokenId));
}

/**
* @dev Internal function to add a token ID to the list of a given address
* Note that this function is left internal to make ERC721Enumerable possible, but is not
@@ -13,6 +13,6 @@ contract ERC721Burnable is ERC721 {
*/
function burn(uint256 tokenId) public {
require(_isApprovedOrOwner(msg.sender, tokenId));
_burn(ownerOf(tokenId), tokenId);
_burn(tokenId);
}
}
@@ -117,7 +117,7 @@ contract ERC721Enumerable is ERC165, ERC721, IERC721Enumerable {
* @dev Internal function to mint a new token
* Reverts if the given token ID already exists
* @param to address the beneficiary that will own the minted token
* @param tokenId uint256 ID of the token to be minted by the msg.sender
* @param tokenId uint256 ID of the token to be minted
*/
function _mint(address to, uint256 tokenId) internal {
super._mint(to, tokenId);
@@ -129,8 +129,9 @@ contract ERC721Enumerable is ERC165, ERC721, IERC721Enumerable {
/**
* @dev Internal function to burn a specific token
* Reverts if the token does not exist
* Deprecated, use _burn(uint256) instead
* @param owner owner of the token to burn
* @param tokenId uint256 ID of the token being burned by the msg.sender
* @param tokenId uint256 ID of the token being burned
*/
function _burn(address owner, uint256 tokenId) internal {
super._burn(owner, tokenId);
@@ -147,10 +148,10 @@ contract ERC721Enumerable is ERC165, ERC721, IERC721Enumerable {
_allTokensIndex[tokenId] = 0;
_allTokensIndex[lastToken] = tokenIndex;
}

/**
* @dev Gets the list of token IDs of the requested owner
* @param owner address owning the tokens
* @param owner address owning the tokens
* @return uint256[] List of token IDs owned by the requested address
*/
function _tokensOfOwner(address owner) internal view returns (uint256[] storage) {
@@ -73,6 +73,7 @@ contract ERC721Metadata is ERC165, ERC721, IERC721Metadata {
/**
* @dev Internal function to burn a specific token
* Reverts if the token does not exist
* Deprecated, use _burn(uint256) instead
* @param owner owner of the token to burn
* @param tokenId uint256 ID of the token being burned by the msg.sender
*/
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.