feat(trader): enable global accounting auto-return on startup - #12
Open
vrogojin wants to merge 1 commit into
Open
feat(trader): enable global accounting auto-return on startup#12vrogojin wants to merge 1 commit into
vrogojin wants to merge 1 commit into
Conversation
vrogojin
added a commit
that referenced
this pull request
May 3, 2026
…er + leak-free worker pool Two critical findings from the steelman review of #10: ## C1: anchor docker --filter name regex with `^` `docker ps --filter name=X` is a SUBSTRING match by default — `name=foo` matches any container whose name CONTAINS "foo", not just those that START with "foo". The PR documented session-prefix as a security-style isolation guarantee, but the unanchored filter could in theory match adjacent-session containers if their session IDs shared leading hex digits. Anchor with `^` (Docker passes the value through to its regexp matcher, so `^prefix` is honored). Verified with two containers `test-prefix-foo` and `xtest-prefix-foo-y`: - without `^`: matches both (substring) - with `^`: matches only the prefix-anchored one ## C2: Promise.allSettled + dispose orphaned tenants on partial failure The previous worker pool used `await Promise.all(workers)`, which rejects on first error. But other in-flight workers continue spawning containers AFTER the function rejects, and those containers never reach `results`, so the caller's `afterAll` never sees them. **Container + /tmp leak, strictly worse than the sequential predecessor.** Fix: each worker catches per-task errors into a shared `errors[]` array and continues draining. After all workers settle, if any errors were recorded, dispose every tenant that DID succeed (they're unreachable to the caller through the rejected promise) before re-throwing the first error. Multiple errors are attached as `.otherErrors` on the primary so they aren't silently swallowed. ## Drive-by: replace top-level `generatePrivateKey` import `@unicitylabs/sphere-sdk` no longer exports `generatePrivateKey` at the package root (moved to the L1 sub-namespace). Replace with an inlined `randomBytes(32).toString('hex')` — a secp256k1 private key is just 32 random bytes, and the probability of generating an invalid value is ~2^-128 (vanishingly small). ## Test plan - 113/113 helper unit tests pass - typecheck clean for the test/ tree (the unrelated `mintFungibleToken` type error in `src/trader/main.ts` belongs to PR #12 and is fixed there) - Docker filter anchor verified empirically with two test containers
vrogojin
added a commit
that referenced
this pull request
May 3, 2026
…Dependencies + README Three review-feedback items from #11: ## W1+W2: validate env-var inputs at preflight startup `Number('abc')` returns NaN; `Number('-1')` returns -1; `Number('0')` returns 0. All of these would propagate to the upstream probe's `setTimeout` and either fire immediately (NaN coerces to 1ms in Node) or never fire — producing misleading "preflight failed" results from a typo. Validate `TRADER_E2E_PREFLIGHT_TIMEOUT_MS` with `Number.isFinite() && > 0` and throw a clean error otherwise. Same hardening for `TRADER_E2E_PREFLIGHT_NETWORK`: was a blind cast to `'testnet' | 'mainnet' | 'dev'`. The upstream `runProbes` would also throw on unknown networks, but tightening locally makes the contract visible at trader-service startup and immune to upstream silent enum extensions. ## W5: move @unicitylabs/infra-probe to devDependencies The probe is only used from `test/e2e-live/` and the `preflight` / `preflight:json` npm scripts. End-users `npm install`ing trader-service as a binary (the `bin: trader-ctl` entrypoint) shouldn't pull in the probe + its transitive deps (@noble/curves, ws). Move to devDependencies; lockfile updated. ## W3: document the four env vars in README A developer hitting "preflight failed" needs to know about `TRADER_E2E_SKIP_PREFLIGHT=1` without grepping the source. README now has an "E2E live tests — preflight gate" section with a table of all four env vars + their defaults + ad-hoc probing instructions. ## Not addressed (deliberately deferred) - C1 (cosmetic): "N service(s) unreachable" log message can over-count when `error` and `unreachable` mix. The gate still fires correctly; the message phrasing is a separate cleanup. - W4 (per-file opt-out): no opt-out mechanism for vitest globalSetup per-file. The `TRADER_E2E_SKIP_PREFLIGHT` escape hatch is the documented workaround. ## Test plan - typecheck clean for the test/ tree (the pre-existing src/trader/main.ts errors belong to PR #12 and are fixed there) - helper unit tests pass; preflight runs and validates env vars correctly - `npm install` re-resolves dep graph after move; no breakage
Call `sphere.accounting.setAutoReturn('*', true)` immediately after
`Sphere.init` so any terminated invoice this wallet is a target of will
have its surplus (`coveredAmount > requestedAmount`) refunded
automatically to each over-paying party at their `refundAddress` ?? `senderAddress`.
Why this matters: the SDK's AccountingModule already tracks per-payer
contributions and emits `invoice:overpayment`, but the actual refund
only fires when auto-return is enabled. With sphere-sdk PR-119 making
`SwapModule.verifyPayout` explicitly fail with `OVER_COVERAGE` on
net > expected, surplus on a trader's payout invoice is detectable AND
must be refunded — this PR wires the refund.
## Review-feedback hardening
**RATE_LIMITED guard** (PR-12 review W2). The SDK throws `RATE_LIMITED`
if `setAutoReturn('*', true)` is called twice within a 5-second cooldown.
Process restart wouldn't normally hit it (cooldown is in-memory only),
but an in-process supervisor that retries `startTrader` on error would.
`RATE_LIMITED` here is functionally a no-op (flag is already true) so we
treat it as success, log distinct event, and continue startup.
**Startup-cost note** (PR-12 review W1). `setAutoReturn('*', true)` is
NOT just a flag flip — when enabled, the SDK iterates
`closedInvoices ∪ cancelledInvoices` (capped at 100) and runs
`_executeAutoReturnFromFrozen` for each, which issues real outbound
payments. Operators should expect the first call after wallet migration
to be slow. Documented in the comment.
## Drive-by
`@unicitylabs/sphere-sdk` no longer exports `generatePrivateKey` at the
package root (moved to the L1 sub-namespace). Replace with inlined
`randomBytes(32).toString('hex')` in `test/e2e-live/helpers/tenant-fixture.ts`.
**The same fix is also in PR #10**; whichever lands first wins, the
other gets a trivial merge. Required for typecheck/tests to pass against
current sphere-sdk.
`mintFungibleToken` was added in sphere-sdk's
`refactor/extract-cli-to-sphere-cli` branch and never landed in main.
Trader-service's `TRADER_TEST_FUND` test-helper code path called it
unconditionally; replace with a guarded shim that throws explicitly when
the method isn't available. The e2e suite uses the faucet path, not
TRADER_TEST_FUND, so this guard never trips in production CI but the
typecheck error blocks docker image build.
## Test plan
- [x] All 671 unit tests pass (was failing before drive-by fix)
- [x] Typecheck + lint clean
- [x] **End-to-end validation** (with sphere-sdk #119 + #120 active):
trader-service e2e-live suite 11/11 passing in 23 min;
multi-agent test went from 26-min OVER_COVERAGE hang → 4.9-min PASS
vrogojin
force-pushed
the
feat/enable-accounting-auto-return
branch
from
May 3, 2026 12:55
cf8d1ce to
e950d76
Compare
vrogojin
added a commit
that referenced
this pull request
May 3, 2026
…tion + dash-aware sanitizer Three review-warning items from #10: ## W4: 32-bit → 64-bit SESSION_ID entropy `randomBytes(4)` = 32 bits. Birthday-bound collision probability at 100 concurrent CI shards: ~1.2e-6 (small but non-zero). At 1000 shards: ~1.2e-4. Bumping to `randomBytes(8)` = 64 bits drops collision probability to ~5e-15 even at thousands of concurrent runs — effectively impossible for any realistic deployment. Cost: 4 additional random bytes. Trivial. ## W5: validate env-var inputs loudly, not silently Two env vars used to silently floor invalid values: - `TRADER_E2E_PROVISION_CONCURRENCY=0` (intent: force sequential, cc=1) used to fall back to DEFAULT_PROVISION_CONCURRENCY=3, surprising the operator. Now throws with a clear "must be >= 1, use cc=1 to force sequential" message. - `VITEST_MAX_FORKS=abc` used to silently fall back to 1 via `|| 1`. Typos slipped through. Now throws with a "must be a positive integer" message. Same treatment for negative/NaN. Both validators preserve the documented defaults when the env var is unset or empty — no behavior change in the common case. ## W6: hex sanitizer respects dashes `TRADER_E2E_SESSION_ID` override was sanitized via `[^0-9a-f]` strip, silently mangling values like `ci-job-1234-abc` (CI driver tag) into `abc`. Tighten to `[^0-9a-z-]` so dashes (which Docker container names allow) are preserved. Length cap raised to 32 chars to match the larger 64-bit auto-generated ID. Non-Docker-safe characters (slashes, colons, spaces) are still stripped to prevent argv injection. ## Drive-by note The pre-existing `mintFungibleToken` typecheck error in `src/trader/main.ts` is fixed by PR #12. Whichever lands first wins; both touch this file. ## Test plan - All 671 helper unit tests pass - Typecheck clean for test/ tree (the mintFungibleToken error is upstream) - VITEST_MAX_FORKS=abc throws "must be a positive integer" - TRADER_E2E_PROVISION_CONCURRENCY=0 throws "must be >= 1" - TRADER_E2E_SESSION_ID=ci-job-1234 round-trips with dashes intact
vrogojin
added a commit
that referenced
this pull request
May 4, 2026
Recovery merge of PR #13 used --theirs for src/trader/main.ts which silently reverted PR #12's setAutoReturn block. Without setAutoReturn('*', true), the trader does NOT refund surplus on terminated invoices — exactly the leak surfaced by negotiation-failures.e2e-live's 'deposit timeout' test (UCT delta=-1000, USDU delta=+500 instead of restored to baseline). Re-applies the block right after sphere_initialized log. Behavior matches PR #12 verbatim: - sphere.accounting.setAutoReturn('*', true) gated on sphere.accounting !== null - RATE_LIMITED treated as success (in-process retry within 5s cooldown — flag already set) - All other errors fail fast (storage layer broken → downstream invoice ops would fail unpredictably anyway) Verified: typecheck clean. Re-running negotiation-failures live test will validate the refund path.
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
Call `sphere.accounting.setAutoReturn('*', true)` immediately after `Sphere.init` so any terminated invoice this wallet is a target of will have its surplus (`coveredAmount > requestedAmount`) refunded automatically to each over-paying party at their `refundAddress ?? senderAddress`.
Why this matters
The SDK's `AccountingModule` already tracks per-payer contributions and emits `invoice:overpayment` when surplus is detected — but the actual refund only fires when auto-return is enabled. With sphere-sdk PR-119 making `SwapModule.verifyPayout` explicitly fail with `OVER_COVERAGE` on net > expected, surplus on a trader's payout invoice is detectable and recoverable — but only if we actually enable auto-return. This wires that final mile.
Why global ('*') over per-invoice
The trader receives many payout invoices over its lifetime (one per completed deal). Setting per-invoice would require us to track every imported payout invoice and call `setAutoReturn(invoiceId, true)` on each — a brittle contract. Global is set once and stays on.
Why fail-fast
If `AccountingModule` can't persist auto-return settings, its storage layer is broken and downstream invoice operations would also fail unpredictably. Better to surface at startup than discover at first deal completion.
Test plan