Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
magicaleks opened this issue Apr 14, 2024 · 7 comments
Open
Labels
invalid This doesn't seem right

Comments

@magicaleks
Copy link

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
Copy link
Contributor

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
Copy link
Contributor

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

@ramilexe
Copy link

Hi @panapol-p
I see function calculateProfit was refactored by @rodrigo-brito in this PR: #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
Copy link
Owner

rodrigo-brito commented May 19, 2024

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

@rodrigo-brito rodrigo-brito added the invalid This doesn't seem right label May 19, 2024
@ramilexe
Copy link

@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
Copy link
Owner

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
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalid This doesn't seem right
Projects
None yet
Development

No branches or pull requests

4 participants