Skip to content

Improve validations for TrailingStopMarketOrder #2607

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.

Sign up for GitHub

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 4 commits into from
May 7, 2025
Merged

Improve validations for TrailingStopMarketOrder #2607

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8235aea
Improve validations for TrailingStopMarketOrder
nicolad May 7, 2025
e5bae65
Small improvement.
nicolad May 7, 2025
46cbcf0
Small improvement.
nicolad May 7, 2025
a13683f
Small improvement.
nicolad May 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions crates/model/src/orders/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ pub mod trailing_stop_market;
pub mod stubs;

// Re-exports
use anyhow::anyhow;
use enum_dispatch::enum_dispatch;
use indexmap::IndexMap;
use nautilus_core::{UUID4, UnixNanos};
Expand Down Expand Up @@ -110,6 +111,8 @@ pub enum OrderError {
AlreadyInitialized,
#[error("Order had no previous state")]
NoPreviousState,
#[error("{0}")]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@cjdsellers this doesn't look right to me, but used this approach to make it pass tests

Invariant(#[from] anyhow::Error),
}

#[must_use]
Expand All @@ -126,6 +129,34 @@ pub fn str_indexmap_to_ustr(h: IndexMap<String, String>) -> IndexMap<Ustr, Ustr>
.collect()
}

#[inline]
pub(crate) fn check_display_qty(
Copy link
Member

Choose a reason for hiding this comment

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

Nice work extracting these two functions.

display_qty: Option<Quantity>,
quantity: Quantity,
) -> Result<(), OrderError> {
if let Some(q) = display_qty {
if q > quantity {
return Err(OrderError::Invariant(anyhow!(
"`display_qty` may not exceed `quantity`"
)));
}
}
Ok(())
}

#[inline]
pub(crate) fn check_time_in_force(
time_in_force: TimeInForce,
expire_time: Option<UnixNanos>,
) -> Result<(), OrderError> {
if time_in_force == TimeInForce::Gtd && expire_time.unwrap_or_default() == 0 {
return Err(OrderError::Invariant(anyhow!(
"`expire_time` is required for `GTD` order"
)));
}
Ok(())
}

impl OrderStatus {
/// Transitions the order state machine based on the given `event`.
///
Expand Down
18 changes: 4 additions & 14 deletions crates/model/src/orders/stop_market.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,7 @@
use std::ops::{Deref, DerefMut};

use indexmap::IndexMap;
use nautilus_core::{
UUID4, UnixNanos,
correctness::{FAILED, check_predicate_true},
};
use nautilus_core::{UUID4, UnixNanos, correctness::FAILED};
use rust_decimal::Decimal;
use serde::{Deserialize, Serialize};
use ustr::Ustr;
Expand All @@ -35,7 +32,7 @@ use crate::{
AccountId, ClientOrderId, ExecAlgorithmId, InstrumentId, OrderListId, PositionId,
StrategyId, Symbol, TradeId, TraderId, Venue, VenueOrderId,
},
orders::OrderError,
orders::{OrderError, check_display_qty, check_time_in_force},
types::{Currency, Money, Price, Quantity, quantity::check_positive_quantity},
};

Expand Down Expand Up @@ -94,16 +91,9 @@ impl StopMarketOrder {
) -> anyhow::Result<Self> {
check_positive_quantity(quantity, stringify!(quantity))?;

if let Some(q) = display_qty {
check_predicate_true(q <= quantity, "`display_qty` may not exceed `quantity`")?;
}
check_display_qty(display_qty, quantity)?;

if time_in_force == TimeInForce::Gtd {
check_predicate_true(
expire_time.unwrap_or_default() > 0,
"`expire_time` is required for `GTD` order",
)?;
}
check_time_in_force(time_in_force, expire_time)?;

let init_order = OrderInitialized::new(
trader_id,
Expand Down
17 changes: 4 additions & 13 deletions crates/model/src/orders/trailing_stop_limit.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,7 @@
use std::ops::{Deref, DerefMut};

use indexmap::IndexMap;
use nautilus_core::{
UUID4, UnixNanos,
correctness::{FAILED, check_predicate_true},
};
use nautilus_core::{UUID4, UnixNanos, correctness::FAILED};
use rust_decimal::Decimal;
use serde::{Deserialize, Serialize};
use ustr::Ustr;
Expand All @@ -35,6 +32,7 @@ use crate::{
AccountId, ClientOrderId, ExecAlgorithmId, InstrumentId, OrderListId, PositionId,
StrategyId, Symbol, TradeId, TraderId, Venue, VenueOrderId,
},
orders::{check_display_qty, check_time_in_force},
types::{Currency, Money, Price, Quantity, quantity::check_positive_quantity},
};

Expand Down Expand Up @@ -103,16 +101,9 @@ impl TrailingStopLimitOrder {
) -> anyhow::Result<Self> {
check_positive_quantity(quantity, stringify!(quantity))?;

if let Some(q) = display_qty {
check_predicate_true(q <= quantity, "`display_qty` may not exceed `quantity`")?;
}
check_display_qty(display_qty, quantity)?;

if time_in_force == TimeInForce::Gtd {
check_predicate_true(
expire_time.unwrap_or_default() > 0,
"`expire_time` is required for `GTD` order",
)?;
}
check_time_in_force(time_in_force, expire_time)?;

let init_order = OrderInitialized::new(
trader_id,
Expand Down
196 changes: 178 additions & 18 deletions crates/model/src/orders/trailing_stop_market.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,10 @@
use std::ops::{Deref, DerefMut};

use indexmap::IndexMap;
use nautilus_core::{UUID4, UnixNanos};
use nautilus_core::{
UUID4, UnixNanos,
correctness::FAILED,
};
use rust_decimal::Decimal;
use serde::{Deserialize, Serialize};
use ustr::Ustr;
Expand All @@ -32,8 +35,8 @@ use crate::{
AccountId, ClientOrderId, ExecAlgorithmId, InstrumentId, OrderListId, PositionId,
StrategyId, Symbol, TradeId, TraderId, Venue, VenueOrderId,
},
orders::OrderError,
types::{Currency, Money, Price, Quantity},
orders::{OrderError,check_display_qty, check_time_in_force},
types::{Currency, Money, Price, Quantity, quantity::check_positive_quantity},
};

#[derive(Clone, Debug, Serialize, Deserialize)]
Expand All @@ -56,8 +59,14 @@ pub struct TrailingStopMarketOrder {

impl TrailingStopMarketOrder {
/// Creates a new [`TrailingStopMarketOrder`] instance.
///
/// # Errors
Copy link
Member

Choose a reason for hiding this comment

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

There's a slightly different format for the errors (and panics) docs. I'll merge and align on develop branch.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thx

///
/// * `quantity` must be strictly positive.
/// * If `time_in_force == GTD`, an `expire_time` > 0 ns must be supplied.
/// * When provided, `display_qty` must not exceed `quantity`.
#[allow(clippy::too_many_arguments)]
pub fn new(
pub fn new_checked(
trader_id: TraderId,
strategy_id: StrategyId,
instrument_id: InstrumentId,
Expand Down Expand Up @@ -85,8 +94,13 @@ impl TrailingStopMarketOrder {
tags: Option<Vec<Ustr>>,
init_id: UUID4,
ts_init: UnixNanos,
) -> Self {
// TODO: Implement new_checked and check quantity positive, add error docs.
) -> anyhow::Result<Self> {
check_positive_quantity(quantity, stringify!(quantity))?;

check_display_qty(display_qty, quantity)?;

check_time_in_force(time_in_force, expire_time)?;

let init_order = OrderInitialized::new(
trader_id,
strategy_id,
Expand All @@ -96,17 +110,17 @@ impl TrailingStopMarketOrder {
OrderType::TrailingStopMarket,
quantity,
time_in_force,
false,
/*post_only=*/ false,
reduce_only,
quote_quantity,
false,
/*is_close=*/ false,
init_id,
ts_init,
ts_init,
None,
/*price=*/ None,
Some(trigger_price),
Some(trigger_type),
None,
/*limit_offset=*/ None,
Some(trailing_offset),
Some(trailing_offset_type),
expire_time,
Expand All @@ -123,7 +137,7 @@ impl TrailingStopMarketOrder {
tags,
);

Self {
Ok(Self {
core: OrderCore::new(init_order),
trigger_price,
trigger_type,
Expand All @@ -134,7 +148,69 @@ impl TrailingStopMarketOrder {
trigger_instrument_id,
is_triggered: false,
ts_triggered: None,
}
})
}

#[allow(clippy::too_many_arguments)]
pub fn new(
trader_id: TraderId,
strategy_id: StrategyId,
instrument_id: InstrumentId,
client_order_id: ClientOrderId,
order_side: OrderSide,
quantity: Quantity,
trigger_price: Price,
trigger_type: TriggerType,
trailing_offset: Decimal,
trailing_offset_type: TrailingOffsetType,
time_in_force: TimeInForce,
expire_time: Option<UnixNanos>,
reduce_only: bool,
quote_quantity: bool,
display_qty: Option<Quantity>,
emulation_trigger: Option<TriggerType>,
trigger_instrument_id: Option<InstrumentId>,
contingency_type: Option<ContingencyType>,
order_list_id: Option<OrderListId>,
linked_order_ids: Option<Vec<ClientOrderId>>,
parent_order_id: Option<ClientOrderId>,
exec_algorithm_id: Option<ExecAlgorithmId>,
exec_algorithm_params: Option<IndexMap<Ustr, Ustr>>,
exec_spawn_id: Option<ClientOrderId>,
tags: Option<Vec<Ustr>>,
init_id: UUID4,
ts_init: UnixNanos,
) -> Self {
Self::new_checked(
trader_id,
strategy_id,
instrument_id,
client_order_id,
order_side,
quantity,
trigger_price,
trigger_type,
trailing_offset,
trailing_offset_type,
time_in_force,
expire_time,
reduce_only,
quote_quantity,
display_qty,
emulation_trigger,
trigger_instrument_id,
contingency_type,
order_list_id,
linked_order_ids,
parent_order_id,
exec_algorithm_id,
exec_algorithm_params,
exec_spawn_id,
tags,
init_id,
ts_init,
)
.expect(FAILED)
}
}

Expand Down Expand Up @@ -438,15 +514,13 @@ impl From<OrderInitialized> for TrailingStopMarketOrder {
event.order_side,
event.quantity,
event
.trigger_price // TODO: Improve this error, model order domain errors
.expect(
"Error initializing order: `trigger_price` was `None` for `TrailingStopMarketOrder`",
),
.trigger_price
.expect("Error initializing order: `trigger_price` was `None` for `TrailingStopMarketOrder`"),
event
.trigger_type
.expect("Error initializing order: `trigger_type` was `None` for `TrailingStopMarketOrder`"),
event.trailing_offset.unwrap(), // TODO
event.trailing_offset_type.unwrap(), // TODO
event.trailing_offset.unwrap(),
event.trailing_offset_type.unwrap(),
event.time_in_force,
event.expire_time,
event.reduce_only,
Expand All @@ -467,3 +541,89 @@ impl From<OrderInitialized> for TrailingStopMarketOrder {
)
}
}

////////////////////////////////////////////////////////////////////////////////
// Tests
////////////////////////////////////////////////////////////////////////////////
#[cfg(test)]
mod tests {
use rstest::rstest;
use rust_decimal_macros::dec;

use crate::{
enums::{OrderSide, OrderType, TimeInForce, TrailingOffsetType, TriggerType},
instruments::{CurrencyPair, stubs::*},
orders::{Order, builder::OrderTestBuilder},
types::{Price, Quantity},
};

#[rstest]
fn test_initialize(_audusd_sim: CurrencyPair) {
let order = OrderTestBuilder::new(OrderType::TrailingStopMarket)
.instrument_id(_audusd_sim.id)
.side(OrderSide::Buy)
.trigger_price(Price::from("0.68000"))
.trigger_type(TriggerType::LastPrice)
.trailing_offset(dec!(10))
.trailing_offset_type(TrailingOffsetType::Price)
.quantity(Quantity::from(1))
.build();

assert_eq!(order.trigger_price(), Some(Price::from("0.68000")));
assert_eq!(order.time_in_force(), TimeInForce::Gtc);

assert_eq!(order.is_triggered(), Some(false));
assert_eq!(order.filled_qty(), Quantity::from(0));
assert_eq!(order.leaves_qty(), Quantity::from(1));

assert_eq!(order.display_qty(), None);
assert_eq!(order.trigger_instrument_id(), None);
assert_eq!(order.order_list_id(), None);
}

#[rstest]
#[should_panic(expected = "Condition failed: `display_qty` may not exceed `quantity`")]
fn test_display_qty_gt_quantity_err(audusd_sim: CurrencyPair) {
OrderTestBuilder::new(OrderType::TrailingStopMarket)
.instrument_id(audusd_sim.id)
.side(OrderSide::Buy)
.trigger_price(Price::from("0.68000"))
.trigger_type(TriggerType::LastPrice)
.trailing_offset(dec!(10))
.trailing_offset_type(TrailingOffsetType::Price)
.quantity(Quantity::from(1))
.display_qty(Quantity::from(2))
.build();
}

#[rstest]
#[should_panic(
expected = "Condition failed: invalid `Quantity` for 'quantity' not positive, was 0"
)]
fn test_quantity_zero_err(audusd_sim: CurrencyPair) {
OrderTestBuilder::new(OrderType::TrailingStopMarket)
.instrument_id(audusd_sim.id)
.side(OrderSide::Buy)
.trigger_price(Price::from("0.68000"))
.trigger_type(TriggerType::LastPrice)
.trailing_offset(dec!(10))
.trailing_offset_type(TrailingOffsetType::Price)
.quantity(Quantity::from(0))
.build();
}

#[rstest]
#[should_panic(expected = "Condition failed: `expire_time` is required for `GTD` order")]
fn test_gtd_without_expire_err(audusd_sim: CurrencyPair) {
OrderTestBuilder::new(OrderType::TrailingStopMarket)
.instrument_id(audusd_sim.id)
.side(OrderSide::Buy)
.trigger_price(Price::from("0.68000"))
.trigger_type(TriggerType::LastPrice)
.trailing_offset(dec!(10))
.trailing_offset_type(TrailingOffsetType::Price)
.time_in_force(TimeInForce::Gtd)
.quantity(Quantity::from(1))
.build();
}
}
Loading