Added logs for trade processing #490
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 4 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.
| @@ -625,6 +625,9 @@ def on_trade_event_poll(): | |||
|
|
|||
| @broker.stream.add_action(broker.NEW_ORDER) | |||
| def on_trade_event_new(order): | |||
| # Log that the order was submitted | |||
| logging.info(f"Processing action for new order {order}") | |||
There was a problem hiding this comment.
The log message for a new order currently includes the entire order object, which may expose sensitive information. Please update the log message to only include relevant non-sensitive details such as the order ID and action type.
| self.logger.info( | ||
| f"Processing held trade event. Trade event received for stored_order: {stored_order}, type_event: {type_event}, price: {price}, filled_quantity: {filled_quantity}, multiplier: {multiplier}" | ||
| ) |
There was a problem hiding this comment.
The log message in _process_trade_event includes detailed order information which could be sensitive. Consider removing or sanitizing these details to enhance security.
| @@ -199,18 +202,33 @@ def add_event(self, event_name, payload): | |||
| self.queue.put((event_name, payload)) | |||
|
|
|||
| def process_event(self, event, payload): | |||
| # Log that we are processing an event. | |||
| self.strategy.logger.info(f"Processing event: {event}, payload: {payload}") | |||
There was a problem hiding this comment.
It looks like the log messages for processing events include the payload directly, which may contain sensitive information. Please ensure that any sensitive data is sanitized before being included in log messages.
| def __init__(self, strategy, asset, quantity, orders=None, hold=0, available=0, avg_fill_price=None): | ||
| self.strategy = strategy | ||
| self.asset = asset | ||
| self.symbol = self.asset.symbol | ||
| self.orders = None | ||
| self.avg_fill_price = avg_fill_price |
There was a problem hiding this comment.
I noticed that you've added an 'avg_fill_price' attribute to the Position class, but it doesn't seem to be used anywhere in the class methods. If this attribute is not needed, I would recommend removing it to avoid confusion. If it is needed, please consider adding methods that use or manipulate this attribute, and add comments explaining its purpose and usage.
No description provided.