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: [bitget] support query trades #1400

Merged
merged 1 commit into from Nov 10, 2023
Merged

Conversation

bailantaotao
Copy link
Collaborator

@bbgokarma-bot
Copy link

Welcome back! @bailantaotao, This pull request may get 733 BBG.

}

limit := options.Limit
if limit > queryOrdersLimit || limit <= 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be queryTradesLimit?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, both the trade and order have a limit of 100, so I am using it. I will rename it to queryLimit.

Comment on lines +405 to +443
if err := queryTradeRateLimiter.Wait(ctx); err != nil {
return nil, fmt.Errorf("trade rate limiter wait error: %w", err)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about moving this rate limit check to the beginning of this function?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The thought is to call it before req.Do, so that we can stay near the rate limit.

req := e.v2Client.NewGetTradeFillsRequest()
req.Symbol(symbol)

if options.StartTime != nil {
Copy link
Owner

Choose a reason for hiding this comment

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

if both StartTime and EndTime are given, you will need to check the time range?

Copy link
Collaborator

@ycdesu ycdesu Nov 8, 2023

Choose a reason for hiding this comment

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

Please ensure the end >= start if both conditions are given

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was just thinking that the caller should use batch.TradeBatchQuery, so I missed this part.

@bbgokarma-bot
Copy link

Re-estimated karma: this pull request may get 818 BBG

@bbgokarma-bot
Copy link

Re-estimated karma: this pull request may get 822 BBG

@bbgokarma-bot
Copy link

Re-estimated karma: this pull request may get 827 BBG

@bbgokarma-bot
Copy link

Re-estimated karma: this pull request may get 833 BBG

@bailantaotao bailantaotao force-pushed the edwin/bitget/submit-orders branch 3 times, most recently from bcf1691 to cb5e305 Compare November 10, 2023 07:47
@bbgokarma-bot
Copy link

Re-estimated karma: this pull request may get 838 BBG

@bbgokarma-bot
Copy link

Re-estimated karma: this pull request may get 843 BBG

Base automatically changed from edwin/bitget/submit-orders to main November 10, 2023 08:03
@bbgokarma-bot
Copy link

Re-estimated karma: this pull request may get 848 BBG

Copy link

codecov bot commented Nov 10, 2023

Codecov Report

Merging #1400 (a26b158) into main (58a810e) will increase coverage by 0.03%.
The diff coverage is 37.89%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1400      +/-   ##
==========================================
+ Coverage   21.24%   21.28%   +0.03%     
==========================================
  Files         576      577       +1     
  Lines       41707    41802      +95     
==========================================
+ Hits         8862     8898      +36     
- Misses      32200    32256      +56     
- Partials      645      648       +3     
Files Coverage Δ
...kg/exchange/bitget/bitgetapi/v2/get_trade_fills.go 0.00% <0.00%> (ø)
pkg/exchange/bitget/convert.go 88.93% <80.00%> (-2.12%) ⬇️
pkg/exchange/bitget/exchange.go 0.00% <0.00%> (ø)

Continue to review full report in Codecov by Sentry.

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

@bailantaotao bailantaotao merged commit 3daa2f8 into main Nov 10, 2023
4 checks passed
@bailantaotao bailantaotao deleted the edwin/bitget/query-trades branch November 10, 2023 08:39
@bbgokarma-bot
Copy link

Hi @bailantaotao,

Well done! 858 BBG has been sent to your polygon wallet. Please check the following tx:

https://polygonscan.com/tx/0xc4f5cf5b14184e58820f09abc817247f504160d57b62f23beb4523f0ad2af899

Thank you for your contribution!

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

5 participants