Connect Ledger via WebHID #12411
Merged
Connect Ledger via WebHID #12411
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…webhid preferences
…ition in preferences controller
…ciding whether to ask for webhid connection
…default transport type to webhid if available; use constants for u2f and webhid
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
danjm
changed the title
Gudahtt
reviewed
Oct 21, 2021
Co-authored-by: Mark Stacey <markjstacey@gmail.com>
Co-authored-by: Mark Stacey <markjstacey@gmail.com>
Gudahtt
reviewed
Oct 21, 2021
| @@ -233,6 +233,12 @@ | |||
| margin-left: 1.875rem; | |||
| } | |||
|
|
|||
| &__inline-link { | |||
| display: initial; | |||
There was a problem hiding this comment.
Nit: Display would already be inline if an anchor tag was used directly, rather than the button component. Does using the Button component here, for a thing that is not a button, have any other advantages I wonder 🤔
There was a problem hiding this comment.
The button component will use an anchor tag if the type is "link" which it is in this case. The advantage is consistency in styling used by other links within the app.
There was a problem hiding this comment.
Yeah I realize that, but it also sets display: flex, which never makes sense for a link. The transition styles and border radius don't seem to make sense here either. The ability of that component to handle links seems like an afterthought - it's not very good at it. Even the font size it sets seems strange - it uses H4 for some reason, and is usually overwritten to be smaller.
I guess it does set the standard colors though. As far as I can tell that's the only useful thing it does right now over an anchor tag.
There was a problem hiding this comment.
Hah, there's even a comment suggesting that it should be broken out:
Ah well. Not something we need to fix in this PR, it's just unfortunate.
Gudahtt
reviewed
Oct 21, 2021
Gudahtt
reviewed
Oct 21, 2021
Gudahtt
reviewed
Oct 21, 2021
Gudahtt
reviewed
Oct 21, 2021
Builds ready [75ed73c]
Page Load Metrics (516 ± 67 ms)
highlights:storybook
|
Gudahtt
reviewed
Oct 21, 2021
Co-authored-by: Mark Stacey <markjstacey@gmail.com>
Co-authored-by: Mark Stacey <markjstacey@gmail.com>
Builds ready [1a0a93a]
Page Load Metrics (388 ± 40 ms)
highlights:storybook
|
darkwing
approved these changes
Oct 21, 2021
| } | ||
| return; | ||
| } | ||
| setLedgerLivePreference(transportType); |
There was a problem hiding this comment.
I think we'd always want this to be updated upon change, correct? Why the return above?
There was a problem hiding this comment.
You may have been looking at an outdated diff 🤔 Weirdly the preview on the main discussion view of the PR doesn't match the diff view at all.
There was a problem hiding this comment.
Yeah, we used to have a return statement here, but now we set the preference at the start of this function
|
|
||
| export const WEBHID_CONNECTED_STATUSES = { | ||
| CONNECTED: 'connected', | ||
| NOT_CONNECTED: 'notConnected', |
There was a problem hiding this comment.
I know it's likely late in the game but would disconnected be a better word to use?
There was a problem hiding this comment.
disconnected implies it was once connected and is no longer, but this covers both that case and the case where it has never been connected
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR improves upon and replaces #12234
The purpose of this PR is to enable users to connect their Ledger devices to metamask via WebHID, if that option is available in the current browser.
When the user goes through the hardware wallet connect flow, by default they will be presented with a browser popup that asks them permission to connect their Ledger device.
In addition to that, other changes/improvements are:
Below is a demo video that shows:
ledgerwebhid-2.mp4