From 4ee267f3d40e827dc5bd9ee2e3679142944025e7 Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Thu, 11 Dec 2025 14:49:14 +0100 Subject: [PATCH 01/11] feat: add TrezorKeyringV2 (wip) --- README.md | 2 + packages/keyring-eth-trezor/package.json | 4 +- packages/keyring-eth-trezor/src/index.ts | 1 + .../src/trezor-keyring-v2.test.ts | 477 ++++++++++++++++++ .../src/trezor-keyring-v2.ts | 183 +++++++ .../keyring-eth-trezor/src/trezor-keyring.ts | 34 +- .../keyring-eth-trezor/tsconfig.build.json | 8 +- packages/keyring-eth-trezor/tsconfig.json | 6 +- yarn.lock | 2 + 9 files changed, 709 insertions(+), 8 deletions(-) create mode 100644 packages/keyring-eth-trezor/src/trezor-keyring-v2.test.ts create mode 100644 packages/keyring-eth-trezor/src/trezor-keyring-v2.ts diff --git a/README.md b/README.md index 0e4aba4e0..4ed622e85 100644 --- a/README.md +++ b/README.md @@ -59,7 +59,9 @@ linkStyle default opacity:0.5 eth_qr_keyring --> keyring_utils; eth_simple_keyring --> keyring_api; eth_simple_keyring --> keyring_utils; + eth_trezor_keyring --> keyring_api; eth_trezor_keyring --> keyring_utils; + eth_trezor_keyring --> account_api; keyring_internal_api --> keyring_api; keyring_internal_api --> keyring_utils; keyring_internal_snap_client --> keyring_api; diff --git a/packages/keyring-eth-trezor/package.json b/packages/keyring-eth-trezor/package.json index 91f1f0cee..60544d846 100644 --- a/packages/keyring-eth-trezor/package.json +++ b/packages/keyring-eth-trezor/package.json @@ -49,6 +49,8 @@ "@ethereumjs/tx": "^5.4.0", "@ethereumjs/util": "^9.1.0", "@metamask/eth-sig-util": "^8.2.0", + "@metamask/keyring-api": "workspace:^", + "@metamask/keyring-utils": "workspace:^", "@metamask/utils": "^11.1.0", "@trezor/connect-plugin-ethereum": "^9.0.5", "@trezor/connect-web": "^9.6.0", @@ -59,8 +61,8 @@ "@ethereumjs/common": "^4.4.0", "@lavamoat/allow-scripts": "^3.2.1", "@lavamoat/preinstall-always-fail": "^2.1.0", + "@metamask/account-api": "workspace:^", "@metamask/auto-changelog": "^3.4.4", - "@metamask/keyring-utils": "workspace:^", "@ts-bridge/cli": "^0.6.3", "@types/ethereumjs-tx": "^1.0.1", "@types/hdkey": "^2.0.1", diff --git a/packages/keyring-eth-trezor/src/index.ts b/packages/keyring-eth-trezor/src/index.ts index 6cb20b9e7..d7d6cc759 100644 --- a/packages/keyring-eth-trezor/src/index.ts +++ b/packages/keyring-eth-trezor/src/index.ts @@ -1,4 +1,5 @@ export * from './trezor-keyring'; +export * from './trezor-keyring-v2'; export * from './onekey-keyring'; export type * from './trezor-bridge'; export * from './trezor-connect-bridge'; diff --git a/packages/keyring-eth-trezor/src/trezor-keyring-v2.test.ts b/packages/keyring-eth-trezor/src/trezor-keyring-v2.test.ts new file mode 100644 index 000000000..bc7f449d6 --- /dev/null +++ b/packages/keyring-eth-trezor/src/trezor-keyring-v2.test.ts @@ -0,0 +1,477 @@ +import type { Bip44Account } from '@metamask/account-api'; +import { + EthAccountType, + EthMethod, + EthScope, + type KeyringAccount, + KeyringAccountEntropyTypeOption, + KeyringType, +} from '@metamask/keyring-api'; +import HDKey from 'hdkey'; + +import type { TrezorBridge } from './trezor-bridge'; +import { TrezorKeyring } from './trezor-keyring'; +import { TrezorKeyringV2 } from './trezor-keyring-v2'; + +/** + * Type alias for Trezor keyring accounts (always BIP-44 derived). + */ +type TrezorAccount = Bip44Account; + +/** + * Test addresses derived from the fake HD key. + */ +const EXPECTED_ACCOUNTS = [ + '0xF30952A1c534CDE7bC471380065726fa8686dfB3', + '0x44fe3Cf56CaF651C4bD34Ae6dbcffa34e9e3b84B', + '0x8Ee3374Fa705C1F939715871faf91d4348D5b906', + '0xEF69e24dE9CdEe93C4736FE29791E45d5D4CFd6A', + '0xC668a5116A045e9162902795021907Cb15aa2620', + '0xbF519F7a6D8E72266825D770C60dbac55a3baeb9', +] as const; + +const fakeXPubKey = + 'xpub6FnCn6nSzZAw5Tw7cgR9bi15UV96gLZhjDstkXXxvCLsUXBGXPdSnLFbdpq8p9HmGsApME5hQTZ3emM2rnY5agb9rXpVGyy3bdW6EEgAtqt'; +const fakeHdKey = HDKey.fromExtendedKey(fakeXPubKey); + +/** + * Fake entropy source representing the device fingerprint. + */ +const entropySource = 'trezor-device-12345'; + +/** + * Expected methods supported by Trezor keyring accounts. + */ +const EXPECTED_METHODS = [ + EthMethod.SignTransaction, + EthMethod.PersonalSign, + EthMethod.SignTypedDataV3, + EthMethod.SignTypedDataV4, +]; + +/** + * Get the first account from an array, throwing if empty. + * + * @param accounts - The accounts array. + * @returns The first account. + */ +function getFirstAccount(accounts: TrezorAccount[]): TrezorAccount { + if (accounts.length === 0) { + throw new Error('Expected at least one account'); + } + return accounts[0] as TrezorAccount; +} + +/** + * 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: TrezorAccount[], index: number): TrezorAccount { + if (accounts.length <= index) { + throw new Error(`Expected account at index ${index}`); + } + return accounts[index] as TrezorAccount; +} + +/** + * Get a mock bridge for the TrezorKeyring. + * + * @returns A mock bridge. + */ +function getMockBridge(): TrezorBridge { + return { + init: jest.fn(), + dispose: jest.fn(), + getPublicKey: jest.fn(), + ethereumSignTransaction: jest.fn(), + ethereumSignMessage: jest.fn(), + ethereumSignTypedData: jest.fn(), + model: undefined, + } as unknown as TrezorBridge; +} + +/** + * Create a TrezorKeyring with the fake HD key. + * + * @returns The inner keyring. + */ +function createInnerKeyring(): TrezorKeyring { + const inner = new TrezorKeyring({ bridge: getMockBridge() }); + // Set up the HDKey so accounts can be derived without unlocking + inner.hdk = fakeHdKey; + return inner; +} + +/** + * Create a TrezorKeyringV2 wrapper with accounts. + * + * @param accountCount - Number of accounts to add. + * @returns The wrapper and inner keyring. + */ +async function createWrapperWithAccounts(accountCount = 3): Promise<{ + wrapper: TrezorKeyringV2; + inner: TrezorKeyring; +}> { + const inner = createInnerKeyring(); + inner.setAccountToUnlock(0); + await inner.addAccounts(accountCount); + + const wrapper = new TrezorKeyringV2({ legacyKeyring: inner, entropySource }); + return { wrapper, inner }; +} + +/** + * Create a TrezorKeyringV2 wrapper without any accounts. + * + * @returns The wrapper and inner keyring. + */ +function createEmptyWrapper(): { + wrapper: TrezorKeyringV2; + inner: TrezorKeyring; +} { + const inner = createInnerKeyring(); + const wrapper = new TrezorKeyringV2({ 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('TrezorKeyringV2', () => { + describe('constructor', () => { + it('creates a wrapper with correct type and capabilities', () => { + const { wrapper } = createEmptyWrapper(); + + expect(wrapper.type).toBe(KeyringType.Trezor); + expect(wrapper.capabilities).toStrictEqual({ + scopes: [EthScope.Eoa], + bip44: { + deriveIndex: true, + derivePath: false, + discover: false, + }, + }); + }); + }); + + describe('getAccounts', () => { + it('returns empty array when no accounts exist', async () => { + const { wrapper } = createEmptyWrapper(); + + 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.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(account).toMatchObject({ + type: EthAccountType.Eoa, + address: EXPECTED_ACCOUNTS[0], + scopes: [EthScope.Eoa], + methods: EXPECTED_METHODS, + options: { + entropy: { + type: KeyringAccountEntropyTypeOption.Mnemonic, + id: entropySource, + 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(); + + accounts.forEach((account, index) => { + expect(account.options.entropy.groupIndex).toBe(index); + }); + }); + + it('uses paths map when available for faster lookup', async () => { + const { wrapper, inner } = await createWrapperWithAccounts(1); + + // Manually populate the paths map (simulating what getFirstPage does) + inner.paths[EXPECTED_ACCOUNTS[0]] = 0; + + const accounts = await wrapper.getAccounts(); + const account = getFirstAccount(accounts); + + expect(account.options.entropy.groupIndex).toBe(0); + }); + + it('throws error when address cannot be found', async () => { + const { wrapper, inner } = await createWrapperWithAccounts(1); + + // Mock the private derive method to speed up the test + // by preventing the 1000 iteration loop + jest.spyOn(inner.hdk, 'derive').mockImplementation(() => { + // Return an HDKey that produces a different address + const differentHdKey = HDKey.fromExtendedKey( + 'xpub661MyMwAqRbcFtXgS5sYJABqqG9YLmC4Q1Rdap9gSE8NqtwybGhePY2gZ29ESFjqJoCu1Rupje8YtGqsefD265TMg7usUDFdp6W1EGMcet8', + ); + return differentHdKey; + }); + + // Clear paths to force the fallback derivation loop + inner.paths = {}; + + await expect(wrapper.getAccounts()).rejects.toThrow(/Unknown address/u); + }); + }); + + describe('deserialize', () => { + it('deserializes the legacy keyring state', async () => { + const inner = createInnerKeyring(); + const wrapper = new TrezorKeyringV2({ + legacyKeyring: inner, + entropySource, + }); + + // Deserialize with accounts pre-set + await wrapper.deserialize({ + hdPath: `m/44'/60'/0'/0`, + accounts: [ + EXPECTED_ACCOUNTS[0], + EXPECTED_ACCOUNTS[1], + EXPECTED_ACCOUNTS[2], + ], + page: 0, + perPage: 5, + }); + + // The inner keyring now has accounts, but the paths may not be populated + // until we call getFirstPage or getNextPage to populate the paths map + const innerAccounts = await inner.getAccounts(); + expect(innerAccounts).toHaveLength(3); + }); + }); + + describe('createAccounts', () => { + it('creates the first account at index 0', async () => { + const { wrapper } = createEmptyWrapper(); + + const newAccounts = await wrapper.createAccounts(deriveIndexOptions(0)); + const account = getFirstAccount(newAccounts); + + 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(deriveIndexOptions(2)); + const account = getFirstAccount(newAccounts); + + 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 existingAccount = getFirstAccount(existingAccounts); + const newAccounts = await wrapper.createAccounts(deriveIndexOptions(0)); + const returnedAccount = getFirstAccount(newAccounts); + + expect(returnedAccount).toBe(existingAccount); + }); + + 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 TrezorKeyring: private-key:import', + ); + }); + + it('throws error for entropy source mismatch', async () => { + const { wrapper } = await createWrapperWithAccounts(0); + + await expect( + wrapper.createAccounts(deriveIndexOptions(0, 'wrong-entropy')), + ).rejects.toThrow(/Entropy source mismatch/u); + }); + + it('allows deriving accounts at any index (non-sequential)', async () => { + const { wrapper } = createEmptyWrapper(); + + const newAccounts = await wrapper.createAccounts(deriveIndexOptions(5)); + const account = getFirstAccount(newAccounts); + + expect(account.address).toBe(EXPECTED_ACCOUNTS[5]); + expect(account.options.entropy.groupIndex).toBe(5); + }); + + it('creates multiple accounts sequentially', async () => { + const { wrapper } = createEmptyWrapper(); + + const results = await Promise.all([ + wrapper.createAccounts(deriveIndexOptions(0)), + wrapper.createAccounts(deriveIndexOptions(1)), + wrapper.createAccounts(deriveIndexOptions(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 { wrapper, inner } = createEmptyWrapper(); + + jest.spyOn(inner, 'addAccounts').mockResolvedValueOnce([]); + + await expect( + wrapper.createAccounts(deriveIndexOptions(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(); + const accountToDelete = getAccountAt(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 = getFirstAccount(accounts); + + await wrapper.deleteAccount(accountToDelete.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 expectedAccount = getFirstAccount(accounts); + const account = await wrapper.getAccount(expectedAccount.id); + + expect(account).toBe(expectedAccount); + }); + + 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('different HD paths', () => { + it('uses the correct derivation path from the inner keyring', async () => { + const inner = createInnerKeyring(); + inner.setHdPath(`m/44'/60'/0'`); // Legacy MEW path + inner.hdk = fakeHdKey; // Reset after setHdPath clears it + inner.setAccountToUnlock(0); + await inner.addAccounts(1); + + const wrapper = new TrezorKeyringV2({ + legacyKeyring: inner, + entropySource, + }); + + const accounts = await wrapper.getAccounts(); + const account = getFirstAccount(accounts); + + expect(account.options.entropy.derivationPath).toBe(`m/44'/60'/0'/0`); + }); + }); +}); diff --git a/packages/keyring-eth-trezor/src/trezor-keyring-v2.ts b/packages/keyring-eth-trezor/src/trezor-keyring-v2.ts new file mode 100644 index 000000000..d74e73572 --- /dev/null +++ b/packages/keyring-eth-trezor/src/trezor-keyring-v2.ts @@ -0,0 +1,183 @@ +import type { Bip44Account } from '@metamask/account-api'; +import { + type CreateAccountOptions, + EthAccountType, + EthKeyringWrapper, + EthMethod, + EthScope, + type KeyringAccount, + KeyringAccountEntropyTypeOption, + type KeyringCapabilities, + type KeyringV2, + KeyringType, + type EntropySourceId, +} from '@metamask/keyring-api'; +import type { AccountId } from '@metamask/keyring-utils'; +import type { Hex } from '@metamask/utils'; + +import type { TrezorKeyring } from './trezor-keyring'; + +/** + * Methods supported by Trezor keyring EOA accounts. + * Trezor keyrings support a subset of signing methods (no encryption, app keys, or EIP-7702). + */ +const TREZOR_KEYRING_METHODS = [ + EthMethod.SignTransaction, + EthMethod.PersonalSign, + EthMethod.SignTypedDataV3, + EthMethod.SignTypedDataV4, +]; + +const trezorKeyringV2Capabilities: KeyringCapabilities = { + scopes: [EthScope.Eoa], + bip44: { + deriveIndex: true, + derivePath: false, + discover: false, + }, +}; + +/** + * Concrete {@link KeyringV2} adapter for {@link TrezorKeyring}. + * + * This wrapper exposes the accounts and signing capabilities of the legacy + * Trezor keyring via the unified V2 interface. + * + * All Trezor keyring accounts are BIP-44 derived from the device. + */ +export type TrezorKeyringV2Options = { + legacyKeyring: TrezorKeyring; + entropySource: EntropySourceId; +}; + +export class TrezorKeyringV2 + extends EthKeyringWrapper> + implements KeyringV2 +{ + readonly entropySource: EntropySourceId; + + constructor(options: TrezorKeyringV2Options) { + super({ + type: KeyringType.Trezor, + inner: options.legacyKeyring, + capabilities: trezorKeyringV2Capabilities, + }); + this.entropySource = options.entropySource; + } + + /** + * Creates a Bip44Account object for the given address. + * + * @param address - The account address. + * @param addressIndex - The account index in the derivation path. + * @returns The created Bip44Account. + */ + #createKeyringAccount( + address: Hex, + addressIndex: number, + ): Bip44Account { + const id = this.registry.register(address); + + const account: Bip44Account = { + id, + type: EthAccountType.Eoa, + address, + scopes: [...this.capabilities.scopes], + methods: [...TREZOR_KEYRING_METHODS], + options: { + entropy: { + type: KeyringAccountEntropyTypeOption.Mnemonic, + id: this.entropySource, + groupIndex: addressIndex, + derivationPath: `${this.inner.hdPath}/${addressIndex}`, + }, + }, + }; + + this.registry.set(account); + return account; + } + + async getAccounts(): Promise[]> { + const addresses = await this.inner.getAccounts(); + + if (addresses.length === 0) { + return []; + } + + 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; + } + } + + const addressIndex = this.inner.getIndexForAddress(address); + return this.#createKeyringAccount(address, addressIndex); + }); + } + + async createAccounts( + options: CreateAccountOptions, + ): Promise[]> { + return this.withLock(async () => { + // Only supports BIP-44 derive index + if (options.type !== 'bip44:derive-index') { + throw new Error( + `Unsupported account creation type for TrezorKeyring: ${options.type}`, + ); + } + + // Validate that the entropy source matches this keyring's entropy source + if (options.entropySource !== this.entropySource) { + throw new Error( + `Entropy source mismatch: expected '${this.entropySource}', got '${options.entropySource}'`, + ); + } + + const targetIndex = options.groupIndex; + + // Check if an account at this index already exists + const currentAccounts = await this.getAccounts(); + const existingAccount = currentAccounts.find( + (account) => account.options.entropy.groupIndex === targetIndex, + ); + if (existingAccount) { + return [existingAccount]; + } + + // Derive the account at the specified index + this.inner.setAccountToUnlock(targetIndex); + const [newAddress] = await this.inner.addAccounts(1); + + if (!newAddress) { + throw new Error('Failed to create new account'); + } + + const newAccount = this.#createKeyringAccount(newAddress, targetIndex); + + 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-trezor/src/trezor-keyring.ts b/packages/keyring-eth-trezor/src/trezor-keyring.ts index 52345e27c..b85f24ad0 100644 --- a/packages/keyring-eth-trezor/src/trezor-keyring.ts +++ b/packages/keyring-eth-trezor/src/trezor-keyring.ts @@ -203,7 +203,7 @@ export class TrezorKeyring implements Keyring { const newAccounts: Hex[] = []; for (let i = from; i < to; i++) { - const address = this.#addressFromIndex(pathBase, i); + const address = this.addressFromIndex(pathBase, i); if (!this.accounts.includes(address)) { this.accounts = [...this.accounts, address]; newAccounts.push(address); @@ -247,7 +247,7 @@ export class TrezorKeyring implements Keyring { const accounts = []; for (let i = from; i < to; i++) { - const address = this.#addressFromIndex(pathBase, i); + const address = this.addressFromIndex(pathBase, i); accounts.push({ address, balance: null, @@ -558,18 +558,36 @@ export class TrezorKeyring implements Keyring { return bytesToHex(buf); } - #addressFromIndex(basePath: string, i: number): Hex { + /** + * Derive an address at a specific index using the HDKey. + * + * @param basePath - The base derivation path (e.g., 'm'). + * @param i - The derivation index. + * @returns The checksummed address. + */ + protected addressFromIndex(basePath: string, i: number): Hex { const dkey = this.hdk.derive(`${basePath}/${i}`); const address = bytesToHex(publicToAddress(dkey.publicKey, true)); return toChecksumAddress(address); } - #pathFromAddress(address: Hex): string { + /** + * Get the account index for a given address. + * + * This method first checks the `paths` map, and if not found, derives + * addresses up to MAX_INDEX to find the matching index. + * + * @param address - The account address. + * @returns The account index. + * @throws If the address is not found. + */ + getIndexForAddress(address: Hex): number { const checksummedAddress = getChecksumAddress(address); let index = this.paths[checksummedAddress]; + if (typeof index === 'undefined') { for (let i = 0; i < MAX_INDEX; i++) { - if (checksummedAddress === this.#addressFromIndex(pathBase, i)) { + if (checksummedAddress === this.addressFromIndex(pathBase, i)) { index = i; break; } @@ -579,6 +597,12 @@ export class TrezorKeyring implements Keyring { if (typeof index === 'undefined') { throw new Error('Unknown address'); } + + return index; + } + + #pathFromAddress(address: Hex): string { + const index = this.getIndexForAddress(address); return `${this.hdPath}/${index}`; } } diff --git a/packages/keyring-eth-trezor/tsconfig.build.json b/packages/keyring-eth-trezor/tsconfig.build.json index 926b19e74..1d0fc0990 100644 --- a/packages/keyring-eth-trezor/tsconfig.build.json +++ b/packages/keyring-eth-trezor/tsconfig.build.json @@ -10,7 +10,13 @@ "lib": ["ES2020"], "target": "es2017" }, - "references": [{ "path": "../keyring-utils/tsconfig.build.json" }], + "references": [ + { "path": "../keyring-utils/tsconfig.build.json" }, + { "path": "../keyring-api/tsconfig.build.json" }, + { + "path": "../account-api/tsconfig.build.json" + } + ], "include": ["./src/**/*.ts"], "exclude": ["./src/**/*.test.ts"] } diff --git a/packages/keyring-eth-trezor/tsconfig.json b/packages/keyring-eth-trezor/tsconfig.json index 16c034161..a9d6ac716 100644 --- a/packages/keyring-eth-trezor/tsconfig.json +++ b/packages/keyring-eth-trezor/tsconfig.json @@ -8,7 +8,11 @@ "lib": ["ES2020"], "target": "es2017" }, - "references": [{ "path": "../keyring-utils" }], + "references": [ + { "path": "../keyring-utils" }, + { "path": "../keyring-api" }, + { "path": "../account-api" } + ], "include": ["./src"], "exclude": ["./dist/**/*"] } diff --git a/yarn.lock b/yarn.lock index 8389165db..52bf7130e 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1883,8 +1883,10 @@ __metadata: "@ethereumjs/util": "npm:^9.1.0" "@lavamoat/allow-scripts": "npm:^3.2.1" "@lavamoat/preinstall-always-fail": "npm:^2.1.0" + "@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" "@trezor/connect-plugin-ethereum": "npm:^9.0.5" From e1f95bdad01fd56e3d69532ddf3c991b8344bab1 Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Thu, 11 Dec 2025 14:54:47 +0100 Subject: [PATCH 02/11] feat: add onekeyv2 wrapper --- packages/keyring-eth-trezor/jest.config.js | 8 +-- packages/keyring-eth-trezor/src/index.ts | 1 + .../src/onekey-keyring-v2.test.ts | 63 +++++++++++++++++++ .../src/onekey-keyring-v2.ts | 28 +++++++++ .../src/trezor-keyring-v2.ts | 3 +- 5 files changed, 98 insertions(+), 5 deletions(-) create mode 100644 packages/keyring-eth-trezor/src/onekey-keyring-v2.test.ts create mode 100644 packages/keyring-eth-trezor/src/onekey-keyring-v2.ts diff --git a/packages/keyring-eth-trezor/jest.config.js b/packages/keyring-eth-trezor/jest.config.js index 0bb9dd487..cd5d31172 100644 --- a/packages/keyring-eth-trezor/jest.config.js +++ b/packages/keyring-eth-trezor/jest.config.js @@ -23,10 +23,10 @@ module.exports = merge(baseConfig, { // An object that configures minimum threshold enforcement for coverage results coverageThreshold: { global: { - branches: 52.38, - functions: 91.22, - lines: 90.15, - statements: 90.35, + branches: 55.63, + functions: 92.64, + lines: 92.62, + statements: 92.74, }, }, }); diff --git a/packages/keyring-eth-trezor/src/index.ts b/packages/keyring-eth-trezor/src/index.ts index d7d6cc759..2f22f6bb8 100644 --- a/packages/keyring-eth-trezor/src/index.ts +++ b/packages/keyring-eth-trezor/src/index.ts @@ -1,5 +1,6 @@ export * from './trezor-keyring'; export * from './trezor-keyring-v2'; export * from './onekey-keyring'; +export * from './onekey-keyring-v2'; export type * from './trezor-bridge'; export * from './trezor-connect-bridge'; diff --git a/packages/keyring-eth-trezor/src/onekey-keyring-v2.test.ts b/packages/keyring-eth-trezor/src/onekey-keyring-v2.test.ts new file mode 100644 index 000000000..25e20b960 --- /dev/null +++ b/packages/keyring-eth-trezor/src/onekey-keyring-v2.test.ts @@ -0,0 +1,63 @@ +import { KeyringType } from '@metamask/keyring-api'; + +import { OneKeyKeyring } from './onekey-keyring'; +import { OneKeyKeyringV2 } from './onekey-keyring-v2'; +import type { TrezorBridge } from './trezor-bridge'; +import { TrezorKeyringV2 } from './trezor-keyring-v2'; + +const entropySource = 'onekey-device-id-123'; + +/** + * Get a mock bridge for the OneKeyKeyring. + * + * @returns A mock bridge. + */ +function getMockBridge(): TrezorBridge { + return { + init: jest.fn(), + dispose: jest.fn(), + getPublicKey: jest.fn(), + ethereumSignTransaction: jest.fn(), + ethereumSignMessage: jest.fn(), + ethereumSignTypedData: jest.fn(), + model: undefined, + } as unknown as TrezorBridge; +} + +describe('OneKeyKeyringV2', () => { + const createInnerKeyring = (): OneKeyKeyring => { + return new OneKeyKeyring({ bridge: getMockBridge() }); + }; + + describe('constructor', () => { + it('extends TrezorKeyringV2', () => { + const inner = createInnerKeyring(); + const wrapper = new OneKeyKeyringV2({ + legacyKeyring: inner, + entropySource, + }); + + expect(wrapper).toBeInstanceOf(TrezorKeyringV2); + }); + + it('sets the type to OneKey', () => { + const inner = createInnerKeyring(); + const wrapper = new OneKeyKeyringV2({ + legacyKeyring: inner, + entropySource, + }); + + expect(wrapper.type).toBe(KeyringType.OneKey); + }); + + it('stores the entropy source', () => { + const inner = createInnerKeyring(); + const wrapper = new OneKeyKeyringV2({ + legacyKeyring: inner, + entropySource, + }); + + expect(wrapper.entropySource).toBe(entropySource); + }); + }); +}); diff --git a/packages/keyring-eth-trezor/src/onekey-keyring-v2.ts b/packages/keyring-eth-trezor/src/onekey-keyring-v2.ts new file mode 100644 index 000000000..528c2d6a5 --- /dev/null +++ b/packages/keyring-eth-trezor/src/onekey-keyring-v2.ts @@ -0,0 +1,28 @@ +import { KeyringType, type EntropySourceId } from '@metamask/keyring-api'; + +import type { OneKeyKeyring } from './onekey-keyring'; +import { TrezorKeyringV2 } from './trezor-keyring-v2'; + +/** + * Options for creating a OneKeyKeyringV2 instance. + */ +export type OneKeyKeyringV2Options = { + legacyKeyring: OneKeyKeyring; + entropySource: EntropySourceId; +}; + +/** + * Concrete {@link KeyringV2} adapter for {@link OneKeyKeyring}. + * + * This wrapper extends {@link TrezorKeyringV2} since OneKeyKeyring extends + * TrezorKeyring. The only difference is the keyring type identifier. + */ +export class OneKeyKeyringV2 extends TrezorKeyringV2 { + constructor(options: OneKeyKeyringV2Options) { + super({ + legacyKeyring: options.legacyKeyring, + entropySource: options.entropySource, + type: KeyringType.OneKey, + }); + } +} diff --git a/packages/keyring-eth-trezor/src/trezor-keyring-v2.ts b/packages/keyring-eth-trezor/src/trezor-keyring-v2.ts index d74e73572..bd35817c2 100644 --- a/packages/keyring-eth-trezor/src/trezor-keyring-v2.ts +++ b/packages/keyring-eth-trezor/src/trezor-keyring-v2.ts @@ -48,6 +48,7 @@ const trezorKeyringV2Capabilities: KeyringCapabilities = { export type TrezorKeyringV2Options = { legacyKeyring: TrezorKeyring; entropySource: EntropySourceId; + type?: KeyringType.Trezor | KeyringType.OneKey; }; export class TrezorKeyringV2 @@ -58,7 +59,7 @@ export class TrezorKeyringV2 constructor(options: TrezorKeyringV2Options) { super({ - type: KeyringType.Trezor, + type: options.type ?? KeyringType.Trezor, inner: options.legacyKeyring, capabilities: trezorKeyringV2Capabilities, }); From 07af7ec3cb22f8f229ee1d329ac156649c7e9d72 Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Mon, 15 Dec 2025 14:52:11 +0100 Subject: [PATCH 03/11] fix: reset coverage thresholds --- packages/keyring-eth-trezor/jest.config.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/keyring-eth-trezor/jest.config.js b/packages/keyring-eth-trezor/jest.config.js index cd5d31172..0bb9dd487 100644 --- a/packages/keyring-eth-trezor/jest.config.js +++ b/packages/keyring-eth-trezor/jest.config.js @@ -23,10 +23,10 @@ module.exports = merge(baseConfig, { // An object that configures minimum threshold enforcement for coverage results coverageThreshold: { global: { - branches: 55.63, - functions: 92.64, - lines: 92.62, - statements: 92.74, + branches: 52.38, + functions: 91.22, + lines: 90.15, + statements: 90.35, }, }, }); From fa7b362e94c85b125b5db836af831b23b681d4b2 Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Mon, 15 Dec 2025 14:58:24 +0100 Subject: [PATCH 04/11] fix: update coverage thresholds --- packages/keyring-eth-trezor/jest.config.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/keyring-eth-trezor/jest.config.js b/packages/keyring-eth-trezor/jest.config.js index 0bb9dd487..c78dfd8bc 100644 --- a/packages/keyring-eth-trezor/jest.config.js +++ b/packages/keyring-eth-trezor/jest.config.js @@ -23,10 +23,10 @@ module.exports = merge(baseConfig, { // An object that configures minimum threshold enforcement for coverage results coverageThreshold: { global: { - branches: 52.38, - functions: 91.22, - lines: 90.15, - statements: 90.35, + branches: 56.93, + functions: 92.64, + lines: 92.59, + statements: 92.71, }, }, }); From 2940a89f56943f14c35530e000f7750e4ac7bf4f Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Mon, 15 Dec 2025 15:04:40 +0100 Subject: [PATCH 05/11] fix: update CHANGELOG --- packages/keyring-eth-trezor/CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/packages/keyring-eth-trezor/CHANGELOG.md b/packages/keyring-eth-trezor/CHANGELOG.md index 813a11b33..6916aa982 100644 --- a/packages/keyring-eth-trezor/CHANGELOG.md +++ b/packages/keyring-eth-trezor/CHANGELOG.md @@ -7,6 +7,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- Add `TrezorKeyringV2` and `OneKeyKeyringV2` classes implementing `KeyringV2` interface ([#412](https://github.com/MetaMask/accounts/pull/412)) + - Wraps legacy `TrezorKeyring` and `OneKeyKeyring` to expose accounts via the unified `KeyringV2` API and the `KeyringAccount` type. + - Extends `EthKeyringWrapper` for common Ethereum logic. + ## [9.0.0] ### Changed From 9d07ffc205c5b444254146237cca92bbab1d3c95 Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Tue, 6 Jan 2026 12:39:58 +0100 Subject: [PATCH 06/11] fix: type constraint --- .../keyring-eth-trezor/src/trezor-keyring-v2.ts | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/packages/keyring-eth-trezor/src/trezor-keyring-v2.ts b/packages/keyring-eth-trezor/src/trezor-keyring-v2.ts index bd35817c2..c07dbe1bf 100644 --- a/packages/keyring-eth-trezor/src/trezor-keyring-v2.ts +++ b/packages/keyring-eth-trezor/src/trezor-keyring-v2.ts @@ -12,7 +12,7 @@ import { KeyringType, type EntropySourceId, } from '@metamask/keyring-api'; -import type { AccountId } from '@metamask/keyring-utils'; +import type { AccountId, EthKeyring } from '@metamask/keyring-utils'; import type { Hex } from '@metamask/utils'; import type { TrezorKeyring } from './trezor-keyring'; @@ -51,8 +51,16 @@ export type TrezorKeyringV2Options = { type?: KeyringType.Trezor | KeyringType.OneKey; }; +// TrezorKeyring.signTransaction returns `TypedTransaction | OldEthJsTransaction` for +// backwards compatibility with old ethereumjs-tx, but EthKeyring expects `TypedTxData`. +// The runtime behavior is correct - we cast the type to satisfy the constraint. +type TrezorKeyringAsEthKeyring = TrezorKeyring & EthKeyring; + export class TrezorKeyringV2 - extends EthKeyringWrapper> + extends EthKeyringWrapper< + TrezorKeyringAsEthKeyring, + Bip44Account + > implements KeyringV2 { readonly entropySource: EntropySourceId; @@ -60,7 +68,7 @@ export class TrezorKeyringV2 constructor(options: TrezorKeyringV2Options) { super({ type: options.type ?? KeyringType.Trezor, - inner: options.legacyKeyring, + inner: options.legacyKeyring as TrezorKeyringAsEthKeyring, capabilities: trezorKeyringV2Capabilities, }); this.entropySource = options.entropySource; From e8e2d737d14c6dd6e8f452a6cafe4f0c4fd6a588 Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Tue, 6 Jan 2026 14:50:25 +0100 Subject: [PATCH 07/11] fix: deserialize issue --- packages/keyring-eth-trezor/jest.config.js | 6 +-- .../src/trezor-keyring-v2.test.ts | 43 ++++++++++++++++--- .../src/trezor-keyring-v2.ts | 31 ++++++++++++- 3 files changed, 71 insertions(+), 9 deletions(-) diff --git a/packages/keyring-eth-trezor/jest.config.js b/packages/keyring-eth-trezor/jest.config.js index c78dfd8bc..2cc8bf11e 100644 --- a/packages/keyring-eth-trezor/jest.config.js +++ b/packages/keyring-eth-trezor/jest.config.js @@ -24,9 +24,9 @@ module.exports = merge(baseConfig, { coverageThreshold: { global: { branches: 56.93, - functions: 92.64, - lines: 92.59, - statements: 92.71, + functions: 92.85, + lines: 92.68, + statements: 92.8, }, }, }); diff --git a/packages/keyring-eth-trezor/src/trezor-keyring-v2.test.ts b/packages/keyring-eth-trezor/src/trezor-keyring-v2.test.ts index bc7f449d6..9c2c0d9ba 100644 --- a/packages/keyring-eth-trezor/src/trezor-keyring-v2.test.ts +++ b/packages/keyring-eth-trezor/src/trezor-keyring-v2.test.ts @@ -271,14 +271,19 @@ describe('TrezorKeyringV2', () => { }); describe('deserialize', () => { - it('deserializes the legacy keyring state', async () => { - const inner = createInnerKeyring(); + it('deserializes the legacy keyring state without crashing when device is locked', async () => { + // Create a keyring WITHOUT pre-initializing hdk (simulating production) + const inner = new TrezorKeyring({ bridge: getMockBridge() }); const wrapper = new TrezorKeyringV2({ legacyKeyring: inner, entropySource, }); - // Deserialize with accounts pre-set + // Verify the device is locked (hdk not initialized) + expect(inner.isUnlocked()).toBe(false); + + // Deserialize with accounts pre-set - this should NOT crash + // even though hdk is not initialized await wrapper.deserialize({ hdPath: `m/44'/60'/0'/0`, accounts: [ @@ -290,10 +295,38 @@ describe('TrezorKeyringV2', () => { perPage: 5, }); - // The inner keyring now has accounts, but the paths may not be populated - // until we call getFirstPage or getNextPage to populate the paths map + // The inner keyring now has accounts restored const innerAccounts = await inner.getAccounts(); expect(innerAccounts).toHaveLength(3); + + // Device is still locked + expect(inner.isUnlocked()).toBe(false); + }); + + it('rebuilds registry when getAccounts is called after device is unlocked', async () => { + const inner = createInnerKeyring(); + const wrapper = new TrezorKeyringV2({ + legacyKeyring: inner, + entropySource, + }); + + // Deserialize with accounts pre-set + await wrapper.deserialize({ + hdPath: `m/44'/60'/0'/0`, + accounts: [ + EXPECTED_ACCOUNTS[0], + EXPECTED_ACCOUNTS[1], + EXPECTED_ACCOUNTS[2], + ], + page: 0, + perPage: 5, + }); + + // Since inner.hdk is pre-initialized in createInnerKeyring(), + // getAccounts should work and populate the registry + const accounts = await wrapper.getAccounts(); + expect(accounts).toHaveLength(3); + expect(accounts[0]?.address).toBe(EXPECTED_ACCOUNTS[0]); }); }); diff --git a/packages/keyring-eth-trezor/src/trezor-keyring-v2.ts b/packages/keyring-eth-trezor/src/trezor-keyring-v2.ts index c07dbe1bf..94bbeb088 100644 --- a/packages/keyring-eth-trezor/src/trezor-keyring-v2.ts +++ b/packages/keyring-eth-trezor/src/trezor-keyring-v2.ts @@ -13,7 +13,7 @@ import { type EntropySourceId, } from '@metamask/keyring-api'; import type { AccountId, EthKeyring } from '@metamask/keyring-utils'; -import type { Hex } from '@metamask/utils'; +import type { Hex, Json } from '@metamask/utils'; import type { TrezorKeyring } from './trezor-keyring'; @@ -74,6 +74,35 @@ export class TrezorKeyringV2 this.entropySource = options.entropySource; } + /** + * Hydrate the underlying keyring from a previously serialized state. + * + * Overrides the base class implementation to avoid calling `getAccounts()` + * when the Trezor device is locked. The base class calls `getAccounts()` to + * rebuild the registry, but for Trezor keyrings this requires the HDKey to + * be initialized (via `unlock()`). Since the device may not be connected + * during deserialization, we skip the registry rebuild here. The registry + * will be populated on the first call to `getAccounts()` after the device + * is unlocked. + * + * @param state - The serialized keyring state. + */ + async deserialize(state: Json): Promise { + await this.withLock(async () => { + // Clear the registry when deserializing + this.registry.clear(); + + // Deserialize the legacy keyring state only. + // We intentionally skip calling getAccounts() here because the Trezor + // device may be locked (HDKey not initialized). The TrezorKeyring's + // deserialize restores the accounts array, but not the paths map, so + // getIndexForAddress would need to derive addresses which requires an + // initialized HDKey. The registry will be populated lazily when + // getAccounts() is called after the device is unlocked. + await this.inner.deserialize(state); + }); + } + /** * Creates a Bip44Account object for the given address. * From ad5ae068b029f308b9eaad15149872725332a645 Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Thu, 8 Jan 2026 14:50:48 +0100 Subject: [PATCH 08/11] feat: align logic with latest LedgerKeyringV2 findings --- packages/keyring-eth-trezor/jest.config.js | 8 +- .../src/trezor-keyring-v2.test.ts | 114 +++++++++++++++++- .../src/trezor-keyring-v2.ts | 110 ++++++++++++++--- 3 files changed, 210 insertions(+), 22 deletions(-) diff --git a/packages/keyring-eth-trezor/jest.config.js b/packages/keyring-eth-trezor/jest.config.js index 2cc8bf11e..d2540eb01 100644 --- a/packages/keyring-eth-trezor/jest.config.js +++ b/packages/keyring-eth-trezor/jest.config.js @@ -23,10 +23,10 @@ module.exports = merge(baseConfig, { // An object that configures minimum threshold enforcement for coverage results coverageThreshold: { global: { - branches: 56.93, - functions: 92.85, - lines: 92.68, - statements: 92.8, + branches: 59.31, + functions: 92.95, + lines: 93.1, + statements: 93.2, }, }, }); diff --git a/packages/keyring-eth-trezor/src/trezor-keyring-v2.test.ts b/packages/keyring-eth-trezor/src/trezor-keyring-v2.test.ts index 9c2c0d9ba..c8e0c9006 100644 --- a/packages/keyring-eth-trezor/src/trezor-keyring-v2.test.ts +++ b/packages/keyring-eth-trezor/src/trezor-keyring-v2.test.ts @@ -96,10 +96,15 @@ function getMockBridge(): TrezorBridge { /** * Create a TrezorKeyring with the fake HD key. * + * @param hdPath - Optional HD path to set (defaults to BIP44 standard). * @returns The inner keyring. */ -function createInnerKeyring(): TrezorKeyring { +function createInnerKeyring(hdPath?: string): TrezorKeyring { const inner = new TrezorKeyring({ bridge: getMockBridge() }); + // Set the HD path before setting HDKey (setHdPath resets HDKey) + if (hdPath) { + inner.setHdPath(hdPath as Parameters[0]); + } // Set up the HDKey so accounts can be derived without unlocking inner.hdk = fakeHdKey; return inner; @@ -159,6 +164,28 @@ function deriveIndexOptions( }; } +/** + * Helper to create account options for bip44:derive-path. + * + * @param derivationPath - The full derivation path. + * @param source - Optional entropy source override. + * @returns The create account options. + */ +function derivePathOptions( + derivationPath: `m/${string}`, + source: string = entropySource, +): { + type: 'bip44:derive-path'; + entropySource: string; + derivationPath: `m/${string}`; +} { + return { + type: 'bip44:derive-path', + entropySource: source, + derivationPath, + }; +} + describe('TrezorKeyringV2', () => { describe('constructor', () => { it('creates a wrapper with correct type and capabilities', () => { @@ -169,7 +196,7 @@ describe('TrezorKeyringV2', () => { scopes: [EthScope.Eoa], bip44: { deriveIndex: true, - derivePath: false, + derivePath: true, discover: false, }, }); @@ -419,6 +446,89 @@ describe('TrezorKeyringV2', () => { }); }); + describe('createAccounts with bip44:derive-path', () => { + it('creates an account with a BIP44 derivation path', async () => { + const { wrapper, inner } = createEmptyWrapper(); + + const newAccounts = await wrapper.createAccounts( + derivePathOptions(`m/44'/60'/0'/0/5`), + ); + const account = getFirstAccount(newAccounts); + + expect(account.address).toBe(EXPECTED_ACCOUNTS[5]); + expect(account.options.entropy.groupIndex).toBe(5); + expect(account.options.entropy.derivationPath).toBe(`m/44'/60'/0'/0/5`); + expect(inner.hdPath).toBe(`m/44'/60'/0'/0`); + }); + + it('creates an account with legacy MEW path', async () => { + // Create wrapper with legacy MEW path pre-set to avoid HDKey reset + const inner = createInnerKeyring(`m/44'/60'/0'`); + const wrapper = new TrezorKeyringV2({ + legacyKeyring: inner, + entropySource, + }); + + const newAccounts = await wrapper.createAccounts( + derivePathOptions(`m/44'/60'/0'/3`), + ); + const account = getFirstAccount(newAccounts); + + expect(account.address).toBe(EXPECTED_ACCOUNTS[3]); + expect(account.options.entropy.groupIndex).toBe(3); + expect(account.options.entropy.derivationPath).toBe(`m/44'/60'/0'/3`); + expect(inner.hdPath).toBe(`m/44'/60'/0'`); + }); + + it('returns existing account if path already exists', async () => { + const { wrapper } = createEmptyWrapper(); + + // Create first account + const firstResult = await wrapper.createAccounts( + derivePathOptions(`m/44'/60'/0'/0/0`), + ); + const firstAccount = getFirstAccount(firstResult); + + // Try to create same path again + const secondResult = await wrapper.createAccounts( + derivePathOptions(`m/44'/60'/0'/0/0`), + ); + const secondAccount = getFirstAccount(secondResult); + + expect(secondAccount).toBe(firstAccount); + }); + + it('throws error for invalid derivation path format', async () => { + const { wrapper } = createEmptyWrapper(); + + await expect( + wrapper.createAccounts( + // @ts-expect-error - intentionally testing invalid path + derivePathOptions('invalid-path'), + ), + ).rejects.toThrow(/Invalid derivation path/u); + }); + + it('throws error for unsupported HD path', async () => { + const { wrapper } = createEmptyWrapper(); + + // Valid format but unsupported base path for Trezor + await expect( + wrapper.createAccounts(derivePathOptions(`m/44'/100'/0'/0/0`)), + ).rejects.toThrow(/Invalid derivation path/u); + }); + + it('throws error for entropy source mismatch with derive-path', async () => { + const { wrapper } = createEmptyWrapper(); + + await expect( + wrapper.createAccounts( + derivePathOptions(`m/44'/60'/0'/0/0`, 'wrong-entropy'), + ), + ).rejects.toThrow(/Entropy source mismatch/u); + }); + }); + describe('deleteAccount', () => { it('removes an account from the keyring', async () => { const { wrapper } = await createWrapperWithAccounts(3); diff --git a/packages/keyring-eth-trezor/src/trezor-keyring-v2.ts b/packages/keyring-eth-trezor/src/trezor-keyring-v2.ts index 94bbeb088..3496560c6 100644 --- a/packages/keyring-eth-trezor/src/trezor-keyring-v2.ts +++ b/packages/keyring-eth-trezor/src/trezor-keyring-v2.ts @@ -32,11 +32,37 @@ const trezorKeyringV2Capabilities: KeyringCapabilities = { scopes: [EthScope.Eoa], bip44: { deriveIndex: true, - derivePath: false, + derivePath: true, discover: false, }, }; +/** + * Allowed HD paths for Trezor keyring. + * These must match the keys in ALLOWED_HD_PATHS from trezor-keyring.ts. + */ +type AllowedHdPath = + | `m/44'/60'/0'/0` + | `m/44'/60'/0'` + | `m/44'/1'/0'/0`; + +/** + * BIP-44 standard HD path prefix constant for Ethereum. + * Used as default for derive-index operations. + */ +const BIP44_HD_PATH_PREFIX: AllowedHdPath = `m/44'/60'/0'/0`; + +/** + * Regex pattern for validating and parsing derivation paths. + * Only matches the allowed Trezor HD paths from the V1 implementation: + * - m/44'/60'/0'/0/{index} (BIP44 standard) + * - m/44'/60'/0'/{index} (legacy MEW) + * - m/44'/1'/0'/0/{index} (SLIP0044 testnet) + * Captures: [1] = base path, [2] = index + */ +const DERIVATION_PATH_PATTERN = + /^(m\/44'\/60'\/0'\/0|m\/44'\/60'\/0'|m\/44'\/1'\/0'\/0)\/(\d+)$/u; + /** * Concrete {@link KeyringV2} adapter for {@link TrezorKeyring}. * @@ -103,6 +129,32 @@ export class TrezorKeyringV2 }); } + /** + * Parses a derivation path to extract the base HD path and account index. + * + * @param derivationPath - The full derivation path (e.g., m/44'/60'/0'/0/5). + * @returns The base HD path and account index. + * @throws If the path format is invalid. + */ + #parseDerivationPath(derivationPath: string): { + basePath: AllowedHdPath; + index: number; + } { + const match = derivationPath.match(DERIVATION_PATH_PATTERN); + if (match) { + return { + basePath: match[1] as AllowedHdPath, + index: parseInt(match[2] as string, 10), + }; + } + + throw new Error( + `Invalid derivation path: ${derivationPath}. ` + + `Expected format: {base}/{index} where base is one of: ` + + `m/44'/60'/0'/0, m/44'/60'/0', m/44'/1'/0'/0.`, + ); + } + /** * Creates a Bip44Account object for the given address. * @@ -115,6 +167,7 @@ export class TrezorKeyringV2 addressIndex: number, ): Bip44Account { const id = this.registry.register(address); + const derivationPath = `${this.inner.hdPath}/${addressIndex}`; const account: Bip44Account = { id, @@ -127,7 +180,7 @@ export class TrezorKeyringV2 type: KeyringAccountEntropyTypeOption.Mnemonic, id: this.entropySource, groupIndex: addressIndex, - derivationPath: `${this.inner.hdPath}/${addressIndex}`, + derivationPath, }, }, }; @@ -162,32 +215,57 @@ export class TrezorKeyringV2 options: CreateAccountOptions, ): Promise[]> { return this.withLock(async () => { - // Only supports BIP-44 derive index - if (options.type !== 'bip44:derive-index') { + if ( + options.type === 'bip44:derive-path' || + options.type === 'bip44:derive-index' + ) { + // Validate that the entropy source matches this keyring's entropy source + if (options.entropySource !== this.entropySource) { + throw new Error( + `Entropy source mismatch: expected '${this.entropySource}', got '${options.entropySource}'`, + ); + } + } else { throw new Error( - `Unsupported account creation type for TrezorKeyring: ${options.type}`, + `Unsupported account creation type for TrezorKeyring: ${String( + options.type, + )}`, ); } - // Validate that the entropy source matches this keyring's entropy source - if (options.entropySource !== this.entropySource) { - throw new Error( - `Entropy source mismatch: expected '${this.entropySource}', got '${options.entropySource}'`, - ); + // Check if an account at this index already exists with the same derivation path + const currentAccounts = await this.getAccounts(); + + let targetIndex: number; + let basePath: AllowedHdPath; + let derivationPath: string; + + if (options.type === 'bip44:derive-path') { + // Parse the derivation path to extract base path and index + const parsed = this.#parseDerivationPath(options.derivationPath); + targetIndex = parsed.index; + basePath = parsed.basePath; + derivationPath = options.derivationPath; + } else { + // derive-index uses BIP-44 standard path by default + targetIndex = options.groupIndex; + basePath = BIP44_HD_PATH_PREFIX; + derivationPath = `${basePath}/${targetIndex}`; } - const targetIndex = options.groupIndex; + const existingAccount = currentAccounts.find((account) => { + return ( + account.options.entropy.groupIndex === targetIndex && + account.options.entropy.derivationPath === derivationPath + ); + }); - // Check if an account at this index already exists - const currentAccounts = await this.getAccounts(); - const existingAccount = currentAccounts.find( - (account) => account.options.entropy.groupIndex === targetIndex, - ); if (existingAccount) { return [existingAccount]; } // Derive the account at the specified index + this.inner.setHdPath(basePath); this.inner.setAccountToUnlock(targetIndex); const [newAddress] = await this.inner.addAccounts(1); From df14a7674592d5d61e4eb657147f99f39e0edd06 Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Thu, 8 Jan 2026 15:00:31 +0100 Subject: [PATCH 09/11] fix: lint issue --- packages/keyring-eth-trezor/src/trezor-keyring-v2.ts | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/packages/keyring-eth-trezor/src/trezor-keyring-v2.ts b/packages/keyring-eth-trezor/src/trezor-keyring-v2.ts index 3496560c6..ec7b7dcd4 100644 --- a/packages/keyring-eth-trezor/src/trezor-keyring-v2.ts +++ b/packages/keyring-eth-trezor/src/trezor-keyring-v2.ts @@ -41,10 +41,7 @@ const trezorKeyringV2Capabilities: KeyringCapabilities = { * Allowed HD paths for Trezor keyring. * These must match the keys in ALLOWED_HD_PATHS from trezor-keyring.ts. */ -type AllowedHdPath = - | `m/44'/60'/0'/0` - | `m/44'/60'/0'` - | `m/44'/1'/0'/0`; +type AllowedHdPath = `m/44'/60'/0'/0` | `m/44'/60'/0'` | `m/44'/1'/0'/0`; /** * BIP-44 standard HD path prefix constant for Ethereum. From 65cd1a0065227e8dc789b1fd8a7abd4742e15ffa Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Thu, 8 Jan 2026 16:39:50 +0100 Subject: [PATCH 10/11] fix: address cursor PR feedback --- packages/keyring-eth-trezor/jest.config.js | 6 +++--- .../src/trezor-keyring-v2.test.ts | 10 ++++++++++ .../keyring-eth-trezor/src/trezor-keyring-v2.ts | 13 ++++++++++++- 3 files changed, 25 insertions(+), 4 deletions(-) diff --git a/packages/keyring-eth-trezor/jest.config.js b/packages/keyring-eth-trezor/jest.config.js index d2540eb01..1b380eea6 100644 --- a/packages/keyring-eth-trezor/jest.config.js +++ b/packages/keyring-eth-trezor/jest.config.js @@ -23,10 +23,10 @@ module.exports = merge(baseConfig, { // An object that configures minimum threshold enforcement for coverage results coverageThreshold: { global: { - branches: 59.31, + branches: 59.58, functions: 92.95, - lines: 93.1, - statements: 93.2, + lines: 93.15, + statements: 93.25, }, }, }); diff --git a/packages/keyring-eth-trezor/src/trezor-keyring-v2.test.ts b/packages/keyring-eth-trezor/src/trezor-keyring-v2.test.ts index c8e0c9006..59d07d234 100644 --- a/packages/keyring-eth-trezor/src/trezor-keyring-v2.test.ts +++ b/packages/keyring-eth-trezor/src/trezor-keyring-v2.test.ts @@ -410,6 +410,16 @@ describe('TrezorKeyringV2', () => { ).rejects.toThrow(/Entropy source mismatch/u); }); + it('throws error for negative groupIndex', async () => { + const { wrapper } = createEmptyWrapper(); + + await expect( + wrapper.createAccounts(deriveIndexOptions(-1)), + ).rejects.toThrow( + 'Invalid groupIndex: -1. Must be a non-negative integer.', + ); + }); + it('allows deriving accounts at any index (non-sequential)', async () => { const { wrapper } = createEmptyWrapper(); diff --git a/packages/keyring-eth-trezor/src/trezor-keyring-v2.ts b/packages/keyring-eth-trezor/src/trezor-keyring-v2.ts index ec7b7dcd4..e60250bae 100644 --- a/packages/keyring-eth-trezor/src/trezor-keyring-v2.ts +++ b/packages/keyring-eth-trezor/src/trezor-keyring-v2.ts @@ -245,6 +245,11 @@ export class TrezorKeyringV2 derivationPath = options.derivationPath; } else { // derive-index uses BIP-44 standard path by default + if (options.groupIndex < 0) { + throw new Error( + `Invalid groupIndex: ${options.groupIndex}. Must be a non-negative integer.`, + ); + } targetIndex = options.groupIndex; basePath = BIP44_HD_PATH_PREFIX; derivationPath = `${basePath}/${targetIndex}`; @@ -261,7 +266,13 @@ export class TrezorKeyringV2 return [existingAccount]; } - // Derive the account at the specified index + // Derive the account at the specified index. + // Note: setHdPath resets the inner keyring's accounts array when the path changes. + // This mirrors the production behavior where switching HD paths (via the dropdown) + // resets the entire account list. The TrezorKeyring operates on a single path at + // a time - accounts from different paths cannot coexist. The V2 registry tracks + // accounts independently, but getAccounts() relies on the inner keyring's account + // list, so accounts from a previous path would become inaccessible after switching. this.inner.setHdPath(basePath); this.inner.setAccountToUnlock(targetIndex); const [newAddress] = await this.inner.addAccounts(1); From 92a41fa3baebe8082215d4cbc98c2d7bc02638e8 Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Mon, 12 Jan 2026 15:50:24 +0100 Subject: [PATCH 11/11] fix: address cursor feedbacks --- packages/keyring-eth-trezor/jest.config.js | 8 +-- .../src/trezor-keyring-v2.test.ts | 68 +++++++++++++++++++ .../src/trezor-keyring-v2.ts | 48 +++++++++++-- 3 files changed, 114 insertions(+), 10 deletions(-) diff --git a/packages/keyring-eth-trezor/jest.config.js b/packages/keyring-eth-trezor/jest.config.js index 1b380eea6..f4fcdcb10 100644 --- a/packages/keyring-eth-trezor/jest.config.js +++ b/packages/keyring-eth-trezor/jest.config.js @@ -23,10 +23,10 @@ module.exports = merge(baseConfig, { // An object that configures minimum threshold enforcement for coverage results coverageThreshold: { global: { - branches: 59.58, - functions: 92.95, - lines: 93.15, - statements: 93.25, + branches: 60.92, + functions: 93.15, + lines: 93.5, + statements: 93.59, }, }, }); diff --git a/packages/keyring-eth-trezor/src/trezor-keyring-v2.test.ts b/packages/keyring-eth-trezor/src/trezor-keyring-v2.test.ts index 59d07d234..f337fd024 100644 --- a/packages/keyring-eth-trezor/src/trezor-keyring-v2.test.ts +++ b/packages/keyring-eth-trezor/src/trezor-keyring-v2.test.ts @@ -626,5 +626,73 @@ describe('TrezorKeyringV2', () => { expect(account.options.entropy.derivationPath).toBe(`m/44'/60'/0'/0`); }); + + it('clears registry when switching HD paths', async () => { + // Create wrapper with account on BIP44 path + const inner = createInnerKeyring(); + inner.setAccountToUnlock(0); + await inner.addAccounts(1); + + const wrapper = new TrezorKeyringV2({ + legacyKeyring: inner, + entropySource, + }); + + // Get initial accounts - this populates the registry + const initialAccounts = await wrapper.getAccounts(); + expect(initialAccounts).toHaveLength(1); + const initialAccountId = getFirstAccount(initialAccounts).id; + + // Switch to legacy MEW path and add account + // This simulates what createAccounts does when path changes + inner.setHdPath(`m/44'/60'/0'`); + inner.hdk = fakeHdKey; // Reset HDKey after setHdPath clears it + inner.setAccountToUnlock(0); + await inner.addAccounts(1); + + // Call createAccounts with the new path - this should clear registry + await wrapper.createAccounts(derivePathOptions(`m/44'/60'/0'/1`)); + + // The old account should no longer be accessible + await expect(wrapper.getAccount(initialAccountId)).rejects.toThrow( + /Account not found/u, + ); + }); + }); + + describe('locked device handling', () => { + it('returns cached accounts when device is locked', async () => { + const { wrapper, inner } = await createWrapperWithAccounts(2); + + // First call with device unlocked populates the cache + const unlockedAccounts = await wrapper.getAccounts(); + expect(unlockedAccounts).toHaveLength(2); + + // Simulate locked device by clearing the HDKey + inner.hdk = {} as typeof fakeHdKey; + + // Should return cached accounts + const lockedAccounts = await wrapper.getAccounts(); + expect(lockedAccounts).toHaveLength(2); + expect(lockedAccounts).toStrictEqual(unlockedAccounts); + }); + + it('throws error when device is locked and accounts not cached', async () => { + const inner = createInnerKeyring(); + inner.setAccountToUnlock(0); + await inner.addAccounts(1); + + const wrapper = new TrezorKeyringV2({ + legacyKeyring: inner, + entropySource, + }); + + // Simulate locked device before accounts are cached + inner.hdk = {} as typeof fakeHdKey; + + await expect(wrapper.getAccounts()).rejects.toThrow( + /Trezor device is locked/u, + ); + }); }); }); diff --git a/packages/keyring-eth-trezor/src/trezor-keyring-v2.ts b/packages/keyring-eth-trezor/src/trezor-keyring-v2.ts index e60250bae..aaaac20f2 100644 --- a/packages/keyring-eth-trezor/src/trezor-keyring-v2.ts +++ b/packages/keyring-eth-trezor/src/trezor-keyring-v2.ts @@ -88,6 +88,12 @@ export class TrezorKeyringV2 { readonly entropySource: EntropySourceId; + /** + * Tracks the current HD path to detect path changes. + * When the path changes, the registry must be cleared. + */ + #currentHdPath: AllowedHdPath; + constructor(options: TrezorKeyringV2Options) { super({ type: options.type ?? KeyringType.Trezor, @@ -95,6 +101,7 @@ export class TrezorKeyringV2 capabilities: trezorKeyringV2Capabilities, }); this.entropySource = options.entropySource; + this.#currentHdPath = this.inner.hdPath as AllowedHdPath; } /** @@ -123,6 +130,9 @@ export class TrezorKeyringV2 // initialized HDKey. The registry will be populated lazily when // getAccounts() is called after the device is unlocked. await this.inner.deserialize(state); + + // Sync the tracked HD path with the deserialized state + this.#currentHdPath = this.inner.hdPath as AllowedHdPath; }); } @@ -193,6 +203,30 @@ export class TrezorKeyringV2 return []; } + // If the device is locked, we cannot derive addresses to find indices. + // Return cached accounts if available, otherwise throw a clear error. + if (!this.inner.isUnlocked()) { + const cachedAccounts = addresses + .map((address) => { + const existingId = this.registry.getAccountId(address); + return existingId ? this.registry.get(existingId) : undefined; + }) + .filter( + (account): account is Bip44Account => + account !== undefined, + ); + + // If we have all accounts cached, return them + if (cachedAccounts.length === addresses.length) { + return cachedAccounts; + } + + // Some accounts are not cached and device is locked + throw new Error( + 'Trezor device is locked. Please unlock the device to access accounts.', + ); + } + return addresses.map((address) => { // Check if we already have this account in the registry const existingId = this.registry.getAccountId(address); @@ -267,12 +301,14 @@ export class TrezorKeyringV2 } // Derive the account at the specified index. - // Note: setHdPath resets the inner keyring's accounts array when the path changes. - // This mirrors the production behavior where switching HD paths (via the dropdown) - // resets the entire account list. The TrezorKeyring operates on a single path at - // a time - accounts from different paths cannot coexist. The V2 registry tracks - // accounts independently, but getAccounts() relies on the inner keyring's account - // list, so accounts from a previous path would become inaccessible after switching. + // If the HD path is changing, clear the registry to avoid stale accounts. + // The TrezorKeyring operates on a single path at a time - accounts from + // different paths cannot coexist in the inner keyring. + if (basePath !== this.#currentHdPath) { + this.registry.clear(); + this.#currentHdPath = basePath; + } + this.inner.setHdPath(basePath); this.inner.setAccountToUnlock(targetIndex); const [newAddress] = await this.inner.addAccounts(1);