feat: request missing on chain events on error #2298
Conversation
🦋 Changeset detectedLatest commit: cebffb7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| }; | ||
| } | ||
|
|
||
| // TODO(aditi): Make sure we don't make multiple requests for a single fid |
There was a problem hiding this comment.
Hmm, this is not a problem now, but when you remove the await when calling the retry function, it will become an issue. You might need to add a lock around the duplicate check. I would add a test case when submitting 10 messages via mergeMessages for the same fid, it only calls retry once.
There was a problem hiding this comment.
This is currently pretty hard to test. We mock the l2 events provider in most tests -- which I think is a good practice. The l2 events tests are slow and the setup for each test is really complicated. It's hard to test the logic that checks for duplicates via mergeMessage
I don't think I understand the failure mode here though. All this code is in JS-land so we don't need to worry about concurrency issues right? Are you worried about the JS runtime switching to a new task between the has and set calls? I also don't think it's the biggest issue if we end up with multiple concurrent calls.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2298 +/- ##
===========================================
- Coverage 74.05% 44.44% -29.62%
===========================================
Files 99 146 +47
Lines 9425 25563 +16138
Branches 2097 9063 +6966
===========================================
+ Hits 6980 11362 +4382
- Misses 2327 12481 +10154
- Partials 118 1720 +1602 ☔ View full report in Codecov by Sentry. |
aditiharini
force-pushed
the
recover-from-missing-on-chain-events
branch
from
2365e16 to
5b5c5fe
Compare
aditiharini
force-pushed
the
recover-from-missing-on-chain-events
branch
from
01b163e to
6b19220
Compare
aditiharini
force-pushed
the
recover-from-missing-on-chain-events
branch
from
6b19220 to
1508b0b
Compare
| if ( | ||
| result.error.errCode === "bad_request.no_storage" || | ||
| "bad_request.unknown_signer" || | ||
| "bad_request.missing_fid" |
There was a problem hiding this comment.
Q: Should that be bad_request.unknown_fid as the name suggested in line 1237
Why is this change needed?
Our hubs regularly fail to get into sync because some of them are missing onchain events. This feature will have us request missing onchain events if there are errors related to missing on chain events when messages are submitted.
Deployed to hoyt for testing: Logs
Merge Checklist
Choose all relevant options below by adding an
xnow or at any time before submitting for reviewPR-Codex overview
The focus of this PR is to enhance error handling and retry mechanisms for on-chain events in the Hubble application.
Detailed summary