chore(deps): bump accounts-controller to v18.1.0 and keyring-api …

…to v8.1.0 (#10821) ## **Description** This PR upgrades two critical dependencies: - `@metamask/accounts-controller` to the latest version 18.1.0 - `@metamask/keyring-api` to the latest version 8.1.0 - removed no longer necessary `patches/@MetaMask+accounts-controller+14.0.0.patch` These upgrades are necessary to enable different account types on mobile and to support account snaps. This change lays the groundwork for more flexible account management and enhanced functionality in the mobile app. Key changes include: - Creation of **Migration 52** to handle any necessary state transitions - Adjustments to existing code to accommodate new features and changes in the upgraded packages ## **Related issues** Fixes: https://github.com/MetaMask/accounts-planning/issues/569#issue-2481610395 ## **Manual testing steps** Here are some suggested testing steps: 1. Perform a clean install of the app and verify that all account-related functions work as expected. 2. Upgrade from a previous version and check that existing accounts are migrated correctly. 3. Test creating new accounts of different types (if applicable). 4. Verify that account switching works properly. 5. Check that account balances and transaction history are displayed correctly for all account types. 6. Ensure that sensitive operations (e.g., sending transactions, signing messages) work correctly for all account types. 7. Verify that the app's performance hasn't been negatively impacted by these changes. ## **Screenshots/Recordings** - Create an account ✅ - Persist selected account on app close ✅ - Send Eth between accounts ✅ - Account properties (name, balance etc correctly displayed) ✅ https://github.com/user-attachments/assets/dd8d27df-a40e-41bb-bc96-e835c6abaa4a ### Deletion of `patches/@MetaMask+accounts-controller+14.0.0.patch` Followed steps to test patch [here](#9764) - app behaves as expected without the patch ✅ https://github.com/user-attachments/assets/67f0f31a-d3d0-42ff-9b2b-5d558cfd9f9f ## **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** - [ ] 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. --------- Co-authored-by: Gustavo Antunes <[email protected]> Co-authored-by: tommasini <[email protected]>
MetaMask · Sep 12, 2024 · 41cb2c6 · 41cb2c6
1 parent e2994bb
commit 41cb2c6
Show file tree

Hide file tree

Showing 10 changed files with 332 additions and 119 deletions.
diff --git a/app/components/hooks/useAccounts/useAccounts.ts b/app/components/hooks/useAccounts/useAccounts.ts
@@ -30,7 +30,7 @@ import {
   UseAccounts,
   UseAccountsParams,
 } from './useAccounts.types';
-import { InternalAccountTypes } from '@metamask/keyring-api';
+import { InternalAccount } from '@metamask/keyring-api';
 import { Hex } from '@metamask/utils';
 
 /**
@@ -125,7 +125,7 @@ const useAccounts = ({
     let yOffset = 0;
     let selectedIndex = 0;
     const flattenedAccounts: Account[] = internalAccounts.map(
-      (internalAccount: InternalAccountTypes, index: number) => {
+      (internalAccount: InternalAccount, index: number) => {
         const {
           address,
           metadata: {

diff --git a/app/core/Engine.test.js b/app/core/Engine.test.js
@@ -1,6 +1,7 @@
 import Engine from './Engine';
 import { backgroundState } from '../util/test/initial-root-state';
 import { zeroAddress } from 'ethereumjs-util';
+import { createMockAccountsControllerState } from '../util/test/accountsControllerTestUtils';
 
 jest.unmock('./Engine');
 jest.mock('../store', () => ({ store: { getState: jest.fn(() => ({})) } }));
@@ -104,12 +105,10 @@ describe('Engine', () => {
     const ethConversionRate = 4000; // $4,000 / ETH
 
     const state = {
-      AccountsController: {
-        internalAccounts: {
-          selectedAccount: selectedAddress,
-          accounts: { [selectedAddress]: { address: selectedAddress } },
-        },
-      },
+      AccountsController: createMockAccountsControllerState(
+        [selectedAddress],
+        selectedAddress,
+      ),
       NetworkController: {
         state: { providerConfig: { chainId, ticker } },
       },

diff --git a/app/core/Engine.ts b/app/core/Engine.ts
@@ -581,7 +581,6 @@ class Engine {
       }),
       state: initialState.LoggingController,
     });
-    // @ts-expect-error TODO: Resolve mismatch between base-controller versions.
     const accountsControllerMessenger: AccountsControllerMessenger =
       this.controllerMessenger.getRestricted({
         name: 'AccountsController',

diff --git a/app/selectors/accountsController.ts b/app/selectors/accountsController.ts
@@ -5,6 +5,7 @@ import { createSelector } from 'reselect';
 import { RootState } from '../reducers';
 import { createDeepEqualSelector } from './util';
 import { selectFlattenedKeyringAccounts } from './keyringController';
+import { InternalAccount } from '@metamask/keyring-api';
 
 /**
  *
@@ -20,7 +21,7 @@ const selectAccountsControllerState = (state: RootState) =>
 export const selectInternalAccounts = createDeepEqualSelector(
   selectAccountsControllerState,
   selectFlattenedKeyringAccounts,
-  (accountControllerState, orderedKeyringAccounts) => {
+  (accountControllerState, orderedKeyringAccounts): InternalAccount[] => {
     const keyringAccountsMap = new Map(
       orderedKeyringAccounts.map((account, index) => [
         account.toLowerCase(),

diff --git a/app/store/migrations/052.test.ts b/app/store/migrations/052.test.ts
@@ -0,0 +1,196 @@
+import migration from './052';
+import { merge } from 'lodash';
+import initialRootState from '../../util/test/initial-root-state';
+import { captureException } from '@sentry/react-native';
+import {
+  expectedUuid,
+  expectedUuid2,
+  internalAccount1,
+  internalAccount2,
+} from '../../util/test/accountsControllerTestUtils';
+
+const oldState = {
+  engine: {
+    backgroundState: {
+      AccountsController: {
+        internalAccounts: {
+          accounts: {
+            [expectedUuid]: internalAccount1,
+            [expectedUuid2]: internalAccount2,
+          },
+          selectedAccount: expectedUuid,
+        },
+      },
+    },
+  },
+};
+
+jest.mock('@sentry/react-native', () => ({
+  captureException: jest.fn(),
+}));
+const mockedCaptureException = jest.mocked(captureException);
+
+describe('Migration #52', () => {
+  beforeEach(() => {
+    jest.restoreAllMocks();
+    jest.resetAllMocks();
+  });
+
+  const invalidStates = [
+    {
+      state: merge({}, initialRootState, {
+        engine: null,
+      }),
+      scenario: 'engine state is invalid',
+      expectedError:
+        "FATAL ERROR: Migration 52: Invalid engine state error: 'object'",
+    },
+    {
+      state: merge({}, initialRootState, {
+        engine: {
+          backgroundState: null,
+        },
+      }),
+      scenario: 'backgroundState is invalid',
+      expectedError:
+        "FATAL ERROR: Migration 52: Invalid engine backgroundState error: 'object'",
+    },
+    {
+      state: merge({}, initialRootState, {
+        engine: {
+          backgroundState: {
+            AccountsController: { internalAccounts: null },
+          },
+        },
+      }),
+      scenario: 'AccountsController internalAccounts state is invalid',
+      expectedError:
+        "FATAL ERROR: Migration 52: Invalid AccountsController state error: internalAccounts is not an object, type: 'object'",
+    },
+    {
+      state: merge({}, initialRootState, {
+        engine: {
+          backgroundState: {
+            AccountsController: {
+              internalAccounts: {
+                accounts: null,
+              },
+            },
+          },
+        },
+      }),
+      scenario: 'AccountsController internalAccounts accounts state is invalid',
+      expectedError:
+        "FATAL ERROR: Migration 52: Invalid AccountsController state error: internalAccounts.accounts is not an object, type: 'object'",
+    },
+    {
+      state: merge({}, initialRootState, {
+        engine: {
+          backgroundState: {
+            AccountsController: {
+              internalAccounts: {
+                accounts: {},
+                selectedAccount: null,
+              },
+            },
+          },
+        },
+      }),
+      scenario:
+        'AccountsController internalAccounts selectedAccount is not a string',
+      expectedError:
+        "FATAL ERROR: Migration 52: Invalid AccountsController state error: internalAccounts.selectedAccount is not a string, type: 'object'",
+    },
+  ];
+
+  for (const { scenario, state, expectedError } of invalidStates) {
+    it(`captures exception if ${scenario}`, () => {
+      const newState = migration(state);
+
+      expect(newState).toStrictEqual(state);
+      expect(mockedCaptureException).toHaveBeenCalledWith(expect.any(Error));
+      expect(mockedCaptureException.mock.calls[0][0].message).toBe(
+        expectedError,
+      );
+    });
+  }
+
+  it('does not change the selectedAccount if it is valid', () => {
+    const newState = migration(oldState) as typeof oldState;
+
+    expect(
+      newState.engine.backgroundState.AccountsController.internalAccounts
+        .selectedAccount,
+    ).toEqual(expectedUuid);
+  });
+
+  it('updates the selectedAccount if it is invalid', () => {
+    const invalidState = merge({}, oldState, {
+      engine: {
+        backgroundState: {
+          AccountsController: {
+            internalAccounts: {
+              selectedAccount: 'invalid-uuid',
+            },
+          },
+        },
+      },
+    });
+
+    const newState = migration(invalidState) as typeof invalidState;
+
+    expect(
+      newState.engine.backgroundState.AccountsController.internalAccounts
+        .selectedAccount,
+    ).toEqual(expectedUuid);
+  });
+
+  it('does not change the state if there are no accounts', () => {
+    const emptyAccountsState = merge({}, oldState, {
+      engine: {
+        backgroundState: {
+          AccountsController: {
+            internalAccounts: {
+              accounts: {},
+              selectedAccount: 'some-uuid',
+            },
+          },
+        },
+      },
+    });
+
+    const newState = migration(emptyAccountsState) as typeof emptyAccountsState;
+
+    expect(newState).toStrictEqual(emptyAccountsState);
+  });
+
+  it('updates the selectedAccount to the first account if current selection is invalid', () => {
+    const invalidSelectedState = merge({}, oldState, {
+      engine: {
+        backgroundState: {
+          AccountsController: {
+            internalAccounts: {
+              selectedAccount: 'non-existent-uuid',
+            },
+          },
+        },
+      },
+    });
+
+    const newState = migration(
+      invalidSelectedState,
+    ) as typeof invalidSelectedState;
+
+    expect(
+      newState.engine.backgroundState.AccountsController.internalAccounts
+        .selectedAccount,
+    ).toEqual(expectedUuid);
+  });
+
+  it('does not modify the state if the selectedAccount is valid', () => {
+    const validState = merge({}, oldState);
+    const newState = migration(validState) as typeof validState;
+
+    expect(newState).toStrictEqual(validState);
+  });
+});
diff --git a/app/store/migrations/052.ts b/app/store/migrations/052.ts
@@ -0,0 +1,98 @@
+import { captureException } from '@sentry/react-native';
+import { hasProperty, isObject } from '@metamask/utils';
+import { ensureValidState } from './util';
+import { AccountsControllerState } from '@metamask/accounts-controller';
+
+function isAccountsControllerState(
+  state: unknown,
+): state is AccountsControllerState {
+  if (!isObject(state)) {
+    captureException(
+      new Error(
+        `FATAL ERROR: Migration 52: Invalid AccountsController state error: AccountsController state is not an object, type: '${typeof state}'`,
+      ),
+    );
+    return false;
+  }
+
+  if (!hasProperty(state, 'internalAccounts')) {
+    captureException(
+      new Error(
+        'FATAL ERROR: Migration 52: Invalid AccountsController state error: missing internalAccounts',
+      ),
+    );
+    return false;
+  }
+
+  if (!isObject(state.internalAccounts)) {
+    captureException(
+      new Error(
+        `FATAL ERROR: Migration 52: Invalid AccountsController state error: internalAccounts is not an object, type: '${typeof state.internalAccounts}'`,
+      ),
+    );
+    return false;
+  }
+
+  if (!hasProperty(state.internalAccounts, 'accounts')) {
+    captureException(
+      new Error(
+        'FATAL ERROR: Migration 52: Invalid AccountsController state error: missing internalAccounts.accounts',
+      ),
+    );
+    return false;
+  }
+
+  if (!isObject(state.internalAccounts.accounts)) {
+    captureException(
+      new Error(
+        `FATAL ERROR: Migration 52: Invalid AccountsController state error: internalAccounts.accounts is not an object, type: '${typeof state
+          .internalAccounts.accounts}'`,
+      ),
+    );
+    return false;
+  }
+
+  if (!hasProperty(state.internalAccounts, 'selectedAccount')) {
+    captureException(
+      new Error(
+        'FATAL ERROR: Migration 52: Invalid AccountsController state error: missing internalAccounts.selectedAccount',
+      ),
+    );
+    return false;
+  }
+
+  if (typeof state.internalAccounts.selectedAccount !== 'string') {
+    captureException(
+      new Error(
+        `FATAL ERROR: Migration 52: Invalid AccountsController state error: internalAccounts.selectedAccount is not a string, type: '${typeof state
+          .internalAccounts.selectedAccount}'`,
+      ),
+    );
+    return false;
+  }
+
+  return true;
+}
+
+export default function migrate(state: unknown) {
+  if (!ensureValidState(state, 52)) {
+    return state;
+  }
+
+  const accountsControllerState =
+    state.engine.backgroundState.AccountsController;
+
+  if (!isAccountsControllerState(accountsControllerState)) {
+    return state;
+  }
+
+  const { accounts, selectedAccount } =
+    accountsControllerState.internalAccounts;
+
+  if (Object.values(accounts).length > 0 && !accounts[selectedAccount]) {
+    accountsControllerState.internalAccounts.selectedAccount =
+      Object.values(accounts)[0].id;
+  }
+
+  return state;
+}
diff --git a/app/store/migrations/index.ts b/app/store/migrations/index.ts
@@ -52,6 +52,7 @@ import migration48 from './048';
 import migration49 from './049';
 import migration50 from './050';
 import migration51 from './051';
+import migration52 from './052';
 
 type MigrationFunction = (state: unknown) => unknown;
 type AsyncMigrationFunction = (state: unknown) => Promise<unknown>;
@@ -116,6 +117,7 @@ export const migrationList: MigrationsList = {
   49: migration49,
   50: migration50,
   51: migration51,
+  52: migration52,
 };
 
 // Enable both synchronous and asynchronous migrations