handle slow payments to buyer#844
Conversation
WalkthroughNew payment status helpers ChangesPayment Status Hardening and Double-Pay Prevention
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed: one or more packages not found in the registry. 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: 2
🤖 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 `@jobs/pending_payments.ts`:
- Around line 86-131: The condition at the start of the confirmed payment block
uses AND logic which means if is_confirmed is true but payment is falsy, the
entire block is skipped and execution falls through to re-payment logic.
Instead, change the primary skip condition to check only is_confirmed, then nest
the notification messages (toAdminChannelPendingPaymentSuccessMessage,
toBuyerPendingPaymentSuccessMessage, and rateUserMessage) inside a separate
check for when payment is present. This prevents re-payment of confirmed
invoices when payment data is missing. Apply the same fix pattern to the
attemptCommunitiesPendingPayments function which has the identical
vulnerability.
In `@ln/pay_request.ts`:
- Around line 245-256: The catch block in getPaymentStatus is returning a
safe-looking response for all errors, including unexpected ones like transient
connectivity issues, which can mask real failures and cause callers to unsafely
proceed with payment. Modify the error handling to only return the
is_confirmed/is_pending false response for legitimate "not found" cases (errors
containing SentPaymentNotFound, PaymentNotFound, or 404), and rethrow all other
unrecognized errors so that callers like payToBuyer and attemptPendingPayments
can properly abort before attempting payment instead of treating an unknown
error state as safe.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5e68f4c3-ad4c-43c0-bf7e-ce206d4b7095
📒 Files selected for processing (5)
bot/modules/community/scenes.tsbot/scenes.tsjobs/pending_payments.tsln/index.tsln/pay_request.ts
| const currentStatus = await getPaymentStatus(pending.payment_request); | ||
|
|
||
| // If already confirmed, process the SUCCESS routine immediately! | ||
| if (currentStatus.is_confirmed && currentStatus.payment) { | ||
| const payment = currentStatus.payment; | ||
| order.status = 'SUCCESS'; | ||
| order.routing_fee = payment.fee; | ||
| pending.paid = true; | ||
| pending.paid_at = new Date(); | ||
| const buyerUser = await User.findOne({ _id: order.buyer_id }); | ||
| if (buyerUser === null) throw Error('buyerUser was not found in DB'); | ||
| const i18nCtx: I18nContext = await getUserI18nContext(buyerUser); | ||
| buyerUser.trades_completed++; | ||
| await buyerUser.save(); | ||
| const sellerUser = await User.findOne({ _id: order.seller_id }); | ||
| if (sellerUser === null) throw Error('sellerUser was not found in DB'); | ||
| sellerUser.trades_completed++; | ||
| await sellerUser.save(); | ||
| logger.info( | ||
| `Invoice with hash: ${pending.hash} already paid, processing SUCCESS routine`, | ||
| ); | ||
| await messages.toAdminChannelPendingPaymentSuccessMessage( | ||
| bot, | ||
| buyerUser, | ||
| order, | ||
| pending, | ||
| payment, | ||
| i18nCtx, | ||
| ); | ||
| await messages.toBuyerPendingPaymentSuccessMessage( | ||
| bot, | ||
| buyerUser, | ||
| order, | ||
| payment, | ||
| i18nCtx, | ||
| ); | ||
| await messages.rateUserMessage(bot, buyerUser, order, i18nCtx); | ||
| continue; | ||
| } | ||
|
|
||
| if (currentStatus.is_pending) { | ||
| logger.info( | ||
| `Order ${order._id}: current payment is already in-flight, skipping retry without incrementing attempts`, | ||
| ); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
is_confirmed && payment can fall through to a re-pay.
If getPaymentStatus ever returns is_confirmed: true with a falsy payment, the block at Line 89 is skipped, the is_pending block at Line 126 is also false, and execution continues to payRequest — re-paying an already-confirmed invoice. Base the skip decision on is_confirmed alone, and only run the notification routine when payment is present. The same pattern exists in attemptCommunitiesPendingPayments at Line 285.
🛡️ Suggested guard
- if (currentStatus.is_confirmed && currentStatus.payment) {
- const payment = currentStatus.payment;
+ if (currentStatus.is_confirmed) {
order.status = 'SUCCESS';
- order.routing_fee = payment.fee;
pending.paid = true;
pending.paid_at = new Date();
- const buyerUser = await User.findOne({ _id: order.buyer_id });
+ const payment = currentStatus.payment;
+ if (!payment) {
+ logger.warning(
+ `Order ${order._id}: confirmed without payment details, marking SUCCESS and skipping retry`,
+ );
+ continue;
+ }
+ order.routing_fee = payment.fee;
+ const buyerUser = await User.findOne({ _id: order.buyer_id });🤖 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 `@jobs/pending_payments.ts` around lines 86 - 131, The condition at the start
of the confirmed payment block uses AND logic which means if is_confirmed is
true but payment is falsy, the entire block is skipped and execution falls
through to re-payment logic. Instead, change the primary skip condition to check
only is_confirmed, then nest the notification messages
(toAdminChannelPendingPaymentSuccessMessage,
toBuyerPendingPaymentSuccessMessage, and rateUserMessage) inside a separate
check for when payment is present. This prevents re-payment of confirmed
invoices when payment data is missing. Apply the same fix pattern to the
attemptCommunitiesPendingPayments function which has the identical
vulnerability.
| } catch (error: any) { | ||
| const message = error.toString(); | ||
| logger.error(`isPendingPayment catch error: ${message}`); | ||
| return false; | ||
| const errStr = error.toString(); | ||
| if ( | ||
| errStr.includes('SentPaymentNotFound') || | ||
| errStr.includes('PaymentNotFound') || | ||
| errStr.includes('404') | ||
| ) { | ||
| return { is_confirmed: false, is_pending: false, payment: undefined }; | ||
| } | ||
| logger.error(`getPaymentStatus error: ${errStr}`); | ||
| return { is_confirmed: false, is_pending: false, payment: undefined }; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major
🧩 Analysis chain
🌐 Web query:
ln-service getPayment error codes for payment not found versus connectivity errors
💡 Result:
ln-service getPayment: “payment not found” vs connectivity errors 1) Payment not found (i.e., the payment hash exists in your request to getPayment, but lnd can’t return it) - For ln-service’s getPayment, the library returns payment lookup results in a structured shape (success/failed/pending) rather than throwing a “connectivity” error. The API doc describes getPayment as a past-payment lookup returning fields like failed/is_pending/is_confirmed and a payment object when found. [1] - In the underlying lnd/router tracking model, “no route can be found” is represented as a payment failure reason no-route (FailureReasonNoRoute = 2). [2] - Note: lnd’s payment failure reasons like TIMEOUT, NO_ROUTE, etc. are not “connectivity errors”; they are in-band payment outcome/failure reasons. [2] Practical mapping you can use: - If getPayment returns a response that indicates a failed payment with a failure reason such as NO_ROUTE or TIMEOUT, treat it as “payment exists but failed / can’t find a route”. [2] - If lnd returns a gRPC NotFound-style response for the lookup itself, treat it as “payment not found” (not connectivity). (lnd has been changing/standardizing NotFound handling for lookups in general; see that LookupInvoice was fixed to return codes.NotFound for missing entities.) [3] 2) Connectivity errors (i.e., you cannot reach lnd / gRPC channel fails) - ln-service documents debugging by enabling gRPC verbosity/tracing via GRPC_VERBOSITY and GRPC_TRACE to inspect the underlying gRPC calls; this is the usual way you identify connectivity/transport problems vs in-band payment results. [4] Practical mapping you can use: - If you see gRPC transport/channel errors (e.g., inability to connect, inactive RPC) rather than a payment/failure result payload, classify it as connectivity. This is distinct from payment failure reasons like NO_ROUTE. [4][5] Gaps / what I could not conclusively map - Your request asks for “ln-service getPayment error codes … for payment not found versus connectivity errors”. The official ln-service getPayment documentation snippet available from the sources I retrieved does not enumerate explicit numeric error codes for “payment not found” vs “connectivity”. It only shows the success/failure/pending return structure and general debugging guidance. [1][4] - The lnrpc PaymentFailureReason enum provides failure reasons for payment outcomes (e.g., NO_ROUTE) but not a dedicated “payment not found” code. [2] If you want, share the exact error object you’re getting from ln-service getPayment (stack + error fields, or the gRPC code/message). I can then tell you which category it is (payment-not-found vs transport) and what specific code/message to branch on.
Citations:
- 1: https://github.com/samotari/ln-service
- 2: https://docs.rs/lnrpc/latest/lnrpc/enum.PaymentFailureReason.html
- 3: rpcserver: return consistent grpc NotFound error for GetChaninfo & LookupInvoice lightningnetwork/lnd#10064
- 4: https://github.com/alexbosworth/lightning/
- 5: Calls to LookupInvoice with invalid r_hash raises an internal rpc exception lightningnetwork/lnd#4936
🏁 Script executed:
# Find and examine the pay_request.ts file
fd -t f "pay_request.ts" | head -5Repository: lnp2pBot/bot
Length of output: 168
🏁 Script executed:
# Get the file structure and size
wc -l ln/pay_request.ts 2>/dev/null || find . -name "pay_request.ts" -exec wc -l {} \;Repository: lnp2pBot/bot
Length of output: 172
🏁 Script executed:
# Use ast-grep to map the getPaymentStatus function
ast-grep outline ln/pay_request.ts --match getPaymentStatus 2>/dev/null || ast-grep outline . --match getPaymentStatus 2>/dev/null | head -20Repository: lnp2pBot/bot
Length of output: 245
🏁 Script executed:
# Read the getPaymentStatus function and surrounding code (lines 230-270)
sed -n '230,270p' ln/pay_request.tsRepository: lnp2pBot/bot
Length of output: 1414
🏁 Script executed:
# Search for where getPayment is called and defined
rg "getPayment" ln/pay_request.ts -B2 -A5 | head -50Repository: lnp2pBot/bot
Length of output: 1489
🏁 Script executed:
# Look for the payToBuyer function to see how it handles getPaymentStatus errors
rg "payToBuyer" ln/pay_request.ts -B3 -A10 | head -60Repository: lnp2pBot/bot
Length of output: 1483
🏁 Script executed:
# Check lightning library imports and see if there's documentation or other usage
rg "from 'lightning'" -A5 | head -30Repository: lnp2pBot/bot
Length of output: 150
🏁 Script executed:
# Search for other error handling patterns with getPayment or similar ln-service calls
rg "getPayment|payViaPaymentRequest" --type ts -B2 -A5 | head -80Repository: lnp2pBot/bot
Length of output: 150
🏁 Script executed:
# Check if there are any tests or examples showing getPayment error behavior
fd -t f "\.test\.ts|\.spec\.ts|test|spec" -x grep -l "getPayment" {} \; 2>/dev/null | head -5Repository: lnp2pBot/bot
Length of output: 150
🏁 Script executed:
# Look for package.json to see the lightning library version
cat package.json | grep -A2 -B2 "lightning"Repository: lnp2pBot/bot
Length of output: 513
🌐 Web query:
lightning npm library getPayment function error handling throws exceptions v10
💡 Result:
I wasn’t able to find a definitive, official “lightning v10 npm getPayment” error-handling section (or an official changelog entry) that explicitly documents which errors are thrown by a specific “getPayment” function in a “lightning” npm package version 10. The closest authoritative material I found concerns the underlying “ln-service” library’s getPayment API and general error-handling guidance for Lightning/WebLN usage. What I can say with sourced support: 1) The commonly referenced getPayment in this ecosystem is provided by ln-service (Alex Bosworth). Its docs describe getPayment as “Lookup details about a past payment” but the snippet I retrieved does not include documented throw/error semantics for getPayment itself. [1] 2) In Lightning/WebLN-style APIs, errors during an API call are delivered by throwing an exception, and the recommended approach is to wrap calls in try/catch. [2] 3) In a separate Lightning API context (LND router payment tracking), failure reasons are represented as enumerated fields (e.g., TIMEOUT, NO_ROUTE, INCORRECT_PAYMENT_DETAILS, etc.), which suggests the preferred pattern is to treat payment failures as returned status/failure fields rather than (or in addition to) exceptions. [3] 4) A concrete example from a codebase shows an implementation pattern: call getPayment inside try/catch, and treat a specific “payment not found” case as non-fatal (return null), otherwise rethrow. [4] Practical guidance to handle “getPayment” exceptions reliably (based on the sourced patterns above): - Always use try/catch around await getPayment(...), because some Lightning API surfaces throw exceptions on failure conditions. [2] - If your runtime indicates “not found” as an exception (example: mapping a 404 + specific error code to null), translate that to a non-exceptional result for your caller; otherwise rethrow or convert to a domain error. [4] - Prefer inspecting returned payment status/failure reason fields (where your library exposes them) for terminal payment outcomes instead of assuming every payment failure is thrown as an exception. [3] If you share the exact npm package name (is it “lightning” by lightning/?? vs “ln-service”?) and the exact import path plus the code that calls getPayment (and the observed error message/stack), I can do a second targeted search to identify the precise documented throw behavior for that specific v10 implementation.
Citations:
- 1: https://registry.npmjs.org/ln-service
- 2: https://github.com/getAlby/webln_guide/blob/main/building-lightning-apps/webln-reference/error-handling.md
- 3: https://lightning.engineering/api-docs/api/lnd/router/track-payments/
- 4: https://cdn.jsdelivr.net/npm/crosslightning-intermediary@10.3.11/src/swaps/tobtcln_abstract/ToBtcLnAbs.ts
🏁 Script executed:
# Check if there are any comments or inline documentation in the code about error handling
rg "getPayment|connectivity|transient" ln/pay_request.ts -B3 -A3Repository: lnp2pBot/bot
Length of output: 1444
🏁 Script executed:
# Look for other try/catch patterns in the codebase to understand error handling philosophy
rg "try \{" ln/ -A15 | head -100Repository: lnp2pBot/bot
Length of output: 5642
Replace fail-open error handling with rethrow on unrecognized errors to prevent false-safe interpretations.
The catch handler at lines 254–255 returns {is_confirmed: false, is_pending: false} for all errors: both legitimate "not found" cases (correct for fresh invoices) and any unexpected errors (e.g. transient connectivity issues). This fail-open behavior is unsafe because every caller (payToBuyer, attemptPendingPayments, etc.) interprets all-false as "safe to proceed to payment"—even if getPayment threw due to a transient LND outage while a payment was actually in-flight or confirmed. This defeats the guard's purpose.
Adopt fail-closed behavior: rethrow unrecognized errors so existing try/catch blocks in callers abort before attempting payment, rather than treating an unknown state as safe.
Suggested fail-closed handling
} catch (error: any) {
const errStr = error.toString();
if (
errStr.includes('SentPaymentNotFound') ||
errStr.includes('PaymentNotFound') ||
errStr.includes('404')
) {
return { is_confirmed: false, is_pending: false, payment: undefined };
}
logger.error(`getPaymentStatus error: ${errStr}`);
- return { is_confirmed: false, is_pending: false, payment: undefined };
+ // Unexpected error: fail closed so callers do not treat an unknown state
+ // as "safe to pay" and re-attempt an in-flight/confirmed payment.
+ throw error;
}🤖 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 `@ln/pay_request.ts` around lines 245 - 256, The catch block in
getPaymentStatus is returning a safe-looking response for all errors, including
unexpected ones like transient connectivity issues, which can mask real failures
and cause callers to unsafely proceed with payment. Modify the error handling to
only return the is_confirmed/is_pending false response for legitimate "not
found" cases (errors containing SentPaymentNotFound, PaymentNotFound, or 404),
and rethrow all other unrecognized errors so that callers like payToBuyer and
attemptPendingPayments can properly abort before attempting payment instead of
treating an unknown error state as safe.
|
What does is_confirmed mean? are we introducing on-chain (non-lightning) payments? If the answer is no, the word "confirmed" might be measleading (i.e. "confirmations"). |
grunch
left a comment
grunch
left a comment
There was a problem hiding this comment.
Strict review
Direction is correct and it fixes two real bugs along the way:
- Good fix: the old
attemptCommunitiesPendingPaymentsdidif (isPending) return;— areturnthat aborted the whole community payout batch, not just that item. Now it'scontinue. - Good fix:
attempts++is no longer incremented on every poll while a payment is in-flight; it's incremented only right before an actual pay attempt. sellerUser.save()→await sellerUser.save().
I don't see a critical fund-loss bug: for the same payment_request, LND's payment-hash idempotency backstops the check (a second payViaPaymentRequest on an in-flight/settled hash fails rather than double-paying). Build (tsc --noEmit), eslint, and the test suite (133 passing) are all green locally. That said, a few things should be addressed before merge — details inline.
Summary
- 🟠 The "healing" branches mark the order
SUCCESSbut skip buyer notification,trades_completed, rating androuting_fee, unlike the full success routine — inconsistent completed-order state. - 🟡
getPaymentStatusfails open on unknown/transient LND errors (returns all-false → caller proceeds to pay). For a double-pay guard this should fail closed (treat unknown error as pending/skip). - 🟡 Not-found detection via
error.toString().includes('404' | 'SentPaymentNotFound')is fragile; inspect the structured error instead. - 🟡
payment?: anydrops SDK typing; the success routine is now triplicated and the heal branches are already diverging from it (see first point). - 🟡 N+1 LND RPCs per job run;
previousPendingPaymentsquery isn't filtered. - 🟢 No tests for the new, fund-sensitive logic.
Verdict: request changes. The must-haves before merge are (1) unify notification/reputation in the heal branches and (2) make getPaymentStatus fail-closed on unknown errors. The rest are recommended but non-blocking.
| return { is_confirmed: false, is_pending: false, payment: undefined }; | ||
| } | ||
| logger.error(`getPaymentStatus error: ${errStr}`); | ||
| return { is_confirmed: false, is_pending: false, payment: undefined }; |
There was a problem hiding this comment.
Fail-open on unknown errors. Any error that isn't *NotFound/404 (e.g. 503, gRPC down, RPC timeout) returns {is_confirmed:false, is_pending:false}, so the caller proceeds to pay. Since the whole point of this PR is preventing a double pay after restart, this should fail closed: on an unknown error, treat the payment as pending (skip and retry next run) rather than free-to-pay. The same-hash case is covered by LND idempotency, but the /setinvoice new-invoice vector relies on isConfirmedPayment(order.buyer_invoice) here, and a transient failure exactly during a restart+settle attack would open it.
| if ( | ||
| errStr.includes('SentPaymentNotFound') || | ||
| errStr.includes('PaymentNotFound') || | ||
| errStr.includes('404') |
There was a problem hiding this comment.
Fragile error matching. The pay/skip gate depends on string-matching error.toString(). The lightning lib returns errors as arrays like [404, 'SentPaymentNotFound', {...}]; this works today only because toString() flattens them, and includes('404') can also false-positive on other codes containing 404. Prefer inspecting the structured error (error[0] === 404 / error[1]).
| interface PaymentStatus { | ||
| is_confirmed: boolean; | ||
| is_pending: boolean; | ||
| payment?: any; |
There was a problem hiding this comment.
payment?: any drops the SDK's type for the payment object (the fields fee, id, secret are read downstream). Consider typing it from getPayment's return. Also: the success routine (set status, trades_completed++, admin/buyer messages, rate) is now triplicated (here-driven branches in the job + community). Extracting a helper would stop these copies from diverging — which they already are (see the heal branches).
| logger.info( | ||
| `Order ${order._id}: original buyer invoice already confirmed, marking SUCCESS and skipping retry`, | ||
| ); | ||
| continue; |
There was a problem hiding this comment.
Healing branch doesn't notify or credit. When order.buyer_invoice is already confirmed the buyer did receive the sats, but here we only set status='SUCCESS' + pending.paid=true — no buyer message, no trades_completed++ for either party, no routing_fee, no rateUserMessage. Same for the previousPendingPayments-confirmed branch below and the /setinvoice heal in bot/scenes.ts. Only the currentStatus.is_confirmed branch runs the full routine. Result: "phantom" completed orders with no reputation/rating and an uninformed buyer. Run the same success routine when healing by confirmation.
| _id: { $ne: pending._id }, | ||
| order_id: order._id, | ||
| is_invoice_expired: false, | ||
| }); |
There was a problem hiding this comment.
N+1 LND RPCs per job run: this now does getPaymentStatus(order.buyer_invoice) + one per previousPendingPayments + one for the current request, for every pending, every 5 min. Acceptable at current scale but worth noting. This query also doesn't filter paid:false/attempts, so it scans more rows than needed.
| if (alreadyPaid) { | ||
| order.status = 'SUCCESS'; | ||
| await order.save(); | ||
| return await messages.invoiceAlreadyUpdatedMessage(ctx); |
There was a problem hiding this comment.
Same consistency gap as the job: heals to SUCCESS but the buyer (who is here precisely because they think they weren't paid) gets invoiceAlreadyUpdatedMessage, which is confusing. If the original invoice is confirmed, tell them they were already paid and credit the success routine.
3 participants
Summary by CodeRabbit