Skip to content

feat(okx): add OKX market data + DEX aggregator + Wallet API MCP tools (Phase 1 + 2) - #499

Open
biwasxyz wants to merge 24 commits into
mainfrom
feat/okx-market-tools
Open

feat(okx): add OKX market data + DEX aggregator + Wallet API MCP tools (Phase 1 + 2)#499
biwasxyz wants to merge 24 commits into
mainfrom
feat/okx-market-tools

Conversation

@biwasxyz

@biwasxyz biwasxyz commented May 2, 2026

Copy link
Copy Markdown
Collaborator

Summary

Phase 1 of OKX integration: four read-only market data MCP tools that work with zero setup — no API key required. Lives in its own src/services/okx/ and src/tools/okx/ folder so future phases (DEX aggregator, Wallet API) can layer on cleanly.

This is the first slice of the broader OKX + Coinbase integration plan. Phase 2 will add OKX DEX/Wallet tools with a lazy credential-prompt UX (the user is asked to run `credentials_set okx_api_key ...` only when they invoke a keyed tool).

What's new

Four tools, all hitting public OKX endpoints (verified anonymous on both `www.okx.com` and `us.okx.com`):

Tool Endpoint Notes
`okx_market_ticker` `GET /api/v5/market/ticker` Single instrument snapshot
`okx_market_tickers` `GET /api/v5/market/tickers` All SPOT/SWAP/FUTURES/OPTION tickers
`okx_market_orderbook` `GET /api/v5/market/books` Depth up to 400 per side
`okx_market_candles` `GET /api/v5/market/candles` OHLCV; 22 verified bar values

US-restricted users can switch host via `OKX_BASE_URL=https://us.okx.com\` (defaults to `https://www.okx.com\`).

Why per-file commits?

Eight commits in dependency order so each file change is independently reviewable:

  1. `types.ts` — envelope, error class, instType/bar unions
  2. `client.ts` — `okxGet` fetch wrapper with envelope unwrap + base URL switch
  3. `market.ts` — four service functions
  4. `services/okx/index.ts` — barrel exports
  5. `market.tools.ts` — four MCP tool registrations
  6. `tools/okx/index.ts` — `registerOkxTools` aggregator
  7. `tools/index.ts` — wire into top-level registry
  8. `tests/okx-client.test.ts` — 13 vitest cases

Verification against live OKX (not just docs)

Before shipping I curl'd every endpoint to catch wrong assumptions. Two real surprises overruled by the live API:

Initial assumption Live result Final code
`MARGIN` is a valid `instType` for `/tickers` `51000 Parameter instType error` Removed — narrowed to `OkxTickersInstType`
Ticker shape ends at `ts` Returns `sodUtc0`/`sodUtc8` too Added to `OkxTicker`
`8H` candle bar is valid `51000 Parameter bar error` NOT added
`2D`/`3D` candle bars are invalid code 0, returns rows Kept
UTC variants only documented `6Hutc` … `3Mutc` all return rows Added 6 UTC variants

End-to-end smoke test against live OKX hit all four endpoints successfully (1237 SPOT instruments, real BTC-USDT orderbook, real 1H candles, `1Dutc` UTC-aligned candles).

Test plan

  • `npm run build` — passes
  • `npm test` — 502/502 pass (13 new + 489 existing, no regressions)
  • Live smoke test against `www.okx.com` for all 4 tools
  • Manual MCP install + invoke each `okx_market_*` tool from Claude
  • Verify `OKX_BASE_URL=https://us.okx.com\` round-trip from a US-allowed account

Out of scope (future phases)

  • Phase 2 — OKX DEX + Wallet tools with HMAC signing and lazy credential prompts (DEX quote/swap, BRC-20/Runes/inscriptions read)
  • Phase 3 — Coinbase (separate decision: AgentKit embedded vs. document `payments-mcp` as sibling)
  • Phase 4 — docs + release

🤖 Generated with Claude Code

biwasxyz and others added 8 commits May 2, 2026 20:42
OKX v5 wraps every response in `{ code, msg, data: T[] }`. Adds the
shared envelope shape, an OkxApiError class for non-zero codes, and the
narrow instType/bar unions used by downstream helpers.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
okxGet wraps fetch, unwraps the OKX envelope, and throws OkxApiError on
non-zero codes or non-2xx HTTP. Base URL is configurable via
OKX_BASE_URL so US-restricted users can route through us.okx.com,
defaulting to www.okx.com.

Phase 1 only uses public market endpoints with no auth — HMAC signing
for DEX/Wallet/CEX private endpoints lands in Phase 2.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Wraps the four public /api/v5/market/* endpoints. Verified live against
www.okx.com on 2026-05-02:

- /tickers does NOT accept MARGIN — narrowed type to OkxTickersInstType
- ticker response includes sodUtc0 and sodUtc8 (start-of-day prices)
- /books sz parameter accepts 1-400 (not the legacy 50/200 cap)
- order book levels are [price, size, liquidatedOrders, numOrders]

Rate limit per OKX v5 docs: 20 requests / 2 seconds per IP.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…dles

Four public market data tools — no API key required. Each tool maps
1:1 onto a verified OKX endpoint, with bar/instType enums narrowed to
the values the live API actually accepts:

- okx_market_ticker        — single instrument snapshot
- okx_market_tickers       — all SPOT/SWAP/FUTURES/OPTION instruments
- okx_market_orderbook     — depth up to 400 per side
- okx_market_candles       — 22 verified bar values incl. UTC variants

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Single entry point so future Phase 2 tools (DEX aggregator, Wallet API)
can be added alongside market data without touching the top-level
registry.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
13 vitest cases covering:

- OKX_BASE_URL default, override, trailing-slash strip
- envelope unwrap on success
- no auth headers attached to public requests
- OkxApiError on non-zero code, on non-2xx HTTP, on non-JSON body
- query string omits undefined/empty params
- getTicker returns first element / undefined
- getCandles passes bar/limit through

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

@arc0btc arc0btc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adds four public OKX market data MCP tools (ticker, tickers, orderbook, candles) — zero-setup Phase 1 with a clean path to authenticated Phase 2. The live-API verification against actual OKX responses is exactly the right approach for a new exchange integration.

What works well:

  • Live endpoint verification caught real gotchas (MARGIN exclusion, sodUtc0/sodUtc8 fields, UTC candle variants) before they became prod bugs
  • OkxApiError with typed code and status fields makes error handling precise downstream
  • buildQuery filtering out undefined/null/empty params is correct and well-tested
  • Tests use vi.resetModules() per-test for env var isolation — avoids module-cache pollution
  • The service/tools split mirrors the existing pattern; wiring into registerAllTools is clean

[suggestion] Tickers tool description contradicts its own schema (src/tools/okx/market.tools.ts:68)

The description says "SPOT/MARGIN/SWAP/FUTURES/OPTION" but MARGIN is intentionally excluded from TICKERS_INSTRUMENT_TYPES. A user reading the description will try MARGIN and get a confusing enum validation error before the request even leaves the client.

        "List tickers for all instruments of a given type (SPOT/SWAP/FUTURES/OPTION). " +
        "MARGIN is not supported on this endpoint (OKX returns 51000). " +
        "Public endpoint — no API key required.",

[question] Candles limit max of 300 vs OKX documented max of 100 (src/tools/okx/market.tools.ts:133)

OKX v5 docs show /api/v5/market/candles caps at 100 bars per request. The schema allows up to 300. Did you verify live that 300 works, or does the API silently truncate? If the latter, users passing 200 will get 100 bars with no error — which is confusing. Either clamp the Zod max to 100, or document the truncation behavior explicitly.

[suggestion] OkxCandle = string[] loses positional semantics (src/services/okx/market.ts:36)

The candle description in the tool correctly lists all 9 fields ([ts, open, high, low, close, vol, volCcy, volCcyQuote, confirm]), but the type doesn't enforce this. A tuple makes the structure self-documenting and prevents index-off-by-one bugs in Phase 2 callers:

export type OkxCandle = [
  ts: string,
  open: string,
  high: string,
  low: string,
  close: string,
  vol: string,
  volCcy: string,
  volCcyQuote: string,
  confirm: string,
];

[nit] Candle bar values defined twice (src/tools/okx/market.tools.ts:19 and src/services/okx/types.ts:45)

CANDLE_BARS const in the tools file and OkxCandleBar union in types.ts are the same values in two places. In Phase 2 when new bars are added, one will get updated and the other won't. Consider exporting a const from types.ts and deriving the Zod enum from it, so there's one source of truth.

Code quality notes:

  • getOkxBaseUrl() reads process.env.OKX_BASE_URL on every call. For Phase 1 this is fine; in Phase 2 with HMAC signing (which probably reads more env vars), consider a lazy singleton to avoid repeated env reads.
  • No rate limiting — OKX public endpoints allow 20 req/2s per IP. For an MCP tool invoked interactively this is fine, but worth noting in the Phase 2 design doc if you're adding batch operations.

Operational note: We run the aibtc-mcp-server ourselves and use it through Arc's sensor/dispatch pipeline. The pattern of a thin services/ layer with typed error classes and a tools/ registration layer has been reliable across all our integrations. This PR follows it correctly.

biwasxyz and others added 9 commits May 2, 2026 20:58
Implements the OKX v5 signing algorithm exactly as specified in the
Authentication > Signature section of the docs:

  pre-sign  = timestamp + UPPER(METHOD) + requestPath + body
  signature = base64(HMAC-SHA256(pre-sign, secret))

Where requestPath is the absolute path including the query string for
GET requests, body is "" for GET, and timestamp is ISO 8601 UTC with
millisecond precision (matching the OK-ACCESS-TIMESTAMP header).

Credentials live in the existing aibtc encrypted credential store
under service="okx" with keys api_key, secret, passphrase, project_id.
project_id is required because all Phase 2 endpoints are WaaS
(web3.okx.com) and need OK-ACCESS-PROJECT.

getOkxCredentials throws OkxCredentialsMissingError listing every
unset key, with instructions on which credentials_set calls to make.
This lets each Phase 2 tool fail-fast with a clear, actionable error
the first time a user invokes a private endpoint.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Extends the REST client with okxAuthGet, which wraps a GET request in
the four OK-ACCESS-* headers (KEY, SIGN, PASSPHRASE, TIMESTAMP) plus
OK-ACCESS-PROJECT for WaaS endpoints. The signature is built over the
exact pre-sign string OKX expects: timestamp + method + path-with-query
+ body, where body is "" for GET.

A new getOkxWeb3BaseUrl() returns the WaaS host (web3.okx.com) used
for DEX aggregator and Wallet API endpoints, configurable via
OKX_WEB3_BASE_URL. Service helpers in Phase 2 will pass this base URL
explicitly via OkxRequestOptions.baseUrl, leaving the public market
data path on www.okx.com unchanged.

Response parsing is factored into a shared parseOkxResponse helper so
public and signed requests share identical envelope/error handling.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Wraps five web3.okx.com/api/v5/dex/aggregator/* endpoints behind typed
service helpers, each calling getOkxCredentials() so a missing key
fails fast with OkxCredentialsMissingError before any network call:

- getDexSupportedChains    list chains the aggregator routes on
- getDexAllTokens          list tradeable tokens on a chainId
- getDexQuote              estimated output (no tx returned)
- getDexSwapTx             pre-built swap transaction
- getDexApproveTx          ERC-20 approval calldata

Critical contract: getDexSwapTx does NOT broadcast. The response's
data[0].tx object contains { data, from, to, value, gas, gasPrice,
minReceiveAmount } that the caller must sign and broadcast through
their own wallet. The Phase 2 MCP tool will surface this clearly so
the model never claims a swap "succeeded" after only fetching tx data.

Cross-chain bridge endpoints (/api/v5/dex/cross-chain/*) intentionally
deferred — paths exist (verified live, return 50111 not 404) but exact
param shapes for /quote, /build-tx, /status weren't verifiable without
a live key, so shipping them risks shipping wrong assumptions.

Param shapes are typed against the well-documented OKX dev portal +
github.com/okx/dex-api-library samples; response payloads are typed
loosely (unknown) since they were not curl-verifiable. Tools render
the JSON verbatim via createJsonResponse so users see real fields.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Wraps three verified web3.okx.com/api/v5/wallet/* endpoints:

- getWalletSupportedChains   /wallet/chain/supported-chains
- getWalletTokenBalances     /wallet/asset/all-token-balances-by-address
- getWalletUtxos             /wallet/utxo/utxos (BTC-family chains)

These three were the only Wallet API paths whose existence I could
verify live (return 50111 with dummy auth, not 404). Other paths exist
in the OKX docs (UTXO detail, transaction history, order history) but
their parameter shapes weren't curl-verifiable, so they're deferred.

Deliberately excluded: BRC-20, Runes, and Inscriptions endpoints. These
are NOT served by WaaS Wallet API — they live on OKLink Explorer
(separate API at oklink.com, separate auth/keys). Probing live confirmed
all guesses under /api/v5/wallet/explorer/btc/* return 404. A future
phase can add OKLink integration if BTC-ecosystem indexer queries are
desired; this phase keeps the credential surface to a single OKX key.

chainIndex values are intentionally not hardcoded — service consumers
must call getWalletSupportedChains() first to discover the authoritative
identifiers (these can vary by account/region). The MCP tool layer will
expose this discovery as a separate okx_wallet_supported_chains tool so
the model can chain calls correctly.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds re-exports for the new auth, dex, and wallet modules so consumers
import from a single ../services/okx/index.js entry. Also surfaces
OkxRequestOptions and OkxCredentials so MCP tools can declare typed
helpers without reaching into implementation files.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Five tools mapping 1:1 to the DEX aggregator service helpers:

- okx_dex_supported_chains   discover routable chains
- okx_dex_tokens             list tokens for a chainId
- okx_dex_quote              read-only output estimate
- okx_dex_swap_tx            pre-built swap transaction (no broadcast)
- okx_dex_approve_tx         ERC-20 approval calldata

Each tool's description ends with a single CRED_NOTE constant pointing
the user at the OKX developer portal and the exact credentials_set
calls needed (service='okx' for keys api_key/secret/passphrase/
project_id). The first invocation throws OkxCredentialsMissingError
listing the unset keys — there is no upfront setup required to install
the MCP server, only to invoke a private tool.

The okx_dex_swap_tx description deliberately spells out twice that
the tool returns transaction data the caller must sign and broadcast
themselves. The model has historically conflated "got a tx object"
with "swap succeeded"; framing the tool name as _swap_tx (not _swap)
plus the explicit description guards against that.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three read-only address-level tools mapping to the verified Wallet API
endpoints:

- okx_wallet_supported_chains   discover valid chainIndex values
- okx_wallet_token_balances     fungible token balances per address
- okx_wallet_utxos              UTXOs for BTC-family chains

The supported_chains tool exists primarily so the model can chain calls
correctly — chainIndex values can vary by account/region and shouldn't
be hardcoded by callers. Tool descriptions point at it as the
discovery step before calling balances or UTXOs.

BRC-20, Runes, and Inscriptions are intentionally absent — those data
sources require OKLink Explorer (separate API + key) and are deferred
to a future phase. The Wallet API path probes confirmed those endpoints
return 404 under /api/v5/wallet/, so shipping them here would be wrong.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
registerOkxTools now registers all three tool groups so the top-level
src/tools/index.ts wiring stays unchanged: a single registerOkxTools
call still picks up market + dex + wallet tools.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
12 vitest cases covering Phase 2 auth + private-client paths:

- signOkxRequest matches an openssl-computed HMAC-SHA256 base64 vector
  (timestamp + UPPER(method) + path + body, base64-encoded). The
  vector was generated independently:
    echo -n "2026-05-02T00:00:00.000ZGET/api/v5/dex/aggregator/quote?chainId=1" \
      | openssl dgst -sha256 -hmac "test-secret-abc123" -binary | base64
    -> JrrjK509uBLPOfB8BUNcUE0pSF5Y/1GuUKRucQR1HU8=
  Matching exactly proves the pre-sign format and HMAC encoding are
  canonical, not a misordered or wrongly-encoded variant.
- Method case-insensitivity: GET and get produce the same signature
- Body inclusion: empty vs JSON body produce different signatures
- Timestamp format: ISO 8601 UTC with millisecond precision
- buildOkxAuthHeaders returns all five OK-ACCESS-* headers with valid
  base64 signature
- getOkxCredentials throws OkxCredentialsMissingError listing every
  unset key (full set, partial set), unlocks the store on demand,
  and returns the typed credential object when complete
- okxAuthGet attaches all OK-ACCESS-* headers to outgoing requests,
  signs the requestPath with query string included, and propagates
  OkxApiError for non-zero codes (e.g. 50111 Invalid OK-ACCESS-KEY)

The credential-loader tests use vi.doMock to stub the credentials
module per-test rather than touching the real ~/.aibtc/credentials.enc
store. The query-string signing test recomputes the expected signature
in-test via signOkxRequest with the captured timestamp, proving the
client's pre-sign string matches what auth.ts produces directly.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@biwasxyz biwasxyz changed the title feat(okx): add OKX public market data MCP tools (Phase 1) feat(okx): add OKX market data + DEX aggregator + Wallet API MCP tools (Phase 1 + 2) May 2, 2026
@biwasxyz

biwasxyz commented May 2, 2026

Copy link
Copy Markdown
Collaborator Author

Phase 2 walkthrough — DEX Aggregator + Wallet API

This comment is a focused review guide for the 9 Phase 2 commits (887883d..3dc9bf4). The commits stack cleanly on Phase 1 — read them in order for the design narrative, or skip to the per-tool sections below.

Architecture at a glance

┌──────────────────────────────────────────────────────────────────┐
│  okx_dex_*  /  okx_wallet_*    (8 MCP tools)                     │
│  src/tools/okx/{dex,wallet}.tools.ts                             │
└─────────────────────┬────────────────────────────────────────────┘
                      ↓ calls service helpers, surfaces errors
┌──────────────────────────────────────────────────────────────────┐
│  DEX + Wallet service helpers (typed param/response interfaces)  │
│  src/services/okx/{dex,wallet}.ts                                │
└──────┬─────────────────────────────────────────┬─────────────────┘
       ↓ requires creds                          ↓ signed GET
┌────────────────────────┐               ┌──────────────────────────┐
│  getOkxCredentials()   │               │  okxAuthGet(path,        │
│  src/services/okx/auth │←──── builds ──│    params, creds, opts)  │
│  - lazy unlock         │   OK-ACCESS-* │  src/services/okx/client │
│  - throws Missing err  │     headers   │  - WaaS base URL         │
└────────────────────────┘               └──────────────────────────┘

Three layered concerns, each in its own file: auth (HMAC + cred loading), client (signed transport), services (typed endpoints). The MCP tools layer above them is a thin try/catch + Zod schema shell.

The signing algorithm (auth.ts)

The single highest-risk piece of Phase 2. OKX rejects with cryptic errors if any byte of the pre-sign string is wrong, so the algorithm is documented inline and tested against an independently-computed vector:

pre-sign  = timestamp + UPPER(METHOD) + requestPath + body
signature = base64(HMAC-SHA256(pre-sign, secret))

Where:

  • timestamp — ISO 8601 UTC with millisecond precision (2026-05-02T00:00:00.000Z), same string in OK-ACCESS-TIMESTAMP header AND in pre-sign
  • requestPath — absolute path including the query string for GET (/api/v5/dex/aggregator/quote?chainId=1&...)
  • body — empty string for GET; stringified JSON for POST

The test in okx-auth.test.ts:43 asserts signOkxRequest(...) produces this exact base64 string:

echo -n "2026-05-02T00:00:00.000ZGET/api/v5/dex/aggregator/quote?chainId=1" \
  | openssl dgst -sha256 -hmac "test-secret-abc123" -binary | base64
# JrrjK509uBLPOfB8BUNcUE0pSF5Y/1GuUKRucQR1HU8=

If the algorithm ever drifts — e.g. someone "helpfully" adds a separator or hex-encodes instead of base64 — that test fails immediately.

Lazy credentials — no upfront setup

Critical UX choice: nothing about this PR forces users to provision OKX keys at install time. Public market data (Phase 1) works immediately. Private tools (Phase 2) only check credentials at invocation time, and surface a single error message that tells the user exactly which credentials_set calls to run:

Error: OKX credentials missing: api_key, secret, passphrase, project_id.
Get them at https://web3.okx.com/onchainos/dev-docs/home/developer-portal then run
credentials_set for each: service='okx' key='api_key', service='okx' key='secret',
service='okx' key='passphrase', service='okx' key='project_id'

When some keys are set, the error lists only what's missing. See okx-auth.test.ts:101 for the partial-set assertion.

getOkxCredentials() also unlocks the credential store on demand, so users with ARC_CREDS_PASSWORD set never see an explicit unlock step.

Two base URLs, one client

OKX hosts CEX and WaaS on different domains:

Domain Used by Env override
https://www.okx.com Phase 1 market data, future CEX trading OKX_BASE_URL
https://web3.okx.com Phase 2 DEX aggregator + Wallet API OKX_WEB3_BASE_URL

okxAuthGet(path, params, creds, opts) accepts opts.baseUrl so each service helper passes the right host explicitly. No hidden domain inference. See dex.ts:23 and wallet.ts:25 — both bind WAAS = { baseUrl: getOkxWeb3BaseUrl() } once at module load.

The okx_dex_swap_tx correctness contract

The most dangerous tool in this PR. OKX's /swap endpoint returns pre-built transaction calldata — it does not broadcast. The model has historically conflated "got a tx object" with "swap succeeded," so I designed three guardrails:

  1. Tool name is okx_dex_swap_tx (not okx_dex_swap) — the _tx suffix nudges the model toward "this is data, not an action"
  2. Description repeats it twice:

    "Returns data[0].tx = { data, from, to, value, gas, gasPrice, minReceiveAmount } which the caller MUST sign and broadcast through their own wallet. This tool does NOT broadcast or claim the swap succeeded."

  3. Service-layer JSDoc at dex.ts:90 and the commit message both spell out the contract

The model, when given the tx object, must pass it to a separate signing tool (EVM tools or bitcoin-builder.ts for BTC routes once added).

Discovery-first design

Two tools exist purely to keep callers honest:

  • okx_dex_supported_chains — descriptions for okx_dex_quote / okx_dex_swap_tx / okx_dex_tokens all say "call this first to enumerate"
  • okx_wallet_supported_chainsokx_wallet_token_balances and okx_wallet_utxos both reference it

This is because chainId / chainIndex values can vary by account/region. Hardcoding "1" for Ethereum or "0" for Bitcoin in the MCP tool would silently fail on accounts where those mappings differ.

What's deliberately NOT in this PR

Three categories I scoped out, with reasons documented in the relevant commits:

  1. BRC-20 / Runes / Inscriptions (d890e1d)
    Live-probed every guess under /api/v5/wallet/explorer/btc/* and /api/v5/wallet/utxo/* — all return 404. These data sources live on OKLink Explorer at oklink.com (separate API, separate auth, separate rate limits). Shipping placeholder tools that fail at runtime would be worse than waiting.

  2. Cross-chain bridge (a9b8403)
    Paths under /api/v5/dex/cross-chain/* exist (verified live: 50111, not 404), but the parameter shapes for /quote, /build-tx, /status couldn't be verified without a real key (OKX validates auth before params). Defer until verifiable.

  3. CEX private trading (okx_spot_order etc.)
    Out of scope for Phase 2. Users who want full CEX trading should install the official @okx_ai/okx-trade-mcp (145 tools, MIT license) as a sibling MCP. Documenting this is a doc task for Phase 4.

Test coverage

okx-auth.test.ts adds 12 vitest cases (514/514 pass overall). Highlights:

Test Why it matters
HMAC matches openssl vector Prevents algorithm drift
Method case-insensitivity OKX uppercases internally; we should too
Body inclusion changes signature Catches "forgot to include body" bugs
Missing-cred error lists every unset key Users see one error, not four sequential ones
okxAuthGet query string in pre-sign Catches "signed path without query" — a top OKX integration bug
Recompute signature in-test Proves client and auth.ts agree on pre-sign format

Credential tests use vi.doMock per-test to stub services/credentials.js — the real ~/.aibtc/credentials.enc is never touched.

Suggested review order

  1. auth.ts (887883d) — read this first; everything else depends on the signing algorithm being right
  2. okx-auth.test.ts (3dc9bf4) — confirm the HMAC vector and pre-sign format are what you'd expect
  3. client.ts diff (327fc86) — verify okxAuthGet builds headers and signs the path-with-query
  4. dex.ts + wallet.ts (a9b8403, d890e1d) — service surface; check the deferred-scope decisions
  5. dex.tools.ts + wallet.tools.ts (d06409b, db9eb3a) — MCP layer; pay attention to okx_dex_swap_tx description language
  6. Barrel + aggregator (8f61032, f2f761b) — wiring only

Follow-ups for future PRs

  • OKLink integration for BRC-20 / Runes / Inscriptions (separate credential service, separate base URL)
  • Cross-chain bridge tools once a real key is available to verify param names
  • Phase 3: Coinbase decision (AgentKit embed vs. document payments-mcp as sibling)
  • CLAUDE.md + README updates for the new tool catalog (Phase 4 — docs + release)

@arc0btc arc0btc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up: Phase 2 additions (auth, DEX aggregator, Wallet API)

My prior review covered Phase 1 market data tools — this covers the auth, DEX, and wallet layers added in the updated commit.

No overlap with JingSwap/Bitflow. JingSwap and Bitflow are Stacks-native DEXes; OKX DEX aggregator targets EVM chains (Ethereum, Base, etc.). Different networks, no collision.

[suggestion] WAAS constant captures env var at module load time (src/services/okx/dex.ts:34, src/services/okx/wallet.ts:22)

const WAAS = { baseUrl: getOkxWeb3BaseUrl() };  // reads env at import time

If OKX_WEB3_BASE_URL is set after the module is first imported (e.g. in tests or lazy env injection), the override won't take effect. The pattern in client.ts correctly reads the env at call time via opts.baseUrl ?? getOkxBaseUrl(). Align dex.ts and wallet.ts to the same pattern — pass the opts object inline rather than caching it:

return okxAuthGet<OkxDexChain>(
  "/api/v5/dex/aggregator/supported/chain",
  undefined,
  creds,
  { baseUrl: getOkxWeb3BaseUrl() }
);

[suggestion] DEX service functions return unknown[] (src/services/okx/dex.ts:71-95)

getDexQuote, getDexSwapTx, and getDexApproveTx return unknown[]. The quote and swap responses have documented shapes (toTokenAmount, router path, gas; and the tx object). Even a minimal interface improves downstream type safety and makes Phase 3 callers self-documenting without requiring them to cast. The WAAS docs provide stable shapes for both endpoints.

[suggestion] Double-cast params as unknown as Record<string, string> (src/services/okx/dex.ts:76, 85, 94)

The OkxDexQuoteParams / OkxDexSwapParams types contain string | undefined values, which TypeScript won't narrow to Record<string, string>. The cast is safe because buildQuery already filters undefined values, but the double-cast through unknown is a code smell. Simplest fix: change okxAuthGet's params type from Record<string, string | number | undefined> | undefined to accept the params directly, or add typed overloads.

Auth tests: strong. The HMAC vector verified against openssl is exactly the right approach — it proves the canonical OKX pre-sign format (timestamp + UPPER(method) + requestPath + body) and that the signature includes the query string for GET requests. The module-isolation pattern with vi.resetModules() + dynamic import() is correct for testing singleton modules.

[nit] project_id always required, even for future CEX private endpoints (src/services/okx/auth.ts:68)

getOkxCredentials() validates all four keys including project_id. The comment correctly notes project_id is only used for WaaS endpoints. If Phase 3 adds CEX private endpoints (trading, account data), they don't need project_id — but callers will still be forced to set it. Consider a requireProjectId flag or a separate getCexCredentials() that only requires three keys. Low priority for now since all current callers are WaaS.

Credential error message UX is good. OkxCredentialsMissingError lists exactly which keys are missing and where to get them. The lazy-credential pattern (fail on first invocation, not on server startup) is the right UX for MCP tools.

Operational note: we run aibtc-mcp-server through Arc's dispatch pipeline. The HMAC signing path is production-critical — the test coverage here is solid enough to be confident about correctness before enabling authenticated endpoints.

biwasxyz and others added 7 commits May 4, 2026 20:47
The okx_market_tickers description listed SPOT/MARGIN/SWAP/FUTURES/OPTION
but the Zod enum (TICKERS_INSTRUMENT_TYPES) excludes MARGIN because the
endpoint rejects it with 51000 (verified live). A user reading the
description would try MARGIN and hit a Zod validation error before the
request even leaves the client, with no hint why.

Now the description matches the enum and explains WHY MARGIN is absent.

Reported in PR review by @arc0btc.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
OKX returns each candle row as a fixed array
[ts, open, high, low, close, vol, volCcy, volCcyQuote, confirm]
(verified live 2026-05-02). The previous string[] type allowed any
length, so downstream callers indexing into row[5] for "vol" had no
compile-time safety against off-by-one bugs.

The named-tuple syntax also acts as documentation in IDE tooltips —
hovering over a candle element shows the field name, not just "string".

Reported in PR review by @arc0btc.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CANDLE_BARS in market.tools.ts and OkxCandleBar in types.ts had the
same 22 values in two places. Adding a bar (e.g. when OKX adds 8H
support) required two edits, with no compile-time link guaranteeing
they stayed aligned.

Now there is one OKX_CANDLE_BARS const in types.ts. The OkxCandleBar
type derives from it via (typeof OKX_CANDLE_BARS)[number], and the
Zod enum in the tools file imports the same const. Adding a bar is a
single-line change with the type and schema staying in lockstep.

The * re-export of types.ts already covered OKX_CANDLE_BARS, but I
added an explicit named re-export in the barrel as well so it's
discoverable at the public API surface.

Reported in PR review by @arc0btc.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The WAAS = { baseUrl: getOkxWeb3BaseUrl() } module-level const captured
the env var once at first import, so any test or runtime path that
mutated OKX_WEB3_BASE_URL after the module was imported would silently
get the original value. This matches a real bug pattern: tests using
vi.stubEnv after the dex.ts/wallet.ts module is already in the cache
would expect the new base URL but hit the cached one.

Now each service helper inlines `{ baseUrl: getOkxWeb3BaseUrl() }` at
the call site, so the env is read once per request — same pattern
client.ts already used for the public market endpoints. The override
behavior is now consistent across the entire OKX service surface.

Reported in PR review by @arc0btc.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two related cleanups in one commit because they couldn't be made
independently — the cast removal depended on relaxing the param
interfaces to conform to okxAuthGet's query-params type.

Typed responses (P2.2):
  - OkxDexQuoteResponse        chainId, dexRouterList, fromToken,
                                toToken, fromTokenAmount, toTokenAmount,
                                estimateGasFee
  - OkxDexSwapResponse         extends Quote with the `tx` object the
                                caller must sign and broadcast
  - OkxDexApproveResponse      ERC-20 approval calldata
  - OkxDexTokenInfo            shared sub-shape for from/toToken

Each interface includes `[k: string]: unknown` so undocumented OKX
fields pass through verbatim without breaking compilation. Shapes are
sourced from the WAAS DEX docs at web3.okx.com/build/docs/waas/dex-swap;
they were not curl-verified end-to-end (would require a live key).

Cast cleanup (P2.3):
  - OkxDexQuoteParams / OkxDexApproveParams gain
    `[k: string]: string | undefined`
  - OkxDexSwapParams inherits from OkxDexQuoteParams so it picks up
    the same index signature
  - The three `params as unknown as Record<string, string>` casts in
    getDexQuote / getDexSwapTx / getDexApproveTx are removed —
    params now flow into okxAuthGet directly

The double-cast was also semantically wrong: it claimed all values were
`string`, but slippage / referrerAddress are `string | undefined`. The
new index signature reflects the actual shape, so callers (and the
type checker) see the truth.

Reported in PR review by @arc0btc.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
OK-ACCESS-PROJECT is only required for WaaS endpoints (DEX + Wallet).
Future CEX-private endpoints (account, trading) don't use that header
and don't need a project id.

The previous implementation forced every signed-OKX caller to
configure project_id, even those that don't need it. Now:

  getOkxCredentials()                  // requireProjectId=true (default)
  getOkxCredentials(false)             // CEX-private callers can omit project_id

OkxCredentials.projectId is now optional. buildOkxAuthHeaders()
conditionally includes OK-ACCESS-PROJECT only when projectId is set,
so a CEX-private signed request will have exactly four OK-ACCESS-*
headers instead of five — matching what the v5 CEX docs require.

All current callers (dex.ts + wallet.ts) use the default
requireProjectId=true and are unchanged. The new behavior unblocks
adding CEX private tools (e.g. okx_account_balance, okx_trade_order)
in a future phase without forcing public-market-only users to also
provision a WaaS project id.

Tests added:
  - getOkxCredentials(false) when project_id is unset → returns
    projectId: undefined, no error
  - getOkxCredentials(false) when project_id IS set → still returned
  - buildOkxAuthHeaders includes OK-ACCESS-PROJECT only when set
  - 4 OK-ACCESS-* headers present when projectId is omitted

Reported in PR review by @arc0btc.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reviewer asked whether the 300 max in our Zod schema matched OKX docs
(some doc pages list 100 as the cap). Re-verified live (2026-05-02):

  limit=100 -> 100 rows  code=0
  limit=200 -> 200 rows  code=0
  limit=300 -> 300 rows  code=0
  limit=400 -> 300 rows  code=0    <- silent server-side cap
  limit=500 -> 300 rows  code=0    <- silent server-side cap

So 1-300 is exact (no truncation), and our Zod max(300) prevents users
from hitting the silent cap because we reject 301+ at the schema layer
before the request leaves the client. Description now spells this out
so future reviewers don't have to re-ask the same question — the older
"100 bars" cap some docs pages still cite is outdated for /market/candles
on the v5 API as of the verification date.

Reported in PR review by @arc0btc.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@biwasxyz

biwasxyz commented May 4, 2026

Copy link
Copy Markdown
Collaborator Author

@arc0btc — addressed all suggestions in 7 atomic fixup commits. Each commit references the suggestion it resolves so the review trail stays linked to the fix.

# Suggestion Resolution Commit
P1.1 tickers description says MARGIN Description rewritten to match enum + explains why MARGIN is excluded (51000) b236261
P1.2 candles limit max 300 vs docs 100 Re-verified live — 1-300 returns exact count, 400/500 silently cap to 300. Defending 300; description now cites the live verification dates so future reviewers don't have to re-ask a5df897
P1.3 OkxCandle = string[] should be tuple Now [ts, open, high, low, close, vol, volCcy, volCcyQuote, confirm] named tuple b586838
P1.4 CANDLE_BARS + OkxCandleBar duplicated Moved to OKX_CANDLE_BARS const in types.ts; type derives via (typeof OKX_CANDLE_BARS)[number]; tools file imports the const eb3b846
P2.1 WAAS captures env at module load Inlined { baseUrl: getOkxWeb3BaseUrl() } at every call site in dex.ts and wallet.ts 338a10f
P2.2 DEX returns unknown[] Typed OkxDexQuoteResponse / OkxDexSwapResponse / OkxDexApproveResponse from WAAS docs (with [k: string]: unknown for forward-compat) f9646b6
P2.3 Double-cast params as unknown as Record<string, string> Added [k: string]: string | undefined index signature to param interfaces — params now flow into okxAuthGet directly, no cast f9646b6
P2.4 project_id always required New requireProjectId = true parameter on getOkxCredentials. OkxCredentials.projectId is now optional, buildOkxAuthHeaders conditionally includes OK-ACCESS-PROJECT. WaaS callers unchanged; future CEX-private callers pass false 3be4b9c

Verification

  • npm run build — passes
  • npm test — 518/518 pass (4 new tests for the requireProjectId flag and conditional OK-ACCESS-PROJECT header)
  • Live re-curl confirmed candles 100/200/300 = exact rows, 400/500 = 300 (silent cap)

One minor "wrong cast" caught along the way

While fixing P2.3 I noticed the original double-cast was also semantically wrong: as unknown as Record<string, string> claimed all values were string, but slippage, referrerAddress, etc. are string | undefined. The new index signature [k: string]: string | undefined reflects the actual shape, so callers (and the type checker) now see the truth. Mentioned this in the commit message of f9646b6 for posterity.

Ready for re-review when you have a moment.

@arc0btc

arc0btc commented May 5, 2026

Copy link
Copy Markdown
Contributor

[blocking] Missing POST Support Documentation

The code only implements okxGet() and okxAuthGet() — all current endpoints (market data, DEX quote/swap, wallet queries) use GET. However, the design should explicitly clarify:

  • Line 158–162 (client.ts): Comment says "Phase 1 only uses public market endpoints" but Phase 2 is shipping DEX/Wallet together, so the phase boundary is slightly misleading.
  • For future CEX private endpoints that require POST (e.g., orders, account endpoints), you'll need okxPost() and okxAuthPost() variants.

Suggestion: Add a brief comment in client.ts noting "Future phases may add POST support for trading endpoints; see PHASE_ROADMAP or the agent-news issue tracking OKX integration."


[suggestion] Rate Limiting Documentation

Line 535 (market.ts) correctly documents the OKX rate limit: "20 requests / 2 seconds per IP". However, the client has no rate-limiting logic. This is likely fine for agent use (agents won't hammer the API), but:

  • If this gets used by high-volume sensors, consider adding a simple rate-limit queue in the client layer.
  • For now, document this assumption in src/services/okx/client.ts top comment.

[suggestion] Cross-Chain Deferred: Add Tracking Issue Ref

Line 289–291 (dex.ts) and Line 694–695 (wallet.ts) both note that cross-chain bridge endpoints are deferred "until verified." This is sensible — param shapes vary by chain and need live key testing.

Add a brief reference (e.g., comment linking to the agent-news issue or internal issue tracker) so future maintainers know where to pick this up. Example:

// Cross-chain bridge endpoints deferred until verified with live key
// (agent-news#XXXX tracks this scope expansion)

[nit] Credential Lazy-Load Error Messages

Line 48–51 (auth.ts) — The OkxCredentialsMissingError has excellent UX, telling users exactly which credentials_set commands to run. Nice!

One small detail: the error message references https://web3.okx.com/onchainos/dev-docs/home/developer-portal. If OKX ever reshuffles their docs, this URL will rot. Consider:

  • Pin this to a specific git commit / docs version if possible
  • Or add a comment pointing to where this URL is documented (e.g., a DEVELOPER.md or similar)

[nit] Test Coverage Gap: Error Path for Non-JSON Responses

lines 1756–1769 (okx-client.test.ts) test non-JSON responses in okxGet(). However, there's no corresponding test for okxAuthGet() with a non-JSON response. Both paths call parseOkxResponse(), so coverage is actually complete, but add a test case explicitly calling okxAuthGet() with a 500 HTML response — it's a small addition that makes the test intent clearer.


[nit] HMAC Vector Pinning

Line 1298 (okx-auth.test.ts) — The openssl vector is excellent validation. Consider pinning the test vector to the OKX v5 API spec version (e.g., "verified against OKX v5 Docs, last checked 2026-05-02"). If OKX ever changes their signing algorithm (unlikely but has happened before with other exchanges), this makes it discoverable.


Operational Notes (Arc Integration)

From the Arc perspective (I run this server 24/7):

Excellent:

  • Credentials stored in the encrypted credential store (good for security)
  • Lazy loading means no upfront credential requirement (good UX)
  • Clear error messages guide users to the next step
  • No external dependencies beyond what's already in the server

⚠️ To Watch:

  • Market data endpoints are public, but DEX/Wallet require credentials + project_id (WaaS). Make sure sensors that use these get reminded to arc creds set first.
  • The comment on line 26 notes "WaaS endpoints (web3.okx.com — DEX + Wallet)". Ensure any sensor or skill using DEX/Wallet knows to route through web3.okx.com, not www.okx.com.

Summary

This is high-quality work. The documentation is thorough, test coverage is comprehensive (including openssl-verified HMAC vectors), and the architecture is clean. The three points above are polish/clarification, not blockers. Once those are addressed, this is ready.

One final thing: Do you have a sensor planned for this? (e.g., OKX market data feed, or just tools for agents to call?) That context would help shape whether the lazy-load credential pattern is the right fit long-term.

@arc0btc

arc0btc commented May 13, 2026

Copy link
Copy Markdown
Contributor

Status check — no new activity since 2026-05-04

Reviewing the thread: my 2026-05-05 review used a [blocking] label on the POST documentation item but then contradicted that in the summary ("polish/clarification, not blockers"). That was a labeling error on my part — none of those items were genuine blockers.

Current state of the PR:

  • Phase 1 (market data): ✅ approved (2026-05-02), all suggestions addressed
  • Phase 2 (auth, DEX, wallet): all suggestions from Phase 2 review addressed in the 2026-05-04 fixup commits
  • Remaining items from 2026-05-05 review: documentation comments and test nits — none require changes before merge

This is ready for merge from my side. The three items from the 2026-05-05 review can be handled as follow-ups or incorporated at the author's discretion.

@arc0btc arc0btc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up: verified all six items from my two prior reviews (Phase 1 market-data pass, Phase 2 auth/DEX/wallet pass) were addressed in the commits since:

  • b236261d — tickers description now matches the TICKERS_INSTRUMENT_TYPES enum and explains why MARGIN is excluded
  • b586838dOkxCandle is now a 9-element named tuple, not string[]
  • eb3b846fOKX_CANDLE_BARS is a single source of truth, derived into both the type union and the Zod enum
  • 338a10fedex.ts/wallet.ts now read OKX_WEB3_BASE_URL at call time instead of caching at module load, matching client.ts's pattern
  • f9646b69 — DEX responses are now typed (OkxDexQuoteResponse/OkxDexSwapResponse/OkxDexApproveResponse) and the double-cast through unknown is gone
  • 3be4b9c8project_id is optional via requireProjectId, unblocking future CEX-private tools without forcing WaaS-only users to provision it
  • a5df897d — candles limit=300 re-verified live against the actual server-side cap (300, not the 100 some doc pages cite), with the silent-truncation behavior above 300 documented

Re-checked the signing/wallet surface specifically since this task asks about wallet exposure: okx_dex_swap_tx/okx_dex_approve_tx return unsigned tx data only (caller signs and broadcasts separately, documented explicitly in the tool descriptions) — no private key material touches this code. signOkxRequest/buildOkxAuthHeaders in auth.ts build request signatures from credentials pulled from the existing encrypted store; secrets aren't included in OkxApiError messages, which only carry OKX's own code/msg/status. CI (build, test, CodeQL, Snyk) all green.

No new findings — approving this round's changes.

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.

2 participants