From 493f07f4c0050eb76812ace39de35e062afff783 Mon Sep 17 00:00:00 2001 From: Vladimir Rogojin Date: Mon, 4 May 2026 00:28:54 +0200 Subject: [PATCH 1/3] fix(trader): align ACP wire shape with trader-service (expiry, volume, status) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three upstream-discovered mismatches between sphere-cli's `sphere trader …` namespace and trader-service's actual ACP-0 wire schema. All three surfaced as failures in trader-service's HMA-orchestrated e2e tests (vrogojin/trader-service#15) and block the full create-intent → cancel-intent → settlement flow from being testable end-to-end. 1. `--volume-total` flag → `--volume-max` flag. The trader's CREATE_INTENT ACP param is `volume_max` (`/home/vrogojin/trader-service/src/trader/acp-types.ts:23`). sphere-cli was sending `volume_total` over the wire, which the trader silently dropped (parameter not in the schema). This PR was already partially staged in the working tree of the `fix/encrypt-decrypt-namespace` branch — landing it cleanly here. 2. `expiry_ms` wire param → `expiry_sec`. The trader validates `params['expiry_sec']` as a finite positive integer ≤ 7 days (`trader-service/src/trader/trader-command-handler.ts:331-342`). sphere-cli was sending `expiry_ms` which the trader rejected with INVALID_PARAM ("expiry_sec must be a finite number"). The `--expiry-ms` CLI flag stays — converting to seconds at the wire boundary keeps the flag ergonomic (matches other timeout flags) while fixing the wire shape. `Math.floor(ms / 1000)` so sub-1000ms expiries floor to 0 and fail the trader's positive-int check (correctly: sub-second expiries make no sense for trade intents). 3. `sphere trader status` removed. STATUS is a SYSTEM-scoped ACP command per the Unicity architecture (system commands like STATUS / SHUTDOWN_GRACEFUL / SET_LOG_LEVEL / EXEC route through the tenant's host manager via HMCP, not direct controller→tenant ACP). The trader correctly rejects direct STATUS calls with UNAUTHORIZED. The subcommand is removed; controllers should use `sphere host inspect ` (HMCP) for trader liveness probes, or rely on `sphere trader portfolio` / `list-intents` succeeding as an implicit liveness signal. The trader-commands.test.ts subcommand-tree assertion drops `status` and renames the test to "exposes the 6 controller-scoped trader subcommands" with a comment explaining the architectural distinction. Verified: $ npm run check → 95/95 tests passing $ node bin/sphere.mjs trader --help → 6 subcommands listed (no status) Verified downstream against live testnet: $ cd /home/vrogojin/trader-service $ TRADER_E2E_SKIP_PREFLIGHT=1 npx vitest run --config vitest.e2e-live.config.ts \ test/e2e-live/hma-trade-flow.e2e-live.test.ts ✓ drives the full trader CLI surface against HMA-spawned tenants (48s) Set-strategy + portfolio + list-intents + create-intent + list-intents + cancel-intent + verify-CANCELLED — all green. Depends on PR #6 (encrypt/decrypt L1 fix) for the typecheck baseline. --- src/trader/trader-commands.test.ts | 8 +++--- src/trader/trader-commands.ts | 42 ++++++++++++++++++------------ 2 files changed, 31 insertions(+), 19 deletions(-) diff --git a/src/trader/trader-commands.test.ts b/src/trader/trader-commands.test.ts index 3002dca..78fc8ca 100644 --- a/src/trader/trader-commands.test.ts +++ b/src/trader/trader-commands.test.ts @@ -78,9 +78,12 @@ describe('resolveTenantAddress', () => { }); describe('createTraderCommand', () => { - it('exposes all 7 expected subcommands', () => { + it('exposes the 6 controller-scoped trader subcommands', () => { const trader = createTraderCommand(); const names = trader.commands.map((c) => c.name()).sort(); + // No `status` here: STATUS is system-scoped per Unicity + // architecture and routes through the host manager via HMCP. + // Use `sphere host inspect ` for trader liveness. expect(names).toEqual([ 'cancel-intent', 'create-intent', @@ -88,7 +91,6 @@ describe('createTraderCommand', () => { 'list-intents', 'portfolio', 'set-strategy', - 'status', ]); }); @@ -120,7 +122,7 @@ describe('createTraderCommand', () => { const optionFlags = cmd!.options.map((o) => o.long); expect(optionFlags).toEqual(expect.arrayContaining([ '--direction', '--base', '--quote', '--rate-min', '--rate-max', - '--volume-min', '--volume-total', '--expiry-ms', + '--volume-min', '--volume-max', '--expiry-ms', ])); }); diff --git a/src/trader/trader-commands.ts b/src/trader/trader-commands.ts index 25edafb..324ee15 100644 --- a/src/trader/trader-commands.ts +++ b/src/trader/trader-commands.ts @@ -41,7 +41,7 @@ interface CreateIntentOpts { rateMin: string; rateMax: string; volumeMin: string; - volumeTotal: string; + volumeMax: string; expiryMs?: string; } @@ -218,7 +218,7 @@ async function handleCreateIntent(cmd: Command, opts: CreateIntentOpts): Promise rate_min: opts.rateMin, rate_max: opts.rateMax, volume_min: opts.volumeMin, - volume_total: opts.volumeTotal, + volume_max: opts.volumeMax, }; if (opts.expiryMs !== undefined) { const n = Number.parseInt(opts.expiryMs, 10); @@ -227,7 +227,16 @@ async function handleCreateIntent(cmd: Command, opts: CreateIntentOpts): Promise process.exitCode = 1; return; } - params['expiry_ms'] = n; + // Trader's ACP CREATE_INTENT param is `expiry_sec` (validated as + // a finite number ≤ 7 days). Sending `expiry_ms` produces an + // INVALID_PARAM rejection. The CLI flag stays in milliseconds + // for ergonomic consistency with other timeout flags; we + // convert at the wire boundary. + // Floor to avoid emitting a non-integer second value if the + // caller passes a sub-1000ms expiry (which would round to 0 + // and then fail the trader's positive-int check below — that's + // the right behaviour: sub-second expiries make no sense). + params['expiry_sec'] = Math.floor(n / 1000); } const response = await transport.sendCommand('CREATE_INTENT', params); emitResult(json, response); @@ -287,12 +296,16 @@ async function handlePortfolio(cmd: Command): Promise { }); } -async function handleStatus(cmd: Command): Promise { - await runWithTransport(cmd, async ({ transport, json }) => { - const response = await transport.sendCommand('STATUS', {}); - emitResult(json, response); - }); -} +// `sphere trader status` was previously wired to send STATUS over ACP +// directly to the trader. STATUS is a SYSTEM-scoped command per the +// Unicity architecture (system commands like STATUS, SHUTDOWN_GRACEFUL, +// SET_LOG_LEVEL, EXEC route through the tenant's host manager via HMCP, +// not direct controller→tenant ACP). The trader correctly rejects +// direct STATUS calls with UNAUTHORIZED. The subcommand has been +// removed from the CLI tree below; controllers should use +// `sphere host inspect ` (HMCP) to probe trader liveness, or +// rely on `sphere trader portfolio`/`list-intents` succeeding as an +// implicit liveness signal. async function handleSetStrategy(cmd: Command, opts: SetStrategyOpts): Promise { await runWithTransport(cmd, async ({ transport, json }) => { @@ -346,7 +359,7 @@ export function createTraderCommand(): Command { .requiredOption('--rate-min ', 'Minimum acceptable rate (string-encoded bigint)') .requiredOption('--rate-max ', 'Maximum acceptable rate (string-encoded bigint)') .requiredOption('--volume-min ', 'Minimum volume per match') - .requiredOption('--volume-total ', 'Total intent volume') + .requiredOption('--volume-max ', 'Total intent volume') .option('--expiry-ms ', 'Expiry duration in milliseconds (default: 24h)') .action(async function (this: Command, opts: CreateIntentOpts) { await handleCreateIntent(this, opts); @@ -385,12 +398,9 @@ export function createTraderCommand(): Command { await handlePortfolio(this); }); - trader - .command('status') - .description('Show STATUS — uptime + adapter info') - .action(async function (this: Command) { - await handleStatus(this); - }); + // `status` removed — STATUS is system-scoped and routes through the + // host manager. See the comment on the deleted handleStatus above. + // Use `sphere host inspect ` for trader liveness probes. trader .command('set-strategy') From 234227bba27cff8994df07af3519bc0a7c2250b6 Mon Sep 17 00:00:00 2001 From: Vladimir Rogojin Date: Mon, 4 May 2026 00:43:35 +0200 Subject: [PATCH 2/3] review: round 1 steelman fixes (CLI guards, extracted pure fn, tests) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Steelman round 1 on PR #7 found three issues: WARNING — Sub-1000ms expiry passed the CLI's `n <= 0` guard, then `Math.floor(n / 1000) = 0`, then the trader rejected with the opaque "expiry_sec must be positive" — confusing UX for what was a positive millisecond value. Add CLI-layer guard `n < 1000` with clear message before the conversion. Same approach for the 7-day upper bound (matches trader's own validation; saves a network round-trip on misuse). NOTE — No unit test for the wire conversion (`expiry_ms`→`expiry_sec` or `--volume-max` → `volume_max`). The PR's correctness rested entirely on the live e2e test in trader-service. Refactor: extract `buildCreateIntentParams(opts): { params } | { error }` as a pure function. The runWithTransport wrapper now just calls it, emits stderr on error-shape, and forwards on success. Add 10 targeted unit tests covering: - volume_max present (not volume_total) - --expiry-ms 5000 → expiry_sec=5 - --expiry-ms 90500 → 90 (floor) - omit --expiry-ms → no expiry_sec / expiry_ms in payload - --expiry-ms 999 → error mentioning "1 second" - --expiry-ms > 7d → error mentioning "7 days" - boundary cases (exactly 1000, exactly 7 days) — accepted - non-numeric/zero/negative — rejected - invalid direction — rejected - bigint-string fields pass through verbatim (no precision loss) NOTE (carry-forward) — trader-service has its own CLI shim (src/cli/main.ts) that ALSO had volume_total/expiry_ms — fixed in trader-service PR (vrogojin/trader-service#15) so direct-docker tests don't break either. Cross-repo coverage now consistent. Verified: 105/105 tests pass (was 95, +10 new wire-shape unit tests). Live e2e in trader-service still passes against testnet. --- src/trader/trader-commands.test.ts | 93 ++++++++++++++++++++++++++++++ src/trader/trader-commands.ts | 89 ++++++++++++++++++---------- 2 files changed, 152 insertions(+), 30 deletions(-) diff --git a/src/trader/trader-commands.test.ts b/src/trader/trader-commands.test.ts index 78fc8ca..0399408 100644 --- a/src/trader/trader-commands.test.ts +++ b/src/trader/trader-commands.test.ts @@ -10,6 +10,7 @@ import { parseTimeout, resolveTenantAddress, createTraderCommand, + buildCreateIntentParams, } from './trader-commands.js'; describe('parseTimeout (trader)', () => { @@ -173,3 +174,95 @@ describe('createTraderCommand', () => { } }); }); + +describe('buildCreateIntentParams (wire shape)', () => { + // Minimal valid opts; individual tests override the field they are exercising. + const baseOpts = { + direction: 'buy', + base: 'UCT', + quote: 'USDU', + rateMin: '100', + rateMax: '200', + volumeMin: '10', + volumeMax: '100', + }; + + it('emits volume_max (not volume_total) on the wire', () => { + const result = buildCreateIntentParams(baseOpts); + expect('error' in result).toBe(false); + if ('error' in result) return; + expect(result.params['volume_max']).toBe('100'); + expect(result.params).not.toHaveProperty('volume_total'); + }); + + it('converts --expiry-ms to expiry_sec at the wire boundary (5000ms → 5s)', () => { + const result = buildCreateIntentParams({ ...baseOpts, expiryMs: '5000' }); + expect('error' in result).toBe(false); + if ('error' in result) return; + expect(result.params['expiry_sec']).toBe(5); + expect(result.params).not.toHaveProperty('expiry_ms'); + }); + + it('floors fractional seconds (90500ms → 90s)', () => { + const result = buildCreateIntentParams({ ...baseOpts, expiryMs: '90500' }); + if ('error' in result) throw new Error(result.error); + expect(result.params['expiry_sec']).toBe(90); + }); + + it('omits expiry_sec when --expiry-ms not provided', () => { + const result = buildCreateIntentParams(baseOpts); + if ('error' in result) throw new Error(result.error); + expect(result.params).not.toHaveProperty('expiry_sec'); + expect(result.params).not.toHaveProperty('expiry_ms'); + }); + + it('rejects sub-1000ms expiries with a clear CLI-layer message', () => { + const result = buildCreateIntentParams({ ...baseOpts, expiryMs: '999' }); + expect('error' in result).toBe(true); + if (!('error' in result)) return; + expect(result.error).toMatch(/at least 1000.*1 second/); + expect(result.error).toContain('999'); + }); + + it('rejects expiries greater than 7 days', () => { + const sevenDaysMs = 7 * 24 * 60 * 60 * 1000; + const result = buildCreateIntentParams({ ...baseOpts, expiryMs: String(sevenDaysMs + 1) }); + expect('error' in result).toBe(true); + if (!('error' in result)) return; + expect(result.error).toMatch(/7 days/); + }); + + it('accepts the boundary cases — exactly 1000ms and exactly 7 days', () => { + const lower = buildCreateIntentParams({ ...baseOpts, expiryMs: '1000' }); + if ('error' in lower) throw new Error(lower.error); + expect(lower.params['expiry_sec']).toBe(1); + + const sevenDaysMs = 7 * 24 * 60 * 60 * 1000; + const upper = buildCreateIntentParams({ ...baseOpts, expiryMs: String(sevenDaysMs) }); + if ('error' in upper) throw new Error(upper.error); + expect(upper.params['expiry_sec']).toBe(sevenDaysMs / 1000); + }); + + it('rejects non-numeric, zero, and negative expiry-ms', () => { + for (const bad of ['abc', '0', '-100', 'NaN']) { + const r = buildCreateIntentParams({ ...baseOpts, expiryMs: bad }); + expect('error' in r, `expected error for expiry-ms="${bad}"`).toBe(true); + } + }); + + it('rejects invalid direction', () => { + const result = buildCreateIntentParams({ ...baseOpts, direction: 'sideways' }); + expect('error' in result).toBe(true); + if (!('error' in result)) return; + expect(result.error).toMatch(/buy.*sell/); + }); + + it('passes through bigint-string fields verbatim (no coercion)', () => { + const huge = '99999999999999999999999999'; + const result = buildCreateIntentParams({ ...baseOpts, volumeMax: huge }); + if ('error' in result) throw new Error(result.error); + // Must preserve the exact string — coercion to Number would lose + // precision and bigint serialization differs across SDKs. + expect(result.params['volume_max']).toBe(huge); + }); +}); diff --git a/src/trader/trader-commands.ts b/src/trader/trader-commands.ts index 324ee15..237c917 100644 --- a/src/trader/trader-commands.ts +++ b/src/trader/trader-commands.ts @@ -204,41 +204,68 @@ function emitResult(json: boolean, response: AcpResultPayload | AcpErrorPayload) // Subcommand handlers // ============================================================================= +/** + * Build the wire payload for CREATE_INTENT from CLI options. Pure + * function so it's unit-testable without a Sphere/DM stack. + * + * Returns `{ params }` on success or `{ error }` with a human- + * readable message the caller can write to stderr. Validation here + * mirrors the trader-side ACP validation + * (trader-service/src/trader/trader-command-handler.ts:331-342) so + * the operator gets a clear diagnostic at the CLI layer instead of + * an opaque INVALID_PARAM from a remote service. + */ +export function buildCreateIntentParams( + opts: CreateIntentOpts, +): { readonly params: Record } | { readonly error: string } { + if (opts.direction !== 'buy' && opts.direction !== 'sell') { + return { error: '--direction must be "buy" or "sell"' }; + } + const params: Record = { + direction: opts.direction, + base_asset: opts.base, + quote_asset: opts.quote, + rate_min: opts.rateMin, + rate_max: opts.rateMax, + volume_min: opts.volumeMin, + volume_max: opts.volumeMax, + }; + if (opts.expiryMs !== undefined) { + const n = Number.parseInt(opts.expiryMs, 10); + if (!Number.isFinite(n) || n <= 0) { + return { error: `--expiry-ms must be a positive integer (got "${opts.expiryMs}")` }; + } + // Trader's ACP CREATE_INTENT param is `expiry_sec` (validated + // as a finite positive integer ≤ 7 days). The CLI flag stays + // in milliseconds for ergonomic consistency with other timeout + // flags; we convert at the wire boundary via floor(ms/1000). + if (n < 1000) { + // Sub-second expiries make no sense for trade intents and + // floor(ms/1000) would map them to 0, which the trader + // rejects with an unhelpful INVALID_PARAM. Catch here. + return { error: `--expiry-ms must be at least 1000 (1 second); got ${n}` }; + } + const sevenDaysMs = 7 * 24 * 60 * 60 * 1000; + if (n > sevenDaysMs) { + // 7-day cap matches the trader's own validation. Catch it + // here too so the operator gets the right diagnostic without + // a network round-trip. + return { error: `--expiry-ms must not exceed 7 days (${sevenDaysMs}ms); got ${n}` }; + } + params['expiry_sec'] = Math.floor(n / 1000); + } + return { params }; +} + async function handleCreateIntent(cmd: Command, opts: CreateIntentOpts): Promise { await runWithTransport(cmd, async ({ transport, json }) => { - if (opts.direction !== 'buy' && opts.direction !== 'sell') { - writeStderr('--direction must be "buy" or "sell"'); + const built = buildCreateIntentParams(opts); + if ('error' in built) { + writeStderr(built.error); process.exitCode = 1; return; } - const params: Record = { - direction: opts.direction, - base_asset: opts.base, - quote_asset: opts.quote, - rate_min: opts.rateMin, - rate_max: opts.rateMax, - volume_min: opts.volumeMin, - volume_max: opts.volumeMax, - }; - if (opts.expiryMs !== undefined) { - const n = Number.parseInt(opts.expiryMs, 10); - if (!Number.isFinite(n) || n <= 0) { - writeStderr(`--expiry-ms must be a positive integer (got "${opts.expiryMs}")`); - process.exitCode = 1; - return; - } - // Trader's ACP CREATE_INTENT param is `expiry_sec` (validated as - // a finite number ≤ 7 days). Sending `expiry_ms` produces an - // INVALID_PARAM rejection. The CLI flag stays in milliseconds - // for ergonomic consistency with other timeout flags; we - // convert at the wire boundary. - // Floor to avoid emitting a non-integer second value if the - // caller passes a sub-1000ms expiry (which would round to 0 - // and then fail the trader's positive-int check below — that's - // the right behaviour: sub-second expiries make no sense). - params['expiry_sec'] = Math.floor(n / 1000); - } - const response = await transport.sendCommand('CREATE_INTENT', params); + const response = await transport.sendCommand('CREATE_INTENT', built.params); emitResult(json, response); }); } @@ -422,3 +449,5 @@ export function createTraderCommand(): Command { // Exported for unit tests. export { parseTimeout }; +// `buildCreateIntentParams` is also exported via its `export function` +// declaration above; named here for discoverability alongside parseTimeout. From cd9bc867f83b1265b772bbec823f3e19eae2bdf2 Mon Sep 17 00:00:00 2001 From: Vladimir Rogojin Date: Mon, 4 May 2026 00:45:57 +0200 Subject: [PATCH 3/3] =?UTF-8?q?review:=20round=202=20polish=20=E2=80=94=20?= =?UTF-8?q?pin=20parseInt('1e6')=20surprise=20behavior?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 2 of steelman loop on PR #7 returned ROUND CLEAN. One NOTE-level test gap was worth adding as polish: `Number.parseInt('1e6', 10)` returns 1 (parses '1', stops at 'e') rather than 1_000_000. A user passing `--expiry-ms 1e6` wanted 1 000 000 ms (16.7 minutes) but gets the truncated value 1, which the < 1000ms guard then rejects with a confusing message saying "got 1". The value is REJECTED (not silently misused), so this isn't a correctness bug — but the surprising parse behavior is worth pinning in a test so a future strict-parse refactor is tracked as a deliberate change. The two WARNING-level findings from round 2 (parseInt prefix- truncation accepts `'1000abc'` as 1000) are a pre-existing file-wide pattern affecting --limit, --max-concurrent, --timeout in addition to --expiry-ms. Tracked as a follow-up: introduce a strict-positive-int helper and apply uniformly. Out of scope for this PR. Verified: 106/106 tests pass (was 105, +1 new pinning test). --- src/trader/trader-commands.test.ts | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/src/trader/trader-commands.test.ts b/src/trader/trader-commands.test.ts index 0399408..465b842 100644 --- a/src/trader/trader-commands.test.ts +++ b/src/trader/trader-commands.test.ts @@ -250,6 +250,27 @@ describe('buildCreateIntentParams (wire shape)', () => { } }); + it('documents parseInt prefix-truncation behavior on scientific notation', () => { + // parseInt('1e6', 10) returns 1 (parses '1', stops at 'e'). + // The user wanted 1_000_000 ms (≈1000s ≈ 16.7m) but got the + // truncated value 1, then our < 1000ms guard fires and rejects. + // Counterintuitive but at least the value is REJECTED rather + // than silently misused. This test pins the surprising + // behavior so a future strict-parse refactor (e.g., using + // `Number(s)` which rejects 'e' in this context) is tracked + // as a deliberate change rather than an accidental drift. + // + // FIXME: harden numeric parsing across this file with a + // strict-positive-int helper. Same pattern affects --limit, + // --max-concurrent, --timeout. Out of scope for this PR. + const result = buildCreateIntentParams({ ...baseOpts, expiryMs: '1e6' }); + expect('error' in result).toBe(true); + if (!('error' in result)) return; + // The error message reports the parsed value (1), not the + // user's original input (1e6) — that's the surprising part. + expect(result.error).toMatch(/got 1\b/); + }); + it('rejects invalid direction', () => { const result = buildCreateIntentParams({ ...baseOpts, direction: 'sideways' }); expect('error' in result).toBe(true);