FIX: [exchange] fix bugs and add integration tests for Coinbase Exchange #1914
Conversation
dboyliao
commented
Feb 26, 2025
•
dboyliao
commented
| default: | ||
| return nil, fmt.Errorf("unsupported time in force: %v", order.TimeInForce) | ||
| // set time in force, using default if not set | ||
| if len(order.TimeInForce) > 0 { |
There was a problem hiding this comment.
Could you set a default timeInForce beforehand?
There was a problem hiding this comment.
I want to leave it unset if it's not given by to user.
So it will fallback to Coinbase API defaults.
I want it to be consistent with Coinbase API.
pkg/exchange/coinbase/exchage.go
Outdated
| @@ -308,25 +310,36 @@ func (e *Exchange) QueryKLines(ctx context.Context, symbol string, interval type | |||
| log.Warnf("limit %d is greater than the maximum limit 300, set to 300", options.Limit) | |||
| options.Limit = DefaultKLineLimit | |||
| } | |||
| granity := interval.String() | |||
| granity := fmt.Sprintf("%d", interval.Seconds()) | |||
There was a problem hiding this comment.
use string(interval) ?
There was a problem hiding this comment.
I believe it's not the right way doing it.
string(interval) will be "1m" but the Coinbase API is expecting "60".
There was a problem hiding this comment.
how about renaming the varialbe "granity" to "granularity"?
There was a problem hiding this comment.
Opps, my bad.
Misspelling.
Will fix it.
pkg/exchange/coinbase/exchage.go
Outdated
| candles := make([]api.Candle, 0, len(res)) | ||
| for _, c := range res { | ||
| candles = append(candles, *c.Candle()) | ||
| } |
There was a problem hiding this comment.
Most of us are accustomed to using pointers, for example make([]*api.Candle, 0, len(res)), so you don't have to dereference.
candles := make([]*api.Candle, 0, len(res))
for _, c := range res {
candles = append(candles, c.Candle())
}
There was a problem hiding this comment.
As your review for PR #1909, I will apply the error handling here for invalid raw candles.
In that case, I think we don't need to use pointers.
pkg/exchange/coinbase/exchage.go
Outdated
| if idx > 0 { | ||
| klines[idx-1].StartTime = kline.EndTime | ||
| } |
There was a problem hiding this comment.
如果你都帶入 granity，那這個 start time和 end time的計算應該可以維持原邏輯？如果coinbase回傳非granity的間隔，那應該很有問題...
There was a problem hiding this comment.
有道理
重看了一下 doc: https://docs.cdp.coinbase.com/exchange/reference/exchangerestapi_getproductcandles
API 回傳的應該是 start time
我應該用下一根的 start time 當 end time 才對~
當時寫的時候應該不小心搞反了
There was a problem hiding this comment.
喔 不對
我應該用 granity 換算才對
dboyliao
force-pushed
the
dboy/coinbase-test-fix
branch
from
29175d8 to
910e1c9
Compare
|
|
||
| klines := make([]types.KLine, 0, len(candles)) | ||
| for _, candle := range candles { | ||
| kline := toGlobalKline(symbol, interval, &candle) |
There was a problem hiding this comment.
Bbgo uses go 1.21.6. Before go 1.22, the variable between each for loop iteration is the same variable. That means the &candle always points to the same address unless you use go 1.22. Please ensure this behavior is expected.
There was a problem hiding this comment.
Since I do not keep the pointer (&candle) in the body of toGlobalKline(), I think it will do what is expected in go 1.21.6.
There was a problem hiding this comment.
I compile and run it with go1.21.6
As shown in the screenshot, there should be no issue where pointers outlive the loop.
