From 644c90c78f15a2e60e87117922f0048178cb0d0e Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Wed, 25 Sep 2024 09:42:28 +0200 Subject: [PATCH] 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 | 4 +- .../UserStorageController.test.ts | 8 ++- .../user-storage/UserStorageController.ts | 56 ++++++++++++++----- .../user-storage/__fixtures__/mockAccounts.ts | 39 ++++++++++++- .../controllers/user-storage/utils.test.ts | 21 ++++++- .../src/controllers/user-storage/utils.ts | 31 ++++++++++ 6 files changed, 140 insertions(+), 19 deletions(-) diff --git a/packages/profile-sync-controller/package.json b/packages/profile-sync-controller/package.json index e5aa848564..682a93eb8b 100644 --- a/packages/profile-sync-controller/package.json +++ b/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", diff --git a/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.test.ts b/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.test.ts index 4dfdb2ccf4..97357da011 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.test.ts +++ b/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() diff --git a/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.ts b/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.ts index 35a2c79459..5cb48e784b 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.ts +++ b/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 => { - 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 { - if (!this.#accounts.canSync()) { + if ( + !this.#accounts.canSync() || + !isEvmAccountType(internalAccount.type) || + !this.#accounts.doesInternalAccountHaveCorrectKeyringType(internalAccount) + ) { return; } diff --git a/packages/profile-sync-controller/src/controllers/user-storage/__fixtures__/mockAccounts.ts b/packages/profile-sync-controller/src/controllers/user-storage/__fixtures__/mockAccounts.ts index 2a50ce0bdb..1c4623b2d0 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/__fixtures__/mockAccounts.ts +++ b/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,9 +29,13 @@ export const MOCK_INTERNAL_ACCOUNTS = { { address: '0x123', id: '1', + type: EthAccountType.Eoa, metadata: { name: 'test', nameLastUpdatedAt: 1, + keyring: { + type: KeyringTypes.hd, + }, }, }, ], @@ -38,9 +43,13 @@ export const MOCK_INTERNAL_ACCOUNTS = { { address: '0x123', id: '1', + type: EthAccountType.Eoa, metadata: { name: `${getMockRandomDefaultAccountName()} 1`, nameLastUpdatedAt: 1, + keyring: { + type: KeyringTypes.hd, + }, }, }, ], @@ -48,8 +57,12 @@ export const MOCK_INTERNAL_ACCOUNTS = { { address: '0x123', id: '1', + type: EthAccountType.Eoa, metadata: { name: 'Internal account custom name without nameLastUpdatedAt', + keyring: { + type: KeyringTypes.hd, + }, }, }, ], @@ -57,9 +70,13 @@ export const MOCK_INTERNAL_ACCOUNTS = { { address: '0x123', id: '1', + type: EthAccountType.Eoa, metadata: { name: 'Internal account custom name with nameLastUpdatedAt', nameLastUpdatedAt: 1, + keyring: { + type: KeyringTypes.hd, + }, }, }, ], @@ -67,9 +84,13 @@ export const MOCK_INTERNAL_ACCOUNTS = { { address: '0x123', id: '1', + type: EthAccountType.Eoa, metadata: { name: 'Internal account custom name with nameLastUpdatedAt', nameLastUpdatedAt: 9999, + keyring: { + type: KeyringTypes.hd, + }, }, }, ], @@ -77,33 +98,49 @@ export const MOCK_INTERNAL_ACCOUNTS = { { 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, + }, }, }, ], 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 index cecdf729f2..fcd569739f 100644 --- 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 } 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'); + }); +}); diff --git a/packages/profile-sync-controller/src/controllers/user-storage/utils.ts b/packages/profile-sync-controller/src/controllers/user-storage/utils.ts index e57e317b63..ea3d44273f 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/utils.ts +++ b/packages/profile-sync-controller/src/controllers/user-storage/utils.ts @@ -26,3 +26,34 @@ export function setIntersection( 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( + getter: () => TVariable, + expectedValue: TVariable, + timeout = 1000, +): Promise { + 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); + }); +}