[Bug]: MV3 - Signatures - Personal Sign not working with Ledger #24588
Labels
MV3
release-11.18.0
Issue or pull request that will be included in release 11.18.0
Sev2-normal
Normal severity; minor loss of service or inconvenience.
team-extension-platform
type-bug
Comments
seaona
added
type-bug
Sev2-normal
|
@seaona Do other signature types work with ledger on mv3? |
|
so:
|
7 tasks
danjm
added a commit
that referenced
this issue
May 22, 2024
…er bridge (#24622) ## **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 [](https://codespaces.new/MetaMask/metamask-extension/pull/24622?quickstart=1) ## **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](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [ ] 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](https://jsdoc.app/) format if applicable - [ ] 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)). 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.
](https://codespaces.new/MetaMask/metamask-extension/pull/24622?quickstart=1){kind=link}
metamaskbot
added
the
release-11.18.0
danjm
added a commit
that referenced
this issue
May 31, 2024
…er bridge (#24622) ## **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 [](https://codespaces.new/MetaMask/metamask-extension/pull/24622?quickstart=1) ## **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](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [ ] 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](https://jsdoc.app/) format if applicable - [ ] 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)). 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
added a commit
that referenced
this issue
May 31, 2024
…er bridge (#24622) ## **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 [](https://codespaces.new/MetaMask/metamask-extension/pull/24622?quickstart=1) ## **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](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [ ] 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](https://jsdoc.app/) format if applicable - [ ] 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)). 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
added a commit
that referenced
this issue
May 31, 2024
…er bridge (#24622) ## **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 [](https://codespaces.new/MetaMask/metamask-extension/pull/24622?quickstart=1) ## **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](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [ ] 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](https://jsdoc.app/) format if applicable - [ ] 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)). 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
added a commit
that referenced
this issue
May 31, 2024
…er bridge (#24622) ## **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 [](https://codespaces.new/MetaMask/metamask-extension/pull/24622?quickstart=1) ## **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](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [ ] 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](https://jsdoc.app/) format if applicable - [ ] 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)). 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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Describe the bug
Whenever I try to perform a Personal Sign with a ledger device using an MV3 build, I can see how it does not trigger anything in my ledger device.
Expected behavior
I should see the signature prompt into my ledger device
Screenshots/Recordings
personal-sign-ledger-mv.mp4
Steps to reproduce
Error messages or log output
No response
Version
develop MV3 build
Build type
None
Browser
Chrome
Operating system
Linux
Hardware wallet
No response
Additional context
No response
Severity
No response
The text was updated successfully, but these errors were encountered: