-
Notifications
You must be signed in to change notification settings - Fork 903
Bitget: Add exchange support #1844
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
base: master
Are you sure you want to change the base?
Bitget: Add exchange support #1844
Conversation
…s discovered exchange-side errors
shazbert
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very fine work. Some quick nits to get started.
| AIDOGE = NewCode("AIDOGE") | ||
| PEPE = NewCode("PEPE") | ||
| USDCM = NewCode("USDCM") | ||
| SUSDT = NewCode("SUSDT") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is sus
| // Public endpoints | ||
| bitgetPublic = "public/" | ||
| bitgetAnnouncements = "annoucements" // Misspelling of announcements | ||
| bitgetTime = "time" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See if you can consolidate this list to the function calls and remove the bitget prefix on the left over consts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by consolidating to function calls?
Removing the prefix will cause some of them to run into conflicts (i.e. time and ticker would conflict with the packages). I could keep prefixes there or add different suffixes, but this would total to a lot of work for a purely cosmetic change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By consolidation I mean you add the string "/somepath" directly to the path if its not shared in different functions and which might reduce this list. Leave that up to you if you want to change the bitget prefix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could definitely consolidate part of these, and the reason to split them out isn't too significant now (it was done so that I could identify new endpoints which share those). But it does seem like a fair bit of work (searching 180 different consts to see how many of them appear multiple times, and appropriately replacing those which only appear once) for minimal reward (editors of this file need to scroll through fewer lines).
I'd rather not change these, but if others disagree, I can.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's fine keep as is. 👍
exchanges/bitget/bitget.go
Outdated
| ) | ||
|
|
||
| var ( | ||
| errBusinessTypeEmpty = errors.New("businessType cannot be empty") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: seems like we might be able to have a common.ErrEmptyValue across this list and then wrap field to it but up to you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you just mean, for most of these values, replacing them with something like = fmt.Errorf("%w: businessType", common.ErrEmptyValue") ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah something like that but up to you if you want to do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather not unless others request it.
samuael
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a cool PR. I have made a quick review. Will get back to it another round
| } | ||
|
|
||
| if b.Asset.String() == "" { | ||
| return errAssetTypeNotSet |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you move this function to the assets folder?
| // Public errors | ||
| var ( | ||
| ErrOrderbookNotFound = errors.New("cannot find orderbook(s)") | ||
| ErrAssetTypeNotSet = errors.New("orderbook asset type not set") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this should be in this package
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes some sense; the orderbook's asset type isn't set, seems fine for the orderbook package.
But I wouldn't be too opposed to moving it.
| case 'n', 't', 'f': // null, true, false | ||
| case 't', 'f': // true, false | ||
| return fmt.Errorf("%w: %s", errInvalidNumberValue, data) | ||
| case 'n': // null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why this is an exception since we only checked the first entry and could not be sure to be sure to see whether it is 'n' for 'null' or for 'north'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it's JSON, only certain values are allowed outside of quote marks. If a different JSON implementation is sending north, not included inside quote marks, it would not be conforming to standard (RFC8259, section 3).
samuael
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just added few comments on top of what I did before.
| params.Values.Set("status", status) | ||
| params.Values.Set("side", side) | ||
| params.Values.Set("language", "en-US") | ||
| if !cryptoCurrency.IsEmpty() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could group these optional variables check together. Just for consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could, but it's aesthetic and can't easily be automatically detected, so I won't bother; I'll continue leaving them in the order the exchange provided them.
exchanges/bitget/bitget_websocket.go
Outdated
| } | ||
|
|
||
| // GenerateDefaultSubscriptions generates default subscriptions | ||
| func (e *Exchange) generateDefaultSubscriptions() (subscription.List, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can modify this implementation by using the new exchange template implementation.
exchanges/bitget/bitget_websocket.go
Outdated
| wg.Add(1) | ||
| go func(req WsRequest) { | ||
| defer wg.Done() | ||
| err := e.Websocket.Conn.SendJSONMessage(context.TODO(), rateSubscription, req) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if err := ...; err != nil {}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ran out of time to completely do this, but I've caught a lot of these. Will catch more next week.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got the rest of these.
exchanges/bitget/bitget_websocket.go
Outdated
| wg.Add(1) | ||
| go func(req WsRequest) { | ||
| defer wg.Done() | ||
| err := e.Websocket.AuthConn.SendJSONMessage(context.TODO(), rateSubscription, req) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shorten here too
exchanges/bitget/bitget_websocket.go
Outdated
|
|
||
| // AccountUpdateDataHandler | ||
| func (e *Exchange) accountUpdateDataHandler(wsResponse *WsResponse, respRaw []byte) error { | ||
| creds, err := e.GetCredentials(context.TODO()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pass ctx from the calling instance or use context.Background()
samuael
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not all of my last review are applied but I have added more
| vals.Set("period", period) | ||
| } | ||
| path := bitgetMargin + bitgetMarket + bitgetIsolatedBorrowRate | ||
| var resp []BorrowRatioResp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you change such occurrences into using a slice of pointers? that way it will become easier to loop over each elements without using indexing
| var resp []BorrowRatioResp | |
| var resp []*BorrowRatioResp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean? It's slightly easier to loop over slices when they aren't of pointers since then you don't need to dereference it afterwards.
sli := []int64{1, 2, 3, 4}
for _, b := range sli {
fmt.Printf("%v\n", b)
}
Compared to
i1, i2, i3, i4 := int64(1), int64(2), int64(3), int64(4)
sli := []*int64{&i1, &i2, &i3, &i4}
for _, b := range sli {
fmt.Printf("%v\n", *b)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are not supposed to de-reference this since you only use it to access the data inside it. When you use list of pointers, you can directly use the value from the loop to access it instead of using a slice indexing(result[i]) to load the value at that index.
| vals.Set("symbol", pair.String()) | ||
| vals.Set("period", period) | ||
| path := bitgetMix + bitgetMarket + bitgetLongShort | ||
| var resp []RatioResp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
| vals := url.Values{} | ||
| vals.Set("symbol", pair.String()) | ||
| path := bitgetSpot + bitgetMarket + bitgetFundNetFlow | ||
| var resp []WhaleFundFlowResp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
| return nil, errSubaccountEmpty | ||
| } | ||
| path := bitgetUser + bitgetCreate + bitgetVirtualSubaccount | ||
| req := struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can replace this with maps and put it directly into the SendAuthenticatedHTTPRequest method call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They were maps; you told me to replace them with structs back in May!
exchanges/bitget/bitget.go
Outdated
| func (e *Exchange) GetConvertHistory(ctx context.Context, startTime, endTime time.Time, limit, pagination int64) (*ConvHistResp, error) { | ||
| var params Params | ||
| params.Values = make(url.Values) | ||
| err := params.prepareDateString(startTime, endTime, false, false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Throughout this code base you can replace such occurances as follow
| err := params.prepareDateString(startTime, endTime, false, false) | |
| if err := params.prepareDateString(startTime, endTime, false, false); err != nil { |
| return nil, currency.ErrCurrencyPairEmpty | ||
| } | ||
| if p.Side == "" { | ||
| return nil, errSideEmpty |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace this with order.ErrSideIsInvalid
| return nil, errSideEmpty | ||
| } | ||
| if p.OrderType == "" { | ||
| return nil, errOrderTypeEmpty |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can be order.ErrUnsupportedOrderType or order.ErrTypeIsInvalid
exchanges/bitget/bitget.go
Outdated
| return nil, errStrategyEmpty | ||
| } | ||
| if p.OrderType == "limit" && p.Price <= 0 { | ||
| return nil, errLimitPriceEmpty |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
replace with order.ErrPriceMustBeSetIfLimitOrder
exchanges/bitget/bitget.go
Outdated
| func (e *Exchange) GetUnfilledOrders(ctx context.Context, pair currency.Pair, tpslType string, startTime, endTime time.Time, limit, pagination, orderID int64, acceptableDelay time.Duration) ([]UnfilledOrdersResp, error) { | ||
| var params Params | ||
| params.Values = make(url.Values) | ||
| err := params.prepareDateString(startTime, endTime, true, true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shorten this too if err := ....
exchanges/bitget/bitget.go
Outdated
| errSideEmpty = fmt.Errorf("%w: empty order side", order.ErrSideIsInvalid) | ||
| errOrderTypeEmpty = fmt.Errorf("%w: empty order type", order.ErrTypeIsInvalid) | ||
| errStrategyEmpty = errors.New("strategy cannot be empty") | ||
| errLimitPriceEmpty = fmt.Errorf("%w: Price below minimum for limit orders", limits.ErrPriceBelowMin) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
replace with order.ErrPriceMustBeSetIfLimitOrder
samuael
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shalooom 🕊️ brother.
Just added another round of review on endpoint functions.
exchanges/bitget/bitget.go
Outdated
| // QueryAnnouncements returns announcements from the exchange, filtered by type and time | ||
| func (e *Exchange) QueryAnnouncements(ctx context.Context, annType string, startTime, endTime time.Time, pagination uint64, limit uint8) ([]AnnouncementResp, error) { | ||
| var params Params | ||
| params.Values = make(url.Values) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move this to below the error check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That can't be done; the error check calls a function on params which operates on params.Values
| } | ||
| vals := url.Values{} | ||
| vals.Set("businessType", businessType) | ||
| var resp []AllTradeRatesResp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can use []* for all response type declarations from now on; for consistency.
Instead of iterating over a slice and use the index to access elements, we can iterate over the slice but use the pointer and reduce the memory overhead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In both cases you'd be putting the underlying data into memory at one point, being a slice of pointers just requires you to keep track of the pointer as well, which is more memory and more work for the GC!
exchanges/bitget/bitget.go
Outdated
| // GetSpotTransactionRecords returns the user's spot transaction records | ||
| func (e *Exchange) GetSpotTransactionRecords(ctx context.Context, cur currency.Code, startTime, endTime time.Time, limit, pagination uint64) ([]SpotTrResp, error) { | ||
| var params Params | ||
| params.Values = make(url.Values) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you move this declaration below the error checks?
Apply to all occurences in this PR. always, params declaration after all the error checks pass.
exchanges/bitget/bitget.go
Outdated
| // GetFuturesTransactionRecords returns the user's futures transaction records | ||
| func (e *Exchange) GetFuturesTransactionRecords(ctx context.Context, productType string, cur currency.Code, startTime, endTime time.Time, limit, pagination uint64) ([]FutureTrResp, error) { | ||
| var params Params | ||
| params.Values = make(url.Values) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
| if pagination > 0 { | ||
| params.Values.Set("idLessThan", strconv.FormatUint(pagination, 10)) | ||
| } | ||
| var resp []P2PTrResp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| var resp []P2PTrResp | |
| var resp []*P2PTrResp |
| } | ||
| vals := url.Values{} | ||
| vals.Set("symbol", pair.String()) | ||
| vals.Set("period", period) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
period is not checked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only check for zero values in cases where zero values cause issues with the way the exchange responds.
| } | ||
| vals := url.Values{} | ||
| vals.Set("symbol", pair.String()) | ||
| vals.Set("period", period) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
period is not checked for zero value
| if period != "" { | ||
| vals.Set("period", period) | ||
| } | ||
| vals.Set("coin", cur.String()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cur is not checked if it is zero value.
| return nil, errSubaccountEmpty | ||
| } | ||
| path := bitgetUser + bitgetCreate + bitgetVirtualSubaccount | ||
| req := struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| req := struct { | |
| req := map[string][]string{"subAccountList": subaccounts} |
| vals := url.Values{} | ||
| vals.Set("symbol", pair.String()) | ||
| vals.Set("precision", precision) | ||
| vals.Set("limit", limit) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these variables not being checked for zero value?
shazbert
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will start to integrate this as priority and will help out with any changes
exchanges/bitget/bitget_wrapper.go
Outdated
| Unsubscriber: e.Unsubscribe, | ||
| GenerateSubscriptions: e.generateDefaultSubscriptions, | ||
| Features: &e.Features.Supports.WebsocketCapabilities, | ||
| MaxWebsocketSubscriptionsPerConnection: 240, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| MaxWebsocketSubscriptionsPerConnection: 240, | |
| MaxWebsocketSubscriptionsPerConnection: 1000, |
A single connection can subscribe up to 1000 Streams;
See: https://bitgetlimited.github.io/apidoc/en/spot/#overview
Also please use multi-connection management for future scaling options see: #2109
Will add a diff or push to branch if I can get around to it before you come back.
shazbert
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have updated accounts which needs more testing, migrated to multi-connection and added some fixes to subs. On login for the private conn it waits until it has a confirmation message.
Some things that need to be addressed:
- futures asset needs to be broken up to USDT margined, USDC margined and Coin Margined futures.
- margin and cross margin subs are semi broken
- linter/misc/spell needs fixing
- bitget config needs to be added to config_example.json
Will keep pushing things as I came across them. 🚀
exchanges/bitget/bitget.go
Outdated
|
|
||
| // QueryAnnouncements returns announcements from the exchange, filtered by type and time | ||
| func (e *Exchange) QueryAnnouncements(ctx context.Context, annType string, startTime, endTime time.Time, pagination uint64, limit uint8) ([]AnnouncementResp, error) { | ||
| var params Params |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Params type can have url.Values unexported and then just have methods to the underlying which calls make when there is a nil reference, stops you having to do params.Values = make(url.Values)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I found a better way to do this.
…dling, plus todos
- Added websocket support for placing spot and futures orders with validation checks. - Implemented cancellation of orders via websocket for both spot and futures. - Created request and response types for websocket interactions. - Added tests for websocket order submission and cancellation to ensure proper functionality. - Updated order types to include settlement currency for derivatives.
|
This PR is stale because it has been open 21 days with no activity. Please provide an update on the progress of this PR. |
PR Description
Implementing Bitget's V2 API, specifically the Common, Spot, Future, Margin, Earn, and Inst Loan sections.
In addition, does some smaller fixes across the codebase:
Type of change
Please delete options that are not relevant and add an
xin[]as item is complete.How has this been tested
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration and also consider improving test coverage whilst working on a certain feature or package.
Checklist