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 8 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
@gbjk gbjk force-pushed the feature/deribit_sub_conf branch from 04871d1 to fa3520b Compare September 18, 2024 06:46
@gbjk gbjk force-pushed the feature/deribit_sub_conf branch from fa3520b to a6e200f Compare December 4, 2024 07:31
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.

tACK! That's a paddlin' good culling

@gloriousCode gloriousCode added the gcrc GloriousCode Review Complete label Dec 5, 2024
gbjk added 6 commits December 12, 2024 09:48
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 )
@gbjk gbjk force-pushed the feature/deribit_sub_conf branch from a196533 to 4fa7bc3 Compare December 12, 2024 02:48
Copy link
Collaborator

@shazbert shazbert left a comment

Choose a reason for hiding this comment

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

Thanks just one nit. 🚀

@@ -3266,11 +3268,79 @@ func setupWs() {
}
}

func TestGenerateDefaultSubscriptions(t *testing.T) {
func TestGenerateSubscriptions(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is erroring: here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed gbjk@c3e5308ae

Was the result of the upstream fix to master to respect disabled websocket assets, and the test didn't know about that.

@gbjk gbjk requested a review from shazbert December 13, 2024 08:15
Copy link
Collaborator

@shazbert shazbert left a comment

Choose a reason for hiding this comment

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

Pretty much tACK. Just one suggestion.

initialDelimiter := currency.DashDelimiter
if subCodes[0] == "USDC" {
q := pair.Quote.String()
if q[:4] == "USDC" && len(q) > 11 { // Linear option
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if q[:4] == "USDC" && len(q) > 11 { // Linear option
if strings.HasPrefix(q, "USDC") && len(q) > 11 { // Linear option

Suggestion: Less panic at the disco.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@gbjk gbjk requested a review from shazbert December 17, 2024 01:57
Copy link
Collaborator

@shazbert shazbert left a comment

Choose a reason for hiding this comment

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

Changes ACK. Thanks!

@shazbert shazbert added the szrc shazbert review complete label Dec 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gcrc GloriousCode Review Complete medium priority review me This pull request is ready for review szrc shazbert review complete
Projects
Status: In review
Development

Successfully merging this pull request may close these issues.

4 participants