fix: bump `@metamask/eth-ledger-bridge-keyring` to `^8.0.3` to fix Ledger's handling of EIP-712 content #29820

dawnseeker8 · 2025-01-21T10:26:35Z

Description

Ledger team request us to upgrade the @ledgerhq/hw-app-eth to 6.42.0 to fix ledger bug for EIP-712 content
Here is some comment from Kevin LAMBERT from ledger team:

and this is original thread https://consensys.slack.com/archives/C02CYKAA8G1/p1737132760664329?thread_ts=1737106010.543919&cid=C02CYKAA8G1

Related issues

Fixes: #29813
Please also check the related PR on release 12.11

Manual testing steps

Will require a full regression test for ledger feature.

Screenshots/Recordings

Before

After

Pre-merge author checklist

I've followed MetaMask Contributor Docs and MetaMask Extension Coding Standards.
I've completed the PR template to the best of my ability
I’ve included tests if applicable
I’ve documented my code using JSDoc format if applicable
I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.

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.

… functions.

github-actions · 2025-01-21T10:26:49Z

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.

socket-security · 2025-01-21T10:27:53Z

👍 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.

View full report↗︎

dawnseeker8 · 2025-01-21T10:28:52Z

@SocketSecurity ignore npm/[email protected]

dawnseeker8 · 2025-01-21T11:40:47Z

@metamaskbot update-policies

metamaskbot · 2025-01-21T11:53:18Z

Policies updated.
👀 Please review the diff for suspicious new powers.

🧠 Learn how: https://lavamoat.github.io/guides/policy-diff/#what-to-look-for-when-reviewing-a-policy-diff

metamaskbot · 2025-01-21T12:32:19Z

Builds ready [745a410]

builds: chrome, firefox
builds (flask): chrome, firefox
builds (MMI): chrome
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Bundle Size Stats
mv2: E2e Actions Stats
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13
- ui: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13
- content-script: 0
- offscreen: 0

Page Load Metrics (1558 ± 88 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	1360	2001	1557	180	86
		domContentLoaded	1351	1987	1542	176	85
		load	1362	2000	1558	184	88
		domInteractive	23	70	34	14	6
		backgroundConnect	6	48	13	12	6
		firstReactRender	14	103	28	24	11
		getState	4	47	8	9	4
		initialActions	0	0	0	0	0
		loadScripts	936	1491	1106	145	70
		setupStore	5	58	15	16	8
		uiStartup	1537	2690	1790	265	127

Bundle size diffs [🚀 Bundle size reduced!]

background: -12.21 KiB (-0.21%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

…ve to confirmation dialog button always disabled for ledger.

metamaskbot · 2025-01-22T16:58:53Z

Builds ready [53d360c]

builds: chrome, firefox
builds (flask): chrome, firefox
builds (MMI): chrome
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Bundle Size Stats
mv2: E2e Actions Stats
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13
- ui: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13
- content-script: 0
- offscreen: 0

Page Load Metrics (1486 ± 39 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	1395	1647	1496	80	38
		domContentLoaded	1357	1635	1467	73	35
		load	1395	1659	1486	82	39
		domInteractive	22	56	32	11	5
		backgroundConnect	6	51	19	15	7
		firstReactRender	15	93	39	30	14
		getState	4	70	13	18	8
		initialActions	0	1	0	0	0
		loadScripts	999	1232	1081	60	29
		setupStore	5	85	23	27	13
		uiStartup	1530	2187	1798	207	99

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: -14.06 KiB (-0.24%)
ui: -105 Bytes (-0.00%)
common: 73.09 KiB (0.84%)

vivek-consensys · 2025-01-23T07:31:05Z

Tested locally and working as expected via clear signing dApp .

Flows covered include:-

Clear signing send txn via dApp
Clear signing sign EIP-712 via dApp (USDC Permit & Uniswap X)
Personal Sign
Deploy ERC 721
Signed Typed Data v4
Sign Permit
Signed Typed Data Variants

dawnseeker8 · 2025-01-23T07:36:25Z

@metamaskbot update-policies

metamaskbot · 2025-01-23T07:49:19Z

Policies updated.
👀 Please review the diff for suspicious new powers.

🧠 Learn how: https://lavamoat.github.io/guides/policy-diff/#what-to-look-for-when-reviewing-a-policy-diff

metamaskbot · 2025-01-23T08:23:38Z

Builds ready [ece2ef7]

builds: chrome, firefox
builds (flask): chrome, firefox
builds (MMI): chrome
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Bundle Size Stats
mv2: E2e Actions Stats
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13
- ui: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13
- content-script: 0
- offscreen: 0

Page Load Metrics (1784 ± 88 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	1450	2266	1782	183	88
		domContentLoaded	1439	2215	1756	178	86
		load	1450	2266	1784	184	88
		domInteractive	21	116	43	25	12
		backgroundConnect	8	63	23	17	8
		firstReactRender	16	100	39	27	13
		getState	4	54	19	17	8
		initialActions	0	0	0	0	0
		loadScripts	1049	1556	1281	113	54
		setupStore	6	72	17	19	9
		uiStartup	1626	2563	2070	231	111

Bundle size diffs [🚀 Bundle size reduced!]

background: -12.2 KiB (-0.21%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

socket-security · 2025-01-23T10:39:19Z

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

Package	New capabilities	Transitives	Size	Publisher
npm/@metamask/[email protected] 🔁 npm/@metamask/[email protected]	None	`0`	316 kB	metamaskbot

View full report↗︎

metamaskbot · 2025-01-23T11:16:49Z

Builds ready [ec7dfd2]

builds: chrome, firefox
builds (flask): chrome, firefox
builds (MMI): chrome
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Bundle Size Stats
mv2: E2e Actions Stats
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13
- ui: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13
- content-script: 0
- offscreen: 0

Page Load Metrics (1786 ± 91 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	1510	2222	1784	184	89
		domContentLoaded	1492	2206	1753	182	87
		load	1513	2223	1786	190	91
		domInteractive	24	105	41	19	9
		backgroundConnect	9	71	33	23	11
		firstReactRender	16	89	49	25	12
		getState	5	67	16	16	8
		initialActions	0	1	0	0	0
		loadScripts	1076	1701	1289	161	77
		setupStore	6	83	16	20	10
		uiStartup	1744	2664	2090	262	126

Bundle size diffs [🚀 Bundle size reduced!]

background: 20.87 KiB (0.35%)
ui: 0 Bytes (0.00%)
common: -79.01 KiB (-0.87%)

package.json

ccharly

LGTM. Haven't tested myself since we already ran full non-regression testing on this PR.

ccharly · 2025-01-27T09:22:24Z

app/scripts/metamask-controller.js

@@ -4876,7 +4876,7 @@ export default class MetamaskController extends EventEmitter {

  async attemptLedgerTransportCreation() {
    return await this.#withKeyringForDevice(
-      HardwareDeviceNames.ledger,
+      { name: HardwareDeviceNames.ledger },


For the next reviewer: Looks like this was missing on this function. All other functions that uses this.#withKeyringForDevice were using "named parameters" already.

ccharly · 2025-01-27T09:23:36Z

offscreen/scripts/ledger.ts

@@ -96,7 +96,7 @@ function setupMessageListeners(iframe: HTMLIFrameElement) {
 export default async function init() {
  return new Promise<void>((resolve) => {
    const iframe = document.createElement('iframe');
-    iframe.src = 'https://metamask.github.io/ledger-iframe-bridge/8.0.0/';
+    iframe.src = 'https://metamask.github.io/ledger-iframe-bridge/8.0.3/';


For the next reviewer: The bridge version has to be aligned with the @metamask/eth-ledger-bridge-keyring package version.

metamaskbot · 2025-01-27T09:53:25Z

Builds ready [e1abe95]

builds: chrome, firefox
builds (flask): chrome, firefox
builds (MMI): chrome
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Bundle Size Stats
mv2: E2e Actions Stats
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13
- ui: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13
- content-script: 0
- offscreen: 0

Page Load Metrics (1629 ± 40 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	224	1776	1565	319	153
		domContentLoaded	1454	1752	1611	78	37
		load	1474	1775	1629	83	40
		domInteractive	26	89	43	19	9
		backgroundConnect	7	81	20	16	8
		firstReactRender	17	102	51	28	13
		getState	5	56	12	13	6
		initialActions	0	0	0	0	0
		loadScripts	1030	1324	1165	71	34
		setupStore	6	65	19	21	10
		uiStartup	1654	2445	1890	187	90

Bundle size diffs [🚀 Bundle size reduced!]

background: -12.2 KiB (-0.21%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

metamaskbot · 2025-01-27T10:33:03Z

Builds ready [de5104b]

builds: chrome, firefox
builds (flask): chrome, firefox
builds (MMI): chrome
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Bundle Size Stats
mv2: E2e Actions Stats
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13
- ui: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13
- content-script: 0
- offscreen: 0

Page Load Metrics (1604 ± 68 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	1371	1997	1616	142	68
		domContentLoaded	1365	1932	1591	139	67
		load	1369	1958	1604	142	68
		domInteractive	24	99	46	22	10
		backgroundConnect	8	42	17	11	5
		firstReactRender	15	100	40	28	14
		getState	4	75	17	21	10
		initialActions	0	0	0	0	0
		loadScripts	933	1432	1156	119	57
		setupStore	7	57	14	14	7
		uiStartup	1552	2629	1878	262	126

Bundle size diffs [🚀 Bundle size reduced!]

background: -12.2 KiB (-0.21%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

metamaskbot · 2025-01-27T18:05:29Z

Builds ready [c3ee4f3]

builds: chrome, firefox
builds (flask): chrome, firefox
builds (MMI): chrome
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Bundle Size Stats
mv2: E2e Actions Stats
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13
- ui: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13
- content-script: 0
- offscreen: 0

Page Load Metrics (1539 ± 34 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	1411	1702	1538	71	34
		domContentLoaded	1399	1638	1515	66	32
		load	1409	1702	1539	71	34
		domInteractive	17	96	41	26	13
		backgroundConnect	9	71	24	18	9
		firstReactRender	15	90	42	27	13
		getState	4	45	13	13	6
		initialActions	0	0	0	0	0
		loadScripts	971	1237	1092	69	33
		setupStore	7	54	11	10	5
		uiStartup	1629	2111	1773	128	61

Bundle size diffs [🚀 Bundle size reduced!]

background: -12.2 KiB (-0.21%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

dawnseeker8 added 2 commits January 21, 2025 13:58

feat: use eth-ledger-bridge-keyring preview version to test extension…

bb1928c

… functions.

fix: create patch for library upgrade to pass the build.

ed62a49

dawnseeker8 added the team-hardware-wallets label Jan 21, 2025

Merge branch 'main' into feat/upgrade-ledger-library

419d020

dawnseeker8 and others added 2 commits January 21, 2025 19:41

Merge branch 'main' into feat/upgrade-ledger-library

4aaa6b0

Update LavaMoat policies

745a410

dawnseeker8 added 2 commits January 23, 2025 00:15

fix: Link to latest ledger-iframe-bridge. also fix serious bug relati…

e7a3c50

…ve to confirmation dialog button always disabled for ledger.

fix: update policy from latest code from main.

53d360c

vivek-consensys added the QA Passed label Jan 23, 2025

Merge branch 'main' into feat/upgrade-ledger-library

e353603

Update LavaMoat policies

ece2ef7

ccharly mentioned this pull request Jan 23, 2025

fix(keyring-eth-ledger-bridge): bump @ledgerhq/hw-app-eth to ^6.42.0 to fix Ledger's handling of EIP-712 MetaMask/accounts#153

Merged

Akaryatrh marked this pull request as ready for review January 23, 2025 09:43

Akaryatrh requested review from a team as code owners January 23, 2025 09:43

Akaryatrh marked this pull request as draft January 23, 2025 10:11

feat: bump to ledger keyring v8.0.3

ec7dfd2

Akaryatrh marked this pull request as ready for review January 23, 2025 10:37

Akaryatrh mentioned this pull request Jan 23, 2025

feat: upgrade ledger keyring to fix EIP-712 issues #29874

Merged

7 tasks

vivek-consensys added the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Jan 23, 2025

gantunesr changed the title ~~feat: Ledger team request us to upgrade the @ledgerhq/hw-app-eth to 6.42.0 to fix ledger bug for EIP-712 content~~ feat: upgrade the @ledgerhq/hw-app-eth to 6.42.0 to fix ledger bug for EIP-712 content Jan 24, 2025

gantunesr changed the title ~~feat: upgrade the @ledgerhq/hw-app-eth to 6.42.0 to fix ledger bug for EIP-712 content~~ feat: upgrade @ledgerhq/hw-app-eth to v6.42.0 to fix EIP-712 content ledger bug Jan 24, 2025

ccharly reviewed Jan 27, 2025

View reviewed changes

package.json Outdated Show resolved Hide resolved

feat: moving patches up on package json

e1abe95

ccharly approved these changes Jan 27, 2025

View reviewed changes

danroc approved these changes Jan 27, 2025

View reviewed changes

ccharly changed the title ~~feat: upgrade @ledgerhq/hw-app-eth to v6.42.0 to fix EIP-712 content ledger bug~~ fix: bump @metamask/eth-ledger-bridge-keyring to ^8.0.3 to fix Ledger's handling of EIP-712 content Jan 27, 2025

Merge branch 'main' into feat/upgrade-ledger-library

de5104b

Merge branch 'main' into feat/upgrade-ledger-library

c3ee4f3

angelcheung22 added this pull request to the merge queue Jan 28, 2025

Merged via the queue into main with commit e2fea0f Jan 28, 2025
70 checks passed

angelcheung22 deleted the feat/upgrade-ledger-library branch January 28, 2025 10:14

github-actions bot removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Jan 28, 2025

github-actions bot locked and limited conversation to collaborators Jan 28, 2025

metamaskbot added the release-12.12.0 Issue or pull request that will be included in release 12.12.0 label Jan 28, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: bump `@metamask/eth-ledger-bridge-keyring` to `^8.0.3` to fix Ledger's handling of EIP-712 content #29820

fix: bump `@metamask/eth-ledger-bridge-keyring` to `^8.0.3` to fix Ledger's handling of EIP-712 content #29820

dawnseeker8 commented Jan 21, 2025 •

edited by Akaryatrh

Loading

github-actions bot commented Jan 21, 2025

socket-security bot commented Jan 21, 2025 •

edited

Loading

dawnseeker8 commented Jan 21, 2025

dawnseeker8 commented Jan 21, 2025

metamaskbot commented Jan 21, 2025

metamaskbot commented Jan 21, 2025

metamaskbot commented Jan 22, 2025

vivek-consensys commented Jan 23, 2025

dawnseeker8 commented Jan 23, 2025

metamaskbot commented Jan 23, 2025

metamaskbot commented Jan 23, 2025

socket-security bot commented Jan 23, 2025 •

edited

Loading

metamaskbot commented Jan 23, 2025

ccharly left a comment

ccharly Jan 27, 2025

ccharly Jan 27, 2025

metamaskbot commented Jan 27, 2025

metamaskbot commented Jan 27, 2025

metamaskbot commented Jan 27, 2025

fix: bump @metamask/eth-ledger-bridge-keyring to ^8.0.3 to fix Ledger's handling of EIP-712 content #29820

fix: bump @metamask/eth-ledger-bridge-keyring to ^8.0.3 to fix Ledger's handling of EIP-712 content #29820

Conversation

dawnseeker8 commented Jan 21, 2025 • edited by Akaryatrh Loading

Description

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented Jan 21, 2025

socket-security bot commented Jan 21, 2025 • edited Loading

dawnseeker8 commented Jan 21, 2025

dawnseeker8 commented Jan 21, 2025

metamaskbot commented Jan 21, 2025

metamaskbot commented Jan 21, 2025

metamaskbot commented Jan 22, 2025

vivek-consensys commented Jan 23, 2025

dawnseeker8 commented Jan 23, 2025

metamaskbot commented Jan 23, 2025

metamaskbot commented Jan 23, 2025

socket-security bot commented Jan 23, 2025 • edited Loading

metamaskbot commented Jan 23, 2025

ccharly left a comment

Choose a reason for hiding this comment

ccharly Jan 27, 2025

Choose a reason for hiding this comment

ccharly Jan 27, 2025

Choose a reason for hiding this comment

metamaskbot commented Jan 27, 2025

metamaskbot commented Jan 27, 2025

metamaskbot commented Jan 27, 2025

fix: bump `@metamask/eth-ledger-bridge-keyring` to `^8.0.3` to fix Ledger's handling of EIP-712 content #29820

fix: bump `@metamask/eth-ledger-bridge-keyring` to `^8.0.3` to fix Ledger's handling of EIP-712 content #29820

dawnseeker8 commented Jan 21, 2025 •

edited by Akaryatrh

Loading

socket-security bot commented Jan 21, 2025 •

edited

Loading

socket-security bot commented Jan 23, 2025 •

edited

Loading