-
Notifications
You must be signed in to change notification settings - Fork 837
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
Kraken: Fix sending trades to the websocket DataHandler #1813
Changes from 5 commits
7888984
26fdb72
b41daf9
0879de9
f086d2a
d9bfce0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -1997,7 +1997,7 @@ func TestGetErrResp(t *testing.T) { | |||||
seen++ | ||||||
switch seen { | ||||||
case 1: // no event | ||||||
assert.ErrorIs(t, testErr, errParsingWSField, "Message with no event Should get correct error type") | ||||||
assert.ErrorIs(t, testErr, common.ErrParsingWSField, "Message with no event Should get correct error type") | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||||||
assert.ErrorContains(t, testErr, "'event'", "Message with no event error should contain missing field name") | ||||||
assert.ErrorContains(t, testErr, "nightjar", "Message with no event error should contain the message") | ||||||
case 2: // with {} for event | ||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -27,6 +27,7 @@ import ( | |||||
"github.com/thrasher-corp/gocryptotrader/exchanges/sharedtestvalues" | ||||||
"github.com/thrasher-corp/gocryptotrader/exchanges/subscription" | ||||||
"github.com/thrasher-corp/gocryptotrader/exchanges/ticker" | ||||||
"github.com/thrasher-corp/gocryptotrader/exchanges/trade" | ||||||
testexch "github.com/thrasher-corp/gocryptotrader/internal/testing/exchange" | ||||||
testsubs "github.com/thrasher-corp/gocryptotrader/internal/testing/subscriptions" | ||||||
mockws "github.com/thrasher-corp/gocryptotrader/internal/testing/websocket" | ||||||
|
@@ -1221,6 +1222,41 @@ func TestWsHandleData(t *testing.T) { | |||||
testexch.FixtureToDataHandler(t, "testdata/wsHandleData.json", k.wsHandleData) | ||||||
} | ||||||
|
||||||
func TestWSProcessTrades(t *testing.T) { | ||||||
t.Parallel() | ||||||
|
||||||
k := new(Kraken) //nolint:govet // Intentional shadow to avoid future copy/paste mistakes | ||||||
require.NoError(t, testexch.Setup(k), "Test instance Setup must not error") | ||||||
err := k.Websocket.AddSubscriptions(k.Websocket.Conn, &subscription.Subscription{Asset: asset.Spot, Pairs: currency.Pairs{spotTestPair}, Channel: subscription.AllTradesChannel, Key: 18788}) | ||||||
require.NoError(t, err, "AddSubscriptions must not error") | ||||||
testexch.FixtureToDataHandler(t, "testdata/wsAllTrades.json", k.wsHandleData) | ||||||
close(k.Websocket.DataHandler) | ||||||
|
||||||
invalid := []any{"trades", []any{[]interface{}{"95873.80000", "0.00051182", "1708731380.3791859"}}} | ||||||
pair := currency.NewPair(currency.XBT, currency.USD) | ||||||
err = k.wsProcessTrades(invalid, pair) | ||||||
require.ErrorContains(t, err, "unexpected trade data length") | ||||||
|
||||||
expJSON := []string{ | ||||||
`{"AssetType":"spot","CurrencyPair":"XBT/USD","Side":"BUY","Price":95873.80000,"Amount":0.00051182,"Timestamp":"2025-02-23T23:29:40.379185914Z"}`, | ||||||
`{"AssetType":"spot","CurrencyPair":"XBT/USD","Side":"SELL","Price":95940.90000,"Amount":0.00011069,"Timestamp":"2025-02-24T02:01:12.853682041Z"}`, | ||||||
} | ||||||
require.Len(t, k.Websocket.DataHandler, len(expJSON), "Must see correct number of trades") | ||||||
for resp := range k.Websocket.DataHandler { | ||||||
switch v := resp.(type) { | ||||||
case trade.Data: | ||||||
i := 1 - len(k.Websocket.DataHandler) | ||||||
exp := trade.Data{Exchange: k.Name, CurrencyPair: spotTestPair} | ||||||
require.NoErrorf(t, json.Unmarshal([]byte(expJSON[i]), &exp), "Must not error unmarshalling json %d: %s", i, expJSON[i]) | ||||||
require.Equalf(t, exp, v, "Trade [%d] should be correct", i) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||||||
case error: | ||||||
t.Error(v) | ||||||
default: | ||||||
t.Errorf("Unexpected type in DataHandler: %T (%s)", v, v) | ||||||
} | ||||||
} | ||||||
} | ||||||
|
||||||
func TestWsOpenOrders(t *testing.T) { | ||||||
t.Parallel() | ||||||
k := new(Kraken) //nolint:govet // Intentional shadow to avoid future copy/paste mistakes | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -79,7 +79,6 @@ func init() { | |
|
||
var ( | ||
authToken string | ||
errParsingWSField = errors.New("error parsing WS field") | ||
errCancellingOrder = errors.New("error cancelling order") | ||
errSubPairMissing = errors.New("pair missing from subscription response") | ||
errInvalidChecksum = errors.New("invalid checksum") | ||
|
@@ -531,7 +530,9 @@ func (k *Kraken) wsProcessTrades(response []any, pair currency.Pair) error { | |
if !ok { | ||
return errors.New("received invalid trade data") | ||
} | ||
if !k.IsSaveTradeDataEnabled() { | ||
saveTradeData := k.IsSaveTradeDataEnabled() | ||
tradeFeed := k.IsTradeFeedEnabled() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OOS: Just highlighting this as trade feed enabled is not necessary as it's a subscription customisation. No change needed. |
||
if !saveTradeData && !tradeFeed { | ||
return nil | ||
} | ||
trades := make([]trade.Data, len(data)) | ||
Beadko marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
@@ -540,24 +541,37 @@ func (k *Kraken) wsProcessTrades(response []any, pair currency.Pair) error { | |
if !ok { | ||
return errors.New("unidentified trade data received") | ||
} | ||
timeData, err := strconv.ParseFloat(t[2].(string), 64) | ||
if len(t) < 4 { | ||
return fmt.Errorf("%w; unexpected trade data length: %d", common.ErrParsingWSField, len(t)) | ||
} | ||
ts, ok := t[2].(string) | ||
Beadko marked this conversation as resolved.
Show resolved
Hide resolved
gbjk marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if !ok { | ||
return common.GetTypeAssertError("string", t[2], "trade.time") | ||
} | ||
timeData, err := strconv.ParseFloat(ts, 64) | ||
if err != nil { | ||
return err | ||
} | ||
|
||
price, err := strconv.ParseFloat(t[0].(string), 64) | ||
p, ok := t[0].(string) | ||
if !ok { | ||
return common.GetTypeAssertError("string", t[0], "trade.price") | ||
} | ||
price, err := strconv.ParseFloat(p, 64) | ||
if err != nil { | ||
return err | ||
} | ||
|
||
amount, err := strconv.ParseFloat(t[1].(string), 64) | ||
v, ok := t[1].(string) | ||
if !ok { | ||
return common.GetTypeAssertError("string", t[1], "trade.volume") | ||
} | ||
amount, err := strconv.ParseFloat(v, 64) | ||
if err != nil { | ||
return err | ||
} | ||
var tSide = order.Buy | ||
s, ok := t[3].(string) | ||
if !ok { | ||
return common.GetTypeAssertError("string", t[3], "side") | ||
return common.GetTypeAssertError("string", t[3], "trade.side") | ||
} | ||
if s == "s" { | ||
tSide = order.Sell | ||
|
@@ -573,7 +587,15 @@ func (k *Kraken) wsProcessTrades(response []any, pair currency.Pair) error { | |
Side: tSide, | ||
} | ||
} | ||
return trade.AddTradesToBuffer(k.Name, trades...) | ||
if tradeFeed { | ||
for i := range trades { | ||
k.Websocket.DataHandler <- trades[i] | ||
} | ||
} | ||
if saveTradeData { | ||
return trade.AddTradesToBuffer(k.Name, trades...) | ||
gbjk marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
return nil | ||
} | ||
|
||
// wsProcessOrderBook handles both partial and full orderbook updates | ||
|
@@ -1331,7 +1353,7 @@ func (k *Kraken) wsCancelOrder(orderID string) error { | |
|
||
status, err := jsonparser.GetUnsafeString(resp, "status") | ||
if err != nil { | ||
return fmt.Errorf("%w 'status': %w from message: %s", errParsingWSField, err, resp) | ||
return fmt.Errorf("%w 'status': %w from message: %s", common.ErrParsingWSField, err, resp) | ||
} else if status == "ok" { | ||
return nil | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
[119930881,[["95873.80000","0.00051182","1740353380.379186","b","l",""]],"trade","XBT/USD"] | ||
[119930881,[["95940.90000","0.00011069","1740362472.853682","s","l",""]],"trade","XBT/USD"] |
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 wasn't your doing, but I'd prefer that we don't use acronyms for error messages
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.
No worries, I originally wrote "websocket" but changed it when I saw the original errors 😄 . Updated