Fix/cancelall pending only#848
Conversation
|
Warning Review limit reached
More reviews will be available in 31 minutes and 44 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. Walkthrough
ChangesCancel-all order scope
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Possibly related issues
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 |
Matobi98
force-pushed
the
fix/cancelall-pending-only
branch
from
b13b91b to
b7b6eaa
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
tests/bot/pending-payments.spec.ts (1)
1-2: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove this job spec under the mirrored
tests/jobs/layout.This tests
jobs/pending_payments.ts, but it is placed intests/bot/. As per coding guidelines, “Write Mocha + Chai specs intests/**with.spec.tssuffix, mirroring source layout.”🤖 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 1 - 2, The pending payments spec is in the wrong test area and should mirror the source layout for jobs. Move the `pending-payments.spec.ts` test that covers `attemptPendingPayments` from `tests/bot/` into the matching `tests/jobs/` structure, keeping the same `.spec.ts` naming so it aligns with `jobs/pending_payments.ts` and the repo’s mirrored test layout convention.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bot/scenes.ts`:
- Around line 181-200: The fallback in `bot/scenes.ts` inside the
confirmed-order branch is incorrectly persisting `order.status = 'SUCCESS'` when
`originalStatus.payment` or `sellerUser` is missing, bypassing the full success
flow in `completeOrderAsSuccess`. Change this path to fail closed: do not save
SUCCESS here, and instead log/alert the missing data and return early so only
the shared success routine handles confirmed orders with all required inputs.
In `@bot/start.ts`:
- Around line 505-509: The /cancelall flow in bot/start.ts is treating a failed
ordersActions.getOrders(ctx.user, 'PENDING') call as an empty result by
collapsing undefined into an empty array. Update the /cancelall logic to
distinguish a real no-orders case from a fetch failure: either let getOrders
propagate the error or handle the error path separately before deciding there
are no pending orders. Use the getOrders method in bot/ordersActions.ts and the
/cancelall handler in start.ts as the key points to adjust.
In `@jobs/pending_payments.ts`:
- Around line 40-67: The confirmed-without-payment path in the pending payments
flow is being treated inconsistently and can be retried as if it were still
pending. In the order-processing branches of pending_payments.ts (including the
logic around original/community status handling and completeOrderAsSuccess),
check is_confirmed first and treat missing payment details as terminal: log an
error/admin alert, mark the order as non-retryable, and do not fall through into
the payment retry path or save a partial success state. Ensure the same behavior
is applied across the original, current, and community branches so confirmed
orders without payment do not get reprocessed.
In `@ln/pay_request.ts`:
- Around line 126-130: The current payToBuyer flow treats both pending and
confirmed invoices as an early return, which skips the normal success handling
for confirmed payments. Update the status check around isPendingOrConfirmed so
only is_pending exits early; when the invoice is is_confirmed, continue through
the same success path that marks the order SUCCESS, sets routing_fee, updates
reputation, and notifies the buyer. Use the existing payToBuyer success logic
and LND payment details, and if those details are unavailable for a confirmed
invoice, fail closed and raise an alert instead of returning silently.
In `@util/completeOrder.ts`:
- Around line 28-31: The success update in completeOrder should only apply to
orders in eligible payment-flow states, not any non-SUCCESS status. Update the
Order.findOneAndUpdate predicate to use an explicit allowed-state check for the
transition handled by completeOrder, so terminal states like CANCELED or
disputed cannot be flipped back to SUCCESS. Keep the change scoped to the status
filter around order._id and the status transition logic.
- Around line 40-43: The trade counter updates in completeOrder are currently
done by mutating buyerUser and sellerUser and saving the documents, which can
lose concurrent increments. Update the logic in completeOrder to use atomic $inc
operations for trades_completed on both user records, then refresh or adjust the
in-memory buyerUser and sellerUser objects if later code in the flow relies on
the updated counts.
---
Nitpick comments:
In `@tests/bot/pending-payments.spec.ts`:
- Around line 1-2: The pending payments spec is in the wrong test area and
should mirror the source layout for jobs. Move the `pending-payments.spec.ts`
test that covers `attemptPendingPayments` from `tests/bot/` into the matching
`tests/jobs/` structure, keeping the same `.spec.ts` naming so it aligns with
`jobs/pending_payments.ts` and the repo’s mirrored test layout convention.
🪄 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: 111ff79d-f6dc-4342-b1e1-d37d1cbc780c
📒 Files selected for processing (9)
bot/modules/community/scenes.tsbot/scenes.tsbot/start.tsjobs/pending_payments.tsln/index.tsln/pay_request.tstests/bot/pending-payments.spec.tstests/ln/get-payment-status.spec.tsutil/completeOrder.ts
Luquitasjeffrey
left a comment
Luquitasjeffrey
left a comment
There was a problem hiding this comment.
tACK, i think this is the intended behavior of the /cancelall command, the 'cancelall_success' message shows to the user "You have cancelled all your published orders!" which from my understanding means that it cancels all untaken orders.
grunch
left a comment
grunch
left a comment
There was a problem hiding this comment.
Review — approve ✅
Strict review of the /cancelall change. The fix is correct, minimal, and a net improvement.
What it does
Previously /cancelall collected PENDING, WAITING_BUYER_INVOICE and WAITING_PAYMENT orders and ran per-order hold-invoice logic inside the loop. The sell + WAITING_PAYMENT case had no branch, so it fell through to the generic CANCELED block: the order was marked CANCELED in the DB but the hold invoice was never canceled on LND and the seller wasn't notified. If the seller then paid, subscribeToInvoice saw the order already CANCELED and the funds were trapped (#829). The PR scopes /cancelall to PENDING orders only.
Strengths (verified)
- Fixes the trapped-funds bug correctly.
PENDINGorders have no locked funds, sostatus = CANCELED+save+deleteOrderFromChannelis the complete, safe operation. In-progress orders are left to/cancel/ dispute flows, which already handle hold-invoice cancellation, cooperative cancel and notifications (bot/commands.ts). - Also fixes a latent second bug. The old loop used
returninside thefor(e.g.sell+WAITING_BUYER_INVOICE→return cancelAddInvoice(...)), which aborted the entire/cancelallafter the first in-progress order, silently leaving the user's other pending orders uncanceled. That's gone now. - Good error discrimination.
getOrdersreturnsundefinedonly when the query throws. The newif (orders === undefined) return;avoids the previous|| []collapse that would show "you have no orders" on a DB failure and silently abandon real pending orders. - No dead imports left behind —
cancelAddInvoice,cancelShowHoldInvoice,cancelHoldInvoiceare all still used elsewhere instart.ts. No type errors introduced.
Non-blocking suggestions
- UX:
successCancelAllOrdersMessageis sent unconditionally. A user who still hasWAITING_PAYMENT/WAITING_BUYER_INVOICEorders gets a blanket "all orders canceled" message with no hint those were intentionally skipped. Consider telling the user that in-progress orders were left untouched and must be handled with/cancel. - Error feedback:
if (orders === undefined) return;returns with no user-facing message on a DB error (only logged). Acceptable, but a short error reply would be friendlier. - Tests: Consider a regression test asserting
/cancelallleaves asell+WAITING_PAYMENTorder untouched and cancels onlyPENDINGorders, to lock in the fix. - Housekeeping: the branch is a few commits behind
main; rebase before merge.
None of these block merging.
3 participants
Summary
Fixes the bug where a buyer running
/cancelallcould cancel asellorder that was still waiting for the seller to pay the hold invoice (WAITING_PAYMENT), trapping the seller's funds in LND./cancelallnow only cancelsPENDINGorders (published orders nobody has taken yet). Orders that are already being taken (WAITING_PAYMENT/WAITING_BUYER_INVOICE) are simply ignored — the command always runs and never blocks on them. Those orders must be handled individually with/cancelor through a dispute.Closes #829
Problem
The
/cancelallhandler collected orders in three states (PENDING,WAITING_BUYER_INVOICE,WAITING_PAYMENT) and tried to cancel all of them, with special-case hold invoice logic inside the loop. The casesell+WAITING_PAYMENTwas not handled, so the order fell through to the genericCANCELEDblock:If the seller then paid the invoice,
subscribeToInvoicefound the order alreadyCANCELEDand did nothing, leaving the funds permanently trapped:Changes
bot/start.ts—/cancelallnow only fetches and cancelsPENDINGorders. Removed the per-order hold invoice logic from the loop (no longer needed, since pending orders have no locked funds). In-progress orders are left untouched and do not block the command.Summary by CodeRabbit