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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 docstrings
🧪 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.
| 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.
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
| let { buyer, order } = ctx.wizard.state; | ||
| const bot = ctx.wizard.bot; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether the project defines or uses a custom wizard.bot property.
rg -n "wizard\\.bot|interface CommunityContext|type CommunityContext|extends Context" --type ts .Repository: lnp2pBot/bot
Length of output: 372
Pass the bot instance explicitly here. bot/scenes.ts:133 — ctx.wizard.bot isn’t part of the wizard context, so completeOrderAsSuccess can receive undefined and the healed-payment path can fail before sending the completion messages.
🤖 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 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.
| 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.
|
|
||
| const currentStatus = await getPaymentStatus(pending.payment_request); | ||
|
|
||
| if (currentStatus.is_confirmed && currentStatus.payment) { |
There was a problem hiding this comment.
GetPaymentResult.payment is optional even when is_confirmed is true. Gating the healing path on currentStatus.payment means a confirmed payment without the optional payload falls through to the retry branch and can double-pay. is_confirmed should be enough to skip/reconcile; if payment details are missing, recover from the stored order/pending row instead of calling payRequest() again.
… 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
4 participants
Summary by CodeRabbit