Skip to content

Commit

Permalink
fix: Create migration 59 to fix undefined selectedAccount (#12177)
Browse files Browse the repository at this point in the history
<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

## **Description**

<!--
Write a short description of the changes included in this pull request,
also include relevant motivation and context. Have in mind the following
questions:
1. What is the reason for the change?
2. What is the improvement/solution?
-->

This PR addresses a crasher related to an undefined selectedAccount
causing the AccountsController to throw an error. Under this scenario,
the solution is to set selectedAccount to an empty string, which the
AccountsController will automatically reset the selectedAccount.

PR similar to #11866
but not the same

## **Related issues**

Fixes: #11488 

## **Manual testing steps**

1. Building on `main`, create a wallet and kill the app
2. Create a migration that sets `selectedAccount` on the
AccountsController to be undefined
3. Run the app, notice it encounters the error. Kill the app
4. Run this branch, which includes migration 59. Notice you are no
longer blocked and can log in
5. Will see an account selected by default after landing on wallet

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->
The error that users eventually see as a result of the migration
failures
<img width="1101" alt="Screenshot 2024-11-04 at 11 21 55 PM"
src="https://github.com/user-attachments/assets/c944e7ce-f68d-4cb6-b949-b8b57a4d8ec8">

### **After**

<!-- [screenshots/recordings] -->

After applying migration

https://github.com/user-attachments/assets/e959c8d9-5dfd-4b69-8f4d-16689bf66d95


## **Pre-merge author checklist**

- [x] I’ve followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile
Coding
Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've completed the PR template to the best of my ability
- [x] I’ve included tests if applicable
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [x] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [x] 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.
  • Loading branch information
Cal-L authored Nov 5, 2024
1 parent b2259b8 commit af62e1b
Show file tree
Hide file tree
Showing 3 changed files with 233 additions and 0 deletions.
164 changes: 164 additions & 0 deletions app/store/migrations/059.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,164 @@
import migrate from './059';
import { merge } from 'lodash';
import { captureException } from '@sentry/react-native';
import initialRootState from '../../util/test/initial-root-state';
import mockedEngine from '../../core/__mocks__/MockedEngine';
import {
expectedUuid,
expectedUuid2,
internalAccount1,
internalAccount2,
} from '../../util/test/accountsControllerTestUtils';

jest.mock('@sentry/react-native', () => ({
captureException: jest.fn(),
}));
const mockedCaptureException = jest.mocked(captureException);

jest.mock('../../core/Engine', () => ({
init: () => mockedEngine.init(),
}));

describe('Migration #59 - Fix crasher related to undefined selectedAccount on AccountsController', () => {
beforeEach(() => {
jest.restoreAllMocks();
jest.resetAllMocks();
});

const invalidStates = [
{
state: null,
errorMessage: "FATAL ERROR: Migration 59: Invalid state error: 'object'",
scenario: 'state is invalid',
},
{
state: merge({}, initialRootState, {
engine: null,
}),
errorMessage:
"FATAL ERROR: Migration 59: Invalid engine state error: 'object'",
scenario: 'engine state is invalid',
},
{
state: merge({}, initialRootState, {
engine: {
backgroundState: null,
},
}),
errorMessage:
"FATAL ERROR: Migration 59: Invalid engine backgroundState error: 'object'",
scenario: 'backgroundState is invalid',
},
];

for (const { errorMessage, scenario, state } of invalidStates) {
it(`should capture exception if ${scenario}`, async () => {
const newState = await migrate(state);

expect(newState).toStrictEqual(state);
expect(mockedCaptureException).toHaveBeenCalledWith(expect.any(Error));
expect(mockedCaptureException.mock.calls[0][0].message).toBe(
errorMessage,
);
});
}

it('should set selectedAccount to empty string if it is undefined', async () => {
const oldState = {
engine: {
backgroundState: {
AccountsController: {
internalAccounts: {
accounts: {},
selectedAccount: undefined,
},
},
},
},
};

const expectedState = {
engine: {
backgroundState: {
AccountsController: {
internalAccounts: {
accounts: {},
selectedAccount: '',
},
},
},
},
};

const migratedState = await migrate(oldState);
expect(migratedState).toStrictEqual(expectedState);
});

it('should set selectedAccount to the id of the first account if accounts exist', async () => {
const oldState = {
engine: {
backgroundState: {
AccountsController: {
internalAccounts: {
accounts: {
[expectedUuid]: internalAccount1,
[expectedUuid2]: internalAccount2,
},
selectedAccount: undefined,
},
},
},
},
};

const expectedState = {
engine: {
backgroundState: {
AccountsController: {
internalAccounts: {
accounts: {
[expectedUuid]: internalAccount1,
[expectedUuid2]: internalAccount2,
},
selectedAccount: expectedUuid,
},
},
},
},
};

const migratedState = await migrate(oldState);
expect(migratedState).toStrictEqual(expectedState);
});

it('should leave selectedAccount alone if it is not undefined', async () => {
const oldState = {
engine: {
backgroundState: {
AccountsController: {
internalAccounts: {
accounts: {},
selectedAccount: '0x1',
},
},
},
},
};

const expectedState = {
engine: {
backgroundState: {
AccountsController: {
internalAccounts: {
accounts: {},
selectedAccount: '0x1',
},
},
},
},
};

const migratedState = await migrate(oldState);
expect(migratedState).toStrictEqual(expectedState);
});
});
67 changes: 67 additions & 0 deletions app/store/migrations/059.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
import { hasProperty, isObject } from '@metamask/utils';
import { ensureValidState } from './util';
import Logger from '../../util/Logger';

/**
* Migration for checking if selectedAccount on AccountsController is undefined
* If it is, set the field to be an empty string.
* An undefined value causes the AccountsController to throw an Error
* Fix: The AccountsController automatically sets the selectedAddress if it is an empty string
* Fixes issue: View fullstack trace in https://github.com/MetaMask/metamask-mobile/issues/11488
*
* @param state
* @returns
*/
export default function migrate(state: unknown) {
if (!ensureValidState(state, 59)) {
// Increment the migration number as appropriate
return state;
}

// Check if selectedAccount is undefined
if (
hasProperty(state.engine.backgroundState, 'AccountsController') &&
isObject(state.engine.backgroundState.AccountsController) &&
hasProperty(
state.engine.backgroundState.AccountsController,
'internalAccounts',
) &&
isObject(
state.engine.backgroundState.AccountsController.internalAccounts,
) &&
hasProperty(
state.engine.backgroundState.AccountsController.internalAccounts,
'selectedAccount',
) &&
state.engine.backgroundState.AccountsController.internalAccounts
.selectedAccount === undefined
) {
// Try to set the selectedAccount to be the first account id
const internalAccounts =
state.engine.backgroundState.AccountsController.internalAccounts;
if (
hasProperty(internalAccounts, 'accounts') &&
isObject(internalAccounts.accounts) &&
Object.keys(internalAccounts.accounts).length > 0
) {
const firstAccount = Object.values(internalAccounts.accounts)[0];
if (isObject(firstAccount) && hasProperty(firstAccount, 'id')) {
Logger.log(
`Migration 59: Setting selectedAccount to the id of the first account.`,
);
state.engine.backgroundState.AccountsController.internalAccounts.selectedAccount =
firstAccount.id;
}
} else {
// Fallback to setting selectedAccount to empty string. AccountsController automatically reconciles the field
Logger.log(
`Migration 59: AccountController's selectedAccount is undefined. Setting it to empty string.`,
);
state.engine.backgroundState.AccountsController.internalAccounts.selectedAccount =
'';
}
}

// Return the modified state
return state;
}
2 changes: 2 additions & 0 deletions app/store/migrations/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ import migration55 from './055';
import migration56 from './056';
import migration57 from './057';
import migration58 from './058';
import migration59 from './059';

type MigrationFunction = (state: unknown) => unknown;
type AsyncMigrationFunction = (state: unknown) => Promise<unknown>;
Expand Down Expand Up @@ -130,6 +131,7 @@ export const migrationList: MigrationsList = {
56: migration56,
57: migration57,
58: migration58,
59: migration59,
};

// Enable both synchronous and asynchronous migrations
Expand Down

0 comments on commit af62e1b

Please sign in to comment.