Kraken: Subscription improvements #1358

gbjk · 2023-10-01T09:24:22Z

Move UpdateTradablePairs to a test
This should not happen in TestMain just because it's a requirement for
most tests. It should just be a non-parallel early test.
That allows unit tests to run quickly without calling it, and allows
proper clear failures
Separate auth and non-auth ws tests
Move SeedAssets from Setup to Start
Having SeedAssets in Setup is cruel and unusual because it calls the
API. Most other interactive data seeding happens in Start.
This made it so that fixing and creating unit tests for Kraken was
painfully slow, particularly on flaky internet.
Use Websocket subscriptionChannels instead of local slice
Remove ChannelID - Deprecated in docs
I went both ways on this N+ times. ChannelID is great, but the bottom line is that it's deprecated, and not available in auth channels. By moving away from it we futureproof, and there's no downside really. Note that with or without
Simplify ping handlers and hardcodes message
Add Depth as configurable orderbook channel param
Simplify auth/non-auth channel updates

Type of change

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

How has this been tested

go test ./... -race
golangci-lint run

exchanges/kraken/kraken_test.go

exchanges/kraken/kraken_websocket.go

shazbert

Thanks for the PR. Just some quick nits and then once done I will continue with review.

exchanges/kraken/kraken_websocket.go

exchanges/kraken/kraken_wrapper.go

exchanges/kraken/kraken_websocket.go

shazbert

LGTM thanks for making those changes. tACK.

There are two residing issues carried over from master (out of scope).

[ERROR] | ORDER | 06/10/2023 10:51:00 |  unsupported asset type

The is due to processing own trades. Could set order.Detail struct with Status: order.Filled to stop it trying to update it.

Also there is noticed that there is a cycling issues when we switch from websocket to rest on orderbooks (💩 coin liquidity issues). I think I might make an update in the future so that we do not switch over if there is an active websocket connection.

[ERROR] | WEBSOCKET | 06/10/2023 10:53:36 | exchange Kraken websocket error - Kraken - unhandled websocket data: Kraken XTZUSDT spot Reason: [orderbook data integrity compromised, rest sync timer lapse with active websocket connection]

gbjk · 2023-10-10T07:19:44Z

Rebased against master.
Fixes for races and Unsub loops added.

gbjk · 2023-10-21T08:08:37Z

Rebased on master and pulled in the latest changes from the bitfinex PR's websockets improvements.

Note: There's a failure on Bitfinex here which is fixed in it's own PR.

gbjk · 2023-11-02T06:35:23Z

@shazbert This is ready for review; Please remove the label.

Rebased on master already.

shazbert

Looks good:

Just some errors running instance:

[ERROR] | WEBSOCKET | 10/11/2023 13:02:35 | exchange Kraken websocket error - Kraken - unhandled websocket data: subscription not found: spot ohlc-1 XBT/USD
[ERROR] | WEBSOCKET | 10/11/2023 13:02:35 | exchange Kraken websocket error - Kraken - unhandled websocket data: subscription not found: spot book-1000 XBT/USD

gbjk · 2023-11-11T04:07:22Z

Just some errors running instance:

Ah damn it.
When I rebased on your checkSubscriptions() recently I introduced an earlier EnsureKeyed.
That breaks the "don't double up" check in Kraken's own ensureKeyed, which in hindsight wasn't very robust.

Okay. On it.

gbjk · 2023-11-11T04:22:34Z

@shazbert Thanks for that.

Fixed 20d8288

gloriousCode

Thanks for those changes! Just need to fix up the build/lint issues from wsProcessSpread changes and I'm happy

gbjk · 2024-01-17T07:44:04Z

@gloriousCode Build issue fixed; Ready for you

gloriousCode

tACK! Thanks for making those changes

shazbert

DM'd a maybe issue.

This should not happen in TestMain just because it's a requirement for most tests. It should just be a non-parallel early test. That allows unit tests to run quickly without calling it, and allows proper clear failures

Having SeedAssets in Setup is cruel and unusual because it calls the API. Most other interactive data seeding happens in Start. This made it so that fixing and creating unit tests for Kraken was painfully slow, particularly on flaky internet.

* Use Websocket subscriptionChannels instead of local slice * Remove ChannelID - Deprecated in docs * Simplify ping handlers and hardcodes message * Add Depth as configurable orderbook channel param * Simplify auth/non-auth channel updates * Add configurable Book depth * Add configurable Candle timeframes

* Also fixes some, but not all, of the auth and WS tests to skip without creds and pass when it has them * Couple of Drivebys for good measure to improve tests

We were using the "cancel many" facility of the Kraken api. However since that doesn't actually report errors individually, it seems saner to just multiplex over it. We were going to get N+ responses anyway. Might as well send N+ requests

Retain the .msg field of a go fmt.Errorf .msg field returned by .Error() when wrapping multiple errors. This fixes a situation where a nested stack of errors would lose formatting information, which is often used to supply identifying context. e.g. ``` err = fmt.Errorf("%w `%s`: %w", errParsingField, fieldName, parsingError) errs = common.AppendError(errs, err) ``` This isn't really an issue with our implementation; Calling Unwrap() on a fmt.Errorf() which returns a wrapErrors will lose that formatting. Our issue was that we were using just Unwrap() to bind together our chain-of-custody.

thrasher-

Very nice! Great improvements across the board and parallelChanOp is sexy. Tested both unauth and auth functionality. Only some minor nits

thrasher- · 2024-01-21T02:36:12Z

exchanges/kraken/kraken_test.go

+	assert.Len(t, k.Websocket.GetSubscriptions(), 1, "Should add 1 Subscription")
+
+	subs := k.Websocket.GetSubscriptions()
+	assert.Len(t, subs, 1, "Should have 1 subscription channel")


Suggested change

assert.Len(t, subs, 1, "Should have 1 subscription channel")

require.Len(t, subs, 1, "Should have 1 subscription channel")

Otherwise the next line will panic out of range

thrasher- · 2024-01-21T02:37:37Z

exchanges/kraken/kraken_test.go

+	assert.Len(t, k.Websocket.GetSubscriptions(), 1, "Should add 1 Subscription")
+
+	subs := k.Websocket.GetSubscriptions()
+	assert.Len(t, subs, 1, "Should have 1 subscription channel")


Suggested change

assert.Len(t, subs, 1, "Should have 1 subscription channel")

require.Len(t, subs, 1, "Should have 1 subscription channel")

Because of the next line

thrasher- · 2024-01-21T02:38:53Z

exchanges/kraken/kraken_test.go

+	subs := k.Websocket.GetSubscriptions()
+	assert.Len(t, subs, 4, "Should have correct number of subscriptions")
+
+	err = k.Unsubscribe(subs[:1])


Also needs panic protection (TM)

thrasher- · 2024-01-21T02:45:48Z

exchanges/kraken/kraken_test.go

 	`[8912,{"as":[["0.01300","850.00000000","1690230914.230506"],["0.01400","323483.99590510","1690256356.615823"],["0.01500","100287.34442717","1690219133.193345"],["0.01600","67995.78441017","1690118389.451216"],["0.01700","41776.38397740","1689676303.381189"],["0.01800","11785.76177777","1688631951.812452"],["0.01900","23700.00000000","1686935422.319042"],["0.02000","3941.17000000","1689415829.176481"],["0.02100","16598.69173066","1689420942.541943"],["0.02200","17572.51572836","1689851425.907427"]],"bs":[["0.01200","14220.66466572","1690256540.842831"],["0.01100","160223.61546438","1690256401.072463"],["0.01000","63083.48958963","1690256604.037673"],["0.00900","6750.00000000","1690252470.633938"],["0.00800","213059.49706376","1690256360.386301"],["0.00700","1000.00000000","1689869458.464975"],["0.00600","4000.00000000","1690221333.528698"],["0.00100","245000.00000000","1690051368.753455"]]},"book-10","GST/EUR"]`,
 	`[8912,{"b":[["0.01000","60583.48958963","1690256620.206768"],["0.01000","63083.48958963","1690256620.206783"]],"c":"69619317"},"book-10","GST/EUR"]`,
 }

 func TestWsOrderbookMax10Depth(t *testing.T) {
 	t.Parallel()
+	for _, c := range []string{"XDG/USD", "LUNA/EUR", "GST/EUR"} {
+		p, err := currency.NewPairFromString(c)
+		assert.NoErrorf(t, err, "NewPairFromString %s should not error", c)


Suggested change

assert.NoErrorf(t, err, "NewPairFromString %s should not error", c)

require.NoErrorf(t, err, "NewPairFromString %s should not error", c)

Extremely low p of it occurring, but would make the successful sub untrue

common/common_test.go

exchanges/kraken/kraken_websocket.go

gbjk · 2024-01-21T03:44:16Z

Structure currently means 1 req per channel/pair for Kraken, so we could better handle failures.
That's not performant enough by a long way with --enableallpairs.

Currently re-structuring slightly Pair 👉 Pairs natively in Subscription.

gbjk · 2024-03-22T04:38:53Z

Will re-open this fresh. It's too cluttered and different to just force push and pretend nothing happened.

gbjk force-pushed the feature/kraken_ws_chans branch 2 times, most recently from d01a5f9 to ebed7a1 Compare October 1, 2023 11:09

gloriousCode requested a review from a team October 1, 2023 22:37

gloriousCode assigned gbjk Oct 1, 2023

gloriousCode added the review me This pull request is ready for review label Oct 1, 2023

gbjk commented Oct 2, 2023

View reviewed changes

exchanges/kraken/kraken_test.go Show resolved Hide resolved

gbjk commented Oct 2, 2023

View reviewed changes

exchanges/kraken/kraken_websocket.go Show resolved Hide resolved

gbjk commented Oct 2, 2023

View reviewed changes

exchanges/kraken/kraken_websocket.go Show resolved Hide resolved

shazbert requested changes Oct 4, 2023

View reviewed changes

exchanges/kraken/kraken_websocket.go Show resolved Hide resolved

exchanges/kraken/kraken_wrapper.go Show resolved Hide resolved

exchanges/kraken/kraken_websocket.go Outdated Show resolved Hide resolved

gbjk requested a review from shazbert October 4, 2023 10:23

shazbert approved these changes Oct 5, 2023

View reviewed changes

shazbert added the rebase/merge of master required label Oct 8, 2023

gbjk force-pushed the feature/kraken_ws_chans branch 2 times, most recently from 66e0e26 to fa504d0 Compare October 10, 2023 07:16

shazbert added rebase/merge of master required and removed rebase/merge of master required labels Oct 10, 2023

gbjk force-pushed the feature/kraken_ws_chans branch 2 times, most recently from 518f836 to b3671a3 Compare October 21, 2023 08:07

gloriousCode removed the rebase/merge of master required label Oct 23, 2023

gbjk mentioned this pull request Oct 24, 2023

Bitfinex: Websocket subscription improvements #1353

Merged

5 tasks

shazbert added the nomerge requires dependency This pull request is dependent on another, so it can't be merged until the dependent one is merged label Oct 24, 2023

gbjk force-pushed the feature/kraken_ws_chans branch 2 times, most recently from 1b1e3c1 to 2814efd Compare November 2, 2023 06:12

shazbert removed the nomerge requires dependency This pull request is dependent on another, so it can't be merged until the dependent one is merged label Nov 2, 2023

shazbert requested changes Nov 10, 2023

View reviewed changes

shazbert added the lint requires fixing label Jan 16, 2024

gloriousCode requested changes Jan 16, 2024

View reviewed changes

gbjk mentioned this pull request Jan 17, 2024

exchanges/websocket: Implement subscription configuration #1394

Merged

4 tasks

gloriousCode removed the lint requires fixing label Jan 18, 2024

gloriousCode approved these changes Jan 18, 2024

View reviewed changes

shazbert requested changes Jan 19, 2024

View reviewed changes

gbjk added 13 commits January 19, 2024 13:20

Kraken: Move UpdateTradablePairs to a test

6cdb72c

This should not happen in TestMain just because it's a requirement for most tests. It should just be a non-parallel early test. That allows unit tests to run quickly without calling it, and allows proper clear failures

Kraken: Separate auth and non-auth ws tests

3029aed

Kraken: Move SeedAssets from Setup to Start

6cc7f66

Having SeedAssets in Setup is cruel and unusual because it calls the API. Most other interactive data seeding happens in Start. This made it so that fixing and creating unit tests for Kraken was painfully slow, particularly on flaky internet.

Kraken: Switch tests to WsConnect

e049075

* Also fixes some, but not all, of the auth and WS tests to skip without creds and pass when it has them * Couple of Drivebys for good measure to improve tests

Kraken: Fix URLMap ignored for websocket URLs

648ac4c

Kraken: Add Mock WS for CancelOrders

bf78832

Bitfinex: Driveby removal of lint hint

db88783

Kraken: Improve format of unhandled ws data err

6835836

Kraken: Add nil pointer errors

aaac1e1

Kraken: More nil protection errs

a28851a

gbjk force-pushed the feature/kraken_ws_chans branch from 71f3cf5 to a28851a Compare January 19, 2024 06:21

thrasher- requested changes Jan 21, 2024

View reviewed changes

gbjk added reconstructing Based on PR feedback, this is currently being reworked and is not to be merged and removed review me This pull request is ready for review labels Jan 22, 2024

gbjk mentioned this pull request Feb 19, 2024

Common: Fix fmt msg lost in AppendError().Error() #1484

Merged

3 tasks

gbjk closed this Mar 22, 2024

gbjk mentioned this pull request Apr 15, 2024

subscriptions: Encapsulate, replace Pair with Pairs and refactor; improve exchange support #1501

Merged

3 tasks

gbjk deleted the feature/kraken_ws_chans branch August 23, 2024 06:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kraken: Subscription improvements #1358

Kraken: Subscription improvements #1358

gbjk commented Oct 1, 2023 •

edited

Loading

shazbert left a comment

shazbert left a comment

gbjk commented Oct 10, 2023

gbjk commented Oct 21, 2023 •

edited

Loading

gbjk commented Nov 2, 2023

shazbert left a comment

gbjk commented Nov 11, 2023

gbjk commented Nov 11, 2023

gloriousCode left a comment •

edited

Loading

gbjk commented Jan 17, 2024

gloriousCode left a comment

shazbert left a comment

thrasher- left a comment

thrasher- Jan 21, 2024

thrasher- Jan 21, 2024

thrasher- Jan 21, 2024

thrasher- Jan 21, 2024

gbjk commented Jan 21, 2024 •

edited

Loading

gbjk commented Mar 22, 2024

	assert.Len(t, subs, 1, "Should have 1 subscription channel")
	require.Len(t, subs, 1, "Should have 1 subscription channel")

	assert.NoErrorf(t, err, "NewPairFromString %s should not error", c)
	require.NoErrorf(t, err, "NewPairFromString %s should not error", c)

Kraken: Subscription improvements #1358

Kraken: Subscription improvements #1358

Conversation

gbjk commented Oct 1, 2023 • edited Loading

Type of change

How has this been tested

shazbert left a comment

Choose a reason for hiding this comment

shazbert left a comment

Choose a reason for hiding this comment

gbjk commented Oct 10, 2023

gbjk commented Oct 21, 2023 • edited Loading

gbjk commented Nov 2, 2023

shazbert left a comment

Choose a reason for hiding this comment

gbjk commented Nov 11, 2023

gbjk commented Nov 11, 2023

gloriousCode left a comment • edited Loading

Choose a reason for hiding this comment

gbjk commented Jan 17, 2024

gloriousCode left a comment

Choose a reason for hiding this comment

shazbert left a comment

Choose a reason for hiding this comment

thrasher- left a comment

Choose a reason for hiding this comment

thrasher- Jan 21, 2024

Choose a reason for hiding this comment

thrasher- Jan 21, 2024

Choose a reason for hiding this comment

thrasher- Jan 21, 2024

Choose a reason for hiding this comment

thrasher- Jan 21, 2024

Choose a reason for hiding this comment

gbjk commented Jan 21, 2024 • edited Loading

gbjk commented Mar 22, 2024

gbjk commented Oct 1, 2023 •

edited

Loading

gbjk commented Oct 21, 2023 •

edited

Loading

gloriousCode left a comment •

edited

Loading

gbjk commented Jan 21, 2024 •

edited

Loading