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

Order Rate Limit #320

Merged
merged 8 commits into from Nov 10, 2021
Merged

Order Rate Limit #320

merged 8 commits into from Nov 10, 2021

Conversation

amaioli
Copy link
Contributor

@amaioli amaioli commented Nov 2, 2021

It's a first tentative version to manage rate limit info from Binance. In this version I've extended the Order Response structure including 10s and 1m order trade limit from header response.

@codecov
Copy link

codecov bot commented Nov 2, 2021

Codecov Report

Merging #320 (40b753e) into master (13013b3) will decrease coverage by 0.03%.
The diff coverage is 86.79%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #320      +/-   ##
==========================================
- Coverage   74.51%   74.48%   -0.04%     
==========================================
  Files          58       58              
  Lines        5482     5487       +5     
==========================================
+ Hits         4085     4087       +2     
- Misses       1024     1025       +1     
- Partials      373      375       +2     
Impacted Files Coverage Δ
v2/futures/client.go 82.38% <57.14%> (ø)
v2/futures/order_service.go 59.25% <75.00%> (+0.21%) ⬆️
v2/futures/account_service.go 69.23% <100.00%> (ø)
v2/futures/depth_service.go 89.47% <100.00%> (ø)
v2/futures/exchange_info_service.go 78.82% <100.00%> (ø)
v2/futures/income_history.go 35.13% <100.00%> (ø)
v2/futures/kline_service.go 86.79% <100.00%> (ø)
v2/futures/mark_price.go 83.33% <100.00%> (ø)
v2/futures/position_margin_history.go 35.13% <100.00%> (ø)
v2/futures/position_risk.go 77.77% <100.00%> (ø)
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 13013b3...40b753e. Read the comment docs.

@@ -333,6 +337,20 @@ func (c *Client) callAPI(ctx context.Context, r *request, opts ...RequestOption)
}
return nil, apiErr
}

err = json.Unmarshal(data, &t)
Copy link
Owner

Choose a reason for hiding this comment

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

Not all the API response are map, for example, maybe array, so it will fail here.
It's not a good idea to mix response headers with response body, maybe we need a dedicated return value for headers:

func (c *Client) callAPI(ctx context.Context, r *request, opts ...RequestOption) (data []byte, headers Headers, err error) 

Copy link
Owner

Choose a reason for hiding this comment

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

And there will be performance issue to Unmarshal & Marshal to add headers for every API call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok let me change the codes adding a return. In that case we've to modify all the others functions but it's more clean

@amaioli
Copy link
Contributor Author

amaioli commented Nov 10, 2021

Hi adshao, did you have the chance to check the changes?

@adshao
Copy link
Owner

adshao commented Nov 10, 2021

Hi adshao, did you have the chance to check the changes?

Yes, I'm reviewing this pr, sorry for the late reply.
Thanks for the contribution! @amaioli

@adshao adshao merged commit 15a4341 into adshao:master Nov 10, 2021
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.

None yet

2 participants