bybit: Add websocket trading functionality across all assets #1672

shazbert · 2024-10-10T02:05:57Z

PR Description

Implement trading functions through specific dedicated websocket connection
Expands IBotExchange wrapper functions to include websocket order operations
Adds validation methods on PlaceOrderParams, AmendOrderParams, CancelOrderParams to be used across websocket and rest.

#1670 Requires dependency

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 with my changes
Any dependent changes have been merged and published in downstream modules

…onnection

Co-authored-by: Scott <gloriousCode@users.noreply.github.com>

Co-authored-by: Adrian Gallagher <adrian.gallagher@thrasher.io>

Copilot

Pull Request Overview

This PR implements websocket trading functionality for the Bybit exchange, expanding the trading capabilities from REST-only to include real-time websocket operations. The implementation adds a dedicated websocket connection for outbound trading requests and expands the exchange wrapper to support websocket-based order operations.

Key changes:

Adds websocket trading operations (create, modify, cancel orders) through dedicated trade connection
Implements validation methods for order parameters to be shared across REST and websocket endpoints
Expands exchange interface with websocket order management methods

Reviewed Changes

Copilot reviewed 20 out of 20 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
exchanges/request/limit.go	Adds no-op rate limiter constant for websocket subscriptions
exchanges/interfaces.go	Extends OrderManagement interface with websocket order operations
exchanges/exchange_types.go	Adds WebsocketTrade URL type for dedicated trading connections
exchanges/exchange.go	Implements base websocket order methods returning not supported errors
exchanges/bybit/bybit_wrapper.go	Major refactoring to support websocket trading with shared validation logic
exchanges/bybit/bybit_websocket_requests.go	New file implementing websocket trading request handlers
exchanges/bybit/ratelimit.go	Adds websocket-specific rate limits and category mapping
exchanges/bybit/bybit_websocket.go	Adds trade connection authentication and data handling
exchanges/bybit/bybit_types.go	Adds validation methods and websocket response types
exchange/websocket/*.go	Extends websocket framework to support subscription-optional connections

Comments suppressed due to low confidence (4)

exchanges/bybit/ratelimit.go:81

The comment references a function 'GetRateLimit' but the function was removed and replaced with a variable 'rateLimits'. The comment should be updated to reflect the current implementation or removed entirely.

var rateLimits = request.RateLimitDefinitions{

exchanges/bybit/ratelimit.go:82

The comment above this line references the removed 'GetRateLimit' function. This function signature line appears to be leftover from a refactoring and should be removed.

	defaultEPL:                               request.NewRateLimitWithWeight(time.Second*5 /* See: https://bybit-exchange.github.io/docs/v5/rate-limit */, 600, 1),

exchanges/bybit/ratelimit.go:83

This return statement appears to be leftover from the removed GetRateLimit function and should be removed since it's now replaced by the rateLimits variable.

	createOrderEPL:                           request.NewRateLimitWithWeight(time.Second, 10, 10),

exchanges/bybit/bybit_types.go:326

The field name 'WhetherToBorrow' is verbose and unclear. It has been replaced with 'EnableBorrow' which is more concise and descriptive. This line should be removed as it appears to be leftover from refactoring.

	EnableBorrow           bool          `json:"-"`                          // '0' for default spot, '1' for Margin trading.

cranktakular

All my suggestions are pretty minor, those aside I can tACK this.

I don't have the credentials to check authenticated stuff, and I haven't had time to check implementations against the documentation, yet.

Relevant functions missing seemingly-achievable coverage: PlaceOrderParams.Validate, PlaceOrderParams.LoadID, AmendOrderParams.Validate, AmendOrderParams.LoadID, AllZero, CancelOrderParams.Validate, CancelOrderParams.LoadID, WebsocketAuthenticatePrivateConnection, WebsocketAuthenticateTradeConnection, wsHandleTradeData, wsHandleAuthenticatedData, Setup, DeriveSubmitOrderArguments, DeriveAmendOrderArguments, DeriveCancelOrderArguments
I've tried my best to prune ones which were missing coverage due to my lack of credentials, but I may have missed some.

Also, don't forget the checklist in the initial post!!

cranktakular · 2025-08-12T04:45:04Z

exchange/websocket/connection.go

@@ -480,3 +513,8 @@ func removeURLQueryString(url string) string {
 func (c *connection) RequireMatchWithData(signature any, incoming []byte) error {
 	return c.Match.RequireMatchWithData(signature, incoming)
 }
+
+// IncomingWithData routes incoming data using the connection specific match system to the correct handler
+func (c *connection) IncomingWithData(signature any, data []byte) bool {


What's the point of this function? Both this and the function it's calling are exported, no data's being added, it's just a slightly less verbose way to call it.

Is that worth it?

It's for fall back, exchanges send requestIDs or similar but we don't need to match back to a caller if not present and the data can be handled sent through the data handler. That is the only difference. Probably can be refactored but OOS.

exchanges/bybit/bybit_types.go

cranktakular · 2025-08-12T05:07:15Z

exchanges/bybit/bybit_types.go

@@ -343,6 +350,60 @@ type PlaceOrderParams struct {
 	SlLimitPrice float64 `json:"slLimitPrice,omitempty,string"`
 }

+// Validate checks the input parameters and returns an error if they are invalid.
+func (p *PlaceOrderParams) Validate() error {


I would like us to have some overarching idea on whether there should be functions in type files or not.

I generally hug validation, marshal, unmarshal methods to types because I am trying to quickly implement, and its easier for me to reference and doesn't pollute the API.go file. If you want me to move it I can.

I think that would be nice 🕊️

exchanges/bybit/bybit_types.go

cranktakular · 2025-08-12T06:53:39Z

exchanges/bybit/bybit_websocket_requests.go

+}
+
+// WebsocketOrderDetails is the order details from the websocket response.
+type WebsocketOrderDetails struct {


I'm similarly unsure about including multiple beefy type declarations in a non-types file.

I can shove them in bybit_websocket_requests_types.go file?

Sounds nice 🕊️

cranktakular · 2025-08-12T07:00:22Z

exchanges/bybit/bybit_websocket.go

-	creds, err := e.GetCredentials(ctx)
+// WebsocketAuthenticatePrivateConnection sends an authentication message to the private websocket for inbound account
+// data
+func (e *Exchange) WebsocketAuthenticatePrivateConnection(ctx context.Context, conn websocket.Connection) error {


I think this function deserves a different name. Both WebsocketAuthenticatePrivateConnection and WebsocketAuthenticateTradeConnection are handling private data.

How about WebsocketAuthenticateReadConnection and WebsocketAuthenticateWriteConnection? Not great, but they're the best I can come up with.

I am just mapping the functions with bybit's API terminology for connection name.

cranktakular · 2025-08-12T07:15:07Z

exchanges/bybit/bybit_websocket.go

+		// Below provides a way of matching an order change to a websocket request. There is no batch support for this
+		// so the first element will be used to match the order link ID.
+		if id, err := jsonparser.GetString(respRaw, "data", "[0]", "orderLinkId"); err == nil {
+			if conn.IncomingWithData(id, respRaw) {


Seems conceptually weird to send the entire respRaw through, if it's only being matched with the first element. Smells like there's a risk of additional data getting swept in there and matched incorrectly.

If it does it will error. Then we can fix it.

exchanges/bybit/bybit_websocket_requests.go

cranktakular · 2025-08-12T07:25:59Z

exchanges/bybit/bybit_websocket_requests.go

+	CumExecQty           types.Number  `json:"cumExecQty"`
+	CumExecValue         types.Number  `json:"cumExecValue"`
+	CumExecFee           types.Number  `json:"cumExecFee"`
+	ClosedPnl            types.Number  `json:"closedPnl"`


GK has said acrnoyms and the like should all be in the same case. That hits a few variable names here, Pnl > PNL, Oco > OCO, Tpsl > TPSL, etc.

I think because we aren't consistent with this across the codebase unlike ID; this is unnecessary pedantry. We can get an AI to standardise this and add in a misc linter for it if anybody cares, but I don't.

It would have taken you less keyboard presses to change it than to reply 😭
It is a rule we try to adhere to, it would. be preferable

🥃 @cranktakular
Sorry; I was a bit lazy on your review, and didn't add citations.

Initialisms and casing is part of the go style guide which GCT now references explicitly

So, yes, I did say that - but I was just highlighting an existing rule for us.

shazbert added 30 commits August 30, 2024 16:22

linter: fix

d29893b

Merge branch 'master' into gateio_ws

79e0eeb

Merge branch 'master' into gateio_ws

4f7bcd9

After merge

45ff199

Add error connection info

06acaac

Merge branch 'gateio_ws' into ws_outbound_req

aa87e83

upgrade to upstream merge

cf2e8d8

Merge branch 'ws_outbound_req' into ws_oubtound_req_futures

e9d50e4

Fix edge case where it does not reconnect made by an already closed c…

af986da

…onnection

stream coverage

1171278

glorious: nits

4be6214

Merge branch 'master' into gateio_ws

a068ddb

glorious: nits removed asset error handling in stream package

81cba36

linter: fix

bdc6954

rm block

3fe44ca

Add basic readme

818584f

Merge branch 'master' into gateio_ws

289ac71

Merge branch 'master' into gateio_ws

dee32a4

Merge branch 'master' into gateio_ws

7063311

fix asset enabled flush cycle for multi connection

0709ba3

spella: fix

b877b38

linter: fix

f7d1ec8

Merge branch 'master' into gateio_ws

818432d

Add glorious suggestions, fix some race thing

bdc7afb

reinstate name before any routine gets spawned

3d6541e

stop on error in mock tests

33c4128

glorious: nits

ea25763

Merge branch 'gateio_ws' into ws_outbound_req

f15374a

Merge branch 'ws_outbound_req' into ws_oubtound_req_futures

ce00326

Set correct price

262c6ef

Ryan O'Hara-Reid and others added 13 commits August 6, 2025 11:15

glorious: nits

7f5b5b2

fix test

6c8a6e2

Merge branch 'master' into bybit_multi_out

1a5a147

fix alignment issue and rm println

96e9912

Update exchanges/bybit/bybit_websocket.go

447ab21

Co-authored-by: Scott <gloriousCode@users.noreply.github.com>

Update exchanges/bybit/bybit_websocket.go

2f23ee5

Co-authored-by: Scott <gloriousCode@users.noreply.github.com>

glorious: fix

80a5edb

Update exchanges/bybit/bybit_websocket.go

05cc628

Co-authored-by: Adrian Gallagher <adrian.gallagher@thrasher.io>

Merge branch 'bybit_multi_out' into bybit_multi_out_requests

8d9b7f1

bybit: use connection method for segregated match on multi-connection

4821ce2

Merge branch 'master' into bybit_multi_out_requests

93a288a

cleanup after master merge

2164a75

fix test and config whoops

ff21cd4

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 labels Aug 8, 2025

shazbert requested review from gbjk, thrasher-, gloriousCode, samuael, cranktakular and Copilot August 8, 2025 06:07

Copilot AI reviewed Aug 8, 2025

View reviewed changes

cranktakular requested changes Aug 12, 2025

View reviewed changes

shazbert added 3 commits August 13, 2025 11:03

cranktakular: nits

da74ff9

Merge branch 'master' into bybit_multi_out_requests

2d17b4d

exchange: add missing tests for base method websocket order funcs

c1dc0d5

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 13, 2025

Ryan O'Hara-Reid added 2 commits August 27, 2025 11:23

Merge branch 'master' into bybit_multi_out_requests

5f51152

Merge branch 'master' into bybit_multi_out_requests

31fe612

bybit: Add websocket trading functionality across all assets #1672

Are you sure you want to change the base?

bybit: Add websocket trading functionality across all assets #1672

Conversation

shazbert commented Oct 10, 2024

PR Description

Type of change

How has this been tested

Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

cranktakular left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gloriousCode Aug 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gloriousCode Aug 13, 2025 •

edited

Loading