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

Bybit: Fix WS ticker processing #1538

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

gbjk
Copy link
Collaborator

@gbjk gbjk commented May 10, 2024

  • 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

@gbjk gbjk added bug review me This pull request is ready for review labels May 10, 2024
@gbjk gbjk self-assigned this May 10, 2024
Copy link

codecov bot commented May 10, 2024

Codecov Report

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

Project coverage is 35.86%. Comparing base (ca9efe7) to head (7dc2f14).
Report is 9 commits behind head on master.

Current head 7dc2f14 differs from pull request most recent head 64977cc

Please upload reports for the commit 64977cc to get more accurate results.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1538      +/-   ##
==========================================
- Coverage   37.76%   35.86%   -1.91%     
==========================================
  Files         409      409              
  Lines      147697   177282   +29585     
==========================================
+ Hits        55780    63577    +7797     
- Misses      84055   105864   +21809     
+ Partials     7862     7841      -21     
Files Coverage Δ
currency/pair.go 85.71% <100.00%> (-1.20%) ⬇️
exchanges/bybit/bybit_types.go 66.66% <ø> (-6.67%) ⬇️
exchanges/sharedtestvalues/sharedtestvalues.go 71.75% <100.00%> (+1.51%) ⬆️
exchanges/ticker/ticker.go 90.39% <ø> (-1.82%) ⬇️
currency/manager.go 94.30% <97.67%> (-0.66%) ⬇️
exchanges/bybit/bybit_websocket.go 46.33% <91.42%> (-4.47%) ⬇️
exchanges/bybit/bybit_wrapper.go 40.65% <43.75%> (-3.80%) ⬇️

... and 380 files with indirect coverage changes

Copy link
Collaborator

@gloriousCode gloriousCode left a comment

Choose a reason for hiding this comment

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

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

exchanges/bybit/bybit_websocket.go Show resolved Hide resolved
exchanges/bybit/bybit_websocket.go Show resolved Hide resolved
exchanges/bybit/testdata/wsTicker.json Outdated Show resolved Hide resolved
exchanges/bybit/bybit_websocket.go Outdated Show resolved Hide resolved
exchanges/bybit/bybit_test.go Show resolved Hide resolved
@gbjk gbjk requested a review from gloriousCode May 15, 2024 07:39
Copy link
Collaborator

@gloriousCode gloriousCode left a comment

Choose a reason for hiding this comment

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

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

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")
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep. That's EqualValues out the window for most use cases then 🪟

Fixed 2639b11

@gbjk gbjk requested a review from gloriousCode May 27, 2024 00:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug high priority review me This pull request is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants