FEATURE: [core] Implement KLine builder, emitting kline events by market trades #2002

dboyliao · 2025-04-24T05:22:52Z

No description provided.

dboyliao · 2025-04-24T06:03:18Z

I just be told we have SerialMarketDataStore in bbgo.
I will update this PR with it.
Don't review it now.

dboyliao · 2025-04-24T13:51:22Z

It's ready for review.

pkg/core/klinebuilder.go

c9s · 2025-04-25T00:14:50Z

pkg/core/klinebuilder.go

+	for interval := range c.intervals {
+		if interval.Duration()%minDuration != 0 {
+			c.logger.Errorf("interval %s is not a multiple of minInterval %s", interval, c.minInterval)
+			panic(interval)


use fmt.Errorf and pass the error to panic instead of Errorf logging here

it's a logical exception (user configuration faults)

c9s · 2025-04-25T00:16:21Z

pkg/core/klinebuilder.go

+		}
+	}
+	c.logger.Infof("kline updater started for %s (%+v)", c.symbol, c.intervals)
+	c.updateTime = types.Time(time.Now())


we can't use time.Now when in replay mode (back testing), it could mess up the "real" data time

This line is just a kick-start value of the update time.
I'll move this line to update.
And in backtest mode, I will not start the update goroutine so it won't mess up the real data then.

pkg/core/klinebuilder.go

bailantaotao · 2025-04-25T02:50:46Z

pkg/core/klinebuilder.go

+	if !c.high.IsZero() {
+		if c.price.Compare(c.high) > 0 {
+			c.high = c.price
+		}
+	} else {
+		c.high = c.price
+	}


Can be simplified

c.high = math.Max(c.high, c.price) ?

Oh, yeah.
Just like my comment below, the high will be zero when the builder is reset.
I was thinking it's more clear to do the update with a if here.
But you are right, it can be simplified.

bailantaotao · 2025-04-25T02:53:07Z

pkg/core/klinebuilder.go

+	if !c.low.IsZero() {
+		if c.price.Compare(c.low) < 0 {
+			c.low = c.price
+		}
+	} else {
+		c.low = c.price
+	}


c.low = math.Min(c.low, c.price)

Nice.
Fixing it.

bailantaotao · 2025-04-25T02:53:46Z

pkg/core/klinebuilder.go

+			c.low = c.price
+		}
+	} else {
+		c.low = c.price


When is "low" equal to 0??

low, high, ...etc will be zero when the builder is being "reset".

When the builder is reset, we should use whatever the price is as its low.

I add a comment on it for clarity.

pkg/exchange/coinbase/stream_handlers.go

bailantaotao · 2025-04-25T03:00:28Z

pkg/exchange/coinbase/stream_handlers.go

@@ -25,6 +26,20 @@ const (
 	balanceChannel     types.Channel = "balance"
 )

+// minInterval: for the kline updater
+var minInterval = types.Interval("1s")


In the init(), it will be updated according to supported intervals on Coinbase.
In this way, if there is any updates to the supported intervals on Coinbase, we won't need to change the min interval here.
It will be updated accordingly.

bailantaotao · 2025-04-25T03:03:10Z

pkg/core/klinebuilder.go

+			klineMinInterval := types.KLine{
+				Symbol:    c.symbol,
+				StartTime: types.Time(startTime),
+				EndTime:   types.Time(tickTime),


tickTime.Add(-1 millisecond)?

The start time and end time cannot overlap

No need to do that.
startTime := tickTime.Add(-1 * intervalDuration).Round(intervalDuration)
Since .Round(...) will round the time to it's neareast multiple of the given duration, startTime will not overlap with tickTime.

Oh, I don't realize what you mean until now.
You mean the next kline start time should not overlap with the end time of the previous one right?
I will update the end time as expected end time minus one millisecond.

bailantaotao · 2025-04-25T03:12:05Z

pkg/core/klinebuilder.go

+			kline.Merge(klineMinInterval)
+		}
+		kline.Interval = interval
+		expectEndTime := kline.StartTime.Time().Add(interval.Duration())


-1 time.millisecond?

I believe we don't need to do that here.

Ideally, kline should end at a timestamp which is a multiple of the interval in the loop.
expectEndTime here is the ideal end time of the kline.

So basically, kline.Closed = ... is checking:

if the kline.EndTime, which in fact is the ticker time, is after expectEndTime.

or the lastUpdateTime, which is the time of the last market trade, is after expectEndTime.

Either case, the kline should be closed for that interval.

pkg/core/klinebuilder.go

c9s · 2025-04-30T02:52:49Z

pkg/bbgo/environment.go

@@ -49,6 +50,7 @@ var BackTestService *service.BacktestService
 func SetBackTesting(s *service.BacktestService) {
 	BackTestService = s
 	IsBackTesting = s != nil
+	coinbase.IsBackTesting = IsBackTesting


why adding a IsBackTestung flag in coinbase?

c9s · 2025-05-02T05:04:22Z

pkg/core/klinebuilder.go

+//   - symbol: symbol to trace on
+//   - minInterval: unit interval, related to your signal timeframe.
+//     All the supported intervals of the binding stream should be multiple of this interval.
+func NewKLineBuilder(symbol string, minInterval types.Interval, isBackTesting bool) *kLineBuilder {


you don't need to pass isBackTesting to this builder, we just need to make this component backtest-able by ensuring not using async in the process

c9s · 2025-05-02T05:05:11Z

pkg/core/klinebuilder.go

+	}
+}
+
+func (kb *kLineBuilder) Subscribe(interval types.Interval) {


call this Subscribe as "AddInterval" and let it return error

c9s · 2025-05-02T05:12:55Z

pkg/core/klinebuilder.go

+}
+
+func (kb *kLineBuilder) BindStream(stream types.Stream) {
+	if kb.isBackTesting {


can't handle backtest like this, would prefer pull out this logic to another layer

c9s · 2025-05-02T05:20:32Z

pkg/core/klinebuilder_state.go

+		Close:       trade.Price,
+		High:        trade.Price,
+		Low:         trade.Price,
+		EndTime:     types.Time(time.Now()), // use current time to fix the timezone issue (temporary)


in the back test mode, we don't have use "time.Now()" because it's a simulation

c9s · 2025-05-02T05:23:12Z

pkg/core/klinebuilder.go

+				queryEndTime := queryStartTime.Add(2 * kline.Interval.Duration())
+				ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+				defer cancel()
+				klines, err := kb.exchange.QueryKLines(


we can't call QueryKlines in the back-test mode, and it seems like you're mixing 2 difference scenario in the same component?

c9s · 2025-05-02T05:25:32Z

pkg/core/klinebuilder.go

+				queryStartTime := kline.StartTime.Time().Add(-kline.Interval.Duration())
+				queryEndTime := queryStartTime.Add(2 * kline.Interval.Duration())
+				ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+				defer cancel()


be careful of the defer, it will not be called at the end of loop, it's executed at the end of function call.

c9s · 2025-05-02T06:48:50Z

pkg/core/klinebuilder.go

+func (kb *kLineBuilder) run(ctx context.Context) {
+	startTime := time.Now()
+	startTimeTruncated := startTime.Truncate(kb.minInterval.Duration())
+	intervalDuration := kb.minInterval.Duration()


since minInterval's purpose is different (for the ticker), we can rename it to tickerInterval, and ensures all the intervals we used are greater than the ticker interval.

c9s · 2025-05-02T06:50:16Z

pkg/core/klinebuilder_state.go

+	}
+}
+
+// Reset will reset the kline of the given interval to a new open kline with the given start time


doc comment uses simple form, "Reset resets"

c9s · 2025-05-02T06:52:14Z

pkg/core/klinebuilder_state.go

+
+}
+
+func updateKlineInPlace(existing, delta *types.KLine) {


why not just pass types.Trade ? because you only need the price and the volume

c9s · 2025-05-02T06:53:42Z

pkg/core/klinebuilder_state.go

+	return lastKLine, ok
+}
+
+func (kb *kLineBuilderState) AddTrade(trade types.Trade) {


when AddTrade is called, you can return the updated klines to the caller, and these updated klines can be pushed to the stream

e.g.

itv1 --------|---itv2-------|---itv3 td1-td2-----|---td3-------|---td4

itv* is the ticker interval, not the kline interval, the ticker interval should be smaller than all the kline intervals, which is for pushing the updated kline periodically.

when td3 is received, we need to return "closed k1" and "new k2"

dboyliao force-pushed the dboy/coinbase-kline branch 2 times, most recently from 63c04cf to a1c3e7f Compare April 24, 2025 05:27

dboyliao changed the title ~~FEATURE: [coinbase] Implement K-Line subscription handling via RESTful API~~ (WIP) FEATURE: [coinbase] Implement K-Line subscription handling via RESTful API Apr 24, 2025

dboyliao changed the title ~~(WIP) FEATURE: [coinbase] Implement K-Line subscription handling via RESTful API~~ WIP: [coinbase] Implement K-Line subscription handling via RESTful API Apr 24, 2025

dboyliao force-pushed the dboy/coinbase-kline branch 3 times, most recently from ccab19d to 9572241 Compare April 24, 2025 13:48

dboyliao requested a review from c9s as a code owner April 24, 2025 13:48

dboyliao changed the title ~~WIP: [coinbase] Implement K-Line subscription handling via RESTful API~~ FEATURE: [coinbase] Implement K-Line subscription handling via RESTful API Apr 24, 2025

dboyliao force-pushed the dboy/coinbase-kline branch 11 times, most recently from 6125959 to bef40b1 Compare April 25, 2025 00:02

dboyliao changed the title ~~FEATURE: [coinbase] Implement K-Line subscription handling via RESTful API~~ FEATURE: [coinbase] Implement K-Line subscription handling via Market Trade Feeds Apr 25, 2025

dboyliao force-pushed the dboy/coinbase-kline branch 2 times, most recently from 1d6371a to 2df742e Compare April 25, 2025 00:12

c9s reviewed Apr 25, 2025

View reviewed changes

dboyliao force-pushed the dboy/coinbase-kline branch 2 times, most recently from 9c5b6cf to f5786c6 Compare April 25, 2025 00:43

bailantaotao reviewed Apr 25, 2025

View reviewed changes

dboyliao requested a review from c9s April 25, 2025 04:04

dboyliao requested a review from bailantaotao April 25, 2025 04:04

dboyliao force-pushed the dboy/coinbase-kline branch 12 times, most recently from 93c7052 to 86695dd Compare May 1, 2025 14:32

dboyliao changed the title ~~FEATURE: [coinbase] Implement K-Line subscription handling via Market Trade Feeds~~ FEATURE: [core] Implement KLine builder, emitting kline events by market trades May 1, 2025

dboyliao force-pushed the dboy/coinbase-kline branch 4 times, most recently from 1076b32 to e870a77 Compare May 1, 2025 15:29

✨ feat(core): kLineBuilder, emit kline events via market trades

b839fd0

dboyliao force-pushed the dboy/coinbase-kline branch from e870a77 to b839fd0 Compare May 2, 2025 03:32

c9s reviewed May 2, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FEATURE: [core] Implement KLine builder, emitting kline events by market trades #2002

FEATURE: [core] Implement KLine builder, emitting kline events by market trades #2002

dboyliao commented Apr 24, 2025

dboyliao commented Apr 24, 2025

dboyliao commented Apr 24, 2025

c9s Apr 25, 2025

dboyliao Apr 25, 2025

c9s Apr 25, 2025

dboyliao Apr 25, 2025

bailantaotao Apr 25, 2025

dboyliao Apr 25, 2025

bailantaotao Apr 25, 2025

dboyliao Apr 25, 2025

bailantaotao Apr 25, 2025

dboyliao Apr 25, 2025 •

edited

Loading

dboyliao Apr 25, 2025 •

edited

Loading

dboyliao Apr 25, 2025 •

edited

Loading

bailantaotao Apr 25, 2025

dboyliao Apr 25, 2025

bailantaotao Apr 25, 2025

dboyliao Apr 25, 2025

dboyliao Apr 29, 2025

bailantaotao Apr 25, 2025

dboyliao Apr 25, 2025 •

edited

Loading

c9s Apr 30, 2025

c9s May 2, 2025

c9s May 2, 2025

c9s May 2, 2025

c9s May 2, 2025

c9s May 2, 2025

c9s May 2, 2025

c9s May 2, 2025

c9s May 2, 2025

c9s May 2, 2025

c9s May 2, 2025

c9s May 2, 2025

FEATURE: [core] Implement KLine builder, emitting kline events by market trades #2002

Are you sure you want to change the base?

FEATURE: [core] Implement KLine builder, emitting kline events by market trades #2002

Conversation

dboyliao commented Apr 24, 2025

dboyliao commented Apr 24, 2025

dboyliao commented Apr 24, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dboyliao Apr 25, 2025 • edited Loading

Choose a reason for hiding this comment

dboyliao Apr 25, 2025 • edited Loading

Choose a reason for hiding this comment

dboyliao Apr 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dboyliao Apr 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dboyliao Apr 25, 2025 •

edited

Loading

dboyliao Apr 25, 2025 •

edited

Loading

dboyliao Apr 25, 2025 •

edited

Loading

dboyliao Apr 25, 2025 •

edited

Loading