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

[Feature]: Pass user custom context to http.NewRequest in the client.go #198

Open
Halimao opened this issue Nov 28, 2024 · 6 comments
Open

Comments

@Halimao
Copy link

Halimao commented Nov 28, 2024

First of all, thank you for making this great SDK.

I want to pass different timeout contexts to different bybit APIs. Is it possible to do this?

This will break down the api, but passing custom context should be better than using default context.Background().

@hirokisan
Copy link
Owner

@Halimao Thank you for your comment!

It should be possible to achieve this by using WithHTTPClient.

For example, it might look something like this.

type customTransport struct {
  transport http.RoundTripper
}

func (t *customTransport) RoundTrip(req *http.Request) (*http.Response, error) {
  ctx, cancel := context.WithTimeout(req.Context(), 2*time.Second)
  defer cancel()

  req = req.WithContext(ctx)
  return t.transport.RoundTrip(req)
}

client := bybit.NewClient().WithHTTPClient(&http.Client{
  Transport: &customTransport{
    transport: http.DefaultTransport,
  },
})

ref. #74 (comment)

@Halimao
Copy link
Author

Halimao commented Nov 29, 2024

I am sorry, I think changing http client timeout will affect all api requests based on this client. Do I miss something here?

For example, I wanna set 5s timeout for 'cancel order' api request, but 1s timeout for 'create order' api request. How to do this with custom http client?

@hirokisan
Copy link
Owner

@Halimao

I see.

I think it is possible to judge by the path.

How about something like this?

func (t *customTransport) RoundTrip(req *http.Request) (*http.Response, error) {
    var timeout time.Duration

    switch req.URL.Path {
    case "/v5/order/cancel":
        timeout = 5 * time.Second
    case "/v5/order/create":
        timeout = 1 * time.Second
    default:
        timeout = 2 * time.Second
    }

    ctx, cancel := context.WithTimeout(req.Context(), timeout)
    defer cancel()

    req = req.WithContext(ctx)
    return t.transport.RoundTrip(req)
}

@Halimao
Copy link
Author

Halimao commented Nov 29, 2024

@hirokisan Okay, I thought about this way before, but this is indeed unacceptable to me.

Actually, I prefer to implement through the way following😁:

Passing context through the whole call chain is always more flexible.

image

@hirokisan
Copy link
Owner

I see. It's like this, isn't it?

type V5OrderServiceI interface {
  CreateOrder(context.Context, V5CreateOrderParam) (*V5CreateOrderResponse, error)
  CancelOrder(context.Context, V5CancelOrderParam) (*V5CancelOrderResponse, error)
}

It seems to me that we should be cautious because of the changes that will be made to the I/F.

However, the change itself seems to be a good idea from a flexibility standpoint.

@Halimao
Copy link
Author

Halimao commented Nov 30, 2024

Yes, this is an api-breaking change.

Maybe we could add this feature if there is a plan for the next version v3 😁

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

No branches or pull requests

2 participants