fix: avoid multiple calls to refresh nft metadata (#4325)

## Explanation

This PR updates the logic of `updateNftMetadata` function and adds a
mutex to synchronize state updates.

Inside `getNftInformation` function, before returning the image, we
prioritize checking for api result then fallback to checking if there
was an image in the blockchain result.

The nft detection will be enabled by default in the future, this will
avoid making unnecessary state updates when the image string returned
from NFT-API is different than the string returned from
`blockchainMetadata`.

## References

* Related to MetaMask/metamask-mobile#9759


## 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/assets-controllers`

- **Added**: Added Mutex lock in the `updateNftMetadata` function.

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

Author: sahar-fehri

packages/assets-controllers/src/NftController.test.ts

@@ -1380,7 +1380,7 @@ describe('NftController', () => {
         nftController.state.allNfts[selectedAddress][ChainId.mainnet][0],
       ).toStrictEqual({
         address: ERC721_KUDOSADDRESS,
-        image: 'Kudos Image (directly from tokenURI)',
+        image: 'url',
         name: 'Kudos Name (directly from tokenURI)',
         description: 'Kudos Description (directly from tokenURI)',
         tokenId: ERC721_KUDOS_TOKEN_ID,
@@ -3917,5 +3917,108 @@ describe('NftController', () => {
 
       expect(spy).toHaveBeenCalledTimes(1);
     });
+
+    it('should call getNftInformation only one time per interval', async () => {
+      const tokenURI = 'https://api.pudgypenguins.io/lil/4';
+      const mockGetERC721TokenURI = jest.fn().mockResolvedValue(tokenURI);
+      const { nftController, triggerPreferencesStateChange } = setupController({
+        options: {
+          getERC721TokenURI: mockGetERC721TokenURI,
+        },
+      });
+      const selectedAddress = OWNER_ADDRESS;
+      const spy = jest.spyOn(nftController, 'updateNft');
+      const testNetworkClientId = 'sepolia';
+      await nftController.addNft('0xtest', '3', {
+        nftMetadata: { name: '', description: '', image: '', standard: '' },
+        networkClientId: testNetworkClientId,
+      });
+
+      nock('https://api.pudgypenguins.io/lil').get('/4').reply(200, {
+        name: 'name pudgy',
+        image: 'url pudgy',
+        description: 'description pudgy',
+      });
+      const testInputNfts: Nft[] = [
+        {
+          address: '0xtest',
+          description: null,
+          favorite: false,
+          image: null,
+          isCurrentlyOwned: true,
+          name: null,
+          standard: 'ERC721',
+          tokenId: '3',
+          tokenURI: 'https://api.pudgypenguins.io/lil/4',
+        },
+      ];
+
+      // Make first call to updateNftMetadata should trigger state update
+      await nftController.updateNftMetadata({
+        nfts: testInputNfts,
+        networkClientId: testNetworkClientId,
+      });
+      expect(spy).toHaveBeenCalledTimes(1);
+
+      expect(
+        nftController.state.allNfts[selectedAddress][SEPOLIA.chainId][0],
+      ).toStrictEqual({
+        address: '0xtest',
+        description: 'description pudgy',
+        image: 'url pudgy',
+        name: 'name pudgy',
+        tokenId: '3',
+        standard: 'ERC721',
+        favorite: false,
+        isCurrentlyOwned: true,
+        tokenURI: 'https://api.pudgypenguins.io/lil/4',
+      });
+
+      spy.mockClear();
+
+      // trigger calling updateNFTMetadata again on the same account should not trigger state update
+      const spy2 = jest.spyOn(nftController, 'updateNft');
+      await nftController.updateNftMetadata({
+        nfts: testInputNfts,
+        networkClientId: testNetworkClientId,
+      });
+      // No updates to state should be made
+      expect(spy2).toHaveBeenCalledTimes(0);
+
+      // trigger preference change and change selectedAccount
+      const testNewAccountAddress = 'OxDifferentAddress';
+      triggerPreferencesStateChange({
+        ...getDefaultPreferencesState(),
+        selectedAddress: testNewAccountAddress,
+      });
+
+      spy.mockClear();
+      await nftController.addNft('0xtest', '4', {
+        nftMetadata: { name: '', description: '', image: '', standard: '' },
+        networkClientId: testNetworkClientId,
+      });
+
+      const testInputNfts2: Nft[] = [
+        {
+          address: '0xtest',
+          description: null,
+          favorite: false,
+          image: null,
+          isCurrentlyOwned: true,
+          name: null,
+          standard: 'ERC721',
+          tokenId: '4',
+          tokenURI: 'https://api.pudgypenguins.io/lil/4',
+        },
+      ];
+
+      const spy3 = jest.spyOn(nftController, 'updateNft');
+      await nftController.updateNftMetadata({
+        nfts: testInputNfts2,
+        networkClientId: testNetworkClientId,
+      });
+      // When the account changed, and updateNftMetadata is called state update should be triggered
+      expect(spy3).toHaveBeenCalledTimes(1);
+    });
   });
 });

packages/assets-controllers/src/NftController.ts

@@ -729,7 +729,7 @@ export class NftController extends BaseController<
       name: blockchainMetadata?.name ?? nftApiMetadata?.name ?? null,
       description:
         blockchainMetadata?.description ?? nftApiMetadata?.description ?? null,
-      image: blockchainMetadata?.image ?? nftApiMetadata?.image ?? null,
+      image: nftApiMetadata?.image ?? blockchainMetadata?.image ?? null,
       standard:
         blockchainMetadata?.standard ?? nftApiMetadata?.standard ?? null,
       tokenURI: blockchainMetadata?.tokenURI ?? null,
@@ -1450,56 +1450,62 @@ export class NftController extends BaseController<
     userAddress?: string;
     networkClientId?: NetworkClientId;
   }) {
-    const chainId = this.#getCorrectChainId({ networkClientId });
+    const releaseLock = await this.#mutex.acquire();
 
-    const nftsWithChecksumAdr = nfts.map((nft) => {
-      return {
-        ...nft,
-        address: toChecksumHexAddress(nft.address),
-      };
-    });
-    const nftMetadataResults = await Promise.all(
-      nftsWithChecksumAdr.map(async (nft) => {
-        const resMetadata = await this.#getNftInformation(
-          nft.address,
-          nft.tokenId,
-          networkClientId,
-        );
+    try {
+      const chainId = this.#getCorrectChainId({ networkClientId });
+
+      const nftsWithChecksumAdr = nfts.map((nft) => {
         return {
-          nft,
-          newMetadata: resMetadata,
+          ...nft,
+          address: toChecksumHexAddress(nft.address),
         };
-      }),
-    );
-
-    // We want to avoid updating the state if the state and fetched nft info are the same
-    const nftsWithDifferentMetadata: NftUpdate[] = [];
-    const { allNfts } = this.state;
-    const stateNfts = allNfts[userAddress]?.[chainId] || [];
-
-    nftMetadataResults.forEach((singleNft) => {
-      const existingEntry: Nft | undefined = stateNfts.find(
-        (nft) =>
-          nft.address.toLowerCase() === singleNft.nft.address.toLowerCase() &&
-          nft.tokenId === singleNft.nft.tokenId,
+      });
+      const nftMetadataResults = await Promise.all(
+        nftsWithChecksumAdr.map(async (nft) => {
+          const resMetadata = await this.#getNftInformation(
+            nft.address,
+            nft.tokenId,
+            networkClientId,
+          );
+          return {
+            nft,
+            newMetadata: resMetadata,
+          };
+        }),
       );
 
-      if (existingEntry) {
-        const differentMetadata = compareNftMetadata(
-          singleNft.newMetadata,
-          existingEntry,
+      // We want to avoid updating the state if the state and fetched nft info are the same
+      const nftsWithDifferentMetadata: NftUpdate[] = [];
+      const { allNfts } = this.state;
+      const stateNfts = allNfts[userAddress]?.[chainId] || [];
+
+      nftMetadataResults.forEach((singleNft) => {
+        const existingEntry: Nft | undefined = stateNfts.find(
+          (nft) =>
+            nft.address.toLowerCase() === singleNft.nft.address.toLowerCase() &&
+            nft.tokenId === singleNft.nft.tokenId,
         );
 
-        if (differentMetadata) {
-          nftsWithDifferentMetadata.push(singleNft);
+        if (existingEntry) {
+          const differentMetadata = compareNftMetadata(
+            singleNft.newMetadata,
+            existingEntry,
+          );
+
+          if (differentMetadata) {
+            nftsWithDifferentMetadata.push(singleNft);
+          }
         }
-      }
-    });
+      });
 
-    if (nftsWithDifferentMetadata.length !== 0) {
-      nftsWithDifferentMetadata.forEach((elm) =>
-        this.updateNft(elm.nft, elm.newMetadata, userAddress, chainId),
-      );
+      if (nftsWithDifferentMetadata.length !== 0) {
+        nftsWithDifferentMetadata.forEach((elm) =>
+          this.updateNft(elm.nft, elm.newMetadata, userAddress, chainId),
+        );
+      }
+    } finally {
+      releaseLock();
     }
   }