-
Notifications
You must be signed in to change notification settings - Fork 355
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
Nft 721 spec #50
Conversation
7864b38
to
fe20d66
Compare
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 Note for extensibility, a contract can return more info than 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. |
464c6f2
to
4f27016
Compare
packages/cw721/README.md
Outdated
`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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
|
||
`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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
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? |
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. |
Thank you for your review. Are you fine with the metadata definition? Anything else (besides good pagination) you think would really benefit this spec? |
Closes #43
Define a 721 spec for NFTs, focusing on transfering and ownership.
How to make MetaData extensible?(not possible with rust unless I use macros, did research)