Coinbase: Update exchange implementation #1480

cranktakular · 2024-02-14T06:07:40Z

PR Description

Updating Coinbase to the Advanced Trade and Sign In With Coinbase APIs, as well as a few tiny fixes of other parts of the codebase found along the way.

Type of change

Please delete options that are not relevant and add an x in [] as item is complete.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
This change requires a documentation update

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.

go test ./... -race
golangci-lint run

Checklist

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation and regenerated documentation via the documentation tool
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally and on Github Actions with my changes

The only tests which seem to be failing (exchanges/kucoin, database/models/postgres, config, and common/file) are parts that I haven't substantively changed, and which seem to be failing on master too.

Continual enhance of Coinbase tests The revamp continues Oh jeez the Orderbook part's unfinished don't look Coinbase revamp, Orderbook still unfinished

…otrader into coinbase_api_revamp

V3 done, onto V2 Coinbase revamp nears completion Coinbase revamp nears completion Test commit should fail Coinbase revamp nears completion

gloriousCode

Thanks for updating against the API docs after last time
Auth and unauth works well, coverage is excellent. Just some really quick changes from me, but I am happy with its state

gloriousCode · 2025-08-11T00:46:46Z

exchanges/order/orders.go

@@ -40,12 +40,12 @@ var (
 	ErrAmountMustBeSet             = errors.New("amount must be set")
 	ErrClientOrderIDMustBeSet      = errors.New("client order ID must be set")
 	ErrUnknownSubmissionAmountType = errors.New("unknown submission amount type")
+	ErrIDNotSet                    = errors.New("ID not set")


[Required] You have exported this but it is unused

gloriousCode · 2025-08-11T01:31:38Z

exchanges/coinbasepro/coinbasepro_test.go

+	sharedtestvalues.SkipTestIfCredentialsUnset(t, e, canManipulateRealOrders)
+	curSweeps, err := e.ListFuturesSweeps(t.Context())
+	assert.NoError(t, err)
+	partialSkip := false


[Optional] Remove this extra handholding please
Test works fine just as

_, err := e.CancelPendingFuturesSweep(t.Context()) assert.NoError(t, err)

gloriousCode · 2025-08-11T01:33:54Z

exchanges/coinbasepro/coinbasepro_test.go

+	sharedtestvalues.SkipTestIfCredentialsUnset(t, e, canManipulateRealOrders)
+	curSweeps, err := e.ListFuturesSweeps(t.Context())
+	assert.NoError(t, err)
+	preCancel := false


[Optional] Remove this handholding stuff. Just call the func

gloriousCode · 2025-08-11T01:37:41Z

exchanges/coinbasepro/coinbasepro_test.go

+	ordID, err := e.ListOrders(t.Context(), nil, nil, nil, nil, nil, nil, "", "", "", "", "", "", 0, 10, time.Time{}, time.Time{}, currency.Code{})
+	assert.NoError(t, err)
+	if ordID == nil || len(ordID.Orders) == 0 {
+		t.Skip(skipInsufficientOrders)


[Optional] rm handholdy

resp, err := e.GetOrderInfo(t.Context(), "123", testPairStable, asset.Spot)

is fine

Looks like you meant to make this comment about the TestGetOrderInfo function that comes way later. I've removed the handholding there, but not here.

gbjk

Great work 👍

Partial review with feedback on first 28 files.

common/common.go

currency/forexprovider/base/base_interface.go

engine/depositaddress.go

exchanges/coinbasepro/coinbasepro.go

gbjk · 2025-08-11T14:01:32Z

exchanges/coinbasepro/coinbasepro.go

+}
+
+// createOrderConfig populates the OrderConfiguration struct
+func createOrderConfig(orderType order.Type, timeInForce order.TimeInForce, stopDirection string, baseAmount, quoteAmount, limitPrice, stopPrice, bucketSize float64, endTime time.Time, postOnly, rfqDisabled bool, bucketNumber int64, bucketDuration time.Duration) (OrderConfiguration, error) {


🚧 Can we create a type for this, and then embed that type into the structs for PreviewOrderInfo and PlaceOrderInfo ?
Also, once we've done that, if there's a big overlap between those 2 types, can we just make an orderInfoReq that contains a union of the fields?

Do you mean a type for the parameters?

I'm a bit concerned about the user experience behind combining those types due to the points where they don't overlap; we'd at least need some communication over which fields are only relevant to one endpoint.

A type for the fields that both of them send to createOrderConfig, yes, embedded in both.

However you don't need to create a orderInfoReq type to union if it doesn't feel right to you 🙂

exchanges/coinbasepro/coinbasepro.go

gbjk

Good work, thanks for those changes.

Another partial review, up to 35 files.

exchanges/coinbasepro/coinbasepro_websocket.go

gbjk · 2025-08-17T08:56:09Z

exchanges/coinbasepro/coinbasepro_websocket.go

+		var sliToSend []ticker.Price
+		aliases := e.pairAliases.GetAliases()
+		for i := range wsTicker {
+			for j := range wsTicker[i].Tickers {
+				tickAlias := aliases[wsTicker[i].Tickers[j].ProductID]
+				newTick := ticker.Price{
+					LastUpdated:  inc.Timestamp,
+					AssetType:    asset.Spot,
+					ExchangeName: e.Name,
+					High:         wsTicker[i].Tickers[j].High24H.Float64(),
+					Low:          wsTicker[i].Tickers[j].Low24H.Float64(),
+					Last:         wsTicker[i].Tickers[j].Price.Float64(),
+					Volume:       wsTicker[i].Tickers[j].Volume24H.Float64(),
+					Bid:          wsTicker[i].Tickers[j].BestBid.Float64(),
+					BidSize:      wsTicker[i].Tickers[j].BestBidQuantity.Float64(),
+					Ask:          wsTicker[i].Tickers[j].BestAsk.Float64(),
+					AskSize:      wsTicker[i].Tickers[j].BestAskQuantity.Float64(),
+				}
+				var errs error
+				for k := range tickAlias {
+					isEnabled, err := e.CurrencyPairs.IsPairEnabled(tickAlias[k], asset.Spot)
+					if err != nil {
+						errs = common.AppendError(errs, err)
+						continue
+					}
+					if isEnabled {
+						newTick.Pair = tickAlias[k]
+						sliToSend = append(sliToSend, newTick)
+					}
+				}


🚧
Can this go in a function please?
Same for the other sections below.
wsHandleData is already unwieldy enough as it is, but inlining all this processing just makes it huge.
Also implies more targeted unit tests, probably.

I've split these out, but have left the tests together for now; the test is already quite thorough, and splitting that out seems like quite a bit more of a hassle.

exchanges/coinbasepro/coinbasepro_websocket.go

gbjk

Changes since last review ... reviewed.

Great work.

Just one thing.

gbjk · 2025-08-19T05:17:35Z

exchanges/coinbasepro/coinbasepro_types.go

@@ -365,29 +365,34 @@ type ErrorResponse struct {
 	NewOrderFailureReason string `json:"new_order_failure_reason"`
 }

+// SharedOrderConfig contains order configuration information used in both PlaceOrderInfo and PreviewOrderInfo
+type SharedOrderConfig struct {


🦍 I don't think we need to say it's shared. It's just OrderConfig.
I think you're only thinking about it being shared because of where you started.

Suggested change

type SharedOrderConfig struct {

type OrderConfig struct {

Do not need to action.

There's already an OrderConfiguration type, which contains the full, properly-formatted depth of information. Typing that out, "shared" isn't the right way to distinguish this, but the other ideas I have (OrderConfigPreparation, OrderConfigConstructor) seem more verbose and also a bit wrong.

I do think that just "OrderConfig" is a bit close for comfort.

You're right, that would not fly.

OrderConfiguration smells like OrderType or OrderConds
Everything in there is about order type conditions.

SharedOrderConfig lives in 2 structs named *OrderInfo. So I think it's actually just OrderInfo 😄

The OrderConfiguration naming comes from what the exchange itself calls it. Would like to double-check changing it.

But I'll apply that change from SharedOrderConfig > OrderInfo.

exchanges/coinbasepro/coinbasepro_websocket.go

gbjk

56 / 57 files reviewed

Only coinbasepro_wrapper.go remaining.

portfolio/withdraw/withdraw_types.go

gbjk · 2025-08-19T05:35:47Z

exchanges/protocol/features.go

@@ -3,6 +3,7 @@ package protocol
 // Features holds all variables for the exchanges supported features
 // for a protocol (e.g REST or Websocket)
 type Features struct {
+	// TickerBatching allows the REST endpoint to fetch the entire ticker list available to the exchange


🐒

I don't think this should say REST

Are we really saying it's always the entire thing. That feels like an exchange implementation thing. I think all we're saying here is "the exchange protocol allows batching". I could be wrong.

Typo at the end of the sentence, but we can omit it.
Guessing we want:

Suggested change

// TickerBatching allows the REST endpoint to fetch the entire ticker list available to the exchange

// TickerBatching allows api to fetch the tickers in batches

Though ... I mean ... it's not adding any information for me 🤷

The state of this value doesn't seem to be directly used by our engine anywhere, so we don't functionally enshrine a definition that way.

It's hard to tell exactly why I added that comment, as it was written 20 months ago, but I suspect that I didn't understand exactly what that meant, asked shazbert/thrasher, and they responded with that. But really, even now, I wouldn't consider myself to have an authoritative view on what the definition ought to be. Maybe wait for input from others?

"the exchange protocol allows batching": this is correct. As in, the exchange supports an API which allows us to do a batch request for all available tickers.

Engine's syncer will attempt to use REST ticker batching if possible since it can update all tickers with a single API request as opposed to iterating over a pairs list and fetching a ticker with numerous requests:

https://github.com/thrasher-corp/gocryptotrader/blob/master/engine/sync_manager.go#L572

Which will then check if b.Features.Supports.RESTCapabilities.TickerBatching == true

This is mainly a feature supported via REST which is why there's an emphasis on it, since websocket will just stream events in realtime or close to it.

As for the comment, both websocket and REST have their own supported Features and I don't think we need a comment for it

I think there's a possible confusion; "allows batching" as in "you request a batch of tickers for a pair, and can receive up to 50 ticks for that pair", or as in "you can simultaneously request the latest ticker for every pair on the exchange", or even "you can simultaneously request the latest ticker for up to N pairs on the exchange".

I expect that's what I was initially confused about.

In all cases it's "you can request the latest tickers for every pair on the exchange in a single batched request". Can't think of any exchanges which require pagination for doing a bulk request of tickers off the top of my head

Ah, I seem to have confused it with candles, my bad.

Yeah it should be fine without a comment then.

Note: Still outstanding to remove the comment.

gbjk · 2025-08-19T05:42:01Z

exchanges/order/order_types.go

@@ -370,6 +379,7 @@ const (
 	Chase           // chase limit order
 	OptimalLimit
 	MarketMakerProtection
+	Bracket // Sets both a profit target and a stop loss simultaneously.


🚧 We already have StopLimit and StopMarket types, which is a synonym for Bracket order.
So I think that this is about treating the text representation of StopLimit as BRACKET for Coinbase, maybe?

Suggested change

Bracket // Sets both a profit target and a stop loss simultaneously.

Bracket // Sets both a profit target and a stop loss simultaneously.

Are those a synonym for Bracket? I thought that Bracket would be Stop | TakeProfit (and now that such hybrid order types are allowed, I could change it to that).

You're right, I wasn't focusing on the difference between TakeProfit and Limit.

I think you're right that we should switch to a hybrid type, but I think that needs more input from others.

gloriousCode

I think this is ready to merge. Tested auth and unauth and ensured the websocket order book stays in sync

Nice work Mr. Crank

gloriousCode · 2025-08-20T01:51:42Z

engine/depositaddress.go

 }

+// ExchangeDepositAddresses is a map of currencies to their deposit addresses
+type ExchangeDepositAddresses map[string][]deposit.Address


A surprising change in the Coinbase PR 😄

gloriousCode · 2025-08-20T01:55:16Z

exchanges/coinbasepro/coinbasepro.go

+	closedStatuses = []string{"FILLED", "CANCELLED", "EXPIRED", "FAILED"}
+	openStatus     = []string{"OPEN"}


I don't really think these are needed, but its not worth a change unless other things come up

gloriousCode · 2025-08-20T01:56:29Z

exchanges/coinbasepro/coinbasepro.go

+		"trade_incentive_metadata": map[string]any{
+			"user_incentive_id": userIncentiveID,
+			"code_val":          codeVal,
+		},


no real need for map[string]any when you only use map[string]string. Not an issue, just an observation

gloriousCode · 2025-08-20T01:59:36Z

exchanges/coinbasepro/coinbasepro.go

 	if productID == "" {
 		return nil, errProductIDEmpty
 	}
-	if !common.StringSliceContains(allowedGranularities, granularity) {
+	gran, ok := allowedGranularities[granularity]


My gran is always allowed to retrieve historical Klines, thank you

gbjk

Thanks - Minor changes

gbjk · 2025-08-25T05:00:28Z

exchanges/coinbasepro/coinbasepro_types.go

@@ -365,29 +365,34 @@ type ErrorResponse struct {
 	NewOrderFailureReason string `json:"new_order_failure_reason"`
 }

+// SharedOrderConfig contains order configuration information used in both PlaceOrderInfo and PreviewOrderInfo
+type SharedOrderConfig struct {


You're right, that would not fly.

OrderConfiguration smells like OrderType or OrderConds
Everything in there is about order type conditions.

SharedOrderConfig lives in 2 structs named *OrderInfo. So I think it's actually just OrderInfo 😄

gbjk · 2025-08-25T05:02:07Z

exchanges/coinbasepro/coinbasepro_websocket.go

-	err := json.Unmarshal(respRaw, &msgType)
-	if err != nil {
+// tickerDataHandler handles ticker data from the websocket
+func (e *Exchange) tickerDataHandler(resp *StandardWebsocketResponse) error {


🚧 Thanks for breaking these out.
Can we normalise naming to match other exchanges, which call these wsProcess* ?
That is: I don't (ever) think "Data" adds anything, and we lack ws context.

Suggested change

func (e *Exchange) tickerDataHandler(resp *StandardWebsocketResponse) error {

func (e *Exchange) wsProcessTicker(resp StandardWebsocketResponse) error {

Can probably hit them all with a regex:

:%s/\s(.*?)DataHandler\(/wsProcess\1/g

portfolio/withdraw/withdraw_types.go

gbjk · 2025-08-25T06:34:55Z

exchanges/protocol/features.go

@@ -3,6 +3,7 @@ package protocol
 // Features holds all variables for the exchanges supported features
 // for a protocol (e.g REST or Websocket)
 type Features struct {
+	// TickerBatching allows the REST endpoint to fetch the entire ticker list available to the exchange


Note: Still outstanding to remove the comment.

gbjk · 2025-08-25T06:38:55Z

exchanges/order/order_types.go

@@ -370,6 +379,7 @@ const (
 	Chase           // chase limit order
 	OptimalLimit
 	MarketMakerProtection
+	Bracket // Sets both a profit target and a stop loss simultaneously.


You're right, I wasn't focusing on the difference between TakeProfit and Limit.

I think you're right that we should switch to a hybrid type, but I think that needs more input from others.

gbjk · 2025-08-26T01:11:25Z

exchanges/coinbasepro/coinbasepro.go

+	req := map[string]any{
+		"client_order_id":              ord.ClientOID,
+		"product_id":                   ord.ProductID,
+		"side":                         ord.Side,
+		"order_configuration":          orderConfig,
+		"retail_portfolio_id":          ord.RetailPortfolioID,
+		"preview_id":                   ord.PreviewID,
+		"attached_order_configuration": ord.AttachedOrderConfiguration,
+	}
+	if ord.MarginType != "" {
+		req["margin_type"] = FormatMarginType(ord.MarginType)
+	}
+	if ord.Leverage != 0 && ord.Leverage != 1 {
+		req["leverage"] = strconv.FormatFloat(ord.Leverage, 'f', -1, 64)
+	}


🚧 I realised that this appears to be a misplaced adhoc map instead of a PlaceOrderInfo having json tags and marhsaling itself correctly.

Sorry if I'm missing something, but everything here appears to suggest that the entire thing should be a normal request struct.

What do you mean? I'd need to rewrite the authenticated request function to be able to place bespoke structs rather than map[string]any objects in for the body of a request. And applying this broadly would require rewriting parts of 20 function calls.

Yep, I think you've understood it already, because "I'd need to X" matches exactly what you need to do.
We use structured data everywhere else.
If it's possible, we should do it.

Would you like me to provide a patch?

That would be quite helpful.

gbjk

Sorry. I know you'll hate me for this, but now is the right time to do this:
Can we rename coinbasepro to coinbase inline with their renaming 3 years ago.

cranktakular added 28 commits October 9, 2023 08:42

Slight enhance of Coinbase tests

a006163

Continual enhance of Coinbase tests The revamp continues Oh jeez the Orderbook part's unfinished don't look Coinbase revamp, Orderbook still unfinished

Merge branch 'master' into coinbase_api_revamp

074f367

Coinbase revamp; CreateReport is still WIP

bd9f2dd

More coinbase improvements; onto sandbox testing

25fcd2e

Coinbase revamp continues

5d3fd99

Coinbase revamp continues

3aca36b

Merge branch 'coinbase_api_revamp' of github.com:cranktakular/gocrypt…

f58b76d

…otrader into coinbase_api_revamp

Coinbasepro revamp is ceaseless

eded160

Coinbase revamp, starting on advanced trade API

5a0ba23

Coinbase Advanced Trade Starts in Ernest

88e6713

V3 done, onto V2 Coinbase revamp nears completion Coinbase revamp nears completion Test commit should fail Coinbase revamp nears completion

Coinbase revamp stage wrapper

aa2efef

Merge branch 'master' into coinbase_api_revamp

3560599

Coinbase wrapper coherence continues

4593828

Coinbase wrapper continues writhing

a8cae40

Coinbase wrapper & codebase cleanup

0564e45

Coinbase updates & wrap progress

4f5ccb5

More Coinbase wrapper progress

9a6c7e6

Wrapper is wrapped, kinda

2896b3e

Test & type checking

ffb43d8

Coinbase REST revamp finished

7dd4589

Merge branch 'master' into coinbase_api_revamp

6d7ac5f

Post-merge fix

2881841

WS revamp begins

2166886

WS Main Revamp Done?

35eda67

CB websocket tidying up

6ac86cd

Coinbase WS wrapperupperer

cfe6e11

Coinbase revamp done??

2f770d5

Merge branch 'master' into coinbase_api_revamp

3e54528

thrasher- changed the title ~~Coinbase api revamp~~ Coinbase: Update exchange implementation Feb 14, 2024

thrasher- requested a review from a team February 14, 2024 23:40

gloriousCode requested changes Aug 11, 2025

View reviewed changes

gbjk requested changes Aug 11, 2025

View reviewed changes

Review fixes

04d2218

cranktakular mentioned this pull request Aug 12, 2025

bybit: Add websocket trading functionality across all assets #1672

Open

15 tasks

gbjk requested changes Aug 17, 2025

View reviewed changes

cranktakular added 3 commits August 19, 2025 14:19

GK's suggestions

c008869

Merge branch 'master' into coinbase_api_revamp

c2726b9

Linter fix

330e848

cranktakular requested review from gbjk and gloriousCode August 19, 2025 05:09

gbjk requested changes Aug 19, 2025

View reviewed changes

thrasher- requested review from samuael and thrasher- August 19, 2025 06:06

Minor gbjk fixes

2e685ce

cranktakular requested a review from gbjk August 19, 2025 06:13

gloriousCode approved these changes Aug 20, 2025

View reviewed changes

gbjk requested changes Aug 25, 2025

View reviewed changes

Nit fixes

0411e46

gbjk requested changes Aug 26, 2025

View reviewed changes

cranktakular added 2 commits August 26, 2025 11:19

Merge branch 'master' into coinbase_api_revamp

6651cda

Merge fix

587eefd

gbjk requested changes Aug 26, 2025

View reviewed changes

cranktakular added 7 commits August 26, 2025 12:09

Lint fixes

2e5b6fe

Merge branch 'master' into coinbase_api_revamp

1ff7838

Coinbase rename stage 1

8a1a020

Coinbase rename stage 2

bd69baa

Coinbase rename stage 3

fce9f4e

Coinbase rename stage 4

a13ce5b

Coinbase rename final fix

b16b543

	// TickerBatching allows the REST endpoint to fetch the entire ticker list available to the exchange
	// TickerBatching allows api to fetch the tickers in batches

	Bracket // Sets both a profit target and a stop loss simultaneously.
	Bracket // Sets both a profit target and a stop loss simultaneously.

		closedStatuses = []string{"FILLED", "CANCELLED", "EXPIRED", "FAILED"}
		openStatus = []string{"OPEN"}

	func (e Exchange) tickerDataHandler(resp StandardWebsocketResponse) error {
	func (e *Exchange) wsProcessTicker(resp StandardWebsocketResponse) error {

Coinbase: Update exchange implementation #1480

Are you sure you want to change the base?

Coinbase: Update exchange implementation #1480

Conversation

cranktakular commented Feb 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Description

Type of change

How has this been tested

Checklist

Uh oh!

gloriousCode left a comment

Choose a reason for hiding this comment

Uh oh!

gloriousCode Aug 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gloriousCode Aug 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gloriousCode Aug 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gloriousCode Aug 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gbjk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gbjk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cranktakular Aug 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gbjk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cranktakular commented Feb 14, 2024 •

edited

Loading

gloriousCode Aug 11, 2025 •

edited

Loading

gloriousCode Aug 11, 2025 •

edited

Loading

gloriousCode Aug 11, 2025 •

edited

Loading

gloriousCode Aug 11, 2025 •

edited

Loading

cranktakular Aug 19, 2025 •

edited

Loading