From 236ec07ecbcfe7a03a27e090173cb7cc59cf100f Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Mon, 13 Jan 2025 21:16:32 +0100 Subject: [PATCH 01/16] chore: remove `@metamask/chain-controller` (#5137) ## Explanation This controller has never been really used. Initially it was added to work in pair with this repository: - https://github.com/MetaMask/accounts-chain-api/ But this Chain API has been revamped and has finally been merged with the existing Keyring API with the addition of the `getAccountBalances` method which replaces this `getBalances` method from the Chain API. ## References N/A ## Changelog N/A ## Checklist - [ ] I've updated the test suite for new or updated code as appropriate - [ ] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [ ] I've highlighted breaking changes using the "BREAKING" category above as appropriate - [ ] I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes --- .github/CODEOWNERS | 3 - README.md | 3 - packages/chain-controller/CHANGELOG.md | 103 ---------- packages/chain-controller/LICENSE | 20 -- packages/chain-controller/README.md | 102 --------- packages/chain-controller/jest.config.js | 26 --- packages/chain-controller/package.json | 84 -------- .../src/ChainController.test.ts | 152 -------------- .../chain-controller/src/ChainController.ts | 194 ------------------ .../src/SnapChainProviderClient.test.ts | 79 ------- .../src/SnapChainProviderClient.ts | 45 ---- .../src/SnapHandlerClient.test.ts | 40 ---- .../chain-controller/src/SnapHandlerClient.ts | 115 ----------- packages/chain-controller/src/index.ts | 1 - packages/chain-controller/tsconfig.build.json | 15 -- packages/chain-controller/tsconfig.json | 12 -- packages/chain-controller/typedoc.json | 7 - teams.json | 1 - tsconfig.build.json | 1 - yarn.lock | 40 ---- 20 files changed, 1043 deletions(-) delete mode 100644 packages/chain-controller/CHANGELOG.md delete mode 100644 packages/chain-controller/LICENSE delete mode 100644 packages/chain-controller/README.md delete mode 100644 packages/chain-controller/jest.config.js delete mode 100644 packages/chain-controller/package.json delete mode 100644 packages/chain-controller/src/ChainController.test.ts delete mode 100644 packages/chain-controller/src/ChainController.ts delete mode 100644 packages/chain-controller/src/SnapChainProviderClient.test.ts delete mode 100644 packages/chain-controller/src/SnapChainProviderClient.ts delete mode 100644 packages/chain-controller/src/SnapHandlerClient.test.ts delete mode 100644 packages/chain-controller/src/SnapHandlerClient.ts delete mode 100644 packages/chain-controller/src/index.ts delete mode 100644 packages/chain-controller/tsconfig.build.json delete mode 100644 packages/chain-controller/tsconfig.json delete mode 100644 packages/chain-controller/typedoc.json diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index f9e06177f48..cec253674b6 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -9,7 +9,6 @@ ## Accounts Team /packages/accounts-controller @MetaMask/accounts-engineers /packages/keyring-controller @MetaMask/accounts-engineers -/packages/chain-controller @MetaMask/accounts-engineers ## Assets Team /packages/assets-controllers @MetaMask/metamask-assets @@ -71,8 +70,6 @@ /packages/approval-controller/CHANGELOG.md @MetaMask/confirmations @MetaMask/wallet-framework-engineers /packages/assets-controllers/package.json @MetaMask/metamask-assets @MetaMask/wallet-framework-engineers /packages/assets-controllers/CHANGELOG.md @MetaMask/metamask-assets @MetaMask/wallet-framework-engineers -/packages/chain-controller/package.json @MetaMask/accounts-engineers @MetaMask/wallet-framework-engineers -/packages/chain-controller/CHANGELOG.md @MetaMask/accounts-engineers @MetaMask/wallet-framework-engineers /packages/ens-controller/package.json @MetaMask/confirmations @MetaMask/wallet-framework-engineers /packages/ens-controller/CHANGELOG.md @MetaMask/confirmations @MetaMask/wallet-framework-engineers /packages/gas-fee-controller/package.json @MetaMask/confirmations @MetaMask/wallet-framework-engineers diff --git a/README.md b/README.md index 9dff752fea5..895a7ebc694 100644 --- a/README.md +++ b/README.md @@ -27,7 +27,6 @@ Each package in this repository has its own README where you can find installati - [`@metamask/assets-controllers`](packages/assets-controllers) - [`@metamask/base-controller`](packages/base-controller) - [`@metamask/build-utils`](packages/build-utils) -- [`@metamask/chain-controller`](packages/chain-controller) - [`@metamask/composable-controller`](packages/composable-controller) - [`@metamask/controller-utils`](packages/controller-utils) - [`@metamask/ens-controller`](packages/ens-controller) @@ -70,7 +69,6 @@ linkStyle default opacity:0.5 assets_controllers(["@metamask/assets-controllers"]); base_controller(["@metamask/base-controller"]); build_utils(["@metamask/build-utils"]); - chain_controller(["@metamask/chain-controller"]); composable_controller(["@metamask/composable-controller"]); controller_utils(["@metamask/controller-utils"]); ens_controller(["@metamask/ens-controller"]); @@ -112,7 +110,6 @@ linkStyle default opacity:0.5 assets_controllers --> network_controller; assets_controllers --> preferences_controller; base_controller --> json_rpc_engine; - chain_controller --> base_controller; composable_controller --> base_controller; composable_controller --> json_rpc_engine; ens_controller --> base_controller; diff --git a/packages/chain-controller/CHANGELOG.md b/packages/chain-controller/CHANGELOG.md deleted file mode 100644 index 8786824ff60..00000000000 --- a/packages/chain-controller/CHANGELOG.md +++ /dev/null @@ -1,103 +0,0 @@ -# Changelog - -All notable changes to this project will be documented in this file. - -The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), -and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). - -## [Unreleased] - -### Changed - -- Bump `@metamask/base-controller` from `^7.0.0` to `^7.1.0` ([#5079](https://github.com/MetaMask/core/pull/5079)) - -## [0.2.2] - -### Changed - -- Remove use of `@metamask/keyring-api` ([#4695](https://github.com/MetaMask/core/pull/4695)) - - `@metamask/providers` and `webextension-polyfill` peer dependencies are no longer required. -- Use new `@metamask/keyring-internal-api@^1.0.0` ([#4695](https://github.com/MetaMask/core/pull/4695)) - - This package has been split out from the Keyring API. Its types are compatible with the `@metamask/keyring-api` package used previously. -- Use new `@metamask/keyring-utils@^1.0.0` ([#4695](https://github.com/MetaMask/core/pull/4695)) - -## [0.2.1] - -### Fixed - -- Make implicit peer dependencies explicit ([#4974](https://github.com/MetaMask/core/pull/4974)) - - Add the following packages as peer dependencies of this package to satisfy peer dependency requirements from other dependencies: - - `@metamask/providers` `^18.1.0` (required by `@metamask/keyring-api`) - - `webextension-polyfill` `^0.10.0 || ^0.11.0 || ^0.12.0` (required by `@metamask/providers`) - - These dependencies really should be present in projects that consume this package (e.g. MetaMask clients), and this change ensures that they now are. - - Furthermore, we are assuming that clients already use these dependencies, since otherwise it would be impossible to consume this package in its entirety or even create a working build. Hence, the addition of these peer dependencies is really a formality and should not be breaking. - -## [0.2.0] - -### Changed - -- **BREAKING:** Bump `@metamask/keyring-api` from `^8.1.3` to `^10.1.0` ([#4948](https://github.com/MetaMask/core/pull/4948)) - - If you are depending on `@metamask/providers` directly, you will need to upgrade to 18.1.0. -- Bump `@metamask/snaps-utils` from `^4.3.6` to `^8.3.0` ([#4948](https://github.com/MetaMask/core/pull/4948)) -- Bump `@metamask/snaps-sdk` from `^6.5.0` to `^6.7.0` ([#4948](https://github.com/MetaMask/core/pull/4948)) -- Bump `@metamask/snaps-controllers` from `^9.7.0`to `^9.10.0` ([#4948](https://github.com/MetaMask/core/pull/4948)) -- Bump `@metamask/utils` from `^9.1.0` to `^10.0.0` ([#4831](https://github.com/MetaMask/core/pull/4831)) - -## [0.1.3] - -### Changed - -- Bump accounts related packages ([#4713](https://github.com/MetaMask/core/pull/4713)), ([#4728](https://github.com/MetaMask/core/pull/4728)) - - Those packages are now built slightly differently and are part of the [accounts monorepo](https://github.com/MetaMask/accounts). - - Bump `@metamask/keyring-api` from `^8.1.0` to `^8.1.4` - -## [0.1.2] - -### Changed - -- Bump `@metamask/keyring-api` from `^8.0.1` to `^8.1.0` ([#4594](https://github.com/MetaMask/core/pull/4594)) -- Bump TypeScript from `~4.9.5` to `~5.2.2` and set `moduleResolution` option to `Node16` ([#3645](https://github.com/MetaMask/core/pull/3645), [#4576](https://github.com/MetaMask/core/pull/4576), [#4584](https://github.com/MetaMask/core/pull/4584)) - -### Fixed - -- Produce and export ESM-compatible TypeScript type declaration files in addition to CommonJS-compatible declaration files ([#4648](https://github.com/MetaMask/core/pull/4648)) - - Previously, this package shipped with only one variant of type declaration - files, and these files were only CommonJS-compatible, and the `exports` - field in `package.json` linked to these files. This is an anti-pattern and - was rightfully flagged by the - ["Are the Types Wrong?"](https://arethetypeswrong.github.io/) tool as - ["masquerading as CJS"](https://github.com/arethetypeswrong/arethetypeswrong.github.io/blob/main/docs/problems/FalseCJS.md). - All of the ATTW checks now pass. -- Remove chunk files ([#4648](https://github.com/MetaMask/core/pull/4648)). - - Previously, the build tool we used to generate JavaScript files extracted - common code to "chunk" files. While this was intended to make this package - more tree-shakeable, it also made debugging more difficult for our - development teams. These chunk files are no longer present. - -## [0.1.1] - -### Changed - -- Upgrade TypeScript version to `~5.0.4` and set `moduleResolution` option to `Node16` ([#3645](https://github.com/MetaMask/core/pull/3645)) -- Bump `@metamask/base-controller` from `^6.0.0` to `^6.0.2` ([#4517](https://github.com/MetaMask/core/pull/4517), [#4544](https://github.com/MetaMask/core/pull/4544)) -- Bump `@metamask/chain-api` from `^0.0.1` to `^0.1.0` ([#3645](https://github.com/MetaMask/core/pull/3645)) -- Bump `@metamask/keyring-api` from `^8.0.0` to `^8.0.1` ([#3645](https://github.com/MetaMask/core/pull/3645)) -- Bump `@metamask/snaps-controllers` from `^8.1.1` to `^9.3.1` ([#3645](https://github.com/MetaMask/core/pull/3645), [#4547](https://github.com/MetaMask/core/pull/4547)) -- Bump `@metamask/snaps-sdk` from `^4.2.0` to `^6.1.1` ([#3645](https://github.com/MetaMask/core/pull/3645), [#4547](https://github.com/MetaMask/core/pull/4547)) -- Bump `@metamask/snaps-utils` from `^7.4.0` to `^7.8.1` ([#3645](https://github.com/MetaMask/core/pull/3645), [#4547](https://github.com/MetaMask/core/pull/4547)) -- Bump `@metamask/utils` from `^8.3.0` to `^9.1.0` ([#4516](https://github.com/MetaMask/core/pull/4516), [#4529](https://github.com/MetaMask/core/pull/4529)) - -## [0.1.0] - -### Changed - -- Initial release - -[Unreleased]: https://github.com/MetaMask/core/compare/@metamask/chain-controller@0.2.2...HEAD -[0.2.2]: https://github.com/MetaMask/core/compare/@metamask/chain-controller@0.2.1...@metamask/chain-controller@0.2.2 -[0.2.1]: https://github.com/MetaMask/core/compare/@metamask/chain-controller@0.2.0...@metamask/chain-controller@0.2.1 -[0.2.0]: https://github.com/MetaMask/core/compare/@metamask/chain-controller@0.1.3...@metamask/chain-controller@0.2.0 -[0.1.3]: https://github.com/MetaMask/core/compare/@metamask/chain-controller@0.1.2...@metamask/chain-controller@0.1.3 -[0.1.2]: https://github.com/MetaMask/core/compare/@metamask/chain-controller@0.1.1...@metamask/chain-controller@0.1.2 -[0.1.1]: https://github.com/MetaMask/core/compare/@metamask/chain-controller@0.1.0...@metamask/chain-controller@0.1.1 -[0.1.0]: https://github.com/MetaMask/core/releases/tag/@metamask/chain-controller@0.1.0 diff --git a/packages/chain-controller/LICENSE b/packages/chain-controller/LICENSE deleted file mode 100644 index 6f8bff03fc4..00000000000 --- a/packages/chain-controller/LICENSE +++ /dev/null @@ -1,20 +0,0 @@ -MIT License - -Copyright (c) 2024 MetaMask - -Permission is hereby granted, free of charge, to any person obtaining a copy -of this software and associated documentation files (the "Software"), to deal -in the Software without restriction, including without limitation the rights -to use, copy, modify, merge, publish, distribute, sublicense, and/or sell -copies of the Software, and to permit persons to whom the Software is -furnished to do so, subject to the following conditions: - -The above copyright notice and this permission notice shall be included in all -copies or substantial portions of the Software. - -THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR -IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, -FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE -AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER -LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, -OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE diff --git a/packages/chain-controller/README.md b/packages/chain-controller/README.md deleted file mode 100644 index b5d26f701a7..00000000000 --- a/packages/chain-controller/README.md +++ /dev/null @@ -1,102 +0,0 @@ -# `@metamask/chain-controller` - -Manages chain-agnostic providers implementing the chain API. - -## Installation - -`yarn add @metamask/chain-controller` - -or - -`npm install @metamask/chain-controller` - -## Contributing - -This package is part of a monorepo. Instructions for contributing can be found in the [monorepo README](https://github.com/MetaMask/core#readme). - -## Description - -This controller is responsible to "bridge" Snaps that implements the [Chain API] and a MetaMask -client. - -The controller maps a "chain provider" to its CAIP-2 (chain id) identifier -(named `scope` in the [Chain API]). -The MetaMask client can then use an external source (outside for current controllers that -only support EVM networks) to fetch information from a non-EVM network. - -The controller itself also implements the [Chain API]. Its uses the `scope` (that is always -required for any methods) to identify the chain provider and then forward the method call to this -provider. - -The calls are dispatched through the `SnapsController:handleRequest`'s action. - -Here's the high-level flow when invoking a [Chain API]'s method: - -```mermaid -%%{init: {"flowchart": {"htmlLabels": false}}}%% -sequenceDiagram - autonumber - - participant MetaMask - participant ChainController - participant Snap - - - box Metamask Client - participant MetaMask - participant ChainController - end - - MetaMask -> Snap: Retrieves Snap ID - MetaMask ->> ChainController: registerProvider(scope, snapId) - note over MetaMask,Snap: Provider must be registered first - - - MetaMask ->> ChainController: chain_method(scope, ...) - activate MetaMask - - alt If there is a chain client for this scope - ChainController ->> ChainController: client = getProviderClient(scope) - - ChainController ->> Snap: client.chain_method(scope, ...) - Snap ->> Snap: Process Chain API request - Snap -->> ChainController: Chain API response - end - - ChainController -->> MetaMask: Chain API Response - - deactivate MetaMask -``` - -Clients and Snap interactions: - -```mermaid -%%{init: {"flowchart": {"htmlLabels": false}}}%% -sequenceDiagram - autonumber - - participant ChainController - participant SnapChainProviderClient - participant SnapHandlerClient - participant SnapsController - - ChainController ->> SnapChainProviderClient: chain_method(scope, ...) - activate ChainController - - note over SnapChainProviderClient: This client also implements the Chain API methods - - SnapChainProviderClient ->> SnapHandlerClient: submitRequest({ snapId, origin, handler, request }) - - SnapHandlerClient ->> SnapsController: :handleRequest(...) - SnapsController -->> SnapHandlerClient: Response - - SnapHandlerClient -->> SnapChainProviderClient: Response - - SnapChainProviderClient -->> ChainController: Response - - deactivate ChainController -``` - -## Resources - -- [Chain API](https://github.com/MetaMask/chain-api/) diff --git a/packages/chain-controller/jest.config.js b/packages/chain-controller/jest.config.js deleted file mode 100644 index ca084133399..00000000000 --- a/packages/chain-controller/jest.config.js +++ /dev/null @@ -1,26 +0,0 @@ -/* - * For a detailed explanation regarding each configuration property and type check, visit: - * https://jestjs.io/docs/configuration - */ - -const merge = require('deepmerge'); -const path = require('path'); - -const baseConfig = require('../../jest.config.packages'); - -const displayName = path.basename(__dirname); - -module.exports = merge(baseConfig, { - // The display name when running multiple projects - displayName, - - // An object that configures minimum threshold enforcement for coverage results - coverageThreshold: { - global: { - branches: 100, - functions: 100, - lines: 100, - statements: 100, - }, - }, -}); diff --git a/packages/chain-controller/package.json b/packages/chain-controller/package.json deleted file mode 100644 index 5e116fb01e0..00000000000 --- a/packages/chain-controller/package.json +++ /dev/null @@ -1,84 +0,0 @@ -{ - "name": "@metamask/chain-controller", - "version": "0.2.2", - "description": "Manages chain-agnostic providers", - "keywords": [ - "MetaMask", - "Ethereum" - ], - "homepage": "https://github.com/MetaMask/core/tree/main/packages/chain-controller#readme", - "bugs": { - "url": "https://github.com/MetaMask/core/issues" - }, - "repository": { - "type": "git", - "url": "https://github.com/MetaMask/core.git" - }, - "license": "MIT", - "sideEffects": false, - "exports": { - ".": { - "import": { - "types": "./dist/index.d.mts", - "default": "./dist/index.mjs" - }, - "require": { - "types": "./dist/index.d.cts", - "default": "./dist/index.cjs" - } - }, - "./package.json": "./package.json" - }, - "main": "./dist/index.cjs", - "types": "./dist/index.d.cts", - "files": [ - "dist/" - ], - "scripts": { - "build": "ts-bridge --project tsconfig.build.json --verbose --clean --no-references", - "build:docs": "typedoc", - "changelog:update": "../../scripts/update-changelog.sh @metamask/chain-controller", - "changelog:validate": "../../scripts/validate-changelog.sh @metamask/chain-controller", - "publish:preview": "yarn npm publish --tag preview", - "since-latest-release": "../../scripts/since-latest-release.sh", - "test": "NODE_OPTIONS=--experimental-vm-modules jest --reporters=jest-silent-reporter", - "test:clean": "NODE_OPTIONS=--experimental-vm-modules jest --clearCache", - "test:verbose": "NODE_OPTIONS=--experimental-vm-modules jest --verbose", - "test:watch": "NODE_OPTIONS=--experimental-vm-modules jest --watch" - }, - "dependencies": { - "@metamask/base-controller": "^7.1.1", - "@metamask/chain-api": "^0.1.0", - "@metamask/keyring-internal-api": "^2.0.0", - "@metamask/keyring-utils": "^1.0.0", - "@metamask/snaps-controllers": "^9.10.0", - "@metamask/snaps-sdk": "^6.7.0", - "@metamask/snaps-utils": "^8.3.0", - "@metamask/utils": "^11.0.1", - "uuid": "^8.3.2" - }, - "devDependencies": { - "@metamask/auto-changelog": "^3.4.4", - "@metamask/providers": "^18.1.1", - "@types/jest": "^27.4.1", - "@types/readable-stream": "^2.3.0", - "deepmerge": "^4.2.2", - "jest": "^27.5.1", - "ts-jest": "^27.1.4", - "typedoc": "^0.24.8", - "typedoc-plugin-missing-exports": "^2.0.0", - "typescript": "~5.2.2", - "webextension-polyfill": "^0.12.0" - }, - "peerDependencies": { - "@metamask/providers": "^18.1.0", - "webextension-polyfill": "^0.10.0 || ^0.11.0 || ^0.12.0" - }, - "engines": { - "node": "^18.18 || >=20" - }, - "publishConfig": { - "access": "public", - "registry": "https://registry.npmjs.org/" - } -} diff --git a/packages/chain-controller/src/ChainController.test.ts b/packages/chain-controller/src/ChainController.test.ts deleted file mode 100644 index 8fe506561f7..00000000000 --- a/packages/chain-controller/src/ChainController.test.ts +++ /dev/null @@ -1,152 +0,0 @@ -import { ControllerMessenger } from '@metamask/base-controller'; -import type { InternalAccount } from '@metamask/keyring-internal-api'; -import type { SnapId } from '@metamask/snaps-sdk'; - -import type { AllowedActions, ChainControllerActions } from './ChainController'; -import { ChainController } from './ChainController'; - -const snapId = 'local:localhost:3000' as SnapId; - -const address = 'bc1qrp0yzgkf8rawkuvdlhnjfj2fnjwm0m8727kgah'; -const scope = 'bip122:000000000019d6689c085ae165831e93'; -const asset = `${scope}/slip44:0`; - -const name = 'ChainController'; - -/** - * Constructs the messenger restricted to ChainController actions and events. - * - * @param actions - A map of actions and their mocked handlers. - * @returns A restricted controller messenger. - */ -function getRestrictedMessenger( - // We could just use a callback here, but having an actions makes the mapping more explicit - actions?: Record, -) { - const controllerMessenger = new ControllerMessenger< - ChainControllerActions | AllowedActions, - never - >(); - - if (actions) { - controllerMessenger.registerActionHandler( - 'SnapController:handleRequest', - actions['SnapController:handleRequest'], - ); - } - - return controllerMessenger.getRestricted( - { - name, - allowedActions: ['SnapController:handleRequest'], - allowedEvents: [], - }, - ); -} - -describe('ChainController', () => { - describe('registerProvider', () => { - it('returns false if there is no known provider', () => { - const messenger = getRestrictedMessenger(); - const controller = new ChainController({ - messenger, - }); - - expect(controller.hasProviderFor(scope)).toBe(false); - }); - - it('registers a chain provider for a given scope', () => { - const messenger = getRestrictedMessenger(); - const controller = new ChainController({ - messenger, - }); - - expect(controller.registerProvider(scope, snapId)).toBeDefined(); - expect(controller.hasProviderFor(scope)).toBe(true); - }); - - it('fails to register another provider for an existing scope', () => { - const messenger = getRestrictedMessenger(); - const controller = new ChainController({ - messenger, - }); - const anotherSnapId = 'local:localhost:4000' as SnapId; - - expect(controller.registerProvider(scope, snapId)).toBeDefined(); - expect(() => controller.registerProvider(scope, anotherSnapId)).toThrow( - `Found an already existing provider for scope: "${scope}"`, - ); - }); - }); - - describe('getBalances', () => { - const response = { - balances: { - [address]: { - [asset]: { - amount: '70.02255139', - }, - }, - }, - }; - - it('is successful', async () => { - const handleRequest = jest.fn(); - const messenger = getRestrictedMessenger({ - 'SnapController:handleRequest': handleRequest, - }); - const controller = new ChainController({ - messenger, - }); - - const provider = controller.registerProvider(scope, snapId); - const providerSpy = jest.spyOn(provider, 'getBalances'); - - handleRequest.mockResolvedValue(response); - const result = await controller.getBalances(scope, [address], [asset]); - - expect(providerSpy).toHaveBeenCalledWith(scope, [address], [asset]); - expect(result).toStrictEqual(response); - }); - - it('is successful (getBalancesFromAccount)', async () => { - const handleRequest = jest.fn(); - const messenger = getRestrictedMessenger({ - 'SnapController:handleRequest': handleRequest, - }); - const controller = new ChainController({ - messenger, - }); - - const provider = controller.registerProvider(scope, snapId); - const providerSpy = jest.spyOn(provider, 'getBalances'); - - const account = { - address, - } as unknown as InternalAccount; - - handleRequest.mockResolvedValue(response); - const result = await controller.getBalancesFromAccount(scope, account, [ - asset, - ]); - - expect(providerSpy).toHaveBeenCalledWith(scope, [address], [asset]); - expect(result).toStrictEqual(response); - }); - }); - - describe('hasProviderFor', () => { - it('fails if not provider is registered', async () => { - const messenger = getRestrictedMessenger(); - const controller = new ChainController({ - messenger, - }); - - // We do not register any provider - - await expect( - async () => await controller.getBalances(scope, [address], [asset]), - ).rejects.toThrow(`No Chain provider found for scope: "${scope}"`); - }); - }); -}); diff --git a/packages/chain-controller/src/ChainController.ts b/packages/chain-controller/src/ChainController.ts deleted file mode 100644 index 4e262bd053c..00000000000 --- a/packages/chain-controller/src/ChainController.ts +++ /dev/null @@ -1,194 +0,0 @@ -import type { - ControllerGetStateAction, - ControllerStateChangeEvent, - RestrictedControllerMessenger, -} from '@metamask/base-controller'; -import { BaseController } from '@metamask/base-controller'; -import type { CaipAssetType, BalancesResult, Chain } from '@metamask/chain-api'; -import type { InternalAccount } from '@metamask/keyring-internal-api'; -import type { HandleSnapRequest as SnapControllerHandleSnapRequestAction } from '@metamask/snaps-controllers'; -import type { SnapId } from '@metamask/snaps-sdk'; -import type { CaipChainId } from '@metamask/utils'; - -import { SnapChainProviderClient } from './SnapChainProviderClient'; -import { SnapHandlerClient } from './SnapHandlerClient'; - -const controllerName = 'ChainController'; - -export type ChainControllerState = Record; - -export type ChainControllerGetStateAction = ControllerGetStateAction< - typeof controllerName, - ChainControllerState ->; - -export type AllowedActions = SnapControllerHandleSnapRequestAction; - -export type ChainControllerActions = never; - -export type ChainControllerChangeEvent = ControllerStateChangeEvent< - typeof controllerName, - ChainControllerState ->; - -export type AllowedEvents = ChainControllerEvents; - -export type ChainControllerEvents = ChainControllerChangeEvent; - -export type ChainControllerMessenger = RestrictedControllerMessenger< - typeof controllerName, - ChainControllerActions | AllowedActions, - ChainControllerEvents | AllowedEvents, - AllowedActions['type'], - AllowedEvents['type'] ->; - -const defaultState: ChainControllerState = {}; - -/** - * Controller that manages chain-agnostic providers throught the chain API. - */ -export class ChainController - extends BaseController< - typeof controllerName, - ChainControllerState, - ChainControllerMessenger - > - implements Chain -{ - #providers: Record; - - /** - * Constructor for ChainController. - * - * @param options - The controller options. - * @param options.messenger - The messenger object. - * @param options.state - Initial state to set on this controller - */ - constructor({ - messenger, - state = {}, - }: { - messenger: ChainControllerMessenger; - state?: ChainControllerState; - }) { - super({ - messenger, - name: controllerName, - metadata: {}, - state: { - ...defaultState, - ...state, - }, - }); - - this.#providers = {}; - - this.#registerMessageHandlers(); - } - - /** - * Get a SnapChainProviderClient for a given scope. - * - * @private - * @param scope - CAIP-2 chain ID. - * @throws If no chain provider has been registered for this scope. - * @returns The associated SnapChainProviderClient. - */ - #getProviderClient(scope: CaipChainId): SnapChainProviderClient { - if (scope in this.#providers) { - return this.#providers[scope]; - } - - const error = `No Chain provider found for scope: "${scope}"`; - console.error(error, this.#providers); - throw new Error(error); - } - - /** - * Fetches asset balances for each given accounts. - * - * @param scope - CAIP-2 chain ID that must compatible with `accounts`. - * @param accounts - Accounts (addresses). - * @param assets - List of CAIP-19 asset identifiers to fetch balances from. - * @returns Assets balances for each accounts. - */ - getBalances = async ( - scope: CaipChainId, - accounts: string[], - assets: CaipAssetType[], - ): Promise => { - return await this.#getProviderClient(scope).getBalances( - scope, - accounts, - assets, - ); - }; - - /** - * Fetches asset balances for a given internal account. - * - * @param scope - CAIP-2 chain ID that must compatible with `accounts`. - * @param account - The internal account. - * @param assets - List of CAIP-19 asset identifiers to fetch balances from. - * @returns Assets balances for the internal accounts. - */ - getBalancesFromAccount = async ( - scope: CaipChainId, - account: InternalAccount, - assets: CaipAssetType[], - ): Promise => { - return this.getBalances(scope, [account.address], assets); - }; - - /** - * Checks whether a chain provider has been registered for a given scope. - * - * @param scope - CAIP-2 chain ID. - * @returns True if there is a registerd provider, false otherwise. - */ - hasProviderFor(scope: CaipChainId): boolean { - return scope in this.#providers; - } - - /** - * Registers a Snap chain provider for a given scope. - * - * @param scope - CAIP-2 chain ID. - * @param snapId - Snap ID that implements the Chain API methods. - * @returns A SnapChainProviderClient for this Snap. - */ - registerProvider( - scope: CaipChainId, - snapId: SnapId, - ): SnapChainProviderClient { - // TODO: Should this be idempotent? - const client = new SnapHandlerClient({ - snapId, - handler: (request) => { - return this.messagingSystem.call( - 'SnapController:handleRequest', - request, - ); - }, - }); - const provider = new SnapChainProviderClient(client); - - if (this.hasProviderFor(scope)) { - // For now, we avoid this to make sure no other provider can replace the existings ones! - throw new Error( - `Found an already existing provider for scope: "${scope}"`, - ); - } - this.#providers[scope] = provider; - return provider; - } - - /** - * Registers message handlers for the ChainController. - * @private - */ - #registerMessageHandlers() { - // TODO - } -} diff --git a/packages/chain-controller/src/SnapChainProviderClient.test.ts b/packages/chain-controller/src/SnapChainProviderClient.test.ts deleted file mode 100644 index c93aab45c12..00000000000 --- a/packages/chain-controller/src/SnapChainProviderClient.test.ts +++ /dev/null @@ -1,79 +0,0 @@ -import { ChainRpcMethod } from '@metamask/chain-api'; -import type { SnapId } from '@metamask/snaps-sdk'; -import { HandlerType } from '@metamask/snaps-utils'; -import type { Json } from '@metamask/utils'; - -import { SnapChainProviderClient } from './SnapChainProviderClient'; -import { SnapHandlerClient } from './SnapHandlerClient'; - -const snapId = 'local:localhost:3000' as SnapId; - -/** - * Builds a Snap chain API request. - * - * @param request - Chain API request object. - * @param request.method - Chain API method to be called. - * @param request.params - Chain API parameters. - * @returns The Snap chain API request object. - */ -function makeRequest({ method, params }: { method: string; params: Json }) { - return { - snapId, - origin: 'metamask', - handler: HandlerType.OnRpcRequest, - request: { - id: expect.any(String), - jsonrpc: '2.0', - method, - params, - }, - }; -} - -/** - * Constructs a Snap handler client. - * - * @param handler - Snap request handler - * @returns A Snap handler client. - */ -function getSnapHandlerClient(handler: jest.Mock) { - return new SnapHandlerClient({ - handler, - snapId, - }); -} - -describe('SnapChainProviderClient', () => { - describe('getBalances', () => { - it('dispatch chain_getBalances', async () => { - const handler = jest.fn(); - const client = new SnapChainProviderClient(getSnapHandlerClient(handler)); - const address = 'bc1qrp0yzgkf8rawkuvdlhnjfj2fnjwm0m8727kgah'; - const scope = 'bip122:000000000019d6689c085ae165831e93'; - const asset = `${scope}/asset:0`; - const request = makeRequest({ - method: ChainRpcMethod.GetBalances, - params: { - scope, - accounts: [address], - assets: [asset], - }, - }); - const response = { - balances: { - [address]: { - [asset]: { - amount: '70.02255139', - }, - }, - }, - }; - handler.mockResolvedValue(response); - - const result = await client.getBalances(scope, [address], [asset]); - - expect(handler).toHaveBeenCalledWith(request); - expect(result).toStrictEqual(response); - }); - }); -}); diff --git a/packages/chain-controller/src/SnapChainProviderClient.ts b/packages/chain-controller/src/SnapChainProviderClient.ts deleted file mode 100644 index b5bc939ef05..00000000000 --- a/packages/chain-controller/src/SnapChainProviderClient.ts +++ /dev/null @@ -1,45 +0,0 @@ -import type { - Chain, - CaipAssetTypeOrId, - BalancesResult, -} from '@metamask/chain-api'; -import { ChainRpcMethod } from '@metamask/chain-api'; -import type { CaipChainId } from '@metamask/utils'; - -import type { SnapHandlerClient } from './SnapHandlerClient'; - -/** - * Snap client that implement the Chain API. - */ -export class SnapChainProviderClient implements Chain { - #client: SnapHandlerClient; - - /** - * Constructor for `SnapChainProviderClient`. - * - * @param client - A Snap handler client. - */ - constructor(client: SnapHandlerClient) { - this.#client = client; - } - - /** - * Fetches asset balances for each given accounts. - * - * @param scope - CAIP-2 chain ID that must compatible with `accounts`. - * @param accounts - Accounts (addresses). - * @param assets - List of CAIP-19 asset identifiers to fetch balances from. - * @returns Assets balances for each accounts. - */ - getBalances = async ( - scope: CaipChainId, - accounts: string[], - assets: CaipAssetTypeOrId[], - ): Promise => { - return (await this.#client.submitRequest(ChainRpcMethod.GetBalances, { - scope, - accounts, - assets, - })) as BalancesResult; - }; -} diff --git a/packages/chain-controller/src/SnapHandlerClient.test.ts b/packages/chain-controller/src/SnapHandlerClient.test.ts deleted file mode 100644 index 08d21a7a365..00000000000 --- a/packages/chain-controller/src/SnapHandlerClient.test.ts +++ /dev/null @@ -1,40 +0,0 @@ -import type { SnapId } from '@metamask/snaps-sdk'; -import { HandlerType } from '@metamask/snaps-utils'; - -import { SnapHandlerClient } from './SnapHandlerClient'; - -const snapId = 'local:localhost:3000' as SnapId; - -describe('SnapHandlerClient', () => { - describe('submitRequest', () => { - const method = 'chain_method'; - const params = {}; - const request = { - snapId, - origin: 'metamask', - handler: HandlerType.OnRpcRequest, - request: { - id: expect.any(String), - jsonrpc: '2.0', - method, - params, - }, - }; - const response = { - success: true, - }; - - it('returns a result when a method is called', async () => { - const handler = jest.fn(); - const client = new SnapHandlerClient({ - handler, - snapId, - }); - - handler.mockResolvedValue(response); - const accounts = await client.submitRequest(method, params); - expect(handler).toHaveBeenCalledWith(request); - expect(accounts).toStrictEqual(response); - }); - }); -}); diff --git a/packages/chain-controller/src/SnapHandlerClient.ts b/packages/chain-controller/src/SnapHandlerClient.ts deleted file mode 100644 index d89d529a69d..00000000000 --- a/packages/chain-controller/src/SnapHandlerClient.ts +++ /dev/null @@ -1,115 +0,0 @@ -import type { JsonRpcRequest } from '@metamask/keyring-utils'; -import type { SnapController } from '@metamask/snaps-controllers'; -import type { SnapId } from '@metamask/snaps-sdk'; -import { HandlerType } from '@metamask/snaps-utils'; -import type { Json } from '@metamask/utils'; -import { v4 as uuid } from 'uuid'; - -/** - * Handler for Snap requests. - */ -export type Handler = SnapController['handleRequest']; - -/** - * Send requests to a Snap through a Snap request handler. - */ -class SnapHandlerSender { - #snapId: SnapId; - - #origin: string; - - #handler: Handler; - - #handlerType: HandlerType; - - /** - * Constructor for `SnapHandlerSender`. - * - * @param handler - The Snap request handler to send requests to. - * @param handlerType - The handler type. - * @param snapId - The ID of the snap to use. - * @param origin - The sender's origin. - */ - constructor( - handler: Handler, - handlerType: HandlerType, - snapId: SnapId, - origin: string, - ) { - this.#snapId = snapId; - this.#origin = origin; - this.#handler = handler; - this.#handlerType = handlerType; - } - - /** - * Sends a request to the snap and return the response. - * - * @param request - JSON-RPC request to send to the snap. - * @returns A promise that resolves to the response of the request. - */ - async send(request: JsonRpcRequest): Promise { - return this.#handler({ - snapId: this.#snapId, - origin: this.#origin, - handler: this.#handlerType, - request, - }); - } -} - -/** - * Snap client to submit requests through a handler that submit requests to - * a Snap. - */ -export class SnapHandlerClient { - #handler: Handler; - - #sender: SnapHandlerSender; - - /** - * Constructor for SnapHandlerClient. - * - * @param options - The client options. - * @param options.handler - A function to submit requests to the Snap handler - * (this should call the SnapController.handleRequest) - * @param options.snapId - The Snap ID. - * @param options.origin - The origin from which the Snap is being invoked. - */ - constructor({ - handler, - // Follow same pattern than for @metamask/keyring-snap-client - snapId, - origin = 'metamask', - }: { - handler: Handler; - snapId: SnapId; - origin?: string; - }) { - this.#handler = handler; - this.#sender = new SnapHandlerSender( - handler, - HandlerType.OnRpcRequest, - snapId, - origin, - ); - } - - /** - * Submit a request to the underlying SnapHandlerSender. - * - * @param method - The RPC handler method to be called. - * @param params - The RPC handler parameters. - * @returns The RPC handler response. - */ - submitRequest = async ( - method: string, - params: Json[] | Record, - ): Promise => - await this.#sender.send({ - jsonrpc: '2.0', - method, - params, - id: uuid(), // TODO: Should allow caller to define this one - }); -} diff --git a/packages/chain-controller/src/index.ts b/packages/chain-controller/src/index.ts deleted file mode 100644 index 9516c5419b0..00000000000 --- a/packages/chain-controller/src/index.ts +++ /dev/null @@ -1 +0,0 @@ -export { ChainController } from './ChainController'; diff --git a/packages/chain-controller/tsconfig.build.json b/packages/chain-controller/tsconfig.build.json deleted file mode 100644 index fe743f185a6..00000000000 --- a/packages/chain-controller/tsconfig.build.json +++ /dev/null @@ -1,15 +0,0 @@ -{ - "extends": "../../tsconfig.packages.build.json", - "compilerOptions": { - "baseUrl": "./", - "outDir": "./dist", - "rootDir": "./src", - "skipLibCheck": true - }, - "references": [ - { - "path": "../base-controller/tsconfig.build.json" - } - ], - "include": ["../../types", "./src"] -} diff --git a/packages/chain-controller/tsconfig.json b/packages/chain-controller/tsconfig.json deleted file mode 100644 index f2d7b67ff66..00000000000 --- a/packages/chain-controller/tsconfig.json +++ /dev/null @@ -1,12 +0,0 @@ -{ - "extends": "../../tsconfig.packages.json", - "compilerOptions": { - "baseUrl": "./" - }, - "references": [ - { - "path": "../base-controller" - } - ], - "include": ["../../types", "./src"] -} diff --git a/packages/chain-controller/typedoc.json b/packages/chain-controller/typedoc.json deleted file mode 100644 index c9da015dbf8..00000000000 --- a/packages/chain-controller/typedoc.json +++ /dev/null @@ -1,7 +0,0 @@ -{ - "entryPoints": ["./src/index.ts"], - "excludePrivate": true, - "hideGenerator": true, - "out": "docs", - "tsconfig": "./tsconfig.build.json" -} diff --git a/teams.json b/teams.json index f59b3edb175..2f941fc5229 100644 --- a/teams.json +++ b/teams.json @@ -6,7 +6,6 @@ "metamask/assets-controllers": "team-assets", "metamask/base-controller": "team-wallet-framework", "metamask/build-utils": "team-wallet-framework", - "metamask/chain-controller": "team-accounts", "metamask/composable-controller": "team-wallet-framework", "metamask/controller-utils": "team-wallet-framework", "metamask/ens-controller": "team-confirmations", diff --git a/tsconfig.build.json b/tsconfig.build.json index 518e786576e..b104bf1b420 100644 --- a/tsconfig.build.json +++ b/tsconfig.build.json @@ -7,7 +7,6 @@ { "path": "./packages/assets-controllers/tsconfig.build.json" }, { "path": "./packages/base-controller/tsconfig.build.json" }, { "path": "./packages/build-utils/tsconfig.build.json" }, - { "path": "./packages/chain-controller/tsconfig.build.json" }, { "path": "./packages/composable-controller/tsconfig.build.json" }, { "path": "./packages/controller-utils/tsconfig.build.json" }, { "path": "./packages/ens-controller/tsconfig.build.json" }, diff --git a/yarn.lock b/yarn.lock index 62f430f3b91..aa94150c2f1 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2509,46 +2509,6 @@ __metadata: languageName: unknown linkType: soft -"@metamask/chain-api@npm:^0.1.0": - version: 0.1.0 - resolution: "@metamask/chain-api@npm:0.1.0" - dependencies: - "@metamask/superstruct": "npm:^3.1.0" - "@metamask/utils": "npm:^9.0.0" - checksum: 10/45855703cc31f19b35ed6973096377192c8ca595afa9cd12ff90a90a5236bb8cbbbb1f0b6dc9ebe1c2fb6ab887cce266b192c94538a432c786d9b4b4e2536b88 - languageName: node - linkType: hard - -"@metamask/chain-controller@workspace:packages/chain-controller": - version: 0.0.0-use.local - resolution: "@metamask/chain-controller@workspace:packages/chain-controller" - dependencies: - "@metamask/auto-changelog": "npm:^3.4.4" - "@metamask/base-controller": "npm:^7.1.1" - "@metamask/chain-api": "npm:^0.1.0" - "@metamask/keyring-internal-api": "npm:^2.0.0" - "@metamask/keyring-utils": "npm:^1.0.0" - "@metamask/providers": "npm:^18.1.1" - "@metamask/snaps-controllers": "npm:^9.10.0" - "@metamask/snaps-sdk": "npm:^6.7.0" - "@metamask/snaps-utils": "npm:^8.3.0" - "@metamask/utils": "npm:^11.0.1" - "@types/jest": "npm:^27.4.1" - "@types/readable-stream": "npm:^2.3.0" - deepmerge: "npm:^4.2.2" - jest: "npm:^27.5.1" - ts-jest: "npm:^27.1.4" - typedoc: "npm:^0.24.8" - typedoc-plugin-missing-exports: "npm:^2.0.0" - typescript: "npm:~5.2.2" - uuid: "npm:^8.3.2" - webextension-polyfill: "npm:^0.12.0" - peerDependencies: - "@metamask/providers": ^18.1.0 - webextension-polyfill: ^0.10.0 || ^0.11.0 || ^0.12.0 - languageName: unknown - linkType: soft - "@metamask/composable-controller@workspace:packages/composable-controller": version: 0.0.0-use.local resolution: "@metamask/composable-controller@workspace:packages/composable-controller" From adea818acd0b4e0adf7001c5322129a7fab40a0a Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Mon, 13 Jan 2025 23:47:45 +0100 Subject: [PATCH 02/16] Release 281.0.0 (#5140) Major release for the `accounts-controller`. The `InternalAccount` and `KeyringAccount` now have a new `scopes` required field. --------- Co-authored-by: Elliot Winkler --- package.json | 2 +- packages/accounts-controller/CHANGELOG.md | 11 ++++- packages/accounts-controller/package.json | 4 +- packages/assets-controllers/CHANGELOG.md | 24 ++++++++++- packages/assets-controllers/package.json | 8 ++-- packages/keyring-controller/CHANGELOG.md | 15 ++++++- packages/keyring-controller/package.json | 2 +- .../CHANGELOG.md | 6 ++- .../package.json | 8 ++-- packages/preferences-controller/package.json | 2 +- packages/profile-sync-controller/CHANGELOG.md | 13 +++++- packages/profile-sync-controller/package.json | 8 ++-- packages/signature-controller/package.json | 2 +- packages/transaction-controller/CHANGELOG.md | 10 ++++- packages/transaction-controller/package.json | 6 +-- .../user-operation-controller/CHANGELOG.md | 6 ++- .../user-operation-controller/package.json | 8 ++-- yarn.lock | 42 +++++++++---------- 18 files changed, 121 insertions(+), 56 deletions(-) diff --git a/package.json b/package.json index 93cc727ff10..990d517bb3a 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@metamask/core-monorepo", - "version": "280.0.0", + "version": "281.0.0", "private": true, "description": "Monorepo for packages shared between MetaMask clients", "repository": { diff --git a/packages/accounts-controller/CHANGELOG.md b/packages/accounts-controller/CHANGELOG.md index 00ae70db7f3..a6c0fd8aafd 100644 --- a/packages/accounts-controller/CHANGELOG.md +++ b/packages/accounts-controller/CHANGELOG.md @@ -7,9 +7,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [21.0.0] + ### Changed -- Bump `@metamask/base-controller` from `^7.0.0` to `^7.1.0` ([#5079](https://github.com/MetaMask/core/pull/5079)) +- **BREAKING:** Add `scopes` field to `KeyringAccount` ([#5066](https://github.com/MetaMask/core/pull/5066)), ([#5136](https://github.com/MetaMask/core/pull/5136)) + - This field is now required and will be used to identify the supported chains (using CAIP-2 chain IDs) for every accounts. +- Bump `@metamask/base-controller` from `^7.0.0` to `^7.1.1` ([#5079](https://github.com/MetaMask/core/pull/5079)), ([#5135](https://github.com/MetaMask/core/pull/5135)) +- Bump `@metamask/utils` to `^11.0.1` ([#5080](https://github.com/MetaMask/core/pull/5080)) +- Bump `@metamask/rpc-errors` to `^7.0.2` ([#5080](https://github.com/MetaMask/core/pull/5080)) ## [20.0.2] @@ -382,7 +388,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Initial release ([#1637](https://github.com/MetaMask/core/pull/1637)) -[Unreleased]: https://github.com/MetaMask/core/compare/@metamask/accounts-controller@20.0.2...HEAD +[Unreleased]: https://github.com/MetaMask/core/compare/@metamask/accounts-controller@21.0.0...HEAD +[21.0.0]: https://github.com/MetaMask/core/compare/@metamask/accounts-controller@20.0.2...@metamask/accounts-controller@21.0.0 [20.0.2]: https://github.com/MetaMask/core/compare/@metamask/accounts-controller@20.0.1...@metamask/accounts-controller@20.0.2 [20.0.1]: https://github.com/MetaMask/core/compare/@metamask/accounts-controller@20.0.0...@metamask/accounts-controller@20.0.1 [20.0.0]: https://github.com/MetaMask/core/compare/@metamask/accounts-controller@19.0.0...@metamask/accounts-controller@20.0.0 diff --git a/packages/accounts-controller/package.json b/packages/accounts-controller/package.json index 756016ca684..dcc430634e2 100644 --- a/packages/accounts-controller/package.json +++ b/packages/accounts-controller/package.json @@ -1,6 +1,6 @@ { "name": "@metamask/accounts-controller", - "version": "20.0.2", + "version": "21.0.0", "description": "Manages internal accounts", "keywords": [ "MetaMask", @@ -62,7 +62,7 @@ }, "devDependencies": { "@metamask/auto-changelog": "^3.4.4", - "@metamask/keyring-controller": "^19.0.2", + "@metamask/keyring-controller": "^19.0.3", "@metamask/providers": "^18.1.1", "@metamask/snaps-controllers": "^9.10.0", "@types/jest": "^27.4.1", diff --git a/packages/assets-controllers/CHANGELOG.md b/packages/assets-controllers/CHANGELOG.md index 2349b7f900b..4c0554fae47 100644 --- a/packages/assets-controllers/CHANGELOG.md +++ b/packages/assets-controllers/CHANGELOG.md @@ -7,9 +7,28 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [46.0.0] + +### Added + +- Add new `MultichainBalancesController` ([#4965](https://github.com/MetaMask/core/pull/4965)) + - This controller has been migrated from the MetaMask extension codebase. +- Added utility function `getKeyByValue` ([#5099](https://github.com/MetaMask/core/pull/5099)) + ### Changed -- Bump `@metamask/base-controller` from `^7.0.0` to `^7.1.0` ([#5079](https://github.com/MetaMask/core/pull/5079)) +- **BREAKING:** Bump `@metamask/accounts-controller` peer dependency from `^20.0.0` to `^21.0.0` ([#5140](https://github.com/MetaMask/core/pull/5140)) +- Bump `@metamask/base-controller` from `^7.0.0` to `^7.1.1` ([#5079](https://github.com/MetaMask/core/pull/5079)), ([#5135](https://github.com/MetaMask/core/pull/5135)) +- Bump `@metamask/keyring-api` from `^12.0.0` to `^13.0.0` ([#5066](https://github.com/MetaMask/core/pull/5066)) +- Bump `@metamask/utils` to `^11.0.1` ([#5080](https://github.com/MetaMask/core/pull/5080)) +- Bump `@metamask/rpc-errors` to `^7.0.2` ([#5080](https://github.com/MetaMask/core/pull/5080)) + +### Fixed + +- Fix Mantle price when calling `fetchMultiExchangeRate` ([#5099](https://github.com/MetaMask/core/pull/5099)) +- Fix multicall revert in `TokenBalancesController` ([#5083](https://github.com/MetaMask/core/pull/5083)) + - `TokenBalancesController` was fixed to fetch erc20 token balances even if there's an invalid token in state whose address does not point to a smart contract. +- Fix state changes for `ignoreTokens` for non-selected networks ([#5014](https://github.com/MetaMask/core/pull/5014)) ## [45.1.2] @@ -1304,7 +1323,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Use Ethers for AssetsContractController ([#845](https://github.com/MetaMask/core/pull/845)) -[Unreleased]: https://github.com/MetaMask/core/compare/@metamask/assets-controllers@45.1.2...HEAD +[Unreleased]: https://github.com/MetaMask/core/compare/@metamask/assets-controllers@46.0.0...HEAD +[46.0.0]: https://github.com/MetaMask/core/compare/@metamask/assets-controllers@45.1.2...@metamask/assets-controllers@46.0.0 [45.1.2]: https://github.com/MetaMask/core/compare/@metamask/assets-controllers@45.1.1...@metamask/assets-controllers@45.1.2 [45.1.1]: https://github.com/MetaMask/core/compare/@metamask/assets-controllers@45.1.0...@metamask/assets-controllers@45.1.1 [45.1.0]: https://github.com/MetaMask/core/compare/@metamask/assets-controllers@45.0.0...@metamask/assets-controllers@45.1.0 diff --git a/packages/assets-controllers/package.json b/packages/assets-controllers/package.json index 8f55b87a865..d2daa3b1ca4 100644 --- a/packages/assets-controllers/package.json +++ b/packages/assets-controllers/package.json @@ -1,6 +1,6 @@ { "name": "@metamask/assets-controllers", - "version": "45.1.2", + "version": "46.0.0", "description": "Controllers which manage interactions involving ERC-20, ERC-721, and ERC-1155 tokens (including NFTs)", "keywords": [ "MetaMask", @@ -77,12 +77,12 @@ }, "devDependencies": { "@babel/runtime": "^7.23.9", - "@metamask/accounts-controller": "^20.0.2", + "@metamask/accounts-controller": "^21.0.0", "@metamask/approval-controller": "^7.1.2", "@metamask/auto-changelog": "^3.4.4", "@metamask/ethjs-provider-http": "^0.3.0", "@metamask/keyring-api": "^13.0.0", - "@metamask/keyring-controller": "^19.0.2", + "@metamask/keyring-controller": "^19.0.3", "@metamask/keyring-internal-api": "^2.0.0", "@metamask/keyring-snap-client": "^2.0.0", "@metamask/network-controller": "^22.1.1", @@ -105,7 +105,7 @@ "webextension-polyfill": "^0.12.0" }, "peerDependencies": { - "@metamask/accounts-controller": "^20.0.0", + "@metamask/accounts-controller": "^21.0.0", "@metamask/approval-controller": "^7.0.0", "@metamask/keyring-controller": "^19.0.0", "@metamask/network-controller": "^22.0.0", diff --git a/packages/keyring-controller/CHANGELOG.md b/packages/keyring-controller/CHANGELOG.md index 8059997ccd1..9b30b478863 100644 --- a/packages/keyring-controller/CHANGELOG.md +++ b/packages/keyring-controller/CHANGELOG.md @@ -7,9 +7,19 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [19.0.3] + ### Changed -- Bump `@metamask/base-controller` from `^7.0.0` to `^7.1.0` ([#5079](https://github.com/MetaMask/core/pull/5079)) +- Bump `@metamask/base-controller` from `^7.0.0` to `^7.1.1` ([#5079](https://github.com/MetaMask/core/pull/5079)), ([#5135](https://github.com/MetaMask/core/pull/5135)) +- Bump `@metamask/keyring-api` from `^12.0.0` to `^13.0.0` ([#5066](https://github.com/MetaMask/core/pull/5066)) +- Bump `@metamask/keyring-internal-api` from `^1.0.0` to `^2.0.0` ([#5066](https://github.com/MetaMask/core/pull/5066)), ([#5136](https://github.com/MetaMask/core/pull/5136)) +- Bump `@metamask/utils` to `^11.0.1` ([#5080](https://github.com/MetaMask/core/pull/5080)) +- Bump `@metamask/rpc-errors` to `^7.0.2` ([#5080](https://github.com/MetaMask/core/pull/5080)) + +### Fixed + +- Make `verifySeedPhrase` mutually exclusive ([#5077](https://github.com/MetaMask/core/pull/5077)) ## [19.0.2] @@ -622,7 +632,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 All changes listed after this point were applied to this package following the monorepo conversion. -[Unreleased]: https://github.com/MetaMask/core/compare/@metamask/keyring-controller@19.0.2...HEAD +[Unreleased]: https://github.com/MetaMask/core/compare/@metamask/keyring-controller@19.0.3...HEAD +[19.0.3]: https://github.com/MetaMask/core/compare/@metamask/keyring-controller@19.0.2...@metamask/keyring-controller@19.0.3 [19.0.2]: https://github.com/MetaMask/core/compare/@metamask/keyring-controller@19.0.1...@metamask/keyring-controller@19.0.2 [19.0.1]: https://github.com/MetaMask/core/compare/@metamask/keyring-controller@19.0.0...@metamask/keyring-controller@19.0.1 [19.0.0]: https://github.com/MetaMask/core/compare/@metamask/keyring-controller@18.0.0...@metamask/keyring-controller@19.0.0 diff --git a/packages/keyring-controller/package.json b/packages/keyring-controller/package.json index 6718b477c07..d0f98e5f1c2 100644 --- a/packages/keyring-controller/package.json +++ b/packages/keyring-controller/package.json @@ -1,6 +1,6 @@ { "name": "@metamask/keyring-controller", - "version": "19.0.2", + "version": "19.0.3", "description": "Stores identities seen in the wallet and manages interactions such as signing", "keywords": [ "MetaMask", diff --git a/packages/notification-services-controller/CHANGELOG.md b/packages/notification-services-controller/CHANGELOG.md index 90be539dd47..6da44d6f950 100644 --- a/packages/notification-services-controller/CHANGELOG.md +++ b/packages/notification-services-controller/CHANGELOG.md @@ -7,8 +7,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [0.16.0] + ### Changed +- **BREAKING:** Bump peer dependency `@metamask/profile-sync-controller` from `^3.0.0` to `^4.0.0` ([#5140](https://github.com/MetaMask/core/pull/5140)) - Bump `@metamask/base-controller` from `^7.0.0` to `^7.1.0` ([#5079](https://github.com/MetaMask/core/pull/5079)) ## [0.15.0] @@ -271,7 +274,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Initial release -[Unreleased]: https://github.com/MetaMask/core/compare/@metamask/notification-services-controller@0.15.0...HEAD +[Unreleased]: https://github.com/MetaMask/core/compare/@metamask/notification-services-controller@0.16.0...HEAD +[0.16.0]: https://github.com/MetaMask/core/compare/@metamask/notification-services-controller@0.15.0...@metamask/notification-services-controller@0.16.0 [0.15.0]: https://github.com/MetaMask/core/compare/@metamask/notification-services-controller@0.14.0...@metamask/notification-services-controller@0.15.0 [0.14.0]: https://github.com/MetaMask/core/compare/@metamask/notification-services-controller@0.13.0...@metamask/notification-services-controller@0.14.0 [0.13.0]: https://github.com/MetaMask/core/compare/@metamask/notification-services-controller@0.12.1...@metamask/notification-services-controller@0.13.0 diff --git a/packages/notification-services-controller/package.json b/packages/notification-services-controller/package.json index 8965b28be6a..d4425ebc0f4 100644 --- a/packages/notification-services-controller/package.json +++ b/packages/notification-services-controller/package.json @@ -1,6 +1,6 @@ { "name": "@metamask/notification-services-controller", - "version": "0.15.0", + "version": "0.16.0", "description": "Manages New MetaMask decentralized Notification system", "keywords": [ "MetaMask", @@ -112,8 +112,8 @@ "@lavamoat/allow-scripts": "^3.0.4", "@lavamoat/preinstall-always-fail": "^2.1.0", "@metamask/auto-changelog": "^3.4.4", - "@metamask/keyring-controller": "^19.0.2", - "@metamask/profile-sync-controller": "^3.3.0", + "@metamask/keyring-controller": "^19.0.3", + "@metamask/profile-sync-controller": "^4.0.0", "@types/jest": "^27.4.1", "@types/readable-stream": "^2.3.0", "contentful": "^10.15.0", @@ -128,7 +128,7 @@ }, "peerDependencies": { "@metamask/keyring-controller": "^19.0.0", - "@metamask/profile-sync-controller": "^3.0.0" + "@metamask/profile-sync-controller": "^4.0.0" }, "engines": { "node": "^18.18 || >=20" diff --git a/packages/preferences-controller/package.json b/packages/preferences-controller/package.json index 4dbaf3fb422..336ffb11a0e 100644 --- a/packages/preferences-controller/package.json +++ b/packages/preferences-controller/package.json @@ -52,7 +52,7 @@ }, "devDependencies": { "@metamask/auto-changelog": "^3.4.4", - "@metamask/keyring-controller": "^19.0.2", + "@metamask/keyring-controller": "^19.0.3", "@types/jest": "^27.4.1", "deepmerge": "^4.2.2", "jest": "^27.5.1", diff --git a/packages/profile-sync-controller/CHANGELOG.md b/packages/profile-sync-controller/CHANGELOG.md index 238397f17cd..be348cea2cc 100644 --- a/packages/profile-sync-controller/CHANGELOG.md +++ b/packages/profile-sync-controller/CHANGELOG.md @@ -7,6 +7,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [4.0.0] + +### Changed + +- **BREAKING:** Bump `@metamask/accounts-controller` peer dependency from `^20.0.0` to `^21.0.0` ([#5140](https://github.com/MetaMask/core/pull/5140)) +- Bump `@metamask/base-controller` from `7.1.0` to `^7.1.1` ([#5135](https://github.com/MetaMask/core/pull/5135)) +- Bump `@metamask/keyring-api` from `^12.0.0` to `^13.0.0` ([#5066](https://github.com/MetaMask/core/pull/5066)) +- Bump `@metamask/keyring-internal-api` from `^1.0.0` to `^2.0.0` ([#5066](https://github.com/MetaMask/core/pull/5066)), ([#5136](https://github.com/MetaMask/core/pull/5136)) +- Bump `@metamask/keyring-controller` from `^19.0.2` to `^19.0.3` ([#5140](https://github.com/MetaMask/core/pull/5140)) + ## [3.3.0] ### Added @@ -400,7 +410,8 @@ 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@3.3.0...HEAD +[Unreleased]: https://github.com/MetaMask/core/compare/@metamask/profile-sync-controller@4.0.0...HEAD +[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 [3.1.1]: https://github.com/MetaMask/core/compare/@metamask/profile-sync-controller@3.1.0...@metamask/profile-sync-controller@3.1.1 diff --git a/packages/profile-sync-controller/package.json b/packages/profile-sync-controller/package.json index a5ae286be33..ceac6b460f8 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": "3.3.0", + "version": "4.0.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", @@ -102,7 +102,7 @@ "dependencies": { "@metamask/base-controller": "^7.1.1", "@metamask/keyring-api": "^13.0.0", - "@metamask/keyring-controller": "^19.0.2", + "@metamask/keyring-controller": "^19.0.3", "@metamask/network-controller": "^22.1.1", "@metamask/snaps-sdk": "^6.7.0", "@metamask/snaps-utils": "^8.3.0", @@ -115,7 +115,7 @@ "devDependencies": { "@lavamoat/allow-scripts": "^3.0.4", "@lavamoat/preinstall-always-fail": "^2.1.0", - "@metamask/accounts-controller": "^20.0.2", + "@metamask/accounts-controller": "^21.0.0", "@metamask/auto-changelog": "^3.4.4", "@metamask/keyring-internal-api": "^2.0.0", "@metamask/providers": "^18.1.1", @@ -133,7 +133,7 @@ "webextension-polyfill": "^0.12.0" }, "peerDependencies": { - "@metamask/accounts-controller": "^20.0.0", + "@metamask/accounts-controller": "^21.0.0", "@metamask/keyring-controller": "^19.0.0", "@metamask/network-controller": "^22.0.0", "@metamask/providers": "^18.1.0", diff --git a/packages/signature-controller/package.json b/packages/signature-controller/package.json index f8f44b9ec37..28666f2c789 100644 --- a/packages/signature-controller/package.json +++ b/packages/signature-controller/package.json @@ -58,7 +58,7 @@ "devDependencies": { "@metamask/approval-controller": "^7.1.2", "@metamask/auto-changelog": "^3.4.4", - "@metamask/keyring-controller": "^19.0.2", + "@metamask/keyring-controller": "^19.0.3", "@metamask/logging-controller": "^6.0.3", "@metamask/network-controller": "^22.1.1", "@types/jest": "^27.4.1", diff --git a/packages/transaction-controller/CHANGELOG.md b/packages/transaction-controller/CHANGELOG.md index 2ae9a0f8e08..e3de4de9635 100644 --- a/packages/transaction-controller/CHANGELOG.md +++ b/packages/transaction-controller/CHANGELOG.md @@ -7,11 +7,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [43.0.0] + ### Added - Add `gasLimitNoBuffer` property to `TransactionMeta` type ([#5113](https://github.com/MetaMask/core/pull/5113)) - `gasLimitNoBuffer` is the estimated gas for the transaction without any buffer applied. +### Changed + +- **BREAKING:** Bump `@metamask/accounts-controller` peer dependency from `^20.0.0` to `^21.0.0` ([#5140](https://github.com/MetaMask/core/pull/5140)) +- Bump `@metamask/base-controller` from `7.1.0` to `^7.1.1` ([#5135](https://github.com/MetaMask/core/pull/5135)) + ## [42.1.0] ### Added @@ -1231,7 +1238,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 All changes listed after this point were applied to this package following the monorepo conversion. -[Unreleased]: https://github.com/MetaMask/core/compare/@metamask/transaction-controller@42.1.0...HEAD +[Unreleased]: https://github.com/MetaMask/core/compare/@metamask/transaction-controller@43.0.0...HEAD +[43.0.0]: https://github.com/MetaMask/core/compare/@metamask/transaction-controller@42.1.0...@metamask/transaction-controller@43.0.0 [42.1.0]: https://github.com/MetaMask/core/compare/@metamask/transaction-controller@42.0.0...@metamask/transaction-controller@42.1.0 [42.0.0]: https://github.com/MetaMask/core/compare/@metamask/transaction-controller@41.1.0...@metamask/transaction-controller@42.0.0 [41.1.0]: https://github.com/MetaMask/core/compare/@metamask/transaction-controller@41.0.0...@metamask/transaction-controller@41.1.0 diff --git a/packages/transaction-controller/package.json b/packages/transaction-controller/package.json index 0dfc3094646..7efdd227f6e 100644 --- a/packages/transaction-controller/package.json +++ b/packages/transaction-controller/package.json @@ -1,6 +1,6 @@ { "name": "@metamask/transaction-controller", - "version": "42.1.0", + "version": "43.0.0", "description": "Stores transactions alongside their periodically updated statuses and manages interactions such as approval and cancellation", "keywords": [ "MetaMask", @@ -69,7 +69,7 @@ }, "devDependencies": { "@babel/runtime": "^7.23.9", - "@metamask/accounts-controller": "^20.0.2", + "@metamask/accounts-controller": "^21.0.0", "@metamask/approval-controller": "^7.1.2", "@metamask/auto-changelog": "^3.4.4", "@metamask/eth-block-tracker": "^11.0.3", @@ -92,7 +92,7 @@ }, "peerDependencies": { "@babel/runtime": "^7.0.0", - "@metamask/accounts-controller": "^20.0.0", + "@metamask/accounts-controller": "^21.0.0", "@metamask/approval-controller": "^7.0.0", "@metamask/eth-block-tracker": ">=9", "@metamask/gas-fee-controller": "^22.0.0", diff --git a/packages/user-operation-controller/CHANGELOG.md b/packages/user-operation-controller/CHANGELOG.md index 38c18d91fe7..71d2bc56cf3 100644 --- a/packages/user-operation-controller/CHANGELOG.md +++ b/packages/user-operation-controller/CHANGELOG.md @@ -7,8 +7,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [22.0.0] + ### Changed +- **BREAKING:** Bump `@metamask/transaction-controller` peer dependency from `^42.0.0` to `^43.0.0` ([#5140](https://github.com/MetaMask/core/pull/5140)) - Bump `@metamask/base-controller` from `^7.0.0` to `^7.1.0` ([#5079](https://github.com/MetaMask/core/pull/5079)) ## [21.0.0] @@ -311,7 +314,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Initial Release ([#3749](https://github.com/MetaMask/core/pull/3749)) -[Unreleased]: https://github.com/MetaMask/core/compare/@metamask/user-operation-controller@21.0.0...HEAD +[Unreleased]: https://github.com/MetaMask/core/compare/@metamask/user-operation-controller@22.0.0...HEAD +[22.0.0]: https://github.com/MetaMask/core/compare/@metamask/user-operation-controller@21.0.0...@metamask/user-operation-controller@22.0.0 [21.0.0]: https://github.com/MetaMask/core/compare/@metamask/user-operation-controller@20.0.1...@metamask/user-operation-controller@21.0.0 [20.0.1]: https://github.com/MetaMask/core/compare/@metamask/user-operation-controller@20.0.0...@metamask/user-operation-controller@20.0.1 [20.0.0]: https://github.com/MetaMask/core/compare/@metamask/user-operation-controller@19.0.0...@metamask/user-operation-controller@20.0.0 diff --git a/packages/user-operation-controller/package.json b/packages/user-operation-controller/package.json index 7e16ea14386..78fa792faff 100644 --- a/packages/user-operation-controller/package.json +++ b/packages/user-operation-controller/package.json @@ -1,6 +1,6 @@ { "name": "@metamask/user-operation-controller", - "version": "21.0.0", + "version": "22.0.0", "description": "Creates user operations and manages their life cycle", "keywords": [ "MetaMask", @@ -65,9 +65,9 @@ "@metamask/auto-changelog": "^3.4.4", "@metamask/eth-block-tracker": "^11.0.3", "@metamask/gas-fee-controller": "^22.0.2", - "@metamask/keyring-controller": "^19.0.2", + "@metamask/keyring-controller": "^19.0.3", "@metamask/network-controller": "^22.1.1", - "@metamask/transaction-controller": "^42.1.0", + "@metamask/transaction-controller": "^43.0.0", "@types/jest": "^27.4.1", "deepmerge": "^4.2.2", "jest": "^27.5.1", @@ -82,7 +82,7 @@ "@metamask/gas-fee-controller": "^22.0.0", "@metamask/keyring-controller": "^19.0.0", "@metamask/network-controller": "^22.0.0", - "@metamask/transaction-controller": "^42.0.0" + "@metamask/transaction-controller": "^43.0.0" }, "engines": { "node": "^18.18 || >=20" diff --git a/yarn.lock b/yarn.lock index aa94150c2f1..6df979fa7ef 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2255,7 +2255,7 @@ __metadata: languageName: node linkType: hard -"@metamask/accounts-controller@npm:^20.0.2, @metamask/accounts-controller@workspace:packages/accounts-controller": +"@metamask/accounts-controller@npm:^21.0.0, @metamask/accounts-controller@workspace:packages/accounts-controller": version: 0.0.0-use.local resolution: "@metamask/accounts-controller@workspace:packages/accounts-controller" dependencies: @@ -2264,7 +2264,7 @@ __metadata: "@metamask/base-controller": "npm:^7.1.1" "@metamask/eth-snap-keyring": "npm:^8.0.0" "@metamask/keyring-api": "npm:^13.0.0" - "@metamask/keyring-controller": "npm:^19.0.2" + "@metamask/keyring-controller": "npm:^19.0.3" "@metamask/keyring-internal-api": "npm:^2.0.0" "@metamask/providers": "npm:^18.1.1" "@metamask/snaps-controllers": "npm:^9.10.0" @@ -2375,7 +2375,7 @@ __metadata: "@ethersproject/contracts": "npm:^5.7.0" "@ethersproject/providers": "npm:^5.7.0" "@metamask/abi-utils": "npm:^2.0.3" - "@metamask/accounts-controller": "npm:^20.0.2" + "@metamask/accounts-controller": "npm:^21.0.0" "@metamask/approval-controller": "npm:^7.1.2" "@metamask/auto-changelog": "npm:^3.4.4" "@metamask/base-controller": "npm:^7.1.1" @@ -2384,7 +2384,7 @@ __metadata: "@metamask/eth-query": "npm:^4.0.0" "@metamask/ethjs-provider-http": "npm:^0.3.0" "@metamask/keyring-api": "npm:^13.0.0" - "@metamask/keyring-controller": "npm:^19.0.2" + "@metamask/keyring-controller": "npm:^19.0.3" "@metamask/keyring-internal-api": "npm:^2.0.0" "@metamask/keyring-snap-client": "npm:^2.0.0" "@metamask/metamask-eth-abis": "npm:^3.1.1" @@ -2422,7 +2422,7 @@ __metadata: uuid: "npm:^8.3.2" webextension-polyfill: "npm:^0.12.0" peerDependencies: - "@metamask/accounts-controller": ^20.0.0 + "@metamask/accounts-controller": ^21.0.0 "@metamask/approval-controller": ^7.0.0 "@metamask/keyring-controller": ^19.0.0 "@metamask/network-controller": ^22.0.0 @@ -3165,7 +3165,7 @@ __metadata: languageName: node linkType: hard -"@metamask/keyring-controller@npm:^19.0.2, @metamask/keyring-controller@workspace:packages/keyring-controller": +"@metamask/keyring-controller@npm:^19.0.3, @metamask/keyring-controller@workspace:packages/keyring-controller": version: 0.0.0-use.local resolution: "@metamask/keyring-controller@workspace:packages/keyring-controller" dependencies: @@ -3412,8 +3412,8 @@ __metadata: "@metamask/auto-changelog": "npm:^3.4.4" "@metamask/base-controller": "npm:^7.1.1" "@metamask/controller-utils": "npm:^11.4.5" - "@metamask/keyring-controller": "npm:^19.0.2" - "@metamask/profile-sync-controller": "npm:^3.3.0" + "@metamask/keyring-controller": "npm:^19.0.3" + "@metamask/profile-sync-controller": "npm:^4.0.0" "@metamask/utils": "npm:^11.0.1" "@types/jest": "npm:^27.4.1" "@types/readable-stream": "npm:^2.3.0" @@ -3432,7 +3432,7 @@ __metadata: uuid: "npm:^8.3.2" peerDependencies: "@metamask/keyring-controller": ^19.0.0 - "@metamask/profile-sync-controller": ^3.0.0 + "@metamask/profile-sync-controller": ^4.0.0 languageName: unknown linkType: soft @@ -3580,7 +3580,7 @@ __metadata: "@metamask/auto-changelog": "npm:^3.4.4" "@metamask/base-controller": "npm:^7.1.1" "@metamask/controller-utils": "npm:^11.4.5" - "@metamask/keyring-controller": "npm:^19.0.2" + "@metamask/keyring-controller": "npm:^19.0.3" "@types/jest": "npm:^27.4.1" deepmerge: "npm:^4.2.2" jest: "npm:^27.5.1" @@ -3594,17 +3594,17 @@ __metadata: languageName: unknown linkType: soft -"@metamask/profile-sync-controller@npm:^3.3.0, @metamask/profile-sync-controller@workspace:packages/profile-sync-controller": +"@metamask/profile-sync-controller@npm:^4.0.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: "@lavamoat/allow-scripts": "npm:^3.0.4" "@lavamoat/preinstall-always-fail": "npm:^2.1.0" - "@metamask/accounts-controller": "npm:^20.0.2" + "@metamask/accounts-controller": "npm:^21.0.0" "@metamask/auto-changelog": "npm:^3.4.4" "@metamask/base-controller": "npm:^7.1.1" "@metamask/keyring-api": "npm:^13.0.0" - "@metamask/keyring-controller": "npm:^19.0.2" + "@metamask/keyring-controller": "npm:^19.0.3" "@metamask/keyring-internal-api": "npm:^2.0.0" "@metamask/network-controller": "npm:^22.1.1" "@metamask/providers": "npm:^18.1.1" @@ -3628,7 +3628,7 @@ __metadata: typescript: "npm:~5.2.2" webextension-polyfill: "npm:^0.12.0" peerDependencies: - "@metamask/accounts-controller": ^20.0.0 + "@metamask/accounts-controller": ^21.0.0 "@metamask/keyring-controller": ^19.0.0 "@metamask/network-controller": ^22.0.0 "@metamask/providers": ^18.1.0 @@ -3792,7 +3792,7 @@ __metadata: "@metamask/base-controller": "npm:^7.1.1" "@metamask/controller-utils": "npm:^11.4.5" "@metamask/eth-sig-util": "npm:^8.0.0" - "@metamask/keyring-controller": "npm:^19.0.2" + "@metamask/keyring-controller": "npm:^19.0.3" "@metamask/logging-controller": "npm:^6.0.3" "@metamask/network-controller": "npm:^22.1.1" "@metamask/utils": "npm:^11.0.1" @@ -3945,7 +3945,7 @@ __metadata: languageName: node linkType: hard -"@metamask/transaction-controller@npm:^42.1.0, @metamask/transaction-controller@workspace:packages/transaction-controller": +"@metamask/transaction-controller@npm:^43.0.0, @metamask/transaction-controller@workspace:packages/transaction-controller": version: 0.0.0-use.local resolution: "@metamask/transaction-controller@workspace:packages/transaction-controller" dependencies: @@ -3956,7 +3956,7 @@ __metadata: "@ethersproject/abi": "npm:^5.7.0" "@ethersproject/contracts": "npm:^5.7.0" "@ethersproject/providers": "npm:^5.7.0" - "@metamask/accounts-controller": "npm:^20.0.2" + "@metamask/accounts-controller": "npm:^21.0.0" "@metamask/approval-controller": "npm:^7.1.2" "@metamask/auto-changelog": "npm:^3.4.4" "@metamask/base-controller": "npm:^7.1.1" @@ -3991,7 +3991,7 @@ __metadata: uuid: "npm:^8.3.2" peerDependencies: "@babel/runtime": ^7.0.0 - "@metamask/accounts-controller": ^20.0.0 + "@metamask/accounts-controller": ^21.0.0 "@metamask/approval-controller": ^7.0.0 "@metamask/eth-block-tracker": ">=9" "@metamask/gas-fee-controller": ^22.0.0 @@ -4010,12 +4010,12 @@ __metadata: "@metamask/eth-block-tracker": "npm:^11.0.3" "@metamask/eth-query": "npm:^4.0.0" "@metamask/gas-fee-controller": "npm:^22.0.2" - "@metamask/keyring-controller": "npm:^19.0.2" + "@metamask/keyring-controller": "npm:^19.0.3" "@metamask/network-controller": "npm:^22.1.1" "@metamask/polling-controller": "npm:^12.0.2" "@metamask/rpc-errors": "npm:^7.0.2" "@metamask/superstruct": "npm:^3.1.0" - "@metamask/transaction-controller": "npm:^42.1.0" + "@metamask/transaction-controller": "npm:^43.0.0" "@metamask/utils": "npm:^11.0.1" "@types/jest": "npm:^27.4.1" bn.js: "npm:^5.2.1" @@ -4034,7 +4034,7 @@ __metadata: "@metamask/gas-fee-controller": ^22.0.0 "@metamask/keyring-controller": ^19.0.0 "@metamask/network-controller": ^22.0.0 - "@metamask/transaction-controller": ^42.0.0 + "@metamask/transaction-controller": ^43.0.0 languageName: unknown linkType: soft From 36d53191d150f6a7e1494bef8239594f6424197a Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Tue, 14 Jan 2025 11:21:03 +0100 Subject: [PATCH 03/16] feat: add sentry context to erroneous situation callbacks (#5139) ## Explanation In order to better understand what's happening during erroneous situations, we are adding with this PR an object to all `onAccountSyncErroneousSituation` calls. This object will be used as a Sentry context on clients. ## References ## Changelog ### `@metamask/profile-sync-controller` - **ADDED**: Add a context object to `onAccountSyncErroneousSituation` for better error understanding ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've highlighted breaking changes using the "BREAKING" category above as appropriate - [x] I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes --- .../UserStorageController.test.ts | 7 +- .../user-storage/UserStorageController.ts | 4 +- .../controller-integration.test.ts | 102 +++++++++++++++--- .../account-syncing/controller-integration.ts | 71 +++++++++++- 4 files changed, 162 insertions(+), 22 deletions(-) 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..619336ccae4 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.ts +++ b/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.ts @@ -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( From 4612b825dd4d2cbf623bf04aeadfa8f850174690 Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Tue, 14 Jan 2025 11:57:56 +0100 Subject: [PATCH 04/16] Release 282.0.0 (#5144) ## Explanation This is a RC for v282.0.0. See changelog for more details @metamask/profile-sync-controller@4.0.1 ## References ## Changelog ```ms ### Changed - Bump `@metamask/profile-sync-controller` from `^4.0.0` to `^4.0.1` ([#5144](https://github.com/MetaMask/core/pull/5144)) ``` ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've highlighted breaking changes using the "BREAKING" category above as appropriate - [x] I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes --- package.json | 2 +- packages/notification-services-controller/package.json | 2 +- packages/profile-sync-controller/CHANGELOG.md | 9 ++++++++- packages/profile-sync-controller/package.json | 2 +- yarn.lock | 4 ++-- 5 files changed, 13 insertions(+), 6 deletions(-) diff --git a/package.json b/package.json index 990d517bb3a..123572835c8 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@metamask/core-monorepo", - "version": "281.0.0", + "version": "282.0.0", "private": true, "description": "Monorepo for packages shared between MetaMask clients", "repository": { diff --git a/packages/notification-services-controller/package.json b/packages/notification-services-controller/package.json index d4425ebc0f4..bd0d95d4a3b 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.0.1", "@types/jest": "^27.4.1", "@types/readable-stream": "^2.3.0", "contentful": "^10.15.0", diff --git a/packages/profile-sync-controller/CHANGELOG.md b/packages/profile-sync-controller/CHANGELOG.md index be348cea2cc..3e21a911ae4 100644 --- a/packages/profile-sync-controller/CHANGELOG.md +++ b/packages/profile-sync-controller/CHANGELOG.md @@ -7,6 +7,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [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 +416,8 @@ 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.0.1...HEAD +[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..d109e073f9c 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.0.1", "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/yarn.lock b/yarn.lock index 6df979fa7ef..6d2be506d2b 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3413,7 +3413,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.0.1" "@metamask/utils": "npm:^11.0.1" "@types/jest": "npm:^27.4.1" "@types/readable-stream": "npm:^2.3.0" @@ -3594,7 +3594,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.0.1, @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: From 6fdfbd748ad1b0d28b78c65f598c557250dac5f0 Mon Sep 17 00:00:00 2001 From: Elliot Winkler Date: Tue, 14 Jan 2025 09:19:30 -0700 Subject: [PATCH 05/16] Start extracting service policy boilerplate to common function (#5141) We would like to use the Cockatiel library in our service classes to ensure that requests are retried using the circuit breaker pattern. Some of our service classes do this already, but we are copying and pasting the code around. This commit extracts some of the boilerplate code to a new function in the `@metamask/controller-utils` package, `createServicePolicy`, so that we no longer have to do this. Because it would require an enormous amount of tests, this commit does not include a complete version of `createServicePolicy`, omitting options such as `maxRetries` as well as an `onDegraded` callback so consumers can handle slow requests. Those will be added later. --- packages/controller-utils/CHANGELOG.md | 4 + packages/controller-utils/jest.config.js | 2 +- packages/controller-utils/package.json | 2 + .../src/create-service-policy.test.ts | 589 ++++++++++++++++++ .../src/create-service-policy.ts | 128 ++++ packages/controller-utils/src/index.test.ts | 75 +++ packages/controller-utils/src/index.ts | 1 + yarn.lock | 2 + 8 files changed, 802 insertions(+), 1 deletion(-) create mode 100644 packages/controller-utils/src/create-service-policy.test.ts create mode 100644 packages/controller-utils/src/create-service-policy.ts create mode 100644 packages/controller-utils/src/index.test.ts diff --git a/packages/controller-utils/CHANGELOG.md b/packages/controller-utils/CHANGELOG.md index 5774eba9cab..313dc0fdd3e 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 ([#5053](https://github.com/MetaMask/core/pull/5053)) + ## [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..cb2ec2ee882 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" }, @@ -65,6 +66,7 @@ "deepmerge": "^4.2.2", "jest": "^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..320141586b2 --- /dev/null +++ b/packages/controller-utils/src/create-service-policy.test.ts @@ -0,0 +1,589 @@ +import { useFakeTimers } from 'sinon'; +import type { SinonFakeTimers } from 'sinon'; + +import { + createServicePolicy, + DEFAULT_CIRCUIT_BREAK_DURATION, + 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('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(); + }); + }); + + 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(); + }); + }); + + 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('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 }); + }); + }); + }); + }); + + 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 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('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('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('returns what the service returns if it is successfully called again after the default 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' }); + }); + }); + }); + }); +}); + +/** + * 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..40651df3c5f --- /dev/null +++ b/packages/controller-utils/src/create-service-policy.ts @@ -0,0 +1,128 @@ +import { + circuitBreaker, + ConsecutiveBreaker, + ExponentialBackoff, + handleAll, + retry, + wrap, +} 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; + +/** + * 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.onBreak - A function which is called when the service fails + * too many times in a row (specifically, more than `maxConsecutiveFailures`). + * @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, + * onBreak: () => { + * console.log('Circuit broke'); + * }, + * }); + * } + * + * 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, + onBreak = () => { + // do nothing + }, + onRetry = () => { + // do nothing + }, +}: { + maxConsecutiveFailures?: number; + onBreak?: () => 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 the default circuit break duration 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: DEFAULT_CIRCUIT_BREAK_DURATION, + 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); + + // 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/yarn.lock b/yarn.lock index 6d2be506d2b..d28def18d56 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2550,11 +2550,13 @@ __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" 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" From 94ac34e18b387d1107fb4f55b86f29ca8768ccad Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Tue, 14 Jan 2025 18:22:22 +0100 Subject: [PATCH 06/16] feat: persist `isAccountSyncingReadyToBeDispatched` statue value (#5147) ## Explanation This PR changes the `isAccountSyncingReadyToBeDispatched` state value so it is persisted. ## References ## Changelog ### `@metamask/profile-sync-controller` - **CHANGED**: `isAccountSyncingReadyToBeDispatched` state value is now persisted ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've highlighted breaking changes using the "BREAKING" category above as appropriate - [x] I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes --- .../src/controllers/user-storage/UserStorageController.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 619336ccae4..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: { From 74f6d9c7ab0af8746fd16e2ad656b8ba77143855 Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Tue, 14 Jan 2025 18:36:56 +0100 Subject: [PATCH 07/16] Release 283.0.0 (#5148) ## Explanation This is a RC for v283.0.0. See changelog for more details @metamask/profile-sync-controller@4.1.0 ## References ## Changelog ```ms ### Changed - Bump `@metamask/profile-sync-controller` from `^4.0.1` to `^4.1.0` ([#5148](https://github.com/MetaMask/core/pull/5148)) ``` ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've highlighted breaking changes using the "BREAKING" category above as appropriate - [x] I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes --- package.json | 2 +- packages/notification-services-controller/package.json | 2 +- packages/profile-sync-controller/CHANGELOG.md | 9 ++++++++- packages/profile-sync-controller/package.json | 2 +- yarn.lock | 4 ++-- 5 files changed, 13 insertions(+), 6 deletions(-) diff --git a/package.json b/package.json index 123572835c8..acd475a8e82 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@metamask/core-monorepo", - "version": "282.0.0", + "version": "283.0.0", "private": true, "description": "Monorepo for packages shared between MetaMask clients", "repository": { diff --git a/packages/notification-services-controller/package.json b/packages/notification-services-controller/package.json index bd0d95d4a3b..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.1", + "@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/profile-sync-controller/CHANGELOG.md b/packages/profile-sync-controller/CHANGELOG.md index 3e21a911ae4..5cf6aa5b8d8 100644 --- a/packages/profile-sync-controller/CHANGELOG.md +++ b/packages/profile-sync-controller/CHANGELOG.md @@ -7,6 +7,12 @@ 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 @@ -416,7 +422,8 @@ 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.1...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 diff --git a/packages/profile-sync-controller/package.json b/packages/profile-sync-controller/package.json index d109e073f9c..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.1", + "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/yarn.lock b/yarn.lock index d28def18d56..3401672a508 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3415,7 +3415,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.1" + "@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" @@ -3596,7 +3596,7 @@ __metadata: languageName: unknown linkType: soft -"@metamask/profile-sync-controller@npm:^4.0.1, @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: From c5d49d7ba5c393406da14827a8ee52840b57bd24 Mon Sep 17 00:00:00 2001 From: OGPoyraz Date: Tue, 14 Jan 2025 19:15:34 +0100 Subject: [PATCH 08/16] fix: Migrate `AbstractMessageManager` from `BaseControllerV1` to `BaseControllerV2` (#5103) ## Explanation This PR aims to remove `BaseControllerV1` usage from `AbstractMessageManager`. As expected this change affected both `DecryptMessageManager` (`DMM`) and `EncryptionPublicKeyManager`(`EPKM`). Since extension already have wrapper classes for both `DMM` & `EPKM` in the extension code, we want to keep changes minimal and make these both classes in the core work like controller but let wrappers sync the state in their classes as we currently do. You can find the extension PR in the references below. ## References * Fixes : https://github.com/MetaMask/MetaMask-planning/issues/3747 * Related extension PR: https://github.com/MetaMask/metamask-extension/pull/29237 ## Changelog ### `@metamask/message-manager` - **BREAKING:** Base class of `DecryptMessageManager` and `EncryptionPublicKeyManager`(`AbstractMessageManager`) now expects new options to initialise - **BREAKING:** Removed internal event emitter (`hub` property) from `AbstractMessageManager` - **BREAKING:** `unapprovedMessage` and `updateBadge` removed from internal events. These events are now emitted from messaging system - Controllers should now listen to `DerivedManagerName:X` event instead of using internal event emitter. ## Checklist - [X] I've updated the test suite for new or updated code as appropriate - [X] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [X] I've highlighted breaking changes using the "BREAKING" category above as appropriate - [X] I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes --- packages/message-manager/CHANGELOG.md | 7 + .../src/AbstractMessageManager.test.ts | 129 ++++--- .../src/AbstractMessageManager.ts | 319 ++++++++++-------- .../src/DecryptMessageManager.test.ts | 32 +- .../src/DecryptMessageManager.ts | 133 +++++--- .../src/EncryptionPublicKeyManager.test.ts | 33 +- .../src/EncryptionPublicKeyManager.ts | 126 ++++--- 7 files changed, 492 insertions(+), 287 deletions(-) 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..10bcc6d4520 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 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, }); From 7409b3fa6eddc1fbc3dad4a55b59db676b7c3211 Mon Sep 17 00:00:00 2001 From: Elliot Winkler Date: Tue, 14 Jan 2025 11:56:06 -0700 Subject: [PATCH 09/16] Fix ESLint config (#5132) The upgrade to ESLint 9 which occurred in a previous commit was not performed correctly. It seems that an array must be passed to `createConfig` instead of a series of arguments, and that was not done. As a result, a lot of lint rules were disabled accidentally. This commit fixes the ESLint config and corrects any lint violations. Unsurprisingly, upgrading the ESLint packages created a bunch of new lint violations, so those have been turned into warnings rather than errors in this commit. To prevent new instances of these violations from occurring, however, a script has been added which acts as a quality gate, running `eslint` as before, but then capturing existing warning counts and comparing them to a previously generated threshold file, exiting with non-zero if there is an increase in warnings for any particular ESLint rule. This script is also now used for `yarn lint:eslint` so that CI will fail if the count increases. --- docs/contributing.md | 4 +- eslint-warning-thresholds.json | 29 ++ eslint.config.mjs | 129 +++++--- package.json | 2 +- packages/controller-utils/package.json | 1 + packages/controller-utils/src/util.ts | 2 +- packages/json-rpc-engine/src/JsonRpcEngine.ts | 2 +- .../src/index.test.ts | 2 +- .../src/KeyringController.ts | 1 - .../src/AbstractMessageManager.ts | 2 +- .../src/create-network-client.ts | 2 - .../tests/NetworkController.test.ts | 7 +- .../services/push/push-web.ts | 2 +- .../src/PermissionController.ts | 1 - .../src/PhishingDetector.test.ts | 2 +- .../src/AbstractPollingController.ts | 1 - .../network-syncing/controller-integration.ts | 2 +- .../tests/SelectedNetworkController.test.ts | 2 +- .../src/SignatureController.ts | 4 +- .../src/TransactionController.ts | 3 +- .../src/helpers/GasFeePoller.ts | 2 +- .../src/helpers/IncomingTransactionHelper.ts | 2 +- .../src/helpers/MethodDataHelper.ts | 2 +- .../src/helpers/PendingTransactionTracker.ts | 2 +- .../src/UserOperationController.ts | 2 +- .../helpers/PendingUserOperationTracker.ts | 2 +- scripts/run-eslint.ts | 289 ++++++++++++++++++ yarn.config.cjs | 7 +- yarn.lock | 13 +- 29 files changed, 448 insertions(+), 73 deletions(-) create mode 100644 eslint-warning-thresholds.json create mode 100644 scripts/run-eslint.ts 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/eslint-warning-thresholds.json b/eslint-warning-thresholds.json new file mode 100644 index 00000000000..dcad81bed13 --- /dev/null +++ b/eslint-warning-thresholds.json @@ -0,0 +1,29 @@ +{ + "@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": 152, + "@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": 104, + "jsdoc/check-tag-names": 372, + "jsdoc/require-returns": 22, + "jsdoc/tag-lines": 329, + "n/no-unsupported-features/node-builtins": 18, + "n/prefer-global/text-encoder": 4, + "n/prefer-global/text-decoder": 4, + "prettier/prettier": 116, + "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": 6 +} diff --git a/eslint.config.mjs b/eslint.config.mjs index ec01ff56f2b..91185d8cb80 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,36 @@ 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', + }, }, { // 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 +95,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 +127,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 +136,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 +188,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/package.json b/package.json index acd475a8e82..a52e92956af 100644 --- a/package.json +++ b/package.json @@ -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/package.json b/packages/controller-utils/package.json index cb2ec2ee882..4cc3ceb4bf2 100644 --- a/packages/controller-utils/package.json +++ b/packages/controller-utils/package.json @@ -65,6 +65,7 @@ "@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", 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/src/AbstractMessageManager.ts b/packages/message-manager/src/AbstractMessageManager.ts index 10bcc6d4520..7027acd6862 100644 --- a/packages/message-manager/src/AbstractMessageManager.ts +++ b/packages/message-manager/src/AbstractMessageManager.ts @@ -7,7 +7,7 @@ import type { import type { ApprovalType } from '@metamask/controller-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'; 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/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/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..cabf068470b --- /dev/null +++ b/scripts/run-eslint.ts @@ -0,0 +1,289 @@ +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 formatter = await eslint.loadFormatter('stylish'); + const resultText = formatter.format(results); + console.log(resultText); + + if (options.quiet) { + results = ESLint.getErrorResults(results); + } + + 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', + ); + } + for (const { ruleId, threshold, count, difference } of changes) { + console.log( + `- ${ruleId}: ${threshold} -> ${count} (${difference > 0 ? '+' : ''}${difference})`, + ); + } + // We are still seeing differences on CI when it comes to linting results. + // Only write the thresholds file if there are regressions in that case + // to prevent the "working directory clean" step in CI from failing. + // Also, it's okay to use `process.env` as this is a script. + // eslint-disable-next-line n/no-process-env + if (regressions.length > 0 || !process.env.CI) { + saveWarningThresholds(warningCounts); + } + } + } +} + +/** + * 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 3401672a508..f88317fe241 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2555,6 +2555,7 @@ __metadata: 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" @@ -5784,11 +5785,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 @@ -10933,9 +10934,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 From 0429d92a124e36c73cc81b67a75fda2be9724641 Mon Sep 17 00:00:00 2001 From: Elliot Winkler Date: Tue, 14 Jan 2025 12:57:51 -0700 Subject: [PATCH 10/16] Add `onDegraded`, `degradedThreshold`, `circuitBreakDuration` to `createServicePolicy` (#5143) This commit is a continuation of a previous commit which added a utility function for use in service classes which ensures that they follow the circuit breaker and retry patterns. This commit adds three more options to `createServicePolicy`: - `onDegraded`, a callback which handles when the service succeeds but takes too long to run - `degradedThreshold`, the duration which counts as "too long" - `circuitBreakDuration`, which governs how long the circuit stays open after breaking before the service is re-evaluated --- packages/controller-utils/CHANGELOG.md | 2 +- .../src/create-service-policy.test.ts | 568 +++++++++++++++++- .../src/create-service-policy.ts | 57 +- 3 files changed, 616 insertions(+), 11 deletions(-) diff --git a/packages/controller-utils/CHANGELOG.md b/packages/controller-utils/CHANGELOG.md index 313dc0fdd3e..8d5452c1495 100644 --- a/packages/controller-utils/CHANGELOG.md +++ b/packages/controller-utils/CHANGELOG.md @@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added -- Add `createServicePolicy` function to assist with reducing boilerplate for service classes ([#5053](https://github.com/MetaMask/core/pull/5053)) +- 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] diff --git a/packages/controller-utils/src/create-service-policy.test.ts b/packages/controller-utils/src/create-service-policy.test.ts index 320141586b2..800023b5e25 100644 --- a/packages/controller-utils/src/create-service-policy.test.ts +++ b/packages/controller-utils/src/create-service-policy.test.ts @@ -4,6 +4,7 @@ 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'; @@ -47,6 +48,66 @@ describe('createServicePolicy', () => { 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', () => { @@ -121,6 +182,24 @@ describe('createServicePolicy', () => { 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', () => { @@ -167,6 +246,28 @@ describe('createServicePolicy', () => { 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', () => { @@ -212,6 +313,28 @@ describe('createServicePolicy', () => { 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'); @@ -284,6 +407,28 @@ describe('createServicePolicy', () => { 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(); + }); }); }); }); @@ -377,6 +522,114 @@ describe('createServicePolicy', () => { 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', () => { @@ -431,6 +684,126 @@ describe('createServicePolicy', () => { 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', () => { @@ -485,6 +858,128 @@ describe('createServicePolicy', () => { 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', () => { @@ -545,7 +1040,7 @@ describe('createServicePolicy', () => { expect(onBreak).toHaveBeenCalledWith({ error }); }); - it('returns what the service returns if it is successfully called again after the default circuit break duration has elapsed', async () => { + it('does not call the onDegraded callback', async () => { const maxConsecutiveFailures = DEFAULT_MAX_RETRIES; let invocationCounter = 0; const error = new Error('failure'); @@ -556,20 +1051,81 @@ describe('createServicePolicy', () => { } throw error; }; + const onDegraded = jest.fn(); const policy = createServicePolicy({ maxConsecutiveFailures, + onDegraded, }); - const firstExecution = policy.execute(mockService); + 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(firstExecution); - clock.tick(DEFAULT_CIRCUIT_BREAK_DURATION); - const result = await policy.execute(mockService); + await ignoreRejection(promise); - expect(result).toStrictEqual({ some: 'data' }); + 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' }); + }); }); }); }); diff --git a/packages/controller-utils/src/create-service-policy.ts b/packages/controller-utils/src/create-service-policy.ts index 40651df3c5f..c985dba9e2d 100644 --- a/packages/controller-utils/src/create-service-policy.ts +++ b/packages/controller-utils/src/create-service-policy.ts @@ -5,6 +5,7 @@ import { handleAll, retry, wrap, + CircuitState, } from 'cockatiel'; import type { IPolicy } from 'cockatiel'; @@ -28,6 +29,12 @@ export const DEFAULT_MAX_CONSECUTIVE_FAILURES = (1 + DEFAULT_MAX_RETRIES) * 3; */ 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 @@ -46,8 +53,17 @@ export const DEFAULT_CIRCUIT_BREAK_DURATION = 30 * 60 * 1000; * @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. @@ -60,9 +76,14 @@ export const DEFAULT_CIRCUIT_BREAK_DURATION = 30 * 60 * 1000; * constructor() { * this.#policy = createServicePolicy({ * maxConsecutiveFailures: 3, + * circuitBreakDuration: 5000, + * degradedThreshold: 2000, * onBreak: () => { * console.log('Circuit broke'); * }, + * onDegraded: () => { + * console.log('Service is degraded'); + * }, * }); * } * @@ -77,15 +98,23 @@ export const DEFAULT_CIRCUIT_BREAK_DURATION = 30 * 60 * 1000; */ 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, { @@ -101,10 +130,10 @@ export function createServicePolicy({ // 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 the default circuit break duration 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: DEFAULT_CIRCUIT_BREAK_DURATION, + // 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), }); @@ -122,6 +151,26 @@ export function createServicePolicy({ // 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); From 532b0c9c8b69fbb7ff54b0fa8afeea1da908f5c7 Mon Sep 17 00:00:00 2001 From: Elliot Winkler Date: Tue, 14 Jan 2025 13:56:25 -0700 Subject: [PATCH 11/16] Fix ESLint script (#5150) There are a few problems with the ESLint script. First, the `--quiet` option (which is supposed to only show errors) doesn't work. That's not a big deal, we can fix this easily. Second, the thresholds file is updated if there are regressions (i.e. any rules that have more violations than a previous run of `eslint`). That's not right; we should update the thresholds file if we've seen improvements (all rules have the same or a fewer number of violations). More importantly, however, the script doesn't exit with 1 if there are any lint violations. This is a big problem because it means that even if a PR introduces violations, CI will pass. In fact, some violations have slipped into the codebase in a recent commit. So that we can get this PR approved without requiring other teams, this commit disables those rules globally, and we will re-enable them in another commit. --- eslint-warning-thresholds.json | 14 ++++++++------ eslint.config.mjs | 2 ++ scripts/run-eslint.ts | 28 ++++++++++++++++------------ 3 files changed, 26 insertions(+), 18 deletions(-) diff --git a/eslint-warning-thresholds.json b/eslint-warning-thresholds.json index dcad81bed13..fc7d2173b69 100644 --- a/eslint-warning-thresholds.json +++ b/eslint-warning-thresholds.json @@ -6,24 +6,26 @@ "@typescript-eslint/no-unsafe-enum-comparison": 59, "@typescript-eslint/no-unused-vars": 36, "@typescript-eslint/prefer-promise-reject-errors": 13, - "@typescript-eslint/prefer-readonly": 152, + "@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": 104, - "jsdoc/check-tag-names": 372, + "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": 329, + "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": 116, + "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": 6 + "no-unused-private-class-members": 5 } diff --git a/eslint.config.mjs b/eslint.config.mjs index 91185d8cb80..dec0b60327c 100644 --- a/eslint.config.mjs +++ b/eslint.config.mjs @@ -80,6 +80,8 @@ const config = createConfig([ // 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', }, }, { diff --git a/scripts/run-eslint.ts b/scripts/run-eslint.ts index cabf068470b..1c70fac5c30 100644 --- a/scripts/run-eslint.ts +++ b/scripts/run-eslint.ts @@ -94,15 +94,20 @@ async function runESLint( options: { quiet: boolean; fix: boolean }, ): Promise { let results = await eslint.lintFiles(['.']); + const errorResults = ESLint.getErrorResults(results); - const formatter = await eslint.loadFormatter('stylish'); - const resultText = formatter.format(results); - console.log(resultText); + if (errorResults.length > 0) { + process.exitCode = 1; + } if (options.quiet) { - results = ESLint.getErrorResults(results); + results = errorResults; } + const formatter = await eslint.loadFormatter('stylish'); + const resultText = formatter.format(results); + console.log(resultText); + if (options.fix) { await ESLint.outputFixes(results); } @@ -156,20 +161,19 @@ function evaluateWarnings(results: ESLint.LintResult[]) { 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})`, ); } - // We are still seeing differences on CI when it comes to linting results. - // Only write the thresholds file if there are regressions in that case - // to prevent the "working directory clean" step in CI from failing. - // Also, it's okay to use `process.env` as this is a script. - // eslint-disable-next-line n/no-process-env - if (regressions.length > 0 || !process.env.CI) { - saveWarningThresholds(warningCounts); - } } } } From af90e6780c66cfdc734d6482c87a597c4c4deb36 Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Tue, 14 Jan 2025 18:57:18 -0330 Subject: [PATCH 12/16] chore(docs): Rename `ControllerMessenger` to `Messenger` (#5089) ## Explanation Rename `ControllerMessenger` to `Messenger` in the controller docs and examples. ## References Relates to #4538 ## Changelog N/A ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've highlighted breaking changes using the "BREAKING" category above as appropriate - [x] I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes --- docs/writing-controllers.md | 64 +++++++++---------- .../src/gas-prices-controller.test.ts | 20 +++--- .../src/gas-prices-controller.ts | 8 +-- .../src/pet-names-controller.test.ts | 27 ++++---- .../src/pet-names-controller.ts | 8 +-- 5 files changed, 59 insertions(+), 68 deletions(-) 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/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 * >(); From f92d7a3f8c6bfc7807e0e2dcdb49e89d8be7884f Mon Sep 17 00:00:00 2001 From: Elliot Winkler Date: Wed, 15 Jan 2025 12:12:22 -0700 Subject: [PATCH 13/16] Relax switch-exhaustiveness-check lint rule (#5157) The default behavior for this rule enforces that a switch statement handle all possible values that the given variable could be, including type unions. This rule seems to be acting differently in different environments, especially CI, preventing developers from merging PRs. We could disable this rule entirely, but let's try relaxing it a bit to see if that improves the situation. --- eslint.config.mjs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/eslint.config.mjs b/eslint.config.mjs index dec0b60327c..2ed81691745 100644 --- a/eslint.config.mjs +++ b/eslint.config.mjs @@ -117,6 +117,14 @@ const config = createConfig([ }, }, rules: { + // These rules have been customized from their defaults. + '@typescript-eslint/switch-exhaustiveness-check': [ + 'error', + { + considerDefaultExhaustiveForUnions: true, + }, + ], + // 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 @@ -151,7 +159,6 @@ const config = createConfig([ '@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', From f41895209e150285ca83d4327ab180c9b506d746 Mon Sep 17 00:00:00 2001 From: Elliot Winkler Date: Wed, 15 Jan 2025 13:08:01 -0700 Subject: [PATCH 14/16] Update ESLint warning thresholds, and produce them consistently (#5151) The ESLint warning thresholds file records warnings that were produced the last time that `eslint` was run, categorizing them by rule. This file should be kept up to date at all times so we can know whether new code changes introduce a regression in the number of warnings or an improvement. Specifically, the lint step will now fail if more warnings are introduced to the codebase. However, in the past we have observed inconsistencies in the behavior of ESLint across different people's machines as well as in CI. With the addition of the quality gate this means that some people are unable to merge PRs because warnings are produced that are not seen in development, and the warnings may not be reproducible by other developers. With that said, we recently learned that if `dist` directories are present from a previous run of `yarn build`, it could affect the behavior of the TypeScript-specific lint rules and cause the observed inconsistencies. To mitigate this problem, this commit ensures that all `dist` directories are removed before running `eslint`. This indeed does change the number of warnings produced, and the thresholds file has been updated to reflect this. --- eslint-warning-thresholds.json | 6 ++---- package.json | 5 +++-- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/eslint-warning-thresholds.json b/eslint-warning-thresholds.json index fc7d2173b69..92cc78a3f77 100644 --- a/eslint-warning-thresholds.json +++ b/eslint-warning-thresholds.json @@ -2,16 +2,14 @@ "@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-unsafe-enum-comparison": 34, "@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, + "import-x/order": 205, "jest/no-conditional-in-test": 129, "jest/prefer-lowercase-title": 2, "jest/prefer-strict-equal": 2, diff --git a/package.json b/package.json index a52e92956af..67464d8c8b3 100644 --- a/package.json +++ b/package.json @@ -14,7 +14,8 @@ ], "scripts": { "build": "yarn ts-bridge --project tsconfig.build.json --verbose", - "build:clean": "rimraf dist '**/*.tsbuildinfo' && yarn build", + "build:clean": "yarn build:only-clean && yarn build", + "build:only-clean": "rimraf -g 'packages/*/dist'", "build:docs": "yarn workspaces foreach --all --no-private --parallel --interlaced --verbose run build:docs", "build:types": "tsc --build tsconfig.build.json --verbose", "changelog:update": "yarn workspaces foreach --all --no-private --parallel --interlaced --verbose run changelog:update", @@ -23,7 +24,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": "yarn ts-node ./scripts/run-eslint.ts --cache", + "lint:eslint": "yarn build:only-clean && 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", From 875e8a19eaddcbb95599828db6ff3459b17dcecc Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Thu, 16 Jan 2025 11:14:26 +0100 Subject: [PATCH 15/16] test(accounts-controller): improve uuid mocks (#5005) ## Explanation Unify the UUID mocking. With this new refactor we longer have to specify each returned values of `mockUUID`. It also handles the non-registered accounts (in this case, a random UUID will be generated). ## References N/A ## Changelog N/A (those changes only impact tests) ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've highlighted breaking changes using the "BREAKING" category above as appropriate - [x] I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes --- .../src/AccountsController.test.ts | 135 +++++++++--------- 1 file changed, 65 insertions(+), 70 deletions(-) diff --git a/packages/accounts-controller/src/AccountsController.test.ts b/packages/accounts-controller/src/AccountsController.test.ts index b28666d1523..8c612d1e18c 100644 --- a/packages/accounts-controller/src/AccountsController.test.ts +++ b/packages/accounts-controller/src/AccountsController.test.ts @@ -133,9 +133,6 @@ const mockAccount4: InternalAccount = { }, }; -/** - * Mock generated normal account ID to an actual "hard-coded" one. - */ class MockNormalAccountUUID { #accountIds: Record = {}; @@ -158,6 +155,18 @@ class MockNormalAccountUUID { } } +/** + * Mock generated normal account ID to their actual mock ID. This function will + * automatically attaches those accounts to `mockUUID`. A random UUID will be + * generated if an account has not been registered. See {@link MockNormalAccountUUID}. + * + * @param accounts - List of normal accounts to map with their mock ID. + */ +function mockUUIDWithNormalAccounts(accounts: InternalAccount[]) { + const mockAccountUUIDs = new MockNormalAccountUUID(accounts); + mockUUID.mockImplementation(mockAccountUUIDs.mock.bind(mockAccountUUIDs)); +} + /** * Creates an `InternalAccount` object from the given normal account properties. * @@ -466,9 +475,8 @@ describe('AccountsController', () => { describe('onKeyringStateChange', () => { it('uses listMultichainAccounts', async () => { const messenger = buildMessenger(); - mockUUID - .mockReturnValueOnce('mock-id') // call to check if its a new account - .mockReturnValueOnce('mock-id2'); // call to add account + + mockUUIDWithNormalAccounts([mockAccount, mockAccount2]); const { accountsController } = setupAccountsController({ initialState: { @@ -562,10 +570,8 @@ describe('AccountsController', () => { describe('adding accounts', () => { it('add new accounts', async () => { const messenger = buildMessenger(); - mockUUID - .mockReturnValueOnce('mock-id') // call to check if its a new account - .mockReturnValueOnce('mock-id2') // call to check if its a new account - .mockReturnValueOnce('mock-id2'); // call to add account + + mockUUIDWithNormalAccounts([mockAccount, mockAccount2, mockAccount3]); const mockNewKeyringState = { isUnlocked: true, @@ -604,7 +610,7 @@ describe('AccountsController', () => { }); it('add Snap accounts', async () => { - mockUUID.mockReturnValueOnce('mock-id'); // call to check if its a new account + mockUUIDWithNormalAccounts([mockAccount]); const messenger = buildMessenger(); messenger.registerActionHandler( @@ -671,7 +677,8 @@ describe('AccountsController', () => { }); it('handle the event when a Snap deleted the account before the it was added', async () => { - mockUUID.mockReturnValueOnce('mock-id'); // call to check if its a new account + mockUUIDWithNormalAccounts([mockAccount]); + const messenger = buildMessenger(); messenger.registerActionHandler( 'KeyringController:getKeyringsByType', @@ -729,11 +736,8 @@ describe('AccountsController', () => { it('increment the default account number when adding an account', async () => { const messenger = buildMessenger(); - mockUUID - .mockReturnValueOnce('mock-id') // call to check if its a new account - .mockReturnValueOnce('mock-id2') // call to check if its a new account - .mockReturnValueOnce('mock-id3') // call to check if its a new account - .mockReturnValueOnce('mock-id3'); // call to add account + + mockUUIDWithNormalAccounts([mockAccount, mockAccount2, mockAccount3]); const mockNewKeyringState = { isUnlocked: true, @@ -785,11 +789,8 @@ describe('AccountsController', () => { it('use the next number after the total number of accounts of a keyring when adding an account, if the index is lower', async () => { const messenger = buildMessenger(); - mockUUID - .mockReturnValueOnce('mock-id') // call to check if its a new account - .mockReturnValueOnce('mock-id2') // call to check if its a new account - .mockReturnValueOnce('mock-id3') // call to check if its a new account - .mockReturnValueOnce('mock-id3'); // call to add account + + mockUUIDWithNormalAccounts([mockAccount, mockAccount2, mockAccount3]); const mockAccount2WithCustomName = createExpectedInternalAccount({ id: 'mock-id2', @@ -847,7 +848,7 @@ describe('AccountsController', () => { }); it('handle when the account to set as selectedAccount is undefined', async () => { - mockUUID.mockReturnValueOnce('mock-id'); // call to check if its a new account + mockUUIDWithNormalAccounts([mockAccount]); const messenger = buildMessenger(); messenger.registerActionHandler( @@ -897,10 +898,8 @@ describe('AccountsController', () => { it('selectedAccount remains the same after adding a new account', async () => { const messenger = buildMessenger(); - mockUUID - .mockReturnValueOnce('mock-id') // call to check if its a new account - .mockReturnValueOnce('mock-id2') // call to check if its a new account - .mockReturnValueOnce('mock-id2'); // call to add account + + mockUUIDWithNormalAccounts([mockAccount, mockAccount2, mockAccount3]); const mockNewKeyringState = { isUnlocked: true, @@ -942,10 +941,8 @@ describe('AccountsController', () => { it('publishes accountAdded event', async () => { const messenger = buildMessenger(); const messengerSpy = jest.spyOn(messenger, 'publish'); - mockUUID - .mockReturnValueOnce(mockAccount.id) // call to check if its a new account - .mockReturnValueOnce(mockAccount2.id) // call to check if its a new account - .mockReturnValueOnce(mockAccount2.id); // call to add account + + mockUUIDWithNormalAccounts([mockAccount, mockAccount2]); setupAccountsController({ initialState: { @@ -987,7 +984,8 @@ describe('AccountsController', () => { describe('deleting account', () => { it('delete accounts if its gone from the keyring state', async () => { const messenger = buildMessenger(); - mockUUID.mockReturnValueOnce('mock-id2'); + + mockUUIDWithNormalAccounts([mockAccount2]); const mockNewKeyringState = { isUnlocked: true, @@ -1027,11 +1025,8 @@ describe('AccountsController', () => { it('delete accounts and set the most recent lastSelected account', async () => { const messenger = buildMessenger(); - mockUUID - .mockReturnValueOnce('mock-id') - .mockReturnValueOnce('mock-id2') - .mockReturnValueOnce('mock-id') - .mockReturnValueOnce('mock-id2'); + + mockUUIDWithNormalAccounts([mockAccount, mockAccount2]); const mockNewKeyringState = { isUnlocked: true, @@ -1083,11 +1078,8 @@ describe('AccountsController', () => { it('delete accounts and set the most recent lastSelected account when there are accounts that have never been selected', async () => { const messenger = buildMessenger(); - mockUUID - .mockReturnValueOnce('mock-id') - .mockReturnValueOnce('mock-id2') - .mockReturnValueOnce('mock-id') - .mockReturnValueOnce('mock-id2'); + + mockUUIDWithNormalAccounts([mockAccount, mockAccount2]); const mockAccount2WithoutLastSelected = { ...mockAccount2, @@ -1147,7 +1139,8 @@ describe('AccountsController', () => { it('delete the account and select the account with the most recent lastSelected', async () => { const currentTime = Date.now(); const messenger = buildMessenger(); - mockUUID.mockReturnValueOnce('mock-id').mockReturnValueOnce('mock-id2'); + + mockUUIDWithNormalAccounts([mockAccount, mockAccount2]); const mockAccountWithoutLastSelected = { ...mockAccount, @@ -1222,10 +1215,8 @@ describe('AccountsController', () => { it('publishes accountRemoved event', async () => { const messenger = buildMessenger(); const messengerSpy = jest.spyOn(messenger, 'publish'); - mockUUID - .mockReturnValueOnce(mockAccount.id) // call to check if its a new account - .mockReturnValueOnce(mockAccount2.id) // call to check if its a new account - .mockReturnValueOnce(mockAccount2.id); // call to add account + + mockUUIDWithNormalAccounts([mockAccount, mockAccount2]); setupAccountsController({ initialState: { @@ -1278,9 +1269,11 @@ describe('AccountsController', () => { address: '0x456', keyringType: KeyringTypes.hd, }); - mockUUID - .mockReturnValueOnce('mock-id2') // call to check if its a new account - .mockReturnValueOnce('mock-id2'); // call to add account + + mockUUIDWithNormalAccounts([ + mockInitialAccount, + mockReinitialisedAccount, + ]); const mockNewKeyringState = { isUnlocked: true, @@ -1361,9 +1354,7 @@ describe('AccountsController', () => { }); mockExistingAccount2.metadata.lastSelected = lastSelectedForAccount2; - mockUUID - .mockReturnValueOnce('mock-id') // call to check if its a new account - .mockReturnValueOnce('mock-id2'); // call to check if its a new account + mockUUIDWithNormalAccounts([mockAccount, mockAccount2]); const { accountsController } = setupAccountsController({ initialState: { @@ -1447,7 +1438,8 @@ describe('AccountsController', () => { }); it('update accounts with normal accounts', async () => { - mockUUID.mockReturnValueOnce('mock-id').mockReturnValueOnce('mock-id2'); + mockUUIDWithNormalAccounts([mockAccount, mockAccount2]); + const messenger = buildMessenger(); messenger.registerActionHandler( 'KeyringController:getAccounts', @@ -1492,6 +1484,7 @@ describe('AccountsController', () => { keyringType: KeyringTypes.hd, }), ]; + mockUUIDWithNormalAccounts(expectedAccounts); await accountsController.updateAccounts(); @@ -1588,7 +1581,8 @@ describe('AccountsController', () => { }); it('set the account with the correct index', async () => { - mockUUID.mockReturnValueOnce('mock-id').mockReturnValueOnce('mock-id2'); + mockUUIDWithNormalAccounts([mockAccount]); + const messenger = buildMessenger(); messenger.registerActionHandler( 'KeyringController:getAccounts', @@ -1630,6 +1624,7 @@ describe('AccountsController', () => { keyringType: KeyringTypes.hd, }), ]; + mockUUIDWithNormalAccounts(expectedAccounts); await accountsController.updateAccounts(); @@ -1639,7 +1634,8 @@ describe('AccountsController', () => { }); it('filter Snap accounts from normalAccounts', async () => { - mockUUID.mockReturnValueOnce('mock-id'); + mockUUIDWithNormalAccounts([mockAccount]); + const messenger = buildMessenger(); messenger.registerActionHandler( 'KeyringController:getKeyringsByType', @@ -1696,7 +1692,8 @@ describe('AccountsController', () => { }); it('filter Snap accounts from normalAccounts even if the snap account is listed before normal accounts', async () => { - mockUUID.mockReturnValue('mock-id'); + mockUUIDWithNormalAccounts([mockAccount]); + const messenger = buildMessenger(); messenger.registerActionHandler( 'KeyringController:getKeyringsByType', @@ -1762,7 +1759,7 @@ describe('AccountsController', () => { KeyringTypes.qr, 'Custody - JSON - RPC', ])('should add accounts for %s type', async (keyringType) => { - mockUUID.mockReturnValue('mock-id'); + mockUUIDWithNormalAccounts([mockAccount]); const messenger = buildMessenger(); messenger.registerActionHandler( @@ -1811,7 +1808,7 @@ describe('AccountsController', () => { }); it('throw an error if the keyring type is unknown', async () => { - mockUUID.mockReturnValue('mock-id'); + mockUUIDWithNormalAccounts([mockAccount]); const messenger = buildMessenger(); messenger.registerActionHandler( @@ -1892,9 +1889,7 @@ describe('AccountsController', () => { }); mockExistingAccount2.metadata.lastSelected = lastSelectedForAccount2; - mockUUID - .mockReturnValueOnce('mock-id') // call to check if its a new account - .mockReturnValueOnce('mock-id2'); // call to check if its a new account + mockUUIDWithNormalAccounts([mockAccount, mockAccount2]); messenger.registerActionHandler( 'KeyringController:getKeyringsByType', @@ -2544,12 +2539,12 @@ describe('AccountsController', () => { it('return the next account number', async () => { const messenger = buildMessenger(); - mockUUID - .mockReturnValueOnce('mock-id') // call to check if its a new account - .mockReturnValueOnce('mock-id2') // call to check if its a new account - .mockReturnValueOnce('mock-id3') // call to check if its a new account - .mockReturnValueOnce('mock-id2') // call to add account - .mockReturnValueOnce('mock-id3'); // call to add account + + mockUUIDWithNormalAccounts([ + mockAccount, + mockSimpleKeyring1, + mockSimpleKeyring2, + ]); const { accountsController } = setupAccountsController({ initialState: { @@ -2582,13 +2577,13 @@ describe('AccountsController', () => { it('return the next account number even with an index gap', async () => { const messenger = buildMessenger(); - const mockAccountUUIDs = new MockNormalAccountUUID([ + + mockUUIDWithNormalAccounts([ mockAccount, mockSimpleKeyring1, mockSimpleKeyring2, mockSimpleKeyring3, ]); - mockUUID.mockImplementation(mockAccountUUIDs.mock.bind(mockAccountUUIDs)); const { accountsController } = setupAccountsController({ initialState: { From 21c4ec654786b994196adf68474cd1e9401804bc Mon Sep 17 00:00:00 2001 From: sahar-fehri Date: Thu, 16 Jan 2025 13:18:09 +0100 Subject: [PATCH 16/16] fix: add thresshold for update NFT metadata (#5134) ## Explanation When `onPreferencesControllerStateChange` is triggered inside nftController, it also triggers a [check](https://github.com/MetaMask/core/blob/76af6ebde290a270f4da6493f0f3b6c9354545cb/packages/assets-controllers/src/NftController.ts#L431) if nfts saved in state needs to be updated. We keep retrying to update NFTs (fetching their metadata) when they have no image, name or description. Note: When switching newtworks; this [line](https://github.com/MetaMask/metamask-extension/blob/b91a9622393f37b79a63ba4a240cda7efa3cc5bc/ui/components/multichain/network-list-menu/network-list-menu.tsx#L296) is the trigger for the `onPreferencesControllerStateChange`. While the updateNftUpdateForAccount should be triggered once for NFTs that actually have a (name, description, image) We will keep retrying for NFTs that don't. (Could be nfts created for testing purposes or some raffle tickets do not have those fields). We had a user reporting that he is seeing an excessive amount of RPC calls made on one of his accounts on Arbitrum Sepolia. This account holds more than a 1000 NFT. Every time he switches the network to Arbitrum Sepolia, he sees more than a 1000 eth_call made. That was the result of updateNftUpdateForAccount being triggered everytime the network is switched + The nfts not having (name,image, description) This PR adds a 500 threshold to avoid sending an excessive amount of RPC calls to fetch nft metadata and adds a check if the triggered onPreferencesControllerStateChange updates any of the concerned params ( ipfsGateway, openseaEnabled, isIpfsGatewayEnabled) **_Better improvement:_** A better improvement for this would be to add support for [BalanceScanner](https://github.com/MetaMask/metafi-sdk/blob/main/packages/eth-scan/contracts/BalanceScanner.sol) contract, which allows to fetch tokenURI for multiple addresses and tokenIds with fct tokenURIsofTokenIdsForTokens. We will control how often this is refreshed. For users that have nfts > NFT_UPDATE_THRESHOLD, we save the timestamp of the first update in a mapping chainId => lastTimestamp of updateNftMetadata, and only update after a specific period of time NFT_UPDATE_REFRESH_PERIOD = 48hours. ## References ## Changelog ### `@metamask/controller-utils` - **ADDED**: Added NFT_UPDATE_THRESHOLD constant ### `@metamask/assets-controllers` - **CHANGED**: Added a check if the nftsToUpdate is less than NFT_UPDATE_THRESHOLD inside `updateNftUpdateForAccount` ## Checklist - [ ] I've updated the test suite for new or updated code as appropriate - [ ] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [ ] I've highlighted breaking changes using the "BREAKING" category above as appropriate - [ ] I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes --------- Co-authored-by: Michele Esposito <34438276+mikesposito@users.noreply.github.com> --- .../src/NftController.test.ts | 43 +++++++++++++++++++ .../assets-controllers/src/NftController.ts | 42 +++++++++++------- 2 files changed, 69 insertions(+), 16 deletions(-) diff --git a/packages/assets-controllers/src/NftController.test.ts b/packages/assets-controllers/src/NftController.test.ts index dc9c9a4a8f2..49e48178c7a 100644 --- a/packages/assets-controllers/src/NftController.test.ts +++ b/packages/assets-controllers/src/NftController.test.ts @@ -4471,6 +4471,49 @@ describe('NftController', () => { }); describe('updateNftMetadata', () => { + it('should not update Nft metadata when preferences change and current and incoming state are the same', async () => { + const { + nftController, + triggerPreferencesStateChange, + triggerSelectedAccountChange, + } = setupController(); + const spy = jest.spyOn(nftController, 'updateNftMetadata'); + triggerSelectedAccountChange(OWNER_ACCOUNT); + // trigger preference change + triggerPreferencesStateChange({ + ...getDefaultPreferencesState(), + }); + + expect(spy).toHaveBeenCalledTimes(0); + }); + + it('should call update Nft metadata when preferences change is triggered and at least ipfsGateway, openSeaEnabled or isIpfsGatewayEnabled change', async () => { + const { + nftController, + mockGetAccount, + triggerPreferencesStateChange, + triggerSelectedAccountChange, + } = setupController({ + defaultSelectedAccount: OWNER_ACCOUNT, + }); + const spy = jest.spyOn(nftController, 'updateNftMetadata'); + const testNetworkClientId = 'mainnet'; + mockGetAccount.mockReturnValue(OWNER_ACCOUNT); + await nftController.addNft('0xtest', '3', { + nftMetadata: { name: '', description: '', image: '', standard: '' }, + networkClientId: testNetworkClientId, + }); + + triggerSelectedAccountChange(OWNER_ACCOUNT); + // trigger preference change + triggerPreferencesStateChange({ + ...getDefaultPreferencesState(), + ipfsGateway: 'https://toto/ipfs/', + }); + + expect(spy).toHaveBeenCalledTimes(1); + }); + it('should update Nft metadata successfully', async () => { const tokenURI = 'https://api.pudgypenguins.io/lil/4'; const mockGetERC721TokenURI = jest.fn().mockResolvedValue(tokenURI); diff --git a/packages/assets-controllers/src/NftController.ts b/packages/assets-controllers/src/NftController.ts index df159b91a9b..292611dc0d7 100644 --- a/packages/assets-controllers/src/NftController.ts +++ b/packages/assets-controllers/src/NftController.ts @@ -100,12 +100,11 @@ type SuggestedNftMeta = { * @property isCurrentlyOwned - Boolean indicating whether the address/chainId combination where it's currently stored currently owns this NFT * @property transactionId - Transaction Id associated with the NFT */ -export type Nft = - | { - tokenId: string; - address: string; - isCurrentlyOwned?: boolean; - } & NftMetadata; +export type Nft = { + tokenId: string; + address: string; + isCurrentlyOwned?: boolean; +} & NftMetadata; type NftUpdate = { nft: Nft; @@ -274,6 +273,8 @@ export const getDefaultNftControllerState = (): NftControllerState => ({ ignoredNfts: [], }); +const NFT_UPDATE_THRESHOLD = 500; + /** * Controller that stores assets and exposes convenience methods */ @@ -421,15 +422,21 @@ export class NftController extends BaseController< 'AccountsController:getSelectedAccount', ); this.#selectedAccountId = selectedAccount.id; - this.#ipfsGateway = ipfsGateway; - this.#openSeaEnabled = openSeaEnabled; - this.#isIpfsGatewayEnabled = isIpfsGatewayEnabled; - - const needsUpdateNftMetadata = - (isIpfsGatewayEnabled && ipfsGateway !== '') || openSeaEnabled; - - if (needsUpdateNftMetadata && selectedAccount) { - await this.#updateNftUpdateForAccount(selectedAccount); + // Get current state values + if ( + this.#ipfsGateway !== ipfsGateway || + this.#openSeaEnabled !== openSeaEnabled || + this.#isIpfsGatewayEnabled !== isIpfsGatewayEnabled + ) { + this.#ipfsGateway = ipfsGateway; + this.#openSeaEnabled = openSeaEnabled; + this.#isIpfsGatewayEnabled = isIpfsGatewayEnabled; + const needsUpdateNftMetadata = + (isIpfsGatewayEnabled && ipfsGateway !== '') || openSeaEnabled; + + if (needsUpdateNftMetadata && selectedAccount) { + await this.#updateNftUpdateForAccount(selectedAccount); + } } } @@ -2053,7 +2060,10 @@ export class NftController extends BaseController< (singleNft) => !singleNft.name && !singleNft.description && !singleNft.image, ); - if (nftsToUpdate.length !== 0) { + if ( + nftsToUpdate.length !== 0 && + nftsToUpdate.length < NFT_UPDATE_THRESHOLD + ) { await this.updateNftMetadata({ nfts: nftsToUpdate, userAddress: account.address,