improv: account syncing performance (#4726)

## Explanation

This PR adds multiple improvements for the account syncing process,
namely:

- check if the account is of EVM type before saving it to user storage
- check if the account keyring type is either HD or simple
- wait for `AccountsController:accountAdded` event to fire before adding
another account

## References


[NOTIFY-1158](https://consensyssoftware.atlassian.net/jira/software/projects/NOTIFY/boards/616?assignee=712020%3A5843b7e2-a7fe-4c45-9fbd-e1f2b2eb58c2&selectedIssue=NOTIFY-1158)

## Changelog

### `@metamask/profile-sync-controller`

- **FIXED**: various account syncing performance improvements and bug
fixes.
- check if `isEvmAccountType` before saving an account in user storage
in account syncing
- check for correct `KeyringType` before saving an account in user
storage in account syncing
- wait for `AccountsController:accountAdded` event to fire before adding
another account in account syncing

## Checklist

- [x] I've updated the test suite for new or updated code as appropriate
- [x] I've updated documentation (JSDoc, Markdown, etc.) for new or
updated code as appropriate
- [x] I've highlighted breaking changes using the "BREAKING" category
above as appropriate
- [x] I've prepared draft pull requests for clients and consumer
packages to resolve any breaking changes


[NOTIFY-1158]:
https://consensyssoftware.atlassian.net/browse/NOTIFY-1158?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ

packages/profile-sync-controller/package.json

@@ -101,6 +101,8 @@
   },
   "dependencies": {
     "@metamask/base-controller": "^7.0.1",
+    "@metamask/keyring-api": "^8.1.2",
+    "@metamask/keyring-controller": "^17.2.1",
     "@metamask/snaps-sdk": "^6.5.0",
     "@metamask/snaps-utils": "^8.1.1",
     "@noble/ciphers": "^0.5.2",
@@ -113,8 +115,6 @@
     "@lavamoat/allow-scripts": "^3.0.4",
     "@metamask/accounts-controller": "^18.2.1",
     "@metamask/auto-changelog": "^3.4.4",
-    "@metamask/keyring-api": "^8.1.2",
-    "@metamask/keyring-controller": "^17.2.1",
     "@metamask/network-controller": "^21.0.1",
     "@metamask/snaps-controllers": "^9.7.0",
     "@types/jest": "^27.4.1",

packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.test.ts

@@ -1433,7 +1433,13 @@ function mockUserStorageMessenger(options?: {
       NotificationServicesControllerDisableNotificationServices['handler']
     >().mockResolvedValue();
 
-  const mockKeyringAddNewAccount = jest.fn().mockResolvedValue('0x123');
+  const mockKeyringAddNewAccount = jest.fn(() => {
+    baseMessenger.publish(
+      'AccountsController:accountAdded',
+      MOCK_INTERNAL_ACCOUNTS.ONE[0] as InternalAccount,
+    );
+    return MOCK_INTERNAL_ACCOUNTS.ONE[0].address;
+  });
 
   const mockAccountsListAccounts = jest
     .fn()

packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.ts

@@ -11,12 +11,13 @@ import type {
   StateMetadata,
 } from '@metamask/base-controller';
 import { BaseController } from '@metamask/base-controller';
-import type { InternalAccount } from '@metamask/keyring-api';
-import type {
-  KeyringControllerGetStateAction,
-  KeyringControllerLockEvent,
-  KeyringControllerUnlockEvent,
-  KeyringControllerAddNewAccountAction,
+import { type InternalAccount, isEvmAccountType } from '@metamask/keyring-api';
+import {
+  type KeyringControllerGetStateAction,
+  type KeyringControllerLockEvent,
+  type KeyringControllerUnlockEvent,
+  type KeyringControllerAddNewAccountAction,
+  KeyringTypes,
 } from '@metamask/keyring-controller';
 import type { NetworkConfiguration } from '@metamask/network-controller';
 import type { HandleSnapRequest } from '@metamask/snaps-controllers';
@@ -48,6 +49,7 @@ import {
   getUserStorageAllFeatureEntries,
   upsertUserStorage,
 } from './services';
+import { waitForExpectedValue } from './utils';
 
 // TODO: add external NetworkController event
 // Need to listen for when a network gets added
@@ -274,6 +276,7 @@ export default class UserStorageController extends BaseController<
     // We will remove this once the feature will be released
     isAccountSyncingEnabled: false,
     isAccountSyncingInProgress: false,
+    addedAccountsCount: 0,
     canSync: () => {
       try {
         this.#assertProfileSyncingEnabled();
@@ -291,8 +294,10 @@ export default class UserStorageController extends BaseController<
         // eslint-disable-next-line @typescript-eslint/no-misused-promises
         async (account) => {
           if (this.#accounts.isAccountSyncingInProgress) {
+            this.#accounts.addedAccountsCount += 1;
             return;
           }
+
           await this.saveInternalAccountToUserStorage(account);
         },
       );
@@ -308,8 +313,20 @@ export default class UserStorageController extends BaseController<
         },
       );
     },
+    doesInternalAccountHaveCorrectKeyringType: (account: InternalAccount) => {
+      return (
+        account.metadata.keyring.type === KeyringTypes.hd ||
+        account.metadata.keyring.type === KeyringTypes.simple
+      );
+    },
     getInternalAccountsList: async (): Promise<InternalAccount[]> => {
-      return this.messagingSystem.call('AccountsController:listAccounts');
+      const internalAccountsList = await this.messagingSystem.call(
+        'AccountsController:listAccounts',
+      );
+
+      return internalAccountsList?.filter(
+        this.#accounts.doesInternalAccountHaveCorrectKeyringType,
+      );
     },
     getUserStorageAccountsList: async (): Promise<
       UserStorageAccount[] | null
@@ -643,7 +660,6 @@ export default class UserStorageController extends BaseController<
    * @param values - data to store, in the form of an array of `[entryKey, entryValue]` pairs
    * @returns nothing. NOTE that an error is thrown if fails to store data.
    */
-
   public async performBatchSetStorage(
     path: UserStoragePathWithFeatureOnly,
     values: [UserStoragePathWithKeyOnly, string][],
@@ -761,6 +777,7 @@ export default class UserStorageController extends BaseController<
 
     try {
       this.#accounts.isAccountSyncingInProgress = true;
+      this.#accounts.addedAccountsCount = 0;
 
       const profileId = await this.#auth.getProfileId();
 
@@ -793,14 +810,21 @@ export default class UserStorageController extends BaseController<
           userStorageAccountsList.length - internalAccountsList.length;
 
         // Create new accounts to match the user storage accounts list
-        const addNewAccountsPromises = Array.from({
+
+        // eslint-disable-next-line @typescript-eslint/no-unused-vars
+        for await (const _ of Array.from({
           length: numberOfAccountsToAdd,
-        }).map(async () => {
+        })) {
+          const expectedAccountsCountAfterAddition =
+            this.#accounts.addedAccountsCount + 1;
           await this.messagingSystem.call('KeyringController:addNewAccount');
+          await waitForExpectedValue(
+            () => this.#accounts.addedAccountsCount,
+            expectedAccountsCountAfterAddition,
+            5000,
+          );
           this.#config?.accountSyncing?.onAccountAdded?.(profileId);
-        });
-
-        await Promise.all(addNewAccountsPromises);
+        }
       }
 
       // Second step: compare account names
@@ -906,7 +930,11 @@ export default class UserStorageController extends BaseController<
   async saveInternalAccountToUserStorage(
     internalAccount: InternalAccount,
   ): Promise<void> {
-    if (!this.#accounts.canSync()) {
+    if (
+      !this.#accounts.canSync() ||
+      !isEvmAccountType(internalAccount.type) ||
+      !this.#accounts.doesInternalAccountHaveCorrectKeyringType(internalAccount)
+    ) {
       return;
     }
 

packages/profile-sync-controller/src/controllers/user-storage/__fixtures__/mockAccounts.ts

@@ -1,4 +1,5 @@
-import type { InternalAccount } from '@metamask/keyring-api';
+import { EthAccountType, type InternalAccount } from '@metamask/keyring-api';
+import { KeyringTypes } from '@metamask/keyring-controller';
 
 import { LOCALIZED_DEFAULT_ACCOUNT_NAMES } from '../accounts/constants';
 import { mapInternalAccountToUserStorageAccount } from '../accounts/user-storage';
@@ -28,82 +29,118 @@ export const MOCK_INTERNAL_ACCOUNTS = {
     {
       address: '0x123',
       id: '1',
+      type: EthAccountType.Eoa,
       metadata: {
         name: 'test',
         nameLastUpdatedAt: 1,
+        keyring: {
+          type: KeyringTypes.hd,
+        },
       },
     },
   ],
   ONE_DEFAULT_NAME: [
     {
       address: '0x123',
       id: '1',
+      type: EthAccountType.Eoa,
       metadata: {
         name: `${getMockRandomDefaultAccountName()} 1`,
         nameLastUpdatedAt: 1,
+        keyring: {
+          type: KeyringTypes.hd,
+        },
       },
     },
   ],
   ONE_CUSTOM_NAME_WITHOUT_LAST_UPDATED: [
     {
       address: '0x123',
       id: '1',
+      type: EthAccountType.Eoa,
       metadata: {
         name: 'Internal account custom name without nameLastUpdatedAt',
+        keyring: {
+          type: KeyringTypes.hd,
+        },
       },
     },
   ],
   ONE_CUSTOM_NAME_WITH_LAST_UPDATED: [
     {
       address: '0x123',
       id: '1',
+      type: EthAccountType.Eoa,
       metadata: {
         name: 'Internal account custom name with nameLastUpdatedAt',
         nameLastUpdatedAt: 1,
+        keyring: {
+          type: KeyringTypes.hd,
+        },
       },
     },
   ],
   ONE_CUSTOM_NAME_WITH_LAST_UPDATED_MOST_RECENT: [
     {
       address: '0x123',
       id: '1',
+      type: EthAccountType.Eoa,
       metadata: {
         name: 'Internal account custom name with nameLastUpdatedAt',
         nameLastUpdatedAt: 9999,
+        keyring: {
+          type: KeyringTypes.hd,
+        },
       },
     },
   ],
   ALL: [
     {
       address: '0x123',
       id: '1',
+      type: EthAccountType.Eoa,
       metadata: {
         name: 'test',
         nameLastUpdatedAt: 1,
+        keyring: {
+          type: KeyringTypes.hd,
+        },
       },
     },
     {
       address: '0x456',
       id: '2',
+      type: EthAccountType.Eoa,
       metadata: {
         name: 'Account 2',
         nameLastUpdatedAt: 2,
+        keyring: {
+          type: KeyringTypes.hd,
+        },
       },
     },
     {
       address: '0x789',
       id: '3',
+      type: EthAccountType.Eoa,
       metadata: {
         name: 'Účet 2',
         nameLastUpdatedAt: 3,
+        keyring: {
+          type: KeyringTypes.hd,
+        },
       },
     },
     {
       address: '0xabc',
       id: '4',
+      type: EthAccountType.Eoa,
       metadata: {
         name: 'My Account 4',
         nameLastUpdatedAt: 4,
+        keyring: {
+          type: KeyringTypes.hd,
+        },
       },
     },
   ],

packages/profile-sync-controller/src/controllers/user-storage/utils.test.ts

@@ -1,4 +1,4 @@
-import { setDifference, setIntersection } from './utils';
+import { setDifference, setIntersection, waitForExpectedValue } from './utils';
 
 describe('utils - setDifference()', () => {
   it('should return the difference between 2 sets', () => {
@@ -27,3 +27,22 @@ describe('utils - setIntersection()', () => {
     expect(inBothSetsWithParamsReversed).toStrictEqual([3]);
   });
 });
+
+describe('utils - waitForExpectedValue()', () => {
+  it('should resolve when the expected value is returned', async () => {
+    const expectedValue = 'expected value';
+    const getter = jest.fn().mockReturnValue(expectedValue);
+
+    const value = await waitForExpectedValue(getter, expectedValue);
+    expect(value).toBe(expectedValue);
+  });
+
+  it('should reject when the timeout is reached', async () => {
+    const expectedValue = 'expected value';
+    const getter = jest.fn().mockReturnValue('wrong value');
+
+    await expect(
+      waitForExpectedValue(getter, expectedValue, 100),
+    ).rejects.toThrow('Timed out waiting for expected value');
+  });
+});

packages/profile-sync-controller/src/controllers/user-storage/utils.ts

@@ -26,3 +26,34 @@ export function setIntersection<TItem>(
   a.forEach((e) => b.has(e) && intersection.add(e));
   return intersection;
 }
+
+/**
+ *
+ * Waits for a value to be returned from a getter function.
+ *
+ * @param getter - Function that returns the value to check
+ * @param expectedValue - The value to wait for
+ * @param timeout - The time to wait before timing out
+ * @returns A promise that resolves when the expected value is returned
+ * or rejects if the timeout is reached.
+ */
+export function waitForExpectedValue<TVariable>(
+  getter: () => TVariable,
+  expectedValue: TVariable,
+  timeout = 1000,
+): Promise<TVariable> {
+  return new Promise((resolve, reject) => {
+    const interval = setInterval(() => {
+      const value = getter();
+      if (value === expectedValue) {
+        clearInterval(interval);
+        resolve(value);
+      }
+    }, 100);
+
+    setTimeout(() => {
+      clearInterval(interval);
+      reject(new Error('Timed out waiting for expected value'));
+    }, timeout);
+  });
+}