-
Notifications
You must be signed in to change notification settings - Fork 388
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
fix(grc721): Make basic metadataNFT
implementation usable from external packages
#3495
base: master
Are you sure you want to change the base?
Conversation
…y (Name, Symbol, BalanceOf, etc.) - add SetTokenMetadata to IGRC721MetadataOnchain interface
metadataNFT
implementation usable from external packages
🛠 PR Checks SummaryAll Automated Checks passed. ✅ Manual Checks (for Reviewers):
Read More🤖 This bot helps streamline PR reviews by verifying automated checks and providing guidance for contributors and reviewers. ✅ Automated Checks (for Contributors):🟢 Maintainers must be able to edit this pull request (more info) ☑️ Contributor Actions:
☑️ Reviewer Actions:
📚 Resources:Debug
|
metadataNFT
implementation usable from external packagesmetadataNFT
implementation usable from external packages
…of ERC721 standard
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 don't know why owner check were done in the existing code, but I think making access control looser as it has been modified now would allow for more flexible application to other projects in the future.
Still, this seems related to policy, we should check with others for their opinions as well. Anyway LGTM
// Check if the caller is the owner of the token | ||
owner, err := s.basicNFT.OwnerOf(tid) | ||
if err != nil { | ||
return err | ||
} | ||
caller := std.PrevRealm().Addr() | ||
if caller != owner { | ||
return ErrCallerIsNotOwner | ||
} | ||
|
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.
Why did you remove this?
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.
As described in the description, I removed the owner check in SetTokenMetadata
. This change ensures that the decision of who can use this method is left to the NFT collection creator, rather than being predefined in the basic implementation.
In most cases, setting token metadata is typically a one-time operation, either automated by the contract or handled by the collection owner. Token owners usually do not have the capability to modify metadata.
By removing the owner check in the basic implementation, we allow greater flexibility, enabling both options.
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.
- grc721 metadata is implementation of opensea metadata standards.
- opensea does supports ERC-7015 to proof ownership. (which is pretty latest spec)
Therefore, removing owner logic might be work for now but as what @leohhhn said about refactoring, we won't have this issue :D
Hopefully we'll do a full refactor of the GRC721 package and we won't have issues with this :) |
I completely agree that the current GRC721 implementation could benefit from a full refactor. I’ve already started considering some approaches to address these issues. However, the current implementation of |
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 really don't have strong opinion for this pr, because full refactoring grc721 won't produce this kind of issues anymore.
For short-term solution, looks good to me.
The current implementation of
grc721_metadata.gno
has a critical usability issue where external packages can't accessbasic_nft.gno
methods when usingNewNFTWithMetadata()
. This is because:metadataNFT
struct ingrc721_metadata.gno
embeds*basicNFT
which is unexportedSetTokenMetadata
,TokenMetadata
)Changes made:
metadataNFT
for allbasicNFT
functionality (Name
,Symbol
,BalanceOf
, etc.)SetTokenMetadata
, since this is something that should be left for NFT collection creator to choose who should be able to use this methodThese changes ensure that external packages can properly use the metadata NFT implementation with full NFT functionality while maintaining proper encapsulation of the underlying
basicNFT
struct interface.An example of a working usage of this updated
grc721_metadata.gno
can be found in this file.