Repository navigation
fix(models): reject bool on int request fields (#295) - #305
Conversation
…ransfers, counts) Adds StrictInt = Annotated[int, BeforeValidator(_reject_bool_int)] in kalshi/types.py and swaps int -> StrictInt on every int field of every Request model that does not already have a dedicated validator. bool is an int subclass so plain-int annotations silently coerce True->1 / False->0; #243 fixed this for CreateOrderRequest.buy_max_cost and #295 extends the same guard to the remaining money-routing / counting ints. Affected request models: - CreateOrderRequest: expiration_ts, subaccount, exchange_index - AmendOrderRequest: subaccount, exchange_index - DecreaseOrderRequest: reduce_by, reduce_to, subaccount, exchange_index - BatchCancelOrdersRequestOrder: subaccount, exchange_index - CreateOrderGroupRequest: contracts_limit, subaccount, exchange_index - UpdateOrderGroupLimitRequest: contracts_limit - CreateRFQRequest: contracts, subaccount - ApplySubaccountTransferRequest: from_subaccount, to_subaccount, amount_cents Response models (Order, Fill, Position, ...) keep plain int — they parse server-emitted ints which are never Python bools. buy_max_cost keeps its existing dedicated validator (#243) which also rejects Decimal/float beyond bool. Adds parametrized test_strict_int_rejects_bool_on_request_models in tests/test_models.py covering every (Model, field) pair on True/False. Closes #295.
Code Review — PR #305:
|
| Model | Fields |
|---|---|
CreateOrderV2Request (line 478–484) |
expiration_time, subaccount, exchange_index |
DecreaseOrderV2Request (line 523) |
exchange_index |
AmendOrderV2Request (line 563) |
exchange_index |
BatchCancelOrdersV2RequestOrder (lines 625–626) |
subaccount, exchange_index |
UpdateSubaccountNettingRequest |
subaccount_number |
CreateQuoteRequest (line 202) |
subaccount |
The PR description marks this as "Wave W1" which is fine — but if there isn't already a follow-up issue for Wave W2 covering V2 endpoints, one should be opened before this merges so the gap is tracked rather than forgotten.
3. StrictInt not exported from kalshi/__init__.py
NullableList is in __all__; StrictInt is not. Anyone extending the SDK or building on top of it who wants to reference the type is left digging into kalshi.types. Should be added to the exports alongside NullableList:
# kalshi/__init__.py
from kalshi.types import NullableList, StrictInt
...
__all__ = [
...
"NullableList",
"StrictInt",
...
]Minor observations
-
importlibin test body:importlib.import_moduleinside each test invocation works, but since these modules are already imported in the same test file, a top-level import is cleaner and consistent with the rest oftest_models.py. No functional issue. -
buy_max_costleft asfield_validator: Intentional and correct — that validator also guards againstDecimal/float, whichStrictIntdoesn't cover. Leaving it separate is the right call; just noting the divergence for anyone reading later.
Positive
- Clean, minimal type alias — no new abstractions, no overengineering.
- Correct
BeforeValidatorplacement ensures rejection happens before Pydantic's coercion. - 20-row parametrized test suite hits every affected model/field combination, including the highest-risk
ApplySubaccountTransferRequestsurface. - Request vs. response separation is precise and well-reasoned.
Summary: Two changes needed before merge — trim the docstrings to one-liners (convention) and export StrictInt. The V2 gap should be tracked as a follow-up issue if not already filed.
…t models Addresses bot review on #305: - Trim _reject_bool_int and StrictInt docstrings to 1 line each (terse style). - Export StrictInt from kalshi/__init__.py (import + __all__). - Apply StrictInt to V2 request-model int fields: - CreateOrderV2Request: expiration_time, subaccount, exchange_index - DecreaseOrderV2Request: exchange_index - AmendOrderV2Request: exchange_index - BatchCancelOrdersV2RequestOrder: subaccount, exchange_index - UpdateSubaccountNettingRequest: subaccount_number - CreateQuoteRequest: subaccount - Add 9 parametrize rows to TestStrictIntRejectsBoolOnRequestModels covering each new field.
|
Round-2 follow-up pushed in c99efda addressing bot review:
Verified: |
Code Review — Round 2 (post follow-up)All issues from round 1 have been addressed. Here's the second-pass assessment. Round-1 Fixes — Verified ✓
Remaining Minor Observations1. The test resolves model classes at call time via # current (inside test body)
module_name, class_name = model_path.split(":")
model_cls = getattr(importlib.import_module(module_name), class_name)
# consistent with rest of file would be a top-level import + a mapping dict2. PR description doesn't mention V2 models in "Affected request models" The round-2 commit extended coverage to V2 endpoints, but the PR description's "Affected request models" section wasn't updated to reflect them. Not a code issue, but the mismatch will make the CHANGELOG entry harder to write accurately. Worth a description edit before merge. 3.
No New Issues
Verdict: Ready to merge with the two minor observations above — fix the PR description and optionally clean up the |
|
Description updated to list the V2 request models added in c99efda. Skipping the importlib-in-test-body cleanup — the per-call import is intentional so the test resolves model classes by FQN string in the parametrize table (lets us add new rows without growing the top-level import list). Re-review welcome. |
Closes #295.
What
Introduces
kalshi.types.StrictInt = Annotated[int, BeforeValidator(_reject_bool_int)]and swapsint→StrictInton every integer field of every Request model that lacks a dedicated validator.boolis anintsubclass, so a plainintannotation silently coercesTrue→1 /False→0. #243 closed this forbuy_max_cost; #295 extends the same guard across every money-routing / counting integer.Affected request models
CreateOrderRequest:expiration_ts,subaccount,exchange_indexAmendOrderRequest:subaccount,exchange_indexDecreaseOrderRequest:reduce_by,reduce_to,subaccount,exchange_indexBatchCancelOrdersRequestOrder:subaccount,exchange_indexCreateOrderGroupRequest:contracts_limit,subaccount,exchange_indexUpdateOrderGroupLimitRequest:contracts_limitCreateRFQRequest:contracts,subaccountApplySubaccountTransferRequest:from_subaccount,to_subaccount,amount_centsCreateOrderV2Request:expiration_time,subaccount,exchange_indexDecreaseOrderV2Request:exchange_indexAmendOrderV2Request:exchange_indexBatchCancelOrdersV2RequestOrder:subaccount,exchange_indexUpdateSubaccountNettingRequest:subaccount_numberCreateQuoteRequest:subaccountResponse models (
Order,Fill,Position, …) keep plainint— they parse server-emitted ints that are never Python bools.Tests
New
TestStrictIntRejectsBoolOnRequestModelsparametrizes 20(Model, field)rows against bothTrueandFalse, assertingValidationErrorwith a"bool"substring match.Behavioral fence
Callers that today pass
True/Falsefor an integer request field were already doing something wrong; they now get a loudValidationErrorinstead of silent routing. Worth a "Breaking" entry in the v2.6.0 release notes.Part of the post-v2.5 audit closure (Wave W1).