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(examples): define metadata & royalty info for GRC721 realm #1962

Merged
merged 27 commits into from
May 10, 2024

Conversation

linhpn99
Copy link
Contributor

@linhpn99 linhpn99 commented Apr 20, 2024

Contributors' checklist...
  • Added new tests, or not needed, or not feasible
  • Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
  • Updated the official documentation or not needed
  • No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
  • Added references to related issues and PRs
  • Provided any useful hints for running manual tests
  • Added new benchmarks to generated graphs, if any. More info here.

@linhpn99 linhpn99 requested review from a team as code owners April 20, 2024 13:17
@github-actions github-actions bot added the 🧾 package/realm Tag used for new Realms or Packages. label Apr 20, 2024
@linhpn99 linhpn99 changed the title chore(examples) : add royalty info for grc721 chore: add royalty info for grc721 Apr 20, 2024
@linhpn99 linhpn99 changed the title chore: add royalty info for grc721 chore: define royalty info for grc721 Apr 20, 2024
@linhpn99 linhpn99 changed the title chore: define royalty info for grc721 feat(examples): define royalty info for grc721 Apr 20, 2024
Copy link
Contributor

@n0izn0iz n0izn0iz left a comment

Choose a reason for hiding this comment

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

I think that purely using a callback based approach might be more suited, so the royalty can be more dynamic
Hence only define RoyaltyInfo in the interface

@sunspirit99
Copy link

I think that purely using a callback based approach might be more suited, so the royalty can be more dynamic Hence only define RoyaltyInfo in the interface

I have considered and updated the code according to your suggestions. Can u take a look ? @n0izn0iz

@linhpn99 linhpn99 requested a review from n0izn0iz April 22, 2024 15:58
@n0izn0iz
Copy link
Contributor

n0izn0iz commented Apr 22, 2024

I see you're trying to follow eth spec, I think we should split the interfaces like so

// base collection info
type IGRC721CollectionMetadata interface {
	Name() string
	Symbol() string
}

// follows eth spec
type IGRC721Metadata interface {
	IGRC721CollectionMetadata
	TokenURI(tid TokenID) (string, error)
}

// on-chain metadata, for on-chain nft games for example
type IGRC721MetadataOnChain interface {
	IGRC721CollectionMetadata
	TokenMetadata(tid TokenID) (Metadata, error)
}

wdyt?

do you intend to close #1960 in favor of this PR?

@n0izn0iz
Copy link
Contributor

also maybe we should rename IGRC721Royalty to IGRC2981 to follow the eth standard
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/interfaces/IERC2981.sol

@linhpn99
Copy link
Contributor Author

I see you're trying to follow eth spec, I think we should split the interfaces like so

// base collection info
type IGRC721CollectionMetadata interface {
	Name() string
	Symbol() string
}

// follows eth spec
type IGRC721Metadata interface {
	IGRC721CollectionMetadata
	TokenURI(tid TokenID) (string, error)
}

// on-chain metadata, for on-chain nft games for example
type IGRC721MetadataOnChain interface {
	IGRC721CollectionMetadata
	TokenMetadata(tid TokenID) (Metadata, error)
}

wdyt?

do you intend to close #1960 in favor of this PR?

yeah, i think it's clear

@linhpn99 linhpn99 changed the title feat(examples): define royalty info for grc721 feat(examples): define medata & royalty info for grc721 realm Apr 22, 2024
@linhpn99 linhpn99 changed the title feat(examples): define medata & royalty info for grc721 realm feat(examples): define medata & royalty info for GRC721 realm Apr 22, 2024
@linhpn99 linhpn99 changed the title feat(examples): define medata & royalty info for GRC721 realm feat(examples): define metadata & royalty info for GRC721 realm Apr 22, 2024
@linhpn99
Copy link
Contributor Author

linhpn99 commented May 7, 2024

@sunspirit9999

Hey, sorry for the delay - part of the team was off for the past week due to national holidays🙏

I need to look into the ERC standards in detail to give you a proper review. I will do this first thing in the morning.

i hope u give some comments for this

Copy link
Contributor

@leohhhn leohhhn left a comment

Choose a reason for hiding this comment

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

Hey, thank you for this contribution, and sorry for the delay again 🙏

I left some comments, mostly nitpicks. Please, when you work on them, commit changes for each comment and post the commit under the convo so that I can see that the change was implemented. 🙏

examples/gno.land/p/demo/grc/grc721/grc721_royalty.gno Outdated Show resolved Hide resolved
examples/gno.land/p/demo/grc/grc721/igrc721_royalty.gno Outdated Show resolved Hide resolved
examples/gno.land/p/demo/grc/grc721/grc721_royalty.gno Outdated Show resolved Hide resolved
@linhpn99
Copy link
Contributor Author

linhpn99 commented May 8, 2024

@leohhhn I have refactored PR after your review

@linhpn99 linhpn99 requested a review from leohhhn May 8, 2024 07:01
@linhpn99
Copy link
Contributor Author

linhpn99 commented May 9, 2024

@leohhhn I have refactored PR after your review

Is this PR eglible for merging ? Please feed back if you have any comments

@leohhhn
Copy link
Contributor

leohhhn commented May 9, 2024

Hey @linhpn99, let me take a look at this tomorrow.

Copy link

codecov bot commented May 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 54.96%. Comparing base (abaf103) to head (b811cd5).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1962      +/-   ##
==========================================
- Coverage   54.96%   54.96%   -0.01%     
==========================================
  Files         481      481              
  Lines       67407    67407              
==========================================
- Hits        37052    37049       -3     
- Misses      27336    27337       +1     
- Partials     3019     3021       +2     

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

@linhpn99
Copy link
Contributor Author

Hey @linhpn99, let me take a look at this tomorrow.

Can u approve the workflows ?

Copy link
Contributor

@leohhhn leohhhn left a comment

Choose a reason for hiding this comment

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

LGTM, good to merge after CI passes

@linhpn99
Copy link
Contributor Author

LGTM, good to merge after CI passes

https://github.com/gnolang/gno/actions/runs/9026807461/job/24822204512?pr=1962

Why am I encountering this error in this workflow? The test still passes locally.

@linhpn99 linhpn99 requested a review from leohhhn May 10, 2024 14:23
@linhpn99
Copy link
Contributor Author

LGTM, good to merge after CI passes

https://github.com/gnolang/gno/actions/runs/9026807461/job/24822204512?pr=1962

Why am I encountering this error in this workflow? The test still passes locally.

This test seem to take a long time to execute ( > 1s ), could it be cancelled due to a timeout ?

@leohhhn
Copy link
Contributor

leohhhn commented May 10, 2024

tm2 / test (1.21.x, _test.pkg.bft) (pull_request) is a test case that is bugged out and fails often. Ignore it :)

Merging now! Thank you for the contribution 🚀

@leohhhn leohhhn merged commit a03eeb3 into gnolang:master May 10, 2024
214 of 215 checks passed
@linhpn99
Copy link
Contributor Author

tm2 / test (1.21.x, _test.pkg.bft) (pull_request) is a test case that is bugged out and fails often. Ignore it :)

Merging now! Thank you for the contribution 🚀

Thank you for your recognition 🤝

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
Status: Done
Status: Done
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants