Electron Ledger + Trezor Support #1836

wbobeirne · 2018-05-23T21:14:28Z

Closes #1621, closes #1616.

Description

This PR is the first of a few to add hardware wallet support to Electron with a new library dubbed Enclave (props to @HenryNguyen5 for the name.) You can read more about its specific approach here: https://github.com/MyCryptoHQ/MyCrypto/blob/enclave-willo-protocol/shared/enclave/README.md. While it's being developed, I'd like to keep it in the MyCrypto repo. But once we're in a steady place with it, I think it could easily be its own module.

Likewise, I think it's still a little rough around the edges. There's maybe too much boilerplate both on the server and client side and the types could probably be better, I'm definitely expecting some feedback and changes on the pr. There's also huge amount of repeated code between common/libs/wallets and enclave/server/wallets. We could potentially expand Enclave to handle web wallets as well, and just have a single API for interacting with wallets, regardless of platform.

I also adjusted the Ledger unlock screen with persistent information (screenshot below.)

Changes

Made Enclave lib with Ledger support, Trezor & KeepKey stubs
Changed Ledger libs over to their @ledgerhq/* packages
Changed our Ledger and Trezor libraries to defer to EnclaveAPI in Electron
Change Ledger tip to display persistently and have messaging about browser support and which Ledger app to open

Steps to Test

Unlock ledger on web
Display address on device with ledger on the web
Send a transaction with ledger on web
Sign a message with ledger on the web
Do all of the above in Electron

Screenshots

…ecessary

…ibs. Run chain code generation thru Enclave.

…-protocol

HenryNguyen5 · 2018-06-13T19:20:25Z

shared/enclave/server/index.ts

+  try {
+    // TODO: Validate params based on provided method
+    const params = JSON.parse(data.bytes.toString());
+    return params as EnclaveMethodParams;


Should turn this into a validation function that performs the job of a user defined type guard instead of casting it

I'm not sure what that would look like, can you describe it a bit more?

HenryNguyen5 · 2018-06-13T19:20:41Z

shared/enclave/server/index.ts

+    const params = JSON.parse(data.bytes.toString());
+    return params as EnclaveMethodParams;
+  } catch (err) {
+    throw new Error(`Invalid JSON blob provided for '${method}'`);


Should we include the actual error in here too?

HenryNguyen5 · 2018-06-13T19:22:18Z

shared/enclave/server/wallets/keepkey.ts

@@ -0,0 +1,21 @@
+import { WalletLib } from 'shared/enclave/types';
+
+const KeepKey: WalletLib = {


Instead of the repetition of implementing a wallet lib here, we should make a base class that has all of the methods throw the unimplemented error

Also we could have a util function that can be used to wrap each method in a try/catch with a supplied error message if it matches some conditional, otherwise throws the original error

I initially decided against classes in favor of singletons since there aren't multiple instances of any of these, they're moreso handlers than instances. However I could still make them classes and just export an instance of them, so I guess that makes more sense.

The util function makes sense though, I'll try to work that out.

HenryNguyen5 · 2018-06-13T19:22:36Z

shared/enclave/server/wallets/ledger.ts

+import { WalletLib } from 'shared/enclave/types';
+import TransportNodeHid from '@ledgerhq/hw-transport-node-hid';
+import LedgerEth from '@ledgerhq/hw-app-eth';
+let transport: any;


Prefer to have this typed

HenryNguyen5 · 2018-06-13T19:23:11Z

shared/enclave/server/wallets/ledger.ts

+  } catch (err) {
+    if (err && err.message && err.message.includes('cannot open device with path')) {
+      throw new Error(
+        'Your Ledger is currently in use with another application. Please wait, or close other wallet applications before trying again.'


Should have this i18n'd i think since its a user error

Absolutely, though I have no way of getting at redux state in here. Probably what I should do is replace all of these human readable errors with ERROR_CODES to be translate'd on the frontend.

That sounds like a good idea

HenryNguyen5 · 2018-06-13T19:24:00Z

shared/enclave/server/wallets/ledger.ts

+        chainCode: res.chainCode as string
+      };
+    } catch (err) {
+      throw new Error('Failed to connect to Ledger');


I feel like we should include the err message here for diagnostics

No way to do that & do i18n at the same time, so I'm going to settle for a console.error. In general we'll probably need to come up with a way to get console logs from users if they experience issues electron-side.

HenryNguyen5 · 2018-06-13T19:27:14Z

common/components/WalletDecrypt/components/LedgerNano.tsx

    const showErr = error ? 'is-showing' : '';

    if (!dPath) {
      return <UnsupportedNetwork walletType={translateRaw('x_Ledger')} />;
    }

-    if (window.location.protocol !== 'https:') {
+    if (!process.env.BUILD_ELECTRON && window.location.protocol !== 'https:') {


We use this env variable everywhere in the codebase, perhaps use another level of indirection by hiding it behind a function like isElectron so we can change it if need be in the future? We could do this for any other environment variables too.

HenryNguyen5 · 2018-06-13T19:41:41Z

I think that the easiest way to remedy the code duplication is to have each duplicated method its own function, then use that function where it used to be duplicated.

… Fix some types

… Fix some types and error messages.

* Type trezor connect. * Check in trezor code * Implement TREZOR wallet * Convert TREZOR to use enclave class like Ledger. * Switch to mycrypto fork of trezor lib. Remove unused dependencies.

…to into enclave-willo-protocol

* Add a new route for AddressBook * Further templating of the AddressBook view * Add initial functionality to handle a table of existing address labels * Make the linter happy * Adjust paths * Factor out TableRow and add common functionality * Add initial Redux boilerplate for addressBook | fix minor linting issues * Swap out terminology and types * Connect up to Redux * Connect data for AddressBookTable to Redux * Use temporary fields for addition * Remove alignment and index column * Stopping point * Adjust the sizing of rows to be consistent * Initial implementation of a dropdown for the address field * Minor styling to dropdown * Stopping point * Apply a focus concept onto the factory * Add keyboard controls for the address field dropdown * Adjust label of address field when it matches an addressBook entry * Properly handle attempting to blur a non-existent component * Minor styling changes on dropdown box * Standardize address casing, add accessibility to dropdown * Create an addressLabel component * Pass refs correctly and fix some typings * Exact version * Add module name mapping for shared/keycodes * addressBook reducer tests * Add functionality to DeterministicModal * Minor changes / Add test for addressBook selectors * Move out AddressBookTable to a component * Typing, translation and restructuring * More typing and translation fixes * More linting fixes * More type changes * Variable name for dropdown background * Fix TS type errors, lint errors, remove unused props * Used a different selector and removed method: AddressBookTable * Linter was mad * Linter mad again :( * Add a translation and adjust styling of AddressBookTable * Move the onBlur to a class method * Prevent the default behavior of up/down/enter for dropdown * Let's do it this way instead * Adjust the styling on DeterministicWalletModal labels * Change `AddressBookTable` into a pseudo-table using section and div * Use readable keys vs. keycodes * Put the dropdown in InputFactory and position it correctly * Sanitation of label adding and changing * Prevent duplicate labels in AddressBook and Row * Add a box shadow and use `invalid` class insted of custom * Use emphasis vs strong for address in dropdown * Display the label undernearth the input vs. changing it * Isolate AccountAddress into its own component * Introduce interactivity to AccountAddress * Fully incorporate with Redux * Validation for AccountAddress * Add validation notifications for address field on AddressBookTable * Minor formatting * Adjust wrappage for optimal flexboxxing * Make AddressBookTable responsive * Show an invalid input in "real time" instead of only on submit * Real time input validation for AddressBookTableRow * Responsive-ize the To address dropdown * Hide identicons instead at small enough screen sizes * Fix repsonsiveness of dropdown further * Fix responsiveness of table rows and inputs * Truncate account info and switch identicons to the right for consistency * Use classnames instead of targetting element directly for DWM * Display a notice if the entered query doesnt match a label and isnt an addr * Don't show an error on the To address if its a label entry * Display an error under AddressBookTableRow in real time * Display errors in real time for AddressBookTable temp inputs * Add realtime validation to AccountAddress * Ensure toChecksumAddress is used when entering labels to address manager * Show errors even after blurring. * Create a ducks/ implementation for addressBook * Duck-ize notifications * Duck-ize customTokens * Duck-ize deterministicWallets * Only show errors on address/label entry if they have been blurred * On certain inputs, show an invalid input immediately * spec files in same directory * Rename top-level redux directory * Duck-ize gas * Add displayed errors for labels with 0x and labels containing ens * Move ENS checking validation out * Add a saga for addLabelForAddress * Completely revamp the redux side of Address Manager and test it all * Adjust components to use new redux addressBook * Incorporate new redux into AddressBookTableRow and clean up for linter * Make linter and tests happy * Another reduxy overhaul * Still fixing it * More redux updates * Finalize redux stuff. * Incorporate new reduxy way into AddressBookTable & Row * Incorporate redux changes into Account Address * Small tests fix * Add and fix some selector tests * Addressing Will's comments * Shortened visibility class for line length reasons. * Incorporate ducks pattern on updates addressBook * Fix typeerror * Migrate messages to ducks * For Henry * Duckify onboardStatus * Duckify paritySigner * Duckify rates * Duckify transactions * Duckerize wallet * Reduckerate config * Adjust exports and tests of every duck so far * Duckify ENS * Duckerificate schedule * Duckificate swap * Actually use the new sagas; fix a circular dependency problem. * Duckify transaction (phew) * Add basics to redux/ directory * Remove non-ducked redux stuff * First sweep of redux/ directory * Combine redundant imports * Fix more linting stuff. * A few more type fixes * Welp... now I know not to use index. * Sweep components/ * Sweep through containers/ * Im really starting to get frustrated * The dawn of a new age. * Linter fixes. * De-flatten config/ reducers * Do my thang on config selectors * Adjust all references to config * Split up ens reducers * Wrap up de-nesting ENS * Big boy refactor * Split transaction into its reducers * Fix reducers in transaction/ * Stopping point * Adjust references to transaction from components * Fix references to selectors * Nest broadcast actions * Nest field actions * Nest meta actions * Nest network actions * Nest sign actions * Nest broadcast types * Nested fields types * Nest meta types * Nested network types * Nested sign types * Implement transaction saga changes * Huh? No prepush problems? * Update snappshot * Reintroduce deleted tests * A few missing tests found * Found three missing transaction tests * Found more tests * Found the rest of the tests, woohoo. * Renamed TypeKeys in TRANSACTION * Specify TRANSACTION_BROADCAST * Pretty up these imports * Specify TRANSACTION_FIELDS * Specify TRANSACTION_META * Specify TRANSACTION_NETWORK * Specify TRANSACTION_SIGN * Adjust imports and add translations * Update config snapshot * Post-merge * Temporary fix for DW/Config sagas so Daniel can continue smoke testing * Remove first circulat dependency * Fix more circular dependencies * Properly structure config indices * Further restructure config * Prepare for idea * Target directly from within features/ * Remove that circular dependency -- woohoo * Remove the circular dependency from Web3Wallet, temporarily comment some tests pending assistance * Un-comment the component-in-redux phenomenon * Move onLoad to the store file * Adjust addressBook imports/exports * Adjusted imports/exports for customTokens * Adjust imports/exports of deterministicWallets * Adjust imports/exports of ens * Restructure imports/exports of gas * Restructure imports/exports for message * Adjust imports/exports of notifications * Restructure onboardStatus imports/exports * Restructure paritySigner imports/exports * Restructure rates imports/exports * Restructure schedule imports/exports * Fix broadcastweb3handler test * Restructure swap imports/exports/ * Restructure transactionS imports/exports * Restructured wallet imports and exports * Hoist all necessary selectors aside from config/**/* and transaction/**/* * Hoist all top-level selectors from transaction * [Fix] Estimate Gas on Value Field Change (#1942) @ skubakdj * Implement right-click context menu (#1780) @ bryanwb * No Private Keys Online (#1466) @ wbobeirne * Fix Stuck Node on Metamask Logout (#1951) @ wbobeirne * [Fix] Make ENS Value Consistent (#1956) @ skubakdj * Auto token add (#1808) @ HenryNguyen5 * Electron Ledger + Trezor Support (#1836) @ wbobeirne * Fix Context Menu Popup Parameters (#1957) * Add RSK network w/ network agnostic refactors (#1939) @ wbobeirne * Change displayed notification back in helpers.tsx * Remove newline on shell files * Re-add newlines * Remove newling on .travis.yml * Prettier two files * Re-add index.scss import in OnboardModal * Restructure transaction subdirectories * Everything in transaction/ except for sagas * Restructure transaction imports/exports * Nest broadcast sagas * Nest fields * Nest meta * Nest network * Nest sign * Use generic names for reduxy stuff in the same directory to save space * Do everything every in the whole wide world

HenryNguyen5 and others added 30 commits April 23, 2018 20:34

Initial scaffold of enclave

bbfb084

Cleanup types

570b3c4

Add comments

e9f0436

Do not truncate errors, pretty output

55a01ac

Introduce helpers for sagas

8b3c5f3

Update yarn lock

e2b569a

Convert enclave into its own lib. Implement client and server.

ac8777b

Merge branch 'develop' of github.com:MyCryptoHQ/MyCrypto into develop

a67f181

Check in progress

50a960f

Initial types

e31648b

Remove unused lib

82302b9

Finish types

bd72b95

cleanup

22e82f0

Switch over to using electron protocol, remove code thats no longer n…

4c7c6d0

…ecessary

Refactor Ledger and Trezor wallets to provide all functionality via l…

e292253

…ibs. Run chain code generation thru Enclave.

Check in trezor work

e910151

Transaction signing

b9bd734

Message signing

b4d8f36

Display address

3b5dd4e

Fix deallocation of trezor

e8c3700

Adjust API

5aa41d9

Remove unused getAddresses

f4b3678

Merge in ledger types

30a01d1

Fix imports and filenames to cooperate with internal typings

4a27224

Merge remote-tracking branch 'origin/ledger-types' into enclave-willo…

fe5e1eb

…-protocol

Fix type uncertainty

34758d7

Merge develop, resolve conflicts.

2b5b484

Add persistent message to Ledger unlock.

a8cd2f5

Update ledger help link to kb

fc6d0d7

Convert ledger over to updated libs

5be9cc9

HenryNguyen5 reviewed Jun 13, 2018

View reviewed changes

wbobeirne mentioned this pull request Jun 13, 2018

Electron TREZOR Support #1946

Merged

wbobeirne and others added 13 commits June 14, 2018 12:27

Type ledger errors

ec49f0e

Reduce enclave client code.

76611dd

No default exports

dd2c622

l18n user facing enclave errors

8d854c3

Reduce repeated enclave code by splitting it into its own wallet lib.…

e2b0746

… Fix some types

tslint

0c15ddf

Reduce repeated enclave code by splitting it into its own wallet lib.…

e360b90

… Fix some types and error messages.

Electron TREZOR Support (#1946)

c9d2a9f

* Type trezor connect. * Check in trezor code * Implement TREZOR wallet * Convert TREZOR to use enclave class like Ledger. * Switch to mycrypto fork of trezor lib. Remove unused dependencies.

Merge branch 'develop' into enclave-willo-protocol

6471da5

remove unnecessary window attachment

ecd8395

tslint

afe010b

Merge branch 'enclave-willo-protocol' of github.com:MyCryptoHQ/MyCryp…

7f3a8d9

…to into enclave-willo-protocol

Merge branch 'develop' into enclave-willo-protocol

b61726f

dternyak changed the title ~~Electron Ledger Support~~ Electron Ledger + Trezor Support Jun 15, 2018

dternyak approved these changes Jun 15, 2018

View reviewed changes

dternyak merged commit cb92f59 into develop Jun 15, 2018

ConnorBryan added a commit that referenced this pull request Jun 16, 2018

Electron Ledger + Trezor Support (#1836) @ wbobeirne

105b3bb

dternyak deleted the enclave-willo-protocol branch June 19, 2018 18:32

dternyak restored the enclave-willo-protocol branch June 19, 2018 18:32

dternyak deleted the enclave-willo-protocol branch June 27, 2018 05:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Electron Ledger + Trezor Support #1836

Electron Ledger + Trezor Support #1836

wbobeirne commented May 23, 2018 •

edited by dternyak

Loading

HenryNguyen5 Jun 13, 2018

wbobeirne Jun 14, 2018

HenryNguyen5 Jun 13, 2018

HenryNguyen5 Jun 13, 2018

HenryNguyen5 Jun 13, 2018

wbobeirne Jun 14, 2018

HenryNguyen5 Jun 13, 2018

HenryNguyen5 Jun 13, 2018

wbobeirne Jun 13, 2018

HenryNguyen5 Jun 13, 2018

HenryNguyen5 Jun 13, 2018

wbobeirne Jun 14, 2018

HenryNguyen5 Jun 13, 2018

HenryNguyen5 commented Jun 13, 2018

		@@ -0,0 +1,21 @@
		import { WalletLib } from 'shared/enclave/types';

		const KeepKey: WalletLib = {

Electron Ledger + Trezor Support #1836

Electron Ledger + Trezor Support #1836

Conversation

wbobeirne commented May 23, 2018 • edited by dternyak Loading

Description

Changes

Steps to Test

Screenshots

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

HenryNguyen5 commented Jun 13, 2018

wbobeirne commented May 23, 2018 •

edited by dternyak

Loading