From 117ee20d3d4ab01811f34bb525ef02aa32bf7124 Mon Sep 17 00:00:00 2001 From: Monte Lai Date: Tue, 28 Oct 2025 21:26:28 +0800 Subject: [PATCH 01/16] feat: add tracing to multichain account service --- .../src/MultichainAccountService.test.ts | 25 +++++++ .../src/MultichainAccountService.ts | 17 ++++- .../src/MultichainAccountWallet.test.ts | 44 ++++++++++++ .../src/MultichainAccountWallet.ts | 72 ++++++++++++++----- .../src/constants/traces.ts | 3 + .../multichain-account-service/src/types.ts | 5 ++ 6 files changed, 145 insertions(+), 21 deletions(-) create mode 100644 packages/multichain-account-service/src/constants/traces.ts diff --git a/packages/multichain-account-service/src/MultichainAccountService.test.ts b/packages/multichain-account-service/src/MultichainAccountService.test.ts index 424b5f49805..a7d454c2e57 100644 --- a/packages/multichain-account-service/src/MultichainAccountService.test.ts +++ b/packages/multichain-account-service/src/MultichainAccountService.test.ts @@ -7,6 +7,7 @@ import { KeyringTypes, type KeyringObject } from '@metamask/keyring-controller'; import type { MultichainAccountServiceOptions } from './MultichainAccountService'; import { MultichainAccountService } from './MultichainAccountService'; +import { TraceName } from './constants/traces'; import type { NamedAccountProvider } from './providers'; import { AccountProviderWrapper } from './providers/AccountProviderWrapper'; import { @@ -226,6 +227,30 @@ describe('MultichainAccountService', () => { ); }); + it('accepts trace callback in config', () => { + const mockTrace = jest.fn().mockImplementation((_request, fn) => fn?.()); + const config = { trace: mockTrace }; + + // Test that the service can be constructed with a trace config + const rootMessenger = getRootMessenger(); + const messenger = getMultichainAccountServiceMessenger(rootMessenger); + + // Mock the required handlers for initialization + rootMessenger.registerActionHandler( + 'KeyringController:getState', + jest.fn().mockReturnValue({ isUnlocked: true, keyrings: [] }), + ); + + const serviceWithTrace = new MultichainAccountService({ + messenger, + config, + }); + + serviceWithTrace.init(); + + expect(serviceWithTrace).toBeDefined(); + }); + it('allows optional configs for some providers', () => { const providerConfigs: MultichainAccountServiceOptions['providerConfigs'] = { diff --git a/packages/multichain-account-service/src/MultichainAccountService.ts b/packages/multichain-account-service/src/MultichainAccountService.ts index 776e057b139..8a6a03e6b1d 100644 --- a/packages/multichain-account-service/src/MultichainAccountService.ts +++ b/packages/multichain-account-service/src/MultichainAccountService.ts @@ -6,6 +6,7 @@ import type { MultichainAccountWalletId, Bip44Account, } from '@metamask/account-api'; +import type { TraceCallback } from '@metamask/controller-utils'; import type { HdKeyring } from '@metamask/eth-hd-keyring'; import { mnemonicPhraseToBytes } from '@metamask/key-tree'; import type { EntropySourceId, KeyringAccount } from '@metamask/keyring-api'; @@ -26,7 +27,10 @@ import { } from './providers/AccountProviderWrapper'; import { EvmAccountProvider } from './providers/EvmAccountProvider'; import { SolAccountProvider } from './providers/SolAccountProvider'; -import type { MultichainAccountServiceMessenger } from './types'; +import type { + MultichainAccountServiceConfig, + MultichainAccountServiceMessenger, +} from './types'; export const serviceName = 'MultichainAccountService'; @@ -40,6 +44,7 @@ export type MultichainAccountServiceOptions = { [EvmAccountProvider.NAME]?: EvmAccountProviderConfig; [SolAccountProvider.NAME]?: SolAccountProviderConfig; }; + config?: MultichainAccountServiceConfig; }; /** Reverse mapping object used to map account IDs and their wallet/multichain account. */ @@ -66,6 +71,8 @@ export class MultichainAccountService { AccountContext> >; + readonly #trace: TraceCallback; + /** * The name of the service. */ @@ -79,16 +86,19 @@ export class MultichainAccountService { * MultichainAccountService. * @param options.providers - Optional list of account * @param options.providerConfigs - Optional provider configs - * providers. + * @param options.config - Optional config. */ constructor({ messenger, providers = [], providerConfigs, + config, }: MultichainAccountServiceOptions) { this.#messenger = messenger; this.#wallets = new Map(); this.#accountIdToContext = new Map(); + this.#trace = + config?.trace ?? (((_request, fn) => fn?.()) as TraceCallback); // TODO: Rely on keyring capabilities once the keyring API is used by all keyrings. this.#providers = [ @@ -181,6 +191,7 @@ export class MultichainAccountService { entropySource, providers: this.#providers, messenger: this.#messenger, + trace: this.#trace.bind(this), }); this.#wallets.set(wallet.id, wallet); @@ -220,6 +231,7 @@ export class MultichainAccountService { entropySource: account.options.entropy.id, providers: this.#providers, messenger: this.#messenger, + trace: this.#trace.bind(this), }); this.#wallets.set(wallet.id, wallet); @@ -377,6 +389,7 @@ export class MultichainAccountService { providers: this.#providers, entropySource: result.id, messenger: this.#messenger, + trace: this.#trace.bind(this), }); this.#wallets.set(wallet.id, wallet); diff --git a/packages/multichain-account-service/src/MultichainAccountWallet.test.ts b/packages/multichain-account-service/src/MultichainAccountWallet.test.ts index d87a2361881..b35d1419b88 100644 --- a/packages/multichain-account-service/src/MultichainAccountWallet.test.ts +++ b/packages/multichain-account-service/src/MultichainAccountWallet.test.ts @@ -14,6 +14,7 @@ import { } from '@metamask/keyring-api'; import type { InternalAccount } from '@metamask/keyring-internal-api'; +import { TraceName } from './constants/traces'; import { MultichainAccountWallet } from './MultichainAccountWallet'; import type { MockAccountProvider } from './tests'; import { @@ -341,6 +342,49 @@ describe('MultichainAccountWallet', () => { expect(mockSolProviderError).toHaveBeenCalled(); }); + it('calls trace callback with correct TraceName during account creation', async () => { + const groupIndex = 1; + const mockEvmAccount = MockAccountBuilder.from(MOCK_HD_ACCOUNT_1) + .withEntropySource(MOCK_HD_KEYRING_1.metadata.id) + .withGroupIndex(0) + .get(); + + const mockTrace = jest.fn().mockImplementation((_request, fn) => fn?.()); + + const { providers, messenger } = setup({ + accounts: [[mockEvmAccount], []], + }); + + const wallet = new MultichainAccountWallet({ + entropySource: MOCK_HD_KEYRING_1.metadata.id, + providers, + messenger, + trace: mockTrace, + }); + + const [evmProvider, solProvider] = providers; + const mockNextEvmAccount = MockAccountBuilder.from(mockEvmAccount) + .withEntropySource(MOCK_HD_KEYRING_1.metadata.id) + .withGroupIndex(groupIndex) + .get(); + + evmProvider.createAccounts.mockResolvedValueOnce([mockNextEvmAccount]); + solProvider.createAccounts.mockResolvedValueOnce([]); + + await wallet.createMultichainAccountGroup(groupIndex); + + // Verify trace was called with correct TraceName for non-EVM provider + expect(mockTrace).toHaveBeenCalledWith( + { + name: TraceName.SnapDiscoverAccounts, + data: { + providerName: expect.any(String), + }, + }, + expect.any(Function), + ); + }); + 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; diff --git a/packages/multichain-account-service/src/MultichainAccountWallet.ts b/packages/multichain-account-service/src/MultichainAccountWallet.ts index f90c0a73694..fa7910a3d4a 100644 --- a/packages/multichain-account-service/src/MultichainAccountWallet.ts +++ b/packages/multichain-account-service/src/MultichainAccountWallet.ts @@ -12,10 +12,12 @@ import { toDefaultAccountGroupId, toMultichainAccountWalletId, } from '@metamask/account-api'; +import type { TraceCallback } from '@metamask/controller-utils'; import type { EntropySourceId, KeyringAccount } from '@metamask/keyring-api'; import { assert } from '@metamask/utils'; import { Mutex } from 'async-mutex'; +import { TraceName } from './constants/traces'; import type { Logger } from './logger'; import { createModuleLogger, @@ -61,6 +63,8 @@ export class MultichainAccountWallet< readonly #log: Logger; + readonly #trace: TraceCallback; + // eslint-disable-next-line @typescript-eslint/prefer-readonly #initialized = false; @@ -70,17 +74,19 @@ export class MultichainAccountWallet< providers, entropySource, messenger, + trace, }: { providers: NamedAccountProvider[]; entropySource: EntropySourceId; messenger: MultichainAccountServiceMessenger; + trace?: TraceCallback; }) { this.#id = toMultichainAccountWalletId(entropySource); this.#providers = providers; this.#entropySource = entropySource; this.#messenger = messenger; this.#accountGroups = new Map(); - + this.#trace = trace ?? (((_request, fn) => fn?.()) as TraceCallback); this.#log = createModuleLogger(log, `[${this.#id}]`); // Initial synchronization (don't emit events during initialization). @@ -338,12 +344,22 @@ export class MultichainAccountWallet< 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, - }), - ), + this.#providers.map(async (provider) => { + return await this.#trace( + { + name: TraceName.SnapDiscoverAccounts, + data: { + providerName: provider.getName(), + }, + }, + async () => { + return await provider.createAccounts({ + entropySource: this.#entropySource, + groupIndex, + }); + }, + ); + }), ); // If any of the provider failed to create their accounts, then we consider the @@ -388,18 +404,36 @@ export class MultichainAccountWallet< } // Create account with other providers in the background - otherProviders.forEach((provider) => { - provider - .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); - }); - }); + for (const provider of otherProviders) { + (async () => { + await this.#trace( + { + name: TraceName.SnapDiscoverAccounts, + data: { + providerName: provider.getName(), + }, + }, + async () => { + await provider + .createAccounts({ + entropySource: this.#entropySource, + groupIndex, + }) + .catch((error) => { + // Log errors from background providers but don't fail the operation + const errorMessage = `Could not create account with provider "${provider.getName()}" for multichain account group index: ${groupIndex}`; + this.#log(`${WARNING_PREFIX} ${errorMessage}:`, error); + }); + }, + ); + })().catch((error) => { + // Handle any unexpected errors in the background operation + this.#log( + `${ERROR_PREFIX} Unexpected error in background account creation:`, + error, + ); + }); + } } // -------------------------------------------------------------------------------- diff --git a/packages/multichain-account-service/src/constants/traces.ts b/packages/multichain-account-service/src/constants/traces.ts new file mode 100644 index 00000000000..bc3d8754dd3 --- /dev/null +++ b/packages/multichain-account-service/src/constants/traces.ts @@ -0,0 +1,3 @@ +export enum TraceName { + 'SnapDiscoverAccounts' = 'Snap Discover Accounts', +} diff --git a/packages/multichain-account-service/src/types.ts b/packages/multichain-account-service/src/types.ts index a0c9f84879c..263a5b5a61e 100644 --- a/packages/multichain-account-service/src/types.ts +++ b/packages/multichain-account-service/src/types.ts @@ -11,6 +11,7 @@ import type { AccountsControllerGetAccountByAddressAction, AccountsControllerListMultichainAccountsAction, } from '@metamask/accounts-controller'; +import type { TraceCallback } from '@metamask/controller-utils'; import type { KeyringAccount } from '@metamask/keyring-api'; import type { KeyringControllerAddNewKeyringAction, @@ -155,3 +156,7 @@ export type MultichainAccountServiceMessenger = Messenger< MultichainAccountServiceActions | AllowedActions, MultichainAccountServiceEvents | AllowedEvents >; + +export type MultichainAccountServiceConfig = { + trace?: TraceCallback; +}; From af3d908aa42d4587626283aa02e78d02788d3c69 Mon Sep 17 00:00:00 2001 From: Monte Lai Date: Thu, 30 Oct 2025 18:05:03 +0800 Subject: [PATCH 02/16] chore: update changelog --- packages/multichain-account-service/CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/multichain-account-service/CHANGELOG.md b/packages/multichain-account-service/CHANGELOG.md index 07fd7b51c8e..9a6472e9803 100644 --- a/packages/multichain-account-service/CHANGELOG.md +++ b/packages/multichain-account-service/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed + +- Adds an optional trace argument to MultichainAccountService and MultichainAccountWallet ([#7006](https://github.com/MetaMask/core/pull/7006)) + ## [2.0.0] ### Changed From d5c1eb398e8ab1ee990f7ea0b939d9503785fae6 Mon Sep 17 00:00:00 2001 From: Monte Lai Date: Thu, 30 Oct 2025 21:19:46 +0800 Subject: [PATCH 03/16] fix: lint --- .../src/MultichainAccountService.test.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/multichain-account-service/src/MultichainAccountService.test.ts b/packages/multichain-account-service/src/MultichainAccountService.test.ts index a7d454c2e57..c42f65c0123 100644 --- a/packages/multichain-account-service/src/MultichainAccountService.test.ts +++ b/packages/multichain-account-service/src/MultichainAccountService.test.ts @@ -7,7 +7,6 @@ import { KeyringTypes, type KeyringObject } from '@metamask/keyring-controller'; import type { MultichainAccountServiceOptions } from './MultichainAccountService'; import { MultichainAccountService } from './MultichainAccountService'; -import { TraceName } from './constants/traces'; import type { NamedAccountProvider } from './providers'; import { AccountProviderWrapper } from './providers/AccountProviderWrapper'; import { From 3190bca86d816af341a08499a395ad3ebf8650aa Mon Sep 17 00:00:00 2001 From: Monte Lai Date: Thu, 30 Oct 2025 21:52:02 +0800 Subject: [PATCH 04/16] fix: add test coverage --- .../src/MultichainAccountWallet.test.ts | 180 ++++++++++++++++++ 1 file changed, 180 insertions(+) diff --git a/packages/multichain-account-service/src/MultichainAccountWallet.test.ts b/packages/multichain-account-service/src/MultichainAccountWallet.test.ts index b35d1419b88..398a9bdcf00 100644 --- a/packages/multichain-account-service/src/MultichainAccountWallet.test.ts +++ b/packages/multichain-account-service/src/MultichainAccountWallet.test.ts @@ -98,6 +98,50 @@ describe('MultichainAccountWallet', () => { expect(wallet.entropySource).toStrictEqual(entropySource); expect(wallet.getMultichainAccountGroups()).toHaveLength(1); // All internal accounts are using index 0, so it means only 1 multichain account. }); + + it('constructs a multichain account wallet with default trace callback', () => { + const entropySource = MOCK_WALLET_1_ENTROPY_SOURCE; + const providers = [ + setupNamedAccountProvider({ accounts: [MOCK_WALLET_1_EVM_ACCOUNT] }), + ]; + const messenger = + getMultichainAccountServiceMessenger(getRootMessenger()); + + const wallet = new MultichainAccountWallet({ + entropySource, + providers, + messenger, + }); + + const expectedWalletId = toMultichainAccountWalletId(entropySource); + expect(wallet.id).toStrictEqual(expectedWalletId); + expect(wallet.status).toBe('ready'); + expect(wallet.type).toBe(AccountWalletType.Entropy); + expect(wallet.entropySource).toStrictEqual(entropySource); + }); + + it('constructs a multichain account wallet with custom trace callback', () => { + const entropySource = MOCK_WALLET_1_ENTROPY_SOURCE; + const providers = [ + setupNamedAccountProvider({ accounts: [MOCK_WALLET_1_EVM_ACCOUNT] }), + ]; + const messenger = + getMultichainAccountServiceMessenger(getRootMessenger()); + const mockTrace = jest.fn().mockImplementation((_request, fn) => fn?.()); + + const wallet = new MultichainAccountWallet({ + entropySource, + providers, + messenger, + trace: mockTrace, + }); + + const expectedWalletId = toMultichainAccountWalletId(entropySource); + expect(wallet.id).toStrictEqual(expectedWalletId); + expect(wallet.status).toBe('ready'); + expect(wallet.type).toBe(AccountWalletType.Entropy); + expect(wallet.entropySource).toStrictEqual(entropySource); + }); }); describe('getMultichainAccountGroup', () => { @@ -385,6 +429,47 @@ describe('MultichainAccountWallet', () => { ); }); + it('handles unexpected errors in background account creation', async () => { + const groupIndex = 1; + const mockEvmAccount = MockAccountBuilder.from(MOCK_HD_ACCOUNT_1) + .withEntropySource(MOCK_HD_KEYRING_1.metadata.id) + .withGroupIndex(0) + .get(); + + const { providers, messenger } = setup({ + accounts: [[mockEvmAccount], []], + }); + + const mockTrace = jest.fn().mockImplementation(() => { + throw new Error('Unexpected trace error'); + }); + + const wallet = new MultichainAccountWallet({ + entropySource: MOCK_HD_KEYRING_1.metadata.id, + providers, + messenger, + trace: mockTrace, + }); + + const [evmProvider, solProvider] = providers; + const mockNextEvmAccount = MockAccountBuilder.from(mockEvmAccount) + .withEntropySource(MOCK_HD_KEYRING_1.metadata.id) + .withGroupIndex(groupIndex) + .get(); + + evmProvider.createAccounts.mockResolvedValueOnce([mockNextEvmAccount]); + solProvider.createAccounts.mockResolvedValueOnce([]); + + await wallet.createMultichainAccountGroup(groupIndex); + + // Wait for background operations to complete + await new Promise((resolve) => setTimeout(resolve, 10)); + + // The test passes if no unhandled promise rejection occurs + // The error handling is internal and covered by the catch block + expect(wallet.status).toBe('ready'); + }); + 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; @@ -410,6 +495,52 @@ describe('MultichainAccountWallet', () => { }); }); + describe('getNextGroupIndex', () => { + it('returns 0 when no groups exist', () => { + const { wallet } = setup({ + accounts: [[], []], // No accounts + }); + + expect(wallet.getNextGroupIndex()).toBe(0); + }); + + it('returns the next sequential index', () => { + const mockEvmAccount1 = MockAccountBuilder.from(MOCK_HD_ACCOUNT_1) + .withEntropySource(MOCK_HD_KEYRING_1.metadata.id) + .withGroupIndex(0) + .get(); + const mockEvmAccount2 = MockAccountBuilder.from(MOCK_HD_ACCOUNT_1) + .withEntropySource(MOCK_HD_KEYRING_1.metadata.id) + .withGroupIndex(2) + .get(); + + const { wallet } = setup({ + accounts: [[mockEvmAccount1, mockEvmAccount2], []], + }); + + // Should return 3 (max index 2 + 1), not filling gaps + expect(wallet.getNextGroupIndex()).toBe(3); + }); + + it('handles non-sequential group indices correctly', () => { + const mockEvmAccount1 = MockAccountBuilder.from(MOCK_HD_ACCOUNT_1) + .withEntropySource(MOCK_HD_KEYRING_1.metadata.id) + .withGroupIndex(5) + .get(); + const mockEvmAccount2 = MockAccountBuilder.from(MOCK_HD_ACCOUNT_1) + .withEntropySource(MOCK_HD_KEYRING_1.metadata.id) + .withGroupIndex(10) + .get(); + + const { wallet } = setup({ + accounts: [[mockEvmAccount1, mockEvmAccount2], []], + }); + + // Should return 11 (max index 10 + 1) + expect(wallet.getNextGroupIndex()).toBe(11); + }); + }); + describe('createNextMultichainAccountGroup', () => { it('creates the next multichain account group (with multiple providers)', async () => { const mockEvmAccount = MockAccountBuilder.from(MOCK_HD_ACCOUNT_1) @@ -510,6 +641,55 @@ describe('MultichainAccountWallet', () => { }); }); + describe('#withLock', () => { + it('properly manages wallet status during locked operations', async () => { + const { wallet, messenger } = setup({ + accounts: [[MOCK_HD_ACCOUNT_1], []], + }); + + const statusChanges: string[] = []; + messenger.subscribe( + 'MultichainAccountService:walletStatusChange', + (_walletId, status) => { + statusChanges.push(status); + }, + ); + + await wallet.alignAccounts(); + + expect(statusChanges).toStrictEqual(['in-progress:alignment', 'ready']); + }); + + it('properly handles errors in locked operations and restores status', async () => { + const { wallet, providers, messenger } = setup({ + accounts: [[MOCK_HD_ACCOUNT_1], []], + }); + + const statusChanges: string[] = []; + messenger.subscribe( + 'MultichainAccountService:walletStatusChange', + (_walletId, status) => { + statusChanges.push(status); + }, + ); + + const [evmProvider] = providers; + evmProvider.createAccounts.mockRejectedValueOnce( + new Error('Provider error'), + ); + + await expect(wallet.createNextMultichainAccountGroup()).rejects.toThrow( + 'Unable to create multichain account group for index: 1', + ); + + expect(statusChanges).toStrictEqual([ + 'in-progress:create-accounts', + 'ready', + ]); + expect(wallet.status).toBe('ready'); + }); + }); + describe('alignGroup', () => { it('aligns a specific multichain account group', async () => { const mockEvmAccount = MockAccountBuilder.from(MOCK_HD_ACCOUNT_1) From 517099480949c87b064b9568cf0a35a245cf6f3e Mon Sep 17 00:00:00 2001 From: Monte Lai Date: Fri, 31 Oct 2025 15:01:48 +0800 Subject: [PATCH 05/16] refactor: discover to AccountProviderWrapper --- .../src/MultichainAccountService.test.ts | 51 ++++ .../src/MultichainAccountService.ts | 8 +- .../src/MultichainAccountWallet.test.ts | 224 ------------------ .../src/MultichainAccountWallet.ts | 72 ++---- .../src/analytics/index.ts | 1 + .../src/analytics/traces.test.ts | 76 ++++++ .../src/analytics/traces.ts | 25 ++ .../src/constants/traces.ts | 1 + .../src/providers/AccountProviderWrapper.ts | 17 +- .../src/providers/EvmAccountProvider.test.ts | 115 +++++++++ .../src/providers/EvmAccountProvider.ts | 75 +++--- 11 files changed, 353 insertions(+), 312 deletions(-) create mode 100644 packages/multichain-account-service/src/analytics/index.ts create mode 100644 packages/multichain-account-service/src/analytics/traces.test.ts create mode 100644 packages/multichain-account-service/src/analytics/traces.ts diff --git a/packages/multichain-account-service/src/MultichainAccountService.test.ts b/packages/multichain-account-service/src/MultichainAccountService.test.ts index c42f65c0123..8ae339d87c8 100644 --- a/packages/multichain-account-service/src/MultichainAccountService.test.ts +++ b/packages/multichain-account-service/src/MultichainAccountService.test.ts @@ -1114,6 +1114,57 @@ describe('MultichainAccountService', () => { (solProvider.isAccountCompatible as jest.Mock).mockReturnValue(false); expect(wrapper.isAccountCompatible(MOCK_HD_ACCOUNT_1)).toBe(false); }); + + it('calls trace callback when discoverAccounts() is enabled', async () => { + const mockTrace = jest.fn().mockImplementation(async (request, fn) => { + expect(request.name).toBe('Snap Discover Accounts'); + expect(request.data).toStrictEqual({ + providerName: solProvider.getName(), + }); + return await fn(); + }); + + const options = { + entropySource: MOCK_HD_ACCOUNT_1.options.entropy.id, + groupIndex: 0, + }; + + // Create wrapper with custom trace callback + const wrapperWithTrace = new AccountProviderWrapper( + getMultichainAccountServiceMessenger( + setup({ accounts: [MOCK_HD_ACCOUNT_1] }).rootMessenger, + ), + solProvider, + mockTrace, + ); + + (solProvider.discoverAccounts as jest.Mock).mockResolvedValue([ + MOCK_HD_ACCOUNT_1, + ]); + + const result = await wrapperWithTrace.discoverAccounts(options); + + expect(result).toStrictEqual([MOCK_HD_ACCOUNT_1]); + expect(mockTrace).toHaveBeenCalledTimes(1); + expect(solProvider.discoverAccounts).toHaveBeenCalledWith(options); + }); + + it('uses fallback trace when no trace callback is provided', async () => { + const options = { + entropySource: MOCK_HD_ACCOUNT_1.options.entropy.id, + groupIndex: 0, + }; + + (solProvider.discoverAccounts as jest.Mock).mockResolvedValue([ + MOCK_HD_ACCOUNT_1, + ]); + + // Wrapper without trace callback should use fallback + const result = await wrapper.discoverAccounts(options); + + expect(result).toStrictEqual([MOCK_HD_ACCOUNT_1]); + expect(solProvider.discoverAccounts).toHaveBeenCalledWith(options); + }); }); describe('createMultichainAccountWallet', () => { diff --git a/packages/multichain-account-service/src/MultichainAccountService.ts b/packages/multichain-account-service/src/MultichainAccountService.ts index 8a6a03e6b1d..561315f3cef 100644 --- a/packages/multichain-account-service/src/MultichainAccountService.ts +++ b/packages/multichain-account-service/src/MultichainAccountService.ts @@ -13,6 +13,7 @@ import type { EntropySourceId, KeyringAccount } from '@metamask/keyring-api'; import { KeyringTypes } from '@metamask/keyring-controller'; import { areUint8ArraysEqual } from '@metamask/utils'; +import { traceFallback } from './analytics'; import { projectLogger as log } from './logger'; import type { MultichainAccountGroup } from './MultichainAccountGroup'; import { MultichainAccountWallet } from './MultichainAccountWallet'; @@ -97,8 +98,7 @@ export class MultichainAccountService { this.#messenger = messenger; this.#wallets = new Map(); this.#accountIdToContext = new Map(); - this.#trace = - config?.trace ?? (((_request, fn) => fn?.()) as TraceCallback); + this.#trace = config?.trace ?? traceFallback; // TODO: Rely on keyring capabilities once the keyring API is used by all keyrings. this.#providers = [ @@ -112,6 +112,7 @@ export class MultichainAccountService { this.#messenger, providerConfigs?.[SolAccountProvider.NAME], ), + this.#trace, ), // Custom account providers that can be provided by the MetaMask client. ...providers, @@ -191,7 +192,6 @@ export class MultichainAccountService { entropySource, providers: this.#providers, messenger: this.#messenger, - trace: this.#trace.bind(this), }); this.#wallets.set(wallet.id, wallet); @@ -231,7 +231,6 @@ export class MultichainAccountService { entropySource: account.options.entropy.id, providers: this.#providers, messenger: this.#messenger, - trace: this.#trace.bind(this), }); this.#wallets.set(wallet.id, wallet); @@ -389,7 +388,6 @@ export class MultichainAccountService { providers: this.#providers, entropySource: result.id, messenger: this.#messenger, - trace: this.#trace.bind(this), }); this.#wallets.set(wallet.id, wallet); diff --git a/packages/multichain-account-service/src/MultichainAccountWallet.test.ts b/packages/multichain-account-service/src/MultichainAccountWallet.test.ts index 398a9bdcf00..d87a2361881 100644 --- a/packages/multichain-account-service/src/MultichainAccountWallet.test.ts +++ b/packages/multichain-account-service/src/MultichainAccountWallet.test.ts @@ -14,7 +14,6 @@ import { } from '@metamask/keyring-api'; import type { InternalAccount } from '@metamask/keyring-internal-api'; -import { TraceName } from './constants/traces'; import { MultichainAccountWallet } from './MultichainAccountWallet'; import type { MockAccountProvider } from './tests'; import { @@ -98,50 +97,6 @@ describe('MultichainAccountWallet', () => { expect(wallet.entropySource).toStrictEqual(entropySource); expect(wallet.getMultichainAccountGroups()).toHaveLength(1); // All internal accounts are using index 0, so it means only 1 multichain account. }); - - it('constructs a multichain account wallet with default trace callback', () => { - const entropySource = MOCK_WALLET_1_ENTROPY_SOURCE; - const providers = [ - setupNamedAccountProvider({ accounts: [MOCK_WALLET_1_EVM_ACCOUNT] }), - ]; - const messenger = - getMultichainAccountServiceMessenger(getRootMessenger()); - - const wallet = new MultichainAccountWallet({ - entropySource, - providers, - messenger, - }); - - const expectedWalletId = toMultichainAccountWalletId(entropySource); - expect(wallet.id).toStrictEqual(expectedWalletId); - expect(wallet.status).toBe('ready'); - expect(wallet.type).toBe(AccountWalletType.Entropy); - expect(wallet.entropySource).toStrictEqual(entropySource); - }); - - it('constructs a multichain account wallet with custom trace callback', () => { - const entropySource = MOCK_WALLET_1_ENTROPY_SOURCE; - const providers = [ - setupNamedAccountProvider({ accounts: [MOCK_WALLET_1_EVM_ACCOUNT] }), - ]; - const messenger = - getMultichainAccountServiceMessenger(getRootMessenger()); - const mockTrace = jest.fn().mockImplementation((_request, fn) => fn?.()); - - const wallet = new MultichainAccountWallet({ - entropySource, - providers, - messenger, - trace: mockTrace, - }); - - const expectedWalletId = toMultichainAccountWalletId(entropySource); - expect(wallet.id).toStrictEqual(expectedWalletId); - expect(wallet.status).toBe('ready'); - expect(wallet.type).toBe(AccountWalletType.Entropy); - expect(wallet.entropySource).toStrictEqual(entropySource); - }); }); describe('getMultichainAccountGroup', () => { @@ -386,90 +341,6 @@ describe('MultichainAccountWallet', () => { expect(mockSolProviderError).toHaveBeenCalled(); }); - it('calls trace callback with correct TraceName during account creation', async () => { - const groupIndex = 1; - const mockEvmAccount = MockAccountBuilder.from(MOCK_HD_ACCOUNT_1) - .withEntropySource(MOCK_HD_KEYRING_1.metadata.id) - .withGroupIndex(0) - .get(); - - const mockTrace = jest.fn().mockImplementation((_request, fn) => fn?.()); - - const { providers, messenger } = setup({ - accounts: [[mockEvmAccount], []], - }); - - const wallet = new MultichainAccountWallet({ - entropySource: MOCK_HD_KEYRING_1.metadata.id, - providers, - messenger, - trace: mockTrace, - }); - - const [evmProvider, solProvider] = providers; - const mockNextEvmAccount = MockAccountBuilder.from(mockEvmAccount) - .withEntropySource(MOCK_HD_KEYRING_1.metadata.id) - .withGroupIndex(groupIndex) - .get(); - - evmProvider.createAccounts.mockResolvedValueOnce([mockNextEvmAccount]); - solProvider.createAccounts.mockResolvedValueOnce([]); - - await wallet.createMultichainAccountGroup(groupIndex); - - // Verify trace was called with correct TraceName for non-EVM provider - expect(mockTrace).toHaveBeenCalledWith( - { - name: TraceName.SnapDiscoverAccounts, - data: { - providerName: expect.any(String), - }, - }, - expect.any(Function), - ); - }); - - it('handles unexpected errors in background account creation', async () => { - const groupIndex = 1; - const mockEvmAccount = MockAccountBuilder.from(MOCK_HD_ACCOUNT_1) - .withEntropySource(MOCK_HD_KEYRING_1.metadata.id) - .withGroupIndex(0) - .get(); - - const { providers, messenger } = setup({ - accounts: [[mockEvmAccount], []], - }); - - const mockTrace = jest.fn().mockImplementation(() => { - throw new Error('Unexpected trace error'); - }); - - const wallet = new MultichainAccountWallet({ - entropySource: MOCK_HD_KEYRING_1.metadata.id, - providers, - messenger, - trace: mockTrace, - }); - - const [evmProvider, solProvider] = providers; - const mockNextEvmAccount = MockAccountBuilder.from(mockEvmAccount) - .withEntropySource(MOCK_HD_KEYRING_1.metadata.id) - .withGroupIndex(groupIndex) - .get(); - - evmProvider.createAccounts.mockResolvedValueOnce([mockNextEvmAccount]); - solProvider.createAccounts.mockResolvedValueOnce([]); - - await wallet.createMultichainAccountGroup(groupIndex); - - // Wait for background operations to complete - await new Promise((resolve) => setTimeout(resolve, 10)); - - // The test passes if no unhandled promise rejection occurs - // The error handling is internal and covered by the catch block - expect(wallet.status).toBe('ready'); - }); - 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; @@ -495,52 +366,6 @@ describe('MultichainAccountWallet', () => { }); }); - describe('getNextGroupIndex', () => { - it('returns 0 when no groups exist', () => { - const { wallet } = setup({ - accounts: [[], []], // No accounts - }); - - expect(wallet.getNextGroupIndex()).toBe(0); - }); - - it('returns the next sequential index', () => { - const mockEvmAccount1 = MockAccountBuilder.from(MOCK_HD_ACCOUNT_1) - .withEntropySource(MOCK_HD_KEYRING_1.metadata.id) - .withGroupIndex(0) - .get(); - const mockEvmAccount2 = MockAccountBuilder.from(MOCK_HD_ACCOUNT_1) - .withEntropySource(MOCK_HD_KEYRING_1.metadata.id) - .withGroupIndex(2) - .get(); - - const { wallet } = setup({ - accounts: [[mockEvmAccount1, mockEvmAccount2], []], - }); - - // Should return 3 (max index 2 + 1), not filling gaps - expect(wallet.getNextGroupIndex()).toBe(3); - }); - - it('handles non-sequential group indices correctly', () => { - const mockEvmAccount1 = MockAccountBuilder.from(MOCK_HD_ACCOUNT_1) - .withEntropySource(MOCK_HD_KEYRING_1.metadata.id) - .withGroupIndex(5) - .get(); - const mockEvmAccount2 = MockAccountBuilder.from(MOCK_HD_ACCOUNT_1) - .withEntropySource(MOCK_HD_KEYRING_1.metadata.id) - .withGroupIndex(10) - .get(); - - const { wallet } = setup({ - accounts: [[mockEvmAccount1, mockEvmAccount2], []], - }); - - // Should return 11 (max index 10 + 1) - expect(wallet.getNextGroupIndex()).toBe(11); - }); - }); - describe('createNextMultichainAccountGroup', () => { it('creates the next multichain account group (with multiple providers)', async () => { const mockEvmAccount = MockAccountBuilder.from(MOCK_HD_ACCOUNT_1) @@ -641,55 +466,6 @@ describe('MultichainAccountWallet', () => { }); }); - describe('#withLock', () => { - it('properly manages wallet status during locked operations', async () => { - const { wallet, messenger } = setup({ - accounts: [[MOCK_HD_ACCOUNT_1], []], - }); - - const statusChanges: string[] = []; - messenger.subscribe( - 'MultichainAccountService:walletStatusChange', - (_walletId, status) => { - statusChanges.push(status); - }, - ); - - await wallet.alignAccounts(); - - expect(statusChanges).toStrictEqual(['in-progress:alignment', 'ready']); - }); - - it('properly handles errors in locked operations and restores status', async () => { - const { wallet, providers, messenger } = setup({ - accounts: [[MOCK_HD_ACCOUNT_1], []], - }); - - const statusChanges: string[] = []; - messenger.subscribe( - 'MultichainAccountService:walletStatusChange', - (_walletId, status) => { - statusChanges.push(status); - }, - ); - - const [evmProvider] = providers; - evmProvider.createAccounts.mockRejectedValueOnce( - new Error('Provider error'), - ); - - await expect(wallet.createNextMultichainAccountGroup()).rejects.toThrow( - 'Unable to create multichain account group for index: 1', - ); - - expect(statusChanges).toStrictEqual([ - 'in-progress:create-accounts', - 'ready', - ]); - expect(wallet.status).toBe('ready'); - }); - }); - describe('alignGroup', () => { it('aligns a specific multichain account group', async () => { const mockEvmAccount = MockAccountBuilder.from(MOCK_HD_ACCOUNT_1) diff --git a/packages/multichain-account-service/src/MultichainAccountWallet.ts b/packages/multichain-account-service/src/MultichainAccountWallet.ts index fa7910a3d4a..f90c0a73694 100644 --- a/packages/multichain-account-service/src/MultichainAccountWallet.ts +++ b/packages/multichain-account-service/src/MultichainAccountWallet.ts @@ -12,12 +12,10 @@ import { toDefaultAccountGroupId, toMultichainAccountWalletId, } from '@metamask/account-api'; -import type { TraceCallback } from '@metamask/controller-utils'; import type { EntropySourceId, KeyringAccount } from '@metamask/keyring-api'; import { assert } from '@metamask/utils'; import { Mutex } from 'async-mutex'; -import { TraceName } from './constants/traces'; import type { Logger } from './logger'; import { createModuleLogger, @@ -63,8 +61,6 @@ export class MultichainAccountWallet< readonly #log: Logger; - readonly #trace: TraceCallback; - // eslint-disable-next-line @typescript-eslint/prefer-readonly #initialized = false; @@ -74,19 +70,17 @@ export class MultichainAccountWallet< providers, entropySource, messenger, - trace, }: { providers: NamedAccountProvider[]; entropySource: EntropySourceId; messenger: MultichainAccountServiceMessenger; - trace?: TraceCallback; }) { this.#id = toMultichainAccountWalletId(entropySource); this.#providers = providers; this.#entropySource = entropySource; this.#messenger = messenger; this.#accountGroups = new Map(); - this.#trace = trace ?? (((_request, fn) => fn?.()) as TraceCallback); + this.#log = createModuleLogger(log, `[${this.#id}]`); // Initial synchronization (don't emit events during initialization). @@ -344,22 +338,12 @@ export class MultichainAccountWallet< if (options?.waitForAllProvidersToFinishCreatingAccounts) { // Create account with all providers and await them. const results = await Promise.allSettled( - this.#providers.map(async (provider) => { - return await this.#trace( - { - name: TraceName.SnapDiscoverAccounts, - data: { - providerName: provider.getName(), - }, - }, - async () => { - return await provider.createAccounts({ - entropySource: this.#entropySource, - groupIndex, - }); - }, - ); - }), + this.#providers.map((provider) => + provider.createAccounts({ + entropySource: this.#entropySource, + groupIndex, + }), + ), ); // If any of the provider failed to create their accounts, then we consider the @@ -404,36 +388,18 @@ export class MultichainAccountWallet< } // Create account with other providers in the background - for (const provider of otherProviders) { - (async () => { - await this.#trace( - { - name: TraceName.SnapDiscoverAccounts, - data: { - providerName: provider.getName(), - }, - }, - async () => { - await provider - .createAccounts({ - entropySource: this.#entropySource, - groupIndex, - }) - .catch((error) => { - // Log errors from background providers but don't fail the operation - const errorMessage = `Could not create account with provider "${provider.getName()}" for multichain account group index: ${groupIndex}`; - this.#log(`${WARNING_PREFIX} ${errorMessage}:`, error); - }); - }, - ); - })().catch((error) => { - // Handle any unexpected errors in the background operation - this.#log( - `${ERROR_PREFIX} Unexpected error in background account creation:`, - error, - ); - }); - } + otherProviders.forEach((provider) => { + provider + .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); + }); + }); } // -------------------------------------------------------------------------------- diff --git a/packages/multichain-account-service/src/analytics/index.ts b/packages/multichain-account-service/src/analytics/index.ts new file mode 100644 index 00000000000..bc9f922b435 --- /dev/null +++ b/packages/multichain-account-service/src/analytics/index.ts @@ -0,0 +1 @@ +export * from './traces'; diff --git a/packages/multichain-account-service/src/analytics/traces.test.ts b/packages/multichain-account-service/src/analytics/traces.test.ts new file mode 100644 index 00000000000..47cc45728c0 --- /dev/null +++ b/packages/multichain-account-service/src/analytics/traces.test.ts @@ -0,0 +1,76 @@ +import type { TraceRequest } from '@metamask/controller-utils'; +import { TraceName } from '../constants/traces'; + +import { traceFallback } from './traces'; + +describe('MultichainAccountService - Traces', () => { + describe('TraceName', () => { + it('contains expected trace names', () => { + expect(TraceName).toStrictEqual({ + SnapDiscoverAccounts: 'Snap Discover Accounts', + EvmDiscoverAccounts: 'EVM Discover Accounts', + }); + }); + }); + + describe('traceFallback', () => { + let mockTraceRequest: TraceRequest; + + beforeEach(() => { + mockTraceRequest = { + name: TraceName.SnapDiscoverAccounts, + id: 'trace-id-123', + tags: {}, + }; + }); + + it('returns undefined when no function is provided', async () => { + const result = await traceFallback(mockTraceRequest); + + expect(result).toBeUndefined(); + }); + + it('executes the provided function and return its result', async () => { + const mockResult = 'test-result'; + const mockFn = jest.fn().mockReturnValue(mockResult); + + const result = await traceFallback(mockTraceRequest, mockFn); + + expect(mockFn).toHaveBeenCalledTimes(1); + expect(mockFn).toHaveBeenCalledWith(); + expect(result).toBe(mockResult); + }); + + it('executes async function and return its result', async () => { + const mockResult = { data: 'async-result' }; + const mockAsyncFn = jest.fn().mockResolvedValue(mockResult); + + const result = await traceFallback(mockTraceRequest, mockAsyncFn); + + expect(mockAsyncFn).toHaveBeenCalledTimes(1); + expect(result).toBe(mockResult); + }); + + it('handles function that throws an error', async () => { + const mockError = new Error('Test error'); + const mockFn = jest.fn().mockImplementation(() => { + throw mockError; + }); + + await expect(traceFallback(mockTraceRequest, mockFn)).rejects.toThrow( + mockError, + ); + expect(mockFn).toHaveBeenCalledTimes(1); + }); + + it('handles function that returns a rejected promise', async () => { + const mockError = new Error('Async error'); + const mockFn = jest.fn().mockRejectedValue(mockError); + + await expect(traceFallback(mockTraceRequest, mockFn)).rejects.toThrow( + mockError, + ); + expect(mockFn).toHaveBeenCalledTimes(1); + }); + }); +}); diff --git a/packages/multichain-account-service/src/analytics/traces.ts b/packages/multichain-account-service/src/analytics/traces.ts new file mode 100644 index 00000000000..57eee11c914 --- /dev/null +++ b/packages/multichain-account-service/src/analytics/traces.ts @@ -0,0 +1,25 @@ +import type { + TraceCallback, + TraceContext, + TraceRequest, +} from '@metamask/controller-utils'; + +/** + * Fallback function for tracing. + * This function is used when no specific trace function is provided. + * It executes the provided function in a trace context if available. + * + * @param _request - The trace request containing additional data and context. + * @param fn - The function to execute within the trace context. + * @returns A promise that resolves to the result of the executed function. + * If no function is provided, it resolves to undefined. + */ +export const traceFallback: TraceCallback = async ( + _request: TraceRequest, + fn?: (context?: TraceContext) => ReturnType, +): Promise => { + if (!fn) { + return undefined as ReturnType; + } + return await Promise.resolve(fn()); +}; diff --git a/packages/multichain-account-service/src/constants/traces.ts b/packages/multichain-account-service/src/constants/traces.ts index bc3d8754dd3..59d74c0a9da 100644 --- a/packages/multichain-account-service/src/constants/traces.ts +++ b/packages/multichain-account-service/src/constants/traces.ts @@ -1,3 +1,4 @@ export enum TraceName { 'SnapDiscoverAccounts' = 'Snap Discover Accounts', + 'EvmDiscoverAccounts' = 'EVM Discover Accounts', } diff --git a/packages/multichain-account-service/src/providers/AccountProviderWrapper.ts b/packages/multichain-account-service/src/providers/AccountProviderWrapper.ts index 51ff6cb2c20..3ad3345db9c 100644 --- a/packages/multichain-account-service/src/providers/AccountProviderWrapper.ts +++ b/packages/multichain-account-service/src/providers/AccountProviderWrapper.ts @@ -1,7 +1,10 @@ import type { Bip44Account } from '@metamask/account-api'; +import type { TraceCallback } from '@metamask/controller-utils'; import type { EntropySourceId, KeyringAccount } from '@metamask/keyring-api'; import { BaseBip44AccountProvider } from './BaseBip44AccountProvider'; +import { traceFallback } from '../analytics'; +import { TraceName } from '../constants/traces'; import type { MultichainAccountServiceMessenger } from '../types'; /** @@ -13,12 +16,16 @@ export class AccountProviderWrapper extends BaseBip44AccountProvider { private readonly provider: BaseBip44AccountProvider; + readonly #trace: TraceCallback; + constructor( messenger: MultichainAccountServiceMessenger, provider: BaseBip44AccountProvider, + trace?: TraceCallback, ) { super(messenger); this.provider = provider; + this.#trace = trace ?? traceFallback; } override getName(): string { @@ -106,7 +113,15 @@ export class AccountProviderWrapper extends BaseBip44AccountProvider { if (!this.isEnabled) { return []; } - return this.provider.discoverAccounts(options); + return this.#trace( + { + name: TraceName.SnapDiscoverAccounts, + data: { + providerName: this.getName(), + }, + }, + async () => this.provider.discoverAccounts(options), + ); } } diff --git a/packages/multichain-account-service/src/providers/EvmAccountProvider.test.ts b/packages/multichain-account-service/src/providers/EvmAccountProvider.test.ts index 251dfd7cb56..3ccc449da12 100644 --- a/packages/multichain-account-service/src/providers/EvmAccountProvider.test.ts +++ b/packages/multichain-account-service/src/providers/EvmAccountProvider.test.ts @@ -408,4 +408,119 @@ describe('EvmAccountProvider', () => { }), ).toStrictEqual([MOCK_HD_ACCOUNT_1]); }); + + it('calls trace callback during account discovery', async () => { + const mockTrace = jest.fn().mockImplementation(async (request, fn) => { + expect(request.name).toBe('EVM Discover Accounts'); + expect(request.data).toStrictEqual({ + providerName: 'EVM', + }); + return await fn(); + }); + + const { messenger, keyring, mocks } = setup({ + accounts: [], + }); + + const account = MockAccountBuilder.from(MOCK_HD_ACCOUNT_1) + .withAddressSuffix('0') + .get(); + + const expectedAccount = { + ...account, + id: expect.any(String), + }; + + mockNextDiscoveryAddressOnce(account.address); + + // Create provider with custom trace callback + const providerWithTrace = new EvmAccountProvider( + getMultichainAccountServiceMessenger(messenger), + { + discovery: { + maxAttempts: 3, + timeoutMs: 500, + backOffMs: 500, + }, + }, + mockTrace, + ); + + const result = await providerWithTrace.discoverAccounts({ + entropySource: MOCK_HD_KEYRING_1.metadata.id, + groupIndex: 0, + }); + + expect(result).toStrictEqual([expectedAccount]); + expect(mockTrace).toHaveBeenCalledTimes(1); + }); + + it('uses fallback trace when no trace callback is provided', async () => { + const { provider } = setup({ + accounts: [], + }); + + const account = MockAccountBuilder.from(MOCK_HD_ACCOUNT_1) + .withAddressSuffix('0') + .get(); + + const expectedAccount = { + ...account, + id: expect.any(String), + }; + + mockNextDiscoveryAddressOnce(account.address); + + // Provider without trace callback should use fallback + const result = await provider.discoverAccounts({ + entropySource: MOCK_HD_KEYRING_1.metadata.id, + groupIndex: 0, + }); + + expect(result).toStrictEqual([expectedAccount]); + }); + + it('trace callback is called even when discovery returns empty results', async () => { + const mockTrace = jest.fn().mockImplementation(async (request, fn) => { + expect(request.name).toBe('EVM Discover Accounts'); + expect(request.data).toStrictEqual({ + providerName: 'EVM', + }); + return await fn(); + }); + + const { messenger } = setup({ + accounts: [], + discovery: { + transactionCount: '0x0', // No transactions, should return empty + }, + }); + + const account = MockAccountBuilder.from(MOCK_HD_ACCOUNT_1) + .withAddressSuffix('0') + .get(); + + mockNextDiscoveryAddressOnce(account.address); + + // Create provider with custom trace callback + const providerWithTrace = new EvmAccountProvider( + getMultichainAccountServiceMessenger(messenger), + { + discovery: { + maxAttempts: 3, + timeoutMs: 500, + backOffMs: 500, + }, + }, + mockTrace, + ); + + const result = await providerWithTrace.discoverAccounts({ + entropySource: MOCK_HD_KEYRING_1.metadata.id, + groupIndex: 0, + }); + + expect(result).toStrictEqual([]); + expect(mockTrace).toHaveBeenCalledTimes(1); + }); }); diff --git a/packages/multichain-account-service/src/providers/EvmAccountProvider.ts b/packages/multichain-account-service/src/providers/EvmAccountProvider.ts index 50c5e256833..ac6b1f4c3b6 100644 --- a/packages/multichain-account-service/src/providers/EvmAccountProvider.ts +++ b/packages/multichain-account-service/src/providers/EvmAccountProvider.ts @@ -1,5 +1,6 @@ import { publicToAddress } from '@ethereumjs/util'; import type { Bip44Account } from '@metamask/account-api'; +import type { TraceCallback } from '@metamask/controller-utils'; import type { HdKeyring } from '@metamask/eth-hd-keyring'; import type { EntropySourceId, KeyringAccount } from '@metamask/keyring-api'; import { EthAccountType } from '@metamask/keyring-api'; @@ -10,7 +11,9 @@ import type { } from '@metamask/keyring-internal-api'; import type { Provider } from '@metamask/network-controller'; import { add0x, assert, bytesToHex, type Hex } from '@metamask/utils'; -import type { MultichainAccountServiceMessenger } from 'src/types'; +import { traceFallback } from '../analytics'; +import { TraceName } from '../constants/traces'; +import type { MultichainAccountServiceMessenger } from '../types'; import { assertAreBip44Accounts, @@ -50,6 +53,8 @@ export class EvmAccountProvider extends BaseBip44AccountProvider { readonly #config: EvmAccountProviderConfig; + readonly #trace: TraceCallback; + constructor( messenger: MultichainAccountServiceMessenger, config: EvmAccountProviderConfig = { @@ -59,9 +64,11 @@ export class EvmAccountProvider extends BaseBip44AccountProvider { backOffMs: 500, }, }, + trace?: TraceCallback, ) { super(messenger); this.#config = config; + this.#trace = trace ?? traceFallback; } isAccountCompatible(account: Bip44Account): boolean { @@ -213,38 +220,48 @@ export class EvmAccountProvider extends BaseBip44AccountProvider { entropySource: EntropySourceId; groupIndex: number; }): Promise[]> { - const provider = this.getEvmProvider(); - const { entropySource, groupIndex } = opts; + return this.#trace( + { + name: TraceName.EvmDiscoverAccounts, + data: { + providerName: this.getName(), + }, + }, + async () => { + const provider = this.getEvmProvider(); + const { entropySource, groupIndex } = opts; - const addressFromGroupIndex = await this.#getAddressFromGroupIndex({ - entropySource, - groupIndex, - }); + const addressFromGroupIndex = await this.#getAddressFromGroupIndex({ + entropySource, + groupIndex, + }); - const count = await this.#getTransactionCount( - provider, - addressFromGroupIndex, - ); - if (count === 0) { - return []; - } + const count = await this.#getTransactionCount( + provider, + addressFromGroupIndex, + ); + if (count === 0) { + return []; + } - // We have some activity on this address, we try to create the account. - const [address] = await this.#createAccount({ - entropySource, - groupIndex, - }); - assert( - addressFromGroupIndex === address, - 'Created account does not match address from group index.', - ); + // We have some activity on this address, we try to create the account. + const [address] = await this.#createAccount({ + entropySource, + groupIndex, + }); + assert( + addressFromGroupIndex === address, + 'Created account does not match address from group index.', + ); - const account = this.messenger.call( - 'AccountsController:getAccountByAddress', - address, + const account = this.messenger.call( + 'AccountsController:getAccountByAddress', + address, + ); + assertInternalAccountExists(account); + assertIsBip44Account(account); + return [account]; + }, ); - assertInternalAccountExists(account); - assertIsBip44Account(account); - return [account]; } } From 0f351ebae62daba2f696f8f88ec9998203f165ef Mon Sep 17 00:00:00 2001 From: Monte Lai Date: Fri, 31 Oct 2025 17:21:40 +0800 Subject: [PATCH 06/16] fix: lint --- .../src/MultichainAccountService.test.ts | 1 - .../multichain-account-service/src/analytics/traces.test.ts | 2 +- .../src/providers/EvmAccountProvider.test.ts | 4 +--- .../src/providers/EvmAccountProvider.ts | 6 +++--- 4 files changed, 5 insertions(+), 8 deletions(-) diff --git a/packages/multichain-account-service/src/MultichainAccountService.test.ts b/packages/multichain-account-service/src/MultichainAccountService.test.ts index 8ae339d87c8..a3b87e1a80a 100644 --- a/packages/multichain-account-service/src/MultichainAccountService.test.ts +++ b/packages/multichain-account-service/src/MultichainAccountService.test.ts @@ -1129,7 +1129,6 @@ describe('MultichainAccountService', () => { groupIndex: 0, }; - // Create wrapper with custom trace callback const wrapperWithTrace = new AccountProviderWrapper( getMultichainAccountServiceMessenger( setup({ accounts: [MOCK_HD_ACCOUNT_1] }).rootMessenger, diff --git a/packages/multichain-account-service/src/analytics/traces.test.ts b/packages/multichain-account-service/src/analytics/traces.test.ts index 47cc45728c0..9f97d94bb0d 100644 --- a/packages/multichain-account-service/src/analytics/traces.test.ts +++ b/packages/multichain-account-service/src/analytics/traces.test.ts @@ -1,7 +1,7 @@ import type { TraceRequest } from '@metamask/controller-utils'; -import { TraceName } from '../constants/traces'; import { traceFallback } from './traces'; +import { TraceName } from '../constants/traces'; describe('MultichainAccountService - Traces', () => { describe('TraceName', () => { diff --git a/packages/multichain-account-service/src/providers/EvmAccountProvider.test.ts b/packages/multichain-account-service/src/providers/EvmAccountProvider.test.ts index 3ccc449da12..43e919f03b8 100644 --- a/packages/multichain-account-service/src/providers/EvmAccountProvider.test.ts +++ b/packages/multichain-account-service/src/providers/EvmAccountProvider.test.ts @@ -418,7 +418,7 @@ describe('EvmAccountProvider', () => { return await fn(); }); - const { messenger, keyring, mocks } = setup({ + const { messenger } = setup({ accounts: [], }); @@ -471,7 +471,6 @@ describe('EvmAccountProvider', () => { mockNextDiscoveryAddressOnce(account.address); - // Provider without trace callback should use fallback const result = await provider.discoverAccounts({ entropySource: MOCK_HD_KEYRING_1.metadata.id, groupIndex: 0, @@ -502,7 +501,6 @@ describe('EvmAccountProvider', () => { mockNextDiscoveryAddressOnce(account.address); - // Create provider with custom trace callback const providerWithTrace = new EvmAccountProvider( getMultichainAccountServiceMessenger(messenger), { diff --git a/packages/multichain-account-service/src/providers/EvmAccountProvider.ts b/packages/multichain-account-service/src/providers/EvmAccountProvider.ts index ac6b1f4c3b6..30219976e4f 100644 --- a/packages/multichain-account-service/src/providers/EvmAccountProvider.ts +++ b/packages/multichain-account-service/src/providers/EvmAccountProvider.ts @@ -11,9 +11,6 @@ import type { } from '@metamask/keyring-internal-api'; import type { Provider } from '@metamask/network-controller'; import { add0x, assert, bytesToHex, type Hex } from '@metamask/utils'; -import { traceFallback } from '../analytics'; -import { TraceName } from '../constants/traces'; -import type { MultichainAccountServiceMessenger } from '../types'; import { assertAreBip44Accounts, @@ -21,6 +18,9 @@ import { BaseBip44AccountProvider, } from './BaseBip44AccountProvider'; import { withRetry, withTimeout } from './utils'; +import { traceFallback } from '../analytics'; +import { TraceName } from '../constants/traces'; +import type { MultichainAccountServiceMessenger } from '../types'; const ETH_MAINNET_CHAIN_ID = '0x1'; From 90d5e8c9cd915da739004c33b31925f9e7794625 Mon Sep 17 00:00:00 2001 From: Monte Lai Date: Fri, 31 Oct 2025 17:42:58 +0800 Subject: [PATCH 07/16] fix: check provider before determining name --- .../src/MultichainAccountService.test.ts | 96 +++++++++++++++++++ .../src/providers/AccountProviderWrapper.ts | 19 +++- 2 files changed, 114 insertions(+), 1 deletion(-) diff --git a/packages/multichain-account-service/src/MultichainAccountService.test.ts b/packages/multichain-account-service/src/MultichainAccountService.test.ts index a3b87e1a80a..49ce4cfd352 100644 --- a/packages/multichain-account-service/src/MultichainAccountService.test.ts +++ b/packages/multichain-account-service/src/MultichainAccountService.test.ts @@ -1164,6 +1164,102 @@ describe('MultichainAccountService', () => { expect(result).toStrictEqual([MOCK_HD_ACCOUNT_1]); expect(solProvider.discoverAccounts).toHaveBeenCalledWith(options); }); + + it('uses correct trace name for Snap-based providers', async () => { + const mockTrace = jest.fn().mockImplementation(async (request, fn) => { + expect(request.name).toBe('Snap Discover Accounts'); + expect(request.data).toStrictEqual({ + providerName: solProvider.getName(), + }); + return await fn(); + }); + + const options = { + entropySource: MOCK_HD_ACCOUNT_1.options.entropy.id, + groupIndex: 0, + }; + + const wrapperWithTrace = new AccountProviderWrapper( + getMultichainAccountServiceMessenger( + setup({ accounts: [MOCK_HD_ACCOUNT_1] }).rootMessenger, + ), + solProvider, + mockTrace, + ); + + (solProvider.discoverAccounts as jest.Mock).mockResolvedValue([ + MOCK_HD_ACCOUNT_1, + ]); + + const result = await wrapperWithTrace.discoverAccounts(options); + + expect(result).toStrictEqual([MOCK_HD_ACCOUNT_1]); + expect(mockTrace).toHaveBeenCalledTimes(1); + }); + + it('uses correct trace name for EVM providers', async () => { + const mockTrace = jest.fn().mockImplementation(async (request, fn) => { + expect(request.name).toBe('EVM Discover Accounts'); + expect(request.data).toStrictEqual({ + providerName: 'EVM', + }); + return await fn(); + }); + + const { rootMessenger } = setup({ accounts: [MOCK_HD_ACCOUNT_1] }); + const evmProvider = new EvmAccountProvider( + getMultichainAccountServiceMessenger(rootMessenger), + ); + + jest.spyOn(evmProvider, 'discoverAccounts'); + jest.spyOn(evmProvider, 'getName').mockReturnValue('EVM'); + + const options = { + entropySource: MOCK_HD_ACCOUNT_1.options.entropy.id, + groupIndex: 0, + }; + + const evmWrapper = new AccountProviderWrapper( + getMultichainAccountServiceMessenger(rootMessenger), + evmProvider, + mockTrace, + ); + + (evmProvider.discoverAccounts as jest.Mock).mockResolvedValue([ + MOCK_HD_ACCOUNT_1, + ]); + + const result = await evmWrapper.discoverAccounts(options); + + expect(result).toStrictEqual([MOCK_HD_ACCOUNT_1]); + expect(mockTrace).toHaveBeenCalledTimes(1); + expect(evmProvider.discoverAccounts).toHaveBeenCalledWith(options); + }); + + it('returns empty array when disabled without calling trace', async () => { + const mockTrace = jest.fn(); + const options = { + entropySource: MOCK_HD_ACCOUNT_1.options.entropy.id, + groupIndex: 0, + }; + + const wrapperWithTrace = new AccountProviderWrapper( + getMultichainAccountServiceMessenger( + setup({ accounts: [MOCK_HD_ACCOUNT_1] }).rootMessenger, + ), + solProvider, + mockTrace, + ); + + // Disable the wrapper + wrapperWithTrace.setEnabled(false); + + const result = await wrapperWithTrace.discoverAccounts(options); + + expect(result).toStrictEqual([]); + expect(mockTrace).not.toHaveBeenCalled(); + expect(solProvider.discoverAccounts).not.toHaveBeenCalled(); + }); }); describe('createMultichainAccountWallet', () => { diff --git a/packages/multichain-account-service/src/providers/AccountProviderWrapper.ts b/packages/multichain-account-service/src/providers/AccountProviderWrapper.ts index 3ad3345db9c..8fb919cc0dd 100644 --- a/packages/multichain-account-service/src/providers/AccountProviderWrapper.ts +++ b/packages/multichain-account-service/src/providers/AccountProviderWrapper.ts @@ -3,6 +3,7 @@ import type { TraceCallback } from '@metamask/controller-utils'; import type { EntropySourceId, KeyringAccount } from '@metamask/keyring-api'; import { BaseBip44AccountProvider } from './BaseBip44AccountProvider'; +import { EVM_ACCOUNT_PROVIDER_NAME } from './EvmAccountProvider'; import { traceFallback } from '../analytics'; import { TraceName } from '../constants/traces'; import type { MultichainAccountServiceMessenger } from '../types'; @@ -28,6 +29,22 @@ export class AccountProviderWrapper extends BaseBip44AccountProvider { this.#trace = trace ?? traceFallback; } + /** + * Determines the appropriate trace name based on the wrapped provider type. + * + * @returns The appropriate TraceName for the wrapped provider. + */ + private getTraceNameForProvider(): string { + const providerName = this.provider.getName(); + + if (providerName === EVM_ACCOUNT_PROVIDER_NAME) { + return TraceName.EvmDiscoverAccounts; + } + + // All other providers (Solana, Bitcoin, Tron) are Snap-based + return TraceName.SnapDiscoverAccounts; + } + override getName(): string { return this.provider.getName(); } @@ -115,7 +132,7 @@ export class AccountProviderWrapper extends BaseBip44AccountProvider { } return this.#trace( { - name: TraceName.SnapDiscoverAccounts, + name: this.getTraceNameForProvider(), data: { providerName: this.getName(), }, From a06ed0a8f783c6338c15575da2fc76c8dc3a1a5e Mon Sep 17 00:00:00 2001 From: Monte Lai Date: Fri, 31 Oct 2025 20:49:52 +0800 Subject: [PATCH 08/16] chore: add comment --- .../src/providers/AccountProviderWrapper.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/multichain-account-service/src/providers/AccountProviderWrapper.ts b/packages/multichain-account-service/src/providers/AccountProviderWrapper.ts index 8fb919cc0dd..eddfce28280 100644 --- a/packages/multichain-account-service/src/providers/AccountProviderWrapper.ts +++ b/packages/multichain-account-service/src/providers/AccountProviderWrapper.ts @@ -130,6 +130,8 @@ export class AccountProviderWrapper extends BaseBip44AccountProvider { if (!this.isEnabled) { return []; } + + // TODO: Try to move the tracing logic later in SnapAccountProvider to be more DRY return this.#trace( { name: this.getTraceNameForProvider(), From 56a9cc24a64c352ecdaf3acffca975a943e0105f Mon Sep 17 00:00:00 2001 From: Monte Lai Date: Mon, 3 Nov 2025 21:52:38 +0800 Subject: [PATCH 09/16] refactor: move trace to concrete provider --- .../src/MultichainAccountService.test.ts | 203 ++----------- .../src/MultichainAccountService.ts | 3 +- .../src/providers/AccountProviderWrapper.ts | 36 +-- .../src/providers/BtcAccountProvider.test.ts | 125 ++++++++ .../src/providers/BtcAccountProvider.ts | 63 +++-- .../src/providers/EvmAccountProvider.test.ts | 11 +- .../src/providers/EvmAccountProvider.ts | 2 +- .../src/providers/SnapAccountProvider.test.ts | 267 ++++++++++++++++++ .../src/providers/SnapAccountProvider.ts | 21 +- .../src/providers/SolAccountProvider.test.ts | 116 +++++++- .../src/providers/SolAccountProvider.ts | 72 +++-- .../src/providers/TrxAccountProvider.test.ts | 129 +++++++++ .../src/providers/TrxAccountProvider.ts | 60 ++-- 13 files changed, 810 insertions(+), 298 deletions(-) diff --git a/packages/multichain-account-service/src/MultichainAccountService.test.ts b/packages/multichain-account-service/src/MultichainAccountService.test.ts index 49ce4cfd352..1fef1974899 100644 --- a/packages/multichain-account-service/src/MultichainAccountService.test.ts +++ b/packages/multichain-account-service/src/MultichainAccountService.test.ts @@ -216,38 +216,17 @@ describe('MultichainAccountService', () => { providerConfigs, }); - expect(mocks.EvmAccountProvider.constructor).toHaveBeenCalledWith( - messenger, - providerConfigs[EvmAccountProvider.NAME], - ); - expect(mocks.SolAccountProvider.constructor).toHaveBeenCalledWith( - messenger, - providerConfigs[SolAccountProvider.NAME], - ); - }); - - it('accepts trace callback in config', () => { - const mockTrace = jest.fn().mockImplementation((_request, fn) => fn?.()); - const config = { trace: mockTrace }; - - // Test that the service can be constructed with a trace config - const rootMessenger = getRootMessenger(); - const messenger = getMultichainAccountServiceMessenger(rootMessenger); - - // Mock the required handlers for initialization - rootMessenger.registerActionHandler( - 'KeyringController:getState', - jest.fn().mockReturnValue({ isUnlocked: true, keyrings: [] }), - ); - - const serviceWithTrace = new MultichainAccountService({ - messenger, - config, - }); - - serviceWithTrace.init(); - - expect(serviceWithTrace).toBeDefined(); + const evmProviderCall = + mocks.EvmAccountProvider.constructor.mock.calls[0]; + expect(evmProviderCall[0]).toBe(messenger); + expect(evmProviderCall[1]).toBe(providerConfigs[EvmAccountProvider.NAME]); + expect(typeof evmProviderCall[2]).toBe('function'); // TraceCallback + + const solProviderCall = + mocks.SolAccountProvider.constructor.mock.calls[0]; + expect(solProviderCall[0]).toBe(messenger); + expect(solProviderCall[1]).toBe(providerConfigs[SolAccountProvider.NAME]); + expect(typeof solProviderCall[2]).toBe('function'); // TraceCallback }); it('allows optional configs for some providers', () => { @@ -276,10 +255,12 @@ describe('MultichainAccountService', () => { expect(mocks.EvmAccountProvider.constructor).toHaveBeenCalledWith( messenger, undefined, + expect.any(Function), ); expect(mocks.SolAccountProvider.constructor).toHaveBeenCalledWith( messenger, providerConfigs[SolAccountProvider.NAME], + expect.any(Function), ); }); }); @@ -585,10 +566,20 @@ describe('MultichainAccountService', () => { ); // Should emit updated event for the existing group + expect(publishSpy).toHaveBeenCalled(); expect(publishSpy).toHaveBeenCalledWith( 'MultichainAccountService:multichainAccountGroupUpdated', - expect.any(Object), + expect.objectContaining({ + groupIndex: 0, + }), ); + + const emittedGroup = publishSpy.mock.calls[0][1]; + expect(emittedGroup).toBeDefined(); + expect(emittedGroup).toHaveProperty('groupIndex', 0); + expect(emittedGroup).toHaveProperty('getAccounts'); + expect(emittedGroup).toHaveProperty('select'); + expect(emittedGroup).toHaveProperty('get'); }); it('creates new detected wallets and update reverse mapping on AccountsController:accountAdded', () => { @@ -1114,152 +1105,6 @@ describe('MultichainAccountService', () => { (solProvider.isAccountCompatible as jest.Mock).mockReturnValue(false); expect(wrapper.isAccountCompatible(MOCK_HD_ACCOUNT_1)).toBe(false); }); - - it('calls trace callback when discoverAccounts() is enabled', async () => { - const mockTrace = jest.fn().mockImplementation(async (request, fn) => { - expect(request.name).toBe('Snap Discover Accounts'); - expect(request.data).toStrictEqual({ - providerName: solProvider.getName(), - }); - return await fn(); - }); - - const options = { - entropySource: MOCK_HD_ACCOUNT_1.options.entropy.id, - groupIndex: 0, - }; - - const wrapperWithTrace = new AccountProviderWrapper( - getMultichainAccountServiceMessenger( - setup({ accounts: [MOCK_HD_ACCOUNT_1] }).rootMessenger, - ), - solProvider, - mockTrace, - ); - - (solProvider.discoverAccounts as jest.Mock).mockResolvedValue([ - MOCK_HD_ACCOUNT_1, - ]); - - const result = await wrapperWithTrace.discoverAccounts(options); - - expect(result).toStrictEqual([MOCK_HD_ACCOUNT_1]); - expect(mockTrace).toHaveBeenCalledTimes(1); - expect(solProvider.discoverAccounts).toHaveBeenCalledWith(options); - }); - - it('uses fallback trace when no trace callback is provided', async () => { - const options = { - entropySource: MOCK_HD_ACCOUNT_1.options.entropy.id, - groupIndex: 0, - }; - - (solProvider.discoverAccounts as jest.Mock).mockResolvedValue([ - MOCK_HD_ACCOUNT_1, - ]); - - // Wrapper without trace callback should use fallback - const result = await wrapper.discoverAccounts(options); - - expect(result).toStrictEqual([MOCK_HD_ACCOUNT_1]); - expect(solProvider.discoverAccounts).toHaveBeenCalledWith(options); - }); - - it('uses correct trace name for Snap-based providers', async () => { - const mockTrace = jest.fn().mockImplementation(async (request, fn) => { - expect(request.name).toBe('Snap Discover Accounts'); - expect(request.data).toStrictEqual({ - providerName: solProvider.getName(), - }); - return await fn(); - }); - - const options = { - entropySource: MOCK_HD_ACCOUNT_1.options.entropy.id, - groupIndex: 0, - }; - - const wrapperWithTrace = new AccountProviderWrapper( - getMultichainAccountServiceMessenger( - setup({ accounts: [MOCK_HD_ACCOUNT_1] }).rootMessenger, - ), - solProvider, - mockTrace, - ); - - (solProvider.discoverAccounts as jest.Mock).mockResolvedValue([ - MOCK_HD_ACCOUNT_1, - ]); - - const result = await wrapperWithTrace.discoverAccounts(options); - - expect(result).toStrictEqual([MOCK_HD_ACCOUNT_1]); - expect(mockTrace).toHaveBeenCalledTimes(1); - }); - - it('uses correct trace name for EVM providers', async () => { - const mockTrace = jest.fn().mockImplementation(async (request, fn) => { - expect(request.name).toBe('EVM Discover Accounts'); - expect(request.data).toStrictEqual({ - providerName: 'EVM', - }); - return await fn(); - }); - - const { rootMessenger } = setup({ accounts: [MOCK_HD_ACCOUNT_1] }); - const evmProvider = new EvmAccountProvider( - getMultichainAccountServiceMessenger(rootMessenger), - ); - - jest.spyOn(evmProvider, 'discoverAccounts'); - jest.spyOn(evmProvider, 'getName').mockReturnValue('EVM'); - - const options = { - entropySource: MOCK_HD_ACCOUNT_1.options.entropy.id, - groupIndex: 0, - }; - - const evmWrapper = new AccountProviderWrapper( - getMultichainAccountServiceMessenger(rootMessenger), - evmProvider, - mockTrace, - ); - - (evmProvider.discoverAccounts as jest.Mock).mockResolvedValue([ - MOCK_HD_ACCOUNT_1, - ]); - - const result = await evmWrapper.discoverAccounts(options); - - expect(result).toStrictEqual([MOCK_HD_ACCOUNT_1]); - expect(mockTrace).toHaveBeenCalledTimes(1); - expect(evmProvider.discoverAccounts).toHaveBeenCalledWith(options); - }); - - it('returns empty array when disabled without calling trace', async () => { - const mockTrace = jest.fn(); - const options = { - entropySource: MOCK_HD_ACCOUNT_1.options.entropy.id, - groupIndex: 0, - }; - - const wrapperWithTrace = new AccountProviderWrapper( - getMultichainAccountServiceMessenger( - setup({ accounts: [MOCK_HD_ACCOUNT_1] }).rootMessenger, - ), - solProvider, - mockTrace, - ); - - // Disable the wrapper - wrapperWithTrace.setEnabled(false); - - const result = await wrapperWithTrace.discoverAccounts(options); - - expect(result).toStrictEqual([]); - expect(mockTrace).not.toHaveBeenCalled(); - expect(solProvider.discoverAccounts).not.toHaveBeenCalled(); - }); }); describe('createMultichainAccountWallet', () => { diff --git a/packages/multichain-account-service/src/MultichainAccountService.ts b/packages/multichain-account-service/src/MultichainAccountService.ts index 561315f3cef..33a93149811 100644 --- a/packages/multichain-account-service/src/MultichainAccountService.ts +++ b/packages/multichain-account-service/src/MultichainAccountService.ts @@ -105,14 +105,15 @@ export class MultichainAccountService { new EvmAccountProvider( this.#messenger, providerConfigs?.[EvmAccountProvider.NAME], + this.#trace.bind(this), ), new AccountProviderWrapper( this.#messenger, new SolAccountProvider( this.#messenger, providerConfigs?.[SolAccountProvider.NAME], + this.#trace.bind(this), ), - this.#trace, ), // Custom account providers that can be provided by the MetaMask client. ...providers, diff --git a/packages/multichain-account-service/src/providers/AccountProviderWrapper.ts b/packages/multichain-account-service/src/providers/AccountProviderWrapper.ts index eddfce28280..51ff6cb2c20 100644 --- a/packages/multichain-account-service/src/providers/AccountProviderWrapper.ts +++ b/packages/multichain-account-service/src/providers/AccountProviderWrapper.ts @@ -1,11 +1,7 @@ import type { Bip44Account } from '@metamask/account-api'; -import type { TraceCallback } from '@metamask/controller-utils'; import type { EntropySourceId, KeyringAccount } from '@metamask/keyring-api'; import { BaseBip44AccountProvider } from './BaseBip44AccountProvider'; -import { EVM_ACCOUNT_PROVIDER_NAME } from './EvmAccountProvider'; -import { traceFallback } from '../analytics'; -import { TraceName } from '../constants/traces'; import type { MultichainAccountServiceMessenger } from '../types'; /** @@ -17,32 +13,12 @@ export class AccountProviderWrapper extends BaseBip44AccountProvider { private readonly provider: BaseBip44AccountProvider; - readonly #trace: TraceCallback; - constructor( messenger: MultichainAccountServiceMessenger, provider: BaseBip44AccountProvider, - trace?: TraceCallback, ) { super(messenger); this.provider = provider; - this.#trace = trace ?? traceFallback; - } - - /** - * Determines the appropriate trace name based on the wrapped provider type. - * - * @returns The appropriate TraceName for the wrapped provider. - */ - private getTraceNameForProvider(): string { - const providerName = this.provider.getName(); - - if (providerName === EVM_ACCOUNT_PROVIDER_NAME) { - return TraceName.EvmDiscoverAccounts; - } - - // All other providers (Solana, Bitcoin, Tron) are Snap-based - return TraceName.SnapDiscoverAccounts; } override getName(): string { @@ -130,17 +106,7 @@ export class AccountProviderWrapper extends BaseBip44AccountProvider { if (!this.isEnabled) { return []; } - - // TODO: Try to move the tracing logic later in SnapAccountProvider to be more DRY - return this.#trace( - { - name: this.getTraceNameForProvider(), - data: { - providerName: this.getName(), - }, - }, - async () => this.provider.discoverAccounts(options), - ); + return this.provider.discoverAccounts(options); } } diff --git a/packages/multichain-account-service/src/providers/BtcAccountProvider.test.ts b/packages/multichain-account-service/src/providers/BtcAccountProvider.test.ts index f9e0c8014ff..20e6c91224e 100644 --- a/packages/multichain-account-service/src/providers/BtcAccountProvider.test.ts +++ b/packages/multichain-account-service/src/providers/BtcAccountProvider.test.ts @@ -324,4 +324,129 @@ describe('BtcAccountProvider', () => { expect(discovered).toStrictEqual([]); }); + + describe('trace functionality', () => { + it('calls trace callback during account discovery', async () => { + const mockTrace = jest.fn().mockImplementation(async (request, fn) => { + expect(request.name).toBe('Snap Discover Accounts'); + expect(request.data).toStrictEqual({ + provider: 'Bitcoin', + }); + return await fn(); + }); + + const { messenger, mocks } = setup({ + accounts: [], + }); + + // Simulate one discovered account at the requested index. + mocks.handleRequest.mockReturnValue([MOCK_BTC_P2TR_DISCOVERED_ACCOUNT_1]); + + const multichainMessenger = + getMultichainAccountServiceMessenger(messenger); + const btcProvider = new BtcAccountProvider( + multichainMessenger, + undefined, + mockTrace, + ); + const provider = new AccountProviderWrapper( + multichainMessenger, + btcProvider, + ); + + const discovered = await provider.discoverAccounts({ + entropySource: MOCK_HD_KEYRING_1.metadata.id, + groupIndex: 0, + }); + + expect(discovered).toHaveLength(1); + expect(mockTrace).toHaveBeenCalledTimes(1); + }); + + it('uses fallback trace when no trace callback is provided', async () => { + const { provider, mocks } = setup({ + accounts: [], + }); + + 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); + // No trace errors, fallback trace should be used silently + }); + + it('trace callback is called even when discovery returns empty results', async () => { + const mockTrace = jest.fn().mockImplementation(async (request, fn) => { + expect(request.name).toBe('Snap Discover Accounts'); + expect(request.data).toStrictEqual({ + provider: 'Bitcoin', + }); + return await fn(); + }); + + const { messenger, mocks } = setup({ + accounts: [], + }); + + mocks.handleRequest.mockReturnValue([]); + + const multichainMessenger = + getMultichainAccountServiceMessenger(messenger); + const btcProvider = new BtcAccountProvider( + multichainMessenger, + undefined, + mockTrace, + ); + const provider = new AccountProviderWrapper( + multichainMessenger, + btcProvider, + ); + + const discovered = await provider.discoverAccounts({ + entropySource: MOCK_HD_KEYRING_1.metadata.id, + groupIndex: 0, + }); + + expect(discovered).toStrictEqual([]); + expect(mockTrace).toHaveBeenCalledTimes(1); + }); + + it('trace callback receives error when discovery fails', async () => { + const mockError = new Error('Discovery failed'); + const mockTrace = jest.fn().mockImplementation(async (_request, fn) => { + return await fn(); + }); + + const { messenger, mocks } = setup({ + accounts: [], + }); + + mocks.handleRequest.mockRejectedValue(mockError); + + const multichainMessenger = + getMultichainAccountServiceMessenger(messenger); + const btcProvider = new BtcAccountProvider( + multichainMessenger, + undefined, + mockTrace, + ); + const provider = new AccountProviderWrapper( + multichainMessenger, + btcProvider, + ); + + await expect( + provider.discoverAccounts({ + entropySource: MOCK_HD_KEYRING_1.metadata.id, + groupIndex: 0, + }), + ).rejects.toThrow(mockError); + + expect(mockTrace).toHaveBeenCalledTimes(1); + }); + }); }); diff --git a/packages/multichain-account-service/src/providers/BtcAccountProvider.ts b/packages/multichain-account-service/src/providers/BtcAccountProvider.ts index 8e004f82bf9..b14be42656c 100644 --- a/packages/multichain-account-service/src/providers/BtcAccountProvider.ts +++ b/packages/multichain-account-service/src/providers/BtcAccountProvider.ts @@ -1,4 +1,5 @@ import { assertIsBip44Account, type Bip44Account } from '@metamask/account-api'; +import type { TraceCallback } from '@metamask/controller-utils'; import type { EntropySourceId, KeyringAccount } from '@metamask/keyring-api'; import { BtcAccountType, BtcScope } from '@metamask/keyring-api'; import type { InternalAccount } from '@metamask/keyring-internal-api'; @@ -9,6 +10,8 @@ import type { Json, JsonRpcRequest } from '@metamask/utils'; import { SnapAccountProvider } from './SnapAccountProvider'; import { withRetry, withTimeout } from './utils'; +import { traceFallback } from '../analytics'; +import { TraceName } from '../constants/traces'; import type { MultichainAccountServiceMessenger } from '../types'; export type BtcAccountProviderConfig = { @@ -45,8 +48,9 @@ export class BtcAccountProvider extends SnapAccountProvider { backOffMs: 1000, }, }, + trace: TraceCallback = traceFallback, ) { - super(BtcAccountProvider.BTC_SNAP_ID, messenger); + super(BtcAccountProvider.BTC_SNAP_ID, messenger, trace); this.#client = this.#getKeyringClientFromSnapId( BtcAccountProvider.BTC_SNAP_ID, ); @@ -110,32 +114,45 @@ export class BtcAccountProvider extends SnapAccountProvider { }: { entropySource: EntropySourceId; groupIndex: number; - }) { - const discoveredAccounts = await withRetry( - () => - withTimeout( - this.#client.discoverAccounts( - [BtcScope.Mainnet], - entropySource, - groupIndex, - ), - this.#config.discovery.timeoutMs, - ), + }): Promise[]> { + return await super.trace( { - maxAttempts: this.#config.discovery.maxAttempts, - backOffMs: this.#config.discovery.backOffMs, + name: TraceName.SnapDiscoverAccounts, + data: { + provider: this.getName(), + }, }, - ); + async () => { + const discoveredAccounts = await withRetry( + () => + withTimeout( + this.#client.discoverAccounts( + [BtcScope.Mainnet], + entropySource, + groupIndex, + ), + this.#config.discovery.timeoutMs, + ), + { + maxAttempts: this.#config.discovery.maxAttempts, + backOffMs: this.#config.discovery.backOffMs, + }, + ); - if (!Array.isArray(discoveredAccounts) || discoveredAccounts.length === 0) { - return []; - } + if ( + !Array.isArray(discoveredAccounts) || + discoveredAccounts.length === 0 + ) { + return []; + } - const createdAccounts = await this.createAccounts({ - entropySource, - groupIndex, - }); + const createdAccounts = await this.createAccounts({ + entropySource, + groupIndex, + }); - return createdAccounts; + return createdAccounts; + }, + ); } } diff --git a/packages/multichain-account-service/src/providers/EvmAccountProvider.test.ts b/packages/multichain-account-service/src/providers/EvmAccountProvider.test.ts index 43e919f03b8..a3e1200d7d4 100644 --- a/packages/multichain-account-service/src/providers/EvmAccountProvider.test.ts +++ b/packages/multichain-account-service/src/providers/EvmAccountProvider.test.ts @@ -12,7 +12,10 @@ import type { import type { Hex } from '@metamask/utils'; import { createBytes } from '@metamask/utils'; -import { EvmAccountProvider } from './EvmAccountProvider'; +import { + EVM_ACCOUNT_PROVIDER_NAME, + EvmAccountProvider, +} from './EvmAccountProvider'; import { TimeoutError } from './utils'; import { getMultichainAccountServiceMessenger, @@ -203,7 +206,7 @@ function setup({ describe('EvmAccountProvider', () => { it('getName returns EVM', () => { const { provider } = setup({ accounts: [] }); - expect(provider.getName()).toBe('EVM'); + expect(provider.getName()).toBe(EVM_ACCOUNT_PROVIDER_NAME); }); it('gets accounts', () => { @@ -413,7 +416,7 @@ describe('EvmAccountProvider', () => { const mockTrace = jest.fn().mockImplementation(async (request, fn) => { expect(request.name).toBe('EVM Discover Accounts'); expect(request.data).toStrictEqual({ - providerName: 'EVM', + provider: EVM_ACCOUNT_PROVIDER_NAME, }); return await fn(); }); @@ -483,7 +486,7 @@ describe('EvmAccountProvider', () => { const mockTrace = jest.fn().mockImplementation(async (request, fn) => { expect(request.name).toBe('EVM Discover Accounts'); expect(request.data).toStrictEqual({ - providerName: 'EVM', + provider: EVM_ACCOUNT_PROVIDER_NAME, }); return await fn(); }); diff --git a/packages/multichain-account-service/src/providers/EvmAccountProvider.ts b/packages/multichain-account-service/src/providers/EvmAccountProvider.ts index 30219976e4f..2e4b60a210f 100644 --- a/packages/multichain-account-service/src/providers/EvmAccountProvider.ts +++ b/packages/multichain-account-service/src/providers/EvmAccountProvider.ts @@ -224,7 +224,7 @@ export class EvmAccountProvider extends BaseBip44AccountProvider { { name: TraceName.EvmDiscoverAccounts, data: { - providerName: this.getName(), + provider: this.getName(), }, }, async () => { diff --git a/packages/multichain-account-service/src/providers/SnapAccountProvider.test.ts b/packages/multichain-account-service/src/providers/SnapAccountProvider.test.ts index 647421fac7e..4a489770df4 100644 --- a/packages/multichain-account-service/src/providers/SnapAccountProvider.test.ts +++ b/packages/multichain-account-service/src/providers/SnapAccountProvider.test.ts @@ -1,8 +1,129 @@ +import type { TraceCallback, TraceRequest } from '@metamask/controller-utils'; + +import { BtcAccountProvider } from './BtcAccountProvider'; import { isSnapAccountProvider } from './SnapAccountProvider'; import { SolAccountProvider } from './SolAccountProvider'; +import { TrxAccountProvider } from './TrxAccountProvider'; +import { traceFallback } from '../analytics'; import type { MultichainAccountServiceMessenger } from '../types'; +jest.mock('../analytics', () => ({ + ...jest.requireActual('../analytics'), + traceFallback: jest + .fn() + .mockImplementation(async (_request: TraceRequest, fn?: () => unknown) => { + if (fn) { + return await fn(); + } + return undefined; + }), +})); + describe('SnapAccountProvider', () => { + describe('constructor default parameters', () => { + const mockMessenger = { + call: jest.fn().mockResolvedValue({}), + registerActionHandler: jest.fn(), + subscribe: jest.fn(), + registerMethodActionHandlers: jest.fn(), + unregisterActionHandler: jest.fn(), + registerInitialEventPayload: jest.fn(), + publish: jest.fn(), + clearEventSubscriptions: jest.fn(), + } as unknown as MultichainAccountServiceMessenger; + + beforeEach(() => { + jest.clearAllMocks(); + (traceFallback as jest.Mock).mockClear(); + }); + + it('creates SolAccountProvider with default trace using 1 parameter', () => { + const provider = new SolAccountProvider(mockMessenger); + expect(provider).toBeDefined(); + expect(provider.snapId).toBe(SolAccountProvider.SOLANA_SNAP_ID); + }); + + it('creates SolAccountProvider with default trace using 2 parameters', () => { + const provider = new SolAccountProvider(mockMessenger, undefined); + expect(provider).toBeDefined(); + expect(provider.snapId).toBe(SolAccountProvider.SOLANA_SNAP_ID); + }); + + it('creates SolAccountProvider with custom trace using 3 parameters', () => { + const customTrace = jest.fn(); + const provider = new SolAccountProvider( + mockMessenger, + undefined, + customTrace, + ); + expect(provider).toBeDefined(); + expect(provider.snapId).toBe(SolAccountProvider.SOLANA_SNAP_ID); + }); + + it('creates SolAccountProvider with custom config and default trace', () => { + const customConfig = { + discovery: { + timeoutMs: 3000, + maxAttempts: 5, + backOffMs: 2000, + }, + createAccounts: { + timeoutMs: 5000, + }, + }; + const provider = new SolAccountProvider(mockMessenger, customConfig); + expect(provider).toBeDefined(); + expect(provider.snapId).toBe(SolAccountProvider.SOLANA_SNAP_ID); + }); + + it('creates BtcAccountProvider with default trace', () => { + // Test other subclasses to ensure branch coverage + const btcProvider = new BtcAccountProvider(mockMessenger); + + expect(btcProvider).toBeDefined(); + expect(isSnapAccountProvider(btcProvider)).toBe(true); + }); + + it('creates TrxAccountProvider with custom trace', () => { + const customTrace = jest.fn(); + + // Explicitly test with all three parameters + const trxProvider = new TrxAccountProvider( + mockMessenger, + undefined, + customTrace, + ); + + expect(trxProvider).toBeDefined(); + expect(isSnapAccountProvider(trxProvider)).toBe(true); + }); + + it('creates provider without trace parameter', () => { + // Test creating provider without passing trace parameter + const provider = new SolAccountProvider(mockMessenger, undefined); + + expect(provider).toBeDefined(); + }); + + it('tests parameter spreading to trigger branch coverage', () => { + type SolConfig = ConstructorParameters[1]; + type ProviderArgs = [ + MultichainAccountServiceMessenger, + SolConfig?, + TraceCallback?, + ]; + const args: ProviderArgs = [mockMessenger]; + const provider1 = new SolAccountProvider(...args); + + args.push(undefined); + args.push(jest.fn()); + const provider2 = new SolAccountProvider(...args); + + expect(provider1).toBeDefined(); + expect(provider2).toBeDefined(); + }); + }); + describe('isSnapAccountProvider', () => { it('returns false for plain object with snapId property', () => { const mockProvider = { snapId: 'test-snap-id' }; @@ -47,4 +168,150 @@ describe('SnapAccountProvider', () => { expect(isSnapAccountProvider(solProvider)).toBe(true); }); }); + + describe('trace functionality', () => { + const mockMessenger = { + call: jest.fn().mockResolvedValue({}), + registerActionHandler: jest.fn(), + subscribe: jest.fn(), + registerMethodActionHandlers: jest.fn(), + unregisterActionHandler: jest.fn(), + registerInitialEventPayload: jest.fn(), + publish: jest.fn(), + clearEventSubscriptions: jest.fn(), + } as unknown as MultichainAccountServiceMessenger; + + beforeEach(() => { + jest.clearAllMocks(); + (traceFallback as jest.Mock).mockClear(); + }); + + it('uses default trace parameter when only messenger is provided', async () => { + const mockTraceCallback = traceFallback as jest.MockedFunction< + typeof traceFallback + >; + mockTraceCallback.mockImplementation(async (_request, fn) => fn?.()); + + // Test with only messenger parameter (uses default config and trace) + const solProvider = new SolAccountProvider(mockMessenger); + const request = { name: 'Test Request', data: {} }; + const fn = jest.fn().mockResolvedValue('defaultResult'); + + // Access protected trace method + // eslint-disable-next-line @typescript-eslint/no-explicit-any + await (solProvider as any).trace(request, fn); + + expect(mockTraceCallback).toHaveBeenCalledTimes(1); + expect(mockTraceCallback).toHaveBeenCalledWith( + request, + expect.any(Function), + ); + expect(fn).toHaveBeenCalledTimes(1); + }); + + it('uses custom trace when explicitly provided with all parameters', async () => { + const customTrace = jest.fn().mockImplementation(async (_request, fn) => { + return await fn(); + }); + + // Test with all parameters including custom trace + const solProvider = new SolAccountProvider( + mockMessenger, + { + discovery: { + timeoutMs: 2000, + maxAttempts: 3, + backOffMs: 1000, + }, + createAccounts: { + timeoutMs: 3000, + }, + }, + customTrace, + ); + const request = { name: 'Test Request', data: {} }; + const fn = jest.fn().mockResolvedValue('customResult'); + + // Access protected trace method + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const result = await (solProvider as any).trace(request, fn); + + expect(result).toBe('customResult'); + expect(customTrace).toHaveBeenCalledTimes(1); + expect(customTrace).toHaveBeenCalledWith(request, expect.any(Function)); + expect(traceFallback).not.toHaveBeenCalled(); + }); + + it('calls trace callback with the correct arguments', async () => { + const mockTrace = jest.fn().mockImplementation(async (request, fn) => { + expect(request).toStrictEqual({ + name: 'Test Request', + data: { test: 'data' }, + }); + return await fn(); + }); + + const solProvider = new SolAccountProvider( + mockMessenger, + undefined, + mockTrace, + ); + const request = { name: 'Test Request', data: { test: 'data' } }; + const fn = jest.fn().mockResolvedValue('testResult'); + + // Access protected trace method + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const result = await (solProvider as any).trace(request, fn); + + expect(result).toBe('testResult'); + expect(mockTrace).toHaveBeenCalledTimes(1); + expect(fn).toHaveBeenCalledTimes(1); + }); + + it('propagates errors through trace callback', async () => { + const mockError = new Error('Test error'); + const mockTrace = jest.fn().mockImplementation(async (_request, fn) => { + return await fn(); + }); + + const solProvider = new SolAccountProvider( + mockMessenger, + undefined, + mockTrace, + ); + const request = { name: 'Test Request', data: {} }; + const fn = jest.fn().mockRejectedValue(mockError); + + // Access protected trace method + // eslint-disable-next-line @typescript-eslint/no-explicit-any + await expect((solProvider as any).trace(request, fn)).rejects.toThrow( + mockError, + ); + + expect(mockTrace).toHaveBeenCalledTimes(1); + expect(fn).toHaveBeenCalledTimes(1); + }); + + it('handles trace callback returning undefined', async () => { + const mockTrace = jest.fn().mockImplementation(async (_request, fn) => { + return await fn(); + }); + + const solProvider = new SolAccountProvider( + mockMessenger, + undefined, + mockTrace, + ); + const request = { name: 'Test Request', data: {} }; + const fn = jest.fn().mockResolvedValue(undefined); + + // Access protected trace method + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const result = await (solProvider as any).trace(request, fn); + + expect(result).toBeUndefined(); + expect(mockTrace).toHaveBeenCalledTimes(1); + expect(fn).toHaveBeenCalledTimes(1); + }); + }); }); diff --git a/packages/multichain-account-service/src/providers/SnapAccountProvider.ts b/packages/multichain-account-service/src/providers/SnapAccountProvider.ts index 6b1e814f9ca..e629222da69 100644 --- a/packages/multichain-account-service/src/providers/SnapAccountProvider.ts +++ b/packages/multichain-account-service/src/providers/SnapAccountProvider.ts @@ -1,12 +1,14 @@ import { type Bip44Account } from '@metamask/account-api'; +import type { TraceCallback, TraceRequest } from '@metamask/controller-utils'; import type { SnapKeyring } from '@metamask/eth-snap-keyring'; import type { EntropySourceId, KeyringAccount } from '@metamask/keyring-api'; import { KeyringTypes } from '@metamask/keyring-controller'; import type { InternalAccount } from '@metamask/keyring-internal-api'; import type { Json, SnapId } from '@metamask/snaps-sdk'; -import type { MultichainAccountServiceMessenger } from 'src/types'; import { BaseBip44AccountProvider } from './BaseBip44AccountProvider'; +import { traceFallback } from '../analytics'; +import type { MultichainAccountServiceMessenger } from '../types'; export type RestrictedSnapKeyringCreateAccount = ( options: Record, @@ -15,10 +17,25 @@ export type RestrictedSnapKeyringCreateAccount = ( export abstract class SnapAccountProvider extends BaseBip44AccountProvider { readonly snapId: SnapId; - constructor(snapId: SnapId, messenger: MultichainAccountServiceMessenger) { + readonly #trace: TraceCallback; + + constructor( + snapId: SnapId, + messenger: MultichainAccountServiceMessenger, + /* istanbul ignore next */ + trace: TraceCallback = traceFallback, + ) { super(messenger); this.snapId = snapId; + this.#trace = trace; + } + + protected async trace( + request: TraceRequest, + fn: () => Promise, + ): Promise { + return this.#trace(request, fn); } protected async getRestrictedSnapAccountCreator(): Promise { diff --git a/packages/multichain-account-service/src/providers/SolAccountProvider.test.ts b/packages/multichain-account-service/src/providers/SolAccountProvider.test.ts index d808d429b4b..af2b245aaf6 100644 --- a/packages/multichain-account-service/src/providers/SolAccountProvider.test.ts +++ b/packages/multichain-account-service/src/providers/SolAccountProvider.test.ts @@ -99,6 +99,7 @@ function setup({ keyring: { createAccount: jest.Mock; }; + trace: jest.Mock; }; } { const keyring = new MockSolanaKeyring(accounts); @@ -113,6 +114,15 @@ function setup({ .mockImplementation((address: string) => keyring.accounts.find((account) => account.address === address), ); + + const mockTrace = jest.fn().mockImplementation(async (request, fn) => { + expect(request.name).toBe('Snap Discover Accounts'); + expect(request.data).toStrictEqual({ + provider: 'Solana', + }); + return await fn(); + }); + messenger.registerActionHandler( 'SnapController:handleRequest', mockHandleRequest, @@ -132,7 +142,7 @@ function setup({ const multichainMessenger = getMultichainAccountServiceMessenger(messenger); const provider = new AccountProviderWrapper( multichainMessenger, - new SolAccountProvider(multichainMessenger), + new SolAccountProvider(multichainMessenger, undefined, mockTrace), ); return { @@ -144,6 +154,7 @@ function setup({ keyring: { createAccount: keyring.createAccount as jest.Mock, }, + trace: mockTrace, }, }; } @@ -307,4 +318,107 @@ describe('SolAccountProvider', () => { expect(discovered).toStrictEqual([]); }); + + describe('trace functionality', () => { + it('calls trace callback during account discovery', async () => { + const { messenger, mocks } = setup({ + accounts: [], + }); + + mocks.handleRequest.mockReturnValue([MOCK_SOL_DISCOVERED_ACCOUNT_1]); + + const multichainMessenger = + getMultichainAccountServiceMessenger(messenger); + const solProvider = new SolAccountProvider( + multichainMessenger, + undefined, + mocks.trace, + ); + const provider = new AccountProviderWrapper( + multichainMessenger, + solProvider, + ); + + const discovered = await provider.discoverAccounts({ + entropySource: MOCK_HD_KEYRING_1.metadata.id, + groupIndex: 0, + }); + + expect(discovered).toHaveLength(1); + expect(mocks.trace).toHaveBeenCalledTimes(1); + }); + + it('uses fallback trace when no trace callback is provided', async () => { + const { provider, mocks } = setup({ + accounts: [], + }); + + 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); + }); + + it('trace callback is called even when discovery returns empty results', async () => { + const { messenger, mocks } = setup({ + accounts: [], + }); + + mocks.handleRequest.mockReturnValue([]); + + const multichainMessenger = + getMultichainAccountServiceMessenger(messenger); + const solProvider = new SolAccountProvider( + multichainMessenger, + undefined, + mocks.trace, + ); + const provider = new AccountProviderWrapper( + multichainMessenger, + solProvider, + ); + + const discovered = await provider.discoverAccounts({ + entropySource: MOCK_HD_KEYRING_1.metadata.id, + groupIndex: 0, + }); + + expect(discovered).toStrictEqual([]); + expect(mocks.trace).toHaveBeenCalledTimes(1); + }); + + it('trace callback receives error when discovery fails', async () => { + const mockError = new Error('Discovery failed'); + const { messenger, mocks } = setup({ + accounts: [], + }); + + mocks.handleRequest.mockRejectedValue(mockError); + + const multichainMessenger = + getMultichainAccountServiceMessenger(messenger); + const solProvider = new SolAccountProvider( + multichainMessenger, + undefined, + mocks.trace, + ); + const provider = new AccountProviderWrapper( + multichainMessenger, + solProvider, + ); + + await expect( + provider.discoverAccounts({ + entropySource: MOCK_HD_KEYRING_1.metadata.id, + groupIndex: 0, + }), + ).rejects.toThrow(mockError); + + expect(mocks.trace).toHaveBeenCalledTimes(1); + }); + }); }); diff --git a/packages/multichain-account-service/src/providers/SolAccountProvider.ts b/packages/multichain-account-service/src/providers/SolAccountProvider.ts index 05a447757be..df30772deb6 100644 --- a/packages/multichain-account-service/src/providers/SolAccountProvider.ts +++ b/packages/multichain-account-service/src/providers/SolAccountProvider.ts @@ -1,4 +1,5 @@ import { assertIsBip44Account, type Bip44Account } from '@metamask/account-api'; +import type { TraceCallback } from '@metamask/controller-utils'; import type { EntropySourceId, KeyringAccount } from '@metamask/keyring-api'; import { SolScope } from '@metamask/keyring-api'; import { @@ -11,10 +12,12 @@ import { KeyringClient } from '@metamask/keyring-snap-client'; import type { SnapId } from '@metamask/snaps-sdk'; import { HandlerType } from '@metamask/snaps-utils'; import type { Json, JsonRpcRequest } from '@metamask/utils'; -import type { MultichainAccountServiceMessenger } from 'src/types'; import { SnapAccountProvider } from './SnapAccountProvider'; import { withRetry, withTimeout } from './utils'; +import { traceFallback } from '../analytics'; +import { TraceName } from '../constants/traces'; +import type { MultichainAccountServiceMessenger } from '../types'; export type SolAccountProviderConfig = { discovery: { @@ -50,8 +53,9 @@ export class SolAccountProvider extends SnapAccountProvider { timeoutMs: 3000, }, }, + trace: TraceCallback = traceFallback, ) { - super(SolAccountProvider.SOLANA_SNAP_ID, messenger); + super(SolAccountProvider.SOLANA_SNAP_ID, messenger, trace); this.#client = this.#getKeyringClientFromSnapId( SolAccountProvider.SOLANA_SNAP_ID, ); @@ -137,36 +141,46 @@ export class SolAccountProvider extends SnapAccountProvider { entropySource: EntropySourceId; groupIndex: number; }): Promise[]> { - const discoveredAccounts = await withRetry( - () => - withTimeout( - this.#client.discoverAccounts( - [SolScope.Mainnet], - entropySource, - groupIndex, - ), - this.#config.discovery.timeoutMs, - ), + return await super.trace( { - maxAttempts: this.#config.discovery.maxAttempts, - backOffMs: this.#config.discovery.backOffMs, + name: TraceName.SnapDiscoverAccounts, + data: { + provider: this.getName(), + }, }, - ); + async () => { + const discoveredAccounts = await withRetry( + () => + withTimeout( + this.#client.discoverAccounts( + [SolScope.Mainnet], + entropySource, + groupIndex, + ), + this.#config.discovery.timeoutMs, + ), + { + maxAttempts: this.#config.discovery.maxAttempts, + backOffMs: this.#config.discovery.backOffMs, + }, + ); - if (!discoveredAccounts.length) { - return []; - } - - const createdAccounts = await Promise.all( - discoveredAccounts.map((d) => - this.#createAccount({ - entropySource, - groupIndex, - derivationPath: d.derivationPath, - }), - ), - ); + if (!discoveredAccounts.length) { + return []; + } + + const createdAccounts = await Promise.all( + discoveredAccounts.map((d) => + this.#createAccount({ + entropySource, + groupIndex, + derivationPath: d.derivationPath, + }), + ), + ); - return createdAccounts; + return createdAccounts; + }, + ); } } diff --git a/packages/multichain-account-service/src/providers/TrxAccountProvider.test.ts b/packages/multichain-account-service/src/providers/TrxAccountProvider.test.ts index 6c95295d316..05c94406650 100644 --- a/packages/multichain-account-service/src/providers/TrxAccountProvider.test.ts +++ b/packages/multichain-account-service/src/providers/TrxAccountProvider.test.ts @@ -308,4 +308,133 @@ describe('TrxAccountProvider', () => { expect(discovered).toStrictEqual([]); }); + + describe('trace functionality', () => { + it('calls trace callback during account discovery', async () => { + const mockTrace = jest.fn().mockImplementation(async (request, fn) => { + expect(request.name).toBe('Snap Discover Accounts'); + expect(request.data).toStrictEqual({ + provider: 'Tron', + }); + return await fn(); + }); + + const { messenger, mocks } = setup({ + accounts: [], + }); + + // Simulate one discovered account at the requested index. + mocks.keyring.discoverAccounts.mockResolvedValue([ + MOCK_TRX_DISCOVERED_ACCOUNT_1, + ]); + + const multichainMessenger = + getMultichainAccountServiceMessenger(messenger); + const trxProvider = new TrxAccountProvider( + multichainMessenger, + undefined, + mockTrace, + ); + const provider = new AccountProviderWrapper( + multichainMessenger, + trxProvider, + ); + + const discovered = await provider.discoverAccounts({ + entropySource: MOCK_HD_KEYRING_1.metadata.id, + groupIndex: 0, + }); + + expect(discovered).toHaveLength(1); + expect(mockTrace).toHaveBeenCalledTimes(1); + }); + + it('uses fallback trace when no trace callback is provided', async () => { + const { provider, mocks } = setup({ + accounts: [], + }); + + 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); + // No trace errors, fallback trace should be used silently + }); + + it('trace callback is called even when discovery returns empty results', async () => { + const mockTrace = jest.fn().mockImplementation(async (request, fn) => { + expect(request.name).toBe('Snap Discover Accounts'); + expect(request.data).toStrictEqual({ + provider: 'Tron', + }); + return await fn(); + }); + + const { messenger, mocks } = setup({ + accounts: [], + }); + + mocks.keyring.discoverAccounts.mockResolvedValue([]); + + const multichainMessenger = + getMultichainAccountServiceMessenger(messenger); + const trxProvider = new TrxAccountProvider( + multichainMessenger, + undefined, + mockTrace, + ); + const provider = new AccountProviderWrapper( + multichainMessenger, + trxProvider, + ); + + const discovered = await provider.discoverAccounts({ + entropySource: MOCK_HD_KEYRING_1.metadata.id, + groupIndex: 0, + }); + + expect(discovered).toStrictEqual([]); + expect(mockTrace).toHaveBeenCalledTimes(1); + }); + + it('trace callback receives error when discovery fails', async () => { + const mockError = new Error('Discovery failed'); + const mockTrace = jest.fn().mockImplementation(async (_request, fn) => { + return await fn(); + }); + + const { messenger, mocks } = setup({ + accounts: [], + }); + + mocks.keyring.discoverAccounts.mockRejectedValue(mockError); + + const multichainMessenger = + getMultichainAccountServiceMessenger(messenger); + const trxProvider = new TrxAccountProvider( + multichainMessenger, + undefined, + mockTrace, + ); + const provider = new AccountProviderWrapper( + multichainMessenger, + trxProvider, + ); + + await expect( + provider.discoverAccounts({ + entropySource: MOCK_HD_KEYRING_1.metadata.id, + groupIndex: 0, + }), + ).rejects.toThrow(mockError); + + expect(mockTrace).toHaveBeenCalledTimes(1); + }); + }); }); diff --git a/packages/multichain-account-service/src/providers/TrxAccountProvider.ts b/packages/multichain-account-service/src/providers/TrxAccountProvider.ts index d62e2c45096..a79916022d8 100644 --- a/packages/multichain-account-service/src/providers/TrxAccountProvider.ts +++ b/packages/multichain-account-service/src/providers/TrxAccountProvider.ts @@ -1,4 +1,5 @@ import { assertIsBip44Account, type Bip44Account } from '@metamask/account-api'; +import type { TraceCallback } from '@metamask/controller-utils'; import type { EntropySourceId, KeyringAccount } from '@metamask/keyring-api'; import { TrxAccountType, TrxScope } from '@metamask/keyring-api'; import { KeyringTypes } from '@metamask/keyring-controller'; @@ -7,10 +8,12 @@ import { KeyringClient } from '@metamask/keyring-snap-client'; import type { SnapId } from '@metamask/snaps-sdk'; import { HandlerType } from '@metamask/snaps-utils'; import type { Json, JsonRpcRequest } from '@metamask/utils'; -import type { MultichainAccountServiceMessenger } from 'src/types'; import { SnapAccountProvider } from './SnapAccountProvider'; import { withRetry, withTimeout } from './utils'; +import { traceFallback } from '../analytics'; +import { TraceName } from '../constants/traces'; +import type { MultichainAccountServiceMessenger } from '../types'; export type TrxAccountProviderConfig = { discovery: { @@ -46,8 +49,9 @@ export class TrxAccountProvider extends SnapAccountProvider { timeoutMs: 3000, }, }, + trace: TraceCallback = traceFallback, ) { - super(TrxAccountProvider.TRX_SNAP_ID, messenger); + super(TrxAccountProvider.TRX_SNAP_ID, messenger, trace); this.#client = this.#getKeyringClientFromSnapId( TrxAccountProvider.TRX_SNAP_ID, ); @@ -112,31 +116,41 @@ export class TrxAccountProvider extends SnapAccountProvider { entropySource: EntropySourceId; groupIndex: number; }): Promise[]> { - const discoveredAccounts = await withRetry( - () => - withTimeout( - this.#client.discoverAccounts( - [TrxScope.Mainnet], - entropySource, - groupIndex, - ), - this.#config.discovery.timeoutMs, - ), + return await super.trace( { - maxAttempts: this.#config.discovery.maxAttempts, - backOffMs: this.#config.discovery.backOffMs, + name: TraceName.SnapDiscoverAccounts, + data: { + provider: this.getName(), + }, }, - ); + async () => { + const discoveredAccounts = await withRetry( + () => + withTimeout( + this.#client.discoverAccounts( + [TrxScope.Mainnet], + entropySource, + groupIndex, + ), + this.#config.discovery.timeoutMs, + ), + { + maxAttempts: this.#config.discovery.maxAttempts, + backOffMs: this.#config.discovery.backOffMs, + }, + ); - if (!discoveredAccounts.length) { - return []; - } + if (!discoveredAccounts.length) { + return []; + } - const createdAccounts = await this.createAccounts({ - entropySource, - groupIndex, - }); + const createdAccounts = await this.createAccounts({ + entropySource, + groupIndex, + }); - return createdAccounts; + return createdAccounts; + }, + ); } } From 019f4f8f3eafd3473f32e63a74d15ec22ce6d9f1 Mon Sep 17 00:00:00 2001 From: Monte Lai Date: Wed, 5 Nov 2025 21:51:05 +0800 Subject: [PATCH 10/16] fix: trace bind --- .../src/MultichainAccountService.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/multichain-account-service/src/MultichainAccountService.ts b/packages/multichain-account-service/src/MultichainAccountService.ts index 3e8736cf284..b019c2275c1 100644 --- a/packages/multichain-account-service/src/MultichainAccountService.ts +++ b/packages/multichain-account-service/src/MultichainAccountService.ts @@ -6,7 +6,6 @@ import type { MultichainAccountWalletId, Bip44Account, } from '@metamask/account-api'; -import type { TraceCallback } from '@metamask/controller-utils'; import type { HdKeyring } from '@metamask/eth-hd-keyring'; import { mnemonicPhraseToBytes } from '@metamask/key-tree'; import type { EntropySourceId, KeyringAccount } from '@metamask/keyring-api'; @@ -74,8 +73,6 @@ export class MultichainAccountService { AccountContext> >; - readonly #trace: TraceCallback; - /** * The name of the service. */ @@ -100,21 +97,24 @@ export class MultichainAccountService { this.#messenger = messenger; this.#wallets = new Map(); this.#accountIdToContext = new Map(); - this.#trace = config?.trace ?? traceFallback; + + // Pass trace callback directly to preserve original 'this' context + // This avoids binding the callback to the MultichainAccountService instance + const traceCallback = config?.trace ?? traceFallback; // TODO: Rely on keyring capabilities once the keyring API is used by all keyrings. this.#providers = [ new EvmAccountProvider( this.#messenger, providerConfigs?.[EvmAccountProvider.NAME], - this.#trace.bind(this), + traceCallback, ), new AccountProviderWrapper( this.#messenger, new SolAccountProvider( this.#messenger, providerConfigs?.[SolAccountProvider.NAME], - this.#trace.bind(this), + traceCallback, ), ), // Custom account providers that can be provided by the MetaMask client. From 553e28314ef66f90cda4dd5866c20c647100ab18 Mon Sep 17 00:00:00 2001 From: Monte Lai Date: Thu, 6 Nov 2025 22:07:34 +0800 Subject: [PATCH 11/16] fix: use constants in test --- .../src/providers/BtcAccountProvider.test.ts | 14 +++++++++----- .../src/providers/EvmAccountProvider.test.ts | 5 +++-- .../src/providers/SolAccountProvider.test.ts | 10 +++++++--- .../src/providers/TrxAccountProvider.test.ts | 10 +++++++--- 4 files changed, 26 insertions(+), 13 deletions(-) diff --git a/packages/multichain-account-service/src/providers/BtcAccountProvider.test.ts b/packages/multichain-account-service/src/providers/BtcAccountProvider.test.ts index 20e6c91224e..6d1458441a8 100644 --- a/packages/multichain-account-service/src/providers/BtcAccountProvider.test.ts +++ b/packages/multichain-account-service/src/providers/BtcAccountProvider.test.ts @@ -8,7 +8,10 @@ import type { } from '@metamask/keyring-internal-api'; import { AccountProviderWrapper } from './AccountProviderWrapper'; -import { BtcAccountProvider } from './BtcAccountProvider'; +import { + BTC_ACCOUNT_PROVIDER_NAME, + BtcAccountProvider, +} from './BtcAccountProvider'; import { getMultichainAccountServiceMessenger, getRootMessenger, @@ -20,6 +23,7 @@ import { MockAccountBuilder, type RootMessenger, } from '../tests'; +import { TraceName } from 'src/constants/traces'; class MockBtcKeyring { readonly type = 'MockBtcKeyring'; @@ -328,9 +332,9 @@ describe('BtcAccountProvider', () => { describe('trace functionality', () => { it('calls trace callback during account discovery', async () => { const mockTrace = jest.fn().mockImplementation(async (request, fn) => { - expect(request.name).toBe('Snap Discover Accounts'); + expect(request.name).toBe(TraceName.SnapDiscoverAccounts); expect(request.data).toStrictEqual({ - provider: 'Bitcoin', + provider: BTC_ACCOUNT_PROVIDER_NAME, }); return await fn(); }); @@ -381,9 +385,9 @@ describe('BtcAccountProvider', () => { it('trace callback is called even when discovery returns empty results', async () => { const mockTrace = jest.fn().mockImplementation(async (request, fn) => { - expect(request.name).toBe('Snap Discover Accounts'); + expect(request.name).toBe(TraceName.SnapDiscoverAccounts); expect(request.data).toStrictEqual({ - provider: 'Bitcoin', + provider: BTC_ACCOUNT_PROVIDER_NAME, }); return await fn(); }); diff --git a/packages/multichain-account-service/src/providers/EvmAccountProvider.test.ts b/packages/multichain-account-service/src/providers/EvmAccountProvider.test.ts index a3e1200d7d4..55e73f8f3d4 100644 --- a/packages/multichain-account-service/src/providers/EvmAccountProvider.test.ts +++ b/packages/multichain-account-service/src/providers/EvmAccountProvider.test.ts @@ -26,6 +26,7 @@ import { MockAccountBuilder, type RootMessenger, } from '../tests'; +import { TraceName } from 'src/constants/traces'; jest.mock('@ethereumjs/util', () => ({ publicToAddress: jest.fn(), @@ -414,7 +415,7 @@ describe('EvmAccountProvider', () => { it('calls trace callback during account discovery', async () => { const mockTrace = jest.fn().mockImplementation(async (request, fn) => { - expect(request.name).toBe('EVM Discover Accounts'); + expect(request.name).toBe(TraceName.EvmDiscoverAccounts); expect(request.data).toStrictEqual({ provider: EVM_ACCOUNT_PROVIDER_NAME, }); @@ -484,7 +485,7 @@ describe('EvmAccountProvider', () => { it('trace callback is called even when discovery returns empty results', async () => { const mockTrace = jest.fn().mockImplementation(async (request, fn) => { - expect(request.name).toBe('EVM Discover Accounts'); + expect(request.name).toBe(TraceName.EvmDiscoverAccounts); expect(request.data).toStrictEqual({ provider: EVM_ACCOUNT_PROVIDER_NAME, }); diff --git a/packages/multichain-account-service/src/providers/SolAccountProvider.test.ts b/packages/multichain-account-service/src/providers/SolAccountProvider.test.ts index af2b245aaf6..3f8ed89ea9a 100644 --- a/packages/multichain-account-service/src/providers/SolAccountProvider.test.ts +++ b/packages/multichain-account-service/src/providers/SolAccountProvider.test.ts @@ -7,7 +7,10 @@ import type { } from '@metamask/keyring-internal-api'; import { AccountProviderWrapper } from './AccountProviderWrapper'; -import { SolAccountProvider } from './SolAccountProvider'; +import { + SOL_ACCOUNT_PROVIDER_NAME, + SolAccountProvider, +} from './SolAccountProvider'; import { getMultichainAccountServiceMessenger, getRootMessenger, @@ -18,6 +21,7 @@ import { MockAccountBuilder, type RootMessenger, } from '../tests'; +import { TraceName } from 'src/constants/traces'; class MockSolanaKeyring { readonly type = 'MockSolanaKeyring'; @@ -116,9 +120,9 @@ function setup({ ); const mockTrace = jest.fn().mockImplementation(async (request, fn) => { - expect(request.name).toBe('Snap Discover Accounts'); + expect(request.name).toBe(TraceName.SnapDiscoverAccounts); expect(request.data).toStrictEqual({ - provider: 'Solana', + provider: SOL_ACCOUNT_PROVIDER_NAME, }); return await fn(); }); diff --git a/packages/multichain-account-service/src/providers/TrxAccountProvider.test.ts b/packages/multichain-account-service/src/providers/TrxAccountProvider.test.ts index 05c94406650..5136250079d 100644 --- a/packages/multichain-account-service/src/providers/TrxAccountProvider.test.ts +++ b/packages/multichain-account-service/src/providers/TrxAccountProvider.test.ts @@ -7,7 +7,10 @@ import type { } from '@metamask/keyring-internal-api'; import { AccountProviderWrapper } from './AccountProviderWrapper'; -import { TrxAccountProvider } from './TrxAccountProvider'; +import { + TRX_ACCOUNT_PROVIDER_NAME, + TrxAccountProvider, +} from './TrxAccountProvider'; import { getMultichainAccountServiceMessenger, getRootMessenger, @@ -18,6 +21,7 @@ import { MockAccountBuilder, type RootMessenger, } from '../tests'; +import { TraceName } from 'src/constants/traces'; class MockTronKeyring { readonly type = 'MockTronKeyring'; @@ -369,9 +373,9 @@ describe('TrxAccountProvider', () => { it('trace callback is called even when discovery returns empty results', async () => { const mockTrace = jest.fn().mockImplementation(async (request, fn) => { - expect(request.name).toBe('Snap Discover Accounts'); + expect(request.name).toBe(TraceName.SnapDiscoverAccounts); expect(request.data).toStrictEqual({ - provider: 'Tron', + provider: TRX_ACCOUNT_PROVIDER_NAME, }); return await fn(); }); From 7d6ffe4875936401ebd902b670fad77e7dc63ebf Mon Sep 17 00:00:00 2001 From: Monte Lai Date: Thu, 6 Nov 2025 22:18:10 +0800 Subject: [PATCH 12/16] fix: create mock provider --- .../src/providers/SnapAccountProvider.test.ts | 122 ++++++++++++++---- 1 file changed, 96 insertions(+), 26 deletions(-) diff --git a/packages/multichain-account-service/src/providers/SnapAccountProvider.test.ts b/packages/multichain-account-service/src/providers/SnapAccountProvider.test.ts index 353a748ba0e..eda35643dcb 100644 --- a/packages/multichain-account-service/src/providers/SnapAccountProvider.test.ts +++ b/packages/multichain-account-service/src/providers/SnapAccountProvider.test.ts @@ -1,6 +1,7 @@ import type { Bip44Account } from '@metamask/account-api'; import type { TraceCallback, TraceRequest } from '@metamask/controller-utils'; import type { EntropySourceId, KeyringAccount } from '@metamask/keyring-api'; +import type { InternalAccount } from '@metamask/keyring-internal-api'; import type { SnapId } from '@metamask/snaps-sdk'; import { BtcAccountProvider } from './BtcAccountProvider'; @@ -33,6 +34,39 @@ const THROTTLED_OPERATION_DELAY_MS = 10; const TEST_SNAP_ID = 'npm:@metamask/test-snap' as SnapId; const TEST_ENTROPY_SOURCE = 'test-entropy-source' as EntropySourceId; +// Helper to create a test provider that exposes protected trace method +class TestSnapAccountProvider extends SnapAccountProvider { + getName(): string { + return 'Test Provider'; + } + + isAccountCompatible(_account: Bip44Account): boolean { + return true; + } + + async discoverAccounts(_options: { + entropySource: EntropySourceId; + groupIndex: number; + }): Promise[]> { + return []; + } + + async createAccounts(_options: { + entropySource: EntropySourceId; + groupIndex: number; + }): Promise[]> { + return []; + } + + // Expose protected trace method as public for testing + async trace( + request: TraceRequest, + fn: () => Promise, + ): Promise { + return super.trace(request, fn); + } +} + // Helper to create a tracked provider that monitors concurrent execution const createTrackedProvider = (maxConcurrency?: number) => { const tracker: { @@ -271,14 +305,26 @@ describe('SnapAccountProvider', () => { >; mockTraceCallback.mockImplementation(async (_request, fn) => fn?.()); - // Test with only messenger parameter (uses default config and trace) - const solProvider = new SolAccountProvider(mockMessenger); + // Test with default config and trace + const defaultConfig = { + discovery: { + timeoutMs: 2000, + maxAttempts: 3, + backOffMs: 1000, + }, + createAccounts: { + timeoutMs: 3000, + }, + }; + const testProvider = new TestSnapAccountProvider( + TEST_SNAP_ID, + mockMessenger, + defaultConfig, + ); const request = { name: 'Test Request', data: {} }; const fn = jest.fn().mockResolvedValue('defaultResult'); - // Access protected trace method - // eslint-disable-next-line @typescript-eslint/no-explicit-any - await (solProvider as any).trace(request, fn); + await testProvider.trace(request, fn); expect(mockTraceCallback).toHaveBeenCalledTimes(1); expect(mockTraceCallback).toHaveBeenCalledWith( @@ -294,7 +340,8 @@ describe('SnapAccountProvider', () => { }); // Test with all parameters including custom trace - const solProvider = new SolAccountProvider( + const testProvider = new TestSnapAccountProvider( + TEST_SNAP_ID, mockMessenger, { discovery: { @@ -311,9 +358,7 @@ describe('SnapAccountProvider', () => { const request = { name: 'Test Request', data: {} }; const fn = jest.fn().mockResolvedValue('customResult'); - // Access protected trace method - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const result = await (solProvider as any).trace(request, fn); + const result = await testProvider.trace(request, fn); expect(result).toBe('customResult'); expect(customTrace).toHaveBeenCalledTimes(1); @@ -330,17 +375,26 @@ describe('SnapAccountProvider', () => { return await fn(); }); - const solProvider = new SolAccountProvider( + const defaultConfig = { + discovery: { + timeoutMs: 2000, + maxAttempts: 3, + backOffMs: 1000, + }, + createAccounts: { + timeoutMs: 3000, + }, + }; + const testProvider = new TestSnapAccountProvider( + TEST_SNAP_ID, mockMessenger, - undefined, + defaultConfig, mockTrace, ); const request = { name: 'Test Request', data: { test: 'data' } }; const fn = jest.fn().mockResolvedValue('testResult'); - // Access protected trace method - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const result = await (solProvider as any).trace(request, fn); + const result = await testProvider.trace(request, fn); expect(result).toBe('testResult'); expect(mockTrace).toHaveBeenCalledTimes(1); @@ -353,19 +407,26 @@ describe('SnapAccountProvider', () => { return await fn(); }); - const solProvider = new SolAccountProvider( + const defaultConfig = { + discovery: { + timeoutMs: 2000, + maxAttempts: 3, + backOffMs: 1000, + }, + createAccounts: { + timeoutMs: 3000, + }, + }; + const testProvider = new TestSnapAccountProvider( + TEST_SNAP_ID, mockMessenger, - undefined, + defaultConfig, mockTrace, ); const request = { name: 'Test Request', data: {} }; const fn = jest.fn().mockRejectedValue(mockError); - // Access protected trace method - // eslint-disable-next-line @typescript-eslint/no-explicit-any - await expect((solProvider as any).trace(request, fn)).rejects.toThrow( - mockError, - ); + await expect(testProvider.trace(request, fn)).rejects.toThrow(mockError); expect(mockTrace).toHaveBeenCalledTimes(1); expect(fn).toHaveBeenCalledTimes(1); @@ -376,17 +437,26 @@ describe('SnapAccountProvider', () => { return await fn(); }); - const solProvider = new SolAccountProvider( + const defaultConfig = { + discovery: { + timeoutMs: 2000, + maxAttempts: 3, + backOffMs: 1000, + }, + createAccounts: { + timeoutMs: 3000, + }, + }; + const testProvider = new TestSnapAccountProvider( + TEST_SNAP_ID, mockMessenger, - undefined, + defaultConfig, mockTrace, ); const request = { name: 'Test Request', data: {} }; const fn = jest.fn().mockResolvedValue(undefined); - // Access protected trace method - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const result = await (solProvider as any).trace(request, fn); + const result = await testProvider.trace(request, fn); expect(result).toBeUndefined(); expect(mockTrace).toHaveBeenCalledTimes(1); From d225a862895445b291b5583f743c229f2bb76d97 Mon Sep 17 00:00:00 2001 From: Monte Lai Date: Thu, 6 Nov 2025 23:07:06 +0800 Subject: [PATCH 13/16] fix: replace mock with spy fix: lint fix: remove cursor rules --- .../src/providers/BtcAccountProvider.test.ts | 2 +- .../src/providers/EvmAccountProvider.test.ts | 2 +- .../src/providers/SnapAccountProvider.test.ts | 36 +++++++++---------- .../src/providers/SolAccountProvider.test.ts | 2 +- .../src/providers/TrxAccountProvider.test.ts | 2 +- 5 files changed, 20 insertions(+), 24 deletions(-) diff --git a/packages/multichain-account-service/src/providers/BtcAccountProvider.test.ts b/packages/multichain-account-service/src/providers/BtcAccountProvider.test.ts index 6d1458441a8..39f279bebe2 100644 --- a/packages/multichain-account-service/src/providers/BtcAccountProvider.test.ts +++ b/packages/multichain-account-service/src/providers/BtcAccountProvider.test.ts @@ -6,6 +6,7 @@ import type { EthKeyring, InternalAccount, } from '@metamask/keyring-internal-api'; +import { TraceName } from 'src/constants/traces'; import { AccountProviderWrapper } from './AccountProviderWrapper'; import { @@ -23,7 +24,6 @@ import { MockAccountBuilder, type RootMessenger, } from '../tests'; -import { TraceName } from 'src/constants/traces'; class MockBtcKeyring { readonly type = 'MockBtcKeyring'; diff --git a/packages/multichain-account-service/src/providers/EvmAccountProvider.test.ts b/packages/multichain-account-service/src/providers/EvmAccountProvider.test.ts index 55e73f8f3d4..82fa822e71d 100644 --- a/packages/multichain-account-service/src/providers/EvmAccountProvider.test.ts +++ b/packages/multichain-account-service/src/providers/EvmAccountProvider.test.ts @@ -11,6 +11,7 @@ import type { } from '@metamask/network-controller'; import type { Hex } from '@metamask/utils'; import { createBytes } from '@metamask/utils'; +import { TraceName } from 'src/constants/traces'; import { EVM_ACCOUNT_PROVIDER_NAME, @@ -26,7 +27,6 @@ import { MockAccountBuilder, type RootMessenger, } from '../tests'; -import { TraceName } from 'src/constants/traces'; jest.mock('@ethereumjs/util', () => ({ publicToAddress: jest.fn(), diff --git a/packages/multichain-account-service/src/providers/SnapAccountProvider.test.ts b/packages/multichain-account-service/src/providers/SnapAccountProvider.test.ts index eda35643dcb..588ebc3e960 100644 --- a/packages/multichain-account-service/src/providers/SnapAccountProvider.test.ts +++ b/packages/multichain-account-service/src/providers/SnapAccountProvider.test.ts @@ -18,17 +18,13 @@ import { } from '../tests'; import type { MultichainAccountServiceMessenger } from '../types'; -jest.mock('../analytics', () => ({ - ...jest.requireActual('../analytics'), - traceFallback: jest - .fn() - .mockImplementation(async (_request: TraceRequest, fn?: () => unknown) => { - if (fn) { - return await fn(); - } - return undefined; - }), -})); +jest.mock('../analytics', () => { + const actual = jest.requireActual('../analytics'); + return { + ...actual, + traceFallback: jest.fn(), + }; +}); const THROTTLED_OPERATION_DELAY_MS = 10; const TEST_SNAP_ID = 'npm:@metamask/test-snap' as SnapId; @@ -147,7 +143,6 @@ describe('SnapAccountProvider', () => { beforeEach(() => { jest.clearAllMocks(); - (traceFallback as jest.Mock).mockClear(); }); it('creates SolAccountProvider with default trace using 1 parameter', () => { @@ -294,16 +289,17 @@ describe('SnapAccountProvider', () => { clearEventSubscriptions: jest.fn(), } as unknown as MultichainAccountServiceMessenger; + const traceFallbackMock = traceFallback as jest.MockedFunction< + typeof traceFallback + >; + beforeEach(() => { jest.clearAllMocks(); - (traceFallback as jest.Mock).mockClear(); + traceFallbackMock.mockClear(); }); it('uses default trace parameter when only messenger is provided', async () => { - const mockTraceCallback = traceFallback as jest.MockedFunction< - typeof traceFallback - >; - mockTraceCallback.mockImplementation(async (_request, fn) => fn?.()); + traceFallbackMock.mockImplementation(async (_request, fn) => fn?.()); // Test with default config and trace const defaultConfig = { @@ -326,8 +322,8 @@ describe('SnapAccountProvider', () => { await testProvider.trace(request, fn); - expect(mockTraceCallback).toHaveBeenCalledTimes(1); - expect(mockTraceCallback).toHaveBeenCalledWith( + expect(traceFallbackMock).toHaveBeenCalledTimes(1); + expect(traceFallbackMock).toHaveBeenCalledWith( request, expect.any(Function), ); @@ -363,7 +359,7 @@ describe('SnapAccountProvider', () => { expect(result).toBe('customResult'); expect(customTrace).toHaveBeenCalledTimes(1); expect(customTrace).toHaveBeenCalledWith(request, expect.any(Function)); - expect(traceFallback).not.toHaveBeenCalled(); + expect(traceFallbackMock).not.toHaveBeenCalled(); }); it('calls trace callback with the correct arguments', async () => { diff --git a/packages/multichain-account-service/src/providers/SolAccountProvider.test.ts b/packages/multichain-account-service/src/providers/SolAccountProvider.test.ts index 3f8ed89ea9a..2be345d38a4 100644 --- a/packages/multichain-account-service/src/providers/SolAccountProvider.test.ts +++ b/packages/multichain-account-service/src/providers/SolAccountProvider.test.ts @@ -5,6 +5,7 @@ import type { EthKeyring, InternalAccount, } from '@metamask/keyring-internal-api'; +import { TraceName } from 'src/constants/traces'; import { AccountProviderWrapper } from './AccountProviderWrapper'; import { @@ -21,7 +22,6 @@ import { MockAccountBuilder, type RootMessenger, } from '../tests'; -import { TraceName } from 'src/constants/traces'; class MockSolanaKeyring { readonly type = 'MockSolanaKeyring'; diff --git a/packages/multichain-account-service/src/providers/TrxAccountProvider.test.ts b/packages/multichain-account-service/src/providers/TrxAccountProvider.test.ts index 5136250079d..ab99246386d 100644 --- a/packages/multichain-account-service/src/providers/TrxAccountProvider.test.ts +++ b/packages/multichain-account-service/src/providers/TrxAccountProvider.test.ts @@ -5,6 +5,7 @@ import type { EthKeyring, InternalAccount, } from '@metamask/keyring-internal-api'; +import { TraceName } from 'src/constants/traces'; import { AccountProviderWrapper } from './AccountProviderWrapper'; import { @@ -21,7 +22,6 @@ import { MockAccountBuilder, type RootMessenger, } from '../tests'; -import { TraceName } from 'src/constants/traces'; class MockTronKeyring { readonly type = 'MockTronKeyring'; From 24b68c62fe22f5da9abd0a8750972f84088059d6 Mon Sep 17 00:00:00 2001 From: Monte Lai Date: Fri, 7 Nov 2025 22:30:00 +0800 Subject: [PATCH 14/16] fix: lint --- .../src/providers/SolAccountProvider.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/multichain-account-service/src/providers/SolAccountProvider.test.ts b/packages/multichain-account-service/src/providers/SolAccountProvider.test.ts index 2be345d38a4..68645fa31d9 100644 --- a/packages/multichain-account-service/src/providers/SolAccountProvider.test.ts +++ b/packages/multichain-account-service/src/providers/SolAccountProvider.test.ts @@ -5,13 +5,13 @@ import type { EthKeyring, InternalAccount, } from '@metamask/keyring-internal-api'; -import { TraceName } from 'src/constants/traces'; import { AccountProviderWrapper } from './AccountProviderWrapper'; import { SOL_ACCOUNT_PROVIDER_NAME, SolAccountProvider, } from './SolAccountProvider'; +import { TraceName } from '../constants/traces'; import { getMultichainAccountServiceMessenger, getRootMessenger, From 3f5ebc444d99fc6230aedec6a8fed852e85a8b03 Mon Sep 17 00:00:00 2001 From: Monte Lai Date: Fri, 7 Nov 2025 22:31:10 +0800 Subject: [PATCH 15/16] fix: lint --- .../src/providers/BtcAccountProvider.test.ts | 2 +- .../src/providers/EvmAccountProvider.test.ts | 2 +- .../src/providers/TrxAccountProvider.test.ts | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/multichain-account-service/src/providers/BtcAccountProvider.test.ts b/packages/multichain-account-service/src/providers/BtcAccountProvider.test.ts index 39f279bebe2..d61b7362146 100644 --- a/packages/multichain-account-service/src/providers/BtcAccountProvider.test.ts +++ b/packages/multichain-account-service/src/providers/BtcAccountProvider.test.ts @@ -6,13 +6,13 @@ import type { EthKeyring, InternalAccount, } from '@metamask/keyring-internal-api'; -import { TraceName } from 'src/constants/traces'; import { AccountProviderWrapper } from './AccountProviderWrapper'; import { BTC_ACCOUNT_PROVIDER_NAME, BtcAccountProvider, } from './BtcAccountProvider'; +import { TraceName } from '../constants/traces'; import { getMultichainAccountServiceMessenger, getRootMessenger, diff --git a/packages/multichain-account-service/src/providers/EvmAccountProvider.test.ts b/packages/multichain-account-service/src/providers/EvmAccountProvider.test.ts index 82fa822e71d..cc10197dfe4 100644 --- a/packages/multichain-account-service/src/providers/EvmAccountProvider.test.ts +++ b/packages/multichain-account-service/src/providers/EvmAccountProvider.test.ts @@ -11,13 +11,13 @@ import type { } from '@metamask/network-controller'; import type { Hex } from '@metamask/utils'; import { createBytes } from '@metamask/utils'; -import { TraceName } from 'src/constants/traces'; import { EVM_ACCOUNT_PROVIDER_NAME, EvmAccountProvider, } from './EvmAccountProvider'; import { TimeoutError } from './utils'; +import { TraceName } from '../constants/traces'; import { getMultichainAccountServiceMessenger, getRootMessenger, diff --git a/packages/multichain-account-service/src/providers/TrxAccountProvider.test.ts b/packages/multichain-account-service/src/providers/TrxAccountProvider.test.ts index ab99246386d..049d5fe3ade 100644 --- a/packages/multichain-account-service/src/providers/TrxAccountProvider.test.ts +++ b/packages/multichain-account-service/src/providers/TrxAccountProvider.test.ts @@ -5,13 +5,13 @@ import type { EthKeyring, InternalAccount, } from '@metamask/keyring-internal-api'; -import { TraceName } from 'src/constants/traces'; import { AccountProviderWrapper } from './AccountProviderWrapper'; import { TRX_ACCOUNT_PROVIDER_NAME, TrxAccountProvider, } from './TrxAccountProvider'; +import { TraceName } from '../constants/traces'; import { getMultichainAccountServiceMessenger, getRootMessenger, From aebc20c00cf504df5c476c27a5df9f6c2599c0b5 Mon Sep 17 00:00:00 2001 From: Monte Lai Date: Sat, 8 Nov 2025 00:01:00 +0800 Subject: [PATCH 16/16] fix: use constantsfor trace names and data --- .../src/providers/TrxAccountProvider.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/multichain-account-service/src/providers/TrxAccountProvider.test.ts b/packages/multichain-account-service/src/providers/TrxAccountProvider.test.ts index 049d5fe3ade..fc0ca2e8c73 100644 --- a/packages/multichain-account-service/src/providers/TrxAccountProvider.test.ts +++ b/packages/multichain-account-service/src/providers/TrxAccountProvider.test.ts @@ -316,9 +316,9 @@ describe('TrxAccountProvider', () => { describe('trace functionality', () => { it('calls trace callback during account discovery', async () => { const mockTrace = jest.fn().mockImplementation(async (request, fn) => { - expect(request.name).toBe('Snap Discover Accounts'); + expect(request.name).toBe(TraceName.SnapDiscoverAccounts); expect(request.data).toStrictEqual({ - provider: 'Tron', + provider: TRX_ACCOUNT_PROVIDER_NAME, }); return await fn(); });