feat(trader)(sphere-sdk#474): roundtrip soak + demo playbook + spec drift audit + #473 investigation - #475
Merged
Merged
Conversation
…rift audit + #473 investigation Closes the deliverables side of #474 -- the autonomous trader-agent roundtrip soak and presenter playbook. Implementation pieces (trader service, sphere-cli, agentic-hosting templates, escrow service) already exist; this commit wires them into a soak the operator can run and a demo a presenter can deliver. Deliverables: - manual-test-trader-roundtrip.sh -- 12-section soak that creates alice + bob controller wallets, faucets them asymmetrically, spawns two trader tenants via the host manager, posts matching SELL/BUY intents on the (UCT, ETH) pair with rate band [0.08, 0.12] ETH/UCT, waits for autonomous negotiation and settlement, and asserts net deltas inside the band. Uses human-friendly floats as the CLI surface (per project owner UX guidance) with an inline bigint-shim fallback for the current trader-cli surface. HOST_MANAGER (or SPHERE_HOST_MANAGER) is mandatory. - docs/DEMO-PLAYBOOK-TRADER-ROUNDTRIP.md -- ~25 minute presenter narrative mirroring the swap playbook skeleton. Highlights the "controllers go quiet; agents negotiate" beat in sections 6 and 7. Includes a pre-flight CLI form check + bigint fallback table for pre-#474-CLI builds. - docs/uxf/PROTOCOL-SPEC-DRIFT-474.md -- audit of trader-service/docs/protocol-spec.md vs trader-service implementation. 21 findings, 5 follow-up sub-issues drafted inline. Load-bearing findings: D1 (rate/volume types: float-in-spec vs bigint-in-code), D2 (NP envelope signature input formula), D3/D4 (deal_id derivation and DealTerms omit 4 fields including proposer_direction -- enables flip attacks), D-NEW (CLI should accept human-friendly floats per owner UX guidance). - docs/uxf/ISSUE-473-INVESTIGATION.md -- root cause + smallest-fix proposal for sphere-sdk#473. M1 is BLOCKING: the Mux advances the chat-side since cursor with wall-clock-now BEFORE the async handler chain (SwapModule.handleIncomingDM) completes. CLI exits during the soak's 3 s poll loop kill the handler mid-flight; the cursor advance persists; alice's gift-wrap is then permanently filtered out by the relay's since filter on subsequent boots. Smallest fix: defer updateLastDmEventTimestamp until after dispatch resolves, and double the look-back buffer at subscription time. No source change applied here; #473 fix ships separately on branch fix/473-since-cursor. Known limitations baked into the soak header: 1. #473 cross-process DM flakiness -- mitigated via with_retry + tenant cursor priming; real fix lands in sibling PR. 2. CLI float-vs-bigint UX -- soak writes float form (post-fix UX) with a bigint shim fallback. CLI fix is a sphere-cli follow-up. 3. trader-agent:v0.1 image staleness -- predates #456 / #457 / #464 / #465. Image rebuild is a follow-up. No SDK source files were modified. Related: - Closes sphere-sdk#474 (deliverables side; G1 testnet walkthrough is operator-only and tracked in the audit's recommendations) - sphere-sdk#437 (swap-roundtrip sibling, closed) - sphere-sdk#456 (escrow nametag rotation, closed) - sphere-sdk#473 (cross-process DM flakiness, sibling PR)
Contributor
Author
|
Status update — soak is paused on three prerequisites. The G1 testnet walkthrough (operator-only acceptance criterion in this PR) is blocked on:
The four artifacts in this PR (soak, playbook, audit, investigation) remain accurate as design docs for the post-prerequisite shape; nothing here needs revising. PR can stay open until the prerequisites land, OR merge as-is with the soak documented as "awaiting wrapper + image" — operator's call. No code changes pending in this PR. |
…rn (sphere trader spawn/stop)
The previous revision of `manual-test-trader-roundtrip.sh` assumed a
single shared Host Manager that both alice and bob spawned trader
tenants against, addressed via `HOST_MANAGER` / `SPHERE_HOST_MANAGER`
env vars and `sphere host spawn --manager $HOST_MANAGER`. That model
has two structural problems:
1. Auth-model collision. The public HM whitelists exactly one
controller pubkey; the soak creates two fresh wallets per run,
neither of which is on the whitelist, so spawn would fail at the
ACP-authorization step.
2. Per project owner guidance (memory: per-user local-HM design),
each developer runs their OWN local HM scoped to their controller
pubkey. The public HM is reserved for shared services (escrow,
faucet). The shared-HM pattern was never the intended model.
This commit reshapes the soak around the new `sphere trader spawn` /
`sphere trader stop` wrapper that landed in
unicity-sphere/sphere-cli#49. The wrapper brings up a per-user local
HM container scoped to the active wallet's controller pubkey + spawns
the trader tenant in one command. Each peer (alice, bob) gets its
own HM; no shared infra dependency at the HM layer.
Changes:
- §3 now calls `sphere trader spawn --name <slug> --trusted-escrows
$ESCROW --json` per peer instead of `sphere host spawn ... --env
UNICITY_CONTROLLER_PUBKEY=$ALICE_PUBKEY ...`.
- §4 simplifies to a single ACP smoke probe (`sphere trader
portfolio`); the wrapper's own `--ready-timeout-ms` covers the
container-RUNNING wait that `sphere host inspect` previously did.
- §11 cleanup switches to `sphere trader stop --name <slug>
[--keep-hm]`. KEEP_TENANTS=1 forwards `--keep-hm` so the per-user
HMs stay running for inspection; the wrapper auto-tears down the
HM when the last tenant attached to it stops.
- TRADER_DEAL_DEADLINE_S default bumped 600 -> 900s to cover
per-user HM bootstrap (two-shot drift-guard restart) on top of
the trader scan interval.
- Env contract: HOST_MANAGER / SPHERE_HOST_MANAGER / TRADER_TEMPLATE_ID
/ TRADER_IMAGE_OVERRIDE / UNICITY_CONTROLLER_PUBKEY all removed.
New env contract documents the sphere-cli#49 prerequisite + docker
requirement.
- KNOWN LIMITATIONS: dropped controller-auth caveat and the host-
CLI-no-image-flag caveat. Image-staleness caveat now points at
vrogojin/agentic_hosting#26 (trader-agent:v0.2 rebuild).
DEMO-PLAYBOOK-TRADER-ROUNDTRIP.md §3 narrative rewritten as "each peer
spins up its own local trader tenant" (stronger demo, and accurate).
§7.5 sidebar switches from `sphere host cmd TAIL_LOGS` to direct
`docker logs -f sphere-trader-<wallet>-<name>` since the per-user HM
exposes the container locally. Pre-flight checklist swaps
`sphere host list` for `docker info`. Cheat sheet + command quick
reference + exit-codes table all updated.
End-to-end 3-of-3 verification is out of scope for this commit:
gated on sphere-cli#49 wrapper merging into sphere-cli main and on
the trader-agent:v0.2 image landing (vrogojin/agentic_hosting#26).
Contributor
Author
Per-user local-HM reshape landed (commit a66b63f)Pushed the reshape work for sphere-sdk#477. The "paused" status referenced in my earlier comment is addressed for the soak-script-side of the work — the wrapper prerequisite ( What this commit does (PR #475 picks it up)
Remaining prerequisites for end-to-end 3-of-3
Tracking issue: sphere-sdk#477. |
vrogojin
added a commit
to vrogojin/trader-service
that referenced
this pull request
Jun 10, 2026
…file: install (#24) Stage-1 of the trader image build uses `npm ci` (lenient — installs from lockfile, doesn't strict-validate the file: dep's own package.json). Stage-2 uses `npm install` (strict — re-resolves the file: dep when the path changes via sed), which validates every encountered version against semver and refuses anything malformed. Upstream sphere-sdk main currently carries `"version": "0.0.a1"` (commit b8b526d, "chore(release): 0.0.a1") — `a1` is not a valid semver prerelease tag (must be `[0-9A-Za-z-]+` and the form here `0.0.a1` parses as `MAJOR.MINOR.PATCH` where `a1` isn't a number). The release-driven build of `trader:v0.2` failed at this exact line with `Invalid Version: 0.0.a1` (run 27281190676). Fix: insert `npm pkg set version=0.0.0-dev --prefix sphere-sdk` between the file: path rewrite and `npm install`. The version field on a file: dep is informational only — npm resolves by path, not by version constraint — so this rewrite has zero functional effect beyond letting the install proceed. Robust against any future invalid-semver drift upstream. A separate upstream PR to sphere-sdk main is appropriate to actually fix the version field there. That's not in this PR's critical path; this Dockerfile patch is enough to unblock the v0.2 image build. Related: vrogojin/agentic_hosting#26 (the rebuild this unblocks), unicity-sphere/sphere-sdk#475 (the soak that consumes v0.2 once it's on ghcr.io)
This was referenced Jun 10, 2026
vrogojin
added a commit
that referenced
this pull request
Jun 10, 2026
MarketModule's PostIntentRequest.price, SearchIntentResult.price, and
SearchFilters.{min,max}Price were typed as `number` — inconsistent with
MarketIntent.price (already `string`) and inconsistent with the SDK's
established bigint-serialization convention (TXF amount fields, transfer
payloads, token amounts everywhere else: bigint internally, decimal-string
on the wire).
This bites real callers. The trader-service intent engine
(trader-service/src/trader/intent-engine.ts:908) takes its bigint
midpoint rate (rate_min + rate_max) / 2n in 18-decimal smallest units and
casts to `Number(...)` before passing to MarketModule.postIntent. For
the trader-roundtrip soak's default 0.08-0.12 ETH/UCT band the midpoint
is 1e17 = 100_000_000_000_000_000, well past Number.MAX_SAFE_INTEGER
(2^53 ≈ 9.007e15). JavaScript Number stores 1e17 as a close-but-not-exact
double, and the market-api server responds with HTTP 500 — non-JSON, so
the trader logs only the status code with no diagnostic.
Fix:
- PostIntentRequest.price number → string
- SearchIntentResult.price number → string
- SearchFilters.{minPrice,maxPrice} number → string
The wire serialization in toSnakeCaseIntent / toSnakeCaseFilters is a
direct pass-through, so changing the input type changes the wire shape
from JSON number to JSON string without any new conversion logic.
MarketIntent.price was already `string`, confirming the server's read
shape uses strings — the server should also accept strings on the write
side without server changes (this is the existing convention for
TIP-0 / MarketModule round-tripping).
Tests:
- Existing test inputs `price: 100`, `price: 99.99`, `minPrice: 10`,
`maxPrice: 200` migrated to string form (`'100'`, `'99990000000000000000'`
i.e. 99.99 in 18-decimal smallest units, `'10'`, `'200'`).
- New precision-preservation test demonstrates the failure mode:
'100000000000000000' (10^17) round-trips through JSON without loss,
proving the string convention is what trader-service should use.
Surfaced by: #475 (trader-roundtrip soak)
end-to-end run §6 — market-api HTTP 500 on every attempt to post the
trader's intent.
Coordinating: trader-service/src/trader/intent-engine.ts will need a
follow-up to drop the `Number(...)` cast and pass `.toString()` instead.
vrogojin
added a commit
to vrogojin/trader-service
that referenced
this pull request
Jun 11, 2026
) * fix(market): pass intent price as decimal-string bigint, not Number intent-engine.ts:908 computed the bigint midpoint of the trader's rate band and cast to Number before calling MarketModule.postIntent. For a typical 18-decimal quote asset, the midpoint of a realistic rate band (e.g. 0.08–0.12 ETH/UCT) is 1e17 = 100_000_000_000_000_000 — past Number.MAX_SAFE_INTEGER (2^53 ≈ 9e15). The IEEE 754 double stored a close-but-not-exact value, JSON serialised it as a number, and the market-api server rejected with HTTP 500 (non-JSON body, so the trader only logged the opaque status code with no actionable diagnostic). Fix: - intent-engine.ts: drop `Number(...)` cast; pass `midpointRate.toString()` - types.ts MarketPostRequest.price: number → string - types.ts MarketSearchResult.price: number → string - types.ts MarketSearchFilters.{min,max}Price: number → string Aligns with sphere-sdk PR #483 which makes the same change on the SDK side (PostIntentRequest.price, SearchIntentResult.price, SearchFilters.{min,max}Price all become `string`). This is the established bigint serialisation convention everywhere else in the SDK (TXF amount fields, transfer payloads, all token amounts) — bigint internally, decimal-string on the wire. Surfaced by: unicity-sphere/sphere-sdk#475 (trader-roundtrip soak) §6 which failed with `HTTP 500 — unexpected response (not JSON)` on two consecutive runs. Tests: e2e/trader-intent-lifecycle.e2e.test.ts T1.1 expectation updated from `postCall.price === 475` to `postCall.price === '475'`. 698/698 test suite passes. The market-api server SHOULD accept string prices (MarketIntent.price in the SDK was ALREADY `string`, so the read shape uses strings — the server clearly round-trips strings on reads). If a server-side change is also needed, that's a follow-up; this is the client-side correctness fix. Related: sphere-sdk PR #483 (SDK type alignment). * ci: bump SPHERE_SDK_SHA to b2fd028 (price-as-string fix) The price-as-string types from sphere-sdk PR #483 land at b2fd028; this PR's intent-engine + types.ts changes need that SDK build to typecheck. Both CI and docker-publish workflow pins moved in lockstep so the next v0.3 release tag builds against the same SHA.
Merged
4 tasks
vrogojin
added a commit
to vrogojin/trader-service
that referenced
this pull request
Jun 11, 2026
) The market-api at market-api.unicity.network is a semantic-search database — counterparties discover us by description matching. The trader was sending a redundant structured `price` field at the top of the request body, and the deployed server hit an unhandled exception on it (HTTP 500 with HTML body, swallowed by MarketModule's parseResponse to an opaque "unexpected response (not JSON)"). The trader's own counterparty-discovery already parses the description (parseDescription extracts rate_min/rate_max/volume_min/volume_max fields per TIP-0), so emitting a separate single-point price was useless even when the server accepted it. Successful posts in the live feed (@alphasentinel0X, @gulungtikar990, @w025ixd) all omit the field. Changes: - intent-engine.ts:904: drop the `price: midpointRate.toString()` argument; the description carries the full rate band. - intent-engine.ts:896: drop the now-unused `midpointRate` calc. - types.ts MarketPostRequest.price: required → optional, with a doc block explaining when (not) to use it. Tests: - e2e/trader-intent-lifecycle.e2e.test.ts T1.1: assertion changed from `postCall.price === '475'` to `postCall.price === undefined`. Description assertions unchanged — the rate band is still asserted to round-trip through encodeDescription. - 698/698 test suite passes. Surfaced by: unicity-sphere/sphere-sdk#475 (trader-roundtrip soak) §6. Final root cause from a chain of three: (1) Number() precision loss fixed in PR #25, (2) types out of step with SDK's bigint-string convention fixed via sphere-sdk PR #483 + this PR's earlier commits, and (3) the actual server-side schema mismatch — the field shouldn't have been there at all. This commit ships the (3) fix. Related: sphere-sdk PR #483 (SDK side type changes — already merged).
4 tasks
vrogojin
added a commit
to vrogojin/trader-service
that referenced
this pull request
Jun 12, 2026
…hout) The trader was serialising rate_min/rate_max/volume_min/volume_max as bigint smallest-units in the description and search query. For 18-decimal quote assets like ETH the rate 0.08-0.12 became the string "80000000000000000-120000000000000000", which the semantic-search engine on the deployed market-api server choked on (large-number-dash-large-number patterns return HTTP 500 with HTML body). It was never tested live — the trader's e2e suite mocks MarketAdapter so the bigint values never round-tripped through the real server. Existing test fixtures (e.g. rate_min: '450', volume_max: '1000') and internal math (rate × volume = 475000) reveal the original design: rates are dimensionless ratios, volumes are in BASE whole units. The bigint typing came from a later misinterpretation that conflated them with token amounts. This change makes that explicit: * TradingIntent.rate_min/rate_max/volume_min/volume_max/volume_filled bigint → string (decimal strings, e.g. "0.08", "50") * DealTerms.rate, DealTerms.volume bigint → string * parseDescription regex: \d+ → \d+(?:\.\d+)? to accept decimals * validateIntentParams / validateDealTerms: accept decimal strings, compare via Number (2^53 ceiling is plenty for trading ratios + volumes) * MAX_RATE / MAX_VOLUME: 2^128 → Number.MAX_SAFE_INTEGER * intent-engine matching, fan-out volume allocation, midpoint calc: switched to Number arithmetic * PaymentsAdapter gets `getDecimals(coinId)` backed by sphere-sdk's TokenRegistry.getTokenDecimals (the one place we DO need decimals — at the smallest-unit boundary) * trader-main.ts onDealAccepted: converts whole-unit volume × rate to smallest-units bigint via toSmallestUnitsBigInt() at the wallet reservation boundary * swap-executor.ts buildSwapDealInput: optional getDecimals lookup converts whole-unit terms to smallest-units integer strings before calling sphere.swap.proposeSwap (which requires positive-integer-string per SwapModule.ts:1186) * Tests sweep: 56 sites of `123n` → `'123'` across intent/deal/match assertions; ledger amount assertions (getAvailable) left as bigint since that side IS smallest-units The four-stop fix chain that landed today on the soak's §6 was: 1. trader-service #25 — drop Number() cast (precision) 2. sphere-sdk #483 — align SDK types to bigint-string 3. trader-service #26 — drop structured price field (server rejected it) 4. this PR — rate/volume in human-units throughout (server rejected the bigint-in-text-query too) Verified: - npx tsc --noEmit: clean - npm test: 698/698 pass (added Number arithmetic tests on top of existing string-fixture coverage) - npm run build: tsup clean Surfaced by: unicity-sphere/sphere-sdk#475 §6/§8 (trader-roundtrip soak) which produced HTTP 500 against /api/intents and /api/search with the bigint-in-text formats.
vrogojin
added a commit
to vrogojin/trader-service
that referenced
this pull request
Jun 12, 2026
…hout) (#27) The trader was serialising rate_min/rate_max/volume_min/volume_max as bigint smallest-units in the description and search query. For 18-decimal quote assets like ETH the rate 0.08-0.12 became the string "80000000000000000-120000000000000000", which the semantic-search engine on the deployed market-api server choked on (large-number-dash-large-number patterns return HTTP 500 with HTML body). It was never tested live — the trader's e2e suite mocks MarketAdapter so the bigint values never round-tripped through the real server. Existing test fixtures (e.g. rate_min: '450', volume_max: '1000') and internal math (rate × volume = 475000) reveal the original design: rates are dimensionless ratios, volumes are in BASE whole units. The bigint typing came from a later misinterpretation that conflated them with token amounts. This change makes that explicit: * TradingIntent.rate_min/rate_max/volume_min/volume_max/volume_filled bigint → string (decimal strings, e.g. "0.08", "50") * DealTerms.rate, DealTerms.volume bigint → string * parseDescription regex: \d+ → \d+(?:\.\d+)? to accept decimals * validateIntentParams / validateDealTerms: accept decimal strings, compare via Number (2^53 ceiling is plenty for trading ratios + volumes) * MAX_RATE / MAX_VOLUME: 2^128 → Number.MAX_SAFE_INTEGER * intent-engine matching, fan-out volume allocation, midpoint calc: switched to Number arithmetic * PaymentsAdapter gets `getDecimals(coinId)` backed by sphere-sdk's TokenRegistry.getTokenDecimals (the one place we DO need decimals — at the smallest-unit boundary) * trader-main.ts onDealAccepted: converts whole-unit volume × rate to smallest-units bigint via toSmallestUnitsBigInt() at the wallet reservation boundary * swap-executor.ts buildSwapDealInput: optional getDecimals lookup converts whole-unit terms to smallest-units integer strings before calling sphere.swap.proposeSwap (which requires positive-integer-string per SwapModule.ts:1186) * Tests sweep: 56 sites of `123n` → `'123'` across intent/deal/match assertions; ledger amount assertions (getAvailable) left as bigint since that side IS smallest-units The four-stop fix chain that landed today on the soak's §6 was: 1. trader-service #25 — drop Number() cast (precision) 2. sphere-sdk #483 — align SDK types to bigint-string 3. trader-service #26 — drop structured price field (server rejected it) 4. this PR — rate/volume in human-units throughout (server rejected the bigint-in-text-query too) Verified: - npx tsc --noEmit: clean - npm test: 698/698 pass (added Number arithmetic tests on top of existing string-fixture coverage) - npm run build: tsup clean Surfaced by: unicity-sphere/sphere-sdk#475 §6/§8 (trader-roundtrip soak) which produced HTTP 500 against /api/intents and /api/search with the bigint-in-text formats.
vrogojin
added a commit
to vrogojin/trader-service
that referenced
this pull request
Jun 13, 2026
…ler (#28) PR #27 changed TradingIntent.rate_min/rate_max/volume_min/volume_max from bigint to string but missed the trader-command-handler.ts gate at the wire boundary. The handler still ran safeParseBigint() first, which rejected '0.08' with "rate_min must be a non-negative integer string". The shape downstream is decimal string everywhere (validateIntentParams, TradingIntent record, encodeDescription) — the handler just needs to assert the input IS a string and pass it through. Detailed validation remains in validateIntentParams (called immediately below). Verified end-to-end in unicity-sphere/sphere-sdk#475 trader-roundtrip soak: alice's `sphere trader create-intent --rate-min 0.08 ...` now succeeds at the trader gate (previously failed before reaching the intent engine). 698 / 698 tests pass.
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
manual-test-trader-roundtrip.sh(~1220 lines) + companiondocs/DEMO-PLAYBOOK-TRADER-ROUNDTRIP.md(~660 lines) — fourth member of the soak family alongside transfer / accounting / swap / full-recovery. 12-section autonomous-trader roundtrip on testnet with deltas asserted inside the rate band.docs/uxf/PROTOCOL-SPEC-DRIFT-474.md(~530 lines) — audit oftrader-service/docs/protocol-spec.mdvs implementation. 21 findings, 5 follow-up sub-issues drafted inline.docs/uxf/ISSUE-473-INVESTIGATION.md(~600 lines) — root cause + smallest-fix proposal for Nostr DM delivery flakiness across CLI process boundaries (intermittent receiver miss) #473 (cross-process Nostr DM flakiness). Transport edit ships in sibling PR onfix/473-since-cursor.Scenario (verbatim from #474)
What's deliberately NOT in this PR
fix/473-since-cursor(sibling PR) so the regression risk of the transport edit is reviewed separately.trader-service. Sub-issues are drafted inline in the audit document — I'll file them after this PR is merged.Known limitations baked into the soak header
with_retry(3 × 5 s back-off) on every controller→tenant call and await_for_tenant_runningwarm-up that primes each tenant'ssincecursor before load-bearing calls. The real fix is a sibling PR.float_to_bigintviapython3) when the deployed CLI rejects floats withINVALID_PARAM. The CLI fix is a sphere-cli follow-up.Test plan
bash -n manual-test-trader-roundtrip.sh— syntax clean.HOST_MANAGER=@<hostmgr-test> bash manual-test-trader-roundtrip.shexits 0 withALL GREEN — trader round-trip succeededagainst testnet.roundtrip-391,accounting-roundtrip,swap-roundtrip,full-recovery) +simple-sendstill pass — no regression risk since no SDK source changed.Related
docs/uxf/ISSUE-473-INVESTIGATION.md; fix infix/473-since-cursor)create-intentshould accept human-friendly floats and convert via token-registry decimals (D-NEW from the audit)