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

fix/trade fill CSV export columns #5078

Merged

Conversation

aarmoa
Copy link
Contributor

@aarmoa aarmoa commented Jan 31, 2022

Before submitting this PR, please make sure:

  • Your code builds clean without any errors or warnings
  • You are using approved title ("feat/", "fix/", "docs/", "refactor/")

A description of the changes proposed in the pull request:
PR created to solve the issue #5057
Change in the markets recorder to use a fixed list of fields from the TradeFill object to include in the export file, instead of determining it dynamically based on the content of each trade fill.

Tests performed by the developer:
Added unit tests to check the list of fields to export is the expected one, and that the fields' content can be accessed without problems.
Manually tested by running a PMM strategy connected with Binance.

Tests performed:

  • Setup pure market making bots running long term and confirmed only one CSV file for each running bot is created for trades that are executed
  • Also did the same test locally on paper trade exchanges with the same result
  • Checked the CSV file and confirmed that it matches with the trades executed on the client and exchange
  • Tried to reproduce the issue using the steps provided on the issue ticket and wasn't able to replicate during testing

Tips for QA testing:
The scenario can be reproduced following the description of the original issue.

This PR comes from CoinAlpha#68

Fixes #5057

… trade fill export to CSV file. Now the list of columns is fixed instead of dinamic
@dennisocana
Copy link
Contributor

@rapcmia
Copy link
Contributor

rapcmia commented Feb 14, 2022

PR update: on-going test

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.

Tested and confirmed working by QA
Test performed:

  • Setup long term testbots running on pure market making and only one CSV created for trades
  • Setup local testbot and ran test each trade event (disconnecting to network, changing market and forced close bot), only one CSV created
  • Tested also on other strategies, all ok
  • Trades from the bot and CSV file matched

@RobHBOT RobHBOT merged commit 58302fe into hummingbot:development Feb 15, 2022
@aarmoa aarmoa deleted the fix/trade_fill_CSV_export_columns branch March 17, 2022 12:25
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

5 participants