Bitfinex: Fix WS trade processing #1754
Conversation
* Add handling for funding trades Fixes thrasher-corp#1746
gbjk
force-pushed
the
bugfix/bitfinex_ws_trade
branch
2 times, most recently
from
0dd865c to
cb416cc
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1754 +/- ##
==========================================
+ Coverage 37.13% 37.37% +0.23%
==========================================
Files 414 415 +1
Lines 180198 178586 -1612
==========================================
- Hits 66925 66746 -179
+ Misses 105413 104042 -1371
+ Partials 7860 7798 -62
|
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.
gbjk
force-pushed
the
bugfix/bitfinex_ws_trade
branch
from
48ebaf7 to
f86fff4
Compare
| @@ -1382,17 +1372,44 @@ func TestWsOrderBook(t *testing.T) { | |||
| assert.ErrorIs(t, err, errNoSeqNo, "handleWSBookUpdate should send correct error") | |||
| } | |||
|
|
|||
| func TestWsTradeResponse(t *testing.T) { | |||
| func TestWSAllTrades(t *testing.T) { | |||
| data, ok := d[2].([]interface{}) | ||
| if !ok { | ||
| return errors.New("trade data type assertion error") | ||
| var wsTrades []*wsTrade |
There was a problem hiding this comment.
Please change this shadowed var name/the original string var name
There was a problem hiding this comment.
Fixed gbjk@3a52f3fd1 and gbjk@0b3fdd6fe
| if valueType != jsonparser.Array { | ||
| errs = common.AppendError(errs, fmt.Errorf("%w `tradesSnapshot[1][*]`: %w `%s`", errParsingWSField, jsonparser.UnknownValueTypeError, valueType)) |
There was a problem hiding this comment.
This is a little bit extra given that there is only one caller to this and its behind a gate checking if its an array. I am normally one for all checks should be within the function, but its that you just checked to get to this function
There was a problem hiding this comment.
How about this simplification?
func (b *Bitfinex) handleWSPublicTradesSnapshot(respRaw []byte) ([]*wsTrade, error) {
var trades []*wsTrade
err := json.Unmarshal(respRaw, &trades)
return trades, err
}Where handleWSAllTrades changes with:
k, valueType, _, err := jsonparser.Get(respRaw, "[1]")
...
if wsTrades, err = b.handleWSPublicTradesSnapshot(k); err != nil {A lot less tangling of additional jsonparser functions, with added bonus of naked return removal
There was a problem hiding this comment.
This is a little bit extra given that there is only one caller to this and its behind a gate checking if its an array.
I think you might have missed that this is checking that every element inside the array is also an array.
That's the answer to why we're doing the second check.
And yeah. I think I didn't re-sniff simplifying this after applying shazbert's suggestion of the UnmarshalJSON on wsTrade. I think we can probably do exactly as you suggest. Will fix the test and confirm.
There was a problem hiding this comment.
Yep. Exactly as you suggested. Extended to the handleWSPublicTradeUpdate as well.
Fixed gbjk@82eb2c02d
exchange Bitfinex websocket error - unhandled WS trade update event:TestWsinstead ofTestWSFixes #1746
Progresses #1323 with a prototype
Type of change