fix(trader): align ACP wire shape with trader-service (expiry, volume, status)#7
Closed
vrogojin wants to merge 3 commits intofix/encrypt-decrypt-namespacefrom
Closed
fix(trader): align ACP wire shape with trader-service (expiry, volume, status)#7vrogojin wants to merge 3 commits intofix/encrypt-decrypt-namespacefrom
vrogojin wants to merge 3 commits intofix/encrypt-decrypt-namespacefrom
Conversation
added 3 commits
May 4, 2026 00:28
…, 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).
vrogojin
pushed a commit
that referenced
this pull request
May 4, 2026
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.
vrogojin
pushed a commit
that referenced
this pull request
May 4, 2026
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).
vrogojin
added a commit
that referenced
this pull request
May 4, 2026
…, drop status) (#8) * fix(trader): align ACP wire shape with trader-service (expiry, volume, 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. * review: round 1 steelman fixes (CLI guards, extracted pure fn, tests) 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. * review: round 2 polish — pin parseInt('1e6') surprise behavior 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). --------- Co-authored-by: Vladimir Rogojin <vrogojin@blockyinnovations.com>
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.
Summary
Three upstream wire-shape mismatches between sphere-cli's
sphere trader …namespace and trader-service's actual ACP-0 wire 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 from being testable end-to-end.Base:
fix/encrypt-decrypt-namespace(PR #6). Re-target tomainonce #6 lands.Three fixes
1.
--volume-total→--volume-maxflagThe trader's
CREATE_INTENTACP param isvolume_maxpertrader-service/src/trader/acp-types.ts:23. sphere-cli was sendingvolume_totalover the wire which the trader silently dropped. Renames CLI flag, internal opt name, and wire param.2.
expiry_mswire param →expiry_secThe 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 sendingexpiry_mswhich the trader rejected withINVALID_PARAM("expiry_sec must be a finite number").The
--expiry-msCLI flag stays (ergonomically consistent with other timeout flags); we convert at the wire boundary:Math.floor(opts.expiryMs / 1000). Sub-1000ms expiries floor to 0 and fail the trader's positive-int check — correctly, since sub-second expiries make no sense for trade intents.3.
sphere trader statusremovedSTATUS is SYSTEM-scoped per the Unicity architecture: system commands (STATUS, SHUTDOWN_GRACEFUL, SET_LOG_LEVEL, EXEC) route through the tenant's host manager via HMCP, not direct controller→tenant ACP. The trader correctly rejected direct STATUS calls with
UNAUTHORIZED("System command STATUS can only be sent by the host manager").The subcommand is removed; controllers should use
sphere host inspect <instance>(HMCP) for trader liveness probes, or rely onsphere trader portfolio/list-intentssucceeding as an implicit liveness signal.The subcommand-tree test renames from "exposes all 7 expected subcommands" to "exposes the 6 controller-scoped trader subcommands" with a comment documenting the architectural distinction.
Verification
Live-testnet verification downstream (vrogojin/trader-service#15 with this branch's built sphere-cli):
The full flow now passes: HMA spawns escrow + 2 traders → set-strategy on each → portfolio reads → list-intents (empty) → create-intent on Alice → list-intents (CREATED) → cancel-intent → list-intents (CANCELLED).
Test plan
npm run checkpassessphere trader --helplists 6 subcommands (no status)SPHERE_CLI_BINat this branch's builtbin/sphere.mjs, runsnpm run test:e2e-live -- hma-trade-flow.e2e-live.test.ts— passes in <1 min.Out of scope
sphere host inspect's implementation) — that's already wired in sphere-cli'shostnamespace.