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/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 diff --git a/packages/keyring-eth-trezor/jest.config.js b/packages/keyring-eth-trezor/jest.config.js index 0bb9dd487..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: 52.38, - functions: 91.22, - lines: 90.15, - statements: 90.35, + branches: 60.92, + functions: 93.15, + lines: 93.5, + statements: 93.59, }, }, }); 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..2f22f6bb8 100644 --- a/packages/keyring-eth-trezor/src/index.ts +++ b/packages/keyring-eth-trezor/src/index.ts @@ -1,4 +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.test.ts b/packages/keyring-eth-trezor/src/trezor-keyring-v2.test.ts new file mode 100644 index 000000000..f337fd024 --- /dev/null +++ b/packages/keyring-eth-trezor/src/trezor-keyring-v2.test.ts @@ -0,0 +1,698 @@ +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. + * + * @param hdPath - Optional HD path to set (defaults to BIP44 standard). + * @returns The inner keyring. + */ +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; +} + +/** + * 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, + }; +} + +/** + * 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', () => { + const { wrapper } = createEmptyWrapper(); + + expect(wrapper.type).toBe(KeyringType.Trezor); + expect(wrapper.capabilities).toStrictEqual({ + scopes: [EthScope.Eoa], + bip44: { + deriveIndex: true, + derivePath: true, + 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 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, + }); + + // 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: [ + EXPECTED_ACCOUNTS[0], + EXPECTED_ACCOUNTS[1], + EXPECTED_ACCOUNTS[2], + ], + page: 0, + perPage: 5, + }); + + // 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]); + }); + }); + + 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('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(); + + 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('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); + + 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`); + }); + + 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 new file mode 100644 index 000000000..aaaac20f2 --- /dev/null +++ b/packages/keyring-eth-trezor/src/trezor-keyring-v2.ts @@ -0,0 +1,343 @@ +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, EthKeyring } from '@metamask/keyring-utils'; +import type { Hex, Json } 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: 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}. + * + * 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; + 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< + TrezorKeyringAsEthKeyring, + Bip44Account + > + implements KeyringV2 +{ + 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, + inner: options.legacyKeyring as TrezorKeyringAsEthKeyring, + capabilities: trezorKeyringV2Capabilities, + }); + this.entropySource = options.entropySource; + this.#currentHdPath = this.inner.hdPath as AllowedHdPath; + } + + /** + * 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); + + // Sync the tracked HD path with the deserialized state + this.#currentHdPath = this.inner.hdPath as AllowedHdPath; + }); + } + + /** + * 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. + * + * @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 derivationPath = `${this.inner.hdPath}/${addressIndex}`; + + 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.registry.set(account); + return account; + } + + async getAccounts(): Promise[]> { + const addresses = await this.inner.getAccounts(); + + if (addresses.length === 0) { + 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); + 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 () => { + 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: ${String( + options.type, + )}`, + ); + } + + // 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 + 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}`; + } + + const existingAccount = currentAccounts.find((account) => { + return ( + account.options.entropy.groupIndex === targetIndex && + account.options.entropy.derivationPath === derivationPath + ); + }); + + if (existingAccount) { + return [existingAccount]; + } + + // Derive the account at the specified index. + // 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); + + 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"