search

DeviaVir / zenbot

Zenbot is a command-line cryptocurrency trading bot using Node.js and MongoDB.
MIT License
8.23k stars 2.04k forks source link

Simulation stopped working after #1971 #1983

Closed mmdiego closed 4 years ago

mmdiego commented 4 years ago

I've updated my local copy of zenbot after some months and simulation stopped working after this commit. So I checked out a fresh copy of zenbot and had the same results. If I rolled back to the commit just before #1971, which is ea7929f it starts working again. I'm testing it using: zenbot.js sim binance.BNB-USDT --strategy=macd --period=1m --days=1 Maybe it's something related with the exchange? I'm using node v8.3, but the same happens with v10.

jorisw commented 4 years ago

That’s really weird mmdiego, as I’m using the same exchange as you are, also used both Node 8.3 and 10, and the commit you referenced, I wrote in order to fix the problem.

I am running on that commit with a simulator that never stops.

I could only suggest debugging the portion of code that I made changes to in my PR. And I’d be curious to know how many other users chime in with this problem.

mmdiego commented 4 years ago

I'm trying to debug it, however I´m not an expert in Node. I've added many prints to see what was happening inside getNext(). I found that it's getting stucked after the second time it's called.

However, I also found that the behavior depends on the "limit" param. I've changed that value to 10 while debugging to simplify the execution. But then I also changed it to 5000 and it started working correctly! However, if I change the simulation to run on BTC-USDT, it started getting stucked again.

I will continue debbugging and I'll post my console output with the prints I've added, but in the meanwhile, can you try your simulation lowering down the limit param?

Thanks!

jorisw commented 4 years ago

That is very helpful info, mmdiego. I’ll look into it asap.

james-ingold commented 4 years ago

Having the same issue, but if I revert to a previous #1971 sim.js, then I get the issue that @jorisw was originally fixing. 😄 I think it will work if you just change this line (294 sim.js) =>
setImmediate(async () => await getNext()) to return getNext()

jorisw commented 4 years ago

I think it will work if you just change this line (294 sim.js)

Could you try that, @mmdiego ? I can't get my simulator to fail (yet), with limit 10. And I never quite understood the reason for the setImmediate call there, so I left it in.

mmdiego commented 4 years ago

Yes, the change proposed seems to fix the problem. It works even with different values of the "limit" param.

mmdiego commented 4 years ago

I didn't have enough time to test the fix, I've only checked that at least simulations are working. However, I noticed simulations run extremely slow compared to the previous code using setImmediate(). It was also weird that I got slightly different results running the same simulation, so I'm not sure yet if it's the right fix.