-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
fix(accounts): add migration to repair missing selectedAccount #27717
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
Closed
+385
−0
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,286 @@ | ||
| import migrate from './126'; | ||
| import { captureException } from '@sentry/react-native'; | ||
|
|
||
| jest.mock('@sentry/react-native', () => ({ | ||
| captureException: jest.fn(), | ||
| })); | ||
| const mockedCaptureException = jest.mocked(captureException); | ||
|
|
||
| const migrationVersion = 126; | ||
|
|
||
| interface InternalAccounts { | ||
| accounts: Record<string, { id: string; address: string }>; | ||
| selectedAccount?: string; | ||
| } | ||
|
|
||
| const createValidState = ( | ||
| internalAccounts?: InternalAccounts | unknown, | ||
| includeController = true, | ||
| ) => ({ | ||
| engine: { | ||
| backgroundState: { | ||
| ...(includeController && { | ||
| AccountsController: { | ||
| ...(internalAccounts !== undefined && { | ||
| internalAccounts, | ||
| }), | ||
| }, | ||
| }), | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| const ACCOUNT_1_ID = 'cf8dace4-9439-4bd4-b3a8-88c821c8fcb3'; | ||
| const ACCOUNT_2_ID = 'e9b38e2b-7d1e-4e5f-8c2a-1a5b3d7e9f01'; | ||
|
|
||
| const validAccounts = { | ||
| [ACCOUNT_1_ID]: { id: ACCOUNT_1_ID, address: '0xabc' }, | ||
| [ACCOUNT_2_ID]: { id: ACCOUNT_2_ID, address: '0xdef' }, | ||
| }; | ||
|
|
||
| describe(`Migration ${migrationVersion}: Fix selectedAccount missing or undefined`, () => { | ||
| beforeEach(() => { | ||
| jest.clearAllMocks(); | ||
| }); | ||
|
|
||
| describe('invalid state structure', () => { | ||
| it('returns state if top-level state is invalid', () => { | ||
| const result = migrate('invalid'); | ||
| expect(result).toBe('invalid'); | ||
| }); | ||
|
|
||
| it('captures exception and returns state if AccountsController is missing', () => { | ||
| const state = createValidState(undefined, false); | ||
| const result = migrate(state); | ||
|
|
||
| expect(result).toStrictEqual(state); | ||
| expect(mockedCaptureException).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| message: expect.stringContaining( | ||
| 'AccountsController state is missing or invalid', | ||
| ), | ||
| }), | ||
| ); | ||
| }); | ||
|
|
||
| it('captures exception and returns state if AccountsController is not an object', () => { | ||
| const state = { | ||
| engine: { | ||
| backgroundState: { | ||
| AccountsController: 'invalid', | ||
| }, | ||
| }, | ||
| }; | ||
| const result = migrate(state); | ||
|
|
||
| expect(result).toStrictEqual(state); | ||
| expect(mockedCaptureException).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| message: expect.stringContaining( | ||
| 'AccountsController state is missing or invalid', | ||
| ), | ||
| }), | ||
| ); | ||
| }); | ||
|
|
||
| it('captures exception and returns state if internalAccounts is missing', () => { | ||
| const state = createValidState(undefined, true); | ||
| const result = migrate(state); | ||
|
|
||
| expect(result).toStrictEqual(state); | ||
| expect(mockedCaptureException).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| message: expect.stringContaining( | ||
| 'internalAccounts is missing or invalid', | ||
| ), | ||
| }), | ||
| ); | ||
| }); | ||
| }); | ||
|
|
||
| describe('valid selectedAccount (no-op)', () => { | ||
| it('does not modify state when selectedAccount is a valid string', () => { | ||
| const state = createValidState({ | ||
| accounts: validAccounts, | ||
| selectedAccount: ACCOUNT_1_ID, | ||
| }); | ||
| const result = migrate(state); | ||
|
|
||
| expect(result).toStrictEqual(state); | ||
| expect(mockedCaptureException).not.toHaveBeenCalled(); | ||
| }); | ||
| }); | ||
|
|
||
| describe('selectedAccount key is missing (JSON.stringify stripped it)', () => { | ||
| it('sets selectedAccount to first account ID when key is absent', () => { | ||
| const internalAccounts = { | ||
| accounts: validAccounts, | ||
| }; | ||
| // Simulate JSON roundtrip: key was stripped | ||
| delete (internalAccounts as Record<string, unknown>).selectedAccount; | ||
|
|
||
| const state = createValidState(internalAccounts); | ||
| const result = migrate(state) as ReturnType<typeof createValidState>; | ||
| const resultAccounts = result.engine.backgroundState | ||
| .AccountsController as Record<string, unknown>; | ||
| const resultInternal = resultAccounts.internalAccounts as Record< | ||
| string, | ||
| unknown | ||
| >; | ||
|
|
||
| expect(resultInternal.selectedAccount).toBe(ACCOUNT_1_ID); | ||
| expect(mockedCaptureException).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| message: expect.stringContaining('key_missing'), | ||
| }), | ||
| ); | ||
| }); | ||
|
|
||
| it('sets selectedAccount to empty string when key is absent and no accounts exist', () => { | ||
| const internalAccounts = { | ||
| accounts: {}, | ||
| }; | ||
| delete (internalAccounts as Record<string, unknown>).selectedAccount; | ||
|
|
||
| const state = createValidState(internalAccounts); | ||
| const result = migrate(state) as ReturnType<typeof createValidState>; | ||
| const resultAccounts = result.engine.backgroundState | ||
| .AccountsController as Record<string, unknown>; | ||
| const resultInternal = resultAccounts.internalAccounts as Record< | ||
| string, | ||
| unknown | ||
| >; | ||
|
|
||
| expect(resultInternal.selectedAccount).toBe(''); | ||
| expect(mockedCaptureException).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| message: expect.stringContaining('key_missing'), | ||
| }), | ||
| ); | ||
| }); | ||
| }); | ||
|
|
||
| describe('selectedAccount is explicitly undefined', () => { | ||
| it('sets selectedAccount to first account ID when value is undefined', () => { | ||
| const state = createValidState({ | ||
| accounts: validAccounts, | ||
| selectedAccount: undefined, | ||
| }); | ||
| const result = migrate(state) as ReturnType<typeof createValidState>; | ||
| const resultAccounts = result.engine.backgroundState | ||
| .AccountsController as Record<string, unknown>; | ||
| const resultInternal = resultAccounts.internalAccounts as Record< | ||
| string, | ||
| unknown | ||
| >; | ||
|
|
||
| expect(resultInternal.selectedAccount).toBe(ACCOUNT_1_ID); | ||
| expect(mockedCaptureException).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| message: expect.stringContaining('selectedAccount is corrupt'), | ||
| }), | ||
| ); | ||
| }); | ||
| }); | ||
|
|
||
| describe('selectedAccount is wrong type', () => { | ||
| it('sets selectedAccount to first account ID when value is a number', () => { | ||
| const state = createValidState({ | ||
| accounts: validAccounts, | ||
| selectedAccount: 42, | ||
| }); | ||
| const result = migrate(state) as ReturnType<typeof createValidState>; | ||
| const resultAccounts = result.engine.backgroundState | ||
| .AccountsController as Record<string, unknown>; | ||
| const resultInternal = resultAccounts.internalAccounts as Record< | ||
| string, | ||
| unknown | ||
| >; | ||
|
|
||
| expect(resultInternal.selectedAccount).toBe(ACCOUNT_1_ID); | ||
| expect(mockedCaptureException).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| message: expect.stringContaining('wrong_type_number'), | ||
| }), | ||
| ); | ||
| }); | ||
|
|
||
| it('sets selectedAccount to first account ID when value is null', () => { | ||
| const state = createValidState({ | ||
| accounts: validAccounts, | ||
| selectedAccount: null, | ||
| }); | ||
| const result = migrate(state) as ReturnType<typeof createValidState>; | ||
| const resultAccounts = result.engine.backgroundState | ||
| .AccountsController as Record<string, unknown>; | ||
| const resultInternal = resultAccounts.internalAccounts as Record< | ||
| string, | ||
| unknown | ||
| >; | ||
|
|
||
| expect(resultInternal.selectedAccount).toBe(ACCOUNT_1_ID); | ||
| expect(mockedCaptureException).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| message: expect.stringContaining('wrong_type_object'), | ||
| }), | ||
| ); | ||
| }); | ||
| }); | ||
|
|
||
| describe('selectedAccount is empty string', () => { | ||
| it('does not modify state when selectedAccount is empty string (AccountsController handles this)', () => { | ||
| const state = createValidState({ | ||
| accounts: validAccounts, | ||
| selectedAccount: '', | ||
| }); | ||
| const result = migrate(state) as ReturnType<typeof createValidState>; | ||
| const resultAccounts = result.engine.backgroundState | ||
| .AccountsController as Record<string, unknown>; | ||
| const resultInternal = resultAccounts.internalAccounts as Record< | ||
| string, | ||
| unknown | ||
| >; | ||
|
|
||
| expect(resultInternal.selectedAccount).toBe(ACCOUNT_1_ID); | ||
| expect(mockedCaptureException).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| message: expect.stringContaining('empty_string'), | ||
| }), | ||
| ); | ||
| }); | ||
| }); | ||
|
|
||
| describe('fallback to empty string', () => { | ||
| it('sets selectedAccount to empty string when accounts have invalid structure', () => { | ||
| const state = createValidState({ | ||
| accounts: { 'bad-id': { notAnId: true } }, | ||
| selectedAccount: undefined, | ||
| }); | ||
| const result = migrate(state) as ReturnType<typeof createValidState>; | ||
| const resultAccounts = result.engine.backgroundState | ||
| .AccountsController as Record<string, unknown>; | ||
| const resultInternal = resultAccounts.internalAccounts as Record< | ||
| string, | ||
| unknown | ||
| >; | ||
|
|
||
| expect(resultInternal.selectedAccount).toBe(''); | ||
| }); | ||
|
|
||
| it('sets selectedAccount to empty string when first account id is empty', () => { | ||
| const state = createValidState({ | ||
| accounts: { 'bad-id': { id: '' } }, | ||
| selectedAccount: undefined, | ||
| }); | ||
| const result = migrate(state) as ReturnType<typeof createValidState>; | ||
| const resultAccounts = result.engine.backgroundState | ||
| .AccountsController as Record<string, unknown>; | ||
| const resultInternal = resultAccounts.internalAccounts as Record< | ||
| string, | ||
| unknown | ||
| >; | ||
|
|
||
| expect(resultInternal.selectedAccount).toBe(''); | ||
| }); | ||
| }); | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,97 @@ | ||
| import { captureException } from '@sentry/react-native'; | ||
| import { hasProperty, isObject } from '@metamask/utils'; | ||
| import { ensureValidState } from './util'; | ||
|
|
||
| const migrationVersion = 126; | ||
|
|
||
| /** | ||
| * Migration 126: Fix AccountsController selectedAccount that is missing or undefined. | ||
| * | ||
| * Migration 059 attempted to fix this but used `hasProperty` which returns false | ||
| * when the key is entirely absent (the common case after JSON.stringify/JSON.parse | ||
| * roundtrip strips undefined values). This migration covers both cases: | ||
| * - selectedAccount key is missing from internalAccounts | ||
| * - selectedAccount is explicitly undefined | ||
| * - selectedAccount is not a string | ||
| * | ||
| * @param state - The persisted Redux state. | ||
| * @returns The migrated Redux state. | ||
| */ | ||
| export default function migrate(state: unknown) { | ||
| if (!ensureValidState(state, migrationVersion)) { | ||
| return state; | ||
| } | ||
|
|
||
| if ( | ||
| !hasProperty(state.engine.backgroundState, 'AccountsController') || | ||
| !isObject(state.engine.backgroundState.AccountsController) | ||
| ) { | ||
| captureException( | ||
| new Error( | ||
| `Migration ${migrationVersion}: AccountsController state is missing or invalid: '${typeof state.engine.backgroundState.AccountsController}'`, | ||
| ), | ||
| ); | ||
| return state; | ||
| } | ||
|
|
||
| const accountsController = state.engine.backgroundState.AccountsController; | ||
|
|
||
| if ( | ||
| !hasProperty(accountsController, 'internalAccounts') || | ||
| !isObject(accountsController.internalAccounts) | ||
| ) { | ||
| captureException( | ||
| new Error( | ||
| `Migration ${migrationVersion}: internalAccounts is missing or invalid: '${typeof accountsController.internalAccounts}'`, | ||
| ), | ||
| ); | ||
| return state; | ||
| } | ||
|
|
||
| const { internalAccounts } = accountsController; | ||
| const hasKey = hasProperty(internalAccounts, 'selectedAccount'); | ||
| const currentValue = (internalAccounts as Record<string, unknown>) | ||
| .selectedAccount; | ||
|
|
||
| if (hasKey && typeof currentValue === 'string' && currentValue.length > 0) { | ||
| return state; | ||
| } | ||
|
|
||
| // Report the exact corruption type to Sentry for diagnostics | ||
| const corruptionType = !hasKey | ||
| ? 'key_missing' | ||
| : currentValue === undefined | ||
| ? 'value_undefined' | ||
| : typeof currentValue !== 'string' | ||
| ? `wrong_type_${typeof currentValue}` | ||
| : 'empty_string'; | ||
|
Check warning on line 67 in app/store/migrations/126.ts
|
||
|
|
||
| captureException( | ||
| new Error( | ||
| `Migration ${migrationVersion}: selectedAccount is corrupt (${corruptionType}). hasKey=${hasKey}, typeof=${typeof currentValue}, value=${String(currentValue)}`, | ||
| ), | ||
| ); | ||
|
|
||
| // Try to recover: set to the first account ID if accounts exist | ||
| if ( | ||
| hasProperty(internalAccounts, 'accounts') && | ||
| isObject(internalAccounts.accounts) | ||
| ) { | ||
| const accountIds = Object.keys(internalAccounts.accounts); | ||
| if (accountIds.length > 0) { | ||
| const firstAccount = internalAccounts.accounts[accountIds[0]]; | ||
| if (isObject(firstAccount) && hasProperty(firstAccount, 'id')) { | ||
| const accountId = firstAccount.id; | ||
| if (typeof accountId === 'string' && accountId.length > 0) { | ||
| (internalAccounts as Record<string, unknown>).selectedAccount = | ||
| accountId; | ||
| return state; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Fallback: set to empty string so AccountsController auto-reconciles | ||
| (internalAccounts as Record<string, unknown>).selectedAccount = ''; | ||
| return state; | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably we want to remove this exception being captured, it seems that will not help with really anything if we get to this point, only pollute sentry