feat(client): surface Poly-RateLimit state on rate-limit errors and via onRateLimitUpdate - #254
Conversation
…it-headers-poly-ratelimit-retry
brunson-bot
left a comment
brunson-bot
left a comment
There was a problem hiding this comment.
Reviewed the whole diff plus the emitting side (polymarket/clob-v2 packages/trading-gateway, main @ d75daff70). The wiring is right: clob + secureClob are the only service clients that can ever see these headers (only the trading gateway sets them), and the listener survives upgrade (clients.ts:499) and logout (clients.ts:768). Header semantics check out against the gateway — Poly-RateLimit-Reset really is now + ResetTime in unix seconds (api.go:268), Remaining really can go negative for standard tiers via the post-cancel debit (api.go:305, rate_limiter.go Debit), Poly-RateLimit-Warning is only ever set to "true" and only in warn mode when the request would have been blocked (api.go:270-273), and the headers are attached before the 429 is written, so RateLimitError.rateLimit is populated. minor bump is right, all CI green.
3 [issue] + 2 [nit] inline, nothing blocking. The one I'd fix before merge is the async-listener case: the documented "errors are ignored" guarantee doesn't hold for async listeners.
| ); | ||
| } | ||
|
|
||
| #notifyRateLimitUpdate(update: RateLimitUpdate): void { |
There was a problem hiding this comment.
[issue] The try/catch only covers synchronous throws, but RateLimitUpdateListener accepts async listeners: (update) => Promise<void> is assignable to (update) => void (verified, tsc 5.9.3). So onRateLimitUpdate: async (update) => { await metrics.report(update) } with a failing report produces an unhandled rejection — process-fatal under Node's default --unhandled-rejections=throw (verified: exit 1). That contradicts the comment on the next line and the onRateLimitUpdate TSDoc at clients.ts:869 ("Errors thrown by the listener are ignored").
#notifyRateLimitUpdate(update: RateLimitUpdate): void {
try {
void Promise.resolve(this.#onRateLimitUpdate?.(update)).catch(() => {});
} catch {
// A consumer-provided listener must never affect request handling.
}
}The ignores rate-limit listener errors test only covers the sync throw, so this stays uncovered.
| * the request was evaluated; `warning` is `false` whenever the response does | ||
| * not report warning mode. | ||
| */ | ||
| export type RateLimitUpdate = { |
There was a problem hiding this comment.
[issue] The update carries no scope, but the gateway keeps two independent buckets per signer — rate_limit:{signer}:order and rate_limit:{signer}:cancel (rate_limiter.go:79), with separate rates/bursts and separate Remaining, both reported through the same header names (api.go:266). Both flow through the same ServiceClient: posts hit secureClob.post('/order'|'/orders') (actions/orders/post.ts:85,123) and cancels hit secureClob.del('/order'|'/orders'|'/cancel-all'|'/cancel-market-orders') (actions/orders/cancel.ts:83,129,165,212).
So a consumer gets one interleaved stream of remaining values from two buckets with no way to tell them apart — remaining: 3 after a cancel says nothing about order capacity, and the documented use ("monitor remaining capacity … adjust request patterns") can't be implemented correctly. Path alone isn't enough to disambiguate downstream either (POST /order vs DELETE /order are the same URL, and Response doesn't carry the method), so it needs to come from the SDK side: #toResult is called from #request (ServiceClient.ts:138) where both method and path are in hand — thread them through and expose either the request context or a derived bucket: 'order' | 'cancel' on the update.
| * request was accounted for. Can be negative for tiers that allow a | ||
| * negative cancellation balance. | ||
| */ | ||
| remaining?: number; |
There was a problem hiding this comment.
[issue] remaining: 0 does not imply exhaustion. handleRateLimit emits the header set whenever mode isn't disabled and Redis didn't error (api.go:259-268), but two paths return a zero-valued result with Allowed: true: an exempt client IP (rate_limiter.go:62-64) and a tier whose rate or burst is unconfigured (:72-74). Both ship Poly-RateLimit-Remaining: 0 with Reset: now. A consumer following the TSDoc guidance would throttle itself to a standstill while actually being unlimited — and the unconfigured-rates case is exactly the rollout state this feature is meant to observe. Worth either documenting the caveat here or having the gateway skip the headers on those two paths.
| * Per-signer rate-limit state reported by order and cancellation responses. | ||
| * | ||
| * @remarks | ||
| * Fields mirror the `Poly-RateLimit-*` response headers. Each optional field |
There was a problem hiding this comment.
[nit] "any subset can be present" is broader than the emitter: Remaining, Tier and Reset are always set together (api.go:266-268), and the post-cancel debit re-sets Remaining + Reset on the same response (api.go:305-306), so the only realistic absence is Tier (empty on the exempt path) or a value that fails to parse. Keeping the fields optional at a wire boundary is fine; the sentence just invites more defensive branching than the platform requires.
| return undefined; | ||
| } | ||
|
|
||
| const parsed = Number(value); |
There was a problem hiding this comment.
[nit] Number() accepts '1.5', '1e3' and '0x10', while #parseRetryAfterHeader in ServiceClient.ts:273 guards the sibling header with /^\d+$/. Both values are strconv.Itoa/FormatInt server-side, so /^-?\d+$/ here would match the existing convention and keep remaining an integer (the - is needed for the negative cancel balance).
brunson-bot
left a comment
brunson-bot
left a comment
There was a problem hiding this comment.
Round 2 (10e0f27 merge + 4081d5c). All five round-1 findings are closed:
- async listener errors —
void Promise.resolve(...).catch(...)inside the existingtrycovers both quadrants (a sync throw propagates out of the argument expression beforePromise.resolveruns, so thecatchblock still catches it), and both are now tested. The TSDoc atclients.ts:900was updated to say returned rejections are ignored too. - bucket ambiguity —
bucket: 'order' | 'cancel'now rides on the update and onRateLimitError.rateLimit. Re-verified the mapping againstclob-v2main (packages/trading-gateway/pkg/api/api.go:84-89): the six routes that callhandleRateLimitare exactlyPOST /order,POST /orders(api.go:404,:539, order bucket) andDELETE /order|/orders|/cancel-all|/cancel-market-orders(:649,:698,:773,:856, cancel bucket). The SDK's table matches all six with nothing missing and nothing extra, and the Redis key israte_limit:{signer}:{bucket}(rate_limiter.go:88) — makers only pick the tier (GetPostUserTier), so "per-signer" in the docs is accurate. remaining: 0≠ exhausted — documented on the field, with the "do not back off solely because this value is zero" line. That covers both zero-valued-but-allowed paths (rate_limiter.go:69-71unlimited IP,:78-80unconfigured rate/burst).- "any subset can be present" and
Number()laxity — reworded, andparseIntegerHeadernow takes/^-?\d+$/forremaining(negative cancel balances) and/^\d+$/forreset, plusNumber.isSafeInteger.
The 10e0f27 merge had a genuine conflict in ServiceClient.test.ts (both sides appended tests at the same spot). Resolution is correct — 11 base + 4 from this branch + 4 from #248 = 19 test titles, none dropped, only deviation from the auto-merge tree is the conflict markers and the brace placement they broke.
Also checked and not a finding: ky.create({ throwHttpErrors: false }) means a 429 never becomes an HTTPError, so ky's default retry (which includes DELETE and status 429) never fires and the listener can't double-report a single cancel.
All CI green. 1 [issue] + 1 [nit] inline, neither blocking.
| return path.startsWith('/') ? path.slice(1) : path; | ||
| } | ||
|
|
||
| #rateLimitBucket( |
There was a problem hiding this comment.
[issue] This puts the CLOB trading-gateway route table inside the service-agnostic transport, four days after #248 took the opposite direction in this same file: 1b55dd0 ("scope trading restrictions to order actions") pulled the 425 / post_only_mode interpretation out of ServiceClient into actions/orders/restrictions.ts, review then removed even the per-request mapRejectedResponse hook (52fb561), and the test that locks it in — does not apply service-specific policy to rejected responses, ServiceClient.test.ts:483 — now sits ~130 lines below the new bucket tests.
The six literals here are a second copy of paths the order actions own: post.ts:86,125 and cancel.ts:84,130,166,213, which already tag those exact calls with .mapErr(mapTradingRestrictionError). Nothing ties the two lists together, so renaming a path in cancel.ts silently degrades bucket to undefined with every test still green. A rateLimitBucket?: RateLimitBucket on the request options, set at those six call sites, keeps the tag as data passing through the transport rather than route knowledge living in it.
I asked for the bucket in round 1 and pointed at #request, so the location is as much my suggestion as your diff — the value is right, it's only ownership I'd reconsider. Non-blocking; leaving it here is defensible if you'd rather not reintroduce a per-request option this soon after #248 removed one.
| /** | ||
| * Listener invoked whenever a response reports per-signer rate-limit state. | ||
| */ | ||
| export type RateLimitUpdateListener = (update: RateLimitUpdate) => void; |
There was a problem hiding this comment.
[nit] The "rejections from returned promises are ignored" contract is documented on PublicClientOptions.onRateLimitUpdate (clients.ts:900-902) but not here, and this is the symbol a consumer hovers while writing the listener. AGENTS.md asks types to describe their own contract; one line would help, since a => void return reads as sync-only even though 4081d5c deliberately supports and tests promise-returning listeners.
3 participants
Implements DEV-480.
RateLimitErrornow carries thePoly-RateLimit-*state reported with a 429 rejection (rateLimitfield), alongside the existingretryAfter.onRateLimitUpdateclient option: a listener invoked whenever a response reports per-signer rate-limit state (remaining,reset,tier,warning), including warning-mode monitoring before live enforcement.https://linear.app/polymarket/issue/DEV-480
Note
Low Risk
Additive API and response handling with isolated listener error swallowing; no changes to auth or request signing.
Overview
Surfaces
Poly-RateLimit-*response headers in@polymarket/clientso integrators can react before hard 429s.RateLimitErrornow includes an optionalrateLimitobject (alongsideretryAfter) when a 429 response carries those headers. A newrate-limitmodule parsesRemaining,Reset,Tier, andWarning, with strict integer validation and optionalorder/cancelbucket tagging for known CLOB order and cancel paths.ServiceClientinvokes an optionalonRateLimitUpdatelistener on any response that reports rate-limit state (success or failure); listener errors cannot affect request handling.createPublicClient/createSecureClientacceptonRateLimitUpdateand pass it through to CLOBServiceClientinstances (including secure CLOB), preserving it afterendAuthentication.Reviewed by Cursor Bugbot for commit 4081d5c. Bugbot is set up for automated code reviews on this repo. Configure here.