mv3 HW support using offscreen #17687
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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. |
bergarces
force-pushed
the
offscreen-trezor
branch
from
9429b4e to
068fb19
Compare
bergarces
force-pushed
the
offscreen-trezor
branch
2 times, most recently
from
444af14 to
9bd2ae5
Compare
bergarces
changed the title
bergarces
force-pushed
the
offscreen-trezor
branch
3 times, most recently
from
14df1ec to
33abd2a
Compare
Contributor
|
Fixes #14847 |
danjm
added
team-extension-client
team-extension-platform
|
👍 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. |
|
Updated dependencies detected. Learn more about Socket for GitHub ↗︎
|
Contributor
|
This PR has been automatically marked as stale because it has not had recent activity in the last 60 days. It will be closed in 14 days. Thank you for your contributions. |
Contributor
|
This PR was closed because there has been no follow up activity in the last 14 days. Thank you for your contributions. |
brad-decker
force-pushed
the
offscreen-trezor
branch
from
c2305a9 to
ab06381
Compare
This was referenced Oct 31, 2023
brad-decker
added a commit
that referenced
this pull request
Nov 30, 2023
## **Description** For MV3 snaps and Hardware wallet functionality we need to run some sets of code in an offscreen document that can communicate with the extension. The purpose of this Pull request is to start the conversation about how to build this. This PR should be a stepping stone for the fundamentals of #17687 and #17008 to incrementally be adopted. The idea here is that we create a new "entry point" into the application in the way of this offscreen.html file. There is a offscreen.ts file that can be used to initialize a PostMessageStream for the iframe itself, which is done in #17008. Essentially the snaps-execution-environment offscreen document sets up lockdown and then initializes the OffscreenSnapExecutor with its PostMessageStream. These pieces can be added in a future PR, although I might need some guidance on the proper way to setup lockdown. In terms of Hardware wallet support, the iframes in #17687 and the supporting typescript files can be included in layers ontop of this folder structure, eventually getting to full hardware wallet support but we should tackle these one at a time. There are a couple PRs opened by @mikesposito to upgrade trezor and ledger keyrings that are also piecing out elements from #17687 that are prereqs for continuing HW support in MV3. This uses post-message-stream which will later be used as well for snaps execution in the offscreen document. ## **Related issues** Partially progresses #21622 ## **Manual testing steps** 1. run `yarn start:mv3` 2. Go to extensions in chrome. 3. You should see both a service-worker and offscreen.html file loaded and be able to inspect both. 4. Open both windows. From the service-worker console, run `chrome.runtime.sendMessage({target: 'child', data: 'test'})` 5. See that the offscreen.html console logs the message. 6. From the offscreen console, run `chrome.runtime.sendMessage({target: 'parent', data: 'test'})` 7. See that the service worker console logs the message. ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** <!-- [screenshots/recordings] --> ### **After** <!-- [screenshots/recordings] --> ## **Pre-merge author checklist** - [x] I’ve followed [MetaMask Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [x] I've clearly explained what problem this PR is solving and how it is solved. - [x] I've linked related issues - [x] I've included manual testing steps - [x] I've included screenshots/recordings if applicable - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] 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. - [x] I’ve properly set the pull request status: - [ ] In case it's not yet "ready for review", I've set it to "draft". - [x] In case it's "ready for review", I've changed it from "draft" to "non-draft". ## **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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Explanation
This PR aims to implement support for hardware wallets on MV3 using Chrome's Offscreen API.
Snaps
The idea is that hardware wallets are dealt with within an iframe inside the offscreen page. That is to ensure there's no conflict with snaps use of the offscreen api. As shown in this PR, in order to enable the snaps offscreen execution environment, an
offscreen.htmlalong with a javascript bundle is imported. Since this PR will be creating theoffscreen.html, that'll have to change so that only the JavaScript bundles are imported.Keyrings
Before this PR can be merged, the two associated PRs to split keyring and bridge logic from trezor and ledger keyrings need to be merged.
Those PRs were motivated by the issues faced implementing the MetaMask Desktop companion app, but they are useful in this case as well, since the SDKs rely on DOM calls that are not present within MV3 service-workers.
Lavamoat
The iframe code is currently bundled separately from the rest and it's not being bundled with lavamoat. Currently, that code only uses one dependency (
@trezor/connect-web), although it should be possible to use no dependency at all if we import the code directly in the iframe from their page (https://connect.trezor.io/9/trezor-connect.js).Trezor
The Trezor case is the simplest. A keyring bridge that forwards a message to the offscren iframe, which uses the TrezorConnect SDK the same way that it is used in the existing MV2 bridge. The SDK needs to be initialised with
env: 'web'to prevent the use of extension APIs that are not supported in offscreen pages.Ledger
The ledger case uses the existing live iframe (https://github.com/MetaMask/eth-ledger-bridge-keyring/blob/gh-pages/ledger-bridge.js). The offscreen ledger iframe just forwards the calls there and awaits a response.
Lattice
For the Lattice case, it was enough to overwrite the
_getCredsmethod of the existing keyring. This solution, however, doesn't take into consideration that the sdk session is stored in the service-worker and, therefore, is bound to be cleared once it becomes inactive. A more robust solution should be possible by splitting the Lattice keyring code in the same fashion as Ledger and Lattice, so that functionality can be moved to the offscreen iframe if needed.Splitting PR
It should be possible to split this PR in smaller ones for each hardware wallet, which should be easier to review.
Manual Testing Steps
For each hardware wallet (trezor, ledger and lattice), the following actions need to be tested for both MV2 and MV3 versions of the extension:
Pre-merge author checklist
Pre-merge reviewer checklist
If further QA is required (e.g. new feature, complex testing steps, large refactor), add the
Extension QA Boardlabel.In this case, a QA Engineer approval will be be required.