Merge pull request #118 from trezor/fix/discovery

Refactoring/discovery

src/actions/DiscoveryActions.js

@@ -5,8 +5,15 @@ import EthereumjsUtil from 'ethereumjs-util';
 import * as DISCOVERY from 'actions/constants/discovery';
 import * as ACCOUNT from 'actions/constants/account';
 import * as NOTIFICATION from 'actions/constants/notification';
+
 import type {
-    ThunkAction, AsyncAction, PromiseAction, Action, GetState, Dispatch, TrezorDevice,
+    ThunkAction,
+    AsyncAction,
+    PromiseAction,
+    PayloadAction,
+    GetState,
+    Dispatch,
+    TrezorDevice,
 } from 'flowtype';
 import type { Discovery, State } from 'reducers/DiscoveryReducer';
 import * as BlockchainActions from './BlockchainActions';
@@ -26,11 +33,6 @@ export type DiscoveryWaitingAction = {
     network: string
 }
 
-export type DiscoveryStopAction = {
-    type: typeof DISCOVERY.STOP,
-    device: TrezorDevice
-}
-
 export type DiscoveryCompleteAction = {
     type: typeof DISCOVERY.COMPLETE,
     device: TrezorDevice,
@@ -40,12 +42,28 @@ export type DiscoveryCompleteAction = {
 export type DiscoveryAction = {
     type: typeof DISCOVERY.FROM_STORAGE,
     payload: State
+} | {
+    type: typeof DISCOVERY.STOP,
+    device: TrezorDevice
 } | DiscoveryStartAction
   | DiscoveryWaitingAction
-  | DiscoveryStopAction
   | DiscoveryCompleteAction;
 
-export const start = (device: TrezorDevice, network: string, ignoreCompleted?: boolean): ThunkAction => (dispatch: Dispatch, getState: GetState): void => {
+// There are multiple async methods during discovery process (trezor-connect and blockchain actions)
+// This method will check after each of async action if process was interrupted (for example by network change or device disconnect)
+const isProcessInterrupted = (process?: Discovery): PayloadAction<boolean> => (dispatch: Dispatch, getState: GetState): boolean => {
+    if (!process) {
+        return false;
+    }
+    const { deviceState, network } = process;
+    const discoveryProcess: ?Discovery = getState().discovery.find(d => d.deviceState === deviceState && d.network === network);
+    if (!discoveryProcess) return false;
+    return discoveryProcess.interrupted;
+};
+
+// Private action
+// Called from "this.begin", "this.restore", "this.addAccount"
+const start = (device: TrezorDevice, network: string, ignoreCompleted?: boolean): ThunkAction => (dispatch: Dispatch, getState: GetState): void => {
     const selected = getState().wallet.selectedDevice;
     if (!selected) {
         // TODO: throw error
@@ -150,8 +168,10 @@ const begin = (device: TrezorDevice, network: string): AsyncAction => async (dis
     }
 
     // check for interruption
-    const discoveryProcess: ?Discovery = getState().discovery.find(d => d.deviceState === device.state && d.network === network);
-    if (discoveryProcess && discoveryProcess.interrupted) return;
+    // corner case: DISCOVERY.START wasn't called yet, but Discovery exists in reducer created by DISCOVERY.WAITING_FOR_DEVICE action
+    // this is why we need to get process instance directly from reducer
+    const discoveryProcess = getState().discovery.find(d => d.deviceState === device.state && d.network === network);
+    if (dispatch(isProcessInterrupted(discoveryProcess))) return;
 
     const basePath: Array<number> = response.payload.path;
 
@@ -181,7 +201,8 @@ const discoverAccount = (device: TrezorDevice, discoveryProcess: Discovery): Asy
     // TODO: check if address was created before
     try {
         const account = await dispatch(BlockchainActions.discoverAccount(device, ethAddress, network));
-        if (discoveryProcess.interrupted) return;
+        // check for interruption
+        if (dispatch(isProcessInterrupted(discoveryProcess))) return;
 
         // const accountIsEmpty = account.transactions <= 0 && account.nonce <= 0 && account.balance === '0';
         const accountIsEmpty = account.nonce <= 0 && account.balance === '0';
@@ -248,7 +269,7 @@ const finish = (device: TrezorDevice, discoveryProcess: Discovery): AsyncAction
 
     await dispatch(BlockchainActions.subscribe(discoveryProcess.network));
 
-    if (discoveryProcess.interrupted) return;
+    if (dispatch(isProcessInterrupted(discoveryProcess))) return;
 
     dispatch({
         type: DISCOVERY.COMPLETE,
@@ -262,34 +283,43 @@ export const reconnect = (network: string): PromiseAction<void> => async (dispat
     dispatch(restore());
 };
 
+// Called after DEVICE.CONNECT ('trezor-connect') or CONNECT.AUTH_DEVICE actions in WalletService
+// OR after BlockchainSubscribe in this.reconnect
 export const restore = (): ThunkAction => (dispatch: Dispatch, getState: GetState): void => {
+    // check if current url has "network" parameter
+    const urlParams = getState().router.location.state;
+    if (!urlParams.network) return;
+
+    // make sure that "selectedDevice" exists
     const selected = getState().wallet.selectedDevice;
-    if (selected && selected.connected && selected.features) {
-        const discoveryProcess: ?Discovery = getState().discovery.find(d => d.deviceState === selected.state && (d.interrupted || d.waitingForDevice || d.waitingForBlockchain));
-        if (discoveryProcess) {
-            dispatch(start(selected, discoveryProcess.network));
-        }
+    if (!selected) return;
+
+    // find discovery process for requested network
+    const discoveryProcess: ?Discovery = getState().discovery.find(d => d.deviceState === selected.state && d.network === urlParams.network);
+
+    // if there was no process befor OR process was interrupted/waiting
+    const shouldStart = !discoveryProcess || (discoveryProcess.interrupted || discoveryProcess.waitingForDevice || discoveryProcess.waitingForBlockchain);
+    if (shouldStart) {
+        dispatch(start(selected, urlParams.network));
     }
 };
 
-// TODO: rename method to something intuitive
-// there is no discovery process but it should be
-// this is possible race condition when "network" was changed in url but device was not authenticated yet
-// try to start discovery after CONNECT.AUTH_DEVICE action
-export const check = (): ThunkAction => (dispatch: Dispatch, getState: GetState): void => {
-    const selected = getState().wallet.selectedDevice;
-    if (!selected) return;
+export const stop = (): ThunkAction => (dispatch: Dispatch, getState: GetState): void => {
+    const device: ?TrezorDevice = getState().wallet.selectedDevice;
+    if (!device) return;
 
-    const urlParams = getState().router.location.state;
-    if (urlParams.network) {
-        const discoveryProcess: ?Discovery = getState().discovery.find(d => d.deviceState === selected.state && d.network === urlParams.network);
-        if (!discoveryProcess) {
-            dispatch(start(selected, urlParams.network));
-        }
+    // get all uncompleted discovery processes which assigned to selected device
+    const discoveryProcesses = getState().discovery.filter(d => d.deviceState === device.state && !d.completed);
+    if (discoveryProcesses.length > 0) {
+        dispatch({
+            type: DISCOVERY.STOP,
+            device,
+        });
     }
 };
 
-export const stop = (device: TrezorDevice): Action => ({
-    type: DISCOVERY.STOP,
-    device,
-});
+export const addAccount = (): ThunkAction => (dispatch: Dispatch, getState: GetState): void => {
+    const selected = getState().wallet.selectedDevice;
+    if (!selected) return;
+    dispatch(start(selected, getState().router.location.state.network, true));
+};

src/actions/RouterActions.js

@@ -250,10 +250,14 @@ export const selectDevice = (device: TrezorDevice | Device): ThunkAction => (dis
 /*
 * Redirect to first device or landing page
 */
-export const selectFirstAvailableDevice = (): ThunkAction => (dispatch: Dispatch): void => {
+export const selectFirstAvailableDevice = (): ThunkAction => (dispatch: Dispatch, getState: GetState): void => {
     const url = dispatch(getFirstAvailableDeviceUrl());
     if (url) {
-        dispatch(goto(url));
+        const currentParams = getState().router.location.state;
+        const requestedParams = dispatch(pathToParams(url));
+        if (currentParams.device !== requestedParams.device || currentParams.deviceInstance !== requestedParams.deviceInstance) {
+            dispatch(goto(url));
+        }
     } else {
         dispatch(gotoLandingPage());
     }

src/actions/SelectedAccountActions.js

@@ -74,13 +74,13 @@ const getAccountStatus = (state: State, selectedAccount: SelectedAccountState):
 
     // account not found (yet). checking why...
     if (!account) {
-        if (!discovery || discovery.waitingForDevice) {
+        if (!discovery || (discovery.waitingForDevice || discovery.interrupted)) {
             if (device.connected) {
                 // case 1: device is connected but discovery not started yet (probably waiting for auth)
                 if (device.available) {
                     return {
                         type: 'info',
-                        title: 'Loading accounts...',
+                        title: 'Authenticating device...',
                         shouldRender: false,
                     };
                 }
@@ -156,7 +156,7 @@ const actions = [
     SEND.TX_COMPLETE,
     WALLET.SET_SELECTED_DEVICE,
     WALLET.UPDATE_SELECTED_DEVICE,
-    ...Object.values(ACCOUNT).filter(v => typeof v === 'string' && v !== ACCOUNT.UPDATE_SELECTED_ACCOUNT), // exported values got unwanted "__esModule: true" as first element
+    ...Object.values(ACCOUNT).filter(v => typeof v === 'string' && v !== ACCOUNT.UPDATE_SELECTED_ACCOUNT && v !== ACCOUNT.DISPOSE), // exported values got unwanted "__esModule: true" as first element
     ...Object.values(DISCOVERY).filter(v => typeof v === 'string'),
     ...Object.values(TOKEN).filter(v => typeof v === 'string'),
     ...Object.values(PENDING).filter(v => typeof v === 'string'),

src/actions/TrezorConnectActions.js

@@ -27,7 +27,6 @@ import type {
     Device,
     TrezorDevice,
 } from 'flowtype';
-import * as DiscoveryActions from './DiscoveryActions';
 
 
 export type TrezorConnectAction = {
@@ -202,13 +201,7 @@ export const getSelectedDeviceState = (): AsyncAction => async (dispatch: Dispat
 };
 
 export const deviceDisconnect = (device: Device): AsyncAction => async (dispatch: Dispatch, getState: GetState): Promise<void> => {
-    const selected: ?TrezorDevice = getState().wallet.selectedDevice;
-
     if (device.features) {
-        if (selected && selected.features && selected.features.device_id === device.features.device_id) {
-            dispatch(DiscoveryActions.stop(selected));
-        }
-
         const instances = getState().devices.filter(d => d.features && device.features && d.state && !d.remember && d.features.device_id === device.features.device_id);
         if (instances.length > 0) {
             dispatch({
@@ -224,17 +217,6 @@ export const deviceDisconnect = (device: Device): AsyncAction => async (dispatch
     }
 };
 
-export const coinChanged = (network: ?string): ThunkAction => (dispatch: Dispatch, getState: GetState): void => {
-    const selected: ?TrezorDevice = getState().wallet.selectedDevice;
-    if (!selected) return;
-
-    dispatch(DiscoveryActions.stop(selected));
-
-    if (network) {
-        dispatch(DiscoveryActions.start(selected, network));
-    }
-};
-
 export function reload(): AsyncAction {
     return async (): Promise<void> => {
     };
@@ -301,12 +283,3 @@ export const duplicateDevice = (device: TrezorDevice): AsyncAction => async (dis
         device: { ...device, ...extended },
     });
 };
-
-
-export function addAccount(): ThunkAction {
-    return (dispatch: Dispatch, getState: GetState): void => {
-        const selected = getState().wallet.selectedDevice;
-        if (!selected) return;
-        dispatch(DiscoveryActions.start(selected, getState().router.location.state.network, true)); // TODO: network nicer
-    };
-}

src/actions/WalletActions.js

@@ -2,6 +2,7 @@
 
 import { LOCATION_CHANGE } from 'react-router-redux';
 import { DEVICE } from 'trezor-connect';
+import * as CONNECT from 'actions/constants/TrezorConnect';
 import * as WALLET from 'actions/constants/wallet';
 import * as reducerUtils from 'reducers/utils';
 
@@ -84,6 +85,7 @@ export const clearUnavailableDevicesData = (prevState: State, device: Device): T
 // other actions will be ignored
 const actions = [
     LOCATION_CHANGE,
+    CONNECT.AUTH_DEVICE,
     ...Object.values(DEVICE).filter(v => typeof v === 'string'),
 ];
 

src/reducers/DevicesReducer.js

@@ -187,10 +187,9 @@ const changeDevice = (state: State, device: Device | TrezorDevice, extended: Obj
 const authDevice = (state: State, device: TrezorDevice, deviceState: string): State => {
     const affectedDevice: ?TrezorDevice = state.find(d => d.path === device.path && d.instance === device.instance);
     // device could already have own state from trezor-connect, do not override it
-    if (affectedDevice && !affectedDevice.state) {
+    if (affectedDevice && !affectedDevice.state && affectedDevice.type === 'acquired') {
         const otherDevices: Array<TrezorDevice> = state.filter(d => d !== affectedDevice);
-        affectedDevice.state = deviceState;
-        return otherDevices.concat([affectedDevice]);
+        return otherDevices.concat([{ ...affectedDevice, state: deviceState }]);
     }
     return state;
 };

src/reducers/DiscoveryReducer.js

@@ -12,7 +12,6 @@ import type { Action, TrezorDevice } from 'flowtype';
 import type {
     DiscoveryStartAction,
     DiscoveryWaitingAction,
-    DiscoveryStopAction,
     DiscoveryCompleteAction,
 } from 'actions/DiscoveryActions';
 
@@ -40,8 +39,8 @@ const findIndex = (state: State, network: string, deviceState: string): number =
 const start = (state: State, action: DiscoveryStartAction): State => {
     const deviceState: string = action.device.state || '0';
     const hdKey: HDKey = new HDKey();
-    hdKey.publicKey = new Buffer(action.publicKey, 'hex');
-    hdKey.chainCode = new Buffer(action.chainCode, 'hex');
+    hdKey.publicKey = Buffer.from(action.publicKey, 'hex');
+    hdKey.chainCode = Buffer.from(action.chainCode, 'hex');
     const instance: Discovery = {
         network: action.network,
         publicKey: action.publicKey,
@@ -90,16 +89,17 @@ const clear = (state: State, devices: Array<TrezorDevice>): State => {
     return newState;
 };
 
-const stop = (state: State, action: DiscoveryStopAction): State => {
-    const newState: State = [...state];
-    return newState.map((d: Discovery) => {
-        if (d.deviceState === action.device.state && !d.completed) {
-            d.interrupted = true;
-            d.waitingForDevice = false;
-            d.waitingForBlockchain = false;
-        }
-        return d;
-    });
+const stop = (state: State, device: TrezorDevice): State => {
+    const affectedProcesses = state.filter(d => d.deviceState === device.state && !d.completed);
+    const otherProcesses = state.filter(d => affectedProcesses.indexOf(d) === -1);
+    const changedProcesses = affectedProcesses.map(d => ({
+        ...d,
+        interrupted: true,
+        waitingForDevice: false,
+        waitingForBlockchain: false,
+    }));
+
+    return otherProcesses.concat(changedProcesses);
 };
 
 const waitingForDevice = (state: State, action: DiscoveryWaitingAction): State => {
@@ -163,7 +163,7 @@ export default function discovery(state: State = initialState, action: Action):
         case ACCOUNT.CREATE:
             return accountCreate(state, action.payload);
         case DISCOVERY.STOP:
-            return stop(state, action);
+            return stop(state, action.device);
         case DISCOVERY.COMPLETE:
             return complete(state, action);
         case DISCOVERY.WAITING_FOR_DEVICE:
@@ -173,8 +173,8 @@ export default function discovery(state: State = initialState, action: Action):
         case DISCOVERY.FROM_STORAGE:
             return action.payload.map((d) => {
                 const hdKey: HDKey = new HDKey();
-                hdKey.publicKey = new Buffer(d.publicKey, 'hex');
-                hdKey.chainCode = new Buffer(d.chainCode, 'hex');
+                hdKey.publicKey = Buffer.from(d.publicKey, 'hex');
+                hdKey.chainCode = Buffer.from(d.chainCode, 'hex');
                 return {
                     ...d,
                     hdKey,

src/reducers/SelectedAccountReducer.js

@@ -38,6 +38,8 @@ export const initialState: State = {
 
 export default (state: State = initialState, action: Action): State => {
     switch (action.type) {
+        case ACCOUNT.DISPOSE:
+            return initialState;
         case ACCOUNT.UPDATE_SELECTED_ACCOUNT:
             return action.payload;
         default:

src/services/TrezorConnectService.js

@@ -4,7 +4,6 @@ import {
     TRANSPORT, DEVICE, BLOCKCHAIN,
 } from 'trezor-connect';
 import * as TrezorConnectActions from 'actions/TrezorConnectActions';
-import * as DiscoveryActions from 'actions/DiscoveryActions';
 import * as BlockchainActions from 'actions/BlockchainActions';
 import * as RouterActions from 'actions/RouterActions';
 import * as ModalActions from 'actions/ModalActions';
@@ -55,14 +54,9 @@ const TrezorConnectService: Middleware = (api: MiddlewareAPI) => (next: Middlewa
             api.dispatch(RouterActions.selectFirstAvailableDevice());
         }
     } else if (action.type === DEVICE.CONNECT || action.type === DEVICE.CONNECT_UNACQUIRED) {
-        api.dispatch(DiscoveryActions.restore());
         api.dispatch(ModalActions.onDeviceConnect(action.device));
-    } else if (action.type === CONNECT.AUTH_DEVICE) {
-        api.dispatch(DiscoveryActions.check());
     } else if (action.type === CONNECT.DUPLICATE) {
         api.dispatch(RouterActions.selectDevice(action.device));
-    } else if (action.type === CONNECT.COIN_CHANGED) {
-        api.dispatch(TrezorConnectActions.coinChanged(action.payload.network));
     } else if (action.type === BLOCKCHAIN.BLOCK) {
         api.dispatch(BlockchainActions.onBlockMined(action.payload.coin));
     } else if (action.type === BLOCKCHAIN.NOTIFICATION) {

src/services/WalletService.js

@@ -10,6 +10,7 @@ import * as LocalStorageActions from 'actions/LocalStorageActions';
 import * as TrezorConnectActions from 'actions/TrezorConnectActions';
 import * as SelectedAccountActions from 'actions/SelectedAccountActions';
 import * as SendFormActionActions from 'actions/SendFormActions';
+import * as DiscoveryActions from 'actions/DiscoveryActions';
 
 import type {
     Middleware,
@@ -77,6 +78,12 @@ const WalletService: Middleware = (api: MiddlewareAPI) => (next: MiddlewareDispa
                     network: currentLocation.state.network,
                 },
             });
+
+            // try to stop currently running discovery on previous network
+            api.dispatch(DiscoveryActions.stop());
+
+            // try to start new discovery on new network
+            api.dispatch(DiscoveryActions.restore());
         }
 
         // watch for account change
@@ -95,6 +102,18 @@ const WalletService: Middleware = (api: MiddlewareAPI) => (next: MiddlewareDispa
             // if "selectedAccount" didn't change observe send form props changes
             api.dispatch(SendFormActionActions.observe(prevState, action));
         }
+    } else if (action.type === CONNECT.AUTH_DEVICE) {
+        // selected device did changed
+        // try to restore discovery after device authentication
+        api.dispatch(DiscoveryActions.restore());
+    }
+
+    // even if "selectedDevice" didn't change because it was updated on DEVICE.CHANGED before DEVICE.CONNECT action
+    // try to restore discovery
+    if (action.type === DEVICE.CONNECT) {
+        api.dispatch(DiscoveryActions.restore());
+    } else if (action.type === DEVICE.DISCONNECT) {
+        api.dispatch(DiscoveryActions.stop());
     }
 
     return action;

src/views/Wallet/components/LeftNavigation/Container.js

@@ -5,6 +5,7 @@ import { connect } from 'react-redux';
 import { withRouter } from 'react-router-dom';
 
 import * as TrezorConnectActions from 'actions/TrezorConnectActions';
+import * as DiscoveryActions from 'actions/DiscoveryActions';
 import * as RouterActions from 'actions/RouterActions';
 import type { MapStateToProps, MapDispatchToProps } from 'react-redux';
 import type { State, Dispatch } from 'flowtype';
@@ -31,7 +32,7 @@ const mapStateToProps: MapStateToProps<State, OwnProps, StateProps> = (state: St
 
 const mapDispatchToProps: MapDispatchToProps<Dispatch, OwnProps, DispatchProps> = (dispatch: Dispatch): DispatchProps => ({
     toggleDeviceDropdown: bindActionCreators(toggleDeviceDropdown, dispatch),
-    addAccount: bindActionCreators(TrezorConnectActions.addAccount, dispatch),
+    addAccount: bindActionCreators(DiscoveryActions.addAccount, dispatch),
     acquireDevice: bindActionCreators(TrezorConnectActions.acquire, dispatch),
     forgetDevice: bindActionCreators(TrezorConnectActions.forget, dispatch),
     duplicateDevice: bindActionCreators(TrezorConnectActions.duplicateDevice, dispatch),

src/views/Wallet/components/LeftNavigation/components/common.js

@@ -1,5 +1,6 @@
 /* @flow */
 import * as TrezorConnectActions from 'actions/TrezorConnectActions';
+import * as DiscoveryActions from 'actions/DiscoveryActions';
 import * as RouterActions from 'actions/RouterActions';
 import { toggleDeviceDropdown } from 'actions/WalletActions';
 import type { State } from 'flowtype';
@@ -18,7 +19,7 @@ export type StateProps = {
 
 export type DispatchProps = {
     toggleDeviceDropdown: typeof toggleDeviceDropdown,
-    addAccount: typeof TrezorConnectActions.addAccount,
+    addAccount: typeof DiscoveryActions.addAccount,
     acquireDevice: typeof TrezorConnectActions.acquire,
     forgetDevice: typeof TrezorConnectActions.forget,
     duplicateDevice: typeof TrezorConnectActions.duplicateDevice,