Skip to content

[FLASK] Implement offscreen execution service for running Snaps in MV3 #17008

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.

Sign up for GitHub

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

Jump to bottom
Closed
Mrtenz wants to merge 9 commits into from
Closed

[FLASK] Implement offscreen execution service for running Snaps in MV3 #17008

Mrtenz wants to merge 9 commits into from
+50 −4

Conversation

@Mrtenz
Copy link
Member

@Mrtenz Mrtenz commented Dec 20, 2022 •
edited
Loading

Explanation

This implements the offscreen execution service in order to run Snaps in MV3. It uses the offscreen document API to create a new window, where the snaps are run.

Manual Testing Steps

  • Run yarn start:mv3 --build-type=flask.
  • Install the extension in Google Chrome Canary.
  • Run a snap using test-snaps.

Pre-merge author checklist

  • I've clearly explained:
    • What problem this PR is solving
    • How this problem was solved
    • How reviewers can test my changes
  • Sufficient automated test coverage has been added

Pre-merge reviewer checklist

  • Manual testing (e.g. pull and build branch, run in browser, test code being changed)
  • PR is linked to the appropriate GitHub issue
  • IF this PR fixes a bug in the release milestone, add this PR to the release milestone

@Mrtenz Mrtenz added area-snaps flask needs-qa Label will automate into QA workspace labels Dec 20, 2022
@Mrtenz
Copy link
Member Author

Mrtenz commented Dec 20, 2022

Currently pending a merge and release of MetaMask/snaps#1082.

Mrtenz
Mrtenz commented Dec 20, 2022
View reviewed changes
package.json Outdated
"lockfile-lint-api@^5.4.6": "patch:lockfile-lint-api@npm%3A5.4.6#./.yarn/patches/lockfile-lint-api-npm-5.4.6-dc86b73900.patch",
"symbol-observable": "^2.0.3"
"symbol-observable": "^2.0.3",
"@metamask/post-message-stream": "^6.1.0"
Copy link
Member Author

@Mrtenz Mrtenz Dec 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be removed once MetaMask/snaps#1082 is merged and released.

@kenhkan
Copy link

kenhkan commented Jan 25, 2023

@Mrtenz MetaMask/snaps#1082 has been merged. What is the remaining action on this?

@brad-decker
Copy link
Contributor

brad-decker commented Apr 20, 2023

@Mrtenz hey there. I'm closing drafts that haven't received commits within the last 90 days. If this can be updated to be reconsidered please do that and reopen the PR!

@github-actions github-actions bot locked and limited conversation to collaborators Apr 20, 2023
@Mrtenz Mrtenz deleted the mrtenz/snaps-mv3 branch April 20, 2023 18:32
@Mrtenz Mrtenz restored the mrtenz/snaps-mv3 branch October 16, 2023 15:11
@Mrtenz Mrtenz reopened this Oct 16, 2023
@brad-decker
Copy link
Contributor

brad-decker commented Oct 17, 2023

@metamaskbot update-policies

@metamaskbot
Copy link
Collaborator

metamaskbot commented Oct 17, 2023

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

@Mrtenz
Copy link
Member Author

Mrtenz commented Oct 17, 2023

@metamaskbot update-policies

@MetaMask MetaMask unlocked this conversation Oct 17, 2023
@metamaskbot
Copy link
Collaborator

metamaskbot commented Oct 17, 2023

Policies updated

@brad-decker brad-decker added the team-extension-platform Extension Platform team label Oct 18, 2023
@brad-decker
Copy link
Contributor

brad-decker commented Oct 18, 2023

@metamaskbot update-policies

@metamaskbot
Copy link
Collaborator

metamaskbot commented Oct 18, 2023

Policies updated

@codecov
Copy link

codecov bot commented Oct 20, 2023 •
edited
Loading

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (845043a) 68.83% compared to head (7d0e814) 68.58%.

❗ Current head 7d0e814 differs from pull request most recent head 5bfd552. Consider uploading reports for the commit 5bfd552 to get more accurate results

Additional details and impacted files 
@@             Coverage Diff             @@
##           develop   #17008      +/-   ##
===========================================
- Coverage    68.83%   68.58%   -0.25%     
===========================================
  Files         1035     1030       -5     
  Lines        41194    41042     -152     
  Branches     10991    10964      -27     
===========================================
- Hits         28353    28147     -206     
- Misses       12841    12895      +54
Files Coverage Δ
app/scripts/metamask-controller.js 55.03% <70.00%> (-0.62%) ⬇️

... and 55 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@metamaskbot
Copy link
Collaborator

metamaskbot commented Oct 20, 2023

Builds ready [7d0e814]
Page Load Metrics (1444 ± 414 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint872711425024
domContentLoaded822161284120
load11224111444863414
domInteractive822161284120
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 198 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@brad-decker
Copy link
Contributor

brad-decker commented Oct 20, 2023

@Mrtenz I think i've got this working for the normal build types, but not sure if the MV3 route with snaps gets tested. Should we add that to this PR to ensure the offscreen communication is working for snaps?

@socket-security
Copy link

socket-security bot commented Oct 26, 2023

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

Packages Version New capabilities Transitives Size Publisher
@metamask/snaps-execution-environments 3.1.0 eval, network, environment +0 5.1 MB metamaskbot

@socket-security
Copy link

socket-security bot commented Oct 26, 2023

🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎

To accept the risk, merge this PR and you will not be notified again.

Issue Package Version Note Source
Network access @metamask/snaps-execution-environments 3.1.0

Next steps

What is network access?

This module accesses the network.

Packages should remove all network access that is functionally unnecessary. Consumers should audit network access to ensure legitimate use.

Take a deeper look at the dependency

Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev.

Remove the package

If you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency.

Mark a package as acceptable risk

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore foo@1.0.0 bar@* or ignore all packages with @SocketSecurity ignore-all

  • @SocketSecurity ignore @metamask/snaps-execution-environments@3.1.0

This was referenced Oct 31, 2023
Merged
Closed
@metamaskbot metamaskbot added in-progress and removed needs-qa Label will automate into QA workspace labels Nov 30, 2023
brad-decker added a commit that referenced this pull request Nov 30, 2023
@brad-decker
feat(offscreen): adds offscreen document (#21631)
8c2bed6 
## **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.
@github-actions
Copy link
Contributor

github-actions bot commented Jan 29, 2024

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.

@github-actions github-actions bot added the stale issues and PRs marked as stale label Jan 29, 2024
@brad-decker
Copy link
Contributor

brad-decker commented Jan 31, 2024

New implementation in #22732 borrowing heavily from this PR and with collaboration with snaps team

@github-actions github-actions bot removed the stale issues and PRs marked as stale label Feb 1, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Mar 4, 2024
@HowardBraham HowardBraham deleted the mrtenz/snaps-mv3 branch January 19, 2026 21:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@kumavis kumavis Awaiting requested review from kumavis kumavis will be requested when the pull request is marked ready for review kumavis is a code owner

Assignees

@Mrtenz Mrtenz

Labels

area-snaps flask in-progress team-extension-platform Extension Platform team

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@Mrtenz @kenhkan @brad-decker @metamaskbot @gauthierpetetin