-
Notifications
You must be signed in to change notification settings - Fork 341
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
feat: clean GRC721 implementation #2117
base: master
Are you sure you want to change the base?
Conversation
LGTM to. As far as I'm concerned, there are no breaking changes. It's nice to have an implementation following the standard ERCs. I will update Flippando to use this realm once the PR is merged. |
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.
The only major thing is the addition of the external Ownable realm, but it's not a breaking change. LGTM.
after this PR is merged, i will update NFT's extensions realms @leohhhn |
<!-- please provide a detailed description of the changes made in this pull request. --> ## Description This PR removes the outdated (and full of mistakes) guide from the docs. After [this PR](#2117) is merged, we can add a create a new how-to guide on how to create a GRC721 token. I've added [an issue](#2288) about this. <details><summary>Contributors' checklist...</summary> - [x] Added new tests, or not needed, or not feasible - [x] Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory - [x] Updated the official documentation or not needed - [x] No breaking changes were made, or a `BREAKING CHANGE: xxx` message was included in the description - [x] Added references to related issues and PRs - [ ] Provided any useful hints for running manual tests - [ ] Added new benchmarks to [generated graphs](https://gnoland.github.io/benchmarks), if any. More info [here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md). </details>
owners *avl.Tree // tokenID > std.Address | ||
balances *avl.Tree // std.Address > # of owned tokens | ||
tokenApprovals *avl.Tree // tokenID > std.Address | ||
operatorApprovals *avl.Tree // "OwnerAddress:OperatorAddress" -> bool | ||
tokenURIs *avl.Tree // tokenID > URI |
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.
since you've at least 3 elements sharing the same key, I suggest having a single avl.Tree but returning an object.
nft.getApproved(tokenId) == owner | ||
} | ||
|
||
func (nft Token) update(to std.Address, tokenId string, auth std.Address) std.Address { |
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.
func (nft Token) update(to std.Address, tokenId string, auth std.Address) std.Address { | |
func (nft *Token) update(to std.Address, tokenId string, auth std.Address) std.Address { |
was the code currently working?
because it shouldn't.
} | ||
} | ||
|
||
std.Emit("Transfer", "from", from.String(), "to", to.String(), "tokenID", tokenId) |
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.
std.Emit("Transfer", "from", from.String(), "to", to.String(), "tokenID", tokenId) | |
std.Emit("Transfer", | |
"from", from.String(), | |
"to", to.String(), | |
"tokenID", tokenId, | |
) |
I suggest this format for emitted events, until we can have typesafe ones.
Description
Ported from here for visibility.
This is an experiment involving a refactor of the GRC721 package. The idea with this is that we might want a fully "by-spec" implementation, that people who know the standard can use out of the box.
The process for writing this was the following:
This PR replaces the old GRC721 implementation with the new one, and updates the
foo721
example realm, and removes thenft
example realm to only have a single example implementation.Contributors' checklist...
BREAKING CHANGE: xxx
message was included in the description