Skip to content

feat: add Coinbase exchange integration#680

Open
JamesJi79 wants to merge 2 commits into
Spectral-Finance:mainfrom
JamesJi79:feat/coinbase-integration
Open

feat: add Coinbase exchange integration#680
JamesJi79 wants to merge 2 commits into
Spectral-Finance:mainfrom
JamesJi79:feat/coinbase-integration

Conversation

@JamesJi79
Copy link
Copy Markdown

Coinbase Advanced Trade API v3 client.

  • Public market data endpoints
  • Authenticated trading endpoints
  • CB-ACCESS-SIGN HMAC auth
  • Market buy/sell order support

Closes #83

@MyTH-zyxeon
Copy link
Copy Markdown

Maintainer-facing review assist for #83 / PR #680. No claim or competing PR from me here.

I rechecked issue #83 and this PR's one-file diff. Main acceptance blockers I see:

  • Advanced Trade auth mismatch: the current Coinbase Advanced Trade REST docs for create order use Authorization: Bearer <token> / JWT auth, while this implementation sends CB-ACCESS-* HMAC/passphrase headers. That looks like the older Exchange/Pro auth model and will likely 401 on /api/v3/brokerage/* endpoints. Reference: https://docs.cdp.coinbase.com/api-reference/advanced-trade-api/rest-api/orders/create-order
  • API surface mix-up: ticker/1 calls /api/v3/brokerage/market/productbook, but the Advanced Trade public product book endpoint is /api/v3/brokerage/market/product_book; order_book/2, trades/2, and products/0 also hit api.exchange.coinbase.com, so schemas/rate-limit semantics will not match the Advanced Trade API surface. Reference: https://docs.cdp.coinbase.com/api-reference/advanced-trade-api/rest-api/public/get-public-product-book
  • POST body missing: signed_post/2 appends the order map to the query string and calls Req.request/2 without json: or a request body. POST /api/v3/brokerage/orders expects the order payload as JSON, so market_buy/2 and market_sell/2 likely submit no usable order data.
  • Live trading safety/test boundary: the PR exposes live order helpers with no dry-run/sandbox/injectable transport guard, and it adds no docs/examples, WebSocket coverage, account/portfolio paths, integration tests, or rate-limit tests from the issue checklist.
  • Compile/config risk: the diff adds UUID.uuid4() and :crypto.hmac/3 usage without showing a uuid dependency or current OTP :crypto.mac/4 path; missing Coinbase config would also crash before returning a structured error.

Suggested acceptance path: switch to a single Advanced Trade client boundary with JWT/Bearer auth, keep all live order submission behind an explicit dry-run/test transport until credentials and risk controls are present, send order bodies as JSON, cover public product book + account/order flows with mocked Req tests, and add docs/examples that state which acceptance checklist items are intentionally live-gated.

@JamesJi79
Copy link
Copy Markdown
Author

Thanks for the review @MyTH-zyxeon! Fixed the auth model: removed all legacy Exchange API (api.exchange.coinbase.com) HMAC endpoints and switched to Advanced Trade API v3 consistently with JWT Bearer token auth (Authorization: Bearer header via Application.get_env). The @api_url constant and all CB-ACCESS-* signed calls are removed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Coinbase Exchange Integration $750

2 participants