Skip to content

Improve implementation, validations and testing for Rust instruments #2723

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jun 21, 2025

Conversation

nicolad
Copy link
Member

@nicolad nicolad commented Jun 19, 2025

Cython / Python Method Rust Method in Instrument Notes on Parity
symbol (property) symbol(&self) -> &str Same return semantics (borrowed &str).
venue (property) venue(&self) -> &str Same return semantics.
get_cost_currency() cost_currency(&self) -> &Currency Renamed for Rust style; logic identical.
make_price(value) make_price(&self, value: f64) Rounds using instrument price-precision.
make_qty(value, round_down=False) make_qty(&self, value: f64, round_down: bool) Round-down flag mirrors Python.
notional_value(qty, price, use_quote) notional_value(&self, qty, price, use_quote_for_inverse: bool) Handles linear & inverse paths.
calculate_base_quantity(qty, last_px) calculate_base_quantity(&self, qty, last_px) One-to-one logic.

@nicolad
Copy link
Member Author

nicolad commented Jun 20, 2025

Area Status What’s still needed (➜ “left”)
Trait vs struct architecture Nothing – existing Instrument trait + concrete structs is sufficient.
Core helpers (make_price, make_qty, tick navigation)
Auto-derive price_increment from price_precision when caller passes None Add default in each constructor (10^-price_precision).
_min_price_increment_precision logic in make_price ⚠️ Re-implement rounding to use min(price precision, tick-increment precision).
Common validations parity with Cython (validate_instrument_common) ⚠️ Add guards for max_price, max_notional > 0 and cross-field “both-or-neither” conditions.
Negative-path unit tests for every validation guard rstest each failure case; assert panic messages.
from_dict (serde) + round-trip to_dict/from_dict tests Implement via serde_json::from_value; test JSON⇄struct equality.
PyO3 conversions (struct ⇄ Python object) Implement #[pymethods] __getstate__/__setstate__; write tests for Python↔Rust round-trip and dict conversions.
Functional tests for inverse & quanto notional / cost-ccy Validate calculate_notional_value and cost_currency across instrument types.
Tick-scheme range tests (min/max price limits) Verify next_bid/ask_prices stop at bounds and respect custom schemes.
Decision on separate Instrument struct No dedicated struct needed – trait approach confirmed.

@nicolad nicolad marked this pull request as ready for review June 20, 2025 19:17
} else {
// Use standard rounding behavior (banker's rounding)
Quantity::new(value, self.size_precision())
let factor = 10f64.powi(self.size_precision() as i32);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is all this additional code to ensure half-even rounding?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concern / Feature “Before” – simple f64 version “After” – rust_decimal + guards Why the extra code?
Numeric type used Pure f64 maths. Decimal (base-10 exact) → then back to f64. Avoids binary-decimal drift.
Rounding when round_down = Some(true) floor() toward −∞. RoundingStrategy::ToZero (truncate toward 0). Symmetric for neg & pos sizes; aligns with most exchange “round-down” rules.
Default rounding (None / false) No rounding at all (just passes value). MidpointNearestEven (half-even, “banker’s”). Yes, half-even rounding is now explicit instead of implicit/absent.
Half-even support on midpoint ties ❌ (relies on Quantity::new or IEEE default). ✅ Guaranteed via MidpointNearestEven. Adds determinism and auditability.
Handling NaN / ∞ Whatever Quantity::new does (implicit). from_f64_retain + expect("non-finite") – explicit panic. Fail fast & loud.
Guard against sub-tick quantities None. Panics if positive input would round to < 10 % of one tick. Prevents zero-size orders.
Cross-platform determinism Dependent on IEEE-754 quirks / FP env. Pure integer math inside Decimal → deterministic. Reproducible replays.
Performance Minimal ops (cheap). Extra Decimal ops; still cheap but slower in tight loops. Usually negligible outside ultra-low-latency paths.
Lines of code / readability Short, easy to scan. Longer, self-documenting; comments explain invariants. Clarity & safety trade-off.
Primary reason for the added code? Not just half-even rounding; also precision-safe conversion, non-finite guard, sub-tick guard, deterministic behaviour. Half-even is one piece of the puzzle.

Is all this additional code only to ensure half-even rounding?

No. Ensuring midpoint-nearest-even rounding is an important part, but the extra lines also:

  1. Eliminate binary floating-point drift by moving to exact base-10 arithmetic during rounding.
  2. Fail fast on NaN/∞ instead of relying on downstream checks.
  3. Guarantee deterministic behaviour across platforms and compiler flags.
  4. Prevent “rounded-to-zero” sizes that would be rejected by exchanges.

So half-even rounding is necessary but not sufficient—the additional safeguards and determinism are equally valuable in a trading context.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let me know if there are any actions I need to take here ☝️

scheme.next_bid_price(value, n, self.price_precision())?
} else {
let inc = self.price_increment().as_f64();
if inc <= 0.0 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prices can potentially be negative (options, spreads, commodities etc).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adjusted, thx

@cjdsellers cjdsellers merged commit acc46d7 into develop Jun 21, 2025
13 checks passed
@cjdsellers cjdsellers deleted the 2630 branch June 21, 2025 23:05
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.

2 participants