Refactor execution message handling in Rust #2291
Conversation
cjdsellers
changed the title
There was a problem hiding this comment.
I'm noticing that using the shared constructs and message handler style traits is not as ergonomic as it could be. There's a lot of cognitive overhead to Rc, RefCell, explicit borrows and wrapper enums that make it hard to understand the core logic.
I believe this is mostly because of the SharedMessageHandler is defined. I'll look into how it can be made a little more flexible either a type level or using proc macros. What do you think?
| pub fn new( | ||
| clock: Rc<RefCell<dyn Clock>>, | ||
| cache: Rc<RefCell<Cache>>, | ||
| msgbus: Rc<RefCell<MessageBus>>, | ||
| ) -> Self { |
There was a problem hiding this comment.
A lot constructs are wrapped by Rc RefCell but it adds to the cognitive load. Perhaps all such constructs can defined a wrapper type alias to make it easier to read.
pub type SharedCache = Rc<RefCell>;
|
|
||
| fn initialize_modify_order_handler(emulator: Rc<RefCell<OrderEmulator>>) { | ||
| let handler = ModifyOrderHandlerAny::OrderEmulator(emulator.clone()); | ||
| emulator.borrow_mut().set_modify_order_handler(handler); |
There was a problem hiding this comment.
Again a common pattern emerging from the Rc RefCell shared nature of the constructs. For core constructs, perhaps it's better to define a new wrapper struct and expose AsRef and AsMutRef on them. So that the explicit borrows can be reduced.
struct SharedOrderEmulator(Rc<RefCell<OrderEmulator>>)
impl AsRef<OrderEmulator> for SharedOrderEmulator {
pub fn ... {
self.0.borrow()
}
}
| pub enum FillMarketOrderHandlerAny { | ||
| OrderMatchingEngine(Rc<RefCell<OrderMatchingEngine>>), | ||
| OrderEmulator(Rc<RefCell<OrderEmulator>>), | ||
| } | ||
|
|
||
| impl FillMarketOrderHandler for FillMarketOrderHandlerAny { | ||
| fn fill_market_order(&mut self, order: &OrderAny) { | ||
| match self { | ||
| FillMarketOrderHandlerAny::OrderMatchingEngine(engine) => { | ||
| engine.borrow_mut().fill_market_order(&mut order.clone()); | ||
| } | ||
| FillMarketOrderHandlerAny::OrderEmulator(emulator) => { | ||
| emulator.borrow_mut().fill_market_order(&mut order.clone()); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Depending on this the FillMarketOrderHandler is consumed it might be better to implement the FillMarketOrderHandler trait for the OrderMatchingEngine and OrderEmulator individually and store it as as Box or Rc that will reduce the number of enums and wrapper types created for this functionality.
Pull Request
modelsandmatching_enginefrombacktesttoexecutioncrateexecutioncrateOrderMatchingCorehandlers:FillMarketOrderHandler,FillLimitOrderHandlerandTriggerStopOrderHandlerwith two variants of implementation forOrderMatchingEngineandOrderEmulatorOrderMatchingCorefrom closures to these handlers with proper setter functionsOrderMatchingEnginenow fills resting limit orders fromOrderMatchingCorewhen iterated if the market bid/ask movedOrderEmulatorcompletely refactored with new handler pattern, component cleaner and checked, and state moved fromOrderEmulatorStateto main struct which simplifies component state change and improves code quality and makes it easier to read