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: bug fixes, test fixes and implement GetCurrencyTradeURL #1558

Merged
merged 13 commits into from
Jun 7, 2024

Conversation

gloriousCode
Copy link
Collaborator

@gloriousCode gloriousCode commented Jun 3, 2024

PR Description

These are the kinds of bugs that can only pop up after proper use, it was still an excellent PR Sam 🕊️

This PR does the following:

  • Speeds up UpdateTradablePairs utilising concurrency since its so slow. Reduces go test -race -count=0 from 28s to 20s
  • Implement GetCurrencyTradeURL
  • Update function IsPerpetualFuture
  • Update optionPairToString and add test coverage
  • Simplifies some tests
  • Rename guessAssetTypeFromInstrument to getAssetFromPair as its not guessing
  • Adds getAssetPairByInstrument to help parse WS pair data after getting some errors
  • Handles currency processing via the any subscriptions (eg trades) using getAssetPairByInstrument
    • This moves the parsing of currency data to inside loops, because any can have multiple currencies in the slice/ the currency isn't in the channel name
  • Fix an issue with a lack of candle data due to incorrect kline choice (fixes large output about missing candle data in tests)
  • Changed TestGetHistoricTrades to not check results, because sometimes there are no trades, but its an expensive test to go back further in time to ensure trades. If there is an issue, deribit will error.
  • Stop checking orderbooks bid/ask len for options.

I made sure that getAssetPairByInstrument was performant before using it

func BenchmarkGetAssetPairByInstrumentFutures(b *testing.B) {
	b.ReportAllocs()
	for i := 0; i < b.N; i++ {
		_, _, err := d.getAssetPairByInstrument("DOGE_USDC-PERPETUAL")
		if err != nil {
			b.Fatal(err)
		}
	}
}

func BenchmarkGetAssetPairByInstrumentOptions(b *testing.B) {
	b.ReportAllocs()
	for i := 0; i < b.N; i++ {
		_, _, err := d.getAssetPairByInstrument("MATIC_USDC-3JUN24-0D64-P")
		if err != nil {
			b.Fatal(err)
		}
	}
}

func BenchmarkGetAssetPairByInstrumentSpot(b *testing.B) {
	b.ReportAllocs()
	for i := 0; i < b.N; i++ {
		_, _, err := d.getAssetPairByInstrument("DOGE_USDC")
		if err != nil {
			b.Fatal(err)
		}
	}
}
BenchmarkGetAssetPairByInstrumentFutures-10    	11103615	       108.5 ns/op	      36 B/op	       2 allocs/op
BenchmarkGetAssetPairByInstrumentOptions-10    	 7306069	       139.4 ns/op	      68 B/op	       2 allocs/op
BenchmarkGetAssetPairByInstrumentSpot-10       	12750432	        96.06 ns/op	      20 B/op	       2 allocs/op

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

How has this been tested

  • go test ./... -race
  • golangci-lint run
  • TestOptionPairToString
  • GetCurrencyTradeURL

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 with my changes
  • Any dependent changes have been merged and published in downstream modules

@gloriousCode gloriousCode added bug review me This pull request is ready for review labels Jun 3, 2024
@gloriousCode gloriousCode requested a review from a team June 3, 2024 02:35
@gloriousCode gloriousCode self-assigned this Jun 3, 2024
Copy link

codecov bot commented Jun 3, 2024

Codecov Report

Attention: Patch coverage is 73.60406% with 52 lines in your changes missing coverage. Please review.

Project coverage is 38.20%. Comparing base (c37a115) to head (bd5d4c9).
Report is 5 commits behind head on master.

Current head bd5d4c9 differs from pull request most recent head 171915c

Please upload reports for the commit 171915c to get more accurate results.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1558      +/-   ##
==========================================
+ Coverage   37.78%   38.20%   +0.41%     
==========================================
  Files         409      419      +10     
  Lines      147697   152314    +4617     
==========================================
+ Hits        55807    58191    +2384     
- Misses      84031    86023    +1992     
- Partials     7859     8100     +241     
Files Coverage Δ
exchanges/kline/kline.go 91.78% <100.00%> (+0.05%) ⬆️
exchanges/orderbook/orderbook.go 92.26% <100.00%> (ø)
exchanges/deribit/deribit.go 45.89% <87.50%> (ø)
exchanges/deribit/deribit_websocket.go 42.68% <64.15%> (ø)
exchanges/deribit/deribit_wrapper.go 43.68% <72.27%> (ø)

... and 70 files with indirect coverage changes

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.

Nice stuff!

exchanges/deribit/deribit.go Outdated Show resolved Hide resolved
exchanges/deribit/deribit_test.go Show resolved Hide resolved
exchanges/deribit/deribit_test.go Show resolved Hide resolved
exchanges/deribit/deribit_test.go Show resolved Hide resolved
exchanges/deribit/deribit_test.go Show resolved Hide resolved
exchanges/deribit/deribit_websocket.go Show resolved Hide resolved
exchanges/deribit/deribit_wrapper.go Outdated Show resolved Hide resolved
exchanges/deribit/deribit_wrapper.go Show resolved Hide resolved
exchanges/deribit/deribit_wrapper.go Outdated Show resolved Hide resolved
exchanges/orderbook/orderbook.go Show resolved Hide resolved
@gloriousCode gloriousCode requested a review from shazbert June 5, 2024 23:34
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 for the changes tACK.

@shazbert shazbert added the szrc shazbert review complete label Jun 7, 2024
Copy link
Collaborator

@thrasher- thrasher- left a comment

Choose a reason for hiding this comment

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

Thanks @gloriousCode ! Just one minor nit

exchanges/deribit/deribit_wrapper.go Outdated Show resolved Hide resolved
@thrasher- thrasher- changed the title Deribit: bug fixes, test fixes, implement GetCurrencyTradeURL Deribit: bug fixes, test fixes and implement GetCurrencyTradeURL Jun 7, 2024
Copy link
Collaborator

@thrasher- thrasher- 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 making those changes

@thrasher- thrasher- merged commit e16e16b into thrasher-corp:master Jun 7, 2024
5 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug review me This pull request is ready for review szrc shazbert review complete
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants