rodrigo-brito / ninjabot

A fast trading bot platform for cryptocurrency in Go (Binance)
https://rodrigo-brito.github.io/ninjabot/
MIT License
1.36k stars 172 forks source link

`controller.calculateProfit` doesn't work properly with sqlite database #321

Open magicaleks opened 2 months ago

magicaleks commented 2 months ago

In this test replace

require.NoError(t, err)

with

require.NoError(t, err)
defer func() {
    os.RemoveAll(file.Name())
}()

storage, err := storage.FromSQL(sqlite.Open(file.Name()), &gorm.Config{})
require.NoError(t, err)

and run this test. You will see the following error: image

panapol-p commented 2 months ago

Hi @magicaleks, thank you for your report. I tried testing the controller using SQL storage, and it seems to be working normally. Perhaps the error stems from your file creation process.

In sqlite package, you can specify a filename instead of manually creating the file. It will be created if it doesn't already exist.

image
panapol-p commented 2 months ago

FYI: calculateProfit was updated to updatePosition nine months ago. Please ensure you're using the latest version.

ramilexe commented 1 month ago

Hi @panapol-p I see function calculateProfit was refactored by @rodrigo-brito in this PR: https://github.com/rodrigo-brito/ninjabot/pull/276 And now it uses map position map[string]*Position. But this will not work if we restart bot when we have open position.

Does it make sense to derive positions from persistence database?

rodrigo-brito commented 1 month ago

Hi @magicaleks Is expected to fail if you drop the database. In the current version, we do not load the position info from the storage in case of restart. In your test, are you dropping the database? I think this is something wrong, it is not an expected behavior

ramilexe commented 1 month ago

@rodrigo-brito the issue was tested on old version of ninjabot when we tried to use sql database with old implementation of calculateProfit method. It is no longer reproduced on latest version but, it seems there is an issue when we restart the bot when we have an open position

rodrigo-brito commented 1 month ago

sure, it is something that requires improvements. Currently, we do no support load open positions after a restart, we do not store the state of avg price. It is something that is not too much complex to deal, I will try to work on this feature.

ramilexe commented 1 month ago

It is something that is not too much complex to deal, I will try to work on this feature.

It would be really helpful. If you don't mind, could you describe how would you do it? Maybe I can implement it myself

rodrigo-brito commented 1 week ago

The main change is: When the bot initialize, it executes a query in the local database looking for open positions, calculate the average price for each pair and store it in memory (controller)