ERC721 full implementation #803

spalladino · 2018-03-07T23:45:29Z

Fixes #640
Fixes #809

Continues the work of @facuspagnuolo in #682

Current status:

ERC721 implementation is complete

Pending discussions:

exists was added to the interface here, though will not be part of the standard
The 50K gas stipend for the onERC721Received call was removed (see here)

dekz · 2018-03-08T17:56:01Z

contracts/token/ERC721/ERC721BasicToken.sol

+    require(_from != address(0));
+    require(_to != address(0));
+    require(_to != ownerOf(_tokenId));
+    require(ownerOf(_tokenId) == _from);


This is checked in the next line in clearApproval and removeToken. It can probably be skipped here as it is redundant.

dekz · 2018-03-08T17:57:33Z

contracts/token/ERC721/ERC721BasicToken.sol

+    tokenOwner[_tokenId] = 0;
+  }
+
+  function checkSafeTransfer(address _from, address _to, uint256 _tokenId, bytes _data) internal returns (bool) {


Thoughts on renaming this to checkAndCallSafeTransfer?

dekz · 2018-03-08T18:05:58Z

contracts/token/ERC721/ERC721BasicToken.sol

+
+  function checkSafeTransfer(address _from, address _to, uint256 _tokenId, bytes _data) internal returns (bool) {
+    return !_to.isContract() ||
+      (ERC721Receiver(_to).onERC721Received.gas(SAFE_TRANSFER_GAS_STIPEND)(_from, _tokenId, _data) == ERC721_RECEIVED);


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

fulldecent · 2018-03-09T00:38:39Z

I am giving a cursory review and everything looks good so far.

carloschida · 2018-03-09T14:07:43Z

Congrats on the awesome job! The implementation and tests seem correct. I have only two comments on it.

`doMint`/`addToken` and `doBurn`/`removeToken`

Consider a contract that is ERC721BasicToken and that implements a public method that calls doMint without any further requirements. Like this, virtually anyone could mint a token to their own account.
My suggestion is to make contract ERC721BasicToken is Ownable and add the modifier onlyOwner to doMint.
In the same line of thought, I'd make addToken a private method so that tokens cannot be added without being minted, ie only via doMint so that the totalTokens count is correctly updated and that the event Transfer is always transmitted.

The same logic applies for the pair doBurn/removeToken.

Practical way of initial massive mint

Consider the following contract:

contract MyERC721 is ERC721BasicToken {
    function MyERC721(uint256 _initialSupply) public {
        for (uint256 i = 0; i < _initialSupply; i++)
            doMint(msg.sender, i);
    }
}

Or even this if we want to save some gas:

contract MyERC721 is ERC721BasicToken {
    function MyERC721(uint256 _initialSupply) public {
        for (uint256 i = 0; i < _initialSupply; i++) {
            tokenOwner[i] = msg.sender;
        }
        ownedTokensCount[msg.sender] = _initialSupply;
        totalTokens = _initialSupply;
    }
}

Both are forbiddingly expensive and limited when it comes to _initialSupply. The maximum number I could assign it in the migrations was 263 (for the second one): deployer.deploy(MyERC721, 263) spent ~0.7 ETH.

My final goal is to make a refundable, capped crowdsale of a ERC721BasicToken. Probably my strategy is wrong. If you guys have a better idea, I'd appreciate your insights.

carloschida

Comments on the assumption that the desired behaviour is that only the owner should be able to mint tokens.

carloschida · 2018-03-09T14:39:36Z

contracts/token/ERC721/ERC721BasicToken.sol

+  * @param _to address representing the new owner of the given token ID
+  * @param _tokenId uint256 ID of the token to be added to the tokens list of the given address
+  */
+  function addToken(address _to, uint256 _tokenId) internal {


Consider making it private rather than internal so that every addition is forced to be made via doMint. In this way we also make sure that the totalTokenscount is always updated and that the corresponding event Transfer is transmitted.

Note that the ERC721Token.sol full implementation would break if we make this method private, since the ownedTokens fields need to be updated there.

carloschida · 2018-03-09T14:40:34Z

contracts/token/ERC721/ERC721BasicToken.sol

+  * @param _from address representing the previous owner of the given token ID
+  * @param _tokenId uint256 ID of the token to be removed from the tokens list of the given address
+  */
+  function removeToken(address _from, uint256 _tokenId) internal {


Foe the same reasons as in addToken, consider changing it to private.

carloschida · 2018-03-09T14:44:02Z

contracts/token/ERC721/ERC721BasicToken.sol

+  * @param _to The address that will own the minted token
+  * @param _tokenId uint256 ID of the token to be minted by the msg.sender
+  */
+  function doMint(address _to, uint256 _tokenId) internal {


Consider adding the modifier onlyOwner (implying that the contract should be Ownable) so that not anyone can mint tokens.

That'll probably come in a Mintable extension (similar to how ERC20 is structured). I didn't want to enforce ownership for the base implementation.

any reason we went with doMint rather than _mint? likewise for doBurn vs _burn. and likewise for the various do* internal functions throughout the contracts.

Personal preference exclusively. As we were using leading underscores for parameters in functions, I wanted to use something different for the internal methods that actually executed the action, and I had seen the do pattern being used in other languages (such as Java). But I can switch to underscore if preferred.

You see that do thing come up occasionally. Personally I found it adds little semantically. Every method "does" something, that's why they're methods, start using it often and you can find whole objects full of "doMethods".

Underscore is fairly common in solidity as some kind of "private" be it as a prefix or post depending on the needs. I was able to understand this usage quickly on my first exposure to https://github.com/OpenZeppelin/zeppelin-solidity/blob/1eea95f9acebada0360f1f4be7c442325db27fa6/contracts/token/ERC721/ERC721Token.sol#L120

Initial response was "Why do they have a mint method which isn't actually used and this isn't a Mintable?" Then I see addToken and that I'm being given an opportunity to inject other logic into this minting flow (something I functionally require to add extras to an NFT at mint-time).

personally I prefer the _ approach since it's separate, in my head, from the function arguments, but I don't have a strong opinion. My only note is that this will very much inform future contract standard—I'd expect future additions to have do* instead of _* prefixes as people read the example code and work off of it.

spalladino · 2018-03-09T16:48:31Z

@carloschida thanks for the comments! Our goal is to provide a base implementation here, without forcing a particular ownership model for minting (that's why the methods are internal). We'll probably be providing a Mintable extension (similar to ERC20's) after this is merged. Stay tuned!

carloschida · 2018-03-09T17:56:58Z

@spalladino Thanks for the prompt response. I see. It does make sense to stay agnostic on that matter.
Nevertheless, I'd still consider it risky that a token can be added or burnt without changing adequately the total supply and without transmitting the corresponding events (here I mean only the internal -> private changes on those methods).

- Tests for new features are pending - ERC721 is abstract, since it requires metadata implementation - Move some methods into DeprecatedERC721 contract - Reorganise base vs full implementation - Pending tokenByIndex

- Remove restrictions from mock mint and burn calls

This allows token implementation to be non-abstract

We only want to keep the interface, for interacting with already deployed contracts

shrugs · 2018-03-22T02:14:47Z

re: transfer to self; I don't really have an opinion on whether or not it should no-op; the spec also doesn't specify. My only opinion is that if we do decide to no-op, it should clear approvals, according to spec (at least, as I've interpreted it).

At the time of any transfer, the approved address for that NFT (if any) is reset to none.

re: gas; If 50k is arbitrary, I'd prefer no limit at all. Who made the original decision on 50k, and what's their rationale?

spalladino · 2018-03-22T21:16:35Z

re: gas; If 50k is arbitrary, I'd prefer no limit at all. Who made the original decision on 50k, and what's their rationale?

@shrugs here is a short discussion on gitter regarding gas, which ends on @eordano agreeing on removing it.

facuspagnuolo

LGTM

facuspagnuolo · 2018-03-22T15:45:59Z

contracts/AddressUtils.sol

+   * @return whether there is code in the target address
+   */
+  function isContract(address addr) internal view returns (bool) {
+    uint size;


nit: uint256 explicitly?

facuspagnuolo · 2018-03-22T15:47:29Z

contracts/mocks/ERC721ReceiverMock.sol

+
+contract ERC721ReceiverMock is ERC721Receiver {
+  bytes4 retval;
+  bool reverts;


I guess the linter will cry if you don't make visibility explicit here

facuspagnuolo · 2018-03-22T17:29:01Z

contracts/token/ERC721/ERC721BasicToken.sol

+  */
+  function ownerOf(uint256 _tokenId) public view returns (address) {
+    address owner = tokenOwner[_tokenId];
+    require(owner != address(0));


giving we are using AddressUtils here, what do you think about adding a function like isNotZero or just requireNonZeroAddress or sth like that?

I'd rather leave that for another refactor

facuspagnuolo · 2018-03-22T21:17:03Z

contracts/token/ERC721/ERC721BasicToken.sol

+
+    if (_safe) {
+      require(checkAndCallSafeTransfer(_from, _to, _tokenId, _data));
+    }


I guess we can move this require to the safeTransferFrom function and remove the safe param here

Also rename addToken and removeToken for added clarity

mudgen · 2018-03-23T21:57:00Z

I found a couple small issues in this function:

  function removeTokenFrom(address _from, uint256 _tokenId) internal {
    super.removeTokenFrom(_from, _tokenId);

    uint256 tokenIndex = ownedTokensIndex[_tokenId];
    uint256 lastTokenIndex = ownedTokens[_from].length.sub(1);
    uint256 lastToken = ownedTokens[_from][lastTokenIndex];

    ownedTokens[_from][tokenIndex] = lastToken;
    ownedTokens[_from][lastTokenIndex] = 0;
    // Note that this will handle single-element arrays. In that case, both tokenIndex and lastTokenIndex are going to
    // be zero. Then we can make sure that we will remove _tokenId from the ownedTokens list since we are first swapping
    // the lastToken to the first position, and then dropping the element placed in the last position of the list

    ownedTokens[_from].length--;
    ownedTokensIndex[_tokenId] = 0;
    ownedTokensIndex[lastToken] = tokenIndex;
  }

First issue: If there is only one element in ownedTokens[_from] then _tokenId and lastToken will be the same. Therefore the last two lines of the function should be switched so that the removed token is not assigned an index in ownedTokensIndex.

It should be like this:

    ownedTokensIndex[lastToken] = tokenIndex;
    ownedTokensIndex[_tokenId] = 0;

Second issue: I think it is clearer, more explicit to use the delete keyword when deleting things. Like this:

delete ownedTokensIndex[_tokenId];

Third issue: I think "lastToken" should be renamed to "lastTokenId" to be consistent with using "_tokenId".

Cheers

spalladino · 2018-03-23T22:25:28Z

Thanks for the review @mudgen! Regarding the first issue:

First issue: If there is only one element in ownedTokens[_from] then _tokenId and lastToken will be the same. Therefore the last two lines of the function should be switched so that the removed token is not assigned an index in ownedTokensIndex.

If there is only one element in ownedTokens[_from], then not only _tokenId and lastToken will be equal, but also tokenIndex will be zero. So the last two lines are doing the same thing, and switching them should change anything. Let me know if I'm missing something though.

mudgen · 2018-03-23T22:28:12Z

@spalladino I looked at it and you are correct and you are not missing anything. tokenIndex will be zero so my earlier statement was incorrect and the current implementation is correct as is. Thanks for pointing that out.

mudgen · 2018-03-23T23:16:56Z

@spalladino what if ownedTokens[_from] is greater than one and lastTokenId and _tokenId are the same because _tokenId is the last one in ownedTokens[_from]. In that case tokenIndex will be greater than 0.

spalladino · 2018-03-26T13:31:22Z

@mudgen let's see a run in that scenario:

// Initial state
_tokenId = 30;
ownedTokens[_from] = [10, 20, 30];
ownedTokensIndex = { 10: 0, 20: 1, 30: 2};

uint256 tokenIndex = ownedTokensIndex[_tokenId]; // tokenIndex = 2
uint256 lastTokenIndex = ownedTokens[_from].length.sub(1); // lastTokenIndex = 2
uint256 lastToken = ownedTokens[_from][lastTokenIndex]; // lastToken = 30

ownedTokens[_from][tokenIndex] = lastToken; // ownedTokens[_from] = [10, 20, 30]
ownedTokens[_from][lastTokenIndex] = 0; // ownedTokens[_from] = [10, 20, 0]

ownedTokens[_from].length--; // ownedTokens[_from] = [10, 20]
ownedTokensIndex[_tokenId] = 0; // ownedTokensIndex = { 10: 0, 20: 1 };
ownedTokensIndex[lastToken] = tokenIndex; // ownedTokensIndex = { 10: 0, 20: 1, 30: 2}

It seems that ownedTokensIndex is not properly cleared. This should not be a major issue, since the token is currently "removed" from its owner, and the index will be corrected on the next "add" call; however, it's not very clean. I'll set up an issue to fix this and add the corresponding tests. Thanks!

falox · 2018-03-26T16:05:28Z

@spalladino In https://github.com/ethereum/EIPs/blob/master/EIPS/eip-721.md the approve/transfer functions are external payable, while here are public. Is there an easy way to extend this implementation to get payable functions? Thanks

spalladino · 2018-03-27T13:37:25Z

@falox I'm afraid that if you extend and change it to be payable you'll get a Overriding function changes state mutability from "nonpayable" to "payable" error. I'm reluctant to make the transfer functions payable in the implementation though.

falox · 2018-03-27T14:15:28Z

@spalladino as I guessed, thanks. May I ask you why you are preferring "nonpayable" to "payable" functions?

I'm asking this because I'm thinking at the use case where a token takes a fee on all transfers, including over-the-counter trades (i.e. in generic ERC721 exchanges and not via specific contract "buy" functions).

spalladino · 2018-03-27T20:51:45Z

@falox it's a tradeoff: though on one hand we are making things more difficult for people who want to add payable functionality to their ERC721s, on the other we are securing all the people who use regular ERC721s, to make sure they don't accidentally send ETH to the token contracts, which would otherwise be lost.

frangio · 2018-03-27T21:18:23Z

Locking this conversation. Please open new issues for any further doubts or problems!

spalladino added the review label Mar 7, 2018

spalladino mentioned this pull request Mar 7, 2018

ERC721 full implementation #682

Closed

spalladino force-pushed the feature/full_erc721 branch from 55113cf to c03b84a Compare March 7, 2018 23:52

dekz reviewed Mar 8, 2018

View reviewed changes

carloschida reviewed Mar 9, 2018

View reviewed changes

spalladino mentioned this pull request Mar 9, 2018

ERC: Non-fungible Token Standard ethereum/EIPs#721

Closed

facuspagnuolo and others added 11 commits March 9, 2018 17:19

Rename current ERC721 implementation to BaseERC721

1192e68

Implement ERC721 optional & approveAll functionality

ca163a8

Support for new ERC721 interface

71cbc51

- Tests for new features are pending - ERC721 is abstract, since it requires metadata implementation - Move some methods into DeprecatedERC721 contract - Reorganise base vs full implementation - Pending tokenByIndex

Add more tests for ERC721

3745025

Implement suggestions by @dekz

559df81

Update comments in ERC721 contracts

d726c79

Implement tokensByIndex extension

54a1d2e

- Remove restrictions from mock mint and burn calls

Add default implementation for metadata URI

851685c

This allows token implementation to be non-abstract

Allow operators to call approve on a token

3cef880

Remove gas stipend restriction in call to 721 receiver

6f180a6

Remove deprecated implementation

6fbe771

We only want to keep the interface, for interacting with already deployed contracts

spalladino force-pushed the feature/full_erc721 branch from 279c487 to 6fbe771 Compare March 9, 2018 20:22

spalladino requested a review from maraoz March 9, 2018 20:30

facuspagnuolo previously approved these changes Mar 22, 2018

View reviewed changes

spalladino added 6 commits March 22, 2018 18:23

Use explicit size for uint

fe6e4ff

Remove unneeded internal function in ERC721

4836279

Also rename addToken and removeToken for added clarity

Use underscore instead of 'do' prefix for internal methods in ERC721

619ae84

Fix failing test due to events reordering in ERC721 safe transfer

2e593f2

Fix bug introduced in 74db03b

6c09d20

Remove do prefix for internal setTokenUri method

37929c8

spalladino dismissed facuspagnuolo’s stale review via 37929c8 March 22, 2018 23:33

shrugs previously approved these changes Mar 23, 2018

View reviewed changes

frangio previously approved these changes Mar 23, 2018

View reviewed changes

Allow transfers to self in ERC721

3676b55

spalladino dismissed stale reviews from frangio and shrugs via 3676b55 March 23, 2018 16:38

frangio approved these changes Mar 23, 2018

View reviewed changes

Merge branch 'master' into feature/full_erc721

7815cc5

frangio merged commit e96164f into OpenZeppelin:master Mar 23, 2018

spalladino mentioned this pull request Mar 26, 2018

Internal ownedTokensIndex not properly cleared when removing a token from its owner in ERC721 #839

Closed

OpenZeppelin locked as resolved and limited conversation to collaborators Mar 27, 2018

ERC721 full implementation #803

ERC721 full implementation #803

Conversation

spalladino commented Mar 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fulldecent commented Mar 9, 2018

carloschida commented Mar 9, 2018

doMint/addToken and doBurn/removeToken

Practical way of initial massive mint

carloschida left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spalladino Mar 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spalladino commented Mar 9, 2018

carloschida commented Mar 9, 2018 • edited Loading

shrugs commented Mar 22, 2018

spalladino commented Mar 22, 2018

facuspagnuolo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mudgen commented Mar 23, 2018 • edited Loading

spalladino commented Mar 23, 2018

mudgen commented Mar 23, 2018

mudgen commented Mar 23, 2018

spalladino commented Mar 26, 2018

falox commented Mar 26, 2018 • edited Loading

spalladino commented Mar 27, 2018

falox commented Mar 27, 2018

spalladino commented Mar 27, 2018

frangio commented Mar 27, 2018

spalladino commented Mar 7, 2018 •

edited

Loading

`doMint`/`addToken` and `doBurn`/`removeToken`

spalladino Mar 21, 2018 •

edited

Loading

carloschida commented Mar 9, 2018 •

edited

Loading

mudgen commented Mar 23, 2018 •

edited

Loading

falox commented Mar 26, 2018 •

edited

Loading