webusb and no verify xpub export #24

humanumbrella · 2020-06-22T18:15:43Z

This PR mostly contains style changes and automatic fixes. However, there were a few bugs and linted errors. E.g. this PR changes several return await in async functions to return per https://eslint.org/docs/rules/no-return-await and finally fixes a couple additional typos.

Tests passing, and test suite ran locally with success.

changes `return await` => `return` per https://eslint.org/docs/rules/no-return-await

…export

humanumbrella · 2020-06-22T18:52:06Z

Ledger Nano S v1.6.0 (MCU v1.12)

Trezor Model 1 v.1.8.1

bucko13

Just some small nits. Nice to get all the linting fixes in though!

One question I had is where can I find the changes related to the no verify xpub export part? I'm less familiar with this library so I'm probably just not looking in the right spot.

src/hermit.js

src/ledger.js

bucko13 · 2020-06-22T22:43:21Z

I think you may need to run npm install and commit the changes to the package-lock.json file in order to make travis happy:
https://travis-ci.com/github/unchained-capital/unchained-wallets/builds/172541993

humanumbrella · 2020-06-23T10:16:37Z

forgot to respond to the other commentary, but the no verify xpub export part
is located at https://github.com/unchained-capital/unchained-wallets/pull/24/files#diff-5da93726c6b0fe6dfa4d1b76cdc3767dL593 (open diff on ledger.js for this link to do any highlighting properly)

waldenraines · 2020-06-23T21:17:22Z

.eslintrc.json

@@ -108,7 +108,7 @@
        "max-classes-per-file": "off",
        "max-depth": "error",
        "max-len": "off",
-        "max-lines": "error",
+        "max-lines": "off",


What is the rationale for this?

Most of the files in this package are over 300 lines (arbitrary) so this causes errors that aren't actually errors. I'm open to suggestion -- this was easier than saying ignore this particular rule in each file.

waldenraines

A couple of issues I'm seeing:

I am still seeing the ledger instructions and images very briefly when importing an xpub. These need to be reworked and shown only during the fallback case.
Importing and signing doesn't work at all in firefox (in linux at least, haven't tried windows). I get navigator.usb is undefined and that's it. I thought we had a fallback for cases when webusb wasn't supported.

humanumbrella · 2020-06-24T04:58:06Z

oops, one sec - got trigger happy on review requests.

humanumbrella · 2020-06-24T05:19:51Z

@waldenraines - you should be able to test now. NOTE: while you can use the Bitcoin Testnet app on the ledger for the webusb transport - you still cannot sign with the u2f transport (in firefox) without switching to the main Bitcoin app.

waldenraines · 2020-06-24T11:26:46Z

@waldenraines - you should be able to test now. NOTE: while you can use the Bitcoin Testnet app on the ledger for the webusb transport - you still cannot sign with the u2f transport (in firefox) without switching to the main Bitcoin app.

Nice, will have a look.

waldenraines

Here are the results of my testing.

Tested with:

ledger nano s: bitcoin app 1.4.2, secure element 1.6.0, mcu 1.11
ledger nano x: bitcoin app 1.4.2, secure element 1.2.4-1, mcu 2.8

And the results:

I was able to import pubkeys and xpubs in chrome, brave, and firefox in both linux and windows.
I was able to sign in chrome and brave on both linux and windows
I was able to sign in firefox on windows
I was unable to sign in firefox in linux (Ledger device: Internal error, please report (0x6f01))
For some reason the nano x didn't work for me in chrome in windows at all but worked wonderfully in linux, would be nice for someone else to confirm.

All in all I think that this is a vast improvement over the previous implementation and it falls back to u2f nicely in firefox.

waldenraines · 2020-06-24T14:53:36Z

@bucko13 did you have any further comments? Looks like you're still marked as "requesting changes".

bucko13 · 2020-06-24T19:00:21Z

src/ledger.js

@@ -40,7 +40,7 @@ import {
  DirectKeystoreInteraction,
 } from "./interaction";

-import IMAGES from "./images";
+//import IMAGES from "./images";


Wow, we're not using the images at all anymore? I guess we don't use them or have them yet for signing?

we were only showing images for xpubs/pubkeys at the moment

we were only showing images for xpubs/pubkeys at the moment

See also unchained-capital/caravan#112

kajalkris · 2020-06-24T19:13:27Z

Here are the results of my testing. Tested with:

ledger nano s: bitcoin app 1.4.2, secure element 1.6.0
ledger nano x: bitcoin app 1.4.2, secure element 1.2.4-1

For Ledger S on Mac & Windows, I can sign on Chrome & Brave browsers. I was not able to sign on Firefox.
For Ledger X on Mac, I was able to sign on Chrome, Firefox, and Brave.
For Ledger X on Windows, I was able to sign on Firefox, but on Chrome & Brave, I was unable to see the option to connect to the Ledger X device. See screenshot below.

humanumbrella · 2020-06-24T20:42:09Z

re: @kajalkris looks like others have reported nano X not showing up on windows ... LedgerHQ/ledgerjs#456 -- I don't know how to handle that one.

For Ledger S -- I could sign on FF on mac (u2f) but had to try multiple times sometimes to finally get it to succeed (it failed, but I just kept trying, I exited and re-launched the btc app)

bucko13 · 2020-06-24T21:30:05Z

LGTM! Tested in Chrome and Safari. Safari also throws the transport error but this isn't something we claim to support either right?

waldenraines · 2020-06-25T18:21:47Z

I think we should go ahead and merge this and if there is anything we can do on our side to fix ledger nano x then we can do that in a follow-up PR. Thoughts @bucko13 @humanumbrella?

humanumbrella added 6 commits May 29, 2020 17:26

code styling, fix errors and typos

f3dae91

changes `return await` => `return` per https://eslint.org/docs/rules/no-return-await

eslint-disable max-lines in eslintrc.json

fdd68cf

include @ledgerhq/transport-webusb and disable verify:true on pubkey …

0b0fa67

…export

bump hw-app-btc version

8d0cb47

code review feedback - not async function

b3ea93e

bump all ledger dependencies

ba79e8f

bucko13 requested changes Jun 22, 2020

View reviewed changes

humanumbrella added 2 commits June 22, 2020 17:50

Explicitly remove U2F and code review

61a755e

update package-lock.json

e516292

humanumbrella requested a review from bucko13 June 23, 2020 01:12

remove unnecessary async's

c3d7270

hw-app-btc version that includes serializeTransactionOutputs

43e24bc

waldenraines reviewed Jun 23, 2020

View reviewed changes

dhruvbansal approved these changes Jun 23, 2020

View reviewed changes

waldenraines requested changes Jun 23, 2020

View reviewed changes

bring hw-transport-u2f back for Firefox fallback option

f380389

humanumbrella requested review from waldenraines and dhruvbansal June 24, 2020 04:45

humanumbrella added 3 commits June 23, 2020 22:08

comment out messages in pubkey export tests for now

1ff9a80

comment out messages in xpub export

b6b4ead

images unused in this release

2449f11

waldenraines approved these changes Jun 24, 2020

View reviewed changes

cleanup and remove commented code

8cd4d6a

bucko13 reviewed Jun 24, 2020

View reviewed changes

remove commented code

7a5b6bd

bucko13 approved these changes Jun 24, 2020

View reviewed changes

bucko13 merged commit c4cfa44 into unchained-capital:master Jun 25, 2020

humanumbrella deleted the webusb-and-no-verify branch July 16, 2020 17:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

webusb and no verify xpub export #24

webusb and no verify xpub export #24

humanumbrella commented Jun 22, 2020

humanumbrella commented Jun 22, 2020

bucko13 left a comment

bucko13 commented Jun 22, 2020

humanumbrella commented Jun 23, 2020

waldenraines Jun 23, 2020

humanumbrella Jun 23, 2020

waldenraines left a comment •

edited

Loading

humanumbrella commented Jun 24, 2020

humanumbrella commented Jun 24, 2020

waldenraines commented Jun 24, 2020

waldenraines left a comment •

edited

Loading

waldenraines commented Jun 24, 2020

bucko13 Jun 24, 2020

humanumbrella Jun 24, 2020

waldenraines Jun 24, 2020

kajalkris commented Jun 24, 2020 •

edited

Loading

humanumbrella commented Jun 24, 2020

bucko13 commented Jun 24, 2020

waldenraines commented Jun 25, 2020

webusb and no verify xpub export #24

webusb and no verify xpub export #24

Conversation

humanumbrella commented Jun 22, 2020

humanumbrella commented Jun 22, 2020

bucko13 left a comment

Choose a reason for hiding this comment

bucko13 commented Jun 22, 2020

humanumbrella commented Jun 23, 2020

waldenraines Jun 23, 2020

Choose a reason for hiding this comment

humanumbrella Jun 23, 2020

Choose a reason for hiding this comment

waldenraines left a comment • edited Loading

Choose a reason for hiding this comment

humanumbrella commented Jun 24, 2020

humanumbrella commented Jun 24, 2020

waldenraines commented Jun 24, 2020

waldenraines left a comment • edited Loading

Choose a reason for hiding this comment

waldenraines commented Jun 24, 2020

bucko13 Jun 24, 2020

Choose a reason for hiding this comment

humanumbrella Jun 24, 2020

Choose a reason for hiding this comment

waldenraines Jun 24, 2020

Choose a reason for hiding this comment

kajalkris commented Jun 24, 2020 • edited Loading

humanumbrella commented Jun 24, 2020

bucko13 commented Jun 24, 2020

waldenraines commented Jun 25, 2020

waldenraines left a comment •

edited

Loading

waldenraines left a comment •

edited

Loading

kajalkris commented Jun 24, 2020 •

edited

Loading