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

Vault corruption recovery flow #4421

Merged
merged 23 commits into from
Mar 13, 2023
Merged

Vault corruption recovery flow #4421

merged 23 commits into from
Mar 13, 2023

Conversation

owencraston
Copy link
Contributor

@owencraston owencraston commented May 30, 2022

Description

  • We are building this feature off the work done in the auth refactor

Why?

  • Every so often the vault gets corrupted due to storage issues with the app. When this happens the users get locked out of MetaMask and in some cases can lose their wallet completely if they have no backed up their SRP.
  • We have tracked this error for a few months and it has occurred 67 000 times and has effected over 2000 users Screenshot 2022-12-20 at 4 31 30 PM
  • This NEEDS to be addressed. Apart from removing this issue altogether (not something we know how to do yet and will involve a massive refactor) this recovery flow is the best we can do now.

What?

  • listens for changes in the keyring controller and makes a backup stores in the SecureKeychain
  • Adds the ability to reinitialize the engine with an existing keyring. Then engine will initialize the keyring controller with the backup if it is passed in, else it will create a new one.
  • When vault corruption error surfaces in Login, we kick them into a vault recovery flow that recreates the engine and and authenticates them back into the app (or send them to login if they used password as a login method).

How?

This is a fairly complex fix so I will try to highlight how this is working....

  • create a method to backup the vault. see app/core/backupVault.ts method backupVault
  • listen for changes on the KeyringController in the Engine and backup the vault when there is a change. See app/core/Engine.js handleVaultBackup
  • modify the Engine.init to accept an optional param of the keyring state.
  • when we pass in a keyringState to the init call of the engine, recreate the keyring controller with the backup.
  • When the vault corruption error occurs in Login/index.js, start the recovery flow by calling handleVaultCorruption which ...
    • gets the vault from backaup via getVaultFromBackup()
    • verifies that the password the user typed into the password field can unlock the vault with parseVaultValue
    • if they can, it is the correct password then we can get the auth type and store the credentials
      • this is needed in case they change the auth type from the previously used type (biometrics, remember me, password etc)
      • if the password storage is successful we will launch the recovery flow since we know we have valid credentials that can unlock the backup of the vault and we have the latest auth type.
    • if the password is wrong we indicate this to the user
  • The first screen in the recovery flow is RestoreWallet. This screen calls EngineService.initializeVaultFromBackup().
    • initializeVaultFromBackup gets the backup, destroys the old engine (not just resets it, removes the old instance), and then calls Engine.init with the backed up keyring state as the keyringState param in Engine.init(state, keyringState)
    • then we setup the controllers listeners on the new engine instance with this.updateControllers
  • On success we will navigate to the WalletRestored screen. Here we will attempt a login (using the previously stored credentials) by calling await Authentication.appTriggeredAuth(selectedAddress); navigation.navigate(Routes.ONBOARDING.HOME_NAV);
    - if we don't have the credentials stored (the case for password login) we will simply navigate the user to Login where they can login normally. the vault/engine have been reinitialized at this point.

Acceptance criteria

  • strings are translated
  • whenever a user experiences the Vault corruption error on Login and they have a valid password, they are successfully launched into the vault recovery flow
    • If the user has biometrics enabled, it will prompt them for biometrics before going into the recovery flow
  • if the user has an incorrect password but tried to recover the vault they are informed that their password is incorrect
  • When the user clicks Restore wallet the old engine is destroyed, a new engine is reinitialized with the vault backup and they are navigated to the WalletRestored screen.
  • On WalletRestored there are three potential outcomes to pressing the Continue to wallet button...
    1. the user is using password and goes back to login
    2. the user is using biometrics/passcode and we get the biometrics on the wallet restored success screen (faceID or passcode or fingerprint) and on success we log you in and you are navigated to the wallet view
    3. the user is using remember me and the user gets logged in automatically and is navigated to the wallet view
  • // TODO figure out the acceptance criteria for WalletResetNeeded screen. This screen only appears if the engine does not successfully get reinitialized in RestoreWallet

TODO

  • Metrics
  • Finalize strings and get them merged into main for translations so we can launch with translations
  • Finalize intended outcomes on WalletResetNeeded screen.
  • Investigate waiting until the Engine is initialized and controller subscription are completely done before navigating to success screen.
  • Write unit tests
    • verify that the old engine instance does not match the new engine instance when calling initializeVaultFromBackup

Checklist

  • There is a related GitHub issue
  • Tests are included if applicable
  • Any added code is fully documented

Designs

Screenshots/Recordings
the version here has some minor UX issues around the loading state of the WalletRestored screen

RPReplay_Final1671576645.mov

Issue

Progresses https://github.com/MetaMask/mobile-planning/issues/139
progresses #5420
Resolves #4645

@github-actions
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.

app/store/index.js Outdated Show resolved Hide resolved
@owencraston owencraston force-pushed the improvement/backup-vault branch 2 times, most recently from 410ffa0 to 31daa76 Compare July 4, 2022 21:38
@owencraston owencraston force-pushed the improvement/backup-vault branch 3 times, most recently from cf7fbc6 to b50ec60 Compare July 12, 2022 20:00
@owencraston owencraston force-pushed the improvement/backup-vault branch 3 times, most recently from f9b5a8e to b5b8107 Compare July 20, 2022 20:02
@owencraston owencraston force-pushed the improvement/backup-vault branch 4 times, most recently from 8e3a949 to 6e1fdfe Compare December 13, 2022 00:11
@owencraston owencraston changed the base branch from main to auth-refactor December 13, 2022 00:12
@owencraston owencraston force-pushed the improvement/backup-vault branch 4 times, most recently from 6637a5d to aaca80d Compare December 16, 2022 02:14
@owencraston owencraston changed the title backup vault Vault corruption recovery flow Dec 20, 2022
@owencraston owencraston added Code Impact - High Major code change that can have an high impact on the codebase functionality team-accounts labels Dec 20, 2022
Copy link

@olenapankina olenapankina left a comment

Choose a reason for hiding this comment

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

Tested on
iPhone Xr iOS 16.1.1
Samsung Galaxy M21 Android 12, One UI Core version 4.1
commit e271313

Issue 01-05 confirmed fixed.

4421_Android_successful backup.mov
4421_iOS_successful backup.mp4
4421_Android_failed backup.mov
4421_iOS_failed backup.mp4

QA Passed ✅

@olenapankina olenapankina added QA Passed A successful QA run through has been done and removed QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed labels Mar 13, 2023
@owencraston owencraston merged commit 9addf9a into main Mar 13, 2023
@owencraston owencraston deleted the improvement/backup-vault branch March 13, 2023 19:33
@github-actions github-actions bot locked and limited conversation to collaborators Mar 13, 2023
@owencraston owencraston added the release-6.3.0 PR for release 6.3.0 label Mar 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Code Impact - High Major code change that can have an high impact on the codebase functionality QA Passed A successful QA run through has been done release-6.3.0 PR for release 6.3.0 team-accounts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Storage limit/corrupt data - reinstall mm
4 participants