Skip to content

feat(fix): order-entry message flow (closes #424)#431

Merged
TexasCoding merged 4 commits into
mainfrom
feat/424-fix-order-entry
Jun 6, 2026
Merged

feat(fix): order-entry message flow (closes #424)#431
TexasCoding merged 4 commits into
mainfrom
feat/424-fix-order-entry

Conversation

@TexasCoding

Copy link
Copy Markdown
Owner

Summary

The order-entry (OE) FIX message flow for both products, over the shared codec/session engine. Builds on the foundation (#422) and repeating-group support (#423).

Messages

Outbound (typed enum vocabulary; required fields enforced so a caller error fails at construction):

Inbound (fields optional; char/int code fields kept raw str/int so a slightly off-spec report parses cleanly rather than rejecting — compare against kalshi.fix.enums):

  • ExecutionReport (35=8) with NoMiscFees / NoPartyIDs / NoCollateralAmountChanges groups and position/fee/collateral detail on ExecType=Trade
  • OrderCancelReject (35=9), OrderMassCancelReport (35=r), BusinessMessageReject (35=j)
  • decode_app_message(raw) dispatches an inbound RawMessage (delivered to FixSession.on_message) to its typed model.

Pricing & quantities

Price (44) rides the FIX PRICE field; units follow the session's UseDollars — integer cents (1–99) on prediction by default, fixed-point dollars on margin / UseDollars=Y. Amounts use DollarDecimal, quantities FixedPointCount (no float drift). Sending an order is the generic, already-typed await session.send(NewOrderSingle(...)) — no OE coupling added to the transport layer.

Also (PR #430 review follow-ups)

  • FixGroupMeta / _GroupSpec now type entry_model as the public FixGroup rather than the private base.
  • Group decoding logs at debug on a non-integer or short NumInGroup instead of silently dropping the group.

Tests (tests/fix/test_order_entry.py, 13 new)

Round-trips for all eight messages (incl. groups), enum + price on-wire values, cents-vs-dollars price encoding, the decode_app_message dispatch (and None for admin), inbound robustness on a sparse report, and a session send-order/receive-ExecutionReport integration test against the mock acceptor.

Verification

  • uv run ruff check . — clean
  • uv run mypy kalshi/ (strict) — clean (150 files)
  • uv run pytest tests/fix/117 passed

Closes #424. Part of #402.

🤖 Generated with Claude Code

Order-entry (OE) FIX messages over the shared codec/session engine, for both
the prediction and margin products.

Outbound (typed enum vocabulary, required fields enforced):
- NewOrderSingle (35=D), OrderCancelRequest (35=F),
  OrderCancelReplaceRequest (35=G), OrderMassCancelRequest (35=q)
- Side / OrdType / TimeInForce / ExecInst (POST_ONLY) / SelfTradePreventionType
  round-trip to the wire; NoPartyIDs group via the #423 components; Price rides
  the FIX PRICE field (integer cents on prediction by default, fixed-point
  dollars on margin / UseDollars=Y); quantities are fractional decimals.

Inbound (fields optional; char/int code fields raw for robustness):
- ExecutionReport (35=8) with NoMiscFees / NoPartyIDs / NoCollateralAmountChanges
  groups, position/fee/collateral detail on ExecType=Trade
- OrderCancelReject (35=9), OrderMassCancelReport (35=r),
  BusinessMessageReject (35=j)
- decode_app_message(raw) dispatches an inbound RawMessage (from
  FixSession.on_message) to its typed model.

Also folds in the PR #430 review follow-ups: FixGroupMeta/_GroupSpec now type
entry_model as the public FixGroup (not the private base), and group decoding
logs (debug) on a non-integer or short NumInGroup instead of silently dropping.

13 new tests (round-trips incl. groups, enum/price wire values, dispatch,
inbound robustness, and a session send/receive integration test).

ruff + mypy --strict clean; 117 FIX tests pass.

Closes #424. Part of #402.

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

claude Bot commented Jun 6, 2026

Copy link
Copy Markdown

Code Review — feat(fix): order-entry message flow (#431)

Overview

Clean, well-scoped PR that adds the full order-entry FIX flow. The outbound/inbound design split (typed enums + required fields for outbound; raw chars/ints + optional fields for inbound) is the right approach for a production FIX client where server messages must parse leniently. The 13 tests cover all message types and the key behavioural paths.


Issues & Risks

1. TransactTime (tag 60) missing from outbound messages

NewOrderSingle (35=D), OrderCancelRequest (35=F), and OrderCancelReplaceRequest (35=G) all omit TransactTime. Per FIX 5.0SP2 these fields are required. The session layer adds SendingTime (52) in the header but not TransactTime (60) in the body. If the Kalshi exchange enforces the spec here, every order will be rejected.

Suggested fix: add transact_time: datetime | None = fixfield(Tag.TRANSACT_TIME, FixType.UTCTIMESTAMP, default=None) and populate it in the session send() alongside SendingTime, or at construction time.


2. price is optional in OrderCancelReplaceRequest but Kalshi only supports limit orders

price: DollarDecimal | None = fixfield(Tag.PRICE, FixType.PRICE, default=None)

Since the exchange only supports limit orders, a cancel-replace without a price will be rejected server-side. Making price required here (matching NewOrderSingle) would catch the error at construction time rather than after a round-trip to the exchange.


3. APP_MESSAGE_MODELS absent from the top-level kalshi/fix/__init__.py __all__

APP_MESSAGE_MODELS is exported in kalshi/fix/messages/__init__.py.__all__ but not wired up in kalshi/fix/__init__.py. A consumer doing from kalshi.fix import APP_MESSAGE_MODELS will fail. Either add it to the top-level __all__ or drop it from kalshi.fix.messages.__all__ if it's intentionally internal.


Test Coverage Gaps

  • The new debug-logging paths added in base.py (_parse_group short-count and non-integer NumInGroup) have no tests. A malformed round-trip fixture that deliberately delivers fewer entries than promised would cover both paths and avoid regressions if the log logic is later changed to raise.
  • decode_app_message is only dispatched for ExecutionReport and the None admin path. OrderCancelReject, OrderMassCancelReport, and BusinessMessageReject dispatch paths are untested.
  • OrderCancelReplaceRequest without a price (the optional case) has no test confirming it serialises and parses cleanly.

Minor Observations

  • FixGroupMeta/_GroupSpec type narrowing (base.py lines 83, 97, 179): upgrading from type[_FixFieldModel] to type[FixGroup] is a correct improvement — the types in __init__.py exports now match the actual constraint.
  • groupfield() default — no default is passed to parties in NewOrderSingle, OrderCancelRequest, and OrderCancelReplaceRequest. Assuming groupfield() returns field(default_factory=list), a caller creating an order without parties doesn't need to pass parties=[]. Worth a one-liner confirmation in a test (NewOrderSingle(..., parties omitted).parties == []).
  • exec_id semicolon format — using "4;7" and "1;1" in tests documents the Kalshi-specific compound exec-ID format. Consider a comment on the exec_id field in ExecutionReport noting this so future readers don't normalize it.
  • The docstring in order_entry.py is thorough and correctly describes the UseDollars price-unit behaviour. No changes needed.

Summary

The design is solid and the implementation follows existing SDK conventions well. The three actionable items above (TransactTime, required price on replace, missing top-level export) should be resolved before merge; the test gaps are lower priority but worth a follow-up issue.

- export APP_MESSAGE_MODELS from kalshi.fix top-level __all__ (was only exported
  from kalshi.fix.messages)
- comment the Kalshi compound "int;int" exec_id format on ExecutionReport
- tests: malformed group decode (short-count + non-integer NumInGroup debug
  paths), decode_app_message dispatch for OrderCancelReject /
  OrderMassCancelReport / BusinessMessageReject, a quantity-only
  OrderCancelReplaceRequest, and the NewOrderSingle empty-parties default

Two review items were intentionally NOT changed — they apply generic FIX 5.0SP2
requiredness, not the Kalshi dictionary v1.03:
- TransactTime(60) is absent from the Kalshi NewOrderSingle/Cancel/Replace
  messages (and the order-entry doc example omits tag 60); adding it would send
  a tag those messages do not define.
- Price is required='N' in OrderCancelReplaceRequest (G); a quantity-only amend
  is valid ("required if changing price"), so price stays optional.

ruff + mypy --strict clean; 122 FIX tests pass.

Part of #424 / #402.

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

Copy link
Copy Markdown
Owner Author

Thanks — addressed in 26637ca. ruff + mypy --strict clean; 122 FIX tests pass.

Actionable items

1. TransactTime (60) on outbound D/F/G — not applicable to the Kalshi dialect (no change). The review applies generic FIX 5.0SP2, but the Kalshi FIX dictionary v1.03 does not include TransactTime in NewOrderSingle / OrderCancelRequest / OrderCancelReplaceRequest, and the order-entry doc's example order omits tag 60:

8=FIXT.1.1|9=200|35=D|...|11=...|38=10|40=2|54=1|55=HIGHNY-23DEC31|44=75|59=1|10=123

Adding tag 60 would send a field those messages don't define (risking a TAG_NOT_DEFINED_FOR_MESSAGE reject). TransactTime is present (conditional) on the inbound ExecutionReport, where I have it.

2. Required price on OrderCancelReplaceRequest — not applicable (no change). Dictionary v1.03 marks Price required='N' in 35=G, and the doc says it's "required if changing price" — a quantity-only amend is explicitly valid. Forcing it would reject legitimate qty-only modifies. Added test_cancel_replace_without_price_is_qty_only to pin this.

3. APP_MESSAGE_MODELS missing from top-level __all__ — fixed. Now exported from kalshi.fix (it was only in kalshi.fix.messages).

Test gaps — added

  • base.py group-decode debug paths: test_short_numingroup_parses_available_entries (NumInGroup=3, one entry delivered) and test_non_integer_numingroup_drops_group.
  • decode_app_message dispatch: test_decode_app_message_all_inbound_types covers OrderCancelReject / OrderMassCancelReport / BusinessMessageReject (plus the existing ExecutionReport + None-admin cases).
  • Qty-only replace: covered by item 2's test.

Minor

  • groupfield() default: added test_new_order_parties_default_empty (omitting parties yields [] and no NoPartyIDs on the wire).
  • exec_id format: added a comment noting the Kalshi compound "int;int" exec id (e.g. "4;7", "-1;-1" for PENDING) is kept raw, not normalized.

Appreciate the thorough pass — the type-narrowing confirmation (type[FixGroup]) and the lenient-inbound design sign-off were reassuring.

@claude

claude Bot commented Jun 6, 2026

Copy link
Copy Markdown

Code Review

Summary: This PR adds the order-entry FIX message flow: four outbound request models, four inbound report models, a typed dispatch function, and 13 tests. The architecture is clean, the outbound/inbound design split is well-motivated, and the follow-up improvements to base.py (logging, type[FixGroup] annotation) are solid. A few points worth discussing before merge:


Issues

1. alloc_account uses INT / int — FIX tag 79 is a STRING field

In order_entry.py:

alloc_account: int | None = fixfield(Tag.ALLOC_ACCOUNT, FixType.INT, default=None)

FIX standard tag 79 (AllocAccount) is typed STRING. Using FixType.INT means the codec would reject a string account ID (e.g. "ACC-123") from the exchange. If Kalshi's spec genuinely uses an integer here, add a comment citing the Kalshi FIX Dictionary v1.03 section — it's surprising enough that the next reader will question it. If not, this should be FixType.STRING / str | None.

This same field appears on NewOrderSingle, OrderCancelRequest, OrderCancelReplaceRequest, and ExecutionReport.


2. NewOrderSingle is missing TransactTime (tag 60)

FIX 5.0 SP2 requires TransactTime on NewOrderSingle. The session layer (_send) injects the FIX envelope header (MSG_TYPE, SENDER_COMP_ID, TARGET_COMP_ID, MSG_SEQ_NUM, SENDING_TIME) but not TransactTime. If Kalshi's spec makes it optional, a comment noting the intentional omission would prevent future confusion.


3. decode_app_message propagates decode exceptions to the caller

def decode_app_message(raw: RawMessage) -> FixMessage | None:
    model = APP_MESSAGE_MODELS.get(raw.msg_type or "")
    return model.from_raw(raw) if model is not None else None

If the registered model's from_raw raises a ValidationError (e.g. a malformed field), the exception surfaces to whoever called on_message. The surrounding robustness story (inbound fields are raw str/int) means this is unlikely for normal off-spec messages, but it's still possible. The function's docstring implies it should always return FixMessage | None — a log-and-return-None on ValidationError would match that contract. At minimum, document the exception behavior.


4. APP_MESSAGE_MODELS is mutable

The dict is exported in __all__ and is module-level mutable. An accidental APP_MESSAGE_MODELS["8"] = SomethingElse in application code would silently corrupt dispatch for the whole process. Wrapping it in types.MappingProxyType costs nothing and makes the intent clearer.


Minor Points

5. OrderMassCancelReport is missing TotalAffectedOrders (tag 533)

The FIX spec includes TotalAffectedOrders on 35=r. If Kalshi sends it, the field is silently dropped during inbound parsing. Fine if Kalshi doesn't use it, but worth checking against the FIX Dictionary v1.03.

6. Field ordering in NewOrderSingleparties group placement

self_trade_prevention_type: SelfTradePreventionType | None = ...
parties: Annotated[list[Party], FixGroupMeta(Tag.NO_PARTY_IDS, Party)] = groupfield()
secondary_cl_ord_id: str | None = ...

The framework serializes in declaration order. Having parties group between self_trade_prevention_type and secondary_cl_ord_id is fine if that matches Kalshi's FIX Dictionary — just needs to be verified. The inbound parse is order-independent, so this only affects outbound.


What's Good

  • Outbound/inbound design split is well-motivated and documented: required fields on outbound fail fast at construction; inbound keeps raw str/int for tolerance. The docstring in order_entry.py explains this clearly.
  • base.py type annotation improvement (type[FixGroup] over type[_FixFieldModel]) is a correct public-surface improvement from the feat(fix): repeating-group support in the message framework (closes #423) #430 review.
  • Debug logging for malformed FIX groups (short NumInGroup, non-integer count) is exactly the right behavior — log and continue rather than silently dropping or raising.
  • Test coverage is solid — round-trips for all 8 messages including groups, enum wire values, the cents-vs-dollars encoding distinction, dispatch (including None for admin), sparse inbound, and a full session integration test.
  • _roundtrip helper in the test file correctly exercises the encode→decode cycle through the actual codec rather than mocking it.

Verdict

Items 1 and 3 are the most actionable before merge (potential runtime bug and undocumented exception contract). Item 2 needs a comment or a small addition. Items 4–6 are lower priority but worth a pass.

🤖 Generated with Claude Code

…stry

- decode_app_message now catches ValidationError / ValueError / ArithmeticError
  and returns None (logged) so a malformed inbound message is never raised into
  the consumer's on_message; regression test added
- APP_MESSAGE_MODELS wrapped in MappingProxyType (read-only dispatch registry)
- comment that AllocAccount (79) is INT per Kalshi dictionary v1.03 (subaccount
  number 0-32), not the FIX-standard STRING
- note the intentional TransactTime (60) omission on NewOrderSingle (absent from
  the Kalshi 35=D message)

Verified against dictionary v1.03, left unchanged:
- AllocAccount (79) is INT in the Kalshi dict.
- OrderMassCancelReport (35=r) has no TotalAffectedOrders (533); tag 533 is not
  in the Kalshi dictionary at all.
- NewOrderSingle field/group order matches the dictionary's 35=D layout.

ruff + mypy --strict clean; 123 FIX tests pass.

Part of #424 / #402.

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

Copy link
Copy Markdown
Owner Author

Thanks — addressed in 247bf2b. ruff + mypy --strict clean; 123 FIX tests pass.

Actionable

3. decode_app_message exception contract — fixed. It now catches ValidationError / ValueError / ArithmeticError and returns None (logged), so a malformed inbound message is swallowed rather than raised into the consumer's on_message. (ArithmeticError covers a bad DecimalInvalidOperation isn't a ValueError.) Added test_decode_app_message_returns_none_on_malformed (bad bool + bad decimal).

4. APP_MESSAGE_MODELS mutability — fixed. Wrapped in types.MappingProxyType (read-only).

1. alloc_account INT vs STRING — correct as-is, comment added. The Kalshi dictionary v1.03 types AllocAccount (79) as INT (<field number='79' name='AllocAccount' type='INT'/>, "subaccount number, 0 primary, 1-32") — a deliberate deviation from FIX-standard STRING, also reflected in the order-entry doc ("Subaccount number (0-32)"). Added a comment citing the dictionary on the field.

2. TransactTime (60) on NewOrderSingle — intentional omission, comment added. Not present on the Kalshi 35=D (per dictionary + the doc's example order); added a docstring note so it isn't re-flagged.

Minor

5. OrderMassCancelReport / TotalAffectedOrders (533) — not applicable. Tag 533 isn't in the Kalshi dictionary at all; 35=r is ClOrdID/OrderID/MassCancelResponse/MassCancelRejectReason only.

6. parties group placement — verified. Matches the dictionary's 35=D field order (…SelfTradePreventionType, NoPartyIDs, SecondaryClOrdID, OrderGroupID, CancelOrderOnPause, MaxExecutionCost, AllocAccount).

Thanks for the careful second pass — the immutable-registry and exception-contract points were good catches.

@claude

claude Bot commented Jun 6, 2026

Copy link
Copy Markdown

Code Review — feat(fix): order-entry message flow (#431)

Overview

Adds the order-entry FIX message flow: four outbound request types (NewOrderSingle, OrderCancelRequest, OrderCancelReplaceRequest, OrderMassCancelRequest) and four inbound report types (ExecutionReport, OrderCancelReject, OrderMassCancelReport, BusinessMessageReject), plus decode_app_message dispatch and the APP_MESSAGE_MODELS registry. Also ships two PR #430 follow-ups: type-widening entry_model to the public FixGroup alias and adding debug logs for malformed NumInGroup values.

Overall this is a clean, well-reasoned implementation. The outbound-typed / inbound-lenient split is the right call for a FIX integration, and the test suite covers the important paths thoroughly.


Strengths

  • Outbound/inbound asymmetry is well-justified. Required fields on outbound (fail at construction), str/int raw codes on inbound (parse robustly, compare against enums at call site). Documented clearly in the module docstring and class docstrings.
  • APP_MESSAGE_MODELS as MappingProxyType prevents accidental mutation of the dispatch table from application code — good defensive choice.
  • groupfield() default via default_factory=list avoids the shared-mutable-default pitfall correctly.
  • _parse_group debug logging (short NumInGroup and non-integer NumInGroup) are solid PR feat(fix): repeating-group support in the message framework (closes #423) #430 follow-ups; the two new tests in test_groups.py pin the behaviour.
  • 13 tests cover round-trips for all eight messages, enum wire values, price unit encoding, decode_app_message dispatch, and the session integration path. Coverage is strong.

Issues

Medium — decode_app_message drops malformed messages with no propagation path

def decode_app_message(raw: RawMessage) -> FixMessage | None:
    ...
    except (ValidationError, ValueError, ArithmeticError):
        logger.warning("failed to decode inbound %s; returning None", raw.msg_type, exc_info=True)
        return None

Returning None for both "no registered model" and "parse failure" means callers cannot distinguish the two cases. An order-fill that arrives with a single malformed field (e.g. a DollarDecimal field the exchange sends in an unexpected format) is silently dropped; the caller's on_message sees it as None and likely ignores it, losing the fill. The warning log fires, but that only helps if someone is watching logs.

Two options to consider:

  1. Separate sentinel / exception: raise a FixDecodeError (swallowed internally in on_message dispatch, exposed to direct callers of decode_app_message), or return a (FixMessage | None, Exception | None) tuple.
  2. on_decode_error hook on FixSession: let callers plug in error handling without changing the FixMessage | None return type.

Not asking for a fix in this PR, but worth a follow-up issue before the flow is wired into production consumers.

Minor — decode_app_message with msg_type=None is untested

raw.msg_type or "" correctly handles a None msg_type, but there is no test for it. A one-liner would close that gap:

def test_decode_app_message_none_msg_type() -> None:
    assert decode_app_message(RawMessage([])) is None

Minor — OrderMassCancelRequest only tests the default mass_cancel_request_type

test_mass_cancel_request_roundtrip hard-codes cl_ord_id="m1" with no mass_cancel_request_type argument, so only the default (CANCEL_FOR_SESSION = "6") is exercised. A non-default value (e.g. MassCancelRequestType.CANCEL_FOR_SECURITY) would confirm the field serialises correctly when changed.

Nit — test formatting inconsistency

test_inbound_report_tolerates_minimal_fields packs four kwargs on two lines against the style used everywhere else in the file:

# current
report = ExecutionReport(
    cl_ord_id="abc", exec_id="-1;-1", exec_type=ExecType.PENDING_NEW.value,
    ord_status=OrdStatus.PENDING_NEW.value,
)

# consistent with surrounding tests
report = ExecutionReport(
    cl_ord_id="abc",
    exec_id="-1;-1",
    exec_type=ExecType.PENDING_NEW.value,
    ord_status=OrdStatus.PENDING_NEW.value,
)

Observations (no action required)

  • NewOrderSingle.expire_time without TimeInForce.GTD: no runtime guard, consistent with the repo's policy of not adding validation for impossible-to-enforce spec rules. Fine as-is; just noting it for anyone reading.
  • asyncio_mode = "auto" in pyproject.toml means the async integration test needs no @pytest.mark.asyncio decorator — confirmed correct.
  • OrderCancelReplaceRequest.ord_type defaults to OrdType.LIMIT: makes sense for Kalshi (limit-only exchange), but worth a comment if the Kalshi FIX dict explicitly forbids non-limit amends.
  • entry_model: type[FixGroup] type widening in base.py is the right direction — using the public alias rather than the internal _FixFieldModel keeps the API surface clean.

Verdict

Approve with minor follow-up items. The medium decode_app_message concern is worth a tracked issue before this flow is consumed in production code, but it doesn't block merge. Everything else is nits.

🤖 Generated with Claude Code

…eview)

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

Copy link
Copy Markdown
Owner Author

Thanks for the approval. Follow-ups handled:

  • Medium — decode_app_message conflates "no model" vs "parse failure". Agreed this matters once the flow is consumed in production; filed as [FIX] Surface inbound decode failures (decode_app_message error routing) #432 (linked under epic perps: [EPIC] FIX API support (prediction + perps) — future #402) to land with the typed inbound-delivery ergonomics — leaning toward an on_decode_error hook on FixSession so the simple FixMessage | None contract stays for casual callers. Left out of this PR per your note.
  • Minor — msg_type=None untested. Added assert decode_app_message(RawMessage([])) is None.
  • Nit — kwargs formatting. test_inbound_report_tolerates_minimal_fields now one-kwarg-per-line.
  • Minor — non-default MassCancelRequestType. N/A in the Kalshi dialect: dictionary v1.03 defines only CANCEL_FOR_SESSION (6) for tag 530, so the default is the sole valid value (there's no CANCEL_FOR_SECURITY to exercise).
  • Observation — OrderCancelReplaceRequest.ord_type default LIMIT. The dictionary's OrdType (40) enumerates only 2=LIMIT, so non-limit isn't representable anyway; left as-is.

(15c1a4a) ruff + mypy --strict clean; 123 FIX tests pass.

@claude

claude Bot commented Jun 6, 2026

Copy link
Copy Markdown

Code Review — PR #431: feat(fix): order-entry message flow

Overall: Solid implementation that follows the established FIX message patterns. The outbound/inbound design split (strict typed enums on send, optional raw codes on receive) is the right call for FIX protocol robustness. The base.py follow-ups from #430 are clean improvements. A few things worth looking at before merge:


Issues

1. decode_app_message conflates two distinct failure modes

None is returned for both "no registered model for this MsgType" (expected for admin messages) and "malformed payload that failed validation" (unexpected / worth alerting on). A caller in an on_message handler cannot distinguish the two without reading logs.

model = APP_MESSAGE_MODELS.get(raw.msg_type or "")
if model is None:
    return None          # <- expected path
...
except (ValidationError, ValueError, ArithmeticError):
    logger.warning(...)
    return None          # <- unexpected path, same return value

For FixSession.on_message consumers that want to surface decode errors (metrics, alerting, bubbling to a dead-letter queue) this is a problem. A simple improvement would be separate return semantics — for example a tuple[FixMessage | None, bool] where the bool indicates "was this a registered type that failed", or raising a custom FixDecodeError and letting callers opt-in to swallowing it. The current design is intentional per the docstring, but the observability cost is real.

2. ArithmeticError catch is broader than documented

The inline comment says # ValueError: bad bool / int; ArithmeticError: bad Decimal (InvalidOperation). decimal.InvalidOperation is a decimal.DecimalException, itself a subclass of ArithmeticError — but ArithmeticError also covers ZeroDivisionError, OverflowError, and FloatingPointError. None of those should surface from Pydantic field parsing, but catching decimal.DecimalException directly would make the intent explicit and avoid swallowing unexpected arithmetic errors.

# More precise:
except (ValidationError, ValueError, decimal.DecimalException):

3. groupfield exposed at the top-level kalshi.fix namespace

groupfield is an implementation helper for defining message models. Lifting it into the public kalshi.fix.__all__ signals it's part of the SDK's public API surface. If the intent is to let callers build custom FixMessage subclasses, that's fine — but it should be documented. If it's accidental, remove it from kalshi/fix/__init__.py.__all__ (it can stay in kalshi.fix.messages.__all__ for internal use).


Minor observations

4. OrderCancelReplaceRequest parties group has no test

OrderCancelReplaceRequest declares a parties group identical to NewOrderSingle, but no test exercises a replace with non-empty parties. The existing test_cancel_replace_roundtrip sends parties=[] (default). One assertion like assert int(Tag.NO_PARTY_IDS) not in {t for t, _ in msg.to_body_fields()} already covers the empty path; a minimal test with parties=[Party(...)] would close the gap.

5. NewOrderSingle: expire_time with GTD has no test

TimeInForce.GTD is in the enum and expire_time is on the model, but there's no test that sends a GTD order with a populated expire_time. Not blocking, but the pairing is worth exercising.

6. OrderMassCancelRequest docstring: "NR only" could be a FixSessionType guard

The "NR only" note in the docstring is accurate, but there's no enforcement. A caller using a multivariate session could accidentally send a mass-cancel and get a confusing exchange rejection. This is a FIX-layer constraint and not easily enforced here — just flagging it as something to revisit if the session layer gains request validation.


What's good

  • The outbound vs inbound type philosophy (typed enums + required fields on send; optional raw codes on receive) is exactly right for a FIX client SDK.
  • MappingProxyType on APP_MESSAGE_MODELS is a nice defensive touch.
  • decode_app_message handling of None msg_type via or "" is clean.
  • The _parse_group debug-log improvements (short count and non-integer count) are well-targeted and the two new regression tests in test_groups.py pin the behavior correctly.
  • FixGroupMeta.entry_model: type[FixGroup] (instead of the private _FixFieldModel) is a clean public-API improvement.
  • asyncio_mode = "auto" — the async integration test works without a decorator, confirmed.
  • 13 tests cover round-trips for all 8 messages, enum wire values, cents-vs-dollars price encoding, dispatch, robustness on sparse inbound, and a full session send/receive integration.

Verdict: Items 1–3 are worth addressing before merge; 4–6 are nice-to-haves. The core implementation is correct and well-tested.

🤖 Generated with Claude Code

@TexasCoding
TexasCoding merged commit fc4af90 into main Jun 6, 2026
5 checks passed
@TexasCoding
TexasCoding deleted the feat/424-fix-order-entry branch June 6, 2026 01:11
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.

[FIX] Order Entry session (prediction + perps)

1 participant