Skip to content

Implement hardware connectivity confirmation screens #12725

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 46 commits into from
Closed

Implement hardware connectivity confirmation screens #12725

wants to merge 46 commits into from

Conversation

darkwing
Copy link
Contributor

@darkwing darkwing commented Nov 16, 2021 •
edited
Loading

Designs found here: https://www.figma.com/file/6Px3A6vVFGy1hYbEUXi343/Hardware-Wallets?node-id=2%3A2020

ToDo:

  • Implement form fields in popover
  • Implement popover padding and other styling
  • Remove hardcoded debug values
  • Implement polling of connectivity status
  • Remove LedgerInstructionsField
  • Update Ledger keyring library

Manual Testing

  1. Pull down this PR
  2. In node_modules/@metamask/eth-ledger-bridge-keyring/index.js, change the BRIDGE_URL to 'https://darkwing.github.io/eth-ledger-bridge-keyring'; that address includes the gh-pages portion of this project (gh-pages: Add method to share connection status  eth-ledger-bridge-keyring#147). Also, comment out the following check for origin:
/*
      if (origin !== this._getOrigin()) {
        return false
      }
*/
  1. Connect a Ledger address, then try to do a basic send on testnet -- you will see the connectivity messaging and functionality!

Considerations

  1. Dropdown mode vs. full screen mode
  2. The different Ledger connectivity options

@darkwing darkwing requested a review from a team as a code owner November 16, 2021 23:40
@darkwing darkwing requested a review from hmalik88 November 16, 2021 23:40
@darkwing darkwing marked this pull request as draft November 16, 2021 23:40
@github-actions
Copy link
Contributor

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.

@darkwing darkwing force-pushed the hardware-connectivity branch from 3b2907b to 5fc7774 Compare November 24, 2021 19:55
@darkwing darkwing force-pushed the hardware-connectivity branch 3 times, most recently from 5d773e5 to faea547 Compare January 18, 2022 21:54
@darkwing darkwing force-pushed the hardware-connectivity branch from faea547 to 31a92f9 Compare January 31, 2022 20:52
@jpuri
Copy link
Contributor

jpuri commented Feb 1, 2022

Hey @darkwing : seems there is no activity on the PR from sometime.

@darkwing
Copy link
Contributor Author

darkwing commented Feb 1, 2022

@jpuri This PR is basically done; the majority of the work needs to be done in the eth-ledger-bridge-keyring repo.

@darkwing darkwing force-pushed the hardware-connectivity branch from 31a92f9 to 436b11f Compare February 8, 2022 16:33
@darkwing darkwing force-pushed the hardware-connectivity branch from e539243 to 092aa7f Compare February 9, 2022 20:38
@darkwing darkwing marked this pull request as ready for review February 9, 2022 21:20
@darkwing darkwing force-pushed the hardware-connectivity branch 2 times, most recently from e53804d to 7a8388c Compare February 18, 2022 19:31
@tmashuang tmashuang mentioned this pull request Feb 22, 2022
Closed
@darkwing darkwing force-pushed the hardware-connectivity branch from 084906f to 18aeeed Compare February 23, 2022 20:04
@tmashuang tmashuang mentioned this pull request Feb 24, 2022
Merged
@darkwing darkwing force-pushed the hardware-connectivity branch from 18aeeed to 743827d Compare February 24, 2022 20:30
@metamaskbot
Copy link
Collaborator

Builds ready [d8a6ed8]
Page Load Metrics (1161 ± 33 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint711363513505243
domContentLoaded1062137011596933
load1062137011616933
domInteractive1062137011596933

highlights:

storybook

danjm
danjm suggested changes Feb 25, 2022
View reviewed changes
Copy link
Contributor

@danjm danjm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks pretty good! Left a couple of small comments. Should we also get a design review from @holantonela ?

@holantonela holantonela self-requested a review February 25, 2022 15:24
@metamaskbot
Copy link
Collaborator

Builds ready [59d7e6c]
Page Load Metrics (1322 ± 65 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint711413494486233
domContentLoaded11471630132013565
load11471631132213565
domInteractive11471630132013565

highlights:

storybook

danjm
danjm suggested changes Mar 1, 2022
View reviewed changes
Copy link
Contributor

@danjm danjm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. We just need to get the ledger keyring library updated

darkwing and others added 13 commits June 21, 2022 10:28
@darkwing darkwing force-pushed the hardware-connectivity branch from dbac31a to 80451ba Compare June 21, 2022 15:28
@metamaskbot
Copy link
Collaborator

Builds ready [80451ba]
Page Load Metrics (1793 ± 48 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint872211132814
domContentLoaded1595194817659948
load16472042179310148
domInteractive1595194817659948

highlights:

storybook

digiwand
digiwand approved these changes Jun 21, 2022
View reviewed changes
Copy link
Contributor

@digiwand digiwand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙌

@brad-decker
Copy link
Contributor

@darkwing @danjm @hilvmason @kevinghim this seems like important work that we failed to get landed and is now seriously out of date. @darkwing i'm sorry that happened to you. Should we put this PR into sprint queue to prioritize review / rebase time for it?

@darkwing
Copy link
Contributor Author

@brad-decker I don't think so. The priority is MV3, and this PR could conflict with the work the team is doing on hardware wallet stuff for MV3. Also, this PR and its dependency heavily rely on setInterval and setTimeout, which will no longer work with MV3, so it's best that the MV3 work gets done first.

@brad-decker
Copy link
Contributor

@darkwing sounds good, but lets plan to revisit this conversation and have the review / rebase time dedicated as part of a future sprint after MV3. In the meantime, i'll convert to draft.

@brad-decker brad-decker marked this pull request as draft November 21, 2022 15:14
@holantonela
Copy link

We now have dedicated extension development capacity at the Key Management team, so perhaps @darkwing and @HowardBraham can work together on this after MV3 priors get updated. @AlexJupiter will coordinate it.

@github-actions github-actions bot locked and limited conversation to collaborators Apr 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@KOOLMOLL KOOLMOLL KOOLMOLL left review comments

@digiwand digiwand digiwand approved these changes

@brad-decker brad-decker brad-decker left review comments

@danjm danjm danjm left review comments

@hmalik88 hmalik88 Awaiting requested review from hmalik88 hmalik88 is a code owner automatically assigned from MetaMask/extension-devs

@holantonela holantonela Awaiting requested review from holantonela

Assignees
No one assigned
Labels
area-hardware team-accounts team-hardware-wallets
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.