GateIO: Fix GetFuturesContractDetails for Deliveries and minor other fixes #1766
Conversation
gbjk
force-pushed
the
bugfix/gateio_GetFuturesContractDetails
branch
from
282c54b to
cb7b226
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1766 +/- ##
==========================================
+ Coverage 37.37% 37.40% +0.03%
==========================================
Files 415 415
Lines 178627 178336 -291
==========================================
- Hits 66760 66715 -45
+ Misses 104089 103847 -242
+ Partials 7778 7774 -4
|
gbjk
force-pushed
the
bugfix/gateio_GetFuturesContractDetails
branch
from
dcf0c6c to
77569e5
Compare
gbjk
changed the title
gbjk
force-pushed
the
bugfix/gateio_GetFuturesContractDetails
branch
2 times, most recently
from
e0ceb88 to
ba00163
Compare
gbjk
force-pushed
the
bugfix/gateio_GetFuturesContractDetails
branch
from
c19f66c to
f21d447
Compare
There was a problem hiding this comment.
Thanks for the update, Just some fields that have been left out:
diff --git a/exchanges/gateio/gateio_types.go b/exchanges/gateio/gateio_types.go
index 3bca9c0cc..1a91b5e24 100644
--- a/exchanges/gateio/gateio_types.go
+++ b/exchanges/gateio/gateio_types.go
@@ -679,16 +679,22 @@ type FuturesContract struct {
OrdersLimit int64 `json:"orders_limit"`
TradeID int64 `json:"trade_id"`
OrderbookID int64 `json:"orderbook_id"`
+ EnableBonus bool `json:"enable_bonus"`
+ EnableCredit bool `json:"enable_credit"`
+ CreateTime types.Time `json:"create_time"`
+ FundingCapRatio types.Number `json:"funding_cap_ratio"`
+ VoucherLeverage types.Number `json:"voucher_leverage"`
}
// TradingHistoryItem represents futures trading history item.
type TradingHistoryItem struct {
- ID int64 `json:"id"`
- CreateTime types.Time `json:"create_time"`
- Contract string `json:"contract"`
- Text string `json:"text"`
- Size float64 `json:"size"`
- Price types.Number `json:"price"`
+ ID int64 `json:"id"`
+ CreateTime types.Time `json:"create_time"`
+ CreateTimeMs types.Time `json:"create_time_ms"`
+ Contract string `json:"contract"`
+ Text string `json:"text"`
+ Size float64 `json:"size"`
+ Price types.Number `json:"price"`
// Added for Derived market trade history datas.
Fee types.Number `json:"fee"`
PointFee types.Number `json:"point_fee"`
Fixed gbjk@1565c773b |
exchanges/gateio/gateio_types.go
Outdated
| // Added for Derived market trade history datas. | ||
| ID int64 `json:"id"` | ||
| CreateTime types.Time `json:"create_time"` | ||
| CreateTimeMs types.Time `json:"create_time_ms"` |
There was a problem hiding this comment.
| CreateTimeMs types.Time `json:"create_time_ms"` | |
| CreateTimeMS types.Time `json:"create_time_ms"` |
Or CreateTimeMilliseconds; I generally prefer the latter to be more human friendly with fields even though someone can infer what it means
There was a problem hiding this comment.
Can't fix this one alone, without fixing all the others. :)
And if we're going to touch all of them, I think we should just ditch the non _ms field and rename the _ms field:
CreateTime types.Time `json:"create_time_ms"`
Unless you object real quick, that's coming in on runway 2 🛩️
There was a problem hiding this comment.
If there's two fields doing the same thing and one with less precision than the other, makes sense. We should always default to whatever gives us the most precision (milliseconds field in this case) 👍 Assuming that both fields are always populated as well
There was a problem hiding this comment.
There's one edge case I'm chasing down now - The V3 implementation under the V4 api url for the options websocket, and the response header not having time_ms.
Anyway, yeah. I'm on it.
There was a problem hiding this comment.
Fixed gbjk@7e730dda2 and gbjk@12122726d
There was a problem hiding this comment.
Benchmark referenced in commit message. Just realised that won't persist.
Here it is for posterity
Not committing because once validated, there's no reason to doubt this will regret.
It was just to ensure that the EachKeys we haven't used before is as performant as their godoc claimed.
gbjk
force-pushed
the
bugfix/gateio_GetFuturesContractDetails
branch
from
3459b22 to
7e730dd
Compare
| @@ -216,6 +217,45 @@ func (g *Gateio) WsHandleSpotData(_ context.Context, respRaw []byte) error { | |||
| return nil | |||
| } | |||
|
|
|||
| func parseWSHeader(msg []byte) (r *WSResponse, errs error) { | |||
There was a problem hiding this comment.
This is definitely more performant than json I did my own benchmarks to validate
jsonparser:
// 1646160 666.3 ns/op 312 B/op 7 allocs/op
func BenchmarkParseHeader(b *testing.B) {
in := `{"time":1726121320,"time_ms":1726121320745,"id":1,"channel":"spot.tickers","event":"subscribe","result":{"status":"success"},"request_id":"a4"}`
b.ReportAllocs()
b.ResetTimer()
for i := 0; i < b.N; i++ {
_, err := parseWSHeader([]byte(in))
if err != nil {
b.Fatal(err)
}
}
}normal json:
// 654258 1810 ns/op 408 B/op 10 allocs/op
func BenchmarkJson(b *testing.B) {
in := []byte(`{"time":1726121320,"time_ms":1726121320745,"id":1,"channel":"spot.tickers","event":"subscribe","result":{"status":"success"},"request_id":"a4"}`)
b.ReportAllocs()
b.ResetTimer()
for i := 0; i < b.N; i++ {
var resp WsResponse
err := json.Unmarshal(in, &resp)
if err != nil {
b.Fatal(err)
}
}
}Using the bench mark above I then switched branch #1623 to use sonic and that has comparative performance using go test --tags=sonic -bench=BenchmarkJson -v but has way less implementation complexity. I would suggest to reverse course just to reduce or limit that complexity and the ability for all websocket asset types pathways to achieve that relative boost without that toil.
BenchmarkJson-12 1775338 643.5 ns/op 361 B/op 7 allocs/op
BenchmarkJson-12 1857788 742.5 ns/op 362 B/op 7 allocs/op
BenchmarkJson-12 1890859 640.5 ns/op 361 B/op 7 allocs/op
BenchmarkJson-12 1871022 686.6 ns/op 362 B/op 7 allocs/op
There was a problem hiding this comment.
Hard pass: I was already on the very very edge of acceptability when dealing with a bugfix.
This implementation is more performant than currently and not more complex for the callers of parseWSHeader.
I'm very excited to adopt sonic, but not in a bug fix
2) I'm wary because of the hit we take on Silicon chips if we go sonic over jsonparser.
That's why I haven't yet gone gungho on it.
In the example here, we don't gain any performance by using sonic, but we lose performance on M* chips.
That's not a debate for here, admittedly.
There was a problem hiding this comment.
You are correct, the current option is still a better approach and more sonic things can be done later when that gets merged, I wasn't suggesting to add sonic to this PR to be clear. In my benchmarks I didn't even account for the UnmarshalJSON edge case which adds further overhead. Silly billy.
Was returning the product of all the contracts, so 1444 instead of 38 contracts.
Using wrong error for pair not enabled
Especially fixes GetSingleContract, which seems misleading to not say Futures. There's a load of `GetSingle*` here that should probably also be fixed, but these two justified a dyno
gbjk
force-pushed
the
bugfix/gateio_GetFuturesContractDetails
branch
from
9d28eda to
9338a2c
Compare
It's been a year, and I'm still getting caught out by govet demanding I don't shadow a var I was deliberately shadowing. Made worse by an increase in clashes with stylecheck when they both want opposite things on the same line.
This unifies handling for time_ms and time in response headers, since options and delivery have only time, but spot has time_ms as well. We use the better of the two results. Also [improves performance 2x](https://gist.github.com/gbjk/7cacb63b9a256e745534bb05ca853c48)
Removes the deprecated _time json fields and populates our Time fields with the time_ms values
gbjk
force-pushed
the
bugfix/gateio_GetFuturesContractDetails
branch
from
9338a2c to
193b255
Compare
GateIO
Was returning the product of all the contracts, so 1444 instead of 38 contracts.
Doesn't touch anything else around this; Futures asset split coming separately
Linter
Type of change
How has this been tested