diff --git a/docs/contributing.md b/docs/contributing.md index 43b9230d95e..e9007a0ccf4 100644 --- a/docs/contributing.md +++ b/docs/contributing.md @@ -46,9 +46,9 @@ If you need to customize the behavior of Jest for a package, see `jest.config.js ## Linting -[ESLint](https://eslint.org/docs/v8.x/) v8 (via [MetaMask's shared ESLint configurations](https://github.com/MetaMask/eslint-config)) is used to check for code quality issues, and [Prettier](https://prettier.io/docs/en/) is used to format files. +[ESLint](https://eslint.org) v9 (via [MetaMask's shared ESLint configurations](https://github.com/MetaMask/eslint-config)) is used to check for code quality issues, and [Prettier](https://prettier.io/docs/en/) is used to format files. -If you need to customize the behavior of ESLint, see `.eslintrc.js` in the root. +If you need to customize the behavior of ESLint, see `eslint.config.mjs` in the root. - Run `yarn lint` to lint all files and show possible violations across the monorepo. - Run `yarn lint:fix` to fix any automatically fixable violations. diff --git a/docs/writing-controllers.md b/docs/writing-controllers.md index 14be710aa6f..58ccfcd22c5 100644 --- a/docs/writing-controllers.md +++ b/docs/writing-controllers.md @@ -223,7 +223,7 @@ If the recipient controller supports the messaging system, however, the callback const name = 'FooController'; -type FooControllerMessenger = RestrictedControllerMessenger< +type FooControllerMessenger = RestrictedMessenger< typeof name, never, never, @@ -247,10 +247,7 @@ class FooController extends BaseController< // === Client repo === -const rootMessenger = new ControllerMessenger< - 'BarController:stateChange', - never ->(); +const rootMessenger = new Messenger<'BarController:stateChange', never>(); const barControllerMessenger = rootMessenger.getRestricted({ name: 'BarController', }); @@ -307,7 +304,7 @@ However, this pattern can be replaced with the use of the messenger: const name = 'FooController'; -type FooControllerMessenger = RestrictedControllerMessenger< +type FooControllerMessenger = RestrictedMessenger< typeof name, never, never, @@ -331,10 +328,7 @@ class FooController extends BaseController< // === Client repo === -const rootMessenger = new ControllerMessenger< - 'FooController:someEvent', - never ->(); +const rootMessenger = new Messenger<'FooController:someEvent', never>(); const fooControllerMessenger = rootMessenger.getRestricted({ name: 'FooController', }); @@ -505,7 +499,7 @@ A controller should define and export a type union that holds all of its actions The name of this type should be `${ControllerName}Actions`. -This type should be only passed to `RestrictedControllerMessenger` as the 2nd type parameter. It should _not_ be included in its 4th type parameter, as that is is used for external actions. +This type should be only passed to `RestrictedMessenger` as the 2nd type parameter. It should _not_ be included in its 4th type parameter, as that is is used for external actions. 🚫 **`FooController['type']` is passed as the 4th type parameter** @@ -514,7 +508,7 @@ export type FooControllerActions = | FooControllerUpdateCurrencyAction | FooControllerUpdateRatesAction; -export type FooControllerMessenger = RestrictedControllerMessenger< +export type FooControllerMessenger = RestrictedMessenger< 'FooController', FooControllerActions, never, @@ -530,7 +524,7 @@ export type FooControllerActions = | FooControllerUpdateCurrencyAction | FooControllerUpdateRatesAction; -export type FooControllerMessenger = RestrictedControllerMessenger< +export type FooControllerMessenger = RestrictedMessenger< 'FooController', FooControllerActions, never, @@ -545,7 +539,7 @@ A controller should define and export a type union that holds all of its events. The name of this type should be `${ControllerName}Events`. -This type should be only passed to `RestrictedControllerMessenger` as the 3rd type parameter. It should _not_ be included in its 5th type parameter, as that is is used for external events. +This type should be only passed to `RestrictedMessenger` as the 3rd type parameter. It should _not_ be included in its 5th type parameter, as that is is used for external events. 🚫 **`FooControllerEvents['type']` is passed as the 5th type parameter** @@ -554,7 +548,7 @@ export type FooControllerEvents = | FooControllerMessageReceivedEvent | FooControllerNotificationAddedEvent; -export type FooControllerMessenger = RestrictedControllerMessenger< +export type FooControllerMessenger = RestrictedMessenger< 'FooController', never, FooControllerEvents, @@ -570,7 +564,7 @@ export type FooControllerEvents = | FooControllerMessageReceivedEvent | FooControllerNotificationAddedEvent; -export type FooControllerMessenger = RestrictedControllerMessenger< +export type FooControllerMessenger = RestrictedMessenger< 'FooController', never, FooControllerEvents, @@ -583,11 +577,11 @@ export type FooControllerMessenger = RestrictedControllerMessenger< A controller may wish to call actions defined by other controllers, and therefore will need to define them in the messenger's allowlist. -In this case, the controller should group these types into a type union so that they can be easily passed to the `RestrictedControllerMessenger` type. However, it should not export this type, as it would then be re-exporting types that another package has already exported. +In this case, the controller should group these types into a type union so that they can be easily passed to the `RestrictedMessenger` type. However, it should not export this type, as it would then be re-exporting types that another package has already exported. The name of this type should be `AllowedActions`. -This type should not only be passed to `RestrictedControllerMessenger` as the 2nd type parameter, but should also be included in its 4th type parameter. +This type should not only be passed to `RestrictedMessenger` as the 2nd type parameter, but should also be included in its 4th type parameter. 🚫 **`never` is passed as the 4th type parameter** @@ -596,7 +590,7 @@ export type AllowedActions = | BarControllerDoSomethingAction | BarControllerDoSomethingElseAction; -export type FooControllerMessenger = RestrictedControllerMessenger< +export type FooControllerMessenger = RestrictedMessenger< 'FooController', AllowedActions, never, @@ -614,7 +608,7 @@ export type AllowedActions = | BarControllerDoSomethingAction | BarControllerDoSomethingElseAction; -export type FooControllerMessenger = RestrictedControllerMessenger< +export type FooControllerMessenger = RestrictedMessenger< 'FooController', AllowedActions, never, @@ -634,7 +628,7 @@ type AllowedActions = | BarControllerDoSomethingAction | BarControllerDoSomethingElseAction; -export type FooControllerMessenger = RestrictedControllerMessenger< +export type FooControllerMessenger = RestrictedMessenger< 'FooController', AllowedActions, never, @@ -649,7 +643,7 @@ If, in a test, you need to access all of the actions included in a controller's // NOTE: You may need to adjust the path depending on where you are import { ExtractAvailableAction } from '../../base-controller/tests/helpers'; -const messenger = new ControllerMessenger< +const messenger = new Messenger< ExtractAvailableAction, never >(); @@ -659,11 +653,11 @@ const messenger = new ControllerMessenger< A controller may wish to subscribe to events defined by other controllers, and therefore will need to define them in the messenger's allowlist. -In this case, the controller should group these types into a type union so that they can be easily passed to the `RestrictedControllerMessenger` type. However, it should not export this type, as it would then be re-exporting types that another package has already exported. +In this case, the controller should group these types into a type union so that they can be easily passed to the `RestrictedMessenger` type. However, it should not export this type, as it would then be re-exporting types that another package has already exported. The name of this type should be `AllowedEvents`. -This type should not only be passed to `RestrictedControllerMessenger` as the 3rd type parameter, but should also be included in its 5th type parameter. +This type should not only be passed to `RestrictedMessenger` as the 3rd type parameter, but should also be included in its 5th type parameter. 🚫 **`never` is passed as the 5th type parameter** @@ -672,7 +666,7 @@ export type AllowedEvents = | BarControllerSomethingHappenedEvent | BarControllerSomethingElseHappenedEvent; -export type FooControllerMessenger = RestrictedControllerMessenger< +export type FooControllerMessenger = RestrictedMessenger< 'FooController', never, AllowedEvents, @@ -690,7 +684,7 @@ export type AllowedEvents = | BarControllerSomethingHappenedEvent | BarControllerSomethingElseHappenedEvent; -export type FooControllerMessenger = RestrictedControllerMessenger< +export type FooControllerMessenger = RestrictedMessenger< 'FooController', never, AllowedEvents, @@ -710,7 +704,7 @@ type AllowedEvents = | BarControllerSomethingHappenedEvent | BarControllerSomethingElseHappenedEvent; -export type FooControllerMessenger = RestrictedControllerMessenger< +export type FooControllerMessenger = RestrictedMessenger< 'FooController', never, AllowedEvents, @@ -725,7 +719,7 @@ If, in a test, you need to access all of the events included in a controller's m // NOTE: You may need to adjust the path depending on where you are import { ExtractAvailableEvent } from '../../base-controller/tests/helpers'; -const messenger = new ControllerMessenger< +const messenger = new Messenger< never, ExtractAvailableEvent >(); @@ -790,7 +784,7 @@ export type AllowedEvents = | ApprovalControllerApprovalRequestApprovedEvent | ApprovalControllerApprovalRequestRejectedEvent; -export type SwapsControllerMessenger = RestrictedControllerMessenger< +export type SwapsControllerMessenger = RestrictedMessenger< 'SwapsController', SwapsControllerActions | AllowedActions, SwapsControllerEvents | AllowedEvents, @@ -802,7 +796,7 @@ export type SwapsControllerMessenger = RestrictedControllerMessenger< A messenger that allows no actions or events (whether internal or external) looks like this: ```typescript -export type SwapsControllerMessenger = RestrictedControllerMessenger< +export type SwapsControllerMessenger = RestrictedMessenger< 'SwapsController', never, never, @@ -1175,7 +1169,7 @@ import { AccountsControllerGetStateAction } from '@metamask/accounts-controller' type AllowedActions = AccountsControllerGetStateAction; -type PreferencesControllerMessenger = RestrictedControllerMessenger< +type PreferencesControllerMessenger = RestrictedMessenger< 'PreferencesController', AllowedActions, never, @@ -1273,7 +1267,7 @@ type AccountsControllerActions = | AccountsControllerGetActiveAccountAction | AccountsControllerGetInactiveAccountsAction; -export type AccountsControllerMessenger = RestrictedControllerMessenger< +export type AccountsControllerMessenger = RestrictedMessenger< 'AccountsController', AccountsControllerActions, never, @@ -1302,7 +1296,7 @@ type AllowedActions = | AccountsControllerGetActiveAccountsAction | AccountsControllerGetInactiveAccountsAction; -export type TokensControllerMessenger = RestrictedControllerMessenger< +export type TokensControllerMessenger = RestrictedMessenger< 'TokensController', AllowedActions, never, @@ -1383,7 +1377,7 @@ export type AccountsControllerGetStateAction = ControllerGetStateAction< type AccountsControllerActions = AccountsControllerGetStateAccountAction; -export type AccountsControllerMessenger = RestrictedControllerMessenger< +export type AccountsControllerMessenger = RestrictedMessenger< 'AccountsController', AccountsControllerActions, never, @@ -1400,7 +1394,7 @@ import { accountsControllerSelectors } from '@metamask/accounts-controller'; type AllowedActions = AccountsControllerGetStateAction; -export type TokensControllerMessenger = RestrictedControllerMessenger< +export type TokensControllerMessenger = RestrictedMessenger< 'TokensController', AllowedActions, never, diff --git a/eslint-warning-thresholds.json b/eslint-warning-thresholds.json new file mode 100644 index 00000000000..fc7d2173b69 --- /dev/null +++ b/eslint-warning-thresholds.json @@ -0,0 +1,31 @@ +{ + "@typescript-eslint/consistent-type-exports": 19, + "@typescript-eslint/no-base-to-string": 3, + "@typescript-eslint/no-duplicate-enum-values": 2, + "@typescript-eslint/no-misused-promises": 3, + "@typescript-eslint/no-unsafe-enum-comparison": 59, + "@typescript-eslint/no-unused-vars": 36, + "@typescript-eslint/prefer-promise-reject-errors": 13, + "@typescript-eslint/prefer-readonly": 145, + "@typescript-eslint/switch-exhaustiveness-check": 10, + "import-x/namespace": 189, + "import-x/no-named-as-default": 1, + "import-x/no-named-as-default-member": 8, + "import-x/order": 209, + "jest/no-conditional-in-test": 129, + "jest/prefer-lowercase-title": 2, + "jest/prefer-strict-equal": 2, + "jsdoc/check-tag-names": 375, + "jsdoc/require-returns": 22, + "jsdoc/tag-lines": 328, + "n/no-unsupported-features/node-builtins": 18, + "n/prefer-global/text-encoder": 4, + "n/prefer-global/text-decoder": 4, + "prettier/prettier": 115, + "promise/always-return": 3, + "promise/catch-or-return": 2, + "promise/param-names": 8, + "no-empty-function": 2, + "no-shadow": 8, + "no-unused-private-class-members": 5 +} diff --git a/eslint.config.mjs b/eslint.config.mjs index ec01ff56f2b..dec0b60327c 100644 --- a/eslint.config.mjs +++ b/eslint.config.mjs @@ -1,19 +1,12 @@ import base, { createConfig } from '@metamask/eslint-config'; -import nodejs from '@metamask/eslint-config-nodejs'; import jest from '@metamask/eslint-config-jest'; +import nodejs from '@metamask/eslint-config-nodejs'; import typescript from '@metamask/eslint-config-typescript'; -const config = createConfig( +const config = createConfig([ + ...base, { ignores: [ - 'yarn.lock', - '**/**.map', - '**/**.tsbuildinfo', - '**/*.json', - '**/*.md', - '**/LICENSE', - '**/*.sh', - '**/.DS_Store', '**/dist/**', '**/docs/**', '**/coverage/**', @@ -22,7 +15,6 @@ const config = createConfig( 'scripts/create-package/package-template/**', ], }, - ...base, { rules: { // Left disabled because various properties throughough this repo are snake_case because the @@ -32,14 +24,12 @@ const config = createConfig( 'id-length': 'off', // TODO: re-enble most of these rules - '@typescript-eslint/naming-convention': 'off', 'function-paren-newline': 'off', 'id-denylist': 'off', 'implicit-arrow-linebreak': 'off', - 'import/no-anonymous-default-export': 'off', - 'import/no-unassigned-import': 'off', + 'import-x/no-anonymous-default-export': 'off', + 'import-x/no-unassigned-import': 'off', 'lines-around-comment': 'off', - 'n/no-sync': 'off', 'no-async-promise-executor': 'off', 'no-case-declarations': 'off', 'no-invalid-this': 'off', @@ -53,6 +43,12 @@ const config = createConfig( 'off', { matchDescription: '^[A-Z`\\d_][\\s\\S]*[.?!`>)}]$' }, ], + + // TODO: These rules created more errors after the upgrade to ESLint 9. + // Re-enable these rules and address any lint violations. + 'import-x/no-named-as-default-member': 'warn', + 'prettier/prettier': 'warn', + 'no-empty-function': 'warn', }, settings: { jsdoc: { @@ -62,26 +58,38 @@ const config = createConfig( }, { files: [ - '**/jest.config.js', - '**/jest.environment.js', - '**/tests/**/*.{ts,js}', - '*.js', - '*.test.{ts,js}', + '**/*.{js,cjs,mjs}', + '**/*.test.{js,ts}', + '**/tests/**/*.{js,ts}', 'scripts/*.ts', - 'scripts/create-package/*.ts', - 'yarn.config.cjs', + 'scripts/create-package/**/*.ts', ], extends: [nodejs], + rules: { + // TODO: Re-enable this + 'n/no-sync': 'off', + // TODO: These rules created more errors after the upgrade to ESLint 9. + // Re-enable these rules and address any lint violations. + 'n/no-unsupported-features/node-builtins': 'warn', + }, }, { - files: ['*.test.{ts,js}', '**/tests/**/*.{ts,js}'], + files: ['**/*.test.{js,ts}', '**/tests/**/*.{js,ts}'], extends: [jest], + rules: { + // TODO: These rules created more errors after the upgrade to ESLint 9. + // Re-enable these rules and address any lint violations. + 'jest/no-conditional-in-test': 'warn', + 'jest/prefer-lowercase-title': 'warn', + 'jest/prefer-strict-equal': 'warn', + }, }, { // These files are test helpers, not tests. We still use the Jest ESLint // config here to ensure that ESLint expects a test-like environment, but // various rules meant just to apply to tests have been disabled. - files: ['**/tests/**/*.{ts,js}', '!*.test.{ts,js}'], + files: ['**/tests/**/*.{js,ts}'], + ignores: ['**/*.test.{js,ts}'], rules: { 'jest/no-export': 'off', 'jest/require-top-level-describe': 'off', @@ -89,20 +97,31 @@ const config = createConfig( }, }, { - files: ['*.js', '*.cjs'], - parserOptions: { + files: ['**/*.{js,cjs}'], + languageOptions: { sourceType: 'script', - ecmaVersion: '2020', + ecmaVersion: 2020, }, }, { - files: ['*.ts'], + files: ['**/*.ts'], extends: [typescript], - parserOptions: { - tsconfigRootDir: import.meta.dirname, - project: ['./tsconfig.packages.json'], + languageOptions: { + parserOptions: { + tsconfigRootDir: import.meta.dirname, + project: './tsconfig.packages.json', + // Disable `projectService` because we run into out-of-memory issues. + // See this ticket for inspiration out how to solve this: + // + projectService: false, + }, }, rules: { + // This rule does not detect multiple imports of the same file where types + // are being imported in one case and runtime values are being imported in + // another + 'import-x/no-duplicates': 'off', + // Enable rules that are disabled in `@metamask/eslint-config-typescript` '@typescript-eslint/no-explicit-any': 'error', @@ -110,6 +129,7 @@ const config = createConfig( '@typescript-eslint/promise-function-async': 'off', // TODO: re-enable most of these rules + '@typescript-eslint/naming-convention': 'off', '@typescript-eslint/no-unnecessary-type-assertion': 'off', '@typescript-eslint/unbound-method': 'off', '@typescript-eslint/prefer-enum-initializers': 'off', @@ -118,26 +138,51 @@ const config = createConfig( '@typescript-eslint/prefer-reduce-type-parameter': 'off', 'no-restricted-syntax': 'off', 'no-restricted-globals': 'off', + + // TODO: These rules created more errors after the upgrade to ESLint 9. + // Re-enable these rules and address any lint violations. + '@typescript-eslint/consistent-type-exports': 'warn', + '@typescript-eslint/explicit-function-return-type': 'off', + '@typescript-eslint/no-base-to-string': 'warn', + '@typescript-eslint/no-duplicate-enum-values': 'warn', + '@typescript-eslint/no-misused-promises': 'warn', + '@typescript-eslint/no-unsafe-enum-comparison': 'warn', + '@typescript-eslint/no-unused-vars': 'warn', + '@typescript-eslint/only-throw-error': 'warn', + '@typescript-eslint/prefer-promise-reject-errors': 'warn', + '@typescript-eslint/prefer-readonly': 'warn', + '@typescript-eslint/switch-exhaustiveness-check': 'warn', + 'import-x/namespace': 'warn', + 'import-x/no-named-as-default': 'warn', + 'import-x/order': 'warn', + 'jsdoc/check-tag-names': 'warn', + 'jsdoc/require-returns': 'warn', + 'jsdoc/tag-lines': 'warn', + 'no-unused-private-class-members': 'warn', + 'promise/always-return': 'warn', + 'promise/catch-or-return': 'warn', + 'promise/param-names': 'warn', }, }, { files: ['tests/setupAfterEnv/matchers.ts'], - parserOptions: { + languageOptions: { sourceType: 'script', }, }, + // This should really be in `@metamask/eslint-config-typescript` { - files: ['*.d.ts'], + files: ['**/*.d.ts'], rules: { '@typescript-eslint/naming-convention': 'warn', - 'import/unambiguous': 'off', + 'import-x/unambiguous': 'off', }, }, { files: ['scripts/*.ts'], rules: { - // All scripts will have shebangs. - 'n/shebang': 'off', + // Scripts may be self-executable and thus have hashbangs. + 'n/hashbang': 'off', }, }, { @@ -145,8 +190,20 @@ const config = createConfig( rules: { // These files run under Node, and thus `require(...)` is expected. 'n/global-require': 'off', + + // TODO: These rules created more errors after the upgrade to ESLint 9. + // Re-enable these rules and address any lint violations. + 'n/prefer-global/text-encoder': 'warn', + 'n/prefer-global/text-decoder': 'warn', + 'no-shadow': 'warn', + }, + }, + { + files: ['**/*.mjs'], + languageOptions: { + sourceType: 'module', }, }, -); +]); export default config; diff --git a/examples/example-controllers/src/gas-prices-controller.test.ts b/examples/example-controllers/src/gas-prices-controller.test.ts index 93a5efc22c9..b6ed3967a97 100644 --- a/examples/example-controllers/src/gas-prices-controller.test.ts +++ b/examples/example-controllers/src/gas-prices-controller.test.ts @@ -1,4 +1,4 @@ -import { ControllerMessenger } from '@metamask/base-controller'; +import { Messenger } from '@metamask/base-controller'; import { GasPricesController } from '@metamask/example-controllers'; import type { GasPricesControllerMessenger } from '@metamask/example-controllers'; @@ -27,7 +27,7 @@ describe('GasPricesController', () => { }, }; const controller = new GasPricesController({ - messenger: getControllerMessenger(), + messenger: getMessenger(), state: givenState, gasPricesService, }); @@ -38,7 +38,7 @@ describe('GasPricesController', () => { it('fills in missing state properties with default values', () => { const gasPricesService = buildGasPricesService(); const controller = new GasPricesController({ - messenger: getControllerMessenger(), + messenger: getMessenger(), gasPricesService, }); @@ -66,14 +66,14 @@ describe('GasPricesController', () => { average: 10, high: 15, }); - const rootMessenger = getRootControllerMessenger({ + const rootMessenger = getRootMessenger({ networkControllerGetStateActionHandler: () => ({ ...getDefaultNetworkControllerState(), chainId: '0x42', }), }); const controller = new GasPricesController({ - messenger: getControllerMessenger(rootMessenger), + messenger: getMessenger(rootMessenger), gasPricesService, }); @@ -112,7 +112,7 @@ type RootEvent = ExtractAvailableEvent; * `NetworkController:getState` action on the messenger. * @returns The unrestricted messenger suited for GasPricesController. */ -function getRootControllerMessenger({ +function getRootMessenger({ networkControllerGetStateActionHandler = jest .fn< ReturnType, @@ -121,8 +121,8 @@ function getRootControllerMessenger({ .mockReturnValue(getDefaultNetworkControllerState()), }: { networkControllerGetStateActionHandler?: NetworkControllerGetStateAction['handler']; -} = {}): ControllerMessenger { - const rootMessenger = new ControllerMessenger(); +} = {}): Messenger { + const rootMessenger = new Messenger(); rootMessenger.registerActionHandler( 'NetworkController:getState', networkControllerGetStateActionHandler, @@ -137,8 +137,8 @@ function getRootControllerMessenger({ * @param rootMessenger - The root messenger to restrict. * @returns The restricted messenger. */ -function getControllerMessenger( - rootMessenger = getRootControllerMessenger(), +function getMessenger( + rootMessenger = getRootMessenger(), ): GasPricesControllerMessenger { return rootMessenger.getRestricted({ name: 'GasPricesController', diff --git a/examples/example-controllers/src/gas-prices-controller.ts b/examples/example-controllers/src/gas-prices-controller.ts index ecfc37a4dc8..5d2fb50930e 100644 --- a/examples/example-controllers/src/gas-prices-controller.ts +++ b/examples/example-controllers/src/gas-prices-controller.ts @@ -1,7 +1,7 @@ import type { ControllerGetStateAction, ControllerStateChangeEvent, - RestrictedControllerMessenger, + RestrictedMessenger, StateMetadata, } from '@metamask/base-controller'; import { BaseController } from '@metamask/base-controller'; @@ -121,7 +121,7 @@ type AllowedEvents = never; * The messenger which is restricted to actions and events accessed by * {@link GasPricesController}. */ -export type GasPricesControllerMessenger = RestrictedControllerMessenger< +export type GasPricesControllerMessenger = RestrictedMessenger< typeof controllerName, GasPricesControllerActions | AllowedActions, GasPricesControllerEvents | AllowedEvents, @@ -151,7 +151,7 @@ export function getDefaultGasPricesControllerState(): GasPricesControllerState { * @example * * ``` ts - * import { ControllerMessenger } from '@metamask/base-controller'; + * import { Messenger } from '@metamask/base-controller'; * import { * GasPricesController, * GasPricesService @@ -164,7 +164,7 @@ export function getDefaultGasPricesControllerState(): GasPricesControllerState { * * // Assuming that you're using this in the browser * const gasPricesService = new GasPricesService({ fetch }); - * const rootMessenger = new ControllerMessenger< + * const rootMessenger = new Messenger< * GasPricesControllerActions | NetworkControllerGetStateAction, * GasPricesControllerEvents * >(); diff --git a/examples/example-controllers/src/pet-names-controller.test.ts b/examples/example-controllers/src/pet-names-controller.test.ts index 8d92282553c..9369dde88ba 100644 --- a/examples/example-controllers/src/pet-names-controller.test.ts +++ b/examples/example-controllers/src/pet-names-controller.test.ts @@ -1,4 +1,4 @@ -import { ControllerMessenger } from '@metamask/base-controller'; +import { Messenger } from '@metamask/base-controller'; import type { ExtractAvailableAction, @@ -20,7 +20,7 @@ describe('PetNamesController', () => { }, }; const controller = new PetNamesController({ - messenger: getControllerMessenger(), + messenger: getMessenger(), state: givenState, }); @@ -29,7 +29,7 @@ describe('PetNamesController', () => { it('fills in missing state properties with default values', () => { const controller = new PetNamesController({ - messenger: getControllerMessenger(), + messenger: getMessenger(), }); expect(controller.state).toMatchInlineSnapshot(` @@ -44,7 +44,7 @@ describe('PetNamesController', () => { for (const blockedKey of PROTOTYPE_POLLUTION_BLOCKLIST) { it(`throws if given a chainId of "${blockedKey}"`, () => { const controller = new PetNamesController({ - messenger: getControllerMessenger(), + messenger: getMessenger(), }); expect(() => @@ -56,7 +56,7 @@ describe('PetNamesController', () => { it('registers the given pet name in state with the given chain ID and address', () => { const controller = new PetNamesController({ - messenger: getControllerMessenger(), + messenger: getMessenger(), state: { namesByChainIdAndAddress: { '0x1': { @@ -80,7 +80,7 @@ describe('PetNamesController', () => { it("creates a new group for the chain if it doesn't already exist", () => { const controller = new PetNamesController({ - messenger: getControllerMessenger(), + messenger: getMessenger(), }); controller.assignPetName('0x1', '0xaaaaaa', 'My Account'); @@ -96,7 +96,7 @@ describe('PetNamesController', () => { it('overwrites any existing pet name for the address', () => { const controller = new PetNamesController({ - messenger: getControllerMessenger(), + messenger: getMessenger(), state: { namesByChainIdAndAddress: { '0x1': { @@ -119,7 +119,7 @@ describe('PetNamesController', () => { it('lowercases the given address before registering it to avoid duplicate entries', () => { const controller = new PetNamesController({ - messenger: getControllerMessenger(), + messenger: getMessenger(), state: { namesByChainIdAndAddress: { '0x1': { @@ -158,11 +158,8 @@ type RootEvent = ExtractAvailableEvent; * * @returns The unrestricted messenger suited for PetNamesController. */ -function getRootControllerMessenger(): ControllerMessenger< - RootAction, - RootEvent -> { - return new ControllerMessenger(); +function getRootMessenger(): Messenger { + return new Messenger(); } /** @@ -172,8 +169,8 @@ function getRootControllerMessenger(): ControllerMessenger< * @param rootMessenger - The root messenger to restrict. * @returns The restricted messenger. */ -function getControllerMessenger( - rootMessenger = getRootControllerMessenger(), +function getMessenger( + rootMessenger = getRootMessenger(), ): PetNamesControllerMessenger { return rootMessenger.getRestricted({ name: 'PetNamesController', diff --git a/examples/example-controllers/src/pet-names-controller.ts b/examples/example-controllers/src/pet-names-controller.ts index cece05f6ec1..5997b8ac2f5 100644 --- a/examples/example-controllers/src/pet-names-controller.ts +++ b/examples/example-controllers/src/pet-names-controller.ts @@ -1,7 +1,7 @@ import type { ControllerGetStateAction, ControllerStateChangeEvent, - RestrictedControllerMessenger, + RestrictedMessenger, StateMetadata, } from '@metamask/base-controller'; import { BaseController } from '@metamask/base-controller'; @@ -89,7 +89,7 @@ type AllowedEvents = never; * The messenger which is restricted to actions and events accessed by * {@link PetNamesController}. */ -export type PetNamesControllerMessenger = RestrictedControllerMessenger< +export type PetNamesControllerMessenger = RestrictedMessenger< typeof controllerName, PetNamesControllerActions | AllowedActions, PetNamesControllerEvents | AllowedEvents, @@ -120,13 +120,13 @@ export function getDefaultPetNamesControllerState(): PetNamesControllerState { * @example * * ``` ts - * import { ControllerMessenger } from '@metamask/base-controller'; + * import { Messenger } from '@metamask/base-controller'; * import type { * PetNamesControllerActions, * PetNamesControllerEvents * } from '@metamask/example-controllers'; * - * const rootMessenger = new ControllerMessenger< + * const rootMessenger = new Messenger< * PetNamesControllerActions, * PetNamesControllerEvents * >(); diff --git a/package.json b/package.json index 990d517bb3a..a52e92956af 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@metamask/core-monorepo", - "version": "281.0.0", + "version": "283.0.0", "private": true, "description": "Monorepo for packages shared between MetaMask clients", "repository": { @@ -23,7 +23,7 @@ "lint": "yarn lint:eslint && yarn lint:misc --check && yarn constraints && yarn lint:dependencies && yarn lint:teams", "lint:dependencies": "depcheck && yarn dedupe --check", "lint:dependencies:fix": "depcheck && yarn dedupe", - "lint:eslint": "eslint . --cache", + "lint:eslint": "yarn ts-node ./scripts/run-eslint.ts --cache", "lint:fix": "yarn lint:eslint --fix && yarn lint:misc --write && yarn constraints --fix && yarn lint:dependencies:fix", "lint:misc": "prettier --no-error-on-unmatched-pattern '**/*.json' '**/*.md' '**/*.yml' '!.yarnrc.yml' '!merged-packages/**' --ignore-path .gitignore", "lint:teams": "ts-node scripts/lint-teams-json.ts", diff --git a/packages/controller-utils/CHANGELOG.md b/packages/controller-utils/CHANGELOG.md index 5774eba9cab..8d5452c1495 100644 --- a/packages/controller-utils/CHANGELOG.md +++ b/packages/controller-utils/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- Add `createServicePolicy` function to assist with reducing boilerplate for service classes ([#5154](https://github.com/MetaMask/core/pull/5154), [#5143](https://github.com/MetaMask/core/pull/5143)) + ## [11.4.5] ### Changed diff --git a/packages/controller-utils/jest.config.js b/packages/controller-utils/jest.config.js index 1d042ba6d40..746ae98e6f5 100644 --- a/packages/controller-utils/jest.config.js +++ b/packages/controller-utils/jest.config.js @@ -18,7 +18,7 @@ module.exports = merge(baseConfig, { coverageThreshold: { global: { branches: 78.12, - functions: 85.41, + functions: 84.61, lines: 87.3, statements: 86.5, }, diff --git a/packages/controller-utils/package.json b/packages/controller-utils/package.json index 8f67f9cfc2b..4cc3ceb4bf2 100644 --- a/packages/controller-utils/package.json +++ b/packages/controller-utils/package.json @@ -55,6 +55,7 @@ "@types/bn.js": "^5.1.5", "bignumber.js": "^9.1.2", "bn.js": "^5.2.1", + "cockatiel": "^3.1.2", "eth-ens-namehash": "^2.0.8", "fast-deep-equal": "^3.1.3" }, @@ -64,7 +65,9 @@ "@types/jest": "^27.4.1", "deepmerge": "^4.2.2", "jest": "^27.5.1", + "jest-environment-jsdom": "^27.5.1", "nock": "^13.3.1", + "sinon": "^9.2.4", "ts-jest": "^27.1.4", "typedoc": "^0.24.8", "typedoc-plugin-missing-exports": "^2.0.0", diff --git a/packages/controller-utils/src/create-service-policy.test.ts b/packages/controller-utils/src/create-service-policy.test.ts new file mode 100644 index 00000000000..800023b5e25 --- /dev/null +++ b/packages/controller-utils/src/create-service-policy.test.ts @@ -0,0 +1,1145 @@ +import { useFakeTimers } from 'sinon'; +import type { SinonFakeTimers } from 'sinon'; + +import { + createServicePolicy, + DEFAULT_CIRCUIT_BREAK_DURATION, + DEFAULT_DEGRADED_THRESHOLD, + DEFAULT_MAX_CONSECUTIVE_FAILURES, + DEFAULT_MAX_RETRIES, +} from './create-service-policy'; + +describe('createServicePolicy', () => { + let clock: SinonFakeTimers; + + beforeEach(() => { + clock = useFakeTimers(); + }); + + afterEach(() => { + clock.restore(); + }); + + describe('wrapping a service that succeeds on the first try', () => { + it('returns a policy that returns what the service returns', async () => { + const mockService = jest.fn(() => ({ some: 'data' })); + const policy = createServicePolicy(); + + const returnValue = await policy.execute(mockService); + + expect(returnValue).toStrictEqual({ some: 'data' }); + }); + + it('only calls the service once before returning', async () => { + const mockService = jest.fn(() => ({ some: 'data' })); + const policy = createServicePolicy(); + + await policy.execute(mockService); + + expect(mockService).toHaveBeenCalledTimes(1); + }); + + it('does not call the onBreak callback, since the circuit never opens', async () => { + const mockService = jest.fn(() => ({ some: 'data' })); + const onBreak = jest.fn(); + const policy = createServicePolicy({ onBreak }); + + await policy.execute(mockService); + + expect(onBreak).not.toHaveBeenCalled(); + }); + + describe(`using the default degraded threshold (${DEFAULT_DEGRADED_THRESHOLD})`, () => { + it('does not call the onDegraded callback if the service execution time is below the threshold', async () => { + const mockService = jest.fn(() => ({ some: 'data' })); + const onDegraded = jest.fn(); + const policy = createServicePolicy({ onDegraded }); + + await policy.execute(mockService); + + expect(onDegraded).not.toHaveBeenCalled(); + }); + + it('calls the onDegraded callback once if the service execution time is beyond the threshold', async () => { + const delay = DEFAULT_DEGRADED_THRESHOLD + 1; + const mockService = jest.fn(() => { + return new Promise((resolve) => { + setTimeout(() => resolve({ some: 'data' }), delay); + }); + }); + const onDegraded = jest.fn(); + const policy = createServicePolicy({ onDegraded }); + + const promise = policy.execute(mockService); + clock.tick(delay); + await promise; + + expect(onDegraded).toHaveBeenCalledTimes(1); + }); + }); + + describe('using a custom degraded threshold', () => { + it('does not call the onDegraded callback if the service execution time below the threshold', async () => { + const degradedThreshold = 2000; + const mockService = jest.fn(() => ({ some: 'data' })); + const onDegraded = jest.fn(); + const policy = createServicePolicy({ degradedThreshold, onDegraded }); + + await policy.execute(mockService); + + expect(onDegraded).not.toHaveBeenCalled(); + }); + + it('calls the onDegraded callback once if the service execution time beyond the threshold', async () => { + const degradedThreshold = 2000; + const delay = degradedThreshold + 1; + const mockService = jest.fn(() => { + return new Promise((resolve) => { + setTimeout(() => resolve({ some: 'data' }), delay); + }); + }); + const onDegraded = jest.fn(); + const policy = createServicePolicy({ degradedThreshold, onDegraded }); + + const promise = policy.execute(mockService); + clock.tick(delay); + await promise; + + expect(onDegraded).toHaveBeenCalledTimes(1); + }); + }); + }); + + describe('wrapping a service that always fails', () => { + it(`calls the service a total of ${ + 1 + DEFAULT_MAX_RETRIES + } times, delaying each retry using a backoff formula`, async () => { + const error = new Error('failure'); + const mockService = jest.fn(() => { + throw error; + }); + const policy = createServicePolicy(); + + const promise = policy.execute(mockService); + // It's safe not to await this promise; adding it to the promise queue is + // enough to prevent this test from running indefinitely. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + clock.runAllAsync(); + await ignoreRejection(promise); + + expect(mockService).toHaveBeenCalledTimes(1 + DEFAULT_MAX_RETRIES); + }); + + it('calls the onRetry callback once per retry', async () => { + const error = new Error('failure'); + const mockService = jest.fn(() => { + throw error; + }); + const onRetry = jest.fn(); + const policy = createServicePolicy({ onRetry }); + + const promise = policy.execute(mockService); + // It's safe not to await this promise; adding it to the promise queue is + // enough to prevent this test from running indefinitely. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + clock.runAllAsync(); + await ignoreRejection(promise); + + expect(onRetry).toHaveBeenCalledTimes(DEFAULT_MAX_RETRIES); + }); + + describe(`using the default max number of consecutive failures (${DEFAULT_MAX_CONSECUTIVE_FAILURES})`, () => { + it('throws what the service throws', async () => { + const error = new Error('failure'); + const mockService = jest.fn(() => { + throw error; + }); + const policy = createServicePolicy(); + + const promise = policy.execute(mockService); + // It's safe not to await this promise; adding it to the promise queue + // is enough to prevent this test from running indefinitely. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + clock.runAllAsync(); + + await expect(promise).rejects.toThrow(error); + }); + + it('does not call the onBreak callback, since the max number of consecutive failures is never reached', async () => { + const error = new Error('failure'); + const mockService = jest.fn(() => { + throw error; + }); + const onBreak = jest.fn(); + const policy = createServicePolicy({ onBreak }); + + const promise = policy.execute(mockService); + // It's safe not to await this promise; adding it to the promise queue + // is enough to prevent this test from running indefinitely. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + clock.runAllAsync(); + await ignoreRejection(promise); + + expect(onBreak).not.toHaveBeenCalled(); + }); + + it('calls the onDegraded callback once, since the circuit is still closed', async () => { + const error = new Error('failure'); + const mockService = jest.fn(() => { + throw error; + }); + const onDegraded = jest.fn(); + const policy = createServicePolicy({ onDegraded }); + + const promise = policy.execute(mockService); + // It's safe not to await this promise; adding it to the promise queue + // is enough to prevent this test from running indefinitely. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + clock.runAllAsync(); + await ignoreRejection(promise); + + expect(onDegraded).toHaveBeenCalledTimes(1); + }); + }); + + describe('using a custom max number of consecutive failures', () => { + describe('if the initial run + retries is less than the max number of consecutive failures', () => { + it('throws what the service throws', async () => { + const maxConsecutiveFailures = DEFAULT_MAX_RETRIES + 2; + const error = new Error('failure'); + const mockService = jest.fn(() => { + throw error; + }); + const onBreak = jest.fn(); + const policy = createServicePolicy({ + maxConsecutiveFailures, + onBreak, + }); + + const promise = policy.execute(mockService); + // It's safe not to await this promise; adding it to the promise queue + // is enough to prevent this test from running indefinitely. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + clock.runAllAsync(); + + await expect(promise).rejects.toThrow(error); + }); + + it('does not call the onBreak callback', async () => { + const maxConsecutiveFailures = DEFAULT_MAX_RETRIES + 2; + const error = new Error('failure'); + const mockService = jest.fn(() => { + throw error; + }); + const onBreak = jest.fn(); + const policy = createServicePolicy({ + maxConsecutiveFailures, + onBreak, + }); + + const promise = policy.execute(mockService); + // It's safe not to await this promise; adding it to the promise queue + // is enough to prevent this test from running indefinitely. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + clock.runAllAsync(); + await ignoreRejection(promise); + + expect(onBreak).not.toHaveBeenCalled(); + }); + + it('calls the onDegraded callback once', async () => { + const maxConsecutiveFailures = DEFAULT_MAX_RETRIES + 2; + const error = new Error('failure'); + const mockService = jest.fn(() => { + throw error; + }); + const onDegraded = jest.fn(); + const policy = createServicePolicy({ + maxConsecutiveFailures, + onDegraded, + }); + + const promise = policy.execute(mockService); + // It's safe not to await this promise; adding it to the promise queue + // is enough to prevent this test from running indefinitely. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + clock.runAllAsync(); + await ignoreRejection(promise); + + expect(onDegraded).toHaveBeenCalledTimes(1); + }); + }); + + describe('if the initial run + retries is equal to the max number of consecutive failures', () => { + it('throws what the service throws', async () => { + const maxConsecutiveFailures = DEFAULT_MAX_RETRIES + 1; + const error = new Error('failure'); + const mockService = jest.fn(() => { + throw error; + }); + const policy = createServicePolicy({ + maxConsecutiveFailures, + }); + + const promise = policy.execute(mockService); + // It's safe not to await this promise; adding it to the promise queue + // is enough to prevent this test from running indefinitely. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + clock.runAllAsync(); + + await expect(promise).rejects.toThrow(error); + }); + + it('calls the onBreak callback once with the error', async () => { + const maxConsecutiveFailures = DEFAULT_MAX_RETRIES + 1; + const error = new Error('failure'); + const mockService = jest.fn(() => { + throw error; + }); + const onBreak = jest.fn(); + const policy = createServicePolicy({ + maxConsecutiveFailures, + onBreak, + }); + + const promise = policy.execute(mockService); + // It's safe not to await this promise; adding it to the promise queue + // is enough to prevent this test from running indefinitely. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + clock.runAllAsync(); + await ignoreRejection(promise); + + expect(onBreak).toHaveBeenCalledTimes(1); + expect(onBreak).toHaveBeenCalledWith({ error }); + }); + + it('never calls the onDegraded callback, since the circuit is open', async () => { + const maxConsecutiveFailures = DEFAULT_MAX_RETRIES + 1; + const error = new Error('failure'); + const mockService = jest.fn(() => { + throw error; + }); + const onDegraded = jest.fn(); + const policy = createServicePolicy({ + maxConsecutiveFailures, + onDegraded, + }); + + const promise = policy.execute(mockService); + // It's safe not to await this promise; adding it to the promise queue + // is enough to prevent this test from running indefinitely. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + clock.runAllAsync(); + await ignoreRejection(promise); + + expect(onDegraded).not.toHaveBeenCalled(); + }); + + it('throws a BrokenCircuitError instead of whatever error the service produces if the service is executed again', async () => { + const maxConsecutiveFailures = DEFAULT_MAX_RETRIES + 1; + const error = new Error('failure'); + const mockService = jest.fn(() => { + throw error; + }); + const policy = createServicePolicy({ + maxConsecutiveFailures, + }); + + const firstExecution = policy.execute(mockService); + // It's safe not to await this promise; adding it to the promise queue + // is enough to prevent this test from running indefinitely. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + clock.runAllAsync(); + await ignoreRejection(firstExecution); + + const secondExecution = policy.execute(mockService); + await expect(secondExecution).rejects.toThrow( + new Error( + 'Execution prevented because the circuit breaker is open', + ), + ); + }); + }); + + describe('if the initial run + retries is greater than the max number of consecutive failures', () => { + it('throws a BrokenCircuitError instead of whatever error the service produces', async () => { + const maxConsecutiveFailures = DEFAULT_MAX_RETRIES; + const error = new Error('failure'); + const mockService = jest.fn(() => { + throw error; + }); + const policy = createServicePolicy({ + maxConsecutiveFailures, + }); + + const promise = policy.execute(mockService); + // It's safe not to await this promise; adding it to the promise queue + // is enough to prevent this test from running indefinitely. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + clock.runAllAsync(); + + await expect(promise).rejects.toThrow( + new Error( + 'Execution prevented because the circuit breaker is open', + ), + ); + }); + + it('calls the onBreak callback once with the error', async () => { + const maxConsecutiveFailures = DEFAULT_MAX_RETRIES; + const error = new Error('failure'); + const mockService = jest.fn(() => { + throw error; + }); + const onBreak = jest.fn(); + const policy = createServicePolicy({ + maxConsecutiveFailures, + onBreak, + }); + + const promise = policy.execute(mockService); + // It's safe not to await this promise; adding it to the promise queue + // is enough to prevent this test from running indefinitely. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + clock.runAllAsync(); + await ignoreRejection(promise); + + expect(onBreak).toHaveBeenCalledTimes(1); + expect(onBreak).toHaveBeenCalledWith({ error }); + }); + + it('never calls the onDegraded callback, since the circuit is open', async () => { + const maxConsecutiveFailures = DEFAULT_MAX_RETRIES; + const error = new Error('failure'); + const mockService = jest.fn(() => { + throw error; + }); + const onDegraded = jest.fn(); + const policy = createServicePolicy({ + maxConsecutiveFailures, + onDegraded, + }); + + const promise = policy.execute(mockService); + // It's safe not to await this promise; adding it to the promise queue + // is enough to prevent this test from running indefinitely. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + clock.runAllAsync(); + await ignoreRejection(promise); + + expect(onDegraded).not.toHaveBeenCalled(); + }); + }); + }); + }); + + describe('wrapping a service that fails continuously and then succeeds on the final try', () => { + it(`calls the service a total of ${ + 1 + DEFAULT_MAX_RETRIES + } times, delaying each retry using a backoff formula`, async () => { + let invocationCounter = 0; + const mockService = jest.fn(() => { + invocationCounter += 1; + if (invocationCounter === DEFAULT_MAX_RETRIES + 1) { + return { some: 'data' }; + } + throw new Error('failure'); + }); + const policy = createServicePolicy(); + + const promise = policy.execute(mockService); + // It's safe not to await this promise; adding it to the promise queue is + // enough to prevent this test from running indefinitely. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + clock.runAllAsync(); + await promise; + + expect(mockService).toHaveBeenCalledTimes(1 + DEFAULT_MAX_RETRIES); + }); + + it('calls the onRetry callback once per retry', async () => { + let invocationCounter = 0; + const mockService = jest.fn(() => { + invocationCounter += 1; + if (invocationCounter === DEFAULT_MAX_RETRIES + 1) { + return { some: 'data' }; + } + throw new Error('failure'); + }); + const onRetry = jest.fn(); + const policy = createServicePolicy({ onRetry }); + + const promise = policy.execute(mockService); + // It's safe not to await this promise; adding it to the promise queue is + // enough to prevent this test from running indefinitely. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + clock.runAllAsync(); + await promise; + + expect(onRetry).toHaveBeenCalledTimes(DEFAULT_MAX_RETRIES); + }); + + describe(`using the default max number of consecutive failures (${DEFAULT_MAX_CONSECUTIVE_FAILURES})`, () => { + it('returns what the service returns', async () => { + let invocationCounter = 0; + const mockService = () => { + invocationCounter += 1; + if (invocationCounter === DEFAULT_MAX_RETRIES + 1) { + return { some: 'data' }; + } + throw new Error('failure'); + }; + const onBreak = jest.fn(); + const policy = createServicePolicy({ onBreak }); + + const promise = policy.execute(mockService); + // It's safe not to await this promise; adding it to the promise queue + // is enough to prevent this test from running indefinitely. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + clock.runAllAsync(); + + expect(await promise).toStrictEqual({ some: 'data' }); + }); + + it('does not call the onBreak callback, since the max number of consecutive failures is never reached', async () => { + let invocationCounter = 0; + const mockService = () => { + invocationCounter += 1; + if (invocationCounter === DEFAULT_MAX_RETRIES + 1) { + return { some: 'data' }; + } + throw new Error('failure'); + }; + const onBreak = jest.fn(); + const policy = createServicePolicy({ onBreak }); + + const promise = policy.execute(mockService); + // It's safe not to await this promise; adding it to the promise queue + // is enough to prevent this test from running indefinitely. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + clock.runAllAsync(); + await promise; + + expect(onBreak).not.toHaveBeenCalled(); + }); + + describe(`using the default degraded threshold (${DEFAULT_DEGRADED_THRESHOLD})`, () => { + it('does not call the onDegraded callback if the service execution time is below the threshold', async () => { + let invocationCounter = 0; + const mockService = () => { + invocationCounter += 1; + if (invocationCounter === DEFAULT_MAX_RETRIES + 1) { + return { some: 'data' }; + } + throw new Error('failure'); + }; + const onDegraded = jest.fn(); + const policy = createServicePolicy({ onDegraded }); + + const promise = policy.execute(mockService); + // It's safe not to await this promise; adding it to the promise queue + // is enough to prevent this test from running indefinitely. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + clock.runAllAsync(); + await promise; + + expect(onDegraded).not.toHaveBeenCalled(); + }); + + it('calls the onDegraded callback once if the service execution time is beyond the threshold', async () => { + let invocationCounter = 0; + const delay = DEFAULT_DEGRADED_THRESHOLD + 1; + const mockService = () => { + invocationCounter += 1; + return new Promise((resolve, reject) => { + if (invocationCounter === DEFAULT_MAX_RETRIES + 1) { + setTimeout(() => resolve({ some: 'data' }), delay); + } else { + reject(new Error('failure')); + } + }); + }; + const onDegraded = jest.fn(); + const policy = createServicePolicy({ onDegraded }); + + const promise = policy.execute(mockService); + // It's safe not to await this promise; adding it to the promise queue + // is enough to prevent this test from running indefinitely. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + clock.runAllAsync(); + await promise; + + expect(onDegraded).toHaveBeenCalledTimes(1); + }); + }); + + describe('using a custom degraded threshold', () => { + it('does not call the onDegraded callback if the service execution time is below the threshold', async () => { + const degradedThreshold = 2000; + let invocationCounter = 0; + const mockService = () => { + invocationCounter += 1; + if (invocationCounter === DEFAULT_MAX_RETRIES + 1) { + return { some: 'data' }; + } + throw new Error('failure'); + }; + const onDegraded = jest.fn(); + const policy = createServicePolicy({ + onDegraded, + degradedThreshold, + }); + + const promise = policy.execute(mockService); + // It's safe not to await this promise; adding it to the promise queue + // is enough to prevent this test from running indefinitely. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + clock.runAllAsync(); + await promise; + + expect(onDegraded).not.toHaveBeenCalled(); + }); + + it('calls the onDegraded callback once if the service execution time is beyond the threshold', async () => { + const degradedThreshold = 2000; + let invocationCounter = 0; + const delay = degradedThreshold + 1; + const mockService = () => { + invocationCounter += 1; + return new Promise((resolve, reject) => { + if (invocationCounter === DEFAULT_MAX_RETRIES + 1) { + setTimeout(() => resolve({ some: 'data' }), delay); + } else { + reject(new Error('failure')); + } + }); + }; + const onDegraded = jest.fn(); + const policy = createServicePolicy({ + onDegraded, + degradedThreshold, + }); + + const promise = policy.execute(mockService); + // It's safe not to await this promise; adding it to the promise queue + // is enough to prevent this test from running indefinitely. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + clock.runAllAsync(); + await promise; + + expect(onDegraded).toHaveBeenCalledTimes(1); + }); + }); + }); + + describe('using a custom max number of consecutive failures', () => { + describe('if the initial run + retries is less than the max number of consecutive failures', () => { + it('returns what the service returns', async () => { + const maxConsecutiveFailures = DEFAULT_MAX_RETRIES + 2; + let invocationCounter = 0; + const mockService = () => { + invocationCounter += 1; + if (invocationCounter === DEFAULT_MAX_RETRIES + 1) { + return { some: 'data' }; + } + throw new Error('failure'); + }; + const onBreak = jest.fn(); + const policy = createServicePolicy({ + maxConsecutiveFailures, + onBreak, + }); + + const promise = policy.execute(mockService); + // It's safe not to await this promise; adding it to the promise queue + // is enough to prevent this test from running indefinitely. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + clock.runAllAsync(); + + expect(await promise).toStrictEqual({ some: 'data' }); + }); + + it('does not call the onBreak callback', async () => { + const maxConsecutiveFailures = DEFAULT_MAX_RETRIES + 2; + let invocationCounter = 0; + const mockService = () => { + invocationCounter += 1; + if (invocationCounter === DEFAULT_MAX_RETRIES + 1) { + return { some: 'data' }; + } + throw new Error('failure'); + }; + const onBreak = jest.fn(); + const policy = createServicePolicy({ + maxConsecutiveFailures, + onBreak, + }); + + const promise = policy.execute(mockService); + // It's safe not to await this promise; adding it to the promise queue + // is enough to prevent this test from running indefinitely. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + clock.runAllAsync(); + await promise; + + expect(onBreak).not.toHaveBeenCalled(); + }); + + describe(`using the default degraded threshold (${DEFAULT_DEGRADED_THRESHOLD})`, () => { + it('does not call the onDegraded callback if the service execution time is below the threshold', async () => { + const maxConsecutiveFailures = DEFAULT_MAX_RETRIES + 2; + let invocationCounter = 0; + const mockService = () => { + invocationCounter += 1; + if (invocationCounter === DEFAULT_MAX_RETRIES + 1) { + return { some: 'data' }; + } + throw new Error('failure'); + }; + const onDegraded = jest.fn(); + const policy = createServicePolicy({ + maxConsecutiveFailures, + onDegraded, + }); + + const promise = policy.execute(mockService); + // It's safe not to await this promise; adding it to the promise + // queue is enough to prevent this test from running indefinitely. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + clock.runAllAsync(); + await promise; + + expect(onDegraded).not.toHaveBeenCalled(); + }); + + it('calls the onDegraded callback once if the service execution time is beyond the threshold', async () => { + const maxConsecutiveFailures = DEFAULT_MAX_RETRIES + 2; + const delay = DEFAULT_DEGRADED_THRESHOLD + 1; + let invocationCounter = 0; + const mockService = () => { + invocationCounter += 1; + return new Promise((resolve, reject) => { + if (invocationCounter === DEFAULT_MAX_RETRIES + 1) { + setTimeout(() => resolve({ some: 'data' }), delay); + } else { + reject(new Error('failure')); + } + }); + }; + const onDegraded = jest.fn(); + const policy = createServicePolicy({ + maxConsecutiveFailures, + onDegraded, + }); + + const promise = policy.execute(mockService); + // It's safe not to await this promise; adding it to the promise + // queue is enough to prevent this test from running indefinitely. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + clock.runAllAsync(); + await promise; + + expect(onDegraded).toHaveBeenCalledTimes(1); + }); + }); + + describe('using a custom degraded threshold', () => { + it('does not call the onDegraded callback if the service execution time is below the threshold', async () => { + const degradedThreshold = 2000; + const maxConsecutiveFailures = DEFAULT_MAX_RETRIES + 2; + let invocationCounter = 0; + const mockService = () => { + invocationCounter += 1; + if (invocationCounter === DEFAULT_MAX_RETRIES + 1) { + return { some: 'data' }; + } + throw new Error('failure'); + }; + const onDegraded = jest.fn(); + const policy = createServicePolicy({ + maxConsecutiveFailures, + onDegraded, + degradedThreshold, + }); + + const promise = policy.execute(mockService); + // It's safe not to await this promise; adding it to the promise + // queue is enough to prevent this test from running indefinitely. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + clock.runAllAsync(); + await promise; + + expect(onDegraded).not.toHaveBeenCalled(); + }); + + it('calls the onDegraded callback once if the service execution time is beyond the threshold', async () => { + const degradedThreshold = 2000; + const maxConsecutiveFailures = DEFAULT_MAX_RETRIES + 2; + const delay = degradedThreshold + 1; + let invocationCounter = 0; + const mockService = () => { + invocationCounter += 1; + return new Promise((resolve, reject) => { + if (invocationCounter === DEFAULT_MAX_RETRIES + 1) { + setTimeout(() => resolve({ some: 'data' }), delay); + } else { + reject(new Error('failure')); + } + }); + }; + const onDegraded = jest.fn(); + const policy = createServicePolicy({ + maxConsecutiveFailures, + onDegraded, + degradedThreshold, + }); + + const promise = policy.execute(mockService); + // It's safe not to await this promise; adding it to the promise + // queue is enough to prevent this test from running indefinitely. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + clock.runAllAsync(); + await promise; + + expect(onDegraded).toHaveBeenCalledTimes(1); + }); + }); + }); + + describe('if the initial run + retries is equal to the max number of consecutive failures', () => { + it('returns what the service returns', async () => { + const maxConsecutiveFailures = DEFAULT_MAX_RETRIES + 1; + let invocationCounter = 0; + const mockService = () => { + invocationCounter += 1; + if (invocationCounter === DEFAULT_MAX_RETRIES + 1) { + return { some: 'data' }; + } + throw new Error('failure'); + }; + const onBreak = jest.fn(); + const policy = createServicePolicy({ + maxConsecutiveFailures, + onBreak, + }); + + const promise = policy.execute(mockService); + // It's safe not to await this promise; adding it to the promise queue + // is enough to prevent this test from running indefinitely. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + clock.runAllAsync(); + + expect(await promise).toStrictEqual({ some: 'data' }); + }); + + it('does not call the onBreak callback', async () => { + const maxConsecutiveFailures = DEFAULT_MAX_RETRIES + 1; + let invocationCounter = 0; + const error = new Error('failure'); + const mockService = () => { + invocationCounter += 1; + if (invocationCounter === DEFAULT_MAX_RETRIES + 1) { + return { some: 'data' }; + } + throw error; + }; + const onBreak = jest.fn(); + const policy = createServicePolicy({ + maxConsecutiveFailures, + onBreak, + }); + + const promise = policy.execute(mockService); + // It's safe not to await this promise; adding it to the promise queue + // is enough to prevent this test from running indefinitely. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + clock.runAllAsync(); + await promise; + + expect(onBreak).not.toHaveBeenCalled(); + }); + + describe(`using the default degraded threshold (${DEFAULT_DEGRADED_THRESHOLD})`, () => { + it('does not call the onDegraded callback if the service execution time is below the threshold', async () => { + const maxConsecutiveFailures = DEFAULT_MAX_RETRIES + 1; + let invocationCounter = 0; + const error = new Error('failure'); + const mockService = () => { + invocationCounter += 1; + if (invocationCounter === DEFAULT_MAX_RETRIES + 1) { + return { some: 'data' }; + } + throw error; + }; + const onDegraded = jest.fn(); + const policy = createServicePolicy({ + maxConsecutiveFailures, + onDegraded, + }); + + const promise = policy.execute(mockService); + // It's safe not to await this promise; adding it to the promise + // queue is enough to prevent this test from running indefinitely. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + clock.runAllAsync(); + await promise; + + expect(onDegraded).not.toHaveBeenCalled(); + }); + + it('calls the onDegraded callback once if the service execution time is beyond the threshold', async () => { + const maxConsecutiveFailures = DEFAULT_MAX_RETRIES + 1; + const delay = DEFAULT_DEGRADED_THRESHOLD + 1; + let invocationCounter = 0; + const mockService = () => { + invocationCounter += 1; + return new Promise((resolve, reject) => { + if (invocationCounter === DEFAULT_MAX_RETRIES + 1) { + setTimeout(() => resolve({ some: 'data' }), delay); + } else { + reject(new Error('failure')); + } + }); + }; + const onDegraded = jest.fn(); + const policy = createServicePolicy({ + maxConsecutiveFailures, + onDegraded, + }); + + const promise = policy.execute(mockService); + // It's safe not to await this promise; adding it to the promise + // queue is enough to prevent this test from running indefinitely. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + clock.runAllAsync(); + await promise; + + expect(onDegraded).toHaveBeenCalledTimes(1); + }); + }); + + describe('using a custom degraded threshold', () => { + it('does not call the onDegraded callback if the service execution time is below the threshold', async () => { + const degradedThreshold = 2000; + const maxConsecutiveFailures = DEFAULT_MAX_RETRIES + 1; + let invocationCounter = 0; + const error = new Error('failure'); + const mockService = () => { + invocationCounter += 1; + if (invocationCounter === DEFAULT_MAX_RETRIES + 1) { + return { some: 'data' }; + } + throw error; + }; + const onDegraded = jest.fn(); + const policy = createServicePolicy({ + maxConsecutiveFailures, + onDegraded, + degradedThreshold, + }); + + const promise = policy.execute(mockService); + // It's safe not to await this promise; adding it to the promise + // queue is enough to prevent this test from running indefinitely. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + clock.runAllAsync(); + await promise; + + expect(onDegraded).not.toHaveBeenCalled(); + }); + + it('calls the onDegraded callback once if the service execution time is beyond the threshold', async () => { + const degradedThreshold = 2000; + const maxConsecutiveFailures = DEFAULT_MAX_RETRIES + 1; + const delay = degradedThreshold + 1; + let invocationCounter = 0; + const mockService = () => { + invocationCounter += 1; + return new Promise((resolve, reject) => { + if (invocationCounter === DEFAULT_MAX_RETRIES + 1) { + setTimeout(() => resolve({ some: 'data' }), delay); + } else { + reject(new Error('failure')); + } + }); + }; + const onDegraded = jest.fn(); + const policy = createServicePolicy({ + maxConsecutiveFailures, + onDegraded, + degradedThreshold, + }); + + const promise = policy.execute(mockService); + // It's safe not to await this promise; adding it to the promise + // queue is enough to prevent this test from running indefinitely. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + clock.runAllAsync(); + await promise; + + expect(onDegraded).toHaveBeenCalledTimes(1); + }); + }); + }); + + describe('if the initial run + retries is greater than the max number of consecutive failures', () => { + it('throws a BrokenCircuitError before the service can succeed', async () => { + const maxConsecutiveFailures = DEFAULT_MAX_RETRIES; + let invocationCounter = 0; + const error = new Error('failure'); + const mockService = () => { + invocationCounter += 1; + if (invocationCounter === DEFAULT_MAX_RETRIES + 1) { + return { some: 'data' }; + } + throw error; + }; + const onBreak = jest.fn(); + const policy = createServicePolicy({ + maxConsecutiveFailures, + onBreak, + }); + + const promise = policy.execute(mockService); + // It's safe not to await this promise; adding it to the promise queue + // is enough to prevent this test from running indefinitely. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + clock.runAllAsync(); + await expect(promise).rejects.toThrow( + new Error( + 'Execution prevented because the circuit breaker is open', + ), + ); + }); + + it('calls the onBreak callback once with the error', async () => { + const maxConsecutiveFailures = DEFAULT_MAX_RETRIES; + let invocationCounter = 0; + const error = new Error('failure'); + const mockService = () => { + invocationCounter += 1; + if (invocationCounter === DEFAULT_MAX_RETRIES + 1) { + return { some: 'data' }; + } + throw error; + }; + const onBreak = jest.fn(); + const policy = createServicePolicy({ + maxConsecutiveFailures, + onBreak, + }); + + const promise = policy.execute(mockService); + // It's safe not to await this promise; adding it to the promise queue + // is enough to prevent this test from running indefinitely. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + clock.runAllAsync(); + await ignoreRejection(promise); + + expect(onBreak).toHaveBeenCalledTimes(1); + expect(onBreak).toHaveBeenCalledWith({ error }); + }); + + it('does not call the onDegraded callback', async () => { + const maxConsecutiveFailures = DEFAULT_MAX_RETRIES; + let invocationCounter = 0; + const error = new Error('failure'); + const mockService = () => { + invocationCounter += 1; + if (invocationCounter === DEFAULT_MAX_RETRIES + 1) { + return { some: 'data' }; + } + throw error; + }; + const onDegraded = jest.fn(); + const policy = createServicePolicy({ + maxConsecutiveFailures, + onDegraded, + }); + + const promise = policy.execute(mockService); + // It's safe not to await this promise; adding it to the promise queue + // is enough to prevent this test from running indefinitely. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + clock.runAllAsync(); + await ignoreRejection(promise); + + expect(onDegraded).not.toHaveBeenCalled(); + }); + + describe(`using the default circuit break duration (${DEFAULT_CIRCUIT_BREAK_DURATION})`, () => { + it('returns what the service returns if it is successfully called again after the circuit break duration has elapsed', async () => { + const maxConsecutiveFailures = DEFAULT_MAX_RETRIES; + let invocationCounter = 0; + const error = new Error('failure'); + const mockService = () => { + invocationCounter += 1; + if (invocationCounter === DEFAULT_MAX_RETRIES + 1) { + return { some: 'data' }; + } + throw error; + }; + const policy = createServicePolicy({ + maxConsecutiveFailures, + }); + + const firstExecution = policy.execute(mockService); + // It's safe not to await this promise; adding it to the promise + // queue is enough to prevent this test from running indefinitely. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + clock.runAllAsync(); + await ignoreRejection(firstExecution); + clock.tick(DEFAULT_CIRCUIT_BREAK_DURATION); + const result = await policy.execute(mockService); + + expect(result).toStrictEqual({ some: 'data' }); + }); + }); + + describe('using a custom circuit break duration', () => { + it('returns what the service returns if it is successfully called again after the circuit break duration has elapsed', async () => { + // This has to be high enough to exceed the exponential backoff + const circuitBreakDuration = 5_000; + const maxConsecutiveFailures = DEFAULT_MAX_RETRIES; + let invocationCounter = 0; + const error = new Error('failure'); + const mockService = () => { + invocationCounter += 1; + if (invocationCounter === DEFAULT_MAX_RETRIES + 1) { + return { some: 'data' }; + } + throw error; + }; + const policy = createServicePolicy({ + maxConsecutiveFailures, + circuitBreakDuration, + }); + + const firstExecution = policy.execute(mockService); + // It's safe not to await this promise; adding it to the promise + // queue is enough to prevent this test from running indefinitely. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + clock.runAllAsync(); + await ignoreRejection(firstExecution); + clock.tick(circuitBreakDuration); + const result = await policy.execute(mockService); + + expect(result).toStrictEqual({ some: 'data' }); + }); + }); + }); + }); + }); +}); + +/** + * Some tests involve a rejected promise that is not necessarily the focus of + * the test. In these cases we don't want to ignore the error in case the + * promise _isn't_ rejected, but we don't want to highlight the assertion, + * either. + * + * @param promise - A promise that rejects. + */ +async function ignoreRejection(promise: Promise) { + await expect(promise).rejects.toThrow(expect.any(Error)); +} diff --git a/packages/controller-utils/src/create-service-policy.ts b/packages/controller-utils/src/create-service-policy.ts new file mode 100644 index 00000000000..c985dba9e2d --- /dev/null +++ b/packages/controller-utils/src/create-service-policy.ts @@ -0,0 +1,177 @@ +import { + circuitBreaker, + ConsecutiveBreaker, + ExponentialBackoff, + handleAll, + retry, + wrap, + CircuitState, +} from 'cockatiel'; +import type { IPolicy } from 'cockatiel'; + +export type { IPolicy as IServicePolicy }; + +/** + * The maximum number of times that a failing service should be re-run before + * giving up. + */ +export const DEFAULT_MAX_RETRIES = 3; + +/** + * The maximum number of times that the service is allowed to fail before + * pausing further retries. + */ +export const DEFAULT_MAX_CONSECUTIVE_FAILURES = (1 + DEFAULT_MAX_RETRIES) * 3; + +/** + * The default length of time (in milliseconds) to temporarily pause retries of + * the service after enough consecutive failures. + */ +export const DEFAULT_CIRCUIT_BREAK_DURATION = 30 * 60 * 1000; + +/** + * The default length of time (in milliseconds) that governs when the service is + * regarded as degraded (affecting when `onDegraded` is called). + */ +export const DEFAULT_DEGRADED_THRESHOLD = 5_000; + +/** + * Constructs an object exposing an `execute` method which, given a function — + * hereafter called the "service" — will retry that service with ever increasing + * delays until it succeeds. If the policy detects too many consecutive + * failures, it will block further retries until a designated time period has + * passed; this particular behavior is primarily designed for services that wrap + * API calls so as not to make needless HTTP requests when the API is down and + * to be able to recover when the API comes back up. In addition, hooks allow + * for responding to certain events, one of which can be used to detect when an + * HTTP request is performing slowly. + * + * Internally, this function makes use of the retry and circuit breaker policies + * from the [Cockatiel](https://www.npmjs.com/package/cockatiel) library; see + * there for more. + * + * @param options - The options to this function. + * @param options.maxConsecutiveFailures - The maximum number of times that the + * service is allowed to fail before pausing further retries. Defaults to 12. + * @param options.circuitBreakDuration - The length of time (in milliseconds) to + * pause retries of the action after the number of failures reaches + * `maxConsecutiveFailures`. + * @param options.degradedThreshold - The length of time (in milliseconds) that + * governs when the service is regarded as degraded (affecting when `onDegraded` + * is called). Defaults to 5 seconds. + * @param options.onBreak - A function which is called when the service fails + * too many times in a row (specifically, more than `maxConsecutiveFailures`). + * @param options.onDegraded - A function which is called when the service + * succeeds before `maxConsecutiveFailures` is reached, but takes more time than + * the `degradedThreshold` to run. + * @param options.onRetry - A function which will be called the moment the + * policy kicks off a timer to re-run the function passed to the policy. This is + * primarily useful in tests where we are mocking timers. + * @returns The service policy. + * @example + * This function is designed to be used in the context of a service class like + * this: + * ``` ts + * class Service { + * constructor() { + * this.#policy = createServicePolicy({ + * maxConsecutiveFailures: 3, + * circuitBreakDuration: 5000, + * degradedThreshold: 2000, + * onBreak: () => { + * console.log('Circuit broke'); + * }, + * onDegraded: () => { + * console.log('Service is degraded'); + * }, + * }); + * } + * + * async fetch() { + * return await this.#policy.execute(async () => { + * const response = await fetch('https://some/url'); + * return await response.json(); + * }); + * } + * } + * ``` + */ +export function createServicePolicy({ + maxConsecutiveFailures = DEFAULT_MAX_CONSECUTIVE_FAILURES, + circuitBreakDuration = DEFAULT_CIRCUIT_BREAK_DURATION, + degradedThreshold = DEFAULT_DEGRADED_THRESHOLD, + onBreak = () => { + // do nothing + }, + onDegraded = () => { + // do nothing + }, + onRetry = () => { + // do nothing + }, +}: { + maxConsecutiveFailures?: number; + circuitBreakDuration?: number; + degradedThreshold?: number; + onBreak?: () => void; + onDegraded?: () => void; + onRetry?: () => void; +} = {}): IPolicy { + const retryPolicy = retry(handleAll, { + // Note that although the option here is called "max attempts", it's really + // maximum number of *retries* (attempts past the initial attempt). + maxAttempts: DEFAULT_MAX_RETRIES, + // Retries of the service will be executed following ever increasing delays, + // determined by a backoff formula. + backoff: new ExponentialBackoff(), + }); + + const circuitBreakerPolicy = circuitBreaker(handleAll, { + // While the circuit is open, any additional invocations of the service + // passed to the policy (either via automatic retries or by manually + // executing the policy again) will result in a BrokenCircuitError. This + // will remain the case until `circuitBreakDuration` passes, after which the + // service will be allowed to run again. If the service succeeds, the + // circuit will close, otherwise it will remain open. + halfOpenAfter: circuitBreakDuration, + breaker: new ConsecutiveBreaker(maxConsecutiveFailures), + }); + + // The `onBreak` callback will be called if the service consistently throws + // for as many times as exceeds the maximum consecutive number of failures. + // Combined with the retry policy, this can happen if: + // - `maxConsecutiveFailures` < the default max retries (3) and the policy is + // executed once + // - `maxConsecutiveFailures` >= the default max retries (3) but the policy is + // executed multiple times, enough for the total number of retries to exceed + // `maxConsecutiveFailures` + circuitBreakerPolicy.onBreak(onBreak); + + // The `onRetryPolicy` callback will be called each time the service is + // invoked (including retries). + retryPolicy.onRetry(onRetry); + + retryPolicy.onGiveUp(() => { + if (circuitBreakerPolicy.state === CircuitState.Closed) { + // The `onDegraded` callback will be called if the number of retries is + // exceeded and the maximum number of consecutive failures has not been + // reached yet (whether the policy is called once or multiple times). + onDegraded(); + } + }); + retryPolicy.onSuccess(({ duration }) => { + if ( + circuitBreakerPolicy.state === CircuitState.Closed && + duration > degradedThreshold + ) { + // The `onDegraded` callback will also be called if the service does not + // throw, but the time it takes for the service to run exceeds the + // `degradedThreshold`. + onDegraded(); + } + }); + + // The retry policy really retries the circuit breaker policy, which invokes + // the service. + return wrap(retryPolicy, circuitBreakerPolicy); +} diff --git a/packages/controller-utils/src/index.test.ts b/packages/controller-utils/src/index.test.ts new file mode 100644 index 00000000000..61ef841826f --- /dev/null +++ b/packages/controller-utils/src/index.test.ts @@ -0,0 +1,75 @@ +import * as allExports from '.'; + +describe('@metamask/controller-utils', () => { + it('has expected JavaScript exports', () => { + expect(Object.keys(allExports)).toMatchInlineSnapshot(` + Array [ + "createServicePolicy", + "BNToHex", + "convertHexToDecimal", + "fetchWithErrorHandling", + "fractionBN", + "fromHex", + "getBuyURL", + "gweiDecToWEIBN", + "handleFetch", + "hexToBN", + "hexToText", + "isNonEmptyArray", + "isPlainObject", + "isSafeChainId", + "isSafeDynamicKey", + "isSmartContractCode", + "isValidJson", + "isValidHexAddress", + "normalizeEnsName", + "query", + "safelyExecute", + "safelyExecuteWithTimeout", + "successfulFetch", + "timeoutFetch", + "toChecksumHexAddress", + "toHex", + "weiHexToGweiDec", + "isEqualCaseInsensitive", + "RPC", + "FALL_BACK_VS_CURRENCY", + "IPFS_DEFAULT_GATEWAY_URL", + "GANACHE_CHAIN_ID", + "MAX_SAFE_CHAIN_ID", + "ERC721", + "ERC1155", + "ERC20", + "ERC721_INTERFACE_ID", + "ERC721_METADATA_INTERFACE_ID", + "ERC721_ENUMERABLE_INTERFACE_ID", + "ERC1155_INTERFACE_ID", + "ERC1155_METADATA_URI_INTERFACE_ID", + "ERC1155_TOKEN_RECEIVER_INTERFACE_ID", + "GWEI", + "ASSET_TYPES", + "TESTNET_TICKER_SYMBOLS", + "BUILT_IN_NETWORKS", + "OPENSEA_PROXY_URL", + "NFT_API_BASE_URL", + "NFT_API_VERSION", + "NFT_API_TIMEOUT", + "ORIGIN_METAMASK", + "ApprovalType", + "CHAIN_ID_TO_ETHERS_NETWORK_NAME_MAP", + "InfuraNetworkType", + "NetworkType", + "isNetworkType", + "isInfuraNetworkType", + "BuiltInNetworkName", + "ChainId", + "NetworksTicker", + "BlockExplorerUrl", + "NetworkNickname", + "parseDomainParts", + "isValidSIWEOrigin", + "detectSIWE", + ] + `); + }); +}); diff --git a/packages/controller-utils/src/index.ts b/packages/controller-utils/src/index.ts index 3d35d62c0a0..b3bd8821e12 100644 --- a/packages/controller-utils/src/index.ts +++ b/packages/controller-utils/src/index.ts @@ -1,3 +1,4 @@ +export { createServicePolicy } from './create-service-policy'; export * from './constants'; export type { NonEmptyArray } from './util'; export { diff --git a/packages/controller-utils/src/util.ts b/packages/controller-utils/src/util.ts index 14b41caedba..7f3358b64c9 100644 --- a/packages/controller-utils/src/util.ts +++ b/packages/controller-utils/src/util.ts @@ -618,7 +618,7 @@ function logOrRethrowError(error: unknown, codesToCatch: number[] = []) { throw error; } } else { - // eslint-disable-next-line @typescript-eslint/no-throw-literal + // eslint-disable-next-line @typescript-eslint/only-throw-error throw error; } } diff --git a/packages/json-rpc-engine/src/JsonRpcEngine.ts b/packages/json-rpc-engine/src/JsonRpcEngine.ts index 62fb5204d0d..c589e13358d 100644 --- a/packages/json-rpc-engine/src/JsonRpcEngine.ts +++ b/packages/json-rpc-engine/src/JsonRpcEngine.ts @@ -489,7 +489,7 @@ export class JsonRpcEngine extends SafeEventEmitter { // Now we re-throw the middleware processing error, if any, to catch it // further up the call chain. if (error) { - // eslint-disable-next-line @typescript-eslint/no-throw-literal + // eslint-disable-next-line @typescript-eslint/only-throw-error throw error; } } diff --git a/packages/json-rpc-middleware-stream/src/index.test.ts b/packages/json-rpc-middleware-stream/src/index.test.ts index d770cad07b3..6395c629a73 100644 --- a/packages/json-rpc-middleware-stream/src/index.test.ts +++ b/packages/json-rpc-middleware-stream/src/index.test.ts @@ -8,7 +8,7 @@ import { createStreamMiddleware, createEngineStream } from '.'; const artificialDelay = async (time = 0) => new Promise((resolve) => setTimeout(resolve, time)); // TODO: Replace `any` with type -// eslint-disable-next-line @typescript-eslint/no-empty-function, @typescript-eslint/no-explicit-any +// eslint-disable-next-line @typescript-eslint/no-empty-function,@typescript-eslint/no-explicit-any const noop = function (_a: any) {}; const jsonrpc = '2.0' as const; diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 4cc90868a8f..bf5b853aeeb 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -1580,7 +1580,6 @@ export class KeyringController extends BaseController< * @deprecated Use `withKeyring` instead. */ async cancelQRSynchronization(): Promise { - // eslint-disable-next-line n/no-sync (await this.getOrAddQRKeyring()).cancelSync(); } diff --git a/packages/message-manager/CHANGELOG.md b/packages/message-manager/CHANGELOG.md index ac90e79a53d..f9ca2a88193 100644 --- a/packages/message-manager/CHANGELOG.md +++ b/packages/message-manager/CHANGELOG.md @@ -9,8 +9,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- **BREAKING:** Base class of `DecryptMessageManager` and `EncryptionPublicKeyManager`(`AbstractMessageManager`) now expects new options to initialise ([#5103](https://github.com/MetaMask/core/pull/5103)) - Bump `@metamask/base-controller` from `^7.0.0` to `^7.1.0` ([#5079](https://github.com/MetaMask/core/pull/5079)) +### Removed + +- **BREAKING:** Removed internal event emitter (`hub` property) from `AbstractMessageManager` ([#5103](https://github.com/MetaMask/core/pull/5103)) +- **BREAKING:** `unapprovedMessage` and `updateBadge` removed from internal events. These events are now emitted from messaging system ([#5103](https://github.com/MetaMask/core/pull/5103)) + - Controllers should now listen to `DerivedManagerName:X` event instead of using internal event emitter. + ## [11.0.3] ### Changed diff --git a/packages/message-manager/src/AbstractMessageManager.test.ts b/packages/message-manager/src/AbstractMessageManager.test.ts index 740ed38e2ca..fcf520854c0 100644 --- a/packages/message-manager/src/AbstractMessageManager.test.ts +++ b/packages/message-manager/src/AbstractMessageManager.test.ts @@ -1,3 +1,4 @@ +import type { RestrictedControllerMessenger } from '@metamask/base-controller'; import { ApprovalType } from '@metamask/controller-utils'; import type { @@ -20,10 +21,15 @@ type ConcreteMessageParamsMetamask = ConcreteMessageParams & { metamaskId?: string; }; +type ConcreteMessageManagerActions = never; +type ConcreteMessageManagerEvents = never; + class AbstractTestManager extends AbstractMessageManager< ConcreteMessage, ConcreteMessageParams, - ConcreteMessageParamsMetamask + ConcreteMessageParamsMetamask, + ConcreteMessageManagerActions, + ConcreteMessageManagerEvents > { addRequestToMessageParams( messageParams: MessageParams, @@ -56,6 +62,26 @@ class AbstractTestManager extends AbstractMessageManager< } } +const MOCK_MESSENGER = { + clearEventSubscriptions: jest.fn(), + publish: jest.fn(), + registerActionHandler: jest.fn(), + registerInitialEventPayload: jest.fn(), +} as unknown as RestrictedControllerMessenger< + 'AbstractMessageManager', + never, + never, + string, + string +>; + +const MOCK_INITIAL_OPTIONS = { + additionalFinishStatuses: undefined, + messenger: MOCK_MESSENGER, + name: 'AbstractMessageManager' as const, + securityProviderRequest: undefined, +}; + const messageId = '1'; const messageId2 = '2'; const from = '0x0123'; @@ -78,20 +104,15 @@ const mockMessageParams = { from, test: testData }; describe('AbstractTestManager', () => { it('should set default state', () => { - const controller = new AbstractTestManager(); + const controller = new AbstractTestManager(MOCK_INITIAL_OPTIONS); expect(controller.state).toStrictEqual({ unapprovedMessages: {}, unapprovedMessagesCount: 0, }); }); - it('should set default config', () => { - const controller = new AbstractTestManager(); - expect(controller.config).toStrictEqual({}); - }); - it('should add a valid message', async () => { - const controller = new AbstractTestManager(); + const controller = new AbstractTestManager(MOCK_INITIAL_OPTIONS); await controller.addMessage({ id: messageId, messageParams: { @@ -115,7 +136,7 @@ describe('AbstractTestManager', () => { }); it('should get all messages', async () => { - const controller = new AbstractTestManager(); + const controller = new AbstractTestManager(MOCK_INITIAL_OPTIONS); const message = { id: messageId, messageParams: { @@ -148,11 +169,10 @@ describe('AbstractTestManager', () => { const securityProviderRequestMock: SecurityProviderRequest = jest .fn() .mockResolvedValue(securityProviderResponseMock); - const controller = new AbstractTestManager( - undefined, - undefined, - securityProviderRequestMock, - ); + const controller = new AbstractTestManager({ + ...MOCK_INITIAL_OPTIONS, + securityProviderRequest: securityProviderRequestMock, + }); await controller.addMessage({ id: messageId, messageParams: { @@ -180,7 +200,7 @@ describe('AbstractTestManager', () => { }); it('should reject a message', async () => { - const controller = new AbstractTestManager(); + const controller = new AbstractTestManager(MOCK_INITIAL_OPTIONS); await controller.addMessage({ id: messageId, messageParams: { @@ -200,7 +220,7 @@ describe('AbstractTestManager', () => { }); it('should sign a message', async () => { - const controller = new AbstractTestManager(); + const controller = new AbstractTestManager(MOCK_INITIAL_OPTIONS); await controller.addMessage({ id: messageId, messageParams: { @@ -221,12 +241,10 @@ describe('AbstractTestManager', () => { }); it('sets message to one of the allowed statuses', async () => { - const controller = new AbstractTestManager( - undefined, - undefined, - undefined, - ['test-status'], - ); + const controller = new AbstractTestManager({ + ...MOCK_INITIAL_OPTIONS, + additionalFinishStatuses: ['test-status'], + }); await controller.addMessage({ id: messageId, messageParams: { @@ -246,12 +264,10 @@ describe('AbstractTestManager', () => { }); it('should set a status to inProgress', async () => { - const controller = new AbstractTestManager( - undefined, - undefined, - undefined, - ['test-status'], - ); + const controller = new AbstractTestManager({ + ...MOCK_INITIAL_OPTIONS, + additionalFinishStatuses: ['test-status'], + }); await controller.addMessage({ id: messageId, messageParams: { @@ -285,7 +301,7 @@ describe('AbstractTestManager', () => { time: 123, type: 'eth_signTypedData', }; - const controller = new AbstractTestManager(); + const controller = new AbstractTestManager(MOCK_INITIAL_OPTIONS); await controller.addMessage(firstMessage); await controller.addMessage(secondMessage); expect(controller.getUnapprovedMessagesCount()).toBe(2); @@ -296,7 +312,7 @@ describe('AbstractTestManager', () => { }); it('should approve message', async () => { - const controller = new AbstractTestManager(); + const controller = new AbstractTestManager(MOCK_INITIAL_OPTIONS); const firstMessage = { from: '0xfoO', test: testData }; await controller.addMessage({ id: messageId, @@ -319,7 +335,7 @@ describe('AbstractTestManager', () => { describe('addRequestToMessageParams', () => { it('adds original request id and origin to messageParams', () => { - const controller = new AbstractTestManager(); + const controller = new AbstractTestManager(MOCK_INITIAL_OPTIONS); const result = controller.addRequestToMessageParams( mockMessageParams, @@ -336,7 +352,7 @@ describe('AbstractTestManager', () => { describe('createUnapprovedMessage', () => { it('creates a Message object with an unapproved status', () => { - const controller = new AbstractTestManager(); + const controller = new AbstractTestManager(MOCK_INITIAL_OPTIONS); const result = controller.createUnapprovedMessage( mockMessageParams, @@ -361,7 +377,7 @@ describe('AbstractTestManager', () => { emit: jest.fn(), })); - const controller = new AbstractTestManager(); + const controller = new AbstractTestManager(MOCK_INITIAL_OPTIONS); await controller.addMessage({ id: messageId, messageParams: { ...mockMessageParams }, @@ -379,7 +395,7 @@ describe('AbstractTestManager', () => { }); it('throws an error if the message is not found', async () => { - const controller = new AbstractTestManager(); + const controller = new AbstractTestManager(MOCK_INITIAL_OPTIONS); expect(() => controller.setMessageStatus(messageId, 'newstatus')).toThrow( 'AbstractMessageManager: Message not found for id: 1.', @@ -393,7 +409,7 @@ describe('AbstractTestManager', () => { emit: jest.fn(), })); - const controller = new AbstractTestManager(); + const controller = new AbstractTestManager(MOCK_INITIAL_OPTIONS); await controller.addMessage({ id: messageId, messageParams: { ...mockMessageParams }, @@ -407,14 +423,13 @@ describe('AbstractTestManager', () => { controller.setMessageStatusAndResult(messageId, 'newRawSig', 'newstatus'); const messageAfter = controller.getMessage(messageId); - // expect(controller.hub.emit).toHaveBeenNthCalledWith(1, 'updateBadge'); expect(messageAfter?.status).toBe('newstatus'); }); }); describe('setMetadata', () => { it('should set the given message metadata', async () => { - const controller = new AbstractTestManager(); + const controller = new AbstractTestManager(MOCK_INITIAL_OPTIONS); await controller.addMessage({ id: messageId, messageParams: { ...mockMessageParams }, @@ -432,7 +447,7 @@ describe('AbstractTestManager', () => { }); it('should throw an error if message is not found', () => { - const controller = new AbstractTestManager(); + const controller = new AbstractTestManager(MOCK_INITIAL_OPTIONS); expect(() => controller.setMetadata(messageId, { foo: 'bar' })).toThrow( 'AbstractMessageManager: Message not found for id: 1.', @@ -442,7 +457,7 @@ describe('AbstractTestManager', () => { describe('waitForFinishStatus', () => { it('signs the message when status is "signed"', async () => { - const controller = new AbstractTestManager(); + const controller = new AbstractTestManager(MOCK_INITIAL_OPTIONS); const promise = controller.waitForFinishStatus( { from: fromMock, @@ -452,7 +467,7 @@ describe('AbstractTestManager', () => { ); setTimeout(() => { - controller.hub.emit(`${messageIdMock}:finished`, { + controller.internalEvents.emit(`${messageIdMock}:finished`, { status: 'signed', rawSig: rawSigMock, }); @@ -462,7 +477,7 @@ describe('AbstractTestManager', () => { }); it('rejects with an error when status is "rejected"', async () => { - const controller = new AbstractTestManager(); + const controller = new AbstractTestManager(MOCK_INITIAL_OPTIONS); const promise = controller.waitForFinishStatus( { from: fromMock, @@ -472,7 +487,7 @@ describe('AbstractTestManager', () => { ); setTimeout(() => { - controller.hub.emit(`${messageIdMock}:finished`, { + controller.internalEvents.emit(`${messageIdMock}:finished`, { status: 'rejected', }); }, 100); @@ -483,7 +498,7 @@ describe('AbstractTestManager', () => { }); it('rejects with an error when finishes with unknown status', async () => { - const controller = new AbstractTestManager(); + const controller = new AbstractTestManager(MOCK_INITIAL_OPTIONS); const promise = controller.waitForFinishStatus( { from: fromMock, @@ -493,7 +508,7 @@ describe('AbstractTestManager', () => { ); setTimeout(() => { - controller.hub.emit(`${messageIdMock}:finished`, { + controller.internalEvents.emit(`${messageIdMock}:finished`, { status: 'unknown', }); }, 100); @@ -508,7 +523,7 @@ describe('AbstractTestManager', () => { }); it('rejects with an error when finishes with errored status', async () => { - const controller = new AbstractTestManager(); + const controller = new AbstractTestManager(MOCK_INITIAL_OPTIONS); const promise = controller.waitForFinishStatus( { from: fromMock, @@ -518,7 +533,7 @@ describe('AbstractTestManager', () => { ); setTimeout(() => { - controller.hub.emit(`${messageIdMock}:finished`, { + controller.internalEvents.emit(`${messageIdMock}:finished`, { status: 'errored', error: 'error message', }); @@ -529,4 +544,26 @@ describe('AbstractTestManager', () => { ); }); }); + + describe('clearUnapprovedMessages', () => { + it('clears the unapproved messages', () => { + const controller = new AbstractTestManager({ + ...MOCK_INITIAL_OPTIONS, + state: { + unapprovedMessages: { + '1': { + id: '1', + messageParams: { from: '0x1', test: 1 }, + status: 'unapproved', + time: 10, + type: 'type', + }, + }, + unapprovedMessagesCount: 1, + }, + }); + controller.clearUnapprovedMessages(); + expect(controller.getUnapprovedMessagesCount()).toBe(0); + }); + }); }); diff --git a/packages/message-manager/src/AbstractMessageManager.ts b/packages/message-manager/src/AbstractMessageManager.ts index bdd8401f54c..7027acd6862 100644 --- a/packages/message-manager/src/AbstractMessageManager.ts +++ b/packages/message-manager/src/AbstractMessageManager.ts @@ -1,29 +1,41 @@ -import type { BaseConfig, BaseState } from '@metamask/base-controller'; -import { BaseControllerV1 } from '@metamask/base-controller'; +import { BaseController } from '@metamask/base-controller'; +import type { + ActionConstraint, + EventConstraint, + RestrictedControllerMessenger, +} from '@metamask/base-controller'; import type { ApprovalType } from '@metamask/controller-utils'; -import type { Hex, Json } from '@metamask/utils'; +import type { Json } from '@metamask/utils'; // This package purposefully relies on Node's EventEmitter module. -// eslint-disable-next-line import/no-nodejs-modules +// eslint-disable-next-line import-x/no-nodejs-modules import { EventEmitter } from 'events'; +import type { Draft } from 'immer'; import { v1 as random } from 'uuid'; +const stateMetadata = { + unapprovedMessages: { persist: false, anonymous: false }, + unapprovedMessagesCount: { persist: false, anonymous: false }, +}; + +const getDefaultState = () => ({ + unapprovedMessages: {}, + unapprovedMessagesCount: 0, +}); + /** * @type OriginalRequest * * Represents the original request object for adding a message. * @property origin? - Is it is specified, represents the origin */ -// This interface was created before this ESLint rule was added. -// Convert to a `type` in a future major version. -// eslint-disable-next-line @typescript-eslint/consistent-type-definitions -export interface OriginalRequest { +export type OriginalRequest = { id?: number; origin?: string; securityAlertResponse?: Record; -} +}; /** - * @type Message + * @type AbstractMessage * * Represents and contains data about a signing type signature request. * @property id - An id to track and identify the message object @@ -33,10 +45,7 @@ export interface OriginalRequest { * @property securityProviderResponse - Response from a security provider, whether it is malicious or not * @property metadata - Additional data for the message, for example external identifiers */ -// This interface was created before this ESLint rule was added. -// Convert to a `type` in a future major version. -// eslint-disable-next-line @typescript-eslint/consistent-type-definitions -export interface AbstractMessage { +export type AbstractMessage = { id: string; time: number; status: string; @@ -46,7 +55,7 @@ export interface AbstractMessage { securityAlertResponse?: Record; metadata?: Json; error?: string; -} +}; /** * @type AbstractMessageParams @@ -57,15 +66,12 @@ export interface AbstractMessage { * @property requestId? - Original request id * @property deferSetAsSigned? - Whether to defer setting the message as signed immediately after the keyring is told to sign it */ -// This interface was created before this ESLint rule was added. -// Convert to a `type` in a future major version. -// eslint-disable-next-line @typescript-eslint/consistent-type-definitions -export interface AbstractMessageParams { +export type AbstractMessageParams = { from: string; origin?: string; requestId?: number; deferSetAsSigned?: boolean; -} +}; /** * @type MessageParamsMetamask @@ -76,12 +82,9 @@ export interface AbstractMessageParams { * @property from - Address from which the message is processed * @property origin? - Added for request origin identification */ -// This interface was created before this ESLint rule was added. -// Convert to a `type` in a future major version. -// eslint-disable-next-line @typescript-eslint/consistent-type-definitions -export interface AbstractMessageParamsMetamask extends AbstractMessageParams { +export type AbstractMessageParamsMetamask = AbstractMessageParams & { metamaskId?: string; -} +}; /** * @type MessageManagerState @@ -90,15 +93,15 @@ export interface AbstractMessageParamsMetamask extends AbstractMessageParams { * @property unapprovedMessages - A collection of all Messages in the 'unapproved' state * @property unapprovedMessagesCount - The count of all Messages in this.unapprovedMessages */ -// This interface was created before this ESLint rule was added. -// Convert to a `type` in a future major version. -// TODO: Either fix this lint violation or explain why it's necessary to ignore. -// eslint-disable-next-line @typescript-eslint/consistent-type-definitions, @typescript-eslint/naming-convention -export interface MessageManagerState - extends BaseState { - unapprovedMessages: { [key: string]: M }; +export type MessageManagerState = { + unapprovedMessages: Record; unapprovedMessagesCount: number; -} +}; + +export type UpdateBadgeEvent = { + type: `${string}:updateBadge`; + payload: []; +}; /** * A function for verifying a message, whether it is malicious or not @@ -108,32 +111,82 @@ export type SecurityProviderRequest = ( messageType: string, ) => Promise; -// TODO: Either fix this lint violation or explain why it's necessary to ignore. -// eslint-disable-next-line @typescript-eslint/naming-convention -type getCurrentChainId = () => Hex; +/** + * AbstractMessageManager constructor options. + * + * @property additionalFinishStatuses - Optional list of statuses that are accepted to emit a finished event. + * @property messenger - Controller messaging system. + * @property name - The name of the manager. + * @property securityProviderRequest - A function for verifying a message, whether it is malicious or not. + * @property state - Initial state to set on this controller. + */ +export type AbstractMessageManagerOptions< + Message extends AbstractMessage, + Action extends ActionConstraint, + Event extends EventConstraint, +> = { + additionalFinishStatuses?: string[]; + messenger: RestrictedControllerMessenger< + string, + Action, + Event | UpdateBadgeEvent, + string, + string + >; + name: string; + securityProviderRequest?: SecurityProviderRequest; + state?: MessageManagerState; +}; /** * Controller in charge of managing - storing, adding, removing, updating - Messages. */ export abstract class AbstractMessageManager< - // TODO: Either fix this lint violation or explain why it's necessary to ignore. - // eslint-disable-next-line @typescript-eslint/naming-convention - M extends AbstractMessage, - // TODO: Either fix this lint violation or explain why it's necessary to ignore. - // eslint-disable-next-line @typescript-eslint/naming-convention - P extends AbstractMessageParams, - // TODO: Either fix this lint violation or explain why it's necessary to ignore. - // eslint-disable-next-line @typescript-eslint/naming-convention - PM extends AbstractMessageParamsMetamask, -> extends BaseControllerV1> { - protected messages: M[]; - - protected getCurrentChainId: getCurrentChainId | undefined; + Message extends AbstractMessage, + Params extends AbstractMessageParams, + ParamsMetamask extends AbstractMessageParamsMetamask, + Action extends ActionConstraint, + Event extends EventConstraint, +> extends BaseController< + string, + MessageManagerState, + RestrictedControllerMessenger< + string, + Action, + Event | UpdateBadgeEvent, + string, + string + > +> { + protected messages: Message[]; private readonly securityProviderRequest: SecurityProviderRequest | undefined; private readonly additionalFinishStatuses: string[]; + internalEvents = new EventEmitter(); + + constructor({ + additionalFinishStatuses, + messenger, + name, + securityProviderRequest, + state = {} as MessageManagerState, + }: AbstractMessageManagerOptions) { + super({ + messenger, + metadata: stateMetadata, + name, + state: { + ...getDefaultState(), + ...state, + }, + }); + this.messages = []; + this.securityProviderRequest = securityProviderRequest; + this.additionalFinishStatuses = additionalFinishStatuses ?? []; + } + /** * Adds request props to the messsage params and returns a new messageParams object. * @param messageParams - The messageParams to add the request props to. @@ -183,11 +236,16 @@ export abstract class AbstractMessageManager< * @param emitUpdateBadge - Whether to emit the updateBadge event. */ protected saveMessageList(emitUpdateBadge = true) { - const unapprovedMessages = this.getUnapprovedMessages(); - const unapprovedMessagesCount = this.getUnapprovedMessagesCount(); - this.update({ unapprovedMessages, unapprovedMessagesCount }); + this.update((state) => { + state.unapprovedMessages = + this.getUnapprovedMessages() as unknown as Record< + string, + Draft + >; + state.unapprovedMessagesCount = this.getUnapprovedMessagesCount(); + }); if (emitUpdateBadge) { - this.hub.emit('updateBadge'); + this.messagingSystem.publish(`${this.name as string}:updateBadge`); } } @@ -200,18 +258,26 @@ export abstract class AbstractMessageManager< protected setMessageStatus(messageId: string, status: string) { const message = this.getMessage(messageId); if (!message) { - throw new Error(`${this.name}: Message not found for id: ${messageId}.`); + throw new Error( + `${this.name as string}: Message not found for id: ${messageId}.`, + ); } - message.status = status; - this.updateMessage(message); - this.hub.emit(`${messageId}:${status}`, message); + const updatedMessage = { + ...message, + status, + }; + this.updateMessage(updatedMessage); + this.internalEvents.emit(`${messageId}:${status}`, updatedMessage); if ( status === 'rejected' || status === 'signed' || status === 'errored' || this.additionalFinishStatuses.includes(status) ) { - this.hub.emit(`${messageId}:finished`, message); + this.internalEvents.emit( + `${messageId as string}:finished`, + updatedMessage, + ); } } @@ -222,7 +288,7 @@ export abstract class AbstractMessageManager< * @param message - A Message that will replace an existing Message (with the id) in this.messages. * @param emitUpdateBadge - Whether to emit the updateBadge event. */ - protected updateMessage(message: M, emitUpdateBadge = true) { + protected updateMessage(message: Message, emitUpdateBadge = true) { const index = this.messages.findIndex((msg) => message.id === msg.id); /* istanbul ignore next */ if (index !== -1) { @@ -237,7 +303,7 @@ export abstract class AbstractMessageManager< * @param message - The message to verify. * @returns A promise that resolves to a secured message with additional security provider response data. */ - private async securityCheck(message: M): Promise { + private async securityCheck(message: Message): Promise { if (this.securityProviderRequest) { const securityProviderResponse = await this.securityProviderRequest( message, @@ -251,42 +317,11 @@ export abstract class AbstractMessageManager< return message; } - /** - * EventEmitter instance used to listen to specific message events - */ - hub: EventEmitter = new EventEmitter(); - - /** - * Name of this controller used during composition - */ - override name = 'AbstractMessageManager'; - - /** - * Creates an AbstractMessageManager instance. - * - * @param config - Initial options used to configure this controller. - * @param state - Initial state to set on this controller. - * @param securityProviderRequest - A function for verifying a message, whether it is malicious or not. - * @param additionalFinishStatuses - Optional list of statuses that are accepted to emit a finished event. - * @param getCurrentChainId - Optional function to get the current chainId. - */ - constructor( - config?: Partial, - state?: Partial>, - securityProviderRequest?: SecurityProviderRequest, - additionalFinishStatuses?: string[], - getCurrentChainId?: getCurrentChainId, - ) { - super(config, state); - this.defaultState = { - unapprovedMessages: {}, - unapprovedMessagesCount: 0, - }; - this.messages = []; - this.securityProviderRequest = securityProviderRequest; - this.additionalFinishStatuses = additionalFinishStatuses ?? []; - this.getCurrentChainId = getCurrentChainId; - this.initialize(); + clearUnapprovedMessages() { + this.update((state) => { + state.unapprovedMessages = {}; + state.unapprovedMessagesCount = 0; + }); } /** @@ -306,10 +341,10 @@ export abstract class AbstractMessageManager< getUnapprovedMessages() { return this.messages .filter((message) => message.status === 'unapproved') - .reduce((result: { [key: string]: M }, message: M) => { + .reduce((result: Record, message) => { result[message.id] = message; return result; - }, {}) as { [key: string]: M }; + }, {}); } /** @@ -318,7 +353,7 @@ export abstract class AbstractMessageManager< * * @param message - The Message to add to this.messages. */ - async addMessage(message: M) { + async addMessage(message: Message) { const securedMessage = await this.securityCheck(message); this.messages.push(securedMessage); this.saveMessageList(); @@ -352,7 +387,7 @@ export abstract class AbstractMessageManager< * plus data added by MetaMask. * @returns Promise resolving to the messageParams with the metamaskId property removed. */ - approveMessage(messageParams: PM): Promise

{ + approveMessage(messageParams: ParamsMetamask): Promise { // eslint-disable-next-line @typescript-eslint/ban-ts-comment // @ts-ignore this.setMessageStatusApproved(messageParams.metamaskId); @@ -414,8 +449,13 @@ export abstract class AbstractMessageManager< if (!message) { return; } - message.rawSig = result; - this.updateMessage(message, false); + this.updateMessage( + { + ...message, + rawSig: result, + }, + false, + ); } /** @@ -424,14 +464,20 @@ export abstract class AbstractMessageManager< * @param messageId - The id of the Message to update * @param metadata - The data with which to replace the metadata property in the message */ - setMetadata(messageId: string, metadata: Json) { const message = this.getMessage(messageId); if (!message) { - throw new Error(`${this.name}: Message not found for id: ${messageId}.`); + throw new Error( + `${this.name as string}: Message not found for id: ${messageId}.`, + ); } - message.metadata = metadata; - this.updateMessage(message, false); + this.updateMessage( + { + ...message, + metadata, + }, + false, + ); } /** @@ -441,7 +487,9 @@ export abstract class AbstractMessageManager< * @param messageParams - The messageParams to modify * @returns Promise resolving to the messageParams with the metamaskId property removed */ - abstract prepMessageForSigning(messageParams: PM): Promise

; + abstract prepMessageForSigning( + messageParams: ParamsMetamask, + ): Promise; /** * Creates a new Message with an 'unapproved' status using the passed messageParams. @@ -454,7 +502,7 @@ export abstract class AbstractMessageManager< * @returns The id of the newly created message. */ abstract addUnapprovedMessage( - messageParams: PM, + messageParams: ParamsMetamask, request: OriginalRequest, version?: string, ): Promise; @@ -481,34 +529,35 @@ export abstract class AbstractMessageManager< ): Promise { const { metamaskId: messageId, ...messageParams } = messageParamsWithId; return new Promise((resolve, reject) => { - // TODO: Either fix this lint violation or explain why it's necessary to ignore. - // eslint-disable-next-line @typescript-eslint/restrict-template-expressions - this.hub.once(`${messageId}:finished`, (data: AbstractMessage) => { - switch (data.status) { - case 'signed': - return resolve(data.rawSig as string); - case 'rejected': - return reject( - new Error( - `MetaMask ${messageName} Signature: User denied message signature.`, - ), - ); - case 'errored': - return reject( - // TODO: Either fix this lint violation or explain why it's necessary to ignore. - // eslint-disable-next-line @typescript-eslint/restrict-template-expressions - new Error(`MetaMask ${messageName} Signature: ${data.error}`), - ); - default: - return reject( - new Error( - `MetaMask ${messageName} Signature: Unknown problem: ${JSON.stringify( - messageParams, - )}`, - ), - ); - } - }); + this.internalEvents.once( + `${messageId as string}:finished`, + (data: AbstractMessage) => { + switch (data.status) { + case 'signed': + return resolve(data.rawSig as string); + case 'rejected': + return reject( + new Error( + `MetaMask ${messageName} Signature: User denied message signature.`, + ), + ); + case 'errored': + return reject( + new Error( + `MetaMask ${messageName} Signature: ${data.error as string}`, + ), + ); + default: + return reject( + new Error( + `MetaMask ${messageName} Signature: Unknown problem: ${JSON.stringify( + messageParams, + )}`, + ), + ); + } + }, + ); }); } } diff --git a/packages/message-manager/src/DecryptMessageManager.test.ts b/packages/message-manager/src/DecryptMessageManager.test.ts index d81b336141a..1e59533ae5f 100644 --- a/packages/message-manager/src/DecryptMessageManager.test.ts +++ b/packages/message-manager/src/DecryptMessageManager.test.ts @@ -1,4 +1,18 @@ import { DecryptMessageManager } from './DecryptMessageManager'; +import type { DecryptMessageManagerMessenger } from './DecryptMessageManager'; + +const mockMessenger = { + registerActionHandler: jest.fn(), + registerInitialEventPayload: jest.fn(), + publish: jest.fn(), + clearEventSubscriptions: jest.fn(), +} as unknown as DecryptMessageManagerMessenger; + +const mockInitialOptions = { + additionalFinishStatuses: undefined, + messenger: mockMessenger, + securityProviderRequest: undefined, +}; describe('DecryptMessageManager', () => { let controller: DecryptMessageManager; @@ -9,7 +23,7 @@ describe('DecryptMessageManager', () => { const dataMock = '0x12345'; beforeEach(() => { - controller = new DecryptMessageManager(); + controller = new DecryptMessageManager(mockInitialOptions); }); it('sets default state', () => { @@ -19,10 +33,6 @@ describe('DecryptMessageManager', () => { }); }); - it('sets default config', () => { - expect(controller.config).toStrictEqual({}); - }); - it('adds a valid message', async () => { const messageData = '0x123'; const messageTime = Date.now(); @@ -52,9 +62,7 @@ describe('DecryptMessageManager', () => { describe('addUnapprovedMessageAsync', () => { beforeEach(() => { - controller = new DecryptMessageManager(undefined, undefined, undefined, [ - 'decrypted', - ]); + controller = new DecryptMessageManager(mockInitialOptions); jest .spyOn(controller, 'addUnapprovedMessage') @@ -72,7 +80,7 @@ describe('DecryptMessageManager', () => { data: dataMock, }); setTimeout(() => { - controller.hub.emit(`${messageIdMock}:finished`, { + controller.internalEvents.emit(`${messageIdMock}:finished`, { status: 'decrypted', rawSig: rawSigMock, }); @@ -88,7 +96,7 @@ describe('DecryptMessageManager', () => { }); setTimeout(() => { - controller.hub.emit(`${messageIdMock}:finished`, { + controller.internalEvents.emit(`${messageIdMock}:finished`, { status: 'rejected', }); }, 100); @@ -105,7 +113,7 @@ describe('DecryptMessageManager', () => { }); setTimeout(() => { - controller.hub.emit(`${messageIdMock}:finished`, { + controller.internalEvents.emit(`${messageIdMock}:finished`, { status: 'errored', }); }, 100); @@ -122,7 +130,7 @@ describe('DecryptMessageManager', () => { }); setTimeout(() => { - controller.hub.emit(`${messageIdMock}:finished`, { + controller.internalEvents.emit(`${messageIdMock}:finished`, { status: 'unknown', }); }, 100); diff --git a/packages/message-manager/src/DecryptMessageManager.ts b/packages/message-manager/src/DecryptMessageManager.ts index 4036e79d901..571ca9e760d 100644 --- a/packages/message-manager/src/DecryptMessageManager.ts +++ b/packages/message-manager/src/DecryptMessageManager.ts @@ -1,14 +1,52 @@ +import type { + ActionConstraint, + EventConstraint, + RestrictedControllerMessenger, +} from '@metamask/base-controller'; import { ApprovalType } from '@metamask/controller-utils'; import type { AbstractMessage, AbstractMessageParams, AbstractMessageParamsMetamask, + MessageManagerState, OriginalRequest, + SecurityProviderRequest, } from './AbstractMessageManager'; import { AbstractMessageManager } from './AbstractMessageManager'; import { normalizeMessageData, validateDecryptedMessageData } from './utils'; +const managerName = 'DecryptMessageManager'; + +export type DecryptMessageManagerState = MessageManagerState; + +export type DecryptMessageManagerUnapprovedMessageAddedEvent = { + type: `${typeof managerName}:unapprovedMessage`; + payload: [AbstractMessageParamsMetamask]; +}; + +export type DecryptMessageManagerUpdateBadgeEvent = { + type: `${typeof managerName}:updateBadge`; + payload: []; +}; + +export type DecryptMessageManagerMessenger = RestrictedControllerMessenger< + string, + ActionConstraint, + | EventConstraint + | DecryptMessageManagerUnapprovedMessageAddedEvent + | DecryptMessageManagerUpdateBadgeEvent, + string, + string +>; + +type DecryptMessageManagerOptions = { + messenger: DecryptMessageManagerMessenger; + securityProviderRequest?: SecurityProviderRequest; + state?: MessageManagerState; + additionalFinishStatuses?: string[]; +}; + /** * @type DecryptMessage * @@ -19,12 +57,9 @@ import { normalizeMessageData, validateDecryptedMessageData } from './utils'; * @property type - The json-prc signing method for which a signature request has been made. * A 'DecryptMessage' which always has a 'eth_decrypt' type */ -// This interface was created before this ESLint rule was added. -// Convert to a `type` in a future major version. -// eslint-disable-next-line @typescript-eslint/consistent-type-definitions -export interface DecryptMessage extends AbstractMessage { +export type DecryptMessage = AbstractMessage & { messageParams: DecryptMessageParams; -} +}; /** * @type DecryptMessageParams @@ -32,12 +67,9 @@ export interface DecryptMessage extends AbstractMessage { * Represents the parameters to pass to the eth_decrypt method once the request is approved. * @property data - A hex string conversion of the raw buffer data of the signature request */ -// This interface was created before this ESLint rule was added. -// Convert to a `type` in a future major version. -// eslint-disable-next-line @typescript-eslint/consistent-type-definitions -export interface DecryptMessageParams extends AbstractMessageParams { +export type DecryptMessageParams = AbstractMessageParams & { data: string; -} +}; /** * @type DecryptMessageParamsMetamask @@ -63,12 +95,26 @@ export interface DecryptMessageParamsMetamask export class DecryptMessageManager extends AbstractMessageManager< DecryptMessage, DecryptMessageParams, - DecryptMessageParamsMetamask + DecryptMessageParamsMetamask, + ActionConstraint, + | EventConstraint + | DecryptMessageManagerUnapprovedMessageAddedEvent + | DecryptMessageManagerUpdateBadgeEvent > { - /** - * Name of this controller used during composition - */ - override name = 'DecryptMessageManager' as const; + constructor({ + additionalFinishStatuses, + messenger, + securityProviderRequest, + state, + }: DecryptMessageManagerOptions) { + super({ + additionalFinishStatuses, + messenger, + name: managerName, + securityProviderRequest, + state, + }); + } /** * Creates a new Message with an 'unapproved' status using the passed messageParams. @@ -86,32 +132,35 @@ export class DecryptMessageManager extends AbstractMessageManager< const messageId = await this.addUnapprovedMessage(messageParams, req); return new Promise((resolve, reject) => { - this.hub.once(`${messageId}:finished`, (data: DecryptMessage) => { - switch (data.status) { - case 'decrypted': - return resolve(data.rawSig as string); - case 'rejected': - return reject( - new Error( - 'MetaMask DecryptMessage: User denied message decryption.', - ), - ); - case 'errored': - return reject( - new Error( - 'MetaMask DecryptMessage: This message cannot be decrypted.', - ), - ); - default: - return reject( - new Error( - `MetaMask DecryptMessage: Unknown problem: ${JSON.stringify( - messageParams, - )}`, - ), - ); - } - }); + this.internalEvents.once( + `${messageId}:finished`, + (data: DecryptMessage) => { + switch (data.status) { + case 'decrypted': + return resolve(data.rawSig as string); + case 'rejected': + return reject( + new Error( + 'MetaMask DecryptMessage: User denied message decryption.', + ), + ); + case 'errored': + return reject( + new Error( + 'MetaMask DecryptMessage: This message cannot be decrypted.', + ), + ); + default: + return reject( + new Error( + `MetaMask DecryptMessage: Unknown problem: ${JSON.stringify( + messageParams, + )}`, + ), + ); + } + }, + ); }); } @@ -144,7 +193,7 @@ export class DecryptMessageManager extends AbstractMessageManager< const messageId = messageData.id; await this.addMessage(messageData); - this.hub.emit(`unapprovedMessage`, { + this.messagingSystem.publish(`${managerName}:unapprovedMessage`, { ...updatedMessageParams, metamaskId: messageId, }); diff --git a/packages/message-manager/src/EncryptionPublicKeyManager.test.ts b/packages/message-manager/src/EncryptionPublicKeyManager.test.ts index 8617c165340..81735a5fdaa 100644 --- a/packages/message-manager/src/EncryptionPublicKeyManager.test.ts +++ b/packages/message-manager/src/EncryptionPublicKeyManager.test.ts @@ -1,4 +1,18 @@ import { EncryptionPublicKeyManager } from './EncryptionPublicKeyManager'; +import type { EncryptionPublicKeyManagerMessenger } from './EncryptionPublicKeyManager'; + +const mockMessenger = { + registerActionHandler: jest.fn(), + registerInitialEventPayload: jest.fn(), + publish: jest.fn(), + clearEventSubscriptions: jest.fn(), +} as unknown as EncryptionPublicKeyManagerMessenger; + +const mockInitialOptions = { + additionalFinishStatuses: undefined, + messenger: mockMessenger, + securityProviderRequest: undefined, +}; describe('EncryptionPublicKeyManager', () => { let controller: EncryptionPublicKeyManager; @@ -8,7 +22,7 @@ describe('EncryptionPublicKeyManager', () => { const rawSigMock = '231124fe67213512='; beforeEach(() => { - controller = new EncryptionPublicKeyManager(); + controller = new EncryptionPublicKeyManager(mockInitialOptions); }); it('sets default state', () => { @@ -18,10 +32,6 @@ describe('EncryptionPublicKeyManager', () => { }); }); - it('sets default config', () => { - expect(controller.config).toStrictEqual({}); - }); - it('adds a valid message', async () => { const messageTime = Date.now(); const messageStatus = 'unapproved'; @@ -48,12 +58,7 @@ describe('EncryptionPublicKeyManager', () => { describe('addUnapprovedMessageAsync', () => { beforeEach(() => { - controller = new EncryptionPublicKeyManager( - undefined, - undefined, - undefined, - ['received'], - ); + controller = new EncryptionPublicKeyManager(mockInitialOptions); jest .spyOn(controller, 'addUnapprovedMessage') @@ -70,7 +75,7 @@ describe('EncryptionPublicKeyManager', () => { }); setTimeout(() => { - controller.hub.emit(`${messageIdMock}:finished`, { + controller.internalEvents.emit(`${messageIdMock}:finished`, { status: 'received', rawSig: rawSigMock, }); @@ -85,7 +90,7 @@ describe('EncryptionPublicKeyManager', () => { }); setTimeout(() => { - controller.hub.emit(`${messageIdMock}:finished`, { + controller.internalEvents.emit(`${messageIdMock}:finished`, { status: 'rejected', }); }, 100); @@ -101,7 +106,7 @@ describe('EncryptionPublicKeyManager', () => { }); setTimeout(() => { - controller.hub.emit(`${messageIdMock}:finished`, { + controller.internalEvents.emit(`${messageIdMock}:finished`, { status: 'unknown', }); }, 100); diff --git a/packages/message-manager/src/EncryptionPublicKeyManager.ts b/packages/message-manager/src/EncryptionPublicKeyManager.ts index 6fdf1518f70..6654bc01329 100644 --- a/packages/message-manager/src/EncryptionPublicKeyManager.ts +++ b/packages/message-manager/src/EncryptionPublicKeyManager.ts @@ -1,14 +1,53 @@ +import type { + ActionConstraint, + EventConstraint, + RestrictedControllerMessenger, +} from '@metamask/base-controller'; import { ApprovalType } from '@metamask/controller-utils'; import type { AbstractMessage, AbstractMessageParams, AbstractMessageParamsMetamask, + MessageManagerState, OriginalRequest, + SecurityProviderRequest, } from './AbstractMessageManager'; import { AbstractMessageManager } from './AbstractMessageManager'; import { validateEncryptionPublicKeyMessageData } from './utils'; +const managerName = 'EncryptionPublicKeyManager'; + +export type EncryptionPublicKeyManagerState = + MessageManagerState; + +export type EncryptionPublicKeyManagerUnapprovedMessageAddedEvent = { + type: `${typeof managerName}:unapprovedMessage`; + payload: [AbstractMessageParamsMetamask]; +}; + +export type EncryptionPublicKeyManagerUpdateBadgeEvent = { + type: `${typeof managerName}:updateBadge`; + payload: []; +}; + +export type EncryptionPublicKeyManagerMessenger = RestrictedControllerMessenger< + string, + ActionConstraint, + | EventConstraint + | EncryptionPublicKeyManagerUnapprovedMessageAddedEvent + | EncryptionPublicKeyManagerUpdateBadgeEvent, + string, + string +>; + +type EncryptionPublicKeyManagerOptions = { + messenger: EncryptionPublicKeyManagerMessenger; + securityProviderRequest?: SecurityProviderRequest; + state?: MessageManagerState; + additionalFinishStatuses?: string[]; +}; + /** * @type EncryptionPublicKey * @@ -20,12 +59,9 @@ import { validateEncryptionPublicKeyMessageData } from './utils'; * A 'Message' which always has a 'eth_getEncryptionPublicKey' type * @property rawSig - Encryption public key */ -// This interface was created before this ESLint rule was added. -// Convert to a `type` in a future major version. -// eslint-disable-next-line @typescript-eslint/consistent-type-definitions -export interface EncryptionPublicKey extends AbstractMessage { +export type EncryptionPublicKey = AbstractMessage & { messageParams: EncryptionPublicKeyParams; -} +}; /** * @type EncryptionPublicKeyParams @@ -46,13 +82,10 @@ export type EncryptionPublicKeyParams = AbstractMessageParams; * @property from - Address from which to extract the encryption public key * @property origin? - Added for request origin identification */ -// This interface was created before this ESLint rule was added. -// Convert to a `type` in a future major version. -// eslint-disable-next-line @typescript-eslint/consistent-type-definitions -export interface EncryptionPublicKeyParamsMetamask - extends AbstractMessageParamsMetamask { - data: string; -} +export type EncryptionPublicKeyParamsMetamask = + AbstractMessageParamsMetamask & { + data: string; + }; /** * Controller in charge of managing - storing, adding, removing, updating - Messages. @@ -60,12 +93,26 @@ export interface EncryptionPublicKeyParamsMetamask export class EncryptionPublicKeyManager extends AbstractMessageManager< EncryptionPublicKey, EncryptionPublicKeyParams, - EncryptionPublicKeyParamsMetamask + EncryptionPublicKeyParamsMetamask, + ActionConstraint, + | EventConstraint + | EncryptionPublicKeyManagerUnapprovedMessageAddedEvent + | EncryptionPublicKeyManagerUpdateBadgeEvent > { - /** - * Name of this controller used during composition - */ - override name = 'EncryptionPublicKeyManager' as const; + constructor({ + additionalFinishStatuses, + messenger, + securityProviderRequest, + state, + }: EncryptionPublicKeyManagerOptions) { + super({ + additionalFinishStatuses, + messenger, + name: managerName, + securityProviderRequest, + state, + }); + } /** * Creates a new Message with an 'unapproved' status using the passed messageParams. @@ -83,26 +130,29 @@ export class EncryptionPublicKeyManager extends AbstractMessageManager< const messageId = await this.addUnapprovedMessage(messageParams, req); return new Promise((resolve, reject) => { - this.hub.once(`${messageId}:finished`, (data: EncryptionPublicKey) => { - switch (data.status) { - case 'received': - return resolve(data.rawSig as string); - case 'rejected': - return reject( - new Error( - 'MetaMask EncryptionPublicKey: User denied message EncryptionPublicKey.', - ), - ); - default: - return reject( - new Error( - `MetaMask EncryptionPublicKey: Unknown problem: ${JSON.stringify( - messageParams, - )}`, - ), - ); - } - }); + this.internalEvents.once( + `${messageId}:finished`, + (data: EncryptionPublicKey) => { + switch (data.status) { + case 'received': + return resolve(data.rawSig as string); + case 'rejected': + return reject( + new Error( + 'MetaMask EncryptionPublicKey: User denied message EncryptionPublicKey.', + ), + ); + default: + return reject( + new Error( + `MetaMask EncryptionPublicKey: Unknown problem: ${JSON.stringify( + messageParams, + )}`, + ), + ); + } + }, + ); }); } @@ -134,7 +184,7 @@ export class EncryptionPublicKeyManager extends AbstractMessageManager< const messageId = messageData.id; await this.addMessage(messageData); - this.hub.emit(`unapprovedMessage`, { + this.messagingSystem.publish(`${this.name as string}:unapprovedMessage`, { ...updatedMessageParams, metamaskId: messageId, }); diff --git a/packages/network-controller/src/create-network-client.ts b/packages/network-controller/src/create-network-client.ts index dab4702a942..e6620184878 100644 --- a/packages/network-controller/src/create-network-client.ts +++ b/packages/network-controller/src/create-network-client.ts @@ -71,7 +71,6 @@ export function createNetworkClient( const rpcProvider = providerFromMiddleware(rpcApiMiddleware); const blockTrackerOpts = - // eslint-disable-next-line n/no-process-env process.env.IN_TEST && networkConfig.type === 'custom' ? { pollingInterval: SECOND } : {}; @@ -190,7 +189,6 @@ function createCustomNetworkMiddleware({ chainId: Hex; rpcApiMiddleware: JsonRpcMiddleware; }): JsonRpcMiddleware { - // eslint-disable-next-line n/no-process-env const testMiddlewares = process.env.IN_TEST ? [createEstimateGasDelayTestMiddleware()] : []; diff --git a/packages/network-controller/tests/NetworkController.test.ts b/packages/network-controller/tests/NetworkController.test.ts index 3f224b7170f..009fdc95138 100644 --- a/packages/network-controller/tests/NetworkController.test.ts +++ b/packages/network-controller/tests/NetworkController.test.ts @@ -13069,6 +13069,7 @@ function refreshNetworkTests({ initialState?: Partial; operation: (controller: NetworkController) => Promise; }) { + // eslint-disable-next-line jest/require-top-level-describe it('emits networkWillChange with state payload', async () => { await withController( { @@ -13097,6 +13098,7 @@ function refreshNetworkTests({ ); }); + // eslint-disable-next-line jest/require-top-level-describe it('emits networkDidChange with state payload', async () => { await withController( { @@ -13126,6 +13128,7 @@ function refreshNetworkTests({ }); if (expectedNetworkClientConfiguration.type === NetworkClientType.Custom) { + // eslint-disable-next-line jest/require-top-level-describe it('sets the provider to a custom RPC provider initialized with the RPC target and chain ID', async () => { await withController( { @@ -13167,8 +13170,7 @@ function refreshNetworkTests({ ); }); } else { - // TODO: Either fix this lint violation or explain why it's necessary to ignore. - // eslint-disable-next-line @typescript-eslint/restrict-template-expressions + // eslint-disable-next-line jest/require-top-level-describe it(`sets the provider to an Infura provider pointed to ${expectedNetworkClientConfiguration.network}`, async () => { await withController( { @@ -13209,6 +13211,7 @@ function refreshNetworkTests({ }); } + // eslint-disable-next-line jest/require-top-level-describe it('replaces the provider object underlying the provider proxy without creating a new instance of the proxy itself', async () => { await withController( { diff --git a/packages/notification-services-controller/package.json b/packages/notification-services-controller/package.json index d4425ebc0f4..7515a0a3e62 100644 --- a/packages/notification-services-controller/package.json +++ b/packages/notification-services-controller/package.json @@ -113,7 +113,7 @@ "@lavamoat/preinstall-always-fail": "^2.1.0", "@metamask/auto-changelog": "^3.4.4", "@metamask/keyring-controller": "^19.0.3", - "@metamask/profile-sync-controller": "^4.0.0", + "@metamask/profile-sync-controller": "^4.1.0", "@types/jest": "^27.4.1", "@types/readable-stream": "^2.3.0", "contentful": "^10.15.0", diff --git a/packages/notification-services-controller/src/NotificationServicesPushController/services/push/push-web.ts b/packages/notification-services-controller/src/NotificationServicesPushController/services/push/push-web.ts index ccfab4e40f5..67b46f68d0c 100644 --- a/packages/notification-services-controller/src/NotificationServicesPushController/services/push/push-web.ts +++ b/packages/notification-services-controller/src/NotificationServicesPushController/services/push/push-web.ts @@ -20,7 +20,7 @@ import type { PushNotificationEnv } from '../../types/firebase'; declare const self: ServiceWorkerGlobalScope; // Exported to help testing -// eslint-disable-next-line import/no-mutable-exports +// eslint-disable-next-line import-x/no-mutable-exports export let supportedCache: boolean | null = null; const getPushAvailability = async () => { diff --git a/packages/permission-controller/src/PermissionController.ts b/packages/permission-controller/src/PermissionController.ts index b5e33393f86..27cf026d018 100644 --- a/packages/permission-controller/src/PermissionController.ts +++ b/packages/permission-controller/src/PermissionController.ts @@ -1982,7 +1982,6 @@ export class PermissionController< target: string, ): void { if (!isPlainObject(caveat)) { - // eslint-disable-next-line @typescript-eslint/no-throw-literal throw new InvalidCaveatError(caveat, origin, target); } diff --git a/packages/phishing-controller/src/PhishingDetector.test.ts b/packages/phishing-controller/src/PhishingDetector.test.ts index f76b8099650..38e2f2b8b67 100644 --- a/packages/phishing-controller/src/PhishingDetector.test.ts +++ b/packages/phishing-controller/src/PhishingDetector.test.ts @@ -1110,7 +1110,7 @@ describe('PhishingDetector', () => { ]; // CID should be blocked - for await (const entry of expectedToBeBlocked) { + for (const entry of expectedToBeBlocked) { await withPhishingDetector( [ { diff --git a/packages/polling-controller/src/AbstractPollingController.ts b/packages/polling-controller/src/AbstractPollingController.ts index d52aab938a0..f22455e4b13 100644 --- a/packages/polling-controller/src/AbstractPollingController.ts +++ b/packages/polling-controller/src/AbstractPollingController.ts @@ -82,7 +82,6 @@ export function AbstractPollingControllerBaseMixin< const callbacks = this.#callbacks.get(keyToDelete); if (callbacks) { for (const callback of callbacks) { - // eslint-disable-next-line n/callback-return callback(JSON.parse(keyToDelete)); } callbacks.clear(); diff --git a/packages/profile-sync-controller/CHANGELOG.md b/packages/profile-sync-controller/CHANGELOG.md index be348cea2cc..5cf6aa5b8d8 100644 --- a/packages/profile-sync-controller/CHANGELOG.md +++ b/packages/profile-sync-controller/CHANGELOG.md @@ -7,6 +7,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [4.1.0] + +### Changed + +- Persist `isAccountSyncingReadyToBeDispatched` state value ([#5147](https://github.com/MetaMask/core/pull/5147)) + +## [4.0.1] + +### Added + +- Add optional sentry context parameter to erroneous situation callbacks ([#5139](https://github.com/MetaMask/core/pull/5139)) + ## [4.0.0] ### Changed @@ -410,7 +422,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Initial release -[Unreleased]: https://github.com/MetaMask/core/compare/@metamask/profile-sync-controller@4.0.0...HEAD +[Unreleased]: https://github.com/MetaMask/core/compare/@metamask/profile-sync-controller@4.1.0...HEAD +[4.1.0]: https://github.com/MetaMask/core/compare/@metamask/profile-sync-controller@4.0.1...@metamask/profile-sync-controller@4.1.0 +[4.0.1]: https://github.com/MetaMask/core/compare/@metamask/profile-sync-controller@4.0.0...@metamask/profile-sync-controller@4.0.1 [4.0.0]: https://github.com/MetaMask/core/compare/@metamask/profile-sync-controller@3.3.0...@metamask/profile-sync-controller@4.0.0 [3.3.0]: https://github.com/MetaMask/core/compare/@metamask/profile-sync-controller@3.2.0...@metamask/profile-sync-controller@3.3.0 [3.2.0]: https://github.com/MetaMask/core/compare/@metamask/profile-sync-controller@3.1.1...@metamask/profile-sync-controller@3.2.0 diff --git a/packages/profile-sync-controller/package.json b/packages/profile-sync-controller/package.json index ceac6b460f8..232671b4cd9 100644 --- a/packages/profile-sync-controller/package.json +++ b/packages/profile-sync-controller/package.json @@ -1,6 +1,6 @@ { "name": "@metamask/profile-sync-controller", - "version": "4.0.0", + "version": "4.1.0", "description": "The profile sync helps developers synchronize data across multiple clients and devices in a privacy-preserving way. All data saved in the user storage database is encrypted client-side to preserve privacy. The user storage provides a modular design, giving developers the flexibility to construct and manage their storage spaces in a way that best suits their needs", "keywords": [ "MetaMask", diff --git a/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.test.ts b/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.test.ts index 5aa1f3b8a12..a9c18980a0d 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.test.ts +++ b/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.test.ts @@ -173,9 +173,8 @@ describe('user-storage/user-storage-controller - performGetStorageAllFeatureEntr getMetaMetricsState: () => true, }); - const result = await controller.performGetStorageAllFeatureEntries( - 'notifications', - ); + const result = + await controller.performGetStorageAllFeatureEntries('notifications'); mockAPI.done(); expect(result).toStrictEqual([MOCK_STORAGE_DATA]); }); @@ -893,7 +892,7 @@ describe('user-storage/user-storage-controller - syncInternalAccountsWithUserSto ) => { onAccountAdded?.(); onAccountNameUpdated?.(); - onAccountSyncErroneousSituation?.('error message'); + onAccountSyncErroneousSituation?.('error message', {}); getMessenger(); getUserStorageControllerInstance(); return undefined; diff --git a/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.ts b/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.ts index 99c5606bdbd..ec620709e3a 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.ts +++ b/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.ts @@ -126,7 +126,7 @@ const metadata: StateMetadata = { anonymous: false, }, isAccountSyncingReadyToBeDispatched: { - persist: false, + persist: true, anonymous: false, }, isAccountSyncingInProgress: { @@ -161,6 +161,7 @@ type ControllerConfig = { onAccountSyncErroneousSituation?: ( profileId: string, situationMessage: string, + sentryContext?: Record, ) => void; }; @@ -864,10 +865,11 @@ export default class UserStorageController extends BaseController< this.#config?.accountSyncing?.onAccountAdded?.(profileId), onAccountNameUpdated: () => this.#config?.accountSyncing?.onAccountNameUpdated?.(profileId), - onAccountSyncErroneousSituation: (situationMessage) => + onAccountSyncErroneousSituation: (situationMessage, sentryContext) => this.#config?.accountSyncing?.onAccountSyncErroneousSituation?.( profileId, situationMessage, + sentryContext, ), }, { diff --git a/packages/profile-sync-controller/src/controllers/user-storage/account-syncing/controller-integration.test.ts b/packages/profile-sync-controller/src/controllers/user-storage/account-syncing/controller-integration.test.ts index d0a9bf109ec..f1638d67f35 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/account-syncing/controller-integration.test.ts +++ b/packages/profile-sync-controller/src/controllers/user-storage/account-syncing/controller-integration.test.ts @@ -298,28 +298,32 @@ describe('user-storage/account-syncing/controller-integration - syncInternalAcco describe('handles corrupted user storage gracefully', () => { const arrangeMocksForBogusAccounts = async () => { + const accountsList = + MOCK_INTERNAL_ACCOUNTS.ONE_DEFAULT_NAME as InternalAccount[]; const { messengerMocks, config, options } = await arrangeMocks({ messengerMockOptions: { accounts: { - accountsList: - MOCK_INTERNAL_ACCOUNTS.ONE_DEFAULT_NAME as InternalAccount[], + accountsList, }, }, }); + const userStorageList = + MOCK_USER_STORAGE_ACCOUNTS.TWO_DEFAULT_NAMES_WITH_ONE_BOGUS; + return { config, options, messengerMocks, + accountsList, + userStorageList, mockAPI: { mockEndpointGetUserStorage: await mockEndpointGetUserStorageAllFeatureEntries( USER_STORAGE_FEATURE_NAMES.accounts, { status: 200, - body: await createMockUserStorageEntries( - MOCK_USER_STORAGE_ACCOUNTS.TWO_DEFAULT_NAMES_WITH_ONE_BOGUS, - ), + body: await createMockUserStorageEntries(userStorageList), }, ), mockEndpointBatchDeleteUserStorage: @@ -363,20 +367,86 @@ describe('user-storage/account-syncing/controller-integration - syncInternalAcco expect(mockAPI.mockEndpointBatchDeleteUserStorage.isDone()).toBe(true); }); - it('fires the onAccountSyncErroneousSituation callback in erroneous situations', async () => { - const onAccountSyncErroneousSituation = jest.fn(); + describe('Fires the onAccountSyncErroneousSituation callback on erroneous situations', () => { + it('And logs if the final state is incorrect', async () => { + const onAccountSyncErroneousSituation = jest.fn(); - const { config, options } = await arrangeMocksForBogusAccounts(); + const { config, options, userStorageList, accountsList } = + await arrangeMocksForBogusAccounts(); - await AccountSyncingControllerIntegrationModule.syncInternalAccountsWithUserStorage( - { - ...config, - onAccountSyncErroneousSituation, - }, - options, - ); + await AccountSyncingControllerIntegrationModule.syncInternalAccountsWithUserStorage( + { + ...config, + onAccountSyncErroneousSituation, + }, + options, + ); - expect(onAccountSyncErroneousSituation).toHaveBeenCalledTimes(1); + expect(onAccountSyncErroneousSituation).toHaveBeenCalledTimes(2); + expect(onAccountSyncErroneousSituation.mock.calls).toEqual([ + [ + 'An account was present in the user storage accounts list but was not found in the internal accounts list after the sync', + { + internalAccountsList: accountsList, + internalAccountsToBeSavedToUserStorage: [], + refreshedInternalAccountsList: accountsList, + userStorageAccountsList: userStorageList, + userStorageAccountsToBeDeleted: [userStorageList[1]], + }, + ], + [ + 'Erroneous situations were found during the sync, and final state does not match the expected state', + { + finalInternalAccountsList: accountsList, + finalUserStorageAccountsList: null, + }, + ], + ]); + }); + + it('And logs if the final state is correct', async () => { + const onAccountSyncErroneousSituation = jest.fn(); + + const { config, options, userStorageList, accountsList } = + await arrangeMocksForBogusAccounts(); + + await mockEndpointGetUserStorageAllFeatureEntries( + USER_STORAGE_FEATURE_NAMES.accounts, + { + status: 200, + body: await createMockUserStorageEntries([userStorageList[0]]), + }, + ); + + await AccountSyncingControllerIntegrationModule.syncInternalAccountsWithUserStorage( + { + ...config, + onAccountSyncErroneousSituation, + }, + options, + ); + + expect(onAccountSyncErroneousSituation).toHaveBeenCalledTimes(2); + expect(onAccountSyncErroneousSituation.mock.calls).toEqual([ + [ + 'An account was present in the user storage accounts list but was not found in the internal accounts list after the sync', + { + internalAccountsList: accountsList, + internalAccountsToBeSavedToUserStorage: [], + refreshedInternalAccountsList: accountsList, + userStorageAccountsList: userStorageList, + userStorageAccountsToBeDeleted: [userStorageList[1]], + }, + ], + [ + 'Erroneous situations were found during the sync, but final state matches the expected state', + { + finalInternalAccountsList: accountsList, + finalUserStorageAccountsList: [userStorageList[0]], + }, + ], + ]); + }); }); }); diff --git a/packages/profile-sync-controller/src/controllers/user-storage/account-syncing/controller-integration.ts b/packages/profile-sync-controller/src/controllers/user-storage/account-syncing/controller-integration.ts index e39498d58b7..027f119879e 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/account-syncing/controller-integration.ts +++ b/packages/profile-sync-controller/src/controllers/user-storage/account-syncing/controller-integration.ts @@ -96,7 +96,10 @@ type SyncInternalAccountsWithUserStorageConfig = AccountSyncingConfig & { maxNumberOfAccountsToAdd?: number; onAccountAdded?: () => void; onAccountNameUpdated?: () => void; - onAccountSyncErroneousSituation?: (errorMessage: string) => void; + onAccountSyncErroneousSituation?: ( + errorMessage: string, + sentryContext?: Record, + ) => void; }; /** @@ -141,6 +144,9 @@ export async function syncInternalAccountsWithUserStorage( ); return; } + // Keep a record if erroneous situations are found during the sync + // This is done so we can send the context to Sentry in case of an erroneous situation + let erroneousSituationsFound = false; // Prepare an array of internal accounts to be saved to the user storage const internalAccountsToBeSavedToUserStorage: InternalAccount[] = []; @@ -191,8 +197,18 @@ export async function syncInternalAccountsWithUserStorage( if (!userStorageAccount) { // If the account was just added in the previous step, skip saving it, it's likely to be a bogus account if (newlyAddedAccounts.includes(internalAccount)) { + erroneousSituationsFound = true; onAccountSyncErroneousSituation?.( 'An account was added to the internal accounts list but was not present in the user storage accounts list', + { + internalAccount, + userStorageAccount, + newlyAddedAccounts, + userStorageAccountsList, + internalAccountsList, + refreshedInternalAccountsList, + internalAccountsToBeSavedToUserStorage, + }, ); continue; } @@ -295,11 +311,64 @@ export async function syncInternalAccountsWithUserStorage( USER_STORAGE_FEATURE_NAMES.accounts, userStorageAccountsToBeDeleted.map((account) => account.a), ); + erroneousSituationsFound = true; onAccountSyncErroneousSituation?.( 'An account was present in the user storage accounts list but was not found in the internal accounts list after the sync', + { + userStorageAccountsToBeDeleted, + internalAccountsList, + refreshedInternalAccountsList, + internalAccountsToBeSavedToUserStorage, + userStorageAccountsList, + }, ); } + if (erroneousSituationsFound) { + const [finalUserStorageAccountsList, finalInternalAccountsList] = + await Promise.all([ + getUserStorageAccountsList(options), + getInternalAccountsList(options), + ]); + + const doesEveryAccountInInternalAccountsListExistInUserStorageAccountsList = + finalInternalAccountsList.every((account) => + finalUserStorageAccountsList?.some( + (userStorageAccount) => userStorageAccount.a === account.address, + ), + ); + + // istanbul ignore next + const doesEveryAccountInUserStorageAccountsListExistInInternalAccountsList = + (finalUserStorageAccountsList?.length || 0) > maxNumberOfAccountsToAdd + ? true + : finalUserStorageAccountsList?.every((account) => + finalInternalAccountsList.some( + (internalAccount) => internalAccount.address === account.a, + ), + ); + + const doFinalListsMatch = + doesEveryAccountInInternalAccountsListExistInUserStorageAccountsList && + doesEveryAccountInUserStorageAccountsListExistInInternalAccountsList; + + const context = { + finalUserStorageAccountsList, + finalInternalAccountsList, + }; + if (doFinalListsMatch) { + onAccountSyncErroneousSituation?.( + 'Erroneous situations were found during the sync, but final state matches the expected state', + context, + ); + } else { + onAccountSyncErroneousSituation?.( + 'Erroneous situations were found during the sync, and final state does not match the expected state', + context, + ); + } + } + // We do this here and not in the finally statement because we want to make sure that // the accounts are saved / updated / deleted at least once before we set this flag await getUserStorageControllerInstance().setHasAccountSyncingSyncedAtLeastOnce( diff --git a/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/controller-integration.ts b/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/controller-integration.ts index 63df3b5721b..359520d8294 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/controller-integration.ts +++ b/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/controller-integration.ts @@ -29,7 +29,7 @@ type PerformMainNetworkSyncProps = { * Ensures that listeners do not fire during main sync (prevent double requests) */ // Exported to help testing -// eslint-disable-next-line import/no-mutable-exports +// eslint-disable-next-line import-x/no-mutable-exports export let isMainNetworkSyncInProgress = false; /** diff --git a/packages/selected-network-controller/tests/SelectedNetworkController.test.ts b/packages/selected-network-controller/tests/SelectedNetworkController.test.ts index fd975016ca1..847f1b5898e 100644 --- a/packages/selected-network-controller/tests/SelectedNetworkController.test.ts +++ b/packages/selected-network-controller/tests/SelectedNetworkController.test.ts @@ -44,7 +44,7 @@ function buildMessenger() { * @param options.getSubjectNames - Permissions controller list of domains with permissions * @returns The network controller restricted messenger. */ -export function buildSelectedNetworkControllerMessenger({ +function buildSelectedNetworkControllerMessenger({ messenger = new ControllerMessenger< SelectedNetworkControllerActions | AllowedActions, SelectedNetworkControllerEvents | AllowedEvents diff --git a/packages/signature-controller/src/SignatureController.ts b/packages/signature-controller/src/SignatureController.ts index 54b8f1e63ec..239cd76386c 100644 --- a/packages/signature-controller/src/SignatureController.ts +++ b/packages/signature-controller/src/SignatureController.ts @@ -30,7 +30,7 @@ import { import type { NetworkControllerGetNetworkClientByIdAction } from '@metamask/network-controller'; import type { Hex, Json } from '@metamask/utils'; // This package purposefully relies on Node's EventEmitter module. -// eslint-disable-next-line import/no-nodejs-modules +// eslint-disable-next-line import-x/no-nodejs-modules import EventEmitter from 'events'; import { v1 as random } from 'uuid'; @@ -782,7 +782,6 @@ export class SignatureController extends BaseController< const originalStatus = metadata.status; - // eslint-disable-next-line n/callback-return callback(metadata); statusChanged = metadata.status !== originalStatus; @@ -802,7 +801,6 @@ export class SignatureController extends BaseController< #updateState(callback: (state: SignatureControllerState) => void) { return this.update((state) => { - // eslint-disable-next-line n/callback-return, n/no-callback-literal callback(state as unknown as SignatureControllerState); const unapprovedRequests = Object.values(state.signatureRequests).filter( diff --git a/packages/transaction-controller/src/TransactionController.ts b/packages/transaction-controller/src/TransactionController.ts index ff7c60f7eaf..a92967f2105 100644 --- a/packages/transaction-controller/src/TransactionController.ts +++ b/packages/transaction-controller/src/TransactionController.ts @@ -47,7 +47,7 @@ import type { Hex } from '@metamask/utils'; import { add0x, hexToNumber } from '@metamask/utils'; import { Mutex } from 'async-mutex'; // This package purposefully relies on Node's EventEmitter module. -// eslint-disable-next-line import/no-nodejs-modules +// eslint-disable-next-line import-x/no-nodejs-modules import { EventEmitter } from 'events'; import { cloneDeep, mapValues, merge, pickBy, sortBy } from 'lodash'; import { v1 as random } from 'uuid'; @@ -3495,7 +3495,6 @@ export class TransactionController extends BaseController< const originalTransactionMeta = cloneDeep(transactionMeta); - // eslint-disable-next-line n/callback-return transactionMeta = callback(transactionMeta) ?? transactionMeta; if (skipValidation !== true) { diff --git a/packages/transaction-controller/src/helpers/GasFeePoller.ts b/packages/transaction-controller/src/helpers/GasFeePoller.ts index b4b87a94dc0..82ad6090fef 100644 --- a/packages/transaction-controller/src/helpers/GasFeePoller.ts +++ b/packages/transaction-controller/src/helpers/GasFeePoller.ts @@ -7,7 +7,7 @@ import type { NetworkClientId, Provider } from '@metamask/network-controller'; import type { Hex } from '@metamask/utils'; import { createModuleLogger } from '@metamask/utils'; // This package purposefully relies on Node's EventEmitter module. -// eslint-disable-next-line import/no-nodejs-modules +// eslint-disable-next-line import-x/no-nodejs-modules import EventEmitter from 'events'; import { projectLogger } from '../logger'; diff --git a/packages/transaction-controller/src/helpers/IncomingTransactionHelper.ts b/packages/transaction-controller/src/helpers/IncomingTransactionHelper.ts index 172f0d8c003..f8d97dd5c4c 100644 --- a/packages/transaction-controller/src/helpers/IncomingTransactionHelper.ts +++ b/packages/transaction-controller/src/helpers/IncomingTransactionHelper.ts @@ -1,7 +1,7 @@ import type { AccountsController } from '@metamask/accounts-controller'; import type { Hex } from '@metamask/utils'; // This package purposefully relies on Node's EventEmitter module. -// eslint-disable-next-line import/no-nodejs-modules +// eslint-disable-next-line import-x/no-nodejs-modules import EventEmitter from 'events'; import { incomingTransactionsLogger as log } from '../logger'; diff --git a/packages/transaction-controller/src/helpers/MethodDataHelper.ts b/packages/transaction-controller/src/helpers/MethodDataHelper.ts index 54dbdb6ab4a..2542cb67178 100644 --- a/packages/transaction-controller/src/helpers/MethodDataHelper.ts +++ b/packages/transaction-controller/src/helpers/MethodDataHelper.ts @@ -3,7 +3,7 @@ import { createModuleLogger } from '@metamask/utils'; import { Mutex } from 'async-mutex'; import { MethodRegistry } from 'eth-method-registry'; // This package purposefully relies on Node's EventEmitter module. -// eslint-disable-next-line import/no-nodejs-modules +// eslint-disable-next-line import-x/no-nodejs-modules import EventEmitter from 'events'; import { projectLogger } from '../logger'; diff --git a/packages/transaction-controller/src/helpers/PendingTransactionTracker.ts b/packages/transaction-controller/src/helpers/PendingTransactionTracker.ts index 5520fe3a717..557a5d7c302 100644 --- a/packages/transaction-controller/src/helpers/PendingTransactionTracker.ts +++ b/packages/transaction-controller/src/helpers/PendingTransactionTracker.ts @@ -5,7 +5,7 @@ import type { NetworkClientId, } from '@metamask/network-controller'; // This package purposefully relies on Node's EventEmitter module. -// eslint-disable-next-line import/no-nodejs-modules +// eslint-disable-next-line import-x/no-nodejs-modules import EventEmitter from 'events'; import { cloneDeep, merge } from 'lodash'; diff --git a/packages/user-operation-controller/src/UserOperationController.ts b/packages/user-operation-controller/src/UserOperationController.ts index 2c70bd80c66..eede19f0893 100644 --- a/packages/user-operation-controller/src/UserOperationController.ts +++ b/packages/user-operation-controller/src/UserOperationController.ts @@ -26,7 +26,7 @@ import { } from '@metamask/transaction-controller'; import { add0x } from '@metamask/utils'; // This package purposefully relies on Node's EventEmitter module. -// eslint-disable-next-line import/no-nodejs-modules +// eslint-disable-next-line import-x/no-nodejs-modules import EventEmitter from 'events'; import type { Patch } from 'immer'; import { cloneDeep } from 'lodash'; diff --git a/packages/user-operation-controller/src/helpers/PendingUserOperationTracker.ts b/packages/user-operation-controller/src/helpers/PendingUserOperationTracker.ts index 54fa8ce6023..551ac6736a1 100644 --- a/packages/user-operation-controller/src/helpers/PendingUserOperationTracker.ts +++ b/packages/user-operation-controller/src/helpers/PendingUserOperationTracker.ts @@ -8,7 +8,7 @@ import type { import { BlockTrackerPollingControllerOnly } from '@metamask/polling-controller'; import { createModuleLogger, type Hex } from '@metamask/utils'; // This package purposefully relies on Node's EventEmitter module. -// eslint-disable-next-line import/no-nodejs-modules +// eslint-disable-next-line import-x/no-nodejs-modules import EventEmitter from 'events'; import { projectLogger } from '../logger'; diff --git a/scripts/run-eslint.ts b/scripts/run-eslint.ts new file mode 100644 index 00000000000..1c70fac5c30 --- /dev/null +++ b/scripts/run-eslint.ts @@ -0,0 +1,293 @@ +import { ESLint } from 'eslint'; +import fs from 'fs'; +import path from 'path'; +import yargs from 'yargs'; + +const EXISTING_WARNINGS_FILE = path.resolve( + __dirname, + '../eslint-warning-thresholds.json', +); + +/** + * An object mapping rule IDs to their warning counts. + */ +type WarningCounts = Record; + +/** + * An object indicating the difference in warnings for a specific rule. + */ +type WarningComparison = { + /** The ID of the ESLint rule. */ + ruleId: string; + /** The previous count of warnings for the rule. */ + threshold: number; + /** The current count of warnings for the rule. */ + count: number; + /** The difference between the count and the threshold for the rule. */ + difference: number; +}; + +/** + * The warning severity of level of an ESLint rule. + */ +const WARNING = 1; + +// Run the script. +main().catch((error) => { + console.error(error); + process.exitCode = 1; +}); + +/** + * The entrypoint to this script. + */ +async function main() { + const { cache, fix, quiet } = parseCommandLineArguments(); + + const eslint = new ESLint({ cache, fix }); + const results = await runESLint(eslint, { fix, quiet }); + const hasErrors = results.some((result) => result.errorCount > 0); + + if (!quiet && !hasErrors) { + evaluateWarnings(results); + } +} + +/** + * Uses `yargs` to parse the arguments given to the script. + * + * @returns The parsed arguments. + */ +function parseCommandLineArguments() { + return yargs(process.argv.slice(2)) + .option('cache', { + type: 'boolean', + description: 'Cache results to speed up future runs', + default: false, + }) + .option('fix', { + type: 'boolean', + description: 'Automatically fix problems', + default: false, + }) + .option('quiet', { + type: 'boolean', + description: + 'Only report errors, disabling the warnings quality gate in the process', + default: false, + }) + .help().argv; +} + +/** + * Runs ESLint on the project files. + * + * @param eslint - The ESLint instance. + * @param options - The options for running ESLint. + * @param options.quiet - Whether to only report errors (true) or not (false). + * @param options.fix - Whether to automatically fix problems (true) or not + * (false). + * @returns A promise that resolves to the lint results. + */ +async function runESLint( + eslint: ESLint, + options: { quiet: boolean; fix: boolean }, +): Promise { + let results = await eslint.lintFiles(['.']); + const errorResults = ESLint.getErrorResults(results); + + if (errorResults.length > 0) { + process.exitCode = 1; + } + + if (options.quiet) { + results = errorResults; + } + + const formatter = await eslint.loadFormatter('stylish'); + const resultText = formatter.format(results); + console.log(resultText); + + if (options.fix) { + await ESLint.outputFixes(results); + } + + return results; +} + +/** + * This function represents the ESLint warnings quality gate, which will cause + * linting to pass or fail depending on how many new warnings have been + * produced. + * + * - If we have no record of warnings from a previous run, then we simply + * capture the new warnings in a file and continue. + * - If we have a record of warnings from a previous run and there are any + * changes to the number of warnings overall, then we list which ESLint rules + * had increases and decreases. If are were more warnings overall then we fail, + * otherwise we pass. + * + * @param results - The results of running ESLint. + */ +function evaluateWarnings(results: ESLint.LintResult[]) { + const warningThresholds = loadWarningThresholds(); + const warningCounts = getWarningCounts(results); + + if (Object.keys(warningThresholds).length === 0) { + console.log( + 'The following ESLint warnings were produced and will be captured as thresholds for future runs:\n', + ); + for (const [ruleId, count] of Object.entries(warningCounts)) { + console.log(`- ${ruleId}: ${count}`); + } + saveWarningThresholds(warningCounts); + } else { + const comparisons = compareWarnings(warningThresholds, warningCounts); + + const changes = comparisons.filter( + (comparison) => comparison.difference !== 0, + ); + const regressions = comparisons.filter( + (comparison) => comparison.difference > 0, + ); + + if (changes.length > 0) { + if (regressions.length > 0) { + console.log( + '🛑 New ESLint warnings have been introduced and need to be resolved for linting to pass:\n', + ); + process.exitCode = 1; + } else { + console.log( + 'The overall number of ESLint warnings have decreased, good work! ❤️ \n', + ); + // We are still seeing differences on CI when it comes to linting + // results. Never write the thresholds file in that case. + // eslint-disable-next-line n/no-process-env + if (!process.env.CI) { + saveWarningThresholds(warningCounts); + } + } + + for (const { ruleId, threshold, count, difference } of changes) { + console.log( + `- ${ruleId}: ${threshold} -> ${count} (${difference > 0 ? '+' : ''}${difference})`, + ); + } + } + } +} + +/** + * Loads previous warning counts from a file. + * + * @returns An object mapping rule IDs to their previous warning counts. + */ +function loadWarningThresholds(): WarningCounts { + if (fs.existsSync(EXISTING_WARNINGS_FILE)) { + const data = fs.readFileSync(EXISTING_WARNINGS_FILE, 'utf-8'); + return JSON.parse(data); + } + return {}; +} + +/** + * Saves current warning counts to a file so they can be used for a future run. + * + * @param warningCounts - An object mapping rule IDs to their current warning + * counts. + */ +function saveWarningThresholds(warningCounts: WarningCounts): void { + fs.writeFileSync( + EXISTING_WARNINGS_FILE, + `${JSON.stringify(warningCounts, null, 2)}\n`, + 'utf-8', + ); +} + +/** + * Given a list of results from an the ESLint run, counts the number of warnings + * produced per rule. + * + * @param results - The ESLint results. + * @returns An object mapping rule IDs to their warning counts, sorted by rule + * ID. + */ +function getWarningCounts(results: ESLint.LintResult[]): WarningCounts { + const warningCounts = results.reduce((acc, result) => { + for (const message of result.messages) { + if (message.severity === WARNING && message.ruleId) { + acc[message.ruleId] = (acc[message.ruleId] ?? 0) + 1; + } + } + return acc; + }, {} as WarningCounts); + + return Object.keys(warningCounts) + .sort(sortRules) + .reduce((sortedWarningCounts, key) => { + return { ...sortedWarningCounts, [key]: warningCounts[key] }; + }, {} as WarningCounts); +} + +/** + * Compares previous and current warning counts. + * + * @param warningThresholds - An object mapping rule IDs to the warning + * thresholds established in a previous run. + * @param warningCounts - An object mapping rule IDs to the current warning + * counts. + * @returns An array of objects indicating comparisons in warnings. + */ +function compareWarnings( + warningThresholds: WarningCounts, + warningCounts: WarningCounts, +): WarningComparison[] { + const ruleIds = Array.from( + new Set([...Object.keys(warningThresholds), ...Object.keys(warningCounts)]), + ); + return ruleIds + .map((ruleId) => { + const threshold = warningThresholds[ruleId] ?? 0; + const count = warningCounts[ruleId] ?? 0; + const difference = count - threshold; + return { ruleId, threshold, count, difference }; + }) + .sort((a, b) => sortRules(a.ruleId, b.ruleId)); +} + +/** + * Sorts rule IDs, ensuring that rules with namespaces appear before rules + * without. + * + * @param ruleIdA - The first rule ID. + * @param ruleIdB - The second rule ID. + * @returns A negative number if the first rule ID should come before the + * second, a positive number if the first should come _after_ the second, or 0 + * if they should stay where they are. + * @example + * ``` typescript + * sortRules( + * '@typescript-eslint/naming-convention', + * '@typescript-eslint/explicit-function-return-type' + * ) //=> 1 (sort A after B) + * sortRules( + * 'explicit-function-return-type', + * '@typescript-eslint/naming-convention' + * ) //=> 1 (sort A after B) + */ +function sortRules(ruleIdA: string, ruleIdB: string): number { + const [namespaceA, ruleA] = ruleIdA.includes('/') + ? ruleIdA.split('/') + : ['', ruleIdA]; + const [namespaceB, ruleB] = ruleIdB.includes('/') + ? ruleIdB.split('/') + : ['', ruleIdB]; + if (namespaceA && !namespaceB) { + return -1; + } + if (!namespaceA && namespaceB) { + return 1; + } + return namespaceA.localeCompare(namespaceB) || ruleA.localeCompare(ruleB); +} diff --git a/yarn.config.cjs b/yarn.config.cjs index b802565a6a0..aa8f334e172 100644 --- a/yarn.config.cjs +++ b/yarn.config.cjs @@ -1,4 +1,9 @@ -// @ts-check +/* @ts-check */ +// The ESLint JSDoc plugin usually disables this rule for TypeScript files, +// but for JavaScript files we are typechecking, we need to disable it manually. +// See: +/* eslint-disable jsdoc/no-undefined-types */ + // This file is used to define, among other configuration, rules that Yarn will // execute when you run `yarn constraints`. These rules primarily check the // manifests of each package in the monorepo to ensure they follow a standard diff --git a/yarn.lock b/yarn.lock index 6d41619277b..e93a6ba3fc2 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2550,11 +2550,14 @@ __metadata: "@types/jest": "npm:^27.4.1" bignumber.js: "npm:^9.1.2" bn.js: "npm:^5.2.1" + cockatiel: "npm:^3.1.2" deepmerge: "npm:^4.2.2" eth-ens-namehash: "npm:^2.0.8" fast-deep-equal: "npm:^3.1.3" jest: "npm:^27.5.1" + jest-environment-jsdom: "npm:^27.5.1" nock: "npm:^13.3.1" + sinon: "npm:^9.2.4" ts-jest: "npm:^27.1.4" typedoc: "npm:^0.24.8" typedoc-plugin-missing-exports: "npm:^2.0.0" @@ -3413,7 +3416,7 @@ __metadata: "@metamask/base-controller": "npm:^7.1.1" "@metamask/controller-utils": "npm:^11.4.5" "@metamask/keyring-controller": "npm:^19.0.3" - "@metamask/profile-sync-controller": "npm:^4.0.0" + "@metamask/profile-sync-controller": "npm:^4.1.0" "@metamask/utils": "npm:^11.0.1" "@types/jest": "npm:^27.4.1" "@types/readable-stream": "npm:^2.3.0" @@ -3594,7 +3597,7 @@ __metadata: languageName: unknown linkType: soft -"@metamask/profile-sync-controller@npm:^4.0.0, @metamask/profile-sync-controller@workspace:packages/profile-sync-controller": +"@metamask/profile-sync-controller@npm:^4.1.0, @metamask/profile-sync-controller@workspace:packages/profile-sync-controller": version: 0.0.0-use.local resolution: "@metamask/profile-sync-controller@workspace:packages/profile-sync-controller" dependencies: @@ -5799,11 +5802,11 @@ __metadata: linkType: hard "acorn-walk@npm:^8.1.1": - version: 8.3.3 - resolution: "acorn-walk@npm:8.3.3" + version: 8.3.4 + resolution: "acorn-walk@npm:8.3.4" dependencies: acorn: "npm:^8.11.0" - checksum: 10/59701dcb7070679622ba8e9c7f37577b4935565747ca0fd7c1c3ad30b3f1b1b008276282664e323b5495eb49f77fa12d3816fd06dc68e18f90fbebe759f71450 + checksum: 10/871386764e1451c637bb8ab9f76f4995d408057e9909be6fb5ad68537ae3375d85e6a6f170b98989f44ab3ff6c74ad120bc2779a3d577606e7a0cd2b4efcaf77 languageName: node linkType: hard @@ -10948,9 +10951,9 @@ __metadata: linkType: hard "nwsapi@npm:^2.2.0": - version: 2.2.12 - resolution: "nwsapi@npm:2.2.12" - checksum: 10/172119e9ef492467ebfb337f9b5fd12a94d2b519377cde3f6ec2f74a86f6d5c00ef3873539bed7142f908ffca4e35383179be2319d04a563071d146bfa3f1673 + version: 2.2.16 + resolution: "nwsapi@npm:2.2.16" + checksum: 10/1e5e086cdd4ca4a45f414d37f49bf0ca81d84ed31c6871ac68f531917d2910845db61f77c6d844430dc90fda202d43fce9603024e74038675de95229eb834dba languageName: node linkType: hard