Conversation
added 3 commits
May 4, 2026 08:17
…, status) 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 <instance>` (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.
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.
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).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replaces #7 — PR-A merged via squash, which deleted the base branch and auto-closed #7. This PR has the same content rebased onto main.
Summary
Three wire-shape mismatches between sphere-cli's
sphere trader …namespace and trader-service's actual ACP-0 schema. All three surfaced as live-testnet failures in trader-service's HMA-orchestrated e2e suite (vrogojin/trader-service#15) and block the full create-intent → cancel-intent → settlement flow.Three fixes
--volume-total→--volume-maxflag. Trader's CREATE_INTENT ACP param isvolume_max(trader-service/src/trader/acp-types.ts:23). Renames CLI flag, internal opt, wire param.expiry_ms→expiry_secwire param. Trader validatesexpiry_secas positive integer ≤ 7d. CLI flag stays--expiry-msfor ergonomic consistency; convert at wire boundary viaMath.floor(n / 1000). CLI-layer guards reject sub-1000ms with clear message and >7d with the trader's own bound mirrored locally.sphere trader statusremoved. STATUS is SYSTEM-scoped (HMA-only). Trader correctly rejects direct ACP STATUS with UNAUTHORIZED. Usesphere host inspect <instance>for trader liveness.Steelman
Steelman loop ran 2 rounds and returned ROUND CLEAN:
buildCreateIntentParams+ 11 unit tests including parseInt(1e6) pinning).Verified
Live-testnet downstream (vrogojin/trader-service#15 with this branch's built sphere-cli):
Full HMA-orchestrated flow now passes: spawn → set-strategy → portfolio → list-intents (empty) → create-intent → list-intents (CREATED) → cancel-intent → list-intents (CANCELLED) → cleanup.