stacksgov / sips

Community-submitted Stacks Improvement Proposals (SIPs)
132 stars 80 forks source link

feat: add NFT standard #3

Closed friedger closed 3 years ago

friedger commented 3 years ago

This PR adds a SIP for the Non-fungible standard.

It fixes #2

hstove commented 3 years ago

@friedger can you rename the file to sips/sip-009-nft-standard.md? This is in line with the existing SIPs, which I just ported in #4.

friedger commented 3 years ago

There is already a discussion about error types.

For transfer?, we could define an error kind for the native transfer:

{kind: "nft-transfer-failed", code: from-nft-transfer}

friedger commented 3 years ago

@hstove We don't really need get-balance, do we? However, we need get-last-id to ensure that it is iteratable

hstove commented 3 years ago

@friedger agreed that we don't need get-balance, and get-last-id makes sense too. I think the error handling part is the only thing that might need some tweaking. I'm in favor of structured errors, but regardless of what we decide as the signature, can we come up with the standard error types for transfer?

Another thing that we should do, and make as a best practice for this kind of SIP, is to require an example implementation. A simple repo with the trait and an implementation would be perfect. Bonus points for (simple) JS code, especially to demonstrate error handling.

All opinions my own, of course - would love to get more eyes on this.

Zk2u commented 3 years ago

@hstove example implementations are a great idea. my thinking would be to create a monorepo under the org that holds all implemented SIP examples, though through drafts they would be held by the implementer.

hstove commented 3 years ago

@xelamade Friedger added a reference implementation in this PR: https://github.com/friedger/clarity-smart-contracts/blob/master/contracts/sips/nft-trait.clar

hstove commented 3 years ago

Hey @friedger - have been doing some reading around ERC721 implementations. The standard includes a function tokenURI - defined in Solidity as function tokenURI(uint256 _tokenId) external view returns (string);.

This returns a URL which includes JSON metadata that clients can use to embed information about the NFT. I believe we should follow a similar practice.

What do you think about adding this function:

(metadata-url (uint) (string-ascii 256))

Additionally, it would likely be best to build off of the existing JSON schema used in the ERC721 standard:

{
    "title": "Asset Metadata",
    "type": "object",
    "properties": {
        "name": {
            "type": "string",
            "description": "Identifies the asset to which this NFT represents"
        },
        "description": {
            "type": "string",
            "description": "Describes the asset to which this NFT represents"
        },
        "image": {
            "type": "string",
            "description": "A URI pointing to a resource with mime type image/* representing the asset to which this NFT represents. Consider making any images at a width between 320 and 1080 pixels and aspect ratio between 1.91:1 and 4:5 inclusive."
        }
    }
}
friedger commented 3 years ago

@hstove I like get-metadata-uri or shorter get-meta-uri, similar to get-owner. Optionally, we can support get-meta that returns the meta data stored on-chain.

Is the length long enough? Why 256?

Should we limit the asset meta data to images? I suggest using "uri" and "mime-type".

We discussed immutability, therefore, I suggest to include a hash in the contract (get-asset-hash (uint) (response (buff 64) uint))

(This is all about the meta data and should go in a separate section of the standard and it should be optional.)

hstove commented 3 years ago

Well, if this is a URI, do you think 256 is not long enough? It's not for storing on-chain data - this method must return a resolvable URI (although it could be on http, ipfs, etc). Supporting get-meta is just more tricky, because then we have to specify a very strict format, that can't be easily extended.

For example, the JSON schema I linked is for image-based NFTs, but in ETH there are now more semi-formal standards for other NFTs - like 3d objects, videos, etc.

I also like simply using an off-chain pointer because there is no need to access this data on-chain.

We discussed immutability, therefore, I suggest to include a hash in the contract (get-asset-hash (uint) (response (buff 64) uint))

This is an interesting idea - this would be a hash of the JSON metadata? I like it - it's light enough and clients can choose how they respond to an invalid hash. However, if you wanted immutability, you could just use an IPFS-resolving URI, which are immutable.

(This is all about the meta data and should go in a separate section of the standard and it should be optional.)

Agreed that all the JSON schema stuff should be left out of this strict standard. I'm mainly advocating for the URI method, as that'll be quite important to support a decentralized ecosystem of NFT apps.

Zk2u commented 3 years ago

could we consider adding an optional way to store and image or video representing the NFT on-chain rather than a URI? is this possible?

friedger commented 3 years ago

could we consider adding an optional way to store and image or video representing the NFT on-chain rather than a URI? is this possible?

You could use a data uri or a uri that reference onchain data where ever that is stored, maybe in an attachment?

hstove commented 3 years ago

FYI I've added get-token-uri to the FT standard, which returns (optional (string-utf8 256)). I like get-token-uri as it's in line with the ERC standards (for both 20 and 721).

I'm not 100% tied to that name or response, but it would be great if both traits had the same spec for that function.

muneebm commented 3 years ago

I think the standard for meta data should also be part of this SIP. Maybe we can define a separate trait for that, something like this:

(define-trait sip-009-metadata
  (
     (get-token-uri (uint) (response (optional (string-ascii 2048)) uint))
  )
)

EIP1155 lists some reasons for not including name() and symbol() in the ERC1155 standard, not sure if it's applicable for us: https://github.com/ethereum/EIPs/issues/1155

Metadata Choices

The symbol function (found in the ERC-20 and ERC-721 standards) was not included as we do not believe this is a globally useful piece of data to identify a generic virtual item / asset and are also prone to collisions. Short-hand symbols are used in tickers and currency trading, but they aren't as useful outside of that space.

The name function (for human-readable asset names, on-chain) was removed from the standard to allow the Metadata JSON to be the definitive asset name and reduce duplication of data. This also allows localization for names, which would otherwise be prohibitively expensive if each language string was stored on-chain, not to mention bloating the standard interface. While this decision may add a small burden on implementers to host a JSON file containing metadata, we believe any serious implementation of ERC-1155 will already utilize JSON Metadata.

jcnelson commented 3 years ago

I'll let @friedger decide what's appropriate here, but I'm generally in favor of having a set of "small" traits that are individually well-understood and incrementally deployed, versus a single "large" trait that may contain a lot of unnecessary boilerplate and would take a lot of deliberation to design and deploy. You're probably right that we'll need something like ERC-1155 at some point, but it's not clear to me that it has to be in this SIP (but again, it's not my choice to make).

Zk2u commented 3 years ago

uploaded as .txt as GitHub wouldn't allow an md file

sip-009-nft-standard.txt

hstove commented 3 years ago

@pxydn can you make those changes in git? I have no way of knowing what you've changed by simply looking at your .txt file. Here's the best way to do that:

Then, we can easily reason about the changes you've made, and Friedger can easily accept your PR, which will in turn make those changes visible here.

hstove commented 3 years ago

Addressing other comments:

friedger commented 3 years ago

I merged in @pxydn comments. Thank you!

muneebm commented 3 years ago

@friedger opened a PR in your repo. Please have a look.

friedger commented 3 years ago

@muneebm I try to incorporate your comments. However, it removes the addition implementation conditions. Is this on purpose? I think the specification must also provide requirements for the contract implementation. These could be tested by application developers.

friedger commented 3 years ago

The trait has been deployed to testnet and mainnet!

jcnelson commented 3 years ago

Awesome! This SIP is now in activation-in-progress status.

hstove commented 3 years ago

Given our earlier discussion and https://github.com/stacksgov/Stacks-Grants/issues/73, are we on the same page about only a uint error code for transfer?

friedger commented 3 years ago

@hstove Yes, sip has been updated, new version deployed to mainnet, new beeple nft published.

314159265359879 commented 3 years ago

@friedger and @markmhx what do you think about making one more change to this trait? The current name is nft-trait(nft-trait) I think if there are more NFT standards in the future you will want to distinguish them from each other. As already suggested by @agraebe for fungible tokens: https://github.com/stacksgov/sips/pull/25#pullrequestreview-658508574 naming it: sip-010-trait. (the current trait deployment for FT also has sip10, in the name).

Perhaps follow a similar line for this NFT trait: (re)deploying it with the sip-009-trait name?

friedger commented 3 years ago

@314159265359879 The name is not part of the sip, IMHO. You can deploy them using any name and as often as you want. I think in the future, we will have some better referencing to sip trait contracts, e.g. via BNS names.

friedger commented 3 years ago

Should this be merged as the sip is activated?

jcnelson commented 3 years ago

And it's official! Congratulations @friedger! SP2PABAF9FTAJYNFZH93XENAJ8FVY99RRM50D2JG9.nft-trait.nft-trait is now officially a standard :tada:

jcnelson commented 3 years ago

Thanks to everyone who helped review and get this ready!