EIP721

[simondlr]

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]

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]

Fixed @simondlr

[GNSPS]

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

[simondlr]

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

[simondlr]

Seems there's some tiny changes in the standard coming in. Notably, 50k gas is removed (we didn't include it). https://github.com/ethereum/EIPs/issues/721#issuecomment-393212619

[simondlr]

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]

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.

https://github.com/fulldecent/EIPs/commit/27788131d5975daacbab607076f2ee04624f9dbb#diff-a87271f8186c59c075e229cd7421f205

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

[simondlr]

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

[fulldecent]

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