Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions packages/multichain-account-service/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<Account>[],
): Promise<{
readyProviders: Bip44AccountProvider<Account>[];
failures: string[];
}> {
const results = await Promise.allSettled(
providers.map((provider) => provider.ensureReady()),
);

const readyProviders: Bip44AccountProvider<Account>[] = [];
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).
*
Expand Down Expand Up @@ -747,10 +784,32 @@ export class MultichainAccountWallet<
// been created yet.
if (!waitForAllProvidersToFinishCreatingAccounts) {
const alignOtherAccounts = async (): Promise<void> => {
// 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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<void> {
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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<void>;
};

export abstract class BaseBip44AccountProvider<
Expand Down Expand Up @@ -234,6 +242,10 @@ export abstract class BaseBip44AccountProvider<
);
}

async ensureReady(): Promise<void> {
// No-op for non-snap providers.
}

abstract get capabilities(): KeyringCapabilities;

abstract getName(): string;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<void> {
override async ensureReady(): Promise<void> {
return this.messenger.call('SnapAccountService:ensureReady', this.snapId);
}

Expand Down
2 changes: 2 additions & 0 deletions packages/multichain-account-service/src/tests/providers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand Down