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.

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

feat: Partially local WebView for Snaps #10214

Merged
merged 20 commits into from
Jul 19, 2024
Merged

feat: Partially local WebView for Snaps #10214

merged 20 commits into from
Jul 19, 2024

Conversation

FrederikBolding
Copy link
Member

Description

Replaces the remote WebView with a locally bundled WebView. This WebView still uses a remote iframe for execution, but the initial load will be using all local code. Dependant on MetaMask/snaps#2528

Also moves the SnapsExecutionWebView to the App index to have it available even when the application is locked, also prevents issues with breaking the execution environment when locking the app.

@FrederikBolding FrederikBolding added the team-snaps-platform Snaps Platform team label Jul 2, 2024
@metamaskbot metamaskbot added the INVALID-PR-TEMPLATE PR's body doesn't match template label Jul 2, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jul 9, 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.

@socket-security Socket Security
Copy link

socket-security bot commented Jul 9, 2024 •
edited
Loading

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

Package New capabilities Transitives Size Publisher
npm/@metamask/[email protected] None 0 5.46 MB metamaskbot

View full report↗︎

@Jonathansoufer Jonathansoufer marked this pull request as ready for review July 11, 2024 15:08
@Jonathansoufer Jonathansoufer requested review from a team as code owners July 11, 2024 15:08
@Jonathansoufer Jonathansoufer self-assigned this Jul 11, 2024
Jonathansoufer
Jonathansoufer previously approved these changes Jul 11, 2024
View reviewed changes
Copy link
Contributor

@Jonathansoufer Jonathansoufer left a comment

Choose a reason for hiding this comment

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

LGTM!

tommasini
tommasini previously approved these changes Jul 12, 2024
View reviewed changes
@Jonathansoufer Jonathansoufer requested a review from tommasini July 12, 2024 14:07
tommasini
tommasini approved these changes Jul 12, 2024
View reviewed changes
Copy link
Contributor

@tommasini tommasini left a comment

Choose a reason for hiding this comment

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

LGTM! It's missing a QA label, since this is not covered by E2E I would suggest add the label "no qa needed"

@Jonathansoufer Jonathansoufer added the No QA Needed Apply this label when your PR does not need any QA effort. label Jul 12, 2024
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Jul 19, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@Jonathansoufer Jonathansoufer added the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Jul 19, 2024
@Jonathansoufer Jonathansoufer requested review from Jonathansoufer and removed request for Jonathansoufer July 19, 2024 11:43
@Jonathansoufer Jonathansoufer requested a review from naugtur July 19, 2024 12:06
@Jonathansoufer Jonathansoufer mentioned this pull request Jul 19, 2024
Open
@Jonathansoufer Jonathansoufer self-requested a review July 19, 2024 12:15
Jonathansoufer
Jonathansoufer approved these changes Jul 19, 2024
View reviewed changes
Copy link
Contributor

@Jonathansoufer Jonathansoufer left a comment

Choose a reason for hiding this comment

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

Approved on behalf of Snaps team since we got already two reviewers from other teams.

@Jonathansoufer Jonathansoufer merged commit aa749b4 into main Jul 19, 2024
35 checks passed
@Jonathansoufer Jonathansoufer deleted the fb/snaps-partially-local-webview branch July 19, 2024 12:16
@github-actions github-actions bot locked and limited conversation to collaborators Jul 19, 2024
@github-actions github-actions bot removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Jul 19, 2024
@metamaskbot metamaskbot added the release-7.28.0 Issue or pull request that will be included in release 7.28.0 label Jul 19, 2024
@naugtur
Copy link

naugtur commented Oct 7, 2024

@FrederikBolding @Jonathansoufer
Here's what people seem to be using to make the html string be considered a secure context: https://github.com/webview-crypto/react-native-webview-crypto/blob/master/index.js#L176

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

@tommasini tommasini tommasini approved these changes

@Jonathansoufer Jonathansoufer Jonathansoufer approved these changes

@naugtur naugtur naugtur approved these changes

Assignees

@Jonathansoufer Jonathansoufer

Labels
INVALID-PR-TEMPLATE PR's body doesn't match template No QA Needed Apply this label when your PR does not need any QA effort. release-7.28.0 Issue or pull request that will be included in release 7.28.0 team-snaps-platform Snaps Platform team
Projects
PR review queue
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@FrederikBolding @naugtur @Jonathansoufer @tommasini @metamaskbot