fix(NOTIFY-1171): fix account syncing performance (#4746)

## Explanation This PR solves various performance issues with user storage encryption / decryption + account syncing specific bugs. More info in the changelog. ## References - https://consensyssoftware.atlassian.net/browse/NOTIFY-1171 - https://consensyssoftware.atlassian.net/browse/NOTIFY-1177 ## Changelog ### `@metamask/profile-sync-controller` - **CHANGED**: batch GET / PUT now encrypts and decrypts sequentially in order to leverage cache - **FIXED**: don't save `nameLastUpdatedAt` to user storage if account name is a default name - **REMOVED**: remove `randomBytes` from the encryption mechanism - **REMOVED**: remove `waitForExpectedValue` and stop waiting for `AccountsController:accountAdded` callback ## 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
MetaMask · Oct 1, 2024 · 991cace · 991cace
1 parent 8d61ac9
commit 991cace
Show file tree

Hide file tree

Showing 7 changed files with 36 additions and 92 deletions.
diff --git a/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.ts b/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.ts
@@ -49,7 +49,6 @@ import {
   getUserStorageAllFeatureEntries,
   upsertUserStorage,
 } from './services';
-import { waitForExpectedValue } from './utils';
 
 // TODO: add external NetworkController event
 // Need to listen for when a network gets added
@@ -320,6 +319,7 @@ export default class UserStorageController extends BaseController<
       );
     },
     getInternalAccountsList: async (): Promise<InternalAccount[]> => {
+      // eslint-disable-next-line @typescript-eslint/await-thenable
       const internalAccountsList = await this.messagingSystem.call(
         'AccountsController:listAccounts',
       );
@@ -811,18 +811,9 @@ export default class UserStorageController extends BaseController<
 
         // Create new accounts to match the user storage accounts list
 
-        // eslint-disable-next-line @typescript-eslint/no-unused-vars
-        for await (const _ of Array.from({
-          length: numberOfAccountsToAdd,
-        })) {
-          const expectedAccountsCountAfterAddition =
-            this.#accounts.addedAccountsCount + 1;
+        for (let i = 0; i < numberOfAccountsToAdd; i++) {
           await this.messagingSystem.call('KeyringController:addNewAccount');
-          await waitForExpectedValue(
-            () => this.#accounts.addedAccountsCount,
-            expectedAccountsCountAfterAddition,
-            5000,
-          );
+
           this.#config?.accountSyncing?.onAccountAdded?.(profileId);
         }
       }
@@ -831,7 +822,7 @@ export default class UserStorageController extends BaseController<
       // Get the internal accounts list again since new accounts might have been added in the previous step
       internalAccountsList = await this.#accounts.getInternalAccountsList();
 
-      for await (const internalAccount of internalAccountsList) {
+      for (const internalAccount of internalAccountsList) {
         const userStorageAccount = userStorageAccountsList.find(
           (account) => account.a === internalAccount.address,
         );

diff --git a/packages/profile-sync-controller/src/controllers/user-storage/accounts/user-storage.ts b/packages/profile-sync-controller/src/controllers/user-storage/accounts/user-storage.ts
@@ -45,11 +45,13 @@ export const mapInternalAccountToUserStorageAccount = (
   internalAccount: InternalAccount,
 ): UserStorageAccount => {
   const { address, id, metadata } = internalAccount;
+  const { name, nameLastUpdatedAt } = metadata;
+
   return {
     [USER_STORAGE_VERSION_KEY]: USER_STORAGE_VERSION,
     a: address,
     i: id,
-    n: metadata.name,
-    nlu: metadata.nameLastUpdatedAt,
+    n: name,
+    ...(isNameDefaultAccountName(name) ? {} : { nlu: nameLastUpdatedAt }),
   };
 };
diff --git a/packages/profile-sync-controller/src/controllers/user-storage/services.ts b/packages/profile-sync-controller/src/controllers/user-storage/services.ts
@@ -136,21 +136,26 @@ export async function getUserStorageAllFeatureEntries(
       return null;
     }
 
-    const decryptedData = userStorage?.flatMap((entry) => {
+    const decryptedData: string[] = [];
+
+    for (const entry of userStorage) {
       if (!entry.Data) {
-        return [];
+        continue;
       }
 
-      return encryption.decryptString(
-        entry.Data,
-        opts.storageKey,
-        nativeScryptCrypto,
-      );
-    });
+      try {
+        const data = await encryption.decryptString(
+          entry.Data,
+          opts.storageKey,
+          nativeScryptCrypto,
+        );
+        decryptedData.push(data);
+      } catch {
+        // do nothing
+      }
+    }
 
-    return (await Promise.allSettled(decryptedData))
-      .map((d) => (d.status === 'fulfilled' ? d.value : undefined))
-      .filter((d): d is string => d !== undefined);
+    return decryptedData;
   } catch (e) {
     log.error('Failed to get user storage', e);
     return null;
@@ -208,18 +213,14 @@ export async function batchUpsertUserStorage(
 
   const { bearerToken, path, storageKey, nativeScryptCrypto } = opts;
 
-  const encryptedData = await Promise.all(
-    data.map(async (d) => {
-      return [
-        createSHA256Hash(d[0] + storageKey),
-        await encryption.encryptString(
-          d[1],
-          opts.storageKey,
-          nativeScryptCrypto,
-        ),
-      ];
-    }),
-  );
+  const encryptedData: string[][] = [];
+
+  for (const d of data) {
+    encryptedData.push([
+      createSHA256Hash(d[0] + storageKey),
+      await encryption.encryptString(d[1], opts.storageKey, nativeScryptCrypto),
+    ]);
+  }
 
   const url = new URL(`${USER_STORAGE_ENDPOINT}/${path}`);
 

diff --git a/packages/profile-sync-controller/src/controllers/user-storage/utils.test.ts b/packages/profile-sync-controller/src/controllers/user-storage/utils.test.ts
@@ -1,4 +1,4 @@
-import { setDifference, setIntersection, waitForExpectedValue } from './utils';
+import { setDifference, setIntersection } from './utils';
 
 describe('utils - setDifference()', () => {
   it('should return the difference between 2 sets', () => {
@@ -27,22 +27,3 @@ 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');
-  });
-});
diff --git a/packages/profile-sync-controller/src/controllers/user-storage/utils.ts b/packages/profile-sync-controller/src/controllers/user-storage/utils.ts
@@ -26,34 +26,3 @@ 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);
-  });
-}
diff --git a/packages/profile-sync-controller/src/shared/encryption/cache.ts b/packages/profile-sync-controller/src/shared/encryption/cache.ts
@@ -6,8 +6,8 @@ type CachedEntry = {
   key: Uint8Array;
 };
 
-const MAX_PASSWORD_CACHES = 3;
-const MAX_SALT_CACHES = 10;
+const MAX_PASSWORD_CACHES = 100;
+const MAX_SALT_CACHES = 100;
 
 /**
  * In-Memory Caching derived keys based from a given salt and password.

diff --git a/packages/profile-sync-controller/src/shared/encryption/encryption.ts b/packages/profile-sync-controller/src/shared/encryption/encryption.ts
@@ -100,7 +100,7 @@ class EncryptorDecryptor {
         p: SCRYPT_p,
         dkLen: ALGORITHM_KEY_SIZE,
       },
-      randomBytes(SCRYPT_SALT_SIZE),
+      undefined,
       nativeScryptCrypto,
     );