multileg notifications bug fix #442
Conversation
|
My review is in progress 📖 - I will have feedback for you in a few minutes! |
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
I have reviewed your code and found 6 potential issues. To discuss my individual comments that I have added, tag me in replies using @korbit-ai.
Please react with a 👍 to my comments that you find helpful and a 👎 to those you find unhelpful - this will help me learn and improve as we collaborate.
| # Create the order object | ||
| order = self._parse_broker_order(leg, strategy_name, strategy_object) | ||
|
|
||
| # TODO: Get the average fill price and quantity |
There was a problem hiding this comment.
The method _parse_broker_order_dict contains a TODO comment to get the average fill price and quantity, but it seems this functionality has not been implemented yet. This could lead to incomplete order objects being created without the necessary information.
| match order.status.lower(): | ||
| case "submitted" | "open": | ||
| self.stream.dispatch(self.NEW_ORDER, order=stored_order) | ||
| case "partial_filled": |
There was a problem hiding this comment.
The do_polling method contains a match case statement with 'partial_filled' which should be 'partially_filled' according to the Order.OrderStatus enumeration. This typo will prevent the correct handling of partially filled orders.
| """ | ||
| Submit multiple orders to the broker. This function will submit the orders in the order they are provided. | ||
| If any order fails to submit, the function will stop submitting orders and return the last successful order. | ||
| """ | ||
| # Check if the orders are empty | ||
| if not orders: | ||
| if not orders or len(orders) == 0: |
There was a problem hiding this comment.
It seems that the check for empty orders list in submit_orders method is redundant. The condition if not orders or len(orders) == 0: can be simplified to if not orders: since the not orders check will already return True for an empty list.
| return | ||
|
|
||
| # Check if it is a multi-leg order | ||
| if is_multileg: | ||
| tag = orders[0].tag if orders[0].tag else orders[0].strategy |
There was a problem hiding this comment.
The submit_orders method no longer accepts tag as a parameter, but the variable is still being used within the method. This could lead to a NameError at runtime. Please ensure that tag is defined within the method or added back to the method signature if necessary.
| @@ -282,7 +285,7 @@ def __init__( | |||
| self.custom_params = custom_params | |||
| self._trail_stop_price = None | |||
| self.tag = tag | |||
| self.avg_fill_price = 0.0 # The weighted average filled price for this order. Calculated if not given by broker | |||
| self.avg_fill_price = avg_fill_price # The weighted average filled price for this order. Calculated if not given by broker | |||
There was a problem hiding this comment.
It looks like the new 'avg_fill_price' parameter is not being validated. We should ensure that this value is a float and non-negative to prevent potential issues with order processing.
| @@ -92,6 +92,7 @@ def __init__( | |||
| trade_cost: float = None, | |||
| custom_params={}, | |||
| identifier=None, | |||
| avg_fill_price=None, | |||
There was a problem hiding this comment.
It looks like the 'avg_fill_price' parameter is added without validation and a default value in the constructor. To maintain consistency with other parameters, please add validation for 'avg_fill_price' and set a default value if none is provided.
No description provided.