Skip to content

feat(integrations): Polymarket trading + market data (#1398, venue 1/3)#2145

Merged
graycyrus merged 37 commits into
tinyhumansai:mainfrom
oxoxDev:feat/1398-polymarket
May 19, 2026
Merged

feat(integrations): Polymarket trading + market data (#1398, venue 1/3)#2145
graycyrus merged 37 commits into
tinyhumansai:mainfrom
oxoxDev:feat/1398-polymarket

Conversation

@oxoxDev
Copy link
Copy Markdown
Contributor

@oxoxDev oxoxDev commented May 18, 2026

Summary

  • Adds the Polymarket integration (read + trading) — the first of three venues called out in Add Polymarket, Kalshi, and Hyperliquid trading integrations #1398 (Kalshi + Hyperliquid follow as separate PRs on the same #1398 umbrella).
  • Read surface: market / event / orderbook / price discovery via Gamma + CLOB public APIs, plus authenticated reads (positions, balance, open orders, USDC.e allowance) once an EOA is configured.
  • Trading surface: place_order + cancel_order against the Polymarket CLOB with EIP-712 typed-data signing on Polygon (chain_id 137).
  • Approval gate: every write action requires arguments.approved == true. Stopgap until Keep triggers read-only until users approve writes #1339's formal approval gate lands — TODO(#1339) recorded inline.
  • Capability catalog split: skills.polymarket_readonly (browse) + skills.polymarket_trading (Beta, Action, destination = "Polymarket CLOB API").

Problem

Per #1398, Polymarket, Kalshi, and Hyperliquid each need explicit tool surfaces rather than being folded into vague "exchange support". Today the repo has no exchange / prediction-market domain in the core, no venue-specific auth or signing primitives, and no tool surface for balances, positions, market discovery, or order flow. Without that, the agent can hold a wallet but cannot reason over a real trading venue.

Solution

New tool module src/openhuman/tools/impl/network/polymarket.rs — single PolymarketTool exposing 11 actions tagged by action enum:

Surface Actions
Public reads list_markets, get_market, list_events, get_orderbook, get_price
Authenticated reads get_positions, get_balance, get_open_orders, get_usdc_allowance
Writes place_order, cancel_order

Signing primitives:

  • src/openhuman/tools/impl/network/clob_auth.rs — L1 handshake (POST /auth/api-key w/ fallback /auth/derive-api-key) + L2 HMAC-SHA256 header signer over timestamp + method + path + body. Timestamp in unix seconds.
  • src/openhuman/tools/impl/network/polymarket_orders.rs — EIP-712 typed-data Order struct + domain_separator (chain_id = 137, Polymarket CLOB Exchange contract) + sign_order returning hex signature.

Polygon RPC wiring kept narrow: polygon_rpc_url lives in PolymarketConfig (no new WalletChain::Polygon variant) so the blast radius stays inside the tool module. USDC.e allowance reads go through eth_call over that RPC.

Config (src/openhuman/config/schema/tools.rs) — additive fields:

Approval gate: require_write_approval(approved: Option<bool>) runs before any write action issues HTTP. If approved != true, returns Polymarket write requires explicit user approval. Re-invoke with arguments.approved = true after confirming with the user. — no HTTP touched. Approval-gate tests verify the no-HTTP property.

Retry / timeout: 3 × 500ms backoff on 429 / 5xx, no retry on 4xx, explicit deadline error on user-configured timeout_secs (default 15s). Same shape as siblings to be added by the Kalshi + Hyperliquid PRs.

Submission Checklist

  • Tests added or updated (happy path + at least one failure / edge case) per Testing Strategy
  • Diff coverage ≥ 80% — 22 polymarket lib tests cover: 11-action schema deserialization; happy paths for list_markets, get_market (id + slug), list_events, get_orderbook, get_price, get_positions, get_balance (USDC.e default), get_open_orders, get_usdc_allowance (Polygon eth_call), place_order (signed POST); approval gate (place_order and cancel_order no-HTTP on missing approval); HMAC signing fixture (clob_auth); EIP-712 fixture (polymarket_orders, chain_id 137); side normalisation; 4xx / 5xx / timeout. Per feedback_cargo_test_lib_full_run.md the full-lib run OOMs locally; CI gates the full matrix.
  • N/A: coverage matrix — docs/TEST-COVERAGE-MATRIX.md does not have a Polymarket / trading-venue row yet; the matrix entry will land in the venue-3 PR alongside Hyperliquid + Kalshi rows so the row covers the whole #1398 umbrella in one edit.
  • All affected feature IDs from the matrix are listed in the PR description under ## Related
  • No new external network dependencies introduced — polymarket_tests.rs uses an in-process TcpListener mock; Gamma + CLOB endpoints are configurable so smoke can target a local mock per Testing Strategy.
  • N/A: release manual smoke — Polymarket trading is feature-flagged via integrations.polymarket.enabled and not part of the release-cut surface yet.
  • N/A: Closes #1398Add Polymarket, Kalshi, and Hyperliquid trading integrations #1398 covers three venues; this PR ships venue 1/3. Closing reference will land on the venue-3 PR. See ## Related.

Impact

  • Runtime: desktop, opt-in via integrations.polymarket.enabled config flag. Off-by-default. No behavioural change for users who don't touch the flag.
  • Network: Gamma (gamma-api.polymarket.com), CLOB (clob.polymarket.com), and a Polygon RPC (default polygon-rpc.com). All configurable.
  • Security: writes are gated by the approved=true stopgap and an EIP-712 typed-data signature over the order on chain_id 137. L1 handshake derives a per-EOA L2 API key + secret which lives in config (will move to SecretStore after Security: decrypt_optional_secret / encrypt_optional_secret never called — config secrets stored in plaintext #1900). No raw wallet privkey leaves the wallet module.
  • Performance: cold-path retry adds 3 × 500ms on 5xx / 429; hot path one HTTP per action. Nonce fetched per submission via CLOB /nonce?user= — extra round-trip on each place_order (documented; caching deferred).
  • Compatibility: additive — new RPC actions on the existing tools_polymarket_execute controller. No public RPC surface change beyond the action enum widening.

Related


AI Authored PR Metadata (required for Codex/Linear PRs)

Linear Issue

Commit & Branch

  • Branch: feat/1398-polymarket
  • Commit SHA: top = 23176452 (test/docs), 12 commits ahead of base + merge of upstream/main

Validation Run

  • cargo check
  • cargo test --lib polymarket — 22 passed
  • Manual smoke: local TcpListener mock on :8901 with [integrations.polymarket] gamma_base_url + clob_base_url overridden in user config — list_markets + get_market action returned mock fixtures via openhuman.tools_polymarket_execute.
  • pnpm typecheck — N/A (no frontend changes in this PR)

Validation Blocked

  • command: cargo test --lib (full lib run)
  • error: SIGKILL'd locally per feedback_cargo_test_lib_full_run.md
  • impact: CI gates the full matrix; scope-filtered run passes 22/22

Behavior Changes

  • Intended: add 11-action Polymarket tool gated behind integrations.polymarket.enabled; trading writes gated by approved=true
  • User-visible: agent can browse Polymarket markets / events / orderbooks / prices, read EOA-owned positions / balance / open orders, and (with explicit approval) place / cancel limit orders

Parity Contract

  • Legacy behavior preserved: integrations disabled by default; existing read-only landing from earlier on this branch unchanged for callers that only use the public-data surface
  • Guard/fallback/dispatch parity checks: tools_polymarket_execute already routes by action field; new actions auto-reachable without schema regen. Approval gate is a function-local check, not a separate middleware — migration to ApprovalGate (Keep triggers read-only until users approve writes #1339) will replace require_write_approval call sites.

Summary by CodeRabbit

  • New Features

    • Polymarket integration (Beta): market discovery, orderbook/price viewing, authenticated balance/position reads, USDC allowance checks, and order placement/cancellation (writes require explicit user approval). Config toggle (defaults off), credential derivation/persistence, and retry/timeout semantics.
  • Documentation

    • New integration guide and table-of-contents entry covering setup, auth/signing, headers/nonce rules, permissions, and error/retry behavior.
  • Tests

    • Extensive fixtures and end-to-end/unit tests covering discovery, CLOB reads, signed requests, write flows, allowance checks, and error handling.

Review Change Stack

oxoxDev added 13 commits May 18, 2026 18:22
…inyhumansai#1398)

Add PolymarketConfig (enabled, gamma_base_url, clob_base_url,
timeout_secs) under the integrations schema, re-export it from the
config module, and add the field on IntegrationsConfig. Defaults to
enabled with the public Gamma + CLOB base URLs; consumers override
via config.toml.

Refs tinyhumansai#1398 (part 1/3 — Polymarket read-only)
…humansai#1398)

Implements a Tool trait impl that browses Polymarket via the public
Gamma and CLOB read endpoints. Five actions: list_markets, get_market
(by id or slug), list_events, get_orderbook, get_price. No wallet
signing; no order placement.

Networking uses the shared proxy-aware reqwest builder, 3x500ms
retry-with-backoff on 429/5xx, no retry on 4xx, explicit deadline
errors on timeout. Error bodies are routed through the existing
utf8-safe truncator to keep multibyte payload previews safe.

Refs tinyhumansai#1398 (part 1/3 — Polymarket read-only)
…inyhumansai#1398)

Wire PolymarketTool into all_tools_with_runtime() so the agent harness
can dispatch through it when an agent's allowlist includes it. Gated
by integrations.polymarket.enabled.

Refs tinyhumansai#1398 (part 1/3 — Polymarket read-only)
…humansai#1398)

Expose the Polymarket tool over JSON-RPC so the Tauri shell and
admin/diagnostic flows can dispatch read-only Polymarket actions
without going through the agent harness. Mirrors the existing
tools_composio_execute / tools_seltz_search / tools_web_search
shape: namespace='tools', function='polymarket_execute', inputs
{action, arguments}, outputs {data}.

Refs tinyhumansai#1398 (part 1/3 — Polymarket read-only)
…humansai#1398)

Add a Skills-category Capability entry pointing at the polymarket
tool, with a CapabilityPrivacy block declaring that metadata leaves
the device to Polymarket's Gamma + CLOB endpoints. Status=Beta
because writes are deferred to a follow-up release.

Refs tinyhumansai#1398 (part 1/3 — Polymarket read-only)
…umansai#1398)

10 unit tests using static JSON fixtures + an embedded tokio
TcpListener mock server (no wiremock dep). Covers all 5 happy paths,
4xx no-retry, 5xx transient retry, timeout deadline, and parameter
schema deserialization for every action.

Refs tinyhumansai#1398 (part 1/3 — Polymarket read-only)
…humansai#1398)

Short architecture page covering v1 scope (read-only Gamma + CLOB),
config fields, error/retry behavior, test strategy, and deferred
phases (writes + Polygon wallet signing). Linked from gitbooks
SUMMARY index.

Refs tinyhumansai#1398 (part 1/3 — Polymarket read-only)
…tinyhumansai#1398)

Extends PolymarketTool with six trading actions:
get_positions, get_balance, get_open_orders, get_usdc_allowance,
place_order, cancel_order. Reads inject L2 HMAC auth headers via
sign_clob_headers from clob_auth; writes additionally route through
the EIP-712 signer in polymarket_orders (chain_id=137). USDC.e
allowance read goes through the configured Polygon RPC.

place_order and cancel_order route through require_write_approval,
which returns the approval-required error and does NOT issue HTTP when
arguments.approved != true. Stopgap until tinyhumansai#1339's formal approval
gate lands — TODO(tinyhumansai#1339) recorded inline.

Refs tinyhumansai#1398 (Polymarket trading)
…inyhumansai#1398)

Adds skills.polymarket_trading (Beta, data_kind=Action,
leaves_device=true, destinations=Polymarket CLOB API) alongside the
existing skills.polymarket_readonly so the about_app surface advertises
the new trading capability separately from the public-data browse
capability that landed earlier.

Refs tinyhumansai#1398 (Polymarket trading)
…nyhumansai#1398)

Refreshes the agent-registry debug line and the
tools_polymarket_execute RPC controller schema so they advertise the
full action surface (now read + trading) instead of the read-only
description the earlier read-only landing shipped.

Functional dispatch already routes via the action field; this commit
is documentation-only at the wire level.

Refs tinyhumansai#1398 (Polymarket trading)
…nyhumansai#1398)

Adds ~15 new test cases for the trading surface: L2 header signing,
HMAC fixture, EIP-712 order signing fixture, query-param signature
canonicalization, nonce parsing across number+string payloads, and
approval-gate tests verifying that place_order / cancel_order return
the approval-required error and issue zero HTTP when approved != true.

Rewrites gitbooks/developing/integrations/polymarket.md to cover the
trading scope: action reference, L1 handshake + L2 HMAC headers,
EIP-712 order signing on chain_id=137, USDC.e allowance contract,
permissions, and configuration fields.

Refs tinyhumansai#1398 (Polymarket trading)
@oxoxDev oxoxDev requested a review from a team May 18, 2026 19:39
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 18, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds a Polymarket integration: docs, configuration schema, CLOB credential derivation and signing, EIP-712 order signing, a PolymarketTool implementing read/write actions (Gamma/CLOB/Polygon flows), RPC/controller wiring, capability catalog entries, and comprehensive tests and fixtures.

Changes

Polymarket Integration v1

Layer / File(s) Summary
Documentation and capability declaration
gitbooks/SUMMARY.md, gitbooks/developing/integrations/polymarket.md, src/openhuman/about_app/catalog.rs
Docs entry and two new capability catalog entries for Polymarket read-only and trading.
Configuration schema and defaults
src/openhuman/config/..., src/openhuman/config/schema/tools.rs
Adds PolymarketClobCredentials, PolymarketConfig, defaults (endpoints, RPC, contracts, timeout), and wires integrations.polymarket into IntegrationsConfig.
Network module wiring
src/openhuman/tools/impl/network/mod.rs
Adds clob_auth, polymarket, polymarket_orders modules and re-exports PolymarketTool.
CLOB authentication infrastructure
src/openhuman/tools/impl/network/clob_auth.rs
Implements credential derivation (POST /auth/api-key with derive fallback), L1 EIP-712 header signing, secret decoding, HMAC-SHA256 CLOB request signing, response parsing, and tests.
EIP-712 order signing
src/openhuman/tools/impl/network/polymarket_orders.rs
Defines Order model, EIP-712 domain builder, sign_order, side normalization, and unit tests.
PolymarketTool implementation
src/openhuman/tools/impl/network/polymarket.rs
Implements PolymarketTool (constructor, request enum), dispatch for Gamma/CLOB/Polygon reads/writes, credential caching/persistence, signer resolution, nonce parsing, signed order build/submit/cancel, HTTP helpers with bounded retries/backoff, Tool trait wiring, and helpers.
RPC/controller registration and wallet access
src/openhuman/tools/ops.rs, src/openhuman/tools/schemas.rs, src/openhuman/wallet/mod.rs
Conditionally registers the tool, adds tools_polymarket_execute controller/schema and handler, updates test counts, and exposes ops::secret_material crate-visible.
Comprehensive test suite and fixtures
src/openhuman/tools/impl/network/polymarket_tests.rs, tests/fixtures/polymarket/*
Adds Tokio mock server, extensive end-to-end action tests (discovery, orderbook/price, authenticated reads, allowance), write approval/security gating tests, retry/timeout semantics tests, utility unit tests, and JSON fixtures.

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant PolymarketTool
  participant GammaAPI
  participant CLOBAPI
  participant PolygonRPC
  participant LocalWallet

  Client->>PolymarketTool: request(action, arguments, approved?, user?)
  PolymarketTool->>GammaAPI: GET /markets or /events (discovery)
  PolymarketTool->>CLOBAPI: GET signed endpoints (positions/balance/open_orders) with signed headers
  PolymarketTool->>PolygonRPC: POST eth_call (allowance) -> returns hex U256
  Client->>PolymarketTool: place_order (approved=true)
  PolymarketTool->>LocalWallet: derive Polymarket signer & sign EIP-712 order
  PolymarketTool->>CLOBAPI: POST /order with signed order and signed headers
  CLOBAPI-->>PolymarketTool: order response / nonce
  PolymarketTool-->>Client: ToolResult (data or error)
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

"A rabbit in a patch, with ledger and quill,
I hop through docs and code with a thrill.
Signed orders nibble, tests jump like leaves,
Credentials tucked safe beneath tiny sleeves.
Hooray—Polymarket blooms; the rabbit believes!"

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 23.14% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed Title 'feat(integrations): Polymarket trading + market data (#1398, venue 1/3)' clearly and concisely summarizes the main change—adding Polymarket trading and market data integration functionality.
Linked Issues check ✅ Passed All core coding requirements from #1398 are met: venue-specific tool architecture defined, 11 actions exposed (reads/authenticated reads/writes), EIP-712 signing implemented, auth/retry semantics with ~22 tests, and diff includes comprehensive test coverage.
Out of Scope Changes check ✅ Passed All changes directly support Polymarket integration: config schemas, CLOB auth, order signing, tool implementation, tests, and fixtures are in-scope per #1398; no unrelated wallet/swap/generic exchange abstractions or autonomous trading loops were introduced.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot added the feature Net-new user-facing capability or product behavior. label May 18, 2026
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 4

🧹 Nitpick comments (5)
src/openhuman/config/schema/tools.rs (1)

609-609: Credentials stored in plain config — migration tracked.

The derived_clob_credentials field stores API credentials directly in config.toml (line 609). This is acknowledged as a temporary solution in the PR objectives: "Writes stored credentials in config until SecretStore migration (#1900)."

While this is not ideal for security (credentials in plain config), it's documented as a known gap with a follow-up issue. Consider prioritizing the SecretStore migration to avoid users storing sensitive CLOB credentials in version-controlled or world-readable config files.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/openhuman/config/schema/tools.rs` at line 609, The field
derived_clob_credentials currently persists PolymarketClobCredentials in plain
config.toml; change this so credentials are no longer stored directly in the
config by replacing the persisted Option<PolymarketClobCredentials> usage with a
secret reference pattern backed by the SecretStore (follow issue `#1900`): remove
or deprecate direct serialization of derived_clob_credentials in the tools.rs
struct, add a SecretStore key/reference field (or Option<SecretRef>) in its
place, update code paths that read/write derived_clob_credentials to fetch from
SecretStore at runtime, and provide a migration helper that transfers any
existing PolymarketClobCredentials from config into SecretStore and writes back
only the secret reference.
src/openhuman/tools/impl/network/polymarket.rs (2)

784-1053: ⚖️ Poor tradeoff

Consider extracting shared retry/error logic.

The get_json, post_json_raw, and delete_json methods share nearly identical retry and error-handling logic (~90 lines each). Extracting a generic request executor with method/body parameters would reduce duplication and make error handling easier to maintain.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/openhuman/tools/impl/network/polymarket.rs` around lines 784 - 1053, The
three methods get_json, post_json_raw, and delete_json duplicate nearly all
retry/response/error handling; extract a single generic executor (e.g.,
execute_request) that accepts HTTP method, url, optional query/body, and
headers, encapsulates the retry loop, timeout/transport error handling, response
status branching, body reading, summarize_error_body usage and final JSON
deserialization, and return serde_json::Value (or Result<Value>); then refactor
get_json, post_json_raw, and delete_json to build the request specifics (method,
query/body) and call execute_request to eliminate the repeated ~90-line blocks
and centralize error messages and retry/backoff logic.

440-461: 💤 Low value

Minor inefficiency: wallet resolved but unused in CancelOrder.

resolve_signer_and_user is called at line 447 but the wallet is discarded. If cached_or_derive_credentials misses the cache, it will call resolve_signer_and_user again internally. Consider passing the already-resolved wallet to avoid redundant mnemonic decryption.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/openhuman/tools/impl/network/polymarket.rs` around lines 440 - 461,
PolymarketRequest::CancelOrder currently calls resolve_signer_and_user and drops
the returned wallet, causing a potential redundant call when
cached_or_derive_credentials misses the cache; change the flow to reuse the
resolved wallet by either calling a variant like
cached_or_derive_credentials_with_wallet(&user, wallet) or adding an overload to
cached_or_derive_credentials that accepts the resolved wallet, then pass that
wallet into cached_or_derive_credentials before calling delete_signed_clob_json
so resolve_signer_and_user is not run twice (refer to functions
resolve_signer_and_user, cached_or_derive_credentials and
delete_signed_clob_json to locate the code to update).
src/openhuman/tools/impl/network/clob_auth.rs (1)

60-120: 💤 Low value

Consider caching timestamp to avoid potential clock drift between auth attempts.

In derive_credentials, now_unix_secs() is called separately for each auth endpoint (lines 68 and 89). If the first request takes time and spans a second boundary, the timestamps will differ. While unlikely to cause issues in practice, using a single timestamp for both requests would be more consistent.

💡 Suggested improvement
 pub(crate) async fn derive_credentials(
     http: &Client,
     signer: &LocalWallet,
     base_url: &str,
     chain_id: u64,
     address: &str,
 ) -> Result<ClobCredentials> {
+    let timestamp = now_unix_secs()?;
+
     let create_path = "/auth/api-key";
-    let create_headers = sign_l1_headers(signer, chain_id, address, 0, now_unix_secs()?).await?;
+    let create_headers = sign_l1_headers(signer, chain_id, address, 0, timestamp).await?;
     // ... first request ...

     let derive_path = "/auth/derive-api-key";
-    let derive_headers = sign_l1_headers(signer, chain_id, address, 0, now_unix_secs()?).await?;
+    let derive_headers = sign_l1_headers(signer, chain_id, address, 0, timestamp).await?;
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/openhuman/tools/impl/network/clob_auth.rs` around lines 60 - 120,
derive_credentials calls now_unix_secs() twice which can yield different
timestamps across the two auth attempts; cache a single timestamp before the two
sign_l1_headers calls (e.g. let ts = now_unix_secs()? ) and pass that ts into
both sign_l1_headers invocations (the ones that produce create_headers and
derive_headers) so both requests use the same timestamp while preserving
existing error handling.
src/openhuman/tools/schemas.rs (1)

503-544: ⚡ Quick win

Add structured RPC debug logs for start/success/failure paths.

This handler currently has no tracing at entry/exit/error branches, unlike neighboring handlers. Please add grep-friendly structured logs (action, enabled flag, outcome).

As per coding guidelines, "Use log / tracing at debug or trace level on RPC entry and exit, error paths, state transitions, and any branch that is hard to infer from tests alone."

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/openhuman/tools/schemas.rs` around lines 503 - 544, In
handle_polymarket_execute, add grep-friendly structured debug/tracing calls: log
at entry (e.g., tracing::debug!) showing the extracted action and whether
config.integrations.polymarket.enabled; log a debug/trace just before calling
tool.execute with the final args (include action and args); on error map_err
branch log an error/tracing::error! with the action, enabled flag and the
formatted error string before returning; on success log a debug with action and
a short outcome/data summary before constructing the RpcOutcome. Reference
symbols: handle_polymarket_execute, action,
config.integrations.polymarket.enabled, args, tool.execute, and RpcOutcome.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@gitbooks/SUMMARY.md`:
- Line 46: The SUMMARY.md entry currently labels the Polymarket integration as
"v1 Read-Only"; update that link text to reflect trading support (e.g., change
"* [Polymarket Integration (v1
Read-Only)](developing/integrations/polymarket.md)" to "* [Polymarket
Integration (v1 Read + Trading)](developing/integrations/polymarket.md)" or
remove the "(Read-Only)" parenthetical) so the TOC matches polymarket.md which
documents read and trading actions like place_order and cancel_order.

In `@src/openhuman/config/schema/tools.rs`:
- Around line 588-626: PolymarketConfig's enabled flag is incorrectly defaulted
true via defaults::default_true() in both the #[serde(default =
"defaults::default_true")] attribute and the Default impl; change both to use a
false default (e.g., defaults::default_false() or a new
default_polymarket_disabled function) so integrations.polymarket.enabled is off
by default, and update any Polymarket integration documentation to state the
feature is disabled by default.

In `@src/openhuman/tools/schemas.rs`:
- Around line 528-533: The current code silently replaces a non-object
`arguments` with `{ "action": ... }`; instead validate that `arguments` is a
JSON object and return a validation error if it is not. In the block around the
`arguments`/`args` handling (variables `arguments`, `args`, and `action` in
src/openhuman/tools/schemas.rs) check `if let Value::Object(ref mut map) =
arguments` and only then insert `"action"` into the map; otherwise return an Err
(or a validation Result variant used in this module) describing that `arguments`
must be an object, so caller-supplied non-object data is not silently discarded.
Ensure you use the function's existing error type/return path so callers get a
proper validation failure.
- Around line 503-526: The handler currently constructs a fresh default policy
(std::sync::Arc::new(crate::openhuman::security::SecurityPolicy::default()))
which bypasses the runtime/session policy; instead obtain the caller/session
security context and pass that into PolymarketTool::new. Replace the default
construction in handle_polymarket_execute with the session-provided
Arc<SecurityPolicy> (for example by extracting it from the incoming
request/session context or params where your framework stores it, or by calling
the existing accessor that returns the current session policy) and pass that
variable into PolymarketTool::new so the tool uses the same runtime security
policy as the rest of the registry.

---

Nitpick comments:
In `@src/openhuman/config/schema/tools.rs`:
- Line 609: The field derived_clob_credentials currently persists
PolymarketClobCredentials in plain config.toml; change this so credentials are
no longer stored directly in the config by replacing the persisted
Option<PolymarketClobCredentials> usage with a secret reference pattern backed
by the SecretStore (follow issue `#1900`): remove or deprecate direct
serialization of derived_clob_credentials in the tools.rs struct, add a
SecretStore key/reference field (or Option<SecretRef>) in its place, update code
paths that read/write derived_clob_credentials to fetch from SecretStore at
runtime, and provide a migration helper that transfers any existing
PolymarketClobCredentials from config into SecretStore and writes back only the
secret reference.

In `@src/openhuman/tools/impl/network/clob_auth.rs`:
- Around line 60-120: derive_credentials calls now_unix_secs() twice which can
yield different timestamps across the two auth attempts; cache a single
timestamp before the two sign_l1_headers calls (e.g. let ts = now_unix_secs()? )
and pass that ts into both sign_l1_headers invocations (the ones that produce
create_headers and derive_headers) so both requests use the same timestamp while
preserving existing error handling.

In `@src/openhuman/tools/impl/network/polymarket.rs`:
- Around line 784-1053: The three methods get_json, post_json_raw, and
delete_json duplicate nearly all retry/response/error handling; extract a single
generic executor (e.g., execute_request) that accepts HTTP method, url, optional
query/body, and headers, encapsulates the retry loop, timeout/transport error
handling, response status branching, body reading, summarize_error_body usage
and final JSON deserialization, and return serde_json::Value (or Result<Value>);
then refactor get_json, post_json_raw, and delete_json to build the request
specifics (method, query/body) and call execute_request to eliminate the
repeated ~90-line blocks and centralize error messages and retry/backoff logic.
- Around line 440-461: PolymarketRequest::CancelOrder currently calls
resolve_signer_and_user and drops the returned wallet, causing a potential
redundant call when cached_or_derive_credentials misses the cache; change the
flow to reuse the resolved wallet by either calling a variant like
cached_or_derive_credentials_with_wallet(&user, wallet) or adding an overload to
cached_or_derive_credentials that accepts the resolved wallet, then pass that
wallet into cached_or_derive_credentials before calling delete_signed_clob_json
so resolve_signer_and_user is not run twice (refer to functions
resolve_signer_and_user, cached_or_derive_credentials and
delete_signed_clob_json to locate the code to update).

In `@src/openhuman/tools/schemas.rs`:
- Around line 503-544: In handle_polymarket_execute, add grep-friendly
structured debug/tracing calls: log at entry (e.g., tracing::debug!) showing the
extracted action and whether config.integrations.polymarket.enabled; log a
debug/trace just before calling tool.execute with the final args (include action
and args); on error map_err branch log an error/tracing::error! with the action,
enabled flag and the formatted error string before returning; on success log a
debug with action and a short outcome/data summary before constructing the
RpcOutcome. Reference symbols: handle_polymarket_execute, action,
config.integrations.polymarket.enabled, args, tool.execute, and RpcOutcome.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 14d430ef-ee10-421b-abaa-4c5f965008e0

📥 Commits

Reviewing files that changed from the base of the PR and between 0b053c5 and 2c9e63c.

📒 Files selected for processing (22)
  • gitbooks/SUMMARY.md
  • gitbooks/developing/integrations/polymarket.md
  • src/openhuman/about_app/catalog.rs
  • src/openhuman/config/mod.rs
  • src/openhuman/config/schema/mod.rs
  • src/openhuman/config/schema/tools.rs
  • src/openhuman/tools/impl/network/clob_auth.rs
  • src/openhuman/tools/impl/network/mod.rs
  • src/openhuman/tools/impl/network/polymarket.rs
  • src/openhuman/tools/impl/network/polymarket_orders.rs
  • src/openhuman/tools/impl/network/polymarket_tests.rs
  • src/openhuman/tools/ops.rs
  • src/openhuman/tools/schemas.rs
  • src/openhuman/wallet/mod.rs
  • tests/fixtures/polymarket/error_client.json
  • tests/fixtures/polymarket/error_server.json
  • tests/fixtures/polymarket/events_list.json
  • tests/fixtures/polymarket/market_by_id.json
  • tests/fixtures/polymarket/market_by_slug.json
  • tests/fixtures/polymarket/markets_list.json
  • tests/fixtures/polymarket/orderbook.json
  • tests/fixtures/polymarket/price.json

Comment thread gitbooks/SUMMARY.md Outdated
Comment thread src/openhuman/config/schema/tools.rs
Comment thread src/openhuman/tools/schemas.rs
Comment thread src/openhuman/tools/schemas.rs Outdated
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/openhuman/config/schema/tools.rs`:
- Around line 585-590: PolymarketClobCredentials currently derives Debug and
will print secret-bearing fields; replace the auto-derived Debug with a manual
implementation that redacts sensitive fields (i.e., implement std::fmt::Debug
for PolymarketClobCredentials and display api_key normally but show redacted
placeholders for secret and passphrase), or simply remove Debug from the derive
list if Debug output isn’t needed; keep Clone, Serialize, Deserialize,
JsonSchema, PartialEq, Eq intact and ensure the custom Debug implementation
references the struct PolymarketClobCredentials and its fields api_key, secret,
and passphrase so secrets are never logged.

In `@src/openhuman/tools/impl/network/clob_auth.rs`:
- Around line 60-121: The derive_credentials function lacks structured
debug/trace logs around the auth request/fallback branches; add tracing calls
(using tracing::debug or tracing::trace) at function entry (log base_url,
chain_id, address masked), before/after the POST to create_path and GET to
derive_path (log path, HTTP status codes), on JSON parse failures (include
error), when creds are obtained (log whether creds.is_complete and masked keys),
and on the final error branch (log derive_status and response body) so each
state transition and error path in derive_credentials, create_path, derive_path,
ClobAuthResponse->into_credentials is observable for production triage.
- Around line 25-30: ClobCredentials currently derives Debug which can leak
secrets; remove Debug from the derive list on the ClobCredentials struct (leave
Serialize, Deserialize, Default, PartialEq, Eq as needed) so the type no longer
implements Debug or can be accidentally logged; after removing Debug, verify
there are no other places calling format!("{:?}", ...) or implicitly requiring
Debug for ClobCredentials and either redact those logs or change them to avoid
printing the struct.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 8939bd55-74f6-4acd-8178-eba1f4c208ad

📥 Commits

Reviewing files that changed from the base of the PR and between 2c9e63c and 2317645.

📒 Files selected for processing (6)
  • gitbooks/SUMMARY.md
  • gitbooks/developing/integrations/polymarket.md
  • src/openhuman/config/schema/tools.rs
  • src/openhuman/tools/impl/network/clob_auth.rs
  • src/openhuman/tools/impl/network/polymarket.rs
  • src/openhuman/tools/schemas.rs
✅ Files skipped from review due to trivial changes (1)
  • gitbooks/SUMMARY.md
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/openhuman/tools/schemas.rs
  • src/openhuman/tools/impl/network/polymarket.rs

Comment thread src/openhuman/config/schema/tools.rs Outdated
Comment thread src/openhuman/tools/impl/network/clob_auth.rs Outdated
Comment thread src/openhuman/tools/impl/network/clob_auth.rs
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
src/openhuman/tools/impl/network/clob_auth.rs (1)

302-306: 💤 Low value

Consider using URL_SAFE encoder directly.

The manual replacement of +- and /_ after standard encoding works correctly, but base64::engine::general_purpose::URL_SAFE produces the same output more directly. Given the "Match official client behavior" comment, the current approach is acceptable if the official client does the same.

♻️ Cleaner alternative (optional)
-    let signature = general_purpose::STANDARD
-        .encode(signature_bytes)
-        .replace('+', "-")
-        .replace('/', "_");
+    let signature = general_purpose::URL_SAFE.encode(signature_bytes);
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/openhuman/tools/impl/network/clob_auth.rs` around lines 302 - 306,
Replace the manual post-processing of the base64 output with the URL-safe
engine: instead of using general_purpose::STANDARD.encode(signature_bytes)
followed by .replace('+', "-").replace('/', "_"), call
general_purpose::URL_SAFE.encode(signature_bytes) to produce the same URL-safe
base64 result; update the code that assigns to signature in clob_auth.rs (the
variable named signature and the input signature_bytes) to use this encoder to
match the official client behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@src/openhuman/tools/impl/network/clob_auth.rs`:
- Around line 302-306: Replace the manual post-processing of the base64 output
with the URL-safe engine: instead of using
general_purpose::STANDARD.encode(signature_bytes) followed by .replace('+',
"-").replace('/', "_"), call general_purpose::URL_SAFE.encode(signature_bytes)
to produce the same URL-safe base64 result; update the code that assigns to
signature in clob_auth.rs (the variable named signature and the input
signature_bytes) to use this encoder to match the official client behavior.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: d3fc54cc-9037-4924-868e-d9fc554facab

📥 Commits

Reviewing files that changed from the base of the PR and between 2317645 and e447b51.

📒 Files selected for processing (2)
  • src/openhuman/config/schema/tools.rs
  • src/openhuman/tools/impl/network/clob_auth.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/openhuman/config/schema/tools.rs

coderabbitai[bot]
coderabbitai Bot previously approved these changes May 19, 2026
@oxoxDev
Copy link
Copy Markdown
Contributor Author

oxoxDev commented May 19, 2026

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 19, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

coderabbitai[bot]
coderabbitai Bot previously approved these changes May 19, 2026
Copy link
Copy Markdown
Contributor

@graycyrus graycyrus left a comment

Choose a reason for hiding this comment

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

Walkthrough

Solid integration adding Polymarket read + trading capabilities (venue 1/3 for #1398). Well-structured module decomposition (polymarket.rs, clob_auth.rs, polymarket_orders.rs), comprehensive test suite (22+ tests with mock server), proper credential redaction in Debug for secret/passphrase, and effective approval gate for write operations. Two security patterns from sibling code (composio.rs) are missing: the enforce_tool_operation security gate and HTTPS URL enforcement for credential-bearing requests.

Change Summary

File Change type Description
polymarket.rs New (1395 lines) Main tool: 11 actions, HTTP helpers with retry, credential caching, order signing
clob_auth.rs New (419 lines) L1 EIP-712 handshake + L2 HMAC-SHA256 signing
polymarket_orders.rs New (158 lines) EIP-712 order typed-data signing
polymarket_tests.rs New (971 lines) 22+ tests: mock server, signing fixtures, approval gates, error paths
config/schema/tools.rs Modified PolymarketConfig + PolymarketClobCredentials structs
tools/schemas.rs Modified RPC controller schema + handler
tools/ops.rs Modified Conditional tool registration
about_app/catalog.rs Modified 2 capability entries (readonly + trading)
wallet/mod.rs Modified secret_material exported as pub(crate)
config/mod.rs, config/schema/mod.rs Modified Re-exports for new types
gitbooks/ New + Modified Integration docs + SUMMARY link
tests/fixtures/polymarket/*.json New (9 files) Test fixtures

Per-file analysis

polymarket.rs — Clean action dispatch with tagged enum deserialization. HTTP helpers follow the retry/error pattern well (4xx no retry, 429/5xx 3× with 500ms backoff). The approval gate properly blocks write actions before any network call. Two security concerns noted inline.

clob_auth.rs — Good separation of L1 and L2 auth concerns. Structured tracing with grep-friendly [polymarket][clob_auth] prefixes. Address masking in logs is a nice touch.

polymarket_orders.rs — Straightforward EIP-712 signing. Domain separator correctly pins chain 137 and exchange contract.

polymarket_tests.rs — Thorough coverage including happy paths, auth header verification, approval gate enforcement (zero-HTTP property), error paths, and signing fixtures.

config/schema/tools.rs — Follows sibling config patterns. Enabled defaults to false.

tools/schemas.rs — Controller handler has good entry/exit logging and validates argument types.

Additional notes

Missing action-level debug logging: handle_request() in polymarket.rs has no entry/exit logging for individual actions. Sibling clob_auth.rs has excellent [polymarket][clob_auth] prefixed logging, but the main dispatch path is silent. Consider adding tracing::debug!(action = ..., "[polymarket] handle_request: entry") and similar for each action's network call, consistent with the watchlist requirement for grep-friendly debug logging in new/changed flows.

Comment thread src/openhuman/tools/impl/network/polymarket.rs
Comment thread src/openhuman/tools/impl/network/polymarket.rs
Comment thread src/openhuman/config/schema/tools.rs
Comment thread src/openhuman/tools/impl/network/clob_auth.rs Outdated
Comment thread src/openhuman/tools/impl/network/polymarket.rs Outdated
tinyhumansai#1398)

Adds ensure_https guard mirroring the composio.rs pattern so the L2 HMAC
auth headers (api_key, passphrase, signature) cannot leak in plaintext if
a user misconfigures clob_base_url to http://. Applied before sending
auth headers in get_signed_clob_json_with_creds, post_signed_clob_json,
delete_signed_clob_json, and the derive_credentials L1 handshake path.

Loopback http (127.0.0.1, ::1, localhost) is explicitly allowed so the
TcpListener-based mock harness and any local proxy can exercise the
signed-request code path without TLS termination. Real Polymarket CLOB
hosts never resolve to loopback so the carve-out cannot weaken
production security.

Three unit tests cover https-pass, loopback-pass, and remote-http-reject.

Addresses graycyrus comment 3264466079.

Refs tinyhumansai#1398 (Polymarket trading)
oxoxDev added 5 commits May 19, 2026 13:49
…inyhumansai#1398)

Defense-in-depth: sign_l1_headers now verifies signer.address() matches
the supplied address before signing the EIP-712 ClobAuth payload.
Callers already validate the pairing in resolve_signer_and_user, but
co-locating the check inside sign_l1_headers prevents future call paths
or refactors from accidentally signing with a mismatched EOA.

Two tests cover the matching and mismatched paths against a fixed
fixture mnemonic.

Addresses CodeRabbit comment 3264647263.

Refs tinyhumansai#1398 (Polymarket trading)
…tinyhumansai#1398)

The raw deserialization target still carried Debug-printable secret and
passphrase in plaintext, so any debug formatting (a stray dbg!(), a
log expansion of a parse error context) re-opened the leak the
ClobCredentials and PolymarketClobCredentials Debug-redactions had
already closed. Drop the derive(Debug) and emit a redacted custom impl
matching the sibling types.

Addresses the CodeRabbit walkthrough duplicate finding on
clob_auth.rs:341-349.

Refs tinyhumansai#1398 (Polymarket trading)
…inyhumansai#1398)

Post-cm7 fmt drift on the test mnemonic phrase layout. No behavioural
change.

Refs tinyhumansai#1398 (Polymarket trading)
…tinyhumansai#1398)

A single socket.read can return partial headers/body on TCP, which made
order-body assertions intermittently truncate the captured request.
Switch the mock-server task to read in 4 KiB chunks and accumulate until
either headers + Content-Length bytes are received (request_is_complete
helper) or EOF.

No behavioural change for already-passing tests; eliminates a class of
flake introduced by larger order payloads.

Addresses CodeRabbit comment 3264647270.

Refs tinyhumansai#1398 (Polymarket trading)
… removing it (tinyhumansai#1398)

Drop was calling remove_var unconditionally, which clobbered any
pre-existing OPENHUMAN_WORKSPACE the developer might have set before
running tests locally. Snapshot the prior value with var_os at
construction time and restore it (or remove if it was None) when the
guard drops.

Addresses CodeRabbit comment 3264647274.

Refs tinyhumansai#1398 (Polymarket trading)
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 19, 2026

Tip

For best results, initiate chat on the files or code changes.

@ozpool Thanks for the follow-up. The fix in f3ca190 looks correct — dropping derive(Debug) and replacing it with a custom redacting impl that prints all three fields as <redacted> brings ClobAuthResponse in line with the ClobCredentials / PolymarketClobCredentials treatment already in place. Consistent redaction across all three credential-bearing types is the right call.

@oxoxDev
Copy link
Copy Markdown
Contributor Author

oxoxDev commented May 19, 2026

@coderabbitai — addressing the duplicate finding on clob_auth.rs:341-349 (ClobAuthResponse Debug leak): Addressed in f3ca190. Dropped the derive(Debug) and replaced with a custom redacting impl that matches the ClobCredentials / PolymarketClobCredentials treatment from earlier commits — all three fields print as <redacted>.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 19, 2026

Tip

For best results, initiate chat on the files or code changes.

@oxoxDev Thanks for the confirmation. The fix in f3ca190 looks correct — dropping derive(Debug) and replacing it with a custom redacting impl that prints all three fields as <redacted> brings ClobAuthResponse in line with the ClobCredentials / PolymarketClobCredentials treatment already in place. Consistent redaction across all credential-bearing types is the right call.

Copy link
Copy Markdown
Contributor

@M3gA-Mind M3gA-Mind left a comment

Choose a reason for hiding this comment

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

Code Review — feat(integrations): Polymarket trading + market data

Overview

Adds a new PolymarketTool exposing 11 actions (5 public reads, 4 authenticated reads, 2 writes) across Gamma + CLOB APIs, with EIP-712 order signing on Polygon. Gated behind integrations.polymarket.enabled (default off). Well-scoped first venue of three.


Security

Credentials stored in plaintext config

  • derived_clob_credentials (CLOB API key + HMAC secret) is written to the TOML config file in plaintext. This is acknowledged as temporary pending #1900. Since this is an on-by-default-off feature, the exposure window is controlled, but it should be called out prominently: any tool that reads the config file (including other agents) can exfiltrate the CLOB key. The TODO comment should include the threat model, not just the issue reference.

ensure_https does not guard the Polygon RPC URL

  • get_usdc_allowance_for_user calls post_json_raw with self.polygon_rpc_url but there is no corresponding ensure_https(&self.polygon_rpc_url)? call before it. The RPC payload doesn't carry CLOB credentials, but it does carry wallet addresses. Recommend adding the guard for consistency.

Concurrent credential derivation race

  • ensure_clob_credentials locks the Mutex, checks for cached creds, drops the lock, then derives and re-locks. Two concurrent callers can both see an empty cache and both issue POST /auth/api-key, generating two separate credentials — one gets silently dropped. Consider using tokio::sync::OnceCell or holding the lock across derivation.

Test code uses unsafe { std::env::set_var }

  • This is UB in multi-threaded tests. Consider serial_test or equivalent if test parallelism is an issue.

Stopgap approval gate

  • require_write_approval is a client-enforced string check. If the agent framework calls execute() directly without the normal approval channel, this gate is all that stands between an LLM hallucinating approved: true and a live order. Worth a security note in the inline comment beyond just the TODO reference.

Code Quality

File size far exceeds the project guideline

  • polymarket.rs is 1,419 lines — nearly 3× the ~500-line target from CLAUDE.md. The HTTP layer (get_json / post_json_raw / delete_json) alone is ~270 lines of near-identical retry logic that should be extracted to a shared send_with_retry helper.

Retry logic is copy-pasted three times

  • get_json, post_json_raw, and delete_json each implement the same ~60-line retry loop differing only in HTTP verb and error message prefix. A single generic helper would cut ~120 lines and make the retry policy consistent.

Dual credential structs with conversion boilerplate

  • ClobCredentials (in clob_auth.rs) and PolymarketClobCredentials (in config) are identical in fields and semantics, connected only by From impls. Unify them — the current split adds 30 lines of glue for no functional benefit.

price / size use f64 for financial amounts

  • Floating-point representation errors can still produce wrong fixed-point outputs for certain inputs (e.g. 0.3 * 1e6 ≠ 300000.0 exactly). Consider accepting price/size as strings and parsing to rust_decimal::Decimal, matching what the CLOB API actually expects.

Mixed rounding modes without explanation

  • round_normal (half-up) for price, round_down (floor) for size. The asymmetry is correct (floor prevents over-selling) but needs a comment — a reader will assume it's a bug.

Raw ABI encoding for eth_call

let data = format!(
    "0xdd62ed3e{:0>64}{:0>64}",
    hex::encode(owner.as_bytes()),
    hex::encode(spender.as_bytes()),
);

This works but is brittle hand-rolled ABI encoding. A comment explaining why hex::encode(addr.as_bytes()) produces exactly 40 chars (producing correctly zero-padded 32-byte ABI words) is warranted, or use alloy-sol-types/ethabi encoding.

salt uses non-CSPRNG rand::random

  • rand::random::<u64>() is not cryptographically random. For an order salt, predictability may be exploitable by a front-runner. Use rand::rngs::OsRng or getrandom directly.

Architecture

is_concurrency_safe returns true for write actions — nonce collision risk

  • Two concurrent place_order calls each fetch /nonce?user= independently and receive the same nonce, causing one to fail at the CLOB. Either return false for write variants (or all variants given the Mutex interior state), or document the nonce collision risk explicitly.

Timestamp reuse on auth fallback

  • derive_credentials computes now_unix_secs() once, then reuses the same timestamp for both the POST /auth/api-key attempt and the fallback GET /auth/derive-api-key. If the server enforces a short anti-replay window, a slow first request can cause the fallback to fail with a stale timestamp. Re-derive now_unix_secs() before the fallback call.

persist_clob_credentials loads + saves whole config on every derivation

  • Acknowledged as temporary, but concurrent config writes from multiple tools could clobber each other. The in-memory Mutex cache should be documented as the source of truth; the persist is best-effort only (the warn log is already there — just add the doc comment).

Test Coverage

Positives:

  • 22 tests covering all 11 actions, HMAC fixture, EIP-712 fixture, approval gate (no-HTTP property), retry/5xx, timeout
  • In-process TcpListener mock server with request capture is clean
  • EnvVarGuard RAII for environment teardown is the right pattern

Missing cases:

  • get_market with neither market_id nor slug → error path untested
  • Credential derivation fallback (POST → GET) not exercised — needs two-response mock sequence
  • persist_clob_credentials failure path — warn log is present but path is untested
  • Concurrent place_order calls → nonce collision scenario not covered

Summary

Severity Issue
Must fix is_concurrency_safetrue for write actions risks nonce collision
Must fix rand::random non-CSPRNG salt for order signing
Must fix ensure_https missing on Polygon RPC URL
Should fix Timestamp reuse between POST and fallback GET in derive_credentials
Should fix Deduplicate retry loop across get_json/post_json_raw/delete_json
Should fix get_market missing-both-params case is untested
Consider Credential derivation race (concurrent callers)
Nit CSPRNG for salt, f64Decimal for price/size, file size split

The overall structure is solid, security thinking is correct for a Beta feature, and the test harness is well-designed. The critical blockers are the concurrency safety flag for writes and the non-CSPRNG salt.

coderabbitai[bot]
coderabbitai Bot previously approved these changes May 19, 2026
Copy link
Copy Markdown
Contributor

@graycyrus graycyrus left a comment

Choose a reason for hiding this comment

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

Walkthrough

Continuation review of the Polymarket integration (venue 1/3 for #1398). All five issues from my previous REQUEST_CHANGES review have been verified as addressed — SecurityPolicy enforcement on writes, HTTPS enforcement before credentials, Debug redaction on both credential types, and the Client::new() fallback replaced with a hard panic. Good work on the follow-up commits.

One new issue found on this pass: the Polygon RPC call for get_usdc_allowance sends a JSON-RPC payload without Content-Type: application/json, which will break with strict RPC providers.

Previous Requested Changes — All Addressed

# Finding Status
1 SecurityPolicy bypass on writes Fixedenforce_tool_operation on both place_order and cancel_order
2 No HTTPS enforcement before CLOB credentials Fixedensure_https() on all 4 credential-bearing paths + derivation
3 PolymarketClobCredentials Debug leaks api_key Fixed — manual Debug impl, all fields redacted
4 ClobCredentials Debug leaks api_key Fixed — manual Debug impl, all fields redacted
5 Client::new() silent fallback Fixed — panics with diagnostic message

Change Summary

File Change type Description
polymarket.rs New (1419 lines) Main tool: 11 actions, retry, HTTPS enforcement, SecurityPolicy
clob_auth.rs New (495 lines) L1 EIP-712 handshake + L2 HMAC signing
polymarket_orders.rs New (158 lines) EIP-712 order typed-data signing
polymarket_tests.rs New (1068 lines) 22 tests: happy paths, approval gate, signing, retry, timeout
tools.rs (config) Modified PolymarketConfig + PolymarketClobCredentials with serde defaults
catalog.rs Modified 2 capability entries (readonly + trading)
schemas.rs Modified Controller schema + handler
ops.rs Modified Conditional tool registration
wallet/mod.rs Modified secret_material exposed as pub(crate)
gitbooks/ New + Modified Integration guide + SUMMARY.md entry
tests/fixtures/ New (8 files) JSON fixtures for mock responses

New Finding

.post_json_raw(
&self.polygon_rpc_url,
"",
Some(HeaderMap::new()),
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.

[major] Polygon RPC eth_call is sent via post_json_raw with an empty HeaderMap — no Content-Type: application/json header. The JSON-RPC spec requires it, and strict providers (Alchemy, Infura, some self-hosted nodes) will reject the request with 415 or silently misparse the body.

The CLOB POST path correctly adds the header in post_signed_clob_json; this path should do the same.

Suggestion:

let mut rpc_headers = HeaderMap::new();
rpc_headers.insert(
    reqwest::header::CONTENT_TYPE,
    HeaderValue::from_static("application/json"),
);
let response = self
    .post_json_raw(
        &self.polygon_rpc_url,
        "",
        Some(rpc_headers),
        serde_json::to_string(&payload)
            .context("Failed to serialize Polygon RPC payload")?,
    )
    .await?;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 38f7777get_usdc_allowance_for_user now builds a HeaderMap with Content-Type: application/json and passes it to post_json_raw. Test extended to assert the header is present on the captured Polygon RPC request (regression guard for the venue 2/3 PRs too).

oxoxDev added 8 commits May 19, 2026 16:42
tinyhumansai#1398)

Two concurrent place_order calls each fetch GET /nonce?user=<eoa>
independently and receive the same nonce, leading the CLOB to silently
reject the second signed order. Returning false from is_concurrency_safe
for write actions lets the agent harness serialize them. Reads stay
concurrency-safe.

Addresses M3gA-Mind review (must-fix #1).

Refs tinyhumansai#1398 (Polymarket trading)
…ai#1398)

rand::random::<u64>() draws from a thread-local PRNG with predictable
seeding — adequate for non-adversarial code paths but unsafe for an
on-chain order salt where predictability is exploitable by front-runners
who observe partial nonce/salt sequences and replay/front-run.

Switch to OsRng (re-exported through chacha20poly1305::aead, the same
import shape encryption/core.rs already uses), draw 8 fresh bytes from
the OS entropy source, and feed those to the existing u64 salt slot.

Addresses M3gA-Mind review (must-fix #2).

Refs tinyhumansai#1398 (Polymarket trading)
…ce read (tinyhumansai#1398)

ensure_https previously guarded only the CLOB host. The
get_usdc_allowance_for_user path POSTs an eth_call payload to
polygon_rpc_url that carries the EOA wallet address; even with no
credentials in the body, leaking the address over plaintext HTTP
narrows the user's identity to anyone on-path. Apply the same
loopback carve-out so the mock harness stays usable.

Addresses M3gA-Mind review (must-fix #3).

Refs tinyhumansai#1398 (Polymarket trading)
…tinyhumansai#1398)

derive_credentials computed now_unix_secs() once and reused it across
the POST /auth/api-key attempt and the fallback GET /auth/derive-api-key
call. If the CLOB enforces a short anti-replay window (the L1 EIP-712
typed-data signature is timestamp-bound), a slow first request could
push the fallback timestamp out of range and surface a spurious 401.

Re-derive the timestamp immediately before the fallback request so the
new signature covers a fresh now value.

Addresses M3gA-Mind review (should-fix #4).

Refs tinyhumansai#1398 (Polymarket trading)
tinyhumansai#1398)

get_json, post_json_raw, and delete_json each carried a ~60-line
identical retry loop differing only in HTTP method label and minor
response-parsing details. Collapse all three into a single
send_with_retry method parameterised by reqwest::Method, optional
query params, optional body. ~180 LOC removed; the retry policy,
timeout handling, status-class classification, and error-message
shape are now consistent across every Polymarket HTTP exit.

GET previously parsed via `response.json::<Value>()` and POST/DELETE
via `text() + serde_json::from_str`. Unified to the text-then-parse
shape so empty bodies normalise to `Value::Null` for all verbs — no
behavioural drift for already-passing tests.

Addresses M3gA-Mind review (should-fix #5).

Refs tinyhumansai#1398 (Polymarket trading)
…inyhumansai#1398)

Adds a happy-path mock route to /markets and asserts that
get_market with neither parameter returns the descriptive
"requires either 'market_id' or 'slug'" error AND issues zero HTTP.
The mock keeps a body in place to prove the no-HTTP property (the
route's call counter must stay at 0).

Partially addresses M3gA-Mind review (should-fix #6 covered for
get_market). Derivation POST→GET fallback test and
persist_clob_credentials failure-path test deferred to a follow-up
that introduces the multi-response mock fixture they need.

Refs tinyhumansai#1398 (Polymarket trading)
…intext threat-model (tinyhumansai#1398)

Six inline doc comments for non-obvious decisions surfaced by the
M3gA-Mind review:

1. require_write_approval — clarifies that it is a client-side string
   check coupled with the framework's SecurityPolicy gate; both layers
   must remain until tinyhumansai#1339 lands.
2. round_down — explains floor for sizes (prevents over-sell).
3. round_normal — explains half-up for prices (prevents systematic
   under-quote); asymmetry vs round_down is intentional.
4. Raw ERC-20 allowance ABI encoding in eth_call — documents the
   selector + 32-byte padding shape so a reader doesn't read it as
   a bug.
5. persist_clob_credentials — declares the in-memory Mutex as the
   authoritative source; persist is best-effort across-restart resume.
6. PolymarketClobCredentials::derived_clob_credentials — threat-model
   call-out (plaintext today, SecretStore migration via tinyhumansai#1900).

No behavioural change. Addresses M3gA-Mind review (doc items 7-11).

Refs tinyhumansai#1398 (Polymarket trading)
…ivation (tinyhumansai#1398)

Two M3gA-Mind review items collapsed into a single co-located change:

1. Unify ClobCredentials (internal, clob_auth.rs) and
   PolymarketClobCredentials (config-facing) into a single
   PolymarketClobCredentials type defined alongside PolymarketConfig.
   The dual struct + From-impl glue was 30 lines of conversion
   boilerplate for two structurally-identical types. clob_auth.rs +
   polymarket.rs now use the config type directly; is_complete() lives
   as an inherent method on the unified type so the cache-warm filter
   still works. Default impl added so test fixtures with empty creds
   keep working.

2. Single-flight credential derivation. ensure_clob_credentials
   previously dropped the cache lock between the cache check and the
   derive call, so two concurrent callers could both observe an empty
   cache and each fire their own POST /auth/api-key — the CLOB would
   silently retain only one set of credentials and reject signatures
   generated with the other. Hold the tokio::sync::Mutex guard across
   the derive call (and drop it before the best-effort
   persist_clob_credentials so other tool calls can proceed against
   the freshly populated cache).

Addresses M3gA-Mind review (Code Quality "Dual credential structs" +
Security "Concurrent credential derivation race").

Refs tinyhumansai#1398 (Polymarket trading)
@oxoxDev
Copy link
Copy Markdown
Contributor Author

oxoxDev commented May 19, 2026

@M3gA-Mind — thank you for the thorough pass. Going through item by item. Eight commits landed on the branch addressing your review, plus an explicit deferral list for the items that need wider architectural alignment.

Must-fix (3/3) ✅

# Item Commit
1 is_concurrency_safefalse for write actions (nonce collision) 012c6397
2 CSPRNG salt via OsRng (no more rand::random) a88af32c
3 ensure_https guard on polygon_rpc_url in allowance read e2e54833

Should-fix (3/3) ✅

# Item Commit
4 Re-derive timestamp before fallback in derive_credentials b440dc72
5 Extract send_with_retry helper — dedupes ~180 LOC across get_json / post_json_raw / delete_json 36034502
6 get_market with neither market_id nor slug test cf3604c3 (POST→GET fallback + persist-failure tests deferred — see below)

Architectural cleanup (in this PR) ✅

# Item Commit
7 Unify ClobCredentials + PolymarketClobCredentials (delete the dual struct + From-impl glue) 2b098ee4
8 ensure_clob_credentials now holds the cache lock across derive_credentials — single-flight derivation, no more concurrent-caller race 2b098ee4
9 Inline doc comments for non-obvious decisions: approval-gate stopgap (with security note), round_down/round_normal asymmetry, raw ERC-20 ABI encoding, persist_clob_credentials best-effort guarantee, derived_clob_credentials plaintext threat model 2b4ce323

Deferred to follow-up (explicit rationale)

Item Why deferred
f64 → rust_decimal::Decimal for price/size Crate addition sets precedent for the Kalshi + Hyperliquid PRs (venues 2/3 + 3/3 on this umbrella). Want architectural buy-in on Decimal vs string-passthrough before paying the dep cost on all three venues. Filing as follow-up after the umbrella merges.
File-size split of polymarket.rs (1419 LOC, >2.8× the project guideline) Genuine refactor that needs module-boundary discussion — likely polymarket/{tool.rs, request.rs, signing.rs, allowance.rs}. Will land in a dedicated PR after the venue-3 PR opens (so Kalshi/Hyperliquid can adopt the same shape).
Replace unsafe { std::env::set_var } with serial_test crate The existing TEST_ENV_LOCK static std::sync::Mutex<()> already serializes env-var tests project-wide; the unsafe blocks are an artefact of Rust 2024 edition's tightened std API and exist throughout the codebase. Migrating just polymarket tests to serial_test adds a new dev-dep + breaks the project's existing serialization convention. Worth a project-wide RFC.
Credential-derivation POST→GET fallback test + persist-failure path test Requires a multi-response mock fixture (the existing route helper takes a single VecDeque per path). Cleanest landed alongside the test-helper refactor I'm planning for Kalshi/Hyperliquid (whose RSA-PSS + EIP-712 signing paths need similar multi-response tests).
SecretStore migration for derived_clob_credentials Explicitly blocked on #1900. Threat-model is now documented inline so the gap is visible until that lands.

What you might also want to look at

  • The HTTPS guard carves out 127.0.0.1 / localhost / [::1] for the in-process TcpListener mock harness — explicitly documented as a test affordance. Real Polymarket CLOB hosts don't resolve to loopback. If you'd prefer #[cfg(test)]-gated allowance instead, happy to refactor.
  • is_concurrency_safe returns false only for place_order / cancel_order — reads stay concurrency-safe. Want it tighter (e.g. include credentialed reads like get_positions)? They don't have the nonce-collision pattern but do share the cache-warm-up window.
  • The require_write_approval doc comment now spells out the framework gate (SecurityPolicy::enforce_tool_operation) + stopgap layering so reviewers know both must remain until Keep triggers read-only until users approve writes #1339.

Re-pushed all 9 commits to oxoxDev:feat/1398-polymarket. CR is re-running on the latest tip.

…h_call (tinyhumansai#1398)

The allowance read path called post_json_raw with an empty HeaderMap.
Strict JSON-RPC providers (Alchemy, Infura, some self-hosted nodes)
reject requests with no Content-Type as 415 Unsupported Media Type or
silently misparse the body. The CLOB POST path already inserts
application/json via post_signed_clob_json; the Polygon RPC path now
matches.

Test extended to assert the header is present on the captured request —
regression guard for the same class of bug across the venue 2/3 PRs.

Addresses graycyrus comment 3265708296.

Refs tinyhumansai#1398 (Polymarket trading)
@graycyrus graycyrus dismissed stale reviews from M3gA-Mind and themself May 19, 2026 13:44

Clean

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

Labels

feature Net-new user-facing capability or product behavior.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add Polymarket, Kalshi, and Hyperliquid trading integrations

3 participants