fix: harden RPC error classification, nonce validation, and builder type honesty - #466
Conversation
…ype honesty Resolve four Stellar Wave issues: - conduit-protocol#456: RateLimitError.fromRpcError() no longer conflates HTTP 503 with 429. A 503 is now reported as a distinct exported RpcServiceUnavailableError, and the internal RPC retry wrapper only backoff-retries genuine RateLimitErrors so callers can fail over to another RPC URL instead of retrying a dead endpoint. - conduit-protocol#457: catchNetworkError() only reclassifies errors that are provably transport failures (canonical fetch/axios messages or a network errno code on the error or its nested cause) instead of substring-matching the whole error text, so unrelated TypeErrors are no longer masked as network outages. - conduit-protocol#458: NonceManager.toSafeBigInt() throws a descriptive error for unparseable nonce strings instead of silently coercing them to 0n. - conduit-protocol#459: StreamBuilder.build() stringifies a numeric ratePerSecond so the runtime value matches the declared `ratePerSecond?: string` return type. Adds regression tests for all four fixes and updates docs + CHANGELOG. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
|
@Teescom Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
|
Thanks for the contribution here — squash-merging this now. Any follow-ups we'll track in a fresh issue. 🚀 |
…ype honesty (#466) Co-authored-by: Teescom <Teescom@users.noreply.github.com>
|
Merged into |
Summary
This PR resolves four Stellar Wave issues (#456, #457, #458, #459) that all fall under the same theme: error classification and type honesty. Each one made the SDK either misreport failures or silently mask caller bugs.
Closes #456, Closes #457, Closes #458,Closes #459
Changes
#456 —
RateLimitError.fromRpcError()conflates HTTP 503 with 429Problem: HTTP 503 (Service Unavailable) and 429 (Too Many Requests) were wrapped into the same
RateLimitError, so a consumer catchingRateLimitErrorto back off and retry the same endpoint would retry forever against a node that is actually down.Fix:
RpcServiceUnavailableErrorclass insrc/errors.ts(mirrorsRateLimitError's shape, includingretryAfterMsparsed fromRetry-After).RateLimitError.fromRpcError()now returns aRateLimitErrorfor 429 (and JSON-RPC429/-32029) and aRpcServiceUnavailableErrorfor 503, with a distinct message telling callers to consider failing over to a different RPC URL.createRpcServerinsrc/soroban.ts) now only backoff-retries genuineRateLimitErrorinstances — a 503 fails fast so it surfaces immediately instead of being retried against a dead endpoint.RpcServiceUnavailableErrorfrom the package root (src/index.ts).#457 —
catchNetworkError()misclassifies unrelatedTypeErrors as network errorsProblem:
catchNetworkError()substring-matched the entire error text against/fetch|network|connect|.../i. A programming bug likeTypeError: Cannot read properties of undefined (reading 'connect')was reported to the caller as a network outage, hiding the real bug.Fix (
src/soroban.ts):TypeErrors are only reclassified when they are provably transport failures:fetch failed(undici),Failed to fetch(Chromium),Network Error(axios),Load failed(Safari).causechain (where Node's undici hides the real errno) — carries a network errno code (ECONNREFUSED,ENOTFOUND,ETIMEDOUT,ENETUNREACH,ERR_NETWORK,UND_ERR_*,ERR_CONN_*, etc.).TypeErrors, is re-thrown unchanged.#458 —
NonceManager.toSafeBigInt()silently coerces unparseable nonces to0nProblem:
new NonceManager({ startNonce: 'not-a-number' })silently started at0n, masking a caller bug (e.g. a stringifiedundefinedor a malformed network value) as an explicit0.Fix (
src/nonce/NonceManager.ts):toSafeBigInt()now throws a descriptive error for unparseable strings — and for empty strings, whichBigInt('')would otherwise coerce to0n— consistent with the constructor's other descriptive guards. Valid numeric strings ('42','9007199254740993') still work, andisNonceValid()semantics are unchanged.#459 —
StreamBuilder.build()claimsratePerSecondis always a stringProblem:
.ratePerSecond(500).build()produced a runtimenumberwhile the declared return type promisedstring(becausebigintSafeStringify()only stringifiesbigintvalues). Callers trusting the type (.trim(), string concatenation) hit runtime errors TypeScript couldn't catch.Fix (
src/builder.ts):build()now coerces a numericratePerSecondto its string form, so the runtime value matches the declaredratePerSecond?: stringtype. The public API (ratePerSecond(val: number | bigint)) is unchanged, and bigint inputs still stringify as before.Tests
src/tests/rate-limit-error.test.ts— 503 →RpcServiceUnavailableError(distinct fromRateLimitError, parsesRetry-After), plus a 429-vs-503 distinguishability test.src/tests/soroban-rate-limit.test.ts— a 503 thrown throughsimulateReadOnlysurfaces asRpcServiceUnavailableErrorwithout being retried (mock called exactly once).src/tests/soroban-network-error.test.ts(new) — 10 cases covering genuine network errors (fetch failed, nestedECONNREFUSEDcause, browserFailed to fetch,ENOTFOUND, axiosERR_NETWORK) vs. unrelatedTypeErrors mentioningconnect/fetch, plus passthrough of already-classified errors.src/tests/nonce-concurrent.test.ts— unparseable and empty-string nonces throw descriptive errors; numeric/bigint/valid-string nonces still work.src/tests/builder.test.ts— numericratePerSecondnow serialises to'500'(string) and survivesJSON.stringify.Verification:
npm run typecheck✅ ·npm run lint✅ ·npm test✅ (738 passed, 2 skipped).Docs & Changelog
docs/api.md— updated the RPC wrapper note: 429 is retried with backoff; 503 is not retried and surfaces asRpcServiceUnavailableError.CHANGELOG.md— documented all four fixes under [Unreleased] → Fixed.Notes for reviewers
fromRpcError()return type widened fromRateLimitError | nulltoRateLimitError | RpcServiceUnavailableError | null— this is the intended behavior change (503 is no longer aRateLimitError), but code that annotated the result strictly asRateLimitError | nullwill need updating.throw RateLimitError.fromRpcError(err) ?? err) keep working unchanged; for a 503 they now throw the typedRpcServiceUnavailableErrorinstead.