diff --git a/.changeset/ledger-live-account-indexes.md b/.changeset/ledger-live-account-indexes.md new file mode 100644 index 000000000..2845ce770 --- /dev/null +++ b/.changeset/ledger-live-account-indexes.md @@ -0,0 +1,7 @@ +--- +'@celo/wallet-ledger': patch +'@celo/viem-account-ledger': patch +'@celo/celocli': patch +--- + +Fix `--ledgerLiveMode` so it iterates BIP-44 hardened account indexes (`m/44'/60'/N'/0/0`) instead of the change index. `LedgerWallet`, `newLedgerWalletWithSetup`, `deriveLedgerAccounts`, and `ledgerToWalletClient` now accept `accountIndexes`. diff --git a/packages/cli/src/base.test.ts b/packages/cli/src/base.test.ts index e59f61d75..133ea9536 100644 --- a/packages/cli/src/base.test.ts +++ b/packages/cli/src/base.test.ts @@ -195,7 +195,7 @@ testWithAnvilL2('BaseCommand', (provider) => { }) describe('with --ledgerLiveMode', () => { - it('--ledgerAddresses passes changeIndexes to LedgerWallet', async () => { + it('--ledgerAddresses passes accountIndexes to LedgerWallet', async () => { await testLocallyWithNode( BasicCommand, ['--useLedger', '--ledgerLiveMode', '--ledgerAddresses', '5'], @@ -205,14 +205,16 @@ testWithAnvilL2('BaseCommand', (provider) => { expect(WalletLedgerExports.newLedgerWalletWithSetup).toHaveBeenCalledWith( expect.anything(), expect.objectContaining({ - changeIndexes: [0, 1, 2, 3, 4], + accountIndexes: [0, 1, 2, 3, 4], + changeIndexes: [0], derivationPathIndexes: [0], }) ) expect(ViemAccountLedgerExports.ledgerToWalletClient).toHaveBeenCalledWith( expect.objectContaining({ - changeIndexes: [0, 1, 2, 3, 4], + accountIndexes: [0, 1, 2, 3, 4], + changeIndexes: [0], derivationPathIndexes: [0], }) ) @@ -244,7 +246,7 @@ testWithAnvilL2('BaseCommand', (provider) => { `) }) describe('with --ledgerCustomAddresses', () => { - it('passes custom changeIndexes to LedgerWallet', async () => { + it('passes custom accountIndexes to LedgerWallet', async () => { await testLocallyWithNode( BasicCommand, ['--useLedger', '--ledgerLiveMode', '--ledgerCustomAddresses', '[1,8,9]'], @@ -254,14 +256,16 @@ testWithAnvilL2('BaseCommand', (provider) => { expect(WalletLedgerExports.newLedgerWalletWithSetup).toHaveBeenCalledWith( expect.anything(), expect.objectContaining({ - changeIndexes: [1, 8, 9], + accountIndexes: [1, 8, 9], + changeIndexes: [0], derivationPathIndexes: [0], }) ) expect(ViemAccountLedgerExports.ledgerToWalletClient).toHaveBeenCalledWith( expect.objectContaining({ - changeIndexes: [1, 8, 9], + accountIndexes: [1, 8, 9], + changeIndexes: [0], derivationPathIndexes: [0], }) ) diff --git a/packages/cli/src/base.ts b/packages/cli/src/base.ts index e4b33a9fd..7b8c669f6 100644 --- a/packages/cli/src/base.ts +++ b/packages/cli/src/base.ts @@ -1,8 +1,8 @@ import { type PublicCeloClient, type WalletCeloClient } from '@celo/actions' import { CELO_DERIVATION_PATH_BASE, - ensureLeading0x, ETHEREUM_DERIVATION_PATH, + ensureLeading0x, StrongAddress, } from '@celo/base' import { type Provider, ReadOnlyWallet } from '@celo/connect' @@ -95,7 +95,7 @@ export abstract class BaseCommand extends Command { dependsOn: ['useLedger'], default: false, description: - 'When set, the 4th postion of the derivation path will be iterated over instead of the 5th. This is useful to use same address on you Ledger with celocli as you do on Ledger Live', + "When set, the BIP-44 account index is iterated instead of the address index. This matches Ledger Live account paths (m/44'/60'/N'/0/0)", }), ledgerCustomAddresses: Flags.string({ dependsOn: ['useLedger'], @@ -330,7 +330,8 @@ export abstract class BaseCommand extends Command { transport: await this.openLedgerTransport(), baseDerivationPath: getDefaultDerivationPath(this.config.configDir), derivationPathIndexes: isLedgerLiveMode ? [0] : indicesToIterateOver, - changeIndexes: isLedgerLiveMode ? indicesToIterateOver : [0], + changeIndexes: [0], + accountIndexes: isLedgerLiveMode ? indicesToIterateOver : undefined, ledgerAddressValidation: ledgerConfirmation, } return ledgerOptions @@ -389,7 +390,8 @@ export abstract class BaseCommand extends Command { this._wallet = await newLedgerWalletWithSetup(await this.openLedgerTransport(), { baseDerivationPath: baseDerivationPath, derivationPathIndexes: isLedgerLiveMode ? [0] : indicesToIterateOver, - changeIndexes: isLedgerLiveMode ? indicesToIterateOver : [0], + changeIndexes: [0], + accountIndexes: isLedgerLiveMode ? indicesToIterateOver : undefined, ledgerAddressValidation: ledgerConfirmation, }) } catch (err) { diff --git a/packages/sdk/wallets/wallet-ledger/src/ledger-wallet.test.ts b/packages/sdk/wallets/wallet-ledger/src/ledger-wallet.test.ts index fa33c2424..f78be0167 100644 --- a/packages/sdk/wallets/wallet-ledger/src/ledger-wallet.test.ts +++ b/packages/sdk/wallets/wallet-ledger/src/ledger-wallet.test.ts @@ -6,9 +6,9 @@ import { recoverTransaction, verifyEIP712TypedDataSigner } from '@celo/wallet-ba import TransportNodeHid from '@ledgerhq/hw-transport-node-hid' import { AddressValidation, CELO_BASE_DERIVATION_PATH, LedgerWallet } from './ledger-wallet' import { + ACCOUNT_ADDRESS_NEVER, ACCOUNT_ADDRESS1, ACCOUNT_ADDRESS2, - ACCOUNT_ADDRESS_NEVER, mockLedgerImplementation, } from './test-utils' import { tokenInfoByAddressAndChainId } from './tokens' @@ -221,6 +221,34 @@ describe('LedgerWallet class', () => { `) expect(wallet.ledger!.getAddress).toHaveBeenCalledTimes(6) }) + it('iterates hardened account indexes for Ledger Live paths', async () => { + // Ledger Live accounts are m/44'/60'/N'/0/0. Index ≥1 is required to distinguish + // that from iterating the change component (m/44'/60'/0'/N/0). + wallet = new LedgerWallet( + {}, + [0], + "m/44'/60'/0'", + [0], + AddressValidation.firstTransactionPerAddress, + [0, 1, 2] + ) + mockLedger(wallet, mockForceValidation) + await wallet.init() + // @ts-expect-error (mock.calls) + expect(wallet.ledger!.getAddress.mock.calls).toEqual([ + ["44'/60'/0'/0/0", false], + ["44'/60'/1'/0/0", false], + ["44'/60'/2'/0/0", false], + ]) + expect(wallet.ledger!.getAddress).toHaveBeenCalledTimes(3) + }) + it('does not iterate the account component when accountIndexes is omitted', async () => { + wallet = new LedgerWallet({}, [0], "m/44'/60'/5'", [0]) + mockLedger(wallet, mockForceValidation) + await wallet.init() + // @ts-expect-error (mock.calls) + expect(wallet.ledger!.getAddress.mock.calls).toEqual([["44'/60'/5'/0/0", false]]) + }) describe('with other ledger apps', () => { describe('with the ethereum-recovery app', () => { beforeEach(() => { diff --git a/packages/sdk/wallets/wallet-ledger/src/ledger-wallet.ts b/packages/sdk/wallets/wallet-ledger/src/ledger-wallet.ts index f263dfe88..ac82531a3 100644 --- a/packages/sdk/wallets/wallet-ledger/src/ledger-wallet.ts +++ b/packages/sdk/wallets/wallet-ledger/src/ledger-wallet.ts @@ -31,6 +31,7 @@ export enum AddressValidation { interface LedgerWalletSetup { derivationPathIndexes?: number[] changeIndexes?: number[] + accountIndexes?: number[] baseDerivationPath?: string ledgerAddressValidation?: AddressValidation } @@ -42,6 +43,7 @@ export async function newLedgerWalletWithSetup( baseDerivationPath, ledgerAddressValidation, changeIndexes, + accountIndexes, }: LedgerWalletSetup ): Promise { const wallet = new LedgerWallet( @@ -49,7 +51,8 @@ export async function newLedgerWalletWithSetup( derivationPathIndexes, baseDerivationPath, changeIndexes, - ledgerAddressValidation + ledgerAddressValidation, + accountIndexes ) await wallet.init() return wallet @@ -74,18 +77,25 @@ export class LedgerWallet extends RemoteWallet implements ReadOnly * Default: [0]. * Example: [0, 1] will retrieve the derivation paths of [`${baseDerivationPath}/0/${address_index}`, `${baseDerivationPath}/1/${address_index}`, `${baseDerivationPath}/2/${address_index}`] * @param ledgerAddressValidation AddressValidation enum to validate addresses. Default: AddressValidation.firstTransactionPerAddress + * @param accountIndexes number array of BIP-44 hardened "account" indexes. + * Default: the account component from `baseDerivationPath`. + * Example: [0, 1, 2] with change/address 0 yields [`44'/60'/0'/0/0`, `44'/60'/1'/0/0`, `44'/60'/2'/0/0`] */ constructor( readonly transport: any = {}, readonly derivationPathIndexes: number[] = zeroRange(ADDRESS_QTY), readonly baseDerivationPath: string = CELO_BASE_DERIVATION_PATH, readonly changeIndexes: number[] = [0], - readonly ledgerAddressValidation: AddressValidation = AddressValidation.firstTransactionPerAddress + readonly ledgerAddressValidation: AddressValidation = AddressValidation.firstTransactionPerAddress, + readonly accountIndexes?: number[] ) { super() validateIndexes(derivationPathIndexes, 'address index') validateIndexes(changeIndexes, 'change index') + if (accountIndexes) { + validateIndexes(accountIndexes, 'account index') + } // Remove the 'm/' prefix if it exists since we dont expect it here but that is how derivaiton path is used in the rest of the code this.baseDerivationPath = baseDerivationPath.startsWith('m/') ? baseDerivationPath.slice(2) @@ -174,22 +184,27 @@ export class LedgerWallet extends RemoteWallet implements ReadOnly const appConfiguration = await this.retrieveAppConfiguration() const validationRequired = this.ledgerAddressValidation === AddressValidation.initializationOnly // https://trezor.io/learn/a/what-is-bip44 - const [purpose, coinType, account] = this.baseDerivationPath.split('/') + const [purpose, coinType, accountFromPath] = this.baseDerivationPath.split('/') + const accounts = this.accountIndexes + ? this.accountIndexes.map((index) => `${index}'`) + : [accountFromPath] // Each address must be retrieved synchronously, (ledger lock) - for (const changeIndex of this.changeIndexes) { - for (const addressIndex of this.derivationPathIndexes) { - const derivationPath = `${purpose}/${coinType}/${account}/${changeIndex}/${addressIndex}` - debug(`Fetching address for derivation path ${derivationPath}`) - const addressInfo = await this.ledger!.getAddress(derivationPath, validationRequired) - addressToSigner.set( - addressInfo.address!, - new LedgerSigner( - this.ledger!, - derivationPath, - this.ledgerAddressValidation, - appConfiguration + for (const account of accounts) { + for (const changeIndex of this.changeIndexes) { + for (const addressIndex of this.derivationPathIndexes) { + const derivationPath = `${purpose}/${coinType}/${account}/${changeIndex}/${addressIndex}` + debug(`Fetching address for derivation path ${derivationPath}`) + const addressInfo = await this.ledger!.getAddress(derivationPath, validationRequired) + addressToSigner.set( + addressInfo.address!, + new LedgerSigner( + this.ledger!, + derivationPath, + this.ledgerAddressValidation, + appConfiguration + ) ) - ) + } } } return addressToSigner diff --git a/packages/viem-account-ledger/src/derive-ledger-accounts.test.ts b/packages/viem-account-ledger/src/derive-ledger-accounts.test.ts index 13cbe2fce..fc662a921 100644 --- a/packages/viem-account-ledger/src/derive-ledger-accounts.test.ts +++ b/packages/viem-account-ledger/src/derive-ledger-accounts.test.ts @@ -1,11 +1,10 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' import { deriveLedgerAccounts } from './derive-ledger-accounts' import * as LedgerAccount from './ledger-to-account' +import { mockLedger } from './test-utils' import { AddressValidation } from './types' import { generateLedger } from './utils' -import { beforeEach, describe, expect, it, vi } from 'vitest' -import { mockLedger } from './test-utils' - vi.mock('./utils', () => ({ generateLedger: vi.fn().mockImplementation(() => { return mockLedger() @@ -85,4 +84,61 @@ describe('deriveLedgerAccounts', () => { }) ).rejects.toThrow(/Invalid change index provided/) }) + + it('iterates hardened account indexes for Ledger Live paths', async () => { + // Index ≥1 distinguishes m/44'/60'/N'/0/0 from iterating change (m/44'/60'/0'/N/0). + ledgerToAccount.mockResolvedValue({ + address: '0x1be31a94361a391bbafb2a4ccd704f57dc04d4bb', + }) + await deriveLedgerAccounts({ + transport: {} as any, + derivationPathIndexes: [0], + changeIndexes: [0], + accountIndexes: [0, 1, 2], + baseDerivationPath: "m/44'/60'/0'", + }) + expect(ledgerToAccount).toHaveBeenCalledTimes(3) + expect( + ledgerToAccount.mock.calls.map(([{ baseDerivationPath, derivationPathIndex }]) => { + return `${baseDerivationPath}/${derivationPathIndex}` + }) + ).toEqual(["44'/60'/0'/0/0", "44'/60'/1'/0/0", "44'/60'/2'/0/0"]) + }) + + it('does not iterate the account component when accountIndexes is omitted', async () => { + ledgerToAccount.mockResolvedValue({ + address: '0x1be31a94361a391bbafb2a4ccd704f57dc04d4bb', + }) + await deriveLedgerAccounts({ + transport: {} as any, + derivationPathIndexes: [0], + changeIndexes: [0], + baseDerivationPath: "m/44'/60'/5'", + }) + expect(ledgerToAccount).toHaveBeenCalledTimes(1) + expect(ledgerToAccount).toHaveBeenCalledWith( + expect.objectContaining({ + derivationPathIndex: 0, + baseDerivationPath: "44'/60'/5'/0", + }) + ) + }) + + it('throws if accountIndexes is empty', async () => { + await expect( + deriveLedgerAccounts({ + transport: {} as any, + accountIndexes: [], + }) + ).rejects.toThrow(/No account index provided/) + }) + + it('throws if accountIndexes contains invalid value', async () => { + await expect( + deriveLedgerAccounts({ + transport: {} as any, + accountIndexes: [-1], + }) + ).rejects.toThrow(/Invalid account index provided/) + }) }) diff --git a/packages/viem-account-ledger/src/derive-ledger-accounts.ts b/packages/viem-account-ledger/src/derive-ledger-accounts.ts index 2c42b9b07..ce71985aa 100644 --- a/packages/viem-account-ledger/src/derive-ledger-accounts.ts +++ b/packages/viem-account-ledger/src/derive-ledger-accounts.ts @@ -9,12 +9,14 @@ export async function deriveLedgerAccounts({ transport, derivationPathIndexes = zeroRange(ADDRESS_QTY), changeIndexes = [0], + accountIndexes, baseDerivationPath = DEFAULT_DERIVATION_PATH, ledgerAddressValidation, }: { transport: TransportNodeHid derivationPathIndexes?: number[] changeIndexes?: number[] + accountIndexes?: number[] baseDerivationPath?: string ledgerAddressValidation?: AddressValidation }) { @@ -22,23 +24,31 @@ export async function deriveLedgerAccounts({ const accounts: LedgerAccount[] = [] validateIndexes(derivationPathIndexes, 'address index') validateIndexes(changeIndexes, 'change index') + if (accountIndexes) { + validateIndexes(accountIndexes, 'account index') + } const _baseDerivationPath = baseDerivationPath.startsWith('m/') ? baseDerivationPath.slice(2) : baseDerivationPath // https://trezor.io/learn/a/what-is-bip44 - const [purpose, coinType, accountIndex] = _baseDerivationPath.split('/') - for (const changeIndex of changeIndexes) { - for (const addressIndex of derivationPathIndexes) { - accounts.push( - await ledgerToAccount({ - ledger, - derivationPathIndex: addressIndex, - baseDerivationPath: `${purpose}/${coinType}/${accountIndex}/${changeIndex}`, - ledgerAddressValidation, - }) - ) + const [purpose, coinType, accountFromPath] = _baseDerivationPath.split('/') + const accountsToIterate = accountIndexes + ? accountIndexes.map((index) => `${index}'`) + : [accountFromPath] + for (const account of accountsToIterate) { + for (const changeIndex of changeIndexes) { + for (const addressIndex of derivationPathIndexes) { + accounts.push( + await ledgerToAccount({ + ledger, + derivationPathIndex: addressIndex, + baseDerivationPath: `${purpose}/${coinType}/${account}/${changeIndex}`, + ledgerAddressValidation, + }) + ) + } } } return accounts diff --git a/packages/viem-account-ledger/src/ledger-to-wallet-client.test.ts b/packages/viem-account-ledger/src/ledger-to-wallet-client.test.ts index 6dbe2d16e..84540dc39 100644 --- a/packages/viem-account-ledger/src/ledger-to-wallet-client.test.ts +++ b/packages/viem-account-ledger/src/ledger-to-wallet-client.test.ts @@ -390,6 +390,23 @@ syntheticDescribe('ledgerToWalletClient (mocked ledger)', () => { ]) expect(spy).toHaveBeenCalledTimes(6) }) + + it('iterates hardened account indexes for Ledger Live paths', async () => { + client = await ledgerToWalletClient({ + transport: await transport, + walletClientOptions: defaultWalletClientOptions, + derivationPathIndexes: [0], + changeIndexes: [0], + accountIndexes: [0, 1, 2], + baseDerivationPath: "m/44'/60'/0'", + }) + expect(spy.mock.calls).toEqual([ + ["44'/60'/0'/0/0", false], + ["44'/60'/1'/0/0", false], + ["44'/60'/2'/0/0", false], + ]) + expect(spy).toHaveBeenCalledTimes(3) + }) }) }) } diff --git a/packages/viem-account-ledger/src/ledger-to-wallet-client.ts b/packages/viem-account-ledger/src/ledger-to-wallet-client.ts index c0809e2c5..631d8b01f 100644 --- a/packages/viem-account-ledger/src/ledger-to-wallet-client.ts +++ b/packages/viem-account-ledger/src/ledger-to-wallet-client.ts @@ -9,6 +9,7 @@ export async function ledgerToWalletClient