FIX: [coinbase] refactor channel bridging logic and update message handling #1945
Conversation
dboyliao
commented
Mar 24, 2025
- refactor channel bridging logic
- handle the bridging on connection
- fix message handling
- emit proper bbgo events
- update tests
9802a9f to
4dc1d4e
Compare
0676901 to
cbcb027
Compare
pkg/exchange/coinbase/stream.go
Outdated
| userOrderOnly bool | ||
| bbgoChannelsOnly bool | ||
| authEnabled bool | ||
| subLocalChannelsMap map[types.Channel]struct{} |
There was a problem hiding this comment.
Locking the map is necessary to prevent race conditions
There was a problem hiding this comment.
I will update the map only in the body of OnConnect.
After that, I'll only read from the map.
So I think it's not necessary to add a lock here.
| switch types.Channel(channel) { | ||
| switch channel { | ||
| case rfqMatchChannel: | ||
| subType = "subscriptions" | ||
| default: | ||
| subType = "subscribe" | ||
| } |
There was a problem hiding this comment.
Consider:
subType = "subscribe"
if channel == rfqMatchChannel {
subType = "subscriptions"
}
| case rfqMatchChannel: | ||
| subType = "subscriptions" | ||
| default: | ||
| subType = "subscribe" | ||
| } | ||
| var subCmd any | ||
| switch types.Channel(channel) { | ||
| switch channel { | ||
| case statusChannel: | ||
| subCmd = subscribeMsgType1{ | ||
| Type: subType, |
There was a problem hiding this comment.
Does 4dc1d4e#diff-ff2579743884f3142934a2212709fd2eb1c2e129710e57ecd0b8ed2a1c219332R200 require authentication?
There was a problem hiding this comment.
I handle the authentication here.
| @@ -300,7 +318,13 @@ func (s *Stream) handleMatchMessage(msg *MatchMessage) { | |||
| return | |||
| } | |||
| trade := msg.Trade() | |||
| s.EmitTradeUpdate(trade) | |||
| if _, fullExist := s.subLocalChannelsMap[fullChannel]; fullExist { | |||
There was a problem hiding this comment.
Why not use isPublicOnly
There was a problem hiding this comment.
I want it to be more explicit.
Though PublicOnly == true does imply fullExist.
There was a problem hiding this comment.
After a second thought, I think you are right.
The logic is simpler to check on .PublicOnly.
It's fixed.
| @@ -79,11 +80,11 @@ func Test_OrdersAPI(t *testing.T) { | |||
| break | |||
| } | |||
| time.Sleep(time.Millisecond * 500) | |||
| order, err = ex.QueryOrder(ctx, types.OrderQuery{Symbol: "ETHUSD", OrderID: order.UUID, ClientOrderID: order.UUID}) | |||
| order, err = ex.QueryOrder(ctx, types.OrderQuery{Symbol: symbol, OrderID: order.UUID, ClientOrderID: order.UUID}) | |||
| assert.NoError(t, err) | |||
| // full: all orders/trades on Coinbase Exchange | ||
| // user: orders/trades belong to the user | ||
| var localChannel types.Channel | ||
| if s.PublicOnly { |
There was a problem hiding this comment.
MarketTradeChannel is only available in the publicOnly mode
8214d8f to
c549032
Compare
c549032 to
c99eb16
Compare
…s for consistency
…d tests for simplification
…rivate channel checks
c99eb16 to
3ff403f
Compare
…arket trade channel to `matches`
d6b68b3 to
ee9867f
Compare