From 208245a6133edd499536db1253270d0515d1903d Mon Sep 17 00:00:00 2001 From: cryptodev-2s <109512101+cryptodev-2s@users.noreply.github.com> Date: Thu, 28 Nov 2024 00:04:20 +0100 Subject: [PATCH] feat: migrate `AppMedataController` to inherit from BaseController V2 (#28783) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** This PR migrate `AppMedataController` to inherit from BaseController V2 [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/28783?quickstart=1) ## **Related issues** Fixes: https://github.com/MetaMask/metamask-extension/issues/28467 ## **Manual testing steps** ## **Screenshots/Recordings** ### **Before** ### **After** ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [x] 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 - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.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. --- app/scripts/controllers/app-metadata.test.ts | 197 ++++++++++++------- app/scripts/controllers/app-metadata.ts | 153 ++++++++++---- app/scripts/metamask-controller.js | 9 +- 3 files changed, 251 insertions(+), 108 deletions(-) diff --git a/app/scripts/controllers/app-metadata.test.ts b/app/scripts/controllers/app-metadata.test.ts index cd9f87e238a0..faff2be5ab10 100644 --- a/app/scripts/controllers/app-metadata.test.ts +++ b/app/scripts/controllers/app-metadata.test.ts @@ -1,11 +1,8 @@ -import AppMetadataController from './app-metadata'; - -const EXPECTED_DEFAULT_STATE = { - currentAppVersion: '', - previousAppVersion: '', - previousMigrationVersion: 0, - currentMigrationVersion: 0, -}; +import { ControllerMessenger } from '@metamask/base-controller'; +import AppMetadataController, { + getDefaultAppMetadataControllerState, + type AppMetadataControllerOptions, +} from './app-metadata'; describe('AppMetadataController', () => { describe('constructor', () => { @@ -16,86 +13,142 @@ describe('AppMetadataController', () => { previousMigrationVersion: 1, currentMigrationVersion: 1, }; - const appMetadataController = new AppMetadataController({ - state: initState, - currentMigrationVersion: 1, - currentAppVersion: '1', - }); - expect(appMetadataController.store.getState()).toStrictEqual(initState); + withController( + { + state: initState, + currentMigrationVersion: 1, + currentAppVersion: '1', + }, + ({ controller }) => { + expect(controller.state).toStrictEqual(initState); + }, + ); }); - it('sets default state and does not modify it', async () => { - const appMetadataController = new AppMetadataController({ - state: {}, + it('sets default state and does not modify it', () => { + withController({ state: {} }, ({ controller }) => { + expect(controller.state).toStrictEqual( + getDefaultAppMetadataControllerState(), + ); }); - expect(appMetadataController.store.getState()).toStrictEqual( - EXPECTED_DEFAULT_STATE, - ); }); - it('sets default state and does not modify it if options version parameters match respective default values', async () => { - const appMetadataController = new AppMetadataController({ - state: {}, - currentMigrationVersion: 0, - currentAppVersion: '', - }); - expect(appMetadataController.store.getState()).toStrictEqual( - EXPECTED_DEFAULT_STATE, + it('sets default state and does not modify it if options version parameters match respective default values', () => { + withController( + { + state: {}, + currentMigrationVersion: 0, + currentAppVersion: '', + }, + ({ controller }) => { + expect(controller.state).toStrictEqual( + getDefaultAppMetadataControllerState(), + ); + }, ); }); - it('updates the currentAppVersion state property if options.currentAppVersion does not match the default value', async () => { - const appMetadataController = new AppMetadataController({ - state: {}, - currentMigrationVersion: 0, - currentAppVersion: '1', - }); - expect(appMetadataController.store.getState()).toStrictEqual({ - ...EXPECTED_DEFAULT_STATE, - currentAppVersion: '1', - }); + it('updates the currentAppVersion state property if options.currentAppVersion does not match the default value', () => { + withController( + { + state: {}, + currentMigrationVersion: 0, + currentAppVersion: '1', + }, + ({ controller }) => { + expect(controller.state).toStrictEqual({ + ...getDefaultAppMetadataControllerState(), + currentAppVersion: '1', + }); + }, + ); }); - it('updates the currentAppVersion and previousAppVersion state properties if options.currentAppVersion, currentAppVersion and previousAppVersion are all different', async () => { - const appMetadataController = new AppMetadataController({ - state: { - currentAppVersion: '2', - previousAppVersion: '1', + it('updates the currentAppVersion and previousAppVersion state properties if options.currentAppVersion, currentAppVersion and previousAppVersion are all different', () => { + withController( + { + state: { + currentAppVersion: '2', + previousAppVersion: '1', + }, + currentAppVersion: '3', + currentMigrationVersion: 0, }, - currentAppVersion: '3', - currentMigrationVersion: 0, - }); - expect(appMetadataController.store.getState()).toStrictEqual({ - ...EXPECTED_DEFAULT_STATE, - currentAppVersion: '3', - previousAppVersion: '2', - }); + ({ controller }) => { + expect(controller.state).toStrictEqual({ + ...getDefaultAppMetadataControllerState(), + currentAppVersion: '3', + previousAppVersion: '2', + }); + }, + ); }); - it('updates the currentMigrationVersion state property if the currentMigrationVersion param does not match the default value', async () => { - const appMetadataController = new AppMetadataController({ - state: {}, - currentMigrationVersion: 1, - }); - expect(appMetadataController.store.getState()).toStrictEqual({ - ...EXPECTED_DEFAULT_STATE, - currentMigrationVersion: 1, - }); + it('updates the currentMigrationVersion state property if the currentMigrationVersion param does not match the default value', () => { + withController( + { + state: {}, + currentMigrationVersion: 1, + }, + ({ controller }) => { + expect(controller.state).toStrictEqual({ + ...getDefaultAppMetadataControllerState(), + currentMigrationVersion: 1, + }); + }, + ); }); - it('updates the currentMigrationVersion and previousMigrationVersion state properties if the currentMigrationVersion param, the currentMigrationVersion state property and the previousMigrationVersion state property are all different', async () => { - const appMetadataController = new AppMetadataController({ - state: { - currentMigrationVersion: 2, - previousMigrationVersion: 1, + it('updates the currentMigrationVersion and previousMigrationVersion state properties if the currentMigrationVersion param, the currentMigrationVersion state property and the previousMigrationVersion state property are all different', () => { + withController( + { + state: { + currentMigrationVersion: 2, + previousMigrationVersion: 1, + }, + currentMigrationVersion: 3, }, - currentMigrationVersion: 3, - }); - expect(appMetadataController.store.getState()).toStrictEqual({ - ...EXPECTED_DEFAULT_STATE, - currentMigrationVersion: 3, - previousMigrationVersion: 2, - }); + ({ controller }) => { + expect(controller.state).toStrictEqual({ + ...getDefaultAppMetadataControllerState(), + currentMigrationVersion: 3, + previousMigrationVersion: 2, + }); + }, + ); }); }); }); + +type WithControllerOptions = Partial; + +type WithControllerCallback = ({ + controller, +}: { + controller: AppMetadataController; +}) => ReturnValue; + +type WithControllerArgs = + | [WithControllerCallback] + | [WithControllerOptions, WithControllerCallback]; + +function withController( + ...args: WithControllerArgs +): ReturnValue { + const [options = {}, fn] = args.length === 2 ? args : [{}, args[0]]; + + const controllerMessenger = new ControllerMessenger(); + + const messenger = controllerMessenger.getRestricted({ + name: 'AppMetadataController', + allowedActions: [], + allowedEvents: [], + }); + + return fn({ + controller: new AppMetadataController({ + messenger, + ...options, + }), + }); +} diff --git a/app/scripts/controllers/app-metadata.ts b/app/scripts/controllers/app-metadata.ts index 0d745730d0c0..495dd547a2d7 100644 --- a/app/scripts/controllers/app-metadata.ts +++ b/app/scripts/controllers/app-metadata.ts @@ -1,5 +1,22 @@ -import EventEmitter from 'events'; -import { ObservableStore } from '@metamask/obs-store'; +import { + BaseController, + ControllerGetStateAction, + ControllerStateChangeEvent, + RestrictedControllerMessenger, +} from '@metamask/base-controller'; + +// Unique name for the controller +const controllerName = 'AppMetadataController'; + +/** + * The options that AppMetadataController takes. + */ +export type AppMetadataControllerOptions = { + state?: Partial; + messenger: AppMetadataControllerMessenger; + currentMigrationVersion?: number; + currentAppVersion?: string; +}; /** * The state of the AppMetadataController @@ -12,19 +29,84 @@ export type AppMetadataControllerState = { }; /** - * The options that NetworkController takes. + * Function to get default state of the {@link AppMetadataController}. */ -export type AppMetadataControllerOptions = { - currentMigrationVersion?: number; - currentAppVersion?: string; - state?: Partial; -}; +export const getDefaultAppMetadataControllerState = + (): AppMetadataControllerState => ({ + currentAppVersion: '', + previousAppVersion: '', + previousMigrationVersion: 0, + currentMigrationVersion: 0, + }); + +/** + * Returns the state of the {@link AppMetadataController}. + */ +export type AppMetadataControllerGetStateAction = ControllerGetStateAction< + typeof controllerName, + AppMetadataControllerState +>; + +/** + * Actions exposed by the {@link AppMetadataController}. + */ +export type AppMetadataControllerActions = AppMetadataControllerGetStateAction; + +/** + * Event emitted when the state of the {@link AppMetadataController} changes. + */ +export type AppMetadataControllerStateChangeEvent = ControllerStateChangeEvent< + typeof controllerName, + AppMetadataControllerState +>; + +export type AppMetadataControllerEvents = AppMetadataControllerStateChangeEvent; + +/** + * Actions that this controller is allowed to call. + */ +type AllowedActions = never; -const defaultState: AppMetadataControllerState = { - currentAppVersion: '', - previousAppVersion: '', - previousMigrationVersion: 0, - currentMigrationVersion: 0, +/** + * Events that this controller is allowed to subscribe. + */ +type AllowedEvents = never; + +/** + * Messenger type for the {@link AppMetadataController}. + */ +type AppMetadataControllerMessenger = RestrictedControllerMessenger< + typeof controllerName, + AppMetadataControllerActions | AllowedActions, + AppMetadataControllerEvents | AllowedEvents, + AllowedActions['type'], + AllowedEvents['type'] +>; + +/** + * {@link AppMetadataController}'s metadata. + * + * This allows us to choose if fields of the state should be persisted or not + * using the `persist` flag; and if they can be sent to Sentry or not, using + * the `anonymous` flag. + */ +const controllerMetadata = { + currentAppVersion: { + persist: true, + anonymous: true, + }, + previousAppVersion: { + persist: true, + anonymous: true, + }, + previousMigrationVersion: { + persist: true, + anonymous: true, + }, + currentMigrationVersion: { + persist: true, + anonymous: true, + }, }; /** @@ -33,30 +115,34 @@ const defaultState: AppMetadataControllerState = { * run migration. * */ -export default class AppMetadataController extends EventEmitter { - /** - * Observable store containing controller data. - */ - store: ObservableStore; - +export default class AppMetadataController extends BaseController< + typeof controllerName, + AppMetadataControllerState, + AppMetadataControllerMessenger +> { /** * Constructs a AppMetadata controller. * * @param options - the controller options * @param options.state - Initial controller state. + * @param options.messenger - Messenger used to communicate with BaseV2 controller. * @param options.currentMigrationVersion * @param options.currentAppVersion */ constructor({ + state = {}, + messenger, currentAppVersion = '', currentMigrationVersion = 0, - state = {}, }: AppMetadataControllerOptions) { - super(); - - this.store = new ObservableStore({ - ...defaultState, - ...state, + super({ + name: controllerName, + metadata: controllerMetadata, + state: { + ...getDefaultAppMetadataControllerState(), + ...state, + }, + messenger, }); this.#maybeUpdateAppVersion(currentAppVersion); @@ -70,12 +156,12 @@ export default class AppMetadataController extends EventEmitter { * @param maybeNewAppVersion */ #maybeUpdateAppVersion(maybeNewAppVersion: string): void { - const oldCurrentAppVersion = this.store.getState().currentAppVersion; + const oldCurrentAppVersion = this.state.currentAppVersion; if (maybeNewAppVersion !== oldCurrentAppVersion) { - this.store.updateState({ - currentAppVersion: maybeNewAppVersion, - previousAppVersion: oldCurrentAppVersion, + this.update((state) => { + state.currentAppVersion = maybeNewAppVersion; + state.previousAppVersion = oldCurrentAppVersion; }); } } @@ -86,13 +172,12 @@ export default class AppMetadataController extends EventEmitter { * @param maybeNewMigrationVersion */ #maybeUpdateMigrationVersion(maybeNewMigrationVersion: number): void { - const oldCurrentMigrationVersion = - this.store.getState().currentMigrationVersion; + const oldCurrentMigrationVersion = this.state.currentMigrationVersion; if (maybeNewMigrationVersion !== oldCurrentMigrationVersion) { - this.store.updateState({ - previousMigrationVersion: oldCurrentMigrationVersion, - currentMigrationVersion: maybeNewMigrationVersion, + this.update((state) => { + state.previousMigrationVersion = oldCurrentMigrationVersion; + state.currentMigrationVersion = maybeNewMigrationVersion; }); } } diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 052e428dd55a..63479c1202f9 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -478,6 +478,11 @@ export default class MetamaskController extends EventEmitter { this.appMetadataController = new AppMetadataController({ state: initState.AppMetadataController, + messenger: this.controllerMessenger.getRestricted({ + name: 'AppMetadataController', + allowedActions: [], + allowedEvents: [], + }), currentMigrationVersion: this.currentMigrationVersion, currentAppVersion: version, }); @@ -2457,7 +2462,7 @@ export default class MetamaskController extends EventEmitter { this.store.updateStructure({ AccountsController: this.accountsController, AppStateController: this.appStateController.store, - AppMetadataController: this.appMetadataController.store, + AppMetadataController: this.appMetadataController, MultichainBalancesController: this.multichainBalancesController, TransactionController: this.txController, KeyringController: this.keyringController, @@ -2512,7 +2517,7 @@ export default class MetamaskController extends EventEmitter { config: { AccountsController: this.accountsController, AppStateController: this.appStateController.store, - AppMetadataController: this.appMetadataController.store, + AppMetadataController: this.appMetadataController, MultichainBalancesController: this.multichainBalancesController, NetworkController: this.networkController, KeyringController: this.keyringController,