diff --git a/packages/multichain-account-service/CHANGELOG.md b/packages/multichain-account-service/CHANGELOG.md index 675a97a942c..4c1f2f9b406 100644 --- a/packages/multichain-account-service/CHANGELOG.md +++ b/packages/multichain-account-service/CHANGELOG.md @@ -32,6 +32,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Prevent wallet's lock by-pass when creating non-EVM account asynchronously ([#7801](https://github.com/MetaMask/core/pull/7801)) - The `waitForAllProvidersToFinishCreatingAccounts` option (when set to `false`) was causing account creation to be asynchronous for non-EVM providers, which was potentially creating accounts after the wallet's internal lock was released. - We now run an internal account alignment operation which locks the wallet properly and runs in the background. +- Wait for Snap keyring in KeyringController before non-EVM account creation ([#8196](https://github.com/MetaMask/core/pull/8196)) + - After wallet reset or restore, the Snap keyring is created lazily (e.g. when `getSnapKeyring()` runs). We now wait for it to appear (via `KeyringController:getState` and `KeyringController:stateChange`) with a timeout, avoiding "Keyring not found" error. ## [7.1.0] diff --git a/packages/multichain-account-service/src/MultichainAccountService.test.ts b/packages/multichain-account-service/src/MultichainAccountService.test.ts index 47b540dc0ae..7d1c01d5a50 100644 --- a/packages/multichain-account-service/src/MultichainAccountService.test.ts +++ b/packages/multichain-account-service/src/MultichainAccountService.test.ts @@ -39,6 +39,7 @@ import { import { MOCK_HD_KEYRING_1, MOCK_HD_KEYRING_2, + MOCK_SNAP_KEYRING, getMultichainAccountServiceMessenger, getRootMessenger, makeMockAccountProvider, @@ -1075,6 +1076,7 @@ describe('MultichainAccountService', () => { it('checks for Snap platform readiness with MultichainAccountService:ensureCanUseSnapPlatform', async () => { const { rootMessenger, spies } = await setup({ accounts: [], + keyrings: [MOCK_HD_KEYRING_1, MOCK_HD_KEYRING_2, MOCK_SNAP_KEYRING], }); await rootMessenger.call( @@ -1435,6 +1437,7 @@ describe('MultichainAccountService', () => { it('delegates Snap platform readiness check to SnapPlatformWatcher (method)', async () => { const { service, spies } = await setup({ accounts: [], + keyrings: [MOCK_HD_KEYRING_1, MOCK_HD_KEYRING_2, MOCK_SNAP_KEYRING], }); await service.ensureCanUseSnapPlatform(); diff --git a/packages/multichain-account-service/src/MultichainAccountService.ts b/packages/multichain-account-service/src/MultichainAccountService.ts index 874a9cdf8a0..75ebd9337f3 100644 --- a/packages/multichain-account-service/src/MultichainAccountService.ts +++ b/packages/multichain-account-service/src/MultichainAccountService.ts @@ -178,6 +178,7 @@ export class MultichainAccountService { this.#watcher = new SnapPlatformWatcher(messenger, { ensureOnboardingComplete, + snapPlatformWaitTimeoutMs: config?.snapWatcher?.timeoutMs, }); this.#messenger.registerMethodActionHandlers( diff --git a/packages/multichain-account-service/src/snaps/SnapPlatformWatcher.test.ts b/packages/multichain-account-service/src/snaps/SnapPlatformWatcher.test.ts index 2c01d5a8f84..e54ad32c0ac 100644 --- a/packages/multichain-account-service/src/snaps/SnapPlatformWatcher.test.ts +++ b/packages/multichain-account-service/src/snaps/SnapPlatformWatcher.test.ts @@ -1,4 +1,5 @@ /* eslint-disable no-void */ +import { KeyringTypes } from '@metamask/keyring-controller'; import { SnapControllerState } from '@metamask/snaps-controllers'; import { createDeferredPromise } from '@metamask/utils'; @@ -26,6 +27,10 @@ function setup( SnapController: { getState: jest.Mock; }; + // eslint-disable-next-line @typescript-eslint/naming-convention + KeyringController: { + getState: jest.Mock<{ keyrings: { type: string }[] }>; + }; }; watcher: SnapPlatformWatcher; } { @@ -33,13 +38,27 @@ function setup( SnapController: { getState: jest.fn(), }, + KeyringController: { + getState: jest.fn(), + }, }; rootMessenger.registerActionHandler( 'SnapController:getState', mocks.SnapController.getState, ); - mocks.SnapController.getState.mockReturnValue({ isReady: false }); + mocks.SnapController.getState.mockReturnValue({ + isReady: false, + } as SnapControllerState); + + rootMessenger.registerActionHandler( + 'KeyringController:getState', + mocks.KeyringController.getState, + ); + // By default, Snap keyring exists so ensureCanUseSnapPlatform can complete (including #waitForSnapKeyring). + mocks.KeyringController.getState.mockReturnValue({ + keyrings: [{ type: KeyringTypes.snap }], + }); const messenger = getMultichainAccountServiceMessenger(rootMessenger); @@ -48,6 +67,46 @@ function setup( return { rootMessenger, messenger, watcher, mocks }; } +/** + * Like setup() but does not create a watcher (avoids leaving a pending platform timeout). + * + * @returns Root messenger, controller messenger, and mocks (no watcher). + */ +function setupWithoutWatcher(): { + rootMessenger: RootMessenger; + messenger: MultichainAccountServiceMessenger; + mocks: { + // eslint-disable-next-line @typescript-eslint/naming-convention + SnapController: { getState: jest.Mock }; + // eslint-disable-next-line @typescript-eslint/naming-convention + KeyringController: { + getState: jest.Mock<{ keyrings: { type: string }[] }>; + }; + }; +} { + const rootMessenger = getRootMessenger(); + const mocks = { + SnapController: { getState: jest.fn() }, + KeyringController: { getState: jest.fn() }, + }; + rootMessenger.registerActionHandler( + 'SnapController:getState', + mocks.SnapController.getState, + ); + mocks.SnapController.getState.mockReturnValue({ + isReady: false, + } as SnapControllerState); + rootMessenger.registerActionHandler( + 'KeyringController:getState', + mocks.KeyringController.getState, + ); + mocks.KeyringController.getState.mockReturnValue({ + keyrings: [{ type: KeyringTypes.snap }], + }); + const messenger = getMultichainAccountServiceMessenger(rootMessenger); + return { rootMessenger, messenger, mocks }; +} + function publishIsReadyState(messenger: RootMessenger, isReady: boolean): void { messenger.publish( 'SnapController:stateChange', @@ -59,15 +118,18 @@ function publishIsReadyState(messenger: RootMessenger, isReady: boolean): void { describe('SnapPlatformWatcher', () => { describe('constructor', () => { it('initializes with isReady as false when not using ensureOnboardingComplete', () => { - const { messenger } = setup(); + jest.useFakeTimers(); + const { messenger } = setupWithoutWatcher(); const watcher = new SnapPlatformWatcher(messenger); expect(watcher).toBeDefined(); expect(watcher.isReady).toBe(false); + jest.useRealTimers(); }); it('still tracks Snap platform state when using ensureOnboardingComplete', () => { - const { messenger } = setup(); + jest.useFakeTimers(); + const { messenger } = setupWithoutWatcher(); const watcher = new SnapPlatformWatcher(messenger, { ensureOnboardingComplete: (): Promise => Promise.resolve(), }); @@ -75,12 +137,13 @@ describe('SnapPlatformWatcher', () => { expect(watcher).toBeDefined(); // isReady reflects SnapController state, not the callback (both are required). expect(watcher.isReady).toBe(false); + jest.useRealTimers(); }); }); describe('ensureCanUsePlatform', () => { it('waits for platform to be ready at least once before resolving', async () => { - const { rootMessenger, messenger } = setup(); + const { rootMessenger, messenger } = setupWithoutWatcher(); const watcher = new SnapPlatformWatcher(messenger); // Start the promise but don't await immediately. @@ -205,7 +268,7 @@ describe('SnapPlatformWatcher', () => { }); it('throws if platform becomes not ready again before the await continuation runs (race guard)', async () => { - const { rootMessenger, messenger } = setup(); + const { rootMessenger, messenger } = setupWithoutWatcher(); const watcher = new SnapPlatformWatcher(messenger); // Start waiting for the platform. @@ -223,7 +286,7 @@ describe('SnapPlatformWatcher', () => { }); it('resolves immediately if platform is already ready', async () => { - const { messenger, mocks } = setup(); + const { messenger, mocks } = setupWithoutWatcher(); // Make the platform ready before creating the watcher. mocks.SnapController.getState.mockReturnValue({ @@ -235,6 +298,172 @@ describe('SnapPlatformWatcher', () => { expect(watcher.isReady).toBe(true); }); + it('resolves when Snap keyring is available (does not throw)', async () => { + const { rootMessenger, watcher, mocks } = setup(); + + publishIsReadyState(rootMessenger, true); + + expect(await watcher.ensureCanUseSnapPlatform()).toBeUndefined(); + + // When keyring exists, getState is used to check for Snap keyring, so we return without throwing. + expect(mocks.KeyringController.getState).toHaveBeenCalled(); + }); + + it('resolves when Snap keyring appears via stateChange (listener path)', async () => { + const rootMessenger = getRootMessenger(); + const mocks = { + SnapController: { getState: jest.fn() }, + KeyringController: { getState: jest.fn() }, + }; + mocks.SnapController.getState.mockReturnValue({ isReady: true }); + mocks.KeyringController.getState.mockReturnValue({ keyrings: [] }); + rootMessenger.registerActionHandler( + 'SnapController:getState', + mocks.SnapController.getState, + ); + rootMessenger.registerActionHandler( + 'KeyringController:getState', + mocks.KeyringController.getState, + ); + const messenger = getMultichainAccountServiceMessenger(rootMessenger); + const subscribeSpy = jest.spyOn(messenger, 'subscribe'); + const watcher = new SnapPlatformWatcher(messenger); + + const ensurePromise = watcher.ensureCanUseSnapPlatform(); + await Promise.resolve(); + await Promise.resolve(); // flush so #waitForSnapKeyring runs and subscribe is called + + expect(subscribeSpy.mock.calls.map((call) => call[0])).toContain( + 'KeyringController:stateChange', + ); + const stateChangeCall = subscribeSpy.mock.calls.find( + (call) => call[0] === 'KeyringController:stateChange', + ); + if (stateChangeCall === undefined) { + throw new Error( + 'KeyringController:stateChange subscribe call not found', + ); + } + const listener = stateChangeCall[1] as (state: { + keyrings: { type: string }[]; + }) => void; + listener({ keyrings: [{ type: KeyringTypes.snap }] }); + + expect(await ensurePromise).toBeUndefined(); + }); + + it('resolves when getState throws but stateChange later delivers Snap keyring (covers #hasSnapKeyring catch path)', async () => { + const rootMessenger = getRootMessenger(); + const mocks = { + SnapController: { getState: jest.fn() }, + KeyringController: { getState: jest.fn() }, + }; + mocks.SnapController.getState.mockReturnValue({ isReady: true }); + mocks.KeyringController.getState.mockImplementation(() => { + throw new Error('KeyringController locked'); + }); + rootMessenger.registerActionHandler( + 'SnapController:getState', + mocks.SnapController.getState, + ); + rootMessenger.registerActionHandler( + 'KeyringController:getState', + mocks.KeyringController.getState, + ); + const messenger = getMultichainAccountServiceMessenger(rootMessenger); + const subscribeSpy = jest.spyOn(messenger, 'subscribe'); + const watcher = new SnapPlatformWatcher(messenger); + + const ensurePromise = watcher.ensureCanUseSnapPlatform(); + await Promise.resolve(); + await Promise.resolve(); + + expect(subscribeSpy.mock.calls.map((call) => call[0])).toContain( + 'KeyringController:stateChange', + ); + const stateChangeCall = subscribeSpy.mock.calls.find( + (call) => call[0] === 'KeyringController:stateChange', + ); + if (stateChangeCall === undefined) { + throw new Error( + 'KeyringController:stateChange subscribe call not found', + ); + } + const listener = stateChangeCall[1] as (state: { + keyrings: { type: string }[]; + }) => void; + listener({ keyrings: [{ type: KeyringTypes.snap }] }); + + expect(await ensurePromise).toBeUndefined(); + }); + + it('rejects with explicit error when Snap keyring does not appear within timeout', async () => { + const { rootMessenger, watcher, mocks } = setup(); + + mocks.KeyringController.getState.mockReturnValue({ keyrings: [] }); + publishIsReadyState(rootMessenger, true); + + jest.useFakeTimers(); + const ensurePromise = watcher.ensureCanUseSnapPlatform(); + // Attach rejection handler before advancing timers to avoid unhandled rejection. + // eslint-disable-next-line jest/valid-expect -- assertion is awaited after advancing timers + const expectRejection = expect(ensurePromise).rejects.toThrow( + 'Snap keyring still not ready. Aborting.', + ); + await Promise.resolve(); + await jest.advanceTimersByTimeAsync(5_000); + jest.useRealTimers(); + + await expectRejection; + }); + + it('rejects when Snap platform does not become ready within timeout', async () => { + const { messenger, mocks } = setupWithoutWatcher(); + mocks.SnapController.getState.mockReturnValue({ + isReady: false, + } as SnapControllerState); + + const watcher = new SnapPlatformWatcher(messenger, { + snapPlatformWaitTimeoutMs: 100, + }); + + jest.useFakeTimers(); + const ensurePromise = watcher.ensureCanUseSnapPlatform(); + // Attach rejection handler before advancing timers to avoid unhandled rejection. + // eslint-disable-next-line jest/valid-expect -- assertion is awaited after advancing timers + const expectRejection = expect(ensurePromise).rejects.toThrow( + 'Snap platform did not become ready in time. Aborting.', + ); + await Promise.resolve(); + await jest.advanceTimersByTimeAsync(100); + jest.useRealTimers(); + + await expectRejection; + }); + + it('uses custom snapPlatformWaitTimeoutMs when provided (keyring wait)', async () => { + const { rootMessenger, messenger, mocks } = setup(); + mocks.KeyringController.getState.mockReturnValue({ keyrings: [] }); + + const watcher = new SnapPlatformWatcher(messenger, { + snapPlatformWaitTimeoutMs: 100, + }); + publishIsReadyState(rootMessenger, true); + + jest.useFakeTimers(); + const ensurePromise = watcher.ensureCanUseSnapPlatform(); + // Attach rejection handler before advancing timers to avoid unhandled rejection. + // eslint-disable-next-line jest/valid-expect -- assertion is awaited after advancing timers + const expectRejection = expect(ensurePromise).rejects.toThrow( + 'Snap keyring still not ready. Aborting.', + ); + await Promise.resolve(); + await jest.advanceTimersByTimeAsync(100); + jest.useRealTimers(); + + await expectRejection; + }); + it('waits for ensureOnboardingComplete first when platform is already ready', async () => { const { rootMessenger, messenger } = setup(); const { promise: onboardingPromise, resolve: resolveOnboarding } = diff --git a/packages/multichain-account-service/src/snaps/SnapPlatformWatcher.ts b/packages/multichain-account-service/src/snaps/SnapPlatformWatcher.ts index baea7452eb2..eaf61668810 100644 --- a/packages/multichain-account-service/src/snaps/SnapPlatformWatcher.ts +++ b/packages/multichain-account-service/src/snaps/SnapPlatformWatcher.ts @@ -1,14 +1,47 @@ +import { KeyringTypes } from '@metamask/keyring-controller'; import { createDeferredPromise, DeferredPromise } from '@metamask/utils'; import { once } from 'lodash'; -import { projectLogger as log } from '../logger'; +import { projectLogger as log, WARNING_PREFIX } from '../logger'; import { MultichainAccountServiceMessenger } from '../types'; +/** Minimal KeyringController state shape needed to detect Snap keyring. */ +type KeyringControllerStateSlice = { + keyrings: { type: KeyringTypes | string }[]; +}; + +/** Default wait for Snap platform or keyring to become ready before rejecting (ms). */ +export const DEFAULT_SNAP_PLATFORM_WAIT_TIMEOUT_MS = 3_000; + +/** Error when Snap platform (SnapController) does not become ready within the timeout. */ +const SNAP_PLATFORM_READY_TIMEOUT_MESSAGE = + 'Snap platform did not become ready in time. Aborting.'; + +/** Error when Snap keyring does not appear within the timeout. */ +const SNAP_KEYRING_TIMEOUT_MESSAGE = 'Snap keyring still not ready. Aborting.'; + +/** + * Returns true if the given KeyringController state slice contains a Snap keyring. + * + * @param state - KeyringController state. + * @returns True if state.keyrings contains a keyring with type KeyringTypes.snap. + */ +function stateHasSnapKeyring(state: KeyringControllerStateSlice): boolean { + return state.keyrings.some((k) => k.type === KeyringTypes.snap); +} + export type SnapPlatformWatcherOptions = { /** * Resolves when onboarding is complete. */ ensureOnboardingComplete?: () => Promise; + /** + * How long to wait for the Snap platform or keyring before rejecting (ms). + * Used for both "platform ready" and "keyring available" waits. + * + * @default DEFAULT_SNAP_PLATFORM_WAIT_TIMEOUT_MS + */ + snapPlatformWaitTimeoutMs?: number; }; export class SnapPlatformWatcher { @@ -16,19 +49,31 @@ export class SnapPlatformWatcher { readonly #ensureOnboardingComplete?: () => Promise; + readonly #snapPlatformWaitTimeoutMs: number; + readonly #isReadyOnce: DeferredPromise; #isReady: boolean; + #platformReadyTimeoutId: ReturnType | undefined; + constructor( messenger: MultichainAccountServiceMessenger, options: SnapPlatformWatcherOptions = {}, ) { this.#messenger = messenger; this.#ensureOnboardingComplete = options.ensureOnboardingComplete; + this.#snapPlatformWaitTimeoutMs = + options.snapPlatformWaitTimeoutMs ?? + DEFAULT_SNAP_PLATFORM_WAIT_TIMEOUT_MS; this.#isReady = false; - this.#isReadyOnce = createDeferredPromise(); + // Timeout in #watch() may reject before anyone calls ensureCanUseSnapPlatform() + // suppress unhandled rejection to avoid the process crash + this.#isReadyOnce = createDeferredPromise({ + suppressUnhandledRejection: true, + }); + this.#platformReadyTimeoutId = undefined; this.#watch(); } @@ -47,6 +92,74 @@ export class SnapPlatformWatcher { if (!this.#isReady) { throw new Error('Snap platform cannot be used now.'); } + + // After a restore/reset, the Snap keyring is created lazily by the client (e.g. when + // getSnapKeyring() is called). Non-EVM account creation needs the keyring to exist, so we + // wait for it here with a timeout to avoid "Keyring not found" errors. + await this.#waitForSnapKeyring(); + } + + /** + * Waits for KeyringController to have a Snap keyring available. + * Checks once, then subscribes to KeyringController:stateChange until the keyring + * appears or the timeout is reached (then throws). + */ + async #waitForSnapKeyring(): Promise { + if (this.#hasSnapKeyring()) { + return; + } + await this.#waitForSnapKeyringViaStateChange(); + } + + /** + * Returns true if KeyringController already has a Snap keyring. + * Logs and returns false on error. + * + * @returns True if a Snap keyring exists, false otherwise or on error. + */ + #hasSnapKeyring(): boolean { + try { + const state = this.#messenger.call( + 'KeyringController:getState', + ) as KeyringControllerStateSlice; + return stateHasSnapKeyring(state); + } catch (error) { + log( + `${WARNING_PREFIX} KeyringController error while waiting for Snap keyring:`, + error, + ); + return false; + } + } + + /** + * Subscribes to KeyringController:stateChange and resolves when a Snap keyring + * appears in state, or rejects with an error after the timeout. + */ + async #waitForSnapKeyringViaStateChange(): Promise { + await new Promise((resolve, reject) => { + const timeoutRef: { id: ReturnType | undefined } = { + id: undefined, + }; + + const listener = (state: KeyringControllerStateSlice): void => { + if (stateHasSnapKeyring(state)) { + clearTimeout(timeoutRef.id); + this.#messenger.unsubscribe( + 'KeyringController:stateChange', + listener, + ); + resolve(); + } + }; + + timeoutRef.id = setTimeout(() => { + this.#messenger.unsubscribe('KeyringController:stateChange', listener); + reject(new Error(SNAP_KEYRING_TIMEOUT_MESSAGE)); + }, this.#snapPlatformWaitTimeoutMs); + + this.#messenger.subscribe('KeyringController:stateChange', listener); + }); } #watch(): void { @@ -56,19 +169,32 @@ export class SnapPlatformWatcher { if (initialState.isReady) { this.#isReady = true; this.#isReadyOnce.resolve(); + return; } - this.#messenger.subscribe( - 'SnapController:stateChange', - (isReady: boolean) => { - this.#isReady = isReady; + const listener = (isReady: boolean): void => { + this.#isReady = isReady; - if (isReady) { - logReadyOnce(); - this.#isReadyOnce.resolve(); + if (isReady) { + if (this.#platformReadyTimeoutId !== undefined) { + clearTimeout(this.#platformReadyTimeoutId); + this.#platformReadyTimeoutId = undefined; } - }, + logReadyOnce(); + this.#isReadyOnce.resolve(); + } + }; + + this.#messenger.subscribe( + 'SnapController:stateChange', + listener, (state) => state.isReady, ); + + this.#platformReadyTimeoutId = setTimeout(() => { + this.#platformReadyTimeoutId = undefined; + this.#messenger.unsubscribe('SnapController:stateChange', listener); + this.#isReadyOnce.reject(new Error(SNAP_PLATFORM_READY_TIMEOUT_MESSAGE)); + }, this.#snapPlatformWaitTimeoutMs); } } diff --git a/packages/multichain-account-service/src/tests/accounts.ts b/packages/multichain-account-service/src/tests/accounts.ts index c7d6238b263..4479a3000ee 100644 --- a/packages/multichain-account-service/src/tests/accounts.ts +++ b/packages/multichain-account-service/src/tests/accounts.ts @@ -72,6 +72,13 @@ export const MOCK_HD_KEYRING_2 = { accounts: ['0x456'], }; +/** Used when tests need ensureCanUseSnapPlatform to resolve (SnapPlatformWatcher waits for Snap keyring). */ +export const MOCK_SNAP_KEYRING = { + type: KeyringTypes.snap, + metadata: { id: 'snap-keyring', name: 'Snap Keyring' }, + accounts: [], +}; + export const MOCK_HD_ACCOUNT_1: Bip44Account = { id: 'mock-id-1', address: '0x123', diff --git a/packages/multichain-account-service/src/types.ts b/packages/multichain-account-service/src/types.ts index 671c5a1d0ed..f540a23c828 100644 --- a/packages/multichain-account-service/src/types.ts +++ b/packages/multichain-account-service/src/types.ts @@ -110,6 +110,17 @@ export type MultichainAccountServiceMessenger = Messenger< MultichainAccountServiceEvents | AllowedEvents >; +/** + * Config for the Snap platform watcher (SnapPlatformWatcher). + */ +export type SnapWatcherConfig = { + /** + * How long to wait for the Snap keyring to appear before rejecting (ms). + */ + timeoutMs?: number; +}; + export type MultichainAccountServiceConfig = { trace?: TraceCallback; + snapWatcher?: SnapWatcherConfig; };