Implement ERC721 standard #627

facuspagnuolo · 2017-12-24T12:57:21Z

This PR tackles #615, it should be merged after #628
@eordano was working on this issue too, but unfortunately, I heard about it once this solution was almost done. I'm pushing this PR just to see how we can merge both jobs.
fixes #615

eordano · 2017-12-27T05:49:29Z

Looking good!

Some comments:

I've been thinking about calling the contract AssetRegistry or TokenRegistry. I think that "Token" is best reserved for the actual non-fungibles stored.
I'd wait on merging any implementation until some comments on the EIP are addressed. In particular, an approveAll function looks like is pretty much essential, delegateAll functionality to allow metadata updates from a third party account, and (my own thoughts) transfer should have ERC223-like functionality (call a function upon transfer to prevent allocating ownership to contracts that can't handle owning nfts)
Great work on test coverage!

eordano · 2017-12-27T05:50:35Z

.gitignore

@@ -1,5 +1,6 @@
 *.swp
 *.swo
+.idea


I'll move it to .git/info/exclude

eordano · 2017-12-27T06:05:56Z

contracts/token/AssetToken.sol

+  */
+  function removeToken(address _from, uint256 _tokenId) internal {
+    uint256 length = balanceOf(_from);
+    if(length <= 1) delete ownedTokens[_from][_tokenId];


I'd go with:

require(length > 0); if (length == 1) { delete ownedTokens[_from][_tokenId]; delete ownedTokens[_from]; }

good point, actually there's no need to handle any specific scenario for single element arrays

facuspagnuolo · 2017-12-27T20:24:05Z

@eordano thanks for reviewing!

I addressed many of your comments. Regarding the name that we should use for the implementation, I'm in favour of using the one that describes what the implementation does better. That said, I really like both TokenRegistry or AssetRegistry.. would you mind @frangio sharing your thoughts about this?

On the other hand, we've been thinking about the scope of this implementation with @frangio and we decided not to cover all the optional functionality, but some of them. For example, we think it could be better to leave the approval-all or transfer-and-call functionalities for further implementations. Thoughts?

We finally decided to add a basic mint and burn functionalities too. Please review :D

eordano · 2017-12-28T16:39:22Z

I think approveAll is really a must. Any 0x-like protocol would otherwise be very expensive to implement (every time you change ownership you have to send an approve to the 0x contract). The way 0x works right now is that they set an approve(2^256-1) on ERC 20 tokens.

cc: @frangio

eordano · 2018-01-03T14:32:36Z

nit: maybe add the implementsEIP721(): bool function? kinda useless if ethereum/EIPs#165 is more widely used

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

I read your last comment of the ERC721 thread, and it convinced me. I will work on an approveAll and a transferAndCall solutions. Regarding the implementsEIP721 function, with @frangio we were saying that we may want to provide a detailed implementation including it with the rest of the optional stuff. Let me know your thoughts.

eordano · 2018-01-05T01:11:16Z

+1 on the additional functionality encapsulated in another contract

shrugs

Really incredible test coverage, I'm so impressed/happy about this.

Some questions:

what's the argument for only a single address being able to be approved or eternallyApproved?
do we need a takeOwnershipFor(address _add, utin256 _tokenId) function to allow contracts that are eternally approved to take ownership of a token on behalf of a user?
do we want to include a convenience "multi transfer" function to make n-of-n atomic swaps easier?

shrugs · 2018-01-05T17:10:43Z

contracts/token/ERC721.sol

@@ -0,0 +1,21 @@
+pragma solidity ^0.4.11;


OZ is on 0.4.18 atm, here and below

shrugs · 2018-01-05T17:20:56Z

contracts/token/NonFungibleToken.sol

+  }
+
+  /**
+  * @dev Gets then token ID at a given index of the tokens list of the requested owner


nitpick: then -> the

shrugs · 2018-01-05T17:22:55Z

contracts/token/NonFungibleToken.sol

+  * @param _tokenId uint256 ID of the token being burned by the msg.sender
+  */
+  function burn(uint256 _tokenId) onlyOwnerOf(_tokenId) public {
+    if(approvedFor(_tokenId) != 0) clearApproval(msg.sender, _tokenId);


I'm personally very into requiring syntax like

if (condition) { doThing() }

regardless of its simplicity. does OZ have a stance on that?

I think we don't but I agree

shrugs · 2018-01-05T17:33:53Z

test/NonFungibleToken.test.js

+      });
+    });
+
+    describe('when the given address owns some tokens', function () {


"does not own some tokens"?

shrugs · 2018-01-05T17:38:49Z

test/NonFungibleToken.test.js

+      });
+
+      describe('when the given address is the zero address', function () {
+        const to = 0x0;


I expect this works cause web3 casts it behind the scenes, but should we make a constant for the null address and use that here and around the test file instead of 0x0000000000000000000000000000000000000000 ?

0x calls this NULL_ADDRESS but we can use ZERO_ADDRESS

like the idea!

shrugs · 2018-01-05T17:44:11Z

contracts/token/NonFungibleToken.sol

+  */
+  function removeToken(address _from, uint256 _tokenId) internal {
+    require(balanceOf(_from) > 0);
+    require(ownerOf(_tokenId) == _from);


these requires are implied by all of the code paths that reach removeToken, so I think we can technically remove them. that said, being explicit about it is 👍 - just making sure that was the intention

I placed those requires there because I wanted to give an internal interface to handle the list of tokens. If someone extends our implementation, given that the state variables are private, the only way they will be able to change the contract's state will be through our interface. This is just to ensure our users that they will be handling this correctly reducing the attack surface. What do you think?

Great point, let's definitely keep them

shrugs · 2018-01-05T18:02:05Z

test/NonFungibleToken.test.js

+  describe('approveAll', function () {
+    const sender = _creator;
+
+    describe('when the address willing to approve is the 0 address', function () {


is it worth including this test case? the code doesn't react differently based on _to being the 0-address or not

(up to you)

facuspagnuolo · 2018-01-11T17:35:17Z

@frangio please review

eordano · 2018-01-12T19:38:53Z

Tomorrow we'll be introducing the ERC 821: DARs (Distinguishable Asset Registries). We're writing a blogpost and the final text of the standard, would like you to take a look at it and hear your feedback

I'd go against (2), cementing this implementation. I want a great standard, not a good enough one...

eordano · 2018-01-12T19:39:18Z

By the way, tomorrow is the https://raredigitalartfestival.splashthat.com/ conference!

frangio · 2018-01-15T15:04:54Z

contracts/token/ERC721.sol

+  event Transfer(address indexed _from, address indexed _to, uint256 _tokenId);
+  event Approval(address indexed _owner, address indexed _approved, uint256 _tokenId);
+
+  function name() public view returns (string _name);


Since name and symbol are optional, they shouldn't be part of the ERC721 interface.

frangio · 2018-01-15T17:51:39Z

contracts/token/NonFungibleToken.sol

+  using SafeMath for uint256;
+
+  // Token name
+  string internal _name;


Since name and symbol are optional, I thought we were going to include them in a different contract in #640.

frangio · 2018-01-15T20:43:26Z

contracts/token/NonFungibleToken.sol

+  * @return owner address currently marked as the owner of the given token ID
+  */
+  function ownerOf(uint256 _tokenId) public view returns (address owner) {
+    owner = tokenOwner[_tokenId];


We don't use this style of returning values in the rest of OpenZeppelin, and I'd use an explicit return here to be consistent.

frangio · 2018-01-15T20:47:53Z

contracts/token/NonFungibleToken.sol

+  * @return uint256 token ID at the given index of the tokens list owned by the requested address
+  */
+  function tokenOfOwnerByIndex(address _owner, uint256 _index) public view returns (uint256) {
+    require(_index < balanceOf(_owner));


This check isn't necessary because Solidity does bounds checking on array accesses.

Solidity throws when checking array bounds, and we are reverting instead

frangio · 2018-01-15T20:51:33Z

contracts/token/NonFungibleToken.sol

+   */
+  function transfer(address _to, uint _tokenId, bytes _data) public onlyOwnerOf(_tokenId) {
+    clearApprovalAndTransfer(msg.sender, _to, _tokenId);
+    require(_to.call.value(0)(_data));


The .value(0) part isn't necessary, because it's the default. We might want to keep it for explicitness, but I'd rather remove it because like this a single additional character can make ether be sent. 😰

frangio · 2018-01-15T22:02:54Z

contracts/token/NonFungibleToken.sol

+  * @param _approved representing the status of the approval
+  */
+  function approve(address _to, bool _approved) public {
+    require(_to != msg.sender); // we are reverting here following the same behaviour as approve does


The comment is confusing because this function is called approve as well. I'm not comfortable with that... Have we considered other names for this one?

frangio · 2018-01-15T22:36:10Z

contracts/token/NonFungibleToken.sol

+  * @param _to address which you want to transfer the token to
+  * @param _tokenId uint256 ID of the token being claimed by the msg.sender
+  */
+  function takeOwnershipFor(address _to, uint256 _tokenId) public onlyApprovedFor(_tokenId) {


Hmmm, this name... But I see that this is the least agreed upon part in the ERC.

Did we make the name up or steal it from somewhere else?

facuspagnuolo · 2018-01-16T20:37:37Z

Based on what we discussed, I'm rollbacking this implementation to hold just the required functionality of the ERC721 standard. Detailed implementation and approveAll/transfer&call functionalities will be provided in another PRs

frangio · 2018-01-17T20:21:33Z

contracts/token/ERC721Token.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 mint(address _to, uint256 _tokenId) internal {


Child contracts are not allowed to change the visibility of inherited functions, so a child contract here won't be able to define public functions called mint or burn. Since those are the names of the public functions in Mintable and Burnable ERC20 tokens, I think I would rename these to avoid the potential clash.

Perhaps _mint and _burn, or doMint and doBurn...

frangio · 2018-01-17T20:21:49Z

contracts/token/ERC721Token.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 {


I'm thinking that addToken and removeToken should be private instead of internal, given that we already expose mint and burn as internal, which guarantee the correct emision of Transfer events as required by the spec.

frangio · 2018-01-17T20:22:09Z

contracts/token/ERC721Token.sol

+  * @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 {
+    require(balanceOf(_from) > 0);


I would remove this require because it should be implied by the second one. If _from owns _tokenId it will have a balance of at least 1, as guaranteed by addToken.

frangio · 2018-01-17T20:41:38Z

contracts/token/ERC721Token.sol

+
+    tokenOwner[_tokenId] = 0;
+    ownedTokens[_from][tokenIndex] = lastToken;
+    ownedTokens[_from][lastTokenIndex] = 0;


Since it's kind of magical that the algorithm works when there is only one item in the list, what do you think about adding a brief note explaining it?

In particular, it feels worthy of mention that the order of lines 193 and 194 is important, because if _tokenId is the only item in the list, in this way it is properly deleted from storage.

frangio · 2018-01-17T20:47:14Z

contracts/token/ERC721Token.sol

+   * @param _tokenId uint256 ID of the token to query the approval of
+   * @return bool whether the msg.sender is approved for the given token ID or not
+   */
+  function isApprovedFor(uint256 _tokenId) internal view returns (bool) {


I'm not comfortable with helpers not being pure. In this case it uses the global msg.sender property of the transaction. I would add an explicit address parameter to make it pure.

I was going to say that the helper should be private or that we should consider removing it, but it may be an internal helper with the purpose of being overriden, such as in the Operatable subcontract. If so, should we make that explicit in the comments?

Sorry, but I don't see how this function can be pure since it is reading the contract state. Would you mind giving further details?

My bad! I didn't use "pure" in a precise way.

Agreed offline to add an additional address parameter.

ajile-in · 2018-01-25T12:06:57Z

@facuspagnuolo Shall this be treated as an intermediate reference implementation? As there is an effort still going on about finalisation of ERC721 here

…ent_erc721_standard Implement ERC721 standard

facuspagnuolo added the pending review label Dec 24, 2017

facuspagnuolo mentioned this pull request Dec 26, 2017

Refactor assert revert helper to encapsulate promises #628

Merged

eordano reviewed Dec 27, 2017

View reviewed changes

facuspagnuolo force-pushed the feature/implement_erc721_standard branch from 5958386 to 4173800 Compare December 27, 2017 20:21

eternauta1337 added review and removed review labels Jan 2, 2018

facuspagnuolo added in progress and removed backlog labels Jan 2, 2018

facuspagnuolo self-assigned this Jan 2, 2018

eternauta1337 removed the pending review label Jan 4, 2018

facuspagnuolo mentioned this pull request Jan 4, 2018

Provide a detailed implementation of the ERC721 standard #640

Closed

facuspagnuolo force-pushed the feature/implement_erc721_standard branch 4 times, most recently from a95016b to c099c97 Compare January 5, 2018 14:06

facuspagnuolo mentioned this pull request Jan 5, 2018

Truffle does not support function overloading trufflesuite/truffle#737

Closed

1 task

facuspagnuolo force-pushed the feature/implement_erc721_standard branch from c099c97 to a4d3036 Compare January 5, 2018 14:18

facuspagnuolo added review and removed in progress labels Jan 5, 2018

eternauta1337 mentioned this pull request Jan 5, 2018

[WIP] Add ERC721 contracts #623

Closed

4 tasks

shrugs reviewed Jan 5, 2018

View reviewed changes

facuspagnuolo force-pushed the feature/implement_erc721_standard branch from 1572511 to 0d9efcf Compare January 6, 2018 00:28

frangio reviewed Jan 15, 2018

View reviewed changes

facuspagnuolo dismissed shrugs’s stale review via 8390f1b January 16, 2018 18:55

facuspagnuolo force-pushed the feature/implement_erc721_standard branch 3 times, most recently from 83fe097 to 4ccf668 Compare January 16, 2018 19:02

facuspagnuolo added 3 commits January 16, 2018 16:04

Provide ERC721 required functionality interface

5fc5ded

Implement ERC721 required functionality

b09d7aa

ERC721 required functionality tests

5b50e99

facuspagnuolo force-pushed the feature/implement_erc721_standard branch from 4ccf668 to 5b50e99 Compare January 16, 2018 19:05

facuspagnuolo mentioned this pull request Jan 16, 2018

ERC721 full implementation #682

Closed

Fix solint errors

af33704

frangio reviewed Jan 17, 2018

View reviewed changes

Address feedback comments for ERC721

23afc74

facuspagnuolo force-pushed the feature/implement_erc721_standard branch from 65e6fb2 to 23afc74 Compare January 18, 2018 13:16

frangio approved these changes Jan 19, 2018

View reviewed changes

facuspagnuolo added 3 commits January 19, 2018 15:36

Merge branch 'master' into feature/implement_erc721_standard

4859489

Merge branch 'master' into feature/implement_erc721_standard

4be30dc

Merge branch 'master' into feature/implement_erc721_standard

e1d32ac

facuspagnuolo merged commit e16c404 into OpenZeppelin:master Jan 23, 2018

facuspagnuolo deleted the feature/implement_erc721_standard branch January 23, 2018 13:47

facuspagnuolo removed the review label Jan 23, 2018

fulldecent mentioned this pull request Jan 28, 2018

Use deed instead of token for ERC-721 #709

Closed

dekz mentioned this pull request Feb 14, 2018

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

Closed

nastassiasachs mentioned this pull request Feb 15, 2018

ERC721Deed #747

Closed

ProphetDaniel pushed a commit to classicdelta/Smart-Contracts that referenced this pull request Mar 9, 2018

Merge pull request OpenZeppelin#627 from facuspagnuolo/feature/implem…

7d8fe68

…ent_erc721_standard Implement ERC721 standard

@@ @@ -1,5 +1,6 @@ @@
               *.swp
               *.swo
+              .idea

Implement ERC721 standard #627

Implement ERC721 standard #627

Conversation

facuspagnuolo commented Dec 24, 2017 • edited by eternauta1337 Loading

eordano commented Dec 27, 2017

eordano Dec 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eordano Dec 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facuspagnuolo commented Dec 27, 2017

eordano commented Dec 28, 2017

eordano commented Jan 3, 2018

facuspagnuolo commented Jan 3, 2018 • edited Loading

eordano commented Jan 5, 2018

shrugs 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facuspagnuolo Jan 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facuspagnuolo commented Jan 11, 2018

eordano commented Jan 12, 2018

eordano commented Jan 12, 2018 • edited Loading

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

facuspagnuolo commented Jan 16, 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

facuspagnuolo Jan 17, 2018 • edited Loading

Choose a reason for hiding this comment

frangio Jan 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajile-in commented Jan 25, 2018 • edited Loading

facuspagnuolo commented Dec 24, 2017 •

edited by eternauta1337

Loading

eordano Dec 27, 2017 •

edited

Loading

eordano Dec 27, 2017 •

edited

Loading

facuspagnuolo commented Jan 3, 2018 •

edited

Loading

facuspagnuolo Jan 5, 2018 •

edited

Loading

eordano commented Jan 12, 2018 •

edited

Loading

facuspagnuolo Jan 17, 2018 •

edited

Loading

frangio Jan 17, 2018 •

edited

Loading

ajile-in commented Jan 25, 2018 •

edited

Loading