Kucoin: Implement GetFuturesTickers, update UpdateTickers and enhance test coverage #1431

Beadko · 2023-12-24T06:24:17Z

PR Description

Kucoin UpdateTickers returning incorrect values on futures.
Create the test to prove the concept.
Create a function GetFuturesTickers as the data is not available via the API call.
Expand the testing.
Fixes # (issue)

Type of change

Please delete options that are not relevant and add an x in [] as item is complete.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How has this been tested

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration and
also consider improving test coverage whilst working on a certain feature or package.

go test ./... -race
golangci-lint run
TestUpdateTickers TestGetFuturesTickers

Checklist

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation and regenerated documentation via the documentation tool
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally and on Github Actions/AppVeyor with my changes
Any dependent changes have been merged and published in downstream modules

shazbert

Thanks for opening this PR! This looks great. Just some minor things I came across.

exchanges/kucoin/kucoin_wrapper.go

shazbert

Thanks for making those changes. tACK! 💃

thrasher-

Thanks @Beadko , the new endpoint and tests look great! A nit wrt to efficiency however

exchanges/kucoin/kucoin_wrapper.go

…airNotFound, return err

codecov · 2024-01-22T09:13:23Z

Codecov Report

Attention: 16 lines in your changes are missing coverage. Please review.

Comparison is base (d907aab) 43.35% compared to head (f211919) 43.82%.
Report is 11 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1431      +/-   ##
==========================================
+ Coverage   43.35%   43.82%   +0.46%     
==========================================
  Files         366      363       -3     
  Lines      147087   143963    -3124     
==========================================
- Hits        63774    63086     -688     
+ Misses      75489    73288    -2201     
+ Partials     7824     7589     -235

Files	Coverage Δ
exchanges/kucoin/kucoin_wrapper.go	`34.90% <75.00%> (+1.17%)`	⬆️
exchanges/kucoin/kucoin_futures.go	`23.39% <69.38%> (+3.72%)`	⬆️

... and 74 files with indirect coverage changes

gloriousCode

Thanks for opening this PR, however I'm not sure if it fits right now

exchanges/kucoin/kucoin_test.go

exchanges/kucoin/kucoin_wrapper.go

exchanges/kucoin/kucoin_test.go

thrasher-

Thanks for making those changes! Have some more feedback

exchanges/kucoin/kucoin_wrapper.go

gloriousCode

Latest builds have a data race:

==================
WARNING: DATA RACE
Write at 0x00c000bfad30 by goroutine 452:
  github.com/thrasher-corp/gocryptotrader/exchanges/kucoin.(*Kucoin).GetFuturesTickers.func1()
      /home/runner/work/gocryptotrader/gocryptotrader/exchanges/kucoin/kucoin_wrapper.go:388 +0x2d1

Previous write at 0x00c000bfad30 by goroutine 453:
  github.com/thrasher-corp/gocryptotrader/exchanges/kucoin.(*Kucoin).GetFuturesTickers.func1()
      /home/runner/work/gocryptotrader/gocryptotrader/exchanges/kucoin/kucoin_wrapper.go:388 +0x2d1

Goroutine 452 (running) created at:
  github.com/thrasher-corp/gocryptotrader/exchanges/kucoin.(*Kucoin).GetFuturesTickers()
      /home/runner/work/gocryptotrader/gocryptotrader/exchanges/kucoin/kucoin_wrapper.go:384 +0x238
  github.com/thrasher-corp/gocryptotrader/exchanges/kucoin.TestGetFuturesTickers()
      /home/runner/work/gocryptotrader/gocryptotrader/exchanges/kucoin/kucoin_test.go:120 +0x64
  testing.tRunner()
      /opt/hostedtoolcache/go/1.21.6/x64/src/testing/testing.go:1595 +0x261
  testing.(*T).Run.func1()
      /opt/hostedtoolcache/go/1.21.6/x64/src/testing/testing.go:1648 +0x44

Goroutine 453 (finished) created at:
  github.com/thrasher-corp/gocryptotrader/exchanges/kucoin.(*Kucoin).GetFuturesTickers()
      /home/runner/work/gocryptotrader/gocryptotrader/exchanges/kucoin/kucoin_wrapper.go:384 +0x238
  github.com/thrasher-corp/gocryptotrader/exchanges/kucoin.TestGetFuturesTickers()
      /home/runner/work/gocryptotrader/gocryptotrader/exchanges/kucoin/kucoin_test.go:120 +0x64
  testing.tRunner()
      /opt/hostedtoolcache/go/1.21.6/x64/src/testing/testing.go:1595 +0x261
  testing.(*T).Run.func1()
      /opt/hostedtoolcache/go/1.21.6/x64/src/testing/testing.go:1648 +0x44
==================
--- FAIL: TestGetFuturesTickers (0.29s)
    testing.go:1465: race detected during execution of test
--- FAIL: TestGetTicker (0.18s)
    testing.go:1465: race detected during execution of test
--- FAIL: TestGetFuturesCurrentFundingRate (0.31s)
    testing.go:1465: race detected during execution of test
--- FAIL: TestUpdateTickers (0.97s)
    testing.go:1465: race detected during execution of test
FAIL
coverage: 38.8% of statements

Beadko · 2024-01-29T09:58:00Z

Amended the error to fix the race issue

gloriousCode · 2024-01-29T21:54:20Z

Unfortunately there is still racing issues: https://github.com/thrasher-corp/gocryptotrader/actions/runs/7694212313/job/20964544279?pr=1431

thrasher- · 2024-01-30T00:12:31Z

That second race looks to be impacting master and not related to this PR as I just noticed it on a deps upgrade: https://github.com/thrasher-corp/gocryptotrader/actions/runs/7691800287/job/20957671281#step:9:394

thrasher-

Just one nit and then this looks good to go

exchanges/kucoin/kucoin_wrapper.go

thrasher-

Thank you for making those changes!

shazbert

Good stuff 2 minor things.

shazbert · 2024-02-01T02:20:02Z

exchanges/kucoin/kucoin_wrapper.go

+			if tick, err2 := ku.GetFuturesTicker(ctx, p.String()); err2 != nil {
+				errC <- err2
+			} else {
+				tickersC <- &ticker.Price{


Bid size and ask size not set

diff --git a/exchanges/kucoin/kucoin_wrapper.go b/exchanges/kucoin/kucoin_wrapper.go index 0ac27b210..8aa921e70 100644 --- a/exchanges/kucoin/kucoin_wrapper.go +++ b/exchanges/kucoin/kucoin_wrapper.go @@ -375,6 +375,8 @@ func (ku *Kucoin) GetFuturesTickers(ctx context.Context) ([]*ticker.Price, error Last: tick.Price.Float64(), Bid: tick.BestBidPrice.Float64(), Ask: tick.BestAskPrice.Float64(), + AskSize: tick.BestAskSize, + BidSize: tick.BestBidSize, Volume: tick.Size, Pair: p, LastUpdated: tick.FilledTime.Time(),

shazbert · 2024-02-01T02:47:21Z

exchanges/kucoin/kucoin_wrapper.go

@@ -347,6 +347,62 @@ func (ku *Kucoin) UpdateTradablePairs(ctx context.Context, forceUpdate bool) err
 	return nil
 }

+// GetFuturesTickers does n * REST requests based on enabled pairs of the futures asset type
+func (ku *Kucoin) GetFuturesTickers(ctx context.Context) ([]*ticker.Price, error) {


Suggestion: shift to kucoin_futures.go as is not technically mapped to exchange.IBotInterfaceface

gloriousCode

tACK! Thanks for making those changes!

gloriousCode · 2024-02-02T04:55:45Z

exchanges/kucoin/kucoin_futures.go

+		go func() {
+			defer wg.Done()
+
+			if tick, err2 := ku.GetFuturesTicker(ctx, p.String()); err2 != nil {


Something I noticed that's outside of scope, but definitely in your interest is that the function ku.GetFuturesTicker now has a rate limit weight of 2, versus it using the default of 1. Given that this is a request per ticker, this could impact you.

On further inspection I think this is impacting a few Kucoin Futures endpoints and the numbers seem a bit whack, so I'm fine with continuing on, with the idea to relook at this in future

thrasher-

re-tACK, thanks @Beadko !

thrasher- added the review me This pull request is ready for review label Dec 26, 2023

thrasher- requested review from gloriousCode, shazbert and thrasher- and removed request for shazbert December 26, 2023 22:39

thrasher- assigned Beadko Dec 26, 2023

shazbert requested changes Dec 27, 2023

View reviewed changes

Beadko requested a review from shazbert December 27, 2023 07:58

shazbert approved these changes Dec 27, 2023

View reviewed changes

shazbert added the szrc shazbert review complete label Dec 27, 2023

thrasher- changed the title ~~Bugfix/kucoin update tickers futures~~ Kucoin: Implement GetFuturesTickers, update UpdateTickers and enhance test coverage Jan 16, 2024

thrasher- reviewed Jan 16, 2024

View reviewed changes

exchanges/kucoin/kucoin_wrapper.go Outdated Show resolved Hide resolved

Beadko added 4 commits January 22, 2024 15:58

Kucoin: update TestUpdateTickers, create GetFuturesTickers

1e2062b

Kucoin: Update UpdateTickers, getFuturesTickers and testing

3319398

Kucoin: Amend the GetFuturesTickers description, remove currency.ErrP…

b1d8548

…airNotFound, return err

Kucoin: Parallelilise GetFuturesTickers

9819129

Beadko force-pushed the bugfix/kucoin_update_tickers_futures branch from 0e77547 to 9819129 Compare January 22, 2024 09:03

gloriousCode requested changes Jan 22, 2024

View reviewed changes

exchanges/kucoin/kucoin_test.go Outdated Show resolved Hide resolved

exchanges/kucoin/kucoin_wrapper.go Outdated Show resolved Hide resolved

exchanges/kucoin/kucoin_test.go Outdated Show resolved Hide resolved

Kucoin: Revert to GetFuturesOpenContracts in GetTickers

0d83d05

thrasher- requested changes Jan 25, 2024

View reviewed changes

exchanges/kucoin/kucoin_wrapper.go Outdated Show resolved Hide resolved

exchanges/kucoin/kucoin_wrapper.go Outdated Show resolved Hide resolved

Kucoin: Append the errors, fixup linter

cadd396

Beadko force-pushed the bugfix/kucoin_update_tickers_futures branch from 0539eb8 to cadd396 Compare January 28, 2024 03:07

Beadko requested review from thrasher- and gloriousCode January 28, 2024 10:21

gloriousCode requested changes Jan 28, 2024

View reviewed changes

shazbert removed the szrc shazbert review complete label Jan 29, 2024

Kucoin: Fix the race warning

de1da8d

Beadko requested a review from gloriousCode January 29, 2024 09:58

thrasher- requested changes Jan 30, 2024

View reviewed changes

exchanges/kucoin/kucoin_wrapper.go Outdated Show resolved Hide resolved

Kucoin: Fixup

8d57a77

Beadko requested a review from thrasher- January 30, 2024 12:36

thrasher- approved these changes Feb 1, 2024

View reviewed changes

shazbert requested changes Feb 1, 2024

View reviewed changes

Kucoin: Move GetFuturesTickers to kucoin_futures

f211919

gloriousCode approved these changes Feb 2, 2024

View reviewed changes

thrasher- approved these changes Feb 2, 2024

View reviewed changes

thrasher- merged commit e16ee53 into thrasher-corp:master Feb 2, 2024
9 of 12 checks passed

Beadko deleted the bugfix/kucoin_update_tickers_futures branch February 3, 2024 07:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kucoin: Implement GetFuturesTickers, update UpdateTickers and enhance test coverage #1431

Kucoin: Implement GetFuturesTickers, update UpdateTickers and enhance test coverage #1431

Beadko commented Dec 24, 2023

shazbert left a comment

shazbert left a comment

thrasher- left a comment

codecov bot commented Jan 22, 2024 •

edited

Loading

gloriousCode left a comment

thrasher- left a comment

gloriousCode left a comment

Beadko commented Jan 29, 2024

gloriousCode commented Jan 29, 2024

thrasher- commented Jan 30, 2024

thrasher- left a comment

thrasher- left a comment

shazbert left a comment

shazbert Feb 1, 2024

shazbert Feb 1, 2024

shazbert Feb 1, 2024

gloriousCode left a comment

gloriousCode Feb 2, 2024

thrasher- left a comment

Kucoin: Implement GetFuturesTickers, update UpdateTickers and enhance test coverage #1431

Kucoin: Implement GetFuturesTickers, update UpdateTickers and enhance test coverage #1431

Conversation

Beadko commented Dec 24, 2023

PR Description

Type of change

How has this been tested

Checklist

shazbert left a comment

Choose a reason for hiding this comment

shazbert left a comment

Choose a reason for hiding this comment

thrasher- left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 22, 2024 • edited Loading

Codecov Report

gloriousCode left a comment

Choose a reason for hiding this comment

thrasher- left a comment

Choose a reason for hiding this comment

gloriousCode left a comment

Choose a reason for hiding this comment

Beadko commented Jan 29, 2024

gloriousCode commented Jan 29, 2024

thrasher- commented Jan 30, 2024

thrasher- left a comment

Choose a reason for hiding this comment

thrasher- left a comment

Choose a reason for hiding this comment

shazbert left a comment

Choose a reason for hiding this comment

shazbert Feb 1, 2024

Choose a reason for hiding this comment

shazbert Feb 1, 2024

Choose a reason for hiding this comment

shazbert Feb 1, 2024

Choose a reason for hiding this comment

gloriousCode left a comment

Choose a reason for hiding this comment

gloriousCode Feb 2, 2024

Choose a reason for hiding this comment

thrasher- left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 22, 2024 •

edited

Loading