Coinbase: Update exchange implementation #1480
Conversation
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
thrasher-
changed the title
samuael
left a comment
samuael
left a comment
There was a problem hiding this comment.
I like this PR 👍 . I just left some reviewws and
will drop another round of reviews when this are resolved.
| errDepositIDEmpty = errors.New("deposit id cannot be empty") | ||
| errInvalidPriceType = errors.New("price type must be spot, buy, or sell") | ||
| errInvalidOrderType = errors.New("order type must be market, limit, or stop") | ||
| errEndTimeInPast = errors.New("end time cannot be in the past") |
There was a problem hiding this comment.
could common.ErrStartAfterEnd replace this?
There was a problem hiding this comment.
Technically, start time could be even further in the past, making this error more apt.
exchanges/coinbase/coinbase.go
Outdated
| errAccountIDEmpty = errors.New("account id cannot be empty") | ||
| errClientOrderIDEmpty = errors.New("client order id cannot be empty") | ||
| errProductIDEmpty = errors.New("product id cannot be empty") | ||
| errOrderIDEmpty = errors.New("order ids cannot be empty") |
There was a problem hiding this comment.
couldn't you use order.ErrOrderIDNotSet instead for consistency.
exchanges/coinbase/coinbase.go
Outdated
|
|
||
| var ( | ||
| errAccountIDEmpty = errors.New("account id cannot be empty") | ||
| errClientOrderIDEmpty = errors.New("client order id cannot be empty") |
There was a problem hiding this comment.
can replace this with order.ErrClientOrderIDMustBeSet
exchanges/coinbase/coinbase.go
Outdated
| // CancelOrders cancels orders by orderID. Can only cancel 100 orders per request | ||
| func (e *Exchange) CancelOrders(ctx context.Context, orderIDs []string) ([]OrderCancelDetail, error) { | ||
| if len(orderIDs) == 0 { | ||
| return nil, errOrderIDEmpty |
There was a problem hiding this comment.
| return nil, errOrderIDEmpty | |
| return nil, order.ErrOrderIDNotSet |
exchanges/coinbase/coinbase.go
Outdated
| // ClosePosition closes a position by client order ID, product ID, and size | ||
| func (e *Exchange) ClosePosition(ctx context.Context, clientOrderID string, productID currency.Pair, size float64) (*SuccessFailureConfig, error) { | ||
| if clientOrderID == "" { | ||
| return nil, errClientOrderIDEmpty |
There was a problem hiding this comment.
can change all occurrences of such type with order.ErrClientOrderIDMustBeSet
exchanges/coinbase/coinbase.go
Outdated
| params.Values.Set("cursor", strconv.FormatInt(req.Cursor, 10)) | ||
| params.Values.Set("limit", strconv.FormatInt(int64(req.Limit), 10)) | ||
| params.Values.Set("user_native_currency", req.UserNativeCurrency.String()) | ||
| params.Values.Set("retail_portfolio_id", req.RetailPortfolioID) // deprecated and only works for legacy API keys |
There was a problem hiding this comment.
I don't see a section where you check the value of these variables if they were required
There was a problem hiding this comment.
As of the time I tested these functions, the parameters are....
- Checked at the start, with an error thrown if empty, if they are required.
- Only set if they're not empty, if assigning them while empty causes an error.
- Set regardless, otherwise.
These values landed in the last camp.
| AttachedOrderConfiguration: &inf.AttachedOrderConfiguration, | ||
| MarginType: FormatMarginType(inf.MarginType), | ||
| } | ||
| var resp *PreviewOrderResp |
There was a problem hiding this comment.
You can use this approach everywhere for consistency. I see you used var resp TheStruct --> return &resp, e.Sen... at some endpoint calls.
exchanges/coinbase/coinbase.go
Outdated
| return nil, err | ||
| } | ||
| var resp *ManyDeposWithdrResp | ||
| if err := e.SendAuthenticatedHTTPRequest(ctx, exchange.RestSpot, http.MethodGet, path, params.Values, nil, false, &resp); err != nil { |
There was a problem hiding this comment.
Why the error check here? couldnt it be handled the calling code
| if walletID == "" { | ||
| return nil, errWalletIDEmpty | ||
| } | ||
| path := v2Path + accountsPath + "/" + walletID + "/" + transactionsPath |
There was a problem hiding this comment.
move this to below the error check
exchanges/coinbase/coinbase_test.go
Outdated
| longResp, err := e.ListAccounts(t.Context(), 49, 0) | ||
| assert.NoError(t, err) | ||
| if longResp == nil || len(longResp.Accounts) == 0 { | ||
| t.Fatal(errExpectedNonEmpty) |
There was a problem hiding this comment.
replace this with require.True
gbjk
left a comment
gbjk
left a comment
There was a problem hiding this comment.
Great work
🐒 Optional nitpick
🌴 This issue lives in multiple other places
🦍 Optional and pedantic nitpick
🚧 Change request
🥃 Just conversational observation
| } | ||
|
|
||
| type pairAliases struct { | ||
| associatedAliases map[currency.Pair]currency.Pairs |
There was a problem hiding this comment.
🦍 Can be simpler
| associatedAliases map[currency.Pair]currency.Pairs | |
| pairs map[currency.Pair]currency.Pairs |
| @@ -1916,25 +1905,13 @@ func TestChannelName(t *testing.T) { | |||
| } | |||
|
|
|||
| func exchangeBaseHelper(e *Exchange) error { | |||
There was a problem hiding this comment.
🐒 Precedence for this is:
| func exchangeBaseHelper(e *Exchange) error { | |
| // testInstance returns a local instance for isolated testing | |
| func testInstance(e *Exchange) error { |
There was a problem hiding this comment.
Is that right? "testInstance" and "local instance" give, to me, the impression that it's mocking functionality. Heck, from the function signature itself it's not "returning" an instance, just executing some common initialisation.
There was a problem hiding this comment.
Sorry, I was lazy and didn't check the rest of the function.
| func exchangeBaseHelper(e *Exchange) error { | |
| func testInstance(tb testing.TB) *Exchange { |
See kucoin_test.go for example
I'd try to use something like that in the places you've got all the bootstrapping for the exchange in this test.
Note: This nit is optional.
exchanges/coinbase/coinbase_types.go
Outdated
| // ValueWithCurrency is a sub-struct used in the types Account, NativeAndRaw, DetailedPortfolioResponse, ErrorMetadata, SubscriptionInfo, FuturesBalanceSummary, ListFuturesSweepsResponse, PerpetualsPortfolioSummary, PerpPositionDetail, FeeStruct, AmScale, and ConvertResponse | ||
| type ValueWithCurrency struct { |
There was a problem hiding this comment.
🚧
- Definitely don't want to set precedent of listing all the places a symbol is used.
🌴 There's a few places we've mentioned where something is used/returned below which are also unnecessary. It just clouds search results and doesn't add information the IDE wouldn't give you. Valuefeels like it'sAmount. But then this entire concept has other names. We could go forCurrAmount, or justMonetary, orMoney. Happy with anything, butValuefeels meaningless (in the context of code, because it's an overloaded term here, and we can't assume financial value)- 🥃 Aside: The term sub-struct is misleading unless we mean embedded anonymous structs. Which we don't like anyway. And this isn't one.
| // ValueWithCurrency is a sub-struct used in the types Account, NativeAndRaw, DetailedPortfolioResponse, ErrorMetadata, SubscriptionInfo, FuturesBalanceSummary, ListFuturesSweepsResponse, PerpetualsPortfolioSummary, PerpPositionDetail, FeeStruct, AmScale, and ConvertResponse | |
| type ValueWithCurrency struct { | |
| // CurrAmount is holds a currencycode and amount | |
| type CurrAmount struct { |
There was a problem hiding this comment.
Yeah, I made a very bad decision when I first started writing comments on this types file, and I have been paying the price ever since. I can easily chuck it out from this struct, but it would be a bit annoying to do that over the whole file.
I'll move to CurrAmount.
There was a problem hiding this comment.
Yeah - sure. Chuck it out of this comment and any others which we spot that are crazy long.
The rest of them can be hit as driveby fixes later, it's fine.
exchanges/coinbase/coinbase_types.go
Outdated
| // ValueWithCurrency is a sub-struct used in the types Account, NativeAndRaw, DetailedPortfolioResponse, ErrorMetadata, SubscriptionInfo, FuturesBalanceSummary, ListFuturesSweepsResponse, PerpetualsPortfolioSummary, PerpPositionDetail, FeeStruct, AmScale, and ConvertResponse | ||
| type ValueWithCurrency struct { | ||
| Value types.Number `json:"value"` | ||
| Currency string `json:"currency"` |
There was a problem hiding this comment.
🚧
| Currency string `json:"currency"` | |
| Currency currency.Code `json:"currency"` |
exchanges/coinbase/coinbase.go
Outdated
| var params Params | ||
| params.Values = url.Values{} | ||
| if err := params.encodeDateRange(startDate, endDate, startDateString, endDateString); err != nil { |
There was a problem hiding this comment.
🚧 I quite liked this pattern at first.
But then I realised we are always declaring it, and then calling a method on it, right?
So ... cut out the type and just have a func that returns a straight url.Values
You can either make the 2 funcs that we call just return a fresh url.Values, or you could pass one in.
The former approach seems like less code everywhere.
All the lines below this just become params.Set instead, and everything is clearer.
| var params Params | |
| params.Values = url.Values{} | |
| if err := params.encodeDateRange(startDate, endDate, startDateString, endDateString); err != nil { | |
| params := encodeDateRange(startDate, endDate, startDateString, endDateString) | |
| if err != nil { |
There was a problem hiding this comment.
Yeah, it was set up this way from the start due to the potential of calling both methods (or any other ones I might add) within one function, but that hasn't made its way to the current version. I'll make that swap.
exchanges/exchange.go
Outdated
| var ErrSymbolCannotBeMatched = errors.New("symbol cannot be matched") | ||
| // Public Errors | ||
| var ( | ||
| ErrSettingProxyAddress = errors.New("setting proxy address error") |
There was a problem hiding this comment.
🚧
| ErrSettingProxyAddress = errors.New("setting proxy address error") | |
| ErrSettingProxyAddress = errors.New("error setting proxy address") |
exchanges/exchange.go
Outdated
| var ( | ||
| ErrSettingProxyAddress = errors.New("setting proxy address error") | ||
| ErrEndpointPathNotFound = errors.New("no endpoint path found for the given key") | ||
| ErrSymbolCannotBeMatched = errors.New("symbol cannot be matched") |
There was a problem hiding this comment.
🐒 Optional change
| ErrSymbolCannotBeMatched = errors.New("symbol cannot be matched") | |
| ErrSymbolNotMatched = errors.New("symbol cannot be matched") |
exchanges/exchange.go
Outdated
| diff.Remove) | ||
| } | ||
| } | ||
| // TODO: Add check for nil config etc. |
There was a problem hiding this comment.
🚧 Please fulfil this TODO 🙂
Note you can use common.NilGuard
|
@codex please review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codex Review: Here are some suggestions.
About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".
| // GetActiveOrders retrieves any orders that are active/open | ||
| func (e *Exchange) GetActiveOrders(ctx context.Context, req *order.MultiOrderRequest) (order.FilteredOrders, error) { | ||
| if req == nil { | ||
| return nil, common.ErrNilPointer | ||
| } | ||
| err := req.Validate() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| var respOrders []GetOrderResponse | ||
| ordStatus := []string{"OPEN"} | ||
| if respOrders, err = e.iterativeGetAllOrders(ctx, req.Pairs, req.Type.String(), req.Side.String(), req.AssetType.Upper(), ordStatus, 1000, req.StartTime, req.EndTime); err != nil { | ||
| return nil, err | ||
| } | ||
| orders := make([]order.Detail, len(respOrders)) | ||
| for i := range respOrders { | ||
| orderRec := e.getOrderRespToOrderDetail(&respOrders[i], respOrders[i].ProductID, asset.Spot) | ||
| orders[i] = *orderRec |
There was a problem hiding this comment.
[P1] Preserve requested asset type when reporting active orders
In GetActiveOrders the converted order.Detail is always created with asset.Spot, ignoring the caller’s requested req.AssetType. This means a futures request will return orders whose AssetType is mislabelled as spot, so downstream filters and order management that depend on the asset type will silently skip or mis-handle those orders. The conversion in GetOrderHistory correctly forwards req.AssetType; the same should happen here.
Useful? React with 👍 / 👎.
exchanges/coinbase/coinbase.go
Outdated
| return sign, regTime.Add(time.Minute * 2), err | ||
| // The code below mostly works, but seems to lead to bad results on the signature step. Deferring until later |
There was a problem hiding this comment.
When I was reviewing this for some fun I asked GPT5 to resolve this and looks like it's working:
// GetJWT generates a new JWT
func (e *Exchange) GetJWT(ctx context.Context, uri string) (string, time.Time, error) {
creds, err := e.GetCredentials(ctx)
if err != nil {
return "", time.Time{}, err
}
block, _ := pem.Decode([]byte(creds.Secret))
if block == nil {
return "", time.Time{}, errCantDecodePrivKey
}
privateKey, err := x509.ParseECPrivateKey(block.Bytes)
if err != nil {
return "", time.Time{}, err
}
nonce, err := common.GenerateRandomString(16, "1234567890ABCDEF")
if err != nil {
return "", time.Time{}, err
}
// Manual JWS (ES256) implementation
head := map[string]any{
"kid": creds.Key,
"typ": "JWT",
"alg": "ES256",
"nonce": nonce,
}
headJSON, err := json.Marshal(head)
if err != nil {
return "", time.Time{}, err
}
headEnc := base64.RawURLEncoding.EncodeToString(headJSON)
regTime := time.Now()
body := map[string]any{
"iss": "cdp",
"nbf": regTime.Unix(),
"exp": regTime.Add(2 * time.Minute).Unix(),
"sub": creds.Key,
}
if uri != "" {
body["uri"] = uri
}
bodyJSON, err := json.Marshal(body)
if err != nil {
return "", time.Time{}, err
}
bodyEnc := base64.RawURLEncoding.EncodeToString(bodyJSON)
// Signing input per JWS
signingInput := headEnc + "." + bodyEnc
hash := sha256.Sum256([]byte(signingInput))
// IMPORTANT: JWS needs raw R||S, not ASN.1 DER
r, s, err := ecdsa.Sign(rand.Reader, privateKey, hash[:])
if err != nil {
return "", time.Time{}, err
}
// Optional: enforce low-S to avoid malleability
n := privateKey.Params().N
halfN := new(big.Int).Rsh(n, 1)
if s.Cmp(halfN) == 1 {
s.Sub(n, s)
}
// Serialise R||S, each left-padded to 32 bytes for P-256
rb := r.Bytes()
sb := s.Bytes()
sig := make([]byte, 64)
copy(sig[32-len(rb):32], rb)
copy(sig[64-len(sb):], sb)
sigEnc := base64.RawURLEncoding.EncodeToString(sig)
return signingInput + "." + sigEnc, regTime.Add(2 * time.Minute), nil
}
Though I'd say we can just leave the current JWT implementation and can rid base64URLEncode since it isn't being used currently
There was a problem hiding this comment.
As per newer information we've discussed (the NPM attack, the fact that we only use the external package for this single exchange), I have tested and will be implementing this GPT5-written code.
We can reconsider the utility of the dependency if/when other exchanges need JWT.
gloriousCode
left a comment
gloriousCode
left a comment
There was a problem hiding this comment.
I didnt come across much from checking changes since my last review. I'll do some more testing but should still be happy with. it
exchanges/coinbase/coinbase.go
Outdated
| return resp, e.SendHTTPRequest(ctx, exchange.RestSpot, v3Path+timePath, nil, &resp) | ||
| } | ||
|
|
||
| type sendMoneyReqBase struct { |
exchanges/coinbase/coinbase.go
Outdated
| return resp, e.SendAuthenticatedHTTPRequest(ctx, exchange.RestSpot, http.MethodGet, path, params.Values, nil, false, &resp) | ||
| } | ||
|
|
||
| type fiatTransferReqBase struct { |
exchanges/coinbase/coinbase.go
Outdated
| var resp *ManyDeposWithdrResp | ||
| err := e.SendAuthenticatedHTTPRequest(ctx, exchange.RestSpot, http.MethodGet, path, params.Values, nil, false, &resp) | ||
| if err != nil { | ||
| if err := e.SendAuthenticatedHTTPRequest(ctx, exchange.RestSpot, http.MethodGet, path, params.Values, nil, false, &resp); err != nil { |
There was a problem hiding this comment.
Seems like a hot area. This could have worked but this isn't holding up anything:
return resp, e.SendAuthenticatedHTTPRequest(ctx, exchange.RestSpot, http.MethodGet, path, params.Values, nil, false, &resp)
thrasher-
left a comment
thrasher-
left a comment
There was a problem hiding this comment.
Thanks for making those changes! Almost there
| ) | ||
|
|
||
| func TestMain(m *testing.M) { | ||
| if testingInSandbox { |
There was a problem hiding this comment.
Needs to be placed after the exchangeBaseHelper otherwise would be overridden
exchanges/coinbase/coinbase_test.go
Outdated
| assert.ErrorIs(t, err, errUnrecognisedStrategyType) | ||
| } | ||
|
|
||
| func TestBase64URLEncode(t *testing.T) { |
There was a problem hiding this comment.
Can be removed now since you're using base64 rawdog encoding which does the same thing
| e.API.Endpoints = e.NewEndpoints() | ||
| if err = e.API.Endpoints.SetDefaultEndpoints(map[exchange.URL]string{ | ||
| exchange.RestSpot: apiURL, | ||
| exchange.RestSandbox: sandboxAPIURL, |
There was a problem hiding this comment.
Noticed that this sandbox URL differs to the one in config_example.json
There was a problem hiding this comment.
Good catch! Fixed.
| return nil, err | ||
| } | ||
| var respOrders []GetOrderResponse | ||
| ordStatus := []string{"OPEN"} |
There was a problem hiding this comment.
Can remove this and use openStatus since they're the same
| e.Enabled = true | ||
| e.API.CredentialsValidator.RequiresKey = true | ||
| e.API.CredentialsValidator.RequiresSecret = true | ||
| e.API.CredentialsValidator.RequiresBase64DecodeSecret = false |
There was a problem hiding this comment.
Can rm this since the default value is false
| for i := range aliases { | ||
| isEnabled, err := e.CurrencyPairs.IsPairEnabled(aliases[i], assetType) | ||
| if err != nil { | ||
| errs = fmt.Errorf("%v %v", errs, err) |
There was a problem hiding this comment.
| errs = fmt.Errorf("%v %v", errs, err) | |
| errs = common.AppendErrors(errs, err) |
| if isEnabled { | ||
| book.Pair = aliases[i] | ||
| if err := book.Process(); err != nil { | ||
| errs = fmt.Errorf("%v %v", errs, err) |
There was a problem hiding this comment.
| errs = fmt.Errorf("%v %v", errs, err) | |
| errs = common.AppendErrors(errs, err) |
exchanges/coinbase/coinbase.go
Outdated
| var params Params | ||
| params.Values = url.Values{} | ||
| if err := params.encodeDateRange(startDate, endDate, startDateString, endDateString); err != nil { | ||
| vals, err := encodeDateRange(startDate, endDate, startDateString, endDateString) |
There was a problem hiding this comment.
🦍 vals isn't a great name. Values is only valid in the context of url.
These are url params, or params.
| vals, err := encodeDateRange(startDate, endDate, startDateString, endDateString) | |
| params, err := encodeDateRange(startDate, endDate, startDateString, endDateString) |
There was a problem hiding this comment.
The variable returned by that function is of the type url.Values, so wouldn't that be fine?
There was a problem hiding this comment.
Because Values only has meaning with the context of url.Values.
As soon as you drop url, you're just calling this things.
I've just seen the func is renamed. That actually helps a lot.
Happy for you to ignore or action.
exchanges/coinbase/coinbase.go
Outdated
| var params Params | ||
| params.Values = url.Values{} | ||
| if err := params.encodeDateRange(startDate, endDate, "start_sequence_timestamp", "end_sequence_timestamp"); err != nil { | ||
| vals, err := encodeDateRange(startDate, endDate, "start_sequence_timestamp", "end_sequence_timestamp") |
There was a problem hiding this comment.
🦍 🌴 Same as above
| vals, err := encodeDateRange(startDate, endDate, "start_sequence_timestamp", "end_sequence_timestamp") | |
| params, err := encodeDateRange(startDate, endDate, "start_sequence_timestamp", "end_sequence_timestamp") |
exchanges/coinbase/coinbase.go
Outdated
| var params Params | ||
| params.Values = make(url.Values) | ||
| if err := params.encodeDateRange(req.StartDate, req.EndDate, startDateString, endDateString); err != nil { | ||
| vals, err := encodeDateRange(req.StartDate, req.EndDate, startDateString, endDateString) |
There was a problem hiding this comment.
🦍 🌴
| vals, err := encodeDateRange(req.StartDate, req.EndDate, startDateString, endDateString) | |
| params, err := encodeDateRange(req.StartDate, req.EndDate, startDateString, endDateString) |
exchanges/coinbase/coinbase.go
Outdated
|
|
||
| // GetAllWallets lists all accounts associated with the API key | ||
| func (e *Exchange) GetAllWallets(ctx context.Context, pag PaginationInp) (*GetAllWalletsResponse, error) { | ||
| vals := encodePagination(pag) |
There was a problem hiding this comment.
🦍 🌴
| vals := encodePagination(pag) | |
| params := encodePagination(pag) |
shazbert
left a comment
shazbert
left a comment
There was a problem hiding this comment.
Changes look great just some minor nits if you so choose.
| if err != nil { | ||
| return book, err | ||
| } | ||
| if orderbookNew, err = e.GetProductBookV3(ctx, p, 1000, 0, false); err != nil { |
There was a problem hiding this comment.
suggestion: doesn't look like you need to ifshort can just short hand this but up to you if you want to change
gbjk
left a comment
gbjk
left a comment
There was a problem hiding this comment.
Good work.
I've left the vals threads open if you want to change them, but happy for you to ignore too, especially now that the func is renamed.
shazbert
left a comment
shazbert
left a comment
There was a problem hiding this comment.
Nothing else from me. Nice work! 🚀🚀 🚀 🚀
thrasher-
left a comment
thrasher-
left a comment
There was a problem hiding this comment.
Thanks for making those changes, double checked sandbox endpoints and all working well! Great work
| }) | ||
| if err != nil { | ||
| log.Fatal("failed to set sandbox endpoint", err) | ||
| log.Fatalf("Coinbase SetDefaultEndpoints sandbox error: %s", err) |
There was a problem hiding this comment.
Erm. Yeah. 💯 you don't have to do this, but just ... yeah. Sorry.
More just for future thinking.
Want to move Error to early in the message. Here we probably allow the exchange context to go first, but that's it.
Using func names is fine, but most of the time just using human readable is better.
| log.Fatalf("Coinbase SetDefaultEndpoints sandbox error: %s", err) | |
| log.Fatalf("Coinbase: Error setting sandbox defaults endpoints: %s", err) |
or if we want to keep the func name (which I don't):
| log.Fatalf("Coinbase SetDefaultEndpoints sandbox error: %s", err) | |
| log.Fatalf("Coinbase: Error in SetDefaultEndpoints for sandbox: %s", err) |
github-project-automation
bot
moved this from In review
to Done
in GoCryptoTrader Kanban
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
xin[]as item is complete.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.
Checklist
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.