-
Notifications
You must be signed in to change notification settings - Fork 822
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
currency: Adds matching lookup table built from available pairs. #1312
Conversation
…string conversion without delimiter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is nice 🍐💨
// MatchSymbolWithAvailablePairs returns a currency pair based on the supplied | ||
// symbol and asset type. If the string is expected to have a delimiter this | ||
// will attempt to screen it out. | ||
func (b *Base) MatchSymbolWithAvailablePairs(symbol string, a asset.Item, hasDelimiter bool) (currency.Pair, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So utilising the same benchmarking as last time for comparison:
func BenchmarkNewPairFromString(b *testing.B) {
b.ReportAllocs()
for i := 0; i < b.N; i++ {
_, _ = currency.NewPairFromString("ZTG_USDT")
}
}
func BenchmarkDeriveFrom(b *testing.B) {
b.ReportAllocs()
for i := 0; i < b.N; i++ {
avail, _ := g.GetAvailablePairs(asset.Spot)
_, _ = avail.DeriveFrom(strings.Replace("ZTG_USDT", "_", "", 1))
}
}
func BenchmarkDeriveFromCheating(b *testing.B) {
b.ReportAllocs()
avail, _ := g.GetAvailablePairs(asset.Spot)
for i := 0; i < b.N; i++ {
_, _ = avail.DeriveFrom(strings.Replace("ZTG_USDT", "_", "", 1))
}
}
func BenchmarkMatchSymbolWithAvailablePairs(b *testing.B) {
b.ReportAllocs()
for i := 0; i < b.N; i++ {
_, _ = g.MatchSymbolWithAvailablePairs("ZTG_USDT", asset.Spot, true)
}
}
Benchmark | times tested | time taken | Bytes per op | Allocations |
---|---|---|---|---|
BenchmarkNewPairFromString-10 | 20982969 | 57.07 ns/op | 4 B/op | 1 allocs/op |
BenchmarkDeriveFrom-10 | 17596 | 67605 ns/op | 294928 B/op | 4 allocs/op |
BenchmarkDeriveFromCheating-10 | 156580 | 7163 ns/op | 17 B/op | 2 allocs/op |
BenchmarkMatchSymbolWithAvailablePairs-10 | 15378223 | 76.50 ns/op | 16 B/op | 2 allocs/op |
Does show a lovely improvement. Would you be considering the removal of Derivefrom
? Because your new feature seems underutilised in this PR
Plus very nice work on the PairFromString improvements!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. Just need to update Bybit to be filtering on enabled pairs to not change behaviour
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy with this. I'm already wanting to use it
Please update tests:
--- FAIL: TestUpdateTicker (1.85s)
bitmex_test.go:1103: pair not found for xrpusd perpetualcontract
--- FAIL: TestUpdateTickers (6.73s)
bitfinex_test.go:504: pair not found for xautf0btcf0 spot
--- FAIL: TestUpdateTicker (0.32s)
bitfinex_test.go:477: pair not found for btcf0eutf0 spot
--- FAIL: TestGetFundingRates (0.00s)
rpcserver_test.go:3031: received: 'pair matcher is nil' but expected: '<nil>'
--- FAIL: TestAllExchangeWrappers/coinut_wrapper_tests (0.43s)
--- FAIL: TestAllExchangeWrappers/coinut_wrapper_tests/MatchSymbolWithAvailablePairs-- (0.00s)
exchange_wrapper_standards_test.go:214: COINUT Func 'MatchSymbolWithAvailablePairs' Error: 'pair not found for 1337 '. Inputs: [1337 false].
And any others. Which now fail due from this update
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for that, just finishing up I think. I know this is annoying 😭
cmd/exchange_wrapper_standards/exchange_wrapper_standards_test.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one thing I noticed, plus please rebase/merge and check that tests are good 🤙
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK! Nice work Shazbert
return nil | ||
} | ||
|
||
// Has Fast path optimisation when int == 64 | ||
i, err := strconv.Atoi(s) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is why we try not to use Atoi, thanks for the fix!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work @shazbert !
…sher-corp#1312) * currency: Add pair matching update (cherry-pick) * exchange/currency: Add tests and update func * linter fix, also if using json unmarshal functionality stop usage of string conversion without delimiter * gemini: fix test * currency/manager: potential optimisation * exchanges: purge derive from wrapper cases and add warning comment * glorious: nits * glorious: nits * linter: fix * glorious: nits * whoops * whoops * glorious: nits continued * glorious: diff THANKS! * hitbtc: fix update tradable pairs strings splitting. continue if not enabled tickers update pair. * glorious: nits * linter: fix * Update exchanges/exmo/exmo_wrapper.go Co-authored-by: Scott <[email protected]> * bitstamp: fix test when 32 biterinos architecturinos * capture more strings for speed * swapsies because whos running 32bit \0/? --------- Co-authored-by: Ryan O'Hara-Reid <[email protected]> Co-authored-by: Scott <[email protected]>
PR Description
@gloriousCode brought up an issue with the old matching system with respective benchmarks here sometimes when using this super fast function can result in incorrect return pair if there is no delimiter. These benchmarks can be run again.
NewPairFromString
for even speedier usage 💨MatchSymbolWithAvailablePairs
currency.Pair
is used for a json field this has become more strict and requires now a delimiter as we have to make sure that we are correctly formatting to match with our stored list.Note: Keeping this as non-invasive as possible, separate PR's can be opened when needed if MatchSymbolWithAvailablePairs is found to be a better use case for speed considerations (mainly websocket throughput).
Type of change
Please delete options that are not relevant and add an
x
in[]
as item is complete.How has this been tested
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration and
also consider improving test coverage whilst working on a certain feature or package.
Checklist