perf(multichain-account-service)!: avoid excessive state syncing

packages/multichain-account-service/CHANGELOG.md

@@ -9,6 +9,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Added
 
+- **BREAKING** A performance refactor was made around all the classes in this package ([#6654](https://github.com/MetaMask/core/pull/6654))
+  - Add logic in the `createMultichainAccountWallet` method in `MultichainAccountService` so that it can handle all entry points: importing an SRP, recovering a vault and creating a new vault.
+  - Add a `getAccountIds` method which returns all the account ids pertaining to a group.
+  - Add an `addAccounts` method on the `BaseBip44AccountProvider` class which keeps track of all the account IDs that pertain to it.
 - Wait for Snap platform to be ready before any wallet/group operations ([#7266](https://github.com/MetaMask/core/pull/7266))
 - Add `SnapAccountProvider.withSnap` protected helper ([#7266](https://github.com/MetaMask/core/pull/7266))
   - This is used to protect any Snap operation behind a guard that checks if the Snap platform is ready.
@@ -106,8 +110,22 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Changed
 
+- **BREAKING** A performance refactor was made around all the classes in this package ([#6654](https://github.com/MetaMask/core/pull/6654))
+  - The `MultichainAccountService` is refactored to construct a top level service state for its `init` function, this state is passed down to the `MultichainAccountWallet` and `MultichainAccountGroup` classes in slices for them to construct their internal states.
+  - Additional state is generated at the entry points where it needs to be updated i.e. `createMultichainAccountGroup`, `discoverAccounts` and `alignAccounts`.
+  - We no longer prevent group creation if some providers' `createAccounts` calls fail during group creation, only if they all fail.
+  - The `getAccounts` method in the `BaseBip44AccountProvider` class no longer relies on fetching the entire list of internal accounts from the `AccountsController`, instead it gets the specific accounts that it stores in its internal accounts list.
+  - The `EvmAccountProvider` no longer fetches from the `AccountController` to get an account for its ID, we deterministically get the associated account ID through `getUUIDFromAddressOfNormalAccount`.
+  - The `EvmAccountProvider` now uses the `getAccount` method from the `AccountsController` when fetching an account after account creation as it is more efficient.
 - Bump `@metamask/base-controller` from `^8.4.1` to `^8.4.2` ([#6917](https://github.com/MetaMask/core/pull/6917))
 
+### Removed
+
+- **BREAKING** A performance refactor was made around all the classes in this package ([#6654](https://github.com/MetaMask/core/pull/6654))
+  - Remove `#handleOnAccountAdded` and `#handleOnAccountRemoved` methods in `MultichainAccountService` due to internal state being updated within the service.
+  - Remove `getAccountContext` (and associated map) in the `MultichainAccountService` as the service no longer uses that method.
+  - Remove the `sync` method in favor of the sole `init` method for both `MultichainAccountWallet` and `MultichainAccountGroup`.
+
 ## [1.6.1]
 
 ### Changed

packages/multichain-account-service/src/MultichainAccountGroup.test.ts

@@ -1,26 +1,29 @@
 import type { Bip44Account } from '@metamask/account-api';
 import {
   AccountGroupType,
+  isBip44Account,
   toMultichainAccountGroupId,
   toMultichainAccountWalletId,
 } from '@metamask/account-api';
 import { EthScope, SolScope } from '@metamask/keyring-api';
 import type { InternalAccount } from '@metamask/keyring-internal-api';
 
+import type { GroupState } from './MultichainAccountGroup';
 import { MultichainAccountGroup } from './MultichainAccountGroup';
 import { MultichainAccountWallet } from './MultichainAccountWallet';
+import type { RootMessenger, MockAccountProvider } from './tests';
 import {
   MOCK_SNAP_ACCOUNT_2,
   MOCK_WALLET_1_BTC_P2TR_ACCOUNT,
   MOCK_WALLET_1_BTC_P2WPKH_ACCOUNT,
   MOCK_WALLET_1_ENTROPY_SOURCE,
   MOCK_WALLET_1_EVM_ACCOUNT,
   MOCK_WALLET_1_SOL_ACCOUNT,
-  setupNamedAccountProvider,
+  setupBip44AccountProvider,
   getMultichainAccountServiceMessenger,
   getRootMessenger,
+  mockCreateAccountsOnce,
 } from './tests';
-import type { MockAccountProvider, RootMessenger } from './tests';
 import type { MultichainAccountServiceMessenger } from './types';
 
 function setup({
@@ -45,8 +48,11 @@ function setup({
   providers: MockAccountProvider[];
   messenger: MultichainAccountServiceMessenger;
 } {
-  const providers = accounts.map((providerAccounts) => {
-    return setupNamedAccountProvider({ accounts: providerAccounts });
+  const providers = accounts.map((providerAccounts, idx) => {
+    return setupBip44AccountProvider({
+      name: `Provider ${idx + 1}`,
+      accounts: providerAccounts,
+    });
   });
 
   const serviceMessenger = getMultichainAccountServiceMessenger(messenger);
@@ -64,6 +70,18 @@ function setup({
     messenger: serviceMessenger,
   });
 
+  // Initialize group state from provided accounts so that constructor tests
+  // observe accounts immediately
+  const groupState = providers.reduce<GroupState>((state, provider, idx) => {
+    const ids = accounts[idx].filter(isBip44Account).map((a) => a.id);
+    if (ids.length > 0) {
+      state[provider.getName()] = ids;
+    }
+    return state;
+  }, {});
+
+  group.init(groupState);
+
   return { wallet, group, providers, messenger: serviceMessenger };
 }
 
@@ -88,6 +106,10 @@ describe('MultichainAccount', () => {
       expect(group.type).toBe(AccountGroupType.MultichainAccount);
       expect(group.groupIndex).toBe(groupIndex);
       expect(group.wallet).toStrictEqual(wallet);
+      expect(group.hasAccounts()).toBe(true);
+      expect(group.getAccountIds()).toStrictEqual(
+        expectedAccounts.map((a) => a.id),
+      );
       expect(group.getAccounts()).toHaveLength(expectedAccounts.length);
       expect(group.getAccounts()).toStrictEqual(expectedAccounts);
     });
@@ -160,62 +182,118 @@ describe('MultichainAccount', () => {
   });
 
   describe('alignAccounts', () => {
-    it('creates missing accounts only for providers with no accounts', async () => {
+    it('aligns accounts for all providers', async () => {
       const groupIndex = 0;
       const { group, providers, wallet } = setup({
         groupIndex,
-        accounts: [
-          [MOCK_WALLET_1_EVM_ACCOUNT], // provider[0] already has group 0
-          [], // provider[1] missing group 0
-        ],
+        accounts: [[MOCK_WALLET_1_EVM_ACCOUNT], []],
       });
 
+      mockCreateAccountsOnce(providers[0], [MOCK_WALLET_1_EVM_ACCOUNT]);
+      mockCreateAccountsOnce(providers[1], [MOCK_WALLET_1_SOL_ACCOUNT]);
+
       await group.alignAccounts();
 
-      expect(providers[0].createAccounts).not.toHaveBeenCalled();
+      expect(providers[0].createAccounts).toHaveBeenCalledWith({
+        entropySource: wallet.entropySource,
+        groupIndex,
+      });
       expect(providers[1].createAccounts).toHaveBeenCalledWith({
         entropySource: wallet.entropySource,
         groupIndex,
       });
+
+      expect(group.getAccounts()).toHaveLength(2);
+      expect(group.getAccounts()).toStrictEqual([
+        MOCK_WALLET_1_EVM_ACCOUNT,
+        MOCK_WALLET_1_SOL_ACCOUNT,
+      ]);
     });
 
-    it('does nothing when already aligned', async () => {
+    it('warns if alignment fails for a single provider', async () => {
       const groupIndex = 0;
-      const { group, providers } = setup({
+      const { group, providers, wallet } = setup({
         groupIndex,
-        accounts: [[MOCK_WALLET_1_EVM_ACCOUNT], [MOCK_WALLET_1_SOL_ACCOUNT]],
+        accounts: [[MOCK_WALLET_1_EVM_ACCOUNT], []],
       });
 
+      const consoleSpy = jest.spyOn(console, 'warn').mockImplementation();
+      providers[1].createAccounts.mockRejectedValueOnce(
+        new Error('Unable to create accounts'),
+      );
+
       await group.alignAccounts();
 
-      expect(providers[0].createAccounts).not.toHaveBeenCalled();
-      expect(providers[1].createAccounts).not.toHaveBeenCalled();
+      expect(providers[1].createAccounts).toHaveBeenCalledWith({
+        entropySource: wallet.entropySource,
+        groupIndex,
+      });
+      expect(consoleSpy).toHaveBeenCalledWith(
+        `Failed to fully align multichain account group for entropy ID: ${wallet.entropySource} and group index: ${groupIndex}, some accounts might be missing. Provider threw the following error:\n- Provider 2: Unable to create accounts`,
+      );
     });
 
-    it('warns if provider alignment fails', async () => {
+    it('warns if alignment fails for multiple providers', async () => {
       const groupIndex = 0;
       const { group, providers, wallet } = setup({
         groupIndex,
-        accounts: [[MOCK_WALLET_1_EVM_ACCOUNT], []],
+        accounts: [[MOCK_WALLET_1_EVM_ACCOUNT], [], []],
       });
 
       const consoleSpy = jest.spyOn(console, 'warn').mockImplementation();
       providers[1].createAccounts.mockRejectedValueOnce(
         new Error('Unable to create accounts'),
       );
 
+      providers[2].createAccounts.mockRejectedValueOnce(
+        new Error('Unable to create accounts'),
+      );
+
       await group.alignAccounts();
 
-      expect(providers[0].createAccounts).not.toHaveBeenCalled();
       expect(providers[1].createAccounts).toHaveBeenCalledWith({
         entropySource: wallet.entropySource,
         groupIndex,
       });
+      expect(providers[2].createAccounts).toHaveBeenCalledWith({
+        entropySource: wallet.entropySource,
+        groupIndex,
+      });
       expect(consoleSpy).toHaveBeenCalledWith(
-        `Failed to fully align multichain account group for entropy ID: ${wallet.entropySource} and group index: ${groupIndex}, some accounts might be missing`,
+        `Failed to fully align multichain account group for entropy ID: ${wallet.entropySource} and group index: ${groupIndex}, some accounts might be missing. Providers threw the following errors:\n- Provider 2: Unable to create accounts\n- Provider 3: Unable to create accounts`,
       );
     });
 
+    it('does not create accounts when a provider is disabled', async () => {
+      const groupIndex = 0;
+      const { group, providers } = setup({
+        groupIndex,
+        accounts: [[], []],
+      });
+
+      providers[1].setEnabled(false);
+      await group.alignAccounts();
+
+      expect(providers[0].createAccounts).toHaveBeenCalled();
+      expect(providers[1].createAccounts).not.toHaveBeenCalled();
+    });
+
+    it('removes accounts from the group when a provider is disabled', async () => {
+      const groupIndex = 0;
+      const { group, providers } = setup({
+        groupIndex,
+        accounts: [[MOCK_WALLET_1_EVM_ACCOUNT], []],
+      });
+
+      providers[0].setEnabled(false);
+      await group.alignAccounts();
+
+      expect(providers[0].createAccounts).not.toHaveBeenCalled();
+      expect(providers[1].createAccounts).toHaveBeenCalled();
+
+      expect(group.getAccount(MOCK_WALLET_1_EVM_ACCOUNT.id)).toBeUndefined();
+    });
+
     it('captures an error when a provider fails to create its account', async () => {
       const groupIndex = 0;
       const { group, providers, messenger } = setup({
@@ -227,7 +305,7 @@ describe('MultichainAccount', () => {
       const captureExceptionSpy = jest.spyOn(messenger, 'captureException');
       await group.alignAccounts();
       expect(captureExceptionSpy).toHaveBeenCalledWith(
-        new Error('Unable to align accounts with provider "Mocked Provider"'),
+        new Error('Unable to align accounts with provider "Provider 2"'),
       );
       expect(captureExceptionSpy.mock.lastCall[0]).toHaveProperty(
         'cause',