Repository navigation
feat(models)!: tighten _fp/_dollars and strike/fee fields to Decimal (#258, #259) - #284
Merged
Merged
Conversation
…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
) 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
|
Code Review: PR 284 - Tighten _fp/_dollars and strike/fee fields to Decimal. Overview: This PR fixes a real precision hazard - Pydantic v2 default Decimal validator routes JSON numbers through float before coercion. The fix applies BeforeValidator(_coerce_decimal) consistently. Motivation is solid. Full review text will follow in replies. |
Code Review Summary\n\nFull structured review below.\n\n### Overview\n\nThis PR fixes a real precision hazard: Pydantic v2 default Decimal validator routes JSON numbers through float before coercion, causing binary float drift (0.65 becomes 0.6500000000000000222...). The fix applies BeforeValidator(_coerce_decimal) via named type aliases to every affected field. Motivation is solid; approach consistent with DollarDecimal/FixedPointCount pattern; test coverage thorough.\n\n### Substantive Issues\n\n1. Version string not updated - kalshi/init.py still reads 2.4.0. The PR description mentions v2.5.0 bump but the change is absent. If deferred to a separate commit/PR that should be noted explicitly.\n\n2. MultiplierDecimal serializes to str, but spec says format: double - _decimal_to_str outputs a JSON string ('0.65') when when_used='json'. For DollarDecimal this is intentional (wire format is dollar-string). But the spec marks Series.fee_multiplier, SeriesFeeChange.fee_multiplier, and Event.fee_multiplier_override as type: number, format: double. Anyone round-tripping these models via model_dump(mode='json') will get '0.65' string instead of 0.65 number, which can cause downstream parse failures. Options: (a) add a docstring note, or (b) define MultiplierDecimal without PlainSerializer so Decimal serializes to JSON number.\n\n### Minor Issues\n\n3. Docstring indentation bug - test_float_field_with_float_value_does_not_raise in tests/integration/test_assertions.py - second paragraph has ~8 extra leading spaces.\n\n4. Three structurally identical type aliases - MultiplierDecimal, DollarDecimal, and FixedPointCount are now three copies of the same Annotated[Decimal, BeforeValidator, PlainSerializer] block. A brief sibling-pointer comment on MultiplierDecimal would help readers understand this is intentional.\n\n5. floor_strike/cap_strike use DollarDecimal but lack _dollars aliases - CLAUDE.md describes DollarDecimal as for 'price/cost fields with _dollars aliases' but these strike fields have no such alias. Coercion is identical; noting for future audit clarity.\n\n6. Missing test: MarketLifecyclePayload.floor_strike with string input - TestMarketLifecycleFloorStrikeDecimal covers float and bool but not the string path like '95000.65'. WS fields are commonly serialized as strings by the server.\n\n### What is Working Well\n\n- Test coverage is thorough: happy path, float-via-str coercion, string input, bool rejection, arithmetic without wrapping, _SDK_NAME_PAYLOADS updated from str to Decimal instances.\n- FakeRateBearing docstring update is accurate - it is now a synthetic negative-case fixture.\n- Removing bare Decimal imports from files that switched to type aliases is clean.\n- Comment on contracts_limit is appropriately terse.\n\n### Summary\n\nTwo items worth addressing before merge: the missing version bump and the string-vs-number serialization concern for MultiplierDecimal on format: double fields (at minimum a docstring note). Everything else is polish. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #258
Closes #259
BREAKING: WS payload fields (OrderGroupPayload.contracts_limit, TickerPayload.dollar_volume/dollar_open_interest) and REST strike/fee fields (Market.floor_strike/cap_strike, Event.fee_multiplier_override, MarketLifecyclePayload.floor_strike, Series.fee_multiplier) now expose
Decimalvalues instead ofstr/float. v2.5.0 minor bump justification.Part of #273 (post-v2.4 audit), Wave 2.