orders: #825 review follow-ups - #829
Merged
Merged
Conversation
Six findings from the #825 review; two deferred to issues. Required submessages. #825 made a present-but-empty `action` Error::Parse, but decode_open_order_proto / decode_completed_order_proto / decode_execution_data_proto still defaulted a wholly absent `order` to Order::default() - action == Buy, the mishandling it removed. Upstream drops such a frame (EDecoder.cs returns before eWrapper.openOrder); this crate has no skip channel, so it errors. Folds decode_order_bound's local `required` into the shared one. repr(i32) and discriminants dropped from all eight integer-coded enums, Liquidity included. Nothing reads them once a payload variant exists - `variant as i32` does not compile either way - so `= 0` duplicated the hand-written From<T> for i32, and repr(i32) was an ABI commitment buying nothing. Wire codes move to each variant's rustdoc. check_wire_code_round_trip takes the Unknown constructor and probes -8..=64 for codes the table omits, so the callers' _every_wire_code names are true: a variant added without a table row now fails. Docs: enum-typing's AuctionStrategy note named an OrderBuilder field as the conversion's one caller, but the field had no setter and the line reading it was unreachable - field deleted, claim corrected, setter gap filed as #828. macros-last-resort said eight macros where grep finds nine; impl_proto_payload! gains a row. impl_wire_enum!'s closed arm reported "unknown $name" for empty input where the fallback arm reported "empty $name"; both now distinguish the two. Deferred: OrderCondition's discriminator fallback and panic (#827), which #825 called a separate change, and Unknown(code) aliasing a known code under derived PartialEq, inherited from the Liquidity precedent.
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.
Addresses the review on #825. Six findings land here; two are deferred to issues. Plan and reasoning:
plans/orders-wire-enum-followups.md.1. The required-
actioninvariant leaked one layer up#825 made a present-but-empty
actionError::Parse. Butdecode_open_order_proto/decode_completed_order_proto/decode_execution_data_protodidso a wholly absent
ordersubmessage still producedOrder::default()—action == Buy, exactly the mishandling §14 says it removed. Present-but-empty errored; absent defaulted.The reference client does neither:
EDecoder.cs'sOpenOrderEventProtoBuf(:2652) andExecutionDataEventProtoBuf(:2918) drop the frame, returning beforeeWrapper.openOrder(..)/execDetails(..)if a submessage is null. They never synthesize a default. This crate has no skip channel, so a missing submessage is nowError::Parse. Defaulting is the one option neither client takes — it hands the caller a phantom BUY order over an empty contract, which reads as real data.decode_order_bound's localrequiredhelper folds into the shared one.2.
#[repr(i32)]and the discriminants were decorativeOnce
From<T> for i32is hand-written,= 0/= 1drive nothing — andvariant as i32does not compile either way, once a payload variant exists. They were the only reason#[repr(i32)]was needed (explicit discriminants on a data-carrying enum require a primitive repr), so it was an ABI commitment in the public API buying nothing.Worse, they were a second source of truth. A missing match arm is a compile error;
Foo = 5paired withFoo => 6is not.Dropped from all eight,
Liquidityincluded — it has noFrom<Liquidity> for i32at all, so its discriminants were never read either, and leaving it out would have splitenum-typing.md's shape description in two. Each variant's wire code is now rustdoc (Wire code `0`.), which is strictly more visible than a discriminant.3.
*_round_trips_every_wire_codedid not check "every"Seven tests so named verified only the rows listed; adding
OcaType::Reserved = 7broke none of them. #822 hit this and answered it withall_tifs_covers_every_variant.Integer enums admit a cheaper guard than a per-enum
modeled_indexmatch:From<i32>is total, so every code outside the table must beUnknown(code).check_wire_code_round_tripnow takes theUnknownconstructor and probes-8..=64, skipping listed codes. Verified by temporarily adding aReserved = 7variant:5.
enum-typing.md'sAuctionStrategyclaim was falseThe node said
From<i32> for AuctionStrategy"converts nothing but thei32a caller handsOrderBuilder".OrderBuilder::auction_strategywas a private field with no setter — the only writes wereNoneat construction, so the.into()reading it was unreachable. Field deleted, claim corrected. The missing typed setter is thebuilder-enum-coveragegap the dead field was hiding: issue #828.6. "All eight macros in
src/" is ninegrep -rn "macro_rules!" src --include=*.rs | wc -l→ 9.impl_proto_payload!had no row. #825 re-derived theimpl_wire_enum!count on the line below but left the total.7.
impl_wire_enum!empty-input message asymmetryThe closed arm reported
"unknown Action"for empty input where thefallbackarm reported"empty $name". Both now distinguish no-value from unrecognized-value. Only observable through a direct"".parse()—parse_requiredintercepts empty first with"missing {label}", which names the field rather than the type.Deferred
decode_order_condition's_ => OrderCondition::Price(PriceCondition::default())still decodes an unmodeled condition type as a zeroed price condition, andFrom<i32> for OrderConditionstill panics. orders: parse wire enums through FromStr, preserving unknown values #825 called this "a separate change" and that holds: preserving the value means anOrderCondition::Unknown(i32)variant on an exhaustive public enum.OcaType::Unknown(1) != OcaType::CancelWithBlockunder derivedPartialEq, yet both encode as1. Only reachable by hand-constructingUnknownwith a known code; decoders never produce it. Inherited from theLiquidity/TimeInForceprecedent, so no action.Gate
cargo fmt --check; clippy × 3 configs;just test(6 legs, 4393 tests, 0 failed);RUSTDOCFLAGS="-D warnings" cargo doc× 3;cargo build --examples× 2;cargo build -p ibapi-integration-{sync,async} --tests;just rules-check— all clean.