-
-
Notifications
You must be signed in to change notification settings - Fork 302
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 QueryClosedOrders() and QueryTrades() for okex #1312
Conversation
MengShue
commented
Sep 18, 2023
- Add QueryClosedOrders() and QueryTrades() and its unit test for OKEX
- Merge conflict is expected when FEATURE: add QueryOrderTrades() for okex #1307 merge into main, Will solve conflict when rebasing.
Welcome back! @MengShue, This pull request may get 795 BBG. |
) | ||
|
||
//go:generate GetRequest -url "/api/v5/trade/orders-history-archive" -type GetOrderHistoriesRequest -responseDataType .APIResponse | ||
type GetOrderHistoriesRequest 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.
just noticed that "history" is uncountable, so it should be "OrderHistory" not "OrderHistories"
e := New(key, secret, passphrase) | ||
|
||
queryOrder := types.OrderQuery{ | ||
Symbol: "BTC-USDT", |
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.
types.OrderQuery is a global type, so the symbol value will be "BTCUSDT" not "BTC-USDT"
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.
Yes, you're right, I use toLocalSymbol
to fix it, and I'm afraid my previous PR made the same error, Will fix it in coming PR, already fix QueryTrades(), QueryClosedOrders(), QueryOrder()
e := New(key, secret, passphrase) | ||
|
||
queryOrder := types.OrderQuery{ | ||
Symbol: "BTC-USDT", |
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
types.OrderQuery is a global type, so the symbol value will be "BTCUSDT" not "BTC-USDT"
Codecov Report
@@ Coverage Diff @@
## main #1312 +/- ##
==========================================
- Coverage 20.86% 20.80% -0.06%
==========================================
Files 556 558 +2
Lines 39621 39743 +122
==========================================
+ Hits 8265 8269 +4
- Misses 30757 30874 +117
- Partials 599 600 +1
... and 10 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Re-estimated karma: this pull request may get 840 BBG |
pkg/exchange/okex/exchange.go
Outdated
|
||
limit := uint64(options.Limit) | ||
if limit > defaultQueryLimit || limit <= 0 { | ||
log.Debugf("limtit is exceeded or zero, update to %d, got: %d", defaultQueryLimit, options.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.
"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.
when limit = 0, we should apply default limit
pkg/exchange/okex/exchange.go
Outdated
log.Debugf("limtit is exceeded or zero, update to %d, got: %d", defaultQueryLimit, options.Limit) | ||
limit = defaultQueryLimit | ||
} | ||
req.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.
if the limit is optional, you can just apply it only when it's given
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.
Updated
pkg/exchange/okex/exchange.go
Outdated
// query by time interval | ||
if options.StartTime != nil || options.EndTime != nil { | ||
if options.StartTime != nil { | ||
req.StartTime(options.StartTime.Local()) |
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.
.Local() means you are converting the time with the local timezone, if it's an unix timestamp field, .Local() does not change the value.
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.
Update, not use .Local()
Re-estimated karma: this pull request may get 905 BBG |
Re-estimated karma: this pull request may get 952 BBG |
aafd3da
to
9c08174
Compare
Re-estimated karma: this pull request may get 968 BBG |
|
pkg/exchange/okex/exchange.go
Outdated
@@ -32,6 +32,11 @@ const ID = "okex" | |||
// PlatformToken is the platform currency of OKEx, pre-allocate static string here | |||
const PlatformToken = "OKB" | |||
|
|||
// Constant For query 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.
Your comment should be above defaultQueryLimit
.
pkg/exchange/okex/exchange.go
Outdated
} | ||
|
||
/* | ||
Query Closed trades can query closed trades in last 3 months, there are no time interval limitations, as long as until >= since. |
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 Go, it is idiomatic to start a comment for a function with the function's name. This is especially relevant for exported functions, as the comment will be used to generate documentation. For example, if you have a function named QueryClosedOrders, the comment could start like this: QueryClosedOrders queries the xxx....
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.
query close trades
? or query close orders
?
pkg/exchange/okex/exchange.go
Outdated
|
||
/* | ||
Query Closed trades can query closed trades in last 3 months, there are no time interval limitations, as long as until >= since. | ||
Please Use LastTradeID as cursor, only return trades earlier than that trade, that trade is not included. |
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.
return trades? or orders?
pkg/exchange/okex/exchange.go
Outdated
req.EndTime(*options.EndTime) | ||
} | ||
|
||
res, err := req.Do(ctx) |
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 assign response here. like response, err = req.Do(ctx)
pkg/exchange/okex/exchange.go
Outdated
} | ||
response = res | ||
} else if options.StartTime == nil && options.EndTime == nil { | ||
res, err := req.Do(ctx) |
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.
ditto
pkg/exchange/okex/exchange.go
Outdated
} | ||
response = res | ||
} else { // query by trade id | ||
res, err := req.Do(ctx) |
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.
ditto
pkg/exchange/okex/exchange.go
Outdated
StartTime(since). | ||
EndTime(until). | ||
Limit(defaultQueryLimit). | ||
After(lastOrder). |
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 we should use Before
instead of After
. The order ID is generally an incrementing number. Am i right @c9s ?
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.
pkg/exchange/okex/exchange.go
Outdated
return nil, fmt.Errorf("failed to call get order histories error: %w", err) | ||
} | ||
for _, trade := range res { | ||
if trade.LastTradeID == lastTradeID { |
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.
The last trade id is a cursor, so you should use before
and pass lastTradeID. Am i right? @c9s
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.
e.q. req.Before(lastTradeId).Do(ctx)
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.
sounds like so
Re-estimated karma: this pull request may get 1158 BBG |
pkg/exchange/okex/exchange.go
Outdated
|
||
/* | ||
QueryClosedOrders can query closed orders in last 3 months, there are no time interval limitations, as long as until >= since. | ||
Please Use lastOrderID as cursor, only return orders earlier than that order, that order is not included. |
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.
earlier -> later?
pkg/exchange/okex/exchange.go
Outdated
If you want to query orders by time range, please just pass since and until. | ||
If you want to query by cursor, please pass lastOrderID. | ||
Because it get correct response even pass all parameters with right time interval and invalid lastOrderID, like 0. | ||
Time interval boundary unit is second, ex. order created in 1694155903, get response if query until 1694155903, get empty if query until 1694155904 |
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.
Are the since
values the same?
pkg/exchange/okex/exchange.go
Outdated
/* | ||
QueryTrades can query trades in last 3 months, there are no time interval limitations, as long as end_time >= start_time. | ||
OKEX do not provide api to query by tradeID, So use /api/v5/trade/orders-history-archive as its official site do. | ||
Please Use LastTradeID as cursor, only return trades earlier than that trade, that trade is not included. |
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.
earlier -> later?
pkg/exchange/okex/exchange.go
Outdated
} | ||
for _, trade := range res { | ||
if trade.LastTradeID == lastTradeID { | ||
response, err = req.Before(trade.OrderID).Do(ctx) |
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.
我的意思是你應該在 L521 就直接設定 before,而不是這裡,這樣撈回來的值就都會是該 order id之後的?
res, err := req.Do(ctx) -> response, err = req.Before(trade.OrderID).Do(ctx)
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.
不能,因為before() after() 裡要的是order id, 我們argument 是trade id, 所以我先撈一筆,找到trade id 是我們要的之後,再用 before( order_id )
|
||
func Test_QueryClosedOrders(t *testing.T) { | ||
|
||
key, secret, passphrase, ok := testutil.IntegrationTestWithPassphraseConfigured(t, "OKEX") |
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.
OKEX -> types.ExchangeOKEx ?
Re-estimated karma: this pull request may get 1248 BBG |
pkg/exchange/okex/exchange.go
Outdated
return nil, ErrSymbolRequired | ||
} | ||
|
||
if err := tradeRateLimiter.Wait(ctx); 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.
Perhaps we need to adjust token limit to avoid rate limiter error
https://www.okx.com/docs-v5/en/#order-book-trading-trade-get-order-history-last-3-months
Rate Limit: 20 requests per 2 seconds
you declare tradeRateLimiter = rate.NewLimiter(rate.Every(100*time.Millisecond), 5) and used in QueryClosedOrders and QueryTrades
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.
Update, 相當於打了三次,我改成 300 milli seconds, 應該可以
Re-estimated karma: this pull request may get 1263 BBG |
pkg/exchange/okex/exchange.go
Outdated
Please Use lastOrderID as cursor, only return orders later than that order, that order is not included. | ||
If you want to query orders by time range, please just pass since and until. | ||
If you want to query by cursor, please pass lastOrderID. | ||
Because it get correct response even pass all parameters with right time interval and invalid lastOrderID, like 0. |
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.
"it gets the correct response"
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.
Update
pkg/exchange/okex/exchange.go
Outdated
Please Use lastOrderID as cursor, only return orders later than that order, that order is not included. | ||
If you want to query orders by time range, please just pass since and until. | ||
If you want to query by cursor, please pass lastOrderID. | ||
Because it get correct response even pass all parameters with right time interval and invalid lastOrderID, like 0. |
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.
"even when you pass"
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.
Update
pkg/exchange/okex/exchange.go
Outdated
Please Use lastOrderID as cursor, only return orders later than that order, that order is not included. | ||
If you want to query orders by time range, please just pass since and until. | ||
If you want to query by cursor, please pass lastOrderID. | ||
Because it get correct response even pass all parameters with right time interval and invalid lastOrderID, like 0. |
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.
"with the right 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.
Update
pkg/exchange/okex/exchange.go
Outdated
return nil, fmt.Errorf("query closed order rate limiter wait error: %w", err) | ||
} | ||
|
||
var lastOrder string = strconv.FormatUint(lastOrderID, 10) |
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.
lastOrderID could be zero, when it's zero, do you need to pass to the request?
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.
Update
pkg/exchange/okex/exchange.go
Outdated
continue | ||
} | ||
|
||
if o.Status.Closed() { |
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.
Doesn't the order history request return only closed orders?
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.
Update
Re-estimated karma: this pull request may get 1357 BBG |
pkg/exchange/okex/exchange.go
Outdated
var err error | ||
var response []okexapi.OrderDetails | ||
// query by time interval | ||
if options.StartTime != nil || options.EndTime != 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.
這邊其實應該可以 startTime == nil && endTime == nil 就直接回傳空 slice?
nest 越少層越好
pkg/exchange/okex/exchange.go
Outdated
return nil, fmt.Errorf("failed to call get order histories error: %w", err) | ||
} | ||
} else { // query by trade id | ||
lastTradeID := strconv.FormatUint(options.LastTradeID, 10) |
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.
lastTradeID is not used before you send the request, why?
pkg/exchange/okex/exchange.go
Outdated
return nil, ErrSymbolRequired | ||
} | ||
|
||
req := e.client.NewGetOrderHistoryRequest().InstrumentID(toLocalSymbol(symbol)) |
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.
為什麼 QueryTrades 需要先 QueryOrderHistory 呀?
Re-estimated karma: this pull request may get 1436 BBG |
pkg/exchange/okex/exchange.go
Outdated
return nil, ErrSymbolRequired | ||
} | ||
|
||
if err := tradeRateLimiter.Wait(ctx); 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.
it's an order query, but you're using trade limiter?
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.
delete trade limit, no need to use it.
… for QueryOrderTrades() and update typo error in QueryOrderTrades()
5a1b260
to
ad72062
Compare
Re-estimated karma: this pull request may get 1450 BBG |
If you want to query trades by time range, please just pass start_time and end_time. | ||
Because it gets the correct response even when you pass all parameters with the right time interval and invalid LastTradeID, like 0. | ||
*/ | ||
func (e *Exchange) QueryTrades(ctx context.Context, symbol string, options *types.TradeQueryOptions) ([]types.Trade, 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.
Perhaps we need to handle query trade with pagination, if you query result greater that 100, the other result will missing?
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.
Already handle, like below
var billID = "" // billId should be emtpy, can't be 0
for { // pagenation should use "after" (earlier than)
res, err := req.
After(billID).
Do(ctx)
if err != nil {
return nil, fmt.Errorf("failed to call get order histories error: %w", err)
}
response = append(response, res...)
if len(res) != int(limit) {
break
}
billID = res[limit-1].BillID
}
Re-estimated karma: this pull request may get 1486 BBG |
|
||
var err error | ||
var response []okexapi.OrderDetails |
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, you're calling QueryOrderHistory in the QueryTrades api? is this expected?
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.
Yes ! because OKX didn't provide any api query by trade_id, but QueryOrderHistory has trade_id in its response. So I call QueryOrderHistory to get the same effect.
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.
OKX didn't provide any api query like /my-trade
either
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.
你可以測試pagination是否可以動嗎?
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 right, 加了unit test 後發現應該要用 limit
而不是 defaultQueryLimit
pkg/exchange/okex/exchange.go
Outdated
req.EndTime(*options.EndTime) | ||
} | ||
|
||
var res []okexapi.OrderDetails |
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.
rm
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.
updated
pkg/exchange/okex/exchange.go
Outdated
res, err = req. | ||
After(lastOrderID). | ||
Do(ctx) |
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.
res, err := 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.
updated
|
||
var err error | ||
var response []okexapi.OrderDetails |
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.
你可以測試pagination是否可以動嗎?
Re-estimated karma: this pull request may get 1558 BBG |
Re-estimated karma: this pull request may get 1608 BBG |
pkg/exchange/okex/exchange.go
Outdated
var err error | ||
var response []okexapi.OrderDetails | ||
if options.StartTime == nil && options.EndTime == nil { | ||
return nil, fmt.Errorf("StartTime and EndTime are require parameter!") |
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.
required
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.
Updated
req.EndTime(*options.EndTime) | ||
} | ||
|
||
var billID = "" // billId should be emtpy, can't be 0 |
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.
A few questions:
- What's the sorting of query result if i take start, end time only?
- Wha't the sorting of query result if i take start, end time, after?
- What happens when I query for data from three months earlier?
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.
All is descending order.
If you query time period 3 months earlier with start time and end time, will return []
empty slice
But If you query time period 3 months earlier JUST with start time, will return like start with 3 months ago.
pkg/exchange/okex/exchange.go
Outdated
if len(res) == int(limit) { | ||
billID = res[limit-1].BillID | ||
} else { | ||
break | ||
} |
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.
It can be simpler:
if len(res) != int(limit) {
break
}
billID = res[limit-1].BillID
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.
Updated
pkg/exchange/okex/exchange.go
Outdated
limit := uint64(options.Limit) | ||
if limit > defaultQueryLimit || limit <= 0 { | ||
limit = defaultQueryLimit | ||
log.Debugf("limit is exceeded default limit %d or zero, got: %d, Do not pass limit", defaultQueryLimit, options.Limit) | ||
} else { | ||
req.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.
Always pass the limit when making a request. If the server updates the default query limit, your pagination may not work.
How about:
req.Limit(uint64(options.Limit))
if limit > defaultQueryLimit || limit <= 0 {
req.Limit(defaultQueryLimit)
}
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.
Ok. That make sense !
|
||
var billID = "" // billId should be emtpy, can't be 0 | ||
for { // pagenation should use "after" (earlier than) | ||
res, err := 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.
Handle rate 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.
I handle it in line 503 - 505
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.
Rate limit的 token應該是每執行1次request就要消耗1個,但你的做法會變成他消耗1個後,有可能發出大於1的requests
這可以下個PR再改
pkg/exchange/okex/convert.go
Outdated
@@ -286,9 +286,10 @@ func toGlobalOrder(okexOrder *okexapi.OrderDetails) (*types.Order, error) { | |||
} | |||
|
|||
func toGlobalTrade(orderDetail *okexapi.OrderDetails) (*types.Trade, error) { | |||
tradeID, err := strconv.ParseInt(orderDetail.LastTradeID, 10, 64) | |||
// Should use tradeId, but okex use billId to perform pagination, so use billID as tradeID instead. | |||
billID, err := strconv.ParseInt(orderDetail.BillID, 10, 64) |
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 the bill id be found at okx console?
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.
console? you mean web page? No, You can't find billId in official site, actually you can't find orderId and tradeId either.
Re-estimated karma: this pull request may get 1756 BBG |
pkg/exchange/okex/okexapi/trade.go
Outdated
@@ -276,6 +276,7 @@ type OrderDetails struct { | |||
LastFilledFee fixedpoint.Value `json:"fillFee"` | |||
LastFilledFeeCurrency string `json:"fillFeeCcy"` | |||
LastFilledPnl fixedpoint.Value `json:"fillPnl"` | |||
BillID string `json:"billId"` |
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 use types.StrInt64 here
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.
ok, will update
} | ||
|
||
// test by order id as a cursor | ||
closedOrder, err := e.QueryClosedOrders(context.Background(), string(queryOrder.Symbol), time.Time{}, time.Time{}, 609869603774656544) |
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 guess this order ID is your own order and which can work for others
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.
Yes, I will comment out the test
…and comment out personal unit test
Re-estimated karma: this pull request may get 1799 BBG |
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.
Others lgtm
|
||
var billID = "" // billId should be emtpy, can't be 0 | ||
for { // pagenation should use "after" (earlier than) | ||
res, err := 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.
Rate limit的 token應該是每執行1次request就要消耗1個,但你的做法會變成他消耗1個後,有可能發出大於1的requests
這可以下個PR再改
Hi @MengShue, Well done! 1834 BBG has been sent to your polygon wallet. Please check the following tx: https://polygonscan.com/tx/0x21ba8c81a8ac1a1b5559248b79b0e0d150727539177c9605c39e9eb44bc633f6 Thank you for your contribution! |