fix: poll immediately and honor the timeout in SendUseroperationResponse.included - #209
fix: poll immediately and honor the timeout in SendUseroperationResponse.included#209Sednaoui wants to merge 2 commits into
Conversation
…nse.included Fixes #130
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughSendUseroperationResponse.included() is refactored to use a deadline-based loop that polls immediately, caps each sleep to remaining time, and validates timing inputs for non-finite values. Tests and CHANGELOG.md updated to reflect and verify the behavior. ChangesPolling Timeout Fix
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3bd146646f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (remainingMs <= 0) { | ||
| break; | ||
| } | ||
| await this.delay(Math.min(requestIntervalInSeconds * 1000, remainingMs)); |
There was a problem hiding this comment.
Validate finite polling inputs
When timeoutInSeconds or requestIntervalInSeconds is NaN (for example from Number(process.env.USEROP_TIMEOUT) with an unset variable), the existing <= 0 and < checks do not reject it, and this new capped-sleep path passes NaN to setTimeout. In Node that becomes a 1 ms timer; with a NaN timeout, remainingMs <= 0 is never true, so included() can poll indefinitely at high frequency, and with a NaN interval it floods until the timeout. Please reject non-finite values before entering the loop.
Useful? React with 👍 / 👎.
sherifahmed990
left a comment
There was a problem hiding this comment.
included is usually used after sendUserOperation, so polling immediately is a guaranteed miss. It is better to wait first then poll to give a chance for the bundler to submit the bundle transaction.
Fixes #130
Red/green verified with real timers (
test/transport/includedPolling.test.js, mock transport): on pre-fix dev, an immediately-available receipt still took the full 3s interval andincluded(2, 2)threw after ~4s; with the fix the receipt returns in ~3ms and the timeout throws at ~2s.Follow-up: non-finite
timeoutInSeconds/requestIntervalInSeconds(e.g.NaNfromNumber(unsetEnvVar)) slipped past the<= 0checks and, with the deadline loop, degraded into a ~1mssetTimeoutpolling indefinitely (measured: 258 polls in 300ms, never settling).included()now rejects non-finite inputs with aRangeErrorup front; covered by a regression test.Summary by CodeRabbit
Release Notes