PR #388 review follow-up: V6-RECOVER ledger consult gaps + soak gate regex misses decimals

[vrogojin]

Follow-up to PR #388 (closes #387). Code review under /code-review --effort high surfaced 15 findings — 3 critical, 3 high, 3 medium, 6 low/cleanup. Soak passed (ALL GREEN, 779 s, all 5 new gates ASSERT OK) but the review identified bypass paths the soak did not exercise and an instrumentation gap that would silently let future pollution slip past the new gate.

PR #388 fixes the original reproduction (integer-amount post-recovery unconfirmed pollution). This issue tracks the gaps the review surfaced.

Critical (consider blocking merge / fix in #388)

1. finalizeReceivedToken overwrites 'invalid' with 'confirmed' from proof-polling callback

modules/payments/PaymentsModule.ts:15475 — the NORMAL finalize path (not finalizeStrandedReceivedToken) builds finalizedToken with status: 'confirmed' and writes to this.tokens with no isV6RecoverPermanentToken check.

Trigger: Cold start — restoreProofPollingJobs (line ~17059) re-registers a finalizeReceivedToken callback for a 'pending' token BEFORE restoreV6RecoverPermanent hydrates the ledger. When the proof arrives via processProofPollingQueue (line ~17463), onProofReceived fires finalizeReceivedToken on a ledgered token, overwriting 'invalid' with 'confirmed'. parsedTokenCache then caches it as spendable.

Fix: Add isV6RecoverPermanentToken guard in finalizeReceivedToken before the status write. OR reorder load() so restoreV6RecoverPermanent runs BEFORE restoreProofPollingJobs.

2. assert_no_unconfirmed_after_finalize regex doesn't match decimal amounts

manual-test-full-recovery.sh:259 — pattern (+ [1-9][0-9]* unconfirmed) requires a leading non-zero ASCII digit. The actual CLI emits decimal amounts (verified in soak: UCT: 100.000000000011 (2 tokens)). A 10-satoshi V6-RECOVER pollution renders as (+ 0.0000001 unconfirmed) and does NOT match.

The gate silently passes on the exact pollution it was designed to catch when amounts are sub-1-UCT. The T9 self-test uses synthetic integer 16 which diverges from real CLI output — false confidence.

Fix:
```bash

Replace grep-based pattern check with numeric awk check:

awk '/(+ / {
match($0, /(+ [0-9.]+/);
n = substr($0, RSTART+3, RLENGTH-3) + 0;
if (n > 0) exit 1
}' "$snapshot"
```

3. normalize_snapshot sed pattern has the same decimal gap

manual-test-full-recovery.sh:226 — s/ \(\+ [0-9]+ unconfirmed\)// only strips integer amounts. Diff-based gates compare un-normalized lines for decimal pollution, either false-passing (both sides equal) or false-failing (one side synced more).

Fix: Replace [0-9]+ with [0-9.]+ in the unconfirmed substitution.

High

4. addToken parameter reassignment leaves caller's reference unchanged

modules/payments/PaymentsModule.ts:10656 — token = { ...token, status: 'invalid', updatedAt: Date.now() } rebinds the local but does NOT mutate the caller's object.

Trigger: Caller pattern const incoming = ...; await module.addToken(incoming); if (added) emitEvent('transfer:incoming', { tokens: [incoming] }); — the event payload carries the pre-mutation status. AccountingModule's _handleTokenChange and UI listeners receive the wrong status.

Fix: Mutate in place — token.status = 'invalid'; token.updatedAt = Date.now(); — so the caller's reference observes the patched status.

5. updateToken bypasses ledger check

modules/payments/PaymentsModule.ts:10717 — directly does this.tokens.set(token.id, token) with caller-supplied status. A finalization worker or future internal caller can overwrite 'invalid' with 'confirmed'. parsedTokenCache then caches it as spendable.

Fix: Mirror addToken: if (this.isV6RecoverPermanentToken(token)) { token.status = 'invalid'; token.updatedAt = Date.now(); }.

6. restoreProofPollingJobs re-registers jobs for ledgered tokens

modules/payments/PaymentsModule.ts:~17059 — runs BEFORE restoreV6RecoverPermanent, so the ledger is empty when re-registration happens. The job survives across restart and fires onProofReceived → finalizeReceivedToken on the next proof arrival (companion to finding #1).

Fix: Reorder load() (restoreV6RecoverPermanent before restoreProofPollingJobs) OR skip re-registration when canonical id is in ledger.

Medium

7. validate() iterates ledgered tokens

modules/payments/PaymentsModule.ts:12084 — for (const token of this.tokens.values()) calls oracle.validateToken() per token with no ledger skip. Wasted aggregator round-trips + misleading 'valid' return array (aggregator may report valid=true for a token the wallet permanently rejected).

Fix: if (this.isV6RecoverPermanentToken(token)) { invalid.push(token); continue; }.

8. scheduleResolveUnconfirmed doesn't check ledger

modules/payments/PaymentsModule.ts:8769 — predicate (t) => t.status === 'submitted' || t.status === 'pending' is asymmetric with hasUnconfirmedOrInflight (line 7960) which DOES check the ledger. Permanently-rejected tokens arm the timer during the cold-start race window.

Fix: (t) => (t.status === 'submitted' || t.status === 'pending') && !this.isV6RecoverPermanentToken(t).

9. getTokens() returns raw tokens without ledger filter

modules/payments/PaymentsModule.ts:8292 — AccountingModule.getInvoiceStatus and SwapModule call paths can observe ledgered tokens as 'pending' during the window between loadFromStorageData and applyV6RecoverPermanentInvalidStatus.

Fix: Filter getTokens() output by isV6RecoverPermanentToken OR document that consumers MUST await load() before reading.

Low + cleanup

10. applyV6RecoverPermanentInvalidStatus doesn't skip 'transferring'

modules/payments/PaymentsModule.ts:17276 — early-exit guard is 'invalid' | 'spent' only. A 'transferring' token whose canonical id collides with a ledger entry would be silently patched to 'invalid', aborting an in-flight send.

Fix: Add 'transferring' to the early-exit set.

11. Save failure divergence in V6-RECOVER stamping

modules/payments/PaymentsModule.ts:9911 — saveV6RecoverPermanent thrown error is logged via warn and execution continues. On CLI exit, the verdict is lost; next session re-runs the full V6-RECOVER cycle.

Fix: Emit transfer:operator-alert on save failure, schedule retry, or escalate.

12. Integration test doesn't verify positive case

tests/integration/payments/v6-recover-invalid-status-387.test.ts:354 — 'Non-ledgered token continues to surface' test never asserts the healthy token's confirmedAmount === '7'. An over-broad ledger filter regression that also excludes the healthy token would pass this test.

Fix: Build healthy token with predicate matching wallet's chainPubkey OR use real SDK signers like v6-recover-real-sdk-recovery.test.ts.

13. updatedAt bumped on every sync cycle

modules/payments/PaymentsModule.ts:17279 — token.updatedAt = Date.now() runs every time applyV6RecoverPermanentInvalidStatus patches a token. Because determineTokenStatus resets status to 'pending' on every TXF reload, the patch — and the updatedAt bump — runs on EVERY sync. Field becomes useless as 'last meaningful change' signal; spurious notifyTokenChange cascades downstream.

Fix: Drop the token.updatedAt = Date.now() line — the status flip is the only signal that matters.

14. Diagnostic grep broader than guard

manual-test-full-recovery.sh:263 — diagnostic uses bare 'unconfirmed', guard uses '(+ [1-9][0-9]* unconfirmed)'. Operator output may show irrelevant 'unconfirmed' lines.

Fix: Use the same (widened-for-decimals per #2) pattern in both.

15. Pre-existing #378 ledger entries — verify backward compatibility

The #378 ledger code stamped entries under tokenId = map iteration key, which is the canonical genesis tokenId post-loadFromStorageData. #388 now stamps under canonical-from-sdkData. For all in-practice paths these agree (the verifier REFUTED a UUID-key concern), but a one-time test loading a #378-produced ledger payload would confirm zero migration risk.

Acceptance criteria

• Sphere SDK #1 — Add ledger guard to finalizeReceivedToken, OR reorder load() to hydrate ledger before proof-polling restoration. Regression test that restores a polling job for a ledgered token, fires the proof, and asserts status remains 'invalid'.
• DMs not working #2, Direct Messages #3 — Soak gate + normalize_snapshot widen to match decimal amounts. New self-test T10 using decimal fixture (e.g. 0.0000001).
• Dev #4 — In-place mutation in addToken; regression test that calls addToken and verifies the caller's reference observes 'invalid'.
• Feature/nametag enhancements #5 — Ledger guard in updateToken; regression test.
• fix(storage): separate token storage formats to prevent duplication #6 — Ordering or guard fix in restoreProofPollingJobs; covered by Sphere SDK #1's regression test.
• feat: add getAssets() method for aggregated token balances by coinId #7, Sphere SDK: complete wallet data cleanup in Sphere.clear() #8, Feature/complete wallet wipe #9 — Ledger consult at all public-API surfaces.
• PROXY transfers rejected — nametag token not loaded before processing incoming transfers #10–[Sphere SDK] Add groupchat support #15 — Address per individual fix guidance.

Context

PR #388: #388
Issue #387: #387
Branch: fix/issue-387-v6-recover-invalid-persistence

Soak result (current branch): ALL GREEN, 779 s, 14 ASSERT OK, zero address-mismatch / VerificationError. The original #387 reproduction is fixed; this issue covers gaps in the surrounding architecture surfaced by the review.