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

feat(p/grc721): add SetTokenURI to IGRC721 #1309

Merged
merged 16 commits into from
Dec 21, 2023

Conversation

irreverentsimplicity
Copy link
Contributor

The current grc721 implementation lacks the ability to set the tokenURI metadata for an NFT. There are placheolders inside the struct, but the actual setter was not written. This PR add the following functionality in the grc folder inside the p folder:

  • new type grc721 interface for TokenURI on top of the existing TokenID
  • new error type in errors.gno
  • SetTokenURI() implementation in basic_nft.gno
  • new test added to basic_nft_test.gno

AFAIK there are no breaking changes in this PR, tests are passing locally.

Let me know any issues.

@irreverentsimplicity irreverentsimplicity requested a review from a team as a code owner October 27, 2023 12:45
@github-actions github-actions bot added the 🧾 package/realm Tag used for new Realms or Packages. label Oct 27, 2023
Copy link

netlify bot commented Nov 11, 2023

Deploy Preview for gno-docs2 canceled.

Name Link
🔨 Latest commit 8eaee43
🔍 Latest deploy log https://app.netlify.com/sites/gno-docs2/deploys/654f493a26f4ca0008189986

Copy link

codecov bot commented Nov 11, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d10aa9b) 56.11% compared to head (543364e) 47.14%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1309      +/-   ##
==========================================
- Coverage   56.11%   47.14%   -8.98%     
==========================================
  Files         432      372      -60     
  Lines       65898    61439    -4459     
==========================================
- Hits        36978    28963    -8015     
- Misses      26036    30097    +4061     
+ Partials     2884     2379     -505     
Flag Coverage Δ
go-1.21.x ∅ <ø> (∅)
misc ∅ <ø> (∅)
misc-_test.genstd ∅ <ø> (∅)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@thehowl thehowl changed the title Adding SetTokenURI to our current grc721 implementation feat(p/grc721): add SetTokenURI to IGRC721 Nov 13, 2023
examples/gno.land/p/demo/grc/grc721/basic_nft.gno Outdated Show resolved Hide resolved
examples/gno.land/p/demo/grc/grc721/basic_nft.gno Outdated Show resolved Hide resolved
examples/gno.land/p/demo/grc/grc721/basic_nft.gno Outdated Show resolved Hide resolved
examples/gno.land/p/demo/grc/grc721/basic_nft_test.gno Outdated Show resolved Hide resolved
@moul
Copy link
Member

moul commented Dec 6, 2023

I'm adding myself as a reviewer (I believe I developed the initial version, if I'm not mistaken). I'll also need NFTs for transferable memberships and attributes related to DAOs in the future. Please @zivkovicmilos and @thehowl, feel free to review. You can also rely on me, particularly for the "DeFi" part.

@moul
Copy link
Member

moul commented Dec 20, 2023

LGTM

Please clean up the unused error constant, and fix the gno lint issues that are causing the CI to fail (ignore the tm2/test CI failure).

@irreverentsimplicity
Copy link
Contributor Author

thanks, on it

Copy link
Member

@zivkovicmilos zivkovicmilos left a comment

Choose a reason for hiding this comment

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

Sorry for the late review on this 🙏

Looks good 💯

For reference:
https://eips.ethereum.org/EIPS/eip-721

@irreverentsimplicity
Copy link
Contributor Author

removed unused error message, fixed linting, required checks seems to work

@thehowl
Copy link
Member

thehowl commented Dec 21, 2023

mod-tidy check is failing :)

@irreverentsimplicity
Copy link
Contributor Author

I just saw, on it

@irreverentsimplicity
Copy link
Contributor Author

all checks passing now, except 2 Codecov. Not sure how I should go about this, or if there's anything needed on my part?

@thehowl
Copy link
Member

thehowl commented Dec 21, 2023

Stupid codecov. Other failures we get all the time.

Going ahead and merging

@thehowl thehowl merged commit f2034b1 into gnolang:master Dec 21, 2023
185 of 187 checks passed
@irreverentsimplicity
Copy link
Contributor Author

my first PR merged, wo-hoo! Somewhere there's a beer waiting for me.

gfanton pushed a commit to moul/gno that referenced this pull request Jan 18, 2024
The current grc721 implementation lacks the ability to set the tokenURI
metadata for an NFT. There are placheolders inside the struct, but the
actual setter was not written. This PR add the following functionality
in the `grc` folder inside the `p` folder:

- new type grc721 interface for `TokenURI` on top of the existing
`TokenID`
- new error type in `errors.gno`
- SetTokenURI() implementation in `basic_nft.gno`
- new test added to `basic_nft_test.gno`

AFAIK there are no breaking changes in this PR, tests are passing
locally.

Let me know any issues.

---------

Co-authored-by: Morgan <git@howl.moe>
@irreverentsimplicity irreverentsimplicity deleted the feat/grc-721_SetTokenURI branch August 2, 2024 05:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🧾 package/realm Tag used for new Realms or Packages.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants