Add RateLimitController #698
Conversation
src/ratelimit/RateLimitController.ts
Outdated
| messenger, | ||
| state: { ...defaultState, ...state }, | ||
| }); | ||
| this.showNativeNotification = showNativeNotification; |
There was a problem hiding this comment.
We could make this controller generic over the actions being rate limited, and pass them in as a mapping of type to implementation.
That would let us extend what is rate limited without altering the controller itself, and would decouple this controller from the things being rate limited.
src/ratelimit/RateLimitController.ts
Outdated
| rateLimitCount?: number; | ||
| messenger: RateLimitMessenger<ApiType>; | ||
| state?: Partial<RateLimitState<ApiType>>; | ||
| implementations: Record<ApiType, (...args: unknown[]) => unknown>; |
There was a problem hiding this comment.
This doesn't seem practical to use. In effect these implementations can't be used by the controller unless they happen to accept unknown or any as parameters. Try updating the unit tests to pass in a function with specific parameter types to see what I mean. Effectively this erases the types of the underlying method, so using would require lots of inconvenient casting and the loss of type safety.
When I suggested making this controller generic, what I had in mind was making it generic over the underlying implementation so their types would be preserved. We need to preserve the types for this to be practical.
I made an example of this here: https://github.com/MetaMask/controllers/tree/rate-limit-controller-types-example
Not sure if it works, and I didn't update the tests, so my apologies if it's broken. But that's generally what I was thinking originally.
There was a problem hiding this comment.
@Gudahtt Hmm, this is a good idea, I think. I can explore this further tomorrow! Thanks!
There was a problem hiding this comment.
In addition to @mcmire's outstanding feedback, RateLimitController.call() must itself be genericized in order to accurately type the parameters for each API type. With the current implementation, the ...args in call() will be a union of the parameters of all API types. I took a stab at this here: 7b29534
FrederikBolding
force-pushed
the
fb/rate-limiting-controller
branch
from
6ff92f3
to
7b3943b
Compare
Fixed. I unfortunately don't see a way to have this type safety on messenger calls with the current implementation. I talked to @Mrtenz and he has some ideas for how to restructure the messenger types to accommodate use-cases like this, but out of scope for this PR 😅 |
|
cc @Gudahtt |
|
Perhaps we could get around these type issues by... not calling the underlying method at all? Then we couldn't have to deal with those types at all. The |
The docstrings contained references to "notifications" from this controller's infancy.
…implementation is async
FrederikBolding
force-pushed
the
fb/rate-limiting-controller
branch
from
740aa7b
to
09b1352
Compare
* Initial implementation of NotificationControllerV2 * Simplify implementation * Fix small type issue * Simplify and start using messaging system * Adapt tests for controller messaging * Simplify tests a bit * Add action handler * Fix types * Fix tests * Add some documentation and exports * Pivot to RateLimitController * Fix export * Remove subject metadata type * Make controller generic and allow caller to pass implementation mapping * Fix PR comments * Fix type name casing * Fix controller docstrings The docstrings contained references to "notifications" from this controller's infancy. * Return potential result from implementation, Wrap in promise in case implementation is async * Improve typing and API * Fix type issue * Fix PR comments * Cast action handler * Change use of useFakeTimers * Throw error when API is rate-limited * Small fixes Co-authored-by: Erik Marks <25517051+rekmarks@users.noreply.github.com>
* Initial implementation of NotificationControllerV2 * Simplify implementation * Fix small type issue * Simplify and start using messaging system * Adapt tests for controller messaging * Simplify tests a bit * Add action handler * Fix types * Fix tests * Add some documentation and exports * Pivot to RateLimitController * Fix export * Remove subject metadata type * Make controller generic and allow caller to pass implementation mapping * Fix PR comments * Fix type name casing * Fix controller docstrings The docstrings contained references to "notifications" from this controller's infancy. * Return potential result from implementation, Wrap in promise in case implementation is async * Improve typing and API * Fix type issue * Fix PR comments * Cast action handler * Change use of useFakeTimers * Throw error when API is rate-limited * Small fixes Co-authored-by: Erik Marks <25517051+rekmarks@users.noreply.github.com>
* Initial implementation of NotificationControllerV2 * Simplify implementation * Fix small type issue * Simplify and start using messaging system * Adapt tests for controller messaging * Simplify tests a bit * Add action handler * Fix types * Fix tests * Add some documentation and exports * Pivot to RateLimitController * Fix export * Remove subject metadata type * Make controller generic and allow caller to pass implementation mapping * Fix PR comments * Fix type name casing * Fix controller docstrings The docstrings contained references to "notifications" from this controller's infancy. * Return potential result from implementation, Wrap in promise in case implementation is async * Improve typing and API * Fix type issue * Fix PR comments * Cast action handler * Change use of useFakeTimers * Throw error when API is rate-limited * Small fixes Co-authored-by: Erik Marks <25517051+rekmarks@users.noreply.github.com>
Added
RateLimitControllerto dynamically and generically introduce rate limits to certain functions fx. called by Snaps.Description
RateLimitControllerChecklist