Skip to content

fix: Send correct message action from ledger offscreen bridge to ledger bridge#24622

Merged
danjm merged 1 commit intodevelopfrom
fix-personal-sign-mv3
May 22, 2024
Merged

fix: Send correct message action from ledger offscreen bridge to ledger bridge#24622
danjm merged 1 commit intodevelopfrom
fix-personal-sign-mv3

Conversation

@danjm
Copy link
Copy Markdown
Contributor

@danjm danjm commented May 20, 2024

Description

Fixes a bug. Personal signatures were broken for ledger on MV3. The problem was that the offscreen bridge named those message actions incorrectly. The ledger bridge expects 'ledger-sign-personal-message': https://github.com/MetaMask/eth-ledger-bridge-keyring/blob/gh-pages/ledger-bridge.js#L35-L37

This is consistent with the implementation of deviceSignMessage on the current iframe bridge: https://github.com/MetaMask/eth-ledger-bridge-keyring/blob/main/src/ledger-iframe-bridge.ts#L219-L226

Open in GitHub Codespaces

Related issues

Fixes: #24588

Manual testing steps

  1. Build MV3
  2. Import a ledger
  3. Go to the test dapp
  4. Try a Personal Sign
  5. Accept in MM
  6. You should see the signature on your ledger and be able to successfully sign

Pre-merge author checklist

  • I’ve followed MetaMask 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.

@danjm danjm requested a review from a team as a code owner May 20, 2024 12:51
@github-actions
Copy link
Copy Markdown
Contributor

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 metamaskbot added the INVALID-PR-TEMPLATE PR's body doesn't match template label May 20, 2024
@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [8126b6e]
Page Load Metrics (812 ± 545 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint682071013718
domContentLoaded9105202110
load5629858121134545
domInteractive9105202110
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 8 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 17 Bytes (0.00%)

@DDDDDanica
Copy link
Copy Markdown
Contributor

Good catch, LGTM !

@danjm danjm changed the title Send correct message action from ledger offscreen bridge to ledger bridge fix: Send correct message action from ledger offscreen bridge to ledger bridge May 22, 2024
@danjm danjm added the team-extension-platform Extension Platform team label May 22, 2024
@danjm danjm merged commit 7baba13 into develop May 22, 2024
@danjm danjm deleted the fix-personal-sign-mv3 branch May 22, 2024 16:09
@github-actions github-actions bot locked and limited conversation to collaborators May 22, 2024
@metamaskbot metamaskbot added release-11.16.6 Issue or pull request that will be included in release 11.16.6 and removed release-11.18.0 labels Jun 4, 2024
@metamaskbot
Copy link
Copy Markdown
Collaborator

Missing release label release-11.16.6 on PR. Adding release label release-11.16.6 on PR and removing other release labels(release-11.18.0), as PR was cherry-picked in branch 11.16.6.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

INVALID-PR-TEMPLATE PR's body doesn't match template release-11.16.6 Issue or pull request that will be included in release 11.16.6 team-extension-platform Extension Platform team

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

[Bug]: MV3 - Signatures - Personal Sign not working with Ledger

4 participants