From a044c149c0fa451236796982e3ca2510756ea46c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ga=C3=ABtan=20Renaudeau?= Date: Fri, 13 Jul 2018 10:08:07 +0200 Subject: [PATCH] Prevent locks to happen during scanning --- src/commands/libcoreScanAccounts.js | 4 +- src/components/DeviceBusyIndicator.js | 9 +- src/helpers/deviceAccess.js | 11 +++ src/helpers/libcore.js | 113 +++++++++++++++----------- src/helpers/withLibcore.js | 3 +- src/renderer/events.js | 4 +- 6 files changed, 86 insertions(+), 58 deletions(-) diff --git a/src/commands/libcoreScanAccounts.js b/src/commands/libcoreScanAccounts.js index 7044220d..b8b0e2e3 100644 --- a/src/commands/libcoreScanAccounts.js +++ b/src/commands/libcoreScanAccounts.js @@ -17,6 +17,7 @@ const cmd: Command = createCommand( 'libcoreScanAccounts', ({ devicePath, currencyId }) => Observable.create(o => { + let unsubscribed = false // TODO scanAccountsOnDevice should directly return a Observable so we just have to pass-in withLibcore(core => scanAccountsOnDevice({ @@ -26,6 +27,7 @@ const cmd: Command = createCommand( onAccountScanned: account => { o.next(account) }, + isUnsubscribed: () => unsubscribed, }).then( () => { o.complete() @@ -37,7 +39,7 @@ const cmd: Command = createCommand( ) function unsubscribe() { - // FIXME not implemented + unsubscribed = true } return unsubscribe diff --git a/src/components/DeviceBusyIndicator.js b/src/components/DeviceBusyIndicator.js index 47f45b99..7ecfbe27 100644 --- a/src/components/DeviceBusyIndicator.js +++ b/src/components/DeviceBusyIndicator.js @@ -14,10 +14,10 @@ const Indicator = styled.div` ` // NB this is done like this to be extremely performant. we don't want redux for this.. -const perPaths = {} +let globalBusy = false const instances = [] -export const onSetDeviceBusy = (path, busy) => { - perPaths[path] = busy +export const onSetDeviceBusy = busy => { + globalBusy = busy instances.forEach(i => i.forceUpdate()) } @@ -30,8 +30,7 @@ class DeviceBusyIndicator extends PureComponent<{}> { instances.splice(i, 1) } render() { - const busy = Object.values(perPaths).reduce((busy, b) => busy || b, false) - return + return } } diff --git a/src/helpers/deviceAccess.js b/src/helpers/deviceAccess.js index 0aa85738..d9c14199 100644 --- a/src/helpers/deviceAccess.js +++ b/src/helpers/deviceAccess.js @@ -1,5 +1,6 @@ // @flow import logger from 'logger' +import throttle from 'lodash/throttle' import type Transport from '@ledgerhq/hw-transport' import TransportNodeHid from '@ledgerhq/hw-transport-node-hid' import { retry } from './promise' @@ -26,10 +27,19 @@ let busy = false TransportNodeHid.setListenDevicesPollingSkip(() => busy) +const refreshBusyUIState = throttle(() => { + process.send({ + type: 'setDeviceBusy', + busy, + }) +}, 100) + export const withDevice: WithDevice = devicePath => job => { const p = queue.then(async () => { busy = true + refreshBusyUIState() try { + // FIXME: remove this retry const t = await retry(() => TransportNodeHid.open(devicePath), { maxRetry: 1 }) t.setDebugMode(logger.apdu) try { @@ -40,6 +50,7 @@ export const withDevice: WithDevice = devicePath => job => { } } finally { busy = false + refreshBusyUIState() } }) diff --git a/src/helpers/libcore.js b/src/helpers/libcore.js index db4dbe15..7ca018b3 100644 --- a/src/helpers/libcore.js +++ b/src/helpers/libcore.js @@ -36,65 +36,63 @@ type Props = { devicePath: string, currencyId: string, onAccountScanned: AccountRaw => void, + isUnsubscribed: () => boolean, } -export function scanAccountsOnDevice(props: Props): Promise { - const { devicePath, currencyId, onAccountScanned, core } = props +export async function scanAccountsOnDevice(props: Props): Promise { + const { devicePath, currencyId, onAccountScanned, core, isUnsubscribed } = props const currency = getCryptoCurrencyById(currencyId) - return withDevice(devicePath)(async transport => { - const hwApp = new Btc(transport) + const commonParams = { + core, + currencyId, + onAccountScanned, + devicePath, + isUnsubscribed, + } - const commonParams = { - core, - currencyId, - onAccountScanned, - hwApp, - } + let allAccounts = [] - let allAccounts = [] + const nonSegwitAccounts = await scanAccountsOnDeviceBySegwit({ + ...commonParams, + showNewAccount: !!SHOW_LEGACY_NEW_ACCOUNT || !currency.supportsSegwit, + isSegwit: false, + isUnsplit: false, + }) + allAccounts = allAccounts.concat(nonSegwitAccounts) - const nonSegwitAccounts = await scanAccountsOnDeviceBySegwit({ + if (currency.supportsSegwit) { + const segwitAccounts = await scanAccountsOnDeviceBySegwit({ ...commonParams, - showNewAccount: !!SHOW_LEGACY_NEW_ACCOUNT || !currency.supportsSegwit, - isSegwit: false, + showNewAccount: true, + isSegwit: true, isUnsplit: false, }) - allAccounts = allAccounts.concat(nonSegwitAccounts) + allAccounts = allAccounts.concat(segwitAccounts) + } + + // TODO: put that info inside currency itself + if (currencyId in SPLITTED_CURRENCIES) { + const splittedAccounts = await scanAccountsOnDeviceBySegwit({ + ...commonParams, + isSegwit: false, + showNewAccount: false, + isUnsplit: true, + }) + allAccounts = allAccounts.concat(splittedAccounts) if (currency.supportsSegwit) { const segwitAccounts = await scanAccountsOnDeviceBySegwit({ ...commonParams, - showNewAccount: true, - isSegwit: true, - isUnsplit: false, - }) - allAccounts = allAccounts.concat(segwitAccounts) - } - - // TODO: put that info inside currency itself - if (currencyId in SPLITTED_CURRENCIES) { - const splittedAccounts = await scanAccountsOnDeviceBySegwit({ - ...commonParams, - isSegwit: false, showNewAccount: false, isUnsplit: true, + isSegwit: true, }) - allAccounts = allAccounts.concat(splittedAccounts) - - if (currency.supportsSegwit) { - const segwitAccounts = await scanAccountsOnDeviceBySegwit({ - ...commonParams, - showNewAccount: false, - isUnsplit: true, - isSegwit: true, - }) - allAccounts = allAccounts.concat(segwitAccounts) - } + allAccounts = allAccounts.concat(segwitAccounts) } + } - return allAccounts - }) + return allAccounts } function encodeWalletName({ @@ -114,17 +112,19 @@ function encodeWalletName({ async function scanAccountsOnDeviceBySegwit({ core, - hwApp, + devicePath, currencyId, onAccountScanned, + isUnsubscribed, isSegwit, isUnsplit, showNewAccount, }: { core: *, - hwApp: Object, + devicePath: string, currencyId: string, onAccountScanned: AccountRaw => void, + isUnsubscribed: () => boolean, isSegwit: boolean, // FIXME all segwit to change to 'purpose' showNewAccount: boolean, isUnsplit: boolean, @@ -135,7 +135,11 @@ async function scanAccountsOnDeviceBySegwit({ const path = `${isSegwit ? '49' : '44'}'/${coinType}'` - const { publicKey } = await hwApp.getWalletPublicKey(path, false, isSegwit) + const { publicKey } = await withDevice(devicePath)(async transport => + new Btc(transport).getWalletPublicKey(path, false, isSegwit), + ) + + if (isUnsubscribed()) return [] const walletName = encodeWalletName({ publicKey, currencyId, isSegwit, isUnsplit }) @@ -148,7 +152,7 @@ async function scanAccountsOnDeviceBySegwit({ const accounts = await scanNextAccount({ core, wallet, - hwApp, + devicePath, currencyId, accountsCount, accountIndex: 0, @@ -157,6 +161,7 @@ async function scanAccountsOnDeviceBySegwit({ isSegwit, isUnsplit, showNewAccount, + isUnsubscribed, }) return accounts @@ -164,12 +169,14 @@ async function scanAccountsOnDeviceBySegwit({ const hexToBytes = str => Array.from(Buffer.from(str, 'hex')) -const createAccount = async (wallet, hwApp) => { +const createAccount = async (wallet, devicePath) => { const accountCreationInfos = await wallet.getNextAccountCreationInfo() await accountCreationInfos.derivations.reduce( (promise, derivation) => promise.then(async () => { - const { publicKey, chainCode } = await hwApp.getWalletPublicKey(derivation) + const { publicKey, chainCode } = await withDevice(devicePath)(async transport => + new Btc(transport).getWalletPublicKey(derivation), + ) accountCreationInfos.publicKeys.push(hexToBytes(publicKey)) accountCreationInfos.chainCodes.push(hexToBytes(chainCode)) }), @@ -222,7 +229,7 @@ async function scanNextAccount(props: { // $FlowFixMe wallet: NJSWallet, core: *, - hwApp: Object, + devicePath: string, currencyId: string, accountsCount: number, accountIndex: number, @@ -231,11 +238,12 @@ async function scanNextAccount(props: { isSegwit: boolean, isUnsplit: boolean, showNewAccount: boolean, + isUnsubscribed: () => boolean, }): Promise { const { core, wallet, - hwApp, + devicePath, currencyId, accountsCount, accountIndex, @@ -244,6 +252,7 @@ async function scanNextAccount(props: { isSegwit, isUnsplit, showNewAccount, + isUnsubscribed, } = props // create account only if account has not been scanned yet @@ -252,13 +261,17 @@ async function scanNextAccount(props: { const njsAccount = hasBeenScanned ? await wallet.getAccount(accountIndex) - : await createAccount(wallet, hwApp) + : await createAccount(wallet, devicePath) + + if (isUnsubscribed()) return [] const shouldSyncAccount = true // TODO: let's sync everytime. maybe in the future we can optimize. if (shouldSyncAccount) { await coreSyncAccount(core, njsAccount) } + if (isUnsubscribed()) return [] + const query = njsAccount.queryOperations() const ops = await query.complete().execute() @@ -273,6 +286,8 @@ async function scanNextAccount(props: { ops, }) + if (isUnsubscribed()) return [] + const isEmpty = ops.length === 0 if (!isEmpty || showNewAccount) { diff --git a/src/helpers/withLibcore.js b/src/helpers/withLibcore.js index f1fe3fd4..ae33d7d1 100644 --- a/src/helpers/withLibcore.js +++ b/src/helpers/withLibcore.js @@ -14,7 +14,8 @@ export default async function withLibcore(job: Job): Promise { busy: true, }) } - return job(core) + const res = await job(core) + return res } finally { if (--counter === 0) { process.send({ diff --git a/src/renderer/events.js b/src/renderer/events.js index 63d266f2..5b0862a5 100644 --- a/src/renderer/events.js +++ b/src/renderer/events.js @@ -105,8 +105,8 @@ export default ({ store }: { store: Object }) => { onSetLibcoreBusy(busy) }) - ipcRenderer.on('setDeviceBusy', (event: any, { busy, devicePath }) => { - onSetDeviceBusy(devicePath, busy) + ipcRenderer.on('setDeviceBusy', (event: any, { busy }) => { + onSetDeviceBusy(busy) }) }