diff --git a/eslint-suppressions.json b/eslint-suppressions.json index 56c4e077b14..a953773a603 100644 --- a/eslint-suppressions.json +++ b/eslint-suppressions.json @@ -1023,9 +1023,6 @@ "packages/multichain-account-service/src/providers/SolAccountProvider.ts": { "@typescript-eslint/naming-convention": { "count": 2 - }, - "id-length": { - "count": 1 } }, "packages/multichain-account-service/src/providers/TrxAccountProvider.ts": { diff --git a/packages/multichain-account-service/CHANGELOG.md b/packages/multichain-account-service/CHANGELOG.md index a6d0627d32d..46a01ac4cab 100644 --- a/packages/multichain-account-service/CHANGELOG.md +++ b/packages/multichain-account-service/CHANGELOG.md @@ -13,6 +13,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- Optimize `SolAccountProvider.createAccounts` for range operations ([#8131](https://github.com/MetaMask/core/pull/8131)) + - Batch account creation with the new `SnapKeyring.createAccounts` method. + - Significantly reduces lock acquisitions and API calls for batch operations. - Optimize `EvmAccountProvider.createAccounts` for range operations ([#7801](https://github.com/MetaMask/core/pull/7801)) - Batch account creation with single a `withKeyring` call for entire range instead of one call per account. - Batch account creation with single `keyring.addAccounts` call. diff --git a/packages/multichain-account-service/src/providers/BtcAccountProvider.test.ts b/packages/multichain-account-service/src/providers/BtcAccountProvider.test.ts index 723e24968d3..6632171c3aa 100644 --- a/packages/multichain-account-service/src/providers/BtcAccountProvider.test.ts +++ b/packages/multichain-account-service/src/providers/BtcAccountProvider.test.ts @@ -7,6 +7,7 @@ import type { InternalAccount, } from '@metamask/keyring-internal-api'; import { SnapControllerState } from '@metamask/snaps-controllers'; +import deepmerge from 'deepmerge'; import { AccountProviderWrapper } from './AccountProviderWrapper'; import { @@ -14,7 +15,7 @@ import { BTC_ACCOUNT_PROVIDER_NAME, BtcAccountProvider, } from './BtcAccountProvider'; -import { SnapAccountProviderConfig } from './SnapAccountProvider'; +import type { SnapAccountProviderConfig } from './SnapAccountProvider'; import { TraceName } from '../constants/traces'; import { getMultichainAccountServiceMessenger, @@ -25,8 +26,18 @@ import { MOCK_HD_ACCOUNT_1, MOCK_HD_KEYRING_1, MockAccountBuilder, + toGroupIndexRangeArray, } from '../tests'; -import type { RootMessenger } from '../tests'; +import type { RootMessenger, DeepPartial } from '../tests'; + +function asConfig( + partial: DeepPartial, +): SnapAccountProviderConfig { + return deepmerge( + BTC_ACCOUNT_PROVIDER_DEFAULT_CONFIG, + partial, + ) as SnapAccountProviderConfig; +} class MockBtcKeyring { readonly type = 'MockBtcKeyring'; @@ -98,7 +109,37 @@ class MockBtcKeyring { return account; }); + + createAccounts: SnapKeyring['createAccounts'] = jest + .fn() + .mockImplementation((_, options) => { + const groupIndices = + options.type === 'bip44:derive-index' + ? [options.groupIndex] + : toGroupIndexRangeArray(options.range); + + return groupIndices.map((groupIndex) => { + const found = this.accounts.find( + (account) => + isBip44Account(account) && + account.options.entropy.groupIndex === groupIndex, + ); + + if (found) { + return found; // Idempotent. + } + + const account = MockAccountBuilder.from(MOCK_BTC_P2WPKH_ACCOUNT_1) + .withUuid() + .withAddressSuffix(`${groupIndex}`) + .withGroupIndex(groupIndex) + .get(); + this.accounts.push(account); + return account; + }); + }); } + class MockBtcAccountProvider extends BtcAccountProvider { override async ensureCanUseSnapPlatform(): Promise { // Override to avoid waiting during tests. @@ -130,6 +171,7 @@ function setup({ handleRequest: jest.Mock; keyring: { createAccount: jest.Mock; + createAccounts: jest.Mock; }; }; } { @@ -193,6 +235,7 @@ function setup({ handleRequest: mockHandleRequest, keyring: { createAccount: keyring.createAccount as jest.Mock, + createAccounts: keyring.createAccounts as jest.Mock, }, }, }; @@ -250,147 +293,298 @@ describe('BtcAccountProvider', () => { expect(provider.isAccountCompatible(account)).toBe(false); }); - it('creates accounts', async () => { - const accounts = [MOCK_BTC_P2WPKH_ACCOUNT_1]; - const { provider, keyring } = setup({ - accounts, - }); + describe('v1', () => { + it('uses v1 by default when no config is provided', async () => { + const accounts = [MOCK_BTC_P2WPKH_ACCOUNT_1]; + const { provider, mocks } = setup({ + accounts, + // Force v1 by not providing the config at all, so it relies on the default value. + config: asConfig({ createAccounts: { v2: undefined } }), + }); - const newGroupIndex = accounts.length; // Group-index are 0-based. - const newAccounts = await provider.createAccounts({ - type: AccountCreationType.Bip44DeriveIndex, - entropySource: MOCK_HD_KEYRING_1.metadata.id, - groupIndex: newGroupIndex, - }); - expect(newAccounts).toHaveLength(1); - expect(keyring.createAccount).toHaveBeenCalled(); - }); + await provider.createAccounts({ + type: AccountCreationType.Bip44DeriveIndex, + entropySource: MOCK_HD_KEYRING_1.metadata.id, + groupIndex: accounts.length, + }); - it('does not re-create accounts (idempotent)', async () => { - const accounts = [MOCK_BTC_P2WPKH_ACCOUNT_1]; - const { provider } = setup({ - accounts, + expect(mocks.keyring.createAccount).toHaveBeenCalled(); + expect(mocks.keyring.createAccounts).not.toHaveBeenCalled(); }); - const newAccounts = await provider.createAccounts({ - type: AccountCreationType.Bip44DeriveIndex, - entropySource: MOCK_HD_KEYRING_1.metadata.id, - groupIndex: 0, - }); - expect(newAccounts).toHaveLength(1); - expect(newAccounts[0]).toStrictEqual(MOCK_BTC_P2WPKH_ACCOUNT_1); - }); + it('creates accounts', async () => { + const accounts = [MOCK_BTC_P2WPKH_ACCOUNT_1]; + const { provider, mocks } = setup({ accounts }); - it('creates multiple accounts using Bip44DeriveIndexRange', async () => { - const accounts = [MOCK_BTC_P2WPKH_ACCOUNT_1]; - const { provider, keyring } = setup({ - accounts, + const newGroupIndex = accounts.length; // Group-index are 0-based. + const newAccounts = await provider.createAccounts({ + type: AccountCreationType.Bip44DeriveIndex, + entropySource: MOCK_HD_KEYRING_1.metadata.id, + groupIndex: newGroupIndex, + }); + expect(newAccounts).toHaveLength(1); + expect(mocks.keyring.createAccount).toHaveBeenCalled(); + expect(mocks.keyring.createAccounts).not.toHaveBeenCalled(); }); - const from = 1; - const newAccounts = await provider.createAccounts({ - type: AccountCreationType.Bip44DeriveIndexRange, - entropySource: MOCK_HD_KEYRING_1.metadata.id, - range: { - from, - to: 3, - }, + it('does not re-create accounts (idempotent)', async () => { + const accounts = [MOCK_BTC_P2WPKH_ACCOUNT_1]; + const { provider } = setup({ accounts }); + + const newAccounts = await provider.createAccounts({ + type: AccountCreationType.Bip44DeriveIndex, + entropySource: MOCK_HD_KEYRING_1.metadata.id, + groupIndex: 0, + }); + expect(newAccounts).toHaveLength(1); + expect(newAccounts[0]).toStrictEqual(MOCK_BTC_P2WPKH_ACCOUNT_1); }); - expect(newAccounts).toHaveLength(3); - expect(keyring.createAccount).toHaveBeenCalledTimes(3); + it('creates multiple accounts using Bip44DeriveIndexRange', async () => { + const accounts = [MOCK_BTC_P2WPKH_ACCOUNT_1]; + const { provider, mocks } = setup({ accounts }); - // Verify each account has the correct group index. - for (const [index, account] of newAccounts.entries()) { - expect(isBip44Account(account)).toBe(true); - expect(account.options.entropy.groupIndex).toBe(from + index); - } - }); + const from = 1; + const newAccounts = await provider.createAccounts({ + type: AccountCreationType.Bip44DeriveIndexRange, + entropySource: MOCK_HD_KEYRING_1.metadata.id, + range: { from, to: 3 }, + }); - it('creates accounts with range starting from 0', async () => { - const { provider, keyring } = setup({ - accounts: [], + expect(newAccounts).toHaveLength(3); + expect(mocks.keyring.createAccount).toHaveBeenCalledTimes(3); + expect(mocks.keyring.createAccounts).not.toHaveBeenCalled(); + + // Verify each account has the correct group index. + for (const [index, account] of newAccounts.entries()) { + expect(isBip44Account(account)).toBe(true); + expect(account.options.entropy.groupIndex).toBe(from + index); + } }); - const newAccounts = await provider.createAccounts({ - type: AccountCreationType.Bip44DeriveIndexRange, - entropySource: MOCK_HD_KEYRING_1.metadata.id, - range: { - from: 0, - to: 2, - }, + it('creates accounts with range starting from 0', async () => { + const { provider, mocks } = setup({ accounts: [] }); + + const newAccounts = await provider.createAccounts({ + type: AccountCreationType.Bip44DeriveIndexRange, + entropySource: MOCK_HD_KEYRING_1.metadata.id, + range: { from: 0, to: 2 }, + }); + + expect(newAccounts).toHaveLength(3); + expect(mocks.keyring.createAccount).toHaveBeenCalledTimes(3); + expect(mocks.keyring.createAccounts).not.toHaveBeenCalled(); }); - expect(newAccounts).toHaveLength(3); - expect(keyring.createAccount).toHaveBeenCalledTimes(3); - }); + it('creates a single account when range from equals to', async () => { + const { provider, mocks } = setup({ accounts: [] }); - it('creates a single account when range from equals to', async () => { - const { provider, keyring } = setup({ - accounts: [], + const newAccounts = await provider.createAccounts({ + type: AccountCreationType.Bip44DeriveIndexRange, + entropySource: MOCK_HD_KEYRING_1.metadata.id, + range: { from: 5, to: 5 }, + }); + + expect(newAccounts).toHaveLength(1); + expect(mocks.keyring.createAccount).toHaveBeenCalledTimes(1); + expect(mocks.keyring.createAccounts).not.toHaveBeenCalled(); + expect( + isBip44Account(newAccounts[0]) && + newAccounts[0].options.entropy.groupIndex, + ).toBe(5); }); - const newAccounts = await provider.createAccounts({ - type: AccountCreationType.Bip44DeriveIndexRange, - entropySource: MOCK_HD_KEYRING_1.metadata.id, - range: { - from: 5, - to: 5, - }, + it('throws if the account creation process takes too long', async () => { + const { provider, mocks } = setup({ accounts: [] }); + + mocks.keyring.createAccount.mockImplementation(() => { + return new Promise((resolve) => { + setTimeout(() => { + resolve(MOCK_BTC_P2TR_ACCOUNT_1); + }, 4000); + }); + }); + + await expect( + provider.createAccounts({ + type: AccountCreationType.Bip44DeriveIndex, + entropySource: MOCK_HD_KEYRING_1.metadata.id, + groupIndex: 0, + }), + ).rejects.toThrow('Timed out'); }); - expect(newAccounts).toHaveLength(1); - expect(keyring.createAccount).toHaveBeenCalledTimes(1); - expect( - isBip44Account(newAccounts[0]) && - newAccounts[0].options.entropy.groupIndex, - ).toBe(5); - }); + // Skip this test for now, since we manually inject those options upon + // account creation, so it cannot fails (until the Bitcoin Snap starts + // using the new typed options). + // eslint-disable-next-line jest/no-disabled-tests + it.skip('throws if the created account is not BIP-44 compatible', async () => { + const accounts = [MOCK_BTC_P2TR_ACCOUNT_1]; + const { provider, mocks } = setup({ + accounts, + }); - it('throws if the account creation process takes too long', async () => { - const { provider, mocks } = setup({ - accounts: [], + mocks.keyring.createAccount.mockResolvedValue({ + ...MOCK_BTC_P2TR_ACCOUNT_1, + options: {}, // No options, so it cannot be BIP-44 compatible. + }); + + await expect( + provider.createAccounts({ + type: AccountCreationType.Bip44DeriveIndex, + entropySource: MOCK_HD_KEYRING_1.metadata.id, + groupIndex: 0, + }), + ).rejects.toThrow('Created account is not BIP-44 compatible'); }); - mocks.keyring.createAccount.mockImplementation(() => { - return new Promise((resolve) => { - setTimeout(() => { - resolve(MOCK_BTC_P2TR_ACCOUNT_1); - }, 4000); + it('discover accounts at a new group index creates an account', async () => { + const { provider, mocks } = setup({ accounts: [] }); + + // Simulate one discovered account at the requested index. + mocks.handleRequest.mockReturnValue([MOCK_BTC_P2TR_DISCOVERED_ACCOUNT_1]); + + const discovered = await provider.discoverAccounts({ + entropySource: MOCK_HD_KEYRING_1.metadata.id, + groupIndex: 0, }); + + expect(discovered).toHaveLength(1); + // Ensure we did go through creation path + expect(mocks.keyring.createAccount).toHaveBeenCalled(); + // Provider should now expose one account (newly created) + expect(provider.getAccounts()).toHaveLength(1); }); + }); - await expect( - provider.createAccounts({ + describe('v2', () => { + it('creates accounts', async () => { + const accounts = [MOCK_BTC_P2WPKH_ACCOUNT_1]; + const { provider, mocks } = setup({ + accounts, + config: asConfig({ createAccounts: { v2: true } }), + }); + + const newGroupIndex = accounts.length; // Group-index are 0-based. + const newAccounts = await provider.createAccounts({ type: AccountCreationType.Bip44DeriveIndex, + entropySource: MOCK_HD_KEYRING_1.metadata.id, + groupIndex: newGroupIndex, + }); + expect(newAccounts).toHaveLength(1); + // Batch endpoint must be called, NOT the singular one. + expect(mocks.keyring.createAccounts).toHaveBeenCalledWith( + BtcAccountProvider.BTC_SNAP_ID, + { + type: AccountCreationType.Bip44DeriveIndex, + entropySource: MOCK_HD_KEYRING_1.metadata.id, + groupIndex: newGroupIndex, + }, + ); + expect(mocks.keyring.createAccount).not.toHaveBeenCalled(); + }); + + it('does not re-create accounts (idempotent)', async () => { + const accounts = [MOCK_BTC_P2WPKH_ACCOUNT_1]; + const { provider } = setup({ + accounts, + config: asConfig({ createAccounts: { v2: true } }), + }); + + const newAccounts = await provider.createAccounts({ entropySource: MOCK_HD_KEYRING_1.metadata.id, groupIndex: 0, - }), - ).rejects.toThrow('Timed out'); - }); + type: AccountCreationType.Bip44DeriveIndex, + }); + expect(newAccounts).toHaveLength(1); + expect(newAccounts[0]).toStrictEqual(MOCK_BTC_P2WPKH_ACCOUNT_1); + }); - // Skip this test for now, since we manually inject those options upon - // account creation, so it cannot fails (until the Bitcoin Snap starts - // using the new typed options). - // eslint-disable-next-line jest/no-disabled-tests - it.skip('throws if the created account is not BIP-44 compatible', async () => { - const accounts = [MOCK_BTC_P2TR_ACCOUNT_1]; - const { provider, mocks } = setup({ - accounts, + it('creates multiple accounts using Bip44DeriveIndexRange', async () => { + const accounts = [MOCK_BTC_P2WPKH_ACCOUNT_1]; + const { provider, mocks } = setup({ + accounts, + config: asConfig({ createAccounts: { v2: true } }), + }); + + const from = 1; + const newAccounts = await provider.createAccounts({ + type: AccountCreationType.Bip44DeriveIndexRange, + entropySource: MOCK_HD_KEYRING_1.metadata.id, + range: { from, to: 3 }, + }); + + expect(newAccounts).toHaveLength(3); + // Single batch call, NOT three individual calls. + expect(mocks.keyring.createAccounts).toHaveBeenCalledTimes(1); + expect(mocks.keyring.createAccount).not.toHaveBeenCalled(); + + // Verify each account has the correct group index. + for (const [index, account] of newAccounts.entries()) { + expect(isBip44Account(account)).toBe(true); + expect(account.options.entropy.groupIndex).toBe(from + index); + } }); - mocks.keyring.createAccount.mockResolvedValue({ - ...MOCK_BTC_P2TR_ACCOUNT_1, - options: {}, // No options, so it cannot be BIP-44 compatible. + it('creates accounts with range starting from 0', async () => { + const { provider, mocks } = setup({ + accounts: [], + config: asConfig({ createAccounts: { v2: true } }), + }); + + const newAccounts = await provider.createAccounts({ + type: AccountCreationType.Bip44DeriveIndexRange, + entropySource: MOCK_HD_KEYRING_1.metadata.id, + range: { from: 0, to: 2 }, + }); + + expect(newAccounts).toHaveLength(3); + expect(mocks.keyring.createAccounts).toHaveBeenCalledTimes(1); + expect(mocks.keyring.createAccount).not.toHaveBeenCalled(); }); - await expect( - provider.createAccounts({ - type: AccountCreationType.Bip44DeriveIndex, + it('creates a single account when range from equals to', async () => { + const { provider, mocks } = setup({ + accounts: [], + config: asConfig({ createAccounts: { v2: true } }), + }); + + const newAccounts = await provider.createAccounts({ + type: AccountCreationType.Bip44DeriveIndexRange, entropySource: MOCK_HD_KEYRING_1.metadata.id, - groupIndex: 0, - }), - ).rejects.toThrow('Created account is not BIP-44 compatible'); + range: { from: 5, to: 5 }, + }); + + expect(newAccounts).toHaveLength(1); + expect(mocks.keyring.createAccounts).toHaveBeenCalledTimes(1); + expect(mocks.keyring.createAccount).not.toHaveBeenCalled(); + expect( + isBip44Account(newAccounts[0]) && + newAccounts[0].options.entropy.groupIndex, + ).toBe(5); + }); + + it('throws if the account creation process takes too long', async () => { + const { provider, mocks } = setup({ + accounts: [], + config: asConfig({ createAccounts: { v2: true } }), + }); + + mocks.keyring.createAccounts.mockImplementation( + () => + new Promise((resolve) => { + setTimeout(() => resolve([MOCK_BTC_P2WPKH_ACCOUNT_1]), 4000); + }), + ); + + await expect( + provider.createAccounts({ + type: AccountCreationType.Bip44DeriveIndex, + entropySource: MOCK_HD_KEYRING_1.metadata.id, + groupIndex: 0, + }), + ).rejects.toThrow('Timed out'); + }); }); it('throws an error when type is not "bip44:derive-index"', async () => { @@ -408,26 +602,6 @@ describe('BtcAccountProvider', () => { ); }); - it('discover accounts at a new group index creates an account', async () => { - const { provider, mocks } = setup({ - accounts: [], - }); - - // Simulate one discovered account at the requested index. - mocks.handleRequest.mockReturnValue([MOCK_BTC_P2TR_DISCOVERED_ACCOUNT_1]); - - const discovered = await provider.discoverAccounts({ - entropySource: MOCK_HD_KEYRING_1.metadata.id, - groupIndex: 0, - }); - - expect(discovered).toHaveLength(1); - // Ensure we did go through creation path - expect(mocks.keyring.createAccount).toHaveBeenCalled(); - // Provider should now expose one account (newly created) - expect(provider.getAccounts()).toHaveLength(1); - }); - it('returns existing account if it already exists at index', async () => { const { provider, mocks } = setup({ accounts: [MOCK_BTC_P2WPKH_ACCOUNT_1], @@ -462,13 +636,11 @@ describe('BtcAccountProvider', () => { it('does not run discovery if disabled', async () => { const { provider } = setup({ accounts: [MOCK_BTC_P2WPKH_ACCOUNT_1], - config: { - ...BTC_ACCOUNT_PROVIDER_DEFAULT_CONFIG, + config: asConfig({ discovery: { - ...BTC_ACCOUNT_PROVIDER_DEFAULT_CONFIG.discovery, enabled: false, }, - }, + }), }); expect( diff --git a/packages/multichain-account-service/src/providers/BtcAccountProvider.ts b/packages/multichain-account-service/src/providers/BtcAccountProvider.ts index 1c17a171c6b..9301ebb58f1 100644 --- a/packages/multichain-account-service/src/providers/BtcAccountProvider.ts +++ b/packages/multichain-account-service/src/providers/BtcAccountProvider.ts @@ -2,6 +2,8 @@ import { assertIsBip44Account } from '@metamask/account-api'; import type { Bip44Account } from '@metamask/account-api'; import type { TraceCallback } from '@metamask/controller-utils'; import type { + CreateAccountBip44DeriveIndexOptions, + CreateAccountBip44DeriveIndexRangeOptions, CreateAccountOptions, EntropySourceId, KeyringAccount, @@ -33,6 +35,7 @@ export const BTC_ACCOUNT_PROVIDER_NAME = 'Bitcoin'; export const BTC_ACCOUNT_PROVIDER_DEFAULT_CONFIG: BtcAccountProviderConfig = { maxConcurrency: 3, createAccounts: { + v2: false, // For now, the Snap is not fully v2 compliant. timeoutMs: 3000, }, discovery: { @@ -75,29 +78,80 @@ export class BtcAccountProvider extends SnapAccountProvider { ); } - async #createAccounts({ - keyring, - entropySource, - groupIndex: index, - }: { - keyring: RestrictedSnapKeyring; - entropySource: EntropySourceId; - groupIndex: number; - }): Promise[]> { + async #createAccounts( + keyring: RestrictedSnapKeyring, + options: + | CreateAccountBip44DeriveIndexOptions + | CreateAccountBip44DeriveIndexRangeOptions, + ): Promise[]> { return this.withMaxConcurrency(async () => { - const account = await withTimeout( - keyring.createAccount({ - entropySource, - index, - addressType: BtcAccountType.P2wpkh, - scope: BtcScope.Mainnet, - }), - this.config.createAccounts.timeoutMs, - ); + let snapAccounts: KeyringAccount[] = []; + + const v2 = this.config.createAccounts.v2 ?? false; + + const { entropySource } = options; - assertIsBip44Account(account); - this.accounts.add(account.id); - return [account]; + if (options.type === `${AccountCreationType.Bip44DeriveIndexRange}`) { + if (v2) { + // Batch account creations. + snapAccounts = await withTimeout( + keyring.createAccounts(options), + this.config.createAccounts.timeoutMs, + ); + } else { + const { range } = options; + + // Create accounts one by one. + for ( + let groupIndex = range.from; + groupIndex <= range.to; + groupIndex++ + ) { + const snapAccount = await withTimeout( + keyring.createAccount({ + entropySource, + index: groupIndex, + addressType: BtcAccountType.P2wpkh, + scope: BtcScope.Mainnet, + }), + this.config.createAccounts.timeoutMs, + ); + + snapAccounts.push(snapAccount); + } + } + } else if (v2) { + // Create account using new v2-like flow (no async flow + no Snap keyring events). + const [snapAccount] = await withTimeout( + keyring.createAccounts(options), + this.config.createAccounts.timeoutMs, + ); + + snapAccounts = [snapAccount]; + } else { + const { groupIndex } = options; + + // Create account using the existing v1 flow. + const snapAccount = await withTimeout( + keyring.createAccount({ + entropySource, + index: groupIndex, + addressType: BtcAccountType.P2wpkh, + scope: BtcScope.Mainnet, + }), + this.config.createAccounts.timeoutMs, + ); + + snapAccounts = [snapAccount]; + } + + const accounts: Bip44Account[] = []; + for (const snapAccount of snapAccounts) { + assertIsBip44Account(snapAccount); + this.accounts.add(snapAccount.id); + accounts.push(snapAccount); + } + return accounts; }); } @@ -109,36 +163,8 @@ export class BtcAccountProvider extends SnapAccountProvider { `${AccountCreationType.Bip44DeriveIndexRange}`, ]); - const { entropySource } = options; - - if (options.type === AccountCreationType.Bip44DeriveIndexRange) { - const { range } = options; - return this.withSnap(async ({ keyring }) => { - const accounts: Bip44Account[] = []; - - // TODO: Use `SnapKeyring.createAccounts` when that functionality is implemented on the Snap - // itself, instead of creating accounts one by one. - for ( - let groupIndex = range.from; - groupIndex <= range.to; - groupIndex++ - ) { - const createdAccounts = await this.#createAccounts({ - keyring, - entropySource, - groupIndex, - }); - accounts.push(...createdAccounts); - } - - return accounts; - }); - } - - const { groupIndex } = options; - return this.withSnap(async ({ keyring }) => { - return this.#createAccounts({ keyring, entropySource, groupIndex }); + return this.#createAccounts(keyring, options); }); } @@ -185,13 +211,11 @@ export class BtcAccountProvider extends SnapAccountProvider { return []; } - const createdAccounts = await this.#createAccounts({ - keyring, + return await this.#createAccounts(keyring, { + type: AccountCreationType.Bip44DeriveIndex, entropySource, groupIndex, }); - - return createdAccounts; }, ); }); diff --git a/packages/multichain-account-service/src/providers/SnapAccountProvider.test.ts b/packages/multichain-account-service/src/providers/SnapAccountProvider.test.ts index 4c987169b29..ccd44702190 100644 --- a/packages/multichain-account-service/src/providers/SnapAccountProvider.test.ts +++ b/packages/multichain-account-service/src/providers/SnapAccountProvider.test.ts @@ -213,6 +213,7 @@ const setup = ({ const keyring = { createAccount: jest.fn(), + createAccounts: jest.fn(), removeAccount: jest.fn(), }; diff --git a/packages/multichain-account-service/src/providers/SnapAccountProvider.ts b/packages/multichain-account-service/src/providers/SnapAccountProvider.ts index dba319101aa..ac65d774077 100644 --- a/packages/multichain-account-service/src/providers/SnapAccountProvider.ts +++ b/packages/multichain-account-service/src/providers/SnapAccountProvider.ts @@ -22,6 +22,7 @@ import { createSentryError } from '../utils'; export type RestrictedSnapKeyring = { createAccount: (options: Record) => Promise; + createAccounts: (options: CreateAccountOptions) => Promise; removeAccount: (address: string) => Promise; }; @@ -34,6 +35,7 @@ export type SnapAccountProviderConfig = { backOffMs: number; }; createAccounts: { + v2: boolean; timeoutMs: number; }; }; @@ -122,9 +124,13 @@ export abstract class SnapAccountProvider extends BaseBip44AccountProvider { // Also, creating account that way won't invalidate the Snap keyring state. The // account will get created and persisted properly with the Snap account creation // flow "asynchronously" (with `notify:accountCreated`). - const createAccount = await this.#withSnapKeyring< - SnapKeyring['createAccount'] - >(async ({ keyring }) => keyring.createAccount.bind(keyring)); + const { createAccount, createAccounts } = await this.#withSnapKeyring<{ + createAccount: SnapKeyring['createAccount']; + createAccounts: SnapKeyring['createAccounts']; + }>(async ({ keyring }) => ({ + createAccount: keyring.createAccount.bind(keyring), + createAccounts: keyring.createAccounts.bind(keyring), + })); return { createAccount: async (options) => @@ -134,6 +140,8 @@ export abstract class SnapAccountProvider extends BaseBip44AccountProvider { displayConfirmation: false, setSelectedAccount: false, }), + createAccounts: async (options) => + await createAccounts(this.snapId, options), removeAccount: async (address: string) => // Though, when removing account, we can use the normal flow. await this.#withSnapKeyring(async ({ keyring }) => { diff --git a/packages/multichain-account-service/src/providers/SolAccountProvider.test.ts b/packages/multichain-account-service/src/providers/SolAccountProvider.test.ts index 5ca02ee8c04..4bc3fcaf10a 100644 --- a/packages/multichain-account-service/src/providers/SolAccountProvider.test.ts +++ b/packages/multichain-account-service/src/providers/SolAccountProvider.test.ts @@ -7,6 +7,7 @@ import type { InternalAccount, } from '@metamask/keyring-internal-api'; import { SnapControllerState } from '@metamask/snaps-controllers'; +import deepmerge from 'deepmerge'; import { AccountProviderWrapper } from './AccountProviderWrapper'; import type { SnapAccountProviderConfig } from './SnapAccountProvider'; @@ -19,13 +20,23 @@ import { TraceName } from '../constants/traces'; import { getMultichainAccountServiceMessenger, getRootMessenger, + toGroupIndexRangeArray, MOCK_HD_ACCOUNT_1, MOCK_HD_KEYRING_1, MOCK_SOL_ACCOUNT_1, MOCK_SOL_DISCOVERED_ACCOUNT_1, MockAccountBuilder, } from '../tests'; -import type { RootMessenger } from '../tests'; +import type { RootMessenger, DeepPartial } from '../tests'; + +function asConfig( + partial: DeepPartial, +): SnapAccountProviderConfig { + return deepmerge( + SOL_ACCOUNT_PROVIDER_DEFAULT_CONFIG, + partial, + ) as SnapAccountProviderConfig; +} class MockSolanaKeyring { readonly type = 'MockSolanaKeyring'; @@ -82,6 +93,35 @@ class MockSolanaKeyring { return account; }); + + createAccounts: SnapKeyring['createAccounts'] = jest + .fn() + .mockImplementation((_, options) => { + const groupIndices = + options.type === 'bip44:derive-index' + ? [options.groupIndex] + : toGroupIndexRangeArray(options.range); + + return groupIndices.map((groupIndex) => { + const found = this.accounts.find( + (account) => + isBip44Account(account) && + account.options.entropy.groupIndex === groupIndex, + ); + + if (found) { + return found; // Idempotent. + } + + const account = MockAccountBuilder.from(MOCK_SOL_ACCOUNT_1) + .withUuid() + .withAddressSuffix(`${groupIndex}`) + .withGroupIndex(groupIndex) + .get(); + this.accounts.push(account); + return account; + }); + }); } class MockSolAccountProvider extends SolAccountProvider { @@ -115,6 +155,7 @@ function setup({ handleRequest: jest.Mock; keyring: { createAccount: jest.Mock; + createAccounts: jest.Mock; }; trace: jest.Mock; }; @@ -192,6 +233,7 @@ function setup({ handleRequest: mockHandleRequest, keyring: { createAccount: keyring.createAccount as jest.Mock, + createAccounts: keyring.createAccounts as jest.Mock, }, trace: mockTrace, }, @@ -250,147 +292,295 @@ describe('SolAccountProvider', () => { expect(provider.isAccountCompatible(account)).toBe(false); }); - it('creates accounts', async () => { - const accounts = [MOCK_SOL_ACCOUNT_1]; - const { provider, keyring } = setup({ - accounts, - }); + describe('v1', () => { + it('uses v1 by default when no config is provided', async () => { + const accounts = [MOCK_SOL_ACCOUNT_1]; + const { provider, mocks } = setup({ + accounts, + // Force v1 by not providing the config at all, so it relies on the default value. + config: asConfig({ createAccounts: { v2: undefined } }), + }); - const newGroupIndex = accounts.length; // Group-index are 0-based. - const newAccounts = await provider.createAccounts({ - type: AccountCreationType.Bip44DeriveIndex, - entropySource: MOCK_HD_KEYRING_1.metadata.id, - groupIndex: newGroupIndex, - }); - expect(newAccounts).toHaveLength(1); - expect(keyring.createAccount).toHaveBeenCalled(); - }); + await provider.createAccounts({ + type: AccountCreationType.Bip44DeriveIndex, + entropySource: MOCK_HD_KEYRING_1.metadata.id, + groupIndex: accounts.length, + }); - it('does not re-create accounts (idempotent)', async () => { - const accounts = [MOCK_SOL_ACCOUNT_1]; - const { provider } = setup({ - accounts, + expect(mocks.keyring.createAccount).toHaveBeenCalled(); + expect(mocks.keyring.createAccounts).not.toHaveBeenCalled(); }); - const newAccounts = await provider.createAccounts({ - entropySource: MOCK_HD_KEYRING_1.metadata.id, - groupIndex: 0, - type: AccountCreationType.Bip44DeriveIndex, - }); - expect(newAccounts).toHaveLength(1); - expect(newAccounts[0]).toStrictEqual(MOCK_SOL_ACCOUNT_1); - }); + it('creates accounts', async () => { + const accounts = [MOCK_SOL_ACCOUNT_1]; + const { provider, mocks } = setup({ accounts }); - it('creates multiple accounts using Bip44DeriveIndexRange', async () => { - const accounts = [MOCK_SOL_ACCOUNT_1]; - const { provider, keyring } = setup({ - accounts, + const newGroupIndex = accounts.length; // Group-index are 0-based. + const newAccounts = await provider.createAccounts({ + type: AccountCreationType.Bip44DeriveIndex, + entropySource: MOCK_HD_KEYRING_1.metadata.id, + groupIndex: newGroupIndex, + }); + expect(newAccounts).toHaveLength(1); + expect(mocks.keyring.createAccount).toHaveBeenCalled(); + expect(mocks.keyring.createAccounts).not.toHaveBeenCalled(); }); - const from = 1; - const newAccounts = await provider.createAccounts({ - type: AccountCreationType.Bip44DeriveIndexRange, - entropySource: MOCK_HD_KEYRING_1.metadata.id, - range: { - from, - to: 3, - }, + it('does not re-create accounts (idempotent)', async () => { + const accounts = [MOCK_SOL_ACCOUNT_1]; + const { provider } = setup({ accounts }); + + const newAccounts = await provider.createAccounts({ + entropySource: MOCK_HD_KEYRING_1.metadata.id, + groupIndex: 0, + type: AccountCreationType.Bip44DeriveIndex, + }); + expect(newAccounts).toHaveLength(1); + expect(newAccounts[0]).toStrictEqual(MOCK_SOL_ACCOUNT_1); }); - expect(newAccounts).toHaveLength(3); - expect(keyring.createAccount).toHaveBeenCalledTimes(3); + it('creates multiple accounts using Bip44DeriveIndexRange', async () => { + const accounts = [MOCK_SOL_ACCOUNT_1]; + const { provider, mocks } = setup({ accounts }); - // Verify each account has the correct group index. - for (const [index, account] of newAccounts.entries()) { - expect(isBip44Account(account)).toBe(true); - expect(account.options.entropy.groupIndex).toBe(from + index); - } - }); + const from = 1; + const newAccounts = await provider.createAccounts({ + type: AccountCreationType.Bip44DeriveIndexRange, + entropySource: MOCK_HD_KEYRING_1.metadata.id, + range: { from, to: 3 }, + }); - it('creates accounts with range starting from 0', async () => { - const { provider, keyring } = setup({ - accounts: [], + expect(newAccounts).toHaveLength(3); + expect(mocks.keyring.createAccount).toHaveBeenCalledTimes(3); + expect(mocks.keyring.createAccounts).not.toHaveBeenCalled(); + + // Verify each account has the correct group index. + for (const [index, account] of newAccounts.entries()) { + expect(isBip44Account(account)).toBe(true); + expect(account.options.entropy.groupIndex).toBe(from + index); + } }); - const newAccounts = await provider.createAccounts({ - type: AccountCreationType.Bip44DeriveIndexRange, - entropySource: MOCK_HD_KEYRING_1.metadata.id, - range: { - from: 0, - to: 2, - }, + it('creates accounts with range starting from 0', async () => { + const { provider, mocks } = setup({ accounts: [] }); + + const newAccounts = await provider.createAccounts({ + type: AccountCreationType.Bip44DeriveIndexRange, + entropySource: MOCK_HD_KEYRING_1.metadata.id, + range: { from: 0, to: 2 }, + }); + + expect(newAccounts).toHaveLength(3); + expect(mocks.keyring.createAccount).toHaveBeenCalledTimes(3); + expect(mocks.keyring.createAccounts).not.toHaveBeenCalled(); }); - expect(newAccounts).toHaveLength(3); - expect(keyring.createAccount).toHaveBeenCalledTimes(3); - }); + it('creates a single account when range from equals to', async () => { + const { provider, mocks } = setup({ accounts: [] }); - it('creates a single account when range from equals to', async () => { - const { provider, keyring } = setup({ - accounts: [], + const newAccounts = await provider.createAccounts({ + type: AccountCreationType.Bip44DeriveIndexRange, + entropySource: MOCK_HD_KEYRING_1.metadata.id, + range: { from: 5, to: 5 }, + }); + + expect(newAccounts).toHaveLength(1); + expect(mocks.keyring.createAccount).toHaveBeenCalledTimes(1); + expect(mocks.keyring.createAccounts).not.toHaveBeenCalled(); + expect( + isBip44Account(newAccounts[0]) && + newAccounts[0].options.entropy.groupIndex, + ).toBe(5); }); - const newAccounts = await provider.createAccounts({ - type: AccountCreationType.Bip44DeriveIndexRange, - entropySource: MOCK_HD_KEYRING_1.metadata.id, - range: { - from: 5, - to: 5, - }, + it('throws if the account creation process takes too long', async () => { + const { provider, mocks } = setup({ accounts: [] }); + + mocks.keyring.createAccount.mockImplementation( + () => + new Promise((resolve) => { + setTimeout(() => resolve(MOCK_SOL_ACCOUNT_1), 4000); + }), + ); + + await expect( + provider.createAccounts({ + type: AccountCreationType.Bip44DeriveIndex, + entropySource: MOCK_HD_KEYRING_1.metadata.id, + groupIndex: 0, + }), + ).rejects.toThrow('Timed out'); }); - expect(newAccounts).toHaveLength(1); - expect(keyring.createAccount).toHaveBeenCalledTimes(1); - expect( - isBip44Account(newAccounts[0]) && - newAccounts[0].options.entropy.groupIndex, - ).toBe(5); - }); + // Skip this test for now, since we manually inject those options upon + // account creation, so it cannot fails (until the Solana Snap starts + // using the new typed options). + // eslint-disable-next-line jest/no-disabled-tests + it.skip('throws if the created account is not BIP-44 compatible', async () => { + const accounts = [MOCK_SOL_ACCOUNT_1]; + const { provider, mocks } = setup({ accounts }); - it('throws if the account creation process takes too long', async () => { - const { provider, mocks } = setup({ - accounts: [], + mocks.keyring.createAccount.mockResolvedValue({ + ...MOCK_SOL_ACCOUNT_1, + options: {}, // No options, so it cannot be BIP-44 compatible. + }); + + await expect( + provider.createAccounts({ + type: AccountCreationType.Bip44DeriveIndex, + entropySource: MOCK_HD_KEYRING_1.metadata.id, + groupIndex: 0, + }), + ).rejects.toThrow('Created account is not BIP-44 compatible'); }); - mocks.keyring.createAccount.mockImplementation(() => { - return new Promise((resolve) => { - setTimeout(() => { - resolve(MOCK_SOL_ACCOUNT_1); - }, 4000); + it('discover accounts at a new group index creates an account', async () => { + const { provider, mocks } = setup({ accounts: [] }); + + // Simulate one discovered account at the requested index. + mocks.handleRequest.mockReturnValue([MOCK_SOL_DISCOVERED_ACCOUNT_1]); + + const discovered = await provider.discoverAccounts({ + entropySource: MOCK_HD_KEYRING_1.metadata.id, + groupIndex: 0, }); + + expect(discovered).toHaveLength(1); + // Ensure we did go through creation path + expect(mocks.keyring.createAccount).toHaveBeenCalled(); + // Provider should now expose one account (newly created) + expect(provider.getAccounts()).toHaveLength(1); }); + }); - await expect( - provider.createAccounts({ + describe('v2', () => { + it('creates accounts', async () => { + const accounts = [MOCK_SOL_ACCOUNT_1]; + const { provider, mocks } = setup({ + accounts, + config: asConfig({ createAccounts: { v2: true } }), + }); + + const newGroupIndex = accounts.length; // Group-index are 0-based. + const newAccounts = await provider.createAccounts({ type: AccountCreationType.Bip44DeriveIndex, + entropySource: MOCK_HD_KEYRING_1.metadata.id, + groupIndex: newGroupIndex, + }); + expect(newAccounts).toHaveLength(1); + // Batch endpoint must be called, NOT the singular one. + expect(mocks.keyring.createAccounts).toHaveBeenCalledWith( + SolAccountProvider.SOLANA_SNAP_ID, + { + type: AccountCreationType.Bip44DeriveIndex, + entropySource: MOCK_HD_KEYRING_1.metadata.id, + groupIndex: newGroupIndex, + }, + ); + expect(mocks.keyring.createAccount).not.toHaveBeenCalled(); + }); + + it('does not re-create accounts (idempotent)', async () => { + const accounts = [MOCK_SOL_ACCOUNT_1]; + const { provider } = setup({ + accounts, + config: asConfig({ createAccounts: { v2: true } }), + }); + + const newAccounts = await provider.createAccounts({ entropySource: MOCK_HD_KEYRING_1.metadata.id, groupIndex: 0, - }), - ).rejects.toThrow('Timed out'); - }); + type: AccountCreationType.Bip44DeriveIndex, + }); + expect(newAccounts).toHaveLength(1); + expect(newAccounts[0]).toStrictEqual(MOCK_SOL_ACCOUNT_1); + }); - // Skip this test for now, since we manually inject those options upon - // account creation, so it cannot fails (until the Solana Snap starts - // using the new typed options). - // eslint-disable-next-line jest/no-disabled-tests - it.skip('throws if the created account is not BIP-44 compatible', async () => { - const accounts = [MOCK_SOL_ACCOUNT_1]; - const { provider, mocks } = setup({ - accounts, + it('creates multiple accounts using Bip44DeriveIndexRange', async () => { + const accounts = [MOCK_SOL_ACCOUNT_1]; + const { provider, mocks } = setup({ + accounts, + config: asConfig({ createAccounts: { v2: true } }), + }); + + const from = 1; + const newAccounts = await provider.createAccounts({ + type: AccountCreationType.Bip44DeriveIndexRange, + entropySource: MOCK_HD_KEYRING_1.metadata.id, + range: { from, to: 3 }, + }); + + expect(newAccounts).toHaveLength(3); + // Single batch call, NOT three individual calls. + expect(mocks.keyring.createAccounts).toHaveBeenCalledTimes(1); + expect(mocks.keyring.createAccount).not.toHaveBeenCalled(); + + // Verify each account has the correct group index. + for (const [index, account] of newAccounts.entries()) { + expect(isBip44Account(account)).toBe(true); + expect(account.options.entropy.groupIndex).toBe(from + index); + } }); - mocks.keyring.createAccount.mockResolvedValue({ - ...MOCK_SOL_ACCOUNT_1, - options: {}, // No options, so it cannot be BIP-44 compatible. + it('creates accounts with range starting from 0', async () => { + const { provider, mocks } = setup({ + accounts: [], + config: asConfig({ createAccounts: { v2: true } }), + }); + + const newAccounts = await provider.createAccounts({ + type: AccountCreationType.Bip44DeriveIndexRange, + entropySource: MOCK_HD_KEYRING_1.metadata.id, + range: { from: 0, to: 2 }, + }); + + expect(newAccounts).toHaveLength(3); + expect(mocks.keyring.createAccounts).toHaveBeenCalledTimes(1); + expect(mocks.keyring.createAccount).not.toHaveBeenCalled(); }); - await expect( - provider.createAccounts({ - type: AccountCreationType.Bip44DeriveIndex, + it('creates a single account when range from equals to', async () => { + const { provider, mocks } = setup({ + accounts: [], + config: asConfig({ createAccounts: { v2: true } }), + }); + + const newAccounts = await provider.createAccounts({ + type: AccountCreationType.Bip44DeriveIndexRange, entropySource: MOCK_HD_KEYRING_1.metadata.id, - groupIndex: 0, - }), - ).rejects.toThrow('Created account is not BIP-44 compatible'); + range: { from: 5, to: 5 }, + }); + + expect(newAccounts).toHaveLength(1); + expect(mocks.keyring.createAccounts).toHaveBeenCalledTimes(1); + expect(mocks.keyring.createAccount).not.toHaveBeenCalled(); + expect( + isBip44Account(newAccounts[0]) && + newAccounts[0].options.entropy.groupIndex, + ).toBe(5); + }); + + it('throws if the account creation process takes too long', async () => { + const { provider, mocks } = setup({ + accounts: [], + config: asConfig({ createAccounts: { v2: true } }), + }); + + mocks.keyring.createAccounts.mockImplementation( + () => + new Promise((resolve) => { + setTimeout(() => resolve([MOCK_SOL_ACCOUNT_1]), 4000); + }), + ); + + await expect( + provider.createAccounts({ + type: AccountCreationType.Bip44DeriveIndex, + entropySource: MOCK_HD_KEYRING_1.metadata.id, + groupIndex: 0, + }), + ).rejects.toThrow('Timed out'); + }); }); it('throws an error when type is not "bip44:derive-index"', async () => { @@ -408,26 +598,6 @@ describe('SolAccountProvider', () => { ); }); - it('discover accounts at a new group index creates an account', async () => { - const { provider, mocks } = setup({ - accounts: [], - }); - - // Simulate one discovered account at the requested index. - mocks.handleRequest.mockReturnValue([MOCK_SOL_DISCOVERED_ACCOUNT_1]); - - const discovered = await provider.discoverAccounts({ - entropySource: MOCK_HD_KEYRING_1.metadata.id, - groupIndex: 0, - }); - - expect(discovered).toHaveLength(1); - // Ensure we did go through creation path - expect(mocks.keyring.createAccount).toHaveBeenCalled(); - // Provider should now expose one account (newly created) - expect(provider.getAccounts()).toHaveLength(1); - }); - it('returns existing account if it already exists at index', async () => { const { provider, mocks } = setup({ accounts: [MOCK_SOL_ACCOUNT_1], @@ -462,13 +632,11 @@ describe('SolAccountProvider', () => { it('does not run discovery if disabled', async () => { const { provider } = setup({ accounts: [MOCK_SOL_ACCOUNT_1], - config: { - ...SOL_ACCOUNT_PROVIDER_DEFAULT_CONFIG, + config: asConfig({ discovery: { - ...SOL_ACCOUNT_PROVIDER_DEFAULT_CONFIG.discovery, enabled: false, }, - }, + }), }); expect( diff --git a/packages/multichain-account-service/src/providers/SolAccountProvider.ts b/packages/multichain-account-service/src/providers/SolAccountProvider.ts index 3143878368d..a7c3ca73c6f 100644 --- a/packages/multichain-account-service/src/providers/SolAccountProvider.ts +++ b/packages/multichain-account-service/src/providers/SolAccountProvider.ts @@ -2,6 +2,8 @@ import { assertIsBip44Account } from '@metamask/account-api'; import type { Bip44Account } from '@metamask/account-api'; import type { TraceCallback } from '@metamask/controller-utils'; import type { + CreateAccountBip44DeriveIndexOptions, + CreateAccountBip44DeriveIndexRangeOptions, CreateAccountOptions, EntropySourceId, KeyringAccount, @@ -41,6 +43,7 @@ export const SOL_ACCOUNT_PROVIDER_DEFAULT_CONFIG: SnapAccountProviderConfig = { backOffMs: 1000, }, createAccounts: { + v2: false, // For now, the Snap is not fully v2 compliant. timeoutMs: 3000, }, }; @@ -77,90 +80,135 @@ export class SolAccountProvider extends SnapAccountProvider { ); } - async #createAccount({ - keyring, + #getDerivationpath(groupIndex: number): string { + return `m/44'/501'/${groupIndex}'/0'`; + } + + #toBip44Account({ + account, entropySource, groupIndex, - derivationPath, }: { - keyring: RestrictedSnapKeyring; + account: KeyringAccount; entropySource: EntropySourceId; groupIndex: number; - derivationPath: string; - }): Promise> { - const account = await withTimeout( - keyring.createAccount({ entropySource, derivationPath }), - this.config.createAccounts.timeoutMs, - ); - + }): Bip44Account { // Ensure entropy is present before type assertion validation account.options.entropy = { type: KeyringAccountEntropyTypeOption.Mnemonic, id: entropySource, groupIndex, - derivationPath, + derivationPath: this.#getDerivationpath(groupIndex), }; assertIsBip44Account(account); - this.accounts.add(account.id); + return account; } - async createAccounts( - options: CreateAccountOptions, + async #createAccounts( + keyring: RestrictedSnapKeyring, + options: + | CreateAccountBip44DeriveIndexOptions + | CreateAccountBip44DeriveIndexRangeOptions, ): Promise[]> { - assertCreateAccountOptionIsSupported(options, [ - `${AccountCreationType.Bip44DeriveIndex}`, - `${AccountCreationType.Bip44DeriveIndexRange}`, - ]); + return this.withMaxConcurrency(async () => { + let groupIndexOffset = 0; + let snapAccounts: KeyringAccount[] = []; - const { entropySource } = options; - - if (options.type === AccountCreationType.Bip44DeriveIndexRange) { - const { range } = options; - return this.withSnap(async ({ keyring }) => { - const accounts: Bip44Account[] = []; - - // TODO: Use `SnapKeyring.createAccounts` when that functionality is implemented on the Snap - // itself, instead of creating accounts one by one. - for ( - let groupIndex = range.from; - groupIndex <= range.to; - groupIndex++ - ) { - const account = await this.withMaxConcurrency(async () => { - const derivationPath = `m/44'/501'/${groupIndex}'/0'`; - return this.#createAccount({ - keyring, - entropySource, - groupIndex, - derivationPath, - }); - }); - accounts.push(account); + const v2 = this.config.createAccounts.v2 ?? false; + + const { entropySource } = options; + + if (options.type === `${AccountCreationType.Bip44DeriveIndexRange}`) { + if (v2) { + // Batch account creations. + snapAccounts = await withTimeout( + keyring.createAccounts(options), + this.config.createAccounts.timeoutMs, + ); + } else { + const { range } = options; + + // Create accounts one by one (async flow + using Snap keyring events). + for ( + let groupIndex = range.from; + groupIndex <= range.to; + groupIndex++ + ) { + const snapAccount = await withTimeout( + keyring.createAccount({ + entropySource, + derivationPath: this.#getDerivationpath(groupIndex), + }), + this.config.createAccounts.timeoutMs, + ); + + snapAccounts.push(snapAccount); + } } - return accounts; - }); - } + // Group indices are sequential, so we just need the starting index. + groupIndexOffset = options.range.from; + } else { + if (v2) { + // Create account using new v2-like flow (no async flow + no Snap keyring events). + const [snapAccount] = await withTimeout( + keyring.createAccounts(options), + this.config.createAccounts.timeoutMs, + ); - const { groupIndex } = options; + snapAccounts = [snapAccount]; + } else { + const { groupIndex } = options; - return this.withSnap(async ({ keyring }) => { - return this.withMaxConcurrency(async () => { - const derivationPath = `m/44'/501'/${groupIndex}'/0'`; - const account = await this.#createAccount({ - keyring, + // Create account (async flow + using Snap keyring events). + const snapAccount = await withTimeout( + keyring.createAccount({ + entropySource, + derivationPath: this.#getDerivationpath(groupIndex), + }), + this.config.createAccounts.timeoutMs, + ); + + snapAccounts = [snapAccount]; + } + + // For single account, there will only be 1 account, so we can use the + // provided group index directly. + groupIndexOffset = options.groupIndex; + } + + // NOTE: We still need to convert accounts to proper BIP-44 accounts for now. + return snapAccounts.map((snapAccount, index) => { + const groupIndex = groupIndexOffset + index; + const account = this.#toBip44Account({ + account: snapAccount, entropySource, groupIndex, - derivationPath, }); - return [account]; + // Finally, we can add the account to the provider's account set. + this.accounts.add(snapAccount.id); + + return account; }); }); } + async createAccounts( + options: CreateAccountOptions, + ): Promise[]> { + assertCreateAccountOptionIsSupported(options, [ + `${AccountCreationType.Bip44DeriveIndex}`, + `${AccountCreationType.Bip44DeriveIndexRange}`, + ]); + + return this.withSnap(async ({ keyring }) => { + return this.#createAccounts(keyring, options); + }); + } + async discoverAccounts({ entropySource, groupIndex, @@ -201,22 +249,14 @@ export class SolAccountProvider extends SnapAccountProvider { return []; } - const createdAccounts = await Promise.all( - discoveredAccounts.map((d) => - this.#createAccount({ - keyring, - entropySource, - groupIndex, - derivationPath: d.derivationPath, - }), - ), - ); - - for (const account of createdAccounts) { - this.accounts.add(account.id); - } - - return createdAccounts; + // NOTE: We know the Solana Snap only return 1 account per group index during discovery. Also, + // we do not use the returned `derivationPath` on purpose. Instead we just create the account + // for this group index and that's all. + return await this.#createAccounts(keyring, { + type: AccountCreationType.Bip44DeriveIndex, + entropySource, + groupIndex, + }); }, ); }); diff --git a/packages/multichain-account-service/src/providers/TrxAccountProvider.test.ts b/packages/multichain-account-service/src/providers/TrxAccountProvider.test.ts index bc57328e20e..f7191b37709 100644 --- a/packages/multichain-account-service/src/providers/TrxAccountProvider.test.ts +++ b/packages/multichain-account-service/src/providers/TrxAccountProvider.test.ts @@ -7,6 +7,7 @@ import type { InternalAccount, } from '@metamask/keyring-internal-api'; import { SnapControllerState } from '@metamask/snaps-controllers'; +import deepmerge from 'deepmerge'; import { AccountProviderWrapper } from './AccountProviderWrapper'; import type { SnapAccountProviderConfig } from './SnapAccountProvider'; @@ -24,8 +25,18 @@ import { MOCK_TRX_ACCOUNT_1, MOCK_TRX_DISCOVERED_ACCOUNT_1, MockAccountBuilder, + toGroupIndexRangeArray, } from '../tests'; -import type { RootMessenger } from '../tests'; +import type { RootMessenger, DeepPartial } from '../tests'; + +function asConfig( + partial: DeepPartial, +): SnapAccountProviderConfig { + return deepmerge( + TRX_ACCOUNT_PROVIDER_DEFAULT_CONFIG, + partial, + ) as SnapAccountProviderConfig; +} class MockTronKeyring { readonly type = 'MockTronKeyring'; @@ -69,9 +80,39 @@ class MockTronKeyring { return account; }); + createAccounts: SnapKeyring['createAccounts'] = jest + .fn() + .mockImplementation((_, options) => { + const groupIndices = + options.type === 'bip44:derive-index' + ? [options.groupIndex] + : toGroupIndexRangeArray(options.range); + + return groupIndices.map((groupIndex) => { + const found = this.accounts.find( + (account) => + isBip44Account(account) && + account.options.entropy.groupIndex === groupIndex, + ); + + if (found) { + return found; // Idempotent. + } + + const account = MockAccountBuilder.from(MOCK_TRX_ACCOUNT_1) + .withUuid() + .withAddressSuffix(`${groupIndex}`) + .withGroupIndex(groupIndex) + .get(); + this.accounts.push(account); + return account; + }); + }); + // Add discoverAccounts method to match the provider's usage discoverAccounts = jest.fn().mockResolvedValue([]); } + class MockTrxAccountProvider extends TrxAccountProvider { override async ensureCanUseSnapPlatform(): Promise { // Override to avoid waiting during tests. @@ -103,6 +144,7 @@ function setup({ handleRequest: jest.Mock; keyring: { createAccount: jest.Mock; + createAccounts: jest.Mock; discoverAccounts: jest.Mock; }; }; @@ -173,6 +215,7 @@ function setup({ handleRequest: mockHandleRequest, keyring: { createAccount: keyring.createAccount as jest.Mock, + createAccounts: keyring.createAccounts as jest.Mock, discoverAccounts: keyring.discoverAccounts, }, }, @@ -231,147 +274,300 @@ describe('TrxAccountProvider', () => { expect(provider.isAccountCompatible(account)).toBe(false); }); - it('creates accounts', async () => { - const accounts = [MOCK_TRX_ACCOUNT_1]; - const { provider, keyring } = setup({ - accounts, - }); + describe('v1', () => { + it('uses v1 by default when no config is provided', async () => { + const accounts = [MOCK_TRX_ACCOUNT_1]; + const { provider, mocks } = setup({ + accounts, + // Force v1 by not providing the config at all, so it relies on the default value. + config: asConfig({ createAccounts: { v2: undefined } }), + }); - const newGroupIndex = accounts.length; // Group-index are 0-based. - const newAccounts = await provider.createAccounts({ - type: AccountCreationType.Bip44DeriveIndex, - entropySource: MOCK_HD_KEYRING_1.metadata.id, - groupIndex: newGroupIndex, - }); - expect(newAccounts).toHaveLength(1); - expect(keyring.createAccount).toHaveBeenCalled(); - }); + await provider.createAccounts({ + type: AccountCreationType.Bip44DeriveIndex, + entropySource: MOCK_HD_KEYRING_1.metadata.id, + groupIndex: accounts.length, + }); - it('does not re-create accounts (idempotent)', async () => { - const accounts = [MOCK_TRX_ACCOUNT_1]; - const { provider } = setup({ - accounts, + expect(mocks.keyring.createAccount).toHaveBeenCalled(); + expect(mocks.keyring.createAccounts).not.toHaveBeenCalled(); }); - const newAccounts = await provider.createAccounts({ - entropySource: MOCK_HD_KEYRING_1.metadata.id, - groupIndex: 0, - type: AccountCreationType.Bip44DeriveIndex, - }); - expect(newAccounts).toHaveLength(1); - expect(newAccounts[0]).toStrictEqual(MOCK_TRX_ACCOUNT_1); - }); + it('creates accounts', async () => { + const accounts = [MOCK_TRX_ACCOUNT_1]; + const { provider, mocks } = setup({ accounts }); - it('creates multiple accounts using Bip44DeriveIndexRange', async () => { - const accounts = [MOCK_TRX_ACCOUNT_1]; - const { provider, keyring } = setup({ - accounts, + const newGroupIndex = accounts.length; // Group-index are 0-based. + const newAccounts = await provider.createAccounts({ + type: AccountCreationType.Bip44DeriveIndex, + entropySource: MOCK_HD_KEYRING_1.metadata.id, + groupIndex: newGroupIndex, + }); + expect(newAccounts).toHaveLength(1); + expect(mocks.keyring.createAccount).toHaveBeenCalled(); + expect(mocks.keyring.createAccounts).not.toHaveBeenCalled(); }); - const from = 1; - const newAccounts = await provider.createAccounts({ - type: AccountCreationType.Bip44DeriveIndexRange, - entropySource: MOCK_HD_KEYRING_1.metadata.id, - range: { - from, - to: 3, - }, + it('does not re-create accounts (idempotent)', async () => { + const accounts = [MOCK_TRX_ACCOUNT_1]; + const { provider } = setup({ accounts }); + + const newAccounts = await provider.createAccounts({ + entropySource: MOCK_HD_KEYRING_1.metadata.id, + groupIndex: 0, + type: AccountCreationType.Bip44DeriveIndex, + }); + expect(newAccounts).toHaveLength(1); + expect(newAccounts[0]).toStrictEqual(MOCK_TRX_ACCOUNT_1); }); - expect(newAccounts).toHaveLength(3); - expect(keyring.createAccount).toHaveBeenCalledTimes(3); + it('creates multiple accounts using Bip44DeriveIndexRange', async () => { + const accounts = [MOCK_TRX_ACCOUNT_1]; + const { provider, mocks } = setup({ accounts }); - // Verify each account has the correct group index. - for (const [index, account] of newAccounts.entries()) { - expect(isBip44Account(account)).toBe(true); - expect(account.options.entropy.groupIndex).toBe(from + index); - } - }); + const from = 1; + const newAccounts = await provider.createAccounts({ + type: AccountCreationType.Bip44DeriveIndexRange, + entropySource: MOCK_HD_KEYRING_1.metadata.id, + range: { from, to: 3 }, + }); - it('creates accounts with range starting from 0', async () => { - const { provider, keyring } = setup({ - accounts: [], + expect(newAccounts).toHaveLength(3); + expect(mocks.keyring.createAccount).toHaveBeenCalledTimes(3); + expect(mocks.keyring.createAccounts).not.toHaveBeenCalled(); + + // Verify each account has the correct group index. + for (const [index, account] of newAccounts.entries()) { + expect(isBip44Account(account)).toBe(true); + expect(account.options.entropy.groupIndex).toBe(from + index); + } }); - const newAccounts = await provider.createAccounts({ - type: AccountCreationType.Bip44DeriveIndexRange, - entropySource: MOCK_HD_KEYRING_1.metadata.id, - range: { - from: 0, - to: 2, - }, + it('creates accounts with range starting from 0', async () => { + const { provider, mocks } = setup({ accounts: [] }); + + const newAccounts = await provider.createAccounts({ + type: AccountCreationType.Bip44DeriveIndexRange, + entropySource: MOCK_HD_KEYRING_1.metadata.id, + range: { from: 0, to: 2 }, + }); + + expect(newAccounts).toHaveLength(3); + expect(mocks.keyring.createAccount).toHaveBeenCalledTimes(3); + expect(mocks.keyring.createAccounts).not.toHaveBeenCalled(); }); - expect(newAccounts).toHaveLength(3); - expect(keyring.createAccount).toHaveBeenCalledTimes(3); - }); + it('creates a single account when range from equals to', async () => { + const { provider, mocks } = setup({ accounts: [] }); - it('creates a single account when range from equals to', async () => { - const { provider, keyring } = setup({ - accounts: [], + const newAccounts = await provider.createAccounts({ + type: AccountCreationType.Bip44DeriveIndexRange, + entropySource: MOCK_HD_KEYRING_1.metadata.id, + range: { from: 5, to: 5 }, + }); + + expect(newAccounts).toHaveLength(1); + expect(mocks.keyring.createAccount).toHaveBeenCalledTimes(1); + expect(mocks.keyring.createAccounts).not.toHaveBeenCalled(); + expect( + isBip44Account(newAccounts[0]) && + newAccounts[0].options.entropy.groupIndex, + ).toBe(5); }); - const newAccounts = await provider.createAccounts({ - type: AccountCreationType.Bip44DeriveIndexRange, - entropySource: MOCK_HD_KEYRING_1.metadata.id, - range: { - from: 5, - to: 5, - }, + it('throws if the account creation process takes too long', async () => { + const { provider, mocks } = setup({ accounts: [] }); + + mocks.keyring.createAccount.mockImplementation(() => { + return new Promise((resolve) => { + setTimeout(() => { + resolve(MOCK_TRX_ACCOUNT_1); + }, 4000); + }); + }); + + await expect( + provider.createAccounts({ + type: AccountCreationType.Bip44DeriveIndex, + entropySource: MOCK_HD_KEYRING_1.metadata.id, + groupIndex: 0, + }), + ).rejects.toThrow('Timed out'); }); - expect(newAccounts).toHaveLength(1); - expect(keyring.createAccount).toHaveBeenCalledTimes(1); - expect( - isBip44Account(newAccounts[0]) && - newAccounts[0].options.entropy.groupIndex, - ).toBe(5); - }); + // Skip this test for now, since we manually inject those options upon + // account creation, so it cannot fails (until the Tron Snap starts + // using the new typed options). + // eslint-disable-next-line jest/no-disabled-tests + it.skip('throws if the created account is not BIP-44 compatible', async () => { + const accounts = [MOCK_TRX_ACCOUNT_1]; + const { provider, mocks } = setup({ + accounts, + }); - it('throws if the account creation process takes too long', async () => { - const { provider, mocks } = setup({ - accounts: [], + mocks.keyring.createAccount.mockResolvedValue({ + ...MOCK_TRX_ACCOUNT_1, + options: {}, // No options, so it cannot be BIP-44 compatible. + }); + + await expect( + provider.createAccounts({ + type: AccountCreationType.Bip44DeriveIndex, + entropySource: MOCK_HD_KEYRING_1.metadata.id, + groupIndex: 0, + }), + ).rejects.toThrow('Created account is not BIP-44 compatible'); }); - mocks.keyring.createAccount.mockImplementation(() => { - return new Promise((resolve) => { - setTimeout(() => { - resolve(MOCK_TRX_ACCOUNT_1); - }, 4000); + it('discover accounts at a new group index creates an account', async () => { + const { provider, mocks } = setup({ accounts: [] }); + + // Simulate one discovered account at the requested index. + mocks.keyring.discoverAccounts.mockResolvedValue([ + MOCK_TRX_DISCOVERED_ACCOUNT_1, + ]); + + const discovered = await provider.discoverAccounts({ + entropySource: MOCK_HD_KEYRING_1.metadata.id, + groupIndex: 0, }); + + expect(discovered).toHaveLength(1); + // Ensure we did go through creation path + expect(mocks.keyring.createAccount).toHaveBeenCalled(); + // Provider should now expose one account (newly created) + expect(provider.getAccounts()).toHaveLength(1); }); + }); - await expect( - provider.createAccounts({ + describe('v2', () => { + it('creates accounts', async () => { + const accounts = [MOCK_TRX_ACCOUNT_1]; + const { provider, mocks } = setup({ + accounts, + config: asConfig({ createAccounts: { v2: true } }), + }); + + const newGroupIndex = accounts.length; // Group-index are 0-based. + const newAccounts = await provider.createAccounts({ type: AccountCreationType.Bip44DeriveIndex, + entropySource: MOCK_HD_KEYRING_1.metadata.id, + groupIndex: newGroupIndex, + }); + expect(newAccounts).toHaveLength(1); + // Batch endpoint must be called, NOT the singular one. + expect(mocks.keyring.createAccounts).toHaveBeenCalledWith( + TrxAccountProvider.TRX_SNAP_ID, + { + type: AccountCreationType.Bip44DeriveIndex, + entropySource: MOCK_HD_KEYRING_1.metadata.id, + groupIndex: newGroupIndex, + }, + ); + expect(mocks.keyring.createAccount).not.toHaveBeenCalled(); + }); + + it('does not re-create accounts (idempotent)', async () => { + const accounts = [MOCK_TRX_ACCOUNT_1]; + const { provider } = setup({ + accounts, + config: asConfig({ createAccounts: { v2: true } }), + }); + + const newAccounts = await provider.createAccounts({ entropySource: MOCK_HD_KEYRING_1.metadata.id, groupIndex: 0, - }), - ).rejects.toThrow('Timed out'); - }); + type: AccountCreationType.Bip44DeriveIndex, + }); + expect(newAccounts).toHaveLength(1); + expect(newAccounts[0]).toStrictEqual(MOCK_TRX_ACCOUNT_1); + }); - // Skip this test for now, since we manually inject those options upon - // account creation, so it cannot fails (until the Tron Snap starts - // using the new typed options). - // eslint-disable-next-line jest/no-disabled-tests - it.skip('throws if the created account is not BIP-44 compatible', async () => { - const accounts = [MOCK_TRX_ACCOUNT_1]; - const { provider, mocks } = setup({ - accounts, + it('creates multiple accounts using Bip44DeriveIndexRange', async () => { + const accounts = [MOCK_TRX_ACCOUNT_1]; + const { provider, mocks } = setup({ + accounts, + config: asConfig({ createAccounts: { v2: true } }), + }); + + const from = 1; + const newAccounts = await provider.createAccounts({ + type: AccountCreationType.Bip44DeriveIndexRange, + entropySource: MOCK_HD_KEYRING_1.metadata.id, + range: { from, to: 3 }, + }); + + expect(newAccounts).toHaveLength(3); + // Single batch call, NOT three individual calls. + expect(mocks.keyring.createAccounts).toHaveBeenCalledTimes(1); + expect(mocks.keyring.createAccount).not.toHaveBeenCalled(); + + // Verify each account has the correct group index. + for (const [index, account] of newAccounts.entries()) { + expect(isBip44Account(account)).toBe(true); + expect(account.options.entropy.groupIndex).toBe(from + index); + } }); - mocks.keyring.createAccount.mockResolvedValue({ - ...MOCK_TRX_ACCOUNT_1, - options: {}, // No options, so it cannot be BIP-44 compatible. + it('creates accounts with range starting from 0', async () => { + const { provider, mocks } = setup({ + accounts: [], + config: asConfig({ createAccounts: { v2: true } }), + }); + + const newAccounts = await provider.createAccounts({ + type: AccountCreationType.Bip44DeriveIndexRange, + entropySource: MOCK_HD_KEYRING_1.metadata.id, + range: { from: 0, to: 2 }, + }); + + expect(newAccounts).toHaveLength(3); + expect(mocks.keyring.createAccounts).toHaveBeenCalledTimes(1); + expect(mocks.keyring.createAccount).not.toHaveBeenCalled(); }); - await expect( - provider.createAccounts({ - type: AccountCreationType.Bip44DeriveIndex, + it('creates a single account when range from equals to', async () => { + const { provider, mocks } = setup({ + accounts: [], + config: asConfig({ createAccounts: { v2: true } }), + }); + + const newAccounts = await provider.createAccounts({ + type: AccountCreationType.Bip44DeriveIndexRange, entropySource: MOCK_HD_KEYRING_1.metadata.id, - groupIndex: 0, - }), - ).rejects.toThrow('Created account is not BIP-44 compatible'); + range: { from: 5, to: 5 }, + }); + + expect(newAccounts).toHaveLength(1); + expect(mocks.keyring.createAccounts).toHaveBeenCalledTimes(1); + expect(mocks.keyring.createAccount).not.toHaveBeenCalled(); + expect( + isBip44Account(newAccounts[0]) && + newAccounts[0].options.entropy.groupIndex, + ).toBe(5); + }); + + it('throws if the account creation process takes too long', async () => { + const { provider, mocks } = setup({ + accounts: [], + config: asConfig({ createAccounts: { v2: true } }), + }); + + mocks.keyring.createAccounts.mockImplementation( + () => + new Promise((resolve) => { + setTimeout(() => resolve([MOCK_TRX_ACCOUNT_1]), 4000); + }), + ); + + await expect( + provider.createAccounts({ + type: AccountCreationType.Bip44DeriveIndex, + entropySource: MOCK_HD_KEYRING_1.metadata.id, + groupIndex: 0, + }), + ).rejects.toThrow('Timed out'); + }); }); it('throws an error when type is not "bip44:derive-index"', async () => { @@ -389,28 +585,6 @@ describe('TrxAccountProvider', () => { ); }); - it('discover accounts at a new group index creates an account', async () => { - const { provider, mocks } = setup({ - accounts: [], - }); - - // Simulate one discovered account at the requested index. - mocks.keyring.discoverAccounts.mockResolvedValue([ - MOCK_TRX_DISCOVERED_ACCOUNT_1, - ]); - - const discovered = await provider.discoverAccounts({ - entropySource: MOCK_HD_KEYRING_1.metadata.id, - groupIndex: 0, - }); - - expect(discovered).toHaveLength(1); - // Ensure we did go through creation path - expect(mocks.keyring.createAccount).toHaveBeenCalled(); - // Provider should now expose one account (newly created) - expect(provider.getAccounts()).toHaveLength(1); - }); - it('returns existing account if it already exists at index', async () => { const { provider, mocks } = setup({ accounts: [MOCK_TRX_ACCOUNT_1], @@ -447,13 +621,11 @@ describe('TrxAccountProvider', () => { it('does not run discovery if disabled', async () => { const { provider } = setup({ accounts: [MOCK_TRX_ACCOUNT_1], - config: { - ...TRX_ACCOUNT_PROVIDER_DEFAULT_CONFIG, + config: asConfig({ discovery: { - ...TRX_ACCOUNT_PROVIDER_DEFAULT_CONFIG.discovery, enabled: false, }, - }, + }), }); expect( diff --git a/packages/multichain-account-service/src/providers/TrxAccountProvider.ts b/packages/multichain-account-service/src/providers/TrxAccountProvider.ts index 64beb19c619..7dfb13beb0c 100644 --- a/packages/multichain-account-service/src/providers/TrxAccountProvider.ts +++ b/packages/multichain-account-service/src/providers/TrxAccountProvider.ts @@ -2,6 +2,8 @@ import { assertIsBip44Account } from '@metamask/account-api'; import type { Bip44Account } from '@metamask/account-api'; import type { TraceCallback } from '@metamask/controller-utils'; import type { + CreateAccountBip44DeriveIndexOptions, + CreateAccountBip44DeriveIndexRangeOptions, CreateAccountOptions, EntropySourceId, KeyringAccount, @@ -40,6 +42,7 @@ export const TRX_ACCOUNT_PROVIDER_DEFAULT_CONFIG: TrxAccountProviderConfig = { backOffMs: 1000, }, createAccounts: { + v2: false, // For now, the Snap is not fully v2 compliant. timeoutMs: 3000, }, }; @@ -76,29 +79,80 @@ export class TrxAccountProvider extends SnapAccountProvider { ); } - async #createAccounts({ - keyring, - entropySource, - groupIndex: index, - }: { - keyring: RestrictedSnapKeyring; - entropySource: EntropySourceId; - groupIndex: number; - }): Promise[]> { + async #createAccounts( + keyring: RestrictedSnapKeyring, + options: + | CreateAccountBip44DeriveIndexOptions + | CreateAccountBip44DeriveIndexRangeOptions, + ): Promise[]> { return this.withMaxConcurrency(async () => { - const account = await withTimeout( - keyring.createAccount({ - entropySource, - index, - addressType: TrxAccountType.Eoa, - scope: TrxScope.Mainnet, - }), - this.config.createAccounts.timeoutMs, - ); + let snapAccounts: KeyringAccount[] = []; + + const v2 = this.config.createAccounts.v2 ?? false; + + const { entropySource } = options; - assertIsBip44Account(account); - this.accounts.add(account.id); - return [account]; + if (options.type === `${AccountCreationType.Bip44DeriveIndexRange}`) { + if (v2) { + // Batch account creations. + snapAccounts = await withTimeout( + keyring.createAccounts(options), + this.config.createAccounts.timeoutMs, + ); + } else { + const { range } = options; + + // Create accounts one by one. + for ( + let groupIndex = range.from; + groupIndex <= range.to; + groupIndex++ + ) { + const snapAccount = await withTimeout( + keyring.createAccount({ + entropySource, + index: groupIndex, + addressType: TrxAccountType.Eoa, + scope: TrxScope.Mainnet, + }), + this.config.createAccounts.timeoutMs, + ); + + snapAccounts.push(snapAccount); + } + } + } else if (v2) { + // Create account using new v2-like flow (no async flow + no Snap keyring events). + const [snapAccount] = await withTimeout( + keyring.createAccounts(options), + this.config.createAccounts.timeoutMs, + ); + + snapAccounts = [snapAccount]; + } else { + const { groupIndex } = options; + + // Create account using the existing v1 flow. + const snapAccount = await withTimeout( + keyring.createAccount({ + entropySource, + index: groupIndex, + addressType: TrxAccountType.Eoa, + scope: TrxScope.Mainnet, + }), + this.config.createAccounts.timeoutMs, + ); + + snapAccounts = [snapAccount]; + } + + const accounts: Bip44Account[] = []; + for (const snapAccount of snapAccounts) { + assertIsBip44Account(snapAccount); + this.accounts.add(snapAccount.id); + accounts.push(snapAccount); + } + return accounts; }); } @@ -110,40 +164,8 @@ export class TrxAccountProvider extends SnapAccountProvider { `${AccountCreationType.Bip44DeriveIndexRange}`, ]); - const { entropySource } = options; - - if (options.type === AccountCreationType.Bip44DeriveIndexRange) { - const { range } = options; - return this.withSnap(async ({ keyring }) => { - const accounts: Bip44Account[] = []; - - // TODO: Use `SnapKeyring.createAccounts` when that functionality is implemented on the Snap - // itself, instead of creating accounts one by one. - for ( - let groupIndex = range.from; - groupIndex <= range.to; - groupIndex++ - ) { - const createdAccounts = await this.#createAccounts({ - keyring, - entropySource, - groupIndex, - }); - accounts.push(...createdAccounts); - } - - return accounts; - }); - } - - const { groupIndex } = options; - return this.withSnap(async ({ keyring }) => { - return this.#createAccounts({ - keyring, - entropySource, - groupIndex, - }); + return this.#createAccounts(keyring, options); }); } @@ -187,13 +209,11 @@ export class TrxAccountProvider extends SnapAccountProvider { return []; } - const createdAccounts = await this.#createAccounts({ - keyring, + return await this.#createAccounts(keyring, { + type: AccountCreationType.Bip44DeriveIndex, entropySource, groupIndex, }); - - return createdAccounts; }, ); }); diff --git a/packages/multichain-account-service/src/tests/index.ts b/packages/multichain-account-service/src/tests/index.ts index 4320db3bfbf..f7f84be2207 100644 --- a/packages/multichain-account-service/src/tests/index.ts +++ b/packages/multichain-account-service/src/tests/index.ts @@ -1,3 +1,4 @@ +export type * from './types'; export * from './accounts'; export * from './messenger'; export * from './providers'; diff --git a/packages/multichain-account-service/src/tests/providers.ts b/packages/multichain-account-service/src/tests/providers.ts index 9ae58c81850..45f99916b94 100644 --- a/packages/multichain-account-service/src/tests/providers.ts +++ b/packages/multichain-account-service/src/tests/providers.ts @@ -4,6 +4,7 @@ import type { KeyringAccount, KeyringCapabilities, } from '@metamask/keyring-api'; +import { GroupIndexRange } from 'src/utils'; import { AccountProviderWrapper, EvmAccountProvider } from '../providers'; @@ -140,3 +141,19 @@ export function mockCreateAccountsOnce( return created; }); } + +/** + * Helper to convert a group index range to an array of group indices, inclusive of the + * start and end indices. + * + * @param range - The range. + * @param range.from - The starting index of the range (inclusive). + * @param range.to - The ending index of the range (inclusive). + * @returns An array of group indices from `from` to `to`, inclusive. + */ +export function toGroupIndexRangeArray({ + from = 0, + to, +}: GroupIndexRange): number[] { + return Array.from({ length: to - from + 1 }, (_, i) => from + i); +} diff --git a/packages/multichain-account-service/src/tests/types.ts b/packages/multichain-account-service/src/tests/types.ts new file mode 100644 index 00000000000..9fb40a956d7 --- /dev/null +++ b/packages/multichain-account-service/src/tests/types.ts @@ -0,0 +1,14 @@ +/** + * A utility type that makes all properties of a type optional, recursively. + */ +export type DeepPartial = Type extends string + ? Type + : { + [Property in keyof Type]?: Type[Property] extends (infer Value)[] + ? DeepPartial[] + : Type[Property] extends readonly (infer Value)[] + ? readonly DeepPartial[] + : Type[Property] extends object + ? DeepPartial + : Type[Property]; + };