Fix issue where mutex is acquired after state is read in addToken resulting in overwritten state with batched addToken/watchAsset calls (#1768)

Fixes a subtle bug: when multiple `wallet_watchAsset` calls of ERC20
tokens are batched, only the last of the tokens added was getting added
because the calls were not properly single threaded, since the mutex was
acquired after reading state in the `addToken` call.

## TokensController
### Fixes
- Fixes bug where batched addToken calls overwrite each other because
mutex is acquired after reading state

Author: adonesky1

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

@@ -1,4 +1,8 @@
-import type { AddApprovalRequest } from '@metamask/approval-controller';
+import {
+  ApprovalController,
+  type AddApprovalRequest,
+  type ApprovalControllerEvents,
+} from '@metamask/approval-controller';
 import { ControllerMessenger } from '@metamask/base-controller';
 import contractMaps from '@metamask/contract-metadata';
 import {
@@ -60,8 +64,25 @@ describe('TokensController', () => {
   let preferences: PreferencesController;
   const messenger = new ControllerMessenger<
     ApprovalActions,
+    ApprovalControllerEvents
+  >();
+
+  const approvalControllerMessenger = messenger.getRestricted({
+    name: 'ApprovalController',
+    allowedActions: ['ApprovalController:addRequest'],
+  });
+
+  const approvalController = new ApprovalController({
+    messenger: approvalControllerMessenger,
+    showApprovalRequest: jest.fn(),
+    typesExcludedFromRateLimiting: [ApprovalType.WatchAsset],
+  });
+
+  const tokensControllerMessenger = messenger.getRestricted<
+    typeof controllerName,
+    ApprovalActions['type'],
     never
-  >().getRestricted<typeof controllerName, ApprovalActions['type'], never>({
+  >({
     name: controllerName,
     allowedActions: ['ApprovalController:addRequest'],
   }) as TokensControllerMessenger;
@@ -93,7 +114,7 @@ describe('TokensController', () => {
       },
       getERC20TokenName: sinon.stub(),
       getNetworkClientById: sinon.stub() as any,
-      messenger,
+      messenger: tokensControllerMessenger,
     });
   });
 
@@ -1094,6 +1115,7 @@ describe('TokensController', () => {
         image: 'image',
         name: undefined,
       };
+
       createEthersStub = stubCreateEthers(tokensController, false);
     });
 
@@ -1365,6 +1387,62 @@ describe('TokensController', () => {
 
       generateRandomIdStub.mockRestore();
     });
+
+    it('stores multiple tokens from a batched watchAsset confirmation screen correctly when user confirms', async function () {
+      const generateRandomIdStub = jest
+        .spyOn(tokensController, '_generateRandomId')
+        .mockImplementationOnce(() => requestId)
+        .mockImplementationOnce(() => '67890');
+
+      const acceptedRequest = new Promise<void>((resolve) => {
+        tokensController.subscribe((state) => {
+          if (
+            state.allTokens?.[ChainId.mainnet]?.[interactingAddress].length ===
+            2
+          ) {
+            resolve();
+          }
+        });
+      });
+
+      const anotherAsset = {
+        address: '0x000000000000000000000000000000000000ABcD',
+        decimals: 18,
+        symbol: 'TEST',
+        image: 'image2',
+        name: undefined,
+      };
+
+      tokensController.watchAsset({ asset, type, interactingAddress });
+      tokensController.watchAsset({
+        asset: anotherAsset,
+        type,
+        interactingAddress,
+      });
+
+      await approvalController.accept(requestId);
+      await approvalController.accept('67890');
+      await acceptedRequest;
+
+      expect(
+        tokensController.state.allTokens[ChainId.mainnet][interactingAddress],
+      ).toHaveLength(2);
+      expect(
+        tokensController.state.allTokens[ChainId.mainnet][interactingAddress],
+      ).toStrictEqual([
+        {
+          isERC721: false,
+          aggregators: [],
+          ...asset,
+        },
+        {
+          isERC721: false,
+          aggregators: [],
+          ...anotherAsset,
+        },
+      ]);
+      generateRandomIdStub.mockRestore();
+    });
   });
 
   describe('onPreferencesStateChange', function () {

packages/assets-controllers/src/TokensController.ts

@@ -299,8 +299,9 @@ export class TokensController extends BaseController<
     interactingAddress?: string;
     networkClientId?: NetworkClientId;
   }): Promise<Token[]> {
-    const { allTokens, allIgnoredTokens, allDetectedTokens } = this.state;
     const { chainId, selectedAddress } = this.config;
+    const releaseLock = await this.mutex.acquire();
+    const { allTokens, allIgnoredTokens, allDetectedTokens } = this.state;
     let currentChainId = chainId;
     if (networkClientId) {
       currentChainId =
@@ -309,7 +310,6 @@ export class TokensController extends BaseController<
 
     const accountAddress = interactingAddress || selectedAddress;
     const isInteractingWithWalletAccount = accountAddress === selectedAddress;
-    const releaseLock = await this.mutex.acquire();
 
     try {
       address = toChecksumHexAddress(address);
@@ -321,8 +321,10 @@ export class TokensController extends BaseController<
       const newTokens: Token[] = [...tokens];
       const [isERC721, tokenMetadata] = await Promise.all([
         this._detectIsERC721(address, networkClientId),
+        // TODO parameterize the token metadata fetch by networkClientId
         this.fetchTokenMetadata(address),
       ]);
+      // TODO remove this once this method is fully parameterized by networkClientId
       if (!networkClientId && currentChainId !== this.config.chainId) {
         throw new Error(
           'TokensController Error: Switched networks while adding token',