From 23eaf51e105b27485da2fc64bb0ef36efffac07c Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Fri, 23 Feb 2024 17:57:42 -0500 Subject: [PATCH 01/11] Rename types to align with conventions followed by other controllers --- packages/rate-limit-controller/CHANGELOG.md | 7 +++++++ .../src/RateLimitController.ts | 13 +++++++------ 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/packages/rate-limit-controller/CHANGELOG.md b/packages/rate-limit-controller/CHANGELOG.md index de86b5806f..45169c8861 100644 --- a/packages/rate-limit-controller/CHANGELOG.md +++ b/packages/rate-limit-controller/CHANGELOG.md @@ -7,6 +7,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed + +- **BREAKING:** Rename types to align with conventions followed by other controllers. ([#3963](https://github.com/MetaMask/core/pull/3963)) + - `GetRateLimitState` is now `RateLimitControllerGetStateAction`. + - `RateLimitStateChange` is now `RateLimitControllerStateChangeEvent`. + - `CallApi` is now `RateLimitControllerCallApiAction`. + ### Fixed - **BREAKING:** Fix `GetRateLimitState`, `RateLimitStateChange` types by replacing `RateLimitedApis` with `RateLimitState` as the state type passed in as generic arguments to `ControllerGetStateAction` and `ControllerStateChangeEvent` ([#3949](https://github.com/MetaMask/core/pull/3949)) diff --git a/packages/rate-limit-controller/src/RateLimitController.ts b/packages/rate-limit-controller/src/RateLimitController.ts index 00f79f70c4..a46b0591e0 100644 --- a/packages/rate-limit-controller/src/RateLimitController.ts +++ b/packages/rate-limit-controller/src/RateLimitController.ts @@ -31,26 +31,27 @@ export type RateLimitState< const name = 'RateLimitController'; -export type RateLimitStateChange< +export type RateLimitControllerStateChangeEvent< RateLimitedApis extends Record, > = ControllerStateChangeEvent>; -export type GetRateLimitState< +export type RateLimitControllerGetStateAction< RateLimitedApis extends Record, > = ControllerGetStateAction>; -export type CallApi> = { +export type RateLimitControllerCallApiAction> = { type: `${typeof name}:call`; handler: RateLimitController['call']; }; export type RateLimitControllerActions< - RateLimitedApis extends Record, -> = GetRateLimitState | CallApi; +> = + | RateLimitControllerGetStateAction + | RateLimitControllerCallApiAction; export type RateLimitControllerEvents< RateLimitedApis extends Record, -> = RateLimitStateChange; +> = RateLimitControllerStateChangeEvent; export type RateLimitMessenger< RateLimitedApis extends Record, From 598d9833b681e8450dead326f032dbfb180f06a1 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Fri, 23 Feb 2024 18:02:32 -0500 Subject: [PATCH 02/11] Define and apply types `RateLimitedApiMap`, `RateLimitedRequests`, add jsdoc --- packages/rate-limit-controller/CHANGELOG.md | 6 ++ .../src/RateLimitController.ts | 59 ++++++++++++------- 2 files changed, 44 insertions(+), 21 deletions(-) diff --git a/packages/rate-limit-controller/CHANGELOG.md b/packages/rate-limit-controller/CHANGELOG.md index 45169c8861..debc59caa6 100644 --- a/packages/rate-limit-controller/CHANGELOG.md +++ b/packages/rate-limit-controller/CHANGELOG.md @@ -7,6 +7,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- Add and export types `RateLimitedApiMap`, `RateLimitedRequests`. ([#3963](https://github.com/MetaMask/core/pull/3963)) + - `RateLimitedApiMap` represents the type of the `RateLimitedApis` generic parameter used throughout the controller. + - `RateLimitedRequests` represents the type of the `request` property of `RateLimitState`. + ### Changed - **BREAKING:** Rename types to align with conventions followed by other controllers. ([#3963](https://github.com/MetaMask/core/pull/3963)) diff --git a/packages/rate-limit-controller/src/RateLimitController.ts b/packages/rate-limit-controller/src/RateLimitController.ts index a46b0591e0..b38ddedf86 100644 --- a/packages/rate-limit-controller/src/RateLimitController.ts +++ b/packages/rate-limit-controller/src/RateLimitController.ts @@ -8,7 +8,8 @@ import { BaseController } from '@metamask/base-controller'; import { rpcErrors } from '@metamask/rpc-errors'; /** - * @type RateLimitedApi + * A rate-limited API endpoint. + * @typedef RateLimitedApi * @property method - The method that is rate-limited. * @property rateLimitTimeout - The time window in which the rate limit is applied (in ms). * @property rateLimitCount - The amount of calls an origin can make in the rate limit time window. @@ -20,48 +21,64 @@ export type RateLimitedApi = { }; /** - * @type RateLimitState - * @property requests - Object containing number of requests in a given interval for each origin and api type combination + * A map of rate-limited API types to APIs. + * @typedef RateLimitedApiMap */ -export type RateLimitState< - RateLimitedApis extends Record, -> = { - requests: Record>; +export type RateLimitedApiMap = Record; + +/** + * A map of rate-limited API types to the number of requests made in a given interval for each origin and api type combination. + * @typedef RateLimitedRequests + * @template RateLimitedApis - A {@link RateLimitedApiMap} containing the rate-limited API endpoints that is used by the {@link RateLimitController}. + */ +export type RateLimitedRequests = + Record>; + +/** + * The state of the {@link RateLimitController}. + * @typedef RateLimitState + * @template RateLimitedApis - A {@link RateLimitedApiMap} containing the rate-limited API endpoints that is used by the {@link RateLimitController}. + * @property requests {@link RateLimitedRequests} - An object containing the number of requests made in a given interval for each origin and api type combination. + */ +export type RateLimitState = { + requests: RateLimitedRequests; }; const name = 'RateLimitController'; export type RateLimitControllerStateChangeEvent< - RateLimitedApis extends Record, + RateLimitedApis extends RateLimitedApiMap, > = ControllerStateChangeEvent>; export type RateLimitControllerGetStateAction< - RateLimitedApis extends Record, + RateLimitedApis extends RateLimitedApiMap, > = ControllerGetStateAction>; -export type RateLimitControllerCallApiAction> = { +export type RateLimitControllerCallApiAction< + RateLimitedApis extends RateLimitedApiMap, +> = { type: `${typeof name}:call`; handler: RateLimitController['call']; }; export type RateLimitControllerActions< + RateLimitedApis extends RateLimitedApiMap, > = | RateLimitControllerGetStateAction | RateLimitControllerCallApiAction; export type RateLimitControllerEvents< - RateLimitedApis extends Record, + RateLimitedApis extends RateLimitedApiMap, > = RateLimitControllerStateChangeEvent; -export type RateLimitMessenger< - RateLimitedApis extends Record, -> = RestrictedControllerMessenger< - typeof name, - RateLimitControllerActions, - RateLimitControllerEvents, - never, - never ->; +export type RateLimitMessenger = + RestrictedControllerMessenger< + typeof name, + RateLimitControllerActions, + RateLimitControllerEvents, + never, + never + >; const metadata = { requests: { persist: false, anonymous: false }, @@ -71,7 +88,7 @@ const metadata = { * Controller with logic for rate-limiting API endpoints per requesting origin. */ export class RateLimitController< - RateLimitedApis extends Record, + RateLimitedApis extends RateLimitedApiMap, > extends BaseController< typeof name, RateLimitState, From d3521dd958a76d522caf9a95654d04ec413ab843 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Fri, 23 Feb 2024 18:03:03 -0500 Subject: [PATCH 03/11] Fix wildcard export by enumerating package-level exports --- packages/rate-limit-controller/src/index.ts | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/packages/rate-limit-controller/src/index.ts b/packages/rate-limit-controller/src/index.ts index 50645817b7..97cbe8505e 100644 --- a/packages/rate-limit-controller/src/index.ts +++ b/packages/rate-limit-controller/src/index.ts @@ -1 +1,13 @@ -export * from './RateLimitController'; +export type { + RateLimitedApi, + RateLimitedApiMap, + RateLimitedRequests, + RateLimitControllerActions, + RateLimitControllerCallApiAction, + RateLimitControllerGetStateAction, + RateLimitControllerEvents, + RateLimitControllerStateChangeEvent, + RateLimitMessenger, + RateLimitState, +} from './RateLimitController'; +export { RateLimitController } from './RateLimitController'; From 7d936df378f2da2393c7cafa3d367e0e8eb6a9f4 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Fri, 23 Feb 2024 18:04:20 -0500 Subject: [PATCH 04/11] Fix three counts of `any` usage --- .../src/RateLimitController.ts | 26 ++++++++++++------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/packages/rate-limit-controller/src/RateLimitController.ts b/packages/rate-limit-controller/src/RateLimitController.ts index b38ddedf86..f2c15f648d 100644 --- a/packages/rate-limit-controller/src/RateLimitController.ts +++ b/packages/rate-limit-controller/src/RateLimitController.ts @@ -202,16 +202,19 @@ export class RateLimitController< const rateLimitTimeout = this.implementations[api].rateLimitTimeout ?? this.rateLimitTimeout; this.update((state) => { - // TODO: Replace `any` with type - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const previous = (state as any).requests[api][origin] ?? 0; - // TODO: Replace `any` with type - // eslint-disable-next-line @typescript-eslint/no-explicit-any - (state as any).requests[api][origin] = previous + 1; - + const previous = + (state as unknown as RateLimitState).requests[api][ + origin + ] ?? 0; if (previous === 0) { setTimeout(() => this.resetRequestCount(api, origin), rateLimitTimeout); } + return Object.assign(state, { + requests: { + ...(state.requests as RateLimitedRequests), + [api]: { [origin]: previous + 1 }, + }, + }); }); } @@ -223,9 +226,12 @@ export class RateLimitController< */ private resetRequestCount(api: keyof RateLimitedApis, origin: string) { this.update((state) => { - // TODO: Replace `any` with type - // eslint-disable-next-line @typescript-eslint/no-explicit-any - (state as any).requests[api][origin] = 0; + return Object.assign(state, { + requests: { + ...(state.requests as RateLimitedRequests), + [api]: { [origin]: 0 }, + }, + }); }); } } From 93fe16893e0961ab7403382a070f85f27229a482 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Fri, 23 Feb 2024 18:06:32 -0500 Subject: [PATCH 05/11] Fix typing: `args` should be queried with the specific `type` passed in, not all possible api types stored in state. `keyof RateLimitedApis` is only the constraint for the `type` param, not its type. --- packages/rate-limit-controller/src/RateLimitController.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/rate-limit-controller/src/RateLimitController.ts b/packages/rate-limit-controller/src/RateLimitController.ts index f2c15f648d..dce847f536 100644 --- a/packages/rate-limit-controller/src/RateLimitController.ts +++ b/packages/rate-limit-controller/src/RateLimitController.ts @@ -140,11 +140,11 @@ export class RateLimitController< this.rateLimitCount = rateLimitCount; this.messagingSystem.registerActionHandler( - `${name}:call` as const, + `${name}:call`, ( origin: string, type: keyof RateLimitedApis, - ...args: Parameters + ...args: Parameters ) => this.call(origin, type, ...args), ); } From c763dde1be332d3738e1f57b218ff376ff7849e0 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Fri, 23 Feb 2024 18:08:31 -0500 Subject: [PATCH 06/11] Narrow type of `acc`, `key` in this reduce operation --- packages/rate-limit-controller/src/RateLimitController.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/rate-limit-controller/src/RateLimitController.ts b/packages/rate-limit-controller/src/RateLimitController.ts index dce847f536..75160a9b1f 100644 --- a/packages/rate-limit-controller/src/RateLimitController.ts +++ b/packages/rate-limit-controller/src/RateLimitController.ts @@ -124,9 +124,11 @@ export class RateLimitController< implementations: RateLimitedApis; }) { const defaultState = { - requests: Object.keys(implementations).reduce( + requests: ( + Object.keys(implementations) as (keyof RateLimitedApis)[] + ).reduce>( (acc, key) => ({ ...acc, [key]: {} }), - {} as Record>, + {} as never, ), }; super({ From ba1e07cd68a9263d75161d63900ba406b744e3c3 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Wed, 6 Mar 2024 07:43:10 -0800 Subject: [PATCH 07/11] Apply suggestions for jsdoc Co-authored-by: Elliot Winkler --- packages/rate-limit-controller/src/RateLimitController.ts | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/packages/rate-limit-controller/src/RateLimitController.ts b/packages/rate-limit-controller/src/RateLimitController.ts index 75160a9b1f..298054278c 100644 --- a/packages/rate-limit-controller/src/RateLimitController.ts +++ b/packages/rate-limit-controller/src/RateLimitController.ts @@ -9,7 +9,6 @@ import { rpcErrors } from '@metamask/rpc-errors'; /** * A rate-limited API endpoint. - * @typedef RateLimitedApi * @property method - The method that is rate-limited. * @property rateLimitTimeout - The time window in which the rate limit is applied (in ms). * @property rateLimitCount - The amount of calls an origin can make in the rate limit time window. @@ -22,13 +21,11 @@ export type RateLimitedApi = { /** * A map of rate-limited API types to APIs. - * @typedef RateLimitedApiMap */ export type RateLimitedApiMap = Record; /** * A map of rate-limited API types to the number of requests made in a given interval for each origin and api type combination. - * @typedef RateLimitedRequests * @template RateLimitedApis - A {@link RateLimitedApiMap} containing the rate-limited API endpoints that is used by the {@link RateLimitController}. */ export type RateLimitedRequests = @@ -36,9 +33,8 @@ export type RateLimitedRequests = /** * The state of the {@link RateLimitController}. - * @typedef RateLimitState * @template RateLimitedApis - A {@link RateLimitedApiMap} containing the rate-limited API endpoints that is used by the {@link RateLimitController}. - * @property requests {@link RateLimitedRequests} - An object containing the number of requests made in a given interval for each origin and api type combination. + * @property requests - An object containing the number of requests made in a given interval for each origin and api type combination. */ export type RateLimitState = { requests: RateLimitedRequests; From 7f2f506ce576a01aa78304821ba12e206ddb0496 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Wed, 6 Mar 2024 10:46:32 -0500 Subject: [PATCH 08/11] Prefer `getKnownPropertyNames` over index signature assertion --- .../rate-limit-controller/src/RateLimitController.ts | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/packages/rate-limit-controller/src/RateLimitController.ts b/packages/rate-limit-controller/src/RateLimitController.ts index 298054278c..098366acfc 100644 --- a/packages/rate-limit-controller/src/RateLimitController.ts +++ b/packages/rate-limit-controller/src/RateLimitController.ts @@ -6,6 +6,7 @@ import type { } from '@metamask/base-controller'; import { BaseController } from '@metamask/base-controller'; import { rpcErrors } from '@metamask/rpc-errors'; +import { getKnownPropertyNames } from '@metamask/utils'; /** * A rate-limited API endpoint. @@ -120,12 +121,9 @@ export class RateLimitController< implementations: RateLimitedApis; }) { const defaultState = { - requests: ( - Object.keys(implementations) as (keyof RateLimitedApis)[] - ).reduce>( - (acc, key) => ({ ...acc, [key]: {} }), - {} as never, - ), + requests: getKnownPropertyNames(implementations).reduce< + RateLimitedRequests + >((acc, key) => ({ ...acc, [key]: {} }), {} as never), }; super({ name, From e9a30742dafc75a0139cf7b24a301aab8bb738ee Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Wed, 6 Mar 2024 10:47:53 -0500 Subject: [PATCH 09/11] Access state value from outside of `update` callback --- packages/rate-limit-controller/src/RateLimitController.ts | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/packages/rate-limit-controller/src/RateLimitController.ts b/packages/rate-limit-controller/src/RateLimitController.ts index 098366acfc..bedec4286e 100644 --- a/packages/rate-limit-controller/src/RateLimitController.ts +++ b/packages/rate-limit-controller/src/RateLimitController.ts @@ -197,11 +197,8 @@ export class RateLimitController< private recordRequest(api: keyof RateLimitedApis, origin: string) { const rateLimitTimeout = this.implementations[api].rateLimitTimeout ?? this.rateLimitTimeout; + const previous = this.state.requests[api][origin] ?? 0; this.update((state) => { - const previous = - (state as unknown as RateLimitState).requests[api][ - origin - ] ?? 0; if (previous === 0) { setTimeout(() => this.resetRequestCount(api, origin), rateLimitTimeout); } From d141d7d89a044790501b30edfa0d20396022913a Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Wed, 6 Mar 2024 10:54:37 -0500 Subject: [PATCH 10/11] Add `@metamask/utils` as dependency --- packages/rate-limit-controller/CHANGELOG.md | 1 + packages/rate-limit-controller/package.json | 3 ++- yarn.lock | 1 + 3 files changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/rate-limit-controller/CHANGELOG.md b/packages/rate-limit-controller/CHANGELOG.md index debc59caa6..7996fba0ec 100644 --- a/packages/rate-limit-controller/CHANGELOG.md +++ b/packages/rate-limit-controller/CHANGELOG.md @@ -19,6 +19,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `GetRateLimitState` is now `RateLimitControllerGetStateAction`. - `RateLimitStateChange` is now `RateLimitControllerStateChangeEvent`. - `CallApi` is now `RateLimitControllerCallApiAction`. +- Add `@metamask/utils` `^8.3.0` as a dependency. ([#3963](https://github.com/MetaMask/core/pull/3963)) ### Fixed diff --git a/packages/rate-limit-controller/package.json b/packages/rate-limit-controller/package.json index ddcbce1b90..29edd11522 100644 --- a/packages/rate-limit-controller/package.json +++ b/packages/rate-limit-controller/package.json @@ -32,7 +32,8 @@ }, "dependencies": { "@metamask/base-controller": "^4.1.1", - "@metamask/rpc-errors": "^6.2.1" + "@metamask/rpc-errors": "^6.2.1", + "@metamask/utils": "^8.3.0" }, "devDependencies": { "@metamask/auto-changelog": "^3.4.4", diff --git a/yarn.lock b/yarn.lock index 887de24e8c..f2056ee3e1 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2621,6 +2621,7 @@ __metadata: "@metamask/auto-changelog": ^3.4.4 "@metamask/base-controller": ^4.1.1 "@metamask/rpc-errors": ^6.2.1 + "@metamask/utils": ^8.3.0 "@types/jest": ^27.4.1 deepmerge: ^4.2.2 jest: ^27.5.1 From 244325e326eae212bd9f185aadbe778fd838b594 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Thu, 7 Mar 2024 13:34:37 -0500 Subject: [PATCH 11/11] Do not return `Object.assign` in `update` callback, so it's clear that state is being mutated --- packages/rate-limit-controller/src/RateLimitController.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/rate-limit-controller/src/RateLimitController.ts b/packages/rate-limit-controller/src/RateLimitController.ts index bedec4286e..2ddcf1c8f3 100644 --- a/packages/rate-limit-controller/src/RateLimitController.ts +++ b/packages/rate-limit-controller/src/RateLimitController.ts @@ -202,7 +202,7 @@ export class RateLimitController< if (previous === 0) { setTimeout(() => this.resetRequestCount(api, origin), rateLimitTimeout); } - return Object.assign(state, { + Object.assign(state, { requests: { ...(state.requests as RateLimitedRequests), [api]: { [origin]: previous + 1 }, @@ -219,7 +219,7 @@ export class RateLimitController< */ private resetRequestCount(api: keyof RateLimitedApis, origin: string) { this.update((state) => { - return Object.assign(state, { + Object.assign(state, { requests: { ...(state.requests as RateLimitedRequests), [api]: { [origin]: 0 },