fix(types): preserve Decimal in DataFrames; positional serialization; reject bool#225
Conversation
… reject bool
- DollarDecimal/FixedPointCount serializer now when_used='json' so model_dump(mode='python') returns live Decimal (fixes Page.to_dataframe / to_polars silent str corruption).
- _decimal_to_str uses f'{v:f}' for positional format; eliminates scientific notation on the wire.
- _coerce_decimal + to_decimal reject bool explicitly (was raising decimal.InvalidOperation via str(True)).
Closes #190, Closes #191, Closes #216
Refs #224
Code Review — PR #225: fix(types): preserve Decimal in DataFrames; positional serialization; reject boolOverviewThree tightly scoped, independent fixes to the type layer. All three are correct and well-reasoned. The 🔴 Blocking: CI Lint FailuresTwo ruff errors are blocking CI. Both are in the new test code. 1.
# current (wrong order):
from __future__ import annotations
import pytest
from pydantic import BaseModel
from decimal import Decimal # ← stdlib after third-party
from kalshi.types import ...
# fix:
from __future__ import annotations
from decimal import Decimal
import pytest
from pydantic import BaseModel
from kalshi.types import ...2. In # fix:
pytest.importorskip("polars")🟡 Minor Issues3. Mid-file import in # line ~259 — after the existing test class body:
from kalshi.types import DollarDecimal # noqa: E402The 4.
5. Bool guard is duplicated across Both functions now contain the identical ✅ What's Well Done
SummaryFix the two lint errors and this is ready to merge. The core logic changes are correct and the tests are solid. |
Code Review — PR #225 (updated review)Three independent, well-scoped fixes to the type layer. All three core logic changes are correct. This review supersedes the earlier automated comment which contained two false positives. Core Logic — All Correct
Bool rejection before the Corrections to Prior ReviewThe previous automated comment flagged two issues that do not exist in the actual PR branch:
Actual IssuesE302 — missing blank lines in No blank lines separate Fix: add two blank lines between those two classes. Mid-file import in
SummaryFix the E302 blank-line issue (and optionally clean up the mid-file import) and this is ready to merge. The core logic is correct and the tests are solid. |
…mal contract Two pre-existing tests asserted that .model_dump() returned wire strings for Decimal fields. With the new when_used='json' serializer (which is the actual fix for #190), model_dump(mode='python') correctly returns live Decimal objects. Tests now exercise both modes.
Code Review — PR #225OverviewThree independent fixes to the type layer: Correctness
This is the right fix. With Confirmed by audit: every resource method in the repo already calls Positional
Bool rejection ✅ Guard is correctly placed before Test Coverage13 new tests, good breadth. Two symmetry gaps worth closing before merge:
Style / CLAUDE.mdComment references issue numbers — minor violation of "Don't reference the current task, fix, or callers". The why (mode mismatch causes string-concat) is worth keeping; stripping the issue number and tightening to one line would align with convention.
Summary
All three fixes are correct and safe to merge. The two coverage gaps (missing |
`buy_max_cost` is integer cents, but bool is an `int` subclass. `buy_max_cost=True` was slipping through as the integer 1 (= a 1 cent cap) — the exact failure mode #225 closed for `DollarDecimal` and `FixedPointCount`. A caller who accidentally passed a flag (e.g. `risk_check_passed`, `dry_run`) where cents were expected ended up placing a real order capped at $0.01 with no error. Prepend an `isinstance(v, bool)` rejection before the Decimal/float branches, mirroring the wording/style of the existing rejections. Audited the file: `buy_max_cost` is the only field with a Decimal/float guard, so no other validators need the same patch. Closes #243
… in buy_max_cost (#281) * fix(orders): require count + action on create() kwarg path Pre-#242 the kwarg-form `client.orders.create(ticker=..., side='yes')` silently defaulted missing `count` to 1 and missing `action` to 'buy'. For a real-money SDK that converted a missing-arg bug into a real 1-contract BUY fill — money risk. Drop the silent defaults on both sync and async kwarg paths and raise `TypeError` before any HTTP request when either is None. Mirror the spec at the model level by removing the `Decimal('1')` default on `CreateOrderRequest.count` (spec lists `count` as required); the `action` field was already required since #172. The `request=CreateOrderRequest(...)` overload path is unaffected: the model now declares both required so a fully-populated request still dispatches normally. BREAKING CHANGE: callers relying on `orders.create(ticker, side)` defaulting to 1-contract buy must now pass `count=` and `action=` explicitly (or build a `CreateOrderRequest`). This is technically breaking but money-risk-driven. Closes #242 * fix(orders): reject bool in CreateOrderRequest.buy_max_cost validator `buy_max_cost` is integer cents, but bool is an `int` subclass. `buy_max_cost=True` was slipping through as the integer 1 (= a 1 cent cap) — the exact failure mode #225 closed for `DollarDecimal` and `FixedPointCount`. A caller who accidentally passed a flag (e.g. `risk_check_passed`, `dry_run`) where cents were expected ended up placing a real order capped at $0.01 with no error. Prepend an `isinstance(v, bool)` rejection before the Decimal/float branches, mirroring the wording/style of the existing rejections. Audited the file: `buy_max_cost` is the only field with a Decimal/float guard, so no other validators need the same patch. Closes #243 --------- Co-authored-by: worker <worker@local>
Replace the per-row model_dump(mode='python') walk in Page.to_dataframe and Page.to_polars with a column-oriented dict[field, list[value]] built once via getattr. For a 1000-row trades_history page this avoids 1000 nested-dict allocations plus per-field Python-level conversion, and lets pandas/polars infer dtypes from homogeneous columns instead of re-inferring from records. Nested BaseModel cells are still dumped per-column (mode='python') so that polars can infer Struct from the resulting dicts — a plain BaseModel column raises 'Decimal without precision/scale set is not a valid Polars datatype'. The #225 / #190 Decimal contract is preserved (DollarDecimal lands as Decimal, list[Decimal] stays list[Decimal], nested Decimal survives Struct round-trip). Empty pages short-circuit to pd.DataFrame() / pl.DataFrame() to avoid indexing items[0] on an empty list. Tests: add 100-row column-oriented regression coverage for both backends + empty-page sentinels in tests/test_page_dataframe.py. Existing #101 nested-shape and #190 DollarDecimal pins still pass unchanged. Closes #264 Co-authored-by: worker <worker@local>
…258, #259) (#284) * fix(ws)!: retype OrderGroup.contracts_limit and Ticker dollar fields to Decimal (#258) BREAKING CHANGE: Three WS payload fields previously typed as str now parse to Decimal per the #225/#230 SDK invariant that every _fp field is FixedPointCount and every _dollars field is DollarDecimal. - OrderGroupPayload.contracts_limit: str | None → FixedPointCount | None - TickerPayload.dollar_volume: str → DollarDecimal - TickerPayload.dollar_open_interest: str → DollarDecimal Callers reading these as Python strings (e.g. concatenation, string compares, JSON dumps without a Decimal serializer) will need to adjust. The motivation is precisely the dual hazard the previous typing carried: manual Decimal(x) wrapping was required for arithmetic, and an empty server payload would trip Decimal('') → InvalidOperation at the call site rather than at parse. With these annotations, arithmetic works directly (msg.dollar_volume + msg.dollar_open_interest) and bool inputs are rejected per the #225 invariant. Wire keys are unchanged: contracts_limit_fp / contracts_limit for the order-group alias choices; dollar_volume / dollar_open_interest for ticker (these have no _fp/_dollars suffix on the wire). Closes #258 * fix!: route strike + fee-multiplier fields through _coerce_decimal (#259) BREAKING CHANGE: Five money-adjacent fields previously typed as bare ``Decimal`` or ``float`` now route through the ``_coerce_decimal`` invariant established in #225. Bare ``Decimal`` accepts a JSON number ``1.23`` by routing through ``float`` first \u2014 the precision-loss path the SDK paid an audit to eliminate. ``float`` is strictly worse: the value commits to binary float on parse (``0.65 -> 0.6500000000000000222``) and that drift propagates into payout math. - Market.floor_strike / cap_strike: Decimal | None \u2192 DollarDecimal | None - Event.fee_multiplier_override: Decimal | None \u2192 MultiplierDecimal | None - MarketLifecyclePayload.floor_strike: Decimal | None \u2192 DollarDecimal | None - Series.fee_multiplier / SeriesFeeChange.fee_multiplier: float \u2192 MultiplierDecimal Adds a new ``MultiplierDecimal`` alias in ``kalshi.types`` (internal \u2014 not re-exported via ``kalshi.__init__``). It applies the same ``_coerce_decimal`` + string-serializer treatment as ``DollarDecimal`` / ``FixedPointCount``, kept distinct to document that these are rate multipliers, not dollar quantities. User-visible consequences: callers reading ``series.fee_multiplier`` as a Python ``float`` will now see a ``Decimal`` (use it directly for arithmetic; cast with ``float()`` only at display boundaries). Bool inputs are rejected per the #225 invariant. The stale doc-comment in ``tests/integration/test_assertions.py`` that named ``Series.fee_multiplier`` as the canonical float-bearing example is updated to reflect the post-#259 typing. Closes #259
…geQueue, http2 test, benches (#289) * fix(auth): recheck _closed under lock in _get_sign_executor (#267) Pre-fix, sign_request_async and close() could race: thread A read _closed=False outside the lock, close() ran end-to-end (set _closed, shut down + nulled the executor), then thread A entered the locked double-checked init and constructed a brand-new ThreadPoolExecutor on a closed auth. The new pool was never shut down (close() already returned), defeating the documented terminality and leaking a 2-thread pool until interpreter shutdown. Move the _closed check inside the lock: keep a lock-free fast path for the already-constructed case, but on the slow path re-read _closed under the lock and raise without instantiating a new executor if close() has won the race. Regression test simulates the interleaving by wrapping the lock's __enter__ to call close() mid-acquire. * fix(transport): clamp negative Retry-After delta to 0.0 for date-form parity (#267) Pre-fix, Retry-After: -5 returned retry_after=None (falls back to computed exponential backoff) while the semantically equivalent ``Retry-After: <past HTTP-date>`` returned retry_after=0.0 (retry immediately). Tests that pinned one shape silently disagreed with the other, and clock-skewed proxies emitting past timestamps got inconsistent behavior depending on which form they used. Pick the more permissive policy and clamp negative delta-seconds to 0.0 \u2014 matching the existing HTTP-date branch. Non-finite (NaN/inf) still falls back to None since those would survive the transport caller's min() cap and either crash time.sleep or schedule a near-infinite delay. Regression test asserts both forms produce identical retry_after. * perf(ws,transport): drop MessageQueue._size; hoist asyncio import (#271) Two small per-message hot-path savings: - MessageQueue._size: an O(1) int counter incremented/decremented on every put / popleft purely so qsize() could exclude the single terminal sentinel. Two extra int ops per message on the WS recv hot path with no functional gain \u2014 derive qsize from len(self._buffer) minus a sentinel adjustment when the queue is closed. The deque's maxlen=maxsize+1 was already the real memory ceiling. - import asyncio inside AsyncTransport.request: a sys.modules lookup on every async request. The module is already imported at top level by KalshiAuth.sign_request_async (and many others); move the import to the file header. Updated the deque-maxlen regression to assert the cap via sustained overflow rather than simulated counter drift (the counter no longer exists). * test(transport,bench): http2 wiring test + iterator/dataframe benches (#271) #271 items 3 + 4 \u2014 testing-coverage polish. tests/test_http2.py: verify KalshiConfig(http2=True) propagates to the underlying httpx.Client / httpx.AsyncClient's connection pool, so a regression that silently drops the kwarg surfaces here instead of landing without any failing test. Guarded by skipif when h2 isn't installed in the env (httpx rejects http2=True without it). scripts/bench_orderbook_iterator.py: drives _OrderbookIterator.__anext__ end-to-end through a synthetic stream that mimics the recv loop (apply_delta_inplace + yield). Existing bench_orderbook_delta only measures the in-place mutation; this one captures the mgr.get + Orderbook materialization that real async for book in ws.orderbook() consumers pay. scripts/bench_page_to_dataframe.py: builds a 1000-item Page[Market] and times to_dataframe() / to_polars(). Catches regressions in the column-oriented build path (#264) and confirms the DollarDecimal contract (#225/#190) doesn't quietly fall back to per-row model_dump. Closes #267 Closes #271
…#370) The SDK's money-safety invariants depend on Pydantic v2 behaviors that stabilized after 2.0: - StrictInt boolean rejection (relied on by #295) was tightened in 2.4. - model_validator(mode='before') inheritance semantics changed through 2.3. - Decimal coercion edge cases (negative-zero, scientific input) used by _coerce_decimal (#270) were fixed across 2.4/2.5. Allowing 2.0-2.3 lets resolvers land a runtime with subtler validators than the test matrix exercises, quietly undermining #225 / #243 / #295. Bump the floor to pydantic>=2.4,<3 so the installed runtime always matches the invariants the changelog advertises. Verified: ruff, mypy, and tests/test_models.py + tests/test_types.py all pass under the regenerated lock (pydantic 2.13.4 resolved). Closes #346
1 participant
Wave 0-A of the 2026-05-21 SDK audit umbrella. Three independent type-layer fixes.
Changes
PlainSerializernow useswhen_used='json'somodel_dump(mode='python')returns a liveDecimalinstead ofstr. Fixes Page.to_dataframe / to_polars silent string-concatenation corruption._decimal_to_strusesf'{v:f}'for positional format — eliminates scientific notation ('1E+10') on the wire._coerce_decimal+to_decimalrejectboolexplicitly with a clear TypeError. Previously raiseddecimal.InvalidOperationviastr(True).Tests
13 new tests; pre-existing 16 tests in scope still pass; mypy --strict clean.
Closes #190, Closes #191, Closes #216
Refs #224