thrasher-corp / gocryptotrader

A cryptocurrency trading bot and framework supporting multiple exchanges written in Golang.
MIT License
2.96k stars 792 forks source link

Kraken: Fix GetLatestFundingRates requiring pair enabled #1522

Closed gbjk closed 3 months ago

gbjk commented 3 months ago

APIs like this should not require the pair to be enabled. Fixes intermittent test failure on cmd/exchange_wrapper_standards:

exchange_wrapper_standards_test.go:227: Kraken Func 'GetLatestFundingRates' Error: 'pair not enabled FF-TEST-DELIM-solusd_240628'. Inputs: [context.Background {futures FF-TEST-DELIM-solusd_240628 true}].

Type of change

How has this been tested

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 85.71429% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 36.06%. Comparing base (7b842c2) to head (f91b833). Report is 1 commits behind head on master.

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/thrasher-corp/gocryptotrader/pull/1522/graphs/tree.svg?width=650&height=150&src=pr&token=41784B23TS&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=thrasher-corp)](https://app.codecov.io/gh/thrasher-corp/gocryptotrader/pull/1522?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=thrasher-corp) ```diff @@ Coverage Diff @@ ## master #1522 +/- ## ========================================== + Coverage 36.04% 36.06% +0.02% ========================================== Files 411 411 Lines 176906 176922 +16 ========================================== + Hits 63765 63807 +42 + Misses 105356 105333 -23 + Partials 7785 7782 -3 ``` | [Files](https://app.codecov.io/gh/thrasher-corp/gocryptotrader/pull/1522?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=thrasher-corp) | Coverage Δ | | |---|---|---| | [currency/manager.go](https://app.codecov.io/gh/thrasher-corp/gocryptotrader/pull/1522?src=pr&el=tree&filepath=currency%2Fmanager.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=thrasher-corp#diff-Y3VycmVuY3kvbWFuYWdlci5nbw==) | `93.43% <100.00%> (+0.30%)` | :arrow_up: | | [exchanges/kraken/kraken\_wrapper.go](https://app.codecov.io/gh/thrasher-corp/gocryptotrader/pull/1522?src=pr&el=tree&filepath=exchanges%2Fkraken%2Fkraken_wrapper.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=thrasher-corp#diff-ZXhjaGFuZ2VzL2tyYWtlbi9rcmFrZW5fd3JhcHBlci5nbw==) | `40.90% <0.00%> (-0.02%)` | :arrow_down: | ... and [13 files with indirect coverage changes](https://app.codecov.io/gh/thrasher-corp/gocryptotrader/pull/1522/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=thrasher-corp)
gloriousCode commented 3 months ago

I like your new helper function!

APIs like this should not require the pair to be enabled

Wrapper functions tend to respect enabled pairs over available pairs. This has had some back and forth over a long time. To that end, if you were to comply what you're saying, you are after a considerable change to many implementations across many exchanges.

So with current approach applied, it would initially appear as though exchange_wrapper_standards has an intermittent issue with enabling pairs. However, upon inspection it's because I did not add a flag to test the endpoint against all asset types. It only tested against one asset type, which may not be one which is supported for funding rates.

As for the reason it failed when it tested the correct asset type, it is because the developer (me) did the inappropriate implementation of not formatting a pair before calling String() on it, so -TEST-DELIM- remained, which is not an available pair within Kraken and so it fails the test.

So in that sense I've made a diff of the changes to apply for fixing exchange_wrapper_standards handling of GetLatestFundingRates and bow my head in shame for the double test bugs introduced

gbjk commented 3 months ago

if you were to comply what you're saying, you are after a considerable change to many implementations across many exchanges.

Agreed. This has come up a few times with @thrasher-.

The direction I'm following:

So on this task, I'll add your fix, but keep the addition to allow FundingRates on Available pairs