fix(payments)(#391): guard checks sourceTokenIds + load-tail SENT reconciliation#392
Merged
Merged
Conversation
7dfff21 to
eaf206c
Compare
3 tasks
Contributor
Author
|
Erratum — commit references to "#393" Commits |
Base automatically changed from
fix/issue-387-v6-recover-invalid-persistence
to
main
June 4, 2026 12:14
…oad-tail SENT reconciliation Two independent fixes for the false-positive "refusing to include token ... already referenced by OUTBOX entry ... (status=delivered-instant)" seen in short-lived CLI usage when a previously-sent recipient token round-trips back to the sender (alice -> bob -> alice -> bob). A. assertNoDuplicateBundleMembership now compares source candidates against each OUTBOX entry's `sourceTokenIds` set, NOT its `tokenIds` (recipient mint-output) set. The pre-fix comparison was a category error: it never caught a real double-spend AND false- positived on legitimate round-trips. The SENT branch is removed in the same edit because `UxfSentLedgerEntry` has no source field — comparing source candidates against SENT's `tokenIds` had the same category-error problem. Pre-H5 OUTBOX entries (no `sourceTokenIds`) are silently skipped, matching the worker's "no recovery target" semantics in types/uxf-outbox.ts:211. B. load() now fires a one-shot SentReconciliationWorker.runScanCycle() tail sweep (fire-and-forget), mirroring the orphan-spending sweep pattern. The reconciliation worker's periodic timer waits one full 60s interval before its first scan, so short-lived processes (CLI invocations, headless scripts) exit before any cycle fires, leaving prior `delivered-instant` OUTBOX entries live indefinitely. The tail sweep closes that gap for short-process consumers; long- running UIs/daemons keep their periodic-timer cadence. Either fix alone breaks the failure mode; both shipped because A corrects the load-bearing invariant and B closes the related forensic- state gap. Tests: - duplicate-bundle-guard.test.ts reworked end-to-end: pins the source- side rejection invariant, adds a #391 round-trip-false-positive regression test, adds a pre-H5 back-compat test, asserts SENT no longer participates. - load-sent-reconciliation.test.ts (new): asserts a pre-existing `delivered-instant` OUTBOX entry with a matching SENT row is tombstoned after load() settles. All 3362 module + payments unit tests pass.
…e guard Adds two pieces of higher-fidelity regression coverage on top of the unit tests landed in 1a59625: 1. `manual-test-roundtrip-391.sh` — bash soak mirroring `manual-test-simple-send.sh` shape. Drives the exact 4-hop CLI flow from the bug report (alice→bob 10, bob→alice 2, alice→bob 91, bob→alice 98.5 UCT) end-to-end against testnet across separate short-lived processes. Each `sphere payments send` is a fresh process, so the OUTBOX-tombstone-via-SentReconciliationWorker timing gap (Fix B) is exercised genuinely. The 4th hop is the critical one: pre-fix it errors with DUPLICATE_BUNDLE_MEMBERSHIP; post-fix it succeeds. Integer-only balance assertions; KEEP=1 workspace preservation; cross-hop scan for the error phrase. 2. `tests/e2e/issue-391-roundtrip.test.ts` — vitest e2e mirroring `uxf-send-receive.test.ts` shape. Drives the same 4-hop scenario in-process via two Sphere instances using USDU (avoids the known UCT/uint64 SMT-path overflow). Gated on RUN_UXF_E2E=1 + NO_TESTNET=1 + the standard infra-probe preflight. Asserts: each hop returns submitted/delivered/completed (not DUPLICATE_BUNDLE_MEMBERSHIP); final balances match net deltas (alice -5 USDU, bob +5 USDU); no transfer:failed events. The unit tests pin the guard contract and load-tail sweep in isolation (deterministic, fast). The soak proves the short-process Fix B path on real testnet. The e2e proves Fix A's guard correctness through the real dispatcher pipeline. Together: full regression surface.
…itch
Adds `AUTOMATED_CID_DELIVERY_ENABLED = false` constant in
`modules/payments/transfer/limits.ts` and gates the auto-promotion
behaviour in three places:
1. `delivery-resolver.ts` `auto` case — bundle exceeds inline cap →
stay inline if <= RELAY_SAFE_CAP_BYTES; throw INLINE_CAR_TOO_LARGE
with a message instructing the caller to use {kind:'force-cid'}
otherwise. The legacy auto→CID code path stays in place behind
the flag for a one-line re-enable.
2. `instant-sender.ts` pre-flight — `wantsCidBranch` collapses to
`strategy.kind === 'force-cid'`. Adds a clean throw for
`auto`+oversized at pre-flight so the operator sees the same
error text whether it surfaces from the dispatcher or the
resolver.
3. `conservative-sender.ts` pre-flight — same change as instant.
`force-cid` and `force-inline` are unaffected. The kill-switch only
neuters the implicit `auto → CID` promotion that fired on bundles
above the (often-small) inline cap.
Why ship this as a kill-switch instead of ripping the code out:
- "for a while" framing in the directive — re-enable expected once
publisher wiring + soak coverage land.
- One constant flip restores the legacy behaviour; tests
auto-resume because they're gated on the same constant.
- The size-promotion path is exercised by 11 unit tests + 1
integration test that would otherwise need extensive rewrites.
Tests:
- 11 unit tests + 1 integration test gated via
`const ifAutoCid = AUTOMATED_CID_DELIVERY_ENABLED ? it : it.skip`
pattern. They skip when the flag is OFF and run when the flag
flips back to ON. No deletions.
- 3 new tests in `delivery-resolver.test.ts` pin the new disabled
behaviour: auto-mode CAR > cap stays inline; auto-mode CAR >
RELAY_SAFE_CAP_BYTES throws INLINE_CAR_TOO_LARGE; force-cid still
works (sanity check).
- One test in `§3.3.1-invalid-inline-cap.test.ts` (the
`inlineCapBytes: 1 (the minimum legal value) → no throw` case)
now branches on the constant — the load-bearing "no throw"
contract still holds; the cid-vs-inline outcome depends on the
flag.
All 8282 unit tests + 81 integration transfer tests + lint + build
pass with the flag OFF.
…oak + e2e After #393 disabled automated CID delivery, the 4-hop round-trip scenario's HOP 4 — where bob bundles 3 source tokens with multi-hop histories — now throws INLINE_CAR_TOO_LARGE at the dispatcher pre-flight instead of trying to publish via IPFS. That throw is EXPECTED post-#393 and is orthogonal to the #391 fix being verified. Both the soak (`manual-test-roundtrip-391.sh`) and the e2e (`tests/e2e/issue-391-roundtrip.test.ts`) are updated to: - Capture the HOP 4 outcome instead of letting it abort. - Hard-fail if the throw is DUPLICATE_BUNDLE_MEMBERSHIP (#391 regression). - Soft-pass if the throw is INLINE_CAR_TOO_LARGE / "automated CID delivery is currently disabled" (#393 documented limit). Log the INFO, skip the receive-poll and balance reconciliation, and exit success — the #391 invariant is verified by the absence of the duplicate-bundle error. - Run the balance reconciliation only when HOP 4 actually delivered (the SUCCESS path). E2E also switched to `transferMode: 'conservative'` to match the working sibling `uxf-send-receive.test.ts` pattern; the instant-mode USDU path has pre-existing UXF orchestrator infrastructure issues (CBOR uint64 overflow on SMT path bignums, sibling header lines 35-58) that are independent of the #391 fix being verified here. CLI soak `manual-test-roundtrip-391.sh` remains the primary verification of the user's actual instant-mode bug repro; the e2e is a deterministic in-process companion focused on the guard invariant.
Three changes:
1. **Root cause of the e2e timeout** — `payments.send({ amount })` takes
the amount in SMALLEST UNITS per the documented API contract
(CLAUDE.md, types/index.ts:73). The #391 e2e was passing
`amount: '100'` thinking it was 100 USDU, but the SDK shipped
exactly 100 smallest-units (= 0.0001 USDU at 6 decimals) and bob's
`expectedReceiveTotal: 100_000_000n` (100 USDU smallest) never
matched. Off by 10^6. Diagnostic logging surfaced this: `bal.confirmed=100`
not `100_000_000` — the send arrived correctly, the test's
expectation was 6 decimal places too large.
Fix: pass amounts as smallest-unit strings ('100000000' for 100 USDU
etc.), matching the value already used in `amountSmallest` and
`expectedReceiveTotal`. Add explicit comment pinning the convention.
2. **Diagnostic logging in `sendHop`** — capture receive() result count,
error string, and balance state on first 5 polls + every 20th. Wire
transfer:incoming and transfer:failed listeners with their own log
lines. Surface the incoming/failed counts in the timeout message.
What used to be a silent 180s wait now shows the actual state
transitions live.
3. **Delete obsolete uint64 warnings** in `uxf-send-receive.test.ts`
header. BOTH documented "UXF orchestrator blockers" are RESOLVED:
- The "CBOR uint64 overflow on SMT path bignums" was fixed by
issue #295 (`uxf/hash.ts:80-83` — SmtPath is now an opaque
STS-canonical CBOR blob; UXF does not touch bignum segments).
- The "symbol → hex coinId resolution missing on UXF dispatch"
was fixed by `PaymentsModule.resolveCoinIdSymbol()`
(PaymentsModule.ts ~line 12592; called from both UXF
dispatchers).
Replaced the stale "Known blockers" block with a brief history
note so future readers know what WAS blocked and has been cleared.
Same cleanup applied to `tests/e2e/issue-391-roundtrip.test.ts`
in-line comment.
4. **transfer:failed filter** — bob's HOP 4 INLINE_CAR_TOO_LARGE
soft-pass legitimately fires `transfer:failed` on the sender side
(the SDK emits the event when the dispatcher throws synchronously).
The test assertion now filters out failures matching the
INLINE_CAR_TOO_LARGE / "automated CID delivery is currently
disabled" patterns while still hard-asserting:
- dupBundleFailures must be empty (#391 invariant — DUPLICATE_BUNDLE_MEMBERSHIP would surface here even if it doesn't reach the synchronous throw path)
- aliceUnexpectedFailures must be empty
- bobUnexpectedFailures (other than the documented soft-pass) must be empty
Verification — RUN_UXF_E2E=1 against testnet:
exit=0, 1/1 test passed, 74 s.
HOP 1 → bob confirmed=100_000_000 (1 token)
HOP 2 → alice confirmed=920_000_000 (2 tokens)
HOP 3 → bob confirmed=990_000_000 (3 tokens, includes round-trip)
HOP 4 → soft-pass (no DUPLICATE_BUNDLE_MEMBERSHIP; #391 verified)
40a155a to
36742e5
Compare
vrogojin
added a commit
that referenced
this pull request
Jun 4, 2026
…0 tokens Pre-existing integration test assumed the old 96 KiB cap; 200 tokens (~140 KiB) exceeded it. Post-#394b the cap is 512 KiB, so the test needs ~800 tokens (~560 KiB) to genuinely cross the ceiling and exercise the INLINE_CAR_TOO_LARGE branch. The boundary test below (50 tokens, ~35 KiB) is unchanged — still well under the new cap. Caught by CI on PR #395; bundle-acquirer flake also fired but passes locally (same flake observed on PR #392's CI run, captured in https://github.com/unicity-sphere/sphere-sdk/runs/26895291755).
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
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.
Fixes #391.
What
Two independent fixes for the false-positive
DUPLICATE_BUNDLE_MEMBERSHIPthrow seen in short-lived CLI usage when a previously-sent recipient token round-trips back to the sender.A.
assertNoDuplicateBundleMembershipnow compares againstsourceTokenIds. The pre-fix comparison againstentry.tokenIdswas a category error —tokenIdsis the RECIPIENT mint-output set; the invariant the guard defends ("don't burn the same source twice") is expressible only against the SOURCE set. SENT branch removed:UxfSentLedgerEntryhas no source field, the prior comparison was the same category error.B.
load()fires a one-shot SENT-reconciliation tail sweep. The periodic worker waits 60s before its first scan; short-lived CLI processes exit before any cycle fires, leavingdelivered-instantOUTBOX entries live indefinitely. The tail sweep closes that gap by tombstoning prior entries whose SENT companion already exists. Mirrors the orphan-spending sweep auto-invocation atPaymentsModule.ts:3782-3810.Why both
Either fix alone breaks the user-visible failure mode:
Both together: correct invariant + clean operational state.
Files
modules/payments/PaymentsModule.ts— guard rework (assertNoDuplicateBundleMembership) and load-tail sweep (after thedetectOrphanSpendingTokens()block atload()tail).tests/unit/modules/payments/duplicate-bundle-guard.test.ts— reworked end-to-end (12 tests). Pins source-side rejection + duplicate-bundle guard false-positives on token round-trips + OUTBOX entry persists indefinitely in short-lived CLI #391 round-trip-false-positive avoidance + pre-H5 back-compat + SENT-no-longer-participates.tests/unit/modules/payments/load-sent-reconciliation.test.ts— new (3 tests). Pins the load-tail tombstone of a pre-existingdelivered-instantentry with matching SENT row.Test plan
npx vitest run tests/unit/modules/payments/duplicate-bundle-guard.test.ts— 12/12 passnpx vitest run tests/unit/modules/payments/load-sent-reconciliation.test.ts— 3/3 passnpx vitest run tests/unit/modules/ tests/unit/payments/— 3362/3362 passnpx tsc --noEmitcleannpm run buildcleannpx eslint modules/payments/PaymentsModule.ts <new test files>— 0 errors (5 pre-existing warnings untouched)Risk
makeOutboxEntry({ sourceTokenIds: ['tok-A'] })→ guard fires on['tok-A']candidate). No behavior change there.load().