From bfe71c4d5fc5387627a72ac14a5d887e62e09104 Mon Sep 17 00:00:00 2001
From: sahar-fehri <sahar.fehri@consensys.net>
Date: Fri, 12 Apr 2024 10:14:23 +0200
Subject: [PATCH] fix: fix getting nft tokenURI (#4136)

## 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

<!--
Are there any issues that this pull request is tied to? Are there other
links that reviewers should consult to understand these changes better?

For example:

* Fixes #12345
* Related to #67890
-->

## Changelog

<!--
If you're making any consumer-facing changes, list those changes here as
if you were updating a changelog, using the template below as a guide.

(CATEGORY is one of BREAKING, ADDED, CHANGED, DEPRECATED, REMOVED, or
FIXED. For security-related issues, follow the Security Advisory
process.)

Please take care to name the exact pieces of the API you've added or
changed (e.g. types, interfaces, functions, or methods).

If there are any breaking changes, make sure to offer a solution for
consumers to follow once they upgrade to the changes.

Finally, if you're only making changes to development scripts or tests,
you may replace the template below with "None".
-->

### `@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
---
 .../src/AssetsContractController.test.ts      | 40 +++++++++++++----
 ...tractControllerWithNetworkClientId.test.ts | 43 ++++++++++++++-----
 .../NftStandards/ERC721/ERC721Standard.ts     |  5 ++-
 3 files changed, 67 insertions(+), 21 deletions(-)

diff --git a/packages/assets-controllers/src/AssetsContractController.test.ts b/packages/assets-controllers/src/AssetsContractController.test.ts
index 21e3cd2c08..1ac2b3b0fc 100644
--- a/packages/assets-controllers/src/AssetsContractController.test.ts
+++ b/packages/assets-controllers/src/AssetsContractController.test.ts
@@ -608,10 +608,15 @@ describe('AssetsContractController', () => {
     messenger.clearEventSubscriptions('NetworkController:networkDidChange');
   });
 
-  it('should throw an error when address given is not an ERC-721 NFT', async () => {
+  it('should not throw an error when address given does not support NFT Metadata interface', async () => {
     const { assetsContract, messenger, provider, networkClientConfiguration } =
       await setupAssetContractControllers();
     assetsContract.configure({ provider });
+    const errorLogSpy = jest
+      .spyOn(console, 'error')
+      .mockImplementationOnce(() => {
+        /**/
+      });
     mockNetworkWithDefaultChainId({
       networkClientConfiguration,
       mocks: [
@@ -630,17 +635,34 @@ describe('AssetsContractController', () => {
             result: '0x',
           },
         },
+        {
+          request: {
+            method: 'eth_call',
+            params: [
+              {
+                to: '0x0000000000000000000000000000000000000000',
+                data: '0xc87b56dd0000000000000000000000000000000000000000000000000000000000000000',
+              },
+              'latest',
+            ],
+          },
+          response: {
+            result:
+              '0x0000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000000000002468747470733a2f2f6170692e676f6473756e636861696e65642e636f6d2f636172642f3000000000000000000000000000000000000000000000000000000000',
+          },
+        },
       ],
     });
-    const result = async () => {
-      await assetsContract.getERC721TokenURI(
-        '0x0000000000000000000000000000000000000000',
-        '0',
-      );
-    };
+    const uri = await assetsContract.getERC721TokenURI(
+      '0x0000000000000000000000000000000000000000',
+      '0',
+    );
+    expect(uri).toBe('https://api.godsunchained.com/card/0');
+    expect(errorLogSpy).toHaveBeenCalledTimes(1);
+    expect(errorLogSpy.mock.calls).toContainEqual([
+      'Contract does not support ERC721 metadata interface.',
+    ]);
 
-    const error = 'Contract does not support ERC721 metadata interface.';
-    await expect(result).rejects.toThrow(error);
     messenger.clearEventSubscriptions('NetworkController:networkDidChange');
   });
 
diff --git a/packages/assets-controllers/src/AssetsContractControllerWithNetworkClientId.test.ts b/packages/assets-controllers/src/AssetsContractControllerWithNetworkClientId.test.ts
index b69148ddba..d064192b68 100644
--- a/packages/assets-controllers/src/AssetsContractControllerWithNetworkClientId.test.ts
+++ b/packages/assets-controllers/src/AssetsContractControllerWithNetworkClientId.test.ts
@@ -420,7 +420,7 @@ describe('AssetsContractController with NetworkClientId', () => {
     messenger.clearEventSubscriptions('NetworkController:stateChange');
   });
 
-  it('should throw an error when address given is not an ERC-721 NFT', async () => {
+  it('should not throw an error when address given is does not support NFT Metadata interface', async () => {
     const { assetsContract, messenger, networkClientConfiguration } =
       await setupAssetContractControllers();
     mockNetworkWithDefaultChainId({
@@ -441,18 +441,39 @@ describe('AssetsContractController with NetworkClientId', () => {
             result: '0x',
           },
         },
+        {
+          request: {
+            method: 'eth_call',
+            params: [
+              {
+                to: '0x0000000000000000000000000000000000000000',
+                data: '0xc87b56dd0000000000000000000000000000000000000000000000000000000000000000',
+              },
+              'latest',
+            ],
+          },
+          response: {
+            result:
+              '0x0000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000000000002468747470733a2f2f6170692e676f6473756e636861696e65642e636f6d2f636172642f3000000000000000000000000000000000000000000000000000000000',
+          },
+        },
       ],
     });
-    const result = async () => {
-      await assetsContract.getERC721TokenURI(
-        '0x0000000000000000000000000000000000000000',
-        '0',
-        'mainnet',
-      );
-    };
-
-    const error = 'Contract does not support ERC721 metadata interface.';
-    await expect(result).rejects.toThrow(error);
+    const errorLogSpy = jest
+      .spyOn(console, 'error')
+      .mockImplementationOnce(() => {
+        /**/
+      });
+    const uri = await assetsContract.getERC721TokenURI(
+      '0x0000000000000000000000000000000000000000',
+      '0',
+      'mainnet',
+    );
+    expect(uri).toBe('https://api.godsunchained.com/card/0');
+    expect(errorLogSpy).toHaveBeenCalledTimes(1);
+    expect(errorLogSpy.mock.calls).toContainEqual([
+      'Contract does not support ERC721 metadata interface.',
+    ]);
     messenger.clearEventSubscriptions('NetworkController:stateChange');
   });
 
diff --git a/packages/assets-controllers/src/Standards/NftStandards/ERC721/ERC721Standard.ts b/packages/assets-controllers/src/Standards/NftStandards/ERC721/ERC721Standard.ts
index d8bfd4bde7..d788e4b9b3 100644
--- a/packages/assets-controllers/src/Standards/NftStandards/ERC721/ERC721Standard.ts
+++ b/packages/assets-controllers/src/Standards/NftStandards/ERC721/ERC721Standard.ts
@@ -91,7 +91,10 @@ export class ERC721Standard {
       address,
     );
     if (!supportsMetadata) {
-      throw new Error('Contract does not support ERC721 metadata interface.');
+      // 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);
   };