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: add get_all_fee_rate and supported_interval #1322

Closed
wants to merge 4 commits into from

Conversation

MengShue
Copy link

  1. add GetAllFeeRates for future use, ex. using in stream.SetBeforeConnect()
  2. add supported interval, so I updated toLocalInterval()
  3. add unit test for them respectively

@bbgokarma-bot
Copy link

Welcome back! @MengShue, This pull request may get 616 BBG.

@codecov
Copy link

codecov bot commented Sep 26, 2023

Codecov Report

Merging #1322 (e75699a) into main (8f40478) will decrease coverage by 0.02%.
Report is 26 commits behind head on main.
The diff coverage is 11.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1322      +/-   ##
==========================================
- Coverage   20.86%   20.84%   -0.02%     
==========================================
  Files         556      557       +1     
  Lines       39621    39672      +51     
==========================================
+ Hits         8265     8269       +4     
- Misses      30757    30803      +46     
- Partials      599      600       +1     
Files Coverage Δ
pkg/risk/riskcontrol/circuit_break.go 77.77% <100.00%> (ø)
pkg/exchange/okex/exchange.go 0.00% <0.00%> (ø)
pkg/exchange/okex/convert.go 0.00% <0.00%> (ø)

... and 10 files with indirect coverage changes


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 cf31796...e75699a. Read the comment docs.

return ok
}

func (e *Exchange) GetAllFeeRates(ctx context.Context) (okexapi.FeeRateList, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why you need GetAllFeeRates?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Bybit uses it because the stream event does not return the fee and the fee currency.

Copy link
Author

Choose a reason for hiding this comment

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

ok, I will remove it, I found out 'order' websocket would return fee, so I don't need GetAllFeeRates
https://www.okx.com/docs-v5/en/?shell#order-book-trading-trade-ws-order-channel

return re.ReplaceAllStringFunc(src, func(w string) string {
return strings.ToUpper(w)
})
func toLocalInterval(interval types.Interval) (string, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add unit test?

Copy link
Author

Choose a reason for hiding this comment

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

in Test_QueryKlines, will add more unit test

@bbgokarma-bot
Copy link

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

})
func toLocalInterval(interval types.Interval) (string, error) {
if _, ok := okexapi.SupportedIntervals[interval]; !ok {
return "", fmt.Errorf("interval not supported: %s", interval)
Copy link
Owner

Choose a reason for hiding this comment

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

"interval %s is not supported"

Copy link
Author

Choose a reason for hiding this comment

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

updated

switch i := interval.String(); {
case strings.HasSuffix(i, "h"), strings.HasSuffix(i, "d"), strings.HasSuffix(i, "w"):
return strings.ToUpper(i), nil
case strings.HasSuffix(i, "mo"):
Copy link
Owner

Choose a reason for hiding this comment

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

this will result 4 calls to HasSuffix, is there a simpler way to convert this?

Copy link
Author

Choose a reason for hiding this comment

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

updated


import "github.com/c9s/bbgo/pkg/types"

var (
Copy link
Owner

Choose a reason for hiding this comment

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

should move this definition to the parent level package

Copy link
Author

Choose a reason for hiding this comment

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

ok ...... but I use pkg/exchange/okex/type.go in #1321 I will relocate struct in that PR

@bbgokarma-bot
Copy link

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

case strings.HasSuffix(i, "mo"):
return "1M", nil
default:
hdwRegex := regexp.MustCompile("h$|d$|w$")
Copy link
Owner

Choose a reason for hiding this comment

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

should be written as:

\d+[hdw]$

@bbgokarma-bot
Copy link

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

@MengShue MengShue closed this Oct 2, 2023
@MengShue MengShue reopened this Oct 2, 2023
@bbgokarma-bot
Copy link

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

@bailantaotao
Copy link
Collaborator

Long time no response, so closed this PR.

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.

4 participants