Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
1192e68
Rename current ERC721 implementation to BaseERC721
facuspagnuolo Feb 26, 2018
ca163a8
Implement ERC721 optional & approveAll functionality
facuspagnuolo Feb 26, 2018
71cbc51
Support for new ERC721 interface
spalladino Mar 7, 2018
3745025
Add more tests for ERC721
spalladino Mar 8, 2018
559df81
Implement suggestions by @dekz
spalladino Mar 8, 2018
d726c79
Update comments in ERC721 contracts
spalladino Mar 8, 2018
54a1d2e
Implement tokensByIndex extension
spalladino Mar 9, 2018
851685c
Add default implementation for metadata URI
spalladino Mar 9, 2018
3cef880
Allow operators to call approve on a token
spalladino Mar 9, 2018
6f180a6
Remove gas stipend restriction in call to 721 receiver
spalladino Mar 9, 2018
6fbe771
Remove deprecated implementation
spalladino Mar 9, 2018
626742e
Add notice to isContract helper on constract constructors
spalladino Mar 20, 2018
95a1f9a
Change natspec delimiters for consistency
spalladino Mar 21, 2018
15f9556
Minor linting fixes
spalladino Mar 21, 2018
b332995
Add constant modifier to ERC721_RECEIVED magic value
spalladino Mar 21, 2018
f4748da
Use 4-params safeTransferFrom for implementing the 3-params overload
spalladino Mar 21, 2018
fb4f728
Minor text changes in natspec comments
spalladino Mar 21, 2018
6b98e4e
Use address(0) instead of 0 or 0x0
spalladino Mar 21, 2018
3f2ea8a
Use if-statements instead of boolean one-liners for clarity
spalladino Mar 21, 2018
74db03b
Keep ownedTokensCount state var in sync in full ERC721 implementation
spalladino Mar 21, 2018
981c6f7
Fix incorrect comparison when burning ERC721 tokens with metadata
spalladino Mar 21, 2018
73b77ae
Use address(0) instead of 0 in one more place in ERC721
spalladino Mar 21, 2018
eee5b0e
Throw when querying balance for the zero address
spalladino Mar 21, 2018
9deb637
Update links to approved version of EIP721
spalladino Mar 21, 2018
fe6e4ff
Use explicit size for uint
spalladino Mar 22, 2018
4836279
Remove unneeded internal function in ERC721
spalladino Mar 22, 2018
619ae84
Use underscore instead of 'do' prefix for internal methods in ERC721
spalladino Mar 22, 2018
2e593f2
Fix failing test due to events reordering in ERC721 safe transfer
spalladino Mar 22, 2018
6c09d20
Fix bug introduced in 74db03ba06
spalladino Mar 22, 2018
37929c8
Remove do prefix for internal setTokenUri method
spalladino Mar 22, 2018
3676b55
Allow transfers to self in ERC721
spalladino Mar 23, 2018
7815cc5
Merge branch 'master' into feature/full_erc721
frangio Mar 23, 2018
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
Prev Previous commit
Next Next commit
Update comments in ERC721 contracts
  • Loading branch information
spalladino committed Mar 9, 2018
commit d726c79e5ff0b63be8c519c6423fb9831c8d75fb
8 changes: 8 additions & 0 deletions contracts/AddressUtils.sol
Original file line number Diff line number Diff line change
@@ -1,7 +1,15 @@
pragma solidity ^0.4.18;

/**
* Utility library of inline functions on addresses
*/
library AddressUtils {

/**
* Returns whether there is code in the target address
* @param addr address address to check
* @return whether there is code in the target address
*/
function isContract(address addr) internal view returns (bool) {
Copy link
Contributor

Choose a reason for hiding this comment

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

A contract address doesn't have code until its constructor finishes executing; during execution of the constructor, isContract will not correctly detect that it is (or will be) a contract.

How this affects ERC721 is not clear. Since the contract still doesn't have code, it would also be unable to execute onERC721Received, so it makes no difference whether isContract returns true or false. OTOH, this situation is likely a programmer error, and it will fail silently. Sadly I don't think there's a solution other than changing the interface to make it explicit that an onERC721Received call is expected.

Given the odd semantics, I'm not sure we should provide isContract as a standalone helper. I'm inclined to say no.
If we do, we should place a disclaimer stating that it will return false if the contract constructor hasn't yet finished running.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding a disclaimer!

uint size;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: uint256 explicitly?

assembly { size := extcodesize(addr) }
Expand Down
6 changes: 6 additions & 0 deletions contracts/token/ERC721/ERC721.sol
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,23 @@ pragma solidity ^0.4.18;

import "./ERC721Basic.sol";

/// @title ERC-721 Non-Fungible Token Standard, optional enumeration extension
Copy link
Contributor

Choose a reason for hiding this comment

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

replace /// with /* */ for consistency

/// @dev See https://github.com/ethereum/EIPs/blob/master/EIPS/eip-721.md
contract ERC721Enumerable is ERC721Basic {
function totalSupply() public view returns (uint256);
function tokenOfOwnerByIndex(address _owner, uint256 _index) public view returns (uint256 _tokenId);
// function tokenByIndex(uint256 _index) public view returns (uint256);
}

/// @title ERC-721 Non-Fungible Token Standard, optional metadata extension
/// @dev See https://github.com/ethereum/EIPs/blob/master/EIPS/eip-721.md
contract ERC721Metadata is ERC721Basic {
function name() public view returns (string _name);
function symbol() public view returns (string _symbol);
function tokenURI(uint256 _tokenId) public view returns (string);
}

/// @title ERC-721 Non-Fungible Token Standard, full implementation interface
/// @dev See https://github.com/ethereum/EIPs/blob/master/EIPS/eip-721.md
contract ERC721 is ERC721Basic, ERC721Enumerable, ERC721Metadata {
}
4 changes: 2 additions & 2 deletions contracts/token/ERC721/ERC721Basic.sol
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
pragma solidity ^0.4.18;

/**
* @title Base ERC721 interface
* @dev see https://github.com/ethereum/eips/issues/721 and https://github.com/ethereum/EIPs/pull/841
* @title ERC721 Non-Fungible Token Standard basic interface
* @dev see https://github.com/ethereum/eips/issues/721
*/
contract ERC721Basic {
event Transfer(address indexed _from, address indexed _to, uint256 _tokenId);
Expand Down
60 changes: 49 additions & 11 deletions contracts/token/ERC721/ERC721BasicToken.sol
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,14 @@ import "../../math/SafeMath.sol";
import "../../AddressUtils.sol";

/**
* @title ERC721BasicToken
* Generic implementation for the required functionality of the ERC721 standard
* @title ERC721 Non-Fungible Token Standard basic implementation
* @dev see https://github.com/ethereum/eips/issues/721
*/
contract ERC721BasicToken is ERC721Basic {
using SafeMath for uint256;
using AddressUtils for address;

// Gas allowed in calls to onERC721Received on safeTransfers
uint256 SAFE_TRANSFER_GAS_STIPEND = 50000;

// Equals to bytes4(keccak256("onERC721Received(address,uint256,bytes)"))
Expand Down Expand Up @@ -42,6 +43,10 @@ contract ERC721BasicToken is ERC721Basic {
_;
}

/**
* @dev Checks msg.sender can transfer a token, by being owner, approved, or operator
* @param _tokenId uint256 ID of the token to validate
*/
modifier canTransfer(uint256 _tokenId) {
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be isApprovedFor(_tokenId) because it can be used for things beyond transfer?

Copy link
Contributor

Choose a reason for hiding this comment

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

I kind of like the name because it clearly distinguishes from isApproved (which van be interpreted as "has been individually approved) and isApprovedForAll or isOwner

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually it's only being used to checking transfers. Where else do you see this being used @shrugs? We could change it to canManage to appease all use cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps a superclass will want to allow an owner to change the metadata of the token on-chain? (or is that not allowed by the spec?). Regardless, you're right; the concept of ownership here is pretty much just "can I transfer this token" which seems fine. I'm not particularly attached to canManage but I feel like it's slightly more general without losing meaning.

require(isApprovedOrOwner(msg.sender, _tokenId));
_;
Expand All @@ -67,13 +72,20 @@ contract ERC721BasicToken is ERC721Basic {
return owner;
}

/**
* @dev Returns whether the specified token exists
* @param _tokenId uint256 ID of the token to query the existance of
* @return whether the token exists
*/
function exists(uint256 _tokenId) public view returns (bool) {
address owner = tokenOwner[_tokenId];
return owner != 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

address(0)

}

/**
* @dev Approves another address to claim for the ownership of the given token ID
* @dev Approves another address to transfer the given token ID
* @dev The zero address indicates there is no approved address.
* @dev There can only be one approved address per token at a given time.
* @param _to address to be approved for the given token ID
* @param _tokenId uint256 ID of the token to be approved
*/
Expand All @@ -87,17 +99,18 @@ contract ERC721BasicToken is ERC721Basic {
}

/**
* @dev Gets the approved address to take ownership of a given token ID
* @dev Gets the approved address for a token ID, or zero if no address set
* @param _tokenId uint256 ID of the token to query the approval of
* @return address currently approved to take ownership of the given token ID
* @return address currently approved for a the given token ID
*/
function getApproved(uint256 _tokenId) public view returns (address) {
return tokenApprovals[_tokenId];
}


/**
* @dev Sets the approval of a given operator
* @dev Sets or unsets the approval of a given operator
* @dev An operator is allowed to transfer all tokens of the sender on their behalf
* @param _to operator address to set the approval
* @param _approved representing the status of the approval to be set
*/
Expand All @@ -119,6 +132,8 @@ contract ERC721BasicToken is ERC721Basic {

/**
* @dev Transfers the ownership of a given token ID to another address
* @dev Usage of this method is discouraged, use `safeTransferFrom` whenever possible
* @dev Requires the msg sender to be the owner, approved, or operator
* @param _from current owner of the token
* @param _to address to receive the ownership of the given token ID
* @param _tokenId uint256 ID of the token to be transferred
Expand All @@ -128,7 +143,12 @@ contract ERC721BasicToken is ERC721Basic {
}

/**
* @dev Transfers the ownership of a given token ID to another address
* @dev Safely transfers the ownership of a given token ID to another address
* @dev If the target address is a contract, it must implement `onERC721Received`,
* which is called upon a safe transfer, and return the magic value
* `bytes4(keccak256("onERC721Received(address,uint256,bytes)"))`; otherwise,
* the transfer is reverted.
* @dev Requires the msg sender to be the owner, approved, or operator
* @param _from current owner of the token
* @param _to address to receive the ownership of the given token ID
* @param _tokenId uint256 ID of the token to be transferred
Expand All @@ -138,7 +158,12 @@ contract ERC721BasicToken is ERC721Basic {
}

/**
* @dev Transfers the ownership of a given token ID to another address
* @dev Safely transfers the ownership of a given token ID to another address
* @dev If the target address is a contract, it must implement `onERC721Received`,
* which is called upon a safe transfer, and return the magic value
* `bytes4(keccak256("onERC721Received(address,uint256,bytes)"))`; otherwise,
* the transfer is reverted.
* @dev Requires the msg sender to be the owner, approved, or operator
* @param _from current owner of the token
* @param _to address to receive the ownership of the given token ID
* @param _tokenId uint256 ID of the token to be transferred
Expand All @@ -149,7 +174,7 @@ contract ERC721BasicToken is ERC721Basic {
}

/**
* @dev Tells whether the given spender can transfer a given token ID
* @dev Returns whether the given spender can transfer a given token ID
* @param _spender address of the spender to query
* @param _tokenId uint256 ID of the token to be transferred
* @return bool whether the msg.sender is approved for the given token ID,
Expand All @@ -161,7 +186,8 @@ contract ERC721BasicToken is ERC721Basic {
}

/**
* @dev Mint token function
* @dev Internal function to mint a new token
* @dev 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
*/
Expand All @@ -173,7 +199,8 @@ contract ERC721BasicToken is ERC721Basic {
}

/**
* @dev Burns a specific token
* @dev Internal function to burn a specific token
* @dev Reverts if the token does not exist
* @param _tokenId uint256 ID of the token being burned by the msg.sender
*/
function doBurn(uint256 _tokenId) onlyOwnerOf(_tokenId) internal {
Expand Down Expand Up @@ -207,6 +234,8 @@ contract ERC721BasicToken is ERC721Basic {

/**
* @dev Internal function to clear current approval of a given token ID
* @dev Reverts if the given address is not indeed the owner of the token
* @param _owner owner of the token
* @param _tokenId uint256 ID of the token to be transferred
*/
function clearApproval(address _owner, uint256 _tokenId) internal {
Expand Down Expand Up @@ -239,6 +268,15 @@ contract ERC721BasicToken is ERC721Basic {
tokenOwner[_tokenId] = 0;
}

/**
* @dev Internal function to invoke `onERC721Received` on a target address
* @dev The call is not executed if the target address is not a contract
* @dev Returns whether the call correctly returned the expected magic value
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason why this wasn't implemented as throwing instead of returning a boolean? If the return value is kept it should be documented with a @return annotation.

* @param _from address representing the previous owner of the given token ID
* @param _to target address that will receive the tokens
* @param _tokenId uint256 ID of the token to be transferred
* @param _data bytes optional data to send along with the call
*/
function checkAndCallSafeTransfer(address _from, address _to, uint256 _tokenId, bytes _data) internal returns (bool) {
return !_to.isContract() ||
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be clearer to break this into an if conditional with a temporary variable for the return value.

Copy link
Contributor Author

@spalladino spalladino Mar 21, 2018

Choose a reason for hiding this comment

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

Hmph I really like to use one-liners with boolean operators (as you have already seen), but I acknowledge that if statements may be easier to follow

(ERC721Receiver(_to).onERC721Received.gas(SAFE_TRANSFER_GAS_STIPEND)(_from, _tokenId, _data) == ERC721_RECEIVED);
Copy link

Choose a reason for hiding this comment

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

As mentioned in gitter I think we should discuss this STIPEND more and its pros and cons.

Expand Down
2 changes: 1 addition & 1 deletion contracts/token/ERC721/ERC721Token.sol
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import "./ERC721BasicToken.sol";
* @title Full ERC721 Token
* This implementation includes all the required and some optional functionality of the ERC721 standard
* Moreover, it includes approve all functionality using operatable terminology
Copy link
Contributor

Choose a reason for hiding this comment

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

operable or operator

* @dev see https://github.com/ethereum/eips/issues/721 and https://github.com/ethereum/EIPs/pull/841
* @dev see https://github.com/ethereum/eips/issues/721
*/
contract ERC721Token is ERC721, ERC721BasicToken {
// Token name
Expand Down