okx: decouple outbound trading functionality from multiplexer #1600
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1600 +/- ##
==========================================
+ Coverage 37.09% 37.17% +0.08%
==========================================
Files 414 415 +1
Lines 180264 179960 -304
==========================================
+ Hits 66860 66902 +42
+ Misses 105549 105203 -346
Partials 7855 7855
|
|
Thank you for your changes on the Okx exchange and for inviting me to review them. This PR is very optimized and very clean. The only proposal I have on this PR is to eliminate the Multiplexer by handling the incoming data by checking possible login error codes from the incoming data and handling the data based on that filter; Since it is only used by the "login" operation. For instance, these are the possible incoming login error codes that can be used to filter login errors.
|
|
This is the change I am referring to. You might need to adjust a few things, but I have attempted to implement my intended modifications to eliminate the multiplexer. |
|
@samuael I added in your patch and did some adjustments to a type and the matching signature. Thanks heaps for looking into this. |
It was speculation, I can remove that wording? So this PR removes what was happening before including the inline JSON calls in the websocket reader/handler, I haven't tested it in a benchmark. That was the reason I suggested it had an higher probability of improved websocket message handling. I can see now why those choice of words are not entirely correct. |
|
Opted to label this as medium priority to highlight that it's complementary to #1420 and it's updates. This does not necessarily need to be merged first but it reduces a lot of code complexity. |
| if intermediary.Code > 1 { | ||
| return fmt.Errorf("error code: %d message: %s", intermediary.Code, intermediary.Message) | ||
| } |
There was a problem hiding this comment.
Order code 2 was previously handled. Your comments on partial success in those functions likely won't get hit with a response of 2
Also Code 1 is failure
edit: hang on, this might be REST
There was a problem hiding this comment.
Yeah good catch; I bumped it to 2 so that anything beyond that can fail here, 1&2 codes don't actually need to fail here. None of my testing actually returned a code 2 that's why I must have forgotten to put it in. Example:
Okx wss://ws.okx.com:8443/ws/v5/private: Sending message: {"id":"1","op":"batch-orders","args":[{"instId":"BTC-USDT","tdMode":"cash","ccy":"USDT","side":"buy","ordType":"limit","sz":"0.0001","px":"-1"},{"instId":"BTC-USDT","tdMode":"cash","ccy":"USDT","side":"buy","ordType":"limit","sz":"0.0001","px":"20000"}]}
Okx wss://ws.okx.com:8443/ws/v5/private: Received response [1/1]: {"id":"1","op":"batch-orders","code":"2","msg":"","data":[{"tag":"","ts":"1728858915989","ordId":"","clOrdId":"","sCode":"51000","sMsg":"Parameter px error"},{"tag":"","ts":"1728858915990","ordId":"1891010883570696192","clOrdId":"","sCode":"0","sMsg":"Order successfully placed."}],"inTime":"1728858915989134","outTime":"1728858915990899"}
Correct code 1 is a failure but the data holds the specifics of the error in sCode and sMsg and that can be checked in the calling function.
There was a problem hiding this comment.
I get that you're trying to put the onus on the caller, but you also don't give the caller the opportunity to do so since you only return the Data property and nothing else. Then with this:
if len(orderResp) != 1 {
return nil, errNoValidResponseFromServer
}There could be a Message and Code (of value 1) from the parent which could contain details but its instead thrown out and you don't know why.
How about the check is Seems like this can't apply
if intermediary.Code != 0 && intermediary.Code != 2 {edit: I want a better handling than errNoValidResponseFromServer
There was a problem hiding this comment.
All code 1 and code 2 API examples don't produce a message; 1 is a total failure and 2 is a partial failure which have data returned with specific scode and smsg to each action. From testing anything above 2 which has a message component; I have only seen request errors pertaining to operation string, connection authentication issues and they don't send any data back for the caller. The only reason I added the length check was to protect against a possible panic.
Happy to add in whatever you want though send me a diff on what you propose and the response you have found anything while testing and I will update it.
shazbert
added
nomerge requires dependency
PR Description
The multiplexer was a good intermediary but added in overhead and code complexity. This PR shifts the burden of websocket message match management to stream.Match using the function SendMessageReturnResponse.
It also shifts the burden of JSON unmarshalling to the request calling routine and the message []byte is shifted via pointer,
which has a higher probability of improved websocket message handling.The function design attempts to mirror REST request flow.
This is probably going to be a multi-stage decoupling for multi-connection management.All done thanks Sam.For matching signature, have opted to use
Unix timestampcommon.Counter. There was slight overhead from common.GenerateRandomString.Removes WsRequestSemaphore due to upstream changes incorporating request rate limiter.
Adds TODO to export request rate limiting functionality so that we can couple rate limits between REST and websocket connections.
Waiting for exchanges: Okx Update #1420
Type of change
Please delete options that are not relevant and add an
xin[]as item is complete.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.
Checklist