Skip to content
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.

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

Deprecate u2f ledger live on chrome #18794

Merged
merged 43 commits into from
Nov 15, 2023

Conversation

montelaidev
Copy link
Contributor

@montelaidev montelaidev commented Apr 25, 2023

Explanation

Changes:

  • deprecates the use of U2F and Ledger Live on chrome
  • adds whats new notification
  • new migration to set the ledgerTransportType to webhid for all chrome users.

Screenshots/Screencaps

After

image

Pre-merge author checklist

  • [ x] I've clearly explained:
    • [ x] What problem this PR is solving
    • [ x] How this problem was solved
    • [ x] How reviewers can test my changes
  • [ x] Sufficient automated test coverage has been added

Pre-merge reviewer checklist

  • Manual testing (e.g. pull and build branch, run in browser, test code being changed)
  • PR is linked to the appropriate GitHub issue
  • IF this PR fixes a bug in the release milestone, add this PR to the release milestone

If further QA is required (e.g. new feature, complex testing steps, large refactor), add the Extension QA Board label.

In this case, a QA Engineer approval will be be required.

@github-actions
Copy link
Contributor

github-actions bot commented Apr 25, 2023

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.

@metamaskbot
Copy link
Collaborator

Builds ready [4207f3f]
Page Load Metrics (1690 ± 71 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint97152122168
domContentLoaded14701972167113766
load14712052169014971
domInteractive14701972167113766
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 bytes
  • ui: -3336 bytes
  • common: 168 bytes

@montelaidev montelaidev marked this pull request as ready for review April 25, 2023 13:33
@montelaidev montelaidev requested a review from a team as a code owner April 25, 2023 13:33
@montelaidev montelaidev force-pushed the deprecate-u2f-ledger-live-on-chrome branch from 69f8c6f to bbdf4ce Compare April 26, 2023 08:50
@metamaskbot
Copy link
Collaborator

Builds ready [2fcc787]
Page Load Metrics (1687 ± 69 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint103151124136
domContentLoaded14931942165211354
load14932061168714469
domInteractive14931942165211354
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 bytes
  • ui: -3336 bytes
  • common: 168 bytes

Copy link
Member

@gantunesr gantunesr left a comment

Choose a reason for hiding this comment

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

LGTM. One question I had while reviewing is that is there any way the user can tell which transport option they're using before connecting the Ledger device?

app/_locales/en/messages.json Outdated Show resolved Hide resolved
@metamaskbot
Copy link
Collaborator

Builds ready [af74213]
Page Load Metrics (1542 ± 41 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint98141112147
domContentLoaded1367167515237134
load1367168715428541
domInteractive1367167515237134
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 bytes
  • ui: -3336 bytes
  • common: 168 bytes

@montelaidev montelaidev added the needs-qa Label will automate into QA workspace label Apr 26, 2023
@metamaskbot
Copy link
Collaborator

Builds ready [3cf0004]
Page Load Metrics (1527 ± 33 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint90152109157
domContentLoaded1418168515197034
load1427168515276933
domInteractive1418168515197034
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 bytes
  • ui: -3336 bytes
  • common: 168 bytes

@metamaskbot
Copy link
Collaborator

Builds ready [677b876]
Page Load Metrics (1800 ± 78 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint111171132189
domContentLoaded15672124177815273
load15672124180016378
domInteractive15672124177815273
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 bytes
  • ui: -3336 bytes
  • common: 168 bytes

@metamaskbot
Copy link
Collaborator

Builds ready [50e098b]
Page Load Metrics (1732 ± 49 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint103150125168
domContentLoaded1586188617189747
load16011886173210149
domInteractive1586188617189747
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 bytes
  • ui: -3336 bytes
  • common: 168 bytes

Copy link
Contributor

@plasmacorral plasmacorral left a comment

Choose a reason for hiding this comment

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

Do we need to include app/scripts/migrations/066.test.js (or 66.js) since there are no longer options for the user to choose a transport type in either Firefox or Chrome?

@plasmacorral
Copy link
Contributor

plasmacorral commented Sep 7, 2023

My earlier comment is only partially addressed for Chrome. If the user has selected "U2F" and takes the update we will automatically switch to webHID and they can sign with Ledger. However, See 09/08/2023 edit below If the user has selected "Ledger Live" (or U2F) as the preferred Ledger connection method in v10.32.0 and then takes this update they will be locked out of being able to use their Ledger device with MetaMask as the migration is not producing the results expected.

Any mention of Ledger Live or Ledger Live bridge should be removed. This would mean removing this pop-up:

as well as the option in the drop-down within the add hardware flow: EDITED: derivation path should NOT be removed from the add hardware flow.

Video of the user experience: https://recordit.co/p7EZjkibLz

UPDATED Steps:

  1. Download and install older version, for this test I used v10.32
  2. configure wallet and pair with Ledger PSD
  3. change Settings>Advanced>preferred Ledger connection type to Ledger Live
  4. Close MM
  5. Download and install this branch
  6. Reload extension from chrome://extensions/
  7. Launch MM and unlock
  8. go to test dapp and connect then attempt to sign v4 typed data
  9. Note that Ledger Live prompt is opened and the user can no longer connect to the Ledger OR set webhid in settings

09/08/2023 update: able to produce this with u2f selected in v10.32.0 before updating to this branch, both locally built.

@plasmacorral plasmacorral added issues-found QA'd but questions A QA run through has been done but you need clarification on minor issues you found and removed needs-qa Label will automate into QA workspace labels Sep 7, 2023
@plasmacorral
Copy link
Contributor

plasmacorral commented Sep 7, 2023

If we have to make further changes, I will once again suggest that we also remove the search result in all browsers when the setting is removed. Although this is not a blocker here.

@HowardBraham HowardBraham dismissed stale reviews from gantunesr and themself via ed29c0e September 12, 2023 07:29
@metamaskbot
Copy link
Collaborator

Builds ready [ed29c0e]
Page Load Metrics (1595 ± 44 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint115180134168
domContentLoaded1458182615949144
load1458182615959144
domInteractive1458182615949144
Bundle size diffs [🚀 Bundle size reduced!]
  • background: -406 Bytes (-0.01%)
  • ui: -3.19 KiB (-0.04%)
  • common: -241 Bytes (-0.01%)

Copy link
Contributor

This PR has been automatically marked as stale because it has not had recent activity in the last 60 days. It will be closed in 14 days. Thank you for your contributions.

@github-actions github-actions bot added the stale issues and PRs marked as stale label Nov 11, 2023
@github-actions github-actions bot removed the stale issues and PRs marked as stale label Nov 14, 2023
@metamaskbot
Copy link
Collaborator

Builds ready [0243a1e]
Page Load Metrics (480 ± 253 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint88132104136
domContentLoaded7212593126
load851356480527253
domInteractive7212593126
Bundle size diffs [🚀 Bundle size reduced!]
  • background: -471 Bytes (-0.01%)
  • ui: -3.19 KiB (-0.05%)
  • common: -252 Bytes (-0.01%)

@plasmacorral
Copy link
Contributor

Unable to repro observation initially shared here. In test today migrating from v10.32.0 to commit 0243a1e1cfbca3bd686df2d8729a7b9878594b5b and was able to observe migration 103 complete successfully for a user that had Ledger Live selected in 10.32. Able to sign with no issue. Confirmed w/ Mac Sonoma 14.0 and Chrome 117.0.5938.92.

@plasmacorral plasmacorral added needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) and removed issues-found QA'd but questions A QA run through has been done but you need clarification on minor issues you found labels Nov 14, 2023
@HowardBraham HowardBraham merged commit 1228068 into develop Nov 15, 2023
64 of 68 checks passed
@HowardBraham HowardBraham deleted the deprecate-u2f-ledger-live-on-chrome branch November 15, 2023 20:42
@github-actions github-actions bot removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Nov 15, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Nov 15, 2023
@metamaskbot metamaskbot added the release-11.7.0 Issue or pull request that will be included in release 11.7.0 label Nov 15, 2023
@plasmacorral plasmacorral added QA Passed and removed DO-NOT-MERGE Pull requests that should not be merged labels Nov 15, 2023
@plasmacorral
Copy link
Contributor

Tested migration from 10.32.0 to this branch in Chrome 117.0.5938.92. Also tested migration from 10.35.1 to this branch under throttled network speeds (slow 3G) in Chrome. Spot checked migration in Firefox with Trezor from 11.3.0 to this branch in Firefox 119.0.1.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
QA Passed release-11.7.0 Issue or pull request that will be included in release 11.7.0 team-accounts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants