Implement different triggers to reset orders #273

bitphage · 2018-08-15T15:10:00Z

Always reset orders whether it was a filled, expired or manually
cancelled order detected
Reset on partial fill (with % threshold, optional)
Allow to define custom order expiration time
Reset orders on market trade (optional)
Reset orders on center price change threshold (optional)
Also disable log message about correct orders to remove log noise
Writing of a trade log entry is disabled because we cannot properly
distinguish an expired order from a filled one

- Always reset orders whether it was a filled, expired or manually cancelled order detected - Reset on partial fill (with % threshold, optional) - Allow to define custom order expiration time - Reset orders on market trade (optional) - Reset orders on center price change threshold (optional) - Also disable log message about correct orders to remove log noise - Writing of a trade log entry is disabled because we cannot properly distinguish an expired order from a filled one Closes: Codaone#226, Codaone#270

mikakoi · 2018-08-28T05:17:29Z

dexbot/strategies/relative_orders.py

+        if need_update:
+            self.update_orders()
+        else:
+            pass


The point of logging "Orders correct on market" is that it's used as a status for the worker in the GUI.
In a case where the strategy is initialized but no action is required to be done after it, a status message "Initializing Relative Orders" will get stuck on the GUI and that will just confuse the user.

I implemented a fix for this here 347eedf#diff-472e57ad5bbb88571e326e56670648b7R173 but I think it's going to take a while until I can merge that to master, so could just copy the change from there?

mikakoi · 2018-08-28T05:18:39Z

dexbot/strategies/relative_orders.py

@@ -108,7 +157,7 @@ def calculate_order_prices(self):
        self.sell_price = self.center_price * math.sqrt(1 + self.spread)

    def update_orders(self):
-        self.log.info('Change detected, updating orders')
+        #self.log.debug('Change detected, updating orders')


Don't leave unneeded comments lying around. Changing this to debug is fine.

mikakoi · 2018-08-28T05:28:32Z

dexbot/strategies/relative_orders.py

-                    self.write_order_log(self.worker_name, order)
+                    need_update = True
+                    self.log.debug('Could not found order on the market, it was filled, expired or cancelled')
+                    # FIXME: writing a log entry is disabled because we cannot distinguish an expired order


I think it would be better to disable order expiration setting for now, so that order logging isn't totally broken

We could also inform users that trades.csv will be useless if they use order expiration. Order expiration is a useful tool otherwise.

If that's fine then it only needs an if statement to check if order expiration setting is on

mikakoi · 2018-08-28T05:37:03Z

dexbot/strategies/relative_orders.py

+
+        # Check for conflicting settings
+        if self.is_reset_on_price_change and not self.is_center_price_dynamic:
+            self.log.error('reset_on_price_change requires Dynamic Center Price')


Please use the human readable format for the names

@mikakoi do you mean human readable format inside the log message?

Yes, Just change the message to 'Reset orders on center price change' requires 'Dynamic center price' or something

mikakoi · 2018-08-28T05:40:57Z

dexbot/strategies/relative_orders.py

+        if self.is_reset_on_price_change and not self.is_center_price_dynamic:
+            self.log.error('reset_on_price_change requires Dynamic Center Price')
+            self.disabled = True
+        self.update_orders()


Is there a reason you changed check_orders() to update_orders()?

Yes, because relative orders strategy is supposed to remove orders on exit, we can assume on initializing that we don't have any orders anyway, so there is no sense in checking something that should not be exist.

Well you can actually keep the orders on the market with relative orders on exit. If you simply quit the program when relative orders is running, it leaves the orders on the market.

I just feel it's a waste to cancel those orders on start up.

Yes, I agree. Furthermore, we can add an option into GUI like "Keep orders on exit" to let the user decide should the bot keep orders or not.

…rs-reset-options

@mikakoi

As described by @mikakoi: The point of logging "Orders correct on market" is that it's used as a status for the worker in the GUI. In a case where the strategy is initialized but no action is required to be done after it, a status message "Initializing Relative Orders" will get stuck on the GUI and that will just confuse the user. Code part is from 347eedf Closes: Codaone#270

There are maybe old orders from previous run, so give a chance to them.

We still need to cancel old orders on startup when "Reset orders on center price change" is active. We don't know what the center price was before.

bitphage · 2018-08-28T07:50:35Z

OK, updated the PR with proposed fixes.

mikakoi · 2018-08-28T11:31:15Z

dexbot/strategies/relative_orders.py

+        delta = datetime.now() - self.last_check
+
+        # Only allow to check orders whether minimal time passed
+        if delta < timedelta(seconds=self.min_check_interval):


This check doesn't work on initialization as self.last_check is set in the constructor

This is fine, we don't need it on initialization, this check is for rate-limiting only.

mikakoi · 2018-08-28T11:31:55Z

dexbot/strategies/relative_orders.py

-                self.log.info("Orders correct on market")
+        if need_update:
+            self.update_orders()
+        elif self.initializing:


Actually you could just check for event == 'init', since you're passing that in the constructor.

This will not work because 'init' event is passed during initialization only, and thus code under if need_update will be used in 99% times. On the next check an event will be a real market event, not 'init'.

Actually, the point of the log message is to display it only during initialization, it doesn't matter if it doesn't get ran after that

During initialization there will be another message "Done placing orders" whether everything fine.

Well not really, if in the initialization phase the worker doesn't need to place any orders so it doesn't print the message. But it's fine since already removed the event parameter

So no further changes required?

Yeah, merged the changes already

mikakoi · 2018-08-28T12:02:22Z

dexbot/strategies/relative_orders.py

+                            #        be correct
+
+        if (self.is_reset_on_market_trade and
+            isinstance(event, FilledOrder)):


This should only be evaluated true if dynamic center price is on

mikakoi · 2018-08-28T12:05:04Z

dexbot/strategies/relative_orders.py

+            ConfigElement('reset_on_partial_fill', 'bool', True, 'Reset orders on partial fill',
+                          'Reset orders when buy or sell order is partially filled', None),
+            ConfigElement('partial_fill_threshold', 'float', 30, 'Fill threshold',
+                          'Fill threshold to reset orders', (0, 100, 2, '%')),


Maybe this should say Order fill threshold to reset orders to be more descriptive

mikakoi · 2018-08-28T12:07:23Z

dexbot/strategies/relative_orders.py

@@ -1,8 +1,11 @@
 import math
+from datetime import datetime


Use from datetime import datetime, timedelta

This is option is actually not needed because it's duplicating behavior of "Reset on center price change" with "threshold = 0"

check_orders() are called from __init__(), so it will not work because initial "self.last_check = now"

bitphage added 2 commits August 15, 2018 20:08

Avoid buy/sell 0 in relative_orders.py

fae970b

mikakoi reviewed Aug 28, 2018

View reviewed changes

bitphage added 9 commits August 28, 2018 11:26

Merge remote-tracking branch 'upstream/master' into 226-relative-orde…

3641cce

…rs-reset-options

Uncomment and enable debug message

ba80c10

Fix option name in the log message

275ce1b

Do not let update_orders() when conflicting settings detected

9f1da45

Call check_orders() on initialization instead of update_orders()

d1abe0d

There are maybe old orders from previous run, so give a chance to them.

Write a trade log entry only when we are not using custom expiration

c43c0c2

Update the log message

7caf08b

Reset orders on startup if is_reset_on_price_change active

28e8dc8

We still need to cancel old orders on startup when "Reset orders on center price change" is active. We don't know what the center price was before.

mikakoi reviewed Aug 28, 2018

View reviewed changes

bitphage added 4 commits August 28, 2018 20:32

Fix imports styling

767dd9d

Update "Fill threshold" option description

b447161

Remove reset_on_market_trade option

db74ed7

This is option is actually not needed because it's duplicating behavior of "Reset on center price change" with "threshold = 0"

Do not ignore market update events while initializing

901e880

check_orders() are called from __init__(), so it will not work because initial "self.last_check = now"

mikakoi merged commit 901e880 into Codaone:master Aug 29, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement different triggers to reset orders #273

Implement different triggers to reset orders #273

bitphage commented Aug 15, 2018

mikakoi Aug 28, 2018

mikakoi Aug 28, 2018

mikakoi Aug 28, 2018

MarkoPaasila Aug 28, 2018

mikakoi Aug 28, 2018

MarkoPaasila Aug 28, 2018

mikakoi Aug 28, 2018

bitphage Aug 28, 2018

mikakoi Aug 28, 2018

mikakoi Aug 28, 2018

bitphage Aug 28, 2018

mikakoi Aug 28, 2018

bitphage Aug 28, 2018

bitphage commented Aug 28, 2018

mikakoi Aug 28, 2018

bitphage Aug 28, 2018

mikakoi Aug 28, 2018

bitphage Aug 28, 2018 •

edited

Loading

mikakoi Aug 29, 2018

bitphage Aug 29, 2018

mikakoi Aug 29, 2018

bitphage Aug 29, 2018 •

edited

Loading

mikakoi Aug 29, 2018

mikakoi Aug 28, 2018

mikakoi Aug 28, 2018

mikakoi Aug 28, 2018

Implement different triggers to reset orders #273

Implement different triggers to reset orders #273

Conversation

bitphage commented Aug 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bitphage commented Aug 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bitphage Aug 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bitphage Aug 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bitphage Aug 28, 2018 •

edited

Loading

bitphage Aug 29, 2018 •

edited

Loading