orderbook: Check assignment of time values and reject if not set #1318

shazbert · 2023-08-11T04:50:54Z

PR Description

From PR #1315 some timestamps where not set. To maintain adequate data quality this should be strictly set and this PR rejects all snapshots and updates without a time value.

NOTES:

exchanges need to be checked REST and WS functionality.
bitfinex,okx: Fix orderbook LastUpdate timestamps #1315 should be merged first.

Fixes # (issue)

Type of change

Please delete options that are not relevant and add an x in [] as item is complete.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

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.

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

Checklist

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation and regenerated documentation via the documentation tool
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally and on Github Actions/AppVeyor with my changes
Any dependent changes have been merged and published in downstream modules

thrasher-

Nice QA PR! First round of nitterinos

thrasher- · 2023-08-22T00:08:04Z

exchanges/bitmex/bitmex.go

@@ -111,6 +111,8 @@ const (
 	bitMEXPriceIndexID          = "MRCXXX"
 	bitMEXLendingPremiumIndexID = "MRRXXX"
 	bitMEXVolatilityIndexID     = "MRIXXX"
+
+	timeLayout = "2006-01-02T15:04:05.999Z"


Can rid this since Go std can handle it natively

thrasher- · 2023-08-22T00:08:15Z

exchanges/bitmex/bitmex_types.go

+	Side      string  `json:"side"`
+	Size      int64   `json:"size"`
+	Symbol    string  `json:"symbol"`
+	Timestamp string  `json:"timestamp"`


Suggested change

Timestamp string `json:"timestamp"`

Timestamp time.Time `json:"timestamp"`

thrasher- · 2023-08-22T00:08:40Z

exchanges/coinbasepro/coinbasepro_test.go

@@ -862,7 +863,7 @@ func TestWsOrderbook(t *testing.T) {
 	pressXToJSON = []byte(`{
  "type": "l2update",
  "product_id": "BTC-USD",
-  "time": "2019-08-14T20:42:27.265Z",
+  "time": "2023-08-15T06:46:57.933713Z"",


Suggested change

"time": "2023-08-15T06:46:57.933713Z"",

"time": "2023-08-15T06:46:57.933713Z",

thrasher- · 2023-08-22T00:08:53Z

exchanges/coinbasepro/coinbasepro_types.go

@@ -441,6 +441,7 @@ type WebsocketOrderbookSnapshot struct {
 	Type      string      `json:"type"`
 	Bids      [][2]string `json:"bids"`
 	Asks      [][2]string `json:"asks"`
+	Time      string      `json:"time"`


Suggested change

Time string `json:"time"`

Time time.Time `json:"time"`

thrasher- · 2023-08-22T00:09:38Z

exchanges/coinbasepro/coinbasepro.go

@@ -50,6 +50,8 @@ const (
 	coinbaseproWithdrawalCrypto        = "withdrawals/crypto"
 	coinbaseproCoinbaseAccounts        = "coinbase-accounts"
 	coinbaseproTrailingVolume          = "users/self/trailing-volume"
+
+	timeLayout = "2006-01-02T15:04:05.999999Z"


Can rid this as Go can handle this

thrasher- · 2023-08-22T00:13:45Z

exchanges/hitbtc/hitbtc_types.go

-		Sequence int64  `json:"sequence"`
+		Symbol    string `json:"symbol"`
+		Sequence  int64  `json:"sequence"`
+		Timestamp string `json:"timestamp"`


Suggested change

Timestamp string `json:"timestamp"`

Timestamp time.Time `json:"timestamp"`

gloriousCode

Lovely

🕊️

gloriousCode · 2023-08-30T23:25:25Z

exchanges/btse/btse_websocket.go

@@ -265,7 +269,7 @@ func (b *BTSE) wsHandleData(respRaw []byte) error {
 			})
 		}
 		return trade.AddTradesToBuffer(b.Name, trades...)
-	case strings.Contains(topic, "orderBookL2Api"):
+	case strings.Contains(topic, "orderBookL2Api"): // TODO: Fix orderbook updates.


What's happened?

BTSE websocket orderbook not working. 🤷

gloriousCode · 2023-08-30T23:29:10Z

exchanges/orderbook/depth_test.go

@@ -253,7 +264,7 @@ func TestInvalidate(t *testing.T) {
 		t.Fatalf("received: '%v' but expected: '%v'", err, ErrOrderbookInvalid)
 	}

-	if err.Error() != "testexchange BTCWABI spot orderbook data integrity compromised Reason: [random reason]" {
+	if err.Error() != "testexchange BTCWABI spot Reason: [orderbook data integrity compromised, random reason]" {


Only if something else comes up, make this errors.Is

gloriousCode · 2023-08-30T23:44:33Z

exchanges/poloniex/poloniex_websocket.go

@@ -391,6 +391,20 @@ func (p *Poloniex) WsProcessOrderbookSnapshot(data []interface{}) error {
 			errTypeAssertionFailure)
 	}

+	if len(data) < 3 {
+		return errNotEnoughData


I think you should wrap this error with the above pair at least to help know that maybe its a terrible currency

exchanges/orderbook/depth.go

gloriousCode · 2023-08-30T23:59:36Z

exchanges/orderbook/depth.go

+	if update.UpdateTime.IsZero() {
+		return fmt.Errorf("%s %s %s %w",
+			d.exchange,
+			d.pair,
+			d.asset,
+			errLastUpdatedNotSet)
+	}


Add coverage

gloriousCode · 2023-08-30T23:59:39Z

exchanges/orderbook/depth.go

+	if update.UpdateTime.IsZero() {
+		return fmt.Errorf("%s %s %s %w",
+			d.exchange,
+			d.pair,
+			d.asset,
+			errLastUpdatedNotSet)
+	}


Add coverage

gloriousCode · 2023-08-30T23:59:42Z

exchanges/orderbook/depth.go

+	if update.UpdateTime.IsZero() {
+		return fmt.Errorf("%s %s %s %w",
+			d.exchange,
+			d.pair,
+			d.asset,
+			errLastUpdatedNotSet)
+	}


Add coverage

gloriousCode · 2023-08-31T00:05:33Z

exchanges/orderbook/depth.go

+			d.pair,
+			d.asset,
+			errLastUpdatedNotSet)
+	}
 	tn := getNow()


Is this still needed now we have an update time?

Nar not really, Last updated is more a time for when the update occurred at a matching engine level on the external exchange. This time is for the stack nodes when it gets pushed onto the stack to be re-used later more like an insertion time so it can be clean if its never been used. Either way the times are going to be like a second or two out depending on colocation from exchange and processing time. Default allowance is 30 seconds so might as well use it and reduce allocation. 🤷 🦅

thrasher-

Thanks for making those changes!

gloriousCode

thanks ACK!

…asher-corp#1318) * orderbook: Check assignment of time values and reject if not set. * linter: fix * buffer: additional linter winter fixter * Implement through pending exchanges * finished push * linty: minty * gomod: tidy * thrasher: nits * glorious: nits * orderbook: purge type now in favour of external call allocation * orderbook: push last param * orderbook: only 1 unlock call is needed --------- Co-authored-by: Ryan O'Hara-Reid <[email protected]>

orderbook: Check assignment of time values and reject if not set.

6dc9f5f

shazbert added nomerge requires dependency This pull request is dependent on another, so it can't be merged until the dependent one is merged blocked labels Aug 11, 2023

shazbert requested a review from a team August 11, 2023 04:50

shazbert self-assigned this Aug 11, 2023

linter: fix

1c20137

shazbert added review me This pull request is ready for review and removed nomerge requires dependency This pull request is dependent on another, so it can't be merged until the dependent one is merged blocked labels Aug 15, 2023

Ryan O'Hara-Reid added 2 commits August 15, 2023 15:24

buffer: additional linter winter fixter

77ef0c6

Implement through pending exchanges

6c8468a

shazbert 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 Aug 15, 2023

Ryan O'Hara-Reid added 2 commits August 16, 2023 10:18

finished push

4d99c4c

Merge branch 'master' into book_time

7b9c899

shazbert added review me This pull request is ready for review and removed reconstructing Based on PR feedback, this is currently being reworked and is not to be merged labels Aug 16, 2023

linty: minty

d680cd9

shazbert added the rebase/merge of master required label Aug 21, 2023

Ryan O'Hara-Reid added 2 commits August 22, 2023 09:48

Merge branch 'master' into book_time

6ec9a8b

gomod: tidy

48253ec

shazbert removed the rebase/merge of master required label Aug 21, 2023

thrasher- requested changes Aug 22, 2023

View reviewed changes

Ryan O'Hara-Reid added 2 commits August 22, 2023 10:38

thrasher: nits

25837b7

Merge branch 'master' into book_time

f25c2bc

gloriousCode requested changes Aug 31, 2023

View reviewed changes

gloriousCode reviewed Aug 31, 2023

View reviewed changes

Ryan O'Hara-Reid added 3 commits September 1, 2023 11:10

glorious: nits

e438965

orderbook: purge type now in favour of external call allocation

9339470

orderbook: push last param

fc7d81c

orderbook: only 1 unlock call is needed

fca7805

thrasher- approved these changes Sep 6, 2023

View reviewed changes

gloriousCode approved these changes Sep 7, 2023

View reviewed changes

thrasher- changed the title ~~orderbook: Check assignment of time values and reject if not set.~~ orderbook: Check assignment of time values and reject if not set Sep 7, 2023

thrasher- merged commit ad9de19 into thrasher-corp:master Sep 7, 2023

shazbert deleted the book_time branch May 3, 2024 00:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

orderbook: Check assignment of time values and reject if not set #1318

orderbook: Check assignment of time values and reject if not set #1318

shazbert commented Aug 11, 2023

thrasher- left a comment

thrasher- Aug 22, 2023

thrasher- Aug 22, 2023

thrasher- Aug 22, 2023

shazbert Aug 22, 2023

thrasher- Aug 22, 2023

thrasher- Aug 22, 2023

thrasher- Aug 22, 2023

gloriousCode left a comment

gloriousCode Aug 30, 2023

shazbert Sep 1, 2023

gloriousCode Aug 30, 2023

gloriousCode Aug 30, 2023

gloriousCode Aug 30, 2023

gloriousCode Aug 30, 2023

gloriousCode Aug 30, 2023

gloriousCode Aug 31, 2023 •

edited

Loading

shazbert Sep 1, 2023

thrasher- left a comment

gloriousCode left a comment

	Timestamp string `json:"timestamp"`
	Timestamp time.Time `json:"timestamp"`

	"time": "2023-08-15T06:46:57.933713Z"",
	"time": "2023-08-15T06:46:57.933713Z",

orderbook: Check assignment of time values and reject if not set #1318

orderbook: Check assignment of time values and reject if not set #1318

Conversation

shazbert commented Aug 11, 2023

PR Description

Type of change

How has this been tested

Checklist

thrasher- left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gloriousCode left a comment

Choose a reason for hiding this comment

🕊️

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gloriousCode Aug 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thrasher- left a comment

Choose a reason for hiding this comment

gloriousCode left a comment

Choose a reason for hiding this comment

gloriousCode Aug 31, 2023 •

edited

Loading