Skip to content
This repository was archived by the owner on Oct 16, 2025. It is now read-only.

PollingBlockTracker: Stop wrapping errors#310

Merged
mcmire merged 4 commits intomainfrom
dont-wrap-errors
Apr 8, 2025
Merged

PollingBlockTracker: Stop wrapping errors#310
mcmire merged 4 commits intomainfrom
dont-wrap-errors

Conversation

@mcmire
Copy link
Copy Markdown
Contributor

@mcmire mcmire commented Mar 11, 2025

If PollingBlockTracker fails to make a request, the error is wrapped with a marker and a more descriptive message (including the stack trace) before being emitted under the "error" event or logged. This wrapping may have been added in the past to assist with debugging, but it is not necessary now, and it complicates the tests.

mcmire added 2 commits March 11, 2025 08:48
We do not presently use this class in any repo under the MetaMask
organization. In general we have found a polling-based approach to be
more reliable than a subscription-based approach.
If PollingBlockTracker fails to make a request, the error is wrapped
with a marker a more descriptive message (including the stack trace)
before being emitted under the "error" event or logged. This wrapping
may have been added in the past to assist with debugging, but it is not
necessary now, and it complicates the tests.
console.error(newErr);
this.emit('error', error);
} catch {
console.error(`Error updating latest block: ${getErrorMessage(error)}`);
Copy link
Copy Markdown
Contributor Author

@mcmire mcmire Mar 11, 2025

Choose a reason for hiding this comment

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

Question: Do we even need to log the error anymore? Can we just let the error be thrown here if there are no "error" listeners? (This error would manifest as an unhandled rejection, but we could catch it in the polling loop so it doesn't leak out.)

Base automatically changed from remove-subscribe-block-tracker to main March 12, 2025 14:35
Comment thread src/PollingBlockTracker.test.ts
@mcmire mcmire marked this pull request as ready for review March 12, 2025 14:39
@mcmire mcmire requested a review from a team as a code owner March 12, 2025 14:39
Copy link
Copy Markdown
Contributor

@cryptodev-2s cryptodev-2s left a comment

Choose a reason for hiding this comment

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

LGTM!

@mcmire mcmire enabled auto-merge (squash) April 8, 2025 20:13
@mcmire mcmire merged commit 36c8928 into main Apr 8, 2025
7 checks passed
@mcmire mcmire deleted the dont-wrap-errors branch April 8, 2025 20:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants