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

Published 20 hours ago • verified publisher icon MetaMask

feat(restore): Restore vault when state corruption occurs

Open brad-decker opened this issue 1 year ago • 9 comments

Description

This pull request adds an automatic saving of the encrypted keyring vault to localStorage, as well as the last good migration number seen. We persist the lastGoodMigration key after migrator runs so it should always be up to date. We compare the value to the persisted state before running the migrator as another way of detecting a corrupted state tree.

We can, in the case of corruption, load the vault from localStorage and update the initialStateTree with that value (as well as onboarding completed to avoid a routing loop) and the extension can recover.

CONS: any third party optin preferences, metametrics, etc will be wiped and the onboarding flow has been bypassed. there's probably a cleaner way to avoid the loop but without this I just keep getting routed back to a full screen onboarding-type flow with an unlock screen. Typing password and submitting just lands me back on that page.

edit At @danjm request/suggestion I added an error handler for this state that will show the MetaMask error screen when the state restore happens. Screeenshots included below.

Related issues

Fixes:

Manual testing steps

  1. Run yarn start
  2. Load the extension
  3. Unlock your wallet
  4. On the background process, inspect and go to application tab.
  5. Inspect local storage.
  6. See you have a 'lastGoodMigration' and 'metaMaskVault' key saved.
  7. In the console run chrome.storage.local.set({ data: null, meta: null });
  8. Restart the extension
  9. Load the extension.
  10. See the error message in the screenshots section.
  11. Click restart
  12. Load extension
  13. See your accounts are set, but you'll get all notifications and etc as a new user would.

Screenshots/Recordings

Screenshot 2024-02-26 at 3 44 33 PM

Before

After

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.

brad-decker avatar Feb 19 '24 21:02 brad-decker

Codecov Report

Attention: Patch coverage is 57.14286% with 24 lines in your changes are missing coverage. Please review.

Project coverage is 68.69%. Comparing base (c0dfd05) to head (8deb78a). Report is 64 commits behind head on develop.

Files Patch % Lines
app/scripts/lib/local-store.js 56.76% 16 Missing :warning:
shared/lib/error-utils.js 33.33% 8 Missing :warning:
Additional details and impacted files 
@@             Coverage Diff             @@
##           develop   #23056      +/-   ##
===========================================
- Coverage    68.73%   68.69%   -0.04%     
===========================================
  Files         1128     1128              
  Lines        43544    43605      +61     
  Branches     11643    11657      +14     
===========================================
+ Hits         29929    29953      +24     
- Misses       13615    13652      +37

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Feb 19 '24 22:02 codecov[bot]

Builds ready [234c4ab]
Page Load Metrics (1108 ± 45 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1143591986431
domContentLoaded13100382411
load891131811089445
domInteractive13100382411
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 533 Bytes (0.02%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

metamaskbot avatar Feb 19 '24 22:02 metamaskbot

@Gudahtt do you think the current method of persisting the vault is sufficient? It currently updates it everytime the entire state tree changes. I could either do a check to see if the value has changed and avoid setting the state again or attempt to identify only the areas in Metamask-controller where the vault would change (addAccount? etc?) and persist it there instead. however keying on the state-persisted event doesn't have any of the side effects I was describing about impacting UI speed.

brad-decker avatar Feb 20 '24 16:02 brad-decker

We could add a listener to the KeyringController directly, with a selector just for the vault. e.g. something like

messenger.on(
  'KeyringController:stateChange',
  (vault) => { ... },
  (state) => state.vault
)

Gudahtt avatar Feb 20 '24 16:02 Gudahtt

Alright i'll work on that today, plus removing the replacement of state when the lastGoodMigration doesn't match up.

brad-decker avatar Feb 20 '24 16:02 brad-decker

Just as an aside

messenger.on(
  'KeyringController:stateChange',
  (vault) => { ... },
  (state) => state.vault
)

This is dope.

brad-decker avatar Feb 21 '24 17:02 brad-decker

Builds ready [30a9172]
Page Load Metrics (1212 ± 461 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint751971303919
domContentLoaded106132157
load6125331212959461
domInteractive106132157
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 924 Bytes (0.02%)
  • ui: 215 Bytes (0.00%)
  • common: 536 Bytes (0.01%)

metamaskbot avatar Feb 27 '24 21:02 metamaskbot

Builds ready [08b8a8b]
Page Load Metrics (1192 ± 429 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint781901293215
domContentLoaded116831178
load6421291192893429
domInteractive116831178
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 924 Bytes (0.02%)
  • ui: 215 Bytes (0.00%)
  • common: 642 Bytes (0.01%)

metamaskbot avatar Feb 27 '24 22:02 metamaskbot

  • [ ] Needs copy updates (e.g. make sure they understand before they click restore that they are opting out of being able to get their data back)
  • [ ] Needs e2e tests (e.g. covering the error screen being shown, store a state tree in the background, restart, recognize state corruption, and hit the error screen)

danjm avatar May 16 '24 16:05 danjm

This PR has been automatically marked as stale because it has not had recent activity in the last 60 days. It will be closed in 14 days. Thank you for your contributions.

github-actions[bot] avatar Sep 17 '24 00:09 github-actions[bot]

This PR was closed because there has been no follow up activity in the last 14 days. Thank you for your contributions.

github-actions[bot] avatar Oct 01 '24 00:10 github-actions[bot]