diff --git a/packages/multichain-account-service/CHANGELOG.md b/packages/multichain-account-service/CHANGELOG.md index f1f369f2c31..b40b795147c 100644 --- a/packages/multichain-account-service/CHANGELOG.md +++ b/packages/multichain-account-service/CHANGELOG.md @@ -7,6 +7,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### 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. + ## [3.0.0] ### Added @@ -53,8 +60,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 diff --git a/packages/multichain-account-service/src/MultichainAccountGroup.test.ts b/packages/multichain-account-service/src/MultichainAccountGroup.test.ts index 7777b72e7d3..7d70c62d710 100644 --- a/packages/multichain-account-service/src/MultichainAccountGroup.test.ts +++ b/packages/multichain-account-service/src/MultichainAccountGroup.test.ts @@ -8,7 +8,10 @@ import { import { EthScope, SolScope } from '@metamask/keyring-api'; import type { InternalAccount } from '@metamask/keyring-internal-api'; -import { MultichainAccountGroup } from './MultichainAccountGroup'; +import { + type GroupState, + MultichainAccountGroup, +} from './MultichainAccountGroup'; import { MultichainAccountWallet } from './MultichainAccountWallet'; import type { MockAccountProvider } from './tests'; import { @@ -18,9 +21,10 @@ import { MOCK_WALLET_1_ENTROPY_SOURCE, MOCK_WALLET_1_EVM_ACCOUNT, MOCK_WALLET_1_SOL_ACCOUNT, - setupNamedAccountProvider, + setupBip44AccountProvider, getMultichainAccountServiceMessenger, getRootMessenger, + mockCreateAccountsOnce, type RootMessenger, } from './tests'; import type { MultichainAccountServiceMessenger } from './types'; @@ -47,8 +51,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); @@ -70,6 +77,20 @@ function setup({ messenger: serviceMessenger, }); + // Initialize group state from provided accounts so that constructor tests + // observe accounts immediately + const groupState = providers.reduce((state, provider, idx) => { + const ids = accounts[idx] + .filter((a) => 'options' in a && a.options?.entropy) + .map((a) => a.id); + if (ids.length > 0) { + state[provider.getName()] = ids; + } + return state; + }, {}); + + group.init(groupState); + return { wallet, group, providers, messenger: serviceMessenger }; } @@ -94,6 +115,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); }); @@ -166,43 +191,62 @@ 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(); @@ -210,18 +254,55 @@ describe('MultichainAccount', () => { 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({ @@ -234,7 +315,7 @@ describe('MultichainAccount', () => { await group.alignAccounts(); expect(callSpy).toHaveBeenCalledWith( 'ErrorReportingService:captureException', - new Error('Unable to align accounts with provider "Mocked Provider"'), + new Error('Unable to align accounts with provider "Provider 2"'), ); expect(callSpy.mock.lastCall[1]).toHaveProperty('cause', providerError); }); diff --git a/packages/multichain-account-service/src/MultichainAccountGroup.ts b/packages/multichain-account-service/src/MultichainAccountGroup.ts index bc3163a1f67..725fef7f965 100644 --- a/packages/multichain-account-service/src/MultichainAccountGroup.ts +++ b/packages/multichain-account-service/src/MultichainAccountGroup.ts @@ -14,11 +14,15 @@ import { createModuleLogger, WARNING_PREFIX, } from './logger'; +import type { ServiceState, StateKeys } from './MultichainAccountService'; import type { MultichainAccountWallet } from './MultichainAccountWallet'; import type { Bip44AccountProvider } from './providers'; import type { MultichainAccountServiceMessenger } from './types'; import { createSentryError } from './utils'; +export type GroupState = + ServiceState[StateKeys['entropySource']][StateKeys['groupIndex']]; + /** * A multichain account group that holds multiple accounts. */ @@ -48,7 +52,6 @@ export class MultichainAccountGroup< readonly #log: Logger; - // eslint-disable-next-line @typescript-eslint/prefer-readonly #initialized = false; constructor({ @@ -71,52 +74,51 @@ export class MultichainAccountGroup< this.#accountToProvider = new Map(); this.#log = createModuleLogger(log, `[${this.#id}]`); - - this.sync(); - this.#initialized = true; } /** - * Force multichain account synchronization. + * Initialize the multichain account group and construct the internal representation of accounts. * - * This can be used if account providers got new accounts that the multichain - * account doesn't know about. + * Note: This method can be called multiple times to update the group state. + * + * @param groupState - The group state. + * @param update - Whether the call is a update operation. */ - sync(): void { - this.#log('Synchronizing with account providers...'); - // Clear reverse mapping and re-construct it entirely based on the refreshed - // list of accounts from each providers. - this.#accountToProvider.clear(); - + init(groupState: GroupState, update = false) { + this.#log('Initializing group state...'); for (const provider of this.#providers) { - // Filter account only for that index. - const accounts = []; - for (const account of provider.getAccounts()) { - if ( - account.options.entropy.id === this.wallet.entropySource && - account.options.entropy.groupIndex === this.groupIndex - ) { - // We only use IDs to always fetch the latest version of accounts. - accounts.push(account.id); - } - } - this.#providerToAccounts.set(provider, accounts); + const accountIds = groupState[provider.getName()]; - // Reverse-mapping for fast indexing. - for (const id of accounts) { - this.#accountToProvider.set(id, provider); + if (accountIds) { + this.#providerToAccounts.set(provider, accountIds); + + for (const accountId of accountIds) { + this.#accountToProvider.set(accountId, provider); + } } } - // Emit update event when group is synced (only if initialized) - if (this.#initialized) { + if (!this.#initialized) { + this.#initialized = true; + } else { this.#messenger.publish( 'MultichainAccountService:multichainAccountGroupUpdated', this, ); } - this.#log('Synchronized'); + update + ? this.#log('Finished updating group state...') + : this.#log('Finished initializing group state...'); + } + + /** + * Update the group state. + * + * @param groupState - The group state. + */ + update(groupState: GroupState) { + this.init(groupState, true); } /** @@ -189,6 +191,15 @@ export class MultichainAccountGroup< return allAccounts; } + /** + * Gets the account IDs for this multichain account. + * + * @returns The account IDs. + */ + getAccountIds(): Account['id'][] { + return [...this.#providerToAccounts.values()].flat(); + } + /** * Gets the account for a given account ID. * @@ -203,6 +214,7 @@ export class MultichainAccountGroup< if (!provider) { return undefined; } + return provider.getAccount(id); } @@ -235,23 +247,32 @@ export class MultichainAccountGroup< async alignAccounts(): Promise { this.#log('Aligning accounts...'); + this.#providerToAccounts.clear(); + this.#accountToProvider.clear(); + const results = await Promise.allSettled( this.#providers.map(async (provider) => { try { - const accounts = this.#providerToAccounts.get(provider); - if (!accounts || accounts.length === 0) { + const [disabled, accounts] = await provider.alignAccounts({ + entropySource: this.wallet.entropySource, + groupIndex: this.groupIndex, + }); + + if (disabled) { + this.#log( + `Account provider "${provider.getName()}" is disabled, skipping alignment...`, + ); + } else if (accounts.length > 0) { this.#log( `Found missing accounts for account provider "${provider.getName()}", creating them now...`, ); - const created = await provider.createAccounts({ - entropySource: this.wallet.entropySource, - groupIndex: this.groupIndex, - }); - this.#log(`Created ${created.length} accounts`); - - return created; + this.#providerToAccounts.set(provider, accounts); + for (const accountId of accounts) { + this.#accountToProvider.set(accountId, provider); + } } - return Promise.resolve(); + + return accounts; } catch (error) { // istanbul ignore next this.#log( @@ -274,8 +295,24 @@ export class MultichainAccountGroup< }), ); - if (results.some((result) => result.status === 'rejected')) { - const message = `Failed to fully align multichain account group for entropy ID: ${this.wallet.entropySource} and group index: ${this.groupIndex}, some accounts might be missing`; + let failureMessage = ''; + let failureCount = 0; + const groupState = results.reduce((state, result, idx) => { + if (result.status === 'fulfilled' && result.value.length) { + state[this.#providers[idx].getName()] = result.value; + } else if (result.status === 'rejected') { + failureCount += 1; + failureMessage += `\n- ${this.#providers[idx].getName()}: ${result.reason.message}`; + } + return state; + }, {}); + + // Update group state + this.update(groupState); + + if (failureCount > 0) { + const hasMultipleFailures = failureCount > 1; + const message = `Failed to fully align multichain account group for entropy ID: ${this.wallet.entropySource} and group index: ${this.groupIndex}, some accounts might be missing. ${hasMultipleFailures ? 'Providers' : 'Provider'} threw the following ${hasMultipleFailures ? 'errors' : 'error'}:${failureMessage}`; this.#log(`${WARNING_PREFIX} ${message}`); console.warn(message); diff --git a/packages/multichain-account-service/src/MultichainAccountService.test.ts b/packages/multichain-account-service/src/MultichainAccountService.test.ts index 9894408b3ca..2b04771389c 100644 --- a/packages/multichain-account-service/src/MultichainAccountService.test.ts +++ b/packages/multichain-account-service/src/MultichainAccountService.test.ts @@ -4,7 +4,8 @@ import { isBip44Account } from '@metamask/account-api'; import { mnemonicPhraseToBytes } from '@metamask/key-tree'; import type { KeyringAccount } from '@metamask/keyring-api'; import { EthAccountType, SolAccountType } from '@metamask/keyring-api'; -import { KeyringTypes, type KeyringObject } from '@metamask/keyring-controller'; +import { type KeyringObject } from '@metamask/keyring-controller'; +import type { EthKeyring } from '@metamask/keyring-internal-api'; import type { MultichainAccountServiceOptions } from './MultichainAccountService'; import { MultichainAccountService } from './MultichainAccountService'; @@ -35,8 +36,7 @@ import { getMultichainAccountServiceMessenger, getRootMessenger, makeMockAccountProvider, - mockAsInternalAccount, - setupNamedAccountProvider, + setupBip44AccountProvider, type RootMessenger, } from './tests'; import type { MultichainAccountServiceMessenger } from './types'; @@ -61,10 +61,16 @@ type Mocks = { getState: jest.Mock; getKeyringsByType: jest.Mock; addNewKeyring: jest.Mock; + createNewVaultAndKeychain: jest.Mock; + createNewVaultAndRestore: jest.Mock; + withKeyring: jest.Mock; }; AccountsController: { listMultichainAccounts: jest.Mock; }; + ErrorReportingService: { + captureException: jest.Mock; + }; EvmAccountProvider: MockAccountProvider; SolAccountProvider: MockAccountProvider; }; @@ -73,18 +79,31 @@ function mockAccountProvider( providerClass: new (messenger: MultichainAccountServiceMessenger) => Provider, mocks: MockAccountProvider, accounts: KeyringAccount[], - type: KeyringAccount['type'], + idx: number, ) { jest.mocked(providerClass).mockImplementation((...args) => { mocks.constructor(...args); return mocks as unknown as Provider; }); - setupNamedAccountProvider({ + setupBip44AccountProvider({ mocks, accounts, - filter: (account) => account.type === type, + index: idx, }); + + // Provide stable provider name and compatibility logic for grouping + if (providerClass === (EvmAccountProvider as unknown)) { + mocks.getName.mockReturnValue(EVM_ACCOUNT_PROVIDER_NAME); + mocks.isAccountCompatible?.mockImplementation( + (account: KeyringAccount) => account.type === EthAccountType.Eoa, + ); + } else if (providerClass === (SolAccountProvider as unknown)) { + mocks.getName.mockReturnValue(SOL_ACCOUNT_PROVIDER_NAME); + mocks.isAccountCompatible?.mockImplementation( + (account: KeyringAccount) => account.type === SolAccountType.DataAccount, + ); + } } async function setup({ @@ -109,10 +128,16 @@ async function setup({ getState: jest.fn(), getKeyringsByType: jest.fn(), addNewKeyring: jest.fn(), + createNewVaultAndKeychain: jest.fn(), + createNewVaultAndRestore: jest.fn(), + withKeyring: jest.fn(), }, AccountsController: { listMultichainAccounts: jest.fn(), }, + ErrorReportingService: { + captureException: jest.fn(), + }, EvmAccountProvider: makeMockAccountProvider(), SolAccountProvider: makeMockAccountProvider(), }; @@ -140,6 +165,21 @@ async function setup({ mocks.KeyringController.addNewKeyring, ); + rootMessenger.registerActionHandler( + 'KeyringController:createNewVaultAndKeychain', + mocks.KeyringController.createNewVaultAndKeychain, + ); + + rootMessenger.registerActionHandler( + 'KeyringController:createNewVaultAndRestore', + mocks.KeyringController.createNewVaultAndRestore, + ); + + rootMessenger.registerActionHandler( + 'ErrorReportingService:captureException', + mocks.ErrorReportingService.captureException, + ); + if (accounts) { mocks.AccountsController.listMultichainAccounts.mockImplementation( () => accounts, @@ -159,13 +199,13 @@ async function setup({ EvmAccountProvider, mocks.EvmAccountProvider, accounts, - EthAccountType.Eoa, + 0, ); mockAccountProvider( SolAccountProvider, mocks.SolAccountProvider, accounts, - SolAccountType.DataAccount, + 1, ); } @@ -404,288 +444,7 @@ describe('MultichainAccountService', () => { }); }); - describe('getAccountContext', () => { - const entropy1 = 'entropy-1'; - const entropy2 = 'entropy-2'; - - const account1 = MockAccountBuilder.from(MOCK_HD_ACCOUNT_1) - .withId('mock-id-1') - .withEntropySource(entropy1) - .withGroupIndex(0) - .get(); - const account2 = MockAccountBuilder.from(MOCK_HD_ACCOUNT_1) - .withId('mock-id-2') - .withEntropySource(entropy1) - .withGroupIndex(1) - .get(); - const account3 = MockAccountBuilder.from(MOCK_HD_ACCOUNT_1) - .withId('mock-id-3') - .withEntropySource(entropy2) - .withGroupIndex(0) - .get(); - - const keyring1 = { - type: KeyringTypes.hd, - accounts: [account1.address, account2.address], - metadata: { id: entropy1, name: '' }, - }; - const keyring2 = { - type: KeyringTypes.hd, - accounts: [account2.address], - metadata: { id: entropy2, name: '' }, - }; - - const keyrings: KeyringObject[] = [keyring1, keyring2]; - - it('gets the wallet and multichain account for a given account ID', async () => { - const accounts = [account1, account2, account3]; - const { service } = await setup({ accounts, keyrings }); - - const wallet1 = service.getMultichainAccountWallet({ - entropySource: entropy1, - }); - const wallet2 = service.getMultichainAccountWallet({ - entropySource: entropy2, - }); - - const [multichainAccount1, multichainAccount2] = - wallet1.getMultichainAccountGroups(); - const [multichainAccount3] = wallet2.getMultichainAccountGroups(); - - const walletAndMultichainAccount1 = service.getAccountContext( - account1.id, - ); - const walletAndMultichainAccount2 = service.getAccountContext( - account2.id, - ); - const walletAndMultichainAccount3 = service.getAccountContext( - account3.id, - ); - - // NOTE: We use `toBe` here, cause we want to make sure we use the same - // references with `get*` service's methods. - expect(walletAndMultichainAccount1?.wallet).toBe(wallet1); - expect(walletAndMultichainAccount1?.group).toBe(multichainAccount1); - - expect(walletAndMultichainAccount2?.wallet).toBe(wallet1); - expect(walletAndMultichainAccount2?.group).toBe(multichainAccount2); - - expect(walletAndMultichainAccount3?.wallet).toBe(wallet2); - expect(walletAndMultichainAccount3?.group).toBe(multichainAccount3); - }); - - it('syncs the appropriate wallet and update reverse mapping on AccountsController:accountAdded', async () => { - const accounts = [account1, account3]; // No `account2` for now. - const { service, rootMessenger, mocks } = await setup({ - accounts, - keyrings, - }); - - const wallet1 = service.getMultichainAccountWallet({ - entropySource: entropy1, - }); - expect(wallet1.getMultichainAccountGroups()).toHaveLength(1); - - // Now we're adding `account2`. - mocks.EvmAccountProvider.accounts = [account1, account2]; - rootMessenger.publish( - 'AccountsController:accountAdded', - mockAsInternalAccount(account2), - ); - expect(wallet1.getMultichainAccountGroups()).toHaveLength(2); - - const [multichainAccount1, multichainAccount2] = - wallet1.getMultichainAccountGroups(); - - const walletAndMultichainAccount1 = service.getAccountContext( - account1.id, - ); - const walletAndMultichainAccount2 = service.getAccountContext( - account2.id, - ); - - // NOTE: We use `toBe` here, cause we want to make sure we use the same - // references with `get*` service's methods. - expect(walletAndMultichainAccount1?.wallet).toBe(wallet1); - expect(walletAndMultichainAccount1?.group).toBe(multichainAccount1); - - expect(walletAndMultichainAccount2?.wallet).toBe(wallet1); - expect(walletAndMultichainAccount2?.group).toBe(multichainAccount2); - }); - - it('syncs the appropriate multichain account and update reverse mapping on AccountsController:accountAdded', async () => { - const otherAccount1 = MockAccountBuilder.from(account2) - .withGroupIndex(0) - .get(); - - const accounts = [account1]; // No `otherAccount1` for now. - const { service, rootMessenger, mocks } = await setup({ - accounts, - keyrings, - }); - - const wallet1 = service.getMultichainAccountWallet({ - entropySource: entropy1, - }); - expect(wallet1.getMultichainAccountGroups()).toHaveLength(1); - - // Now we're adding `account2`. - mocks.EvmAccountProvider.accounts = [account1, otherAccount1]; - rootMessenger.publish( - 'AccountsController:accountAdded', - mockAsInternalAccount(otherAccount1), - ); - // Still 1, that's the same multichain account, but a new "blockchain - // account" got added. - expect(wallet1.getMultichainAccountGroups()).toHaveLength(1); - - const [multichainAccount1] = wallet1.getMultichainAccountGroups(); - - const walletAndMultichainAccount1 = service.getAccountContext( - account1.id, - ); - const walletAndMultichainOtherAccount1 = service.getAccountContext( - otherAccount1.id, - ); - - // NOTE: We use `toBe` here, cause we want to make sure we use the same - // references with `get*` service's methods. - expect(walletAndMultichainAccount1?.wallet).toBe(wallet1); - expect(walletAndMultichainAccount1?.group).toBe(multichainAccount1); - - expect(walletAndMultichainOtherAccount1?.wallet).toBe(wallet1); - expect(walletAndMultichainOtherAccount1?.group).toBe(multichainAccount1); - }); - - it('emits multichainAccountGroupUpdated event when syncing existing group on account added', async () => { - const otherAccount1 = MockAccountBuilder.from(account2) - .withGroupIndex(0) - .get(); - - const accounts = [account1]; // No `otherAccount1` for now. - const { rootMessenger, messenger, mocks } = await setup({ - accounts, - keyrings, - }); - const publishSpy = jest.spyOn(messenger, 'publish'); - - // Now we're adding `otherAccount1` to an existing group. - mocks.EvmAccountProvider.accounts = [account1, otherAccount1]; - rootMessenger.publish( - 'AccountsController:accountAdded', - mockAsInternalAccount(otherAccount1), - ); - - // Should emit updated event for the existing group - expect(publishSpy).toHaveBeenCalled(); - expect(publishSpy).toHaveBeenCalledWith( - 'MultichainAccountService:multichainAccountGroupUpdated', - expect.objectContaining({ - groupIndex: 0, - }), - ); - - const emittedGroup = publishSpy.mock.calls[0][1]; - expect(emittedGroup).toBeDefined(); - expect(emittedGroup).toHaveProperty('groupIndex', 0); - expect(emittedGroup).toHaveProperty('getAccounts'); - expect(emittedGroup).toHaveProperty('select'); - expect(emittedGroup).toHaveProperty('get'); - }); - - it('creates new detected wallets and update reverse mapping on AccountsController:accountAdded', async () => { - const accounts = [account1, account2]; // No `account3` for now (associated with "Wallet 2"). - const { service, rootMessenger, mocks } = await setup({ - accounts, - keyrings: [keyring1], - }); - - const wallet1 = service.getMultichainAccountWallet({ - entropySource: entropy1, - }); - expect(wallet1.getMultichainAccountGroups()).toHaveLength(2); - - // No wallet 2 yet. - expect(() => - service.getMultichainAccountWallet({ entropySource: entropy2 }), - ).toThrow('Unknown wallet, no wallet matching this entropy source'); - - // Now we're adding `account3`. - mocks.KeyringController.keyrings = [keyring1, keyring2]; - mocks.EvmAccountProvider.accounts = [account1, account2, account3]; - rootMessenger.publish( - 'AccountsController:accountAdded', - mockAsInternalAccount(account3), - ); - const wallet2 = service.getMultichainAccountWallet({ - entropySource: entropy2, - }); - expect(wallet2).toBeDefined(); - expect(wallet2.getMultichainAccountGroups()).toHaveLength(1); - - const [multichainAccount3] = wallet2.getMultichainAccountGroups(); - - const walletAndMultichainAccount3 = service.getAccountContext( - account3.id, - ); - - // NOTE: We use `toBe` here, cause we want to make sure we use the same - // references with `get*` service's methods. - expect(walletAndMultichainAccount3?.wallet).toBe(wallet2); - expect(walletAndMultichainAccount3?.group).toBe(multichainAccount3); - }); - - it('ignores non-BIP-44 accounts on AccountsController:accountAdded', async () => { - const accounts = [account1]; - const { service, rootMessenger } = await setup({ - accounts, - keyrings, - }); - - const wallet1 = service.getMultichainAccountWallet({ - entropySource: entropy1, - }); - const oldMultichainAccounts = wallet1.getMultichainAccountGroups(); - expect(oldMultichainAccounts).toHaveLength(1); - expect(oldMultichainAccounts[0].getAccounts()).toHaveLength(1); - - // Now we're publishing a new account that is not BIP-44 compatible. - rootMessenger.publish( - 'AccountsController:accountAdded', - mockAsInternalAccount(MOCK_SNAP_ACCOUNT_2), - ); - - const newMultichainAccounts = wallet1.getMultichainAccountGroups(); - expect(newMultichainAccounts).toHaveLength(1); - expect(newMultichainAccounts[0].getAccounts()).toHaveLength(1); - }); - - it('syncs the appropriate wallet and update reverse mapping on AccountsController:accountRemoved', async () => { - const accounts = [account1, account2]; - const { service, rootMessenger, mocks } = await setup({ - accounts, - keyrings, - }); - - const wallet1 = service.getMultichainAccountWallet({ - entropySource: entropy1, - }); - expect(wallet1.getMultichainAccountGroups()).toHaveLength(2); - - // Now we're removing `account2`. - mocks.EvmAccountProvider.accounts = [account1]; - rootMessenger.publish('AccountsController:accountRemoved', account2.id); - expect(wallet1.getMultichainAccountGroups()).toHaveLength(1); - - const walletAndMultichainAccount2 = service.getAccountContext( - account2.id, - ); - - expect(walletAndMultichainAccount2).toBeUndefined(); - }); - }); - - describe('createNextMultichainAccount', () => { + describe('createNextMultichainAccountGroup', () => { it('creates the next multichain account group', async () => { const mockEvmAccount = MockAccountBuilder.from(MOCK_HD_ACCOUNT_1) .withEntropySource(MOCK_HD_KEYRING_1.metadata.id) @@ -828,7 +587,6 @@ describe('MultichainAccountService', () => { entropySource: MOCK_HD_KEYRING_1.metadata.id, groupIndex: 0, }); - expect(mocks.EvmAccountProvider.createAccounts).not.toHaveBeenCalled(); }); }); @@ -928,7 +686,6 @@ describe('MultichainAccountService', () => { entropySource: MOCK_HD_KEYRING_1.metadata.id, groupIndex: 0, }); - expect(mocks.EvmAccountProvider.createAccounts).not.toHaveBeenCalled(); }); it('aligns all multichain account wallets with MultichainAccountService:alignWallets', async () => { @@ -979,6 +736,8 @@ describe('MultichainAccountService', () => { it('creates a multichain account wallet with MultichainAccountService:createMultichainAccountWallet', async () => { const { messenger, mocks } = await setup({ accounts: [], keyrings: [] }); + const mnemonic = mnemonicPhraseToBytes(MOCK_MNEMONIC); + mocks.KeyringController.getKeyringsByType.mockImplementationOnce( () => [], ); @@ -990,7 +749,7 @@ describe('MultichainAccountService', () => { const wallet = await messenger.call( 'MultichainAccountService:createMultichainAccountWallet', - { mnemonic: MOCK_MNEMONIC }, + { mnemonic, type: 'import' }, ); expect(wallet).toBeDefined(); @@ -1051,22 +810,15 @@ describe('MultichainAccountService', () => { const providerError = new Error('Unable to resync accounts'); mocks.SolAccountProvider.resyncAccounts.mockRejectedValue(providerError); - const mockCaptureException = jest.fn(); - rootMessenger.registerActionHandler( - 'ErrorReportingService:captureException', - mockCaptureException, - ); - await service.resyncAccounts(); // Should not throw. expect(mocks.EvmAccountProvider.resyncAccounts).toHaveBeenCalled(); expect(mocks.SolAccountProvider.resyncAccounts).toHaveBeenCalled(); - expect(mockCaptureException).toHaveBeenCalled(); - expect(mockCaptureException.mock.lastCall[0]).toHaveProperty( - 'cause', - providerError, - ); + expect(mocks.ErrorReportingService.captureException).toHaveBeenCalled(); + expect( + mocks.ErrorReportingService.captureException.mock.lastCall[0], + ).toHaveProperty('cause', providerError); }); }); @@ -1209,48 +961,143 @@ describe('MultichainAccountService', () => { }); describe('createMultichainAccountWallet', () => { - it('creates a new multichain account wallet with the given mnemonic', async () => { - const { mocks, service } = await setup({ - accounts: [], - keyrings: [], + it('throws an error if the create wallet parameters are invalid', async () => { + const { service } = await setup({ accounts: [], keyrings: [] }); + await expect(() => + service.createMultichainAccountWallet({ + type: 'create', + }), + ).rejects.toThrow('Invalid create wallet parameters.'); + }); + + describe('createWalletByImport', () => { + it('creates a new multichain account wallet by the import flow', async () => { + const { mocks, service } = await setup({ + accounts: [], + keyrings: [], + }); + + const mnemonic = mnemonicPhraseToBytes(MOCK_MNEMONIC); + + mocks.KeyringController.getKeyringsByType.mockImplementationOnce(() => [ + {}, + ]); + + mocks.KeyringController.addNewKeyring.mockImplementationOnce(() => ({ + id: 'abc', + name: '', + })); + + const wallet = await service.createMultichainAccountWallet({ + mnemonic, + type: 'import', + }); + + expect(wallet).toBeDefined(); + expect(wallet.entropySource).toBe('abc'); }); - mocks.KeyringController.getKeyringsByType.mockImplementationOnce(() => [ - {}, - ]); + it("throws an error if there's already an existing keyring from the same mnemonic", async () => { + const { service, mocks } = await setup({ accounts: [], keyrings: [] }); - mocks.KeyringController.addNewKeyring.mockImplementationOnce(() => ({ - id: 'abc', - name: '', - })); + const mnemonic = mnemonicPhraseToBytes(MOCK_MNEMONIC); - const wallet = await service.createMultichainAccountWallet({ - mnemonic: MOCK_MNEMONIC, + mocks.KeyringController.getKeyringsByType.mockImplementationOnce(() => [ + { + mnemonic, + }, + ]); + + await expect( + service.createMultichainAccountWallet({ + mnemonic, + type: 'import', + }), + ).rejects.toThrow( + 'This Secret Recovery Phrase has already been imported.', + ); + + // Ensure we did not attempt to create a new keyring when duplicate is detected + expect(mocks.KeyringController.addNewKeyring).not.toHaveBeenCalled(); }); + }); - expect(wallet).toBeDefined(); - expect(wallet.entropySource).toBe('abc'); + describe('createWalletByNewVault', () => { + it('creates a new multichain account wallet by the new vault flow', async () => { + const { service, mocks, rootMessenger } = await setup({ + accounts: [], + keyrings: [], + }); + + const password = 'password'; + + mocks.KeyringController.createNewVaultAndKeychain.mockImplementationOnce( + () => { + mocks.KeyringController.keyrings.push(MOCK_HD_KEYRING_1); + }, + ); + + rootMessenger.registerActionHandler( + 'KeyringController:withKeyring', + async (_, operation) => { + const newKeyring = mocks.KeyringController.keyrings.find( + (keyring) => keyring.type === 'HD Key Tree', + ) as KeyringObject; + return operation({ + keyring: {} as unknown as EthKeyring, + metadata: newKeyring.metadata, + }); + }, + ); + + const newWallet = await service.createMultichainAccountWallet({ + password, + type: 'create', + }); + + expect(newWallet).toBeDefined(); + expect(newWallet.entropySource).toBe(MOCK_HD_KEYRING_1.metadata.id); + }); }); - it("throws an error if there's already an existing keyring from the same mnemonic", async () => { - const { service, mocks } = await setup({ accounts: [], keyrings: [] }); + describe('createWalletByRestore', () => { + it('creates a new multichain account wallet by the restore flow', async () => { + const { service, mocks, rootMessenger } = await setup({ + accounts: [], + keyrings: [], + }); - const mnemonic = mnemonicPhraseToBytes(MOCK_MNEMONIC); + const mnemonic = mnemonicPhraseToBytes(MOCK_MNEMONIC); + const password = 'password'; - mocks.KeyringController.getKeyringsByType.mockImplementationOnce(() => [ - { - mnemonic, - }, - ]); + mocks.KeyringController.createNewVaultAndRestore.mockImplementationOnce( + () => { + mocks.KeyringController.keyrings.push(MOCK_HD_KEYRING_1); + }, + ); + + rootMessenger.registerActionHandler( + 'KeyringController:withKeyring', + async (_, operation) => { + const newKeyring = mocks.KeyringController.keyrings.find( + (keyring) => keyring.type === 'HD Key Tree', + ) as KeyringObject; + return operation({ + keyring: {} as unknown as EthKeyring, + metadata: newKeyring.metadata, + }); + }, + ); - await expect( - service.createMultichainAccountWallet({ mnemonic: MOCK_MNEMONIC }), - ).rejects.toThrow( - 'This Secret Recovery Phrase has already been imported.', - ); + const newWallet = await service.createMultichainAccountWallet({ + password, + mnemonic, + type: 'restore', + }); - // Ensure we did not attempt to create a new keyring when duplicate is detected - expect(mocks.KeyringController.addNewKeyring).not.toHaveBeenCalled(); + expect(newWallet).toBeDefined(); + expect(newWallet.entropySource).toBe(MOCK_HD_KEYRING_1.metadata.id); + }); }); }); }); diff --git a/packages/multichain-account-service/src/MultichainAccountService.ts b/packages/multichain-account-service/src/MultichainAccountService.ts index d779736c75a..fbf1565f819 100644 --- a/packages/multichain-account-service/src/MultichainAccountService.ts +++ b/packages/multichain-account-service/src/MultichainAccountService.ts @@ -7,10 +7,10 @@ import type { Bip44Account, } from '@metamask/account-api'; import type { HdKeyring } from '@metamask/eth-hd-keyring'; -import { mnemonicPhraseToBytes } from '@metamask/key-tree'; import type { EntropySourceId, KeyringAccount } from '@metamask/keyring-api'; import { KeyringTypes } from '@metamask/keyring-controller'; -import { areUint8ArraysEqual } from '@metamask/utils'; +import type { InternalAccount } from '@metamask/keyring-internal-api'; +import { areUint8ArraysEqual, assert } from '@metamask/utils'; import { traceFallback } from './analytics'; import { projectLogger as log } from './logger'; @@ -50,12 +50,55 @@ export type MultichainAccountServiceOptions = { config?: MultichainAccountServiceConfig; }; -/** Reverse mapping object used to map account IDs and their wallet/multichain account. */ -type AccountContext> = { - wallet: MultichainAccountWallet; - group: MultichainAccountGroup; +/** + * The keys used to identify an account in the service state. + */ +export type StateKeys = { + entropySource: EntropySourceId; + groupIndex: number; + providerName: string; +}; + +/** + * The service state. + */ +export type ServiceState = { + [entropySource: StateKeys['entropySource']]: { + [groupIndex: string]: { + [ + providerName: StateKeys['providerName'] + ]: Bip44Account['id'][]; + }; + }; +}; + +export type CreateWalletType = 'restore' | 'import' | 'create'; + +type RestoreType = Extract; +type ImportType = Extract; +type CreateType = Extract; + +type CreateWalletParams = { + type: CreateWalletType; + password?: string; + mnemonic?: Uint8Array; }; +type CreateWalletValidatedParams = + | { + type: RestoreType; + password: string; + mnemonic: Uint8Array; + } + | { + type: ImportType; + mnemonic: Uint8Array; + } + | { + type: CreateType; + password: string; + }; + /** * Service to expose multichain accounts capabilities. */ @@ -69,11 +112,6 @@ export class MultichainAccountService { MultichainAccountWallet> >; - readonly #accountIdToContext: Map< - Bip44Account['id'], - AccountContext> - >; - /** * The name of the service. */ @@ -97,7 +135,6 @@ export class MultichainAccountService { }: MultichainAccountServiceOptions) { this.#messenger = messenger; this.#wallets = new Map(); - this.#accountIdToContext = new Map(); // Pass trace callback directly to preserve original 'this' context // This avoids binding the callback to the MultichainAccountService instance @@ -166,13 +203,66 @@ export class MultichainAccountService { 'MultichainAccountService:resyncAccounts', (...args) => this.resyncAccounts(...args), ); + } - this.#messenger.subscribe('AccountsController:accountAdded', (account) => - this.#handleOnAccountAdded(account), - ); - this.#messenger.subscribe('AccountsController:accountRemoved', (id) => - this.#handleOnAccountRemoved(id), + /** + * Get the keys used to identify an account in the service state. + * + * @param account - The account to get the keys for. + * @returns The keys used to identify an account in the service state. + * Returns null if the account is not compatible with any provider. + */ + #getStateKeys(account: InternalAccount): StateKeys | null { + for (const provider of this.#providers) { + if (isBip44Account(account) && provider.isAccountCompatible(account)) { + return { + entropySource: account.options.entropy.id, + groupIndex: account.options.entropy.groupIndex, + providerName: provider.getName(), + }; + } + } + return null; + } + + /** + * Construct the service and provider state. + * + * @returns The service and provider state. + */ + #constructServiceState() { + const accounts = this.#messenger.call( + 'AccountsController:listMultichainAccounts', ); + + const serviceState: ServiceState = {}; + const { keyrings } = this.#messenger.call('KeyringController:getState'); + + // We set up the wallet level keys for this constructed state object + for (const keyring of keyrings) { + if (keyring.type === (KeyringTypes.hd as string)) { + serviceState[keyring.metadata.id] = {}; + } + } + + const providerState = this.#providers.reduce< + Record['id'][]> + >((acc, provider) => { + acc[provider.getName()] = []; + return acc; + }, {}); + + for (const account of accounts) { + const keys = this.#getStateKeys(account); + if (keys) { + const { entropySource, groupIndex, providerName } = keys; + serviceState[entropySource][groupIndex] ??= {}; + serviceState[entropySource][groupIndex][providerName] ??= []; + serviceState[entropySource][groupIndex][providerName].push(account.id); + providerState[providerName].push(account.id); + } + } + return { serviceState, providerState }; } /** @@ -183,36 +273,24 @@ export class MultichainAccountService { log('Initializing...'); this.#wallets.clear(); - this.#accountIdToContext.clear(); - // Create initial wallets. - const { keyrings } = this.#messenger.call('KeyringController:getState'); - for (const keyring of keyrings) { - if (keyring.type === (KeyringTypes.hd as string)) { - // Only HD keyrings have an entropy source/SRP. - const entropySource = keyring.metadata.id; - - log(`Adding new wallet for entropy: "${entropySource}"`); - - // This will automatically "associate" all multichain accounts for that wallet - // (based on the accounts owned by each account providers). - const wallet = new MultichainAccountWallet({ - entropySource, - providers: this.#providers, - messenger: this.#messenger, - }); - this.#wallets.set(wallet.id, wallet); - - // Reverse mapping between account ID and their multichain wallet/account: - for (const group of wallet.getMultichainAccountGroups()) { - for (const account of group.getAccounts()) { - this.#accountIdToContext.set(account.id, { - wallet, - group, - }); - } - } - } + const { serviceState, providerState } = this.#constructServiceState(); + + // Add the accounts to the providers' internal list of account IDs + for (const [providerName, accountIds] of Object.entries(providerState)) { + this.#providers + .find((p) => p.getName() === providerName) + ?.init(accountIds); + } + + for (const entropySource of Object.keys(serviceState)) { + const wallet = new MultichainAccountWallet({ + entropySource, + providers: this.#providers, + messenger: this.#messenger, + }); + wallet.init(serviceState[entropySource]); + this.#wallets.set(wallet.id, wallet); } log('Initialized'); @@ -262,84 +340,13 @@ export class MultichainAccountService { log('Providers got re-synced!'); } - #handleOnAccountAdded(account: KeyringAccount): void { - // We completely omit non-BIP-44 accounts! - if (!isBip44Account(account)) { - return; - } - - let sync = true; - - let wallet = this.#wallets.get( - toMultichainAccountWalletId(account.options.entropy.id), - ); - if (!wallet) { - log( - `Adding new wallet for entropy: "${account.options.entropy.id}" (for account: "${account.id}")`, - ); - - // That's a new wallet. - wallet = new MultichainAccountWallet({ - entropySource: account.options.entropy.id, - providers: this.#providers, - messenger: this.#messenger, - }); - this.#wallets.set(wallet.id, wallet); - - // If that's a new wallet wallet. There's nothing to "force-sync". - sync = false; - } - - let group = wallet.getMultichainAccountGroup( - account.options.entropy.groupIndex, - ); - if (!group) { - // This new account is a new multichain account, let the wallet know - // it has to re-sync with its providers. - if (sync) { - wallet.sync(); - } - - group = wallet.getMultichainAccountGroup( - account.options.entropy.groupIndex, - ); - - // If that's a new multichain account. There's nothing to "force-sync". - sync = false; - } - - // We have to check against `undefined` in case `getMultichainAccount` is - // not able to find this multichain account (which should not be possible...) - if (group) { - if (sync) { - group.sync(); - } - - // Same here, this account should have been already grouped in that - // multichain account. - this.#accountIdToContext.set(account.id, { - wallet, - group, - }); - } - } - - #handleOnAccountRemoved(id: KeyringAccount['id']): void { - // Force sync of the appropriate wallet if an account got removed. - const found = this.#accountIdToContext.get(id); - if (found) { - const { wallet } = found; - - log( - `Re-synchronize wallet [${wallet.id}] since account "${id}" got removed`, - ); - wallet.sync(); - } - - // Safe to call delete even if the `id` was not referencing a BIP-44 account. - this.#accountIdToContext.delete(id); - } - + /** + * Get the wallet matching the given entropy source. + * + * @param entropySource - The entropy source of the wallet. + * @returns The wallet matching the given entropy source. + * @throws If no wallet matches the given entropy source. + */ #getWallet( entropySource: EntropySourceId, ): MultichainAccountWallet> { @@ -354,19 +361,6 @@ export class MultichainAccountService { return wallet; } - /** - * Gets the account's context which contains its multichain wallet and - * multichain account group references. - * - * @param id - Account ID. - * @returns The account context if any, undefined otherwise. - */ - getAccountContext( - id: KeyringAccount['id'], - ): AccountContext> | undefined { - return this.#accountIdToContext.get(id); - } - /** * Gets a reference to the multichain account wallet matching this entropy source. * @@ -395,52 +389,187 @@ export class MultichainAccountService { } /** - * Creates a new multichain account wallet with the given mnemonic. - * - * NOTE: This method should only be called in client code where a mutex lock is acquired. - * `discoverAndCreateAccounts` should be called after this method to discover and create accounts. + * Gets the validated create wallet parameters. * * @param options - Options. * @param options.mnemonic - The mnemonic to use to create the new wallet. - * @throws If the mnemonic has already been imported. - * @returns The new multichain account wallet. + * @param options.password - The password to encrypt the vault with. + * @param options.type - The flow type to use to create the new wallet. + * @returns The validated create wallet parameters. */ - async createMultichainAccountWallet({ + #getValidatedCreateWalletParams({ mnemonic, - }: { - mnemonic: string; - }): Promise>> { + password, + type, + }: CreateWalletParams): CreateWalletValidatedParams { + if (type === 'restore' && password && mnemonic) { + return { + password, + mnemonic, + type: 'restore', + }; + } + + if (type === 'import' && mnemonic) { + return { mnemonic, type: 'import' }; + } + + if (type === 'create' && password) { + return { password, type: 'create' }; + } + + throw new Error('Invalid create wallet parameters.'); + } + + /** + * Creates a new multichain account wallet by importing an existing mnemonic. + * + * @param mnemonic - The mnemonic to use to create the new wallet. + * @returns The new multichain account wallet. + */ + async #createWalletByImport( + mnemonic: Uint8Array, + ): Promise>> { + log(`Creating new wallet by importing an existing mnemonic...`); const existingKeyrings = this.#messenger.call( 'KeyringController:getKeyringsByType', KeyringTypes.hd, ) as HdKeyring[]; - const mnemonicAsBytes = mnemonicPhraseToBytes(mnemonic); - const alreadyHasImportedSrp = existingKeyrings.some((keyring) => { if (!keyring.mnemonic) { return false; } - return areUint8ArraysEqual(keyring.mnemonic, mnemonicAsBytes); + return areUint8ArraysEqual(keyring.mnemonic, mnemonic); }); if (alreadyHasImportedSrp) { throw new Error('This Secret Recovery Phrase has already been imported.'); } - log(`Creating new wallet...`); - const result = await this.#messenger.call( 'KeyringController:addNewKeyring', KeyringTypes.hd, { mnemonic }, ); - const wallet = new MultichainAccountWallet({ + // The wallet is ripe for discovery + return new MultichainAccountWallet({ providers: this.#providers, entropySource: result.id, messenger: this.#messenger, }); + } + + /** + * Creates a new multichain account wallet by creating a new vault and keychain. + * + * @param password - The password to encrypt the vault with. + * @returns The new multichain account wallet. + */ + async #createWalletByNewVault( + password: string, + ): Promise>> { + log(`Creating new wallet by creating a new vault and keychain...`); + await this.#messenger.call( + 'KeyringController:createNewVaultAndKeychain', + password, + ); + + const entropySourceId = (await this.#messenger.call( + 'KeyringController:withKeyring', + { type: KeyringTypes.hd }, + async ({ metadata }) => { + return metadata.id; + }, + )) as string; + + // The wallet is ripe for discovery + return new MultichainAccountWallet({ + providers: this.#providers, + entropySource: entropySourceId, + messenger: this.#messenger, + }); + } + + /** + * Creates a new multichain account wallet by restoring a vault and keyring. + * + * @param password - The password to encrypt the vault with. + * @param mnemonic - The mnemonic to use to restore the new wallet. + * @returns The new multichain account wallet. + */ + async #createWalletByRestore( + password: string, + mnemonic: Uint8Array, + ): Promise>> { + log(`Creating new wallet by restoring vault and keyring...`); + await this.#messenger.call( + 'KeyringController:createNewVaultAndRestore', + password, + mnemonic, + ); + + const entropySourceId = (await this.#messenger.call( + 'KeyringController:withKeyring', + { type: KeyringTypes.hd }, + async ({ metadata }) => { + return metadata.id; + }, + )) as string; + + // The wallet is ripe for discovery + return new MultichainAccountWallet({ + providers: this.#providers, + entropySource: entropySourceId, + messenger: this.#messenger, + }); + } + + /** + * Creates a new multichain account wallet by either importing an existing mnemonic, + * creating a new vault and keychain, or restoring a vault and keyring. + * + * NOTE: This method should only be called in client code where a mutex lock is acquired. + * `discoverAccounts` should be called after this method to discover and create accounts. + * + * @param options - Options. + * @param options.mnemonic - The mnemonic to use to create the new wallet. + * @param options.password - The password to encrypt the vault with. + * @param options.type - The flow type to use to create the new wallet. + * @throws If the mnemonic has already been imported. + * @returns The new multichain account wallet. + */ + async createMultichainAccountWallet({ + mnemonic, + password, + type, + }: CreateWalletParams): Promise< + MultichainAccountWallet> + > { + const params: CreateWalletValidatedParams = + this.#getValidatedCreateWalletParams({ + mnemonic, + password, + type, + }); + + let wallet: + | MultichainAccountWallet> + | undefined; + + if (params.type === 'import') { + wallet = await this.#createWalletByImport(params.mnemonic); + } else if (params.type === 'create') { + wallet = await this.#createWalletByNewVault(params.password); + } else if (params.type === 'restore') { + wallet = await this.#createWalletByRestore( + params.password, + params.mnemonic, + ); + } + + assert(wallet, 'Failed to create wallet.'); this.#wallets.set(wallet.id, wallet); diff --git a/packages/multichain-account-service/src/MultichainAccountWallet.test.ts b/packages/multichain-account-service/src/MultichainAccountWallet.test.ts index 38db8426ae3..fa7eda5d780 100644 --- a/packages/multichain-account-service/src/MultichainAccountWallet.test.ts +++ b/packages/multichain-account-service/src/MultichainAccountWallet.test.ts @@ -11,10 +11,14 @@ import { EthAccountType, SolAccountType, type EntropySourceId, + KeyringAccountEntropyTypeOption, } from '@metamask/keyring-api'; import type { InternalAccount } from '@metamask/keyring-internal-api'; -import { MultichainAccountWallet } from './MultichainAccountWallet'; +import { + type WalletState, + MultichainAccountWallet, +} from './MultichainAccountWallet'; import type { MockAccountProvider } from './tests'; import { MOCK_HD_ACCOUNT_1, @@ -27,7 +31,7 @@ import { MOCK_WALLET_1_EVM_ACCOUNT, MOCK_WALLET_1_SOL_ACCOUNT, MockAccountBuilder, - setupNamedAccountProvider, + setupBip44AccountProvider, getMultichainAccountServiceMessenger, getRootMessenger, type RootMessenger, @@ -57,13 +61,15 @@ function setup({ providers: MockAccountProvider[]; messenger: MultichainAccountServiceMessenger; } { - providers ??= accounts.map((providerAccounts, i) => { - return setupNamedAccountProvider({ - name: `Mocked Provider ${i}`, - accounts: providerAccounts, - index: i, + const providersList = + providers ?? + accounts.map((providerAccounts, i) => { + return setupBip44AccountProvider({ + name: `Mocked Provider ${i}`, + accounts: providerAccounts, + index: i, + }); }); - }); const serviceMessenger = getMultichainAccountServiceMessenger(messenger); @@ -74,11 +80,34 @@ function setup({ const wallet = new MultichainAccountWallet>({ entropySource, - providers, + providers: providersList, messenger: serviceMessenger, }); - return { wallet, providers, messenger: serviceMessenger }; + const walletState = accounts.reduce( + (state, providerAccounts, idx) => { + const providerName = providersList[idx].getName(); + for (const account of providerAccounts) { + if ( + 'options' in account && + account.options?.entropy?.type === + KeyringAccountEntropyTypeOption.Mnemonic + ) { + const groupIndexKey = account.options.entropy.groupIndex; + state[groupIndexKey] ??= {}; + const groupState = state[groupIndexKey]; + groupState[providerName] ??= []; + groupState[providerName].push(account.id); + } + } + return state; + }, + {}, + ); + + wallet.init(walletState); + + return { wallet, providers: providersList, messenger: serviceMessenger }; } describe('MultichainAccountWallet', () => { @@ -148,130 +177,42 @@ describe('MultichainAccountWallet', () => { }); }); - describe('sync', () => { - it('force sync wallet after account provider got new account', () => { - const mockEvmAccount = MOCK_WALLET_1_EVM_ACCOUNT; - const provider = setupNamedAccountProvider({ - accounts: [mockEvmAccount], - }); - const { wallet } = setup({ - providers: [provider], - }); - - expect(wallet.getMultichainAccountGroups()).toHaveLength(1); - expect(wallet.getAccountGroups()).toHaveLength(1); // We can still get "basic" groups too. - - // Add a new account for the next index. - provider.getAccounts.mockReturnValue([ - mockEvmAccount, - { - ...mockEvmAccount, - options: { - ...mockEvmAccount.options, - entropy: { - ...mockEvmAccount.options.entropy, - groupIndex: 1, - }, - }, - }, - ]); - - // Force sync, so the wallet will "find" a new multichain account. - wallet.sync(); - expect(wallet.getAccountGroups()).toHaveLength(2); - expect(wallet.getMultichainAccountGroups()).toHaveLength(2); - }); - - it('skips non-matching wallet during sync', () => { - const mockEvmAccount = MOCK_WALLET_1_EVM_ACCOUNT; - const provider = setupNamedAccountProvider({ - accounts: [mockEvmAccount], - }); - const { wallet } = setup({ - providers: [provider], - }); - - expect(wallet.getMultichainAccountGroups()).toHaveLength(1); - - // Add a new account for another index but not for this wallet. - provider.getAccounts.mockReturnValue([ - mockEvmAccount, - { - ...mockEvmAccount, - options: { - ...mockEvmAccount.options, - entropy: { - ...mockEvmAccount.options.entropy, - id: 'mock-unknown-entropy-id', - groupIndex: 1, - }, - }, - }, - ]); - - // Even if we have a new account, it's not for this wallet, so it should - // not create a new multichain account! - wallet.sync(); - expect(wallet.getMultichainAccountGroups()).toHaveLength(1); - }); - - it('cleans up old multichain account group during sync', () => { - const mockEvmAccount = MOCK_WALLET_1_EVM_ACCOUNT; - const provider = setupNamedAccountProvider({ - accounts: [mockEvmAccount], - }); - const { wallet } = setup({ - providers: [provider], - }); - - expect(wallet.getMultichainAccountGroups()).toHaveLength(1); - - // Account for index 0 got removed, thus, the multichain account for index 0 - // will also be removed. - provider.getAccounts.mockReturnValue([]); - - // We should not have any multichain account anymore. - wallet.sync(); - expect(wallet.getMultichainAccountGroups()).toHaveLength(0); - }); - }); - describe('createMultichainAccountGroup', () => { - it('creates a multichain account group for a given index', async () => { - const groupIndex = 1; - - const mockEvmAccount = MockAccountBuilder.from(MOCK_HD_ACCOUNT_1) - .withEntropySource(MOCK_HD_KEYRING_1.metadata.id) - .withGroupIndex(0) - .get(); + it('creates a multichain account group for a given index (waitForAllProvidersToFinishCreatingAccounts = false)', async () => { + const groupIndex = 0; const { wallet, providers } = setup({ - accounts: [[mockEvmAccount]], // 1 provider + accounts: [[], []], // 1 provider }); - const [provider] = providers; - const mockNextEvmAccount = MockAccountBuilder.from(mockEvmAccount) + const [evmProvider, solProvider] = providers; + const mockNextEvmAccount = MockAccountBuilder.from(MOCK_HD_ACCOUNT_1) .withEntropySource(MOCK_HD_KEYRING_1.metadata.id) .withGroupIndex(groupIndex) .get(); // 1. Create the accounts for the new index and returns their IDs. - provider.createAccounts.mockResolvedValueOnce([mockNextEvmAccount]); + evmProvider.createAccounts.mockResolvedValueOnce([mockNextEvmAccount]); // 2. When the wallet creates a new multichain account group, it will query // all accounts for this given index (so similar to the one we just created). - provider.getAccounts.mockReturnValueOnce([mockNextEvmAccount]); + evmProvider.getAccounts.mockReturnValueOnce([mockNextEvmAccount]); // 3. Required when we call `getAccounts` (below) on the multichain account. - provider.getAccount.mockReturnValueOnce(mockNextEvmAccount); + evmProvider.getAccount.mockReturnValueOnce(mockNextEvmAccount); + + solProvider.createAccounts.mockResolvedValueOnce([MOCK_SOL_ACCOUNT_1]); + solProvider.getAccounts.mockReturnValueOnce([MOCK_SOL_ACCOUNT_1]); + solProvider.getAccount.mockReturnValueOnce(MOCK_SOL_ACCOUNT_1); const specificGroup = await wallet.createMultichainAccountGroup(groupIndex); expect(specificGroup.groupIndex).toBe(groupIndex); const internalAccounts = specificGroup.getAccounts(); - expect(internalAccounts).toHaveLength(1); + expect(internalAccounts).toHaveLength(2); expect(internalAccounts[0].type).toBe(EthAccountType.Eoa); + expect(internalAccounts[1].type).toBe(SolAccountType.DataAccount); }); - it('returns the same reference when re-creating using the same index', async () => { + it('returns the same reference when re-creating using the same index (waitForAllProvidersToFinishCreatingAccounts = false)', async () => { const { wallet } = setup({ accounts: [[MOCK_HD_ACCOUNT_1]], }); @@ -282,7 +223,7 @@ describe('MultichainAccountWallet', () => { expect(newGroup).toBe(group); }); - it('fails to create an account beyond the next index', async () => { + it('fails to create an account beyond the next index (waitForAllProvidersToFinishCreatingAccounts = false)', async () => { const { wallet } = setup({ accounts: [[MOCK_HD_ACCOUNT_1]], }); @@ -295,79 +236,81 @@ describe('MultichainAccountWallet', () => { ); }); - it('fails to create an account group if the EVM provider fails to create its account', async () => { + it('creates an account group if only some of the providers fail to create its account (waitForAllProvidersToFinishCreatingAccounts = true)', async () => { const groupIndex = 1; + // Baseline accounts at index 0 for two providers const mockEvmAccount = MockAccountBuilder.from(MOCK_HD_ACCOUNT_1) .withEntropySource(MOCK_HD_KEYRING_1.metadata.id) .withGroupIndex(0) .get(); + const mockSolAccount = MockAccountBuilder.from(MOCK_SOL_ACCOUNT_1) + .withEntropySource(MOCK_HD_KEYRING_1.metadata.id) + .withGroupIndex(0) + .get(); const { wallet, providers } = setup({ - accounts: [[mockEvmAccount]], // 1 provider + accounts: [[mockEvmAccount], [mockSolAccount]], // 2 providers }); - const [provider] = providers; - provider.createAccounts.mockRejectedValueOnce( - new Error('Unable to create accounts'), - ); + const [succeedingProvider, failingProvider] = providers; - await expect( - wallet.createMultichainAccountGroup(groupIndex), - ).rejects.toThrow( - 'Unable to create multichain account group for index: 1 with provider "Mocked Provider 0"', + // Arrange: first provider fails, second succeeds creating one account at index 1 + failingProvider.createAccounts.mockRejectedValueOnce( + new Error('Unable to create accounts'), ); - }); - it('does not fail to create an account group if a non-EVM provider fails to create its account', async () => { - const groupIndex = 0; - const mockEvmAccount = MockAccountBuilder.from(MOCK_HD_ACCOUNT_1) + const mockNextEvmAccount = MockAccountBuilder.from(mockEvmAccount) .withEntropySource(MOCK_HD_KEYRING_1.metadata.id) .withGroupIndex(groupIndex) .get(); - const { wallet, providers } = setup({ - accounts: [[], []], - }); - - const [evmProvider, solProvider] = providers; + succeedingProvider.createAccounts.mockResolvedValueOnce([ + mockNextEvmAccount, + ]); - const mockSolProviderError = jest - .fn() - .mockRejectedValue('Unable to create'); - evmProvider.createAccounts.mockResolvedValueOnce([mockEvmAccount]); - solProvider.createAccounts.mockImplementation(mockSolProviderError); + succeedingProvider.getAccounts.mockReturnValueOnce([mockNextEvmAccount]); + succeedingProvider.getAccount.mockReturnValueOnce(mockNextEvmAccount); - await wallet.createMultichainAccountGroup(groupIndex); + const consoleSpy = jest.spyOn(console, 'warn').mockImplementation(); + const group = await wallet.createMultichainAccountGroup(groupIndex, { + waitForAllProvidersToFinishCreatingAccounts: true, + }); - expect( - await wallet.createMultichainAccountGroup(groupIndex), - ).toBeDefined(); - expect(mockSolProviderError).toHaveBeenCalled(); + // Should warn about partial failure but still create the group + expect(consoleSpy).toHaveBeenCalledWith( + `Unable to create some accounts for group index: ${groupIndex}. Providers threw the following errors:\n- Mocked Provider 1: Unable to create accounts`, + ); + expect(group.groupIndex).toBe(groupIndex); + const internalAccounts = group.getAccounts(); + expect(internalAccounts).toHaveLength(1); + expect(internalAccounts[0]).toStrictEqual(mockNextEvmAccount); }); - it('fails to create an account group if any of the provider fails to create its account and waitForAllProvidersToFinishCreatingAccounts is true', async () => { - const groupIndex = 1; - - const mockEvmAccount = MockAccountBuilder.from(MOCK_HD_ACCOUNT_1) - .withEntropySource(MOCK_HD_KEYRING_1.metadata.id) - .withGroupIndex(0) - .get(); + it('fails to create an account group if all providers fail to create their accounts (waitForAllProvidersToFinishCreatingAccounts = true)', async () => { const { wallet, providers } = setup({ - accounts: [[mockEvmAccount]], // 1 provider + accounts: [[], []], }); - const [provider] = providers; - provider.createAccounts.mockRejectedValueOnce( + + const [failingProvider1, failingProvider2] = providers; + + failingProvider1.createAccounts.mockRejectedValueOnce( + new Error('Unable to create accounts'), + ); + + failingProvider2.createAccounts.mockRejectedValueOnce( new Error('Unable to create accounts'), ); await expect( - wallet.createMultichainAccountGroup(groupIndex, { + wallet.createMultichainAccountGroup(0, { waitForAllProvidersToFinishCreatingAccounts: true, }), ).rejects.toThrow( - 'Unable to create multichain account group for index: 1', + 'Unable to create multichain account group for index: 0 due to provider failures:\n- Mocked Provider 0: Unable to create accounts\n- Mocked Provider 1: Unable to create accounts', ); + + expect(wallet.getAccountGroups()).toHaveLength(0); }); it('captures an error when a provider fails to create its account', async () => { @@ -379,11 +322,7 @@ describe('MultichainAccountWallet', () => { const providerError = new Error('Unable to create accounts'); provider.createAccounts.mockRejectedValueOnce(providerError); const callSpy = jest.spyOn(messenger, 'call'); - await expect( - wallet.createMultichainAccountGroup(groupIndex), - ).rejects.toThrow( - 'Unable to create multichain account group for index: 1', - ); + await wallet.createMultichainAccountGroup(groupIndex); expect(callSpy).toHaveBeenCalledWith( 'ErrorReportingService:captureException', new Error('Unable to create account with provider "Mocked Provider 0"'), @@ -392,50 +331,26 @@ describe('MultichainAccountWallet', () => { }); it('aggregates non-EVM failures when waiting for all providers', async () => { - const startingIndex = 0; - - const mockEvmAccount = MockAccountBuilder.from(MOCK_HD_ACCOUNT_1) - .withEntropySource(MOCK_HD_KEYRING_1.metadata.id) - .withGroupIndex(startingIndex) - .get(); - const { wallet, providers } = setup({ - providers: [ - setupNamedAccountProvider({ accounts: [mockEvmAccount], index: 0 }), - setupNamedAccountProvider({ - name: 'Non-EVM Provider', - accounts: [], - index: 1, - }), - ], + accounts: [[], []], }); - const nextIndex = 1; - const nextEvmAccount = MockAccountBuilder.from(mockEvmAccount) - .withGroupIndex(nextIndex) - .get(); - - const [evmProvider, solProvider] = providers; - evmProvider.createAccounts.mockResolvedValueOnce([nextEvmAccount]); - evmProvider.getAccounts.mockReturnValueOnce([nextEvmAccount]); - evmProvider.getAccount.mockReturnValueOnce(nextEvmAccount); + const [succeedingProvider, failingProvider] = providers; - const warnSpy = jest.spyOn(console, 'warn').mockImplementation(); + succeedingProvider.createAccounts.mockResolvedValueOnce([ + MOCK_HD_ACCOUNT_1, + ]); - const SOL_PROVIDER_ERROR = 'SOL create failed'; - solProvider.createAccounts.mockRejectedValueOnce( - new Error(SOL_PROVIDER_ERROR), + failingProvider.createAccounts.mockRejectedValueOnce( + new Error('Unable to create accounts'), ); - await expect( - wallet.createMultichainAccountGroup(nextIndex, { - waitForAllProvidersToFinishCreatingAccounts: true, - }), - ).rejects.toThrow( - `Unable to create multichain account group for index: ${nextIndex}:\n- Error: ${SOL_PROVIDER_ERROR}`, - ); + const consoleSpy = jest.spyOn(console, 'warn').mockImplementation(); + await wallet.createMultichainAccountGroup(0); - expect(warnSpy).toHaveBeenCalled(); + expect(consoleSpy).toHaveBeenCalledWith( + 'Unable to create some accounts for group index: 0 with provider "Mocked Provider 1". Error: Unable to create accounts', + ); }); }); @@ -487,6 +402,7 @@ describe('MultichainAccountWallet', () => { expect(internalAccounts).toHaveLength(2); // EVM + SOL. expect(internalAccounts[0].type).toBe(EthAccountType.Eoa); expect(internalAccounts[1].type).toBe(SolAccountType.DataAccount); + expect(wallet.getAccountGroups()).toHaveLength(2); }); }); @@ -528,9 +444,6 @@ describe('MultichainAccountWallet', () => { await wallet.alignAccounts(); - // EVM provider already has group 0 and 1; should not be called. - expect(providers[0].createAccounts).not.toHaveBeenCalled(); - // Sol provider is missing group 1; should be called to create it. expect(providers[1].createAccounts).toHaveBeenCalledWith({ entropySource: wallet.entropySource, @@ -573,9 +486,6 @@ describe('MultichainAccountWallet', () => { await wallet.alignAccountsOf(0); - // EVM provider already has group 0; should not be called. - expect(providers[0].createAccounts).not.toHaveBeenCalled(); - // Sol provider is missing group 0; should be called to create it. expect(providers[1].createAccounts).toHaveBeenCalledWith({ entropySource: wallet.entropySource, diff --git a/packages/multichain-account-service/src/MultichainAccountWallet.ts b/packages/multichain-account-service/src/MultichainAccountWallet.ts index 045138d51a9..75c37b4dccf 100644 --- a/packages/multichain-account-service/src/MultichainAccountWallet.ts +++ b/packages/multichain-account-service/src/MultichainAccountWallet.ts @@ -23,10 +23,14 @@ import { projectLogger as log, WARNING_PREFIX, } from './logger'; -import { MultichainAccountGroup } from './MultichainAccountGroup'; -import { EvmAccountProvider, type Bip44AccountProvider } from './providers'; +import { + type GroupState, + MultichainAccountGroup, +} from './MultichainAccountGroup'; +import type { ServiceState, StateKeys } from './MultichainAccountService'; +import { type Bip44AccountProvider, EvmAccountProvider } from './providers'; import type { MultichainAccountServiceMessenger } from './types'; -import { createSentryError, toRejectedErrorMessage } from './utils'; +import { createSentryError } from './utils'; /** * The context for a provider discovery. @@ -40,6 +44,11 @@ type AccountProviderDiscoveryContext< accounts: Account[]; }; +export type WalletState = ServiceState[StateKeys['entropySource']]; + +// type alias to make clear this state is generated by discovery +type DiscoveredGroupsState = WalletState; + /** * A multichain account wallet that holds multiple multichain accounts (one multichain account per * group index). @@ -62,7 +71,6 @@ export class MultichainAccountWallet< readonly #log: Logger; - // eslint-disable-next-line @typescript-eslint/prefer-readonly #initialized = false; #status: MultichainAccountWalletStatus; @@ -86,69 +94,37 @@ export class MultichainAccountWallet< // Initial synchronization (don't emit events during initialization). this.#status = 'uninitialized'; - this.sync(); - this.#initialized = true; - this.#status = 'ready'; } /** - * Force wallet synchronization. + * Initialize the wallet and construct the internal representation of multichain account groups. * - * This can be used if account providers got new accounts that the wallet - * doesn't know about. + * @param walletState - The wallet state. */ - sync(): void { - this.#log('Synchronizing with account providers...'); - for (const provider of this.#providers) { - for (const account of provider.getAccounts()) { - const { entropy } = account.options; - - // Filter for this wallet only. - if (entropy.id !== this.entropySource) { - continue; - } - - // This multichain account might exists already. - let multichainAccount = this.#accountGroups.get(entropy.groupIndex); - if (!multichainAccount) { - multichainAccount = new MultichainAccountGroup({ - groupIndex: entropy.groupIndex, - wallet: this, - providers: this.#providers, - messenger: this.#messenger, - }); - - // This existing multichain account group might differ from the - // `createMultichainAccountGroup` behavior. When creating a new - // group, we expect the providers to all succeed. But here, we're - // just fetching the account lists from them, so this group might - // not be "aligned" yet (e.g having a missing Solana account). - // - // Since "aligning" is an async operation, it would have to be run - // after the first-sync. - // TODO: Implement align mechanism to create "missing" accounts. - - this.#log(`Found a new group: [${multichainAccount.id}]`); - this.#accountGroups.set(entropy.groupIndex, multichainAccount); - } - } + init(walletState: WalletState) { + this.#log('Initializing wallet state...'); + for (const [groupIndex, groupState] of Object.entries(walletState)) { + // Have to convert to number because the state keys become strings when we construct the state object in the service + const indexAsNumber = Number(groupIndex); + const group = new MultichainAccountGroup({ + groupIndex: indexAsNumber, + wallet: this, + providers: this.#providers, + messenger: this.#messenger, + }); + + this.#log(`Creating new group for index ${indexAsNumber}...`); + + group.init(groupState); + + this.#accountGroups.set(indexAsNumber, group); } - - // Now force-sync all remaining multichain accounts. - for (const [ - groupIndex, - multichainAccount, - ] of this.#accountGroups.entries()) { - multichainAccount.sync(); - - // Clean up old multichain accounts. - if (!multichainAccount.hasAccounts()) { - this.#log(`Deleting group: [${multichainAccount.id}]`); - this.#accountGroups.delete(groupIndex); - } + if (!this.#initialized) { + this.#initialized = true; + this.#status = 'ready'; } - this.#log('Synchronized'); + this.#log('Finished initializing wallet state...'); } /** @@ -230,6 +206,8 @@ export class MultichainAccountWallet< * @param options.groupIndex - The group index to create accounts for. * @param options.providers - The non‑EVM account providers. * @param options.awaitAll - Whether to wait for all providers to finish. + * @param options.group - The group object pertaining to the group index to create accounts for. + * @param options.evmError - The error message if the EVM provider failed to create accounts. * @throws If awaitAll is true and any provider fails to create accounts. * @returns A promise that resolves when done (if awaitAll is true) or immediately (if false). */ @@ -237,10 +215,14 @@ export class MultichainAccountWallet< groupIndex, providers, awaitAll, + group, + evmError, }: { groupIndex: number; providers: Bip44AccountProvider[]; awaitAll: boolean; + group: MultichainAccountGroup; + evmError?: string; }): Promise { if (awaitAll) { const tasks = providers.map((provider) => @@ -267,45 +249,72 @@ export class MultichainAccountWallet< ); const results = await Promise.allSettled(tasks); - if (results.some((r) => r.status === 'rejected')) { - const errorMessage = toRejectedErrorMessage( - `Unable to create multichain account group for index: ${groupIndex}`, - results, - ); + let failures = 0; + + const providerFailures = results.reduce((acc, result, idx) => { + if (result.status === 'rejected') { + failures += 1; + acc += `\n- ${providers[idx].getName()}: ${result.reason.message}`; + } + return acc; + }, ''); - this.#log(`${WARNING_PREFIX} ${errorMessage}`); - console.warn(errorMessage); - throw new Error(errorMessage); + if (failures === providers.length && evmError?.length) { + // We throw an error if there's a failure on every provider + throw new Error( + `Unable to create multichain account group for index: ${groupIndex} due to provider failures:${evmError}${providerFailures}`, + ); + } else if (providerFailures.length) { + // We warn there's failures on some providers and thus misalignment, but we still create the group + const message = `Unable to create some accounts for group index: ${groupIndex}. Providers threw the following errors:${providerFailures}`; + console.warn(message); + this.#log(`${WARNING_PREFIX} ${message}`); } - return; - } - // Background mode: start tasks and log errors. - // Optional throttling is handled internally by each provider based on its config. - providers.forEach((provider) => { - // eslint-disable-next-line no-void - void provider - .createAccounts({ - entropySource: this.#entropySource, - groupIndex, - }) - .catch((error) => { - const errorMessage = `Unable to create multichain account group for index: ${groupIndex} (background mode with provider "${provider.getName()}")`; - this.#log(`${WARNING_PREFIX} ${errorMessage}:`, error); - const sentryError = createSentryError( - `Unable to create account with provider "${provider.getName()}"`, - error, - { - groupIndex, - provider: provider.getName(), - }, - ); - this.#messenger.call( - 'ErrorReportingService:captureException', - sentryError, + // No need to fetch the accounts list from the AccountsController since we already have the account IDs to be used in the controller + const groupState = results.reduce((state, result, idx) => { + if (result.status === 'fulfilled') { + state[providers[idx].getName()] = result.value.map( + (account) => account.id, ); - }); - }); + } + return state; + }, {}); + + group.update(groupState); + } else { + // Create account with other providers in the background + providers.forEach((provider) => { + provider + .createAccounts({ + entropySource: this.#entropySource, + groupIndex, + }) + .then((accounts) => { + const accountIds = accounts.map((account) => account.id); + group?.update({ [provider.getName()]: accountIds }); + return group; + }) + .catch((error) => { + // Log errors from background providers but don't fail the operation + const errorMessage = `Unable to create some accounts for group index: ${groupIndex} with provider "${provider.getName()}". Error: ${(error as Error).message}`; + console.warn(errorMessage); + this.#log(`${WARNING_PREFIX} ${errorMessage}:`, error); + const sentryError = createSentryError( + `Unable to create account with provider "${provider.getName()}"`, + error, + { + groupIndex, + provider: provider.getName(), + }, + ); + this.#messenger.call( + 'ErrorReportingService:captureException', + sentryError, + ); + }); + }); + } } /** @@ -410,11 +419,8 @@ export class MultichainAccountWallet< } let group = this.getMultichainAccountGroup(groupIndex); - if (group) { - // If the group already exists, we just `sync` it and returns the same - // reference. - group.sync(); + if (group) { this.#log( `Trying to re-create existing group: [${group.id}] (idempotent)`, ); @@ -431,28 +437,45 @@ export class MultichainAccountWallet< 'EVM account provider must be first', ); - try { - await evmProvider.createAccounts({ + // Create the group here because the EVM provider will not fail. + // There isn't a failure scenario here since this function is only used by createNextMultichainAccountGroup (no throw on gap error). + // We have to deterministically create the group here because otherwise we can't set the group in state. + group = new MultichainAccountGroup({ + wallet: this, + providers: this.#providers, + groupIndex, + messenger: this.#messenger, + }); + + let evmError = ''; + + await evmProvider + .createAccounts({ entropySource: this.#entropySource, groupIndex, + }) + .then((account) => { + group?.init({ [evmProvider.getName()]: [account[0].id] }); + return group; + }) + .catch((error) => { + const errorMessage = `Unable to create some accounts for group index: ${groupIndex} with provider "${evmProvider.getName()}". Error: ${(error as Error).message}`; + console.warn(errorMessage); + this.#log(`${ERROR_PREFIX} ${errorMessage}:`, error); + evmError = `\n- ${evmProvider.getName()}: ${(error as Error).message}`; + const sentryError = createSentryError( + `Unable to create account with provider "${evmProvider.getName()}"`, + error as Error, + { + groupIndex, + provider: evmProvider.getName(), + }, + ); + this.#messenger.call( + 'ErrorReportingService:captureException', + sentryError, + ); }); - } catch (error) { - const errorMessage = `Unable to create multichain account group for index: ${groupIndex} with provider "${evmProvider.getName()}". Error: ${(error as Error).message}`; - this.#log(`${ERROR_PREFIX} ${errorMessage}:`, error); - const sentryError = createSentryError( - `Unable to create account with provider "${evmProvider.getName()}"`, - error as Error, - { - groupIndex, - provider: evmProvider.getName(), - }, - ); - this.#messenger.call( - 'ErrorReportingService:captureException', - sentryError, - ); - throw new Error(errorMessage); - } // We then create accounts with other providers (some being throttled if configured). // Depending on the options, we either await all providers or run them in background. @@ -461,6 +484,8 @@ export class MultichainAccountWallet< groupIndex, providers: otherProviders, awaitAll: true, + group, + evmError, }); } else { // eslint-disable-next-line no-void @@ -468,46 +493,13 @@ export class MultichainAccountWallet< groupIndex, providers: otherProviders, awaitAll: false, + group, }); } - // -------------------------------------------------------------------------------- - // READ THIS CAREFULLY: - // - // Since we're not "fully supporting multichain" for now, we still rely on single - // :accountCreated events to sync multichain account groups and wallets. Which means - // that even if of the provider fails, some accounts will still be created on some - // other providers and will become "available" on the `AccountsController`, like: - // - // 1. Creating a multichain account group for index 1 - // 2. EvmAccountProvider.createAccounts returns the EVM account for index 1 - // * AccountsController WILL fire :accountCreated for this account - // * This account WILL BE "available" on the AccountsController state - // 3. SolAccountProvider.createAccounts fails to create a Solana account for index 1 - // * AccountsController WON't fire :accountCreated for this account - // * This account WON'T be "available" on the Account - // 4. MultichainAccountService will receive a :accountCreated for the EVM account from - // step 2 and will create a new multichain account group for index 1, but it won't - // receive any event for the Solana account of this group. Thus, this group won't be - // "aligned" (missing "blockchain account" on this group). - // - // -------------------------------------------------------------------------------- - - // Because of the :accountAdded automatic sync, we might already have created the - // group, so we first try to get it. - group = this.getMultichainAccountGroup(groupIndex); - if (!group) { - // If for some reason it's still not created, we're creating it explicitly now: - group = new MultichainAccountGroup({ - wallet: this, - providers: this.#providers, - groupIndex, - messenger: this.#messenger, - }); - } + // Register the account(s) to our internal map. + this.#accountGroups.set(groupIndex, group); - // Register the account to our internal map. - this.#accountGroups.set(groupIndex, group); // `group` cannot be undefined here. this.#log(`New group created: [${group.id}]`); if (this.#initialized) { @@ -584,6 +576,17 @@ export class MultichainAccountWallet< // Start with the next available group index (so we can resume the discovery // from there). let maxGroupIndex = this.getNextGroupIndex(); + const discoveredGroupsState: DiscoveredGroupsState = {}; + + const addDiscoveryResultToState = ( + result: Account[], + providerName: string, + groupIndex: number, + ) => { + const accountIds = result.map((account) => account.id); + discoveredGroupsState[groupIndex] ??= {}; + discoveredGroupsState[groupIndex][providerName] = accountIds; + }; // One serialized loop per provider; all run concurrently const runProviderDiscovery = async ( @@ -601,10 +604,10 @@ export class MultichainAccountWallet< let accounts: Account[] = []; try { - accounts = await context.provider.discoverAccounts({ + accounts = (await context.provider.discoverAccounts({ entropySource: this.#entropySource, groupIndex: targetGroupIndex, - }); + })) as Account[]; } catch (error) { context.stopped = true; console.error(error); @@ -642,6 +645,8 @@ export class MultichainAccountWallet< context.accounts = context.accounts.concat(accounts); + addDiscoveryResultToState(accounts, providerName, targetGroupIndex); + const nextGroupIndex = targetGroupIndex + 1; context.groupIndex = nextGroupIndex; @@ -662,9 +667,20 @@ export class MultichainAccountWallet< // Start discovery for each providers. await Promise.all(providerContexts.map(runProviderDiscovery)); - // Sync the wallet after discovery to ensure that the newly added accounts are added into their groups. - // We can potentially remove this if we know that this race condition is not an issue in practice. - this.sync(); + // Create discovered groups + for (const [groupIndex, groupState] of Object.entries( + discoveredGroupsState, + )) { + const indexAsNumber = Number(groupIndex); + const group = new MultichainAccountGroup({ + wallet: this, + providers: this.#providers, + groupIndex: indexAsNumber, + messenger: this.#messenger, + }); + group.init(groupState); + this.#accountGroups.set(indexAsNumber, group); + } // Align missing accounts from group. This is required to create missing account from non-discovered // indexes for some providers. diff --git a/packages/multichain-account-service/src/providers/AccountProviderWrapper.ts b/packages/multichain-account-service/src/providers/AccountProviderWrapper.ts index e2807bb892c..5f1f997e70b 100644 --- a/packages/multichain-account-service/src/providers/AccountProviderWrapper.ts +++ b/packages/multichain-account-service/src/providers/AccountProviderWrapper.ts @@ -35,6 +35,10 @@ export class AccountProviderWrapper extends BaseBip44AccountProvider { this.isEnabled = enabled; } + isDisabled(): boolean { + return !this.isEnabled; + } + /** * Override resyncAccounts to not execute it when disabled. * @@ -61,6 +65,16 @@ export class AccountProviderWrapper extends BaseBip44AccountProvider { return this.provider.getAccounts(); } + override async alignAccounts(options: { + entropySource: EntropySourceId; + groupIndex: number; + }): Promise<[boolean, Bip44Account['id'][]]> { + if (this.isDisabled()) { + return [true, []]; + } + return await this.provider.alignAccounts(options); + } + /** * Override getAccount to throw when disabled. * diff --git a/packages/multichain-account-service/src/providers/BaseBip44AccountProvider.ts b/packages/multichain-account-service/src/providers/BaseBip44AccountProvider.ts index 67eddd59709..4542e2006c1 100644 --- a/packages/multichain-account-service/src/providers/BaseBip44AccountProvider.ts +++ b/packages/multichain-account-service/src/providers/BaseBip44AccountProvider.ts @@ -42,7 +42,15 @@ export type Bip44AccountProvider< Account extends Bip44Account = Bip44Account, > = AccountProvider & { getName(): string; - + init(accounts: Bip44Account['id'][]): void; + isAccountCompatible(account: Bip44Account): boolean; + alignAccounts({ + entropySource, + groupIndex, + }: { + entropySource: EntropySourceId; + groupIndex: number; + }): Promise<[boolean, Account['id'][]]>; /** * Re-synchronize MetaMask accounts and the providers accounts if needed. * @@ -53,47 +61,62 @@ export type Bip44AccountProvider< resyncAccounts(accounts: Bip44Account[]): Promise; }; -export abstract class BaseBip44AccountProvider implements Bip44AccountProvider { +export abstract class BaseBip44AccountProvider< + Account extends Bip44Account = Bip44Account, +> implements Bip44AccountProvider +{ protected readonly messenger: MultichainAccountServiceMessenger; + accountsList: Set['id']> = new Set(); + constructor(messenger: MultichainAccountServiceMessenger) { this.messenger = messenger; } - abstract getName(): string; - - #getAccounts( - filter: (account: KeyringAccount) => boolean = () => true, - ): Bip44Account[] { - const accounts: Bip44Account[] = []; - - for (const account of this.messenger.call( - // NOTE: Even though the name is misleading, this only fetches all internal - // accounts, including EVM and non-EVM. We might wanna change this action - // name once we fully support multichain accounts. - 'AccountsController:listMultichainAccounts', - )) { - if ( - isBip44Account(account) && - this.isAccountCompatible(account) && - filter(account) - ) { - accounts.push(account); - } + /** + * Add accounts to the provider. + * + * @param accounts - The accounts to add. + */ + init(accounts: Account['id'][]): void { + for (const account of accounts) { + this.accountsList.add(account); } + } - return accounts; + /** + * Get the accounts list for the provider. + * + * @returns The accounts list. + */ + #getAccountIds(): Account['id'][] { + return [...this.accountsList]; } - getAccounts(): Bip44Account[] { - return this.#getAccounts(); + /** + * Get the accounts list for the provider from the AccountsController. + * + * @returns The accounts list. + */ + getAccounts(): Account[] { + const accountsIds = this.#getAccountIds(); + const internalAccounts = this.messenger.call( + 'AccountsController:getAccounts', + accountsIds, + ); + // we cast here because we know that the accounts are BIP-44 compatible + return internalAccounts as unknown as Account[]; } - getAccount( - id: Bip44Account['id'], - ): Bip44Account { - // TODO: Maybe just use a proper find for faster lookup? - const [found] = this.#getAccounts((account) => account.id === id); + /** + * Get the account for the provider. + * + * @param id - The account ID. + * @returns The account. + * @throws If the account is not found. + */ + getAccount(id: Account['id']): Account { + const found = this.getAccounts().find((account) => account.id === id); if (!found) { throw new Error(`Unable to find account: ${id}`); @@ -125,6 +148,23 @@ export abstract class BaseBip44AccountProvider implements Bip44AccountProvider { return result as CallbackResult; } + async alignAccounts({ + entropySource, + groupIndex, + }: { + entropySource: EntropySourceId; + groupIndex: number; + }): Promise<[boolean, Account['id'][]]> { + const accounts = await this.createAccounts({ + entropySource, + groupIndex, + }); + const accountIds = accounts.map((account) => account.id); + return [true, accountIds]; + } + + abstract getName(): string; + abstract resyncAccounts( accounts: Bip44Account[], ): Promise; @@ -137,7 +177,7 @@ export abstract class BaseBip44AccountProvider implements Bip44AccountProvider { }: { entropySource: EntropySourceId; groupIndex: number; - }): Promise[]>; + }): Promise; abstract discoverAccounts({ entropySource, @@ -145,5 +185,5 @@ export abstract class BaseBip44AccountProvider implements Bip44AccountProvider { }: { entropySource: EntropySourceId; groupIndex: number; - }): Promise[]>; + }): Promise; } diff --git a/packages/multichain-account-service/src/providers/BtcAccountProvider.test.ts b/packages/multichain-account-service/src/providers/BtcAccountProvider.test.ts index d61b7362146..986591ed822 100644 --- a/packages/multichain-account-service/src/providers/BtcAccountProvider.test.ts +++ b/packages/multichain-account-service/src/providers/BtcAccountProvider.test.ts @@ -125,7 +125,7 @@ function setup({ const keyring = new MockBtcKeyring(accounts); messenger.registerActionHandler( - 'AccountsController:listMultichainAccounts', + 'AccountsController:getAccounts', () => accounts, ); @@ -205,6 +205,22 @@ describe('BtcAccountProvider', () => { ); }); + it('returns true if an account is compatible', () => { + const account = MOCK_BTC_P2WPKH_ACCOUNT_1; + const { provider } = setup({ + accounts: [account], + }); + expect(provider.isAccountCompatible(account)).toBe(true); + }); + + it('returns false if an account is not compatible', () => { + const account = MOCK_HD_ACCOUNT_1; + const { provider } = setup({ + accounts: [account], + }); + expect(provider.isAccountCompatible(account)).toBe(false); + }); + it('creates accounts', async () => { const accounts = [MOCK_BTC_P2WPKH_ACCOUNT_1]; const { provider, keyring } = setup({ diff --git a/packages/multichain-account-service/src/providers/BtcAccountProvider.ts b/packages/multichain-account-service/src/providers/BtcAccountProvider.ts index 1065737e64d..b329f3abc27 100644 --- a/packages/multichain-account-service/src/providers/BtcAccountProvider.ts +++ b/packages/multichain-account-service/src/providers/BtcAccountProvider.ts @@ -73,6 +73,7 @@ export class BtcAccountProvider extends SnapAccountProvider { ); assertIsBip44Account(account); + this.accountsList.add(account.id); return [account]; }); } diff --git a/packages/multichain-account-service/src/providers/EvmAccountProvider.test.ts b/packages/multichain-account-service/src/providers/EvmAccountProvider.test.ts index ca7922c89f4..b1a122dbdb3 100644 --- a/packages/multichain-account-service/src/providers/EvmAccountProvider.test.ts +++ b/packages/multichain-account-service/src/providers/EvmAccountProvider.test.ts @@ -1,5 +1,6 @@ /* eslint-disable jsdoc/require-jsdoc */ import { publicToAddress } from '@ethereumjs/util'; +import { getUUIDFromAddressOfNormalAccount } from '@metamask/accounts-controller'; import { type KeyringMetadata } from '@metamask/keyring-controller'; import type { EthKeyring, @@ -24,13 +25,19 @@ import { MOCK_HD_ACCOUNT_1, MOCK_HD_ACCOUNT_2, MOCK_HD_KEYRING_1, + MOCK_SOL_ACCOUNT_1, MockAccountBuilder, + mockAsInternalAccount, type RootMessenger, } from '../tests'; -jest.mock('@ethereumjs/util', () => ({ - publicToAddress: jest.fn(), -})); +jest.mock('@ethereumjs/util', () => { + const actual = jest.requireActual('@ethereumjs/util'); + return { + ...actual, + publicToAddress: jest.fn(), + }; +}); function mockNextDiscoveryAddress(address: string) { jest.mocked(publicToAddress).mockReturnValue(createBytes(address as Hex)); @@ -135,28 +142,32 @@ function setup({ messenger: RootMessenger; keyring: MockEthKeyring; mocks: { - getAccountByAddress: jest.Mock; mockProviderRequest: jest.Mock; + mockGetAccount: jest.Mock; }; } { const keyring = new MockEthKeyring(accounts); messenger.registerActionHandler( - 'AccountsController:listMultichainAccounts', + 'AccountsController:getAccounts', () => accounts, ); - const mockGetAccountByAddress = jest - .fn() - .mockImplementation((address: string) => - keyring.accounts.find((account) => account.address === address), + const mockGetAccount = jest.fn().mockImplementation((id) => { + return keyring.accounts.find( + (account) => + account.id === id || + getUUIDFromAddressOfNormalAccount(account.address) === id, ); + }); - const mockProviderRequest = jest.fn().mockImplementation(({ method }) => { - if (method === 'eth_getTransactionCount') { - return discovery?.transactionCount ?? '0x2'; - } - throw new Error(`Unknown method: ${method}`); + messenger.registerActionHandler( + 'AccountsController:getAccount', + mockGetAccount, + ); + + const mockGetAccountByAddress = jest.fn().mockImplementation((address) => { + return keyring.accounts.find((account) => account.address === address); }); messenger.registerActionHandler( @@ -164,6 +175,13 @@ function setup({ mockGetAccountByAddress, ); + const mockProviderRequest = jest.fn().mockImplementation(({ method }) => { + if (method === 'eth_getTransactionCount') { + return discovery?.transactionCount ?? '0x2'; + } + throw new Error(`Unknown method: ${method}`); + }); + messenger.registerActionHandler( 'KeyringController:withKeyring', async (_, operation) => operation({ keyring, metadata: keyring.metadata }), @@ -198,8 +216,8 @@ function setup({ messenger, keyring, mocks: { - getAccountByAddress: mockGetAccountByAddress, mockProviderRequest, + mockGetAccount, }, }; } @@ -220,12 +238,15 @@ describe('EvmAccountProvider', () => { }); it('gets a specific account', () => { - const account = MOCK_HD_ACCOUNT_1; + const customId = 'custom-id-123'; + const account = MockAccountBuilder.from(MOCK_HD_ACCOUNT_1) + .withId(customId) + .get(); const { provider } = setup({ accounts: [account], }); - expect(provider.getAccount(account.id)).toStrictEqual(account); + expect(provider.getAccount(customId)).toStrictEqual(account); }); it('throws if account does not exist', () => { @@ -240,6 +261,22 @@ describe('EvmAccountProvider', () => { ); }); + it('returns true if an account is compatible', () => { + const account = MOCK_HD_ACCOUNT_1; + const { provider } = setup({ + accounts: [account], + }); + expect(provider.isAccountCompatible(account)).toBe(true); + }); + + it('returns false if an account is not compatible', () => { + const account = MOCK_SOL_ACCOUNT_1; + const { provider } = setup({ + accounts: [account], + }); + expect(provider.isAccountCompatible(account)).toBe(false); + }); + it('does not re-create accounts (idempotent)', async () => { const accounts = [MOCK_HD_ACCOUNT_1, MOCK_HD_ACCOUNT_2]; const { provider } = setup({ @@ -260,8 +297,8 @@ describe('EvmAccountProvider', () => { accounts, }); - mocks.getAccountByAddress.mockReturnValue({ - ...MOCK_HD_ACCOUNT_1, + mocks.mockGetAccount.mockReturnValue({ + ...mockAsInternalAccount(MOCK_HD_ACCOUNT_1), options: {}, // No options, so it cannot be BIP-44 compatible. }); @@ -292,7 +329,7 @@ describe('EvmAccountProvider', () => { }); // Simulate an account not found. - mocks.getAccountByAddress.mockImplementation(() => undefined); + mocks.mockGetAccount.mockImplementation(() => undefined); await expect( provider.createAccounts({ diff --git a/packages/multichain-account-service/src/providers/EvmAccountProvider.ts b/packages/multichain-account-service/src/providers/EvmAccountProvider.ts index 7837ed0d387..48e1726dd4b 100644 --- a/packages/multichain-account-service/src/providers/EvmAccountProvider.ts +++ b/packages/multichain-account-service/src/providers/EvmAccountProvider.ts @@ -1,5 +1,6 @@ import { publicToAddress } from '@ethereumjs/util'; import type { Bip44Account } from '@metamask/account-api'; +import { getUUIDFromAddressOfNormalAccount } from '@metamask/accounts-controller'; import type { TraceCallback } from '@metamask/controller-utils'; import type { HdKeyring } from '@metamask/eth-hd-keyring'; import type { EntropySourceId, KeyringAccount } from '@metamask/keyring-api'; @@ -99,6 +100,28 @@ export class EvmAccountProvider extends BaseBip44AccountProvider { return provider; } + /** + * Get the account ID for an EVM account. + * + * Note: Since the account ID is deterministic at the AccountsController level, + * we can use this method to get the account ID from the address. + * + * @param address - The address of the account. + * @returns The account ID. + */ + #getAccountId(address: Hex): string { + return getUUIDFromAddressOfNormalAccount(address); + } + + /** + * Create an EVM account. + * + * @param opts - The options for the creation of the account. + * @param opts.entropySource - The entropy source to use for the creation of the account. + * @param opts.groupIndex - The index of the group to create the account for. + * @param opts.throwOnGap - Whether to throw an error if the account index is not contiguous. + * @returns The account ID and a boolean indicating if the account was created. + */ async #createAccount({ entropySource, groupIndex, @@ -129,6 +152,14 @@ export class EvmAccountProvider extends BaseBip44AccountProvider { return result; } + /** + * Create accounts for the EVM provider. + * + * @param opts - The options for the creation of the accounts. + * @param opts.entropySource - The entropy source to use for the creation of the accounts. + * @param opts.groupIndex - The index of the group to create the accounts for. + * @returns The accounts for the EVM provider. + */ async createAccounts({ entropySource, groupIndex, @@ -142,9 +173,11 @@ export class EvmAccountProvider extends BaseBip44AccountProvider { throwOnGap: true, }); + const accountId = this.#getAccountId(address); + const account = this.messenger.call( - 'AccountsController:getAccountByAddress', - address, + 'AccountsController:getAccount', + accountId, ); // We MUST have the associated internal account. @@ -153,9 +186,18 @@ export class EvmAccountProvider extends BaseBip44AccountProvider { const accountsArray = [account]; assertAreBip44Accounts(accountsArray); + this.accountsList.add(account.id); return accountsArray; } + /** + * Get the transaction count for an EVM account. + * This method uses a retry and timeout mechanism to handle transient failures. + * + * @param provider - The provider to use for the transaction count. + * @param address - The address of the account. + * @returns The transaction count. + */ async #getTransactionCount( provider: Provider, address: Hex, @@ -260,6 +302,7 @@ export class EvmAccountProvider extends BaseBip44AccountProvider { ); assertInternalAccountExists(account); assertIsBip44Account(account); + this.accountsList.add(account.id); return [account]; }, ); diff --git a/packages/multichain-account-service/src/providers/SolAccountProvider.test.ts b/packages/multichain-account-service/src/providers/SolAccountProvider.test.ts index 68645fa31d9..cbe50460ed0 100644 --- a/packages/multichain-account-service/src/providers/SolAccountProvider.test.ts +++ b/packages/multichain-account-service/src/providers/SolAccountProvider.test.ts @@ -109,7 +109,7 @@ function setup({ const keyring = new MockSolanaKeyring(accounts); messenger.registerActionHandler( - 'AccountsController:listMultichainAccounts', + 'AccountsController:getAccounts', () => accounts, ); @@ -199,6 +199,22 @@ describe('SolAccountProvider', () => { ); }); + it('returns true if an account is compatible', () => { + const account = MOCK_SOL_ACCOUNT_1; + const { provider } = setup({ + accounts: [account], + }); + expect(provider.isAccountCompatible(account)).toBe(true); + }); + + it('returns false if an account is not compatible', () => { + const account = MOCK_HD_ACCOUNT_1; + const { provider } = setup({ + accounts: [account], + }); + expect(provider.isAccountCompatible(account)).toBe(false); + }); + it('creates accounts', async () => { const accounts = [MOCK_SOL_ACCOUNT_1]; const { provider, keyring } = setup({ diff --git a/packages/multichain-account-service/src/providers/SolAccountProvider.ts b/packages/multichain-account-service/src/providers/SolAccountProvider.ts index 98cf2db6d5a..487d476ac52 100644 --- a/packages/multichain-account-service/src/providers/SolAccountProvider.ts +++ b/packages/multichain-account-service/src/providers/SolAccountProvider.ts @@ -99,6 +99,7 @@ export class SolAccountProvider extends SnapAccountProvider { derivationPath, }); + this.accountsList.add(account.id); return [account]; }); } @@ -148,6 +149,10 @@ export class SolAccountProvider extends SnapAccountProvider { ), ); + for (const account of createdAccounts) { + this.accountsList.add(account.id); + } + return createdAccounts; }, ); diff --git a/packages/multichain-account-service/src/providers/TrxAccountProvider.test.ts b/packages/multichain-account-service/src/providers/TrxAccountProvider.test.ts index fc0ca2e8c73..5146fd20873 100644 --- a/packages/multichain-account-service/src/providers/TrxAccountProvider.test.ts +++ b/packages/multichain-account-service/src/providers/TrxAccountProvider.test.ts @@ -98,7 +98,7 @@ function setup({ const keyring = new MockTronKeyring(accounts); messenger.registerActionHandler( - 'AccountsController:listMultichainAccounts', + 'AccountsController:getAccounts', () => accounts, ); @@ -185,6 +185,22 @@ describe('TrxAccountProvider', () => { ); }); + it('returns true if an account is compatible', () => { + const account = MOCK_TRX_ACCOUNT_1; + const { provider } = setup({ + accounts: [account], + }); + expect(provider.isAccountCompatible(account)).toBe(true); + }); + + it('returns false if an account is not compatible', () => { + const account = MOCK_HD_ACCOUNT_1; + const { provider } = setup({ + accounts: [account], + }); + expect(provider.isAccountCompatible(account)).toBe(false); + }); + it('creates accounts', async () => { const accounts = [MOCK_TRX_ACCOUNT_1]; const { provider, keyring } = setup({ diff --git a/packages/multichain-account-service/src/providers/TrxAccountProvider.ts b/packages/multichain-account-service/src/providers/TrxAccountProvider.ts index f436e47ce84..ad182c233cc 100644 --- a/packages/multichain-account-service/src/providers/TrxAccountProvider.ts +++ b/packages/multichain-account-service/src/providers/TrxAccountProvider.ts @@ -74,6 +74,7 @@ export class TrxAccountProvider extends SnapAccountProvider { ); assertIsBip44Account(account); + this.accountsList.add(account.id); return [account]; }); } diff --git a/packages/multichain-account-service/src/tests/messenger.ts b/packages/multichain-account-service/src/tests/messenger.ts index a6150212283..d4658991b5b 100644 --- a/packages/multichain-account-service/src/tests/messenger.ts +++ b/packages/multichain-account-service/src/tests/messenger.ts @@ -63,6 +63,9 @@ export function getMultichainAccountServiceMessenger( 'KeyringController:addNewKeyring', 'NetworkController:findNetworkClientIdByChainId', 'NetworkController:getNetworkClientById', + 'KeyringController:createNewVaultAndKeychain', + 'KeyringController:createNewVaultAndRestore', + 'AccountsController:getAccounts', ], events: [ 'KeyringController:stateChange', diff --git a/packages/multichain-account-service/src/tests/providers.ts b/packages/multichain-account-service/src/tests/providers.ts index 6c204ad8759..c0c8ea1c0b3 100644 --- a/packages/multichain-account-service/src/tests/providers.ts +++ b/packages/multichain-account-service/src/tests/providers.ts @@ -1,21 +1,30 @@ /* eslint-disable jsdoc/require-jsdoc */ import type { Bip44Account } from '@metamask/account-api'; -import { isBip44Account } from '@metamask/account-api'; -import type { KeyringAccount } from '@metamask/keyring-api'; +import type { EntropySourceId, KeyringAccount } from '@metamask/keyring-api'; -import { EvmAccountProvider } from '../providers'; +import { + AccountProviderWrapper, + EvmAccountProvider, + BaseBip44AccountProvider, +} from '../providers'; export type MockAccountProvider = { accounts: KeyringAccount[]; + accountsList: Set; constructor: jest.Mock; + alignAccounts: jest.Mock; + init: jest.Mock; resyncAccounts: jest.Mock; getAccount: jest.Mock; getAccounts: jest.Mock; createAccounts: jest.Mock; discoverAccounts: jest.Mock; - isAccountCompatible?: jest.Mock; + isAccountCompatible: jest.Mock; getName: jest.Mock; + isEnabled: boolean; + isDisabled: jest.Mock; + setEnabled: jest.Mock; }; export function makeMockAccountProvider( @@ -23,7 +32,10 @@ export function makeMockAccountProvider( ): MockAccountProvider { return { accounts, + accountsList: new Set(), constructor: jest.fn(), + alignAccounts: jest.fn(), + init: jest.fn(), resyncAccounts: jest.fn(), getAccount: jest.fn(), getAccounts: jest.fn(), @@ -31,14 +43,16 @@ export function makeMockAccountProvider( discoverAccounts: jest.fn(), isAccountCompatible: jest.fn(), getName: jest.fn(), + isDisabled: jest.fn(), + setEnabled: jest.fn(), + isEnabled: true, }; } -export function setupNamedAccountProvider({ +export function setupBip44AccountProvider({ name = 'Mocked Provider', accounts, mocks = makeMockAccountProvider(), - filter = () => true, index, }: { name?: string; @@ -50,10 +64,16 @@ export function setupNamedAccountProvider({ // You can mock this and all other mocks will re-use that list // of accounts. mocks.accounts = accounts; + mocks.accountsList = new Set(accounts.map((account) => account.id)); + // Toggle enabled state only + mocks.setEnabled.mockImplementation((enabled: boolean) => { + mocks.isEnabled = enabled; + }); + mocks.isDisabled.mockImplementation(() => !mocks.isEnabled); const getAccounts = () => - mocks.accounts.filter( - (account) => isBip44Account(account) && filter(account), + mocks.accounts.filter((account) => + [...mocks.accountsList].includes(account.id), ); mocks.getName.mockImplementation(() => name); @@ -65,6 +85,61 @@ export function setupNamedAccountProvider({ getAccounts().find((account) => account.id === id), ); mocks.createAccounts.mockResolvedValue([]); + mocks.alignAccounts.mockImplementation( + async ({ + entropySource, + groupIndex, + }: { + entropySource: EntropySourceId; + groupIndex: number; + }) => { + if (mocks.isDisabled()) { + // Execute AccountProviderWrapper disabled path to cover line 58 + const wrapperAlign = ( + AccountProviderWrapper.prototype as unknown as { + alignAccounts: ( + this: { isEnabled: boolean }, + opts: { entropySource: EntropySourceId; groupIndex: number }, + ) => Promise<[boolean, string[]]>; + } + ).alignAccounts; + const [disabled, ids] = await wrapperAlign.call( + { isEnabled: false, isDisabled: () => true }, + { entropySource, groupIndex }, + ); + return [disabled, ids]; + } + const createdAccounts = await mocks.createAccounts({ + entropySource, + groupIndex, + }); + // Execute BaseBip44AccountProvider's mapping path to cover 149-154 + const baseAlign = ( + BaseBip44AccountProvider.prototype as unknown as { + alignAccounts: ( + this: { + createAccounts: (o: { + entropySource: EntropySourceId; + groupIndex: number; + }) => Promise; + }, + opts: { entropySource: EntropySourceId; groupIndex: number }, + ) => Promise<[boolean, string[]]>; + } + ).alignAccounts; + const [, ids] = await baseAlign.call( + { createAccounts: async () => createdAccounts }, + { entropySource, groupIndex }, + ); + // Normalize tuple to the expected [disabled=false, ids] + return [false, ids]; + }, + ); + mocks.init.mockImplementation( + (accountIds: Bip44Account['id'][]) => { + accountIds.forEach((id) => mocks.accountsList.add(id)); + }, + ); if (index === 0) { // Make the first provider to always be an `EvmAccountProvider`, since we @@ -72,5 +147,37 @@ export function setupNamedAccountProvider({ Object.setPrototypeOf(mocks, EvmAccountProvider.prototype); } + if (index !== 0) { + Object.setPrototypeOf(mocks, AccountProviderWrapper.prototype); + } + return mocks; } + +/** + * Helper to mock a single createAccounts call while updating the provider's + * internal state so subsequent getAccount/getAccounts can resolve the accounts. + * + * @param provider - The mock provider whose createAccounts call to mock. + * @param created - The accounts to be returned and persisted in the mock state. + */ +export function mockCreateAccountsOnce( + provider: MockAccountProvider, + created: KeyringAccount[], +): void { + provider.createAccounts.mockImplementationOnce(async () => { + // Add newly created accounts to the provider's internal store + for (const acc of created) { + if (!provider.accounts.some((a) => a.id === acc.id)) { + provider.accounts.push(acc); + } + } + // Merge IDs into the visible list used by getAccounts/getAccount + const ids = created.map((a) => a.id); + for (const id of ids) { + provider.accountsList.add(id); + } + + return created; + }); +} diff --git a/packages/multichain-account-service/src/types.ts b/packages/multichain-account-service/src/types.ts index 6c498687fa5..ad2a9f6e6e3 100644 --- a/packages/multichain-account-service/src/types.ts +++ b/packages/multichain-account-service/src/types.ts @@ -9,6 +9,7 @@ import type { AccountsControllerAccountRemovedEvent, AccountsControllerGetAccountAction, AccountsControllerGetAccountByAddressAction, + AccountsControllerGetAccountsAction, AccountsControllerListMultichainAccountsAction, } from '@metamask/accounts-controller'; import type { TraceCallback } from '@metamask/controller-utils'; @@ -16,6 +17,8 @@ import type { ErrorReportingServiceCaptureExceptionAction } from '@metamask/erro import type { KeyringAccount } from '@metamask/keyring-api'; import type { KeyringControllerAddNewKeyringAction, + KeyringControllerCreateNewVaultAndKeychainAction, + KeyringControllerCreateNewVaultAndRestoreAction, KeyringControllerGetKeyringsByTypeAction, KeyringControllerGetStateAction, KeyringControllerStateChangeEvent, @@ -135,6 +138,7 @@ export type MultichainAccountServiceEvents = */ type AllowedActions = | AccountsControllerListMultichainAccountsAction + | AccountsControllerGetAccountsAction | AccountsControllerGetAccountAction | AccountsControllerGetAccountByAddressAction | SnapControllerHandleSnapRequestAction @@ -144,6 +148,8 @@ type AllowedActions = | KeyringControllerAddNewKeyringAction | NetworkControllerGetNetworkClientByIdAction | NetworkControllerFindNetworkClientIdByChainIdAction + | KeyringControllerCreateNewVaultAndKeychainAction + | KeyringControllerCreateNewVaultAndRestoreAction | ErrorReportingServiceCaptureExceptionAction; /** diff --git a/packages/multichain-account-service/src/utils.ts b/packages/multichain-account-service/src/utils.ts index 9c418187f7f..1c446077e18 100644 --- a/packages/multichain-account-service/src/utils.ts +++ b/packages/multichain-account-service/src/utils.ts @@ -1,16 +1,3 @@ -export const toRejectedErrorMessage = ( - prefix: string, - results: PromiseSettledResult[], -) => { - let errorMessage = `${prefix}:`; - for (const r of results) { - if (r.status === 'rejected') { - errorMessage += `\n- ${r.reason}`; - } - } - return errorMessage; -}; - /** * Creates a Sentry error from an error message, an inner error and a context. *