feat: Enable Ledger clear signing feature #28909

dawnseeker8 · 2024-12-04T01:53:09Z

Description

This PR enable the clear signing feature in metamask extension.

Please refer to this feature requests for detail: https://github.com/MetaMask/accounts-planning/issues/544

Related issues

Fixes: https://github.com/MetaMask/accounts-planning/issues/544

Manual testing steps

Test the clear signing using this dapp provided by ledger team: https://clear-signing-tester.vercel.app/
EIP712 sign message (Polygon mainnet or Ethereum mainnet).
Sign transaction (Linea testnet).
Swap in a layer 2 chain like Linea or Polygon.

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.

github-actions · 2024-12-04T01:53:22Z

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 · 2024-12-04T01:54:26Z

No dependency changes detected. Learn more about Socket for GitHub ↗︎

👍 No dependency changes detected in pull request

dawnseeker8 · 2024-12-04T07:34:21Z

@SocketSecurity ignore npm/[email protected]

danjm · 2024-12-17T14:11:55Z

@metamaskbot update-policies

metamaskbot · 2024-12-17T14:20:49Z

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

…ait for lib upgrade a release.

…patch to work before library upgrade.

Akaryatrh · 2024-12-18T12:09:34Z

@metamaskbot update-policies

metamaskbot · 2024-12-18T12:23:02Z

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 · 2024-12-18T12:57:36Z

Builds ready [32e5e57]

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, 5, 6, 7
- common: 0, 1, 10, 11, 12, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 12, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1700 ± 93 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	357	2004	1611	336	161
		domContentLoaded	1423	2055	1674	188	90
		load	1468	2094	1700	194	93
		domInteractive	23	81	42	19	9
		backgroundConnect	9	77	26	21	10
		firstReactRender	17	81	44	24	12
		getState	5	47	11	12	6
		initialActions	0	1	0	0	0
		loadScripts	1024	1515	1239	157	75
		setupStore	6	10	8	1	1
		uiStartup	1655	2385	1966	236	113

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

background: 71.09 KiB (1.30%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

vivek-consensys · 2024-12-20T10:39:27Z

Tested on local build using Ledger Nano X and Ledger Stax. Flows tested on the clear signing test dapp include:-

Send ETH (Linea Sepolia)
Sign EIP-712 : Pemit 2 & 1inch Fusion typed messages (Polygon Mainnet)

Also, regular flows tested on test-dapp include:-

Personal sign
Sign typed data
Sign typed data v4
Sign permit
SIWE

dawnseeker8 · 2025-01-07T12:16:26Z

@metamaskbot update-policies

metamaskbot · 2025-01-07T12:29:04Z

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

offscreen/scripts/ledger.ts

metamaskbot · 2025-01-07T13:02:25Z

Builds ready [440bc2f]

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: 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, 10, 11, 12, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 12, 13, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1558 ± 37 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	1451	1721	1560	77	37
		domContentLoaded	1422	1659	1531	68	33
		load	1431	1718	1558	78	37
		domInteractive	22	176	43	37	18
		backgroundConnect	8	71	32	22	10
		firstReactRender	16	103	38	28	13
		getState	5	35	9	6	3
		initialActions	0	1	0	0	0
		loadScripts	1027	1259	1126	65	31
		setupStore	6	87	14	20	10
		uiStartup	1649	2436	1783	165	79

vivek-consensys · 2025-01-08T07:02:42Z

LGTM. Did @vivek-consensys tests include both iOS and Android? We should also test swaps

Hey @gantunesr , this is the PR for MM extension. Mobile PR has already been merged and included in the 7.38.0 RC

gantunesr · 2025-01-08T14:10:40Z

Hey @gantunesr , this is the PR for MM extension. Mobile PR has already been merged and included in the 7.38.0 RC

My bad @vivek-consensys, will hide the other comment as it irrelevant. Were you able to test swaps?

ccharly

Code-wise LGTM. I haven't tested myself, but this feature has been QA'd already.

metamaskbot · 2025-01-09T12:07:45Z

Builds ready [fc9aa89]

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, 10, 11, 12, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 12, 13, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1656 ± 63 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	274	2030	1529	392	188
		domContentLoaded	1477	1934	1626	113	54
		load	1491	2027	1656	131	63
		domInteractive	25	198	57	48	23
		backgroundConnect	8	99	31	29	14
		firstReactRender	16	100	40	31	15
		getState	5	89	17	22	11
		initialActions	0	1	0	0	0
		loadScripts	1079	1491	1215	102	49
		setupStore	6	83	25	26	12
		uiStartup	1677	2666	1968	295	142

metamaskbot · 2025-01-09T14:50:54Z

Builds ready [71ae932]

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, 10, 11, 12, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 12, 13, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1663 ± 90 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	1368	2244	1664	191	92
		domContentLoaded	1362	2167	1639	178	85
		load	1370	2236	1663	187	90
		domInteractive	20	155	52	37	18
		backgroundConnect	7	71	26	20	9
		firstReactRender	15	86	37	24	12
		getState	3	87	18	22	11
		initialActions	0	1	0	0	0
		loadScripts	1004	1678	1233	152	73
		setupStore	6	51	13	13	6
		uiStartup	1521	2548	1919	243	116

dawnseeker8 added the team-hardware-wallets label Dec 4, 2024

dawnseeker8 changed the title ~~Feat: ledger clear signing~~ feat: Enable Ledger clear signing feature Dec 4, 2024

Akaryatrh force-pushed the feat/ledger-clear-signing branch from 844163a to 93d3a8c Compare December 17, 2024 10:29

Akaryatrh added the needs-qa Label will automate into QA workspace label Dec 17, 2024

Akaryatrh marked this pull request as ready for review December 17, 2024 10:35

Akaryatrh requested review from a team as code owners December 17, 2024 10:35

metamaskbot added the INVALID-PR-TEMPLATE PR's body doesn't match template label Dec 17, 2024

Akaryatrh force-pushed the feat/ledger-clear-signing branch from 93d3a8c to 717c864 Compare December 17, 2024 11:39

dawnseeker8 and others added 10 commits December 18, 2024 09:35

feat: make a backup of ledger clear signing implementation, need to w…

fae2903

…ait for lib upgrade a release.

feat: add code for clear signing in metamask extension. will require …

a09ca8e

…patch to work before library upgrade.

feat: update URL to use custom github page to test clear signing.

d34e35c

feat: use preview build for eth-ledger-bridge-keyring to test.

e2bfb72

feat: upgrade preview build version.

d23ba33

feat: upgrade to use latest eth-ledger-bridge-keyring

9a630ea

feat: merge from main

a7acabf

feat: use preview build for eth-ledger-bridge-keyring to test.

1aef785

feat: switch to v8.0.0 released version

d1df9ac

build: update yarn.lock and policies

aec3d12

Akaryatrh force-pushed the feat/ledger-clear-signing branch from ed517c9 to aec3d12 Compare December 18, 2024 08:44

Update LavaMoat policies

32e5e57

feat: Revert to main version.

13517a4

dawnseeker8 and others added 2 commits January 7, 2025 20:16

Merge branch 'main' into feat/ledger-clear-signing

53afe91

Update LavaMoat policies

440bc2f

dawnseeker8 commented Jan 7, 2025

View reviewed changes

offscreen/scripts/ledger.ts Show resolved Hide resolved

This comment was marked as off-topic.

Sign in to view

ccharly approved these changes Jan 9, 2025

View reviewed changes

vivek-consensys added this pull request to the merge queue Jan 9, 2025

dawnseeker8 removed this pull request from the merge queue due to a manual request Jan 9, 2025

dawnseeker8 added this pull request to the merge queue Jan 9, 2025

dawnseeker8 removed this pull request from the merge queue due to a manual request Jan 9, 2025

Merge branch 'main' into feat/ledger-clear-signing

fc9aa89

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

dawnseeker8 added 3 commits January 9, 2025 21:10

Merge branch 'main' into feat/ledger-clear-signing

fb2a897

Merge branch 'main' into feat/ledger-clear-signing

dcec975

Merge branch 'main' into feat/ledger-clear-signing

71ae932

dawnseeker8 added this pull request to the merge queue Jan 9, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 9, 2025

dawnseeker8 added this pull request to the merge queue Jan 9, 2025

Merged via the queue into main with commit 2d443ff Jan 9, 2025
77 checks passed

dawnseeker8 deleted the feat/ledger-clear-signing branch January 9, 2025 17:16

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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Enable Ledger clear signing feature #28909

feat: Enable Ledger clear signing feature #28909

dawnseeker8 commented Dec 4, 2024 •

edited by gantunesr

Loading

github-actions bot commented Dec 4, 2024

socket-security bot commented Dec 4, 2024 •

edited

Loading

dawnseeker8 commented Dec 4, 2024

danjm commented Dec 17, 2024

metamaskbot commented Dec 17, 2024

Akaryatrh commented Dec 18, 2024

metamaskbot commented Dec 18, 2024

metamaskbot commented Dec 18, 2024

vivek-consensys commented Dec 20, 2024 •

edited

Loading

dawnseeker8 commented Jan 7, 2025

metamaskbot commented Jan 7, 2025

metamaskbot commented Jan 7, 2025

This comment was marked as off-topic.

vivek-consensys commented Jan 8, 2025

gantunesr commented Jan 8, 2025 •

edited

Loading

ccharly left a comment

metamaskbot commented Jan 9, 2025

metamaskbot commented Jan 9, 2025

feat: Enable Ledger clear signing feature #28909

feat: Enable Ledger clear signing feature #28909

Conversation

dawnseeker8 commented Dec 4, 2024 • edited by gantunesr Loading

Description

Related issues

Manual testing steps

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented Dec 4, 2024

socket-security bot commented Dec 4, 2024 • edited Loading

dawnseeker8 commented Dec 4, 2024

danjm commented Dec 17, 2024

metamaskbot commented Dec 17, 2024

Akaryatrh commented Dec 18, 2024

metamaskbot commented Dec 18, 2024

metamaskbot commented Dec 18, 2024

vivek-consensys commented Dec 20, 2024 • edited Loading

dawnseeker8 commented Jan 7, 2025

metamaskbot commented Jan 7, 2025

metamaskbot commented Jan 7, 2025

This comment was marked as off-topic.

vivek-consensys commented Jan 8, 2025

gantunesr commented Jan 8, 2025 • edited Loading

ccharly left a comment

Choose a reason for hiding this comment

metamaskbot commented Jan 9, 2025

metamaskbot commented Jan 9, 2025

dawnseeker8 commented Dec 4, 2024 •

edited by gantunesr

Loading

socket-security bot commented Dec 4, 2024 •

edited

Loading

vivek-consensys commented Dec 20, 2024 •

edited

Loading

gantunesr commented Jan 8, 2025 •

edited

Loading