Skip to content
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

NFT Module #4046

Closed
4 tasks
okwme opened this issue Apr 4, 2019 · 17 comments · Fixed by #4209
Closed
4 tasks

NFT Module #4046

okwme opened this issue Apr 4, 2019 · 17 comments · Fixed by #4209
Assignees
Milestone

Comments

@okwme
Copy link
Member

@okwme okwme commented Apr 4, 2019

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? the emerging ERC-1155 standard? the EOS dgoods standard?

For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@alexanderbez
Copy link
Contributor

@alexanderbez alexanderbez commented Apr 4, 2019

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
Copy link
Member

@jackzampolin jackzampolin commented Apr 4, 2019

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

@okwme
Copy link
Member Author

@okwme okwme commented Apr 8, 2019

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:

  • transferFrom(address _from, address _to, uint256 _tokenId, bytes data)
    • This is a transfer function that includes the ability to transfer an asset on behalf of another user. We'd need to keep track of allowances so that a user would be able to give permission to another party to move assets on their behalf. This might be necessary for pegs as well–maybe the permission includes chain-ids as well?
  • approve(address _approved, uint256 _tokenId)
    • This would be a necessary component of transferFrom to approve transfers on behalf of other parties.
  • setApprovalForAll(address _operator, bool _approved)
    • Variation of approve to allow multiple approvals at once
  • safeTransferFrom(address _from, address _to, uint256 _tokenId)
    • This version of TransferFrom checks with the party receiving the NFT to confirm they are prepared to accept the asset or whether they have an action that should be performed upon receiving it. Maybe out of scope for first Cosmo NFT spec but also good to consider this. Maybe we'd have a registry of recipients who can decide whether they accept an NFT sent to them or reject it?
  • safeTransferFrom(address _from, address _to, uint256 _tokenId, bytes data)
    • This is a variation of safeTransferFrom without a data parameter. This data parameter is used to daisy-chain arbitrary on chain transactions. Something similar might be another Msg type that might want to be executed directly following the first transaction. QUESTION: Is it possible to create arbitrary transaction that bundle multiple MsgTypes? Maybe for 1st draft this implementation should not be reproduced on Cosmos.* safeTransferFrom(address _from, address _to, uint256 _tokenId, bytes data)

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

  • balanceOf(address _owner)
    • Number of NFTs the address owns
  • ownerOf(uint256 _tokenId)
    • Owner address of a specific NFT
  • getApproved(uint256 _tokenId)
    • If transferFrom includes the ability to move on behalf of others it would be necessary to check the approvals
  • isApprovedForAll(address _owner, address _operator)
    • An extension to the ability to check for approvals

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:

  • name()
    • Return a string for the NFT family name
  • symbol()
    • Return a string for the NFT family symbol
  • tokenURI
    • This should be replaced with an actual metadata data object. Even if all data is stored off chain the tokenURI for that data could be contained within the returned data object.
  • metadata
    • This would return an object with a minimum of the ERC-721 metadata scheme, plus optional additional info under tokenURI. The return format may be a parameter so that if the recipient is prepared to parse JSON they can do so, but may also be able to request it with a different serialization.
{
    "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."
        },
        "tokenURI": {
            "type": "string",
            "description": "A URI pointing to a resource with more token related metadata that doesn't belong on chain."
        }
    }
}

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
Copy link
Member Author

@okwme okwme commented Apr 18, 2019

Some thoughts as we build the draft implementation here: #4118

  • Should the NFT module support multiple denominations of NFTs like the Cosmos Bank?
    • If so, the denom needs to be included in all queries
    • It could also be that the denom is only required if there is more than one denom registered there
  • 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
  • Should the standard come with optional marketplace functionality like buying NFTs?
    • We thought this was useful but should be separated from the NFT module

@okwme
Copy link
Member Author

@okwme okwme commented Apr 18, 2019

Some really great discussion related to this topic over at #4153

@shrugs
Copy link

@shrugs shrugs commented Apr 18, 2019

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
Copy link

@shrugs shrugs commented Apr 18, 2019

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
Copy link

@vshvsh vshvsh commented Apr 19, 2019

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
Copy link
Member

@jackzampolin jackzampolin commented Apr 23, 2019

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

@okwme
Copy link
Member Author

@okwme okwme commented Apr 25, 2019

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

  • 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.

    • The current Rest endpoints available for the SDK's bank and account modules don't specify which coin denomination is being accessed. All coins are returned at once for an account(link). I agree with you that we should first offer what is already available with the cousin fungible endpoint: this would be an all encompassing endpoint for NFTs. However I think the ability to specify which NFT denom should also be included.
  • 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

    • I think tokenURI is more descriptive as the URI part tells what kind of data will be in this key. Your query example is silky but will still be possible with any name. Maybe extensionURI should be considered tho?
  • 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"

    • It would be nice but I think that's already too much assumption. I may want to use an NFT system for something which only ever needs an ID. I wouldn't want to force unnecessary storage on anyone. You also open yourself up to maintaining the integrity of names like preventing collision, limiting length or special characters that i'd rather avoid as well.
  • 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.

    • Totally agree but think @jackzampolin is right here that the chain-id should function the exact same way. I'd love to see a global registry of chain-id and NFTs but I think that's out of scope for the standard. We should also be seeing systems come into place for keeping track of origin chains across IBC communication. Again I think this should be part of another spec related to Inter-Blockchain Communication.

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
Copy link

@vshvsh vshvsh commented Apr 25, 2019

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
Copy link
Contributor

@rigelrozanski rigelrozanski commented Apr 26, 2019

@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
Copy link

@fulldecent fulldecent commented Jun 10, 2019

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.)

  • ERC-721 and dGoods are non-fungible tokens. You may study which best meets the goals of this project.
  • ERC-1155 is very useful (and should be final soon) but it is not most clearly a non-fungible token. A use case study that chooses ERC-1155 for non-fungibles should be extra careful and have a stronger threshold to meet because of its additional complexity.
  • ERC-1155 is actually beautiful for fungible applications and (after it is final) I would recommend this approach as a best choice for fungible applications.

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

@coinfork
Copy link

@coinfork coinfork commented Jun 12, 2019

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
Copy link
Member Author

@okwme okwme commented Jun 17, 2019

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
Copy link
Member Author

@okwme okwme commented Jun 28, 2019

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
Copy link

@mikedeshazer mikedeshazer commented Jul 19, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.