EIP721 #130

simondlr · 2018-04-25T20:21:18Z

Hey all.

Here's a WIP of EIP721 along with an extensive test suite. https://github.com/ethereum/EIPs/blob/master/EIPS/eip-721.md.

EIP721 is non-fungible token standard of the Ethereum community. It contains several interfaces for implementing NFT:

A Base interface: This is all normal functionality (transferring, approving, etc).
An Enumerable Interface: This interface makes it easier to enumerate over tokens [all or owned by specific owners].
A Metadata Interface: Used for storing off-chain & on-chain information about the NFT.

The goal was to implement without too many dependencies into one specific EIP721 contract. It borrows from OpenZeppelin's implementation with various stylistic changes.

There's some things that I want to point out:

Please Double Check Conformity to EIP721 standard

Please help double check if I got all functions & interfaces & recommendations.

Naming

I spent a lot of time thinking about different naming of variables. Having to keep track of tokens using index arrays makes it complicated and sometimes cumbersome. Are the variable naming clear? If not, are there any other recommendations? I feel it could be even more clearer, but had trouble thinking up appropriate names.

EIP721 itself has inconsistent naming: eg usage of approve to denote approving one owner per token ID vs setApprovalForAll which denotes setting a specific operator to be able to transfer & approve all tokens of a specific owner.

Other inconsistencies in the standard include:

Mismatch of camelcase usage: eg tokenId vs tokenURI.
Mismatch of dangling underscore: eg, last variable here does not have a dangling underscore function safeTransferFrom(address _from, address _to, uint256 _tokenId, bytes data) external payable;. Event logs also hate dangling underscores since eslint shouts about it.

Even though there are inconsistencies in the standard, I'd err on not changing an this, but the things we can change, I want more advice on this.

No Function To Retrieve Token Arrays

EIP721 does not specify a way to retrieve arrays of tokens. This seems like a shortsight? In the enumerable interface, it would've been nice to retrieve batches of arrays. Is this an extension we want to include perhaps? eg, return all tokens or return an owner's tokens. Otherwise, currently, you'll have to enumerate yourself with several eth_calls vs one eth_call.

Default Payable?

EIP721 by default sets transfer functions to add payable to transfer & approve functions. This does not seem the most secure to me. Should we perhaps think of forcing it to not have this by default?

Blocking onERC721Received

The standard specifies that receiver should not consumer more than 50000 gas. I find this an odd restriction.

This function MUST use 50,000 gas or less.

What are thoughts on this?

Revert Reason

During the development of this EIP721, it became possible to add reasons for reverts (eg, using revert or require). Do we want to add this? I think it could help. > 0.4.22 of Solidity. I think for now, best to keep it simple, but in the future, perhaps upgrade to add in reasons. :)

Naming Implementation

The only unimplemented interface is naming & symbols. The reason being that's specified as external pure. This means you can't even read state, so the only way to implement naming is to hardcode it in the function, which I feel is stylistically awkward. eg

function name() external pure returns (string _name) {
    return 'NFT name';
}

So, I changed to public as visibility specifier. This however does not change the interface signatures.

This PR also contains another bump of Truffle 4.1.7.

For now that it is it. I will update if more comes to mind.

Here's a pic of a puppy. NOTE: This is not a collectible. Do not attempt to buy it. ;)

simondlr · 2018-04-25T20:24:07Z

Not sure why merging into staging isn't appropriately matching the updates merged into master. That's where the conflict is coming from. Let me know if I should rebase or resubmit the PR.

skmgoldin · 2018-04-28T20:59:14Z

Fixed @simondlr

ebaizel · 2018-05-03T19:36:35Z

test/eip721/eip721.js

+  });
+
+  it('creation: create multiple tokens to one user', async () => {
+    await nft.createToken(accounts[0], { from: accounts[0] });


looks like the createToken method signature only takes one, so maybe the second arg isn't needed?

maurelian · 2018-05-06T15:52:01Z

contracts/eip721/EIP721.sol

+        //2) delete last item (since it's now a duplicate)
+        delete allTokens[allTokensLength-1];
+        //3) reduce length of array
+        allTokens.length -= 1;


Is there a possible underflow here?

maurelian · 2018-05-06T15:52:11Z

contracts/eip721/EIP721.sol

+        //2) delete last item (since it's now a duplicate)
+        delete ownedTokens[_from][ownerLength-1];
+        //3) reduce length of array
+        ownedTokens[_from].length -= 1;


Is there a possible underflow here?

maurelian · 2018-05-06T15:57:31Z

contracts/eip721/EIP721.sol

+    /// @param _to The new owner
+    /// @param _tokenId The NFT to transfer
+    /// @param data Additional data with no specified format, sent in call to `_to`
+    function safeTransferFrom(address _from, address _to, uint256 _tokenId, bytes data) external payable


Does this need to be payable?

As stated in the PR, I wondered about this too.

EIP721 by default sets transfer functions to add payable to transfer & approve functions. This does not seem the most secure to me. Should we perhaps think of forcing it to not have this by default?

I'm okay with following the standard and keeping payable.

Ahh, sorry. Started the review on the plane, skipped the PR notes. Will review before I dig in again tomorrow.

The standard is actually quite flexible on this item.

So payable is unnecessary to stick with the standard.

maurelian · 2018-05-08T19:49:07Z

contracts/eip721/EIP721.sol

+    /// @dev Throws if `_tokenId` is not a valid NFT. URIs are defined in RFC
+    ///  3986. The URI may point to a JSON file that conforms to the "ERC721
+    ///  Metadata JSON Schema".
+    function tokenURI(uint256 _tokenId) external view returns (string) {


I can't find a setter to actually populate the URI mapping?

Yes. It's not in the standard.

The standard also says this function is optional. I would remove it.

Part of the goal for me was to implement all functionality in one contract only (subjective choice for readability). Removing this means the metadata interface is only partly implemented, which will also break the interface signature.

It's a subjective choice though.

maurelian · 2018-05-08T19:50:36Z

contracts/eip721/EIP721.sol

+    function safeTransferFrom(address _from, address _to, uint256 _tokenId, bytes data) external payable
+    tokenExists(_tokenId)
+    allowedToTransfer(_from, _to, _tokenId) {
+        settleTransfer(_from, _to, _tokenId);


I don't think it's a big deal, but since you have a possible revert later in this function, I think settleTransfer() would be best called at the end.

This would be in keeping with the "Checks, effects, interactions" pattern.

I don't think that's the case here since if you call the callback before settling the transfer the callback would have no different state to handle

Only problem is that it's a call, which shouldn't that be usually after internal state handling? either/or, not a big problem, but wondering what is the best practice here.

GNSPS · 2018-05-08T20:30:49Z

About the gas limit in the specs I actually think that limiting variance of the gas spent in different implementations is good! 👍

akuanti · 2018-05-09T18:55:05Z

contracts/eip721/EIP721.sol

+        // Note: the following code is equivalent to: require(getApproved(_tokenId) != 0) || _approved != 0);
+        // However: I found the following easier to read & understand.
+        if (approvedOwnerOfToken[_tokenId] == 0 && _approved == 0) {
+            revert(); // add reason for revert?


Add a comment such as "Revert on NOOP" for readability.

akuanti · 2018-05-09T18:57:13Z

contracts/eip721/EIP721.sol

+    /// @param _to The new owner
+    /// @param _tokenId The NFT to transfer
+    /// @param data Additional data with no specified format, sent in call to `_to`
+    function safeTransferFrom(address _from, address _to, uint256 _tokenId, bytes data) external payable


Regarding naming: I believe data can be _data (or the other parameters can have no underscore), since the function ID only uses the types of the parameters, not their Solidity names.

akuanti · 2018-05-09T19:03:46Z

contracts/eip721/EIP721.sol

+
+    // all tokens
+    uint256[] internal allTokens;
+    mapping(uint256 => uint256) internal allTokensIndex;


Mapping of tokenID => position in allTokens. Maybe a comment here?

skmgoldin · 2018-05-09T19:20:24Z

contracts/eip721/EIP721.sol

+        uint256 allTokensLength = allTokens.length;
+        //1) Put last item into index of token to be removed.
+        allTokens[allIndex] = allTokens[allTokensLength - 1];
+        allTokensIndex[allTokensLength - 1] = allIndex;


What is this line doing? Is it necessary? Did you mean to do something else? Here is a mental code execution.

At time zero, we have these data structures:
allTokens = [A, B, C]
allTokensIndex = { A : 0, B : 1, C: 2 }

Say we want to get rid of NFT B. We will correspond line numbers in this code to our mental execution:

286: allIndex = 1
287: allTokensArrayLength = 3

289: allTokens = [A, C, C] // Replace B with the content of the last array item
290: allTokensIndex = { A : 0, B : 1, C: 2, 2 : 1 } // ????

Execution continues and successfully truncates the array:
292: allTokens = [A, C, -]
293: allTokens = [A, C]

But what is happening, or what was supposed to happen on line 290? In a mapping of tokenIds to array indexes, you added an array index as a key with an array index as a value.

Same question on line 303

I think you found a bug @skmgoldin. Here's what should happen.

Here's a mental exercise.

allTokens = [100, 123, 142, 161, 170] // using larger ids for example
allTokensIndex = { 100: 0, 123: 1, 142: 2, 161: 3, 170: 4 }

Remove Token 123.

286: allIndex = 1
287: allTokensArrayLength = 5

289: allTokens = [100, 170, 142, 161, 170]

Now, in line 290, allTokensIndex of id 170, should be changed to 1 (replacing 123's index). So the line should be:

allTokensIndex[allTokens[allTokensLength-1]] = allIndex;

This takes the id of the last token (170) and changes its index to the index of the token just removes (123).

Can we got a confirm on this @GNSPS @maurelian?

[variable naming is hard]

This crept through because the tests have the same indexes as its IDs.

I should basically change the TestImplementation to start ids at 10 [so that it doesn't match the indexes]. Fix incoming.

Confirmed! ❤️ Eternal love to @skmgoldin & crew for doing this bug hunting!

akuanti · 2018-05-09T19:26:45Z

contracts/eip721/EIP721.sol

+        ownedTokensIndex[_tokenId] = ownedTokens[_to].length-1;
+    }
+
+    function removeToken(address _from, uint256 _tokenId) internal {


_owner would be a better name for this parameter, since it is used to get the owner of the token.

Both remove/add can be used in context's where it's not the owner specifically that's removing it.

akuanti · 2018-05-09T19:31:44Z

contracts/eip721/EIP721.sol

+        ownerOfToken[_tokenId] = _to;
+        // add that token to an array keeping track of tokens owned by that address
+        ownedTokens[_to].push(_tokenId);
+        // shorten length


This comment does not seem to correspond to what is happening. It's actually filling in the position of the just-added token in the owner's token list.

akuanti · 2018-05-09T19:32:01Z

contracts/eip721/EIP721.sol

+        emit Transfer(_from, _to, _tokenId);
+    }
+
+    function addToken(address _to, uint256 _tokenId) internal {


_to should probably be _owner.

skmgoldin · 2018-05-09T20:01:12Z

contracts/eip721/EIP721.sol

+        assembly { size := extcodesize(_to) }  // solhint-disable-line no-inline-assembly
+        if (size > 0) {
+            // call on onERC721Received.
+            require(EIP721TokenReceiverInterface(_to).onERC721Received(_from, _tokenId, data) == 0xf0b9e5ba);


Why not add a constant for this signature like you have for others towards the top of the file?

skmgoldin · 2018-05-09T20:14:27Z

contracts/eip721/EIP721.sol

+    mapping(uint256 => address) internal approvedOwnerOfToken;
+
+    // An operator is allowed to manage all assets of another owner.
+    mapping(address => mapping (address => bool)) internal approvedOperators;


Could this just be called operators? It technically contains all operators, just with their boolean value flag set to false. Like, if I have some operator who I approve, and then I revoke the approval, we make the revocation assignment to the _approved_Operators mapping, which seems semantically questionable.

simondlr · 2018-05-09T20:39:08Z

Thanks @skmgoldin for finding that bug. Thanks @maurelian @GNSPS @akuanti for inputs. I fixed the bug and added some stylistic changes.

simondlr · 2018-05-30T15:57:16Z

Seems there's some tiny changes in the standard coming in. Notably, 50k gas is removed (we didn't include it). ethereum/EIPs#721 (comment)

simondlr · 2018-06-05T08:38:46Z

There's been 3 changes to the spec. Minor changes.

As expected, the ERC721 spec changed the "pure" to "view" on name & symbol. Knew that didn't make sense. Even though it's not directly utilised like that in this implementation, I will change it to reflect that.
Transfer & Approval added indexed to tokenIDs.
The 50k gas requirement was removed. I will change the comments to reflect that.

simondlr · 2018-06-25T09:15:53Z

Since the last call, some changes have been made.

Notably: the on receive function added an "operator" so that the recipient knows who a token belongs to if it was transferred to them.

fulldecent/EIPs@2778813#diff-a87271f8186c59c075e229cd7421f205

There's also small stylistic changes to documentation to improve the language and remove ambiguity. Will need to add this.

simondlr · 2018-07-03T21:22:20Z

Bumped it FINAL spec on ERC721 and tidied the PR some more. Ready for final review.

fulldecent · 2019-02-25T22:10:25Z

Here is the updated reference implementation / https://github.com/0xcert/ethereum-erc721

First WIP of EIP721.

c9d1c8f

simondlr added the in progress label Apr 25, 2018

simondlr changed the title ~~Feat/eip721~~ EIP721 Apr 25, 2018

skmgoldin force-pushed the staging branch from 839f688 to 812791d Compare April 28, 2018 20:58

ebaizel reviewed May 3, 2018

View reviewed changes

maurelian reviewed May 6, 2018

View reviewed changes

maurelian reviewed May 8, 2018

View reviewed changes

akuanti reviewed May 9, 2018

View reviewed changes

skmgoldin reviewed May 9, 2018

View reviewed changes

akuanti reviewed May 9, 2018

View reviewed changes

skmgoldin reviewed May 9, 2018

View reviewed changes

Fixed bug along with addition of other smaller improvements/changes.

99ac72b

jasonrhaas mentioned this pull request May 31, 2018

Do tradeoff study for erc721 contracts gitcoinco/gitcoin-erc721#36

Closed

Bring in final updates to ERC721. Also bumped the solidity linter.

360f0ac

simondlr mentioned this pull request Jun 15, 2018

Implement ERC721 For Encapsulating Art Generated by Artonomous artonomous/artonomous-mvp#8

Closed

bump to spec, bump truffle, tidy and patch tests

ddfa466

EIP721 #130

Are you sure you want to change the base?

EIP721 #130

Conversation

simondlr commented Apr 25, 2018 • edited Loading

simondlr commented Apr 25, 2018

skmgoldin commented Apr 28, 2018

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

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

GNSPS commented May 8, 2018

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

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

Choose a reason for hiding this comment

simondlr commented May 9, 2018

simondlr commented May 30, 2018

simondlr commented Jun 5, 2018

simondlr commented Jun 25, 2018

simondlr commented Jul 3, 2018

fulldecent commented Feb 25, 2019

simondlr commented Apr 25, 2018 •

edited

Loading