New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EIP 821: Distinguishable Assets Registry (Contract for NFTs) #821

Closed
eordano opened this Issue Jan 5, 2018 · 84 comments

Comments

Projects
None yet
@eordano

eordano commented Jan 5, 2018

EIP: <not assigned>
Title: Distinguishable Assets Registry
Author: Esteban Ordano <esteban@decentraland.org>
Type: Standard Track
Category: ERC
Status: Draft
Created: 2018-01-05
Updated: 2018-02-21

Summary

A Distinguishable Assets Registry (DAR for short) is a contract that tracks ownership of, and information about a set of assets that are distinguishable from each other, and are commonly refferred to as "NFTs", for "Non Fungible Tokens".

See https://github.com/decentraland/erc821 for a reference implementation.

See the "Revisions" section for a history of this ERC.

Abstract

Tracking the ownership of physical or digital distinguishable items on a blockchain has a broad range of applications, from virtual collectibles to physical art pieces. This proposal aims at standardizing a way to reference distinguishable assets along with the foreseeable required operations and interfaces for effective management of those assets on a blockchain.

Introduction

The number of virtual collectibles tracked on the Ethereum blockchain is rapidly growing, creating a demand for a more robust standard for distinguishable digital assets. This proposal suggests improvements to the vocabulary used to refer to such assets, and attempts to provide a solid and future-proof reference implementation of the basic functionality needed. This EIP also proposes better naming conventions and vocabulary for the different components of the NFT economy: the assets, the NFTs (representations of those assets on the blockchain), the DARs (the contracts registering such assets), distinguishing ownership from holding addresses, and more.

See also: ERC #721, ERC #20, ERC #223, and ERC #777.

The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT", "SHOULD", "SHOULD NOT", "RECOMMENDED", "MAY", and "OPTIONAL" in this document are to be interpreted as described in RFC 2119.

Specification

A non-fungible token (NFT) is a distinguishable asset that has a unique representation as a register in a smart contract. This proposal specifies such contracts, referenced as Distinguishable Asset Registries, or DARs. NFTs MAY also be reffered to as "deeds".

DARs can be identified by the blockchain in which they were deployed, and the 160-bit address of the contract instance. NFTs are identified by an ID, a 256 bit number, which MAY correspond to some cryptographic hash of the non-fungible's natural key.

NFTs SHOULD be referenced by a URI that follows this schema:

nft://<chain's common name>/<DAR's address>/<NFT's ID>

The NFT's ID SHOULD have a hint that helps to decode it. For instance, if the encoding of the number is in hexadecimal, the NFT’s ID SHOULD start with 0x. The DAR's address SHOULD follow the checksum by casing as proposed on #55.

Some common names for Ethereum blockchains are:

  • ethereum, livenet, or mainnet
  • ropsten, testnet
  • kovan
  • rinkeby

Some examples of NFT URIs follow:

  • nft://ethereum/0xF87E31492Faf9A91B02Ee0dEAAd50d51d56D5d4d/0
  • nft://ropsten/0xF87E31492Faf9A91B02Ee0dEAAd50d51d56D5d4d/0xfaa5be24e996feadf4c96b905af2c77c456e2debd075bab4d8fd5f70f209de44

Every NFT MUST have a owner address associated with it. NFTs associated with the null address are assumed non-existent or destroyed.

An owner MAY assign one or multiple operator addresses. These addresses will be able to transfer any asset of the owner.

DARs MUST trigger Transfer events every time a NFT's owner changes. This might happen under three circumstances:

  • A NFT is created. In this case, the from value of the Transfer event MUST be the zero address.
  • A NFT is transferred to a different owner.
  • A NFT is destroyed. In this case, the to value of the Transfer event MUST be the zero address.

Transfer events SHALL NOT simultaneously have a zero value in both the from and to fields (this means, the same Transfer event can't both create and destroy a token).

Associated Metadata

Metadata associated with each asset is out of scope for this standard.

DAR global methods

totalSupply():uint256

Return the total amount of assets under this DAR. This method SHALL NOT throw.

supportsInterface(bytes8 interfaceId):bool

This method returns true if the interfaceId is a supported interface (165, corresponding to 0x01ffc9a7, or 821, corresponding to 0x959d7abb). This method SHALL NOT throw.

NFT getter methods

exists(uint256 assetId):bool

This method returns a boolean value, true if the asset identified with the given assetId exists under this DAR. This method SHALL NOT throw.

ownerOf(uint256 assetId):address

This method returns the address of the owner of the NFT. This method SHALL NOT throw. If the assetId does not exist, the return value MUST be the null address.

Owner-centric getter methods

balanceOf(address owner):uint256

This method returns the amount of NFTs held by the owner address under this DAR. This method MUST not throw.

assetByIndex(address owner, uint256 index):uint256

This method returns the ID of the indexth NFT held by the owner address under this DAR, when all the IDs of the NFTs held by such address are stored as an array.

This method MUST throw if assetCount(owner) >= index. This method MUST throw if index >= 2^128.

The DAR MAY change the order assigned to any NFT held by a particular address.

This method is expected to be used by other contracts to iterate through an owner's assets. Contracts implementing such iterations SHOULD be aware of race conditions that might occur, if this iteration happens over multiple transactions.

assetsOf(address owner):uint256[]

This method returns an array of IDs of the NFTs held by owner. This method SHALL NOT throw.

Operator getters

isAuthorizedBy(address operator, address owner):bool

This method returns true if owner has called the method authorize with parameters (operator, true) and has not called authorize(operator, false) afterwards. This method returns false otherwise.

This method MUST return true if operator == owner.

This method SHALL NOT throw.

isApprovedFor(address operator, uint256 assetId):bool

This method returns true if owner has called the method approve with parameters (operator, assetId) and has not called it with a different operator value afterwards. This method returns false otherwise.

This method MUST return true if operator == owner.

This method SHALL NOT throw.

Transfers

transfer(address to, uint256 assetId, bytes userData, bytes operatorData)

Transfers holding of the NFT referenced by assetId from ownerOf(assetId) to the address to.

to SHALL NOT be the zero address. If to is the zero address, the call MUST throw.

to SHALL NOT be ownerOf(assetId). If this condition is met, the call MUST throw.

isAuthorizedBy(msg.sender, ownerOf(assetId)) MUST return true as a precondition.

This means that the msg.sender MUST be ownerOf(assetId) or an authorized operator.

If the NFT referenced by assetId does not exist, then the call MUST throw.

If there was any single authorized operator (see the approve() method below), this authorization MUST be cleared.

If the call doesn't throw, it triggers the event Transfer with the following parameters:

  • from: value of ownerOf(assetId) before the call
  • to: the to argument
  • assetId: the assetId argument
  • operator: msg.sender
  • userData: the userData argument
  • operatorData: the operatorData argument

If to is a contract's address, this call MUST verify that the contract can receive the tokens. In order to do so, the method MUST do a #165 check for support to handle tokens to the target contract.
The implementation for the receiver is as follows:

interface IAssetHolder {
  function onAssetReceived(
    /* address _assetRegistry == msg.sender */
    uint256 _assetId,
    address _previousOwner,
    address _currentOwner,
    bytes   _userData
  ) public;
}

If the supportsInterface method call fails, the transfer must be reversed and this call MUST throw. Otherwise, the method onAssetReceived MUST be invoked with the corresponding information, after the asset has been transferred.

transfer(address to, uint256 assetId, bytes userData)

Shorthand method that MUST be equivalent to calling transfer(to, assetId, userData, EMPTY_BYTES).

transfer(address to, uint256 assetId)

Shorthand method that MUST be equivalent to calling transfer(to, assetId, EMPTY_BYTES, EMPTY_BYTES).

transferFrom(address from, address to, uint256 assetId, bytes userData, bytes operatorData)

Transfers holding of the NFT referenced by assetId from ownerOf(assetId) to the address to, iff from == ownerOf(assetId).

After checking that the asset is owned by the from address, this method MUST behave exactly as calling transfer(to, assetId, EMPTY_BYTES, EMPTY_BYTES).

transferFrom(address from, address to, uint256 assetId, bytes userData)

Shorthand method that MUST be equivalent to calling transferFrom(to, assetId, userData, EMPTY_BYTES).

transferFrom(address from, address to, uint256 assetId)

Shorthand method that MUST be equivalent to calling transferFrom(to, assetId, EMPTY_BYTES, EMPTY_BYTES).

Authorization

authorize(address operator, bool authorized)

If authorized is true, allows operator to transfer any NFT held by msg.sender.

This method MUST throw if operator is the zero address. This method MUST throw if authorized is true and operator is already authorized by the sender. This method MUST throw if authorized is false and operator is unauthorized.

This method MUST throw if msg.sender == operator.

This method MUST trigger an AuthorizeOperator event if it doesn't throw.

approve(address operator, uint256 assetId)

Allow operator to transfer an asset without delegating full access to all assets.

Only the owner of an asset can call this method. Otherwise, the call MUST fail.

Only one address can receive approval at the same time. Clearing approval for a transfer can be done by setting the operator value to the zero address.

This method MUST trigger an Approve event if it doesn't throw.

Events

interface AssetRegistryEvents {
  event Transfer(
    address indexed from,
    address indexed to,
    uint256 indexed assetId,
    address operator,
    bytes userData,
    bytes operatorData
  );
  event AuthorizeOperator(
    address indexed operator,
    address indexed holder,
    bool authorized
  );
  event Approve(
    address indexed owner,
    address indexed operator,
    uint256 indexed assetId
  );
}

Interfaces

interface IAssetRegistry {
  function totalSupply() public view returns (uint256);

  function exists(uint256 assetId) public view returns (bool);
  function ownerOf(uint256 assetId) public view returns (address);

  function balanceOf(address holder) public view returns (uint256);

  function assetByIndex(address holder, uint256 index) public view returns (uint256);
  function assetsOf(address holder) external view returns (uint256[]);

  function transfer(address to, uint256 assetId) public;
  function transfer(address to, uint256 assetId, bytes userData) public;
  function transfer(address to, uint256 assetId, bytes userData, bytes operatorData) public;

  function transferFrom(address from, address to, uint256 assetId) public;
  function transferFrom(address from, address to, uint256 assetId, bytes userData) public;
  function transferFrom(address from, address to, uint256 assetId, bytes userData, bytes operatorData) public;

  function approveAll(address operator, bool authorized) public;
  function approve(address operator, uint256 assetId) public;

  function isAuthorizedBy(address operator, address assetOwner) public view returns (bool);
  function isApprovedFor(address operator, uint256 assetId) public view returns (bool);
  function approvedFor(uint256 assetId) public view returns (address);
}

Implementation

https://github.com/decentraland/erc821

Revisions

  • 2018/02/21: RC2, drop name, symbol, description
  • 2018/02/21: RC1
  • 2018/02/20: Drop metadata
  • 2018/02/20: Add transferFrom
  • 2018/02/20: Try to remove operator from public facing interfaces
  • 2018/02/20: Simplify holder -> owner to use the most common name
  • 2018/02/20: Drop all ERC820 references
  • 2018/02/03: Add approve to approve individual assets to be transferred
  • 2018/01/27: Add exception for msg.sender being the manager
  • 2018/01/27: Add table for ERC820 and IAssetHolder interface check
  • 2018/01/27: Change IAssetOwner for IAssetHolder to reflect that another contract might hold tokens for one
  • 2018/01/27: Errata on transfer text
  • 2018/01/26: Alias "balanceOf" to "assetCount" for ERC20 compatibility
  • 2018/01/26: Add decimals for more ERC20 compatibility
  • 2018/01/26: Propose more metadata to be stored on-chain.
  • 2018/01/26: Make EIP820 compatibility optional to receive tokens if onAssetReceived(...) is implemented.
  • 2018/01/26: Add isERC821 flag to detect support.
  • 2018/01/26: Revert holder to owner.
  • 2018/01/18: transfer: to MUST NOT be holderOf(assetId)
  • 2018/01/17: Added safeAssetData method
  • 2018/01/17: Clarification to transfer: it MUST throw if the asset does not exist.
  • 2018/01/17: Published first version of the specification
  • 2018/01/16: Published implementation
  • 2018/01/05: Initial draft

Copyright

Copyright and related rights waived via CC0.

@ImAllInNow

This comment has been minimized.

Show comment
Hide comment
@ImAllInNow

ImAllInNow Jan 16, 2018

As far as I understand it from the implementation, assetByIndex will constantly change (slightly) as users trade their assets around (since the last item in their asset array is moved around). Is there any real need for the assetByIndex function then since that ordering is not very useful? An indexOfAsset(assetId) function seems more necessary as the main way UI is going to display your assets is to use something like the following:

var assetIds = assetregistry.assetsOf(user);
var sortedAssets = new Array(assetIds.length);
for (var i = 0; i < assetIds.length; ++i) {
    sortedAssets[assetregistry.indexOf(assetIds[i])] = assetId;
}

Edit:
It may be that assetByIndex should be implemented to return the asset such that it's actual id in the _indexOfAsset map == index (which it doesn't in your current implementation). It could be that assetByIndex needs to be implemented more like below:

function assetByIndex(address holder, uint256 index) external view returns (uint256) {
    uint size = _assetsOf[holder].length;
    require(index < size);
    for (uint i = 0; i < size; i++) {
        uint256 asset = _assetOf[holder][i];
        if (_indexOfAsset[asset] == index) {
           return asset;
        }
    }
    assert(1 == 0);
}

ImAllInNow commented Jan 16, 2018

As far as I understand it from the implementation, assetByIndex will constantly change (slightly) as users trade their assets around (since the last item in their asset array is moved around). Is there any real need for the assetByIndex function then since that ordering is not very useful? An indexOfAsset(assetId) function seems more necessary as the main way UI is going to display your assets is to use something like the following:

var assetIds = assetregistry.assetsOf(user);
var sortedAssets = new Array(assetIds.length);
for (var i = 0; i < assetIds.length; ++i) {
    sortedAssets[assetregistry.indexOf(assetIds[i])] = assetId;
}

Edit:
It may be that assetByIndex should be implemented to return the asset such that it's actual id in the _indexOfAsset map == index (which it doesn't in your current implementation). It could be that assetByIndex needs to be implemented more like below:

function assetByIndex(address holder, uint256 index) external view returns (uint256) {
    uint size = _assetsOf[holder].length;
    require(index < size);
    for (uint i = 0; i < size; i++) {
        uint256 asset = _assetOf[holder][i];
        if (_indexOfAsset[asset] == index) {
           return asset;
        }
    }
    assert(1 == 0);
}
@ImAllInNow

This comment has been minimized.

Show comment
Hide comment
@ImAllInNow

ImAllInNow Jan 16, 2018

I don't think create() (and possibly update()) should necessarily be part of a standard ERC821 token. They seem optional, and for many assets, their creation probably won't be a publicly accessible function and updating might not be relevant for that contract.

Edit:
Thinking about it more, I think there is a potential to front-run the create() function too. If a malicious user sees a create call being made pending, they could create that same asset for themselves first. It would make managing these assets difficult to have to try and ignore maliciously added assetIds.

ImAllInNow commented Jan 16, 2018

I don't think create() (and possibly update()) should necessarily be part of a standard ERC821 token. They seem optional, and for many assets, their creation probably won't be a publicly accessible function and updating might not be relevant for that contract.

Edit:
Thinking about it more, I think there is a potential to front-run the create() function too. If a malicious user sees a create call being made pending, they could create that same asset for themselves first. It would make managing these assets difficult to have to try and ignore maliciously added assetIds.

@eordano

This comment has been minimized.

Show comment
Hide comment
@eordano

eordano Jan 16, 2018

Thanks @ImAllInNow!

  1. Regarding assetByIndex. The idea behind this is to have that accessed from a smart contract. Use case: access from another contract for migrating all assets in batch mode to a new owner. allAssets might consume too much gas, or kill the evm stack

  2. You're right about create. Front-running makes it a no-go. I'll keep them in the standard implementation as internal.

eordano commented Jan 16, 2018

Thanks @ImAllInNow!

  1. Regarding assetByIndex. The idea behind this is to have that accessed from a smart contract. Use case: access from another contract for migrating all assets in batch mode to a new owner. allAssets might consume too much gas, or kill the evm stack

  2. You're right about create. Front-running makes it a no-go. I'll keep them in the standard implementation as internal.

@eordano

This comment has been minimized.

Show comment
Hide comment
@eordano

eordano Jan 16, 2018

Regarding update and the metadata: is there really a case where no errata would be published? For my quick checklist:

  • Real world art pieces: update is good for appending new information (change of location, status, news)
  • Virtual trading card games: update is great for amendment on card rules
  • Virtual collectibles: update is used for errata or fixing typos
  • Decentraland Land: update is used to store the data for the land

The rule of the owner being the one authorized to make the changes is policy. You're right, it should not be part of the standard (same as before, I'll keep the mechanism as internal so the implementor can decide on the policy)

eordano commented Jan 16, 2018

Regarding update and the metadata: is there really a case where no errata would be published? For my quick checklist:

  • Real world art pieces: update is good for appending new information (change of location, status, news)
  • Virtual trading card games: update is great for amendment on card rules
  • Virtual collectibles: update is used for errata or fixing typos
  • Decentraland Land: update is used to store the data for the land

The rule of the owner being the one authorized to make the changes is policy. You're right, it should not be part of the standard (same as before, I'll keep the mechanism as internal so the implementor can decide on the policy)

@eordano

This comment has been minimized.

Show comment
Hide comment
@eordano

eordano Jan 16, 2018

Last note on assetsOf: there's a problem of concurrency, it would be nice to have a function that atomically returns all assetIds and all the associated data (otherwise if the order changes between fetches the UI will display erroneous information).

I'll add a note that UIs should list the items in sorted order, or keep their own information based on the transfer logs or user preferences.

eordano commented Jan 16, 2018

Last note on assetsOf: there's a problem of concurrency, it would be nice to have a function that atomically returns all assetIds and all the associated data (otherwise if the order changes between fetches the UI will display erroneous information).

I'll add a note that UIs should list the items in sorted order, or keep their own information based on the transfer logs or user preferences.

@eordano eordano changed the title from EIP (draft): Distinguishable Assets Registry to EIP 821: Distinguishable Assets Registry (Contract for NFTs) Jan 16, 2018

@ImAllInNow

This comment has been minimized.

Show comment
Hide comment
@ImAllInNow

ImAllInNow Jan 16, 2018

@eordano You're right about update. I was getting it confused with updating the assetId rather than the data associated with the assetId.

ImAllInNow commented Jan 16, 2018

@eordano You're right about update. I was getting it confused with updating the assetId rather than the data associated with the assetId.

@abcoathup

This comment has been minimized.

Show comment
Hide comment
@abcoathup

abcoathup Jan 17, 2018

For those coming straight to ERC821 @eordano has written a distinguishable asset manifesto: https://blog.decentraland.org/the-non-fungibles-revolution-of-2018-304270525b05

abcoathup commented Jan 17, 2018

For those coming straight to ERC821 @eordano has written a distinguishable asset manifesto: https://blog.decentraland.org/the-non-fungibles-revolution-of-2018-304270525b05

@abcoathup

This comment has been minimized.

Show comment
Hide comment
@abcoathup

abcoathup Jan 17, 2018

My original thinking for (unique) assets was given existing ERC20 support in wallets was to create assets that are ERC20 compatible, rather than get wallets to support (unique) assets but if we can get enough support for a unique assets standard for wallets to implement, then all the better.

abcoathup commented Jan 17, 2018

My original thinking for (unique) assets was given existing ERC20 support in wallets was to create assets that are ERC20 compatible, rather than get wallets to support (unique) assets but if we can get enough support for a unique assets standard for wallets to implement, then all the better.

@abcoathup

This comment has been minimized.

Show comment
Hide comment
@abcoathup

abcoathup Jan 17, 2018

Cost
For me reducing the cost of creation and send are hugely important. So want to think about how the standard impacts the implementation and hence the cost.

ERC821 meta data
(Similar to my comments on ERC721)
For assets to be displayed in wallets we ideally want a name, logo and possibly a symbol representing the assets in ERC21 registry.
MetaMask uses name and logo with optional symbol in their metadata. https://github.com/MetaMask/eth-contract-metadata
Ideally the logo would be non-optional, though there will likely be use cases where assets won’t want to be displayed in wallets

CUD
CUD access may depend on the implementation. E.g.:
Create may only be called by the contract owner.
Update may only be called by the contract owner or the asset owner depending on what is in data.
Destroy may only be called by the asset owner.

Send
Does the standard need two send functions?

abcoathup commented Jan 17, 2018

Cost
For me reducing the cost of creation and send are hugely important. So want to think about how the standard impacts the implementation and hence the cost.

ERC821 meta data
(Similar to my comments on ERC721)
For assets to be displayed in wallets we ideally want a name, logo and possibly a symbol representing the assets in ERC21 registry.
MetaMask uses name and logo with optional symbol in their metadata. https://github.com/MetaMask/eth-contract-metadata
Ideally the logo would be non-optional, though there will likely be use cases where assets won’t want to be displayed in wallets

CUD
CUD access may depend on the implementation. E.g.:
Create may only be called by the contract owner.
Update may only be called by the contract owner or the asset owner depending on what is in data.
Destroy may only be called by the asset owner.

Send
Does the standard need two send functions?

@eordano

This comment has been minimized.

Show comment
Hide comment
@eordano

eordano Jan 17, 2018

Thanks for the comments, @abcoathup! Please note that I've updated the body of the proposal to include as much detail as I thought necessary

  1. Cost: I can run some benchmarks tomorrow and give you some estimate

  2. Should we add a logo? Should there be some kind of specification, perhaps following iOS or Android's standard for logo sizes?

  3. CUD: Updated to reflect your thoughts. Note:

Our implementation tries to follow the UNIX principle of providing mechanisms, not policy. That's why it contains functions to create, update, and destroy tokens, but they are marked as internal.

  1. Does it save a significant amount of gas? I think not, except perhaps for calls between contracts?

eordano commented Jan 17, 2018

Thanks for the comments, @abcoathup! Please note that I've updated the body of the proposal to include as much detail as I thought necessary

  1. Cost: I can run some benchmarks tomorrow and give you some estimate

  2. Should we add a logo? Should there be some kind of specification, perhaps following iOS or Android's standard for logo sizes?

  3. CUD: Updated to reflect your thoughts. Note:

Our implementation tries to follow the UNIX principle of providing mechanisms, not policy. That's why it contains functions to create, update, and destroy tokens, but they are marked as internal.

  1. Does it save a significant amount of gas? I think not, except perhaps for calls between contracts?
@abcoathup

This comment has been minimized.

Show comment
Hide comment
@abcoathup

abcoathup Jan 17, 2018

@eordano rather than description for the DAR, I was thinking this should be data (metaData) in the same format as the data for the NFT and include a logo.

Suggest reaching out to wallets and dApp browsers (e.g. the likes of MetaMask, Toshi, Status.im and Cipher Browser) for input on logo specifications.

abcoathup commented Jan 17, 2018

@eordano rather than description for the DAR, I was thinking this should be data (metaData) in the same format as the data for the NFT and include a logo.

Suggest reaching out to wallets and dApp browsers (e.g. the likes of MetaMask, Toshi, Status.im and Cipher Browser) for input on logo specifications.

@abcoathup

This comment has been minimized.

Show comment
Hide comment
@abcoathup

abcoathup Jan 17, 2018

Is there a need for both holderOf and 'safeHolderOf' when there is an exists?

Should all functions that accept an assetId require that it exists so there is consistent behavior?

abcoathup commented Jan 17, 2018

Is there a need for both holderOf and 'safeHolderOf' when there is an exists?

Should all functions that accept an assetId require that it exists so there is consistent behavior?

@eordano

This comment has been minimized.

Show comment
Hide comment
@eordano

eordano Jan 17, 2018

safeHolderOf is a cheaper way of calling exists and holderOf.

Discussion comes from #721 throwing on ownerOf if the asset doesn't exist. I think this prevented exists from being calculated.

My take on this is that both are valid use cases... that you might want to save some gas by just calling safeHolderOf, but that there might be some useful way of checking holderOf and calculating exists yourself. For example, when used as an external function and you want to do only one roundtrip.

The other functions that accept an assetId are not relevantly modified by this behavior, except maybe on the case of assetData, but it might be expected that safeHolderOf would be called first, or the logic implemented "client"-side.

eordano commented Jan 17, 2018

safeHolderOf is a cheaper way of calling exists and holderOf.

Discussion comes from #721 throwing on ownerOf if the asset doesn't exist. I think this prevented exists from being calculated.

My take on this is that both are valid use cases... that you might want to save some gas by just calling safeHolderOf, but that there might be some useful way of checking holderOf and calculating exists yourself. For example, when used as an external function and you want to do only one roundtrip.

The other functions that accept an assetId are not relevantly modified by this behavior, except maybe on the case of assetData, but it might be expected that safeHolderOf would be called first, or the logic implemented "client"-side.

@AnthonyAkentiev

This comment has been minimized.

Show comment
Hide comment
@AnthonyAkentiev

AnthonyAkentiev Jan 17, 2018

Thx for the great idea.
I think that you have to describe how #821 is different/better from the #721.

As long as #721 was added before, it is your goal to clearly describe why you decided to add new non-fungible token standard. Thx again.

AnthonyAkentiev commented Jan 17, 2018

Thx for the great idea.
I think that you have to describe how #821 is different/better from the #721.

As long as #721 was added before, it is your goal to clearly describe why you decided to add new non-fungible token standard. Thx again.

@eordano

This comment has been minimized.

Show comment
Hide comment
@eordano

eordano Jan 17, 2018

I'll expand later, but for the time being:

  1. If I have 100 NFTs, calling approve 100 times with the same to value (a decentralized exchange probably) is very inefficient, specially given that it's usually used that way (see Etherdelta, 0x)
  2. Lack of exists function in #721 -- There's no way to do a "try-catch" for ownerOf from a contract.
  3. Lack of transferAndCall functionality -- More than 4 million USD have been lost according to https://github.com/Dexaran/ERC223-token-standard. It's critical that we have a way to not loose tokens for sending them to a contract that doesn't handle it.
  4. Possibility to log more information in the Transfer event, following #777

eordano commented Jan 17, 2018

I'll expand later, but for the time being:

  1. If I have 100 NFTs, calling approve 100 times with the same to value (a decentralized exchange probably) is very inefficient, specially given that it's usually used that way (see Etherdelta, 0x)
  2. Lack of exists function in #721 -- There's no way to do a "try-catch" for ownerOf from a contract.
  3. Lack of transferAndCall functionality -- More than 4 million USD have been lost according to https://github.com/Dexaran/ERC223-token-standard. It's critical that we have a way to not loose tokens for sending them to a contract that doesn't handle it.
  4. Possibility to log more information in the Transfer event, following #777
@dete

This comment has been minimized.

Show comment
Hide comment
@dete

dete Jan 17, 2018

Lots of good thinking has gone into this. Decentraland brings a perspective to NFTs that we don't have at CryptoKitties. Thanks for bringing this to the community, @eordano!

I'm confused as to why you think this needs to be an whole new standard, tho. I don't know about anyone else, but I don't consider ERC-721 as finalized, and I'm not sure it's appropriate to have a whole new standard that is largely the same.

I appreciate that I've been a bad steward of ERC-721 for the last few weeks, and I can see how you might be frustrated by the lack of forward progress, but I would rather we find a way to move forward with one standard, rather than two. I mean, it's kind of the point of standards! 😅

dete commented Jan 17, 2018

Lots of good thinking has gone into this. Decentraland brings a perspective to NFTs that we don't have at CryptoKitties. Thanks for bringing this to the community, @eordano!

I'm confused as to why you think this needs to be an whole new standard, tho. I don't know about anyone else, but I don't consider ERC-721 as finalized, and I'm not sure it's appropriate to have a whole new standard that is largely the same.

I appreciate that I've been a bad steward of ERC-721 for the last few weeks, and I can see how you might be frustrated by the lack of forward progress, but I would rather we find a way to move forward with one standard, rather than two. I mean, it's kind of the point of standards! 😅

@eordano

This comment has been minimized.

Show comment
Hide comment
@eordano

eordano Jan 17, 2018

Hey Dieter, I thought this was a better way to express the wholeness of the changes I was proposing (as you see, they are quite a lot).

The three things were I think we've got completely different approaches are:

  • Using authorizeOperator instead of individually approve-ing every asset
  • The need for transfer to prevent tokens from getting lost to contracts that can't handle them
  • Upgradability through #820 w

eordano commented Jan 17, 2018

Hey Dieter, I thought this was a better way to express the wholeness of the changes I was proposing (as you see, they are quite a lot).

The three things were I think we've got completely different approaches are:

  • Using authorizeOperator instead of individually approve-ing every asset
  • The need for transfer to prevent tokens from getting lost to contracts that can't handle them
  • Upgradability through #820 w
@dete

This comment has been minimized.

Show comment
Hide comment
@dete

dete Jan 17, 2018

Using authorizeOperator instead of individually approve-ing every asset

I like this! I haven't had a chance to integrate it into the 721 draft, however.

The need for transfer to prevent tokens from getting lost to contracts that can't handle them

Unfortunately, we do disagree on this. I think that preventing user errors is a porcelain problem, not a plumbing problem. It's not the job of the filesystem to decide if the file you've named .jpg is actually compliant with the JPEG standard. (That's admittedly an exaggerated analogy, but I don't think Ethereum will ever see mass adoption if we continue to assume that users will be directly interacting with smart contracts!)

Upgradability through #820

EIP-820 is obviously very new, and looks pretty smart. So far as I can tell, it doesn't dictate anything about how an interface is structured, though, so it should be possible to use it for any standard past or future (even good ol' ERC-20).

dete commented Jan 17, 2018

Using authorizeOperator instead of individually approve-ing every asset

I like this! I haven't had a chance to integrate it into the 721 draft, however.

The need for transfer to prevent tokens from getting lost to contracts that can't handle them

Unfortunately, we do disagree on this. I think that preventing user errors is a porcelain problem, not a plumbing problem. It's not the job of the filesystem to decide if the file you've named .jpg is actually compliant with the JPEG standard. (That's admittedly an exaggerated analogy, but I don't think Ethereum will ever see mass adoption if we continue to assume that users will be directly interacting with smart contracts!)

Upgradability through #820

EIP-820 is obviously very new, and looks pretty smart. So far as I can tell, it doesn't dictate anything about how an interface is structured, though, so it should be possible to use it for any standard past or future (even good ol' ERC-20).

@ImAllInNow

This comment has been minimized.

Show comment
Hide comment
@ImAllInNow

ImAllInNow Jan 18, 2018

I agree with @dete that moving towards one NFT standard is preferable to multiple standards especially when this concept is new and has not been specifically supported by wallets/exchanges yet (that I know of).

Definitely some good improvements suggested in the interface as described here. I like the use of an authorized operator and the idea of storing an index mapping to allow for a users assets to be stored as an array for easy retrieval through MetaMask without timing out when there are large numbers of assets (I recently noticed I can't retrieve all CryptoKitties for a user anymore :( ).

I think deciding to prevent user transfer errors can be left up to the contract itself. It is a slight extra cost to make this check so if a contract wants to add to the gas cost of transfers to protect their users, that's great, but I don't think it necessarily needs to be mandated in the standard.

ImAllInNow commented Jan 18, 2018

I agree with @dete that moving towards one NFT standard is preferable to multiple standards especially when this concept is new and has not been specifically supported by wallets/exchanges yet (that I know of).

Definitely some good improvements suggested in the interface as described here. I like the use of an authorized operator and the idea of storing an index mapping to allow for a users assets to be stored as an array for easy retrieval through MetaMask without timing out when there are large numbers of assets (I recently noticed I can't retrieve all CryptoKitties for a user anymore :( ).

I think deciding to prevent user transfer errors can be left up to the contract itself. It is a slight extra cost to make this check so if a contract wants to add to the gas cost of transfers to protect their users, that's great, but I don't think it necessarily needs to be mandated in the standard.

@simondlr

This comment has been minimized.

Show comment
Hide comment
@simondlr

simondlr Jan 18, 2018

I think that preventing user errors is a porcelain problem, not a plumbing problem.

I think regardless whether the UI should prevent the user from doing this (I think it should), it's still valuable to have another contract be notified with only requiring 1 transaction (from a plumbing perspective).

simondlr commented Jan 18, 2018

I think that preventing user errors is a porcelain problem, not a plumbing problem.

I think regardless whether the UI should prevent the user from doing this (I think it should), it's still valuable to have another contract be notified with only requiring 1 transaction (from a plumbing perspective).

@macalinao

This comment has been minimized.

Show comment
Hide comment
@macalinao

macalinao Jan 21, 2018

Have you considered implementing something for lienholders?

Let's say you have a plot of LAND that you are using as collateral for a loan, e.g. a mortgage. What if there was a way to allow the lender contract to transfer ownership of a plot of LAND without the land owner being able to revoke this access?

macalinao commented Jan 21, 2018

Have you considered implementing something for lienholders?

Let's say you have a plot of LAND that you are using as collateral for a loan, e.g. a mortgage. What if there was a way to allow the lender contract to transfer ownership of a plot of LAND without the land owner being able to revoke this access?

@shrugs

This comment has been minimized.

Show comment
Hide comment
@shrugs

shrugs Jan 21, 2018

I expect lienholders (new word for me!) to be a strong usecase for NFTs, especially as we move to a tokenized future. Think loans collateralized by fine art (or, hell, cryptokitties). Same with, say, propy houses.

It'd be possible to create that functionality with a "wrapper" contract (like an escrow), but that's not entirely ideal.

Could either be an optional extension to this EIP (nice) or another EIP (long process).

shrugs commented Jan 21, 2018

I expect lienholders (new word for me!) to be a strong usecase for NFTs, especially as we move to a tokenized future. Think loans collateralized by fine art (or, hell, cryptokitties). Same with, say, propy houses.

It'd be possible to create that functionality with a "wrapper" contract (like an escrow), but that's not entirely ideal.

Could either be an optional extension to this EIP (nice) or another EIP (long process).

@hyperfekt

This comment has been minimized.

Show comment
Hide comment
@hyperfekt

hyperfekt Jan 22, 2018

I suggest that every ERC821 asset registry must register the "IAssetRegistry" interface with ERC820, that way contracts can automatically discern if a given NFT registry conforms to this standard.
An example of how that would be used: https://github.com/hyperfekt/ethereum-auctions/blob/master/NFTAuction.sol#L29

hyperfekt commented Jan 22, 2018

I suggest that every ERC821 asset registry must register the "IAssetRegistry" interface with ERC820, that way contracts can automatically discern if a given NFT registry conforms to this standard.
An example of how that would be used: https://github.com/hyperfekt/ethereum-auctions/blob/master/NFTAuction.sol#L29

@AusIV

This comment has been minimized.

Show comment
Hide comment
@AusIV

AusIV Jan 24, 2018

What is the operatorData argument of transfer() for? The description of "the operatorData argument" is a self-referential description, and says nothing about what data goes there.

I'm kind of assuming it's just a blob of information that people can use when they need to communicate something not defined by the spec, but I feel like it needs more clarification.

AusIV commented Jan 24, 2018

What is the operatorData argument of transfer() for? The description of "the operatorData argument" is a self-referential description, and says nothing about what data goes there.

I'm kind of assuming it's just a blob of information that people can use when they need to communicate something not defined by the spec, but I feel like it needs more clarification.

@AusIV

This comment has been minimized.

Show comment
Hide comment
@AusIV

AusIV Jan 24, 2018

I feel like there's still room for an approve() call.

Calling recipient.onAssetReceived() eliminates part of the need for approve() in that you an send an asset and invoke the recipient in the same call, rather than needing to call approve(), invoke a separate contract and have it execute transferFrom().

Having authorizeOperator() addresses another part of the need for approve(), for the scenario where there may be a considerable period of time between authorization and transfer (such as waiting for an order to be filled on an exchange). But I feel authorizing another account to transfer all of your registered assets, much like authorizing a contract to transfer an unlimited number of your ERC20 tokens, is a bit of an anti-pattern. It saves gas, which is obviously not insignificant, but it also means that if the security of authorized operator is compromised all assets are subject to transfer, not just the ones you intended that operator to transfer on your behalf.

In light of the numerous security compromises we've already seen (the DAO hack, Parity Multisig Wallets Rounds 1 and 2) building a standard on the assumption that contracts will always work as intended seems short sighted. An approve() method would allow people to be more conservative, only risking assets they anticipate transferring in the event that the operator contract gets compromised. That would come at a higher gas cost for the authorizer, but if they are confident in the contract they are authorizing, they would still have the option to call authorizeOperator().

AusIV commented Jan 24, 2018

I feel like there's still room for an approve() call.

Calling recipient.onAssetReceived() eliminates part of the need for approve() in that you an send an asset and invoke the recipient in the same call, rather than needing to call approve(), invoke a separate contract and have it execute transferFrom().

Having authorizeOperator() addresses another part of the need for approve(), for the scenario where there may be a considerable period of time between authorization and transfer (such as waiting for an order to be filled on an exchange). But I feel authorizing another account to transfer all of your registered assets, much like authorizing a contract to transfer an unlimited number of your ERC20 tokens, is a bit of an anti-pattern. It saves gas, which is obviously not insignificant, but it also means that if the security of authorized operator is compromised all assets are subject to transfer, not just the ones you intended that operator to transfer on your behalf.

In light of the numerous security compromises we've already seen (the DAO hack, Parity Multisig Wallets Rounds 1 and 2) building a standard on the assumption that contracts will always work as intended seems short sighted. An approve() method would allow people to be more conservative, only risking assets they anticipate transferring in the event that the operator contract gets compromised. That would come at a higher gas cost for the authorizer, but if they are confident in the contract they are authorizing, they would still have the option to call authorizeOperator().

@eordano

This comment has been minimized.

Show comment
Hide comment
@eordano

eordano Jan 27, 2018

@AusIV the operatorData is there for protocols like 0x where you'd like to log some information about why the transaction was successful, for better auditability. For example, you could have the market making order be the operatorData, and the userData would be whatever arguments you want to pass on to the contract receiving the asset.

I think you're right about allowing approve as well as approveAll. I was thinking that one could just have a "filter" contract that can take care of this granularity, to save the complexity of implementing that to the DAR, but now that I think of it again, I'm not convinced. What do you think?

eordano commented Jan 27, 2018

@AusIV the operatorData is there for protocols like 0x where you'd like to log some information about why the transaction was successful, for better auditability. For example, you could have the market making order be the operatorData, and the userData would be whatever arguments you want to pass on to the contract receiving the asset.

I think you're right about allowing approve as well as approveAll. I was thinking that one could just have a "filter" contract that can take care of this granularity, to save the complexity of implementing that to the DAR, but now that I think of it again, I'm not convinced. What do you think?

@eordano

This comment has been minimized.

Show comment
Hide comment
@eordano

eordano Jan 27, 2018

@hyperfekt I think the correct way to go about this is that the DAR should register the ERC821 interface (an empty interface, just using it for the name) as a way of signaling that it implements the standard... what do you think? In any case, I added the isERC821() method because of feedback from potential users that they want to have an easier way to check for compatibility.

eordano commented Jan 27, 2018

@hyperfekt I think the correct way to go about this is that the DAR should register the ERC821 interface (an empty interface, just using it for the name) as a way of signaling that it implements the standard... what do you think? In any case, I added the isERC821() method because of feedback from potential users that they want to have an easier way to check for compatibility.

@jashmenn

This comment has been minimized.

Show comment
Hide comment
@jashmenn

jashmenn Feb 17, 2018

I've started a Google doc that attempts to show the compatibility table between ERC-721 NFTs / #841 Deeds / #821 etc. Here's the link. Feel free to comment or request edit access.

Unfortunately I can't make it to the meeting at ETHDenver, but maybe this chart can be helpful there.

screenshot 2018-02-17 15 08 38

/cc @fulldecent

jashmenn commented Feb 17, 2018

I've started a Google doc that attempts to show the compatibility table between ERC-721 NFTs / #841 Deeds / #821 etc. Here's the link. Feel free to comment or request edit access.

Unfortunately I can't make it to the meeting at ETHDenver, but maybe this chart can be helpful there.

screenshot 2018-02-17 15 08 38

/cc @fulldecent

@fulldecent

This comment has been minimized.

Show comment
Hide comment
@fulldecent

fulldecent Feb 18, 2018

Contributor

I do not like what happened with erc721.org. There are several problems and inaccuracies:

  • This should definitely be a project that is open source / hosted from GitHub or some other open access mechanism and which allows collaboration (and the domain is registered under privacy)
  • It presents a brand new interface, which is not discussed anywhere -- EIP is an established process for discussing ERCs, see EIP-1 -- but this is just a website that does not support collaboration
  • "ERC-721 has since moved out of beta and into a finalized v1 spec" is misleading at best -- this appears to be intentionally circumventing the EIP process -- a correct statement might be "ERC-721 is an established standards-track draft" (and that is even premature because the "standard" on that webpage is nowhere on github)
  • The effort put into that website is significantly less than the discussions in #821 and #721, this hurts adoption

I plan to have an updated specification at #721 soon which nearly matches the ideas on erc721.org. In addition, it will be an actual standard. Please get a hold of me and let's work on this together and at the same time recognize the benefits of collaboration and of respecting the EIP process.

Contributor

fulldecent commented Feb 18, 2018

I do not like what happened with erc721.org. There are several problems and inaccuracies:

  • This should definitely be a project that is open source / hosted from GitHub or some other open access mechanism and which allows collaboration (and the domain is registered under privacy)
  • It presents a brand new interface, which is not discussed anywhere -- EIP is an established process for discussing ERCs, see EIP-1 -- but this is just a website that does not support collaboration
  • "ERC-721 has since moved out of beta and into a finalized v1 spec" is misleading at best -- this appears to be intentionally circumventing the EIP process -- a correct statement might be "ERC-721 is an established standards-track draft" (and that is even premature because the "standard" on that webpage is nowhere on github)
  • The effort put into that website is significantly less than the discussions in #821 and #721, this hurts adoption

I plan to have an updated specification at #721 soon which nearly matches the ideas on erc721.org. In addition, it will be an actual standard. Please get a hold of me and let's work on this together and at the same time recognize the benefits of collaboration and of respecting the EIP process.

@tomhschmidt

This comment has been minimized.

Show comment
Hide comment
@tomhschmidt

tomhschmidt Feb 18, 2018

Hey @fulldecent -- I made erc721.org for the ETH Denver hackathon.

erc721.org is currently being served using GH Pages from this GH repo: https://github.com/0xProject/erc721-website. The site could definitely use some improvements, so feel free to submit a PR if there are changes you'd like to see!

As @dekz mentioned in the discussion under EIP-721, many in the community are worried by the fragmentation of NFT standards. It's currently extremely confusing and frustrating for new developers to get started, and equally frustrating for contributors like 0x and OpenZeppelin to be able to support the community when many people -- most of whom aren't even building NFTs or NFT-related projects -- are dragging their heels on committing to a standard or are continuing to expand the scope of this standard into unrelated areas.

As shown on the site, large number of community members from all sides of the NFT ecosystem had a chance to meet at ETH Denver and agree to a minimum viable spec necessary to support the current digital NFT use case that also adds sufficient functionally to receive support from other teams like 0x, OpenZeppelin, and Toshi. We will all implement this spec or support for this spec in the immediate future.

@Recmo and @abandeali1 are currently working on PRs for a formal ERC and @spalladino's team is updating the OpenZeppelin solidity contracts to support this standard. We'd love to have the community's support in spreading and canonicalizing the final version of this standard.

tomhschmidt commented Feb 18, 2018

Hey @fulldecent -- I made erc721.org for the ETH Denver hackathon.

erc721.org is currently being served using GH Pages from this GH repo: https://github.com/0xProject/erc721-website. The site could definitely use some improvements, so feel free to submit a PR if there are changes you'd like to see!

As @dekz mentioned in the discussion under EIP-721, many in the community are worried by the fragmentation of NFT standards. It's currently extremely confusing and frustrating for new developers to get started, and equally frustrating for contributors like 0x and OpenZeppelin to be able to support the community when many people -- most of whom aren't even building NFTs or NFT-related projects -- are dragging their heels on committing to a standard or are continuing to expand the scope of this standard into unrelated areas.

As shown on the site, large number of community members from all sides of the NFT ecosystem had a chance to meet at ETH Denver and agree to a minimum viable spec necessary to support the current digital NFT use case that also adds sufficient functionally to receive support from other teams like 0x, OpenZeppelin, and Toshi. We will all implement this spec or support for this spec in the immediate future.

@Recmo and @abandeali1 are currently working on PRs for a formal ERC and @spalladino's team is updating the OpenZeppelin solidity contracts to support this standard. We'd love to have the community's support in spreading and canonicalizing the final version of this standard.

@fulldecent

This comment has been minimized.

Show comment
Hide comment
@fulldecent

fulldecent Feb 18, 2018

Contributor
Contributor

fulldecent commented Feb 18, 2018

@rca03

This comment has been minimized.

Show comment
Hide comment
@rca03

rca03 Feb 19, 2018

The 0x team has demonstrated a pattern of trying to run roughshod over any semblance of process to establishing a standard here. It is ultimately self defeating.

rca03 commented Feb 19, 2018

The 0x team has demonstrated a pattern of trying to run roughshod over any semblance of process to establishing a standard here. It is ultimately self defeating.

@willwarren89

This comment has been minimized.

Show comment
Hide comment
@willwarren89

willwarren89 Feb 19, 2018

It is unfortunate that not everyone was able to attend the working group. It was never intended to be some sort of official process. That being said, it was certainly not opaque. Almost all of the NFT projects were represented and, IMO, it was a huge relief to be able to communicate directly with these projects IRL rather than through this thread, which has become noisy and lacks focus.

It was an incredibly productive session. We walked through the issues one by one, discussed potential solutions (most of which are already buried within this thread), and ensured that every project felt comfortable with a proposed solution before moving on. What we have converged on at erc721.org is a general interface that satisfies everyone's use cases and needs.

willwarren89 commented Feb 19, 2018

It is unfortunate that not everyone was able to attend the working group. It was never intended to be some sort of official process. That being said, it was certainly not opaque. Almost all of the NFT projects were represented and, IMO, it was a huge relief to be able to communicate directly with these projects IRL rather than through this thread, which has become noisy and lacks focus.

It was an incredibly productive session. We walked through the issues one by one, discussed potential solutions (most of which are already buried within this thread), and ensured that every project felt comfortable with a proposed solution before moving on. What we have converged on at erc721.org is a general interface that satisfies everyone's use cases and needs.

@rca03

This comment has been minimized.

Show comment
Hide comment
@rca03

rca03 Feb 19, 2018

@willwarren89 There are many projects that are not accounted for in your summary representation (including private, commercial B2B use cases)—this is not to make a value judgement on the significant contributions made by the hastily organized working group over the weekend.

My only caution here would be towards the lack of participation in the well-established, community EIP process by this select cohort. Deviating from that established process only furthers the risk of fragmentation.

rca03 commented Feb 19, 2018

@willwarren89 There are many projects that are not accounted for in your summary representation (including private, commercial B2B use cases)—this is not to make a value judgement on the significant contributions made by the hastily organized working group over the weekend.

My only caution here would be towards the lack of participation in the well-established, community EIP process by this select cohort. Deviating from that established process only furthers the risk of fragmentation.

@shrugs

This comment has been minimized.

Show comment
Hide comment
@shrugs

shrugs Feb 19, 2018

For context, the plan is to work within the EIP process as a join effort by the stewards of 721 and 821.

Regarding private, commercial, etc requirements, the plan (as I understand it) is to produce a standard that is as general as possible and allow additional features (like fine-grain access control, pull-payments, etc) as layers as additional EIP standards.

shrugs commented Feb 19, 2018

For context, the plan is to work within the EIP process as a join effort by the stewards of 721 and 821.

Regarding private, commercial, etc requirements, the plan (as I understand it) is to produce a standard that is as general as possible and allow additional features (like fine-grain access control, pull-payments, etc) as layers as additional EIP standards.

@fulldecent

This comment has been minimized.

Show comment
Hide comment
@fulldecent

fulldecent Feb 19, 2018

Contributor

@willwarren89:

It was never intended to be some sort of official process.

The artifact of this meeting, erc721.org includes the statements that make this appear as an official process.

screen shot 2018-02-18 at 8 03 57 pm

screen shot 2018-02-18 at 8 04 05 pm


To be clear. I am interpreting this as a good-faith effort to get shit done, with no intentional motive of excluding the community at large.

Anyway, right now I am working on turning that website into an actual usable standard at #841. It is already pretty close.

At that time (coming tonight or tomorrow) I will circle back here. Also I will seek to credit any authors at the top of the spec if I have lifted your words. Then I will propose that everyone come together on this under the name 721. I see 0x has purchased domain names with 721, so I think we can get this done.

Contributor

fulldecent commented Feb 19, 2018

@willwarren89:

It was never intended to be some sort of official process.

The artifact of this meeting, erc721.org includes the statements that make this appear as an official process.

screen shot 2018-02-18 at 8 03 57 pm

screen shot 2018-02-18 at 8 04 05 pm


To be clear. I am interpreting this as a good-faith effort to get shit done, with no intentional motive of excluding the community at large.

Anyway, right now I am working on turning that website into an actual usable standard at #841. It is already pretty close.

At that time (coming tonight or tomorrow) I will circle back here. Also I will seek to credit any authors at the top of the spec if I have lifted your words. Then I will propose that everyone come together on this under the name 721. I see 0x has purchased domain names with 721, so I think we can get this done.

@dekz

This comment has been minimized.

Show comment
Hide comment
@dekz

dekz Feb 19, 2018

To be clear, this is the first time a diverse representation of the community has sat down together and actually worked on a NFT proposal as a group together.

Let's try and get these Github issues back on track and see if they can be as productive as the Working group was.

dekz commented Feb 19, 2018

To be clear, this is the first time a diverse representation of the community has sat down together and actually worked on a NFT proposal as a group together.

Let's try and get these Github issues back on track and see if they can be as productive as the Working group was.

@amittmahajan

This comment has been minimized.

Show comment
Hide comment
@amittmahajan

amittmahajan Feb 19, 2018

This works for us (RareBits, https://rarebits.io) and has our support. As an ERC721 marketplace, we have interacted with a lot of these contracts that are live and are supportive of this effort to standardize the implementation. While this doesn't include everything that the community will want eventually, it at least it locks in a "v1" API that devs can build against safely.

amittmahajan commented Feb 19, 2018

This works for us (RareBits, https://rarebits.io) and has our support. As an ERC721 marketplace, we have interacted with a lot of these contracts that are live and are supportive of this effort to standardize the implementation. While this doesn't include everything that the community will want eventually, it at least it locks in a "v1" API that devs can build against safely.

@hyperfekt

This comment has been minimized.

Show comment
Hide comment
@hyperfekt

hyperfekt Feb 19, 2018

Is my understanding correct that the intended process by the 'working group' is to now discuss their interface from the very beginning instead of continuing the work / understanding already done / come to here? This would include everything agreed upon at the meeting, since there is no discussion available for these decisions.
To me that seems highly counterproductive. If you could instead produce a document that clearly outlines all your considerations so others can follow them and benefit from the work done, that should expedite the standardization process instead of bogging it down.

While this doesn't include everything that the community will want eventually, it at least it locks in a "v1" API that devs can build against safely.

The argument for later extension seems to be omnipresent - are we certain this is actually feasible, and the interface published on erc721.org does not lock in things that could prevent some of the features currently being discussed? /cc @fulldecent

hyperfekt commented Feb 19, 2018

Is my understanding correct that the intended process by the 'working group' is to now discuss their interface from the very beginning instead of continuing the work / understanding already done / come to here? This would include everything agreed upon at the meeting, since there is no discussion available for these decisions.
To me that seems highly counterproductive. If you could instead produce a document that clearly outlines all your considerations so others can follow them and benefit from the work done, that should expedite the standardization process instead of bogging it down.

While this doesn't include everything that the community will want eventually, it at least it locks in a "v1" API that devs can build against safely.

The argument for later extension seems to be omnipresent - are we certain this is actually feasible, and the interface published on erc721.org does not lock in things that could prevent some of the features currently being discussed? /cc @fulldecent

@fulldecent

This comment has been minimized.

Show comment
Hide comment
@fulldecent

fulldecent Feb 19, 2018

Contributor

Hello everyone. As promised, I have made significant changes at pull request #841. Cross post from #841 (comment)


Major change implemented:

  • Rename functions to harmonize better with ERC-20
  • Add delegate function which reduces the entire Operator extension into a single line
  • Switch to one transfer function (still works the same way as we had before here)
  • Greatly expand our rationale for the transfer function and add great documentation there
  • Move the total count out of the required to the metadata extension

This addresses complaints from ETHDenver (0xproject is still preparing to release the notes. Please hurry up!):

  • Crypto Kitties used a for loop and it doesn't scale -- our standard here now explicitly scale and it is a design goal, we scale better than XKCD 865, we now reference XKCD 865
  • They did not like how some functions are optional -- we now more clearly state that all functions in the main interface are required, but we better explain how you can implement the required function and still throw, we provide examples where this is used in production contracts
  • They complained that too many functions were required -- that was a complaint to #821, our #841 interface was simpler from the beginning

In all, please help to review specifically the transfer documentation and the new transfer rationale. I am really excited by this. It is just such a powerful concept and so simple. (This is inspired by "approveAll" from the ETHDenver meeting.)

Contributor

fulldecent commented Feb 19, 2018

Hello everyone. As promised, I have made significant changes at pull request #841. Cross post from #841 (comment)


Major change implemented:

  • Rename functions to harmonize better with ERC-20
  • Add delegate function which reduces the entire Operator extension into a single line
  • Switch to one transfer function (still works the same way as we had before here)
  • Greatly expand our rationale for the transfer function and add great documentation there
  • Move the total count out of the required to the metadata extension

This addresses complaints from ETHDenver (0xproject is still preparing to release the notes. Please hurry up!):

  • Crypto Kitties used a for loop and it doesn't scale -- our standard here now explicitly scale and it is a design goal, we scale better than XKCD 865, we now reference XKCD 865
  • They did not like how some functions are optional -- we now more clearly state that all functions in the main interface are required, but we better explain how you can implement the required function and still throw, we provide examples where this is used in production contracts
  • They complained that too many functions were required -- that was a complaint to #821, our #841 interface was simpler from the beginning

In all, please help to review specifically the transfer documentation and the new transfer rationale. I am really excited by this. It is just such a powerful concept and so simple. (This is inspired by "approveAll" from the ETHDenver meeting.)

@fulldecent

This comment has been minimized.

Show comment
Hide comment
@fulldecent

fulldecent Feb 19, 2018

Contributor

@dekz Correction. ETHDenver was the second diverse working group for NFT. I have documented the history of consensus-building meetings at #841.

Contributor

fulldecent commented Feb 19, 2018

@dekz Correction. ETHDenver was the second diverse working group for NFT. I have documented the history of consensus-building meetings at #841.

@fulldecent

This comment has been minimized.

Show comment
Hide comment
@fulldecent

fulldecent Feb 19, 2018

Contributor

I just want to say I appreciate the effort of setting up that last minute meeting and getting it through. I hope you enjoy my latest update.

Let's review, let's talk. The major departure is acceptAll. I have made that slightly different in my standard and took into account many people's interests -- specifically doing two-step transfer for single items.

Now is a critical time because I think we can come together on this.

FYI, I'm in New York time zone. So you can see I'm dedicated to getting this done, and getting it done right.

Contributor

fulldecent commented Feb 19, 2018

I just want to say I appreciate the effort of setting up that last minute meeting and getting it through. I hope you enjoy my latest update.

Let's review, let's talk. The major departure is acceptAll. I have made that slightly different in my standard and took into account many people's interests -- specifically doing two-step transfer for single items.

Now is a critical time because I think we can come together on this.

FYI, I'm in New York time zone. So you can see I'm dedicated to getting this done, and getting it done right.

@fulldecent

This comment has been minimized.

Show comment
Hide comment
@fulldecent

fulldecent Feb 19, 2018

Contributor

@hyperfekt I do not support "v1". a) It is not a standard, you can't implement it b) it is incomplete.

We have taken "v1" and with considerable effort, discussion and consensus building (from people that have identified skin in the game) turned it into a standard at #841. This was not too bad because "v1" is very similar to what we already had.

Also, we have documented CONSIDERABLE amounts of rationale for all decisions.

Many people are on the record saying that #841 is feasible. Additionally we have a baseline standard (required) and two optional add-ons (metadata and enumeration). We have also enumerated and thoroughly discussed many use cases. Even if we do not support them (functions for paying real estate taxes?) we have considered how those functions would be compatible with the standard.

In summary: the standard has room to support many imagined use cases and we have documented it.

Contributor

fulldecent commented Feb 19, 2018

@hyperfekt I do not support "v1". a) It is not a standard, you can't implement it b) it is incomplete.

We have taken "v1" and with considerable effort, discussion and consensus building (from people that have identified skin in the game) turned it into a standard at #841. This was not too bad because "v1" is very similar to what we already had.

Also, we have documented CONSIDERABLE amounts of rationale for all decisions.

Many people are on the record saying that #841 is feasible. Additionally we have a baseline standard (required) and two optional add-ons (metadata and enumeration). We have also enumerated and thoroughly discussed many use cases. Even if we do not support them (functions for paying real estate taxes?) we have considered how those functions would be compatible with the standard.

In summary: the standard has room to support many imagined use cases and we have documented it.

@alexanderatallah

This comment has been minimized.

Show comment
Hide comment
@alexanderatallah

alexanderatallah Feb 20, 2018

We at OpenSea (https://opensea.io, an NFT marketplace) endorse this v1 API. At the EthDenver meet, the emphasis wasn't on finalizing the standard, but instead on establishing a name and location to send to devs who want to work with non-fungible tokens. This is important because many, many tokens haven't been following any standard at all, in part due to all these balkanized github EIPs. @tomhschmidt 's site is a step in the right direction

alexanderatallah commented Feb 20, 2018

We at OpenSea (https://opensea.io, an NFT marketplace) endorse this v1 API. At the EthDenver meet, the emphasis wasn't on finalizing the standard, but instead on establishing a name and location to send to devs who want to work with non-fungible tokens. This is important because many, many tokens haven't been following any standard at all, in part due to all these balkanized github EIPs. @tomhschmidt 's site is a step in the right direction

@hyperfekt

This comment has been minimized.

Show comment
Hide comment
@hyperfekt

hyperfekt Feb 20, 2018

@fulldecent
Sorry for the confusion, you might have misunderstood me. My comments were addressed at v1.
I have been following #841; and was doubting the arguments for using v1 at all in the presence of #841, specifically asking if v1 prevents functionality #841 allows for; and asking for rationale for decisions made in v1 so they may be considered for #841 instead.

hyperfekt commented Feb 20, 2018

@fulldecent
Sorry for the confusion, you might have misunderstood me. My comments were addressed at v1.
I have been following #841; and was doubting the arguments for using v1 at all in the presence of #841, specifically asking if v1 prevents functionality #841 allows for; and asking for rationale for decisions made in v1 so they may be considered for #841 instead.

@eordano

This comment has been minimized.

Show comment
Hide comment
@eordano

eordano Feb 20, 2018

I do not agree with https://erc21.org and I'm asking to get Decentraland's logo out of there (it's irresponsible to champion a standard without transfer security), but I've got the upmost respect for the 0x team.

eordano commented Feb 20, 2018

I do not agree with https://erc21.org and I'm asking to get Decentraland's logo out of there (it's irresponsible to champion a standard without transfer security), but I've got the upmost respect for the 0x team.

@shrugs

This comment has been minimized.

Show comment
Hide comment
@shrugs

shrugs Feb 20, 2018

imo transfer security can/should be part of a secondary EIP that doesn't enforce a specific use-case onto a general standard

shrugs commented Feb 20, 2018

imo transfer security can/should be part of a secondary EIP that doesn't enforce a specific use-case onto a general standard

@eordano

This comment has been minimized.

Show comment
Hide comment
@eordano

eordano Feb 21, 2018

@shrugs doesn't that defeat the purpose of security? I'm baffled about the idea of allowing a protocol to exist that can lead to loss of funds "because reasons".

eordano commented Feb 21, 2018

@shrugs doesn't that defeat the purpose of security? I'm baffled about the idea of allowing a protocol to exist that can lead to loss of funds "because reasons".

@okwme

This comment has been minimized.

Show comment
Hide comment
@okwme

okwme Feb 21, 2018

@eordano shouldnt the eip allow interoperability without enforceing specific use cases? If your token that you deploy needs an extra layer of security you should force unsafe/unwanted transactions to fail. As discussed at ethdenver this will still adhere to the api.

okwme commented Feb 21, 2018

@eordano shouldnt the eip allow interoperability without enforceing specific use cases? If your token that you deploy needs an extra layer of security you should force unsafe/unwanted transactions to fail. As discussed at ethdenver this will still adhere to the api.

@shrugs

This comment has been minimized.

Show comment
Hide comment
@shrugs

shrugs Feb 21, 2018

There exist perfectly valid reasons to allow ERC20s to be sent to arbitrary addresses. For example, perhaps they represent debt. Or are tokens representing a "claim" against an address to signify KYC or something. This is even more relevant for NFTs.

Sure, it's not the primary usecase, but that's not the point of a general standard. Transfer security can and should be part of a secondary standard that defines a method to enforce that receivers respect an interface that supports your transfer.

In the case of ERC20 transfer security should be part of something like 223 (except ideally it would not be just talked about for a year without progress). 223 will never be finalized at this rate, so perhaps it's worth providing a simple, scoped EIP that adds optional transfer security to EIP20.

shrugs commented Feb 21, 2018

There exist perfectly valid reasons to allow ERC20s to be sent to arbitrary addresses. For example, perhaps they represent debt. Or are tokens representing a "claim" against an address to signify KYC or something. This is even more relevant for NFTs.

Sure, it's not the primary usecase, but that's not the point of a general standard. Transfer security can and should be part of a secondary standard that defines a method to enforce that receivers respect an interface that supports your transfer.

In the case of ERC20 transfer security should be part of something like 223 (except ideally it would not be just talked about for a year without progress). 223 will never be finalized at this rate, so perhaps it's worth providing a simple, scoped EIP that adds optional transfer security to EIP20.

@eordano

This comment has been minimized.

Show comment
Hide comment
@eordano

eordano Feb 21, 2018

I see your point but I dismiss it, as the use cases seem to me to be out of scope for the meaning of transfer and can be implemented, like you suggested, as an additional standard on top of a secure one (it's easy to break the chain, not easy to build it).

But I've only come this far and I'll agree to disagree.

Like I said in private, I'm concerned about negligently championing a standard that leads to the loss of millions in value. It's not good craftsmanship, it's not ethical, and it might come with legal liability, given there's precedent of 5 million usd lost and you're in the position to improve it, but decidedly choose to go the other way.

eordano commented Feb 21, 2018

I see your point but I dismiss it, as the use cases seem to me to be out of scope for the meaning of transfer and can be implemented, like you suggested, as an additional standard on top of a secure one (it's easy to break the chain, not easy to build it).

But I've only come this far and I'll agree to disagree.

Like I said in private, I'm concerned about negligently championing a standard that leads to the loss of millions in value. It's not good craftsmanship, it's not ethical, and it might come with legal liability, given there's precedent of 5 million usd lost and you're in the position to improve it, but decidedly choose to go the other way.

@eordano

This comment has been minimized.

Show comment
Hide comment
@eordano

eordano Feb 21, 2018

Updated the main text

  • Added totalSupply
  • Dropped name, description, symbol

eordano commented Feb 21, 2018

Updated the main text

  • Added totalSupply
  • Dropped name, description, symbol
@shrugs

This comment has been minimized.

Show comment
Hide comment
@shrugs

shrugs Feb 22, 2018

aside: I don't expect any legal argument around this to stand up in court. Ethereum is a tool, just like a hammer, and that precedent has been set. If you're actually worried about legal implications of not including transfer security, that has everything to do with your specific implementation of the NFT standard rather than the standard itself. Unnecessarily restricting user behavior because of your perceived benefits is a very slippery slope, philosophically.


Adding an unsafeTransfer as a future EIP defeats the purpose of the layered approach, as I'm sure you've realized. The point is to build more specific functionality over general implementations, not create n specific implementations of transfer().


The proposed solution is to create a general NFT standard and then augment it with additional standards for transfer security, specific role-based delegate standards, pull-transfers, metadata, and more. If the community works together on this, we champion for them all at the same time and result in the situation where:

  • 0x is happy because they can support the allowAll UX without worrying about fragmentation
  • Decentraland is happy because they can implement transfer security and pull-transfers
  • CryptoKitties is happy because they can add transfer security
  • Users are happy because gas isn't wasted on code they don't want to run
  • wallets are happy because they can use introspection to figure out what standard a DAR supports
  • everyone is happy because we've standardized metadata for displaying NFTs

(edit) I also want to point out that, in 721, you are 👍 regarding:

So I would strongly make the argument that any standard that is likely to be successful at this point should restrict itself to enabling possibilities rather than trying to dictate implementations.

Which is very much counter to enforcing transfer security on the base interaction standard. The point is that we can't know right away if unsafe transfers are a feature or a bug. I think I've indicated that it certainly has the ability to be a feature, perhaps in ways we haven't thought of yet. And if it does end up being a specific feature, we'd need to create a new standard on top of this restrictive one, fragmenting the implementation space yet again.

shrugs commented Feb 22, 2018

aside: I don't expect any legal argument around this to stand up in court. Ethereum is a tool, just like a hammer, and that precedent has been set. If you're actually worried about legal implications of not including transfer security, that has everything to do with your specific implementation of the NFT standard rather than the standard itself. Unnecessarily restricting user behavior because of your perceived benefits is a very slippery slope, philosophically.


Adding an unsafeTransfer as a future EIP defeats the purpose of the layered approach, as I'm sure you've realized. The point is to build more specific functionality over general implementations, not create n specific implementations of transfer().


The proposed solution is to create a general NFT standard and then augment it with additional standards for transfer security, specific role-based delegate standards, pull-transfers, metadata, and more. If the community works together on this, we champion for them all at the same time and result in the situation where:

  • 0x is happy because they can support the allowAll UX without worrying about fragmentation
  • Decentraland is happy because they can implement transfer security and pull-transfers
  • CryptoKitties is happy because they can add transfer security
  • Users are happy because gas isn't wasted on code they don't want to run
  • wallets are happy because they can use introspection to figure out what standard a DAR supports
  • everyone is happy because we've standardized metadata for displaying NFTs

(edit) I also want to point out that, in 721, you are 👍 regarding:

So I would strongly make the argument that any standard that is likely to be successful at this point should restrict itself to enabling possibilities rather than trying to dictate implementations.

Which is very much counter to enforcing transfer security on the base interaction standard. The point is that we can't know right away if unsafe transfers are a feature or a bug. I think I've indicated that it certainly has the ability to be a feature, perhaps in ways we haven't thought of yet. And if it does end up being a specific feature, we'd need to create a new standard on top of this restrictive one, fragmenting the implementation space yet again.

@AusIV

This comment has been minimized.

Show comment
Hide comment
@AusIV

AusIV Feb 25, 2018

Another thing that crossed my mind with regard to transfer security:

Account abstraction is coming with the constantinople hard fork, after which I expect we can see many people starting to use contracts as their primary wallets. Wallet contracts are generally structured such that they can proxy arbitrary transactions once those transactions have been validated. In general, they can do anything a key based account can do, but with their own rules around what conditions must be met first.

My reading of this spec suggests that a multisig wallet which didn't implement onAssetReceived wouldn't be able to receive deeds, even though it was built to be able to handle arbitrary transactions. That may not be that big of a deal now, since account abstraction isn't here yet and most people will create such contracts after this proposal is finalized, but once lots of people are already using contracts as their daily wallets, adding functions that they have to implement in order to interact with broad classes of other contracts seems like a non-starter, because it would force people to migrate to new wallet contracts to support interaction with new standards.

AusIV commented Feb 25, 2018

Another thing that crossed my mind with regard to transfer security:

Account abstraction is coming with the constantinople hard fork, after which I expect we can see many people starting to use contracts as their primary wallets. Wallet contracts are generally structured such that they can proxy arbitrary transactions once those transactions have been validated. In general, they can do anything a key based account can do, but with their own rules around what conditions must be met first.

My reading of this spec suggests that a multisig wallet which didn't implement onAssetReceived wouldn't be able to receive deeds, even though it was built to be able to handle arbitrary transactions. That may not be that big of a deal now, since account abstraction isn't here yet and most people will create such contracts after this proposal is finalized, but once lots of people are already using contracts as their daily wallets, adding functions that they have to implement in order to interact with broad classes of other contracts seems like a non-starter, because it would force people to migrate to new wallet contracts to support interaction with new standards.

@eordano

This comment has been minimized.

Show comment
Hide comment
@eordano

eordano Mar 4, 2018

Closing this as #721 now implements the changes we wanted from it here #841

eordano commented Mar 4, 2018

Closing this as #721 now implements the changes we wanted from it here #841

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment