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/kucoin query trading fees #6236

Merged
merged 16 commits into from May 9, 2023

Conversation

riven314
Copy link
Contributor

@riven314 riven314 commented Apr 18, 2023

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:
Fix failure to get trading fees when trading symbols are more than 10 in Kucoin exchange

Tests performed by the developer:

Tips for QA testing:

@riven314 riven314 changed the title fix key error when parsing trading fee response Fix/kucoin query trading fees Apr 18, 2023
rapcmia
rapcmia previously approved these changes Apr 19, 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.

LGTM

  • Created user script and crosscheck on development branch and able to reproduce the issue
  • Use the same script on this PR and not getting the error

test.py.log

@riven314
Copy link
Contributor Author

seems test coverage is not enough
any suggestion on how I can update the unit test to increase the coverage? @rapcmia

cardosofede
cardosofede previously approved these changes Apr 19, 2023
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
Copy link
Contributor

@riven314 the PR is good to go, but the coverage is not enough. To know what you can add I will recommend you the following:

  • make test
  • make development-diff-cover
    That will show you the lines of your changes that are not covered.
    Also you can use PyCharm to do it, if you have questions about it ping me on discord ;) and congrats for your first PR!!!
    @rapcmia we can't merge this pr until the coverage is met.

@rapcmia
Copy link
Contributor

rapcmia commented Apr 24, 2023

the PR is good to go, but the coverage is not enough. To know what you can add I will recommend you the following:

  • make test
  • make development-diff-cover
    That will show you the lines of your changes that are not covered.

Hi @riven314 Checking if you can have a look with the test coverage so we can complete this PR? thank you

@riven314
Copy link
Contributor Author

@rapcmia
Ok, I will add related test coverage this weekend

@nikspz
Copy link
Contributor

nikspz commented May 1, 2023

Hi @riven314 is there any updates on this PR? thanks

@nikspz
Copy link
Contributor

nikspz commented May 4, 2023

@riven314 Could you please ping me here if you're still working on it

@riven314
Copy link
Contributor Author

riven314 commented May 4, 2023

hey @nikspz @rapcmia

I need some help on how I should update the unit tests, got a bit stuck
coz I noticed there are already some unit tests covering self.exchange._update_trading_fees, for example:
https://github.com/hummingbot/hummingbot/blob/master/test/hummingbot/connector/exchange/kucoin/test_kucoin_exchange.py#L402

I am not sure why my changes are not covered (my change is mainly on _update_trading_fees method).

@nikspz
Copy link
Contributor

nikspz commented May 5, 2023

@riven314 You could try to run make test locally under the root hummingbot folder to see what lines need more coverage

@riven314
Copy link
Contributor Author

riven314 commented May 7, 2023

@nikspz @rapcmia
I have updated branch and it should pass coverage test now
my changes included a try catch for KeyError and I tried to add a unit test to cover the raise of KeyError
but for some reason it still was not covered, so I decided to remove the try-catch
this makes the coverage test pass

@riven314
Copy link
Contributor Author

riven314 commented May 9, 2023

@rapcmia @cardosofede
The build has passed. Could u help review again?

Copy link
Contributor

@nikspz nikspz left a comment

Choose a reason for hiding this comment

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

Test performed:

  • Cloned and installed fix branch
  • Connected kucoin
  • Created/started pureMM strategy using MATIC-USDT
  • Review filled order events
  • Review fees matched with the exchange
  • Used user script and crosschecked on development branch were able to reproduce the issue
  • Use the Ralph's script on this PR and not getting the error
  • Manually created docker image successfully

@nikspz nikspz merged commit a4613f4 into hummingbot:development May 9, 2023
2 checks passed
@nikspz
Copy link
Contributor

nikspz commented May 9, 2023

Merged the PR in behalf of foundation team to development and will be available on the next version 1.16.0

@riven314 riven314 deleted the fix/kucoin_query_trade_fee branch May 15, 2023 18:00
@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.

Kucoin - Failed to query trading fees when its running on more than 10 trading pairs
4 participants