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

chore: Chore/1742 remove vault recreation log in #9577

Merged
merged 7 commits into from
May 9, 2024

Conversation

Cal-L
Copy link
Contributor

@Cal-L Cal-L commented May 9, 2024

Description

This PR removes a condition in the Authentication service where the vault would be recreated upon first time log in (excluding password creation during wallet creation). This logic existed for backwards compatibility but was never needed since the app was always using the "original" encryption lib. Since vault recreation isn't needed, this omits the need to pass selectedAddress, which appears in most of the changes in this PR. The change in https://github.com/MetaMask/metamask-mobile/pull/9508/files#diff-75ad251e628bf4ac096dc3ad4e46d4942f61d1576784c71c6150920588b89e1eR45 already prevents this code from running. This PR further removes remaining traces.

Related issues

Fixes: #1742

Manual testing steps

App behavior remains the same as before throughout the listed scenarios:

  • Create a wallet from scratch
  • Immediate lock option should be respected and user is still able to log in after backgrounding/foregrounding app
  • Remember me option should be respected + continues to override immediate lock timer
  • Log in after killing app + reopening should work without issues
  • Upgrading from before (main) -> (changes in this PR) should still allow users to log in successfully
  • Vault recovery flow recovers wallet successfully

Screenshots/Recordings

  • Create a wallet from scratch
wallet-creation.mov
  • Immediate lock option should be respected and user is still able to log in after backgrounding/foregrounding app
immediate-lock.mov
  • Remember me option should be respected + continues to override immediate lock timer
remember-me.mov
  • Log in after killing app + reopening should work without issues
kill-reopen.mov
  • Upgrading from before (main) -> (changes in this PR) should still allow users to log in successfully
upgrade.mov
  • Vault recovery flow recovers wallet successfully
recovery.mov

Before

After

Pre-merge author checklist

  • I’ve followed MetaMask Coding Standards.
  • I've completed the PR template to the best of my ability
  • 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.

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.

@Cal-L Cal-L requested a review from a team as a code owner May 9, 2024 00:03
Copy link
Contributor

github-actions bot commented May 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.

@Cal-L Cal-L added needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) needs-qa Any New Features that needs a full manual QA prior to being added to a release. Run Smoke E2E Triggers smoke e2e on Bitrise labels May 9, 2024
@Cal-L Cal-L added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels May 9, 2024
Copy link
Contributor

github-actions bot commented May 9, 2024

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: da68a8f
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/cb2fe358-0a8d-4ae2-950c-21dcf7d2ae73

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

@Cal-L
Copy link
Contributor Author

Cal-L commented May 9, 2024

@Cal-L Cal-L requested a review from owencraston May 9, 2024 00:08
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.

Awesome cleaning! It seems that can have a few more cleaning lint is failing

Copy link

sonarqubecloud bot commented May 9, 2024

@Cal-L
Copy link
Contributor Author

Cal-L commented May 9, 2024

Latest commit just removes unused imports. Commit before that passes both smoke and regression PR ✌️

@Cal-L Cal-L added No QA Needed Apply this label when your PR does not need any QA effort. and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels May 9, 2024
Copy link
Contributor

@MarioAslau MarioAslau left a comment

Choose a reason for hiding this comment

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

Looks great! Great catch Cal!

@Cal-L Cal-L merged commit a337fcc into main May 9, 2024
33 of 35 checks passed
@Cal-L Cal-L deleted the chore/1742-remove-vault-recreation-log-in branch May 9, 2024 17:00
@github-actions github-actions bot locked and limited conversation to collaborators May 9, 2024
@github-actions github-actions bot removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label May 9, 2024
@metamaskbot metamaskbot added the release-7.23.0 Issue or pull request that will be included in release 7.23.0 label May 9, 2024
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.23.0 Issue or pull request that will be included in release 7.23.0 Run Smoke E2E Triggers smoke e2e on Bitrise team-mobile-platform
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants