fix: Improve migration 121.1 state validation #26773
Conversation
Migration 121.1 has been updated to include more state validation, so that it does not throw an error in the event that state is corrupted in some unexpected way. Unexpected corrupted state is now reported to Sentry as well.
| @@ -28,22 +29,114 @@ export async function migrate( | |||
| return versionedData; | |||
| } | |||
|
|
|||
| function transformState(state: Record<string, unknown>) { | |||
| const accountsControllerState = state?.AccountsController as | |||
There was a problem hiding this comment.
The intent of using unknown was to force runtime validation of types, rather than assuming state is in the correct shape. We've seen unexpected state corruption in the past, so we try to be more cautious about this. Using a type cast here is defeating the purpose of using unknown, as it lets you skip past this validation.
This also goes against our general TypeScript guidelines, see here for more information: https://github.com/MetaMask/contributor-docs/blob/main/docs/typescript.md#avoid-type-assertions-by-using-type-guards-to-improve-type-inference
| } | ||
|
|
||
| if ( | ||
| Object.keys(accountsControllerState.internalAccounts.accounts).length === 0 |
There was a problem hiding this comment.
These two cases might be encountered for users who have not completed onboarding. It seemed fine to skip the migration in this case, and the types of AccountsController seem to allow it as well. A console log was added just in case this happens unexpectedly.
| } else if (typeof firstAccount.id !== 'string') { | ||
| global.sentry?.captureException( | ||
| new Error( | ||
| `Migration ${version}: Invalid AccountsController internalAccounts.accounts state, entry found with an id of type '${typeof firstAccount.id}'`, |
There was a problem hiding this comment.
I didn't strictly need to validate the presence and type of the id property, TypeScript didn't demand it of me. But it seemed like a good idea. If the id was corrupted somehow, we wouldn't want to propagate that.
| @@ -25,7 +32,8 @@ describe('migration #121.1', () => { | |||
| }, | |||
| }; | |||
|
|
|||
| const newStorage = await migrate(oldStorage); | |||
| const newStorage = await migrate(cloneDeep(oldStorage)); | |||
There was a problem hiding this comment.
I updated the existing tests to use cloneDeep to ensure we don't accidentally forget to clone state in the migration itself. The clone happens in the migration in the copy+pasted portion, so it's not likely to be missed, but it's easy to cover in tests so why not.
| expect(newStorage.data).toStrictEqual(oldStorage.data); | ||
| }); | ||
|
|
||
| it('does nothing if AccountsController state is missing', async () => { |
There was a problem hiding this comment.
I added a few tests for these "expected missing state" cases, which were already handled correctly but were not tested
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #26773 +/- ##
===========================================
+ Coverage 70.08% 70.10% +0.02%
===========================================
Files 1414 1414
Lines 49330 49365 +35
Branches 13782 13792 +10
===========================================
+ Hits 34570 34605 +35
Misses 14760 14760 ☔ View full report in Codecov by Sentry. |
|
metamaskbot
added
the
release-12.5.0
Builds ready [afa7976]
Page Load Metrics (1781 ± 92 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
gauthierpetetin
added
release-12.4.0
Description
Migration 121.1 has been updated to include more state validation, so that it does not throw an error in the event that state is corrupted in some unexpected way. Unexpected corrupted state is now reported to Sentry as well.
Related issues
Related: #26377
Manual testing steps
The unit tests outline the scenarios that the added validation are meant to cover. Probably not worth anyone's time to manually test those.
To test the migration in general though, here are the steps:
dist/chromedirectory and proceedthrough onboarding
chrome.storage.local.get( null, (state) => { state.data.AccountsController.internalAccounts.selectedAccount = 'unknown id'; chrome.storage.local.set(state, () => chrome.runtime.reload()); } );error
chrome.storage.local.get(console.log)to check that theAccountsController state is now valid
Screenshots/Recordings
N/A
Pre-merge author checklist
Pre-merge reviewer checklist