cosmos / cosmos-sdk

:chains: A Framework for Building High Value Public Blockchains :sparkles:
https://cosmos.network/
Apache License 2.0
6.07k stars 3.49k forks source link

NFT Module #4046

Closed okwme closed 4 years ago

okwme commented 5 years ago
## Summary

Either an extension to the bank module to support non-fungible tokens or a new module specifically for non-fungible tokens.

Problem Definition

There is currently no simple solution for supporting non-fungible tokens which make up a large and growing category of decentralized applications.

Proposal

Request for comment about the best way to implement this feature and what exactly this feature should entail. Questions include: * Should this process actually take place in the ICS repo? * Should this be part of bank module or it's own module? * Should metadata be included as part of this module? * Should this spec follow or extend the [ERC-721 standard](https://github.com/ethereum/EIPs/blob/master/EIPS/eip-721.md)? the emerging [ERC-1155](https://github.com/ethereum/EIPs/issues/1155) standard? the [EOS dgoods standard](https://github.com/MythicalGames/dgoods/blob/master/dgoods_spec.md)? ____ #### For Admin Use - [ ] Not duplicate issue - [ ] Appropriate labels applied - [ ] Appropriate contributors tagged - [ ] Contributor assigned/self-assigned
alexanderbez commented 5 years ago

Thanks for creating this issue @okwme and brining light to NFTs in the SDK. This is something I would love to see come from a community contribution but is also something I think we can plan into a future milestone.

Some thoughts, I really do enjoy the ERC-721 open standard. I think it's very well documented and has numerous implementations already (e.g. 0x, CryptoKitties, Trust Wallet, etc...). I haven't actually read ERC-1155 in it's entirety yet, but it seems that there is continuing discourse on its adoption (correct me if I'm wrong). Essentially, I think we can adopt and incorporate a lot of useful concepts from ERC-721.

My initial thoughts are that NFTs should not be incorporated into the existing bank module, but instead should be their own module(s). NFTs can be separate by domains in which case each domain would be it's own NFT module (e.g. x/kitties_nft) or we could have one domain-agnostic NFT module too. Either way, I absolutely think the SDK should contain a reference implementation + spec.

What are your thoughts @okwme?

jackzampolin commented 5 years ago

I really really like the idea of using existing standards here. Lets not reinvent the wheel if we don't have to.

okwme commented 5 years ago

I agree on both accounts that it should follow the work done on 721 that's already been tested as useful. Most of 1155 is about increasing the gas efficiency of 721 and opinions about combining registries of erc-20 and erc-721. These features don't really apply to an SDK NFT.

I think the biggest change between an Ethereum based NFT and an SDK NFT is the ability to store much more Metadata on chain. Because storage on ETH is shared across all participants it's expensive and so the solution was to put metadata off chain and just include a tokenURI endpoint that returns an RFC 3986 URI—typically http(s) or ipfs. That URI returns a JSON object with information about that NFT. This makes it difficult for that information to be accessed and used for on-chain transactions. This could be a problem if for example the NFT were an asset like a Centrifuge-style invoice where the contents of the invoice are only represented by a hash but another on chain transaction needs access to actual data in that invoice—maybe hourly rate. You'd need an oracle to confirm that content if it lived off-chain but with an SDK chain it can be readily accessible.

I think this feature should be supported by modifying the 721 spec where the tokenURI doesn't return the URI but the actual metadata resource itself. This resource may elect to contain a tokenURI that points to off-chain data, in case storage of such data does in fact make more sense off-chain. However it also enables onchain data to be accessible in a predictable format in case it is deemed useful on chain.

The following methods from the 721 standard would need to become Msg Types:

The following queriers would need to be created from the 721 spec:

The Metadata spec is actually separated from the required fields of the NFT. This might also be good to make optional with a reference to whether or not they exist. Something like EIP 165 would be needed for that and should also be considered. In the mean time we could expect the following queriers for metadata:

It might also be useful to have more Cosmos specific information per NFT. Maybe a parameter for origin chain? If the specific token is being held across chains, maybe it would be good to distinguish which NFTs are being held by users and which are being held by validator sets. If the asset is being held by a validator set it would be good to be able to tell whether a user holds the wrapped version of the NFT on the other side of the validator set.

Curious as to other attributes that may be useful for NFTs in the context of Cosmos, anyone else have some ideas?

okwme commented 5 years ago

Some thoughts as we build the draft implementation here: https://github.com/cosmos/cosmos-sdk/pull/4118

okwme commented 5 years ago

Some really great discussion related to this topic over at https://github.com/cosmos/cosmos-sdk/issues/4153

shrugs commented 5 years ago

Unless there's an existing pattern around having name and symbol as top-level queries (forgive me, I know nothing about cosmos), it could make sense to put those as part of the metadata query and associated spec. ERC-20's existence mandated that ERC-721 did the same with name/symbol.

shrugs commented 5 years ago

If the purpose of tokenURI within the metadata query is to provide an extension to the on-chain metadata, perhaps it's best to just call it extension, so that implementers can do something like

const resolvedMetadata = {
   ...metadata,
   ...(await resolveExtendedMetadata(metadata.extension))
}

and call it a day.

I'd also make the metadata spec enforce the presence of name for no other reason than "it'd be nice if everything could be guaranteed to have a name"

vshvsh commented 5 years ago

There's one more property of ERC-721 that is widely used in practice - namely, the corresponding smart contract address. It is basically used as an authentication: this NFT was issued by CryptoKitties smart contract ergo it's a cryptokittie and can be accepted/traded on a decentralized marketplace. Cosmos-based NFT should have some easily accessed authenticated data field too, like issuer or maybe some signature/certificate.

jackzampolin commented 5 years ago

Maybe the chain-id would be enough to mark the NFT?

okwme commented 5 years ago

Love this discussion! Some thoughts on the points from @shrugs and @vshvsh:

A question I still have with regard to organizing Go code and extending modules in the SDK:

Should the Module include Mint and Burn abilities or should they be separate Modules? What is the standard way of including / extending modules? This is actually relevant to whether there should be a single nft module as well as a nfts module for when multiple denoms are needed

Maybe @rigelrozanski has opinions on this?

vshvsh commented 5 years ago

I think chain-id won't cut it. Suppose there is an NFT marketplace that has a whitelist of issuers that are allowed to trade; whitelist is enforced in module/smart contract using onchain data only. It's a very realistic scenario.

There is a need for some field that can authenticate an issuer: in ethereum it's smart contract address. In more general sense it can be digital signature or any bytestring that can be linked to issuer through cryptography or onchain state machine.

I'd say add issuerFingerprint untyped byte string data field, filled and interpreted according to the custom blockchain rules.

rigelrozanski commented 5 years ago

@okwme

I think this feature should be supported by modifying the 721 spec where the tokenURI doesn't return the URI but the actual metadata resource itself. This resource may elect to contain a tokenURI that points to off-chain data, in case storage of such data does in fact make more sense off-chain. However it also enables onchain data to be accessible in a predictable format in case it is deemed useful on chain.

I think I agree - does this basically entail that we allow the meta-data to optionally be stored on-chain or off-chain on a platform like IPFS (whereby the protocol would retrieve the metadata from the 3rd party)?

It might also be useful to have more Cosmos specific information per NFT. Maybe a parameter for origin chain?

Absolutely

Should the NFT module support multiple denominations of NFTs like the Cosmos Bank?

I'm not sure either way. What would the purpose be really of having multiple denoms? Wouldn't each NFT potentially cary drastically different rulesets? It may be worth to simply allow for multiple NFT wallets under the same key. But I'm not sure, just curious.

Should the Module include Mint and Burn abilities or should they be separate Modules?

I'm inclined to say that each NFT should be able to define custom mint and burn logic which could occur on a blockly basis, and be able to register that logic through the NFT interface. In this way Mint and Burn rules would typically be defined at the App level then registered with NFT - during the NFT endblock or beginblock custom registered logic would be executed


RE: between approve and SetApprovalForAll from 721 these functions should probably be structured using functions like

type Approval struct {
    Address sdk.Address
    TokenID sdk.Int
}

SetApprovals(approvals ...Approval) { . . . }
RejectApprovals(approvals ...Approval) { . . . }
fulldecent commented 5 years ago

Just a few notes, and please forgive me that I am not fully a user of Cosmos (yet).

I would generally recommend that fungibles and non-fungibles have separate user interfaces. You keep assets in a bank and you keep collectables with a custodian. This is a great simplification, and ignores two important distinctions, first do you have custody of the tokens or your assignee?, and second what do these tokens represent?

In this instance, like many other, a detailed use case study is the best way to choose the appropriate end-user experience. (Which, of course, guides the decisions for SDKs and other levers of decisions.)

Regarding metadata. Yes. It is a very simple and powerful way to connect tokens to the real world (i.e. things with URIs).

coinfork commented 5 years ago

Good intro to these tokens, fulldecent.

As a purely non-fungible token, ERC-1155 might seem like it has some complexity, but there are actually a couple of ways to do NFTs with it:

We've been using ERC-1155 for non-fungibles intensively by separating the token ID bits into a base ID and an index, please see the section on this here: https://github.com/ethereum/EIPs/blob/master/EIPS/eip-1155.md#non-fungible-tokens

On the other hand, a very simple way to do non-fungibles with ERC-1155 is to simply give each non-fungible token ID a quantity/supply of 1. This means that each token ID is very naturally either a fungible or non-fungible token based on if it has a quantity > 1. This exactly mirrors the real world. Unique items are naturally one-of-a-kind, while mass-produced items may be created in runs of thousands.

okwme commented 5 years ago

I think the "semi-fungible" aspects of tokens on the SDK is easily solved with a simple NFT module and the traditional bank module. If some NFTs needs to be fungible there should be an app-specific method to do so. You could simply "burn" the NFTs and mint new fungible tokens that represent them. They can be bought and sold in quantities as fungible tokens and then redeemed for NFTs later on if need be.

okwme commented 5 years ago

Reposting from issue #1980

I'd be really into a separation of asset and metadata. I think the fungible and non-fungible token should do only what they're meant to do: represent an asset with denomination (+origin chain) and amount (ID in the case of NFTs). The metadata should be linked to the generalized asset type of fungible or non-fungible token and should be able to have as much or as little depth as needed.

I'm kind of into the schema.org way of nested categorization: https://schema.org/docs/full.html

Assets could use this format as well as some way of describing which aspects of this format they satisfy. There could also be a protocol for expanding the schema with versions that various projects could keep in sync with. For games: https://schema.org/VideoGame General assets NFT or otherwise: https://schema.org/TypeAndQuantityNode Currency: https://schema.org/currency (maybe needs to be extended)

As a standard used outside of our specific context with an organization dedicated to it it might be good as a common denominator between all sorts of assets which require metadata

https://github.com/schemaorg/schemaorg

mikedeshazer commented 4 years ago

In favor of fungible yet ERC-721-like tokens. Essentially like rewards points with equal value, but different meta data like expiration, transferability date etc.