feat: Properly show missing vault error #30684

danjm · 2025-03-03T16:03:21Z

Description

This PR updates background.js, and some related files, to ensure that an error is properly shown to the user in two cases:

If the vault is missing from state when it should be present
For the case of a known corrupted state error with the message "block checksum mismatch"

To achieve this, this PR adds logic to detect if the fault is missing and then throws an error, which in turn is sent via postmessage to the UI, where it is handled by a custom error screen.

Related issues

Fixes:

Manual testing steps

Install and onboard
Open up the service worker console and enter this code:

chrome.storage.local.get(({ data, meta }) => {
	chrome.storage.local.set(
		{ data: {} , meta },
		() => { chrome.runtime.reload() }
        );

});

After opening the UI again, you should see an error message indicating the state is corrupt

Alternatively

Install and onboard
Edit one of the level db files in which the extensions data is saved by deleting a single character in the file
Reload the extension
After opening the UI again, you should see an error message indicating the state is corrupt

Screenshots/Recordings

Pre-merge author checklist

I've followed MetaMask Contributor Docs and MetaMask Extension 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.

github-actions · 2025-03-03T16:03:38Z

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.

…vault data when we should be able to

davidmurdoch

I haven't fully reviewed just, just trying to get some requests for changes out to you sooner rather than later.

davidmurdoch · 2025-03-03T20:33:21Z

app/_locales/en/messages.json

+    "message": "MetaMask's state file is damaged. This can happen when the browser database directs us to a state file that doesn't exist. The location of the file depends on your operating system and browser. Visit our support page for more information.\n\nYou can restore your MetaMask wallet, but doing so will erase your settings and preferences. This will also delete your state file if it still exists. You can try to back up or restore your state file manually before you decide to restore your wallet. Learn more."
+  },
+  "stateCorruptionDetectedWithBackup": {
+    "message": "MetaMask's state file is damaged. This usually happens when the browser database directs us to a state file that doesn't exist. The location of the file depends on your operating system and browser.\n\n A copy of your vault can be found in a secondary location. You can use it to restore your accounts, but you'll need to set up your preferences and settings again.\n\nWhen you restore your accounts, the old MetaMask files in your browser will be deleted. If you want to keep those files, back them up before you restore your accounts."


The new lines here don't do anything in the rendered UI :-(

Also, the

Learn more.

Text is a bit strange, since you'd think it'd be a link but it isn't.

davidmurdoch · 2025-03-03T20:39:38Z

shared/lib/error-utils.js

+  const textDirection = ['ar', 'dv', 'fa', 'he', 'ku'].includes(preferredLocale)
+    ? 'rtl'
+    : 'auto';


since this is reused twice in this file can you abstract it out to a helper funciton?

davidmurdoch · 2025-03-03T20:45:08Z

shared/lib/error-utils.js

@@ -29,7 +29,7 @@ const _setupLocale = async (currentLocale) => {
  return { currentLocaleMessages, enLocaleMessages };
 };

-export const setupLocale = memoize(_setupLocale);
+export const setupErrorLocale = memoize(_setupLocale);


Why the rename? I think this function's return values are used for a translations all throughout the app?

davidmurdoch · 2025-03-03T20:45:44Z

shared/lib/error-utils.js

+    <div class="critical-error__container">
+      <div class="critical-error">
+        <div class="critical-error__icon">
+          <svg width="24" height="24" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 512 512">
+            <path d="m443 342l-126-241c-16-32-40-50-65-50-26 0-50 18-66 50l-126 241c-16 30-18 60-5 83 13 23 38 36 71 36l251 0c33 0 58-13 71-36 13-23 11-53-5-83z m-206-145c0-8 6-15 15-15 8 0 14 7 14 15l0 105c0 8-6 15-14 15-9 0-15-7-15-15z m28 182c-1 1-2 2-3 3-1 0-2 1-3 1-1 1-2 1-4 2-1 0-2 0-3 0-2 0-3 0-4 0-2-1-3-1-4-2-1 0-2-1-3-1-1-1-2-2-3-3-4-4-6-9-6-15 0-5 2-11 6-15 1 0 2-1 3-2 1-1 2-2 3-2 1-1 2-1 4-1 2-1 5-1 7 0 2 0 3 0 4 1 1 0 2 1 3 2 1 1 2 2 3 2 4 4 6 10 6 15 0 6-2 11-6 15z"/>
+          </svg>
+        </div>
+        <div>
+          <p>
+            ${
+              hasBackup === true
+                ? t('stateCorruptionDetectedWithBackup')
+                : t('stateCorruptionDetectedNoBackup')
+            }
+          </p>
+          <p class="critical-error__footer">
+            <span>${t('unexpectedBehavior')}</span>
+            <a
+              href=${supportLink}
+              class="critical-error__link"
+              target="_blank"
+              rel="noopener noreferrer">
+                ${t('sendBugReport')}
+              </a>
+          </p>
+        </div>
+      </div>
+    </div>


since this is almost this same between the two uses in this file, can you abstract out part of it (even just the svg code would be fine)

davidmurdoch · 2025-03-03T20:48:41Z

shared/lib/error-utils.js

+    metamaskState?.data?.PreferencesController?.initializationFlags
+      ?.vaultBackedUp;
+  /**
+   * The pattern ${errorKey === 'troubleStarting' ? t('troubleStarting') : ''}


this comment is for the other function

davidmurdoch · 2025-03-03T20:51:17Z

app/scripts/background.js

+  );
+
+  if (vaultHasNotYetBeenCreated === undefined && !vaultDataPresent) {
+    throw new Error('Data error: storage.local does not contain vault data');


Can this string and the use of it in STATE_CORRUPTION_ERRORS be shared? I could see someone updating the string here but not in the STATE_CORRUPTION_ERRORS array.

davidmurdoch · 2025-03-03T20:54:47Z

app/scripts/background.js

+  const { vaultHasNotYetBeenCreated } = await browser.storage.local.get(
+    'vaultHasNotYetBeenCreated',
+  );
+
  // read from disk
  // first from preferred, async API:
-  const preMigrationVersionedData =
-    (await persistenceManager.get()) ||
-    migrator.generateInitialState(firstTimeState);
+  let preMigrationVersionedData = await persistenceManager.get();
+
+  const vaultDataPresent = Boolean(
+    preMigrationVersionedData?.data?.KeyringController?.vault,
+  );
+
+  if (vaultHasNotYetBeenCreated === undefined && !vaultDataPresent) {


Can you refactor this to avoid the extra work in the usual case where: vault data is present. If we have vault data, we don't need to fetch vaultHasNotYetBeenCreated at all.

Ignore me if this isn't actually true, but I think it is?

davidmurdoch · 2025-03-03T20:56:38Z

app/scripts/background.js

+  if (!preMigrationVersionedData.data && !preMigrationVersionedData.meta) {
+    const initialState = migrator.generateInitialState(firstTimeState);
+    preMigrationVersionedData = {
+      ...preMigrationVersionedData,


Why the need for the preMigrationVersionedData spread here? If it doesn't have a data or meta property, are there other properties we might need from it?

davidmurdoch · 2025-03-03T21:08:05Z

app/scripts/ui.js

+  const html = await getStateCorruptionErrorHtml(SUPPORT_LINK, metamaskState);
+  container.innerHTML = html;
+
+  const button = document.getElementById('critical-error-button');


Since getStateCorruptionErrorHtml function never returns an element I think this is always going to be undefined.

DDDDDanica · 2025-03-03T21:57:59Z

app/_locales/en/messages.json

@@ -5210,6 +5210,12 @@
  "stake": {
    "message": "Stake"
  },
+  "stateCorruptionDetectedNoBackup": {


we should also copy them in en_GB, right?

DDDDDanica · 2025-03-03T22:06:18Z

shared/lib/error-utils.js

+          </p>
+          <p class="critical-error__footer">
+            <span>${t('unexpectedBehavior')}</span>
+            <a


There is an update for support link, we can have a separate ticket to integrate with the support modal
#30415

wdyt？

metamaskbot added the team-extension-platform Extension Platform team label Mar 3, 2025

Detect and throw error, and display it to the user, if we cannot get …

132a5b3

…vault data when we should be able to

danjm marked this pull request as ready for review March 3, 2025 16:42

danjm force-pushed the display-vault-missing-error branch from 3fa2d2c to 132a5b3 Compare March 3, 2025 16:42

danjm changed the title ~~Properly show missing vault error~~ feat: Properly show missing vault error Mar 3, 2025

gauthierpetetin assigned danjm Mar 3, 2025

danjm mentioned this pull request Mar 3, 2025

Display missing vault error #30246

Closed

7 tasks

danjm added 2 commits March 3, 2025 13:24

Lint message.json

63a4620

Lint fix

98bf55b

davidmurdoch requested changes Mar 3, 2025

View reviewed changes

DDDDDanica reviewed Mar 3, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Properly show missing vault error #30684

feat: Properly show missing vault error #30684

danjm commented Mar 3, 2025 •

edited

Loading

github-actions bot commented Mar 3, 2025

davidmurdoch left a comment

davidmurdoch Mar 3, 2025

davidmurdoch Mar 3, 2025

davidmurdoch Mar 3, 2025

davidmurdoch Mar 3, 2025

davidmurdoch Mar 3, 2025

davidmurdoch Mar 3, 2025

davidmurdoch Mar 3, 2025

davidmurdoch Mar 3, 2025

davidmurdoch Mar 3, 2025

DDDDDanica Mar 3, 2025

DDDDDanica Mar 3, 2025

feat: Properly show missing vault error #30684

Are you sure you want to change the base?

feat: Properly show missing vault error #30684

Conversation

danjm commented Mar 3, 2025 • edited Loading

Description

Related issues

Manual testing steps

Screenshots/Recordings

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented Mar 3, 2025

davidmurdoch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danjm commented Mar 3, 2025 •

edited

Loading