Skip to content

Add error property to "degraded" events #6188

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 12 commits into from
Aug 12, 2025
Merged

Add error property to "degraded" events #6188

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
889b5df
WIP - Add error to NetworkController:rpcEndpointDegraded
mcmire Jul 24, 2025
1e29c44
Update changelog
mcmire Jul 24, 2025
7dc0e90
Fix changelog
mcmire Jul 24, 2025
4693d47
Recategorize under Added
mcmire Jul 24, 2025
8379d0e
Merge branch 'main' into add-error-to-rpc-endpoint-degraded
mcmire Aug 6, 2025
54cf37f
Fix changelog
mcmire Aug 6, 2025
0b19e79
Fix changelog again
mcmire Aug 6, 2025
611dc37
Really really fix changelog
mcmire Aug 6, 2025
25772fc
Downgrade the changes to non-breaking
mcmire Aug 6, 2025
19968e3
Remove comment
mcmire Aug 11, 2025
e51abf5
Merge branch 'main' into add-error-to-rpc-endpoint-degraded
mcmire Aug 11, 2025
b891700
Merge branch 'main' into add-error-to-rpc-endpoint-degraded
mcmire Aug 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions packages/controller-utils/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

### Added
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be Changed ? because it feels a bit strange to have under Added section Update as a first keyword ?

Copy link
Contributor Author

@mcmire mcmire Aug 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Keep a Changelog spec says that "Added" is for new features and "Changed" is for changes in existing functionality. I've always found this to be a bit lacking, though.

The way I've been treating "Changed" is that it is for changes in existing behavior or functionality that do not come with changes to the API itself. For instance, maybe a method was updated so that the logic is different but returns data in the same shape. "Added", on the other hand, is for changes that extend the API in some way, so this could be something larger like a new class, function, method, etc. but also something smaller like a new argument or a new property on a type.

I guess I could have said something like:

- `createServicePolicy`: Add `error` property to the payload for `onDegraded` ([#6188](https://github.com/MetaMask/core/pull/6188))
  - This can be used to access the error produced by the last request when the maximum number of retries is exceeded
  - This property will not be present if the degraded event merely represents a slow request

Would that sound like it fits better? Or what is your view on Added vs. Changed?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes looks good


- Update `onDegraded` in `createServicePolicy` so that its event payload now includes an `error` property which can be used to access the error produced by the last request when the maximum number of retries is exceeded ([#6188](https://github.com/MetaMask/core/pull/6188))
- This `error` property will be `undefined` if the degraded event merely represents a slow request

## [11.11.0]

### Added
Expand Down
10 changes: 6 additions & 4 deletions packages/controller-utils/src/create-service-policy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import {
import type {
CircuitBreakerPolicy,
Event as CockatielEvent,
FailureReason,
IBackoffFactory,
IPolicy,
Policy,
Expand Down Expand Up @@ -95,7 +96,7 @@ export type ServicePolicy = IPolicy & {
* never succeeds before the retry policy gives up and before the maximum
* number of consecutive failures has been reached.
*/
onDegraded: CockatielEvent<void>;
onDegraded: CockatielEvent<FailureReason<unknown> | void>;
/**
* A function which will be called by the retry policy each time the service
* fails and the policy kicks off a timer to re-run the service. This is
Expand Down Expand Up @@ -229,10 +230,11 @@ export function createServicePolicy(
});
const onBreak = circuitBreakerPolicy.onBreak.bind(circuitBreakerPolicy);

const onDegradedEventEmitter = new CockatielEventEmitter<void>();
retryPolicy.onGiveUp(() => {
const onDegradedEventEmitter =
new CockatielEventEmitter<FailureReason<unknown> | void>();
retryPolicy.onGiveUp((data) => {
if (circuitBreakerPolicy.state === CircuitState.Closed) {
onDegradedEventEmitter.emit();
onDegradedEventEmitter.emit(data);
}
});
retryPolicy.onSuccess(({ duration }) => {
Expand Down
5 changes: 5 additions & 0 deletions packages/network-controller/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

### Added

- The object in the `NetworkController:rpcEndpointDegraded` event payload now includes an `error` property, which can be used to access the error produced by the last request when the maximum number of retries is exceeded ([#6188](https://github.com/MetaMask/core/pull/6188))
- This `error` property will be `undefined` if the degraded event merely represents a slow request

### Changed

- Bump `@metamask/base-controller` from `^8.0.1` to `^8.1.0` ([#6284](https://github.com/MetaMask/core/pull/6284))
Expand Down
1 change: 1 addition & 0 deletions packages/network-controller/src/NetworkController.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -470,6 +470,7 @@ export type NetworkControllerRpcEndpointDegradedEvent = {
{
chainId: Hex;
endpointUrl: string;
error: unknown;
},
];
};
Expand Down
10 changes: 9 additions & 1 deletion packages/network-controller/src/create-network-client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,10 +111,18 @@ export function createNetworkClient({
error,
});
});
rpcServiceChain.onDegraded(({ endpointUrl }) => {
rpcServiceChain.onDegraded(({ endpointUrl, ...rest }) => {
let error: unknown;
if ('error' in rest) {
error = rest.error;
} else if ('value' in rest) {
error = rest.value;
}

messenger.publish('NetworkController:rpcEndpointDegraded', {
chainId: configuration.chainId,
endpointUrl,
error,
});
});
rpcServiceChain.onRetry(({ endpointUrl, attempt }) => {
Expand Down
4 changes: 2 additions & 2 deletions packages/network-controller/src/rpc-service/rpc-service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -361,8 +361,8 @@ export class RpcService implements AbstractRpcService {
{ endpointUrl: string }
>,
) {
return this.#policy.onDegraded(() => {
listener({ endpointUrl: this.endpointUrl.toString() });
return this.#policy.onDegraded((data) => {
listener({ ...(data ?? {}), endpointUrl: this.endpointUrl.toString() });
});
}

Expand Down
5 changes: 1 addition & 4 deletions packages/network-controller/src/rpc-service/shared.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,5 @@ export type FetchOptions = RequestInit;
*/
export type AddToCockatielEventData<EventListener, AdditionalData> =
EventListener extends (data: infer Data) => void
? // Prevent Data from being split if it's a type union
[Data] extends [void]
? (data: AdditionalData) => void
: (data: Data & AdditionalData) => void
? (data: Data extends void ? AdditionalData : Data & AdditionalData) => void
Copy link
Contributor Author

@mcmire mcmire Aug 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was an interesting one to figure out.

The purpose of this utility type is to extend the type for a Cockatiel event with additional payload data. We need to do this because NetworkController:rpcEndpointDegraded and NetworkController:rpcEndpointAvailable are based on existing Cockatiel events but with additional properties we add.

However, this type did not properly account for when EventListener was CockatielEvent<FailureReason<unknown> | void> (which is the same thing as saying (data: void | { error: Error } | { value: unknown }) => void).

If we say the following

AddToCockatielEventData<
  (data: void | { error: Error } | { value: unknown }) => void,
  { foo: 'bar' }
>

we want this to resolve to:

(data: { foo: 'bar' } | ({ error: Error } & { foo: 'bar' }) | ({ value: unknown } & { foo: 'bar' })) => void

(Essentially, we want the void to be treated as {}.)

But instead this was resolving as:

(data: ((void | { error: Error } | { value: unknown }) & { foo: 'bar' }) => void

which distributes to:

(data: (void & { foo: 'bar' }) | ({ error: Error } & { foo: 'bar' }) | ({ value: unknown } & { foo: 'bar' })) => void

This is incorrect, because void & { foo: 'bar' } doesn't make sense.

What we want is for TypeScript to distribute void | { error: Error } | { value: unknown } over the condition. So first, despite the comment here, we don't want [Data] extends [void], because that prevents TypeScript from applying the distribution.

But the second thing is that even if we say Data extends void then TypeScript still won't apply the distribution, because the condition is actually in the wrong place. What we want is not a type union of three function types, but a single type with a type union for the arguments.

Even if the condition were in the place we wanted it, TypeScript seems to distribute the type union only in a particular circumstance. Looking a bit deeper at the TypeScript 2.8 release notes where this concept was introduced, it says: "Distributive conditional types are automatically distributed over union types during instantiation." In this case, we also need to place the condition so it becomes a part of the type that we want to "return" from this utility type and is not used to simply determine the return type. This way TypeScript evaluates the condition when that return type is used and not beforehand.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change make sense.

: never;
Loading