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 721 spec #50

Merged
merged 10 commits into from
Aug 24, 2020
Merged

Nft 721 spec #50

merged 10 commits into from
Aug 24, 2020

Conversation

ethanfrey
Copy link
Member

@ethanfrey ethanfrey commented Aug 20, 2020

Closes #43

Define a 721 spec for NFTs, focusing on transfering and ownership.

  • Base 721 Spec
  • Metadata and Enumeration extensions
  • Reflect on TokenUri? (all metadata off-chain)
  • How to make MetaData extensible? (not possible with rust unless I use macros, did research)
  • Implement Receiver interface
  • Implement helpers (wrappers)
  • Unify Expiration type with cw20 (and cw1) -> cw0 common repo? (In Add Expration to cw0 #51)

@ethanfrey ethanfrey marked this pull request as ready for review August 21, 2020 20:11
@ethanfrey
Copy link
Member Author

@sunnya97 @okwme

If you have time, I would love your review on this. If can just be the README file, or you can look at the enums in msg.rs and query.rs but feedback on the interfaces would be great. I know both of you have solid experience with ERC721 and it's usage.

Note for extensibility, a contract can return more info than NftInfoResponse on an NftInfo query. But it must return those fields as a base. A client can parse the result with a superset of this struct if it know there are other fields (like "points") in the result. I chose to keep name and description on-chain for a much nicer interface and only the heavy image off-chain.

Please help me refine this one. Also tips on implementing a specific implementation (like allowing a minter) are welcome, even if they don't make it in the spec.

packages/cw721/README.md Outdated Show resolved Hide resolved
`Approve{approved, token_id, expires}` - Grants permission to `approved` to
transfer or send the given token. This can only be performed when
`env.sender` is the owner of the given `token_id` or an `operator`.
There can only be one approved account per token, and it is cleared once
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this could be annoying if i want to list the token for sale on multiple platforms at once?i like the idea of better accounting for approvals in general tho.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had multiple. But then I saw the erc721 spec which only allows exactly one. And figured it is simpler to implement, and they probably had a reason.

I am happy to allow N if you feel this is a real use case (list on multiple exchanges)

packages/cw721/README.md Outdated Show resolved Hide resolved

`ApproveAll{operator, expires}` - Grant `operator` permission to transfer or send
all tokens owner by `env.sender`. This is tied to the owner, not the
tokens and applies to any future token that the owner receives as well.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can ApproveAll be given to more than one operator at once?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, any number. Again taken from her 721 specs. Not sure why this is unlimited and approval is limited

@okwme
Copy link

okwme commented Aug 22, 2020

Looks good! Sorry for comments that I deleted, I ended up answering them myself just by reading more.

querying contracts in general kinda sucks, i appreciate the note about future improvements to pagination. One reason it sucks is cause you have to do stuff like "get total supply, then use total supply to increment through each index requesting token at index n". It would be nice to have pagination instead, but in lieu of pagination at this point is there the fallback alternative? are NFTs stored in a way that you can still query them if you don't know exactly what their ID is?

@ethanfrey
Copy link
Member Author

I would store them in a prefixed db by id. The sdk doesn't expose raw range searches, just raw key searches I can refine this pagination part and then just saw we implement that. It is not too hard, just a wrapper around the iterators.

@ethanfrey
Copy link
Member Author

Thank you for your review. Are you fine with the metadata definition?

Anything else (besides good pagination) you think would really benefit this spec?

@ethanfrey ethanfrey merged commit b0a47d0 into master Aug 24, 2020
@ethanfrey ethanfrey deleted the nft-721-spec branch August 24, 2020 21:14
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 this pull request may close these issues.

Define Spec of NFTs
3 participants