From e42077cbba01bfb91e075afcaed0afe49c1b25c1 Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Mon, 8 Dec 2025 15:42:37 +0100 Subject: [PATCH 1/9] feat: wip --- packages/keyring-eth-qr/package.json | 2 + packages/keyring-eth-qr/src/index.ts | 1 + .../keyring-eth-qr/src/qr-keyring-v2.test.ts | 495 ++++++++++++++++++ packages/keyring-eth-qr/src/qr-keyring-v2.ts | 281 ++++++++++ packages/keyring-eth-qr/tsconfig.json | 6 +- yarn.lock | 2 + 6 files changed, 786 insertions(+), 1 deletion(-) create mode 100644 packages/keyring-eth-qr/src/qr-keyring-v2.test.ts create mode 100644 packages/keyring-eth-qr/src/qr-keyring-v2.ts diff --git a/packages/keyring-eth-qr/package.json b/packages/keyring-eth-qr/package.json index 080ecb5d8..b22df5a00 100644 --- a/packages/keyring-eth-qr/package.json +++ b/packages/keyring-eth-qr/package.json @@ -53,6 +53,7 @@ "@ethereumjs/util": "^9.1.0", "@keystonehq/bc-ur-registry-eth": "^0.19.1", "@metamask/eth-sig-util": "^8.2.0", + "@metamask/keyring-api": "workspace:^", "@metamask/keyring-utils": "workspace:^", "@metamask/utils": "^11.1.0", "async-mutex": "^0.5.0", @@ -63,6 +64,7 @@ "@ethereumjs/common": "^4.4.0", "@keystonehq/metamask-airgapped-keyring": "^0.15.2", "@lavamoat/allow-scripts": "^3.2.1", + "@metamask/account-api": "workspace:^", "@metamask/auto-changelog": "^3.4.4", "@types/hdkey": "^2.0.1", "@types/jest": "^29.5.12", diff --git a/packages/keyring-eth-qr/src/index.ts b/packages/keyring-eth-qr/src/index.ts index 36a60d855..3fdfed9e4 100644 --- a/packages/keyring-eth-qr/src/index.ts +++ b/packages/keyring-eth-qr/src/index.ts @@ -17,3 +17,4 @@ export { type SerializedQrKeyringState, type SerializedUR, } from './qr-keyring'; +export { QrKeyringV2, type QrKeyringV2Options } from './qr-keyring-v2'; diff --git a/packages/keyring-eth-qr/src/qr-keyring-v2.test.ts b/packages/keyring-eth-qr/src/qr-keyring-v2.test.ts new file mode 100644 index 000000000..c66af4c7c --- /dev/null +++ b/packages/keyring-eth-qr/src/qr-keyring-v2.test.ts @@ -0,0 +1,495 @@ +import type { Bip44Account } from '@metamask/account-api'; +import { + EthAccountType, + EthMethod, + EthScope, + type KeyringAccount, + KeyringAccountEntropyTypeOption, + KeyringType, +} from '@metamask/keyring-api'; +import type { Hex } from '@metamask/utils'; + +import type { QrKeyringBridge } from '.'; +import { QrKeyring } from '.'; +import { QrKeyringV2 } from './qr-keyring-v2'; +import { + ACCOUNT_SERIALIZED_KEYRING_WITH_ACCOUNTS, + EXPECTED_ACCOUNTS, + HDKEY_SERIALIZED_KEYRING_WITH_ACCOUNTS, + HDKEY_SERIALIZED_KEYRING_WITH_NO_ACCOUNTS, + KNOWN_CRYPTO_ACCOUNT_UR, + KNOWN_HDKEY_UR, +} from '../test/fixtures'; + +/** + * Get a mock bridge for the QrKeyring. + * + * @returns A mock bridge with a requestScan method. + */ +function getMockBridge(): QrKeyringBridge { + return { + requestScan: jest.fn(), + }; +} + +/** + * Create a QrKeyringV2 wrapper with a paired HD mode device and accounts. + * + * @param accountCount - Number of accounts to add. + * @returns The wrapper and inner keyring. + */ +async function createWrapperWithAccounts(accountCount = 3): Promise<{ + wrapper: QrKeyringV2; + inner: QrKeyring; +}> { + const inner = new QrKeyring({ + bridge: getMockBridge(), + ur: KNOWN_HDKEY_UR, + }); + await inner.addAccounts(accountCount); + + const wrapper = new QrKeyringV2({ legacyKeyring: inner }); + return { wrapper, inner }; +} + +describe('QrKeyringV2', () => { + describe('constructor', () => { + it('creates a wrapper with correct type and capabilities', () => { + const inner = new QrKeyring({ + bridge: getMockBridge(), + ur: KNOWN_HDKEY_UR, + }); + const wrapper = new QrKeyringV2({ legacyKeyring: inner }); + + expect(wrapper.type).toBe(KeyringType.Qr); + expect(wrapper.capabilities).toStrictEqual({ + scopes: [EthScope.Eoa], + bip44: { + deriveIndex: true, + derivePath: false, + discover: false, + }, + }); + }); + }); + + describe('getAccounts', () => { + it('returns empty array when no device is paired', async () => { + const inner = new QrKeyring({ bridge: getMockBridge() }); + const wrapper = new QrKeyringV2({ legacyKeyring: inner }); + + const accounts = await wrapper.getAccounts(); + + expect(accounts).toStrictEqual([]); + }); + + it('returns all accounts from the legacy keyring', async () => { + const { wrapper } = await createWrapperWithAccounts(3); + + const accounts = await wrapper.getAccounts(); + + expect(accounts).toHaveLength(3); + expect(accounts.map((a) => a.address)).toStrictEqual([ + EXPECTED_ACCOUNTS[0], + EXPECTED_ACCOUNTS[1], + EXPECTED_ACCOUNTS[2], + ]); + }); + + it('creates KeyringAccount objects with correct structure', async () => { + const { wrapper } = await createWrapperWithAccounts(1); + + const accounts = await wrapper.getAccounts(); + + expect(accounts).toHaveLength(1); + const account = accounts[0] as Bip44Account; + expect(account).toMatchObject({ + type: EthAccountType.Eoa, + address: EXPECTED_ACCOUNTS[0], + scopes: [EthScope.Eoa], + methods: [ + EthMethod.SignTransaction, + EthMethod.PersonalSign, + EthMethod.SignTypedDataV4, + ], + options: { + entropy: { + type: KeyringAccountEntropyTypeOption.Mnemonic, + id: HDKEY_SERIALIZED_KEYRING_WITH_NO_ACCOUNTS.xfp, + groupIndex: 0, + derivationPath: `m/44'/60'/0'/0/0`, + }, + }, + }); + expect(account.id).toBeDefined(); + }); + + it('caches KeyringAccount objects', async () => { + const { wrapper } = await createWrapperWithAccounts(1); + + const accounts1 = await wrapper.getAccounts(); + const accounts2 = await wrapper.getAccounts(); + + expect(accounts1[0]).toBe(accounts2[0]); + }); + + it('returns correct groupIndex for multiple accounts', async () => { + const { wrapper } = await createWrapperWithAccounts(3); + + const accounts = await wrapper.getAccounts(); + + expect( + (accounts[0] as Bip44Account).options.entropy + ?.groupIndex, + ).toBe(0); + expect( + (accounts[1] as Bip44Account).options.entropy + ?.groupIndex, + ).toBe(1); + expect( + (accounts[2] as Bip44Account).options.entropy + ?.groupIndex, + ).toBe(2); + }); + }); + + describe('deserialize', () => { + it('deserializes the legacy keyring state', async () => { + const inner = new QrKeyring({ bridge: getMockBridge() }); + const wrapper = new QrKeyringV2({ legacyKeyring: inner }); + + await wrapper.deserialize(HDKEY_SERIALIZED_KEYRING_WITH_ACCOUNTS); + + const accounts = await wrapper.getAccounts(); + expect(accounts).toHaveLength(3); + expect(accounts.map((a) => a.address)).toStrictEqual([ + EXPECTED_ACCOUNTS[0], + EXPECTED_ACCOUNTS[1], + EXPECTED_ACCOUNTS[2], + ]); + }); + + it('clears the cache and rebuilds it', async () => { + const { wrapper, inner } = await createWrapperWithAccounts(2); + + const accountsBefore = await wrapper.getAccounts(); + expect(accountsBefore).toHaveLength(2); + + // Deserialize with more accounts + await wrapper.deserialize(HDKEY_SERIALIZED_KEYRING_WITH_ACCOUNTS); + + const accountsAfter = await wrapper.getAccounts(); + expect(accountsAfter).toHaveLength(3); + + // The accounts should be new objects (cache was cleared) + expect(accountsAfter[0]).not.toBe(accountsBefore[0]); + }); + }); + + describe('createAccounts', () => { + const entropySource = HDKEY_SERIALIZED_KEYRING_WITH_NO_ACCOUNTS.xfp; + + it('creates the first account at index 0', async () => { + const inner = new QrKeyring({ + bridge: getMockBridge(), + ur: KNOWN_HDKEY_UR, + }); + const wrapper = new QrKeyringV2({ legacyKeyring: inner }); + + const newAccounts = await wrapper.createAccounts({ + type: 'bip44:derive-index', + entropySource, + groupIndex: 0, + }); + + expect(newAccounts).toHaveLength(1); + expect(newAccounts[0]?.address).toBe(EXPECTED_ACCOUNTS[0]); + }); + + it('creates an account at a specific index', async () => { + const { wrapper } = await createWrapperWithAccounts(2); + + const newAccounts = await wrapper.createAccounts({ + type: 'bip44:derive-index', + entropySource, + groupIndex: 2, + }); + + expect(newAccounts).toHaveLength(1); + expect(newAccounts[0]?.address).toBe(EXPECTED_ACCOUNTS[2]); + }); + + it('returns existing account if groupIndex already exists', async () => { + const { wrapper } = await createWrapperWithAccounts(2); + + const existingAccounts = await wrapper.getAccounts(); + const newAccounts = await wrapper.createAccounts({ + type: 'bip44:derive-index', + entropySource, + groupIndex: 0, + }); + + expect(newAccounts).toHaveLength(1); + expect(newAccounts[0]).toBe(existingAccounts[0]); + }); + + it('throws error for unsupported account creation type', async () => { + const { wrapper } = await createWrapperWithAccounts(0); + + await expect( + wrapper.createAccounts({ + type: 'private-key:import', + accountType: EthAccountType.Eoa, + encoding: 'hexadecimal', + privateKey: '0xabc', + }), + ).rejects.toThrow( + 'Unsupported account creation type for QrKeyring in HD mode: private-key:import', + ); + }); + + it('throws error for entropy source mismatch', async () => { + const { wrapper } = await createWrapperWithAccounts(0); + + await expect( + wrapper.createAccounts({ + type: 'bip44:derive-index', + entropySource: 'wrong-entropy', + groupIndex: 0, + }), + ).rejects.toThrow(/Entropy source mismatch/u); + }); + + it('throws error when no device is paired', async () => { + const inner = new QrKeyring({ bridge: getMockBridge() }); + const wrapper = new QrKeyringV2({ legacyKeyring: inner }); + + await expect( + wrapper.createAccounts({ + type: 'bip44:derive-index', + entropySource: 'some-entropy', + groupIndex: 0, + }), + ).rejects.toThrow('No device paired. Cannot create accounts.'); + }); + + it('throws when trying to skip indices', async () => { + const { wrapper } = await createWrapperWithAccounts(0); + + await expect( + wrapper.createAccounts({ + type: 'bip44:derive-index', + entropySource, + groupIndex: 5, + }), + ).rejects.toThrow( + 'Can only create the next account in sequence. Expected groupIndex 0, got 5.', + ); + }); + + it('creates multiple accounts sequentially', async () => { + const inner = new QrKeyring({ + bridge: getMockBridge(), + ur: KNOWN_HDKEY_UR, + }); + const wrapper = new QrKeyringV2({ legacyKeyring: inner }); + + const account1 = await wrapper.createAccounts({ + type: 'bip44:derive-index', + entropySource, + groupIndex: 0, + }); + const account2 = await wrapper.createAccounts({ + type: 'bip44:derive-index', + entropySource, + groupIndex: 1, + }); + const account3 = await wrapper.createAccounts({ + type: 'bip44:derive-index', + entropySource, + groupIndex: 2, + }); + + expect(account1[0]?.address).toBe(EXPECTED_ACCOUNTS[0]); + expect(account2[0]?.address).toBe(EXPECTED_ACCOUNTS[1]); + expect(account3[0]?.address).toBe(EXPECTED_ACCOUNTS[2]); + }); + + it('throws error when inner keyring fails to create account', async () => { + const inner = new QrKeyring({ + bridge: getMockBridge(), + ur: KNOWN_HDKEY_UR, + }); + const wrapper = new QrKeyringV2({ legacyKeyring: inner }); + + // Mock addAccounts to return empty array + jest.spyOn(inner, 'addAccounts').mockResolvedValueOnce([]); + + await expect( + wrapper.createAccounts({ + type: 'bip44:derive-index', + entropySource, + groupIndex: 0, + }), + ).rejects.toThrow('Failed to create new account'); + }); + }); + + describe('deleteAccount', () => { + it('removes an account from the keyring', async () => { + const { wrapper } = await createWrapperWithAccounts(3); + + const accountsBefore = await wrapper.getAccounts(); + expect(accountsBefore).toHaveLength(3); + + const accountToDelete = accountsBefore[1]; + await wrapper.deleteAccount(accountToDelete!.id); + + const accountsAfter = await wrapper.getAccounts(); + expect(accountsAfter).toHaveLength(2); + expect(accountsAfter.map((a) => a.address)).not.toContain( + accountToDelete!.address, + ); + }); + + it('removes the account from the cache', async () => { + const { wrapper } = await createWrapperWithAccounts(2); + + const accounts = await wrapper.getAccounts(); + const accountToDelete = accounts[0]; + + await wrapper.deleteAccount(accountToDelete!.id); + + // The account should not be found by ID + await expect(wrapper.getAccount(accountToDelete!.id)).rejects.toThrow( + /Account not found/u, + ); + }); + + it('throws error for non-existent account', async () => { + const { wrapper } = await createWrapperWithAccounts(1); + + await expect(wrapper.deleteAccount('non-existent-id')).rejects.toThrow( + /Account not found/u, + ); + }); + }); + + describe('getAccount', () => { + it('returns the account by ID', async () => { + const { wrapper } = await createWrapperWithAccounts(2); + + const accounts = await wrapper.getAccounts(); + const account = await wrapper.getAccount(accounts[0]!.id); + + expect(account).toBe(accounts[0]); + }); + + it('throws error for non-existent account', async () => { + const { wrapper } = await createWrapperWithAccounts(1); + + await expect(wrapper.getAccount('non-existent-id')).rejects.toThrow( + /Account not found/u, + ); + }); + }); + + describe('serialize', () => { + it('serializes the legacy keyring state', async () => { + const { wrapper, inner } = await createWrapperWithAccounts(2); + + const wrapperSerialized = await wrapper.serialize(); + const innerSerialized = await inner.serialize(); + + expect(wrapperSerialized).toStrictEqual(innerSerialized); + }); + }); + + describe('Account Mode (CryptoAccount)', () => { + const accountModeAddress = + '0x2043858DA83bCD92Ae342C1bAaD4D5F5B5C328B3' as Hex; + const accountModeEntropySourceId = + ACCOUNT_SERIALIZED_KEYRING_WITH_ACCOUNTS.xfp; + + /** + * Create a QrKeyringV2 wrapper with a paired Account mode device. + * + * @param addAccount - Whether to add the pre-defined account. + * @returns The wrapper and inner keyring. + */ + async function createAccountModeWrapper(addAccount = true): Promise<{ + wrapper: QrKeyringV2; + inner: QrKeyring; + }> { + const inner = new QrKeyring({ + bridge: getMockBridge(), + ur: KNOWN_CRYPTO_ACCOUNT_UR, + }); + if (addAccount) { + await inner.addAccounts(1); + } + + const wrapper = new QrKeyringV2({ legacyKeyring: inner }); + return { wrapper, inner }; + } + + describe('getAccounts', () => { + it('returns accounts with PrivateKey entropy type', async () => { + const { wrapper } = await createAccountModeWrapper(); + + const accounts = await wrapper.getAccounts(); + + expect(accounts).toHaveLength(1); + expect(accounts[0]?.address).toBe(accountModeAddress); + expect(accounts[0]?.options?.entropy).toStrictEqual({ + type: KeyringAccountEntropyTypeOption.PrivateKey, + }); + }); + + it('does not include BIP-44 derivation fields', async () => { + const { wrapper } = await createAccountModeWrapper(); + + const accounts = await wrapper.getAccounts(); + const entropy = accounts[0]?.options?.entropy as Record< + string, + unknown + >; + + // Account mode accounts should NOT have these BIP-44 fields + expect(entropy).not.toHaveProperty('id'); + expect(entropy).not.toHaveProperty('groupIndex'); + expect(entropy).not.toHaveProperty('derivationPath'); + }); + }); + + describe('createAccounts', () => { + it('throws error when trying to create accounts in Account mode', async () => { + const { wrapper } = await createAccountModeWrapper(false); + + await expect( + wrapper.createAccounts({ + type: 'bip44:derive-index', + entropySource: accountModeEntropySourceId, + groupIndex: 0, + }), + ).rejects.toThrow( + 'Cannot create accounts in Account mode. Accounts are pre-defined by the device.', + ); + }); + }); + + describe('deleteAccount', () => { + it('removes an account from the keyring', async () => { + const { wrapper, inner } = await createAccountModeWrapper(); + + const accounts = await wrapper.getAccounts(); + expect(accounts).toHaveLength(1); + + await wrapper.deleteAccount(accounts[0]!.id); + + const remainingAddresses = await inner.getAccounts(); + expect(remainingAddresses).toHaveLength(0); + }); + }); + }); +}); diff --git a/packages/keyring-eth-qr/src/qr-keyring-v2.ts b/packages/keyring-eth-qr/src/qr-keyring-v2.ts new file mode 100644 index 000000000..f2dfa16b7 --- /dev/null +++ b/packages/keyring-eth-qr/src/qr-keyring-v2.ts @@ -0,0 +1,281 @@ +import type { Bip44Account } from '@metamask/account-api'; +import { + type CreateAccountOptions, + EthAccountType, + EthKeyringWrapper, + EthMethod, + EthScope, + type KeyringAccount, + KeyringAccountEntropyTypeOption, + type KeyringCapabilities, + type KeyringV2, + KeyringType, +} from '@metamask/keyring-api'; +import type { AccountId } from '@metamask/keyring-utils'; +import type { Hex } from '@metamask/utils'; + +import { DeviceMode } from './device'; +import type { QrKeyring, SerializedQrKeyringState } from './qr-keyring'; + +/** + * Methods supported by QR keyring EOA accounts. + * QR keyrings support a subset of signing methods (no encryption, app keys, or EIP-7702). + */ +const QR_KEYRING_METHODS = [ + EthMethod.SignTransaction, + EthMethod.PersonalSign, + EthMethod.SignTypedDataV4, +]; + +const qrKeyringV2Capabilities: KeyringCapabilities = { + scopes: [EthScope.Eoa], + bip44: { + deriveIndex: true, + derivePath: false, + discover: false, + }, +}; + +/** + * Account type for QR keyring - can be either BIP-44 derived (HD mode) or imported (Account mode). + */ +type QrKeyringAccount = Bip44Account | KeyringAccount; + +/** + * Concrete {@link KeyringV2} adapter for {@link QrKeyring}. + * + * This wrapper exposes the accounts and signing capabilities of the legacy + * QR keyring via the unified V2 interface. + * + * QR keyrings support two modes: + * - **HD Mode**: Derives accounts from an xpub using BIP-44 paths (Bip44Account) + * - **Account Mode**: Uses pre-defined accounts from CryptoAccount (KeyringAccount with PrivateKey entropy) + */ +export type QrKeyringV2Options = { + legacyKeyring: QrKeyring; +}; + +export class QrKeyringV2 + extends EthKeyringWrapper + implements KeyringV2 +{ + constructor(options: QrKeyringV2Options) { + super({ + type: KeyringType.Qr, + inner: options.legacyKeyring, + capabilities: qrKeyringV2Capabilities, + }); + } + + /** + * Get the device state including mode and entropy source ID. + * For QR keyrings, the entropy source ID is the device fingerprint (xfp). + * + * @returns The device state, or undefined if no device is paired. + */ + async #getDeviceState(): Promise< + | { + mode: DeviceMode; + entropySourceId: string; + indexes: Record; + } + | undefined + > { + const state = + (await this.inner.serialize()) as SerializedQrKeyringState | null; + if (!state?.initialized) { + return undefined; + } + return { + mode: state.keyringMode, + entropySourceId: state.xfp, + indexes: state.indexes, + }; + } + + /** + * Creates a KeyringAccount object for HD mode (BIP-44 derived). + * + * @param address - The account address. + * @param addressIndex - The account index in the derivation path. + * @param entropySourceId - The entropy source ID (device fingerprint). + * @returns The created Bip44Account. + */ + #createHdModeAccount( + address: Hex, + addressIndex: number, + entropySourceId: string, + ): Bip44Account { + const id = this.registry.register(address); + + const account: Bip44Account = { + id, + type: EthAccountType.Eoa, + address, + scopes: [...this.capabilities.scopes], + methods: [...QR_KEYRING_METHODS], + options: { + entropy: { + type: KeyringAccountEntropyTypeOption.Mnemonic, + id: entropySourceId, + groupIndex: addressIndex, + // QR keyrings don't expose the full derivation path + derivationPath: `m/44'/60'/0'/0/${addressIndex}`, + }, + }, + }; + + this.registry.set(account); + return account; + } + + /** + * Creates a KeyringAccount object for Account mode (pre-defined accounts). + * + * @param address - The account address. + * @returns The created KeyringAccount. + */ + #createAccountModeAccount(address: Hex): KeyringAccount { + const id = this.registry.register(address); + + const account: KeyringAccount = { + id, + type: EthAccountType.Eoa, + address, + scopes: [...this.capabilities.scopes], + methods: [...QR_KEYRING_METHODS], + options: { + entropy: { + type: KeyringAccountEntropyTypeOption.PrivateKey, + }, + }, + }; + + this.registry.set(account); + return account; + } + + async getAccounts(): Promise { + const addresses = await this.inner.getAccounts(); + const deviceState = await this.#getDeviceState(); + + if (!deviceState) { + // No device paired yet, return empty + return []; + } + + const { mode, entropySourceId, indexes } = deviceState; + + return addresses.map((address) => { + // Check if we already have this account in the registry + const existingId = this.registry.getAccountId(address); + if (existingId) { + const cached = this.registry.get(existingId); + if (cached) { + return cached; + } + } + + // Create account based on device mode + if (mode === DeviceMode.HD) { + // HD mode: BIP-44 derived accounts + const addressIndex = indexes[address] ?? 0; + return this.#createHdModeAccount( + address, + addressIndex, + entropySourceId, + ); + } + // Account mode: pre-defined accounts (like imported private keys) + return this.#createAccountModeAccount(address); + }); + } + + async createAccounts( + options: CreateAccountOptions, + ): Promise { + return this.withLock(async () => { + const deviceState = await this.#getDeviceState(); + + if (!deviceState) { + throw new Error('No device paired. Cannot create accounts.'); + } + + const { mode, entropySourceId } = deviceState; + + // Account mode doesn't support creating new accounts - they're pre-defined + if (mode === DeviceMode.ACCOUNT) { + throw new Error( + 'Cannot create accounts in Account mode. Accounts are pre-defined by the device.', + ); + } + + // HD mode: only supports BIP-44 derive index + if (options.type !== 'bip44:derive-index') { + throw new Error( + `Unsupported account creation type for QrKeyring in HD mode: ${options.type}`, + ); + } + + // Validate that the entropy source matches this keyring's entropy source + if (options.entropySource !== entropySourceId) { + throw new Error( + `Entropy source mismatch: expected '${entropySourceId}', got '${options.entropySource}'`, + ); + } + + // Sync with the inner keyring state in case it was modified externally + const currentAccounts = await this.getAccounts(); + const currentCount = currentAccounts.length; + const targetIndex = options.groupIndex; + + // Check if an account at this index already exists + const existingAccount = currentAccounts[targetIndex]; + if (existingAccount) { + return [existingAccount]; + } + + // Only allow derivation of the next account in sequence + if (targetIndex !== currentCount) { + throw new Error( + `Can only create the next account in sequence. ` + + `Expected groupIndex ${currentCount}, got ${targetIndex}.`, + ); + } + + // Set the account index to unlock and add the account + this.inner.setAccountToUnlock(targetIndex); + const [newAddress] = await this.inner.addAccounts(1); + + if (!newAddress) { + throw new Error('Failed to create new account'); + } + + const newAccount = this.#createHdModeAccount( + newAddress, + targetIndex, + entropySourceId, + ); + + return [newAccount]; + }); + } + + /** + * Delete an account from the keyring. + * + * @param accountId - The account ID to delete. + */ + async deleteAccount(accountId: AccountId): Promise { + await this.withLock(async () => { + const { address } = await this.getAccount(accountId); + const hexAddress = this.toHexAddress(address); + + // Remove from the legacy keyring + this.inner.removeAccount(hexAddress); + + // Remove from the registry + this.registry.delete(accountId); + }); + } +} diff --git a/packages/keyring-eth-qr/tsconfig.json b/packages/keyring-eth-qr/tsconfig.json index f6a8db2fb..50f5c182d 100644 --- a/packages/keyring-eth-qr/tsconfig.json +++ b/packages/keyring-eth-qr/tsconfig.json @@ -3,7 +3,11 @@ "compilerOptions": { "baseUrl": "./" }, - "references": [{ "path": "../keyring-utils" }], + "references": [ + { "path": "../keyring-utils" }, + { "path": "../keyring-api" }, + { "path": "../account-api" } + ], "include": ["./src", "./test"], "exclude": ["./dist/**/*"] } diff --git a/yarn.lock b/yarn.lock index 8389165db..f2e428d0c 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1746,8 +1746,10 @@ __metadata: "@keystonehq/bc-ur-registry-eth": "npm:^0.19.1" "@keystonehq/metamask-airgapped-keyring": "npm:^0.15.2" "@lavamoat/allow-scripts": "npm:^3.2.1" + "@metamask/account-api": "workspace:^" "@metamask/auto-changelog": "npm:^3.4.4" "@metamask/eth-sig-util": "npm:^8.2.0" + "@metamask/keyring-api": "workspace:^" "@metamask/keyring-utils": "workspace:^" "@metamask/utils": "npm:^11.1.0" "@types/hdkey": "npm:^2.0.1" From c14f22b36c6bec4e20734c6a8bc9237e9457f5db Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Wed, 10 Dec 2025 10:47:16 +0100 Subject: [PATCH 2/9] feat: default to bip44 accounts and simplify flows --- packages/keyring-eth-hd/src/hd-keyring-v2.ts | 1 + .../keyring-eth-qr/src/qr-keyring-v2.test.ts | 143 +++++++++-------- packages/keyring-eth-qr/src/qr-keyring-v2.ts | 145 ++++++------------ packages/keyring-eth-qr/src/qr-keyring.ts | 11 ++ .../src/simple-keyring-v2.ts | 1 + 5 files changed, 142 insertions(+), 159 deletions(-) diff --git a/packages/keyring-eth-hd/src/hd-keyring-v2.ts b/packages/keyring-eth-hd/src/hd-keyring-v2.ts index ed504d0d0..3e292be3e 100644 --- a/packages/keyring-eth-hd/src/hd-keyring-v2.ts +++ b/packages/keyring-eth-hd/src/hd-keyring-v2.ts @@ -116,6 +116,7 @@ export class HdKeyringV2 groupIndex: addressIndex, derivationPath: `${this.inner.hdPath}/${addressIndex}`, }, + exportable: true, }, }; diff --git a/packages/keyring-eth-qr/src/qr-keyring-v2.test.ts b/packages/keyring-eth-qr/src/qr-keyring-v2.test.ts index c66af4c7c..1cad9f616 100644 --- a/packages/keyring-eth-qr/src/qr-keyring-v2.test.ts +++ b/packages/keyring-eth-qr/src/qr-keyring-v2.test.ts @@ -7,7 +7,6 @@ import { KeyringAccountEntropyTypeOption, KeyringType, } from '@metamask/keyring-api'; -import type { Hex } from '@metamask/utils'; import type { QrKeyringBridge } from '.'; import { QrKeyring } from '.'; @@ -21,6 +20,8 @@ import { KNOWN_HDKEY_UR, } from '../test/fixtures'; +const entropySource = HDKEY_SERIALIZED_KEYRING_WITH_NO_ACCOUNTS.xfp; + /** * Get a mock bridge for the QrKeyring. * @@ -48,7 +49,7 @@ async function createWrapperWithAccounts(accountCount = 3): Promise<{ }); await inner.addAccounts(accountCount); - const wrapper = new QrKeyringV2({ legacyKeyring: inner }); + const wrapper = new QrKeyringV2({ legacyKeyring: inner, entropySource }); return { wrapper, inner }; } @@ -59,7 +60,7 @@ describe('QrKeyringV2', () => { bridge: getMockBridge(), ur: KNOWN_HDKEY_UR, }); - const wrapper = new QrKeyringV2({ legacyKeyring: inner }); + const wrapper = new QrKeyringV2({ legacyKeyring: inner, entropySource }); expect(wrapper.type).toBe(KeyringType.Qr); expect(wrapper.capabilities).toStrictEqual({ @@ -76,7 +77,7 @@ describe('QrKeyringV2', () => { describe('getAccounts', () => { it('returns empty array when no device is paired', async () => { const inner = new QrKeyring({ bridge: getMockBridge() }); - const wrapper = new QrKeyringV2({ legacyKeyring: inner }); + const wrapper = new QrKeyringV2({ legacyKeyring: inner, entropySource }); const accounts = await wrapper.getAccounts(); @@ -151,12 +152,52 @@ describe('QrKeyringV2', () => { ?.groupIndex, ).toBe(2); }); + + it('throws error in HD mode when address is not in indexes map', async () => { + const inner = new QrKeyring({ + bridge: getMockBridge(), + ur: KNOWN_HDKEY_UR, + }); + await inner.addAccounts(1); + const wrapper = new QrKeyringV2({ legacyKeyring: inner, entropySource }); + + // Mock serialize to return an inconsistent state + // where the keyring mode is HD but the indexes map is empty + jest.spyOn(inner, 'serialize').mockResolvedValue({ + ...HDKEY_SERIALIZED_KEYRING_WITH_ACCOUNTS, + indexes: {}, // Empty indexes - inconsistent with accounts + }); + + await expect(wrapper.getAccounts()).rejects.toThrow( + /not found in device indexes/u, + ); + }); + + it('uses empty string for derivationPath when getPathFromAddress returns undefined', async () => { + const inner = new QrKeyring({ + bridge: getMockBridge(), + ur: KNOWN_HDKEY_UR, + }); + await inner.addAccounts(1); + const wrapper = new QrKeyringV2({ legacyKeyring: inner, entropySource }); + + // Mock getPathFromAddress to return undefined + jest.spyOn(inner, 'getPathFromAddress').mockReturnValue(undefined); + + const accounts = await wrapper.getAccounts(); + + expect(accounts).toHaveLength(1); + expect( + (accounts[0] as Bip44Account).options.entropy + .derivationPath, + ).toBe(''); + }); }); describe('deserialize', () => { it('deserializes the legacy keyring state', async () => { const inner = new QrKeyring({ bridge: getMockBridge() }); - const wrapper = new QrKeyringV2({ legacyKeyring: inner }); + const wrapper = new QrKeyringV2({ legacyKeyring: inner, entropySource }); await wrapper.deserialize(HDKEY_SERIALIZED_KEYRING_WITH_ACCOUNTS); @@ -187,14 +228,12 @@ describe('QrKeyringV2', () => { }); describe('createAccounts', () => { - const entropySource = HDKEY_SERIALIZED_KEYRING_WITH_NO_ACCOUNTS.xfp; - it('creates the first account at index 0', async () => { const inner = new QrKeyring({ bridge: getMockBridge(), ur: KNOWN_HDKEY_UR, }); - const wrapper = new QrKeyringV2({ legacyKeyring: inner }); + const wrapper = new QrKeyringV2({ legacyKeyring: inner, entropySource }); const newAccounts = await wrapper.createAccounts({ type: 'bip44:derive-index', @@ -244,7 +283,7 @@ describe('QrKeyringV2', () => { privateKey: '0xabc', }), ).rejects.toThrow( - 'Unsupported account creation type for QrKeyring in HD mode: private-key:import', + 'Unsupported account creation type for QrKeyring: private-key:import', ); }); @@ -262,7 +301,7 @@ describe('QrKeyringV2', () => { it('throws error when no device is paired', async () => { const inner = new QrKeyring({ bridge: getMockBridge() }); - const wrapper = new QrKeyringV2({ legacyKeyring: inner }); + const wrapper = new QrKeyringV2({ legacyKeyring: inner, entropySource }); await expect( wrapper.createAccounts({ @@ -273,18 +312,26 @@ describe('QrKeyringV2', () => { ).rejects.toThrow('No device paired. Cannot create accounts.'); }); - it('throws when trying to skip indices', async () => { - const { wrapper } = await createWrapperWithAccounts(0); + it('allows deriving accounts at any index (non-sequential)', async () => { + const inner = new QrKeyring({ + bridge: getMockBridge(), + ur: KNOWN_HDKEY_UR, + }); + const wrapper = new QrKeyringV2({ legacyKeyring: inner, entropySource }); - await expect( - wrapper.createAccounts({ - type: 'bip44:derive-index', - entropySource, - groupIndex: 5, - }), - ).rejects.toThrow( - 'Can only create the next account in sequence. Expected groupIndex 0, got 5.', - ); + // Skip index 0 and go directly to index 5 + const accounts = await wrapper.createAccounts({ + type: 'bip44:derive-index', + entropySource, + groupIndex: 5, + }); + + expect(accounts).toHaveLength(1); + expect(accounts[0]?.address).toBe(EXPECTED_ACCOUNTS[5]); + expect( + (accounts[0] as Bip44Account).options.entropy + .groupIndex, + ).toBe(5); }); it('creates multiple accounts sequentially', async () => { @@ -292,7 +339,7 @@ describe('QrKeyringV2', () => { bridge: getMockBridge(), ur: KNOWN_HDKEY_UR, }); - const wrapper = new QrKeyringV2({ legacyKeyring: inner }); + const wrapper = new QrKeyringV2({ legacyKeyring: inner, entropySource }); const account1 = await wrapper.createAccounts({ type: 'bip44:derive-index', @@ -320,7 +367,7 @@ describe('QrKeyringV2', () => { bridge: getMockBridge(), ur: KNOWN_HDKEY_UR, }); - const wrapper = new QrKeyringV2({ legacyKeyring: inner }); + const wrapper = new QrKeyringV2({ legacyKeyring: inner, entropySource }); // Mock addAccounts to return empty array jest.spyOn(inner, 'addAccounts').mockResolvedValueOnce([]); @@ -406,9 +453,8 @@ describe('QrKeyringV2', () => { }); describe('Account Mode (CryptoAccount)', () => { - const accountModeAddress = - '0x2043858DA83bCD92Ae342C1bAaD4D5F5B5C328B3' as Hex; - const accountModeEntropySourceId = + const accountModeAddress = '0x2043858DA83bCD92Ae342C1bAaD4D5F5B5C328B3'; + const accountModeEntropySource = ACCOUNT_SERIALIZED_KEYRING_WITH_ACCOUNTS.xfp; /** @@ -429,52 +475,29 @@ describe('QrKeyringV2', () => { await inner.addAccounts(1); } - const wrapper = new QrKeyringV2({ legacyKeyring: inner }); + const wrapper = new QrKeyringV2({ + legacyKeyring: inner, + entropySource: accountModeEntropySource, + }); return { wrapper, inner }; } describe('getAccounts', () => { - it('returns accounts with PrivateKey entropy type', async () => { + it('returns accounts with Mnemonic entropy type (BIP-44 derived)', async () => { const { wrapper } = await createAccountModeWrapper(); const accounts = await wrapper.getAccounts(); expect(accounts).toHaveLength(1); expect(accounts[0]?.address).toBe(accountModeAddress); - expect(accounts[0]?.options?.entropy).toStrictEqual({ - type: KeyringAccountEntropyTypeOption.PrivateKey, - }); - }); - - it('does not include BIP-44 derivation fields', async () => { - const { wrapper } = await createAccountModeWrapper(); - const accounts = await wrapper.getAccounts(); - const entropy = accounts[0]?.options?.entropy as Record< - string, - unknown - >; - - // Account mode accounts should NOT have these BIP-44 fields - expect(entropy).not.toHaveProperty('id'); - expect(entropy).not.toHaveProperty('groupIndex'); - expect(entropy).not.toHaveProperty('derivationPath'); - }); - }); - - describe('createAccounts', () => { - it('throws error when trying to create accounts in Account mode', async () => { - const { wrapper } = await createAccountModeWrapper(false); - - await expect( - wrapper.createAccounts({ - type: 'bip44:derive-index', - entropySource: accountModeEntropySourceId, - groupIndex: 0, - }), - ).rejects.toThrow( - 'Cannot create accounts in Account mode. Accounts are pre-defined by the device.', + const account = accounts[0] as Bip44Account; + expect(account.options.entropy.type).toBe( + KeyringAccountEntropyTypeOption.Mnemonic, ); + expect(account.options.entropy.id).toBe(accountModeEntropySource); + expect(account.options.entropy.groupIndex).toBe(0); + expect(account.options.entropy.derivationPath).toBeDefined(); }); }); diff --git a/packages/keyring-eth-qr/src/qr-keyring-v2.ts b/packages/keyring-eth-qr/src/qr-keyring-v2.ts index f2dfa16b7..f101da8d5 100644 --- a/packages/keyring-eth-qr/src/qr-keyring-v2.ts +++ b/packages/keyring-eth-qr/src/qr-keyring-v2.ts @@ -10,12 +10,13 @@ import { type KeyringCapabilities, type KeyringV2, KeyringType, + EntropySourceId, } from '@metamask/keyring-api'; import type { AccountId } from '@metamask/keyring-utils'; import type { Hex } from '@metamask/utils'; import { DeviceMode } from './device'; -import type { QrKeyring, SerializedQrKeyringState } from './qr-keyring'; +import type { QrKeyring } from './qr-keyring'; /** * Methods supported by QR keyring EOA accounts. @@ -36,75 +37,65 @@ const qrKeyringV2Capabilities: KeyringCapabilities = { }, }; -/** - * Account type for QR keyring - can be either BIP-44 derived (HD mode) or imported (Account mode). - */ -type QrKeyringAccount = Bip44Account | KeyringAccount; - /** * Concrete {@link KeyringV2} adapter for {@link QrKeyring}. * * This wrapper exposes the accounts and signing capabilities of the legacy * QR keyring via the unified V2 interface. * - * QR keyrings support two modes: - * - **HD Mode**: Derives accounts from an xpub using BIP-44 paths (Bip44Account) - * - **Account Mode**: Uses pre-defined accounts from CryptoAccount (KeyringAccount with PrivateKey entropy) + * All QR keyring accounts are BIP-44 derived (both HD and Account modes use + * derivation paths from the hardware device). */ export type QrKeyringV2Options = { legacyKeyring: QrKeyring; + entropySource: EntropySourceId; }; export class QrKeyringV2 - extends EthKeyringWrapper + extends EthKeyringWrapper> implements KeyringV2 { + readonly entropySource: EntropySourceId; + constructor(options: QrKeyringV2Options) { super({ type: KeyringType.Qr, inner: options.legacyKeyring, capabilities: qrKeyringV2Capabilities, }); + this.entropySource = options.entropySource; } /** - * Get the device state including mode and entropy source ID. - * For QR keyrings, the entropy source ID is the device fingerprint (xfp). + * Get the device state from the inner keyring. * * @returns The device state, or undefined if no device is paired. */ async #getDeviceState(): Promise< - | { - mode: DeviceMode; - entropySourceId: string; - indexes: Record; - } - | undefined + { mode: DeviceMode; indexes: Record } | undefined > { - const state = - (await this.inner.serialize()) as SerializedQrKeyringState | null; + const state = await this.inner.serialize(); + if (!state?.initialized) { return undefined; } + return { mode: state.keyringMode, - entropySourceId: state.xfp, indexes: state.indexes, }; } /** - * Creates a KeyringAccount object for HD mode (BIP-44 derived). + * Creates a Bip44Account object for the given address. * * @param address - The account address. * @param addressIndex - The account index in the derivation path. - * @param entropySourceId - The entropy source ID (device fingerprint). * @returns The created Bip44Account. */ - #createHdModeAccount( + #createAccount( address: Hex, addressIndex: number, - entropySourceId: string, ): Bip44Account { const id = this.registry.register(address); @@ -117,36 +108,9 @@ export class QrKeyringV2 options: { entropy: { type: KeyringAccountEntropyTypeOption.Mnemonic, - id: entropySourceId, + id: this.entropySource, groupIndex: addressIndex, - // QR keyrings don't expose the full derivation path - derivationPath: `m/44'/60'/0'/0/${addressIndex}`, - }, - }, - }; - - this.registry.set(account); - return account; - } - - /** - * Creates a KeyringAccount object for Account mode (pre-defined accounts). - * - * @param address - The account address. - * @returns The created KeyringAccount. - */ - #createAccountModeAccount(address: Hex): KeyringAccount { - const id = this.registry.register(address); - - const account: KeyringAccount = { - id, - type: EthAccountType.Eoa, - address, - scopes: [...this.capabilities.scopes], - methods: [...QR_KEYRING_METHODS], - options: { - entropy: { - type: KeyringAccountEntropyTypeOption.PrivateKey, + derivationPath: this.inner.getPathFromAddress(address) ?? '', }, }, }; @@ -155,7 +119,7 @@ export class QrKeyringV2 return account; } - async getAccounts(): Promise { + async getAccounts(): Promise[]> { const addresses = await this.inner.getAccounts(); const deviceState = await this.#getDeviceState(); @@ -164,9 +128,9 @@ export class QrKeyringV2 return []; } - const { mode, entropySourceId, indexes } = deviceState; + const { mode, indexes } = deviceState; - return addresses.map((address) => { + return addresses.map((address, arrayIndex) => { // Check if we already have this account in the registry const existingId = this.registry.getAccountId(address); if (existingId) { @@ -176,24 +140,28 @@ export class QrKeyringV2 } } - // Create account based on device mode + let addressIndex: number; if (mode === DeviceMode.HD) { - // HD mode: BIP-44 derived accounts - const addressIndex = indexes[address] ?? 0; - return this.#createHdModeAccount( - address, - addressIndex, - entropySourceId, - ); + // HD mode: index must be in the map + const index = indexes[address]; + if (index === undefined) { + throw new Error( + `Address ${address} not found in device indexes. This indicates an inconsistent keyring state.`, + ); + } + addressIndex = index; + } else { + // Account mode: use array position (indexes map is not populated) + addressIndex = arrayIndex; } - // Account mode: pre-defined accounts (like imported private keys) - return this.#createAccountModeAccount(address); + + return this.#createAccount(address, addressIndex); }); } async createAccounts( options: CreateAccountOptions, - ): Promise { + ): Promise[]> { return this.withLock(async () => { const deviceState = await this.#getDeviceState(); @@ -201,49 +169,32 @@ export class QrKeyringV2 throw new Error('No device paired. Cannot create accounts.'); } - const { mode, entropySourceId } = deviceState; - - // Account mode doesn't support creating new accounts - they're pre-defined - if (mode === DeviceMode.ACCOUNT) { - throw new Error( - 'Cannot create accounts in Account mode. Accounts are pre-defined by the device.', - ); - } - - // HD mode: only supports BIP-44 derive index + // Only supports BIP-44 derive index if (options.type !== 'bip44:derive-index') { throw new Error( - `Unsupported account creation type for QrKeyring in HD mode: ${options.type}`, + `Unsupported account creation type for QrKeyring: ${options.type}`, ); } // Validate that the entropy source matches this keyring's entropy source - if (options.entropySource !== entropySourceId) { + if (options.entropySource !== this.entropySource) { throw new Error( - `Entropy source mismatch: expected '${entropySourceId}', got '${options.entropySource}'`, + `Entropy source mismatch: expected '${this.entropySource}', got '${options.entropySource}'`, ); } - // Sync with the inner keyring state in case it was modified externally - const currentAccounts = await this.getAccounts(); - const currentCount = currentAccounts.length; const targetIndex = options.groupIndex; // Check if an account at this index already exists - const existingAccount = currentAccounts[targetIndex]; + const currentAccounts = await this.getAccounts(); + const existingAccount = currentAccounts.find( + (account) => account.options.entropy.groupIndex === targetIndex, + ); if (existingAccount) { return [existingAccount]; } - // Only allow derivation of the next account in sequence - if (targetIndex !== currentCount) { - throw new Error( - `Can only create the next account in sequence. ` + - `Expected groupIndex ${currentCount}, got ${targetIndex}.`, - ); - } - - // Set the account index to unlock and add the account + // Derive the account at the specified index this.inner.setAccountToUnlock(targetIndex); const [newAddress] = await this.inner.addAccounts(1); @@ -251,11 +202,7 @@ export class QrKeyringV2 throw new Error('Failed to create new account'); } - const newAccount = this.#createHdModeAccount( - newAddress, - targetIndex, - entropySourceId, - ); + const newAccount = this.#createAccount(newAddress, targetIndex); return [newAccount]; }); diff --git a/packages/keyring-eth-qr/src/qr-keyring.ts b/packages/keyring-eth-qr/src/qr-keyring.ts index f821ba975..d0adce915 100644 --- a/packages/keyring-eth-qr/src/qr-keyring.ts +++ b/packages/keyring-eth-qr/src/qr-keyring.ts @@ -157,6 +157,17 @@ export class QrKeyring implements Keyring { this.#accounts = (state.accounts ?? []).map(normalizeAddress); } + /** + * Get the derivation path for a given address. + * + * @param address - The address to get the derivation path for. + * @returns The derivation path for the address, or undefined if the device + * is not paired or the address is not found. + */ + getPathFromAddress(address: Hex): string | undefined { + return this.#device?.pathFromAddress(address); + } + /** * Adds accounts to the QrKeyring * diff --git a/packages/keyring-eth-simple/src/simple-keyring-v2.ts b/packages/keyring-eth-simple/src/simple-keyring-v2.ts index 236ca68ae..68e03051c 100644 --- a/packages/keyring-eth-simple/src/simple-keyring-v2.ts +++ b/packages/keyring-eth-simple/src/simple-keyring-v2.ts @@ -88,6 +88,7 @@ export class SimpleKeyringV2 entropy: { type: KeyringAccountEntropyTypeOption.PrivateKey, }, + exportable: true, }, }; From b910609ca7d0255c738492e2fed3d757f0480222 Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Wed, 10 Dec 2025 11:20:12 +0100 Subject: [PATCH 3/9] fix: refine UTs + fix eslint errors --- .../keyring-eth-qr/src/qr-keyring-v2.test.ts | 322 +++++++++--------- packages/keyring-eth-qr/src/qr-keyring-v2.ts | 2 +- 2 files changed, 154 insertions(+), 170 deletions(-) diff --git a/packages/keyring-eth-qr/src/qr-keyring-v2.test.ts b/packages/keyring-eth-qr/src/qr-keyring-v2.test.ts index 1cad9f616..f6aee0b02 100644 --- a/packages/keyring-eth-qr/src/qr-keyring-v2.test.ts +++ b/packages/keyring-eth-qr/src/qr-keyring-v2.test.ts @@ -20,8 +20,49 @@ import { KNOWN_HDKEY_UR, } from '../test/fixtures'; +/** + * Type alias for QR keyring accounts (always BIP-44 derived). + */ +type QrAccount = Bip44Account; + +/** + * Get the first account from an array, throwing if empty. + * + * @param accounts - The accounts array. + * @returns The first account. + */ +function getFirstAccount(accounts: QrAccount[]): QrAccount { + if (accounts.length === 0) { + throw new Error('Expected at least one account'); + } + return accounts[0] as QrAccount; +} + +/** + * Get an account at a specific index, throwing if not present. + * + * @param accounts - The accounts array. + * @param index - The index to retrieve. + * @returns The account at the index. + */ +function getAccountAt(accounts: QrAccount[], index: number): QrAccount { + if (accounts.length <= index) { + throw new Error(`Expected account at index ${index}`); + } + return accounts[index] as QrAccount; +} + const entropySource = HDKEY_SERIALIZED_KEYRING_WITH_NO_ACCOUNTS.xfp; +/** + * Expected methods supported by QR keyring accounts. + */ +const EXPECTED_METHODS = [ + EthMethod.SignTransaction, + EthMethod.PersonalSign, + EthMethod.SignTypedDataV4, +]; + /** * Get a mock bridge for the QrKeyring. * @@ -33,6 +74,18 @@ function getMockBridge(): QrKeyringBridge { }; } +/** + * Create a fresh QrKeyring with HD mode device. + * + * @returns The inner keyring. + */ +function createInnerKeyring(): QrKeyring { + return new QrKeyring({ + bridge: getMockBridge(), + ur: KNOWN_HDKEY_UR, + }); +} + /** * Create a QrKeyringV2 wrapper with a paired HD mode device and accounts. * @@ -43,24 +96,50 @@ async function createWrapperWithAccounts(accountCount = 3): Promise<{ wrapper: QrKeyringV2; inner: QrKeyring; }> { - const inner = new QrKeyring({ - bridge: getMockBridge(), - ur: KNOWN_HDKEY_UR, - }); + const inner = createInnerKeyring(); await inner.addAccounts(accountCount); const wrapper = new QrKeyringV2({ legacyKeyring: inner, entropySource }); return { wrapper, inner }; } +/** + * Create a QrKeyringV2 wrapper without any accounts. + * + * @returns The wrapper and inner keyring. + */ +function createEmptyWrapper(): { wrapper: QrKeyringV2; inner: QrKeyring } { + const inner = createInnerKeyring(); + const wrapper = new QrKeyringV2({ legacyKeyring: inner, entropySource }); + return { wrapper, inner }; +} + +/** + * Helper to create account options for bip44:derive-index. + * + * @param groupIndex - The group index to derive. + * @param source - Optional entropy source override. + * @returns The create account options. + */ +function deriveIndexOptions( + groupIndex: number, + source: string = entropySource, +): { + type: 'bip44:derive-index'; + entropySource: string; + groupIndex: number; +} { + return { + type: 'bip44:derive-index' as const, + entropySource: source, + groupIndex, + }; +} + describe('QrKeyringV2', () => { describe('constructor', () => { it('creates a wrapper with correct type and capabilities', () => { - const inner = new QrKeyring({ - bridge: getMockBridge(), - ur: KNOWN_HDKEY_UR, - }); - const wrapper = new QrKeyringV2({ legacyKeyring: inner, entropySource }); + const { wrapper } = createEmptyWrapper(); expect(wrapper.type).toBe(KeyringType.Qr); expect(wrapper.capabilities).toStrictEqual({ @@ -90,33 +169,26 @@ describe('QrKeyringV2', () => { const accounts = await wrapper.getAccounts(); expect(accounts).toHaveLength(3); - expect(accounts.map((a) => a.address)).toStrictEqual([ - EXPECTED_ACCOUNTS[0], - EXPECTED_ACCOUNTS[1], - EXPECTED_ACCOUNTS[2], - ]); + expect(accounts.map((a) => a.address)).toStrictEqual( + EXPECTED_ACCOUNTS.slice(0, 3), + ); }); it('creates KeyringAccount objects with correct structure', async () => { const { wrapper } = await createWrapperWithAccounts(1); const accounts = await wrapper.getAccounts(); + const account = getFirstAccount(accounts); - expect(accounts).toHaveLength(1); - const account = accounts[0] as Bip44Account; expect(account).toMatchObject({ type: EthAccountType.Eoa, address: EXPECTED_ACCOUNTS[0], scopes: [EthScope.Eoa], - methods: [ - EthMethod.SignTransaction, - EthMethod.PersonalSign, - EthMethod.SignTypedDataV4, - ], + methods: EXPECTED_METHODS, options: { entropy: { type: KeyringAccountEntropyTypeOption.Mnemonic, - id: HDKEY_SERIALIZED_KEYRING_WITH_NO_ACCOUNTS.xfp, + id: entropySource, groupIndex: 0, derivationPath: `m/44'/60'/0'/0/0`, }, @@ -139,30 +211,15 @@ describe('QrKeyringV2', () => { const accounts = await wrapper.getAccounts(); - expect( - (accounts[0] as Bip44Account).options.entropy - ?.groupIndex, - ).toBe(0); - expect( - (accounts[1] as Bip44Account).options.entropy - ?.groupIndex, - ).toBe(1); - expect( - (accounts[2] as Bip44Account).options.entropy - ?.groupIndex, - ).toBe(2); + accounts.forEach((account, index) => { + expect(account.options.entropy.groupIndex).toBe(index); + }); }); it('throws error in HD mode when address is not in indexes map', async () => { - const inner = new QrKeyring({ - bridge: getMockBridge(), - ur: KNOWN_HDKEY_UR, - }); - await inner.addAccounts(1); - const wrapper = new QrKeyringV2({ legacyKeyring: inner, entropySource }); + const { wrapper, inner } = await createWrapperWithAccounts(1); // Mock serialize to return an inconsistent state - // where the keyring mode is HD but the indexes map is empty jest.spyOn(inner, 'serialize').mockResolvedValue({ ...HDKEY_SERIALIZED_KEYRING_WITH_ACCOUNTS, indexes: {}, // Empty indexes - inconsistent with accounts @@ -174,23 +231,14 @@ describe('QrKeyringV2', () => { }); it('uses empty string for derivationPath when getPathFromAddress returns undefined', async () => { - const inner = new QrKeyring({ - bridge: getMockBridge(), - ur: KNOWN_HDKEY_UR, - }); - await inner.addAccounts(1); - const wrapper = new QrKeyringV2({ legacyKeyring: inner, entropySource }); + const { wrapper, inner } = await createWrapperWithAccounts(1); - // Mock getPathFromAddress to return undefined jest.spyOn(inner, 'getPathFromAddress').mockReturnValue(undefined); const accounts = await wrapper.getAccounts(); + const account = getFirstAccount(accounts); - expect(accounts).toHaveLength(1); - expect( - (accounts[0] as Bip44Account).options.entropy - .derivationPath, - ).toBe(''); + expect(account.options.entropy.derivationPath).toBe(''); }); }); @@ -203,25 +251,21 @@ describe('QrKeyringV2', () => { const accounts = await wrapper.getAccounts(); expect(accounts).toHaveLength(3); - expect(accounts.map((a) => a.address)).toStrictEqual([ - EXPECTED_ACCOUNTS[0], - EXPECTED_ACCOUNTS[1], - EXPECTED_ACCOUNTS[2], - ]); + expect(accounts.map((a) => a.address)).toStrictEqual( + EXPECTED_ACCOUNTS.slice(0, 3), + ); }); it('clears the cache and rebuilds it', async () => { - const { wrapper, inner } = await createWrapperWithAccounts(2); + const { wrapper } = await createWrapperWithAccounts(2); const accountsBefore = await wrapper.getAccounts(); expect(accountsBefore).toHaveLength(2); - // Deserialize with more accounts await wrapper.deserialize(HDKEY_SERIALIZED_KEYRING_WITH_ACCOUNTS); const accountsAfter = await wrapper.getAccounts(); expect(accountsAfter).toHaveLength(3); - // The accounts should be new objects (cache was cleared) expect(accountsAfter[0]).not.toBe(accountsBefore[0]); }); @@ -229,47 +273,32 @@ describe('QrKeyringV2', () => { describe('createAccounts', () => { it('creates the first account at index 0', async () => { - const inner = new QrKeyring({ - bridge: getMockBridge(), - ur: KNOWN_HDKEY_UR, - }); - const wrapper = new QrKeyringV2({ legacyKeyring: inner, entropySource }); + const { wrapper } = createEmptyWrapper(); - const newAccounts = await wrapper.createAccounts({ - type: 'bip44:derive-index', - entropySource, - groupIndex: 0, - }); + const newAccounts = await wrapper.createAccounts(deriveIndexOptions(0)); + const account = getFirstAccount(newAccounts); - expect(newAccounts).toHaveLength(1); - expect(newAccounts[0]?.address).toBe(EXPECTED_ACCOUNTS[0]); + expect(account.address).toBe(EXPECTED_ACCOUNTS[0]); }); it('creates an account at a specific index', async () => { const { wrapper } = await createWrapperWithAccounts(2); - const newAccounts = await wrapper.createAccounts({ - type: 'bip44:derive-index', - entropySource, - groupIndex: 2, - }); + const newAccounts = await wrapper.createAccounts(deriveIndexOptions(2)); + const account = getFirstAccount(newAccounts); - expect(newAccounts).toHaveLength(1); - expect(newAccounts[0]?.address).toBe(EXPECTED_ACCOUNTS[2]); + expect(account.address).toBe(EXPECTED_ACCOUNTS[2]); }); it('returns existing account if groupIndex already exists', async () => { const { wrapper } = await createWrapperWithAccounts(2); const existingAccounts = await wrapper.getAccounts(); - const newAccounts = await wrapper.createAccounts({ - type: 'bip44:derive-index', - entropySource, - groupIndex: 0, - }); + const existingAccount = getFirstAccount(existingAccounts); + const newAccounts = await wrapper.createAccounts(deriveIndexOptions(0)); + const returnedAccount = getFirstAccount(newAccounts); - expect(newAccounts).toHaveLength(1); - expect(newAccounts[0]).toBe(existingAccounts[0]); + expect(returnedAccount).toBe(existingAccount); }); it('throws error for unsupported account creation type', async () => { @@ -291,11 +320,7 @@ describe('QrKeyringV2', () => { const { wrapper } = await createWrapperWithAccounts(0); await expect( - wrapper.createAccounts({ - type: 'bip44:derive-index', - entropySource: 'wrong-entropy', - groupIndex: 0, - }), + wrapper.createAccounts(deriveIndexOptions(0, 'wrong-entropy')), ).rejects.toThrow(/Entropy source mismatch/u); }); @@ -304,80 +329,42 @@ describe('QrKeyringV2', () => { const wrapper = new QrKeyringV2({ legacyKeyring: inner, entropySource }); await expect( - wrapper.createAccounts({ - type: 'bip44:derive-index', - entropySource: 'some-entropy', - groupIndex: 0, - }), + wrapper.createAccounts(deriveIndexOptions(0, 'some-entropy')), ).rejects.toThrow('No device paired. Cannot create accounts.'); }); it('allows deriving accounts at any index (non-sequential)', async () => { - const inner = new QrKeyring({ - bridge: getMockBridge(), - ur: KNOWN_HDKEY_UR, - }); - const wrapper = new QrKeyringV2({ legacyKeyring: inner, entropySource }); + const { wrapper } = createEmptyWrapper(); - // Skip index 0 and go directly to index 5 - const accounts = await wrapper.createAccounts({ - type: 'bip44:derive-index', - entropySource, - groupIndex: 5, - }); + const newAccounts = await wrapper.createAccounts(deriveIndexOptions(5)); + const account = getFirstAccount(newAccounts); - expect(accounts).toHaveLength(1); - expect(accounts[0]?.address).toBe(EXPECTED_ACCOUNTS[5]); - expect( - (accounts[0] as Bip44Account).options.entropy - .groupIndex, - ).toBe(5); + expect(account.address).toBe(EXPECTED_ACCOUNTS[5]); + expect(account.options.entropy.groupIndex).toBe(5); }); it('creates multiple accounts sequentially', async () => { - const inner = new QrKeyring({ - bridge: getMockBridge(), - ur: KNOWN_HDKEY_UR, - }); - const wrapper = new QrKeyringV2({ legacyKeyring: inner, entropySource }); + const { wrapper } = createEmptyWrapper(); - const account1 = await wrapper.createAccounts({ - type: 'bip44:derive-index', - entropySource, - groupIndex: 0, - }); - const account2 = await wrapper.createAccounts({ - type: 'bip44:derive-index', - entropySource, - groupIndex: 1, - }); - const account3 = await wrapper.createAccounts({ - type: 'bip44:derive-index', - entropySource, - groupIndex: 2, - }); + const results = await Promise.all([ + wrapper.createAccounts(deriveIndexOptions(0)), + wrapper.createAccounts(deriveIndexOptions(1)), + wrapper.createAccounts(deriveIndexOptions(2)), + ]); - expect(account1[0]?.address).toBe(EXPECTED_ACCOUNTS[0]); - expect(account2[0]?.address).toBe(EXPECTED_ACCOUNTS[1]); - expect(account3[0]?.address).toBe(EXPECTED_ACCOUNTS[2]); + results.forEach((accounts, index) => { + const account = getFirstAccount(accounts); + expect(account.address).toBe(EXPECTED_ACCOUNTS[index]); + }); }); it('throws error when inner keyring fails to create account', async () => { - const inner = new QrKeyring({ - bridge: getMockBridge(), - ur: KNOWN_HDKEY_UR, - }); - const wrapper = new QrKeyringV2({ legacyKeyring: inner, entropySource }); + const { wrapper, inner } = createEmptyWrapper(); - // Mock addAccounts to return empty array jest.spyOn(inner, 'addAccounts').mockResolvedValueOnce([]); await expect( - wrapper.createAccounts({ - type: 'bip44:derive-index', - entropySource, - groupIndex: 0, - }), + wrapper.createAccounts(deriveIndexOptions(0)), ).rejects.toThrow('Failed to create new account'); }); }); @@ -387,15 +374,14 @@ describe('QrKeyringV2', () => { const { wrapper } = await createWrapperWithAccounts(3); const accountsBefore = await wrapper.getAccounts(); - expect(accountsBefore).toHaveLength(3); + const accountToDelete = getAccountAt(accountsBefore, 1); - const accountToDelete = accountsBefore[1]; - await wrapper.deleteAccount(accountToDelete!.id); + await wrapper.deleteAccount(accountToDelete.id); const accountsAfter = await wrapper.getAccounts(); expect(accountsAfter).toHaveLength(2); expect(accountsAfter.map((a) => a.address)).not.toContain( - accountToDelete!.address, + accountToDelete.address, ); }); @@ -403,12 +389,11 @@ describe('QrKeyringV2', () => { const { wrapper } = await createWrapperWithAccounts(2); const accounts = await wrapper.getAccounts(); - const accountToDelete = accounts[0]; + const accountToDelete = getFirstAccount(accounts); - await wrapper.deleteAccount(accountToDelete!.id); + await wrapper.deleteAccount(accountToDelete.id); - // The account should not be found by ID - await expect(wrapper.getAccount(accountToDelete!.id)).rejects.toThrow( + await expect(wrapper.getAccount(accountToDelete.id)).rejects.toThrow( /Account not found/u, ); }); @@ -427,9 +412,10 @@ describe('QrKeyringV2', () => { const { wrapper } = await createWrapperWithAccounts(2); const accounts = await wrapper.getAccounts(); - const account = await wrapper.getAccount(accounts[0]!.id); + const expectedAccount = getFirstAccount(accounts); + const account = await wrapper.getAccount(expectedAccount.id); - expect(account).toBe(accounts[0]); + expect(account).toBe(expectedAccount); }); it('throws error for non-existent account', async () => { @@ -487,16 +473,14 @@ describe('QrKeyringV2', () => { const { wrapper } = await createAccountModeWrapper(); const accounts = await wrapper.getAccounts(); + const account = getFirstAccount(accounts); - expect(accounts).toHaveLength(1); - expect(accounts[0]?.address).toBe(accountModeAddress); - - const account = accounts[0] as Bip44Account; - expect(account.options.entropy.type).toBe( - KeyringAccountEntropyTypeOption.Mnemonic, - ); - expect(account.options.entropy.id).toBe(accountModeEntropySource); - expect(account.options.entropy.groupIndex).toBe(0); + expect(account.address).toBe(accountModeAddress); + expect(account.options.entropy).toMatchObject({ + type: KeyringAccountEntropyTypeOption.Mnemonic, + id: accountModeEntropySource, + groupIndex: 0, + }); expect(account.options.entropy.derivationPath).toBeDefined(); }); }); @@ -506,9 +490,9 @@ describe('QrKeyringV2', () => { const { wrapper, inner } = await createAccountModeWrapper(); const accounts = await wrapper.getAccounts(); - expect(accounts).toHaveLength(1); + const account = getFirstAccount(accounts); - await wrapper.deleteAccount(accounts[0]!.id); + await wrapper.deleteAccount(account.id); const remainingAddresses = await inner.getAccounts(); expect(remainingAddresses).toHaveLength(0); diff --git a/packages/keyring-eth-qr/src/qr-keyring-v2.ts b/packages/keyring-eth-qr/src/qr-keyring-v2.ts index f101da8d5..8bc09d641 100644 --- a/packages/keyring-eth-qr/src/qr-keyring-v2.ts +++ b/packages/keyring-eth-qr/src/qr-keyring-v2.ts @@ -10,7 +10,7 @@ import { type KeyringCapabilities, type KeyringV2, KeyringType, - EntropySourceId, + type EntropySourceId, } from '@metamask/keyring-api'; import type { AccountId } from '@metamask/keyring-utils'; import type { Hex } from '@metamask/utils'; From 7aa6062df91ea0b428b9ead3a6c394056e8d15ae Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Wed, 10 Dec 2025 13:38:28 +0100 Subject: [PATCH 4/9] fix: update tsconfig.build.json --- packages/keyring-eth-qr/tsconfig.build.json | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/packages/keyring-eth-qr/tsconfig.build.json b/packages/keyring-eth-qr/tsconfig.build.json index 08ef33751..7c5eabdc3 100644 --- a/packages/keyring-eth-qr/tsconfig.build.json +++ b/packages/keyring-eth-qr/tsconfig.build.json @@ -9,6 +9,12 @@ "references": [ { "path": "../keyring-utils/tsconfig.build.json" + }, + { + "path": "../keyring-api/tsconfig.build.json" + }, + { + "path": "../account-api/tsconfig.build.json" } ], "include": ["./src/**/*.ts"], From 0e592997a4b1d49dccfd7ce300f57b4026353eb4 Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Wed, 10 Dec 2025 14:51:11 +0100 Subject: [PATCH 5/9] fix: update README --- README.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/README.md b/README.md index 0e4aba4e0..4fb7302a8 100644 --- a/README.md +++ b/README.md @@ -56,7 +56,9 @@ linkStyle default opacity:0.5 eth_hd_keyring --> keyring_utils; eth_hd_keyring --> account_api; eth_ledger_bridge_keyring --> keyring_utils; + eth_qr_keyring --> keyring_api; eth_qr_keyring --> keyring_utils; + eth_qr_keyring --> account_api; eth_simple_keyring --> keyring_api; eth_simple_keyring --> keyring_utils; eth_trezor_keyring --> keyring_utils; From a0b7e053550b4f602b7f1f9945d1c813493dff91 Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Wed, 10 Dec 2025 16:24:10 +0100 Subject: [PATCH 6/9] fix: better UT structure --- .../keyring-eth-qr/src/qr-keyring-v2.test.ts | 66 ++++++++----------- 1 file changed, 26 insertions(+), 40 deletions(-) diff --git a/packages/keyring-eth-qr/src/qr-keyring-v2.test.ts b/packages/keyring-eth-qr/src/qr-keyring-v2.test.ts index f6aee0b02..b54881525 100644 --- a/packages/keyring-eth-qr/src/qr-keyring-v2.test.ts +++ b/packages/keyring-eth-qr/src/qr-keyring-v2.test.ts @@ -114,6 +114,28 @@ function createEmptyWrapper(): { wrapper: QrKeyringV2; inner: QrKeyring } { return { wrapper, inner }; } +/** + * Create a QrKeyringV2 wrapper with a paired Account mode device. + * + * @returns The wrapper and inner keyring. + */ +async function createAccountModeWrapper(): Promise<{ + wrapper: QrKeyringV2; + inner: QrKeyring; +}> { + const inner = new QrKeyring({ + bridge: getMockBridge(), + ur: KNOWN_CRYPTO_ACCOUNT_UR, + }); + await inner.addAccounts(1); + + const wrapper = new QrKeyringV2({ + legacyKeyring: inner, + entropySource: ACCOUNT_SERIALIZED_KEYRING_WITH_ACCOUNTS.xfp, + }); + return { wrapper, inner }; +} + /** * Helper to create account options for bip44:derive-index. * @@ -197,15 +219,6 @@ describe('QrKeyringV2', () => { expect(account.id).toBeDefined(); }); - it('caches KeyringAccount objects', async () => { - const { wrapper } = await createWrapperWithAccounts(1); - - const accounts1 = await wrapper.getAccounts(); - const accounts2 = await wrapper.getAccounts(); - - expect(accounts1[0]).toBe(accounts2[0]); - }); - it('returns correct groupIndex for multiple accounts', async () => { const { wrapper } = await createWrapperWithAccounts(3); @@ -439,35 +452,6 @@ describe('QrKeyringV2', () => { }); describe('Account Mode (CryptoAccount)', () => { - const accountModeAddress = '0x2043858DA83bCD92Ae342C1bAaD4D5F5B5C328B3'; - const accountModeEntropySource = - ACCOUNT_SERIALIZED_KEYRING_WITH_ACCOUNTS.xfp; - - /** - * Create a QrKeyringV2 wrapper with a paired Account mode device. - * - * @param addAccount - Whether to add the pre-defined account. - * @returns The wrapper and inner keyring. - */ - async function createAccountModeWrapper(addAccount = true): Promise<{ - wrapper: QrKeyringV2; - inner: QrKeyring; - }> { - const inner = new QrKeyring({ - bridge: getMockBridge(), - ur: KNOWN_CRYPTO_ACCOUNT_UR, - }); - if (addAccount) { - await inner.addAccounts(1); - } - - const wrapper = new QrKeyringV2({ - legacyKeyring: inner, - entropySource: accountModeEntropySource, - }); - return { wrapper, inner }; - } - describe('getAccounts', () => { it('returns accounts with Mnemonic entropy type (BIP-44 derived)', async () => { const { wrapper } = await createAccountModeWrapper(); @@ -475,10 +459,12 @@ describe('QrKeyringV2', () => { const accounts = await wrapper.getAccounts(); const account = getFirstAccount(accounts); - expect(account.address).toBe(accountModeAddress); + expect(account.address).toBe( + ACCOUNT_SERIALIZED_KEYRING_WITH_ACCOUNTS.accounts[0], + ); expect(account.options.entropy).toMatchObject({ type: KeyringAccountEntropyTypeOption.Mnemonic, - id: accountModeEntropySource, + id: ACCOUNT_SERIALIZED_KEYRING_WITH_ACCOUNTS.xfp, groupIndex: 0, }); expect(account.options.entropy.derivationPath).toBeDefined(); From 6796f3bbd87ab645d1e9f58fb55b8dd3fc3865a2 Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Wed, 10 Dec 2025 16:25:33 +0100 Subject: [PATCH 7/9] fix: update CHANGELOG --- packages/keyring-eth-qr/CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/packages/keyring-eth-qr/CHANGELOG.md b/packages/keyring-eth-qr/CHANGELOG.md index a4ed08a05..e0b41a186 100644 --- a/packages/keyring-eth-qr/CHANGELOG.md +++ b/packages/keyring-eth-qr/CHANGELOG.md @@ -7,6 +7,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- Add `QrKeyringV2` class implementing `KeyringV2` interface ([#411](https://github.com/MetaMask/accounts/pull/411)) + - Wraps legacy `QrKeyring` to expose accounts via the unified `KeyringV2` API and the `KeyringAccount` type. + - Extends `EthKeyringWrapper` for common Ethereum logic. + ## [1.1.0] ### Added From aa6f016d7f5a36494aa78a3a6c379a0d16d94274 Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Thu, 11 Dec 2025 11:22:36 +0100 Subject: [PATCH 8/9] fix: address some PR feedbacks --- packages/keyring-eth-hd/src/hd-keyring-v2.ts | 1 - .../keyring-eth-qr/src/qr-keyring-v2.test.ts | 9 ++++----- packages/keyring-eth-qr/src/qr-keyring-v2.ts | 16 ++++++++++++---- .../keyring-eth-simple/src/simple-keyring-v2.ts | 1 - 4 files changed, 16 insertions(+), 11 deletions(-) diff --git a/packages/keyring-eth-hd/src/hd-keyring-v2.ts b/packages/keyring-eth-hd/src/hd-keyring-v2.ts index 3e292be3e..ed504d0d0 100644 --- a/packages/keyring-eth-hd/src/hd-keyring-v2.ts +++ b/packages/keyring-eth-hd/src/hd-keyring-v2.ts @@ -116,7 +116,6 @@ export class HdKeyringV2 groupIndex: addressIndex, derivationPath: `${this.inner.hdPath}/${addressIndex}`, }, - exportable: true, }, }; diff --git a/packages/keyring-eth-qr/src/qr-keyring-v2.test.ts b/packages/keyring-eth-qr/src/qr-keyring-v2.test.ts index b54881525..5120e7d2d 100644 --- a/packages/keyring-eth-qr/src/qr-keyring-v2.test.ts +++ b/packages/keyring-eth-qr/src/qr-keyring-v2.test.ts @@ -243,15 +243,14 @@ describe('QrKeyringV2', () => { ); }); - it('uses empty string for derivationPath when getPathFromAddress returns undefined', async () => { + it('throws error when getPathFromAddress returns undefined', async () => { const { wrapper, inner } = await createWrapperWithAccounts(1); jest.spyOn(inner, 'getPathFromAddress').mockReturnValue(undefined); - const accounts = await wrapper.getAccounts(); - const account = getFirstAccount(accounts); - - expect(account.options.entropy.derivationPath).toBe(''); + await expect(wrapper.getAccounts()).rejects.toThrow( + /derivation path not found in keyring/u, + ); }); }); diff --git a/packages/keyring-eth-qr/src/qr-keyring-v2.ts b/packages/keyring-eth-qr/src/qr-keyring-v2.ts index 8bc09d641..ee3001f8d 100644 --- a/packages/keyring-eth-qr/src/qr-keyring-v2.ts +++ b/packages/keyring-eth-qr/src/qr-keyring-v2.ts @@ -93,12 +93,20 @@ export class QrKeyringV2 * @param addressIndex - The account index in the derivation path. * @returns The created Bip44Account. */ - #createAccount( + #createKeyringAccount( address: Hex, addressIndex: number, ): Bip44Account { const id = this.registry.register(address); + const derivationPath = this.inner.getPathFromAddress(address); + + if (!derivationPath) { + throw new Error( + `Cannot create account for address ${address}: derivation path not found in keyring.`, + ); + } + const account: Bip44Account = { id, type: EthAccountType.Eoa, @@ -110,7 +118,7 @@ export class QrKeyringV2 type: KeyringAccountEntropyTypeOption.Mnemonic, id: this.entropySource, groupIndex: addressIndex, - derivationPath: this.inner.getPathFromAddress(address) ?? '', + derivationPath, }, }, }; @@ -155,7 +163,7 @@ export class QrKeyringV2 addressIndex = arrayIndex; } - return this.#createAccount(address, addressIndex); + return this.#createKeyringAccount(address, addressIndex); }); } @@ -202,7 +210,7 @@ export class QrKeyringV2 throw new Error('Failed to create new account'); } - const newAccount = this.#createAccount(newAddress, targetIndex); + const newAccount = this.#createKeyringAccount(newAddress, targetIndex); return [newAccount]; }); diff --git a/packages/keyring-eth-simple/src/simple-keyring-v2.ts b/packages/keyring-eth-simple/src/simple-keyring-v2.ts index 68e03051c..236ca68ae 100644 --- a/packages/keyring-eth-simple/src/simple-keyring-v2.ts +++ b/packages/keyring-eth-simple/src/simple-keyring-v2.ts @@ -88,7 +88,6 @@ export class SimpleKeyringV2 entropy: { type: KeyringAccountEntropyTypeOption.PrivateKey, }, - exportable: true, }, }; From f1eb0252d1a6cd39a03bdf26d874463a1656ac7c Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Mon, 15 Dec 2025 14:46:22 +0100 Subject: [PATCH 9/9] feat: wip qr keyring account mode support --- .../keyring-eth-qr/src/qr-keyring-v2.test.ts | 57 +++++++++---- packages/keyring-eth-qr/src/qr-keyring-v2.ts | 81 ++++++++++++++----- 2 files changed, 105 insertions(+), 33 deletions(-) diff --git a/packages/keyring-eth-qr/src/qr-keyring-v2.test.ts b/packages/keyring-eth-qr/src/qr-keyring-v2.test.ts index 5120e7d2d..34df77871 100644 --- a/packages/keyring-eth-qr/src/qr-keyring-v2.test.ts +++ b/packages/keyring-eth-qr/src/qr-keyring-v2.test.ts @@ -21,9 +21,9 @@ import { } from '../test/fixtures'; /** - * Type alias for QR keyring accounts (always BIP-44 derived). + * Type alias for QR keyring HD mode accounts (BIP-44 derived). */ -type QrAccount = Bip44Account; +type QrHdAccount = Bip44Account; /** * Get the first account from an array, throwing if empty. @@ -31,11 +31,24 @@ type QrAccount = Bip44Account; * @param accounts - The accounts array. * @returns The first account. */ -function getFirstAccount(accounts: QrAccount[]): QrAccount { +function getFirstAccount(accounts: KeyringAccount[]): KeyringAccount { if (accounts.length === 0) { throw new Error('Expected at least one account'); } - return accounts[0] as QrAccount; + return accounts[0] as KeyringAccount; +} + +/** + * Get the first HD mode account from an array, throwing if empty. + * + * @param accounts - The accounts array. + * @returns The first account cast as QrHdAccount. + */ +function getFirstHdAccount(accounts: KeyringAccount[]): QrHdAccount { + if (accounts.length === 0) { + throw new Error('Expected at least one account'); + } + return accounts[0] as QrHdAccount; } /** @@ -45,11 +58,14 @@ function getFirstAccount(accounts: QrAccount[]): QrAccount { * @param index - The index to retrieve. * @returns The account at the index. */ -function getAccountAt(accounts: QrAccount[], index: number): QrAccount { +function getAccountAt( + accounts: KeyringAccount[], + index: number, +): KeyringAccount { if (accounts.length <= index) { throw new Error(`Expected account at index ${index}`); } - return accounts[index] as QrAccount; + return accounts[index] as KeyringAccount; } const entropySource = HDKEY_SERIALIZED_KEYRING_WITH_NO_ACCOUNTS.xfp; @@ -225,7 +241,7 @@ describe('QrKeyringV2', () => { const accounts = await wrapper.getAccounts(); accounts.forEach((account, index) => { - expect(account.options.entropy.groupIndex).toBe(index); + expect((account as QrHdAccount).options.entropy.groupIndex).toBe(index); }); }); @@ -349,7 +365,7 @@ describe('QrKeyringV2', () => { const { wrapper } = createEmptyWrapper(); const newAccounts = await wrapper.createAccounts(deriveIndexOptions(5)); - const account = getFirstAccount(newAccounts); + const account = getFirstHdAccount(newAccounts); expect(account.address).toBe(EXPECTED_ACCOUNTS[5]); expect(account.options.entropy.groupIndex).toBe(5); @@ -452,7 +468,7 @@ describe('QrKeyringV2', () => { describe('Account Mode (CryptoAccount)', () => { describe('getAccounts', () => { - it('returns accounts with Mnemonic entropy type (BIP-44 derived)', async () => { + it('returns accounts with PrivateKey entropy type (pre-defined addresses)', async () => { const { wrapper } = await createAccountModeWrapper(); const accounts = await wrapper.getAccounts(); @@ -461,12 +477,25 @@ describe('QrKeyringV2', () => { expect(account.address).toBe( ACCOUNT_SERIALIZED_KEYRING_WITH_ACCOUNTS.accounts[0], ); - expect(account.options.entropy).toMatchObject({ - type: KeyringAccountEntropyTypeOption.Mnemonic, - id: ACCOUNT_SERIALIZED_KEYRING_WITH_ACCOUNTS.xfp, - groupIndex: 0, + expect(account.options.entropy).toStrictEqual({ + type: KeyringAccountEntropyTypeOption.PrivateKey, }); - expect(account.options.entropy.derivationPath).toBeDefined(); + }); + }); + + describe('createAccounts', () => { + it('throws error when trying to derive by index', async () => { + const { wrapper } = await createAccountModeWrapper(); + + await expect( + wrapper.createAccounts({ + type: 'bip44:derive-index', + entropySource: ACCOUNT_SERIALIZED_KEYRING_WITH_ACCOUNTS.xfp, + groupIndex: 0, + }), + ).rejects.toThrow( + 'Cannot create accounts by index for Account mode devices', + ); }); }); diff --git a/packages/keyring-eth-qr/src/qr-keyring-v2.ts b/packages/keyring-eth-qr/src/qr-keyring-v2.ts index ee3001f8d..bd1d73e0b 100644 --- a/packages/keyring-eth-qr/src/qr-keyring-v2.ts +++ b/packages/keyring-eth-qr/src/qr-keyring-v2.ts @@ -43,8 +43,11 @@ const qrKeyringV2Capabilities: KeyringCapabilities = { * This wrapper exposes the accounts and signing capabilities of the legacy * QR keyring via the unified V2 interface. * - * All QR keyring accounts are BIP-44 derived (both HD and Account modes use - * derivation paths from the hardware device). + * Account handling differs by device mode: + * - **HD mode**: Accounts are BIP-44 derived with reliable `groupIndex` values. + * - **Account mode**: Accounts are treated as private key imports since the + * device provides pre-defined addresses with arbitrary paths, making + * `groupIndex` unreliable for derivation. */ export type QrKeyringV2Options = { legacyKeyring: QrKeyring; @@ -52,7 +55,7 @@ export type QrKeyringV2Options = { }; export class QrKeyringV2 - extends EthKeyringWrapper> + extends EthKeyringWrapper implements KeyringV2 { readonly entropySource: EntropySourceId; @@ -87,13 +90,13 @@ export class QrKeyringV2 } /** - * Creates a Bip44Account object for the given address. + * Creates a Bip44Account for HD mode devices. * * @param address - The account address. * @param addressIndex - The account index in the derivation path. * @returns The created Bip44Account. */ - #createKeyringAccount( + #createHdModeAccount( address: Hex, addressIndex: number, ): Bip44Account { @@ -127,7 +130,36 @@ export class QrKeyringV2 return account; } - async getAccounts(): Promise[]> { + /** + * Creates a KeyringAccount for Account mode devices. + * + * Account mode devices provide pre-defined addresses with arbitrary derivation + * paths, so we treat them as private key imports rather than BIP-44 accounts. + * + * @param address - The account address. + * @returns The created KeyringAccount. + */ + #createAccountModeAccount(address: Hex): KeyringAccount { + const id = this.registry.register(address); + + const account: KeyringAccount = { + id, + type: EthAccountType.Eoa, + address, + scopes: [...this.capabilities.scopes], + methods: [...QR_KEYRING_METHODS], + options: { + entropy: { + type: KeyringAccountEntropyTypeOption.PrivateKey, + }, + }, + }; + + this.registry.set(account); + return account; + } + + async getAccounts(): Promise { const addresses = await this.inner.getAccounts(); const deviceState = await this.#getDeviceState(); @@ -138,7 +170,7 @@ export class QrKeyringV2 const { mode, indexes } = deviceState; - return addresses.map((address, arrayIndex) => { + return addresses.map((address) => { // Check if we already have this account in the registry const existingId = this.registry.getAccountId(address); if (existingId) { @@ -148,28 +180,25 @@ export class QrKeyringV2 } } - let addressIndex: number; if (mode === DeviceMode.HD) { // HD mode: index must be in the map - const index = indexes[address]; - if (index === undefined) { + const addressIndex = indexes[address]; + if (addressIndex === undefined) { throw new Error( `Address ${address} not found in device indexes. This indicates an inconsistent keyring state.`, ); } - addressIndex = index; - } else { - // Account mode: use array position (indexes map is not populated) - addressIndex = arrayIndex; + return this.#createHdModeAccount(address, addressIndex); } - return this.#createKeyringAccount(address, addressIndex); + // Account mode: treat as private key import + return this.#createAccountModeAccount(address); }); } async createAccounts( options: CreateAccountOptions, - ): Promise[]> { + ): Promise { return this.withLock(async () => { const deviceState = await this.#getDeviceState(); @@ -184,6 +213,16 @@ export class QrKeyringV2 ); } + // Account mode devices don't support index-based derivation since they + // provide pre-defined addresses with arbitrary paths. The groupIndex + // would not reliably match the actual derivation index. + if (deviceState.mode !== DeviceMode.HD) { + throw new Error( + 'Cannot create accounts by index for Account mode devices. ' + + 'Account mode devices provide pre-defined addresses that cannot be derived by index.', + ); + } + // Validate that the entropy source matches this keyring's entropy source if (options.entropySource !== this.entropySource) { throw new Error( @@ -193,10 +232,14 @@ export class QrKeyringV2 const targetIndex = options.groupIndex; - // Check if an account at this index already exists + // Check if an account at this index already exists (only consider HD/BIP-44 accounts) const currentAccounts = await this.getAccounts(); const existingAccount = currentAccounts.find( - (account) => account.options.entropy.groupIndex === targetIndex, + (account) => + account.options.entropy?.type === + KeyringAccountEntropyTypeOption.Mnemonic && + (account.options.entropy as { groupIndex: number }).groupIndex === + targetIndex, ); if (existingAccount) { return [existingAccount]; @@ -210,7 +253,7 @@ export class QrKeyringV2 throw new Error('Failed to create new account'); } - const newAccount = this.#createKeyringAccount(newAddress, targetIndex); + const newAccount = this.#createHdModeAccount(newAddress, targetIndex); return [newAccount]; });