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

feat: Enable Ledger clear signing feature #28909

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

dawnseeker8
Copy link
Contributor

@dawnseeker8 dawnseeker8 commented Dec 4, 2024

Description

This PR enable the clear signing feature in metamask mobile.
Please refer to this feature requests for detail: https://github.com/MetaMask/accounts-planning/issues/544

Open in GitHub Codespaces

Related issues

Fixes:

Manual testing steps

Test the clear signing using this dapp provided by ledger team:
https://clear-signing-tester.vercel.app/
When test in EIP712 sign message, please use polygon mainnet or ethereum mainnet.
sign transaction can use linea testnet.

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.

Copy link
Contributor

github-actions bot commented Dec 4, 2024

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.

Copy link

socket-security bot commented Dec 4, 2024

👍 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 dawnseeker8 changed the title Feat: ledger clear signing feat: Enable Ledger clear signing feature Dec 4, 2024
@dawnseeker8
Copy link
Contributor Author

@SocketSecurity ignore npm/[email protected]

Copy link

socket-security bot commented Dec 10, 2024

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/@metamask/[email protected] None +1 449 kB metamaskbot

🚮 Removed packages: npm/@metamask/[email protected]

View full report↗︎

@Akaryatrh Akaryatrh force-pushed the feat/ledger-clear-signing branch from 844163a to 93d3a8c Compare December 17, 2024 10:29
@Akaryatrh Akaryatrh added the needs-qa Label will automate into QA workspace label Dec 17, 2024
@Akaryatrh Akaryatrh marked this pull request as ready for review December 17, 2024 10:35
@Akaryatrh Akaryatrh requested review from a team as code owners December 17, 2024 10:35
@metamaskbot metamaskbot added the INVALID-PR-TEMPLATE PR's body doesn't match template label Dec 17, 2024
@Akaryatrh Akaryatrh force-pushed the feat/ledger-clear-signing branch from 93d3a8c to 717c864 Compare December 17, 2024 11:39
@danjm
Copy link
Contributor

danjm commented Dec 17, 2024

@metamaskbot update-policies

@metamaskbot
Copy link
Collaborator

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

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

@metamaskbot update-policies

@metamaskbot
Copy link
Collaborator

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
Copy link
Collaborator

Builds ready [32e5e57]
Page Load Metrics (1700 ± 93 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint35720041611336161
domContentLoaded14232055167418890
load14682094170019493
domInteractive238142199
backgroundConnect977262110
firstReactRender1781442412
getState54711126
initialActions01000
loadScripts10241515123915775
setupStore610811
uiStartup165523851966236113
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
Copy link

vivek-consensys commented Dec 20, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
INVALID-PR-TEMPLATE PR's body doesn't match template needs-qa Label will automate into QA workspace team-hardware-wallets
Projects
Status: Needs dev review
Development

Successfully merging this pull request may close these issues.

5 participants