diff --git a/docs/code-guidelines/controller-guidelines.md b/docs/code-guidelines/controller-guidelines.md index 372fb67fe31..0fc1c13872d 100644 --- a/docs/code-guidelines/controller-guidelines.md +++ b/docs/code-guidelines/controller-guidelines.md @@ -954,6 +954,51 @@ A messenger that allows no actions or events (whether internal or external) look export type FooServiceMessenger = Messenger<'FooService', never, never>; ``` +## Do not call messenger actions in the constructor + +One of the responsibilities of the messenger is to act as a liaison between controllers and services. A controller should not require direct access to another controller or service in order to communicate with it. This decouples controllers and services from each other and allows clients to initialize controllers and services in any order. + +Calling `this.messenger.call(...)` inside a controller's constructor, however, prevents this goal from being achieved. The action's handler must already be registered by the time the constructor runs, which means the controller that owns that handler must have been instantiated first. + +Instead of accessing actions in controller constructors, move any calls to an `init()` method (which clients are free to call after instantiating all controllers and services). + +🚫 **A messenger action is called during construction** + +```typescript +class TokensController extends BaseController { + #selectedAccountId: string; + + constructor({ messenger }: { messenger: TokensControllerMessenger }) { + super({ messenger /* ... */ }); + + // This requires AccountsController to have already been initialized + const { id } = this.messenger.call('AccountsController:getSelectedAccount'); + this.#selectedAccountId = id; + + // ... + } +} +``` + +✅ **The call is deferred to an `init()` method** + +```typescript +class TokensController extends BaseController { + #selectedAccountId: string | null = null; + + constructor({ messenger }: { messenger: TokensControllerMessenger }) { + super({ messenger /* ... */ }); + + // ... + } + + init() { + const { id } = this.messenger.call('AccountsController:getSelectedAccount'); + this.#selectedAccountId = id; + } +} +``` + ## Define and export a type for the controller's state The name of this type should be `${ControllerName}State`. It should be exported. diff --git a/eslint-suppressions.json b/eslint-suppressions.json index 546bda917aa..dd1102c6f21 100644 --- a/eslint-suppressions.json +++ b/eslint-suppressions.json @@ -137,16 +137,51 @@ "count": 6 } }, + "packages/assets-controller/src/AssetsController.ts": { + "no-restricted-syntax": { + "count": 5 + } + }, "packages/assets-controller/src/__fixtures__/MockAssetControllerMessenger.ts": { "import-x/no-relative-packages": { "count": 1 } }, + "packages/assets-controller/src/data-sources/AccountsApiDataSource.ts": { + "no-restricted-syntax": { + "count": 1 + } + }, + "packages/assets-controller/src/data-sources/PriceDataSource.ts": { + "no-restricted-syntax": { + "count": 1 + } + }, + "packages/assets-controller/src/data-sources/RpcDataSource.ts": { + "no-restricted-syntax": { + "count": 2 + } + }, + "packages/assets-controller/src/data-sources/SnapDataSource.ts": { + "no-restricted-syntax": { + "count": 1 + } + }, "packages/assets-controller/src/index.ts": { "no-restricted-syntax": { "count": 9 } }, + "packages/assets-controller/src/selectors/balance.ts": { + "no-restricted-syntax": { + "count": 1 + } + }, + "packages/assets-controller/src/utils/formatStateForTransactionPay.ts": { + "no-restricted-syntax": { + "count": 1 + } + }, "packages/assets-controllers/jest.environment.js": { "n/prefer-global/text-decoder": { "count": 1 @@ -163,6 +198,11 @@ "count": 3 } }, + "packages/assets-controllers/src/AccountTrackerController.ts": { + "no-restricted-syntax": { + "count": 1 + } + }, "packages/assets-controllers/src/AssetsContractController.test.ts": { "@typescript-eslint/explicit-function-return-type": { "count": 5 @@ -174,6 +214,9 @@ "packages/assets-controllers/src/AssetsContractController.ts": { "@typescript-eslint/explicit-function-return-type": { "count": 5 + }, + "no-restricted-syntax": { + "count": 2 } }, "packages/assets-controllers/src/CurrencyRateController.test.ts": { @@ -226,6 +269,9 @@ }, "@typescript-eslint/naming-convention": { "count": 1 + }, + "no-restricted-syntax": { + "count": 1 } }, "packages/assets-controllers/src/MultichainBalancesController/MultichainBalancesController.test.ts": { @@ -242,6 +288,9 @@ }, "@typescript-eslint/prefer-nullish-coalescing": { "count": 1 + }, + "no-restricted-syntax": { + "count": 2 } }, "packages/assets-controllers/src/NftController.test.ts": { @@ -276,6 +325,14 @@ }, "no-param-reassign": { "count": 2 + }, + "no-restricted-syntax": { + "count": 6 + } + }, + "packages/assets-controllers/src/NftDetectionController.ts": { + "no-restricted-syntax": { + "count": 1 } }, "packages/assets-controllers/src/RatesController/RatesController.test.ts": { @@ -333,6 +390,16 @@ "count": 2 } }, + "packages/assets-controllers/src/TokenBalancesController.ts": { + "no-restricted-syntax": { + "count": 2 + } + }, + "packages/assets-controllers/src/TokenDetectionController.ts": { + "no-restricted-syntax": { + "count": 4 + } + }, "packages/assets-controllers/src/TokenRatesController.test.ts": { "@typescript-eslint/explicit-function-return-type": { "count": 4 @@ -441,6 +508,11 @@ "count": 4 } }, + "packages/assets-controllers/src/multi-chain-accounts-service/api-balance-fetcher.ts": { + "no-restricted-syntax": { + "count": 1 + } + }, "packages/assets-controllers/src/multi-chain-accounts-service/multi-chain-accounts.test.ts": { "@typescript-eslint/explicit-function-return-type": { "count": 3 @@ -462,6 +534,9 @@ "packages/assets-controllers/src/multicall.ts": { "id-length": { "count": 2 + }, + "no-restricted-syntax": { + "count": 2 } }, "packages/assets-controllers/src/rpc-service/rpc-balance-fetcher.test.ts": { @@ -490,6 +565,11 @@ "count": 1 } }, + "packages/assets-controllers/src/token-prices-service/codefi-v2.ts": { + "no-restricted-syntax": { + "count": 2 + } + }, "packages/assets-controllers/src/utils/formatters.ts": { "@typescript-eslint/explicit-function-return-type": { "count": 9 @@ -569,6 +649,11 @@ "count": 2 } }, + "packages/bridge-controller/src/utils/quote.ts": { + "no-restricted-syntax": { + "count": 2 + } + }, "packages/bridge-controller/src/utils/slippage.ts": { "@typescript-eslint/explicit-function-return-type": { "count": 1 @@ -587,6 +672,9 @@ "packages/bridge-controller/src/utils/trade-utils.ts": { "no-restricted-globals": { "count": 1 + }, + "no-restricted-syntax": { + "count": 5 } }, "packages/bridge-controller/src/utils/validators.ts": { @@ -602,11 +690,21 @@ "count": 2 } }, + "packages/bridge-status-controller/src/utils/snaps.ts": { + "no-restricted-syntax": { + "count": 3 + } + }, "packages/bridge-status-controller/src/utils/swap-received-amount.ts": { "@typescript-eslint/explicit-function-return-type": { "count": 3 } }, + "packages/bridge-status-controller/src/utils/transaction.ts": { + "no-restricted-syntax": { + "count": 4 + } + }, "packages/chain-agnostic-permission/src/caip25Permission.ts": { "@typescript-eslint/explicit-function-return-type": { "count": 11 @@ -671,11 +769,46 @@ "count": 2 } }, + "packages/claims-controller/src/utils.ts": { + "no-restricted-syntax": { + "count": 1 + } + }, + "packages/composable-controller/src/ComposableController.ts": { + "no-restricted-syntax": { + "count": 3 + } + }, "packages/config-registry-controller/src/index.ts": { "no-restricted-syntax": { "count": 2 } }, + "packages/controller-utils/src/create-service-policy.ts": { + "no-restricted-syntax": { + "count": 1 + } + }, + "packages/controller-utils/src/util.ts": { + "no-restricted-syntax": { + "count": 1 + } + }, + "packages/core-backend/src/AccountActivityService.ts": { + "no-restricted-syntax": { + "count": 1 + } + }, + "packages/core-backend/src/BackendWebSocketService.ts": { + "no-restricted-syntax": { + "count": 4 + } + }, + "packages/core-backend/src/api/shared-types.ts": { + "no-restricted-syntax": { + "count": 1 + } + }, "packages/core-backend/src/index.ts": { "no-restricted-syntax": { "count": 2 @@ -820,6 +953,21 @@ "count": 6 } }, + "packages/eth-json-rpc-middleware/src/fetch.ts": { + "no-restricted-syntax": { + "count": 1 + } + }, + "packages/eth-json-rpc-middleware/src/inflight-cache.ts": { + "no-restricted-syntax": { + "count": 1 + } + }, + "packages/eth-json-rpc-provider/src/internal-provider.ts": { + "no-restricted-syntax": { + "count": 1 + } + }, "packages/foundryup/src/cli.ts": { "no-restricted-globals": { "count": 1 @@ -876,6 +1024,9 @@ }, "id-denylist": { "count": 2 + }, + "no-restricted-syntax": { + "count": 1 } }, "packages/foundryup/types/unzipper.d.ts": { @@ -905,7 +1056,7 @@ "count": 1 }, "no-restricted-syntax": { - "count": 14 + "count": 16 } }, "packages/gas-fee-controller/src/gas-util.ts": { @@ -916,6 +1067,31 @@ "count": 1 } }, + "packages/json-rpc-engine/src/JsonRpcEngine.ts": { + "no-restricted-syntax": { + "count": 1 + } + }, + "packages/json-rpc-engine/src/v2/JsonRpcEngineV2.ts": { + "no-restricted-syntax": { + "count": 1 + } + }, + "packages/json-rpc-engine/src/v2/MiddlewareContext.ts": { + "no-restricted-syntax": { + "count": 1 + } + }, + "packages/json-rpc-engine/src/v2/utils.ts": { + "no-restricted-syntax": { + "count": 1 + } + }, + "packages/keyring-controller/src/KeyringController.ts": { + "no-restricted-syntax": { + "count": 5 + } + }, "packages/logging-controller/src/LoggingController.test.ts": { "@typescript-eslint/explicit-function-return-type": { "count": 1 @@ -927,6 +1103,9 @@ "packages/logging-controller/src/LoggingController.ts": { "@typescript-eslint/explicit-function-return-type": { "count": 2 + }, + "no-restricted-syntax": { + "count": 1 } }, "packages/message-manager/src/AbstractMessageManager.test.ts": { @@ -1091,6 +1270,9 @@ }, "no-negated-condition": { "count": 1 + }, + "no-restricted-syntax": { + "count": 1 } }, "packages/multichain-api-middleware/src/middlewares/MultichainMiddlewareManager.ts": { @@ -1119,6 +1301,9 @@ }, "id-length": { "count": 4 + }, + "no-restricted-syntax": { + "count": 1 } }, "packages/multichain-transactions-controller/src/MultichainTransactionsController.test.ts": { @@ -1132,6 +1317,9 @@ }, "@typescript-eslint/no-misused-promises": { "count": 2 + }, + "no-restricted-syntax": { + "count": 1 } }, "packages/name-controller/src/NameController.ts": { @@ -1213,9 +1401,87 @@ "count": 1 } }, + "packages/network-controller/src/NetworkController.ts": { + "no-restricted-syntax": { + "count": 6 + } + }, + "packages/network-controller/src/create-auto-managed-network-client.ts": { + "no-restricted-syntax": { + "count": 4 + } + }, + "packages/network-controller/src/create-network-client.ts": { + "no-restricted-syntax": { + "count": 2 + } + }, + "packages/network-controller/src/rpc-service/rpc-service.ts": { + "no-restricted-syntax": { + "count": 3 + } + }, + "packages/network-enablement-controller/src/NetworkEnablementController.ts": { + "no-restricted-syntax": { + "count": 1 + } + }, "packages/network-enablement-controller/src/selectors.ts": { "@typescript-eslint/explicit-function-return-type": { "count": 3 + }, + "no-restricted-syntax": { + "count": 2 + } + }, + "packages/perps-controller/src/PerpsController.ts": { + "no-restricted-syntax": { + "count": 1 + } + }, + "packages/perps-controller/src/providers/HyperLiquidProvider.ts": { + "no-restricted-syntax": { + "count": 12 + } + }, + "packages/perps-controller/src/services/HyperLiquidSubscriptionService.ts": { + "no-restricted-syntax": { + "count": 4 + } + }, + "packages/perps-controller/src/services/TradingService.ts": { + "no-restricted-syntax": { + "count": 1 + } + }, + "packages/perps-controller/src/types/index.ts": { + "no-restricted-syntax": { + "count": 2 + } + }, + "packages/perps-controller/src/types/transactionTypes.ts": { + "no-restricted-syntax": { + "count": 4 + } + }, + "packages/perps-controller/src/utils/errorUtils.ts": { + "no-restricted-syntax": { + "count": 1 + } + }, + "packages/perps-controller/src/utils/hyperLiquidAdapter.ts": { + "no-restricted-syntax": { + "count": 2 + } + }, + "packages/perps-controller/src/utils/hyperLiquidValidation.ts": { + "no-restricted-syntax": { + "count": 1 + } + }, + "packages/perps-controller/src/utils/marketDataTransform.ts": { + "no-restricted-syntax": { + "count": 1 } }, "packages/perps-controller/src/utils/myxAdapter.ts": { @@ -1260,6 +1526,9 @@ }, "@typescript-eslint/prefer-nullish-coalescing": { "count": 6 + }, + "no-restricted-syntax": { + "count": 9 } }, "packages/phishing-controller/src/PhishingDetector.test.ts": { @@ -1291,6 +1560,9 @@ }, "@typescript-eslint/prefer-nullish-coalescing": { "count": 1 + }, + "no-restricted-syntax": { + "count": 4 } }, "packages/polling-controller/src/AbstractPollingController.ts": { @@ -1357,6 +1629,9 @@ }, "id-length": { "count": 1 + }, + "no-restricted-syntax": { + "count": 2 } }, "packages/profile-sync-controller/src/controllers/user-storage/__fixtures__/mockMessenger.ts": { @@ -1490,6 +1765,11 @@ "count": 1 } }, + "packages/profile-sync-controller/src/sdk/authentication-jwt-bearer/services.ts": { + "no-restricted-syntax": { + "count": 3 + } + }, "packages/profile-sync-controller/src/sdk/authentication-jwt-bearer/types.ts": { "@typescript-eslint/naming-convention": { "count": 5 @@ -1520,6 +1800,9 @@ "packages/profile-sync-controller/src/sdk/errors.ts": { "id-length": { "count": 1 + }, + "no-restricted-syntax": { + "count": 1 } }, "packages/profile-sync-controller/src/sdk/mocks/userstorage.ts": { @@ -1618,6 +1901,11 @@ "count": 1 } }, + "packages/ramps-controller/src/RampsController.ts": { + "no-restricted-syntax": { + "count": 1 + } + }, "packages/ramps-controller/src/index.ts": { "no-restricted-syntax": { "count": 1 @@ -1628,9 +1916,37 @@ "count": 1 } }, + "packages/remote-feature-flag-controller/src/utils/user-segmentation-utils.ts": { + "no-restricted-syntax": { + "count": 1 + } + }, + "packages/remote-feature-flag-controller/src/utils/version.ts": { + "no-restricted-syntax": { + "count": 1 + } + }, + "packages/seedless-onboarding-controller/src/SecretMetadata.ts": { + "no-restricted-syntax": { + "count": 2 + } + }, + "packages/seedless-onboarding-controller/src/assertions.ts": { + "no-restricted-syntax": { + "count": 10 + } + }, + "packages/seedless-onboarding-controller/src/errors.ts": { + "no-restricted-syntax": { + "count": 4 + } + }, "packages/selected-network-controller/src/SelectedNetworkController.ts": { "@typescript-eslint/explicit-function-return-type": { "count": 4 + }, + "no-restricted-syntax": { + "count": 3 } }, "packages/selected-network-controller/src/SelectedNetworkMiddleware.ts": { @@ -1669,6 +1985,11 @@ "count": 1 } }, + "packages/signature-controller/src/utils/delegations.ts": { + "no-restricted-syntax": { + "count": 1 + } + }, "packages/signature-controller/src/utils/normalize.ts": { "@typescript-eslint/explicit-function-return-type": { "count": 2 @@ -1702,11 +2023,41 @@ "count": 2 } }, + "packages/subscription-controller/src/errors.ts": { + "no-restricted-syntax": { + "count": 1 + } + }, "packages/subscription-controller/src/index.ts": { "no-restricted-syntax": { "count": 2 } }, + "packages/transaction-controller/src/TransactionController.ts": { + "no-restricted-syntax": { + "count": 3 + } + }, + "packages/transaction-controller/src/utils/retry.ts": { + "no-restricted-syntax": { + "count": 3 + } + }, + "packages/transaction-controller/src/utils/utils.ts": { + "no-restricted-syntax": { + "count": 1 + } + }, + "packages/transaction-pay-controller/src/strategy/across/across-actions.ts": { + "no-restricted-syntax": { + "count": 2 + } + }, + "packages/transaction-pay-controller/src/strategy/relay/hyperliquid-withdraw.ts": { + "no-restricted-syntax": { + "count": 1 + } + }, "packages/transaction-pay-controller/src/utils/source-amounts.ts": { "import-x/no-relative-packages": { "count": 1 diff --git a/eslint.config.mjs b/eslint.config.mjs index def3343abc8..e4cbc478c0e 100644 --- a/eslint.config.mjs +++ b/eslint.config.mjs @@ -5,6 +5,19 @@ import typescript from '@metamask/eslint-config-typescript'; const NODE_LTS_VERSION = 22; +/** + * Arguments to the `no-restricted-syntax` rule that prevents messsenger actions + * from being called in constructors. + */ +const NO_MESSENGER_ACTIONS_IN_CONSTRUCTORS_RULES = [ + { + selector: + 'MethodDefinition[kind="constructor"] CallExpression[callee.type="MemberExpression"][callee.property.name="call"][callee.object.type="MemberExpression"][callee.object.object.type="ThisExpression"][callee.object.property.name="messenger"]', + message: + 'Do not call messenger actions in the constructor, as this forces clients to instantiate controllers or services in a specific order. Move this call to an init() method instead. Read the controller guidelines for more: https://github.com/MetaMask/core/blob/main/docs/code-guidelines/controller-guidelines.md#do-not-call-messenger-actions-in-the-constructor', + }, +]; + /** * Collects all options for a given array-valued rule across one or more flat * config arrays, excluding the leading severity element. @@ -109,39 +122,6 @@ const config = createConfig([ 'jsdoc/require-jsdoc': 'off', }, }, - { - // Prohibit exporting certain types from package index files. - // Extends the upstream no-restricted-syntax selectors from base and - // typescript configs with additional selectors specific to this monorepo. - files: ['packages/*/src/index.ts'], - rules: { - 'no-restricted-syntax': [ - 'error', - ...collectExistingRuleOptions('no-restricted-syntax', [ - base, - typescript, - ]), - { - selector: - 'ExportNamedDeclaration > ExportSpecifier[local.name=/AllowedActions$/]', - message: - 'Do not export AllowedActions types from package index files. These types describe external messenger dependencies and are obtainable from the packages that define them directly. Read the controller guidelines for more: https://github.com/MetaMask/core/blob/main/docs/code-guidelines/controller-guidelines.md#define-but-do-not-export-a-type-union-for-external-action-types', - }, - { - selector: - 'ExportNamedDeclaration > ExportSpecifier[local.name=/AllowedEvents$/]', - message: - 'Do not export AllowedEvents types from package index files. These types describe external messenger dependencies and are obtainable from the packages that define them directly. Read the controller guidelines for more: https://github.com/MetaMask/core/blob/main/docs/code-guidelines/controller-guidelines.md#define-but-do-not-export-a-type-union-for-external-event-types', - }, - { - selector: - 'ExportNamedDeclaration > ExportSpecifier[local.name=/MethodActions$/]', - message: - 'Do not export *MethodActions types from package index files. Internal messenger actions are already available via the *Actions type. Export the individual action types (along with *Actions) instead. Read the controller guidelines for more: https://github.com/MetaMask/core/blob/main/docs/code-guidelines/controller-guidelines.md#expose-controller-methods-through-messenger-in-bulk', - }, - ], - }, - }, { files: ['**/*.test.{js,ts}', '**/tests/**/*.{js,ts}'], extends: [jest], @@ -211,6 +191,54 @@ const config = createConfig([ 'import-x/no-relative-packages': 'error', }, }, + // Prevent calling messenger actions in controller/service constructors + { + files: ['packages/*/src/**/*.ts'], + ignores: ['**/*.test.ts', '**/tests/**/*.ts'], + rules: { + 'no-restricted-syntax': [ + 'error', + ...collectExistingRuleOptions('no-restricted-syntax', [ + base, + typescript, + ]), + ...NO_MESSENGER_ACTIONS_IN_CONSTRUCTORS_RULES, + ], + }, + }, + { + // Prohibit exporting *AllowedActions, *AllowedEvents, and *MethodActions + // from package index files + files: ['packages/*/src/index.ts'], + rules: { + 'no-restricted-syntax': [ + 'error', + ...collectExistingRuleOptions('no-restricted-syntax', [ + base, + typescript, + ]), + ...NO_MESSENGER_ACTIONS_IN_CONSTRUCTORS_RULES, + { + selector: + 'ExportNamedDeclaration > ExportSpecifier[local.name=/AllowedActions$/]', + message: + 'Do not export AllowedActions types from package index files. These types describe external messenger dependencies and are obtainable from the packages that define them directly. Read the controller guidelines for more: https://github.com/MetaMask/core/blob/main/docs/code-guidelines/controller-guidelines.md#define-but-do-not-export-a-type-union-for-external-action-types', + }, + { + selector: + 'ExportNamedDeclaration > ExportSpecifier[local.name=/AllowedEvents$/]', + message: + 'Do not export AllowedEvents types from package index files. These types describe external messenger dependencies and are obtainable from the packages that define them directly. Read the controller guidelines for more: https://github.com/MetaMask/core/blob/main/docs/code-guidelines/controller-guidelines.md#define-but-do-not-export-a-type-union-for-external-event-types', + }, + { + selector: + 'ExportNamedDeclaration > ExportSpecifier[local.name=/MethodActions$/]', + message: + 'Do not export *MethodActions types from package index files. Internal messenger actions are already available via the *Actions type. Export the individual action types (along with *Actions) instead. Read the controller guidelines for more: https://github.com/MetaMask/core/blob/main/docs/code-guidelines/controller-guidelines.md#expose-controller-methods-through-messenger-in-bulk', + }, + ], + }, + }, { files: ['packages/foundryup/**/*.{js,ts}'], rules: {