Skip to content

feat: order amendments, decrease, and queue positions (v0.5.0)#27

Merged
TexasCoding merged 13 commits into
mainfrom
feat/order-amendments
Apr 16, 2026
Merged

feat: order amendments, decrease, and queue positions (v0.5.0)#27
TexasCoding merged 13 commits into
mainfrom
feat/order-amendments

Conversation

@TexasCoding

Copy link
Copy Markdown
Owner

Summary

Adds 4 new order management endpoints to the SDK, bringing coverage from 24 to 28 of 77 Kalshi API endpoints.

New methods on OrdersResource + AsyncOrdersResource:

  • amend(order_id, *, ticker, side, action, yes_price, no_price, count, ...) — amend order price and/or quantity. Returns AmendOrderResponse with both pre and post-amendment state.
  • decrease(order_id, *, reduce_by, reduce_to, subaccount) — reduce order quantity by amount or to amount
  • queue_positions(*, market_tickers, event_ticker, subaccount) — bulk queue position lookup for all resting orders
  • queue_position(order_id) — single order queue position, returns Decimal

New models:

  • AmendOrderResponse(old_order: Order, order: Order) — preserves both pre/post amendment state
  • OrderQueuePosition(order_id, market_ticker, queue_position) — queue position data

Testing: 29 new tests (557 total). Sync/async happy paths, error paths (404, 400), serialization verification, and auth guards for all 4 methods.

Spec drift: Contract map entries added for both new models. Integration coverage harness updated.

Review fix: queue_position() now wraps KeyError in KalshiError instead of leaking raw exceptions.

Test Coverage

557 tests passing. mypy clean. ruff clean.

Tests: 528 → 557 (+29 new)

All new code paths covered: happy path, error path, serialization, auth guards, sync + async mirrors.

Pre-Landing Review

Pre-Landing Review: 3 issues (0 critical, 3 informational)

  • AUTO-FIXED: OrderQueuePosition contract map entry added
  • AUTO-FIXED: KeyError wrapped in KalshiError for queue_position()
  • SKIPPED: .get() fallback pattern (follows existing SDK convention)

Adversarial review ran with both Claude + Codex. PR Quality Score: 9/10.

TODOS

  • Completed: "P3: Integration test — order amendments and decrease" (also added queue positions as companion)

Test plan

  • All pytest tests pass (557 passed, 0 failures)
  • mypy --strict passes (47 source files, 0 errors)
  • ruff check passes (0 issues)

🤖 Generated with Claude Code

TexasCoding and others added 7 commits April 15, 2026 20:23
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ersResource

Add four new sync methods to OrdersResource:
- amend() - amend a resting order's price/count
- decrease() - reduce order quantity by amount or to target
- queue_positions() - list queue positions with optional filters
- queue_position() - get single order's queue position as Decimal

Includes 13 new tests covering happy paths, serialization verification,
error handling (404/400), empty responses, and filter parameter passing.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…osition()

Review fixes from /review adversarial pass:
- Add OrderQueuePosition to _contract_map.py for spec drift coverage
- Wrap bare KeyError in queue_position() with KalshiError for clean error hierarchy

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Apr 16, 2026

Copy link
Copy Markdown

Code Review: feat: order amendments, decrease, and queue positions (v0.5.0)

Overview

Solid addition of 4 new order management endpoints with full sync/async parity, good test coverage, and proper model design. The implementation follows existing SDK conventions well. A few issues worth addressing before landing.


Issues

Medium: market_tickers type hint is misleading (and possibly wrong)

def queue_positions(self, *, market_tickers: str | None = None, ...

The parameter is named market_tickers (plural) but typed as str | None. Users will naturally pass a list[str] and get a confusing runtime failure. Verify against the OpenAPI spec and either change the type to list[str] | None (with serialization joining them) or add a docstring clarifying the expected comma-separated format (e.g., "MKT-A,MKT-B").


Medium: decrease() silently sends an empty body

Calling orders.decrease("ord-123") (no args) POSTs {} to the API. Passing both reduce_by and reduce_to is also a silent bug. Add early guards:

if reduce_by is None and reduce_to is None:
    raise ValueError("decrease() requires either reduce_by or reduce_to")
if reduce_by is not None and reduce_to is not None:
    raise ValueError("decrease() accepts reduce_by or reduce_to, not both")

Add two regression tests for these cases.


Low: float in price type hints conflicts with SDK convention

yes_price: float | str | int | None — CLAUDE.md says "All prices use Decimal" and the SDK avoids float for IEEE 754 reasons. Prefer Decimal | str | int | None to match the public API contract.


Low: queue_position() key lookup inconsistency with its own model

OrderQueuePosition accepts both queue_position_fp and queue_position via AliasChoices, but queue_position() hard-codes only data["queue_position_fp"]. If the API returns queue_position (without suffix), queue_positions() handles it gracefully while queue_position() raises KalshiError. Mirror the alias logic with a .get() fallback.


Observations (non-blocking)

  • In-method imports in auth guard testsfrom kalshi.errors import AuthRequiredError inside each test body is inconsistent with module-level imports elsewhere in the test files.
  • Missing test: amend() with no_price — serialization test covers yes_price/count_fp but not no_price to no_price_dollars.
  • Missing test: queue_positions() with market_tickers filter — filter test covers event_ticker but not market_tickers.

What's done well

  • Full sync/async parity with identical logic — no drift between the two classes.
  • _require_auth() called first in every method, no accidental unauthenticated requests.
  • builtins.list[T] used correctly inside resource classes (per CLAUDE.md).
  • KeyError -> KalshiError with from None — clean exception chain suppression.
  • Contract map entries added for both new models — spec drift detection stays current.
  • AmendOrderResponse.extra = "allow" is correct for a response model (vs CreateOrderRequest's "forbid").
  • Serialization test (test_amend_serializes_dollars_and_count) verifying the wire format is a great pattern.

Summary: Two medium issues (market_tickers type and unguarded empty decrease() body) should be addressed before merge. The rest are polish. Overall quality is high.

…rt, queue_position fallback

- Add ValueError guards on decrease() for missing/conflicting reduce_by/reduce_to
- Accept list[str] | str for market_tickers on queue_positions() (auto-joins with comma)
- Add .get() fallback for queue_position_fp/queue_position key in queue_position()
- Fix async test that called decrease() with no args (now raises ValueError)
- Add 4 regression tests for the new guards (sync + async)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Apr 16, 2026

Copy link
Copy Markdown

Code Review: feat: order amendments, decrease, and queue positions (v0.5.0)

Overview

Solid addition of 4 new order management endpoints. The code follows existing SDK conventions, test coverage is thorough, and sync/async parity is maintained. A few issues worth addressing before landing.


Bug: Falsy short-circuit in queue_position()

File: kalshi/resources/orders.py (same in both sync and async variants)

The current code:

raw = data.get('queue_position_fp') or data.get('queue_position')

Using or silently falls through to the second key if the first returns a falsy value (e.g. integer 0). While Kalshi uses string-encoded FixedPoint in practice, this is fragile. Prefer an explicit is not None guard:

raw = data.get('queue_position_fp')
if raw is None:
    raw = data.get('queue_position')
if raw is None:
    raise KalshiError(...)
return to_decimal(raw)

Missing validation: amend() with no price or count

File: kalshi/resources/orders.py

amend() lets callers omit yes_price, no_price, and count simultaneously. The resulting body only contains ticker/side/action and will fail server-side with a vague error. A client-side guard mirrors the pattern already used in decrease():

if yes_price is None and no_price is None and count is None:
    raise ValueError('amend() requires at least one of yes_price, no_price, or count')

Missing test: market_tickers list → comma-joined query param

Files: tests/test_orders.py, tests/test_async_orders.py

No test passes market_tickers as a list and verifies the comma-join reaches the URL. Easy to break silently:

orders.queue_positions(market_tickers=['MKT-A', 'MKT-B'])
params = dict(route.calls[0].request.url.params)
assert params['market_tickers'] == 'MKT-A,MKT-B'

Missing test: queue_position() fallback key path

The 'queue_position' (non-_fp) fallback branch is never exercised. Add a test that mocks a response with only {'queue_position': '5'} and asserts the correct Decimal is returned.


Nit: module-level imports inside test bodies

import json is buried inside test_amend_serializes_dollars_and_count, and from kalshi.models.orders import OrderQueuePosition is imported inside an async test method. Both should move to the top of their respective files, consistent with the rest of the test suite.


Informational: integer vs FixedPoint asymmetry

amend() passes count through to_decimal() and sends it as count_fp. decrease() sends reduce_by/reduce_to as raw integers. This looks intentional (the field names lack the _fp suffix), but a brief comment or contract map note would help future maintainers confirm it is by design.


Summary

Severity Issue
Bug or falsy short-circuit in queue_position() — use is not None
Improvement Client-side guard: amend() with no price or count raises ValueError
Test gap market_tickers list comma-join is never asserted
Test gap queue_position() fallback key ('queue_position') is never exercised
Nit import json and model imports inside test function bodies

The core logic, model design, auth guards, and error mapping all look correct and follow project conventions. The or short-circuit in queue_position() is the one thing worth fixing before landing — everything else is low-risk.

- Add ValueError guard on amend() when no price or count provided
- Fix falsy short-circuit in queue_position() (or → is not None)
- Fix line-too-long lint errors in queue_positions() tickers_str
- Add test for market_tickers list comma-join
- Add test for queue_position() fallback key path
- Add test for amend() no-price-or-count guard
- Fix existing tests that called amend() without price args

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Apr 16, 2026

Copy link
Copy Markdown

Code Review: feat: order amendments, decrease, and queue positions (v0.5.0)

Overview

This PR adds 4 new order management endpoints to OrdersResource + AsyncOrdersResource: amend(), decrease(), queue_positions(), and queue_position(). It brings coverage from 24 to 28 of 77 API endpoints, adds 2 new models, and ships 29 new tests. The implementation is clean and well-tested — the issues below are minor.


Issues

1. Missing test: queue_position() never exercises the KalshiError branch

File: tests/test_orders.py and tests/test_async_orders.py

queue_position() has three code paths: queue_position_fp key present, queue_position fallback key present, and neither key present — which raises KalshiError. The third path is never tested.

# Missing in both test files:
@respx.mock
def test_queue_position_missing_key_raises(self, orders: OrdersResource) -> None:
    respx.get(
        "https://test.kalshi.com/trade-api/v2/portfolio/orders/ord-999/queue_position"
    ).mock(return_value=httpx.Response(200, json={"unexpected_field": "value"}))
    with pytest.raises(KalshiError, match="missing 'queue_position_fp'"):
        orders.queue_position("ord-999")

This is the only untested error path in the new code. Worth adding for both sync and async.


2. float in price type hints risks silent precision loss

File: kalshi/resources/orders.py:201-202 and the async mirror

yes_price: float | str | int | None = None,
no_price: float | str | int | None = None,

If a caller passes yes_price=0.65, Python's float representation is 0.6499999999999.... Whether to_decimal() handles this safely depends on its implementation — if it does Decimal(float_value) (not string-routed), the serialized value will be wrong.

CreateOrderRequest uses DollarDecimal which forces string/Decimal input at the model level. Consistent approach for amend() would be to drop float from the union:

yes_price: Decimal | str | int | None = None,
no_price: Decimal | str | int | None = None,

Or verify that to_decimal() routes floats through str(float) first, and document that.


3. str(to_decimal(count)) for integer count is unexpected

File: kalshi/resources/orders.py:221

if count is not None:
    body["count_fp"] = str(to_decimal(count))

to_decimal(20)Decimal(20)str(...)"20" — this works, but routing an integer through to_decimal() is surprising. count_fp is a contract quantity (integer contracts); str(count) is clearer and avoids any edge cases with how to_decimal handles integers. Minor, but worth noting.


Observations (no action required)

  • Sync/async duplication is expected — CLAUDE.md documents the dual-transport architecture; the body of all 4 methods is correctly duplicated with await keywords.
  • data.get("order", data) fallback in decrease() — follows the same pattern used elsewhere in the SDK. Acceptable.
  • model_config = {"extra": "allow"} on AmendOrderResponse — correct; the API can add fields without breaking deserialization.
  • isinstance(market_tickers, list) vs builtins.list — the builtins.list note in CLAUDE.md applies only to type annotations; using bare list in isinstance() is fine since method names don't shadow builtins in the local namespace.
  • Auth guard before ValueError in amend() — calling _require_auth() before the yes_price is None guard is the right order (fail fast on missing creds before doing any computation).

Test Coverage Summary

29 new tests covering: happy paths, error paths (404, 400), serialization verification, auth guards, and fallback key handling for queue_position. The only gap is the missing KalshiError branch in issue #1 above.


Verdict: The PR is solid. One untested branch, one type-hint inconsistency worth fixing before merge, and one minor style point. Happy to approve once issue #1 (the missing test) is addressed.

🤖 Generated with Claude Code

Covers the third code path in queue_position() where neither
queue_position_fp nor queue_position key is present in the response.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Apr 16, 2026

Copy link
Copy Markdown

Code Review — PR #27: Order amendments, decrease, and queue positions (v0.5.0)

Overview

Adds 4 new order management endpoints (amend, decrease, queue_positions, queue_position) to both OrdersResource and AsyncOrdersResource, with matching Pydantic models, contract map entries, and 29 new tests. The implementation follows existing SDK patterns closely. Overall this is clean work with solid coverage.


Issues

Minor: AsyncOrdersAmend test class has less coverage than its sync counterpart

The sync TestOrdersAmend has 5 tests; the async TestAsyncOrdersAmend only has 2. Missing equivalents:

  • test_amend_serializes_dollars_and_count — verifies yes_price_dollars / count_fp wire format
  • test_amend_validation_error — exercises the 400 → KalshiValidationError path
  • test_amend_requires_price_or_count — confirms ValueError when neither price nor count is given (can't be caught by the auth-guard test since _require_auth() runs first)

The PR description claims "sync/async happy paths, error paths, serialization verification" for all 4 methods, but amend's async side is missing the serialization and validation branches.

Minor: decrease() sends raw ints — verify field names against spec

body["reduce_by"] = reduce_by   # plain int, no _fp suffix
body["reduce_to"] = reduce_to   # plain int, no _fp suffix

Other count fields in the SDK use _fp suffixes (count_fp, remaining_count_fp). If the Kalshi spec expects reduce_by_fp / reduce_to_fp, these calls will silently send the wrong field names. Worth a quick cross-check against the OpenAPI spec (the amend() count field correctly uses count_fp, so the pattern is established).

Informational: float accepted for price parameters

yes_price: float | str | int | None = None

float is technically accepted here and converted via to_decimal(), but a float input like 0.1 + 0.2 can carry silent precision errors before to_decimal() sees it. Decimal | str | int would be stricter. This is consistent with the existing codebase pattern, so it's fine to leave as-is — just worth noting for callers.

Informational: No test for market_tickers as a single string

queue_positions(market_tickers="MKT-A") takes the else: tickers_str = market_tickers branch. There's a test for the list-join path but not for the single-string passthrough. Low risk, but a one-liner test would close the gap.


What's done well

  • Auth-first ordering: _require_auth() is the first call in all 4 methods, consistent with existing resources.
  • Input validation: The mutual-exclusion guard in decrease() and the "at least one" guard in amend() raise ValueError before any network call — good fail-fast behavior.
  • KalshiError on missing key: queue_position() raises a typed SDK error instead of a raw KeyError leaking through. The queue_position fallback key is tested.
  • Model config choices: AmendOrderResponse uses extra="allow" (correct for inbound), CreateOrderRequest stays extra="forbid" (correct for outbound).
  • builtins.list[T] in type annotations: Correctly follows the project convention to avoid shadowing by .list() methods.
  • Contract map entries: Both new models are registered for spec-drift detection.
  • CHANGELOG / version bump: Clean and complete.

Summary

The core implementation is correct and consistent with SDK conventions. The two items worth acting on before merge:

  1. Add the missing async amend tests (serialization verification + ValueError path).
  2. Verify reduce_by / reduce_to field names against the OpenAPI spec — if the spec uses _fp suffixes, the current code sends incorrect field names.

Everything else is informational. Happy to approve once those are addressed.

Brings async amend coverage to parity with sync:
- test_amend_serializes_dollars_and_count (wire format verification)
- test_amend_validation_error (400 → KalshiValidationError)
- test_amend_requires_price_or_count (ValueError guard)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Apr 16, 2026

Copy link
Copy Markdown

Code Review — PR #27: Order amendments, decrease, and queue positions (v0.5.0)

Overall: Solid, well-structured addition. The implementation follows existing SDK conventions closely and the test coverage is thorough. A few gaps in the async test suite and one serialization question to clarify before merge.


What this PR does

Adds 4 new order management methods (amend, decrease, queue_positions, queue_position) to both OrdersResource and AsyncOrdersResource, with two new models (AmendOrderResponse, OrderQueuePosition), 29 new tests, and contract map entries for drift detection.


Issues to fix before merge

1. Async test suite has gaps vs. sync

The sync tests in TestOrdersQueuePositions and TestOrdersDecrease cover cases that TestAsyncOrdersQueuePositions and TestAsyncOrdersDecrease do not. Missing async tests:

  • TestAsyncOrdersDecrease — no test_decrease_to (reduce_to=0 path) or test_decrease_not_found
  • TestAsyncOrdersQueuePositions — no test_queue_positions_with_list_tickers, test_queue_position_fallback_key, or test_queue_position_not_found

These gaps mean async regressions on those paths won't be caught. Each sync test that covers a distinct code path should have an async mirror.

2. count serialization via to_decimal() may not match API expectations

In amend():

body["count_fp"] = str(to_decimal(count))

str(to_decimal(20))"20" (fine), but str(to_decimal(20)) via Decimal could produce "20" or "20.00" depending on how to_decimal normalizes. Check the OpenAPI spec: if count_fp expects an integer string "20" rather than a decimal string, passing Decimal("20.00") would be wrong. The existing yes_price_dollars/no_price_dollars paths use the same pattern and are price fields, but count is a quantity — consider using str(count) directly if the spec expects a plain integer representation.


Minor issues

3. Inline imports in test methods

Several tests use mid-function imports, which is inconsistent with the rest of the test files:

# tests/test_orders.py — test_amend_serializes_dollars_and_count
import json  # ← inside the test body

# tests/test_async_orders.py — auth guard tests
from kalshi.errors import AuthRequiredError  # ← inside every guard test

import json and all error-class imports should be at module level. The error class imports are already at module level in some tests in the same files; this inconsistency looks like a copy-paste oversight.

4. queue_position() error message is slightly misleading

raise KalshiError(
    f"Unexpected response for queue_position: missing 'queue_position_fp' in {data!r}"
)

The code actually falls through if either queue_position_fp or queue_position is present, so the error only fires when both are absent. The message says "missing 'queue_position_fp'" but doesn't mention the queue_position fallback. Suggest:

raise KalshiError(
    f"Unexpected response for queue_position: missing 'queue_position_fp' and 'queue_position' in {data!r}"
)

5. ticker, side, action could use Literal types

The Kalshi API accepts only specific string values for side ("yes" / "no") and action ("buy" / "sell"). Typing these as Literal["yes", "no"] and Literal["buy", "sell"] would catch misuse at the type-checker level and improve IDE autocomplete, consistent with how Order model fields are typed elsewhere.


What looks good

  • Auth guards (_require_auth()) are called before any validation or network I/O — correct ordering.
  • amend() validation (requires at least one of price/count) and decrease() validation (requires exactly one of reduce_by/reduce_to) are both clean and well-tested.
  • builtins.list[T] used correctly inside resource classes (per CLAUDE.md).
  • AmendOrderResponse uses extra = "allow" for forward compatibility — appropriate.
  • OrderQueuePosition.queue_position uses AliasChoices("queue_position_fp", "queue_position") — consistent with SDK field aliasing convention.
  • Both sync/async implementations are true mirrors (no sync-wrapping-async).
  • Contract map entries added for both new models — good spec drift hygiene.
  • The decrease() response uses .get("order", data) fallback — consistent with existing SDK convention (noted in PR description).

Summary

Fix the 6 missing async tests and verify the count_fp serialization format against the spec. The rest are nits. Once addressed, this is ready to merge.

- Add async test_decrease_to and test_decrease_not_found
- Add async test_queue_positions_with_list_tickers
- Add async test_queue_position_fallback_key and test_queue_position_not_found
- Fix queue_position() error message to mention both fallback keys

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Apr 16, 2026

Copy link
Copy Markdown

Code Review: feat: order amendments, decrease, and queue positions (v0.5.0)

Overall: Well-structured, clean PR with solid test coverage. Follows existing SDK conventions consistently. A few minor items to address.


Overview

Adds four new order management endpoints (amend, decrease, queue_positions, queue_position) with corresponding models (AmendOrderResponse, OrderQueuePosition), full sync/async parity, and 29 new tests. Follows the spec-first hybrid pattern established by the rest of the SDK.


Issues

Moderate

Inline imports in test methods (tests/test_orders.py:802, tests/test_async_orders.py:580,632)

# Inside test body:
import json
from kalshi.errors import KalshiError
from kalshi.models.orders import OrderQueuePosition

These should be at module level. Mid-function imports signal the module's __all__ or top-level imports weren't updated when the PR was assembled. They work, but they obscure what the test file depends on and cause a minor overhead penalty on every test run.


Minor

amend() accepts float for price parameters

yes_price: float | str | int | None = None

float is included but Decimal is not. Callers who pass yes_price=0.57 get lucky because Python's str(0.57) happens to produce '0.57' before to_decimal, but Decimal(0.57) directly yields Decimal('0.569999...'). The type signature should either match CreateOrderRequest's approach (use DollarDecimal via Pydantic) or explicitly exclude float in favour of Decimal | str | int to prevent subtle precision footguns. Consider how to_decimal handles the float path — if it does Decimal(float_val) without stringifying first, a unit test for amend(yes_price=0.57) would silently serialize a wrong value.

Missing test: decrease() fallback path

decrease() has a defensive fallback:

order_data = data.get("order", data)

There's no test for the case where the API returns a bare Order object (no "order" key). If the API ever responds without the wrapper, this silently falls back. A one-line test covering this branch would prevent a regression if the fallback is ever removed.

Missing test: amend() with no_price

All amend serialization tests use yes_price. A single test verifying no_price is serialized as "no_price_dollars" in the body would complete the coverage of the conditional logic.


Informational

Sync/async duplication

The amend() and decrease() bodies are byte-for-byte identical between OrdersResource and AsyncOrdersResource, differing only in await. This is consistent with the existing SDK architecture (dual transport, not sync-wrapping-async) so it's not a defect, but worth flagging: any future bug in amend() body construction needs to be fixed in both places. A shared _build_amend_body() helper would eliminate that risk without changing the transport pattern.

queue_position() vs queue_positions() return type asymmetry

queue_positions() returns list[OrderQueuePosition] (full model), while queue_position() returns bare Decimal. The asymmetry is understandable because the single-order endpoint only returns a position number in its response body — the model's order_id and market_ticker would have to be re-echoed from the request. The current approach is pragmatic, but worth documenting in a docstring so callers know not to expect the full model from the single-order call.

Auth guard ordering

The auth guard tests for amend() call the method without any price/count argument:

unauth_orders.amend("ord-123", ticker="T", side="yes", action="buy")

This works because _require_auth() is called before the price/count validation. It's correct, but if the order is ever swapped, the test would fail with ValueError instead of AuthRequiredError, which would be confusing. A brief inline comment in the test (# _require_auth() fires before price/count validation) would save the next reader from having to verify this.


What's working well

  • ValueError guards (amend requires price/count, decrease requires exactly one of reduce_by/reduce_to) are clean and well-tested
  • KalshiError wrapping for missing queue_position_fp key (instead of raw KeyError) is the right call
  • AliasChoices("queue_position_fp", "queue_position") on OrderQueuePosition correctly handles spec drift for the field name
  • "extra": "allow" on inbound models (AmendOrderResponse, OrderQueuePosition) vs "extra": "forbid" on outbound (CreateOrderRequest) follows the established convention
  • builtins.list[T] used correctly to avoid shadowing
  • Contract map entries added — good spec drift hygiene

🤖 Generated with Claude Code

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Apr 16, 2026

Copy link
Copy Markdown

Code Review: feat: order amendments, decrease, and queue positions (v0.5.0)

Overview

This PR adds 4 new order management methods to OrdersResource and AsyncOrdersResource: amend(), decrease(), queue_positions(), and queue_position(). The implementation follows existing SDK patterns well and comes with thorough test coverage (29 new tests).


Issues

1. Potential API field name mismatch in decrease() body — worth verifying

amend() uses count_fp as the key for quantity, consistent with the Kalshi _fp field suffix convention. But decrease() sends reduce_by and reduce_to without the _fp suffix:

# amend() — uses _fp suffix ✓
body["count_fp"] = str(to_decimal(count))

# decrease() — no _fp suffix, may be wrong
body["reduce_by"] = reduce_by
body["reduce_to"] = reduce_to

If the Kalshi spec defines these as reduce_by_fp / reduce_to_fp, this will silently send the wrong field names and the API will likely reject or ignore them. Please verify against the OpenAPI spec (POST /portfolio/orders/{order_id}/decrease).

2. float accepted for price parameters in amend() introduces precision risk

The yes_price and no_price parameters accept float | str | int | None. IEEE-754 floats can't represent many cent values exactly — 0.57 in Python is actually 0.56999999.... The to_decimal() call converts it, but if it uses Decimal(float_value) internally it will propagate the float's imprecision.

The SDK convention for price fields is DollarDecimal (which uses string parsing). Using Decimal | str | int | None — or checking that to_decimal() coerces through str() for floats — would be safer and more consistent with the rest of the codebase. The test assert body["yes_price_dollars"] == "0.55" only passes because 0.55 happens to round-trip cleanly; 0.57 might not.


Minor

3. Inline import statements inside test methods

Several tests do from kalshi.errors import KalshiError and import json inside the test body. Per Python conventions these should be at module level (top of file). It's not harmful but inconsistent with the rest of the test files.

# test_orders.py line ~988 — inside test body
from kalshi.errors import KalshiError

# test_orders.py / test_async_orders.py — inside test body
import json

4. queue_positions() with a single-string market_tickers is not tested

The isinstance(market_tickers, list) branch is tested for the list case, but the pass-through string path (when market_tickers is already a comma-joined string) has no test. Worth adding a one-liner to lock in that behaviour.

5. amend() happy-path tests only exercise yes_price

There is no test for passing no_price alone (or count alone without any price). Given that no_price takes a different body key (no_price_dollars), a serialization test for no_price would close the same gap that test_amend_serializes_dollars_and_count closes for yes_price.


What's working well

  • Correct auth guard ordering: _require_auth() is called before the parameter-validation ValueError, so unauthenticated callers always get AuthRequiredError regardless of other argument mistakes.
  • KeyErrorKalshiError wrapping in queue_position(): The explicit None-check with a descriptive error message is cleaner than letting a KeyError bubble up.
  • builtins.list[T] annotations: Correctly avoids the .list() shadowing issue documented in CLAUDE.md.
  • extra = "allow" on both new models: Appropriate for API responses that may gain new fields.
  • Sync/async parity: Both transports implement identical logic; no sync-wraps-async shortcuts.
  • Test structure: Happy path + error path + serialization + auth guard for every method is exactly the right pattern.

Summary

The implementation is solid. Two items worth addressing before merge: verify the reduce_by/reduce_to field names against the OpenAPI spec (could be a silent breakage), and confirm that to_decimal() is safe to call with raw float values (or narrow the type annotation to Decimal | str | int). The rest are informational.

@TexasCoding TexasCoding merged commit feee365 into main Apr 16, 2026
4 checks passed
@TexasCoding TexasCoding deleted the feat/order-amendments branch April 16, 2026 02:33
TexasCoding added a commit that referenced this pull request May 17, 2026
…ath (#28) (#142)

* fix(ws): wire SequenceTracker into MessageDispatcher

PR #137 added an optional seq_tracker kwarg on MessageDispatcher.__init__
so server-initiated unsubscribe (#81) can clear stale seq state alongside
the sid mapping. The wiring at the construction site in KalshiWebSocket
was never landed — the dispatcher always saw seq_tracker=None and could
only clear half the state.

Closes #27 (post-Wave-3 integration tracker).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(ws): close #84 F-O-05 + #86 — drop exc_info leak, single parse path

F-O-05 / Closes #84 (final half):
The validation-failure log in MessageDispatcher.dispatch used
`logger.warning("Failed to parse %s message", msg_type, exc_info=True)`.
Pydantic's ValidationError stringification echoes the full input — for
trade/fill/order_group channels that includes price, count, and
sometimes user identifiers, dumped straight into stdout/Sentry/log
infrastructure. Dropped exc_info; surface type + exception class name
only. The underlying exception is still available via __cause__ for
local debug.

#86 dispatcher side:
- Signature change: dispatch(raw: str) -> dispatch(data: dict, *,
  pre_validated: BaseModel | None = None). The recv loop now owns the
  json.loads (it already had one); the dispatcher's redundant second
  parse is gone.
- pre_validated lets the recv loop hand off an orderbook message it
  already validated for the local OrderbookManager, so the dispatcher
  routes the same instance to the subscription queue without re-running
  Pydantic. Eliminates the double-validate the issue called out
  (~3 µs/msg).
- json import dropped from dispatch.py (now unused).

Recv loop client.py side (in the previous commit alongside the
seq_tracker wiring): _process_frame captures the typed orderbook
message and passes pre_validated to dispatch.

Test fallout:
- 21 tests in test_dispatch.py updated from `dispatcher.dispatch(raw)`
  to `dispatcher.dispatch(json.loads(raw))` via sed.
- Dropped test_dispatch_invalid_json — that path now lives in the
  recv loop's exception ladder (covered by
  test_malformed_frame_logged_with_traceback_and_continues).
- 2 tests in test_recv_loop_hardening.py monkey-patched dispatch with
  the old (raw: str) signature; updated to (data: dict[str, Any], **kw).
- New regression: test_dispatch_pre_validated_skips_revalidation —
  asserts dispatch routes the same instance when pre_validated matches
  the expected model_cls.

Closes #84
Closes #86

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* review(#142): drop false __cause__ claim in dispatch parse-fail log

Per bot review on PR #142: the comment said "the underlying exception
is preserved on __cause__ for debug" — but the except block catches,
logs the type name, and returns. Nothing is re-raised, so no
__cause__ chain is established. Dropped the misleading sentence;
kept the security rationale (Pydantic input echo).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.

1 participant