Connect with Ledger

[micahalcorn]

Confirm that the DApp connects with Ledger hardware wallets.

[mikeshultz]

Ledger appears to work, but it's using the legacy account path format. Ledger has updated their apps to use the new HD path.

• OLD: 44'/60'/0'/0
• NEW: 44'/60'/0'/0/0

So, basically users that setup their ledger since this change(last year?) might be connecting an account they were not expecting. I'll look into the dapp's supporting library and see if they have anything to say about it.

If we have granular access, we can also try a few derivations in order until we find an account with ETH and select that. Or present the user with account options. Ledger is kinda tricky because of this, and something I ran into when trying to create a Python wrapping library for it.

Possibly a separate issue is that it doesn't show any kind of reaction on connection error. My first attempts to connect(before I restarted Chrome after device setup on my OS) were just silent button clicks.

CC @sparrowDom

[sparrowDom]

Tested our code quickly and error is fired and disabled is set to true here for all buttons once I press on the ledger button. So there are at least 2 bugs in the code:

• when there is an error with a connector, disable only the button for that connector and not all
• disabled property is set to true on the button, but the style of the button / visual error state is not rendered.

As for the first part about the account format I have on knowledge of that. Do you want to take charge of that part @mikeshultz ?

[mikeshultz]

I can, sure. I'm trying to test locally and having no luck so far even getting it to behave like the deployed version and having to dig through react abstractions isn't helping. We may need to create our own web3-react connector to behave how we want it to.

Will continue to poke today and report what I find.

[mikeshultz]

Just adding some notes. Definitely suppressing errors here:

https://github.com/oplabs/origin-dollar/blob/b1219703791053c416db095cd18105a06b53093b/dapp/src/components/LoginWidget.js#L57

Replacing with this shows some useful info:

activate(currentConnector, (err) => {console.error('err:', err)}, true)

TransportError: U2F browser support is needed for Ledger. Please use Chrome, Opera or Firefox with a U2F extension. Also make sure you're on an HTTPS connection

So... to test locally I'll need to setup an https reverse proxy locally. Will continue to dig after lunch.

[mikeshultz]

Here's a reference for the derivation path change with Ledger: MyCryptoHQ/MyCrypto#2070

Some useful TransportErrors are:

• 6804: Locked
• 6700 or 6d00: Ethereum app not open

@sparrowDom you seemed to have some kind of error handling in mind for this? Can you add support for errors coming from activate()?

[sparrowDom]

@mikeshultz when testing with my ledger firmware version 1.6.0 it errors out for me on this line with the error message "can not read handleRequest of null". Seems that getRPCProvider returns null in my case.

If it helps this is the this.engine._providers in my case:

Maybe important to note my Ledger account was created on an old ledger (that got bricked when I tried updating firmware from ~1.2.0 and they've sent me a replacement). That's why it probably shows Legacy in my Ledger Live:

[sparrowDom]

@mikeshultz and thanks for the error handling idea. It is really nice, though on the Ledger connector I can not reliably reproduce the errors (ledger locked, not in Ethereum app) on my end.

And when signed in with Ledger all the contract calls fail for me (like getApr, account balances and so forth) I suspect that has something to do with the "can not read handleRequest of null" error. Does that part work on your end perhaps?

[mikeshultz]

@sparrowDom I'll see if I can repro your issues today.

What do you mean you can't repro the errors? Are they different errors or no errors?

[mikeshultz]

There's another issue we might run into, that Ledger has some issues with contract calls. They're known to work with (most?) simple ERC20 calls, but it has some issues with larger transactions(I don't know what this limit is). I've spent dozens of hours trying to get it to work for a personal project to no avail. At least not until they finally release an up to date debug firmware. Unless the 0x LedgerSubprovider solved this already, we may see it in some heavy-parameter calls.

Something to keep an eye out for when we're testing.

[sparrowDom]

@mikeshultz when I locked the ledger or exited the Ethereum app, I would only encounter the 6804 error but not the other one ( 6700 or 6d00 ). Also for some weird reason the 6804 error doesn't get thrown consistently on my setup. Do these errors get thrown reliably for you?

Good point regarding the larger transactions. We should probably test that before providing a Ledger login.

@micahalcorn should we comment out Ledger and WalletConnect (not sure if someone has tested this one yet) on NODE_ENV === 'production' as long as we don't have it thoroughly tested?

[sparrowDom]

@mikeshultz thanks for the fix when trying to approve for Vault to use USDT I get this error now. Sorry have not debugged it further. I've sent some USDT and Eth to my Ledger account and start the dapp against mainnet: yarn run build && yarn run start:production. And have a reverse proxy setup so Ledger login works.

Though that approach is slower when you want to do rapid changes / debugging. So might not be ideal for development.

[mikeshultz]

Got a new error we should maybe handle:

ApproveCurrencyRow.js?af38:79 Exception happened:  Error: TOO_OLD_LEDGER_FIRMWARE
    at LedgerSubprovider.eval (ledger.js?79e1:188)
    at step (ledger.js?79e1:45)
    at Object.eval [as next] (ledger.js?79e1:26)
    at fulfilled (ledger.js?79e1:17)

This also seems like it might occur when trying to use the ledger on a network other than mainnet:

https://github.com/0xProject/0x-monorepo/blob/34538f2cedb0d134e88d436ce1f0e5afdf67d15d/packages/subproviders/src/subproviders/ledger.ts#L131-L137

[mikeshultz]

Another one that'll be important to handle:

{
    message: "Please enable Contract data on the Ethereum app Settings"
    name: "EthAppPleaseEnableContractData"
}

Contract data will have to be explicitly enabled(somehow excluding ERC20s) for contract calls.

[sparrowDom]

That is useful 👍 @mikeshultz
Just wondering do in your environment those errors get thrown as expected. And if yes does it happen here?

I need to give it another go, but the last time I was experimenting with locking Ledger and exiting the Ethereum app, I wouldn't see those errors consistently.

[mikeshultz]

They're all different, it seems and thrown in different spots. Notice the second to the last one had a trace and the last one was just an object. That's how I pulled them from the console. The last one came when attempting to do a mint. The one before that was when trying to sign a transaction(approve()).

I could probably help deal with these(or at least maybe come up with repro) if you can point me in the right direction for react-style error handling in the dapp.

[sparrowDom]

@mikeshultz not sure if this reply is still relevant, if you find any more places where we could add error handling for Ledger inside the dapp we can handle that similarly as it is in this PR: #246 .

Login widget has got a simple catch statement that triggers an error displayed in the widget.
The BuySellWidget passes on an onError function to any component that might want to throw a Ledger type error and those errors are all handled inside the BuySellWidget component.

[sparrowDom]

If there are no other things to address in the Ledger UX improvement PR: #246 I think we can close this issue for now. The behaviour on Ledger is currently not ideal, but I think we got it to a good enough usable state.

Any objections? cc:// @mikeshultz @micahalcorn