Skip to content

feat(perps): expose server time - #292

Open
kartojal wants to merge 2 commits into
mainfrom
feature/dev-466-plan-integrate-server-time-from-v1infotime-into-perps
Open

feat(perps): expose server time#292
kartojal wants to merge 2 commits into
mainfrom
feature/dev-466-plan-integrate-server-time-from-v1infotime-into-perps

Conversation

@kartojal

@kartojal kartojal commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

DEV-466: Exposes the Perps server clock.

Tests: lint, typecheck, build, unit PASS; public live PASS.


Note

Low Risk
Additive, read-only public API following existing Perps info patterns; no auth, signing, or request-timestamp behavior changes.

Overview
Adds a public Perps clock so callers can read the exchange server time in epoch milliseconds via GET /v1/info/time.

Bindings introduce GetServerTimeResponseSchema (integer ms only). Client adds getServerTime on the low-level actions and client.getServerTime() on PublicPerpsActions, returning branded EpochMilliseconds with the usual transport/rate-limit error guard. Docs stress this is a read-only probe and does not change how the SDK timestamps or signs Perps requests today.

Coverage includes schema tests, type-level export checks, and a live integration assertion that server time is within ±60s of local wall clock. Minor version bumps are recorded for @polymarket/bindings and @polymarket/client.

Reviewed by Cursor Bugbot for commit 1a2fe01. Bugbot is set up for automated code reviews on this repo. Configure here.

@kartojal
kartojal marked this pull request as ready for review August 14, 2026 10:44

@brunson-bot brunson-bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the whole diff plus surrounding context. Behavior is right and the change is small and additive — the endpoint call, error union, @experimental tagging on all five new surfaces, and the minor/minor changeset all check out, and EpochMillisecondsSchema's .int() runs before toEpochMilliseconds, so the non-integer case fails as a Zod issue rather than escaping safeParse as a raw TypeError.

Three things, none blocking. The naming one is the only one I'd want settled before merge, since it's a public surface.

  • [issue] getServerTime is the only get* in the repo and the only Perps method without a Perps qualifier
  • [issue] the happy-path unit test replaces a live response for behavior the live integration test already covers
  • [issue] a read-only clock with no way to reach the SDK's own signing timestamps — what workflow does it unblock?

Comment thread packages/client/src/actions/perps.ts
Comment thread packages/client/src/actions/perps.test.ts Outdated
Comment thread packages/client/src/decorators/perps.ts
@kartojal

Copy link
Copy Markdown
Contributor Author

Naming and scope are acceptance decisions recorded on DEV-466: the ticket explicitly requires getServerTime, and the paired DEV-466/DEV-467 clock decision deliberately ships only the additive server-clock read while deferring signed-request clock policy. I removed the redundant mocked happy-path client test; binding tests retain response validation and the public integration test proves the real endpoint.

@brunson-bot

Copy link
Copy Markdown
Contributor

Round 2 (1a2fe01) — test finding closed.

Both MSW cases are gone; the happy path stays with the live getServerTime() assertion at tests/integration/perps.test.ts:36, and the as unknown as BaseClient fixture is back to its three pre-existing pagination callers. No orphaned imports — http/HttpResponse/createClient are all still used by the remaining tests.

Worth noting the malformed-response case didn't need to go with it: GetServerTimeResponseSchema.parse({ time: …0.5 }) at packages/bindings/src/perps/market.test.ts:21 covers the schema rejection, but nothing now covers validateWith mapping that into UnexpectedResponseError on this action. That's low signal given validateWith is shared across every Perps action, so I wouldn't add it back — just flagging that the deletion went one case further than the finding asked for.

Still open from round 1, both non-blocking:

  • actions/perps.ts:828getServerTime is still the only get* in actions/ (47 fetch*, 31 list*) and the only Perps method without a Perps qualifier, so client.getServerTime() doesn't say whose clock. fetchPerpsServerTime would match fetchPerpsFees 40 lines up.
  • decorators/perps.ts:269 — no way to feed the returned time back into signing (session.ts:984, perps.ts:1265/:1369/:1422 all stamp local Date.now()). Fine if that's a follow-up, just want it stated.

@brunson-bot

Copy link
Copy Markdown
Contributor

Verified both on DEV-466 — closing my two open findings, nothing further from me.

  • Scope: the shared clock decision comment (2026-08-14, paired with DEV-467) explicitly lands only the additive read and defers offset/TTL/single-flight/offline-signing to a policy PR, with the TS timestamp inventory already recorded. That answers the signing-lever question directly.
  • Naming: the ticket scope line does say getServerTime. Your call, and I'm not relitigating it — one note for whoever picks up the follow-up: AGENTS.md:74 says not to preserve ticket wording when a better SDK shape exists, and the ticket says "on the perps client" while perpsActions() is spread flat into allActions(), so the shipped surface is client.getServerTime() next to CLOB/Gamma/Data/relayer. If the DEV-467 policy PR adds anything clock-shaped, that's the moment to decide whether the pair reads as Perps-scoped.

CI: everything green except Verify / Run still pending; review red is the webhook that triggered me, not PR content.

*
* @experimental This API may change in a breaking way in any release, including patch releases.
*/
export async function getServerTime(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What developer workflow is this standalone read intended to solve? DEV-466 starts by asking where server time is needed and whether the SDK should abstract clock synchronization, but this PR leaves signed requests on Date.now() and gives callers no way to feed the returned value into signing. If server time is needed for correctness, I would expect the SDK to own the offset and synchronization rather than require callers to fetch it. If there is an independent use case for exposing the raw read, can we document it? As a smaller naming point, getServerTime is generic on the flat client and does not follow the existing fetchPerps* convention.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trial period: automated Brunson reviews on this repo are being evaluated. Suggestions are a work in progress and not necessarily accurate or vetted by the team yet — please verify before acting on them.

Checked both claims against head 1a2fe01 — they hold, and the second one has a concrete victim in-tree.

  • No seam into signing. Every signed Perps op takes ts from the local clock with no override: createPerpsCredentials (packages/client/src/actions/perps.ts:1422-1423), revokePerpsCredentials (:1265), withdrawFromPerps (:1369), and PerpsSession.#createSignedCommand (packages/client/src/websockets/perps/session.ts:983-984). PerpsOpSignatureRequest is module-private, so there is no public path for the returned value to reach a signature.
  • Skew already bites in one place. packages/client/src/actions/perps.ts:1496 compares the server-issued proxyKey.expiresAt against local Date.now(), and :1422 derives the requested expiry from Date.now() + expiresIn. That is a real cross-clock comparison today, and getServerTime() as shipped does not fix it — nothing applies the offset. If the answer to DEV-466 is "the SDK owns synchronization", that call site is the one to fix first.
  • Naming. getServerTime is the only get*-prefixed method across every decorator in packages/client/src/decorators/; everything else is fetch*/list*, and every other public Perps read carries the domain word (fetchPerpsBook, fetchPerpsFees, …). AGENTS.md defines only list* and fetch*. fetchPerpsServerTime conforms on both axes.

My read: the raw endpoint read is a reasonable primitive, but as merged it is a probe nobody in the SDK consumes, and the TSDoc's "does not change how the SDK timestamps or signs" is documenting the gap rather than closing it. Either land the offset alongside it or state the standalone use case in the TSDoc.

Slack discussion

@osr21

osr21 commented Aug 31, 2026

Copy link
Copy Markdown

For the standalone clock probe, a small usage note could prevent consumers from estimating skew incorrectly. The useful offset is based on the request midpoint, not the response time:

t0 = Date.now(); server = await client.getServerTime(); t1 = Date.now();

offsetMs = server - Math.floor((t0 + t1) / 2)
uncertaintyMs = Math.ceil((t1 - t0) / 2)

Callers should discard/highlight samples with large RTT, keep the lowest-RTT recent sample, and report both offset and uncertainty. Computing server - t1 systematically folds network latency into the apparent clock skew.

I agree with the existing discussion that signing should eventually consume an SDK-owned offset rather than exposing timestamp overrides on every command. Until that follow-up lands, documenting the midpoint pattern gives this read-only primitive a concrete and correct observability use case: warn that the local clock is outside tolerance before starting a delegated Perps session, without implying that the current SDK corrects signatures.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants