Add settled_by_admin field in order.ts model and improve getDetailedOrder output.#747
Add settled_by_admin field in order.ts model and improve getDetailedOrder output.#747Luquitasjeffrey wants to merge 9 commits intomainfrom
Conversation
…d_by_admin' field denoting that, and modified the 'getDetailedOrder' function output to show if an order was completed by an admin or not
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughReplaces the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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
🤖 Fix all issues with AI agents
In `@bot/start.ts`:
- Around line 547-551: The code currently sets order.settled_by_admin = true and
calls await order.save() before calling settleHoldInvoice, which can leave the
flag persisted if settleHoldInvoice throws; change the flow so you first call
await settleHoldInvoice({ secret: order.secret }) and only after it succeeds set
order.settled_by_admin = true and call await order.save(), or wrap both calls in
a try/catch and roll back (reset) order.settled_by_admin and avoid saving if
settleHoldInvoice fails; refer to the order object, its save() method, and the
settleHoldInvoice function when updating the logic.
In `@locales/fa.yaml`:
- Around line 183-184: Replace the English label "Settled by admin:
${settledByAdmin}" in the Persian locale with a Persian translation to keep
localization consistent; update the entry in locales/fa.yaml (the string
"Settled by admin: ${settledByAdmin}") to a Persian equivalent such as "تسویه
شده توسط ادمین: ${settledByAdmin}" ensuring the placeholder ${settledByAdmin}
remains unchanged.
🚨 Request Changes - Critical Issues FoundI need to request changes before this PR can be merged. There are critical security and data integrity issues that must be addressed: 1. BREAKING CHANGE without Data Migration (CRITICAL)Removing "COMPLETED_BY_ADMIN" from the status enum is a breaking change that will break existing orders in production. Problem: Orders currently in "COMPLETED_BY_ADMIN" status will be in an invalid state after deployment. Required: Add a database migration script that:
Without this migration, existing orders will be unreachable and potentially cause runtime errors. 2. Data Integrity Issue in bot/start.ts (SECURITY)Current (WRONG): order.settled_by_admin = true;
await order.save();
await settleHoldInvoice({ secret: order.secret });Problem: If settleHoldInvoice() fails, the order is already marked as settled by admin but the hold invoice was NOT settled. This creates an inconsistent state where the database says "settled by admin" but the Lightning payment did not complete. Required (CORRECT): try {
await settleHoldInvoice({ secret: order.secret });
order.settled_by_admin = true;
await order.save();
} catch (error) {
// Handle error - do NOT mark as settled_by_admin
throw error;
}The settlement must succeed BEFORE marking the order as settled by admin. 3. Schema Issue - Optional FieldIn models/order.ts: settled_by_admin?: boolean; // Optional - BADProblem: Optional fields can be undefined, leading to inconsistent data. Required: settled_by_admin: { type: Boolean, default: false, required: true }4. Missing CodeRabbit ReviewThis PR has been pending CodeRabbit review. I should have waited for that automated analysis before reviewing. Please ensure all automated checks pass. Summary of Required Changes
This is financial software handling real Bitcoin. We cannot merge breaking changes without proper migration and data integrity guarantees. Status: 🔴 Request Changes - DO NOT MERGE until issues are resolved. |
|
Hey @Luquitasjeffrey, friendly reminder — the issues from my previous review are still unresolved. The last two commits are upstream merges, no fixes. Here's a quick recap: 1. 🔴 Missing database migration (CRITICAL)You're removing // Find orders with the old status and migrate them
db.orders.updateMany(
{ status: "COMPLETED_BY_ADMIN" },
{ $set: { status: "PAID_HOLD_INVOICE", settled_by_admin: true } }
)2. 🔴 Order of operations in
|
…correcting order of operations in bot/start.ts due to a potential failure while settling the hold invoice
There was a problem hiding this comment.
♻️ Duplicate comments (1)
bot/start.ts (1)
547-551:⚠️ Potential issue | 🔴 CriticalSet
settled_by_adminbeforesettleHoldInvoiceto avoid a race.At Line 548, settlement can trigger async order-update handlers before Line 549–550 runs, which can leave inconsistent state for admin-settled orders.
Suggested fix
if (order.secret) { - await settleHoldInvoice({ secret: order.secret }); order.settled_by_admin = true; await order.save(); + await settleHoldInvoice({ secret: order.secret }); }Based on learnings: In bot/start.ts /settleorder command,
order.settled_by_admin = truemust be set and saved before callingsettleHoldInvoice({ secret: order.secret })to prevent a race condition.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bot/start.ts` around lines 547 - 551, The order settlement code can race because settleHoldInvoice(...) may trigger async handlers that read order state before we mark it as admin-settled; update the sequence so you set order.settled_by_admin = true and await order.save() before calling settleHoldInvoice({ secret: order.secret }) (refer to the settleorder command, order.settled_by_admin, order.save, and settleHoldInvoice symbols) so the persisted flag exists for any downstream handlers.
🧹 Nitpick comments (1)
scripts/migrate_completed_by_admin_orders.ts (1)
30-35: Preferfinallyfor deterministic connection cleanup.Line 33–35 exits on error without guaranteed close. Consider using
finallyto always close the Mongo connection and setprocess.exitCodeinstead of immediateprocess.exit(...).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/migrate_completed_by_admin_orders.ts` around lines 30 - 35, The migration currently calls mongoose.connection.close() only on success and uses process.exit(...) inside the catch which can leave the DB open; refactor the try/catch to a try/catch/finally where you always await mongoose.connection.close() in the finally block (handling a potentially undefined connection) and replace direct calls to process.exit(0)/process.exit(1) with setting process.exitCode = 0 or 1 so shutdown can occur deterministically; update references in this file to remove immediate process.exit usage and ensure logger.error(`Migration failed: ${error}`) remains in the catch before setting process.exitCode.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@bot/start.ts`:
- Around line 547-551: The order settlement code can race because
settleHoldInvoice(...) may trigger async handlers that read order state before
we mark it as admin-settled; update the sequence so you set
order.settled_by_admin = true and await order.save() before calling
settleHoldInvoice({ secret: order.secret }) (refer to the settleorder command,
order.settled_by_admin, order.save, and settleHoldInvoice symbols) so the
persisted flag exists for any downstream handlers.
---
Nitpick comments:
In `@scripts/migrate_completed_by_admin_orders.ts`:
- Around line 30-35: The migration currently calls mongoose.connection.close()
only on success and uses process.exit(...) inside the catch which can leave the
DB open; refactor the try/catch to a try/catch/finally where you always await
mongoose.connection.close() in the finally block (handling a potentially
undefined connection) and replace direct calls to
process.exit(0)/process.exit(1) with setting process.exitCode = 0 or 1 so
shutdown can occur deterministically; update references in this file to remove
immediate process.exit usage and ensure logger.error(`Migration failed:
${error}`) remains in the catch before setting process.exitCode.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: aae5b76a-95fe-43af-8241-9fac1e0f31ae
📒 Files selected for processing (3)
bot/start.tsscripts/migrate_completed_by_admin_orders.tstsconfig.json
✅ Files skipped from review due to trivial changes (1)
- tsconfig.json
I addressed your review.
|
![mostronatorcoder[bot]](https://avatars.githubusercontent.com/in/2922838?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
This PR removes the COMPLETED_BY_ADMIN status and replaces it with a settled_by_admin boolean field. The design is architecturally correct and includes a critical fix for operation ordering.
✅ Strengths
1. Correct architectural design
- Removing
COMPLETED_BY_ADMINstatus and replacing it with asettled_by_adminboolean is the right approach - Status should reflect workflow state, not who completed it
- Boolean flag captures metadata without polluting the state machine
2. Migration script included
scripts/migrate_completed_by_admin_orders.tsmigrates existing orders- Transforms
status: COMPLETED_BY_ADMIN→status: PAID_HOLD_INVOICE+settled_by_admin: true - Safe logic using
updateMany
3. Complete status cleanup
- All
COMPLETED_BY_ADMINreferences removed except:- Migration script (correct, necessary)
- Locale files (message translations, OK)
4. Critical fix: Operation ordering in bot/start.ts
Before:
if (order.secret) await settleHoldInvoice({ secret: order.secret });
order.status = 'COMPLETED_BY_ADMIN'; // ❌ Status changes even if settle fails
await order.save();Problem: If settleHoldInvoice() throws, order ends up in COMPLETED_BY_ADMIN status but the invoice is NOT settled in LND → critical inconsistency.
Now:
if (order.secret) {
await settleHoldInvoice({ secret: order.secret }); // ✅ If fails, exception exits here
order.settled_by_admin = true; // ✅ Only executes if settle OK
await order.save();
}Analysis: ✅ CORRECT
- If
settleHoldInvoice()fails → exception exits, nothing modified - If settle OK → marks
settled_by_admin = trueand saves - Status remains
PAID_HOLD_INVOICE(correct state after settle)
This is a real bug fix.
5. Display in checkorder
- Now shows "Settled by admin: Yes/No" in
/checkorderoutput - Useful for audit and debugging
⚠️ Issues Found
🔴 CRITICAL: Missing tests
The PR modifies:
- Critical business logic (settle flow in
bot/start.ts) - Data model (
models/order.ts) - Order queries (
bot/commands.ts,bot/modules/block/commands.ts)
But:
git diff cb1838e..7e9a117 -- tests/
(no output)No new or updated tests.
Test cases that SHOULD exist:
1. Migration script
it('should migrate COMPLETED_BY_ADMIN orders to PAID_HOLD_INVOICE + settled_by_admin', async () => {
// Given: order with status COMPLETED_BY_ADMIN
// When: run migration
// Then: status = PAID_HOLD_INVOICE && settled_by_admin = true
});2. Admin settle flow (bot/start.ts)
it('should set settled_by_admin when admin settles with secret', async () => {
// Given: order with secret
// When: admin executes settle
// Then: settled_by_admin = true
});
it('should not modify order if settleHoldInvoice fails', async () => {
// Given: order with invalid secret
// When: admin executes settle (LND fails)
// Then: order status/flags unchanged
});3. Display in checkorder
it('should show "Settled by admin: Yes" when settled_by_admin = true', async () => {
// Given: order with settled_by_admin = true
// When: /checkorder
// Then: output contains "Settled by admin: Yes"
});4. Block queries
it('should exclude settled orders from pending count', async () => {
// Given: orders with settled_by_admin = true
// When: user tries /block
// Then: those orders do NOT count as pending
});Impact: Without tests, any regression in this logic will go unnoticed until production.
🟡 Minor: Unnecessary tsconfig.json change
+ "scripts/**/*"Adding scripts/ to tsconfig include is fine for compiling the migration script, BUT:
- Migration script runs once manually
- Doesn't need to be in production build
- Better approach: Run migration with
ts-nodedirectly, without modifying tsconfig
Not critical, but unnecessary noise in the PR.
🟡 Minor: Unrelated formatting changes
- "lib":["ES2021", "DOM"],
+ "lib": [
+ "ES2021",
+ "DOM"
+ ],This is just formatting. Better to do it in a separate "chore: prettier" commit to avoid contaminating the functional diff.
🔍 Logic Validation
Query in /setinvoice
Before:
const where = {
$and: [
{ buyer_id: user._id },
{
$or: [
{ status: 'PAID_HOLD_INVOICE' },
{ status: 'COMPLETED_BY_ADMIN' }, // ❌ No longer exists
],
},
],
};Now:
const where = {
buyer_id: user._id,
status: 'PAID_HOLD_INVOICE', // ✅ Simplified
};Analysis: ✅ CORRECT
- Previously searched for orders in
PAID_HOLD_INVOICEORCOMPLETED_BY_ADMIN - Now only searches
PAID_HOLD_INVOICE(the only valid status after migration) - Simpler and correct query
📊 Findings Summary
| Severity | Issue | Impact |
|---|---|---|
| 🔴 Critical | No tests | Risk of regression in settle logic and queries |
| 🟡 Minor | tsconfig include scripts/ | Noise in build config |
| 🟡 Minor | Formatting changes | Contaminated diff |
✅ Recommendations
BEFORE MERGE:
- Add tests for migration script
- Add tests for settle flow with
settled_by_admin - Add tests for checkorder/block queries
- Verify in staging that migration script works correctly
NICE TO HAVE:
- Separate formatting changes in separate commit
- Evaluate if tsconfig change is necessary (alternative:
ts-nodefor migration)
POST-MERGE:
- Add grep in CI to prevent accidental use of
COMPLETED_BY_ADMINin the future - Document in README or CHANGELOG the model change (breaking change for external integrations reading the DB)
🎯 Verdict
Status:
Reasons:
- Lack of tests is critical for settle logic and migration
- The code itself is correct and improves the system (fixes operation ordering bug)
- The architectural design is solid
Required action:
- Add minimum tests for migration and settle flow
- Validate migration in staging before merge
Once tests are added: ✅ APPROVE
Additional Notes
I verified all COMPLETED_BY_ADMIN references have been removed from queries:
git grep -n "COMPLETED_BY_ADMIN" | grep -v "locales/"
# Only found in migration script (correct)I also checked for potential broken queries:
git grep -n "status.*:" bot/ util/ models/ | grep -i "success\|paid_hold\|canceled\|expired"
# All queries updated correctlyThe PR is well-structured and the logic is sound. The only blocker is the missing test coverage for critical business logic.
2 participants
This PR removes the 'COMPLETED_BY_ADMIN' status in an Order and adds a new boolean field 'settled_by_admin' that, as the name suggests, denotes if an order was indeed completed or not by an admin.
Also, now when an admin uses the checkorder command, it will display if the order was completed by an admin
Summary by CodeRabbit
New Features
Updates
Chores