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

Fix: Android Shasum Validation #7751

Merged
merged 4 commits into from
Nov 10, 2023
Merged

Fix: Android Shasum Validation #7751

merged 4 commits into from
Nov 10, 2023

Conversation

owencraston
Copy link
Contributor

@owencraston owencraston commented Nov 10, 2023

Description

Before :

this change we needed to disable shasum validation on snaps. The reason for this was due to the fact that my custom RnTar native module was reading the parsed snap code by line. This lead to small differences in the output file that differed from the npm source code. The result was that the computed shasum would be different that the one present in the manifest and woulld fail to install.

Now:

  • Now we read the source code file by file which maintains the integrity and results in a matching shasum.
  • I also converted this module to run on a separate thread so we don't block the main thread which can lead to perceived performance drop.

Related issues

Fixes: https://github.com/MetaMask/mobile-planning/issues/952

Manual testing steps

  1. You need android studio for this change
  2. pull branch
  3. run yarn setup
  4. run start:android
  5. open the app in android studio
  6. alternativily you can run in android studio
  7. create wallet
  8. navigate to https://metamask.github.io/snaps/test-snaps/latest
  9. install a snap
  10. the snap should prompt the user with a connection request, permission request and success screen

Screenshots/Recordings

Screenshot 2023-11-10 at 10 22 33 AM
Screen.Recording.2023-11-10.at.10.07.19.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.

@owencraston owencraston marked this pull request as ready for review November 10, 2023 05:21
@owencraston owencraston requested a review from a team as a code owner November 10, 2023 05:21
Copy link
Contributor

E2E test started on Bitrise: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/95f36b8a-e0f3-4d23-ad92-067714a01b3d
You can also kick off another Bitrise E2E smoke test by removing and re-applying the (Run Smoke E2E) label

@owencraston owencraston merged commit 2ee271b into mobile-snaps Nov 10, 2023
@owencraston owencraston deleted the fix/android-shasum branch November 10, 2023 20:26
@github-actions github-actions bot locked and limited conversation to collaborators Nov 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant