fix: fallback to outgoing payments in Phoenixd LookupInvoice#2447
Conversation
|
Warning Review limit reached
More reviews will be available in 2 minutes and 31 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughLookupInvoice now attempts an incoming-payment lookup first and, on any error, falls back to an outgoing-payment lookup; outgoing Phoenixd responses are converted into lnclient.Transaction via a new helper. ChangesPhoenixd outgoing payment lookup
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lnclient/phoenixd/phoenixd.go (1)
273-299: ⚡ Quick winWrap the new helper errors with endpoint-specific context.
The new lookup/conversion helpers return several bare
errvalues, so reconciliation logs lose which Phoenixd call or decode step actually failed. Please wrap the new error paths with contextualfmt.Errorf(...: %w)messages.💡 Suggested fix
req, err := http.NewRequestWithContext(ctx, http.MethodGet, svc.Address+"/payments/outgoing", nil) if err != nil { - return nil, err + return nil, fmt.Errorf("build phoenixd outgoing payments request: %w", err) } ... resp, err := client.Do(req) if err != nil { - return nil, err + return nil, fmt.Errorf("call phoenixd outgoing payments endpoint: %w", err) } ... if err := json.Unmarshal(body, &outgoingPayments); err != nil { - return nil, err + return nil, fmt.Errorf("decode phoenixd outgoing payments response: %w", err) } ... paymentRequest, err := decodepay.Decodepay(payment.Invoice) if err != nil { logger.Logger.WithFields(logrus.Fields{ "bolt11": payment.Invoice, }).Errorf("Failed to decode bolt11 invoice: %v", err) - return nil, err + return nil, fmt.Errorf("decode phoenixd outgoing payment bolt11: %w", err) }As per coding guidelines, "Wrap errors with fmt.Errorf("context: %w", err) for debugging".
Also applies to: 305-327, 537-542
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lnclient/phoenixd/phoenixd.go` around lines 273 - 299, In lookupIncomingPayment (method PhoenixService.lookupIncomingPayment) wrap each returned error with endpoint-specific context using fmt.Errorf("phoenixd lookup incoming %s: %w", ...) or similar — e.g., wrap errors from http.NewRequestWithContext, client.Do, io.ReadAll, json.Unmarshal and the call to phoenixInvoiceToTransaction so logs show which HTTP call or decode step failed; apply the same pattern to the other PhoenixService lookup/convert helpers referenced in the review (the similar functions around the other diffs) so every bare err return becomes fmt.Errorf("phoenixd <operation> <identifier>: %w", err).Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lnclient/phoenixd/phoenixd.go`:
- Around line 263-270: The current fallback to lookupOutgoingPayment is
unconditional on any error from lookupIncomingPayment; change this so the
outgoing fallback is invoked only when lookupIncomingPayment returns a clear
“not found” / 404-style error. In the LookupInvoice flow, update the block
around svc.lookupIncomingPayment(ctx, paymentHash) to detect the specific
not-found sentinel (or HTTP 404 / lnrpc not-found error / custom ErrNotFound
used elsewhere) and only call svc.lookupOutgoingPayment(ctx, paymentHash) in
that case; for all other errors (request failures, 5xxs, decode errors,
phoenixInvoiceToTransaction failures) return the original error instead of
falling back. Ensure you reference and use the same error type/variable used by
lookupIncomingPayment for accurate detection.
---
Nitpick comments:
In `@lnclient/phoenixd/phoenixd.go`:
- Around line 273-299: In lookupIncomingPayment (method
PhoenixService.lookupIncomingPayment) wrap each returned error with
endpoint-specific context using fmt.Errorf("phoenixd lookup incoming %s: %w",
...) or similar — e.g., wrap errors from http.NewRequestWithContext, client.Do,
io.ReadAll, json.Unmarshal and the call to phoenixInvoiceToTransaction so logs
show which HTTP call or decode step failed; apply the same pattern to the other
PhoenixService lookup/convert helpers referenced in the review (the similar
functions around the other diffs) so every bare err return becomes
fmt.Errorf("phoenixd <operation> <identifier>: %w", err).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f680c9a5-162a-4413-a54b-03d0206d1117
📒 Files selected for processing (1)
lnclient/phoenixd/phoenixd.go
G12c4
force-pushed
the
fix/phoenixd-lookup-outgoing-payments
branch
from
05e7570 to
f3f6ad8
Compare
LookupInvoice only queried /payments/incoming/{hash}, returning 404 for
outgoing payments. This caused all outgoing Lightning payments to remain
permanently stuck as PENDING in Alby Hub.
The fix tries incoming first (preserving existing behavior), then falls
back to listing outgoing payments and matching by paymentHash.
Fixes getAlby#2442
G12c4
force-pushed
the
fix/phoenixd-lookup-outgoing-payments
branch
from
f3f6ad8 to
5e9191a
Compare
1 participant
Problem
Outgoing Lightning payments via Phoenixd backend are permanently displayed as PENDING in Alby Hub, even after Phoenixd confirms they are settled (
isPaid=true).Closes #2442
Root Cause
LookupInvoiceonly queries/payments/incoming/{paymentHash}. For outgoing payments this returns 404, and the reconciliation loop intransactions_service.go:718skips the payment, leaving it as PENDING forever.Fix
When
/payments/incoming/{hash}fails, fall back to/payments/outgoingbyhash/{hash}— a dedicated Phoenixd endpoint that looks up outgoing payments directly by payment hash (added in Phoenixd v0.x). This is an O(1) lookup, not a list+scan.The
LNClient.LookupInvoiceinterface accepts only apaymentHashwith no transaction type hint. Ideally it would accept a type so we could skip the incoming probe for known-outgoing payments, but changing theLNClientinterface touches every backend (LND, LDK, CLN, Bark, Cashu) and is out of scope for this bugfix. The incoming→outgoing fallback is safe, minimal, and preserves all existing behavior.Changes
LookupInvoice— tries incoming first (preserving existing behavior), then falls back to outgoinglookupOutgoingPayment— uses/payments/outgoingbyhash/{hash}for direct O(1) lookupoutgoingPaymentToTransaction— convertsOutgoingPaymentResponsetolnclient.Transaction(mirrors existingphoenixInvoiceToTransaction)Error()log on incoming 404 — the error is still returned and logged by the caller incheckUnsettledTransactionTesting
go build ./lnclient/phoenixd/)isPaid=trueat Phoenixd level while Alby Hub showed them as PENDING/payments/outgoingbyhashendpoint from Phoenixd source (Api.kt) — returns the best match (succeeded > pending > failed)Summary by CodeRabbit