Skip to content
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

feat: added wss futures client and implemted 'order.place', 'order.ca… #609

Merged
merged 4 commits into from
Nov 6, 2024

Conversation

vigodsky
Copy link
Contributor

@vigodsky vigodsky commented Aug 23, 2024

Hello,
Here is implementation of Binance Futures wss client with reconnect backoff and send/receive functions
Also two endpoints were implemented - 'order.place' and 'order.cancel'

Example of usage

func main() {
    orderPlaceService, _ := futures.NewOrderPlaceWsService(apiKey, secretKey)
    
    ctx, cancel := context.WithCancel(context.Background())
    
    c := make(chan os.Signal, 1)
    signal.Notify(c, os.Interrupt)
    go func() {
        select {
            case <-c:
            cancel()
        }
    }()

    request := futures.NewOrderPlaceWsRequest()
    request.
        Symbol("BTCUSDT").
        Side(futures.SideTypeSell).
        Type(futures.OrderTypeLimit).
        Price("68198.00").
        Quantity("0.002").
        TimeInForce(futures.TimeInForceTypeGTC)

    // sender
    go func() {
        for {
            select {
            case <-ctx.Done():
                return
            default:
                err := orderPlaceService.Do("id", request)
                if err != nil {
                    return
                }
            }
        }
    }()

    wg := &sync.WaitGroup{}
    wg.Add(1)
    go listenOrderPlaceResponse(ctx, wg, orderPlaceService)
    wg.Wait()

    log.Println("exit")
}

func listenOrderPlaceResponse(ctx context.Context, wg *sync.WaitGroup, orderPlaceService *futures.OrderPlaceWsService) {
    defer wg.Done()
    
    go func() {
        for msg := range orderPlaceService.GetReadChannel() {
            log.Println("order place response", string(msg))
        }
    }()
    
    go func() {
        for err := range orderPlaceService.GetReadErrorChannel() {
            log.Println("order place error", err)
        }
    }()

    select {
    case <-ctx.Done():
        orderPlaceService.ReceiveAllDataBeforeStop(10 * time.Second)
    }
}

Feel free to ask questions!

@xyq-c-cpp
Copy link
Collaborator

xyq-c-cpp commented Aug 26, 2024

  1. replace getSignature with the function of v2/common/sign.go, and it also should support three sign method in wss api. Implement it properly.
  2. add support proxy for websocket api client, is okay?
  3. add the unit test for order_service_ws.go
  4. can XxxWsService.do support sync method? like the rest api that send a request and return a target response. we talk more about this.
    @vigodsky

@adshao take a review this.

@vigodsky
Copy link
Contributor Author

vigodsky commented Aug 26, 2024

Hi @xyq-c-cpp ,
could you please give more details of proxy for client and sync method? Maybe you have other discussions about it, I did not get it

@xyq-c-cpp
Copy link
Collaborator

xyq-c-cpp commented Aug 26, 2024

  1. What I mean is that the WebSocket API needs to support network proxy. I saw that your code already supports network proxy, but it is a default value. The implementation of the wsServe function in v2/futures/websocket.go is related to this @vigodsky
  2. The meaning of synchronization method is whether an interface can be provided to send requests, read and return them in the implementation, and ultimately return the target structure such as CreateOrderWs Response to the user. @vigodsky

@vigodsky
Copy link
Contributor Author

@xyq-c-cpp
Thanks, now it's clear!

@xyq-c-cpp
Copy link
Collaborator

@xyq-c-cpp Thanks, now it's clear!

hello, do you still work for this? @vigodsky

@vigodsky
Copy link
Contributor Author

Hi @xyq-c-cpp ,

Yes, I'm still working, almost done

Thanks

@vigodsky
Copy link
Contributor Author

vigodsky commented Sep 23, 2024

Hello @xyq-c-cpp ,
I added changes that you proposed, please check.
WriteSync method should be used without using async write into socket. Current implementation is not supported any kind of routing for parallel using websocket connection.
If application needs to have multiple workers with WriteSync method it can use multiple instance of services (service=client=single connection).

I also moved keep alive handler in connection state and added some mutex for fix data race in it.

@vigodsky vigodsky force-pushed the feat/wss-futures-client branch from e585cb5 to 8a2cb02 Compare September 23, 2024 16:06
…s, sync write method and support 3 sign functions
@vigodsky vigodsky force-pushed the feat/wss-futures-client branch from 8a2cb02 to 2c5c386 Compare September 23, 2024 16:07
v2/futures/order_service_ws.go Outdated Show resolved Hide resolved
v2/futures/websocket.go Outdated Show resolved Hide resolved
v2/futures/client_ws.go Show resolved Hide resolved

// NewOrderPlaceWsService init OrderPlaceWsService
func NewOrderPlaceWsService(apiKey, secretKey string) (*OrderPlaceWsService, error) {
conn, err := newConnection()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whether synchronous or asynchronous, the same connection is required, which is a long connection in WebSocket. Is this the current situation where all these similar interfaces share a common connection. is that right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No it's not. Due to limited time I created WriteSync method that using the same connection but leaved comment for this. If you propose to have separate client with it's own connection, I agree. But It will takes sometime to implement

Copy link
Contributor Author

@vigodsky vigodsky Oct 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current scheme is
service->client(sync && async write)->single connection

OrderPlaceService and OrderCancel will have different connections
I think it's ok to not have only one connection for all services,
It's easier and more fault tolerant.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know, but the biggest advantage of WebSocket is sharing long connections. If you use a separate connection, I think it's better to use the REST interface. This is also why some people use WebSocket. Please take some time to implement this, it's almost done. @vigodsky

@xyq-c-cpp
Copy link
Collaborator

@vigodsky hello, take a look to the review.

@xyq-c-cpp
Copy link
Collaborator

@vigodsky hello, take a look to the review.

@vigodsky hello, do you see this message?

@vigodsky
Copy link
Contributor Author

vigodsky commented Oct 8, 2024

@xyq-c-cpp Hi,
Yes I saw your messages,
sorry I don't have enough time to do due to my job
I will fix/answer to you as soon as I will have free time

@xyq-c-cpp
Copy link
Collaborator

@xyq-c-cpp Hi, Yes I saw your messages, sorry I don't have enough time to do due to my job I will fix/answer to you as soon as I will have free time

got it. @vigodsky

@vigodsky
Copy link
Contributor Author

Hi @xyq-c-cpp
I answered to your comments, If you propose to have separate client for sync calls, I will implement it

@xyq-c-cpp
Copy link
Collaborator

Hi @xyq-c-cpp I answered to your comments, If you propose to have separate client for sync calls, I will implement it

It means sharing one connection for all request service interfaces, is that okay? if you see this message, please leave a comment to let me know @vigodsky

@vigodsky
Copy link
Contributor Author

vigodsky commented Oct 21, 2024

Hi @xyq-c-cpp
Now I understand you.

But I'd like to explain why it was chosen that way.
Having several websocket connections (one for each endpoint) is not such a big problem in terms of CPU or other resources consumption, especially since Binance has quite high limits on orders per second (from their documentation this parameter varies from 1200 to 1800 per minute on VIP accounts). I mean, we don't need a lot of performance in this case in terms of networking. Plus in this case we have better fault tolerance - one downed connection does not affect the others and vice versa.
As for what you suggested using REST as an alternative - Binance's HTTP API has seriously degraded recently in terms of processing a single order (I don't know what this is related to). After switching to the websocket API, orders started to be executed 4 times faster than via HTTP.

Your suggestion to make a single connection also has its pros and cons, but it introduces additional complexity to create routing on the library side. You will have to decide which channel to send the message to. I tried to make such an implementation, but ran into some problems:
As you might have noticed, a hash map is used to store the list of sent requests (the main purpose of it is to wait all data from connection while we stopped app), and the first problem was that if an application sends the same request with the same ID, it cannot be processed (ID duplication).

For example, simultaneous execution of
OrderPlace(“ID”, Request)
ModfyOrder(“ID”, Request)
may cause this problem.

This could be solved by using a composite key that we can pass to clientOrderID for Binance to give it back to us
For example,
ID_order.place
ID_order.modify
In this case, we could then understand from the response from Binance what the recipient of this message is.
But there is a problem with validation on the Binance side, it does not allow adding more characters to this field than there are in the UUID.

Faced with these limitations, I came to the conclusion that splitting the connets would be the simplest implementation.

@vigodsky
Copy link
Contributor Author

vigodsky commented Nov 4, 2024

@xyq-c-cpp Hi,
Can we use this approach for first implementation?

xyq-c-cpp
xyq-c-cpp previously approved these changes Nov 6, 2024
@xyq-c-cpp xyq-c-cpp merged commit 5c80e99 into adshao:master Nov 6, 2024
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants