Bybit: Fix WS ticker processing #1538

gbjk · 2024-05-10T05:33:48Z

Fix spot ticker not sent to DataHandler
Fix no ticker updates sent to ticker system
Fix Options Low using Last field
Move ticker.Price Bid/AskSize fields out of "Funding Rates" section - They apply to options
Unify ticker structures for WS, Rest and asset types; They don't conflict
Unify WS ticker processing across assets
Remove ExtractCurrencyPair from tests
Add Test Coverage
Some test refactors

Type of change

Bug fix (non-breaking change which fixes an issue)

How has this been tested

go test ./... -race
golangci-lint run
TestWsTicker

codecov · 2024-05-10T05:39:10Z

Codecov Report

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

Project coverage is 35.86%. Comparing base (095394f) to head (7dc2f14).
Report is 5 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1538      +/-   ##
==========================================
- Coverage   36.03%   35.86%   -0.18%     
==========================================
  Files         412      409       -3     
  Lines      177779   177282     -497     
==========================================
- Hits        64060    63577     -483     
+ Misses     105868   105864       -4     
+ Partials     7851     7841      -10

Files	Coverage Δ
currency/pair.go	`85.71% <100.00%> (+0.93%)`	⬆️
exchanges/bybit/bybit_types.go	`66.66% <ø> (ø)`
exchanges/sharedtestvalues/sharedtestvalues.go	`71.75% <100.00%> (-0.22%)`	⬇️
exchanges/ticker/ticker.go	`90.39% <ø> (ø)`
currency/manager.go	`94.30% <97.67%> (+0.87%)`	⬆️
exchanges/bybit/bybit_websocket.go	`46.33% <91.42%> (-0.48%)`	⬇️
exchanges/bybit/bybit_wrapper.go	`40.65% <43.75%> (-0.09%)`	⬇️

... and 28 files with indirect coverage changes

gloriousCode

Thanks for the improvements! I found an issue earlier than this change though

exchanges/bybit/bybit_websocket.go

gloriousCode · 2024-05-13T00:01:56Z

exchanges/bybit/testdata/wsTicker.json

+{"topic":"tickers.BTC-USDT","type":"snapshot","ts":233366401000,"cs":2588407389,"data":{"symbol":"BTCUSDT","lastPrice":"21109.77","highPrice24h":"21426.99","lowPrice24h":"20575","prevPrice24h":"20704.93","volume24h":"6780.866843","turnover24h":"141946527.22907118","price24hPcnt":"0.0196","usdIndexPrice":"21120.2400136"}}
+{"topic":"tickers.BTC_USDT","type":"snapshot","cs":24987956059,"ts":233366402000,"data":{"symbol":"BTCUSDT","tickDirection":"PlusTick","price24hPcnt":"0.017103","lastPrice":"17216.00","prevPrice24h":"16926.50","highPrice24h":"17281.50","lowPrice24h":"16915.00","prevPrice1h":"17238.00","markPrice":"17217.33","indexPrice":"17227.36","openInterest":"68744.761","openInterestValue":"1183601235.91","turnover24h":"1570383121.943499","volume24h":"91705.276","nextFundingTime":"1673280000000","fundingRate":"-0.000212","bid1Price":"17215.50","bid1Size":"84.489","ask1Price":"17216.00","ask1Size":"83.020"}}
+{"topic":"tickers.BTC-6JAN23-17500-C","type":"snapshot","ts":233366403000,"data":{"symbol":"BTC-USD-220930-28000-P","bidPrice":"7","bidSize":"4","bidIv":"0","askPrice":"11","askSize":"5.1","askIv":"0.514","lastPrice":"10","highPrice24h":"25","lowPrice24h":"5","markPrice":"7.86976724","indexPrice":"16823.73","markPriceIv":"0.4896","underlyingPrice":"16815.1","openInterest":"49.85","turnover24h":"446802.8473","volume24h":"26.55","totalVolume":"86","totalTurnover":"1437431","delta":"0.047831","gamma":"0.00021453","vega":"0.81351067","theta":"-19.9115368","predictedDeliveryPrice":"0","change24h":"-0.33333334"},"id":"tickers.BTC-6JAN23-17500-C-2480334983-1672917511074"}
+{"topic":"tickers.BTC_USDT","type":"delta","cs":24987956060,"ts":233366404000,"data":{"symbol":"BTC_USDT","tickDirection":"PlusTick","price24hPcnt":"0.017105","lastPrice":"17220.00","prevPrice24h":"17215.00","highPrice24h":"17304.99","lowPrice24h":"16910.01","prevPrice1h":"17244.00","markPrice":"17218.12","indexPrice":"17218.14","openInterest":"68750.881","openInterestValue":"1199601235.91","turnover24h":"1680383121.943499","volume24h":"92705.276","nextFundingTime":"1673290000000","fundingRate":"-0.000214","bid1Price":"17216.50","bid1Size":"80.289","ask1Price":"17220.04","ask1Size":"94.200"}}


Incoming data does not contain delimiters for this (I didnt grab an options update though). I'll go to the next comment to explain the issue

// spot {"topic":"tickers.DOGEUSDT","ts":1715558885609,"type":"snapshot","cs":38210900354,"data":{"symbol":"DOGEUSDT","lastPrice":"0.14159","highPrice24h":"0.14445","lowPrice24h":"0.13882","prevPrice24h":"0.1433","volume24h":"222597025.1","turnover24h":"31594648.839236","price24hPcnt":"-0.0119","usdIndexPrice":"0.141552"}} // linear contract snapshot {"topic":"tickers.BTCUSDT","type":"snapshot","data":{"symbol":"BTCUSDT","tickDirection":"ZeroMinusTick","price24hPcnt":"0.012012","lastPrice":"61500.00","prevPrice24h":"60770.00","highPrice24h":"61854.20","lowPrice24h":"60566.60","prevPrice1h":"61307.30","markPrice":"61500.00","indexPrice":"61519.69","openInterest":"59682.882","openInterestValue":"3670497243.00","turnover24h":"2559982229.7407","volume24h":"41822.6530","nextFundingTime":"1715558400000","fundingRate":"-0.00000694","bid1Price":"61500.00","bid1Size":"1.712","ask1Price":"61500.10","ask1Size":"18.074"},"cs":175430504644,"ts":1715557731033} // linear contract update {"topic":"tickers.BTCUSDT","type":"delta","data":{"symbol":"BTCUSDT","turnover24h":"2560051663.3536","volume24h":"41823.7820","ask1Price":"61500.10","ask1Size":"16.842"},"cs":175430511028,"ts":1715557732633}

Fixed - Grabbed real data.

gloriousCode · 2024-05-13T00:04:15Z

exchanges/bybit/bybit_websocket.go

+		return err
+	}
+
+	p, err := currency.NewPairFromString(tickResp.Symbol)


I think you should be using the matcher functions as incoming data does not contain delimiters. Take the following example:
As a regarded trader, I have DOGE_USDT enabled. NewPairFromString converts DOGEUSDT into DOG, EUSDT
Things still save, but you will not be able to retreive the data from RPC endpoints for example as it can't match anymore

Surprisingly, knowledge of this behaviour was in the removed function ExtractCurrencyPair, but it was not used in actual code 🤷 I understand that you did not do the initial implementation, but may as well update it now to a more appropriate function

As a regarded trader, I have DOGE_USDT enabled

😂 😭

but it was not used in actual code

Also 😂 😭

Yep, on it. I think I skipped running this properly, which is a really bad oversight tbh.

Fixed 29f2a5d and 720061c
Also had a few other fixes alongside, as well as fixing up the matcher being broken, so I pulled that in from #1534

Is this now dependent on 1534? I'll prioritise that first. Also please look at linter

Nope, Not dependent.
I pulled one commit from it, but it doesn't matter which PR that commit ends up in.

Just spotted the issue on that.
I had a DO NOT COMMIT marker on the commented section. Dunno where it went but that should have stopped me making that mistake of commenting out the DisableWebsocket calls 🤦 Maybe I need a DO NOT REMOVE marker above the DO NOT COMMIT marker 😂

Kucoin fixed 79216e0
Linter fixed 7dc2f14

gloriousCode · 2024-05-13T00:15:36Z

exchanges/bybit/bybit_test.go

-	}
-
-	// Options update processing
-	data = `{"symbol": "BTC-USD-220930-28000-P", "bidPrice": "0", "bidSize": "0", "bidIv": "0", "askPrice": "10", "askSize": "5.1", "askIv": "0.514", "lastPrice": "10", "highPrice24h": "25", "lowPrice24h": "5", "markPrice": "7.86976724", "indexPrice": "16823.73", "markPriceIv": "0.4896", "underlyingPrice": "16815.1", "openInterest": "49.85", "turnover24h": "446802.8473", "volume24h": "26.55", "totalVolume": "86", "totalTurnover": "1437431", "delta": "0.047831", "gamma": "0.00021453", "vega": "0.81351067", "theta": "-19.9115368", "predictedDeliveryPrice": "0", "change24h": "-0.33333334" }`


It was sorta nice that we were testing updates for each type instead of just USDTMarginedFutures BTCUSDT, but given that we don't have multi-websocket support it is optional for that

Sure we do?
Did you note assetRouting?
I was pretty chuffed with how the closure in TestFixtureToDataHandler was versatile to allow for that.

So yeah. Also on it.

I did see that. I think I was looking too hard at the json file when I made the comment

Fixed 720061c

Everything is tested now, with real data.

gloriousCode

Thanks for the updates, it all looks swell. Just a linter fix regarding 32bit

gloriousCode · 2024-05-24T00:25:11Z

exchanges/bybit/bybit_test.go

-				assert.Equal(t, 11.00, v.Ask, "Ask should be correct")
-				assert.Equal(t, 5.10, v.AskSize, "AskSize should be correct")
-				assert.Equal(t, "BTC-USD-220930-28000-P", v.Pair.String(), "Pair should be correct")
+				assert.EqualValues(t, 233366401000, v.LastUpdated.UnixMilli(), "LastUpdated should be correct")


looks like you'll need to wrap with int64(...)

Bybit: Fix WS ticker processing

da63ab0

gbjk added bug review me This pull request is ready for review labels May 10, 2024

gbjk requested review from thrasher-, gloriousCode and shazbert May 10, 2024 05:33

gbjk self-assigned this May 10, 2024

gloriousCode requested changes May 13, 2024

View reviewed changes

gbjk added 8 commits May 13, 2024 13:33

fixup! Bybit: Fix WS ticker processing

859bfa5

fixup! Bybit: Fix WS ticker processing

b133fb9

Bybit: Refactor SetDefaults

fb2b06c

Currency: Fix PairsManager.Load breaking matcher

944f0dd

fixup! Bybit: Fix WS ticker processing

29f2a5d

fixup! fixup! Bybit: Fix WS ticker processing

39f7d22

fixup! Bybit: Refactor SetDefaults

7ffea29

fixup! Bybit: Fix WS ticker processing

720061c

gbjk requested a review from gloriousCode May 15, 2024 07:39

gbjk added 2 commits May 16, 2024 08:24

Kucoin: Unify test instance usage

79216e0

fixup! fixup! Bybit: Refactor SetDefaults

7dc2f14

gloriousCode requested changes May 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bybit: Fix WS ticker processing #1538

Bybit: Fix WS ticker processing #1538

gbjk commented May 10, 2024

codecov bot commented May 10, 2024 •

edited

gloriousCode left a comment

gloriousCode May 13, 2024 •

edited

gbjk May 15, 2024

gloriousCode May 13, 2024 •

edited

gbjk May 13, 2024

gbjk May 15, 2024

gloriousCode May 15, 2024 •

edited

gbjk May 16, 2024

gbjk May 16, 2024

gloriousCode May 13, 2024 •

edited

gbjk May 13, 2024

gloriousCode May 13, 2024

gbjk May 15, 2024

gloriousCode left a comment

gloriousCode May 24, 2024

Bybit: Fix WS ticker processing #1538

Are you sure you want to change the base?

Bybit: Fix WS ticker processing #1538

Conversation

gbjk commented May 10, 2024

Type of change

How has this been tested

codecov bot commented May 10, 2024 • edited

Codecov Report

gloriousCode left a comment

Choose a reason for hiding this comment

gloriousCode May 13, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gloriousCode May 13, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gloriousCode May 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gloriousCode May 13, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gloriousCode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 10, 2024 •

edited

gloriousCode May 13, 2024 •

edited

gloriousCode May 13, 2024 •

edited

gloriousCode May 15, 2024 •

edited

gloriousCode May 13, 2024 •

edited