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

fix(L-02): Inconsistent Implementations in ERC-721 Extension #331 #333

Merged
merged 13 commits into from
Oct 17, 2024

Conversation

bidzyyys
Copy link
Collaborator

Resolves #312

@bidzyyys bidzyyys self-assigned this Oct 10, 2024

fn base_uri(&self) -> String {
Copy link
Member

Choose a reason for hiding this comment

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

In this case #[interface_id] won't be that helpful. Because methods inside IErc721Metadata trait are not same as in solidity (token_uri is missing). So computed interface id will be different..

Copy link
Collaborator

Choose a reason for hiding this comment

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

If this is the only thing preventing this PR I would say we don't expose #[interface_id] for the metadata extension(since I don't think it is part of the EIP itself).
One thing we should do is add in the Anotra docs or at the top of the contract that people should expose token_uri manually(I know it is on the internal function description, but still)

Copy link
Collaborator

@ggonzalez94 ggonzalez94 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 overall, I like using the impl as a parameter to the function(keeps things more clean). Let's finalize the discussion around ERC165 support and if we need to add a note to the docs

contracts/src/token/erc721/extensions/metadata.rs Outdated Show resolved Hide resolved

fn base_uri(&self) -> String {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this is the only thing preventing this PR I would say we don't expose #[interface_id] for the metadata extension(since I don't think it is part of the EIP itself).
One thing we should do is add in the Anotra docs or at the top of the contract that people should expose token_uri manually(I know it is on the internal function description, but still)

Copy link
Collaborator

@ggonzalez94 ggonzalez94 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator Author

@bidzyyys bidzyyys left a comment

Choose a reason for hiding this comment

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

Good job @qalisander !

@qalisander qalisander self-requested a review October 17, 2024 12:32
Copy link
Member

@qalisander qalisander left a comment

Choose a reason for hiding this comment

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

LGTM

@bidzyyys bidzyyys linked an issue Oct 17, 2024 that may be closed by this pull request
1 task
@bidzyyys bidzyyys merged commit 47d2d9c into v0.1.0 Oct 17, 2024
20 checks passed
@bidzyyys bidzyyys deleted the bidzyyys/fix-L-02 branch October 17, 2024 13:08
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.

[Audit]: L-02 Inconsistent Implementations in ERC-721 Extension
3 participants