fix: trade quote/execute JSON output; schema wallet + unknown-cmd; deprecated help examples#167
fix: trade quote/execute JSON output; schema wallet + unknown-cmd; deprecated help examples#167Nicolai1205 wants to merge 6 commits intomainfrom
Conversation
…precated help examples - trade quote: return structured JSON to stdout on success; throw structured errors on failure (previously all output went to stderr, stdout was silent) - trade execute: same -- return structured JSON on success/failure - schema wallet: add wallet to schema.json so 'nansen schema wallet' returns the wallet subcommand schema instead of the full schema dump - schema <unknown>: throw error instead of silently returning full schema - schema trade execute: add missing --quote required param - --help examples: fix deprecated 'nansen smart-money' paths to 'nansen research smart-money' (and all other research category prefixes) - generateSubcommandHelp: auto-prefix 'research' for research categories Fixes findings from clawfooding session: P0 #1-3, P1 #5-6, P2 #10-11 Does not overlap with open PRs: #58, #71, #72, #103, #137, #156 Co-Authored-By: Nicolai Sondergaard (Openclaw Bot) <93130718+nansen-devops@users.noreply.github.com>
- Remove unused 'exit' from buildTradingCommands deps destructuring - Replace no-useless-catch in execute handler with meaningful normalization (codeless errors get TRADE_ERROR code for consistent downstream handling) - Update 17 tests that expected old errorOutput+exit pattern to expect structured throws (MISSING_PARAM, WALLET_REQUIRED, UNSUPPORTED_CHAIN, INVALID_AMOUNT, INVALID_QUOTE, TRADE_FAILED, UNKNOWN_COMMAND) - Update schema unknown-command test to expect error throw - Update deprecated quote route test to expect error type result - Wrap 'pass validation' execute tests in try/catch (downstream fails without network but we only care the safety check passed) npm run lint: 0 errors npm test: 682 passed, 0 failed, 2 skipped Co-Authored-By: Nicolai Sondergaard (Openclaw Bot) <93130718+nansen-devops@users.noreply.github.com>
- GAP 1: Add trade quote success-path test verifying structured return
(quoteId, chain, walletAddress, executeCommand, quotes array)
- GAP 2: Add schema('wallet') test asserting all 7 subcommand keys
- GAP 3: Replace 3x weak rejects.toBeDefined() with
rejects.toMatchObject({ code: 'TRADE_FAILED' })
- GAP 4: Already covered (fetch NOT called assertion exists)
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Nicolai Sondergaard (Openclaw Bot) <93130718+nansen-devops@users.noreply.github.com>
- Remove `status: null` from 18 client-side validation throws (formatError already defaults to null) - Remove dead `err.code === 'INVALID_AMOUNT'` branch in quote catch (thrown before the try block, can never reach catch) - Remove unused `details: err.details || null` from catch re-throws (caught errors never have .details, and formatError reads .data not .details) - Shorten verbose error messages: WALLET_REQUIRED, execute MISSING_PARAM, TRADE_REVERTED (remove implementation details and speculative explanations) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Co-Authored-By: Nicolai Sondergaard (Openclaw Bot) <93130718+nansen-devops@users.noreply.github.com>
0xlaveen
left a comment
There was a problem hiding this comment.
Cody's Review — fix/trade-json-output
Verdict: Approve with minor nits. Solid PR. The core pattern shift (errorOutput+exit → structured throws with codes) is the right call for an agent-first CLI. PR description is exemplary. Test coverage is thorough.
What's good
- Consistent error contract. Every error path now throws
{ code, message }—MISSING_PARAM,WALLET_REQUIRED,NO_QUOTES,TRADE_FAILED,TRADE_REVERTED,UNSUPPORTED_CHAIN,INVALID_QUOTE,UNKNOWN_COMMAND. This gives agents a machine-parseable vocabulary to branch on. - Structured return values.
trade quotereturns{ quoteId, chain, walletAddress, executeCommand, quotes }.trade executereturns{ status, txHash, chain, explorerUrl, ... }. No moreundefinedleaking to stdout. - Schema completeness. Wallet as first-class schema entry,
--quoteparam on execute, unknown-command rejection — all three were real agent blockers. - generateSubcommandHelp auto-prefixing. Detects research subcategories dynamically instead of hardcoding. Less drift surface.
- Test migration is clean. 19 tests moved from asserting
exitCalled+logs.some(...)torejects.toMatchObject({ code, message }). The new assertions are tighter.
Issues to address
1. package-lock.json version bump (1.9.3 → 1.10.1) should not be in this PR
Changesets handles versioning at release time. Committing a manual version bump here will conflict with every other open PR that touches package-lock.json and can desync the version from what changesets would produce. Revert this hunk — let the release pipeline own the version.
2. Error normalization in execute's catch block loses the stack trace
// trading.js — execute catch block
if (!err.code) {
throw Object.assign(new Error(err.message), { code: 'TRADE_ERROR', status: err.status || null });
}new Error(err.message) creates a fresh stack trace pointing at the catch block, not the original throw site. For debugging, mutate the existing error instead:
if (!err.code) {
err.code = 'TRADE_ERROR';
err.status = err.status || null;
}
throw err;Same issue exists in the quote handler's amount-error augmentation path.
3. details: null pollutes the error shape
{ code: 'NO_QUOTES', details: warnings.length ? { warnings } : null }Prefer omitting the key entirely when there's nothing to report:
{ code: 'NO_QUOTES', ...(warnings.length ? { details: { warnings } } : {}) }Downstream consumers checking if (err.details) won't care, but JSON.stringify will include "details":null which is noise in agent output.
Minor nits (non-blocking)
- Swallowed errors in "pass validation" tests. The
try { ... } catch (_) {}pattern absorbs everything. If the safety-check logic changes and starts throwing early, these tests still pass green. Consider asserting that any caught error is NOT a validation code:catch (e) { expect(e.code).not.toBe('TRADE_FAILED'); }or similar. .gitignoreaddition of.worktrees/is out of scope for this fix PR. Harmless but should be its own commit or separate PR for clean history.- Changeset bump is
minor. Defensible sincetrade quote/executenow return new shapes (net-new API surface), but the title saysfix:. Just make sure the version strategy is intentional — if this lands asminor, the next release is a feature bump.
tl;dr: The pattern is right, the coverage is right. Fix the package-lock version leak and the stack-trace-losing rethrow, and this is good to ship. 🦁
…ace, omit null details - Revert package-lock.json version bump (1.10.1 → 1.9.3) — changesets handles versioning - Preserve original stack trace in error normalization (mutate instead of new Error) - Omit details key when null instead of setting details: null (NO_QUOTES) - Strengthen pass-validation test catch blocks to assert no validation error codes - Remove out-of-scope .worktrees/ gitignore addition Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Co-Authored-By: Nicolai Sondergaard (Openclaw Bot) <93130718+nansen-devops@users.noreply.github.com>
|
All review comments addressed in commit Required:
Minor nits:
684 tests passing, lint clean. |
Problem
Three P0/P1 issues found during clawfooding (agent-first testing of v1.10.1).
1.
trade quoteandtrade executeproduce no JSON output (P0)Both commands returned
undefinedto the CLI runner, meaning stdout was entirely silent. All output went toerrorOutput(stderr). For a CLI marketed as "structured JSON output for AI agents", this is a critical gap.Repro:
Fix: Both commands now return structured objects on success. Human-readable output is preserved on stderr for TTY users. All validation errors (
MISSING_PARAM,WALLET_REQUIRED,NO_QUOTES,TRADE_FAILED,TRADE_REVERTED) now throw structured errors caught by the CLI runner and emitted as{"success":false,"error":"..."}on stdout.trade quote success output:
{ "success": true, "data": { "quoteId": "1772442074192-702ec3c3", "chain": "solana", "walletAddress": "7L369cy...", "executeCommand": "nansen trade execute --quote 1772442074192-702ec3c3", "quotes": [{ "aggregator": "okx", "inAmount": "1000000000", "outAmount": "83229795", ... }] } }2.
nansen schema walletreturned the full schema;nansen schema blahsilently returned the full schema (P1)walletwas not inSCHEMA.commands, so the lookup fell through tocompactSchema(SCHEMA)— returning all 37 commands. Unknown commands did the same with no error.Fix:
walletas a first-class entry inschema.jsonwith all 7 subcommands and their options.{"success":false,"error":"Unknown schema command: blah. Available: research, trade, wallet"}.3.
schema trade executewas missing the required--quoteparam (P0)The schema showed only
chainandwalletfortrade execute. The only required input —--quote <quoteId>— was absent. An agent reading the schema before calling the command could not discover this requirement.Fix: Added
--quoteasrequired: truein the execute subcommand schema.4. Deprecated command paths in
--helpexamples (P2)All
--helpexamples and no-arg command outputs used the old pre-namespace paths (e.g.nansen smart-money netflow) instead ofnansen research smart-money netflow. Affected 6 hardcoded example strings and thegenerateSubcommandHelpauto-generator.Fix: All examples updated.
generateSubcommandHelpnow detects research categories and auto-prefixesresearch.Conflict check
Reviewed all 14 open PRs. No overlap with: #58 (wallet password), #71 (EIP-1559 signing), #72 (Hyperliquid perps), #103 (profiler compare args), #135/#137 (schema options), #156 (sparse trade --help). Changes to
trading.jsadd structured returns and convert errorOutput+exit to throws; feature additions in #71/#72 are separate code paths.Files changed
src/trading.js—trade quoteandtrade executehandlers: structured JSON returns + throw-based error handlingsrc/schema.json— addedwalletcommand schema; added--quotetotrade executesrc/cli.js— schema dispatcher: unknown-command error + wallet lookup;generateSubcommandHelp: research prefix; 6 hardcoded deprecated examples.changeset/fix-trade-json-output-schema.md— minor version bumpPost-review audit (second commit)
After filing this PR, ran a full lint + test pass:
Lint fixes (0 errors):
exitfrombuildTradingCommandsdeps (was orphaned after converting errorOutput+exit to throws)TRADE_ERRORcode for consistent downstream handling)Test updates (682 passing, 0 failing):
await expect(...).rejects.toMatchObject({ code: '...', message: ... })schema unknown commandtest to expectUNKNOWN_COMMANDthrowdeprecated quote routetest to expectresult.type === 'error'