-
Notifications
You must be signed in to change notification settings - Fork 5k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
refactor: modular controller init (#28948)
## **Description** Architecture to support modular controller initialisation to avoid the current monolithic `metamask-controller.js`. Includes: - `PPOMController` as a simple example. - `TransactionController` as perhaps the most complex example. ### Overview Uses isolated functions (in `app/scripts/controller-init/`) with a generic `ControllerInitFunction` type, each of which returns the following given a standardised and generic `ControllerInitRequest`: - Required `controller` instance. - Optional `api` object to declare background API functions. - Optional `persistedStateKey` to specify persisted state key, or disable. - Optional `memStateKey` to specify mem-store state key, or disable. Dependent controllers can be retrieved via a type safe `getController` function in the `ControllerInitRequest` to provide a standard error message if controller is not yet initialised. Messengers are intentionally defined and assigned in separate isolated files to allow alternate code ownership, and not over-power the controller logic and introduce bugs. Initialisation logic is encapsulated in `app/scripts/controller-init/utils.ts`, which iterates over the functions to construct the controller instances, plus generate the API and store properties. ### Advantages - Backwards compatible with almost no changes to legacy controller initialisation. - Facilitates an iterative approach meaning teams can refactor individual controllers ad-hoc. - Facilitates iterative Typescript adoption of controller initialisation code. - Supports modular unit testing of controller / messenger event handlers. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/28948?quickstart=1) ## **Related issues** ## **Manual testing steps** ## **Screenshots/Recordings** ### **Before** ### **After** ## **Pre-merge author checklist** - [ ] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [ ] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/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: Mark Stacey <[email protected]>
- Loading branch information
1 parent
2290ce0
commit 75be75c
Showing
14 changed files
with
1,681 additions
and
276 deletions.
There are no files selected for viewing
This file contains 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
106 changes: 106 additions & 0 deletions
106
app/scripts/controller-init/confirmations/ppom-controller-init.test.ts
This file contains 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,106 @@ | ||
import { | ||
PPOMController, | ||
PPOMControllerMessenger, | ||
} from '@metamask/ppom-validator'; | ||
import { ControllerMessenger } from '@metamask/base-controller'; | ||
import { PreferencesController } from '../../controllers/preferences-controller'; | ||
import { buildControllerInitRequestMock, CHAIN_ID_MOCK } from '../test/utils'; | ||
import { ControllerInitRequest } from '../types'; | ||
import { | ||
getPPOMControllerInitMessenger, | ||
getPPOMControllerMessenger, | ||
PPOMControllerInitMessenger, | ||
} from '../messengers/ppom-controller-messenger'; | ||
import { PPOMControllerInit } from './ppom-controller-init'; | ||
|
||
type PPOMControllerOptions = ConstructorParameters<typeof PPOMController>[0]; | ||
|
||
jest.mock('@metamask/ppom-validator'); | ||
|
||
/** | ||
* Build a mock PreferencesController. | ||
* | ||
* @param partialMock - A partial mock object for the PreferencesController, merged | ||
* with the default mock. | ||
* @returns A mock PreferencesController. | ||
*/ | ||
function buildControllerMock( | ||
partialMock?: Partial<PreferencesController>, | ||
): PreferencesController { | ||
const defaultPreferencesControllerMock = { | ||
state: { securityAlertsEnabled: true }, | ||
}; | ||
|
||
// @ts-expect-error Incomplete mock, just includes properties used by code-under-test. | ||
return { | ||
...defaultPreferencesControllerMock, | ||
...partialMock, | ||
}; | ||
} | ||
|
||
function buildInitRequestMock(): jest.Mocked< | ||
ControllerInitRequest<PPOMControllerMessenger, PPOMControllerInitMessenger> | ||
> { | ||
const baseControllerMessenger = new ControllerMessenger(); | ||
|
||
const requestMock = { | ||
...buildControllerInitRequestMock(), | ||
controllerMessenger: getPPOMControllerMessenger(baseControllerMessenger), | ||
initMessenger: getPPOMControllerInitMessenger(baseControllerMessenger), | ||
}; | ||
|
||
requestMock.getController.mockReturnValue(buildControllerMock()); | ||
|
||
return requestMock; | ||
} | ||
|
||
describe('PPOM Controller Init', () => { | ||
const ppomControllerClassMock = jest.mocked(PPOMController); | ||
|
||
/** | ||
* Extract a constructor option passed to the controller. | ||
* | ||
* @param option - The option to extract. | ||
* @param dependencyProperties - Any properties required on the controller dependencies. | ||
* @returns The extracted option. | ||
*/ | ||
function testConstructorOption<T extends keyof PPOMControllerOptions>( | ||
option: T, | ||
dependencyProperties?: Record<string, unknown>, | ||
): PPOMControllerOptions[T] { | ||
const requestMock = buildInitRequestMock(); | ||
|
||
requestMock.getController.mockReturnValue( | ||
buildControllerMock(dependencyProperties), | ||
); | ||
|
||
PPOMControllerInit(requestMock); | ||
|
||
return ppomControllerClassMock.mock.calls[0][0][option]; | ||
} | ||
|
||
beforeEach(() => { | ||
jest.resetAllMocks(); | ||
}); | ||
|
||
it('returns controller instance', () => { | ||
const requestMock = buildInitRequestMock(); | ||
expect(PPOMControllerInit(requestMock).controller).toBeInstanceOf( | ||
PPOMController, | ||
); | ||
}); | ||
|
||
it('determines if security alerts enabled using preference', () => { | ||
const securityAlertsEnabled = testConstructorOption( | ||
'securityAlertsEnabled', | ||
{ state: { securityAlertsEnabled: true } }, | ||
); | ||
|
||
expect(securityAlertsEnabled).toBe(true); | ||
}); | ||
|
||
it('sets chain ID to global chain ID', () => { | ||
const chainId = testConstructorOption('chainId'); | ||
expect(chainId).toBe(CHAIN_ID_MOCK); | ||
}); | ||
}); |
52 changes: 52 additions & 0 deletions
52
app/scripts/controller-init/confirmations/ppom-controller-init.ts
This file contains 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,52 @@ | ||
import { | ||
PPOMController, | ||
PPOMControllerMessenger, | ||
} from '@metamask/ppom-validator'; | ||
import { IndexedDBPPOMStorage } from '../../lib/ppom/indexed-db-backend'; | ||
import * as PPOMModule from '../../lib/ppom/ppom'; | ||
import { ControllerInitFunction } from '../types'; | ||
import { PPOMControllerInitMessenger } from '../messengers/ppom-controller-messenger'; | ||
|
||
export const PPOMControllerInit: ControllerInitFunction< | ||
PPOMController, | ||
PPOMControllerMessenger, | ||
PPOMControllerInitMessenger | ||
> = (request) => { | ||
const { | ||
controllerMessenger, | ||
initMessenger, | ||
getController, | ||
getGlobalChainId, | ||
getProvider, | ||
persistedState, | ||
} = request; | ||
|
||
const preferencesController = () => getController('PreferencesController'); | ||
|
||
const controller = new PPOMController({ | ||
messenger: controllerMessenger, | ||
storageBackend: new IndexedDBPPOMStorage('PPOMDB', 1), | ||
provider: getProvider(), | ||
ppomProvider: { | ||
// @ts-expect-error Controller and PPOM wrapper have different argument types in `new` and `validateJsonRpc` | ||
PPOM: PPOMModule.PPOM, | ||
ppomInit: () => PPOMModule.default(process.env.PPOM_URI), | ||
}, | ||
// @ts-expect-error State type is not `Partial` in controller. | ||
state: persistedState.PPOMController, | ||
chainId: getGlobalChainId(), | ||
securityAlertsEnabled: preferencesController().state.securityAlertsEnabled, | ||
// @ts-expect-error `onPreferencesChange` type signature is incorrect in `PPOMController` | ||
onPreferencesChange: initMessenger.subscribe.bind( | ||
initMessenger, | ||
'PreferencesController:stateChange', | ||
), | ||
// Both values have defaults in `builds.yml` so should always be defined. | ||
cdnBaseUrl: process.env.BLOCKAID_FILE_CDN as string, | ||
blockaidPublicKey: process.env.BLOCKAID_PUBLIC_KEY as string, | ||
}); | ||
|
||
return { | ||
controller, | ||
}; | ||
}; |
189 changes: 189 additions & 0 deletions
189
app/scripts/controller-init/confirmations/transaction-controller-init.test.ts
This file contains 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,189 @@ | ||
import { | ||
TransactionController, | ||
TransactionControllerMessenger, | ||
TransactionControllerOptions, | ||
} from '@metamask/transaction-controller'; | ||
import { ControllerMessenger } from '@metamask/base-controller'; | ||
import { NetworkController } from '@metamask/network-controller'; | ||
import { buildControllerInitRequestMock, CHAIN_ID_MOCK } from '../test/utils'; | ||
import { | ||
getTransactionControllerInitMessenger, | ||
getTransactionControllerMessenger, | ||
TransactionControllerInitMessenger, | ||
} from '../messengers/transaction-controller-messenger'; | ||
import { ControllerInitRequest } from '../types'; | ||
import { TransactionControllerInit } from './transaction-controller-init'; | ||
|
||
jest.mock('@metamask/transaction-controller'); | ||
|
||
/** | ||
* Build a mock NetworkController. | ||
* | ||
* @param partialMock - A partial mock object for the NetworkController, merged | ||
* with the default mock. | ||
* @returns A mock NetworkController. | ||
*/ | ||
function buildControllerMock( | ||
partialMock?: Partial<NetworkController>, | ||
): NetworkController { | ||
const defaultNetworkControllerMock = { | ||
getNetworkClientRegistry: jest.fn().mockReturnValue({}), | ||
}; | ||
|
||
// @ts-expect-error Incomplete mock, just includes properties used by code-under-test. | ||
return { | ||
...defaultNetworkControllerMock, | ||
...partialMock, | ||
}; | ||
} | ||
|
||
function buildInitRequestMock(): jest.Mocked< | ||
ControllerInitRequest< | ||
TransactionControllerMessenger, | ||
TransactionControllerInitMessenger | ||
> | ||
> { | ||
const baseControllerMessenger = new ControllerMessenger(); | ||
|
||
const requestMock = { | ||
...buildControllerInitRequestMock(), | ||
controllerMessenger: getTransactionControllerMessenger( | ||
baseControllerMessenger, | ||
), | ||
initMessenger: getTransactionControllerInitMessenger( | ||
baseControllerMessenger, | ||
), | ||
}; | ||
|
||
requestMock.getController.mockReturnValue(buildControllerMock()); | ||
|
||
return requestMock; | ||
} | ||
|
||
describe('Transaction Controller Init', () => { | ||
const transactionControllerClassMock = jest.mocked(TransactionController); | ||
|
||
/** | ||
* Extract a constructor option passed to the controller. | ||
* | ||
* @param option - The option to extract. | ||
* @param dependencyProperties - Any properties required on the controller dependencies. | ||
* @returns The extracted option. | ||
*/ | ||
function testConstructorOption<T extends keyof TransactionControllerOptions>( | ||
option: T, | ||
dependencyProperties: Record<string, unknown> = {}, | ||
): TransactionControllerOptions[T] { | ||
const requestMock = buildInitRequestMock(); | ||
|
||
requestMock.getController.mockReturnValue( | ||
buildControllerMock(dependencyProperties), | ||
); | ||
|
||
TransactionControllerInit(requestMock); | ||
|
||
return transactionControllerClassMock.mock.calls[0][0][option]; | ||
} | ||
|
||
beforeEach(() => { | ||
jest.resetAllMocks(); | ||
}); | ||
|
||
it('returns controller instance', () => { | ||
const requestMock = buildInitRequestMock(); | ||
expect(TransactionControllerInit(requestMock).controller).toBeInstanceOf( | ||
TransactionController, | ||
); | ||
}); | ||
|
||
it('retrieves saved gas fees from preferences', () => { | ||
const getSavedGasFees = testConstructorOption('getSavedGasFees', { | ||
state: { | ||
advancedGasFee: { | ||
[CHAIN_ID_MOCK]: { | ||
maxBaseFee: '0x1', | ||
priorityFee: '0x2', | ||
}, | ||
}, | ||
}, | ||
}); | ||
|
||
expect(getSavedGasFees?.(CHAIN_ID_MOCK)).toStrictEqual({ | ||
maxBaseFee: '0x1', | ||
priorityFee: '0x2', | ||
}); | ||
}); | ||
|
||
describe('determines incoming transactions is enabled', () => { | ||
it('when enabled in preferences and onboarding complete', () => { | ||
const incomingTransactionsIsEnabled = testConstructorOption( | ||
'incomingTransactions', | ||
{ | ||
state: { | ||
completedOnboarding: true, | ||
incomingTransactionsPreferences: { | ||
[CHAIN_ID_MOCK]: true, | ||
}, | ||
}, | ||
}, | ||
)?.isEnabled; | ||
|
||
expect(incomingTransactionsIsEnabled?.()).toBe(true); | ||
}); | ||
|
||
it('unless enabled in preferences but onboarding incomplete', () => { | ||
const incomingTransactionsIsEnabled = testConstructorOption( | ||
'incomingTransactions', | ||
{ | ||
state: { | ||
completedOnboarding: false, | ||
incomingTransactionsPreferences: { | ||
[CHAIN_ID_MOCK]: true, | ||
}, | ||
}, | ||
}, | ||
)?.isEnabled; | ||
|
||
expect(incomingTransactionsIsEnabled?.()).toBe(false); | ||
}); | ||
|
||
it('unless disabled in preferences and onboarding complete', () => { | ||
const incomingTransactionsIsEnabled = testConstructorOption( | ||
'incomingTransactions', | ||
{ | ||
state: { | ||
completedOnboarding: true, | ||
incomingTransactionsPreferences: { | ||
[CHAIN_ID_MOCK]: false, | ||
}, | ||
}, | ||
}, | ||
)?.isEnabled; | ||
|
||
expect(incomingTransactionsIsEnabled?.()).toBe(false); | ||
}); | ||
}); | ||
|
||
it('determines if first time interaction enabled using preference', () => { | ||
const isFirstTimeInteractionEnabled = testConstructorOption( | ||
'isFirstTimeInteractionEnabled', | ||
{ | ||
state: { | ||
securityAlertsEnabled: true, | ||
}, | ||
}, | ||
); | ||
|
||
expect(isFirstTimeInteractionEnabled?.()).toBe(true); | ||
}); | ||
|
||
it('determines if simulation enabled using preference', () => { | ||
const isSimulationEnabled = testConstructorOption('isSimulationEnabled', { | ||
state: { | ||
useTransactionSimulations: true, | ||
}, | ||
}); | ||
|
||
expect(isSimulationEnabled?.()).toBe(true); | ||
}); | ||
}); |
Oops, something went wrong.