fix: exceeding max open trades

[uhliksk]

Description

• Allow buy orders to be placed for already open trades but prevent opening new trades.
• If Grid Trade #1 buy order is executed and max. open trades limit is reached, all other Grid Trade #1 buy orders are cancelled. This will prevent overshooting the limit while it will allow to wait in multiple symbol for the first buy order to be triggered.

Related Issue

#398, #457

Motivation and Context

This will solve the problem with more opened trades than allowed while also will fix the issue when buy orders in already open trades weren't allowed if the limit was exceeded.

How Has This Been Tested?

I tested changing the limit on the fly to open more trades and then limiting them to lower value. Then waiting if excess buy orders are cancelled and buy orders in already opened trades are allowed.

Screenshots (if appropriate):

[uhliksk]

@chrisleekr Please, don't merge yet as there is still an issue with less traded symbols. As the executeTrailingTrade needs to be executed to cancel the excess buy orders, sometimes those orders are not cancelled on time and with just next price change they are executed on Binance right before the bot had a chance to remove them. I'm working on commit to fire executeTrailingTrade when needed on those buy orders which have to be cancelled before there are any websocket data received from Binance.

[chrisleekr]

@uhliksk Is this ready to review?

[uhliksk]

@chrisleekr Not yet. I was busy and I didn't finished the commit which will help to cancel the buy order even when there is no price change since the max. open trade limit is reached.

[uhliksk]

@habibalkhabbaz I'm modifying app/cronjob/trailingTrade/step/ensure-grid-trade-order-executed.js. I simply added const queue = require('../../trailingTradeHelper/queue'); and then later in code I used queue.executeFor(logger, symbol);, but it's throwing TypeError: queue.executeFor is not a function. Am I overlooking something?

[chrisleekr]

@uhliksk Ah, just to confirm, do you want to include this fix in the release? Then we can wait for the release.

[uhliksk]

@chrisleekr Yes, I was just waiting until #464 is merged to replace my simple queue to the new queueing mechanism. I thought it'll be simple function replace but I have an issue with queue.executeFor and I don't know what I'm doing wrong right now.

[habibalkhabbaz]

@habibalkhabbaz I'm modifying app/cronjob/trailingTrade/step/ensure-grid-trade-order-executed.js. I simply added const queue = require('../../trailingTradeHelper/queue'); and then later in code I used queue.executeFor(logger, symbol);, but it's throwing TypeError: queue.executeFor is not a function. Am I overlooking something?

Hello @uhliksk I couldn't see an issue with your code. Maybe you forgot to rebuild or so after migrating?

[uhliksk]

@habibalkhabbaz Sorry, I forgot to push last commit.

[uhliksk]

@habibalkhabbaz This is the line throwing the error.

https://github.com/chrisleekr/binance-trading-bot/blob/1fe9e7284a6564c79c9a214ec894a8caafcc27c8/app/cronjob/trailingTrade/step/ensure-grid-trade-order-executed.js#L262

[uhliksk]

@chrisleekr If it's not throwing the error for you, then you can merge, but for me it's logging this when buy order is filled:

{"name":"binance-api","version":"0.0.88","hostname":"2fb1d3018816","pid":46,"gitHash":"unspecified","level":50,"err":{"message":"p.executeFor is not a function","name":"TypeError","stack":"TypeError: p.executeFor is not a function\n at /srv/dist/server.js:1:32112\n at Array.map (<anonymous>)\n at execute (/srv/dist/server.js:1:32088)\n at async /srv/dist/server.js:1:20849\n at async errorHandlerWrapper (/srv/dist/server.js:1:100025)\n at async execute (/srv/dist/server.js:1:19521)"},"msg":"p.executeFor is not a function","time":"2022-08-14T08:41:25.672Z","v":0}

[habibalkhabbaz]

@chrisleekr If it's not throwing the error for you, then you can merge, but for me it's logging this when buy order is filled:

{"name":"binance-api","version":"0.0.88","hostname":"2fb1d3018816","pid":46,"gitHash":"unspecified","level":50,"err":{"message":"p.executeFor is not a function","name":"TypeError","stack":"TypeError: p.executeFor is not a function\n at /srv/dist/server.js:1:32112\n at Array.map (<anonymous>)\n at execute (/srv/dist/server.js:1:32088)\n at async /srv/dist/server.js:1:20849\n at async errorHandlerWrapper (/srv/dist/server.js:1:100025)\n at async execute (/srv/dist/server.js:1:19521)"},"msg":"p.executeFor is not a function","time":"2022-08-14T08:41:25.672Z","v":0}

@uhliksk It seems that the reported error not exist with the latest commit because I see the reported error for p.executeFor but your code is fine. I just added one comment.

[uhliksk]

@chrisleekr You're right, thank you. I replaced map with forEach and removed async. I'm waiting for buy order to be executed to see if the error will be logged again.

[habibalkhabbaz]

That's great @uhliksk If no more issues I think this PR will be ready to be reviewed by @chrisleekr Do you agree @uhliksk?

[uhliksk]

@habibalkhabbaz Maybe. It's still throwing p.executeFor is not a function to me, but I'm also getting another error in another part of code Error: Illegal characters found in parameter 'symbol'. Is it possible I have corrupted database? I'll check. If there is something corrupted then the error is probably problem in my environment only and the code itself is fine for review.

[uhliksk]

@habibalkhabbaz Another observation, the error is logged single time when buy order is executed, but forEach will run it multiple times for all symbols so it looks like just a single symbol is throwing the error. That's should proof it's just my local problem and the code is really good to go.

Edit: Actually I removed async so it looks like the error will be thrown single time because of this. I didn't found any corruption in database. Strange issue.

[uhliksk]

The Illegal characters found is from node_modules/binance-api-node/dist/http-client.js. It's not related. I'm sorry I'm throwing meaningless assumptions here. I'm just frustrated because I don't see why this p.executeFor error is occuring.

[habibalkhabbaz]

@uhliksk To be honest, I am not sure from where the issue is coming. Can you log symbols just before forEach to see what's happening?

[uhliksk]

@habibalkhabbaz

Before the forEach: {"name":"binance-api","version":"0.0.88","hostname":"b4e46d6ba733","pid":46,"gitHash":"unspecified","server":"binance","helper":"queue","jobName":"trailingTrade","correlationId":"df54aa42-8ae0-4e41-8e77-6143e80af09f","symbol":"ATOMBUSD","stepName":"ensure-grid-trade-order-executed","level":50,"symbols":["BTCBUSD","ETHBUSD","XRPBUSD","ADABUSD","SOLBUSD","DOGEBUSD","DOTBUSD","TRXBUSD","MATICBUSD","SHIBBUSD","AVAXBUSD","LTCBUSD","UNIBUSD","ETCBUSD","FTTBUSD","LINKBUSD","NEARBUSD","XMRBUSD","ATOMBUSD","XLMBUSD","BCHBUSD","ALGOBUSD","APEBUSD","FLOWBUSD","VETBUSD","ICPBUSD","MANABUSD","SANDBUSD","XTZBUSD","HBARBUSD","THETABUSD","EGLDBUSD","AXSBUSD","QNTBUSD","AAVEBUSD","EOSBUSD","HNTBUSD","MKRBUSD","ZECBUSD","IOTABUSD","FTMBUSD","RUNEBUSD","FILBUSD","GRTBUSD","CHZBUSD","KLAYBUSD","XECBUSD","NEOBUSD","LDOBUSD","CRVBUSD","WAVESBUSD","BATBUSD","STXBUSD","ENJBUSD","CAKEBUSD","ZILBUSD","PAXGBUSD","LRCBUSD","DASHBUSD","GMTBUSD"],"msg":">>> Ready to start queueing","time":"2022-08-14T12:55:28.119Z","v":0}

Inside the forEach:

{"name":"binance-api","version":"0.0.88","hostname":"b4e46d6ba733","pid":46,"gitHash":"unspecified","server":"binance","helper":"queue","jobName":"trailingTrade","correlationId":"df54aa42-8ae0-4e41-8e77-6143e80af09f","symbol":"ATOMBUSD","stepName":"ensure-grid-trade-order-executed","level":50,"symbols":["BTCBUSD","ETHBUSD","XRPBUSD","ADABUSD","SOLBUSD","DOGEBUSD","DOTBUSD","TRXBUSD","MATICBUSD","SHIBBUSD","AVAXBUSD","LTCBUSD","UNIBUSD","ETCBUSD","FTTBUSD","LINKBUSD","NEARBUSD","XMRBUSD","ATOMBUSD","XLMBUSD","BCHBUSD","ALGOBUSD","APEBUSD","FLOWBUSD","VETBUSD","ICPBUSD","MANABUSD","SANDBUSD","XTZBUSD","HBARBUSD","THETABUSD","EGLDBUSD","AXSBUSD","QNTBUSD","AAVEBUSD","EOSBUSD","HNTBUSD","MKRBUSD","ZECBUSD","IOTABUSD","FTMBUSD","RUNEBUSD","FILBUSD","GRTBUSD","CHZBUSD","KLAYBUSD","XECBUSD","NEOBUSD","LDOBUSD","CRVBUSD","WAVESBUSD","BATBUSD","STXBUSD","ENJBUSD","CAKEBUSD","ZILBUSD","PAXGBUSD","LRCBUSD","DASHBUSD","GMTBUSD"],"symbolToQueue":"BTCBUSD","msg":">>> Add to queue BTCBUSD","time":"2022-08-14T12:55:28.119Z","v":0}

Right after the logger there is the executeFor:

{"name":"binance-api","version":"0.0.88","hostname":"b4e46d6ba733","pid":46,"gitHash":"unspecified","server":"binance","helper":"queue","jobName":"trailingTrade","correlationId":"df54aa42-8ae0-4e41-8e77-6143e80af09f","symbol":"ATOMBUSD","level":50,"err":{"message":"p.executeFor is not a function","name":"TypeError","stack":"TypeError: p.executeFor is not a function\n at /srv/dist/server.js:1:32223\n at Array.forEach (<anonymous>)\n at execute (/srv/dist/server.js:1:32139)\n at async /srv/dist/server.js:1:20849\n at async errorHandlerWrapper (/srv/dist/server.js:1:100137)\n at async execute (/srv/dist/server.js:1:19521)"},"debug":true,"saveLog":true,"msg":"⚠ Execution failed.","time":"2022-08-14T12:55:28.119Z","v":0}

It just don't see the function at all no matter what the parameters are.

[uhliksk]

@habibalkhabbaz I even tried to log queue but I'm not sure if ít is supposed to return anything.

{"name":"binance-api","version":"0.0.88","hostname":"7581d55b7dcb","pid":46,"gitHash":"unspecified","server":"binance","helper":"queue","jobName":"trailingTrade","correlationId":"f43cdd2a-2988-41bd-a367-06f229962dbd","symbol":"SHIBBUSD","stepName":"ensure-grid-trade-order-executed","level":50,"queue":{},"msg":">>> What is queue?","time":"2022-08-14T13:03:37.139Z","v":0}

[uhliksk]

@habibalkhabbaz I tried to log moment to compare the result. If I compare the results there was empty array queue logged previously but in moment there is nothing logged at all. Does that mean the queue is initialized as empty array instead of object pointing to functions? Why require is not initializing it properly?

{"name":"binance-api","version":"0.0.88","hostname":"c34ea28efdd5","pid":46,"gitHash":"unspecified","server":"binance","helper":"queue","jobName":"trailingTrade","correlationId":"a6b0569a-8c55-42f1-a62f-89c281bd5044","symbol":"LDOBUSD","stepName":"ensure-grid-trade-order-executed","level":50,"msg":">>> What is moment?","time":"2022-08-14T13:10:55.081Z","v":0}

[habibalkhabbaz]

@uhliksk Okay, I think I found the issue. The queue cannot be referenced inside the trailing trade steps. It should be in anywhere else because the queue has reference to the trailing trade. As a solution, we may add a helper function in common file to add a symbol to queue from trailing trade steps.

However, what I prefer is, we can check the open orders and cancel them directly if we exceeded the max open orders instead of adding all symbols to the queue. It would be faster and we will have better performance.

What do you think?

[uhliksk]

@habibalkhabbaz Do you mean checking the open orders and cancel them in ensure-grid-trade-order-executed.js where I have executeFor right now?

[habibalkhabbaz]

@uhliksk You are right.

So, here is a draft of what I suggest.

After an order is executed it should do the following.

if (exceeded the max open trades) {
    // 1. Retrieve the open orders from cache
    // 2. Loop through them
    // 3. Cancel the order
}

By doing this it will call the queue automatically from the WebSocket updates.

Update: if we did the above, we can revert the change in the handle-open-orders as not needed any more

[uhliksk]

@habibalkhabbaz Ok, I'll rework the thing. Maybe I should also move the cancelOrder from handle-open-orders to common to not duplicate the same function here.

[habibalkhabbaz]

@uhliksk Yes. Let me know if you need any help in the implementation, I will be happy to work on this too.

[uhliksk]

@habibalkhabbaz Yes, you can rework the tests after moved cancelOrder to common if you like. Thank you.

[uhliksk]

@habibalkhabbaz You can actually move the cancelOrder by yourself as I can't commit the change with the tests failing.

[habibalkhabbaz]

@habibalkhabbaz You can actually move the cancelOrder by yourself as I can't commit the change with the tests failing.

Okay, let me work on this.

[habibalkhabbaz]

@uhliksk Done. I moved cancelOrder to common and I updated handle-open-orders accordingly.

[habibalkhabbaz]

@uhliksk I think you have to merge the last commit by @chrisleekr from the master branch.