WIP: Add signers #32

justinmoon · 2019-09-13T06:00:11Z

Wallet page nicely displays current and potential signers in tables
Can add signers
Modal for entering Trezor PIN
Some backend improvements

One big problem: activeWallet goes stale after updates to the wallet list. Perhaps it would be better to just keep track of the activeWalletName and have a selector that uses it to looks up activeWallet ...

justinmoon

Some notes

justinmoon · 2019-09-13T06:01:46Z

src/components/AddSigners.tsx

+    pinEntryDevice: null,
+  }
+
+  toggle() {


This method and state.modal needs to be renamed to like toggleDeviceInstructionsModal or something.

Originally I just had 1 modal and the generic names haven't been updated yet ...

justinmoon · 2019-09-13T06:03:38Z

api.py

@@ -14,6 +14,15 @@
 logger = logging.getLogger(__name__)
 CLIENT = None

+def kill_client():


The same instance of trezor.TrezorClient() must prompt and send the pin. Therefore, we implement it as a global variable. But we need to close it when we're done using or we get "device is busy" errors when creating competing instances. Therefore, I close it wherever possible.

justinmoon · 2019-09-13T06:04:04Z

api.py

-        # FIXME: or path???
-        'fingerprint': { 'type': 'string' },  # FIXME: regex
+        # device_id is fingerprint or path
+        'device_id': { 'type': 'string' },  # FIXME: regex


@wbobeirne went with your idea here to accept fingerprint or path

justinmoon · 2019-09-13T06:05:41Z

src/components/AddSigners.tsx

+
+class AddDevice extends React.Component<AddDeviceProps> {
+  handleUnlock(device: Device) {
+    api.promptPin({ path: device.path }).then(this.props.displayPinEntry)


The modal is weird. We want to have HWI prompt pin every time the modal is displayed, but because the modal component is always rendered and just pops in and out of view, we can't prompt pin entry on componentWillMount or something like that ...

I'm sure there's a better way than this ...

Two ways to go about this. One is to make a sub-component that only renders when the modal is open, and use that component's componentDidMount. Alternatively, you can use componentDidUpdate like so:

componentDidUpdate(prevProps: Props) { if (this.props.isOpen && !prevProps.isOpen) { // Do thing on modal open } }

justinmoon · 2019-09-13T06:06:45Z

src/components/AddSigners.tsx

+  modal: boolean;
+  pinModalActive: boolean;
+  // FIXME: keeping track of the device outside the modal sucks
+  pinEntryDevice: Device | null;


This sucks. Since the modal is rendered before a user chooses which device to unlock, we have to keep track of the chosen device in the parent component and pass to the pin entry component. But it works.

This seems pretty reasonable to me, IMO. However, maybe the "cooler" option would be to have a global component rendered way further up in the stack, and to trigger pin entries via redux. There could be a state prop called pinDevice or something like that, and you just call this.props.promptPin(device), it sets it in redux state, and causes that modal to render and take it from there.

justinmoon · 2019-09-13T06:07:38Z

src/components/AddSigners.tsx

+    }));
+  }
+
+  openModal(device: Device) {


Janky code to communicate the device to modal

Could just consolidate the two props into one be informing modal openness from whether or not you have a pin entry device set, i.e. isOpen={!!this.state.pinEntryDevice}

justinmoon · 2019-09-13T06:08:34Z

src/components/DeviceInstructionsModal.tsx

+	isOpen: boolean;
+}
+
+export default class DeviceInstructionsModal extends React.Component<Props> {


Someone is a VSCode noob who haven't figured out how to do indentation properly ...

We can get prettier setup on this sucker to avoid code style issues 💪

justinmoon · 2019-09-13T06:09:44Z

src/components/EnterPinModal.tsx

+  }
+
+  handleMouseDown(digit: number) {
+    if (!this.state.submitting) {


Is this a good way to dispable buttons when they shouldn't be clicked?

Or should I use a disabled=bool HTML attribute?

disabled works pretty nicely on Reactstrap buttons, so I'd go with that. But doing it in both places doesn't hurt either.

justinmoon · 2019-09-13T06:14:27Z

src/store/wallet/selectors.ts

+// This is hard to use 
+export function selectActiveWallet(state: AppState) {
+  const { activeWallet } = state.wallet;
+  // if (activeWallet === null) {


I tried this selector that throws if there is no active wallet. I found it hard to use. React is always trying to render ... this approach needs some way to tell react not to render until wallets are loaded. Also, some way to redirect user through onboarding steps (#1 update settings, #2 create wallet) if wallets are loaded and no active wallet exists.

Yep, you'd want a wrapper or higher order component for that, sorry for the my earlier red herring solution (Which does work in some cases, just not the one here.) Something like:

<RequireWallet component={MyComponent} />

where RequireWallet's render looks like

if (this.props.activeWallet) { return <MyComponent activeWallet={this.props.activeWallet} /> } else { return 'Oh no you need a wallet fam'; }

justinmoon · 2019-09-13T06:15:12Z

src/types/device.ts

 }

+// FIXME: some of these are null at times
+// but I don't really want typescript yelling at me all the time ...


This is weird. I should be saying that some of these properties can be be null. But then typescript relentlessly yells at me in places where I know the properties will exist.

We'd need union types for those. What are the cases where you know it will be null, and know it won't?

wbobeirne

Haven't run yet, but here are some notes!

wbobeirne · 2019-09-13T16:12:41Z

src/components/AddSigners.tsx

+
+class AddDevice extends React.Component<AddDeviceProps> {
+  handleUnlock(device: Device) {
+    api.promptPin({ path: device.path }).then(this.props.displayPinEntry)


Two ways to go about this. One is to make a sub-component that only renders when the modal is open, and use that component's componentDidMount. Alternatively, you can use componentDidUpdate like so:

componentDidUpdate(prevProps: Props) { if (this.props.isOpen && !prevProps.isOpen) { // Do thing on modal open } }

wbobeirne · 2019-09-13T16:14:25Z

src/api/index.ts

+
+  deletePrompt() {
+    return this.request<any>('DELETE', '/prompt',)
+  }


We should type all of these responses at some point, <any> was only meant as a placeholder.

wbobeirne · 2019-09-13T16:15:06Z

src/components/AddSigners.tsx

+        rightComponent = <Button onClick={() => this.handleUnlock(device)}>Unlock</Button>
+      // TODO
+      // } else if (device.needs_pin_sent) {
+      //   rightComponent = <Button onClick={enterPassphrase}>Unlock</Button> 


Is this conditional right? Or is it mean for needs_password_somethingorother?

wbobeirne · 2019-09-13T16:16:56Z

src/components/AddSigners.tsx

+  modal: boolean;
+  pinModalActive: boolean;
+  // FIXME: keeping track of the device outside the modal sucks
+  pinEntryDevice: Device | null;


This seems pretty reasonable to me, IMO. However, maybe the "cooler" option would be to have a global component rendered way further up in the stack, and to trigger pin entries via redux. There could be a state prop called pinDevice or something like that, and you just call this.props.promptPin(device), it sets it in redux state, and causes that modal to render and take it from there.

wbobeirne · 2019-09-13T16:19:06Z

src/components/AddSigners.tsx

+    }));
+  }
+
+  openModal(device: Device) {


Could just consolidate the two props into one be informing modal openness from whether or not you have a pin entry device set, i.e. isOpen={!!this.state.pinEntryDevice}

wbobeirne · 2019-09-13T16:31:53Z

src/components/Wallet.tsx

+        {activeWallet.balances.unconfirmed > 0 &&
+          <div>Unconfirmed Balance: {activeWallet.balances.unconfirmed}</div>}
+        {signersComponent}
+        {addSigners}


Might be good to stick with one style of conditional rendering for consistency's sake. Here both variable assignment in conditional, and in-jsx checking is done. addSigners and signersComponent could both be done with in-jsx conditionals.

wbobeirne · 2019-09-13T16:33:16Z

src/store/wallet/selectors.ts

+import { Signer, Device } from '../../types'
+
+export function selectCandidateDevicesForActiveWallet(state: AppState) {
+  // FIXME this check sucks


Seems reasonable to me 🤷‍♂

wbobeirne · 2019-09-13T16:33:51Z

src/store/wallet/selectors.ts

+        candidates.push(device);
+      }
+      return candidates;
+  }, [])


filter is probably a better method than reduce for this.

wbobeirne · 2019-09-13T16:36:23Z

src/store/wallet/selectors.ts

+// This is hard to use 
+export function selectActiveWallet(state: AppState) {
+  const { activeWallet } = state.wallet;
+  // if (activeWallet === null) {


Yep, you'd want a wrapper or higher order component for that, sorry for the my earlier red herring solution (Which does work in some cases, just not the one here.) Something like:

<RequireWallet component={MyComponent} />

where RequireWallet's render looks like

if (this.props.activeWallet) { return <MyComponent activeWallet={this.props.activeWallet} /> } else { return 'Oh no you need a wallet fam'; }

wbobeirne · 2019-09-13T16:37:33Z

src/types/device.ts

 }

+// FIXME: some of these are null at times
+// but I don't really want typescript yelling at me all the time ...


We'd need union types for those. What are the cases where you know it will be null, and know it won't?

…ed components with RequireData wrapper component.

justinmoon added 9 commits September 11, 2019 21:28

fix navbar wallet selection link

33102bf

Change active wallet upon new wallet creation

ddaebf3

First pass at "add signer" component

2a290c1

Add signers prototype

01ab252

Crappy spinner displayed when signer being added

f0b9448

cleanup

43e777b

Device instructions modal

984bdce

Progress unlocking trezors

c281064

Modal for Trezor One pin entry

0be1815

justinmoon commented Sep 13, 2019

View reviewed changes

wbobeirne reviewed Sep 13, 2019

View reviewed changes

wbobeirne added 2 commits September 13, 2019 14:30

Fix a few null / undefined issues.

f796229

Typed all device types. Fixed trezor PIN styles and behavior. Protect…

ebd8e7e

…ed components with RequireData wrapper component.

justinmoon merged commit 03653de into develop Sep 13, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Add signers #32

WIP: Add signers #32

justinmoon commented Sep 13, 2019 •

edited

Loading

justinmoon left a comment

justinmoon Sep 13, 2019

justinmoon Sep 13, 2019

justinmoon Sep 13, 2019

justinmoon Sep 13, 2019

wbobeirne Sep 13, 2019

justinmoon Sep 13, 2019

wbobeirne Sep 13, 2019

justinmoon Sep 13, 2019

wbobeirne Sep 13, 2019

justinmoon Sep 13, 2019

wbobeirne Sep 13, 2019

justinmoon Sep 13, 2019

wbobeirne Sep 13, 2019

justinmoon Sep 13, 2019

wbobeirne Sep 13, 2019

justinmoon Sep 13, 2019

wbobeirne Sep 13, 2019

wbobeirne left a comment

wbobeirne Sep 13, 2019

wbobeirne Sep 13, 2019

wbobeirne Sep 13, 2019

wbobeirne Sep 13, 2019

wbobeirne Sep 13, 2019

wbobeirne Sep 13, 2019

wbobeirne Sep 13, 2019

wbobeirne Sep 13, 2019

wbobeirne Sep 13, 2019

wbobeirne Sep 13, 2019

WIP: Add signers #32

WIP: Add signers #32

Conversation

justinmoon commented Sep 13, 2019 • edited Loading

justinmoon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wbobeirne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinmoon commented Sep 13, 2019 •

edited

Loading