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: fix getting nft tokenURI #4136

Merged
merged 6 commits into from
Apr 12, 2024
Merged

fix: fix getting nft tokenURI #4136

merged 6 commits into from
Apr 12, 2024

Conversation

sahar-fehri
Copy link
Contributor

@sahar-fehri sahar-fehri commented Apr 8, 2024

Explanation

User has reached out because when they import their NFT they are not able to see it.
The smart contract is deployed on linea-mainnet. It is a ERC404 contract (mixed implementation of ERC721/ERC20).
The contract does not support Metadata interface but it has the function tokenURI().

Contract address with this issue : 0x93A5467fbC9D7dbB5713752Ec5a934a5A752AC07

References

Changelog

@metamask/package-assets-controllers

  • Fixed: Instead of throwing error inside getTokenURI fct when the contract does not support Metadata interface, i am only logging the error.

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've highlighted breaking changes using the "BREAKING" category above as appropriate

@sahar-fehri sahar-fehri requested a review from a team as a code owner April 8, 2024 10:24
// Do not throw error here, supporting Metadata interface is optional even though majority of ERC721 nfts do support it.
// This change is made because of instances of NFTs that are ERC404( mixed ERC20 / ERC721 implementation).
// As of today, ERC404 is unofficial but some people use it, the contract does not support Metadata interface, but it has the tokenURI() fct.
console.error('Contract does not support ERC721 metadata interface.');
}
return contract.tokenURI(tokenId);
Copy link
Contributor Author

@sahar-fehri sahar-fehri Apr 8, 2024

Choose a reason for hiding this comment

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

In cases where contract is ERC721 and does not have fct tokenURI, we are already catching the error here

, the user will be able to add the nft, but the image wont be displayed.

@sahar-fehri sahar-fehri marked this pull request as draft April 8, 2024 10:46
@sahar-fehri sahar-fehri marked this pull request as ready for review April 8, 2024 12:59
@bergeron
Copy link
Contributor

bergeron commented Apr 9, 2024

Can you share the contract address of this NFT?
We're saying it has tokenURI, but does not support the metadata interface?
Since this is the metadata interface, this means it does not have either name() or symbol?

function name() returns string
function symbol() returns string
function tokenURI(uint256 _tokenId) returns string

@sahar-fehri
Copy link
Contributor Author

Can you share the contract address of this NFT? We're saying it has tokenURI, but does not support the metadata interface? Since this is the metadata interface, this means it does not have either name() or symbol?

function name() returns string
function symbol() returns string
function tokenURI(uint256 _tokenId) returns string

This is the contract address: 0x93A5467fbC9D7dbB5713752Ec5a934a5A752AC07
The contract does have a name/symbol/tokenURI. It is just not extending the ERC721Metadata interface, so when we do check if the contract supports that interfaceId 0x5b5e139f we get a false.

The extension of the metadata interface is optional overall for ERC721, but i think a lot of decent NFTs support it already, mostly because latest openzeppelin ERC721 extend the IERC721Metadata automatically

@bergeron
Copy link
Contributor

bergeron commented Apr 10, 2024

I think I figured out my confusion. It looked to me like the contract supports erc-165. And has name, symbol , and tokenUri. So I was thinking it should pass the metadata interface check. But I guess its this part:

  function supportsInterface(
    bytes4 interfaceId
  ) public view virtual returns (bool) {
    return
      interfaceId == type(IERC404).interfaceId ||
      interfaceId == type(IERC165).interfaceId;
  }

where they only announce support for IERC404. Therefore there are cases where it makes sense to call tokenUri even if we don't pass the supportsInterface check for metadata.

bergeron
bergeron previously approved these changes Apr 10, 2024
Copy link
Contributor

@kanthesha kanthesha left a comment

Choose a reason for hiding this comment

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

LGTM.

@sahar-fehri sahar-fehri merged commit bfe71c4 into main Apr 12, 2024
139 checks passed
@sahar-fehri sahar-fehri deleted the fix/fix-get-nft-uri branch April 12, 2024 08:14
@montelaidev montelaidev mentioned this pull request Apr 17, 2024
3 tasks
montelaidev added a commit that referenced this pull request Apr 17, 2024
## Explanation

## References

## Changelog

## [13.0.0] Accounts Controller

### Changed

- Fix update setSelectedAccount to throw if the id is not found
([#4167](#4167))
- Fix normal account indexing naming with index gap
([#4089](#4089))
- **BREAKING** Bump peer dependency `@metamask/snaps-controllers` to
`^6.0.3` and dependencies `@metamask/snaps-sdk` to `^3.1.1`,
`@metamask/eth-snap-keyring` to
`^3.0.0`([#4090](#4090))

## [28.0.0] Assets Controller

### Added

- Add reservoir migration
([#4030](#4030))

### Changed

- Fix getting nft tokenURI
([#4136](#4136))
- **BREAKING** Bump peer dependency on `@metamask/keyring-controller`
([#4090](#4090))
- Fix token detection during account change
([#4133](#4133))
- Fix update nft metadata when toggles off
([#4096](#4096))
- Adds `tokenMethodIncreaseAllowance`
([#4069](#4069))
- Fix mantle token mispriced
([#4045](#4045))

## [15.0.0] Keyring Controller

### Changed

- **BREAKING** use getAccounts on HD Keyring when calling addNewAccount
([#4158](#4158))
- Pass CAIP-2 scope to execution context
([#4090](#4090))
- Allow gas limits to be changed during #addPaymasterData
([#3942](#3942))

## [10.0.0] Preferences Controller

### Changed

- **BREAKING** Bump peer dependency on `@metamask/keyring-controller` to
`^15.0.0` ([#4090](#4090))
- Restore previous behavior of toChecksumHexAddress
([#4046](#4046))

## [15.0.0] Signature Controller

### Changed

- **BREAKING** Bump peer dependency on `@metamask/keyring-controller` to
`^15.0.0` ([#4090](#4090))

## [8.0.0] User Operation Controller 

### Changed

- **BREAKING** Bump peer dependency on `@metamask/keyring-controller` to
`^15.0.0` and Pass CAIP-2 scope to execution context
([#4090](#4090))
- Allow gas limits to be changed during #addPaymasterData
([#3942](#3942))


## Checklist

- [ ] I've updated the test suite for new or updated code as appropriate
- [ ] I've updated documentation (JSDoc, Markdown, etc.) for new or
updated code as appropriate
- [ ] I've highlighted breaking changes using the "BREAKING" category
above as appropriate
sahar-fehri added a commit to MetaMask/metamask-extension that referenced this pull request Apr 19, 2024
## **Description**

We have been contacted about an NFT on linea mainnet that was not
showing properly on MM.
There is this core PR that has been merged to fix fetch behavior for
ERC404: MetaMask/core#4136

This PR adds a patch for the core fix.

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/24029?quickstart=1)

## **Related issues**

Fixes:
Related: MetaMask/core#4136

## **Manual testing steps**

1. Go to this page...
2.
3.

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->


https://github.com/MetaMask/metamask-extension/assets/10994169/cea457c3-87f2-4157-9efb-2ebc69176ac0


### **After**

<!-- [screenshots/recordings] -->



https://github.com/MetaMask/metamask-extension/assets/10994169/783d4292-c2c0-4ea9-9c7d-59dde88b4b3d


## **Pre-merge author checklist**

- [ ] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've completed the PR template to the best of my ability
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.
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.

None yet

3 participants