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: RNTar native modules (Android and iOS) #7955

Merged
merged 7 commits into from
Dec 1, 2023
Merged

Conversation

owencraston
Copy link
Contributor

@owencraston owencraston commented Nov 30, 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.

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

Related issues

Progresses: https://github.com/MetaMask/accounts-planning/issues/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
  • 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
Screenshot 2023-11-29 at 11 45 27 PM

Screenshots/Recordings

After

720.copy.Screen.Recording.2023-11-29.at.11.20.20.PM.mov
Screen.Recording.2023-11-30.at.10.44.04.AM.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.

@codecov-commenter
Copy link

codecov-commenter commented Nov 30, 2023

Codecov Report

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

Comparison is base (196ebc5) 36.64% compared to head (2243c06) 36.61%.

Files Patch % Lines
...iews/Snaps/SnapsSettingsList/SnapsSettingsList.tsx 8.82% 31 Missing ⚠️
...naps/SnapsSettingsList/SnapsSettingsList.styles.ts 25.00% 3 Missing ⚠️
app/components/Nav/Main/MainNavigator.js 50.00% 1 Missing ⚠️
app/components/Views/Settings/index.tsx 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7955      +/-   ##
==========================================
- Coverage   36.64%   36.61%   -0.04%     
==========================================
  Files        1090     1092       +2     
  Lines       29113    29155      +42     
  Branches     2668     2670       +2     
==========================================
+ Hits        10668    10674       +6     
- Misses      17839    17875      +36     
  Partials      606      606              

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

@owencraston owencraston force-pushed the rntar-native-modules branch 2 times, most recently from 5d6d03e to fecc3f6 Compare November 30, 2023 06:48
@owencraston owencraston marked this pull request as ready for review November 30, 2023 07:47
@owencraston owencraston requested a review from a team as a code owner November 30, 2023 07:47
Copy link
Contributor

E2E test started on Bitrise: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/2237b682-8368-4082-8dde-6f8231efc422
You can also kick off another Bitrise E2E smoke test by removing and re-applying the (Run Smoke E2E) label

Copy link
Contributor

E2E test started on Bitrise: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/94969d5c-7eb1-4489-a642-7c7b43ed8ae9
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 Nov 30, 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

@owencraston owencraston force-pushed the rntar-native-modules branch 2 times, most recently from 7c01e0a to ac12833 Compare November 30, 2023 21:26
@owencraston
Copy link
Contributor Author

I created a QA build and confirmed that the new snaps settings page is in fact not in this build.

Screenshot_20231130-140858

Copy link
Contributor

github-actions bot commented Dec 1, 2023

E2E test started on Bitrise: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/0e392af7-948b-41b1-9a0a-66fde293cb66
You can also kick off another Bitrise E2E smoke test by removing and re-applying the (Run Smoke E2E) label

Copy link

sonarqubecloud bot commented Dec 1, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

13.0% 13.0% Coverage
0.0% 0.0% Duplication

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.

LGTM. Just left a small nit

@owencraston owencraston merged commit 770033f into main Dec 1, 2023
@owencraston owencraston deleted the rntar-native-modules branch December 1, 2023 05:15
@github-actions github-actions bot locked and limited conversation to collaborators Dec 1, 2023
@metamaskbot metamaskbot added the release-7.14.0 Issue or pull request that will be included in release 7.14.0 label Dec 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-7.14.0 Issue or pull request that will be included in release 7.14.0 team-accounts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants