Update `@metamask/eth-ledger-bridge-keyring` #21149

mikesposito · 2023-10-02T13:38:04Z

Description

This PR updates the @metamask/eth-ledger-bridge-keyring version to latest (^1.0.0).
This brings a major breaking change: the transport logic (the way Metamask connects to the device) has been split from the keyring logic (the interface supported by our KeyringController).

Due to this, @metamask/eth-ledger-bridge-keyring needs a transport bridge to be passed on construction and successively initialized calling Keyring.init().

As this is a new behavior compared to the usual way we build keyrings, we have to use a different keyring builder factory, specific for this kind of "bridged keyring". That is, a keyring that needs a bridge class instance as a constructor option.

The bridge initialization is, instead, done outside the keyring builder (as it is async).

Note
Test coverage for hardware wallets is insufficient: I did some manual smoke tests with my Ledger Nano X (account unlock, send eth transaction sign), but this likely needs additional QA

Related issues

Fixes Update @metamask/eth-ledger-bridge-keyring #21144

Manual testing steps

Connect a new Ledger hardware wallet
Signing transaction should work as before

Screenshots/Recordings

Before

n/a

After

n/a

Pre-merge author checklist

Pre-merge reviewer checklist

I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

github-actions · 2023-10-02T13:38:16Z

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.

mikesposito · 2023-10-02T13:40:07Z

Waiting for #21145

socket-security · 2023-10-02T13:41:16Z

Updated dependencies detected. Learn more about Socket for GitHub ↗︎

Packages	Version	New capabilities	Transitives	Size	Publisher
@metamask/eth-ledger-bridge-keyring	0.15.0...2.0.0	None	`+0/-1`	107 kB	metamaskbot

socket-security · 2023-10-02T13:41:19Z

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

metamaskbot · 2023-10-02T15:29:08Z

Builds ready [afb031b]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4
- common: 0, 1, 2, 3, 4, 5, 6, 7
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 12, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (957 ± 405 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	90	155	112	16	8
		domContentLoaded	76	151	104	19	9
		load	87	2045	957	844	405
		domInteractive	76	151	104	19	9

Bundle size diffs [🚀 Bundle size reduced!]

background: -21.68 KiB (-0.59%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

codecov · 2023-10-02T15:29:24Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (32b249a) 68.59% compared to head (afb031b) 68.48%.

❗ Current head afb031b differs from pull request most recent head beab905. Consider uploading reports for the commit beab905 to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #21149      +/-   ##
===========================================
- Coverage    68.59%   68.48%   -0.11%     
===========================================
  Files         1046     1013      -33     
  Lines        41612    40582    -1030     
  Branches     11110    10846     -264     
===========================================
- Hits         28542    27790     -752     
+ Misses       13070    12792     -278

Files	Coverage Δ
...pp/scripts/lib/hardware-keyring-builder-factory.ts	`100.00% <100.00%> (ø)`
app/scripts/metamask-controller.js	`53.43% <100.00%> (-1.68%)`	⬇️

... and 229 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

brad-decker · 2023-10-31T17:13:47Z

I'll also need this one soon. So let me know when i cna review : )

legobeat · 2023-10-31T23:26:03Z

Waiting for #21145

Also MetaMask/eth-ledger-bridge-keyring#202 or preferred doing the updates separate?

## **Description** This PR updates the `@metamask/eth-trezor-keyring` version to latest (`^2.0.0`). This brings a major breaking change: the transport logic (the way Metamask connects to the device) has been split from the keyring logic (the interface supported by our KeyringController). Due to this, `@metamask/eth-trezor-keyring` ([and soon `@metamask/eth-ledger-bridge-keyring`](#21149)) needs a transport bridge to be passed on construction and successively initialized calling `Keyring.init()`. As this is a new behavior compared to the usual way we build keyrings, we have to use a different keyring builder factory, specific for this kind of "bridged keyring". That is, a keyring that needs a bridge class instance as a constructor option. The bridge initialization is, instead, done outside the keyring builder (as it is async) by `@metamask/eth-keyring-controller` after the keyring construction **Note** Test coverage for hardware wallets is insufficient: I did some manual smoke tests with my Trezor Model One (account unlock, send eth transaction sign), but this likely needs additional QA ## **Manual testing steps** - Connect a new Trezor hardware wallet - Signing transaction should work as before ## **Related issues** * Fixes #21143 ## **Pre-merge author checklist** - [x] I’ve followed [MetaMask Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [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] I’ve indicated what issue this PR is linked to: Fixes #??? - [x] I’ve included tests if applicable. - [x] I’ve documented any added code. - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)). - [x] I’ve properly set the pull request status: - [x] In case it's not yet "ready for review", I've set it to "draft". - [x] In case it's "ready for review", I've changed it from "draft" to "non-draft". ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. --------- Co-authored-by: MetaMask Bot <[email protected]> Co-authored-by: Brad Decker <[email protected]>

brad-decker · 2023-11-07T22:44:25Z

@mikesposito could we update this to upgrade to 2.0.0 that was released recently?

mikesposito · 2023-11-08T11:46:54Z

@metamaskbot update-policies

metamaskbot · 2023-11-08T11:51:22Z

Policy update failed. You can review the logs or retry the policy update here

legobeat · 2023-11-21T23:57:27Z

Sounds like we need to ensure that @metamask/eth-sig-util is bumped to 7.0.1 in yarn.lock, yes?

That would be it yeah, now that it is released.

legobeat · 2023-11-22T12:17:17Z

@mikesposito This should be good to go after a rebase on develop and regeneration of LavaMoat policies now that #21928 is merged.

mikesposito · 2023-11-22T12:33:15Z

@metamaskbot update-policies

metamaskbot · 2023-11-22T12:40:31Z

Policies updated

metamaskbot · 2023-11-22T16:56:07Z

Builds ready [c671ed8]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4
- common: 0, 1, 2, 3, 4, 5, 6
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (298 ± 205 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	82	114	91	8	4
		domContentLoaded	65	98	76	10	5
		load	77	1261	298	428	205
		domInteractive	65	98	76	10	5

Bundle size diffs [🚀 Bundle size reduced!]

background: -35.05 KiB (-0.91%)
ui: 0 Bytes (0.00%)
common: 132 Bytes (0.00%)

mikesposito changed the base branch from develop to chore/update-trezor October 2, 2023 13:38

This was referenced Oct 2, 2023

Update hardware keyrings to the latest version #20513

Closed

Update @metamask/eth-trezor-keyring #21145

Merged

mikesposito added needs-qa Label will automate into QA workspace team-wallet-framework labels Oct 2, 2023

mikesposito force-pushed the chore/update-ledger branch 2 times, most recently from 58035d2 to afb031b Compare October 2, 2023 14:48

mikesposito force-pushed the chore/update-trezor branch 2 times, most recently from a5a1c22 to 37b7f33 Compare November 2, 2023 11:10

brad-decker mentioned this pull request Oct 31, 2023

Connect Ledger to Offscreen Document API #21624

Closed

4 tasks

Base automatically changed from chore/update-trezor to develop November 7, 2023 15:55

metamaskbot added the INVALID-PR-TEMPLATE PR's body doesn't match template label Nov 7, 2023

mikesposito force-pushed the chore/update-ledger branch from afb031b to beab905 Compare November 8, 2023 09:50

mikesposito marked this pull request as ready for review November 8, 2023 09:51

mikesposito requested review from a team as code owners November 8, 2023 09:51

brad-decker previously approved these changes Nov 8, 2023

View reviewed changes

mikesposito and others added 12 commits November 22, 2023 13:29

chore: update @metamask/eth-trezor-keyring

55b56c8

Update LavaMoat policies

012a510

refactor: convert builder factory to typescript

0060077

chore: update @metamask/eth-ledger-bridge-keyring

c17f8fb

refactor: hw transport bridge class in factory

dbd831f

refactor: use ledger keyring v2

d5d5d83

update policies

0b3071f

yarn dedupe

0cab809

chore: update policies

41f4c13

Update lockfile

b07ed94

Update LavaMoat policies

96d08bd

chore: update build policies

e3d5427

mikesposito dismissed mcmire’s stale review via e3d5427 November 22, 2023 12:33

mikesposito force-pushed the chore/update-ledger branch from 5be2ffb to e3d5427 Compare November 22, 2023 12:33

Update LavaMoat policies

c671ed8

legobeat requested a review from kumavis November 22, 2023 12:44

legobeat approved these changes Nov 22, 2023

View reviewed changes

kumavis approved these changes Nov 22, 2023

View reviewed changes

metamaskbot removed the INVALID-PR-TEMPLATE PR's body doesn't match template label Nov 22, 2023

mikesposito removed the needs-qa Label will automate into QA workspace label Nov 22, 2023

mikesposito merged commit 3713c18 into develop Nov 22, 2023
68 of 70 checks passed

mikesposito deleted the chore/update-ledger branch November 22, 2023 17:25

github-actions bot locked and limited conversation to collaborators Nov 22, 2023

metamaskbot added the release-11.7.0 Issue or pull request that will be included in release 11.7.0 label Nov 22, 2023

Gudahtt restored the chore/update-ledger branch November 24, 2023 21:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update `@metamask/eth-ledger-bridge-keyring` #21149

Update `@metamask/eth-ledger-bridge-keyring` #21149

mikesposito commented Oct 2, 2023 •

edited

Loading

github-actions bot commented Oct 2, 2023

mikesposito commented Oct 2, 2023

socket-security bot commented Oct 2, 2023 •

edited

Loading

socket-security bot commented Oct 2, 2023 •

edited

Loading

metamaskbot commented Oct 2, 2023

codecov bot commented Oct 2, 2023 •

edited

Loading

brad-decker commented Oct 31, 2023

legobeat commented Oct 31, 2023 •

edited

Loading

brad-decker commented Nov 7, 2023

mikesposito commented Nov 8, 2023

metamaskbot commented Nov 8, 2023

legobeat commented Nov 21, 2023

legobeat commented Nov 22, 2023 •

edited

Loading

mikesposito commented Nov 22, 2023

metamaskbot commented Nov 22, 2023

metamaskbot commented Nov 22, 2023

Update @metamask/eth-ledger-bridge-keyring #21149

Update @metamask/eth-ledger-bridge-keyring #21149

Conversation

mikesposito commented Oct 2, 2023 • edited Loading

Description

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented Oct 2, 2023

mikesposito commented Oct 2, 2023

socket-security bot commented Oct 2, 2023 • edited Loading

socket-security bot commented Oct 2, 2023 • edited Loading

metamaskbot commented Oct 2, 2023

codecov bot commented Oct 2, 2023 • edited Loading

Codecov Report

brad-decker commented Oct 31, 2023

legobeat commented Oct 31, 2023 • edited Loading

brad-decker commented Nov 7, 2023

mikesposito commented Nov 8, 2023

metamaskbot commented Nov 8, 2023

legobeat commented Nov 21, 2023

legobeat commented Nov 22, 2023 • edited Loading

mikesposito commented Nov 22, 2023

metamaskbot commented Nov 22, 2023

metamaskbot commented Nov 22, 2023

Update `@metamask/eth-ledger-bridge-keyring` #21149

Update `@metamask/eth-ledger-bridge-keyring` #21149

mikesposito commented Oct 2, 2023 •

edited

Loading

socket-security bot commented Oct 2, 2023 •

edited

Loading

socket-security bot commented Oct 2, 2023 •

edited

Loading

codecov bot commented Oct 2, 2023 •

edited

Loading

legobeat commented Oct 31, 2023 •

edited

Loading

legobeat commented Nov 22, 2023 •

edited

Loading