Skip to content

feat(multichain-account-service): add option to decide what to await for when creating group #6759

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mathieuartu merged 5 commits into from
Oct 2, 2025
Merged

feat(multichain-account-service): add option to decide what to await for when creating group #6759

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d686ee9
feat(multichain-account-service): add option to decide what to await …
mathieuartu Sep 30, 2025
fcb0343
fix: update CHANGELOG
mathieuartu Sep 30, 2025
ec1b52e
fix: update CHANGELOG
mathieuartu Sep 30, 2025
403d744
Update packages/multichain-account-service/CHANGELOG.md
mathieuartu Sep 30, 2025
80fdc2c
Merge branch 'main' into feat/multichain-account-group-creation-optio…
mathieuartu Oct 2, 2025
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
5 changes: 5 additions & 0 deletions packages/multichain-account-service/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

### Added

- Add an optional `options` parameter to `MultichainAccountWallet.createMultichainAccountGroup()` ([#6759](https://github.com/MetaMask/core/pull/6759))
- Introduces `options.waitForAllProvidersToFinishCreatingAccounts`, that will make `createMultichainAccountGroup` await either only the EVM provider or all the providers to have created their accounts depending on the value. Defaults to `false` (only awaits for EVM accounts creation by default).

## [1.4.0]

### Changed
Expand Down
25 changes: 24 additions & 1 deletion packages/multichain-account-service/src/MultichainAccountWallet.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,9 +347,32 @@ describe('MultichainAccountWallet', () => {
expect(
await wallet.createMultichainAccountGroup(groupIndex),
).toBeDefined();
await new Promise(process.nextTick);
expect(mockSolProviderError).toHaveBeenCalled();
});

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();
const { wallet, providers } = setup({
accounts: [[mockEvmAccount]], // 1 provider
});
Comment on lines +360 to +362

@fabiobozzo fabiobozzo Oct 1, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This case only uses 1 provider but claims to test "all providers" behavior, do I read that right? Should it use multiple providers to properly test instead?

@ccharly ccharly Oct 2, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does claim that "if any of the provider fails", so the test is ok.

Though, I think we could have used the scenario EVM + Solana provider where Solana fails, that could have been clearer indeed 👍

const [provider] = providers;
provider.createAccounts.mockRejectedValueOnce(
new Error('Unable to create accounts'),
);

await expect(
wallet.createMultichainAccountGroup(groupIndex, {
waitForAllProvidersToFinishCreatingAccounts: true,
}),
).rejects.toThrow(
'Unable to create multichain account group for index: 1',
);
});

@cursor cursor Bot Sep 30, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Multichain Account Creation Tests Fail

The createMultichainAccountGroup tests have two issues related to asynchronous provider account creation. An existing test has a race condition because await new Promise(process.nextTick) was removed, leading to premature assertions for background non-EVM operations. Additionally, a new test for waitForAllProvidersToFinishCreatingAccounts: true is incomplete, as it only uses a single provider and doesn't verify waiting for all providers.

Fix in Cursor Fix in Web

});

describe('createNextMultichainAccountGroup', () => {
Expand Down
108 changes: 76 additions & 32 deletions packages/multichain-account-service/src/MultichainAccountWallet.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,11 +296,22 @@ export class MultichainAccountWallet<
* NOTE: This operation WILL lock the wallet's mutex.
*
* @param groupIndex - The group index to use.
* @throws If any of the account providers fails to create their accounts.
* @param options - Options to configure the account creation.
* @param options.waitForAllProvidersToFinishCreatingAccounts - Whether to wait for all
* account providers to finish creating their accounts before returning. If `false`, only
* the EVM provider will be awaited, while all other providers will create their accounts
* in the background. Defaults to `false`.
* @throws If any of the account providers fails to create their accounts and
* the `waitForAllProvidersToFinishCreatingAccounts` option is set to `true`. If `false`,
* errors from non-EVM providers will be logged but ignored, and only errors from the
* EVM provider will be thrown.
* @returns The multichain account group for this group index.
*/
async createMultichainAccountGroup(
groupIndex: number,
options: {
waitForAllProvidersToFinishCreatingAccounts?: boolean;
} = { waitForAllProvidersToFinishCreatingAccounts: false },
): Promise<MultichainAccountGroup<Account>> {
return await this.#withLock('in-progress:create-accounts', async () => {
const nextGroupIndex = this.getNextGroupIndex();
Expand All @@ -324,41 +335,72 @@ export class MultichainAccountWallet<

this.#log(`Creating new group for index ${groupIndex}...`);

// Extract the EVM provider from the list of providers.
// We will only await the EVM provider to create its accounts, while
// all other providers will be started in the background.
const [evmProvider, ...otherProviders] = this.#providers;
assert(
evmProvider instanceof EvmAccountProvider,
'EVM account provider must be first',
);
if (options?.waitForAllProvidersToFinishCreatingAccounts) {
// Create account with all providers and await them.
const results = await Promise.allSettled(
this.#providers.map((provider) =>
provider.createAccounts({
entropySource: this.#entropySource,
groupIndex,
}),
),
);

// Create account with the EVM provider first and await it.
// If it fails, we don't start creating accounts with other providers.
try {
await evmProvider.createAccounts({
entropySource: this.#entropySource,
groupIndex,
});
} 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);
throw new Error(errorMessage);
}
// 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 message = `${error}:`;
for (const result of results) {
if (result.status === 'rejected') {
message += `\n- ${result.reason}`;
}
}
this.#log(`${WARNING_PREFIX} ${message}`);
console.warn(message);

throw new Error(error);
}
} else {
// Extract the EVM provider from the list of providers.
// We will only await the EVM provider to create its accounts, while
// all other providers will be started in the background.
const [evmProvider, ...otherProviders] = this.#providers;

@fabiobozzo fabiobozzo Oct 1, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does that assume EVM provider is always first? Can't the provider order change in theory?

@fabiobozzo fabiobozzo Oct 1, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

const evmProvider = this.#providers.find(p => p instanceof EvmAccountProvider);
assert(evmProvider, 'EVM account provider not found');
const otherProviders = this.#providers.filter(p => p !== evmProvider);

Maybe overkill 😋

@ccharly ccharly Oct 2, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not needed IMO. We could have an extra assert after we build the list of providers to enforce this precondition (like EVM provider always comes first)

assert(
evmProvider instanceof EvmAccountProvider,
'EVM account provider must be first',
);

// Create account with other providers in the background
otherProviders.forEach((provider) => {
provider
.createAccounts({
// Create account with the EVM provider first and await it.
// If it fails, we don't start creating accounts with other providers.
try {
await evmProvider.createAccounts({
entropySource: this.#entropySource,
groupIndex,
})
.catch((error) => {
// Log errors from background providers but don't fail the operation
const errorMessage = `Could not to create account with provider "${provider.getName()}" for multichain account group index: ${groupIndex}`;
this.#log(`${WARNING_PREFIX} ${errorMessage}:`, error);
});
});
} 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);
throw new Error(errorMessage);
}

// Create account with other providers in the background
otherProviders.forEach((provider) => {
provider
.createAccounts({
Comment on lines +391 to +393

@fabiobozzo fabiobozzo Oct 1, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we track background promises too? Or is it the meaning to ignore failures here and future alignments will fix it? or do I miss something? 🤔

@mathieuartu mathieuartu Oct 2, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes exactly, we willingly ignore failures for providers creating accounts in the background

@ccharly ccharly Oct 2, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that's expected to not throw in that case, mostly because would not be any way to catch them truly

Though, we make sure the EvmAccountProvider is successful.

If any of the other providers fail, then yes we would need an alignment for this (unfortunately, we don't have any state to tell us a group is misaligned for now, and we might need that in the near future IMO)

entropySource: this.#entropySource,
groupIndex,
})
.catch((error) => {
// Log errors from background providers but don't fail the operation
const errorMessage = `Could not to create account with provider "${provider.getName()}" for multichain account group index: ${groupIndex}`;
this.#log(`${WARNING_PREFIX} ${errorMessage}:`, error);
});
});
}

// --------------------------------------------------------------------------------
// READ THIS CAREFULLY:
Expand Down Expand Up @@ -419,7 +461,9 @@ export class MultichainAccountWallet<
async createNextMultichainAccountGroup(): Promise<
MultichainAccountGroup<Account>
> {
return this.createMultichainAccountGroup(this.getNextGroupIndex());
return this.createMultichainAccountGroup(this.getNextGroupIndex(), {

@fabiobozzo fabiobozzo Oct 1, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a breaking change isn't it? Do we have to point that out in changelog? 👀

@ccharly ccharly Oct 2, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last change we made around this method was also altering the behavior a bit, but we did not treat it as a breaking change either.

I think, it could be seen as such, but for now, the only guarantee that we have for the multichain account group is that we ALWAYS HAVE at least an EVM account, and that still holds true with the previous change we made AND this one IMO.

Lastly, for simplicity, I would like to avoid breaking change in those packages to avoid bubbling up to other dependents controllers (we need this fix to be merged ASAP).

If we change our mind, we'll make a new 2.0.0 and amend the changelog if really needed later!

waitForAllProvidersToFinishCreatingAccounts: true,
});
}

/**
Expand Down
Loading