Refactor services to align with eventual RpcService #5109
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mcmire
force-pushed
the
extract-create-service-policy
branch
from
627b096 to
530bc4c
Compare
mcmire
force-pushed
the
refactor-service-classes
branch
from
1c790a7 to
987ec77
Compare
mcmire
force-pushed
the
refactor-service-classes
branch
from
987ec77 to
778223c
Compare
mcmire
changed the base branch from
extract-create-service-policy
to
extract-create-service-policy-3
mcmire
changed the title
mcmire
force-pushed
the
refactor-service-classes
branch
2 times, most recently
from
9d15cfb to
3b0eaff
Compare
Both RetryPolicy and CircuitBreakerPolicy from the Cockatiel library allow for listening for events using `on*` methods. Our "service policy" allows for listening to some of the same events as well as the "degraded" event. For parity with Cockatiel — and for easier use in general — this commit removes the callbacks that `createServicePolicy` takes and replaces them with `on*` methods.
mcmire
force-pushed
the
refactor-service-classes
branch
2 times, most recently
from
476c1a8 to
fe74a57
Compare
mcmire
force-pushed
the
refactor-service-classes
branch
4 times, most recently
from
78b5120 to
dba35e9
Compare
mcmire
changed the title
In a future commit, we are adding an RpcService class to `network-controller` to handle automatic failovers. This class is different from the existing service classes in a couple of ways: - RpcService will make use of a new `createServicePolicy` function which encapsulates retry and circuit breaker logic. This will relieve the consuming module for RpcService from needing to test this logic. - RpcService will have `onBreak` and `onDegraded` methods instead of taking `onBreak` and `onDegraded` callbacks as constructor options. This is reflected not only in the class but also in the abstract RPC service interface. To these ends, this commit makes the following changes to `CodefiTokenPricesServiceV2` in `assets-controllers` and `ClientConfigApiService` in `remote-feature-flag-controller`: - Refactor service class to use `createServicePolicy` - Update the abstract service class interface to extend `ServicePolicy` from `controller-utils`, and prepend `I` to the type to indicate that it's not a superclass, but an interface - Deprecate the `onBreak` and `onDegraded` constructor options, and add methods instead
mcmire
force-pushed
the
refactor-service-classes
branch
from
dba35e9 to
2e29f94
Compare
mcmire
commented
Jan 23, 2025
| @@ -829,8 +1055,9 @@ describe('CodefiTokenPricesServiceV2', () => { | |||
| clock.restore(); | |||
| }); | |||
|
|
|||
| it('does not call onDegraded when requests succeeds faster than threshold', async () => { | |||
There was a problem hiding this comment.
We don't need these tests because this functionality is already covered in the tests for createServicePolicy. We just need to test that the onDegraded callback option works somehow.
| @@ -1187,7 +1126,7 @@ describe('CodefiTokenPricesServiceV2', () => { | |||
| clock.restore(); | |||
| }); | |||
|
|
|||
| it('stops making fetch requests after too many consecutive failures', async () => { | |||
There was a problem hiding this comment.
Similar for this test, it's already covered.
|
|
||
| /** | ||
| * A service object responsible for fetching feature flags. | ||
| */ | ||
| export type AbstractClientConfigApiService = | ||
| PublicInterface<ClientConfigApiService>; | ||
| export type IAbstractClientConfigApiService = Partial< |
There was a problem hiding this comment.
I opted not to use PublicInterface here because it aligns with AbstractTokenPricesService, and it's more explicit IMO.
| }); | ||
|
|
||
| describe('circuit breaker', () => { | ||
| it('opens the circuit breaker after consecutive failures', async () => { |
There was a problem hiding this comment.
Similar to tests in codefi-v2.test.ts, we don't need these since they're already covered by the createServicePolicy tests.
Prithpal-Sooriya
approved these changes
Jan 24, 2025
|
|
||
| - Deprecate `ClientConfigApiService` constructor options `onBreak` and `onDegraded` in favor of methods ([#5109](https://github.com/MetaMask/core/pull/5109)) | ||
| - Add `@metamask/controller-utils@^11.4.5` as a dependency ([#5109](https://github.com/MetaMask/core/pull/5109)) | ||
| - `cockatiel` should still be in the dependency tree because it's now a dependency of `@metamask/controller-utils` |
There was a problem hiding this comment.
(comment, can be resolved) IIRC extension assets controller had added patches for cockatiel imports. It would be interesting to see if they are still needed, or if the patch is instead moved to this utils package.
There was a problem hiding this comment.
Ah interesting. I wonder if Cockatiel defines a default export in addition to named exports. But to answer your question yes I think these patches may need to be moved to controller-utils.
zone-live
pushed a commit
that referenced
this pull request
Jan 27, 2025
In a future commit, we are adding an RpcService class to `network-controller` to handle automatic failovers. This class is different from the existing service classes in a couple of ways: - RpcService will make use of a new `createServicePolicy` function which encapsulates retry and circuit breaker logic. This will relieve any module that consumes RpcService from needing to test this logic. - RpcService will have `onBreak` and `onDegraded` methods instead of taking `onBreak` and `onDegraded` callbacks as constructor options. This is reflected not only in the class but also in the abstract RPC service interface. To these ends, this commit makes the following changes to `CodefiTokenPricesServiceV2` in `assets-controllers` and `ClientConfigApiService` in `remote-feature-flag-controller`: - Refactor service class to use `createServicePolicy` - Update the abstract service class interface to extend `ServicePolicy` from `controller-utils` - Deprecate the `onBreak` and `onDegraded` constructor options, and add methods instead Note that we are not including `TokenSearchApiService` in this commit, even though it is a class, because it does not use the retry or circuit breaker policy.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Explanation
In a future commit, we are adding an RpcService class to
network-controllerto handle automatic failovers. This class is different from the existing service classes in a couple of ways:createServicePolicyfunction which encapsulates retry and circuit breaker logic. This will relieve any module that consumes RpcService from needing to test this logic.onBreakandonDegradedmethods instead of takingonBreakandonDegradedcallbacks as constructor options. This is reflected not only in the class but also in the abstract RPC service interface.To these ends, this commit makes the following changes to
CodefiTokenPricesServiceV2inassets-controllersandClientConfigApiServiceinremote-feature-flag-controller:createServicePolicyServicePolicyfromcontroller-utilsonBreakandonDegradedconstructor options, and add methods insteadNote that we are not including
TokenSearchApiServicein this commit, even though it is a class, because it does not use the retry or circuit breaker policy.References
Closes #4994.
Changelog
(Updated in changelog.)
Checklist