diff --git a/packages/multichain-account-service/CHANGELOG.md b/packages/multichain-account-service/CHANGELOG.md index ceb0e3ec8c4..28f18a501d9 100644 --- a/packages/multichain-account-service/CHANGELOG.md +++ b/packages/multichain-account-service/CHANGELOG.md @@ -11,6 +11,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Bump `@metamask/accounts-controller` from `^39.0.7` to `^39.1.0` ([#9807](https://github.com/MetaMask/core/pull/9807)) +### Fixed + +- Ensure providers are ready before running post-alignment ([#9812](https://github.com/MetaMask/core/pull/9812)) + - This prevents to lock a multichain account wallet if one of its provider is not ready yet to proceed. + - By checking this before locking the multichain account wallet, we prevent potential deadlocks. + ## [13.0.1] ### Changed diff --git a/packages/multichain-account-service/src/MultichainAccountWallet.test.ts b/packages/multichain-account-service/src/MultichainAccountWallet.test.ts index 7cf978cf5fc..40aa3285b97 100644 --- a/packages/multichain-account-service/src/MultichainAccountWallet.test.ts +++ b/packages/multichain-account-service/src/MultichainAccountWallet.test.ts @@ -210,8 +210,9 @@ describe('MultichainAccountWallet', () => { // EVM provider is called during group creation. expect(evmProvider.createAccounts).toHaveBeenCalled(); - // The fire-and-forget alignment acquires the lock as a microtask before the test - // resumes, so by the time we reach here SOL has already been called with the batch API. + // Alignment fires as fire-and-forget, so wait for this. + await waitForOtherProvidersToHaveBeenCalled([solProvider]); + expect(solProvider.createAccounts).toHaveBeenCalledWith({ type: AccountCreationType.Bip44DeriveIndexRange, entropySource: wallet.entropySource, @@ -798,6 +799,62 @@ describe('MultichainAccountWallet', () => { expect(wallet.getMultichainAccountGroup(1)).toBeUndefined(); }); + it('calls ensureReady on non-EVM providers before acquiring the wallet lock in the fire-and-forget alignment path', async () => { + const { wallet, providers } = setup({ + accounts: [[MOCK_WALLET_1_EVM_ACCOUNT], []], + }); + + const [, solProvider] = providers; + const statusAtEnsureReady: string[] = []; + + solProvider.ensureReady.mockImplementation(async () => { + // The wallet lock must NOT be held when ensureReady is called. + statusAtEnsureReady.push(wallet.status); + }); + + await wallet.createMultichainAccountGroups({ from: 0, to: 0 }); + + // Wait for the fire-and-forget alignment to complete. + await waitForOtherProvidersToHaveBeenCalled([solProvider]); + + expect(solProvider.ensureReady).toHaveBeenCalledTimes(1); + expect(statusAtEnsureReady[0]).toBe('ready'); + }); + + it('skips a provider that fails ensureReady but still aligns the others', async () => { + // EVM + two non-EVM providers; SOL fails ensureReady, BTC succeeds. + const { wallet, providers } = setup({ + accounts: [ + [MOCK_WALLET_1_EVM_ACCOUNT], + [], // SOL — will fail ensureReady + [], // BTC — will succeed ensureReady + ], + }); + + const [, solProvider, btcProvider] = providers; + + solProvider.ensureReady.mockRejectedValueOnce( + new Error('Snap platform not ready'), + ); + + // Use a deferred promise as a reliable signal that the BTC alignment ran. + const { promise: btcAligned, resolve: resolveBtcAligned } = + createDeferredPromise(); + btcProvider.createAccounts.mockImplementationOnce(async () => { + resolveBtcAligned(); + return []; + }); + + await wallet.createMultichainAccountGroups({ from: 0, to: 0 }); + + // Wait until BTC alignment has actually run. + await btcAligned; + + // SOL was excluded (ensureReady failed); BTC proceeded normally. + expect(solProvider.createAccounts).not.toHaveBeenCalled(); + expect(btcProvider.createAccounts).toHaveBeenCalled(); + }); + it('logs an error to console when post-alignment fails unexpectedly', async () => { // Group 0 exists for EVM; SOL has no accounts yet (will be aligned). const { wallet, providers, messenger } = setup({ diff --git a/packages/multichain-account-service/src/MultichainAccountWallet.ts b/packages/multichain-account-service/src/MultichainAccountWallet.ts index c7144bced5a..30a39a034d4 100644 --- a/packages/multichain-account-service/src/MultichainAccountWallet.ts +++ b/packages/multichain-account-service/src/MultichainAccountWallet.ts @@ -482,6 +482,43 @@ export class MultichainAccountWallet< }); } + /** + * Calls `ensureReady` on every provider concurrently (best-effort). + * + * Returns the subset of providers that became ready and a list of failure + * messages for the ones that did not, following the same `{ ..., failures }` + * pattern used by {@link MultichainAccountWallet.#buildGroupState}. + * + * @param providers - Providers to check. + * @returns Ready providers and failure messages for those that were not. + */ + async #ensureReadyProviders( + providers: Bip44AccountProvider[], + ): Promise<{ + readyProviders: Bip44AccountProvider[]; + failures: string[]; + }> { + const results = await Promise.allSettled( + providers.map((provider) => provider.ensureReady()), + ); + + const readyProviders: Bip44AccountProvider[] = []; + const failures: string[] = []; + + for (const [i, result] of results.entries()) { + const provider = providers[i]; + if (result.status === 'fulfilled') { + readyProviders.push(provider); + } else { + failures.push( + `[${provider?.getName()}] ${toErrorMessage(result.reason)}`, + ); + } + } + + return { readyProviders, failures }; + } + /** * Align accounts for a range of group indices (non-locking). * @@ -747,10 +784,32 @@ export class MultichainAccountWallet< // been created yet. if (!waitForAllProvidersToFinishCreatingAccounts) { const alignOtherAccounts = async (): Promise => { + // Ensure the Snap platform is ready for each non-EVM provider BEFORE + // acquiring the wallet lock. Without this guard the lock would be held + // while waiting for onboarding to complete, blocking all subsequent + // wallet operations that also need the lock. + // + // This is best-effort: providers that fail to become ready are excluded + // from this round. Explicit alignments triggered later will recover them. + const { readyProviders, failures } = + await this.#ensureReadyProviders(otherProviders); + + if (failures.length) { + const error = failures.reduce( + (message, failure) => `${message}\n- ${failure}`, + 'Some providers are not ready and will be skipped for post-alignment:', + ); + this.#log(`${WARNING_PREFIX} ${error}`); + } + + if (readyProviders.length === 0) { + return; + } + this.#log(`Aligning accounts... (post)`); await this.#withLock('in-progress:alignment', async () => { - await this.#alignAccountsForRange({ from, to }, otherProviders, { + await this.#alignAccountsForRange({ from, to }, readyProviders, { trace: { data: { post: true, // Tag to identify post-alignment traces in analytics. diff --git a/packages/multichain-account-service/src/providers/AccountProviderWrapper.test.ts b/packages/multichain-account-service/src/providers/AccountProviderWrapper.test.ts index 14432405e95..f449f1978a7 100644 --- a/packages/multichain-account-service/src/providers/AccountProviderWrapper.test.ts +++ b/packages/multichain-account-service/src/providers/AccountProviderWrapper.test.ts @@ -18,6 +18,27 @@ function setup(): { } describe('AccountProviderWrapper', () => { + describe('ensureReady', () => { + it('delegates to the inner provider when enabled', async () => { + const { wrapper, innerProvider } = setup(); + const ensureReadySpy = jest.spyOn(innerProvider, 'ensureReady'); + + await wrapper.ensureReady(); + + expect(ensureReadySpy).toHaveBeenCalledTimes(1); + }); + + it('returns immediately without calling the inner provider when disabled', async () => { + const { wrapper, innerProvider } = setup(); + wrapper.setEnabled(false); + const ensureReadySpy = jest.spyOn(innerProvider, 'ensureReady'); + + await wrapper.ensureReady(); + + expect(ensureReadySpy).not.toHaveBeenCalled(); + }); + }); + describe('isAligned', () => { it('returns true unconditionally when the wrapper is disabled', () => { const { wrapper } = setup(); diff --git a/packages/multichain-account-service/src/providers/AccountProviderWrapper.ts b/packages/multichain-account-service/src/providers/AccountProviderWrapper.ts index af9aae270cb..39afdd92d15 100644 --- a/packages/multichain-account-service/src/providers/AccountProviderWrapper.ts +++ b/packages/multichain-account-service/src/providers/AccountProviderWrapper.ts @@ -167,6 +167,19 @@ export class AccountProviderWrapper extends BaseBip44AccountProvider { return this.provider.createAccounts(options); } + /** + * Returns immediately when disabled. Delegates to the wrapped provider otherwise, + * waiting for the underlying platform (e.g. snap runtime) to be ready. + * + * @returns A promise that resolves when the provider is ready to use. + */ + override async ensureReady(): Promise { + if (!this.isEnabled) { + return; + } + await this.provider.ensureReady(); + } + /** * Forwards to the wrapped provider unconditionally, because deletion must run even * when the wrapper is disabled, so that wallet-removal flows can clean up diff --git a/packages/multichain-account-service/src/providers/BaseBip44AccountProvider.ts b/packages/multichain-account-service/src/providers/BaseBip44AccountProvider.ts index 5d2bccf63c1..6554a1229fe 100644 --- a/packages/multichain-account-service/src/providers/BaseBip44AccountProvider.ts +++ b/packages/multichain-account-service/src/providers/BaseBip44AccountProvider.ts @@ -116,6 +116,14 @@ export type Bip44AccountProvider< context: { entropySource: EntropySourceId; groupIndex: number }, accountIds: Account['id'][], ): boolean; + /** + * Ensures the provider is ready before any account operation is attempted: + * - EVM providers return immediately. + * - Snap providers will wait for the Snap platform and keyring to be available. + * + * @returns A promise that resolves when the provider is ready to use. + */ + ensureReady(): Promise; }; export abstract class BaseBip44AccountProvider< @@ -234,6 +242,10 @@ export abstract class BaseBip44AccountProvider< ); } + async ensureReady(): Promise { + // No-op for non-snap providers. + } + abstract get capabilities(): KeyringCapabilities; abstract getName(): string; diff --git a/packages/multichain-account-service/src/providers/SnapAccountProvider.ts b/packages/multichain-account-service/src/providers/SnapAccountProvider.ts index 53d68dc160f..a3181939ebc 100644 --- a/packages/multichain-account-service/src/providers/SnapAccountProvider.ts +++ b/packages/multichain-account-service/src/providers/SnapAccountProvider.ts @@ -151,7 +151,7 @@ export abstract class SnapAccountProvider extends BaseBip44AccountProvider { * @returns A promise that resolves when the Snap is ready. * @throws An error if the Snap could not become ready. */ - async ensureReady(): Promise { + override async ensureReady(): Promise { return this.messenger.call('SnapAccountService:ensureReady', this.snapId); } diff --git a/packages/multichain-account-service/src/tests/providers.ts b/packages/multichain-account-service/src/tests/providers.ts index 676651cff67..4f09c453b29 100644 --- a/packages/multichain-account-service/src/tests/providers.ts +++ b/packages/multichain-account-service/src/tests/providers.ts @@ -31,6 +31,7 @@ export type MockAccountProvider = { isAccountCompatible: jest.Mock; isAligned: jest.Mock; getName: jest.Mock; + ensureReady: jest.Mock; isEnabled: boolean; isDisabled: jest.Mock; setEnabled: jest.Mock; @@ -65,6 +66,7 @@ export function makeMockAccountProvider( isAccountCompatible: jest.fn(), isAligned: jest.fn().mockReturnValue(false), getName: jest.fn(), + ensureReady: jest.fn().mockResolvedValue(undefined), isDisabled: jest.fn(), setEnabled: jest.fn(), isEnabled: true,