Skip to content

Commit

Permalink
Ledger: Avoid confirming wallet address at each operation #127
Browse files Browse the repository at this point in the history
- bind ledger communication to the open/close events
- unify naming of the transaction dialog
  • Loading branch information
TobiaszCudnik committed Nov 9, 2018
1 parent 6bc8af8 commit 4974b40
Show file tree
Hide file tree
Showing 7 changed files with 68 additions and 38 deletions.
4 changes: 2 additions & 2 deletions src/containers/wallet/AddSecondPassphraseDialog.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { reaction, IReactionDisposer } from 'mobx';
import { inject, observer } from 'mobx-react';
import { RouterStore } from 'mobx-router-rise';
import * as React from 'react';
import TransactionDialog from './TransactionDialog';
import ConfirmTransactionDialog from './ConfirmTransactionDialog';
import AddSecondPassphraseDialogContent from '../../components/content/AddSecondPassphraseDialogContent';
import { accountSettingsPassphraseRoute } from '../../routes';
import RootStore, { RouteLink } from '../../stores/root';
Expand Down Expand Up @@ -120,7 +120,7 @@ class AddSecondPassphraseDialog extends React.Component<Props, State> {
const canGoBack = step !== 'form';

return (
<TransactionDialog
<ConfirmTransactionDialog
open={this.isOpen}
account={account}
transaction={
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,14 +49,15 @@ interface State {
@inject('walletStore')
@inject('ledgerStore')
@observer
class TransactionDialog extends React.Component<Props, State> {
class ConfirmTransactionDialog extends React.Component<Props, State> {
state: State = {
transaction: null,
step: 'confirm',
signedTx: null,
sendError: ''
};

open: boolean = false;
private ledger: LedgerChannel;
private lastLedgerSignId = 0;
private disposeLedgerMonitor: null | IReactionDisposer = null;
Expand Down Expand Up @@ -101,7 +102,20 @@ class TransactionDialog extends React.Component<Props, State> {
}
}

handleClose = (ev: React.SyntheticEvent<{}>) => {
onClose = (ev: React.SyntheticEvent<{}>) => {
// ledger part
if (!this.open) {
return;
}
this.open = false;
if (this.disposeLedgerMonitor !== null) {
this.disposeLedgerMonitor();
this.disposeLedgerMonitor = null;
}

this.ledger.close();

// navigation part
const { store, onClose, closeLink } = this.injected;
if (onClose) {
onClose(ev);
Expand Down Expand Up @@ -215,7 +229,11 @@ class TransactionDialog extends React.Component<Props, State> {
return walletStore.fees.get(feeMap[transaction.kind])!;
}

componentDidMount() {
onOpen() {
if (this.open) {
return;
}
this.open = true;
const { ledgerStore } = this.injected;
this.ledger = ledgerStore.openChannel();

Expand All @@ -225,15 +243,6 @@ class TransactionDialog extends React.Component<Props, State> {
);
}

componentWillUnmount() {
if (this.disposeLedgerMonitor !== null) {
this.disposeLedgerMonitor();
this.disposeLedgerMonitor = null;
}

this.ledger.close();
}

canSignOnLedger = () => {
const { transaction, account, ledgerStore } = this.injected;
const { step } = this.state;
Expand Down Expand Up @@ -293,9 +302,14 @@ class TransactionDialog extends React.Component<Props, State> {
render() {
const { open } = this.injected;

return <Dialog open={open} {...this.dialogProps} />;
if (open) {
this.onOpen();
}

return <Dialog open={open} {...this.dialogProps} onClose={this.onClose} />;
}

// TODO simply, describe
get dialogProps(): Pick<
DialogProps,
'onClose' | 'closeLink' | 'onNavigateBack' | 'navigateBackLink' | 'children'
Expand All @@ -308,17 +322,14 @@ class TransactionDialog extends React.Component<Props, State> {
navigateBackLink
} = this.injected;

// TODO comment needed
if (!transaction) {
const { children } = this.injected;
return { onClose, closeLink, onNavigateBack, navigateBackLink, children };
}

const closeProps: Pick<DialogProps, 'onClose' | 'closeLink'> = {};
if (closeLink) {
closeProps.closeLink = closeLink;
} else {
closeProps.onClose = this.handleClose;
}
closeProps.onClose = this.onClose;

const backProps: Pick<
DialogProps,
Expand Down Expand Up @@ -424,7 +435,7 @@ class TransactionDialog extends React.Component<Props, State> {
type="broadcast-failed"
reason={sendError}
onRetry={canRetry ? this.handleRetryTransaction : undefined}
onClose={this.handleClose}
onClose={this.onClose}
/>
</ConfirmTransactionDialogContent>
);
Expand All @@ -443,7 +454,7 @@ class TransactionDialog extends React.Component<Props, State> {
>
<ConfirmTxStatusFooter
type="broadcast-succeeded"
onClose={this.handleClose}
onClose={this.onClose}
/>
</ConfirmTransactionDialogContent>
);
Expand Down Expand Up @@ -473,4 +484,4 @@ class TransactionDialog extends React.Component<Props, State> {
}
}

export default TransactionDialog;
export default ConfirmTransactionDialog;
4 changes: 2 additions & 2 deletions src/containers/wallet/RegisterDelegateDialog.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { reaction, IReactionDisposer } from 'mobx';
import { inject, observer } from 'mobx-react';
import { RouterStore } from 'mobx-router-rise';
import * as React from 'react';
import TransactionDialog from './TransactionDialog';
import ConfirmTransactionDialog from './ConfirmTransactionDialog';
import RegisterDelegateDialogContent from '../../components/content/RegisterDelegateDialogContent';
import { accountSettingsDelegateRoute } from '../../routes';
import RootStore, { RouteLink } from '../../stores/root';
Expand Down Expand Up @@ -127,7 +127,7 @@ class RegisterDelegateDialog extends React.Component<Props, State> {
const canGoBack = step !== 'form';

return (
<TransactionDialog
<ConfirmTransactionDialog
open={this.isOpen}
account={account}
transaction={transaction ? {
Expand Down
4 changes: 2 additions & 2 deletions src/containers/wallet/SendCoinsDialog.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import AccountStore from '../../stores/account';
import AddressBookStore from '../../stores/addressBook';
import WalletStore from '../../stores/wallet';
import { RawAmount } from '../../utils/amounts';
import TransactionDialog from './TransactionDialog';
import ConfirmTransactionDialog from './ConfirmTransactionDialog';

interface Props {
account: AccountStore;
Expand Down Expand Up @@ -157,7 +157,7 @@ class SendCoinsDialog extends React.Component<Props, State> {
const canGoBack = step !== 'form';

return (
<TransactionDialog
<ConfirmTransactionDialog
open={this.isOpen}
account={account}
transaction={
Expand Down
33 changes: 25 additions & 8 deletions src/containers/wallet/VerifyLedgerDialog.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,7 @@ class VerifyLedgerDialog extends React.Component<DecoratedProps, State> {
state: State = {
confirmed: false
};
open: boolean = false;
private ledger: LedgerChannel;
private disposeDeviceLoader: null | IReactionDisposer = null;
private countdownId: null | number = null;
Expand All @@ -152,7 +153,11 @@ class VerifyLedgerDialog extends React.Component<DecoratedProps, State> {
return this.injected.account || this.injected.accountStore;
}

componentWillMount() {
onOpen = () => {
if (this.open) {
return;
}
this.open = true;
const { ledgerStore, intl } = this.injected;
this.ledger = ledgerStore.openChannel();

Expand All @@ -168,13 +173,18 @@ class VerifyLedgerDialog extends React.Component<DecoratedProps, State> {
this.handleVerifyLedger();
}

componentWillUnmount() {
onClose = () => {
if (!this.open) {
return;
}
this.open = false;
if (this.disposeDeviceLoader) {
this.disposeDeviceLoader();
this.disposeDeviceLoader = null;
}

this.ledger.close();
this.setState({ confirmed: false });
}

@action
Expand Down Expand Up @@ -212,7 +222,8 @@ class VerifyLedgerDialog extends React.Component<DecoratedProps, State> {
}
}

handleClose = () => {
handleCloseButton = () => {
this.onClose();
this.injected.store.navigateTo(this.injected.navigateBackLink);
}

Expand All @@ -225,16 +236,22 @@ class VerifyLedgerDialog extends React.Component<DecoratedProps, State> {
classes,
ledgerStore
} = this.injected;
const { deviceId } = this.ledger;
const { confirmed } = this.state;
const account = this.account;
let deviceId;
let confirmed;

const isOpen =
open || routerStore.currentView === accountSettingsLedgerRoute;

const account = this.account;
if (isOpen) {
this.onOpen();

deviceId = this.ledger.deviceId;
confirmed = this.state.confirmed;
}

return (
<Dialog open={isOpen} closeLink={navigateBackLink}>
<Dialog open={isOpen} closeLink={navigateBackLink} onClose={this.onClose}>
{!ledgerStore.hasBrowserSupport ? (
<Grid container={true} className={classes.content} spacing={16}>
<Grid item={true} xs={12}>
Expand Down Expand Up @@ -314,7 +331,7 @@ class VerifyLedgerDialog extends React.Component<DecoratedProps, State> {
type="submit"
fullWidth={true}
children={intl.formatMessage(messages.closeDialog)}
onClick={this.handleClose}
onClick={this.handleCloseButton}
/>
</Grid>
</Grid>
Expand Down
4 changes: 2 additions & 2 deletions src/containers/wallet/VoteDelegateDialog.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { inject, observer } from 'mobx-react';
import { RouterStore } from 'mobx-router-rise';
import * as React from 'react';
import { normalizeAddress } from '../../utils/utils';
import TransactionDialog from './TransactionDialog';
import ConfirmTransactionDialog from './ConfirmTransactionDialog';
import VoteDelegateDialogContent from '../../components/content/VoteDelegateDialogContent';
import { accountSettingsVoteRoute } from '../../routes';
import RootStore, { RouteLink } from '../../stores/root';
Expand Down Expand Up @@ -279,7 +279,7 @@ class VoteDelegateDialog extends React.Component<Props, State> {
const canGoBack = step !== 'vote';

return (
<TransactionDialog
<ConfirmTransactionDialog
open={this.isOpen}
account={account}
transaction={
Expand Down
4 changes: 3 additions & 1 deletion src/stores/ledger.ts
Original file line number Diff line number Diff line change
Expand Up @@ -411,7 +411,9 @@ class LedgerHub implements LedgerTaskRunner {
const task = new LedgerTask(
null,
handleResponse,
(reason) => handleResponse(null),
() => {},

This comment has been minimized.

Copy link
@roosmaa

roosmaa Nov 9, 2018

Contributor

@TobiaszCudnik what kind of "flooding" are you referring to. Because blindly just removing the error propagation like you've done here probably just creates several more bugs.

This comment has been minimized.

Copy link
@TobiaszCudnik

TobiaszCudnik Nov 9, 2018

Author Contributor

deviceId was switching back and forth. Its better to not have an error handler than to have a buggy one. Fix will follow in #137.

This comment has been minimized.

Copy link
@roosmaa

roosmaa Nov 9, 2018

Contributor

It will only switch back when the device isn't ready to process things. What kind of flooding are you talking about?

// TODO error handler
// (reason) => handleResponse(null),
async () => {
const accountPath = new DposAccount()
.coinIndex(SupportedCoin.RISE)
Expand Down

0 comments on commit 4974b40

Please sign in to comment.