Query for name and symbol on ERC721 contracts regardless of whether or not they support the metadata interface

[adonesky1]

Some ERC721 contracts (e.g. Decentraland) only partially implement the metadata interface (described in the original EIP). We should query for fields included on the interface (name, symbol and tokenURI) regardless of whether or not the interface is fully implemented.

• CHANGED:
  • Removed requirement that EIP721 metadata be fully implemented to query for name, symbol and tokenURI fields on a contract.

Checklist

• Tests are included if applicable

Issue

Resolves MetaMask/metamask-extension#14459 & MetaMask/metamask-extension#14589 & MetaMask/metamask-extension#14518

[digiwand]

👍

[Gudahtt]

Does it make sense to use metadata at all if we don't understand the format? I'm 100% behind us making this class compatible with ERC721 contracts with alternative metadata formats, or no metadata. The format described in the EIP is optional after all. But if the contract doesn't support this metadata format, should we be assuming the fields we do find can be used in the same way?

e.g. if the contract had metadata of the format { name: ___, fullName: ___, where name is the symbol and fullName is a longer human-readable name. In that case we'd be wrongly using the symbol as the name.

Adding explicit support for other popular metadata formats (in later PRs) would be a safer alternate path for supporting those cases.

[FrederikBolding]

Does it make sense to use metadata at all if we don't understand the format? I'm 100% behind us making this class compatible with ERC721 contracts with alternative metadata formats, or no metadata. The format described in the EIP is optional after all. But if the contract doesn't support this metadata format, should we be assuming the fields we do find can be used in the same way?

e.g. if the contract had metadata of the format { name: ___, fullName: ___, where name is the symbol and fullName is a longer human-readable name. In that case we'd be wrongly using the symbol as the name.

Adding explicit support for other popular metadata formats (in later PRs) would be a safer alternate path for supporting those cases.

I don't think I understand what you mean here. We are not parsing the tokenURI data at this point. We are just calling functions on the contract that return metadata. These functions are pretty standardized.

[Gudahtt]

Hmm. ERC-721 seemed to imply they were optional, which led me to think they might be used for something else. But maybe you're right. Even if not explicitly guaranteed by the standard, it seems unlikely they'd be reused for some other purpose. Especially since they're also defined in ERC-20.

[FrederikBolding]

I am fine to approve this as long as we create issues to resolve the infinite loading problem in the extension and explore the idea of trying to parse non-standard contracts.

[Gudahtt]

LGTM!