Merge pull request #1100 from gre/do-not-lock-device-during-whole-scanning

Prevent locks to happen during scanning

src/commands/libcoreScanAccounts.js

@@ -17,6 +17,7 @@ const cmd: Command<Input, Result> = 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<Input, Result> = createCommand(
           onAccountScanned: account => {
             o.next(account)
           },
+          isUnsubscribed: () => unsubscribed,
         }).then(
           () => {
             o.complete()
@@ -37,7 +39,7 @@ const cmd: Command<Input, Result> = createCommand(
       )
 
       function unsubscribe() {
-        // FIXME not implemented
+        unsubscribed = true
       }
 
       return unsubscribe

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 <Indicator busy={busy} />
+    return <Indicator busy={globalBusy} />
   }
 }
 

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()
     }
   })
 

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<AccountRaw[]> {
-  const { devicePath, currencyId, onAccountScanned, core } = props
+export async function scanAccountsOnDevice(props: Props): Promise<AccountRaw[]> {
+  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<AccountRaw[]> {
   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) {

src/helpers/withLibcore.js

@@ -14,7 +14,8 @@ export default async function withLibcore<A>(job: Job<A>): Promise<A> {
         busy: true,
       })
     }
-    return job(core)
+    const res = await job(core)
+    return res
   } finally {
     if (--counter === 0) {
       process.send({

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)
     })
   }