handle slow payments to buyer#844
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR adds structured LN payment status checks, updates invoice guards to treat confirmed payments as blocking or healable, refactors pending-payment retries around status checks, and introduces a shared success-completion helper with tests for the new flows. ChangesPayment Status Hardening and Success Reconciliation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 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
|
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.
| 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.
fix: address grunch review on handle-delayed-payment PR
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
bot/scenes.ts (1)
181-200: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDon’t persist partial
SUCCESSoutside the success helper.The fallback sets only
order.status = 'SUCCESS'. That bypassescompleteOrderAsSuccess, so routing fee, trade counters, and success/rating messages can be skipped; once status isSUCCESS, the helper’s compare-and-set cannot heal those side effects later.Suggested guard split
if (originalStatus?.is_confirmed) { const i18nCtx = await getUserI18nContext(buyer); + if (!originalStatus.payment) { + logger.error( + `Order ${order._id}: confirmed buyer invoice has no payment details`, + ); + await messages.invoiceAlreadyUpdatedMessage(ctx); + return ctx.scene.leave(); + } const sellerUser = await User.findOne({ _id: order.seller_id }); - if (originalStatus.payment && sellerUser !== null) { - // Shared, idempotent success routine: if the pending-payments job - // already closed this order the compare-and-set inside makes this a - // no-op so the buyer is not notified twice. - await completeOrderAsSuccess( - bot, - order, - originalStatus.payment, - buyer, - sellerUser, - i18nCtx, - ); - } else { - order.status = 'SUCCESS'; - await order.save(); - await messages.invoiceAlreadyUpdatedMessage(ctx); - } + if (sellerUser === null) throw Error('sellerUser was not found in DB'); + await completeOrderAsSuccess( + bot, + order, + originalStatus.payment, + buyer, + sellerUser, + i18nCtx, + ); return ctx.scene.leave(); }🤖 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 `@bot/scenes.ts` around lines 181 - 200, The fallback path in scenes.ts is only setting order.status to SUCCESS, which skips the side effects handled by completeOrderAsSuccess. Update the originalStatus?.is_confirmed branch in the same flow that calls completeOrderAsSuccess so the order cannot be partially persisted as SUCCESS without routing fee, trade counter, and rating/message updates; if the helper cannot run, keep the order in its prior state or gate the fallback on the same conditions used by completeOrderAsSuccess rather than saving SUCCESS directly.
🧹 Nitpick comments (1)
tests/bot/pending-payments.spec.ts (1)
111-117: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the CAS call, not just the side effects.
These cases currently pass as long as
order.statusis mutated in memory. They never prove that the success path used the intended atomicfindOneAndUpdate(...status != SUCCESS...)guard, so a regression back to a plainsave()would slip through. Keep that stub in a variable and assert its filter/update in one happy-path case.Also applies to: 172-218, 258-291
🤖 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 `@tests/bot/pending-payments.spec.ts` around lines 111 - 117, The pending-payments tests are only checking side effects and not proving the atomic compare-and-set path is used. In the happy-path cases in pending-payments.spec.ts, keep the Order.findOneAndUpdate stub in a named variable and assert it was called with the expected filter guarding against already-SUCCESS orders and the intended update payload, so completeOrderAsSuccess cannot regress to a plain save() without failing these tests.
🤖 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 `@bot/scenes.ts`:
- Around line 132-133: The healed-payment flow is pulling the bot from
ctx.wizard.bot, but that property is not available in the wizard context and can
leave completeOrderAsSuccess with undefined. Update the scene logic in scenes.ts
to accept and use the bot instance explicitly from the surrounding
handler/context, and pass that concrete bot through to completeOrderAsSuccess so
the completion messages can always be sent.
In `@tests/ln/get-payment-status.spec.ts`:
- Around line 20-23: The test setup in get-payment-status.spec.ts derives
destination from an unchecked random private key via ecc.pointFromScalar(),
which can intermittently fail when makeInvoice() runs. Update the test fixture
near the priv/destination setup to use a deterministic valid key pair or add
validation and retry logic until pointFromScalar() returns a non-null point.
Keep the change localized to the test helper setup so the invoice creation path
is always fed a valid destination.
In `@util/completeOrder.ts`:
- Around line 28-61: The completion flow in completeOrder is not retryable
because Order.findOneAndUpdate writes SUCCESS before buyerUser.save,
sellerUser.save, and the message sends, so a later failure leaves the order
partially completed. Update completeOrder to make the DB updates and completion
side effects retry-safe by using a transaction/atomic completion marker or an
outbox pattern, and ensure the status transition, trade counters, and
pending/payment notifications can be resumed if any await fails.
- Around line 40-43: The `trades_completed` updates in `completeOrder` are still
using read-modify-write via `buyerUser.save()` and `sellerUser.save()`, which
can lose increments under concurrent order completions. Update the counter
writes to use atomic `$inc` operations on the user records instead of mutating
the loaded documents, and if the order status change must stay in sync with both
counter updates, group the `findOneAndUpdate` logic and the user counter updates
in a transaction.
---
Duplicate comments:
In `@bot/scenes.ts`:
- Around line 181-200: The fallback path in scenes.ts is only setting
order.status to SUCCESS, which skips the side effects handled by
completeOrderAsSuccess. Update the originalStatus?.is_confirmed branch in the
same flow that calls completeOrderAsSuccess so the order cannot be partially
persisted as SUCCESS without routing fee, trade counter, and rating/message
updates; if the helper cannot run, keep the order in its prior state or gate the
fallback on the same conditions used by completeOrderAsSuccess rather than
saving SUCCESS directly.
---
Nitpick comments:
In `@tests/bot/pending-payments.spec.ts`:
- Around line 111-117: The pending-payments tests are only checking side effects
and not proving the atomic compare-and-set path is used. In the happy-path cases
in pending-payments.spec.ts, keep the Order.findOneAndUpdate stub in a named
variable and assert it was called with the expected filter guarding against
already-SUCCESS orders and the intended update payload, so
completeOrderAsSuccess cannot regress to a plain save() without failing these
tests.
🪄 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: d36da1fb-86fe-4571-a4cd-aa5c2a67c93d
📒 Files selected for processing (7)
bot/scenes.tsjobs/pending_payments.tsln/index.tsln/pay_request.tstests/bot/pending-payments.spec.tstests/ln/get-payment-status.spec.tsutil/completeOrder.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- jobs/pending_payments.ts
| const priv = crypto.randomBytes(32); | ||
| const destination = Buffer.from(ecc.pointFromScalar(priv, true)).toString( | ||
| 'hex', | ||
| ); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the test file and any local references to makeInvoice / pointFromScalar.
git ls-files 'tests/ln/get-payment-status.spec.ts' 'package.json' 'package-lock.json' 'pnpm-lock.yaml' 'yarn.lock' 2>/dev/null || true
printf '\n--- FILE ---\n'
sed -n '1,220p' tests/ln/get-payment-status.spec.ts
printf '\n--- SEARCH ---\n'
rg -n "pointFromScalar|makeInvoice|randomBytes\\(32\\)|Buffer\\.from\\(" tests/ln/get-payment-status.spec.ts tests -g '!**/node_modules/**' || true
printf '\n--- PACKAGE INFO ---\n'
sed -n '1,220p' package.json 2>/dev/null || trueRepository: lnp2pBot/bot
Length of output: 8878
🌐 Web query:
tiny-secp256k1 pointFromScalar null invalid scalar documentation
💡 Result:
In the tiny-secp256k1 library, the pointFromScalar(d[, compressed]) function handles scalar inputs to derive a point on the secp256k1 elliptic curve [1][2]. According to the library's documentation, the function behaves as follows regarding null returns and invalid scalars: 1. Null Returns: The function returns null if the resulting point is at infinity [1][3]. This occurs when the scalar provided is invalid or specifically results in the point at infinity (for example, if the scalar is zero or a multiple of the curve order) [1][2]. 2. Invalid Scalars: If the provided scalar input (d) is invalid, the function throws an error [1][2]. Specifically, it throws an 'Expected Private' error if the input fails the isPrivate(d) check, which verifies that the scalar is a valid private key (i.e., it must be a 32-byte buffer and numerically greater than 0 and less than the curve order) [1][3]. In summary, the function distinguishes between mathematically valid scalars that result in the point at infinity (which return null) and inputs that are not valid scalars (which throw an error) [1][2].
Citations:
- 1: https://github.com/bitcoinjs/tiny-secp256k1
- 2: https://github.com/bitcoinjs/tiny-secp256k1/blob/master/README.md
- 3: https://www.npmjs.com/package/tiny-secp256k1
🌐 Web query:
Buffer.from(null) TypeError Node.js documentation
💡 Result:
In Node.js, calling Buffer.from(null) throws a TypeError because null is not a valid input type for this method [1]. The Buffer.from method requires its input to be one of the following types: string, Buffer, ArrayBuffer, Array, or an Array-like object [1][2]. Because null does not meet these criteria, Node.js explicitly validates the input and throws an ERR_INVALID_ARG_TYPE error (which is an instance of TypeError) to prevent incorrect memory allocation or unexpected behavior [1][3]. This strict validation is intended to ensure that the method only receives data that can be correctly converted into a Buffer [3]. If you encounter this error in your code, it usually means a variable you expected to contain data was unexpectedly null at the time Buffer.from was called [1]. To resolve it, ensure the input is sanitized or checked for nullity before passing it to Buffer.from.
Citations:
- 1: Buffer.from doesn't allow "null" chunk nock/nock#1944
- 2: https://github.com/nodejs/node/blob/main/doc/api/buffer.md
- 3: nodejs/node@75eaf25e78
Avoid deriving the test invoice from an unchecked random private key. tiny-secp256k1.pointFromScalar() can throw on invalid scalars and return null in edge cases, so makeInvoice() can fail intermittently here. Use a fixed fixture or retry until a valid point is produced.
🤖 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 `@tests/ln/get-payment-status.spec.ts` around lines 20 - 23, The test setup in
get-payment-status.spec.ts derives destination from an unchecked random private
key via ecc.pointFromScalar(), which can intermittently fail when makeInvoice()
runs. Update the test fixture near the priv/destination setup to use a
deterministic valid key pair or add validation and retry logic until
pointFromScalar() returns a non-null point. Keep the change localized to the
test helper setup so the invoice creation path is always fed a valid
destination.
| const won = await Order.findOneAndUpdate( | ||
| { _id: order._id, status: { $ne: 'SUCCESS' } }, | ||
| { $set: { status: 'SUCCESS', routing_fee: payment.fee } }, | ||
| ); | ||
| if (won === null) return false; | ||
| // Keep the in-memory document consistent for any later save by the caller. | ||
| order.status = 'SUCCESS'; | ||
| order.routing_fee = payment.fee; | ||
| if (pending) { | ||
| pending.paid = true; | ||
| pending.paid_at = new Date(); | ||
| } | ||
| buyerUser.trades_completed++; | ||
| await buyerUser.save(); | ||
| sellerUser.trades_completed++; | ||
| await sellerUser.save(); | ||
| if (pending) { | ||
| await messages.toAdminChannelPendingPaymentSuccessMessage( | ||
| bot, | ||
| buyerUser, | ||
| order, | ||
| pending, | ||
| payment, | ||
| i18nCtx, | ||
| ); | ||
| } | ||
| await messages.toBuyerPendingPaymentSuccessMessage( | ||
| bot, | ||
| buyerUser, | ||
| order, | ||
| payment, | ||
| i18nCtx, | ||
| ); | ||
| await messages.rateUserMessage(bot, buyerUser, order, i18nCtx); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make the completion path retryable
status: 'SUCCESS' is written before the user saves and notifications. If any later await throws, retries exit on the CAS (won === null), leaving a SUCCESS order with incomplete counters or missing messages. Wrap the DB writes in a transaction or persist an outbox/completion marker so the remaining side effects can be retried.
🤖 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 `@util/completeOrder.ts` around lines 28 - 61, The completion flow in
completeOrder is not retryable because Order.findOneAndUpdate writes SUCCESS
before buyerUser.save, sellerUser.save, and the message sends, so a later
failure leaves the order partially completed. Update completeOrder to make the
DB updates and completion side effects retry-safe by using a transaction/atomic
completion marker or an outbox pattern, and ensure the status transition, trade
counters, and pending/payment notifications can be resumed if any await fails.
| buyerUser.trades_completed++; | ||
| await buyerUser.save(); | ||
| sellerUser.trades_completed++; | ||
| await sellerUser.save(); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== file list ==\n'
git ls-files util/completeOrder.ts util | sed -n '1,120p'
printf '\n== outline util/completeOrder.ts ==\n'
ast-grep outline util/completeOrder.ts --view expanded || true
printf '\n== relevant lines util/completeOrder.ts ==\n'
nl -ba util/completeOrder.ts | sed -n '1,220p'
printf '\n== search trades_completed usages ==\n'
rg -n "trades_completed" -S . || true
printf '\n== search transaction/session usage in util ==\n'
rg -n "startSession|withTransaction|session:" util models jobs . -g '!node_modules' || trueRepository: lnp2pBot/bot
Length of output: 549
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== util/completeOrder.ts ==\n'
cat -n util/completeOrder.ts | sed -n '1,220p'
printf '\n== trades_completed references ==\n'
rg -n "trades_completed" -S util models jobs . -g '!node_modules' || true
printf '\n== transaction/session references near complete order flow ==\n'
rg -n "startSession|withTransaction|session:" util models jobs . -g '!node_modules' || true
printf '\n== related order helpers ==\n'
rg -n "completeOrderAsSuccess|completeOrder" util models jobs . -g '!node_modules' || trueRepository: lnp2pBot/bot
Length of output: 7676
Use atomic increments for trades_completed. findOneAndUpdate only guards the order transition; these save() calls are still read-modify-write and can drop increments if the same user completes multiple orders concurrently. Switch to $inc here, and wrap the order/status + counter updates in a transaction if they must stay coupled.
🤖 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 `@util/completeOrder.ts` around lines 40 - 43, The `trades_completed` updates
in `completeOrder` are still using read-modify-write via `buyerUser.save()` and
`sellerUser.save()`, which can lose increments under concurrent order
completions. Update the counter writes to use atomic `$inc` operations on the
user records instead of mutating the loaded documents, and if the order status
change must stay in sync with both counter updates, group the `findOneAndUpdate`
logic and the user counter updates in a transaction.
![ermeme[bot]](https://avatars.githubusercontent.com/in/3690068?s=60&v=4){kind=small}
There was a problem hiding this comment.
Strict review
Blocking issues remain in the payment recovery path:
payToBuyer()now fails closed on status-lookup errors, but it has no retry/pending-payment fallback. A transient LND lookup failure will silently drop the payout instead of deferring it safely.- The healing branch still requires
paymentto be present whenis_confirmedis true. Thelightningtypes allow confirmed responses without apaymentpayload, and that path can still fall through to a fresh pay attempt.
This is a sensitive path; I would not merge until both are made crash-safe and retry-safe.
| }; | ||
|
|
||
| export { payRequest, payToBuyer, isPendingPayment }; | ||
| const isPendingOrConfirmed = async (request: string) => { |
There was a problem hiding this comment.
Fail-closed here is the right instinct for preventing double-pays, but payToBuyer() has no retry/pending-payment fallback. This branch is only reached from the hold-invoice settlement flow, so a transient getPaymentStatus() error now drops the payout entirely. Please persist a retryable state or route this case through the existing pending-payment machinery instead of returning silently.
… extra defensive guard on pendingPayments
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ln/pay_request.ts (1)
237-265: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDon't coerce invoice-parse failures into
is_pending.parsePaymentRequest()is inside thetry, so a malformed stored invoice falls through to{ is_pending: true }.jobs/pending_payments.tstreats that as in-flight and skips retries, so the order can stall indefinitely instead of surfacing the bad invoice. Parse first, then only wrap the LND lookup/transport call.🤖 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 237 - 265, The getPaymentStatus flow is swallowing parsePaymentRequest failures and incorrectly returning is_pending for malformed invoices. Move parsePaymentRequest({ request }) out of the try/catch in getPaymentStatus so invoice parsing failures are not treated like transient LND lookup errors, and keep the catch only around getPayment({ lnd, id }) and related transport errors. Preserve the existing not-found handling for the LND response, but ensure bad stored invoices surface as failures instead of being coerced into pending by jobs/pending_payments.ts.
🤖 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.
Outside diff comments:
In `@ln/pay_request.ts`:
- Around line 237-265: The getPaymentStatus flow is swallowing
parsePaymentRequest failures and incorrectly returning is_pending for malformed
invoices. Move parsePaymentRequest({ request }) out of the try/catch in
getPaymentStatus so invoice parsing failures are not treated like transient LND
lookup errors, and keep the catch only around getPayment({ lnd, id }) and
related transport errors. Preserve the existing not-found handling for the LND
response, but ensure bad stored invoices surface as failures instead of being
coerced into pending by jobs/pending_payments.ts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0481fe4e-0a78-46c0-a40d-96dd717cdb80
📒 Files selected for processing (3)
ln/pay_request.tstests/bot/pending-payments.spec.tsutil/completeOrder.ts
💤 Files with no reviewable changes (1)
- util/completeOrder.ts
There was a problem hiding this comment.
Code Review Summary
Verdict: Changes Requested
I re-read the PR conversation and the current head. The direction is good, but I still see two funds-safety issues that need to be fixed before merge:
Blocking
jobs/pending_payments.ts: the retry gate still requirescurrentStatus.paymentwhenis_confirmedis true. A confirmed payment without the optional payload should not be treated as retryable — that creates a re-pay window in a funds-sensitive flow.bot/scenes.ts: when the original payment is confirmed but the payment payload is missing, the code marks the orderSUCCESSand exits. In a loss-of-funds edge case, I would rather see a safe manual/operational resolution than a partial success path that can hide an inconsistent settlement state.
Why this is blocking
- These are exactly the kinds of edge cases where we should fail closed and force manual resolution if the code cannot prove the payment state safely.
- The rest of the recovery logic is moving in the right direction, but these two branches still leave room for unsafe ambiguity.
If you want, I can re-check the next iteration once these branches are tightened.
grunch
left a comment
grunch
left a comment
There was a problem hiding this comment.
Strict review. The architecture (double-pay prevention after restart) is sound and well-tested — I built the branch and ran the new suites (15/15 pass). I'm requesting changes for one real bug in the new code.
🔴 HIGH — bot is undefined in the PHI-wizard heal path
bot/scenes.ts:133 (in addInvoicePHIWizard):
const bot = ctx.wizard.bot; // undefined at runtimeThe bot is passed into scene state — bot/commands.ts:684:
ctx.scene.enter('ADD_INVOICE_PHI_WIZARD_SCENE_ID', { order, buyer, bot });so it lives at ctx.wizard.state.bot, exactly how the sibling addInvoiceWizard reads it (bot/scenes.ts:53: let { bot, buyer, seller, order } = ctx.wizard.state).
ctx.wizard.bot only compiles because CommunityWizard declares an aspirational bot: Telegraf<...> field (communityContext.ts:60) — but nothing in the codebase ever assigns ctx.wizard.bot (the only reference is this new line), and Telegraf does not populate it.
Failure scenario: the buyer's original invoice was already settled when they re-run /setinvoice (the restart-mid-payment case this PR targets) → originalStatus.is_confirmed is true → completeOrderAsSuccess(bot = undefined, …) runs. Inside, the order is CAS-flipped to SUCCESS and both users' trades_completed are incremented and saved, then messages.toBuyerPendingPaymentSuccessMessage(undefined, …) calls bot.telegram.sendMessage → TypeError, which the wizard's try/catch swallows (just logger.error). Net result: the order silently closes but the buyer is never told they were paid and gets no rating prompt. No fund loss / no double-pay, but it breaks the user-facing half of the exact feature being added — and the new tests don't cover this wizard path, so CI stays green.
Fix: const { bot } = ctx.wizard.state; (match addInvoiceWizard). A test that drives the is_confirmed heal branch and asserts the buyer notification fires would lock it in.
🟡 MEDIUM — fail-closed can wedge a pending payment indefinitely (by design, but unobserved)
getPaymentStatus returns is_pending: true on any unknown LND error (correct — avoids double-pay), so the job skips the retry without incrementing attempts. For a persistent non-404 fault (LND misconfig, cert/permission error), that pending payment retries every cycle forever, never expires, and never alerts an admin. Right safety tradeoff, but consider a cap/alert after N consecutive unknown-error skips so a stuck payout becomes visible. Not blocking.
🟢 What's correct / good
completeOrderAsSuccessidempotency: the atomicfindOneAndUpdate({ _id, status: { $ne: 'SUCCESS' } })CAS correctly gates the side-effects (notify, rating,trades_completed) so a job/wizard race can't double-notify or double-count. The unconditionalorder.save()in the job'sfinallyonly re-writesstatus/routing_feewith the same value, so it doesn't defeat the CAS.attempts++moved to just before the realpayRequest— in-flight/healed skips no longer burn retry budget.- Real pre-existing bug fixed: the community loop's
if (isPending) return;(which aborted the whole batch) is now a per-itemcontinue. getPaymentStatuserror parsing handles thelightninglib's array-shaped errors[code, message, details]and treats only 404 /PaymentNotFoundas "safe to pay". Well covered by the new fail-closed / not-found / happy-path tests.- Community confirmed/expired branches correctly rely on
finally { await pending.save() }to persistpaid/is_invoice_expired— verified.
Once the ctx.wizard.bot fix lands (ideally with a covering test), this is an easy approve.
…admin channel This commit adds a new ERROR state for orders. Orders in ERROR state may require manual resolution.
There was a problem hiding this comment.
Code Review Summary
Verdict: Request changes (funds-safety consistency)
I re-read the full PR conversation and verified the current head (9f7cc2b) against the prior blockers from the last two review rounds.
Previous blockers — resolved on this head
- ✅
bot/scenes.ts:133(grunch's HIGH):ctx.wizard.botis gone. The PHI wizard now readsconst { bot } = ctx.wizard.state;(same pattern as the siblingaddInvoiceWizard). No moreundefinedbot on the heal path. - ✅
jobs/pending_payments.tscurrent-payment branch (ermeme): theis_confirmedgate no longer requirescurrentStatus.payment. When confirmed but payload is missing, the code now moves the order toERRORand logs to admin instead of re-paying. Double-pay window closed on that branch. - ✅
bot/scenes.ts:181-215(ermeme): theoriginalStatus.is_confirmed && !paymentcase inaddInvoicePHIWizardnow fails closed toERROR+logOrderError+ user-facing error instead of silently markingSUCCESS.
Remaining blocker — inconsistent fail-closed policy in the job
The same funds-safety fix was applied to scenes.ts and to the current pending payment branch, but the two sibling branches earlier in jobs/pending_payments.ts still silently mark SUCCESS when confirmed-without-payload — creating a partial-success state (no routing_fee, no trades_completed++, no buyer/admin notifications, no rating prompt) and inconsistent policy across paths reachable from the same job.
jobs/pending_payments.ts:60-66—originalStatus.is_confirmed && !originalStatus.paymentonorder.buyer_invoice: still doesorder.status = 'SUCCESS'; pending.paid = true; logger.warn(...).jobs/pending_payments.ts:138-143—prevStatus.is_confirmed && !prevStatus.paymenton a previous pending payment: still doesorder.status = 'SUCCESS'; logger.warn(...).
Please align these two branches with the policy already used at jobs/pending_payments.ts:162-178 and bot/scenes.ts:196-213: mark the order as ERROR, call logOrderError() so the admin channel is notified, and leave it for manual/operational resolution. That way every "confirmed but no payload" path fails closed the same way and no code path can produce a "phantom" completed order.
Non-blocking
- No new blockers on the current head beyond the consistency fix above.
There was a problem hiding this comment.
Code Review Summary
Verdict: Request changes
I re-read the full PR conversation and verified the current head (f0c1d9a).
Previous blockers — resolved on this head
- ✅
jobs/pending_payments.ts:60-73— theoriginalStatus.is_confirmed && !paymentbranch (buyer_invoice) now fails closed: moves the order toERROR, callslogOrderError, and skips the retry. No more silent partial-SUCCESS. - ✅
jobs/pending_payments.ts:145-158— theprevStatus.is_confirmed && !paymentbranch (previous pending payment) applies the same ERROR +logOrderErrorpolicy. - ✅ Both new branches match the policy already used at
jobs/pending_payments.ts:174-192andbot/scenes.ts:196-213. Consistency achieved on the buyer flow.
Remaining blocker — same double-pay pattern still open in attemptCommunitiesPendingPayments
attemptCommunitiesPendingPayments (jobs/pending_payments.ts:366) still gates the confirmed branch on both conditions:
if (status.is_confirmed && status.payment) { ... continue; }
if (status.is_pending) { ... continue; }
// falls through to payRequest(...)If LND returns is_confirmed: true with a missing/undefined payment payload, line 366 is false, line 395 (is_pending) is false, and execution falls through to pending.attempts++ and then to payRequest(...) at line 412 — re-paying a community withdrawal that LND already confirmed. This is the exact same double-pay pattern that CodeRabbit originally flagged for both attemptPendingPayments and attemptCommunitiesPendingPayments back on 2026-06-23; the fix was applied to the buyer flow (and later tightened to ERROR) but the community flow was left in the pre-fix shape.
Please apply the same fail-closed policy here as well: gate the skip on is_confirmed alone, and when payment is missing, do not re-pay — flag the pending as terminal (for example pending.is_invoice_expired = true + pending.paid = true with last_error populated, or an equivalent "needs manual resolution" marker), notify the admin channel, and continue. Otherwise the whole point of this PR (preventing double-pays after restart) still has a hole on the community withdrawal path.
Non-blocking
- No other blockers on the current head.
| try { | ||
| const status = await getPaymentStatus(pending.payment_request); | ||
|
|
||
| if (status.is_confirmed && status.payment) { |
There was a problem hiding this comment.
Blocking — same double-pay pattern the PR is supposed to close, still open here. When status.is_confirmed is true but status.payment is missing/undefined this branch is skipped, the is_pending branch below is also false, and execution falls through to pending.attempts++ and payRequest(...) at line 412 — re-paying a community withdrawal that LND already confirmed. This is exactly the double-pay pattern CodeRabbit flagged on 2026-06-23 for both attemptPendingPayments and attemptCommunitiesPendingPayments; the buyer function was fixed (and later tightened at lines 60-73, 145-158 and 174-192) but the community function was left in the pre-fix shape. Please gate the skip on is_confirmed alone and, when payment is missing, avoid re-paying by marking the pending as terminal (e.g. pending.paid = true + pending.is_invoice_expired = true + last_error), notify the admin channel, and continue — same fail-closed intent as the buyer flow.
4 participants
Summary by CodeRabbit