swapwallet: harden credit activity projection - #954
Conversation
|
@claude review Please review this two-commit follow-up to #950 with sputn1ck’s five post-merge findings as the acceptance criteria. Commit one makes activity projection monotonic and rich: terminal state cannot regress to PENDING even under concurrent writers; sparse lifecycle updates merge immutable context before both persistence and emission; request-only enrichment is compared semantically; and live plus resumable events carry the effective stored snapshot. Commit two makes credit ownership explicit and durable: a CREDIT settlement is suppressed only when a wallet-local credit operation owns its payment hash, ownership is reconstructed after restart, and terminal polling advances its memo only after a successful durable write. Please focus on concurrent state ordering, whether public StartPay CREDIT activity remains visible, effective event snapshot parity, and retry behavior after a transient ActivityStore failure. Formatting, SQLC generation, commit-message lint, focused db/swapwallet tests, and all 71 configured linters pass locally. |
|
Claude finished @bhandras's task in 4m 9s —— View job Review — PR #954: swapwallet: harden credit activity projection
I reviewed both commits against sputn1ck's five findings, with the four focus areas as acceptance criteria. Overall this is a solid, well-tested follow-up — the monotonic guard, the effective-entry merge, and the durable-retry gating all land cleanly, and the new tests exercise exactly the races described. Below are the per-area verdicts plus a couple of notes worth confirming before merge. 1. Concurrent state ordering (monotonic terminal) ✅The guard is correctly implemented as a double barrier:
One design note (not a bug): the guard only blocks regression to PENDING; terminal→terminal transitions (e.g. COMPLETE→FAILED) still pass. That matches the stated finding scope, but if genuine terminal monotonicity is intended, it isn't enforced here. 2. Public StartPay CREDIT visibility ✅Ownership is now explicit: 3. Effective snapshot parity ✅
4. Retry after transient ActivityStore failure ✅
Notes worth confirminga) Restart ownership-reconstruction window (behavior change worth a second look). It is self-healing: the monotonic guard keeps a terminal row terminal, and the next credit poll corrects the amount (a mutable field b) Request-only enrichment appends a same-status event. c) Minor — None of the notes are blocking. The core correctness claims all hold, generated sqlc matches the query change, and test coverage maps directly onto the four focus areas. Note (a) is the one I'd most want the author's confirmation on, since it's a genuine behavioral trade vs. #950. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances the activity store and credit projector to prevent stale pending updates from overwriting terminal states and to preserve immutable request context across sparse projections. Key changes include updating the SQL upsert query to return affected rows and enforce state transitions, introducing semantic JSON comparison for request payloads, and tracking locally owned credit swaps within the runtime. Feedback on the changes suggests optimizing the semantic JSON comparison helper by adding a fast-path check for identical strings to avoid unnecessary unmarshaling and deep equality checks.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| func jsonValueChanges(next, stored string) bool { | ||
| if next == "" { | ||
| return false | ||
| } |
There was a problem hiding this comment.
An identical next and stored string is guaranteed to be semantically equal. Adding a fast-path check next == stored avoids the overhead of calling json.Unmarshal twice and performing a deep reflection comparison (reflect.DeepEqual) for unchanged JSON values, which is a very common case during re-projections.
| func jsonValueChanges(next, stored string) bool { | |
| if next == "" { | |
| return false | |
| } | |
| func jsonValueChanges(next, stored string) bool { | |
| if next == "" || next == stored { | |
| return false | |
| } |
There was a problem hiding this comment.
Folded into the first commit. jsonValueChanges now returns immediately when the incoming and stored strings are identical, avoiding both unmarshals and reflection for the common unchanged re-projection path. The semantic comparison remains for differently encoded but equivalent JSON.
8a448df to
92467d4
Compare
|
@claude review I folded both follow-ups into their logical commits and force-pushed the two-commit branch. The restart-window note is now closed synchronously: resumeAll reconstructs credit-only ownership from the durable registry before activity backfill runs and before start launches the live monitor. TestResumeAllRestoresCreditOwnershipBeforeMonitor verifies a CREDIT summary is owned immediately after the wallet-ready resume phase, including when no swap backend is configured. The ordinary async poll still reconstructs and projects as before. Gemini’s identical-JSON fast path is also folded into the persistence commit and answered inline. Repeated-status request enrichment remains intentional: activity events are canonical-entry upserts, not counters, and immutable context becoming available is an observable snapshot change. Terminal-to-terminal updates also remain permitted deliberately because a wallet timeout overlay may later reconcile to the source operation’s authoritative completion. Focused db and tagged swapwallet tests, formatting, commit-message lint, and diff checks pass on rewritten head 92467d4. Please re-review this head, especially startup ordering and the synchronous ownership restore. |
|
Claude finished @bhandras's task in 3m 7s —— View job Re-review — PR #954: swapwallet: harden credit activity projection
Re-reviewed the two-commit head Restart-window fix — closed synchronously ✅The ordering is exactly right.
Placing the restore before the Gemini fast-path ✅
Four focus areas — still hold ✅
Generated sqlc ( Non-blocking notesa) b) Repeated-status request enrichment — Confirmed intentional per your note (activity events are canonical-entry upserts, not counters; immutable context becoming available is an observable snapshot change). No action. c) Minor doc wrap — The Nothing blocking. The startup ordering and synchronous ownership restore you asked me to focus on are correct, and the prior review's one substantive concern (the restart window) is now closed with a matching test. LGTM. |
A fast credit child can project COMPLETE before the router writes its initial PENDING row. The activity upsert previously accepted that stale write and appended a backward event, leaving the wallet permanently pending even though the source operation had completed. Reject terminal-to-pending updates in the SQL upsert and suppress their events. Compare request JSON semantically so an otherwise unchanged row can still gain immutable invoice context. Read and merge the current row before projecting a sparse lifecycle update. Live and replayed events now contain the effective memo, request, and correlation fields instead of the sparse terminal payload.
Settlement type identifies how a swap settled, but it does not prove that the wallet credit registry owns its activity row. A caller can use the public swap service directly and create a CREDIT summary without a matching local credit operation. Track payment hashes admitted through the local registry and rebuild the set from durable credit operations. The monitor and history reconciler only defer to the credit projector when both the rail and local owner match. Memoize projected credit state and clear pending tracking only after the activity write succeeds. A transient store failure therefore retries on the next poll instead of permanently hiding a terminal transition.
92467d4 to
da03c39
Compare
|
@claude review Final folded head is
The branch remains two signed logical commits on merged #952. |
|
Claude finished @bhandras's task in 52s —— View job Re-review — PR #954: swapwallet: harden credit activity projection
Re-reviewed head Note (a) —
|
|
CI note: the first Postgres unit job failed only in unchanged I filed the unrelated flake with the exact runner, log excerpt, scope analysis, and diagnostic acceptance criteria as #955, then reran only the failed job. No branch change was made. |
1 participant
Summary
This follow-up addresses every correctness finding from sputn1ck’s post-merge review of #950.
The activity store now treats terminal state as monotonic, emits the same enriched snapshot it persisted, and recognizes request-only enrichment. Credit activity suppression now requires explicit wallet-local ownership instead of relying on the server settlement rail alone, while durable polling retries terminal projection after transient store failures.
Commit structure
activity: Preserve monotonic rich projections
swapwallet: Track credit activity ownership
Races covered
A fast internal settlement may project COMPLETE before the router writes its initial PENDING row. The SQL transition guard now leaves COMPLETE authoritative and does not append a stale event.
A sparse terminal credit projection may race a rich pending receive or pay projection. The projector now merges immutable context before both the current-row upsert and event append, keeping live and replayed events identical.
A terminal store write may fail transiently. Polling now retains the prior memo and pending marker until the durable write succeeds, allowing the unchanged terminal operation to be retried on the next poll.
Validation
All checks pass locally, including all 71 configured linters.
Follow-up to #950.