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.

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

feat: snaps controllers integration (Flask Only) #7942

Merged
merged 56 commits into from
Dec 13, 2023

Conversation

owencraston
Copy link
Contributor

@owencraston owencraston commented Nov 29, 2023

Description

This pr enables snaps to be installed and executed on mobile. This change will be code fenced for flask only. We achieve this by doing the following..

All of these changes have been implemented and tested in this PR. This PR updates that logic and brings it into main behind a code fence.

Guide for reviewers

  • All modified code should be wrapped inside a code fence ///: BEGIN:ONLY_INCLUDE_IF(snaps)...///: END:ONLY_INCLUDE_IF
  • all new files should be wrapped in a code fence ///: BEGIN:ONLY_INCLUDE_IF(snaps)...///: END:ONLY_INCLUDE_IF
  • The most important files for to review are...
    1. Engine.ts
    2. BackgroundBridge.js
    3. useApprovalRequest.ts
    4. EngineService.ts
    5. npm.ts
    6. location.ts
    7. fetch.ts

Related issues

Progresses: https://github.com/MetaMask/accounts-planning/issues/143

Manual testing steps

  1. checkout this branch
  2. run yarn setup
  3. open the .js.env
  4. modify the METAMASK_BUILD_TYPE to export METAMASK_BUILD_TYPE="flask"
  5. in your terminal run source .js.env and then run yarn watch:clean
  6. in xcode select the flask variant and press play
  7. open http://metamask.github.io/snaps/test-snaps/1.1.0 in the browser
Screenshot 2023-12-08 at 12 04 41 AM
  1. create a wallet
  2. navigate to the browser
  3. open this url: https://metamask.github.io/snaps/test-snaps/1.1.0/
  4. install a snap, I usually install the bip32/bip44 snap
  5. the install flow should appear with 3 steps including the a success screen
  6. click the Get Public key button on the snap site, you should get a result
  7. navigate to settings/snaps
  8. you should see a list of all your installed snaps

Checking code fencing

  1. reset your .js.env file so that the METAMASK_BUILD_TYPE is set to main
  2. restart your watchman server and refresh the app
  3. the snaps section in the settings list should no longer exist
  4. installing a snap should fail.

Screenshots/Recordings

After

Screen.Recording.2023-12-07.at.11.42.58.PM.mov

Pre-merge author checklist

  • I’ve followed MetaMask Coding Standards.
  • I've clearly explained what problem this PR is solving and how it is solved.
  • I've linked related issues
  • I've included manual testing steps
  • I've included screenshots/recordings if applicable
  • 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.
  • I’ve properly set the pull request status:
    • In case it's not yet "ready for review", I've set it to "draft".
    • 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.

Copy link
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.

Copy link

socket-security bot commented Nov 29, 2023

Copy link

socket-security bot commented Nov 29, 2023

👍 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.

Ignoring: [email protected], @metamask/[email protected], @metamask/[email protected], @metamask/[email protected]

Next steps

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 [email protected] bar@* or ignore all packages with @SocketSecurity ignore-all

@owencraston owencraston force-pushed the feat/snaps-controllers-integration branch 2 times, most recently from b20703d to 111f7dd Compare November 30, 2023 23:31
@owencraston owencraston changed the base branch from main to rntar-native-modules November 30, 2023 23:32
@owencraston owencraston changed the title Feat/snaps controllers integration Feat: snaps controllers integration (Flask Only) Dec 1, 2023
Base automatically changed from rntar-native-modules to main December 1, 2023 05:15
owencraston added a commit that referenced this pull request Dec 1, 2023
## **Description**

This PR adds a custom native module RNTar. This module takes a file
system path to a `.tgz` (tar gzip) file. The module reads this file,
decompresses it and places it in the target directory. It then returns
the new location of the decompressed data. This PR is needed for the
mobile snaps implementation and will be used in this following
[PR](#7942).

- All of the javascript code is code fenced so non of it will appear in
the main MetaMask app
- the native modules cannot be code fenced so they will be included in
the main app. They are never called from the UI so it should not
interfere.

Most of the native code (java and swift) was implemented and reviewed in
previous prs. This change is bringing them into main.

- [Original Android
PR](#6300)
- [Original iOS
PR](#5926)

## **Related issues**

Progresses: MetaMask/accounts-planning#143

## **Manual testing steps**
- clone this branch
- run `yarn setup`
- open `js.env`
- change `export METAMASK_BUILD_TYPE` to `export
METAMASK_BUILD_TYPE="flask"`
- run `source .js.env`
- run `yarn watch:clean`
- run the app in ios or android
- navigate to settings/Snaps
- click the `Test RNTar` button
- it should succeed and log the output directory
- if you open the output directory (logged in the console) you should
see the package folder with all of the code for the
[filesnap](https://github.com/filecoin-project/filsnap)
- once you have confirmed that the module is properly decompressed, go
back to `.js.env`
- change `METAMASK_BUILD_TYPE` back to `main`
- run `source .js.env`
- reload watchman (`yarn watch:clean`)
- navigate back to settings
- the `Snaps` section should no longer be present
- repeat these steps with the opposite operating system


### Running android unit tests
- open this branch in Android studio
- open the `RNTarTest.java`
- run the test suite
- it should pass

<img width="1728" alt="Screenshot 2023-11-29 at 11 45 27 PM"
src="https://github.com/MetaMask/metamask-mobile/assets/22918444/982b899e-fc9c-46ed-9ffe-84af5e9c3f89">

## **Screenshots/Recordings**

### **After**


https://github.com/MetaMask/metamask-mobile/assets/22918444/2b95b1c1-54d7-4060-8f95-3d8d565b0203



https://github.com/MetaMask/metamask-mobile/assets/22918444/273be30e-c39d-4906-8828-04b8e93ca2af


## **Pre-merge author checklist**

- [x] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.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-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.
- [x] I’ve properly set the pull request status:
  - [x] In case it's not yet "ready for review", I've set it to "draft".
- [ ] In case it's "ready for review", I've changed it from "draft" to
"non-draft".

## **Pre-merge reviewer checklist**

- [x] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [x] 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.
@owencraston owencraston force-pushed the feat/snaps-controllers-integration branch 2 times, most recently from e2b8774 to 8e46e4a Compare December 5, 2023 00:27
app/core/Snaps/WebviewExecutionService.ts Show resolved Hide resolved
app/core/Snaps/location/npm.ts Show resolved Hide resolved
app/util/snaps/checkSnapsBlockList.js Outdated Show resolved Hide resolved
app/core/Snaps/location/http.ts Outdated Show resolved Hide resolved
app/core/Engine.ts Outdated Show resolved Hide resolved
app/core/Snaps/SnapsMethodMiddleware.ts Outdated Show resolved Hide resolved
app/constants/permissions.ts Outdated Show resolved Hide resolved
app/core/Permissions/constants.ts Outdated Show resolved Hide resolved
app/core/RPCMethods/RPCMethodMiddleware.ts Outdated Show resolved Hide resolved
@owencraston owencraston force-pushed the feat/snaps-controllers-integration branch 2 times, most recently from a6499c5 to eb97784 Compare December 7, 2023 03:01
app/core/Engine.ts Outdated Show resolved Hide resolved
@owencraston owencraston force-pushed the feat/snaps-controllers-integration branch from fd75952 to d354a18 Compare December 7, 2023 20:48
@owencraston owencraston changed the title Feat: snaps controllers integration (Flask Only) feat: snaps controllers integration (Flask Only) Dec 8, 2023
@owencraston owencraston marked this pull request as ready for review December 8, 2023 08:31
@owencraston owencraston requested a review from a team as a code owner December 8, 2023 08:31
Copy link
Contributor

github-actions bot commented Dec 8, 2023

E2E test started on Bitrise: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/077d55c7-03bd-4338-bff4-272f5cd151f2
You can also kick off another Bitrise E2E smoke test by removing and re-applying the (Run Smoke E2E) label

Copy link
Contributor

github-actions bot commented Dec 8, 2023

E2E test started on Bitrise: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/48b3f443-236a-4d09-ba0d-a398f8029316
You can also kick off another Bitrise E2E smoke test by removing and re-applying the (Run Smoke E2E) label

Jonathansoufer
Jonathansoufer previously approved these changes Dec 8, 2023
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! Left some comments.

app/components/UI/SnapsExecutionWebView/styles.ts Outdated Show resolved Hide resolved
app/constants/snaps.ts Show resolved Hide resolved
app/core/Engine.ts Show resolved Hide resolved
app/core/Engine.ts Show resolved Hide resolved
app/core/Engine.ts Show resolved Hide resolved
app/core/Engine.ts Show resolved Hide resolved
app/core/Snaps/WebviewExecutionService.ts Show resolved Hide resolved
Copy link
Contributor

E2E test started on Bitrise: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/dcaaa896-5313-4c87-9f6e-bff67a06f05e
You can also kick off another Bitrise E2E smoke test by removing and re-applying the (Run Smoke E2E) label

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.

Engine, RpcMethodMiddleware, EngineService, BackgroundBridge and Engine LGTM!

Cool thing that we use code fencing only for snaps and this way we can create easily other feature to include on the diferent featureSet!
Awesome work going on 🚀

@owencraston owencraston dismissed FrederikBolding’s stale review December 12, 2023 22:29

we have decided to iteratively ship improvements to this implementation after it merges.

@owencraston
Copy link
Contributor Author

e2e smoke test passed

gantunesr
gantunesr previously approved these changes Dec 12, 2023
Copy link
Member

@gantunesr gantunesr left a comment

Choose a reason for hiding this comment

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

The overall code looks good to me, there are a lot of improvements to be done but we need to take into consideration that this is the first iteration of the code working and this is a great step into opening the snaps platform in the mobile app to the dev community. Since it is behind code fencing I don't have any reason to not merge it as it is and start working on smaller refactors that can be shipped faster and stop dedicating time into constant rebases.

The comments I left in the PR are suggestion I'd like to see done but I don't mind it being addressed in a follow up PR.

To do in a follow up PR:

  • Address pending comments
  • Add unit tests to all files in core/Snaps
  • The Engine is getting too big, this code would be a good candidate to break it down to another file

Great work @owencraston 🎉 🚢

app/core/Snaps/location/npm.ts Outdated Show resolved Hide resolved
patches/@metamask+post-message-stream+7.0.0.patch Outdated Show resolved Hide resolved
app/core/Snaps/location/fetch.ts Outdated Show resolved Hide resolved
app/core/Snaps/WebviewPostMessageStream.ts Outdated Show resolved Hide resolved
app/core/Snaps/WebviewPostMessageStream.ts Outdated Show resolved Hide resolved
app/core/Snaps/WebviewPostMessageStream.ts Show resolved Hide resolved
app/core/Snaps/WebviewExecutionService.ts Outdated Show resolved Hide resolved
app/core/Snaps/SnapWebviewPostMessageStream.ts Outdated Show resolved Hide resolved
Jonathansoufer
Jonathansoufer previously approved these changes Dec 13, 2023
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.

Excellent work putting all this together @owencraston ! Minor comments of mine, but approved since they can be addressed in a follow up PR.

Copy link

sonarcloud bot commented Dec 13, 2023

Quality Gate Failed Quality Gate failed

Failed conditions

4.9% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

@owencraston
Copy link
Contributor Author

Copy link
Contributor

E2E test started on Bitrise: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/908dbf9e-632e-4d36-8059-ecbd4f3212bc
You can also kick off another Bitrise E2E smoke test by removing and re-applying the (Run Smoke E2E) label

@sethkfman sethkfman merged commit d7869a6 into main Dec 13, 2023
28 of 29 checks passed
@sethkfman sethkfman deleted the feat/snaps-controllers-integration branch December 13, 2023 02:31
@github-actions github-actions bot locked and limited conversation to collaborators Dec 13, 2023
@metamaskbot metamaskbot added the release-7.15.0 Issue or pull request that will be included in release 7.15.0 label Dec 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
No QA Needed Apply this label when your PR does not need any QA effort. release-7.15.0 Issue or pull request that will be included in release 7.15.0 team-accounts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants