diff --git a/packages/multichain-account-service/CHANGELOG.md b/packages/multichain-account-service/CHANGELOG.md index c1cc881d7b..b594eaaba0 100644 --- a/packages/multichain-account-service/CHANGELOG.md +++ b/packages/multichain-account-service/CHANGELOG.md @@ -7,6 +7,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- Accept P2TR (taproot) accounts as compatible Bitcoin accounts in `BtcAccountProvider` +- Add testnet4 (`BtcScope.Testnet4`) to Bitcoin provider capabilities and account discovery + +### Fixed + +- Guard against accounts with empty scopes during resync to prevent undefined scope in re-creation +- Propagate re-creation failure after keyring removal to prevent silent state inconsistency +- Align discovery scopes with advertised capabilities (include `BtcScope.Testnet`) + ### Changed - **BREAKING:** The service messenger now requires the `SnapAccountService:ensureReady` action to be declared ([#8715](https://github.com/MetaMask/core/pull/8715)) diff --git a/packages/multichain-account-service/src/providers/BtcAccountProvider.test.ts b/packages/multichain-account-service/src/providers/BtcAccountProvider.test.ts index c1e7dc05b1..16c9538a44 100644 --- a/packages/multichain-account-service/src/providers/BtcAccountProvider.test.ts +++ b/packages/multichain-account-service/src/providers/BtcAccountProvider.test.ts @@ -1,6 +1,10 @@ import { isBip44Account } from '@metamask/account-api'; import type { SnapKeyring } from '@metamask/eth-snap-keyring'; -import { AccountCreationType, BtcAccountType } from '@metamask/keyring-api'; +import { + AccountCreationType, + BtcAccountType, + BtcScope, +} from '@metamask/keyring-api'; import type { KeyringMetadata } from '@metamask/keyring-controller'; import type { EthKeyring, @@ -53,6 +57,8 @@ class MockBtcKeyring { this.accounts = accounts; } + removeAccount = jest.fn().mockResolvedValue(undefined); + #getIndexFromDerivationPath(derivationPath: string): number { // eslint-disable-next-line prefer-regex-literals const derivationPathIndexRegex = new RegExp( @@ -295,6 +301,14 @@ describe('BtcAccountProvider', () => { expect(provider.isAccountCompatible(account)).toBe(true); }); + it('returns true if a P2TR account is compatible', () => { + const account = MOCK_BTC_P2TR_ACCOUNT_1; + const { provider } = setup({ + accounts: [account], + }); + expect(provider.isAccountCompatible(account)).toBe(true); + }); + it('returns false if an account is not compatible', () => { const account = MOCK_HD_ACCOUNT_1; const { provider } = setup({ @@ -661,6 +675,32 @@ describe('BtcAccountProvider', () => { ).toStrictEqual([]); }); + it('discovers accounts across all advertised scopes', async () => { + const { provider, mocks } = setup({ + accounts: [], + }); + + mocks.handleRequest.mockReturnValue([MOCK_BTC_P2TR_DISCOVERED_ACCOUNT_1]); + + await provider.discoverAccounts({ + entropySource: MOCK_HD_KEYRING_1.metadata.id, + groupIndex: 0, + }); + + // Verify discoverAccounts was called with all three scopes matching + // the provider's advertised capabilities. + expect(mocks.handleRequest).toHaveBeenCalledWith( + expect.objectContaining({ + request: expect.objectContaining({ + method: 'keyring_discoverAccounts', + params: expect.objectContaining({ + scopes: [BtcScope.Mainnet, BtcScope.Testnet, BtcScope.Testnet4], + }), + }), + }), + ); + }); + describe('trace functionality', () => { it('calls trace callback during account discovery', async () => { const { provider, mocks } = setup({ @@ -742,6 +782,155 @@ describe('BtcAccountProvider', () => { }); }); + describe('resyncAccounts', () => { + it('re-creates a P2TR account preserving the address type', async () => { + const account = { + ...MOCK_BTC_P2TR_ACCOUNT_1, + metadata: { + ...MOCK_BTC_P2TR_ACCOUNT_1.metadata, + snap: { + ...MOCK_BTC_P2TR_ACCOUNT_1.metadata.snap, + id: BtcAccountProvider.BTC_SNAP_ID, + }, + }, + }; + const { provider, mocks } = setup({ + accounts: [account], + }); + + // Snap reports no accounts — triggers the re-creation path. + mocks.handleRequest.mockReturnValue([]); + + await provider.resyncAccounts([account]); + + expect(mocks.keyring.createAccount).toHaveBeenCalledWith( + expect.anything(), + expect.objectContaining({ + addressType: BtcAccountType.P2tr, + scope: BtcScope.Testnet, + }), + expect.anything(), + ); + }); + + it('re-creates a testnet4 account preserving the scope', async () => { + const account = { + ...MOCK_BTC_P2WPKH_ACCOUNT_1, + scopes: [BtcScope.Testnet4], + metadata: { + ...MOCK_BTC_P2WPKH_ACCOUNT_1.metadata, + snap: { + ...MOCK_BTC_P2WPKH_ACCOUNT_1.metadata.snap, + id: BtcAccountProvider.BTC_SNAP_ID, + }, + }, + }; + const { provider, mocks } = setup({ + accounts: [account], + }); + + mocks.handleRequest.mockReturnValue([]); + + await provider.resyncAccounts([account]); + + expect(mocks.keyring.createAccount).toHaveBeenCalledWith( + expect.anything(), + expect.objectContaining({ + addressType: BtcAccountType.P2wpkh, + scope: BtcScope.Testnet4, + }), + expect.anything(), + ); + }); + + it('does not alter a P2WPKH mainnet account during resync', async () => { + const account = { + ...MOCK_BTC_P2WPKH_ACCOUNT_1, + metadata: { + ...MOCK_BTC_P2WPKH_ACCOUNT_1.metadata, + snap: { + ...MOCK_BTC_P2WPKH_ACCOUNT_1.metadata.snap, + id: BtcAccountProvider.BTC_SNAP_ID, + }, + }, + }; + const { provider, mocks } = setup({ + accounts: [account], + }); + + mocks.handleRequest.mockReturnValue([]); + + await provider.resyncAccounts([account]); + + expect(mocks.keyring.createAccount).toHaveBeenCalledWith( + expect.anything(), + expect.objectContaining({ + addressType: BtcAccountType.P2wpkh, + scope: BtcScope.Mainnet, + }), + expect.anything(), + ); + }); + + it('reports error when account has no scopes during resync', async () => { + const account = { + ...MOCK_BTC_P2WPKH_ACCOUNT_1, + scopes: [] as string[], + metadata: { + ...MOCK_BTC_P2WPKH_ACCOUNT_1.metadata, + snap: { + ...MOCK_BTC_P2WPKH_ACCOUNT_1.metadata.snap, + id: BtcAccountProvider.BTC_SNAP_ID, + }, + }, + }; + const { provider, mocks } = setup({ + accounts: [account], + }); + + // Snap reports no accounts — triggers re-creation path. + mocks.handleRequest.mockReturnValue([]); + + // Should not throw — error is reported but swallowed by the outer catch. + await provider.resyncAccounts([account]); + + // createAccount should never have been called. + expect(mocks.keyring.createAccount).not.toHaveBeenCalled(); + }); + + it('reports error when re-creation fails after keyring removal', async () => { + const account = { + ...MOCK_BTC_P2TR_ACCOUNT_1, + metadata: { + ...MOCK_BTC_P2TR_ACCOUNT_1.metadata, + snap: { + ...MOCK_BTC_P2TR_ACCOUNT_1.metadata.snap, + id: BtcAccountProvider.BTC_SNAP_ID, + }, + }, + }; + const { provider, keyring, mocks } = setup({ + accounts: [account], + }); + + // Snap reports no accounts — triggers re-creation path. + mocks.handleRequest.mockReturnValue([]); + + // createAccount will throw after removeAccount succeeds. + mocks.keyring.createAccount.mockRejectedValue( + new Error('Snap crashed during re-creation'), + ); + + // Should not throw — the outer catch swallows it. + await provider.resyncAccounts([account]); + + // removeAccount was called (account was removed from keyring). + expect(keyring.removeAccount).toHaveBeenCalledWith(account.address); + // createAccount was attempted. + expect(mocks.keyring.createAccount).toHaveBeenCalled(); + }); + }); + describe('isDisabled', () => { it('returns false when the provider is enabled (default)', () => { const { provider } = setup(); diff --git a/packages/multichain-account-service/src/providers/BtcAccountProvider.ts b/packages/multichain-account-service/src/providers/BtcAccountProvider.ts index 5661dbaa30..7ddcbd8063 100644 --- a/packages/multichain-account-service/src/providers/BtcAccountProvider.ts +++ b/packages/multichain-account-service/src/providers/BtcAccountProvider.ts @@ -12,6 +12,8 @@ import type { SnapId } from '@metamask/snaps-sdk'; import { traceFallback } from '../analytics'; import { TraceName } from '../analytics/traces'; +import { reportError } from '../errors'; +import { projectLogger as log, WARNING_PREFIX } from '../logger'; import type { MultichainAccountServiceMessenger } from '../types'; import { SnapAccountProvider } from './SnapAccountProvider'; import type { @@ -47,7 +49,7 @@ export class BtcAccountProvider extends SnapAccountProvider { static BTC_SNAP_ID = 'npm:@metamask/bitcoin-wallet-snap' as SnapId; readonly capabilities: KeyringCapabilities = { - scopes: [BtcScope.Mainnet, BtcScope.Testnet], + scopes: [BtcScope.Mainnet, BtcScope.Testnet, BtcScope.Testnet4], bip44: { deriveIndex: true, deriveIndexRange: true, @@ -68,11 +70,132 @@ export class BtcAccountProvider extends SnapAccountProvider { isAccountCompatible(account: Bip44Account): boolean { return ( - account.type === BtcAccountType.P2wpkh && - Object.values(BtcAccountType).includes(account.type) + account.type === BtcAccountType.P2wpkh || + account.type === BtcAccountType.P2tr ); } + /** + * Override resyncAccounts to preserve account type and scope when + * re-creating missing Snap accounts. The base implementation funnels + * re-creation through createAccountV1 which hardcodes P2WPKH / Mainnet, + * silently converting P2TR or testnet4 accounts and making the original + * addresses unreachable. + * + * @param accounts - The MetaMask-side BIP-44 accounts managed by this provider. + */ + async resyncAccounts( + accounts: Bip44Account[], + ): Promise { + await this.withSnap(async ({ client, keyring }) => { + const localSnapAccounts = accounts.filter( + (account) => account.metadata.snap?.id === this.snapId, + ); + const snapAccounts = new Set( + (await client.listAccounts()).map((account) => account.id), + ); + + // Snap has more accounts than MetaMask — delete the extras. + if (localSnapAccounts.length < snapAccounts.size) { + const autoRemoveExtraSnapAccounts = + this.config.resyncAccounts?.autoRemoveExtraSnapAccounts ?? true; + + if (autoRemoveExtraSnapAccounts) { + const localAccountIds = new Set( + localSnapAccounts.map((account) => account.id), + ); + + await Promise.all( + [...snapAccounts].map(async (snapAccountId) => { + try { + if (!localAccountIds.has(snapAccountId)) { + await client.deleteAccount(snapAccountId); + snapAccounts.delete(snapAccountId); + } + } catch (error) { + reportError( + this.messenger, + `Unable to delete de-synced Snap account: ${this.snapId}`, + error, + { + provider: this.getName(), + snapAccountId, + }, + ); + } + }), + ); + } else { + const message = `Snap "${this.snapId}" has de-synced accounts, Snap has more accounts than MetaMask! (${localSnapAccounts.length} < ${snapAccounts.size})`; + log(`${WARNING_PREFIX} ${message}`); + console.warn(message); + return; + } + } + + // MetaMask has more accounts than the Snap — re-create the missing + // ones using the original account type and scope so the derived + // address matches. + if (localSnapAccounts.length > snapAccounts.size) { + await Promise.all( + localSnapAccounts.map(async (account) => { + const { id: entropySource, groupIndex } = + account.options.entropy; + + try { + if (!snapAccounts.has(account.id)) { + const scope = account.scopes?.[0]; + if (!scope) { + throw new Error( + `Account ${account.id} has no scopes, cannot determine target network for re-creation`, + ); + } + + await keyring.removeAccount(account.address); + try { + await withTimeout( + () => + keyring.createAccount({ + entropySource, + index: groupIndex, + addressType: account.type, + scope, + }), + this.config.createAccounts.timeoutMs, + ); + } catch (createError) { + // Account was removed from keyring but re-creation failed. + // State is now inconsistent — propagate so the caller knows. + reportError( + this.messenger, + `Account removed from keyring but re-creation failed, state is inconsistent`, + createError, + { + provider: this.getName(), + groupIndex, + accountId: account.id, + }, + ); + throw createError; + } + } + } catch (error) { + reportError( + this.messenger, + 'Unable to re-sync accounts', + error, + { + provider: this.getName(), + groupIndex, + }, + ); + } + }), + ); + } + }); + } + protected override createAccountV1( keyring: RestrictedSnapKeyring, { @@ -113,7 +236,7 @@ export class BtcAccountProvider extends SnapAccountProvider { withTimeout( () => client.discoverAccounts( - [BtcScope.Mainnet], + [BtcScope.Mainnet, BtcScope.Testnet, BtcScope.Testnet4], entropySource, groupIndex, ),