Hardware wallets #184

liamzebedee · 2020-02-18T14:48:11Z

Work in this branch implements support for Ledger and Trezor for use as Ethereum wallets, and in a future PR that is not far far away, Bitcoin wallets too. It uses web3-react, a well-written library for Ethereum interactions in React, which supports various types of wallets (eg. WalletConnect).

This PR implements "connect a wallet" functionality, with UX inspired from the Uniswap dApp. Some screenshots are attached below to illustrate. Ignore the terrible design until [WIP] is removed.

Use WSS for ElectrumX connection, as hardware wallets require HTTPS with no mixed transport security.
Integrate Ledger support
Fix Ledger error handling when it's on the lockscreen - TransportStatusError: Ledger device: UNKNOWN_ERROR (0x6804). Looks like a weird Ledger bug.
UX audit of choose wallet
Trezor support
Trusted certificate for ElectrumX server (refs: keep-dev/test: SSL #194)

Depends-On: #195

Epic: #177

Image assets

Logos were taken from:

liamzebedee · 2020-02-18T14:49:54Z

Example of Uniswap's distinction between Web3 connection and wallet connection.

Another example of the "Connect Wallet" dialog, which this is modelled on.

- remove our HOC helpers which were barely used: withAccount, withContract, withBalance, withConnectDapp - Begin using two Web3 connections - one to the network, and a second to the wallet (which could be a hardware wallet, mobile wallet via WalletConnect, etc.). We will be "connecting the wallet" as a separate step to connecting to Web3 (as Trezor/Ledger don't have a built-in connection to Infura... 😛). The rationale for this is detailed more in #184. - temporarily remove logic for Web3Status - fix Start page to use the react-web3 hook, useWeb3React.

liamzebedee · 2020-02-19T10:35:08Z

Github doesn't support webm/mp4, so here's a brief video on Flowdock of the brutalist functional UI.

liz-shinn · 2020-03-18T19:53:35Z

Nice! Thanks for linking me to this. I had looked into the "connect wallet" dialog when doing initial research on the token dash. I've attached some screenshots below of other approaches I've seen.

From Basechain:

From IoTex:

From Staked:

Shadowfiend · 2020-04-02T22:40:37Z

The LOC on this change scared the bespitters out of me (before I realized there was a huge SVG here, that is).

Can we actually split this out to:

Existing react-web3 PR (Integrate react-web3 into dApp architecture #195).
PR that introduces the dialog but maybe only supports Metamask.
1 additional PR per added wallet

? Or would that be too much of a pain? It would be helpful for me to maintain context and keep review turnaround times short, even if each PR is built on the other (as long as the base branch is set to keep the diff to just what that PR is adding).

liamzebedee · 2020-04-03T10:54:13Z

@Shadowfiend If that helps us ship faster, then of course. Thanks for explaining why this would help too, that's something I find really useful.

Feeling kinda documenting, might delete later.

Shadowfiend · 2020-04-03T16:37:37Z

Now that I see this updated diff, feels fine as is. Will try and do a full pass today.

Shadowfiend

Left some thoughts, questions, and ideas. Feels like we're pretty close, particularly if we punt Trezor to a separate PR.

docs/testing-with-hardware-wallets.md

src/components/lib/ConnectWalletDialog.js

Shadowfiend · 2020-04-06T03:00:55Z

src/components/lib/ConnectWalletDialog.js

@@ -22,12 +23,33 @@ const injectedConnector = new InjectedConnector({
 	supportedChainIds: SUPPORTED_CHAIN_IDS
 })

+const ledgerConnector = new LedgerConnector({
+	// We use the chainId of mainnet here to workaround an issue with the ledgerjs library.
+	// It currently throws an error for the default chainId of 1377 used by Geth/Ganache.


Our chainId is 1101 though, no?

Yah - we may have changed the default for Geth? In either case, they're both >8 bits.

Or geth changed theirs. Can't remember for sure.

Shadowfiend · 2020-04-06T03:00:59Z

src/components/lib/ConnectWalletDialog.js

+	// 
+	// At some point, Ledger had to update their firmware, to swap from a uint8 chainId to a uint32 chainId [1].
+	// 
+	// They updated their client library with a 'workaround' [2], but it doesn't appear to work.


This workaround actually seems to do the opposite, masking any >8-bit chain id to only keep the lowest 8 bits. Guessing it's designed not to screw up when communicating with the ledger, rather than being designed to support larger chain ids?

Guessing it's designed not to screw up when communicating with the ledger, rather than being designed to support larger chain ids?

Yup. Poor wording on my end, deriving from a poor understanding. Basically, Ledger has implemented 32 bit chainID support in their device firmware, but the Ledger Ethereum app still transports the truncated 8 bit ID, I'm guessing. eg. one chain, Piri, has its 32 bit chainID working.

That and my assumptions were wrong - the LedgerSubprovider from 0x uses an older version of @ledgerhq/hw-app-eth, without this workaround. Soooo, going to address that in a bit.

Shadowfiend · 2020-04-06T03:01:45Z

src/components/lib/ConnectWalletDialog.js

+		if (wallet == 'Ledger') {
+			connector = ledgerConnector
+		} else if (wallet == 'Metamask') {
+			connector = injectedConnector


This may be a good opportunity to use an object to map wallet name -> connector.

Shadowfiend · 2020-04-06T03:03:02Z

src/connectors/ledger.js

+    baseDerivationPath
+  }) {
+    super({
+      supportedChainIds: [chainId]


To what extent does the issue with chain ids above break our ability to interact with our internal testnet, out of curiosity?

Just swapping out the current subproviders with ones provided by 0x - they provided more consistent exceptions, that I wasn't hellbent on reimplementing.

They may not support the larger chainID. We'll see what we can do.

Shadowfiend · 2020-04-06T03:09:48Z

src/connectors/ledger.js

+  }
+
+  /**
+   * @returns {Promise<null>}


An explanation of what this promise represents would be ideal 😬

Also I tend to use @return rather than @returns. @return is more widely and historically supported both in JS and across languages, so it's just a bit easier of a habit 🤷‍♂️

src/connectors/ledger.js

package.json

The 0x API's are generally of a higher engineering quality. The exceptions encapsulate the error cases better, and they also manage the Ledger Transport lifecycle, which was a flaw of the previous implementation. We use them in keep-core too.

We use @0x/subproviders for the Ledger/Trezor integration. @ledgerhq/hw-app-eth is being upgraded as an older version was installed by accident.

"Stolen" from the Compound dApp. We'll leave these here as a placeholder, until we get the real UX in.

Shadowfiend

Few more notes; only the one about the annotations is blocking atm I think.

Shadowfiend · 2020-04-07T02:21:02Z

src/components/lib/ConnectWalletDialog.js

@@ -103,22 +130,52 @@ export const ConnectWalletDialog = ({ shown, onConnected }) => {
 	}

 	const ConnectToWalletStep = () => {
+		if(error) {


Let's at least log this error… We had this issue in the token dApp also where we were displaying a super-generic error and there was no easy way to get to the underlying one.

Yep, got that covered. The error is set in chooseWallet, where it is thrown immediately after -

https://github.com/keep-network/tbtc-dapp/blob/master/src/components/lib/ConnectWalletDialog.js#L52

Shadowfiend · 2020-04-07T02:56:46Z

src/components/lib/ConnectWalletDialog.js

+		if(chosenWallet == 'Ledger') {
+			return <>
+				<header>
+					<div className="title">Plug In Ledger & Enter Pin</div>


Do we need an extra div here?

Shadowfiend · 2020-04-07T02:57:06Z

src/components/lib/ConnectWalletDialog.js

 		return <>
 			<header>
-				<div className="title">Connect To A Wallet</div>
+				<div className="title">Connect to a wallet</div>


As above re: the div.

Shadowfiend · 2020-04-07T02:57:14Z

src/components/lib/ConnectWalletDialog.js

+	const ErrorConnecting = () => {
+		return <>
+			<header>
+				<div className="title">Connect to a wallet</div>


Shadowfiend · 2020-04-07T03:00:04Z

src/components/lib/ConnectWalletDialog.js

 			{ error && <p>{error}</p> }
 		</>
 	}

 	const ConnectedView = () => {
 		return <div className='connected-view'>
 			<header>
-				<div className="title">Connect To A Wallet</div>
+				<div className="title">Wallet connected</div>


Shadowfiend · 2020-04-07T03:17:12Z

src/components/lib/ConnectWalletDialog.js

@@ -130,6 +187,9 @@ export const ConnectWalletDialog = ({ shown, onConnected }) => {
 	return <div>
 		<div className={`modal connect-wallet ${shown ? 'open' : 'closed'}`}>
 			<div className="modal-body">
+				<div className="close">
+					<div className="x" onClick={onClose}>&#9587;</div>
+				</div>


I would rather prefer a boutonnière, my dear sir.

I'll chuck that in another PR - we've still got the lingering global styles on <button>.

Shadowfiend · 2020-04-07T03:19:32Z

src/connectors/ledger.js

-   * @returns {Promise<Object>}
-   */
-  async activate(): Promise {
+  async activate(): Promise<ConnectorUpdate> {


Uh-oh, looks like some TypeScript annotations sneaked through on this JS file.

Weirdly enough, they didn't break my build? create-react-app / babel must have some support, somewhere in those depths of hell.

I guarantee it, yes. This is why babel makes me squeamish… I like to know what's actually going on <_<

Shadowfiend · 2020-04-07T03:19:45Z

src/connectors/ledger.js

-      }))
-
-      engine.start()
+      let ledgerEthereumClientFactoryAsync = async () => {


Could be a const yah?

Shadowfiend · 2020-04-07T03:20:15Z

src/connectors/ledger.js

+        // Ledger will automatically timeout the U2F "sign" request after `exchangeTimeout` ms.
+        // This will result in a cryptic error:
+        //   `{name: "TransportError", message: "Failed to sign with Ledger device: U2F DEVICE_INELIGIBLE", ...}`
+        // Setting the exchange timeout fixes that, although I haven't seen it documented anywhere else in the Ledger docs.


Fixes it in what sense? The error message is better?

Oops. exchangeTimeout is set unrealistically low by default (5s or so).

Shadowfiend · 2020-04-07T03:20:51Z

src/connectors/ledger.js

+      engine.addProvider(
+        new LedgerSubprovider({
+          networkId: this.chainId,
+          ledgerEthereumClientFactoryAsync,


What a property name.

I mean it's no AbstractSingletonProxyFactoryBean... 😄

liamzebedee force-pushed the hardware-wallets branch from 786e8f8 to 46e0568 Compare February 19, 2020 10:14

liamzebedee force-pushed the hardware-wallets branch from 586a1e8 to 9fdf4aa Compare March 27, 2020 10:22

liamzebedee marked this pull request as ready for review March 27, 2020 11:47

liamzebedee mentioned this pull request Mar 31, 2020

Integrate react-web3 into dApp architecture #195

Merged

liamzebedee force-pushed the hardware-wallets branch from 53c60f0 to 70148d1 Compare April 1, 2020 16:01

liamzebedee changed the title ~~[WIP] Hardware wallets~~ Hardware wallets Apr 1, 2020

liamzebedee changed the base branch from master to react-to-web3 April 3, 2020 09:42

liamzebedee added 3 commits April 4, 2020 00:37

Implement a Ledger wallet connector

18d65bd

Improve doc for LedgerConnector

0c615d5

Begin documenting the process of testing the hardware wallets

afaec4e

Feeling kinda documenting, might delete later.

liamzebedee force-pushed the hardware-wallets branch from 08ea814 to afaec4e Compare April 3, 2020 14:40

Shadowfiend changed the base branch from react-to-web3 to master April 3, 2020 16:23

Shadowfiend reviewed Apr 6, 2020

View reviewed changes

liamzebedee added 10 commits April 6, 2020 20:07

Update Trezor links to their documentation

0b9cdac

Clicking Web3Status when "Connected" displays dialog

e2302ae

fix: U2F DEVICE_INELIGIBLE with Ledger TransportU2F

d26756e

Add close button to dialog

50d39d7

Show connected network in dialog details

ce2e747

Add @0x/subproviders, upgrade @ledgerhq/hw-app-eth

621773c

We use @0x/subproviders for the Ledger/Trezor integration. @ledgerhq/hw-app-eth is being upgraded as an older version was installed by accident.

Correct title in ConnectedView

6fd969f

Add ErrorConnecting view with "Try Again" button

c73798a

Add Ledger-specific connection instructions

e25fc67

"Stolen" from the Compound dApp. We'll leave these here as a placeholder, until we get the real UX in.

Consistent capitalisation

847449a

Shadowfiend reviewed Apr 7, 2020

View reviewed changes

liamzebedee mentioned this pull request Apr 15, 2020

Heath Ledger: Add Ledger wallet support #205

Merged

liamzebedee closed this Apr 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hardware wallets #184

Hardware wallets #184

liamzebedee commented Feb 18, 2020 •

edited

Loading

liamzebedee commented Feb 18, 2020

liamzebedee commented Feb 19, 2020

liz-shinn commented Mar 18, 2020 •

edited

Loading

Shadowfiend commented Apr 2, 2020 •

edited

Loading

liamzebedee commented Apr 3, 2020

Shadowfiend commented Apr 3, 2020

Shadowfiend left a comment

Shadowfiend Apr 6, 2020

liamzebedee Apr 6, 2020

Shadowfiend Apr 7, 2020

Shadowfiend Apr 6, 2020

liamzebedee Apr 6, 2020

Shadowfiend Apr 6, 2020

Shadowfiend Apr 6, 2020

liamzebedee Apr 6, 2020

liamzebedee Apr 6, 2020

Shadowfiend Apr 6, 2020

Shadowfiend left a comment

Shadowfiend Apr 7, 2020

liamzebedee Apr 15, 2020

Shadowfiend Apr 7, 2020

Shadowfiend Apr 7, 2020

Shadowfiend Apr 7, 2020

Shadowfiend Apr 7, 2020

Shadowfiend Apr 7, 2020

liamzebedee Apr 16, 2020

Shadowfiend Apr 7, 2020

liamzebedee Apr 15, 2020

Shadowfiend Apr 15, 2020

Shadowfiend Apr 7, 2020

Shadowfiend Apr 7, 2020

liamzebedee Apr 15, 2020

Shadowfiend Apr 7, 2020

liamzebedee Apr 15, 2020

Hardware wallets #184

Hardware wallets #184

Conversation

liamzebedee commented Feb 18, 2020 • edited Loading

Image assets

liamzebedee commented Feb 18, 2020

liamzebedee commented Feb 19, 2020

liz-shinn commented Mar 18, 2020 • edited Loading

Shadowfiend commented Apr 2, 2020 • edited Loading

liamzebedee commented Apr 3, 2020

Shadowfiend commented Apr 3, 2020

Shadowfiend 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

Shadowfiend 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

liamzebedee commented Feb 18, 2020 •

edited

Loading

liz-shinn commented Mar 18, 2020 •

edited

Loading

Shadowfiend commented Apr 2, 2020 •

edited

Loading