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

Add NFT metadata #38

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

xavikh
Copy link

@xavikh xavikh commented Dec 10, 2024

No description provided.

@xavikh xavikh requested a review from jordaniza December 10, 2024 14:16
@@ -38,7 +49,7 @@ contract Lock is ILock, ERC721Enumerable, UUPSUpgradeable, DaoAuthorizable, Reen

function supportsInterface(
bytes4 _interfaceId
) public view override(ERC721Enumerable) returns (bool) {
) public view override(ERC721Enumerable, ERC721URIStorageUpgradeable) returns (bool) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Strictly speaking, we should test that the supports interface check is correctly returning as expected for Enumerable vs URI Storage. Of the 2 I think Enumerable is more important to be able to introspect so would want to make sure that is not lost through the override

Copy link
Author

Choose a reason for hiding this comment

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

True. I added 2 asserts on test/escrow/escrow/Lock.t.sol to verify both interfaces are supported. I also checked and all the parent functions use || super.supportsInterface(..) so they should propagate correctly through the inheritance chain.

Copy link
Collaborator

@jordaniza jordaniza left a comment

Choose a reason for hiding this comment

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

Looks good

@xavikh xavikh requested a review from jordaniza December 11, 2024 10:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants