Inactive orders #1206
Draft
Inactive orders #1206
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
GuillaumeDSM
force-pushed
the
inactive_orders
branch
from
550972c to
2a0f2cf
Compare
Herklos
reviewed
Apr 29, 2025
Comment on lines
408
to
412
| if self.simulate: | ||
| self.logger.error(f"Can't update order as inactive on simulated trading.") | ||
| return False |
There was a problem hiding this comment.
Shouldn't we check self.enable_inactive_orders instead self.simulate here ?
Comment on lines
99
to
102
| self.is_active = True # When is_active=False order is not pushed to exchanges | ||
| self.active_trigger_price: decimal.Decimal = None # price threshold from which the order becomes active | ||
| # when True, order becomes active when current price >= active_trigger_price | ||
| self.active_trigger_above: bool = None | ||
| self._active_trigger_event: asyncio.Event = None # will be set when the price is hit | ||
| # waiter that will call on_active_trigger() when active_trigger_event is set | ||
| self._active_trigger_task: asyncio.Task = None | ||
| # True when a transition between active and inactive is being made | ||
| self.is_in_active_inactive_transition = False |
There was a problem hiding this comment.
I think we should create an ActiveState or something like this to manage all attributes and functions related to order activeness. I think it will be easier to abstract the activeness logic inside a dedicated class.
There was a problem hiding this comment.
done in order.active_trigger using the new PriceTrigger class
GuillaumeDSM
force-pushed
the
inactive_orders
branch
from
ab80116 to
f773a3c
Compare
Herklos
reviewed
May 3, 2025
Comment on lines
438
to
456
| def create_or_update_active_trigger(self, active_trigger_price: decimal.Decimal, active_trigger_above: bool): | ||
| if self.active_trigger is None: | ||
| self.active_trigger = price_trigger.PriceTrigger( | ||
| self.on_active_trigger, (None, None), active_trigger_price, active_trigger_above | ||
| ) | ||
| else: | ||
| self.active_trigger.trigger_price = active_trigger_price | ||
| self.active_trigger.trigger_above = active_trigger_above | ||
|
|
||
| async def set_as_inactive(self, active_trigger_price: decimal.Decimal, active_trigger_above: bool): | ||
| """ | ||
| Marks the instance as inactive and ensures the inactive order watcher is scheduled. | ||
| """ | ||
| if active_trigger_price is None or active_trigger_above is None: | ||
| raise ValueError( | ||
| f"Both active_trigger_price and active_trigger_above must be provided to set an order as inactive" | ||
| ) | ||
| logging.get_logger(self.get_logger_name()).info("Order is switching to inactive") | ||
| self.is_active = False | ||
| self.create_or_update_active_trigger(active_trigger_price, active_trigger_above) | ||
| # enforce attributes in case order has been canceled | ||
| self.status = enums.OrderStatus.OPEN | ||
| self.canceled_time = 0 | ||
| await self._ensure_inactive_order_watcher() |
There was a problem hiding this comment.
I don't think this is consistent with the trigger architecture. I believe Order should only be able to add/update a trigger but not create it. In other words, it should not take an 'active_trigger_price' argument but only an 'active_trigger,' and it’s the responsibility of the caller to provide the instance of the correct trigger class based on what is desired.
There was a problem hiding this comment.
good point, i'll update this, thanks !
Herklos
reviewed
May 3, 2025
| import octobot_trading.personal_data.orders.triggers.base_trigger as base_trigger | ||
|
|
||
|
|
||
| class PriceTrigger(base_trigger.BaseTrigger): |
GuillaumeDSM
force-pushed
the
inactive_orders
branch
from
f773a3c to
bd6b3a0
Compare
Herklos
reviewed
May 4, 2025
| finally: | ||
| self.is_in_active_inactive_transition = previous_value | ||
|
|
||
| def use_active_trigger(self, active_trigger: price_trigger.PriceTrigger): |
There was a problem hiding this comment.
Suggested change
| def use_active_trigger(self, active_trigger: price_trigger.PriceTrigger): | |
| def use_active_trigger(self, active_trigger: base_trigger.BaseTrigger): |
| self.active_trigger.trigger_price = active_trigger.trigger_price | ||
| self.active_trigger.trigger_above = active_trigger.trigger_above | ||
|
|
||
| async def set_as_inactive(self, active_trigger: price_trigger.PriceTrigger): |
There was a problem hiding this comment.
Suggested change
| async def set_as_inactive(self, active_trigger: price_trigger.PriceTrigger): | |
| async def set_as_inactive(self, active_trigger: base_trigger.BaseTrigger): |
There was a problem hiding this comment.
indeed, it's up, I also added unit tests.
GuillaumeDSM
force-pushed
the
inactive_orders
branch
from
bd6b3a0 to
bb61376
Compare
GuillaumeDSM
force-pushed
the
inactive_orders
branch
from
bb61376 to
3db76c7
Compare
GuillaumeDSM
force-pushed
the
inactive_orders
branch
from
3db76c7 to
4e5a8ab
Compare
Herklos
reviewed
May 6, 2025
| def test_is_pending_when_not_initialized(trigger): | ||
| assert not trigger.is_pending() | ||
|
|
||
| def test_str_representation(trigger): |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.