fix(cashu): settlement-queue review fixes that missed the #68 merge - #69
Merged
Conversation
…merge #68 was merged from the original commit while the review workflow was still pushing fixes to its branch; none of them landed on main. This carries the branch's full fix content: confirmed-spend-only settlement, queue schema versioning with forward reads, corrupt-blob quarantine, drain re-entrancy/backoff, ambiguous-failure handling, and the recovery (resignWitness) path, with their tests. Content-identical to feat/cashu-offline-settlement tip 5b78692 relative to the merged base. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016MsAGwtS4sNodzWu2VMTKR
…signing oracle The three findings the review's final round left open on #68. - Migration arms now fill only what is absent, never overwrite what is present, and the version doc states that as a contract. A downgraded build re-stamps a newer blob at its own version while preserving the newer fields, so the version tag alone cannot prove a field is missing — an unconditional `mintUrl: LEGACY_MINT_URL` arm would overwrite the real mint with the stand-in on the rollforward, the adapter would fall back to the configured mint, the proof would be rejected as unknown, and owed money booked as lost. (Not fixed by stamping the read version back: a v1 build would then write entries lacking the v2 field under a v2 tag, and the v2 validator drops them as corrupt, which is worse.) - quarantine() now writes each corrupt blob's copy once per process. It runs from every read path and a corrupt blob is only cleared by the next write, so an exposure banner polling pendingExposure() once a second ground the Keychain at two writes per second over byte-identical bytes for as long as the app stayed open. In-process set, keyed by content, so a different corrupt blob still gets its copy; cleared in the relaunch seam so a restart re-verifies the copy. - __signArbitraryForTests is gone. A __ prefix is a naming convention, not an access control, and the export was an oracle for a BIP-340 signature under the card's P2PK identity over any 32 bytes a caller chose. What it existed to test was dead code: the length guard in signArbitrary, which its only caller (resignWitness) can never reach — that guard is deleted too, and stays in spendProof where the message is a real parameter. The wrong-length-signature case moved onto resignWitness, and the only-signer regression test now pins both names. Both behavioural fixes verified by mutation: reverting each makes exactly its test fail. 441 tests, typecheck and lint clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016MsAGwtS4sNodzWu2VMTKR
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.
#68 was merged from its original commit while the review workflow was still pushing fixes to the branch — none of them landed on main. This PR carries all of it, in two commits:
Commit 1 — the review fixes that missed the merge (content-identical to the branch tip)
Confirmed-spend-only settlement, queue schema versioning with forward reads, corrupt-blob quarantine, drain re-entrancy/backoff, ambiguous-failure handling, and the
resignWitnessrecovery path, with their tests. ~3,100 lines including tests.Commit 2 — the final round's three findings
Additive-only migration arms. A downgraded build re-stamps a newer blob at its own version while preserving the newer fields, so the version tag can't prove a field is missing. The old unconditional arm (
mintUrl: LEGACY_MINT_URL) would overwrite a real mint URL with the stand-in on rollforward → adapter falls back to the configured mint → proof rejected as unknown → owed money booked as lost. Arms now fill only absent fields, and the version doc block states that as a contract for the next author.Quarantine write-rate bound.
quarantine()runs from every read path; a corrupt blob is only cleared by the next write; taps may be hours apart. An exposure banner polling once a second ground the Keychain at 2 writes/sec over byte-identical bytes for as long as the app stayed open. Now one write per corrupt blob per process (content-keyed set, cleared on the relaunch seam).The signing oracle is gone.
__signArbitraryForTestsexported "sign any 32 bytes under the card's P2PK identity" — a__prefix is a convention, not access control. What it existed to test was dead code: a length guard its only real caller could never reach. Deleted both; the guard stays inspendProofwhere the message is a real parameter; the only-signer regression test now pins both names.Verification
Both behavioural fixes mutation-tested — reverting each makes exactly its test fail:
Full suite 441/441, typecheck and lint clean.
🤖 Generated with Claude Code
https://claude.ai/code/session_016MsAGwtS4sNodzWu2VMTKR