Skip to content

fix(core): centralize time defaults#950

Open
realfishsam wants to merge 3 commits into
mainfrom
fix/easy-maintenance-constants
Open

fix(core): centralize time defaults#950
realfishsam wants to merge 3 commits into
mainfrom
fix/easy-maintenance-constants

Conversation

@realfishsam

Copy link
Copy Markdown
Contributor

Summary

  • Add shared core time utilities for timestamp unit detection and WebSocket reconnect defaults
  • Standardize timestamp seconds/milliseconds heuristics across core and SDK server managers
  • Standardize exchange WebSocket reconnect fallbacks on a shared 5s default and preserve explicit 0 values with nullish coalescing

Fixes #206
Fixes #225

Test Plan

  • git diff --check
  • python3 -m py_compile sdks/python/pmxt/server_manager.py
  • npm run build --workspace=pmxt-core (attempted; tsc was killed by the local environment with exit 137)
  • npx tsc --noEmit --pretty false --incremental false --skipLibCheck (attempted; killed by the local environment with exit 137)

@realfishsam

Copy link
Copy Markdown
Contributor Author

PR Review: FAIL

What This Does

Centralizes reconnect/timestamp defaults across exchange WebSockets, normalizers, and SDK server-manager status conversion. This should be an internal cleanup with no intended SDK response-shape change.

Blast Radius

Core WebSocket clients for Gemini Titan, Kalshi, Limitless, Opinion, Probable; Polymarket/Probable normalizers; Python and TypeScript server-manager status helpers.

Consumer Verification

Before (base branch):
Base branch has duplicated literals such as 5000ms reconnect delays and local timestamp seconds/ms conversion. I could not run a consumer API comparison because the PR branch does not build.

After (PR branch):
PR branch fails before the sidecar can start: npm run build --workspace=pmxt-core reports src/exchanges/probable/websocket.ts(2,33): error TS2440: Import declaration conflicts with local declaration of 'QueuedPromise'.

Test Results

  • Build: FAIL (npm run build --workspace=pmxt-core TypeScript error)
  • Unit tests: NOT RUN (build failed)
  • Server starts: FAIL (server not started because build failed)
  • E2E smoke: FAIL (blocked by build failure)

Findings

  1. core/src/exchanges/probable/websocket.ts:2 imports QueuedPromise from ../../types while the same file still declares local interface QueuedPromise<T> at line 7. TypeScript rejects the duplicate name, so the core package cannot build and the sidecar cannot ship.

PMXT Pipeline Check

  • Field propagation (3-layer): N/A
  • OpenAPI sync: N/A
  • Financial precision: N/A
  • Type safety: ISSUE — duplicate QueuedPromise declaration prevents compilation
  • Auth safety: N/A

Semver Impact

patch -- intended internal cleanup, but currently unmergeable because it breaks the build.

Risk

No runtime behavior could be verified because the TypeScript build fails.

@realfishsam

Copy link
Copy Markdown
Contributor Author

Pushed follow-up commits to repair the deterministic build blocker: removed the duplicate local QueuedPromise declaration in core/src/exchanges/probable/websocket.ts and refreshed generated API-doc metadata. Local validation passed: git diff --check, python3 -m py_compile sdks/python/pmxt/server_manager.py, npm run build --workspace=pmxt-core, and npm test --workspace=pmxt-core -- --runInBand --passWithNoTests (24 passed, 1 skipped). After other PRs landed, this PR is now conflicting again and the remaining CodeQL actions failure is an infra/auth error (Requires authentication), not a code finding.

@realfishsam

Copy link
Copy Markdown
Contributor Author

PR Review: PASS (NOT VERIFIED)

What This Does

Centralizes timestamp conversion and reconnect default constants, then applies them across several exchange websocket/normalizer/fetcher paths and SDK server-manager uptime parsing. This reduces duplicated epoch-second/ms heuristics and reconnect magic numbers.

Blast Radius

Core websocket clients and timestamp normalizers for Gemini Titan, Kalshi, Limitless, Opinion, Polymarket, and Probable; Python and TypeScript server manager uptime parsing; generated API doc metadata timestamp only.

Consumer Verification

Before (base branch):
Different files used local timestamp thresholds (1e12, 10000000000, > 1e12) and reconnect defaults (1000 or 5000) directly.

After (PR branch):
Core paths use timestampToMs() / DEFAULT_RECONNECT_INTERVAL_MS from core/src/utils/time.ts, and SDK server managers use the same 10,000,000,000 threshold for lock-file timestamps. This is not directly observable through a simple sidecar HTTP call without mocked venue websocket/timestamp payloads.

Test Results

  • Build: PASS (npm run build --workspace=pmxt-core)
  • Core unit tests: PASS (24 suites / 644 tests passed; 1 suite / 3 tests skipped)
  • Full npm test: FAIL in this checkout during Python SDK collection because pmxt_internal / eth_account are not available.
  • Server starts: Not separately run for this PR
  • E2E smoke: Not run against live websocket feeds

Findings

No blocking findings.

PMXT Pipeline Check

  • Field propagation (3-layer): N/A
  • OpenAPI sync: OK/N/A (only generated metadata timestamp changed)
  • Financial precision: N/A
  • Type safety: OK
  • Auth safety: N/A

Semver Impact

patch -- internal utility/refactor with unchanged public response shape.

Risk

The Limitless websocket fallback reconnect delay changes from 1000ms to the shared 5000ms default; that seems intentional for consistency but was not validated against live feed recovery behavior.

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.

Magic numbers: WebSocket reconnect intervals Magic numbers: Timestamp conversion thresholds

1 participant