fix(http-client-proof-provider): honor per-call proveTxConfig.timeout#983
Conversation
httpClientProofProvider().proveTx() previously accepted a ProveTxConfig parameter but discarded it (the parameter was prefixed with _). The underlying httpClientProvingProvider was built once at construction time with a fixed timeout, so the per-call proveTxConfig.timeout had no effect. This change: - Builds a fresh httpClientProvingProvider per proveTx call so the per-call timeout is honored. - Spreads the construction-time config so non-timeout fields (e.g. custom headers) still carry through to every call. - Imports DEFAULT_TIMEOUT from the sibling module instead of hardcoding 300000, keeping the timeout source of truth in one place. - Documents the precedence rule (per-call > construction > default) in a resolveTimeout helper with a reference to issue midnightntwrk#974. Fixes midnightntwrk#974
|
|
Add a Vitest describe block exercising the per-call
proveTxConfig.timeout behavior introduced by the previous commit.
The five tests cover the precedence rule
(per-call > construction-time > DEFAULT_TIMEOUT), preservation of
non-timeout construction-time config fields, and the per-call
ProvingProvider construction that is the whole point of the fix.
Uses vi.mock('../http-client-proving-provider') to replace the
low-level factory with a capturing mock, so the tests assert on the
ProvingProviderConfig each call site wires through without needing a
live proof server. Closes midnightntwrk#974.
|
Just added a test for the per-call timeout precedence (5 tests covering the rule, config preservation, and per-call provider construction). All using vi.mock to assert on what timeout gets wired through without needing a live proof server. Ready for the CLA + CI pipeline. |
This comment has been minimized.
This comment has been minimized.
|
Heads up — the three failing checks all fail at the same step ( Filed a separate issue with reproduction + suggested fixes: #989. Happy to retest once CI is unblocked. |
CjDabrow
left a comment
There was a problem hiding this comment.
Thanks for picking this up, and for the clean fix. I filed #974, so it is great to see the per-call timeout actually plumbed through. A few notes, mostly confirmation plus one thing worth a follow-up.
Looks right against the issue's acceptance criteria. The resolveTimeout precedence (per-call > construction-time > DEFAULT_TIMEOUT) matches the spec exactly, and building the proving provider per call rather than once at construction is the clean way to honor the per-call value without sharing mutable timeout state between calls, so there is no leakage across sequential or concurrent calls. Swapping the magic 300000 for the exported DEFAULT_TIMEOUT is a nice touch too. And the tests asserting the timeout actually wired into each provider call, rather than just that proveTx returns, are exactly the right shape for this.
One thing worth a follow-up: timeout vs the retry budget. You flagged this yourself on the issue, and I think it is worth surfacing for callers. The underlying provider's fetch-retry (retries plus exponential backoff) means proveTxConfig.timeout is effectively a per-request timeout, not a hard wall-clock ceiling for the whole proveTx call. So after this fix the per-call value is honored, but a caller reading "the timeout for the request" on ProveTxConfig might expect a total upper bound and not get one. A short follow-up issue, or even a one-line JSDoc note on ProveTxConfig.timeout clarifying it is per-request, would set expectations. Not a blocker for this PR, which fixes the bug as filed.
Minor:
- Building a fresh
httpClientProvingProvideron everyproveTxcall repeats its setup (URL validation, closure setup) each time. Negligible for normal use, and the per-call approach is the right call for correctness, just noting it in caseproveTxever runs in a tight loop. - If it is not already in the suite, a test that does two sequential calls (one with a per-call timeout, then one without) and asserts the second falls back to the construction-time or default value would lock down the no-leak criterion explicitly.
Overall this looks like a correct, well-tested fix. Thanks again for taking it.
…lock Addresses the follow-up from the PR midnightntwrk#983 review (chaps/CjDabrow): the field's existing doc said only 'The timeout for the request.', which a caller could read as a hard wall-clock ceiling for the whole proveTx call. The underlying proof provider has its own fetch-retry with exponential backoff, so the per-call value here is honored per request, not as a total upper bound for proveTx. Refs midnightntwrk#974.
|
@CjDabrow thanks — addressed the jsdoc follow-up in a new commit on the branch (docs(types): clarify ProveTxConfig.timeout is per-request, not wall-clock, refs #974). The sequential-calls test was already in the suite under 'each proveTx call builds its own ProvingProvider (no fixed provider reuse)' so the no-leak criterion is locked down there. Re-requesting your review once the rebase lands. Heads-up on the three failing checks — those aren't the code, they're a fork-pr CI infrastructure bug I filed separately as #989 (ghcr.io auth not granted to pull_request events from forks, so yarn build / test never actually ran). Maintainer-side fix, not in scope for this PR. |
What this fixes
Closes #974.
httpClientProofProvider().proveTx(unprovenTx, proveTxConfig?)accepted aProveTxConfigparameter but silently discarded it — the parameter was even prefixed with_partialProveTxConfigto mark it as intentionally unused. The underlyinghttpClientProvingProviderwas constructed once athttpClientProofProvider(...)call time with a fixed timeout, so passingproveTxConfig.timeoutper-call had no effect.This change makes the per-call timeout actually take effect.
What changed
packages/http-client-proof-provider/src/http-client-proof-provider.ts(+29 / −6)ProvingProviderperproveTxcall. Previously the provider was constructed once athttpClientProofProvider()time and reused forever; now it's built insideproveTxso each call gets its own timeout.proveTxConfig.timeout ?? config.timeout ?? DEFAULT_TIMEOUT. Per-call > construction-time > default. The helper carries a JSDoc that links back to @midnight-ntwrk/midnight-js-http-client-proof-provider: proveTx ignores its ProveTxConfig (per-call timeout has no effect) #974 so future maintainers understand the rationale.config({ ...config, timeout: resolveTimeout(...) }) so non-timeout fields like customheadersstill carry through to every call. Callers don't have to re-supply them.DEFAULT_TIMEOUTfrom the sibling module instead of hardcoding300000. Removes a duplicated constant; the value is defined once inhttp-client-proving-provider.ts.No changes to public types (
ProveTxConfig,ProofProvider,ProvingProviderinterfaces are untouched). No changes to other packages.Why I didn't bundle the related concern
While reading
http-client-proving-provider.ts, I noticed thatfetch-retryis configured withretries: 3, retryOn: [500, 503]and exponential backoff (2 ** attempt * 1000). That means even with the per-call timeout correctly plumbed, the effective wall-clock ceiling under sustained 503s is roughlytimeout × 8(1 + 1 + 2 + 4 seconds of backoff before the final retry fails).So this PR makes the timeout parameter honored, but it isn't a hard ceiling under retry amplification. I'm flagging it here rather than bundling it in, because:
ProvingProviderindependently;Happy to file a follow-up issue / PR for the retry cap if maintainers want it tracked.
Verification
I could not run the full monorepo test suite locally (
yarn installrequires installing yarn first; the monorepo uses Turborepo + workspace protocol dependencies that I didn't want to risk breaking the user's machine on). The change is small enough that the CI in this repo (which runsturbo run testandeslint) will exercise it cleanly. I read the existing tests in the package — they coverhttpClientProvingProviderdirectly, not the high-levelhttpClientProofProviderwrapper, so there is no existing test that regresses; adding a focused test for this specific code path would benefit from guidance on whether the maintainers wantvi.spyOnmocking of the lower-level provider or a behavior test against a live proof server.Checklist
mainand tracks the upstream repo— Harley