From 84a4b7d1c16d008ffbf07ba5b22c61b025b5165f Mon Sep 17 00:00:00 2001 From: Jacob Evans Date: Tue, 10 Apr 2018 14:39:43 +1000 Subject: Refactor ledger to support multiple accounts with from address --- packages/subproviders/src/subproviders/ledger.ts | 111 ++++++++++++--------- .../subproviders/mnemonic_wallet_subprovider.ts | 5 +- packages/subproviders/src/types.ts | 1 + packages/subproviders/src/utils/wallet_utils.ts | 76 ++++++++++++++ packages/subproviders/src/walletUtils.ts | 58 ----------- .../test/integration/ledger_subprovider_test.ts | 3 +- .../test/unit/ledger_subprovider_test.ts | 8 +- .../test/unit/mnemonic_wallet_subprovider_test.ts | 5 +- packages/subproviders/test/utils/fixture_data.ts | 2 + 9 files changed, 155 insertions(+), 114 deletions(-) create mode 100644 packages/subproviders/src/utils/wallet_utils.ts delete mode 100644 packages/subproviders/src/walletUtils.ts (limited to 'packages/subproviders') diff --git a/packages/subproviders/src/subproviders/ledger.ts b/packages/subproviders/src/subproviders/ledger.ts index 23ed3c93e..a7b79c128 100644 --- a/packages/subproviders/src/subproviders/ledger.ts +++ b/packages/subproviders/src/subproviders/ledger.ts @@ -8,6 +8,7 @@ import { Lock } from 'semaphore-async-await'; import { Callback, + DerivedHDKey, LedgerEthereumClient, LedgerEthereumClientFactoryAsync, LedgerSubproviderConfigs, @@ -16,6 +17,7 @@ import { ResponseWithTxParams, WalletSubproviderErrors, } from '../types'; +import { walletUtils } from '../utils/wallet_utils'; import { BaseWalletSubprovider } from './base_wallet_subprovider'; @@ -34,10 +36,11 @@ export class LedgerSubprovider extends BaseWalletSubprovider { private _connectionLock = new Lock(); private _networkId: number; private _derivationPath: string; - private _derivationPathIndex: number; private _ledgerEthereumClientFactoryAsync: LedgerEthereumClientFactoryAsync; private _ledgerClientIfExists?: LedgerEthereumClient; private _shouldAlwaysAskForConfirmation: boolean; + private _addressSearchLimit: number; + private _hardenedKey: boolean = true; /** * Instantiates a LedgerSubprovider. Defaults to derivationPath set to `44'/60'/0'`. * TestRPC/Ganache defaults to `m/44'/60'/0'/0`, so set this in the configs if desired. @@ -54,7 +57,11 @@ export class LedgerSubprovider extends BaseWalletSubprovider { !_.isUndefined(config.accountFetchingConfigs.shouldAskForOnDeviceConfirmation) ? config.accountFetchingConfigs.shouldAskForOnDeviceConfirmation : ASK_FOR_ON_DEVICE_CONFIRMATION; - this._derivationPathIndex = 0; + this._addressSearchLimit = + !_.isUndefined(config.accountFetchingConfigs) && + !_.isUndefined(config.accountFetchingConfigs.numAddressesToReturn) + ? config.accountFetchingConfigs.numAddressesToReturn + : DEFAULT_NUM_ADDRESSES_TO_FETCH; } /** * Retrieve the set derivation path @@ -70,15 +77,6 @@ export class LedgerSubprovider extends BaseWalletSubprovider { public setPath(derivationPath: string) { this._derivationPath = derivationPath; } - /** - * Set the final derivation path index. If a user wishes to sign a message with the - * 6th address in a derivation path, before calling `signPersonalMessageAsync`, you must - * call this method with pathIndex `6`. - * @param pathIndex Desired derivation path index - */ - public setPathIndex(pathIndex: number) { - this._derivationPathIndex = pathIndex; - } /** * Retrieve a users Ledger accounts. The accounts are derived from the derivationPath, * master public key and chainCode. Because of this, you can request as many accounts @@ -89,34 +87,15 @@ export class LedgerSubprovider extends BaseWalletSubprovider { * @return An array of accounts */ public async getAccountsAsync(numberOfAccounts: number = DEFAULT_NUM_ADDRESSES_TO_FETCH): Promise { - this._ledgerClientIfExists = await this._createLedgerClientAsync(); - - let ledgerResponse; - try { - ledgerResponse = await this._ledgerClientIfExists.getAddress( - this._derivationPath, - this._shouldAlwaysAskForConfirmation, - SHOULD_GET_CHAIN_CODE, - ); - } finally { - await this._destroyLedgerClientAsync(); - } - - const hdKey = new HDNode(); - hdKey.publicKey = new Buffer(ledgerResponse.publicKey, 'hex'); - hdKey.chainCode = new Buffer(ledgerResponse.chainCode, 'hex'); - - const accounts: string[] = []; - for (let i = 0; i < numberOfAccounts; i++) { - const derivedHDNode = hdKey.derive(`m/${i + this._derivationPathIndex}`); - const derivedPublicKey = derivedHDNode.publicKey; - const shouldSanitizePublicKey = true; - const ethereumAddressUnprefixed = ethUtil - .publicToAddress(derivedPublicKey, shouldSanitizePublicKey) - .toString('hex'); - const ethereumAddressPrefixed = ethUtil.addHexPrefix(ethereumAddressUnprefixed); - accounts.push(ethereumAddressPrefixed.toLowerCase()); - } + const initialHDKey = await this._initialHDKeyAsync(); + const derivedKeys = walletUtils._calculateDerivedHDKeys( + initialHDKey, + this._derivationPath, + numberOfAccounts, + 0, + true, + ); + const accounts = _.map(derivedKeys, 'address'); return accounts; } /** @@ -129,6 +108,11 @@ export class LedgerSubprovider extends BaseWalletSubprovider { */ public async signTransactionAsync(txParams: PartialTxParams): Promise { LedgerSubprovider._validateTxParams(txParams); + const initialHDKey = await this._initialHDKeyAsync(); + const derivedKey = _.isUndefined(txParams.from) + ? walletUtils._firstDerivedKey(initialHDKey, this._derivationPath, this._hardenedKey) + : this._findDerivedKeyByPublicAddress(initialHDKey, txParams.from); + this._ledgerClientIfExists = await this._createLedgerClientAsync(); const tx = new EthereumTx(txParams); @@ -140,7 +124,7 @@ export class LedgerSubprovider extends BaseWalletSubprovider { const txHex = tx.serialize().toString('hex'); try { - const derivationPath = this._getDerivationPath(); + const derivationPath = `${derivedKey.derivationPath}/${derivedKey.derivationIndex}`; const result = await this._ledgerClientIfExists.signTransaction(derivationPath, txHex); // Store signature in transaction tx.r = Buffer.from(result.r, 'hex'); @@ -165,22 +149,28 @@ export class LedgerSubprovider extends BaseWalletSubprovider { } /** * Sign a personal Ethereum signed message. The signing address will be the one - * retrieved given a derivationPath and pathIndex set on the subprovider. + * either the provided address or the first address on the ledger device. * The Ledger adds the Ethereum signed message prefix on-device. If you've added * the LedgerSubprovider to your app's provider, you can simply send an `eth_sign` * or `personal_sign` JSON RPC request, and this method will be called auto-magically. * If you are not using this via a ProviderEngine instance, you can call it directly. * @param data Message to sign + * @param address Address to sign with * @return Signature hex string (order: rsv) */ - public async signPersonalMessageAsync(data: string): Promise { + public async signPersonalMessageAsync(data: string, address?: string): Promise { if (_.isUndefined(data)) { throw new Error(WalletSubproviderErrors.DataMissingForSignPersonalMessage); } assert.isHexString('data', data); + const initialHDKey = await this._initialHDKeyAsync(); + const derivedKey = _.isUndefined(address) + ? walletUtils._firstDerivedKey(initialHDKey, this._derivationPath, this._hardenedKey) + : this._findDerivedKeyByPublicAddress(initialHDKey, address); + this._ledgerClientIfExists = await this._createLedgerClientAsync(); try { - const derivationPath = this._getDerivationPath(); + const derivationPath = `${derivedKey.derivationPath}/${derivedKey.derivationIndex}`; const result = await this._ledgerClientIfExists.signPersonalMessage( derivationPath, ethUtil.stripHexPrefix(data), @@ -198,10 +188,6 @@ export class LedgerSubprovider extends BaseWalletSubprovider { throw err; } } - private _getDerivationPath() { - const derivationPath = `${this.getPath()}/${this._derivationPathIndex}`; - return derivationPath; - } private async _createLedgerClientAsync(): Promise { await this._connectionLock.acquire(); if (!_.isUndefined(this._ledgerClientIfExists)) { @@ -222,4 +208,35 @@ export class LedgerSubprovider extends BaseWalletSubprovider { this._ledgerClientIfExists = undefined; this._connectionLock.release(); } + private async _initialHDKeyAsync(): Promise { + this._ledgerClientIfExists = await this._createLedgerClientAsync(); + + let ledgerResponse; + try { + ledgerResponse = await this._ledgerClientIfExists.getAddress( + this._derivationPath, + this._shouldAlwaysAskForConfirmation, + SHOULD_GET_CHAIN_CODE, + ); + } finally { + await this._destroyLedgerClientAsync(); + } + const hdKey = new HDNode(); + hdKey.publicKey = new Buffer(ledgerResponse.publicKey, 'hex'); + hdKey.chainCode = new Buffer(ledgerResponse.chainCode, 'hex'); + return hdKey; + } + private _findDerivedKeyByPublicAddress(initalHDKey: HDNode, address: string): DerivedHDKey { + const matchedDerivedKey = walletUtils._findDerivedKeyByAddress( + address, + initalHDKey, + this._derivationPath, + this._addressSearchLimit, + this._hardenedKey, + ); + if (_.isUndefined(matchedDerivedKey)) { + throw new Error(`${WalletSubproviderErrors.AddressNotFound}: ${address}`); + } + return matchedDerivedKey; + } } diff --git a/packages/subproviders/src/subproviders/mnemonic_wallet_subprovider.ts b/packages/subproviders/src/subproviders/mnemonic_wallet_subprovider.ts index 456bde05c..3ff437659 100644 --- a/packages/subproviders/src/subproviders/mnemonic_wallet_subprovider.ts +++ b/packages/subproviders/src/subproviders/mnemonic_wallet_subprovider.ts @@ -5,12 +5,12 @@ import HDNode = require('hdkey'); import * as _ from 'lodash'; import { DerivedHDKey, PartialTxParams, WalletSubproviderErrors } from '../types'; -import { walletUtils } from '../walletUtils'; +import { walletUtils } from '../utils/wallet_utils'; import { BaseWalletSubprovider } from './base_wallet_subprovider'; import { PrivateKeyWalletSubprovider } from './private_key_wallet_subprovider'; -const DEFAULT_DERIVATION_PATH = `44'/60'/0'`; +const DEFAULT_DERIVATION_PATH = `44'/60'/0'/0`; const DEFAULT_NUM_ADDRESSES_TO_FETCH = 10; const DEFAULT_ADDRESS_SEARCH_LIMIT = 1000; @@ -23,7 +23,6 @@ export class MnemonicWalletSubprovider extends BaseWalletSubprovider { private _addressSearchLimit: number; private _derivationPath: string; private _hdKey: HDNode; - constructor( mnemonic: string, derivationPath: string = DEFAULT_DERIVATION_PATH, diff --git a/packages/subproviders/src/types.ts b/packages/subproviders/src/types.ts index 105ffa7cc..fe7ae921e 100644 --- a/packages/subproviders/src/types.ts +++ b/packages/subproviders/src/types.ts @@ -114,6 +114,7 @@ export enum NonceSubproviderErrors { export interface DerivedHDKey { address: string; derivationPath: string; + derivationIndex: number; hdKey: HDNode; } diff --git a/packages/subproviders/src/utils/wallet_utils.ts b/packages/subproviders/src/utils/wallet_utils.ts new file mode 100644 index 000000000..6c698a006 --- /dev/null +++ b/packages/subproviders/src/utils/wallet_utils.ts @@ -0,0 +1,76 @@ +import ethUtil = require('ethereumjs-util'); +import HDNode = require('hdkey'); +import * as _ from 'lodash'; + +import { DerivedHDKey, WalletSubproviderErrors } from '../types'; + +const DEFAULT_ADDRESS_SEARCH_OFFSET = 0; +const BATCH_SIZE = 10; + +// Derivation Paths +// BIP44 m / purpose' / coin_type' / account' / change / address_index +// m/44'/60'/0'/0 +// m/44'/60'/0'/0/0 +// m/44'/60'/0'/0/{account_index} - testrpc +// m/44'/60'/0' - ledger + +export const walletUtils = { + _calculateDerivedHDKeys( + initialHDKey: HDNode, + derivationPath: string, + searchLimit: number, + offset: number = DEFAULT_ADDRESS_SEARCH_OFFSET, + hardened: boolean = false, + ): DerivedHDKey[] { + const derivedKeys: DerivedHDKey[] = []; + _.times(searchLimit, i => { + const derivationIndex = offset + i; + const path = hardened ? `m/${derivationIndex}` : `m/${derivationPath}/${derivationIndex}`; + const hdKey = initialHDKey.derive(path); + const derivedPublicKey = hdKey.publicKey; + const shouldSanitizePublicKey = true; + const ethereumAddressUnprefixed = ethUtil + .publicToAddress(derivedPublicKey, shouldSanitizePublicKey) + .toString('hex'); + const address = ethUtil.addHexPrefix(ethereumAddressUnprefixed); + const derivedKey: DerivedHDKey = { + derivationPath, + hdKey, + address, + derivationIndex, + }; + derivedKeys.push(derivedKey); + }); + return derivedKeys; + }, + + _findDerivedKeyByAddress( + address: string, + initialHDKey: HDNode, + derivationPath: string, + searchLimit: number, + hardened: boolean = false, + ): DerivedHDKey | undefined { + let matchedKey: DerivedHDKey | undefined; + for (let index = 0; index < searchLimit; index = index + BATCH_SIZE) { + const derivedKeys = walletUtils._calculateDerivedHDKeys( + initialHDKey, + derivationPath, + BATCH_SIZE, + index, + hardened, + ); + matchedKey = _.find(derivedKeys, derivedKey => derivedKey.address === address); + if (matchedKey) { + break; + } + } + return matchedKey; + }, + + _firstDerivedKey(initialHDKey: HDNode, derivationPath: string, hardened: boolean = false): DerivedHDKey { + const derivedKeys = walletUtils._calculateDerivedHDKeys(initialHDKey, derivationPath, 1, 0, hardened); + const firstDerivedKey = derivedKeys[0]; + return firstDerivedKey; + }, +}; diff --git a/packages/subproviders/src/walletUtils.ts b/packages/subproviders/src/walletUtils.ts deleted file mode 100644 index 631636a71..000000000 --- a/packages/subproviders/src/walletUtils.ts +++ /dev/null @@ -1,58 +0,0 @@ -import ethUtil = require('ethereumjs-util'); -import HDNode = require('hdkey'); -import * as _ from 'lodash'; - -import { DerivedHDKey, WalletSubproviderErrors } from './types'; - -const DEFAULT_ADDRESS_SEARCH_OFFSET = 0; -const BATCH_SIZE = 10; -export const walletUtils = { - _calculateDerivedHDKeys( - initialHDKey: HDNode, - derivationPath: string, - searchLimit: number, - offset: number = DEFAULT_ADDRESS_SEARCH_OFFSET, - ): DerivedHDKey[] { - const derivedKeys: DerivedHDKey[] = []; - _.times(searchLimit, i => { - const path = `m/${derivationPath}/${i + offset}`; - const hdKey = initialHDKey.derive(path); - const derivedPublicKey = hdKey.publicKey; - const shouldSanitizePublicKey = true; - const ethereumAddressUnprefixed = ethUtil - .publicToAddress(derivedPublicKey, shouldSanitizePublicKey) - .toString('hex'); - const address = ethUtil.addHexPrefix(ethereumAddressUnprefixed); - const derivedKey: DerivedHDKey = { - derivationPath: path, - hdKey, - address, - }; - derivedKeys.push(derivedKey); - }); - return derivedKeys; - }, - - _findDerivedKeyByAddress( - address: string, - initialHDKey: HDNode, - derivationPath: string, - searchLimit: number, - ): DerivedHDKey | undefined { - let matchedKey: DerivedHDKey | undefined; - for (let index = 0; index < searchLimit; index = index + BATCH_SIZE) { - const derivedKeys = walletUtils._calculateDerivedHDKeys(initialHDKey, derivationPath, BATCH_SIZE, index); - matchedKey = _.find(derivedKeys, derivedKey => derivedKey.address === address); - if (matchedKey) { - break; - } - } - return matchedKey; - }, - - _firstDerivedKey(initialHDKey: HDNode, derivationPath: string): DerivedHDKey { - const derivedKeys = walletUtils._calculateDerivedHDKeys(initialHDKey, derivationPath, 1); - const firstDerivedKey = derivedKeys[0]; - return firstDerivedKey; - }, -}; diff --git a/packages/subproviders/test/integration/ledger_subprovider_test.ts b/packages/subproviders/test/integration/ledger_subprovider_test.ts index 503618089..88a3c6db1 100644 --- a/packages/subproviders/test/integration/ledger_subprovider_test.ts +++ b/packages/subproviders/test/integration/ledger_subprovider_test.ts @@ -42,9 +42,10 @@ describe('LedgerSubprovider', () => { expect(accounts[0]).to.not.be.an('undefined'); expect(accounts.length).to.be.equal(10); }); - it('returns the expected first account from a ledger set up with the test mnemonic', async () => { + it('returns the expected accounts from a ledger set up with the test mnemonic', async () => { const accounts = await ledgerSubprovider.getAccountsAsync(); expect(accounts[0]).to.be.equal(fixtureData.TEST_RPC_ACCOUNT_0); + expect(accounts[1]).to.be.equal(fixtureData.TEST_RPC_ACCOUNT_1); }); it('returns requested number of accounts', async () => { const numberOfAccounts = 20; diff --git a/packages/subproviders/test/unit/ledger_subprovider_test.ts b/packages/subproviders/test/unit/ledger_subprovider_test.ts index c18506681..d3a3fbe26 100644 --- a/packages/subproviders/test/unit/ledger_subprovider_test.ts +++ b/packages/subproviders/test/unit/ledger_subprovider_test.ts @@ -132,7 +132,7 @@ describe('LedgerSubprovider', () => { const payload = { jsonrpc: '2.0', method: 'eth_sign', - params: ['0x0000000000000000000000000000000000000000', messageHex], + params: [FAKE_ADDRESS, messageHex], id: 1, }; const callback = reportCallbackErrors(done)((err: Error, response: JSONRPCResponsePayload) => { @@ -149,7 +149,7 @@ describe('LedgerSubprovider', () => { const payload = { jsonrpc: '2.0', method: 'personal_sign', - params: [messageHex, '0x0000000000000000000000000000000000000000'], + params: [messageHex, FAKE_ADDRESS], id: 1, }; const callback = reportCallbackErrors(done)((err: Error, response: JSONRPCResponsePayload) => { @@ -190,7 +190,7 @@ describe('LedgerSubprovider', () => { const payload = { jsonrpc: '2.0', method: 'eth_sign', - params: ['0x0000000000000000000000000000000000000000', nonHexMessage], + params: [FAKE_ADDRESS, nonHexMessage], id: 1, }; const callback = reportCallbackErrors(done)((err: Error, response: JSONRPCResponsePayload) => { @@ -205,7 +205,7 @@ describe('LedgerSubprovider', () => { const payload = { jsonrpc: '2.0', method: 'personal_sign', - params: [nonHexMessage, '0x0000000000000000000000000000000000000000'], + params: [nonHexMessage, FAKE_ADDRESS], id: 1, }; const callback = reportCallbackErrors(done)((err: Error, response: JSONRPCResponsePayload) => { diff --git a/packages/subproviders/test/unit/mnemonic_wallet_subprovider_test.ts b/packages/subproviders/test/unit/mnemonic_wallet_subprovider_test.ts index 7aaef4944..16981cf86 100644 --- a/packages/subproviders/test/unit/mnemonic_wallet_subprovider_test.ts +++ b/packages/subproviders/test/unit/mnemonic_wallet_subprovider_test.ts @@ -28,9 +28,12 @@ describe('MnemonicWalletSubprovider', () => { }); describe('direct method calls', () => { describe('success cases', () => { - it('returns the account', async () => { + it('returns the accounts', async () => { const accounts = await subprovider.getAccountsAsync(); + // tslint:disable-next-line:no-console + console.log(accounts); expect(accounts[0]).to.be.equal(fixtureData.TEST_RPC_ACCOUNT_0); + expect(accounts[1]).to.be.equal(fixtureData.TEST_RPC_ACCOUNT_1); expect(accounts.length).to.be.equal(10); }); it('signs a personal message', async () => { diff --git a/packages/subproviders/test/utils/fixture_data.ts b/packages/subproviders/test/utils/fixture_data.ts index 5ce3ff08f..62b76e132 100644 --- a/packages/subproviders/test/utils/fixture_data.ts +++ b/packages/subproviders/test/utils/fixture_data.ts @@ -1,8 +1,10 @@ const TEST_RPC_ACCOUNT_0 = '0x5409ed021d9299bf6814279a6a1411a7e866a631'; +const TEST_RPC_ACCOUNT_1 = '0x6ecbe1db9ef729cbe972c83fb886247691fb6beb'; const networkId = 42; export const fixtureData = { TEST_RPC_ACCOUNT_0, TEST_RPC_ACCOUNT_0_ACCOUNT_PRIVATE_KEY: 'F2F48EE19680706196E2E339E5DA3491186E0C4C5030670656B0E0164837257D', + TEST_RPC_ACCOUNT_1, TEST_RPC_MNEMONIC: 'concert load couple harbor equip island argue ramp clarify fence smart topic', TEST_RPC_MNEMONIC_DERIVATION_PATH: `44'/60'/0'/0`, PERSONAL_MESSAGE_STRING: 'hello world', -- cgit v1.2.3