From 5574cadd620d411644f5c49c2c4e5da8ace633c4 Mon Sep 17 00:00:00 2001 From: Cal-L Date: Mon, 25 Nov 2024 10:13:30 -0800 Subject: [PATCH 1/4] Rename accounts directory --- app/core/Engine/Engine.ts | 4 ++-- .../controllers/{AccountsController => accounts}/constants.ts | 0 .../{AccountsController => accounts}/utils.test.ts | 0 .../controllers/{AccountsController => accounts}/utils.ts | 0 4 files changed, 2 insertions(+), 2 deletions(-) rename app/core/Engine/controllers/{AccountsController => accounts}/constants.ts (100%) rename app/core/Engine/controllers/{AccountsController => accounts}/utils.test.ts (100%) rename app/core/Engine/controllers/{AccountsController => accounts}/utils.ts (100%) diff --git a/app/core/Engine/Engine.ts b/app/core/Engine/Engine.ts index eeb4ddc899c..40af1139b88 100644 --- a/app/core/Engine/Engine.ts +++ b/app/core/Engine/Engine.ts @@ -152,8 +152,8 @@ import { AccountsControllerSelectedAccountChangeEvent, AccountsControllerAccountAddedEvent, AccountsControllerAccountRenamedEvent, -} from './controllers/AccountsController/constants'; -import { createAccountsController } from './controllers/AccountsController/utils'; +} from './controllers/accounts/constants'; +import { createAccountsController } from './controllers/accounts/utils'; import { captureException } from '@sentry/react-native'; import { lowerCase } from 'lodash'; import { diff --git a/app/core/Engine/controllers/AccountsController/constants.ts b/app/core/Engine/controllers/accounts/constants.ts similarity index 100% rename from app/core/Engine/controllers/AccountsController/constants.ts rename to app/core/Engine/controllers/accounts/constants.ts diff --git a/app/core/Engine/controllers/AccountsController/utils.test.ts b/app/core/Engine/controllers/accounts/utils.test.ts similarity index 100% rename from app/core/Engine/controllers/AccountsController/utils.test.ts rename to app/core/Engine/controllers/accounts/utils.test.ts diff --git a/app/core/Engine/controllers/AccountsController/utils.ts b/app/core/Engine/controllers/accounts/utils.ts similarity index 100% rename from app/core/Engine/controllers/AccountsController/utils.ts rename to app/core/Engine/controllers/accounts/utils.ts From e07f99121362822ec19fceeeb0afe4048a80aad3 Mon Sep 17 00:00:00 2001 From: Cal-L Date: Mon, 25 Nov 2024 11:11:46 -0800 Subject: [PATCH 2/4] Move restricted messenger into Engine --- app/core/Engine/Engine.ts | 17 ++++++- .../Engine/controllers/accounts/README.md | 3 ++ .../Engine/controllers/accounts/utils.test.ts | 48 +++++++++++++------ app/core/Engine/controllers/accounts/utils.ts | 24 ++-------- 4 files changed, 57 insertions(+), 35 deletions(-) create mode 100644 app/core/Engine/controllers/accounts/README.md diff --git a/app/core/Engine/Engine.ts b/app/core/Engine/Engine.ts index 40af1139b88..a23bed16d3e 100644 --- a/app/core/Engine/Engine.ts +++ b/app/core/Engine/Engine.ts @@ -153,6 +153,7 @@ import { AccountsControllerAccountAddedEvent, AccountsControllerAccountRenamedEvent, } from './controllers/accounts/constants'; +import { AccountsControllerMessenger } from '@metamask/accounts-controller'; import { createAccountsController } from './controllers/accounts/utils'; import { captureException } from '@sentry/react-native'; import { lowerCase } from 'lodash'; @@ -339,8 +340,22 @@ export class Engine { }); // Create AccountsController + const accountsControllerMessenger: AccountsControllerMessenger = + this.controllerMessenger.getRestricted({ + name: 'AccountsController', + allowedEvents: [ + 'SnapController:stateChange', + 'KeyringController:accountRemoved', + 'KeyringController:stateChange', + ], + allowedActions: [ + 'KeyringController:getAccounts', + 'KeyringController:getKeyringsByType', + 'KeyringController:getKeyringForAccount', + ], + }); const accountsController = createAccountsController({ - messenger: this.controllerMessenger, + messenger: accountsControllerMessenger, initialState: initialState.AccountsController, }); diff --git a/app/core/Engine/controllers/accounts/README.md b/app/core/Engine/controllers/accounts/README.md new file mode 100644 index 00000000000..24ea735bd59 --- /dev/null +++ b/app/core/Engine/controllers/accounts/README.md @@ -0,0 +1,3 @@ +# Controllers owned by the Accounts team + +This folder contains controllers that are owned by the Accounts team. diff --git a/app/core/Engine/controllers/accounts/utils.test.ts b/app/core/Engine/controllers/accounts/utils.test.ts index 25f95dd2e14..67b640cb448 100644 --- a/app/core/Engine/controllers/accounts/utils.test.ts +++ b/app/core/Engine/controllers/accounts/utils.test.ts @@ -1,10 +1,12 @@ -import { AccountsControllerState } from '@metamask/accounts-controller'; +import { + AccountsControllerMessenger, + AccountsControllerState, +} from '@metamask/accounts-controller'; import { ExtendedControllerMessenger } from '../../../ExtendedControllerMessenger'; import { createAccountsController, defaultAccountsControllerState, } from './utils'; -import { ControllerMessenger } from '../../'; import { withScope } from '@sentry/react-native'; import { AGREED, METRICS_OPT_IN } from '../../../../constants/storage'; import StorageWrapper from '../../../../store/storage-wrapper'; @@ -16,7 +18,24 @@ const mockedWithScope = jest.mocked(withScope); describe('accountControllersUtils', () => { describe('createAccountsController', () => { + let accountsControllerMessenger: AccountsControllerMessenger; + beforeEach(() => { + const globalMessenger = new ExtendedControllerMessenger(); + accountsControllerMessenger = globalMessenger.getRestricted({ + name: 'AccountsController', + allowedEvents: [ + 'SnapController:stateChange', + 'KeyringController:accountRemoved', + 'KeyringController:stateChange', + ], + allowedActions: [ + 'KeyringController:getAccounts', + 'KeyringController:getKeyringsByType', + 'KeyringController:getKeyringForAccount', + ], + }); + // Mock required for Logger StorageWrapper.getItem = jest.fn((key: string) => { switch (key) { case METRICS_OPT_IN: @@ -32,14 +51,12 @@ describe('accountControllersUtils', () => { }); it('AccountsController state should be default state when no initial state is passed in', () => { - const controllerMessenger = new ExtendedControllerMessenger(); const accountsController = createAccountsController({ - messenger: controllerMessenger, + messenger: accountsControllerMessenger, }); expect(accountsController.state).toEqual(defaultAccountsControllerState); }); it('AccountsController state should be initial state when initial state is passed in', () => { - const controllerMessenger = new ExtendedControllerMessenger(); const initialAccountsControllerState: AccountsControllerState = { internalAccounts: { accounts: {}, @@ -47,27 +64,28 @@ describe('accountControllersUtils', () => { }, }; const accountsController = createAccountsController({ - messenger: controllerMessenger, + messenger: accountsControllerMessenger, initialState: initialAccountsControllerState, }); expect(accountsController.state).toEqual(initialAccountsControllerState); }); it('AccountsController name should be AccountsController', () => { - const controllerMessenger = new ExtendedControllerMessenger(); const accountsControllerName = 'AccountsController'; const accountsController = createAccountsController({ - messenger: controllerMessenger, + messenger: accountsControllerMessenger, }); expect(accountsController.name).toEqual(accountsControllerName); }); - it('should catch error when controller fails to initialize', async () => { - const controllerMessenger = - 'controllerMessenger' as unknown as ControllerMessenger; - const accountsController = await createAccountsController({ - messenger: controllerMessenger, - }); + it('should detect and log an error when controller fails to initialize', async () => { + const brokenAccountsControllerMessenger = + 'controllerMessenger' as unknown as AccountsControllerMessenger; + await expect(() => + createAccountsController({ + messenger: brokenAccountsControllerMessenger, + }), + ).toThrow(); + expect(mockedWithScope).toHaveBeenCalledTimes(1); - expect(accountsController).toEqual({}); }); }); }); diff --git a/app/core/Engine/controllers/accounts/utils.ts b/app/core/Engine/controllers/accounts/utils.ts index 44325a3bd65..8a9b96fb628 100644 --- a/app/core/Engine/controllers/accounts/utils.ts +++ b/app/core/Engine/controllers/accounts/utils.ts @@ -3,7 +3,6 @@ import { AccountsControllerMessenger, AccountsControllerState, } from '@metamask/accounts-controller'; -import { ControllerMessenger } from '../../types'; import Logger from '../../../../util/Logger'; // Default AccountsControllerState @@ -25,35 +24,22 @@ export const createAccountsController = ({ messenger, initialState, }: { - messenger: ControllerMessenger; + messenger: AccountsControllerMessenger; initialState?: AccountsControllerState; }): AccountsController => { let accountsController = {} as AccountsController; try { - const accountsControllerMessenger: AccountsControllerMessenger = - messenger.getRestricted({ - name: 'AccountsController', - allowedEvents: [ - 'SnapController:stateChange', - 'KeyringController:accountRemoved', - 'KeyringController:stateChange', - ], - allowedActions: [ - 'KeyringController:getAccounts', - 'KeyringController:getKeyringsByType', - 'KeyringController:getKeyringForAccount', - ], - }); - accountsController = new AccountsController({ - messenger: accountsControllerMessenger, + messenger, state: initialState ?? defaultAccountsControllerState, }); } catch (error) { // Report error while initializing AccountsController - // TODO: Direct to vault recovery to reset controller states Logger.error(error as Error, 'Failed to initialize AccountsController'); + + // TODO: Direct to vault recovery to reset controller states + throw error; } return accountsController; From e0b61918620d43ea99102d1f8baae0ce7f0997fb Mon Sep 17 00:00:00 2001 From: Cal-L Date: Mon, 25 Nov 2024 11:18:32 -0800 Subject: [PATCH 3/4] Clean up accounts controller logic --- app/core/Engine/controllers/accounts/utils.ts | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/app/core/Engine/controllers/accounts/utils.ts b/app/core/Engine/controllers/accounts/utils.ts index 8a9b96fb628..881e86e43ef 100644 --- a/app/core/Engine/controllers/accounts/utils.ts +++ b/app/core/Engine/controllers/accounts/utils.ts @@ -27,13 +27,12 @@ export const createAccountsController = ({ messenger: AccountsControllerMessenger; initialState?: AccountsControllerState; }): AccountsController => { - let accountsController = {} as AccountsController; - try { - accountsController = new AccountsController({ + const accountsController = new AccountsController({ messenger, state: initialState ?? defaultAccountsControllerState, }); + return accountsController; } catch (error) { // Report error while initializing AccountsController Logger.error(error as Error, 'Failed to initialize AccountsController'); @@ -41,6 +40,4 @@ export const createAccountsController = ({ // TODO: Direct to vault recovery to reset controller states throw error; } - - return accountsController; }; From 4943178beb0f46eeffd14267a1ab60e05fac10c2 Mon Sep 17 00:00:00 2001 From: Cal-L Date: Mon, 25 Nov 2024 11:22:15 -0800 Subject: [PATCH 4/4] Update codeowners --- .github/CODEOWNERS | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index 0950da58cae..796a961d095 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -44,7 +44,7 @@ app/util/walletconnect.js @MetaMask/sdk-devs @MetaMask/mobile-platform # Accounts Team app/core/Encryptor/ @MetaMask/accounts-engineers -app/core/Engine/controller/AccountsController @MetaMask/accounts-engineers +app/core/Engine/controllers/accounts @MetaMask/accounts-engineers # Swaps Team app/components/UI/Swaps @MetaMask/swaps-engineers @MetaMask/mobile-platform