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

Deribit: Add Subscription Configuration #1636

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

Conversation

gbjk
Copy link
Collaborator

@gbjk gbjk commented Aug 30, 2024

Deribit

  • Add subscription configuration
  • Consolidate subscription requests
  • Fix error on authenticated subscription for orderbook

Misc

  • Fix kline.Raw marshalling

Stacked Dependencies

Type of change

  • New feature (non-breaking change which adds functionality)

@gbjk gbjk self-assigned this Aug 30, 2024
Copy link

codecov bot commented Aug 30, 2024

Codecov Report

Attention: Patch coverage is 64.34783% with 41 lines in your changes missing coverage. Please review.

Project coverage is 36.53%. Comparing base (649f3df) to head (3bb37fe).

Files with missing lines Patch % Lines
exchanges/deribit/deribit_websocket.go 46.75% 41 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1636      +/-   ##
==========================================
+ Coverage   36.50%   36.53%   +0.02%     
==========================================
  Files         414      414              
  Lines      179351   179087     -264     
==========================================
- Hits        65472    65422      -50     
+ Misses     106004   105791     -213     
+ Partials     7875     7874       -1     
Files with missing lines Coverage Δ
exchanges/deribit/deribit.go 46.03% <100.00%> (-0.03%) ⬇️
exchanges/deribit/deribit_wrapper.go 41.71% <100.00%> (+0.22%) ⬆️
exchanges/deribit/deribit_ws_endpoints.go 45.38% <ø> (ø)
exchanges/exchange.go 76.56% <100.00%> (-0.08%) ⬇️
exchanges/kline/kline.go 90.96% <100.00%> (+0.12%) ⬆️
exchanges/kucoin/kucoin_websocket.go 55.33% <100.00%> (-0.18%) ⬇️
exchanges/subscription/list.go 100.00% <100.00%> (ø)
exchanges/subscription/subscription.go 100.00% <ø> (ø)
exchanges/deribit/deribit_websocket.go 46.87% <46.75%> (+8.07%) ⬆️

... and 11 files with indirect coverage changes

@gbjk gbjk force-pushed the feature/deribit_sub_conf branch 4 times, most recently from 7a6cee6 to 3bb37fe Compare August 30, 2024 07:19
@gbjk gbjk added the review me This pull request is ready for review label Aug 30, 2024
Since I had to ask what this abbreviation meant, I think we should
abandon it
Calling Setup twice would race on the assignment to this package var.

There was an option to just move the assignment to the package var declaration, but this
change improves the performance and allocations:
```
BenchmarkOptionPairToString-8            1000000              1239 ns/op             485 B/op         10 allocs/op
BenchmarkOptionPairToString2-8           3473804               656.2 ns/op           348 B/op          7 allocs/op
```

I've also removed the t.Run because even success the -v output from
tests would be very noisy, and I don't think we were getting any benefit
from it at all:
```
=== RUN   TestOptionPairToString
=== PAUSE TestOptionPairToString
=== CONT  TestOptionPairToString
=== RUN   TestOptionPairToString/BTC-30MAY24-61000-C
=== PAUSE TestOptionPairToString/BTC-30MAY24-61000-C
=== RUN   TestOptionPairToString/ETH-1JUN24-3200-P
=== PAUSE TestOptionPairToString/ETH-1JUN24-3200-P
=== RUN   TestOptionPairToString/SOL_USDC-31MAY24-162-P
=== PAUSE TestOptionPairToString/SOL_USDC-31MAY24-162-P
=== RUN   TestOptionPairToString/MATIC_USDC-6APR24-0d98-P
=== PAUSE TestOptionPairToString/MATIC_USDC-6APR24-0d98-P
=== CONT  TestOptionPairToString/BTC-30MAY24-61000-C
=== CONT  TestOptionPairToString/SOL_USDC-31MAY24-162-P
=== CONT  TestOptionPairToString/ETH-1JUN24-3200-P
=== CONT  TestOptionPairToString/MATIC_USDC-6APR24-0d98-P
--- PASS: TestOptionPairToString (0.00s)
    --- PASS: TestOptionPairToString/BTC-30MAY24-61000-C (0.00s)
    --- PASS: TestOptionPairToString/ETH-1JUN24-3200-P (0.00s)
    --- PASS: TestOptionPairToString/SOL_USDC-31MAY24-162-P (0.00s)
    --- PASS: TestOptionPairToString/MATIC_USDC-6APR24-0d98-P (0.00s)
```

( And that got worse with me adding more tests )
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
medium priority review me This pull request is ready for review
Projects
Status: In review
Development

Successfully merging this pull request may close these issues.

2 participants