metamask-mobile icon indicating copy to clipboard operation
metamask-mobile copied to clipboard

chore: Chore/1742 remove vault recreation log in

Open Cal-L opened this issue 1 year ago • 4 comments

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

https://github.com/MetaMask/metamask-mobile/assets/10508597/12a881d2-8016-4180-a4d1-b87362cda9d0

  • Immediate lock option should be respected and user is still able to log in after backgrounding/foregrounding app

https://github.com/MetaMask/metamask-mobile/assets/10508597/1c68d13a-89b2-491b-8d4f-ac77b5ceff8b

  • Remember me option should be respected + continues to override immediate lock timer

https://github.com/MetaMask/metamask-mobile/assets/10508597/08c64684-b152-42df-a46c-4b187827193e

  • Log in after killing app + reopening should work without issues

https://github.com/MetaMask/metamask-mobile/assets/10508597/aee455a5-34ac-4fda-85a8-6f43cf1afd67

  • Upgrading from before (main) -> (changes in this PR) should still allow users to log in successfully

https://github.com/MetaMask/metamask-mobile/assets/10508597/72129fa6-8c19-4f7e-a4f3-6ca781776846

  • Vault recovery flow recovers wallet successfully

https://github.com/MetaMask/metamask-mobile/assets/10508597/72f9925b-f442-4aa7-9d45-c681d46bbeaa

Before

After

Pre-merge author checklist

  • [x] I’ve followed MetaMask Coding Standards.
  • [x] I've completed the PR template to the best of my ability
  • [x] I’ve included tests if applicable
  • [x] I’ve documented my code using JSDoc format if applicable
  • [x] I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.

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.

Cal-L avatar May 09 '24 00:05 Cal-L

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.

github-actions[bot] avatar May 09 '24 00:05 github-actions[bot]

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: da68a8fc76aa6d17b6ee75ad40c93e3902715bd1 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

github-actions[bot] avatar May 09 '24 00:05 github-actions[bot]

E2E regression pipeline run here - https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/f910275e-247c-4c42-b6a6-fa975d4f74f7

Cal-L avatar May 09 '24 00:05 Cal-L

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

Cal-L avatar May 09 '24 16:05 Cal-L