Skip to content

swapwallet: Sign credit-only sends as outflows - #830

Merged
ellemouton merged 1 commit into
mainfrom
ellemouton/fix-829-credit-pay-activity
Jul 7, 2026
Merged

swapwallet: Sign credit-only sends as outflows#830
ellemouton merged 1 commit into
mainfrom
ellemouton/fix-829-credit-pay-activity

Conversation

@ellemouton

@ellemouton ellemouton commented Jun 30, 2026

Copy link
Copy Markdown
Member

What changed

Credit-only Lightning sends (the sub-dust pay path that cannot be
represented as a normal vHTLC-backed swap) reached the wallet activity
feed with a positive amount, so they rendered as incoming
transfers. This signs them as outflows, matching every other outgoing
row.

Both places that build a credit SEND row copied the operation amount
verbatim, unlike normal swap sends which swapEntryFromSummary already
normalizes to a negative amount:

  • router.go creditPayEntry — the pending row now carries -amountSat;
  • credit_projector.go creditEntryFromSummary — the projected row negates AmountSat for SEND.

Fixes #829.

Scope note

This PR was originally larger. Since it was opened, the #774 canonical
activity-log epic landed on main:

That already fixes the disappearing-row half of #829 (a settled credit
send returning NOT_FOUND on activity inspect): the store is now the
durable source, so the earlier read-time collectCreditEntries backfill
is no longer needed and has been dropped. The credit-op timestamp
plumbing has likewise been dropped as out of scope for #829. What remains
is the sign fix — the one part of the original bug still unaddressed on
main.

Validation

  • go test -tags 'walletdkrpc swapruntime' ./swapwallet ./credit
  • make fmt-changed
  • make lint-changed-local

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request integrates credit operations into the wallet activity history, adding CreatedAtUnix and UpdatedAtUnix timestamps to CreditOpSummary and ensuring outgoing send entries correctly reflect negative amounts. The review feedback is highly constructive, pointing out that uninitialized Go zero-time values can yield negative Unix timestamps. It is recommended to check IsZero() before calling .Unix() in the registry, and to use <= 0 checks instead of == 0 in the credit projector to robustly handle these uninitialized timestamps.

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.

Comment thread credit/registry.go Outdated
Comment thread swapwallet/credit_projector.go Outdated
@ellemouton
ellemouton marked this pull request as ready for review June 30, 2026 19:21
@ellemouton
ellemouton force-pushed the ellemouton/fix-829-credit-pay-activity branch from 3cf42d6 to b9d8c5d Compare June 30, 2026 19:24
@ellemouton

Copy link
Copy Markdown
Member Author

@claude review this

@claude

claude Bot commented Jun 30, 2026

Copy link
Copy Markdown

Claude finished @ellemouton's task in 2m 49s —— View job


Review: swapwallet: Keep credit-only sends in activity

  • Read the diff and changed files
  • Analyze correctness, edge cases, style
  • Assess the gemini review comments
  • Post review feedback

Overall this is a clean, well-targeted fix with good test coverage. The two bug stories (positive-signed SEND amount and the disappearing terminal row) are both real and both addressed, and the negative-amount convention is now consistent between the live router row (router.go:337), the projected row (credit_projector.go:209-211), and the durable List projection. Nice work. A few notes below.


On the Gemini "zero-time .Unix()" comments — not actually reachable here

Both bot comments warn that rec.CreatedAt.Unix() / rec.UpdatedAt.Unix() could return -62135596800 for a Go zero time.Time{}, and recommend <= 0 in the projector. I dug into the store and this scenario cannot occur on this path:

  • The store reads timestamps as CreatedAt: time.Unix(row.CreatedAt, 0) / UpdatedAt: time.Unix(row.UpdatedAt, 0) (db/credit_operation_store.go:353-354). time.Unix(n, 0) is never a Go zero value and never IsZero(), so .Unix() always returns a sane n.
  • On write, the store already defaults a zero CreatedAt to now (db/credit_operation_store.go:163-166), and UpdatedAt is always nowUnix.

So rec.CreatedAt.Unix() here returns a normal positive timestamp (worst case 0 for a hypothetical time.Unix(0,0) legacy row), never a large negative. That means the existing == 0 guards in creditEntryFromSummary (credit_projector.go:188-193) are correct as written. Gemini's <= 0 suggestion is harmless but not necessary, and the IsZero() wrap in registry.go would be dead defensiveness. I'd leave these as-is unless you want belt-and-suspenders.

Things worth confirming (not blockers)

  1. collectCreditEntries now surfaces credit RECVs in List too, not just SENDs. creditEntryFromSummary projects KindReceive (credit_projector.go:168-174), and collectCreditEntries pulls PendingOnly: false, so pending and terminal credit receives will now appear in ListActivity where previously they only reached live subscribers. That looks like a desirable side-effect, but it's broader than the PR title/description (which only mention sends). Worth a one-line confirmation that receives are intended to surface here, and that a completed credit receive doesn't also appear via another source (e.g. the auto-redeem → VTXO path) under a different id — different ids wouldn't be merged by dedupeByID (history.go:212).

  2. No pagination/limit applied to the credit source. collectCreditEntries returns the full projected set; bounding happens only at the final paginate. This matches the existing collectSwapEntries pattern, so it's consistent — just flagging that the credit-ops table is unbounded over time and every List call re-reads and re-projects all of it.

  3. Both kinds are pulled even when only one is requested. The block is gated on SEND || RECV (history.go:135-147) but collectCreditEntries always projects both pays and receives; the unwanted kind is dropped later by filterEntries. Minor wasted work, not incorrect.

Style / convention nits

  1. Commit subject prefix. The commit touches both credit/ (messages.go, registry.go) and swapwallet/. Per CLAUDE.md ("multi: for multiple"), a multi: prefix is arguably more accurate than swapwallet:. Minor.

  2. Duplicated shouldInclude(SEND || RECV) guard. history.go:118-121 and history.go:135-138 evaluate the identical condition back-to-back. Keeping them separate keeps swap vs. credit sources visually distinct, so this is a judgment call, but they could fold into one block. Fix this →

Tests

The added tests (TestHistoryIncludesTerminalCreditOnlyPay, the -200/-500/-500 amount assertions) directly cover the regression and the sign convention. 👍
· ellemouton/fix-829-credit-pay-activity

@ellemouton
ellemouton force-pushed the ellemouton/fix-829-credit-pay-activity branch from b9d8c5d to a094e2d Compare June 30, 2026 19:45
@ellemouton ellemouton changed the title swapwallet: Keep credit-only sends in activity multi: Keep credit-only sends in activity Jun 30, 2026
@ellemouton
ellemouton force-pushed the ellemouton/fix-829-credit-pay-activity branch from a094e2d to 62f88fe Compare June 30, 2026 19:45
@ellemouton

Copy link
Copy Markdown
Member Author

@claude re-review

@claude

claude Bot commented Jun 30, 2026

Copy link
Copy Markdown

Claude finished @ellemouton's task in 2m 6s —— View job


Re-review: multi: Keep credit-only sends in activity

  • Read the current diff and changed files
  • Check what changed since the last review
  • Re-assess correctness, edge cases, and the prior review notes
  • Post re-review feedback

The revision since my last pass cleanly folds in the earlier feedback. Tracing the full read path one more time, I'm happy with this. LGTM.


What changed since the last review

  1. Commit prefix → multi: — addresses prior nit multi: stage boarding actor client  #4. Accurate now that the commit touches credit/ and swapwallet/.
  2. Duplicated SEND||RECV guard folded into a single includeSwapKinds (history.go:118-144) — addresses nits chainbackend+chainsource: add chainsource actor and backend impl #3/harness: basic infrastructure test environment #5. Credit collection now lives inside that one block, so swap + credit sources share one gate.
  3. Zero-time hardened defensivelyunixOrZero in registry.go (IsZero() → 0) plus the <= 0 guards in creditEntryFromSummary (credit_projector.go:188-193). As I noted last time this path can't actually emit a negative unix value, so this is belt-and-suspenders rather than a fix — but it's correct, self-documenting, and cheap. Fine to keep.

Correctness re-check (the part I most wanted to confirm)

The thing I wanted to be sure of is that a credit-only pay doesn't double-count while it's still pending, now that there are two live sources:

  • the runtime-local pending row from creditPayEntry (router.go:337, now -amountSat), surfaced via collectWalletLocalPendingEntries/pendingSnapshot (history.go:483), and
  • the durable projected row from collectCreditEntriescreditEntryFromSummary (history.go:137-143).

Both are keyed by the payment-hash hex (credit_projector.go:162 trims pay: off the op key; the router uses paymentHashHex as Id), so dedupeByID (history.go:209) merges them into one row. Crucially, the router.go sign fix means both rows now carry -amount, so the merge is sign-consistent regardless of which row wins the updated_at tie-break. Terminal credit rows project as COMPLETE/FAILED, so applyOverlays skips them (history.go:998) and the deadline overlay can't wrongly flip a settled SEND to FAILED. This all hangs together. 👍

Items from last time that still stand (non-blocking)

  • Credit RECVs now surface in List, keyed by op.OpID. I re-checked the collision worry: a credit-only receive has no swap summary and isn't re-emitted by the swap/ledger paths under a different id (auto-redeem produces a VTXO, which lands in the VTXOs view, not Activity), so there's no cross-source duplicate to merge. Safe — just confirming the broader-than-the-title behavior (receives, not only sends) is intended.
  • Unbounded credit source / both kinds always projected. collectCreditEntries re-reads and re-projects the entire credit-ops table on every List, and projects both pays and receives even when only one kind is requested (dropped later by filterEntries). Matches the existing collectSwapEntries pattern, so it's consistent — flagging only as a future scaling note, not a change request.

Tests

TestHistoryIncludesTerminalCreditOnlyPay covers the disappearing-terminal-row regression and the -200 sign; the -500/-500 assertions in router_test.go and credit_projector_test.go lock the negative-amount convention across the live and projected paths. Good coverage for the fix.
· ellemouton/fix-829-credit-pay-activity

@levmi levmi added activity Activity feed / wallet-entry projection bug Something isn't working oor P2 Priority 2 — medium labels Jul 6, 2026
@ellemouton
ellemouton force-pushed the ellemouton/fix-829-credit-pay-activity branch from 62f88fe to 72533ac Compare July 7, 2026 17:02
@ellemouton ellemouton changed the title multi: Keep credit-only sends in activity swapwallet: Sign credit-only sends as outflows Jul 7, 2026
A credit-only Lightning send (the sub-dust pay path) reached the
wallet activity feed with a positive amount, so it rendered as an
incoming transfer. Both the pending row built in creditPayEntry and
the projected row built in creditEntryFromSummary copied the
operation amount verbatim, unlike normal swap sends which
swapEntryFromSummary already normalizes to a negative amount.

Negate the amount for SEND rows in both places so credit-only sends
match the outflow sign convention of every other outgoing row.

The disappearing-row half of #829 (a settled credit send no longer
inspectable) is now handled by the canonical activity store: the
credit projector persists its rows through projectAndEmit and
ListActivity reads them back from the store, so no separate read-path
backfill is needed.

Fixes #829.
@ellemouton
ellemouton force-pushed the ellemouton/fix-829-credit-pay-activity branch from 72533ac to 72093aa Compare July 7, 2026 17:07
@ellemouton

Copy link
Copy Markdown
Member Author

@claude review

@claude

claude Bot commented Jul 7, 2026

Copy link
Copy Markdown

Claude finished @ellemouton's task in 1m 13s —— View job


Re-review: swapwallet: Sign credit-only sends as outflows

  • Read the current (rescoped) diff
  • Verify sign convention consistency across live + projected paths
  • Check for double-count / sign-mismatch on merge
  • Post review feedback

The PR has been trimmed to exactly the sign fix, and it's correct. LGTM.


Correctness

The negative-amount convention is now consistent across all three places that emit an OOR send:

  • normalize.go:91entry.AmountSat = -amount for normal swap sends (pre-existing baseline).
  • router.go:342 — the live pending credit-pay row: -int64(intent.amountSat).
  • credit_projector.go:200-203 — the durable projected row: -op.AmountSat gated on EntryKind_ENTRY_KIND_SEND.

Because both the live pending row (keyed by payment-hash hex) and the projected row (strings.TrimPrefix(op.OpKey, "pay:"), also the payment-hash hex) now carry -amount, dedupeByID merges them sign-consistently regardless of which wins the updated_at tie-break. The previous risk of a positive/negative flip depending on which source won the merge is gone.

The projector correctly negates only SEND — KindReceive keeps op.AmountSat positive as an inflow, which is right.

No overflow concern

Both negations are safe:

  • intent.amountSat is uint64 but validated > math.MaxInt64 → ErrAmountInvalid at router.go:121 before the intent is built, so -int64(...) can't wrap.
  • credit.CreditOpSummary.AmountSat is int64 sourced from int64(uint64) (credit/op_actor.go:395), always non-negative, so -op.AmountSat is a plain sign flip.

On the two Gemini comments — now moot

Both Gemini comments target zero-time .Unix() handling in credit/registry.go and the <= 0 guard in the projector's timestamp defaulting. Those files/hunks are no longer in this diff — the rescope dropped the timestamp-plumbing changes (as the description notes, that half of #829 was already resolved on main by #840/#842). Nothing to action here.

Tests

  • router_test.go:476 asserts -500 on the live pending row.
  • credit_projector_test.go:110 asserts -500 on the projected terminal row.

Both lock the regression directly across the live and projected paths. Good, tightly-scoped coverage for a one-line-behavior fix.

Nice, clean fix. 👍
· ellemouton/fix-829-credit-pay-activity

@ellemouton
ellemouton merged commit 073dc2e into main Jul 7, 2026
20 of 21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

activity Activity feed / wallet-entry projection bug Something isn't working oor P2 Priority 2 — medium

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unable to send small amount, shows as incoming transaction

2 participants