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

Include impact of deducted fees in the performance report balances #6095

Merged
merged 7 commits into from May 22, 2023

Conversation

keithbaum
Copy link
Contributor

@keithbaum keithbaum commented Feb 17, 2023

Performance report accounts for initial balances based on the TradeFills from the current state backwards to the initial state.
Issue appears when fees are deducted from returns because the fee asset is not adjusted accordingly.
In this PR I am including an additional method to account for this impact and also allowing negative fees to be included when accounting balances

PS: Also changed minimally binance candles feed tests to improve compatibility with numpy libs

@fengtality
Copy link
Sponsor Contributor

@carlitogetaladajr please nominate this issue for next Priority Issues poll

@cardosofede
Copy link
Contributor

@fengtality @carlitogetaladajr I think that this bounty can be a good task for @MementoRC that is very good with unit test

rapcmia
rapcmia previously approved these changes May 15, 2023
Copy link
Contributor

@rapcmia rapcmia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Crosscheck with Development branch
  • Compare trade data between client, csv and exchange, all ok
  • Compare if there are any change on CSV, all ok
  • Build docker client, all ok

@rapcmia
Copy link
Contributor

rapcmia commented May 15, 2023

hi @keithbaum can you please check the failing tests

image

@rapcmia
Copy link
Contributor

rapcmia commented May 17, 2023

Hi @keithbaum good day, just checking if you can fix the failed GitHub test? thank you

@keithbaum
Copy link
Contributor Author

hi @keithbaum can you please check the failing tests

image

Done

Copy link
Contributor

@rapcmia rapcmia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

  • Compare with development branch:
    • Trade data matched client history, CSV and exchange trade history
  • Check changes on candles feed by running candles_example script ✅
  • Build docker image, all ok

Copy link
Contributor

@cardosofede cardosofede left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@cardosofede cardosofede merged commit f6e9dc5 into hummingbot:development May 22, 2023
3 checks passed
@nikspz nikspz mentioned this pull request May 22, 2023
2 tasks
@rapcmia rapcmia mentioned this pull request May 23, 2023
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants