fix(legacy-cli): align imports with sphere-sdk L1 namespace + missing exports#6
Merged
Merged
Conversation
added 3 commits
May 3, 2026 22:52
… exports
Two upstream regressions broke `sphere --help` and `npm run typecheck`
against the latest @unicitylabs/sphere-sdk:
1. encrypt/decrypt/hexToWIF/generatePrivateKey/generateAddressFromMasterKey
are no longer top-level exports — they live in the L1 (alpha-chain)
namespace. Import L1 once and destructure at module load to keep the
diff to one block (call sites unchanged).
2. CreateInvoiceRequest, InvoiceRequestedAsset, GetInvoicesOptions,
PayInvoiceParams, ReturnPaymentParams are declared inside SDK's
AccountingModule but not re-exported at the package root. Extract
them structurally via Parameters<> on Sphere.prototype.accounting
method signatures so the types stay synced to the SDK without
manual mirroring — a future SDK signature change surfaces here as
a typecheck error.
Symptoms before this fix:
$ node bin/sphere.mjs --help
SyntaxError: '@unicitylabs/sphere-sdk' does not provide an export
named 'decrypt' / 'generateAddressFromMasterKey'
$ npm run typecheck
TS2459: Module '@unicitylabs/sphere-sdk' declares 'CreateInvoiceRequest'
locally, but it is not exported. (× 5 types)
After:
$ npm run check
Lint: 0 errors (warnings pre-existing). Typecheck: clean. Tests: 94/94.
$ sphere --help → exits 0, lists subcommands.
$ sphere host help → reaches the DM transport layer (needs a wallet+manager).
Unblocks the SkipIf-gated `sphere-cli-roundtrip.e2e-live.test.ts` in
agentic-hosting PR #22 (refactor/delete-ahctl) which probes
`sphere --help` exit code to decide whether to run.
…et note Round 1 of steelman loop on PR #6. Two findings addressed: MEDIUM — Zero behavioral test coverage of the changed code paths. The 30-test scaffold suite never imports `legacy-cli.ts` directly (every existing test goes through `createCli()` / `buildLegacyArgv()` which lazily delegate to legacy-cli at call time, AFTER the test runner has imported its own modules). A future SDK rename of any L1-destructured symbol would pass `npm run typecheck` + `npm test` while breaking `sphere wallet …` at runtime — exactly the failure mode that motivated this PR. Fix: add `src/legacy/legacy-cli-imports.test.ts` that statically imports `legacy-cli.ts` and asserts `legacyMain` is defined. If the top-of-file `const { encrypt, decrypt, hexToWIF, generatePrivateKey, generateAddressFromMasterKey } = L1;` ever fails (e.g., L1 becomes undefined or a symbol is renamed), Vitest now reports a clean TypeError at the precise line. A second test verifies each L1 symbol is a function at runtime — coverage stays shallow on purpose (we don't exercise the encrypt/decrypt round-trip; that needs a wallet fixture and crosses into integration-test territory). LOW — `Parameters<>`-extracted Invoice* types are structurally identical to the SDK's named interfaces but TS error messages at call sites display the structural expansion instead of the named type. No correctness impact, but worth a comment so a future reader knows to migrate back to direct imports if/when the SDK re-exports the names. Added a QoL note under the type-aliases block. Verified: 96/96 tests pass (was 94, +2 new), npm run check clean.
Round 2 of steelman loop on PR #6. Two findings addressed: HIGH — `legacy-cli-imports.test.ts` had two tests, but the second ("exposes the SDK-derived runtime helpers as functions") re-imported the SDK independently and checked `sdk.L1` directly. That tests the SDK in isolation, not the legacy-cli module under review — if legacy-cli destructured from the wrong path while the SDK was fine, the second test would still pass. False sense of two coverage vectors where only one (test 1's dynamic import) actually exists. The dynamic `await import('./legacy-cli.js')` in test 1 fully subsumes the intent: any L1 destructure failure throws at module evaluation time and Vitest reports a clean error pointing at the bad symbol. Deleted test 2 and updated test 1's docstring to explain why no further checks are needed. LOW — QoL comment in legacy-cli.ts about Invoice* type extraction had no issue-tracker reference. Added a TODO line linking the intent to a future sphere-sdk re-export request, so the comment prompts action when the SDK changes rather than being silently forgotten. Verified: 95/95 tests pass (was 96; -1 from deleted redundant test), lint+typecheck clean.
3 tasks
vrogojin
pushed a commit
that referenced
this pull request
May 4, 2026
…, 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.
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
sphere --helpcurrently exits withSyntaxError: '@unicitylabs/sphere-sdk' does not provide an export named 'decrypt'(and a follow-up forgenerateAddressFromMasterKey).npm run typecheckalso fails with 5 TS2459 errors on Invoice* types. Both are upstream-import drift after the latest sphere-sdk refactor.Root cause
encrypt,decrypt,hexToWIF,generatePrivateKey,generateAddressFromMasterKeyare no longer top-level exports of@unicitylabs/sphere-sdk— they live in the L1 (alpha-chain) namespace. The SDK still exposes them at runtime viaimport { L1 } from '@unicitylabs/sphere-sdk'.CreateInvoiceRequest,InvoiceRequestedAsset,GetInvoicesOptions,PayInvoiceParams,ReturnPaymentParamsare declared inside the SDK'sAccountingModulebut absent from the package's rootexport {…}block.Fix
src/legacy/legacy-cli.ts— two contained changes:L1once and destructure the five missing runtime helpers at module load. Call sites unchanged.Parameters<>onSphere.prototype.accountingmethod signatures. This couples the local types to whatever the SDK actually exposes — no manual mirroring required, and a future SDK signature change surfaces here as a typecheck error rather than runtime breakage.Diff:
+24 / -11in one file.Verification
Why this PR matters beyond CI
The DM-only host-manager just shipped in agentic-hosting PR #22 (https://github.com/vrogojin/agentic_hosting/pull/22). That branch's e2e-live suite includes a
sphere-cli-roundtrip.e2e-live.test.tsthat shells out tosphere host helpagainst a live host-manager. It currently hasdescribe.skipIf(!probe.ok)becausesphere --helpexits 1 — this PR's fix flips that probe and the test starts passing.Test plan
npm run checkpasses locally on this branch.node bin/sphere.mjs --helpexits 0 and lists subcommands.agentic-hosting/refactor/delete-ahctl, runnpm run test:e2e-live— the previously-skippedsphere-cli-roundtriptest should now execute (will need a built sphere-cli onSPHERE_CLI_BINpath).Out of scope
no-consoledirectives etc.) and unrelated to this fix.volume-total→volume-maxinsrc/trader/trader-commands.tswas deliberately NOT included — that's a separate change.