diff --git a/packages/accounts-controller/src/AccountsController.ts b/packages/accounts-controller/src/AccountsController.ts index 3ca7bee9817..05c7102aba1 100644 --- a/packages/accounts-controller/src/AccountsController.ts +++ b/packages/accounts-controller/src/AccountsController.ts @@ -124,6 +124,11 @@ export type AccountsControllerGetAccountAction = { handler: AccountsController['getAccount']; }; +export type AccountsControllerGetAccountsAction = { + type: `${typeof controllerName}:getAccounts`; + handler: AccountsController['getAccounts']; +}; + export type AccountsControllerUpdateAccountMetadataAction = { type: `${typeof controllerName}:updateAccountMetadata`; handler: AccountsController['updateAccountMetadata']; @@ -145,6 +150,7 @@ export type AccountsControllerActions = | AccountsControllerGetSelectedAccountAction | AccountsControllerGetNextAvailableAccountNameAction | AccountsControllerGetAccountAction + | AccountsControllerGetAccountsAction | AccountsControllerGetSelectedMultichainAccountAction | AccountsControllerUpdateAccountMetadataAction; @@ -303,6 +309,16 @@ export class AccountsController extends BaseController< return this.state.internalAccounts.accounts[accountId]; } + /** + * Returns the internal account objects for the given account IDs, if they exist. + * + * @param accountIds - The IDs of the accounts to retrieve. + * @returns The internal account objects, or undefined if the account(s) do not exist. + */ + getAccounts(accountIds: string[]): (InternalAccount | undefined)[] { + return accountIds.map((accountId) => this.getAccount(accountId)); + } + /** * Returns an array of all evm internal accounts. * diff --git a/packages/accounts-controller/src/index.ts b/packages/accounts-controller/src/index.ts index 2894b9d6e71..8c75201adcc 100644 --- a/packages/accounts-controller/src/index.ts +++ b/packages/accounts-controller/src/index.ts @@ -13,6 +13,7 @@ export type { AccountsControllerGetAccountByAddressAction, AccountsControllerGetNextAvailableAccountNameAction, AccountsControllerGetAccountAction, + AccountsControllerGetAccountsAction, AccountsControllerUpdateAccountMetadataAction, AllowedActions, AccountsControllerActions, diff --git a/packages/multichain-account-service/src/MultichainAccountGroup.test.ts b/packages/multichain-account-service/src/MultichainAccountGroup.test.ts index 553fc0ba58c..66980eb2e43 100644 --- a/packages/multichain-account-service/src/MultichainAccountGroup.test.ts +++ b/packages/multichain-account-service/src/MultichainAccountGroup.test.ts @@ -199,7 +199,7 @@ describe('MultichainAccount', () => { expect(providers[1].createAccounts).not.toHaveBeenCalled(); }); - it('warns if provider alignment fails', async () => { + it('warns if alignment fails for a single provider', async () => { const groupIndex = 0; const { group, providers, wallet } = setup({ groupIndex, @@ -208,7 +208,7 @@ describe('MultichainAccount', () => { const consoleSpy = jest.spyOn(console, 'warn').mockImplementation(); providers[1].createAccounts.mockRejectedValueOnce( - new Error('Unable to create accounts'), + new Error('Provider 2: Unable to create accounts'), ); await group.alignAccounts(); @@ -219,7 +219,39 @@ describe('MultichainAccount', () => { 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. Provider threw the following error:\n- Error: Provider 2: Unable to create accounts`, + ); + }); + + it('warns if alignment fails for multiple providers', async () => { + const groupIndex = 0; + const { group, providers, wallet } = setup({ + groupIndex, + accounts: [[MOCK_WALLET_1_EVM_ACCOUNT], [], []], + }); + + const consoleSpy = jest.spyOn(console, 'warn').mockImplementation(); + providers[1].createAccounts.mockRejectedValueOnce( + new Error('Provider 2: Unable to create accounts'), + ); + + providers[2].createAccounts.mockRejectedValueOnce( + new Error('Provider 3: Unable to create accounts'), + ) + + await group.align(); + + 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. Providers threw the following errors:\n- Error: Provider 2: Unable to create accounts\n- Error: Provider 3: Unable to create accounts`, ); }); }); diff --git a/packages/multichain-account-service/src/MultichainAccountGroup.ts b/packages/multichain-account-service/src/MultichainAccountGroup.ts index b41c3c38c2d..11e43b88d2e 100644 --- a/packages/multichain-account-service/src/MultichainAccountGroup.ts +++ b/packages/multichain-account-service/src/MultichainAccountGroup.ts @@ -8,10 +8,14 @@ import type { Bip44Account } from '@metamask/account-api'; import type { AccountSelector } from '@metamask/account-api'; import { type KeyringAccount } from '@metamask/keyring-api'; +import type { ServiceState, StateKeys } from './MultichainAccountService'; import type { MultichainAccountWallet } from './MultichainAccountWallet'; -import type { NamedAccountProvider } from './providers'; +import type { BaseBip44AccountProvider } from './providers'; import type { MultichainAccountServiceMessenger } from './types'; +export type GroupState = + ServiceState[StateKeys['entropySource']][StateKeys['groupIndex']]; + /** * A multichain account group that holds multiple accounts. */ @@ -25,21 +29,14 @@ export class MultichainAccountGroup< readonly #groupIndex: number; - readonly #providers: NamedAccountProvider[]; + readonly #providers: BaseBip44AccountProvider[]; - readonly #providerToAccounts: Map< - NamedAccountProvider, - Account['id'][] - >; + readonly #providerToAccounts: Map; - readonly #accountToProvider: Map< - Account['id'], - NamedAccountProvider - >; + readonly #accountToProvider: Map; readonly #messenger: MultichainAccountServiceMessenger; - // eslint-disable-next-line @typescript-eslint/prefer-readonly #initialized = false; constructor({ @@ -50,7 +47,7 @@ export class MultichainAccountGroup< }: { groupIndex: number; wallet: MultichainAccountWallet; - providers: NamedAccountProvider[]; + providers: BaseBip44AccountProvider[]; messenger: MultichainAccountServiceMessenger; }) { this.#id = toMultichainAccountGroupId(wallet.id, groupIndex); @@ -60,44 +57,37 @@ export class MultichainAccountGroup< this.#messenger = messenger; this.#providerToAccounts = new Map(); this.#accountToProvider = new Map(); - - this.sync(); - this.#initialized = true; } /** - * Force multichain account synchronization. + * Initialize the multichain account group and construct the internal representation of accounts. + * + * Note: This method can be called multiple times to update the group state. * - * This can be used if account providers got new accounts that the multichain - * account doesn't know about. + * @param groupState - The group state. */ - sync(): void { - // Clear reverse mapping and re-construct it entirely based on the refreshed - // list of accounts from each providers. - this.#accountToProvider.clear(); - + init(groupState: GroupState) { 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) { + for (const accountId of accountIds) { + this.#accountToProvider.set(accountId, provider); + } + const providerAccounts = this.#providerToAccounts.get(provider); + if (!providerAccounts) { + this.#providerToAccounts.set(provider, accountIds); + } else { + providerAccounts.push(...accountIds); + } + // Add the accounts to the provider's internal list of account IDs + provider.addAccounts(accountIds); } } - // 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, @@ -167,7 +157,8 @@ export class MultichainAccountGroup< // If for some reason we cannot get this account from the provider, it // might means it has been deleted or something, so we just filter it // out. - allAccounts.push(account); + // We cast here because TS cannot infer the type of the account from the provider + allAccounts.push(account as Account); } } } @@ -175,6 +166,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. * @@ -189,7 +189,9 @@ export class MultichainAccountGroup< if (!provider) { return undefined; } - return provider.getAccount(id); + + // We cast here because TS cannot infer the type of the account from the provider + return provider.getAccount(id) as Account; } /** @@ -228,13 +230,33 @@ export class MultichainAccountGroup< groupIndex: this.groupIndex, }); } - return Promise.resolve(); + return Promise.reject(new Error('Already aligned')); }), ); + const groupState = results.reduce((state, result, idx) => { + if (result.status === 'fulfilled') { + state[this.#providers[idx].getName()] = result.value.map( + (account) => account.id, + ); + } + return state; + }, {}); + + // Update group state + this.init(groupState); + if (results.some((result) => result.status === 'rejected')) { + const rejectedResults = results.filter( + (result) => + result.status === 'rejected' && result.reason !== 'Already aligned', + ) as PromiseRejectedResult[]; + const errors = rejectedResults + .map((result) => `- ${result.reason}`) + .join('\n'); + const hasMultipleFailures = rejectedResults.length > 1; console.warn( - `Failed to fully align multichain account group for entropy ID: ${this.wallet.entropySource} and group index: ${this.groupIndex}, some accounts might be missing`, + `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'}:\n${errors}`, ); } } diff --git a/packages/multichain-account-service/src/MultichainAccountService.ts b/packages/multichain-account-service/src/MultichainAccountService.ts index c16be5c8989..c6102f33900 100644 --- a/packages/multichain-account-service/src/MultichainAccountService.ts +++ b/packages/multichain-account-service/src/MultichainAccountService.ts @@ -10,13 +10,14 @@ 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 type { InternalAccount } from '@metamask/keyring-internal-api'; import { areUint8ArraysEqual } from '@metamask/utils'; import type { MultichainAccountGroup } from './MultichainAccountGroup'; import { MultichainAccountWallet } from './MultichainAccountWallet'; import type { + BaseBip44AccountProvider, EvmAccountProviderConfig, - NamedAccountProvider, SolAccountProviderConfig, } from './providers'; import { @@ -34,7 +35,7 @@ export const serviceName = 'MultichainAccountService'; */ export type MultichainAccountServiceOptions = { messenger: MultichainAccountServiceMessenger; - providers?: NamedAccountProvider[]; + providers?: BaseBip44AccountProvider[]; providerConfigs?: { [EvmAccountProvider.NAME]?: EvmAccountProviderConfig; [SolAccountProvider.NAME]?: SolAccountProviderConfig; @@ -47,24 +48,41 @@ type AccountContext> = { 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'][]; + }; + }; +}; + /** * Service to expose multichain accounts capabilities. */ export class MultichainAccountService { readonly #messenger: MultichainAccountServiceMessenger; - readonly #providers: NamedAccountProvider[]; + readonly #providers: BaseBip44AccountProvider[]; readonly #wallets: Map< MultichainAccountWalletId, MultichainAccountWallet> >; - readonly #accountIdToContext: Map< - Bip44Account['id'], - AccountContext> - >; - /** * The name of the service. */ @@ -87,7 +105,6 @@ export class MultichainAccountService { }: MultichainAccountServiceOptions) { this.#messenger = messenger; this.#wallets = new Map(); - this.#accountIdToContext = new Map(); // TODO: Rely on keyring capabilities once the keyring API is used by all keyrings. this.#providers = [ @@ -146,123 +163,89 @@ export class MultichainAccountService { 'MultichainAccountService:createMultichainAccountWallet', (...args) => this.createMultichainAccountWallet(...args), ); - - this.#messenger.subscribe('AccountsController:accountAdded', (account) => - this.#handleOnAccountAdded(account), - ); - this.#messenger.subscribe('AccountsController:accountRemoved', (id) => - this.#handleOnAccountRemoved(id), - ); } /** - * Initialize the service and constructs the internal reprensentation of - * multichain accounts and wallets. + * 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. */ - init(): void { - 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; - - // 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, - }); - } - } + #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; } - #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), + /** + * Construct the service state. + * + * @returns The service state. + */ + #constructServiceState() { + const accounts = this.#messenger.call( + 'AccountsController:listMultichainAccounts', ); - if (!wallet) { - // 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; - } + const serviceState: ServiceState = {}; + const { keyrings } = this.#messenger.call('KeyringController:getState'); - 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(); + // 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] = {}; } - - 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(); + for (const account of accounts) { + const keys = this.#getStateKeys(account); + if (keys) { + serviceState[keys.entropySource][keys.groupIndex][keys.providerName] ??= + []; + // ok to cast here because at this point we know that the account is BIP-44 compatible + serviceState[keys.entropySource][keys.groupIndex][ + keys.providerName + ].push(account.id); } - - // Same here, this account should have been already grouped in that - // multichain account. - this.#accountIdToContext.set(account.id, { - wallet, - group, - }); } + + return serviceState; } - #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; + /** + * Initialize the service and constructs the internal reprensentation of + * multichain accounts and wallets. + */ + init(): void { + this.#wallets.clear(); - wallet.sync(); + const serviceState = this.#constructServiceState(); + 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); } - - // 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> { @@ -277,19 +260,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. * diff --git a/packages/multichain-account-service/src/MultichainAccountWallet.ts b/packages/multichain-account-service/src/MultichainAccountWallet.ts index 9a1ff12261b..d995ca9f2f0 100644 --- a/packages/multichain-account-service/src/MultichainAccountWallet.ts +++ b/packages/multichain-account-service/src/MultichainAccountWallet.ts @@ -19,8 +19,12 @@ import { import { createProjectLogger } from '@metamask/utils'; import { Mutex } from 'async-mutex'; -import { MultichainAccountGroup } from './MultichainAccountGroup'; -import type { NamedAccountProvider } from './providers'; +import { + type GroupState, + MultichainAccountGroup, +} from './MultichainAccountGroup'; +import type { ServiceState, StateKeys } from './MultichainAccountService'; +import type { BaseBip44AccountProvider } from './providers'; import type { MultichainAccountServiceMessenger } from './types'; /** @@ -29,12 +33,17 @@ import type { MultichainAccountServiceMessenger } from './types'; type AccountProviderDiscoveryContext< Account extends Bip44Account, > = { - provider: NamedAccountProvider; + provider: BaseBip44AccountProvider; stopped: boolean; groupIndex: number; accounts: Account[]; }; +type WalletState = ServiceState[StateKeys['entropySource']]; + +// type alias to make clear this state is generated by discovery +type DiscoveredGroupsState = WalletState; + const log = createProjectLogger('multichain-account-service'); /** @@ -49,7 +58,7 @@ export class MultichainAccountWallet< readonly #id: MultichainAccountWalletId; - readonly #providers: NamedAccountProvider[]; + readonly #providers: BaseBip44AccountProvider[]; readonly #entropySource: EntropySourceId; @@ -57,7 +66,6 @@ export class MultichainAccountWallet< readonly #messenger: MultichainAccountServiceMessenger; - // eslint-disable-next-line @typescript-eslint/prefer-readonly #initialized = false; #status: MultichainAccountWalletStatus; @@ -67,7 +75,7 @@ export class MultichainAccountWallet< entropySource, messenger, }: { - providers: NamedAccountProvider[]; + providers: BaseBip44AccountProvider[]; entropySource: EntropySourceId; messenger: MultichainAccountServiceMessenger; }) { @@ -79,63 +87,31 @@ 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 { - 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.#accountGroups.set(entropy.groupIndex, multichainAccount); - } - } + init(walletState: WalletState) { + for (const groupIndex of Object.keys(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, + }); + + group.init(walletState[groupIndex]); + + 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.#accountGroups.delete(groupIndex); - } + if (!this.#initialized) { + this.#initialized = true; + this.#status = 'ready'; } } @@ -298,11 +274,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) { return group; } @@ -315,61 +288,50 @@ export class MultichainAccountWallet< ), ); - // -------------------------------------------------------------------------------- - // 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). - // - // -------------------------------------------------------------------------------- - - // If any of the provider failed to create their accounts, then we consider the - // multichain account group to have failed too. - if (results.some((result) => result.status === 'rejected')) { - // NOTE: Some accounts might still have been created on other account providers. We - // don't rollback them. - const error = `Unable to create multichain account group for index: ${groupIndex}`; - - let warn = `${error}:`; - for (const result of results) { - if (result.status === 'rejected') { - warn += `\n- ${result.reason}`; - } + const everyProviderFailed = results.every( + (result) => result.status === 'rejected', + ); + + const providerFailures = results.reduce((acc, result) => { + if (result.status === 'rejected') { + acc += `\n- ${result.reason}`; } - console.warn(warn); + return acc; + }, ''); - throw new Error(error); + if (everyProviderFailed) { + // 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:${providerFailures}`, + ); + } else if (providerFailures) { + // We warn there's failures on some providers and thus misalignment, but we still create the group + console.warn( + `Unable to create some accounts for group index: ${groupIndex}. Providers threw the following errors:${providerFailures}`, + ); } - // 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, - }); - } + // 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[this.#providers[idx].getName()] = result.value.map( + (account) => account.id, + ); + } + return state; + }, {}); + + group = new MultichainAccountGroup({ + wallet: this, + providers: this.#providers, + groupIndex, + messenger: this.#messenger, + }); - // Register the account to our internal map. - this.#accountGroups.set(groupIndex, group); // `group` cannot be undefined here. + group.init(groupState); + + // Register the account(s) to our internal map. + this.#accountGroups.set(groupIndex, group); if (this.#initialized) { this.#messenger.publish( @@ -443,6 +405,16 @@ 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][providerName] = accountIds; + }; // One serialized loop per provider; all run concurrently const runProviderDiscovery = async ( @@ -459,10 +431,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); @@ -480,6 +452,10 @@ export class MultichainAccountWallet< context.accounts = context.accounts.concat(accounts); + const providerName = context.provider.getName(); + + addDiscoveryResultToState(accounts, providerName, targetGroupIndex); + const nextGroupIndex = targetGroupIndex + 1; context.groupIndex = nextGroupIndex; @@ -500,9 +476,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/BaseBip44AccountProvider.ts b/packages/multichain-account-service/src/providers/BaseBip44AccountProvider.ts index fd3e853d0c2..aade4098762 100644 --- a/packages/multichain-account-service/src/providers/BaseBip44AccountProvider.ts +++ b/packages/multichain-account-service/src/providers/BaseBip44AccountProvider.ts @@ -46,44 +46,58 @@ export type NamedAccountProvider< export abstract class BaseBip44AccountProvider implements NamedAccountProvider { protected readonly messenger: MultichainAccountServiceMessenger; + accounts: Bip44Account['id'][] = []; + 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. + */ + addAccounts(accounts: Bip44Account['id'][]): void { + this.accounts.push(...accounts); + } - return accounts; + /** + * Get the accounts list for the provider. + * + * @returns The accounts list. + */ + #getAccountsList(): Bip44Account['id'][] { + return this.accounts; } + /** + * Get the accounts list for the provider from the AccountsController. + * + * @returns The accounts list. + */ getAccounts(): Bip44Account[] { - return this.#getAccounts(); + const accountsList = this.#getAccountsList(); + const internalAccounts = this.messenger.call( + 'AccountsController:getAccounts', + accountsList, + ); + // we cast here because we know that the accounts are BIP-44 compatible + return internalAccounts as Bip44Account[]; } + /** + * Get the account for the provider. + * + * @param id - The account ID. + * @returns The account. + * @throws If the account is not found. + */ getAccount( id: Bip44Account['id'], ): Bip44Account { - // TODO: Maybe just use a proper find for faster lookup? - const [found] = this.#getAccounts((account) => account.id === id); + const found = this.getAccounts().find((account) => account.id === id); if (!found) { throw new Error(`Unable to find account: ${id}`); diff --git a/packages/multichain-account-service/src/providers/EvmAccountProvider.ts b/packages/multichain-account-service/src/providers/EvmAccountProvider.ts index 50c5e256833..d5dcb7cc013 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 { HdKeyring } from '@metamask/eth-hd-keyring'; import type { EntropySourceId, KeyringAccount } from '@metamask/keyring-api'; import { EthAccountType } from '@metamask/keyring-api'; @@ -92,6 +93,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, @@ -122,6 +145,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, @@ -135,9 +166,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. @@ -149,6 +182,14 @@ export class EvmAccountProvider extends BaseBip44AccountProvider { 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, @@ -239,10 +280,13 @@ export class EvmAccountProvider extends BaseBip44AccountProvider { 'Created account does not match address from group index.', ); + const accountId = this.#getAccountId(address); + const account = this.messenger.call( - 'AccountsController:getAccountByAddress', - address, + 'AccountsController:getAccount', + accountId, ); + assertInternalAccountExists(account); assertIsBip44Account(account); return [account]; diff --git a/packages/multichain-account-service/src/types.ts b/packages/multichain-account-service/src/types.ts index 39372186d6a..e144cf9b136 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 { RestrictedMessenger } from '@metamask/base-controller'; @@ -127,6 +128,7 @@ export type MultichainAccountServiceEvents = */ export type AllowedActions = | AccountsControllerListMultichainAccountsAction + | AccountsControllerGetAccountsAction | AccountsControllerGetAccountAction | AccountsControllerGetAccountByAddressAction | SnapControllerHandleSnapRequestAction