feat: add recurring pledge model, scheduler, and signing flow - #52
Conversation
WalkthroughChangesRecurring pledge flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant PledgeAPI
participant Scheduler
participant MongoDB
participant Stellar
User->>PledgeAPI: Create pledge
PledgeAPI->>MongoDB: Store active pledge
Scheduler->>MongoDB: Find due pledges
Scheduler->>MongoDB: Create due cycle and advance nextDueAt
Scheduler-->>User: Send pledge_due notification
User->>PledgeAPI: Initialize cycle
PledgeAPI->>MongoDB: Store pending transaction
PledgeAPI-->>User: Return unsigned XDR
User->>PledgeAPI: Submit signed XDR
PledgeAPI->>Stellar: Verify and submit payment
PledgeAPI->>MongoDB: Mark cycle paid and update pledge stats
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/models/Notification.js (1)
15-30: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winAdd
pledgeIdandcycleIdtoNotification.data
pledge_duenotifications now send those ids, butNotification.dataonly declarescourseId,bookId,spaceId,reelId, andcommentId. Mongoose drops undeclared nested keys on save, so the reminder loses the references needed for the pledge deep-link.🤖 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 `@src/models/Notification.js` around lines 15 - 30, Update the Notification schema’s data definition to declare pledgeId and cycleId alongside the existing courseId, bookId, spaceId, reelId, and commentId fields, ensuring pledge_due notification references are preserved when documents are saved.
🤖 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 `@src/controllers/stellar/pledgeController.js`:
- Around line 56-88: Wrap pausePledge, resumePledge, cancelPledge, and
listPledges in try/catch blocks matching the established error-handling pattern
used by initializePledgeCycle and submitPledgeCycle. Preserve their existing
success and not-found responses, and return caught failures through the
controller’s consistent { success, message, data } JSON error shape, including
malformed pledgeId and database errors.
- Around line 133-153: Update submitPledgeCycle to reject cycles whose status is
already "paid" before validating or submitting the signed payment, alongside the
existing missing-cycle and missing-transaction checks. Return the established
not-found or appropriate conflict response without modifying pledge streak or
lifetime totals, while preserving the normal processing path for unpaid cycles.
- Around line 90-96: Update getPledgeStats so nextDueAt is selected from the
earliest valid upcoming due date rather than pledges[0]. Exclude paused or
cancelled pledges when determining the next due pledge, sort or reduce the
remaining pledges by nextDueAt, and preserve null when no eligible pledge has a
due date.
- Around line 22-54: Update createPledge to validate anchorDay before
Pledge.create, rejecting missing, non-integer, or out-of-range values according
to the schema’s 1–31 constraint with the same transaction-abort and 400-response
pattern used for publicKey, amount, and cadence. Keep persistence unchanged for
valid anchorDay values.
- Around line 98-131: Update initializePledgeCycle to require pledge.status to
be "active" after loading and ownership validation, rejecting paused, cancelled,
or other non-active pledges before building the payment transaction or saving
changes. Preserve the existing not-found response pattern and transaction-abort
behavior for this validation failure.
- Around line 154-183: Refactor the pledge-cycle flow around submitTransaction
and verifyPaymentOperations so Stellar network calls occur outside the MongoDB
transaction, keeping only document updates inside the session. In the
verification-failure branch, set transaction.status to "failed", persist it,
increment paymentsFailed with the donation type, then commit and return the
existing 400 response; preserve the confirmed updates and paymentsConfirmed
increment for successful verification.
In `@src/models/PledgeCycle.js`:
- Around line 3-32: The pledgeCycleSchema lacks the indexes and race handling
required by processDuePledges. Add a unique compound index on pledge and dueAt,
add a compound index on status and windowEndsAt for the lapsed-cycle sweep, and
update the processDuePledges upsert handling to catch and ignore duplicate-key
errors so one concurrent collision does not abort the batch.
In `@src/workers/pledgeScheduler.js`:
- Around line 9-32: Update computeNextDueAt so monthly targetDay preserves valid
anchorDay values through the days-in-month clamp; remove the premature cap at 28
while retaining the default fallback for missing values. Ensure anchor days
29–31 resolve to the lesser of the requested day and the next month’s actual
length, and update the affected leap-year test expectation from 28 to 29.
---
Outside diff comments:
In `@src/models/Notification.js`:
- Around line 15-30: Update the Notification schema’s data definition to declare
pledgeId and cycleId alongside the existing courseId, bookId, spaceId, reelId,
and commentId fields, ensuring pledge_due notification references are preserved
when documents are saved.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7e0aeb34-8408-4e9a-8608-aab0112f6452
📒 Files selected for processing (10)
app.jsserver.jssrc/controllers/stellar/pledgeController.jssrc/models/Notification.jssrc/models/Pledge.jssrc/models/PledgeCycle.jssrc/routes/stellar/pledgeRoutes.jssrc/workers/pledgeScheduler.jstest/pledgeScheduler.test.jstest/setupMongo.js
| export const createPledge = async (req, res) => { | ||
| const session = await mongoose.startSession(); | ||
| session.startTransaction(); | ||
| try { | ||
| const userId = req.user._id; | ||
| const { publicKey, amount, cadence, anchorDay } = req.body; | ||
|
|
||
| if (!publicKey || !isValidPublicKey(publicKey)) { | ||
| await session.abortTransaction(); | ||
| return res.status(400).json({ success: false, message: "Invalid Stellar public key" }); | ||
| } | ||
| if (!validatePledgeAmount(amount)) { | ||
| await session.abortTransaction(); | ||
| return res.status(400).json({ success: false, message: "Invalid amount. Must be a positive number with at most 7 decimal places" }); | ||
| } | ||
| if (!['daily', 'weekly', 'monthly'].includes(cadence)) { | ||
| await session.abortTransaction(); | ||
| return res.status(400).json({ success: false, message: "Invalid cadence" }); | ||
| } | ||
|
|
||
| const nextDueAt = new Date(Date.now()); | ||
| const pledge = await Pledge.create([{ user: userId, publicKey, amount: amount.toString(), cadence, anchorDay, nextDueAt, status: 'active' }], { session }); | ||
|
|
||
| await session.commitTransaction(); | ||
| res.status(201).json({ success: true, pledge: pledge[0] }); | ||
| } catch (error) { | ||
| await session.abortTransaction(); | ||
| logger.error("Create pledge error", error); | ||
| res.status(500).json({ success: false, message: "Failed to create pledge" }); | ||
| } finally { | ||
| session.endSession(); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
anchorDay isn't validated before persistence.
publicKey, amount, and cadence are all explicitly validated with dedicated 400 responses, but anchorDay is passed straight into Pledge.create unchecked. An out-of-range value (e.g. anchorDay: 99) will fail Mongoose's schema validator (min:1, max:31) inside the try block, fall into the generic catch (Line 47), and return a 500 "Failed to create pledge" instead of a 400 — inconsistent with the rest of this function's input-validation contract.
As per path instructions, src/**/*.js: "Flag missing auth/ownership checks, unvalidated request input, and unhandled promise rejections."
🔧 Proposed fix
if (!['daily', 'weekly', 'monthly'].includes(cadence)) {
await session.abortTransaction();
return res.status(400).json({ success: false, message: "Invalid cadence" });
}
+ if (anchorDay !== undefined && (!Number.isInteger(anchorDay) || anchorDay < 1 || anchorDay > 31)) {
+ await session.abortTransaction();
+ return res.status(400).json({ success: false, message: "Invalid anchor day" });
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const createPledge = async (req, res) => { | |
| const session = await mongoose.startSession(); | |
| session.startTransaction(); | |
| try { | |
| const userId = req.user._id; | |
| const { publicKey, amount, cadence, anchorDay } = req.body; | |
| if (!publicKey || !isValidPublicKey(publicKey)) { | |
| await session.abortTransaction(); | |
| return res.status(400).json({ success: false, message: "Invalid Stellar public key" }); | |
| } | |
| if (!validatePledgeAmount(amount)) { | |
| await session.abortTransaction(); | |
| return res.status(400).json({ success: false, message: "Invalid amount. Must be a positive number with at most 7 decimal places" }); | |
| } | |
| if (!['daily', 'weekly', 'monthly'].includes(cadence)) { | |
| await session.abortTransaction(); | |
| return res.status(400).json({ success: false, message: "Invalid cadence" }); | |
| } | |
| const nextDueAt = new Date(Date.now()); | |
| const pledge = await Pledge.create([{ user: userId, publicKey, amount: amount.toString(), cadence, anchorDay, nextDueAt, status: 'active' }], { session }); | |
| await session.commitTransaction(); | |
| res.status(201).json({ success: true, pledge: pledge[0] }); | |
| } catch (error) { | |
| await session.abortTransaction(); | |
| logger.error("Create pledge error", error); | |
| res.status(500).json({ success: false, message: "Failed to create pledge" }); | |
| } finally { | |
| session.endSession(); | |
| } | |
| }; | |
| export const createPledge = async (req, res) => { | |
| const session = await mongoose.startSession(); | |
| session.startTransaction(); | |
| try { | |
| const userId = req.user._id; | |
| const { publicKey, amount, cadence, anchorDay } = req.body; | |
| if (!publicKey || !isValidPublicKey(publicKey)) { | |
| await session.abortTransaction(); | |
| return res.status(400).json({ success: false, message: "Invalid Stellar public key" }); | |
| } | |
| if (!validatePledgeAmount(amount)) { | |
| await session.abortTransaction(); | |
| return res.status(400).json({ success: false, message: "Invalid amount. Must be a positive number with at most 7 decimal places" }); | |
| } | |
| if (!['daily', 'weekly', 'monthly'].includes(cadence)) { | |
| await session.abortTransaction(); | |
| return res.status(400).json({ success: false, message: "Invalid cadence" }); | |
| } | |
| if (anchorDay !== undefined && (!Number.isInteger(anchorDay) || anchorDay < 1 || anchorDay > 31)) { | |
| await session.abortTransaction(); | |
| return res.status(400).json({ success: false, message: "Invalid anchor day" }); | |
| } | |
| const nextDueAt = new Date(Date.now()); | |
| const pledge = await Pledge.create([{ user: userId, publicKey, amount: amount.toString(), cadence, anchorDay, nextDueAt, status: 'active' }], { session }); | |
| await session.commitTransaction(); | |
| res.status(201).json({ success: true, pledge: pledge[0] }); | |
| } catch (error) { | |
| await session.abortTransaction(); | |
| logger.error("Create pledge error", error); | |
| res.status(500).json({ success: false, message: "Failed to create pledge" }); | |
| } finally { | |
| session.endSession(); | |
| } | |
| }; |
🤖 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 `@src/controllers/stellar/pledgeController.js` around lines 22 - 54, Update
createPledge to validate anchorDay before Pledge.create, rejecting missing,
non-integer, or out-of-range values according to the schema’s 1–31 constraint
with the same transaction-abort and 400-response pattern used for publicKey,
amount, and cadence. Keep persistence unchanged for valid anchorDay values.
Source: Path instructions
| export const pausePledge = async (req, res) => { | ||
| const { pledgeId } = req.params; | ||
| const pledge = await Pledge.findOne({ _id: pledgeId, user: req.user._id }); | ||
| if (!pledge) return res.status(404).json({ success: false, message: "Pledge not found" }); | ||
| if (pledge.status === "cancelled") return res.status(400).json({ success: false, message: "Cancelled pledges cannot be paused" }); | ||
| pledge.status = "paused"; | ||
| await pledge.save(); | ||
| res.status(200).json({ success: true, pledge }); | ||
| }; | ||
|
|
||
| export const resumePledge = async (req, res) => { | ||
| const { pledgeId } = req.params; | ||
| const pledge = await Pledge.findOne({ _id: pledgeId, user: req.user._id }); | ||
| if (!pledge) return res.status(404).json({ success: false, message: "Pledge not found" }); | ||
| if (pledge.status === "cancelled") return res.status(400).json({ success: false, message: "Cancelled pledges cannot be resumed" }); | ||
| pledge.status = "active"; | ||
| await pledge.save(); | ||
| res.status(200).json({ success: true, pledge }); | ||
| }; | ||
|
|
||
| export const cancelPledge = async (req, res) => { | ||
| const { pledgeId } = req.params; | ||
| const pledge = await Pledge.findOne({ _id: pledgeId, user: req.user._id }); | ||
| if (!pledge) return res.status(404).json({ success: false, message: "Pledge not found" }); | ||
| pledge.status = "cancelled"; | ||
| await pledge.save(); | ||
| res.status(200).json({ success: true, pledge }); | ||
| }; | ||
|
|
||
| export const listPledges = async (req, res) => { | ||
| const pledges = await Pledge.find({ user: req.user._id }).sort({ createdAt: -1 }); | ||
| res.status(200).json({ success: true, pledges }); | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Missing error handling breaks the file's response-shape contract.
pausePledge, resumePledge, cancelPledge, and listPledges have no try/catch, unlike every other handler in this file. Express 5 will auto-forward a thrown/rejected error to the error-handling middleware (so the process won't crash), but the response won't be the { success, message, data } shape this file otherwise guarantees — e.g. a malformed pledgeId throws a Mongoose CastError that lands in whatever generic error handler app.js has configured, not this controller's consistent JSON contract. Wrap these in try/catch like initializePledgeCycle/submitPledgeCycle do.
As per path instructions, src/**/*.js: "consistent response shapes ({ success, message, data })" and "Flag missing auth/ownership checks, unvalidated request input, and unhandled promise rejections."
🤖 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 `@src/controllers/stellar/pledgeController.js` around lines 56 - 88, Wrap
pausePledge, resumePledge, cancelPledge, and listPledges in try/catch blocks
matching the established error-handling pattern used by initializePledgeCycle
and submitPledgeCycle. Preserve their existing success and not-found responses,
and return caught failures through the controller’s consistent { success,
message, data } JSON error shape, including malformed pledgeId and database
errors.
Source: Path instructions
| export const getPledgeStats = async (req, res) => { | ||
| const pledges = await Pledge.find({ user: req.user._id }); | ||
| const totalPaidStroops = pledges.reduce((sum, pledge) => sum + BigInt(pledge.totalPaidStroops || "0"), 0n); | ||
| const longestStreak = pledges.reduce((max, pledge) => Math.max(max, pledge.longestStreak || 0), 0); | ||
| const currentStreak = pledges.reduce((max, pledge) => Math.max(max, pledge.consecutivePaid || 0), 0); | ||
| res.status(200).json({ success: true, stats: { currentStreak, longestStreak, totalPaidStroops: totalPaidStroops.toString(), nextDueAt: pledges[0]?.nextDueAt || null } }); | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
nextDueAt in stats is an arbitrary pledge, not the soonest one.
Pledge.find({ user: req.user._id }) has no .sort(), so pledges[0] is whatever order MongoDB happens to return (typically insertion order), not the pledge with the earliest upcoming due date. A user with multiple pledges will get a nextDueAt in their stats that has nothing to do with what's actually next due — and it can even reflect a paused/cancelled pledge's stale value.
🔧 Proposed fix
export const getPledgeStats = async (req, res) => {
const pledges = await Pledge.find({ user: req.user._id });
const totalPaidStroops = pledges.reduce((sum, pledge) => sum + BigInt(pledge.totalPaidStroops || "0"), 0n);
const longestStreak = pledges.reduce((max, pledge) => Math.max(max, pledge.longestStreak || 0), 0);
const currentStreak = pledges.reduce((max, pledge) => Math.max(max, pledge.consecutivePaid || 0), 0);
- res.status(200).json({ success: true, stats: { currentStreak, longestStreak, totalPaidStroops: totalPaidStroops.toString(), nextDueAt: pledges[0]?.nextDueAt || null } });
+ const nextDueAt = pledges
+ .filter((p) => p.status === "active")
+ .reduce((min, p) => (!min || p.nextDueAt < min ? p.nextDueAt : min), null);
+ res.status(200).json({ success: true, stats: { currentStreak, longestStreak, totalPaidStroops: totalPaidStroops.toString(), nextDueAt } });
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const getPledgeStats = async (req, res) => { | |
| const pledges = await Pledge.find({ user: req.user._id }); | |
| const totalPaidStroops = pledges.reduce((sum, pledge) => sum + BigInt(pledge.totalPaidStroops || "0"), 0n); | |
| const longestStreak = pledges.reduce((max, pledge) => Math.max(max, pledge.longestStreak || 0), 0); | |
| const currentStreak = pledges.reduce((max, pledge) => Math.max(max, pledge.consecutivePaid || 0), 0); | |
| res.status(200).json({ success: true, stats: { currentStreak, longestStreak, totalPaidStroops: totalPaidStroops.toString(), nextDueAt: pledges[0]?.nextDueAt || null } }); | |
| }; | |
| export const getPledgeStats = async (req, res) => { | |
| const pledges = await Pledge.find({ user: req.user._id }); | |
| const totalPaidStroops = pledges.reduce((sum, pledge) => sum + BigInt(pledge.totalPaidStroops || "0"), 0n); | |
| const longestStreak = pledges.reduce((max, pledge) => Math.max(max, pledge.longestStreak || 0), 0); | |
| const currentStreak = pledges.reduce((max, pledge) => Math.max(max, pledge.consecutivePaid || 0), 0); | |
| const nextDueAt = pledges | |
| .filter((p) => p.status === "active") | |
| .reduce((min, p) => (!min || p.nextDueAt < min ? p.nextDueAt : min), null); | |
| res.status(200).json({ success: true, stats: { currentStreak, longestStreak, totalPaidStroops: totalPaidStroops.toString(), nextDueAt } }); | |
| }; |
🤖 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 `@src/controllers/stellar/pledgeController.js` around lines 90 - 96, Update
getPledgeStats so nextDueAt is selected from the earliest valid upcoming due
date rather than pledges[0]. Exclude paused or cancelled pledges when
determining the next due pledge, sort or reduce the remaining pledges by
nextDueAt, and preserve null when no eligible pledge has a due date.
| export const initializePledgeCycle = async (req, res) => { | ||
| const session = await mongoose.startSession(); | ||
| session.startTransaction(); | ||
| try { | ||
| const { pledgeId, cycleId } = req.params; | ||
| const pledge = await Pledge.findOne({ _id: pledgeId, user: req.user._id }).session(session); | ||
| if (!pledge) { | ||
| await session.abortTransaction(); | ||
| return res.status(404).json({ success: false, message: "Pledge not found" }); | ||
| } | ||
| const cycle = await PledgeCycle.findOne({ _id: cycleId, pledge: pledge._id }).session(session); | ||
| if (!cycle || cycle.status === "paid") { | ||
| await session.abortTransaction(); | ||
| return res.status(404).json({ success: false, message: "Cycle not found or already paid" }); | ||
| } | ||
| const memo = buildPledgeMemo(pledge._id); | ||
| const paymentTx = await buildPaymentTransaction({ sourcePublicKey: pledge.publicKey, destinationPublicKey: DONATION_WALLET_PUBLIC_KEY, amount: pledge.amount, memo }); | ||
| const sep7Uri = buildSep7Uri({ destination: DONATION_WALLET_PUBLIC_KEY, amount: pledge.amount, memo }); | ||
| const transaction = new Transaction({ type: "donation", buyer: req.user._id, buyerWallet: pledge.publicKey, creatorWallet: DONATION_WALLET_PUBLIC_KEY, amount: pledge.amount, network: NETWORK, status: "pending", expectedHash: paymentTx.hash, memo }); | ||
| await transaction.save({ session }); | ||
| cycle.transaction = transaction._id; | ||
| cycle.status = "notified"; | ||
| await cycle.save({ session }); | ||
| await session.commitTransaction(); | ||
| paymentsInitialized.inc({ type: "donation" }); | ||
| res.status(200).json({ success: true, transactionId: transaction._id, transactionXdr: paymentTx.xdr, sep7Uri, networkPassphrase: paymentTx.networkPassphrase }); | ||
| } catch (error) { | ||
| await session.abortTransaction(); | ||
| logger.error("Initialize pledge cycle error", error); | ||
| res.status(500).json({ success: false, message: "Failed to initialize pledge cycle" }); | ||
| } finally { | ||
| session.endSession(); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
initializePledgeCycle doesn't check the pledge is still active.
The PR objective calls for validating "pledge state transitions," but this endpoint only checks the cycle isn't already paid — it never checks pledge.status. A paused or cancelled pledge can still have its outstanding cycle initialized (and, per the next comment, submitted), silently letting a user pay on a pledge they explicitly stopped.
As per PR objectives: "Validate ownership, amounts, Stellar public keys, and pledge state transitions."
🔧 Proposed fix
if (!pledge) {
await session.abortTransaction();
return res.status(404).json({ success: false, message: "Pledge not found" });
}
+ if (pledge.status !== "active") {
+ await session.abortTransaction();
+ return res.status(400).json({ success: false, message: "Pledge is not active" });
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const initializePledgeCycle = async (req, res) => { | |
| const session = await mongoose.startSession(); | |
| session.startTransaction(); | |
| try { | |
| const { pledgeId, cycleId } = req.params; | |
| const pledge = await Pledge.findOne({ _id: pledgeId, user: req.user._id }).session(session); | |
| if (!pledge) { | |
| await session.abortTransaction(); | |
| return res.status(404).json({ success: false, message: "Pledge not found" }); | |
| } | |
| const cycle = await PledgeCycle.findOne({ _id: cycleId, pledge: pledge._id }).session(session); | |
| if (!cycle || cycle.status === "paid") { | |
| await session.abortTransaction(); | |
| return res.status(404).json({ success: false, message: "Cycle not found or already paid" }); | |
| } | |
| const memo = buildPledgeMemo(pledge._id); | |
| const paymentTx = await buildPaymentTransaction({ sourcePublicKey: pledge.publicKey, destinationPublicKey: DONATION_WALLET_PUBLIC_KEY, amount: pledge.amount, memo }); | |
| const sep7Uri = buildSep7Uri({ destination: DONATION_WALLET_PUBLIC_KEY, amount: pledge.amount, memo }); | |
| const transaction = new Transaction({ type: "donation", buyer: req.user._id, buyerWallet: pledge.publicKey, creatorWallet: DONATION_WALLET_PUBLIC_KEY, amount: pledge.amount, network: NETWORK, status: "pending", expectedHash: paymentTx.hash, memo }); | |
| await transaction.save({ session }); | |
| cycle.transaction = transaction._id; | |
| cycle.status = "notified"; | |
| await cycle.save({ session }); | |
| await session.commitTransaction(); | |
| paymentsInitialized.inc({ type: "donation" }); | |
| res.status(200).json({ success: true, transactionId: transaction._id, transactionXdr: paymentTx.xdr, sep7Uri, networkPassphrase: paymentTx.networkPassphrase }); | |
| } catch (error) { | |
| await session.abortTransaction(); | |
| logger.error("Initialize pledge cycle error", error); | |
| res.status(500).json({ success: false, message: "Failed to initialize pledge cycle" }); | |
| } finally { | |
| session.endSession(); | |
| } | |
| }; | |
| export const initializePledgeCycle = async (req, res) => { | |
| const session = await mongoose.startSession(); | |
| session.startTransaction(); | |
| try { | |
| const { pledgeId, cycleId } = req.params; | |
| const pledge = await Pledge.findOne({ _id: pledgeId, user: req.user._id }).session(session); | |
| if (!pledge) { | |
| await session.abortTransaction(); | |
| return res.status(404).json({ success: false, message: "Pledge not found" }); | |
| } | |
| if (pledge.status !== "active") { | |
| await session.abortTransaction(); | |
| return res.status(400).json({ success: false, message: "Pledge is not active" }); | |
| } | |
| const cycle = await PledgeCycle.findOne({ _id: cycleId, pledge: pledge._id }).session(session); | |
| if (!cycle || cycle.status === "paid") { | |
| await session.abortTransaction(); | |
| return res.status(404).json({ success: false, message: "Cycle not found or already paid" }); | |
| } | |
| const memo = buildPledgeMemo(pledge._id); | |
| const paymentTx = await buildPaymentTransaction({ sourcePublicKey: pledge.publicKey, destinationPublicKey: DONATION_WALLET_PUBLIC_KEY, amount: pledge.amount, memo }); | |
| const sep7Uri = buildSep7Uri({ destination: DONATION_WALLET_PUBLIC_KEY, amount: pledge.amount, memo }); | |
| const transaction = new Transaction({ type: "donation", buyer: req.user._id, buyerWallet: pledge.publicKey, creatorWallet: DONATION_WALLET_PUBLIC_KEY, amount: pledge.amount, network: NETWORK, status: "pending", expectedHash: paymentTx.hash, memo }); | |
| await transaction.save({ session }); | |
| cycle.transaction = transaction._id; | |
| cycle.status = "notified"; | |
| await cycle.save({ session }); | |
| await session.commitTransaction(); | |
| paymentsInitialized.inc({ type: "donation" }); | |
| res.status(200).json({ success: true, transactionId: transaction._id, transactionXdr: paymentTx.xdr, sep7Uri, networkPassphrase: paymentTx.networkPassphrase }); | |
| } catch (error) { | |
| await session.abortTransaction(); | |
| logger.error("Initialize pledge cycle error", error); | |
| res.status(500).json({ success: false, message: "Failed to initialize pledge cycle" }); | |
| } finally { | |
| session.endSession(); | |
| } | |
| }; |
🤖 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 `@src/controllers/stellar/pledgeController.js` around lines 98 - 131, Update
initializePledgeCycle to require pledge.status to be "active" after loading and
ownership validation, rejecting paused, cancelled, or other non-active pledges
before building the payment transaction or saving changes. Preserve the existing
not-found response pattern and transaction-abort behavior for this validation
failure.
| export const submitPledgeCycle = async (req, res) => { | ||
| const session = await mongoose.startSession(); | ||
| session.startTransaction(); | ||
| try { | ||
| const { pledgeId, cycleId } = req.params; | ||
| const { signedXdr } = req.body; | ||
| const pledge = await Pledge.findOne({ _id: pledgeId, user: req.user._id }).session(session); | ||
| if (!pledge) { | ||
| await session.abortTransaction(); | ||
| return res.status(404).json({ success: false, message: "Pledge not found" }); | ||
| } | ||
| const cycle = await PledgeCycle.findOne({ _id: cycleId, pledge: pledge._id }).session(session); | ||
| if (!cycle || !cycle.transaction) { | ||
| await session.abortTransaction(); | ||
| return res.status(404).json({ success: false, message: "Cycle not found" }); | ||
| } | ||
| const transaction = await Transaction.findById(cycle.transaction).session(session); | ||
| if (!transaction) { | ||
| await session.abortTransaction(); | ||
| return res.status(404).json({ success: false, message: "Transaction not found" }); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Missing cycle.status === "paid" guard lets a resubmission double-count streaks and lifetime totals.
initializePledgeCycle rejects an already-paid cycle (Line 109: cycle.status === "paid"), but submitPledgeCycle only checks !cycle || !cycle.transaction (Line 145) — it never rejects a cycle whose status is already "paid". A retried/duplicated submit request for a cycle that was already paid will re-run validateSignedPaymentXdr, resubmit to Stellar, and then re-increment consecutivePaid, longestStreak, and totalPaidStroops (Lines 171-174) a second time for the same payment — directly corrupting the pledge's lifetime/streak stats that the PR explicitly requires to be accurate ("Update streak and lifetime totals using integer stroop arithmetic").
🔧 Proposed fix
const cycle = await PledgeCycle.findOne({ _id: cycleId, pledge: pledge._id }).session(session);
if (!cycle || !cycle.transaction) {
await session.abortTransaction();
return res.status(404).json({ success: false, message: "Cycle not found" });
}
+ if (cycle.status === "paid") {
+ await session.abortTransaction();
+ return res.status(400).json({ success: false, message: "Cycle already paid" });
+ }Also applies to: 169-178
🤖 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 `@src/controllers/stellar/pledgeController.js` around lines 133 - 153, Update
submitPledgeCycle to reject cycles whose status is already "paid" before
validating or submitting the signed payment, alongside the existing
missing-cycle and missing-transaction checks. Return the established not-found
or appropriate conflict response without modifying pledge streak or lifetime
totals, while preserving the normal processing path for unpaid cycles.
| validateSignedPaymentXdr(signedXdr, [{ destination: transaction.creatorWallet, amount: transaction.amount }], transaction.memo, transaction.buyerWallet, true); | ||
| transaction.status = "submitted"; | ||
| await transaction.save({ session }); | ||
| paymentsSubmitted.inc({ type: "donation" }); | ||
| const result = await submitTransaction(signedXdr); | ||
| const verification = await verifyPaymentOperations(result.hash, [{ destination: transaction.creatorWallet, amount: transaction.amount }]); | ||
| if (!verification.verified) { | ||
| await session.commitTransaction(); | ||
| return res.status(400).json({ success: false, message: "Payment could not be verified" }); | ||
| } | ||
| transaction.stellarTxHash = result.hash; | ||
| transaction.stellarLedger = result.ledger; | ||
| transaction.status = "confirmed"; | ||
| transaction.confirmedAt = new Date(); | ||
| await transaction.save({ session }); | ||
| cycle.status = "paid"; | ||
| await cycle.save({ session }); | ||
| pledge.consecutivePaid = (pledge.consecutivePaid || 0) + 1; | ||
| pledge.longestStreak = Math.max(pledge.longestStreak || 0, pledge.consecutivePaid); | ||
| pledge.totalPaidStroops = (BigInt(pledge.totalPaidStroops || "0") + BigInt(toStroops(pledge.amount))).toString(); | ||
| pledge.lastPaidAt = new Date(); | ||
| await pledge.save({ session }); | ||
| await session.commitTransaction(); | ||
| paymentsConfirmed.inc({ type: "donation" }); | ||
| res.status(200).json({ success: true, message: "Pledge cycle paid" }); | ||
| } catch (error) { | ||
| await session.abortTransaction(); | ||
| logger.error("Submit pledge cycle error", error); | ||
| res.status(500).json({ success: false, message: "Failed to submit pledge cycle" }); | ||
| } finally { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🌐 Web query:
What is MongoDB's default transaction time limit and what happens to a session if a transaction stays open across a slow external network call?
💡 Result:
The default transaction time limit in MongoDB is 60 seconds [1][2]. This limit is controlled by the transactionLifetimeLimitSeconds server parameter [3][4]. If a transaction remains open—for example, due to a slow external network call—and exceeds this 60-second limit, it is considered expired. A periodic cleanup process on the MongoDB server will identify the expired transaction and abort it [3][4]. Because the transaction has been aborted by the server, any subsequent attempts to perform operations within that transaction (including the eventual commit) will fail, typically requiring the application to handle the error and restart the transaction logic [5][6].
Citations:
- 1: https://www.mongodb.com/docs/manual/reference/method/session.withtransaction/
- 2: https://www.mongodb.com/docs/manual/reference/limits/
- 3: https://www.mongodb.com/docs/manual/core/transactions-production-consideration/
- 4: https://www.mongodb.com/docs/v8.2/core/transactions-production-consideration/
- 5: https://www.mongodb.com/docs/manual/core/transactions/
- 6: https://specifications.readthedocs.io/en/latest/transactions/transactions/
🏁 Script executed:
sed -n '120,210p' src/controllers/stellar/pledgeController.jsRepository: Deen-Bridge/dnb-backend
Length of output: 3388
🏁 Script executed:
sed -n '1,120p' src/controllers/stellar/pledgeController.js
rg -n "paymentsFailed|status =" src/controllers/stellar/pledgeController.jsRepository: Deen-Bridge/dnb-backend
Length of output: 6900
Keep the MongoDB transaction short and mark failed submissions as failed
submitTransaction() and verifyPaymentOperations() run while the MongoDB session is still open, so a slow Stellar round trip can hold the transaction open long enough to time out. Move those network calls outside the DB transaction, or split the write so only the document updates are wrapped in the session.
When verification fails, transaction.status is still committed as "submitted". Set a terminal "failed" state before committing and increment paymentsFailed so failed payments don’t stay in limbo.
🤖 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 `@src/controllers/stellar/pledgeController.js` around lines 154 - 183, Refactor
the pledge-cycle flow around submitTransaction and verifyPaymentOperations so
Stellar network calls occur outside the MongoDB transaction, keeping only
document updates inside the session. In the verification-failure branch, set
transaction.status to "failed", persist it, increment paymentsFailed with the
donation type, then commit and return the existing 400 response; preserve the
confirmed updates and paymentsConfirmed increment for successful verification.
| const pledgeCycleSchema = new mongoose.Schema( | ||
| { | ||
| pledge: { | ||
| type: mongoose.Schema.Types.ObjectId, | ||
| ref: "Pledge", | ||
| required: true, | ||
| index: true, | ||
| }, | ||
| dueAt: { | ||
| type: Date, | ||
| required: true, | ||
| index: true, | ||
| }, | ||
| windowEndsAt: { | ||
| type: Date, | ||
| required: true, | ||
| }, | ||
| status: { | ||
| type: String, | ||
| enum: ["due", "notified", "paid", "skipped", "lapsed"], | ||
| default: "due", | ||
| index: true, | ||
| }, | ||
| transaction: { | ||
| type: mongoose.Schema.Types.ObjectId, | ||
| ref: "Transaction", | ||
| }, | ||
| }, | ||
| { timestamps: true } | ||
| ); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does MongoDB findOneAndUpdate with upsert:true guarantee no duplicate documents are created under concurrent calls without a unique index on the query fields?
💡 Result:
No, MongoDB does not guarantee that duplicate documents will be avoided under concurrent calls to findOneAndUpdate with upsert:true unless a unique index is present on the query fields [1][2][3]. When multiple concurrent operations execute with upsert:true and no matching document exists, all operations may perform the initial "query" phase and find no match [2][3]. Consequently, each of these operations may proceed to perform an insert, resulting in the creation of multiple documents that satisfy the filter criteria [2][3]. To prevent duplicate documents in this scenario, you must create a unique index on the fields used in the query filter [1][2]. With a unique index in place, if multiple concurrent operations attempt to upsert the same document, exactly one will succeed in the insert, while the others will either update the newly created document (if specific conditions are met) or fail with a duplicate key error (E11000) [2][3]. In the case of a duplicate key error, the application must be designed to handle the exception, such as by retrying the operation [4].
Citations:
- 1: https://www.mongodb.com/docs/manual/reference/method/db.collection.findoneandupdate/
- 2: https://www.mongodb.com/docs/manual/reference/method/db.collection.findandmodify/
- 3: https://www.mongodb.com/docs/manual/reference/command/update/
- 4: https://www.mongodb.com/community/forums/t/what-is-the-correct-way-to-do-an-upsert/294181
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the model and scheduler around the cited paths.
git ls-files src/models/PledgeCycle.js src/workers/pledgeScheduler.js src/server.js
echo '--- src/models/PledgeCycle.js ---'
cat -n src/models/PledgeCycle.js
echo '--- src/workers/pledgeScheduler.js ---'
cat -n src/workers/pledgeScheduler.js
echo '--- src/server.js (scheduler startup context) ---'
cat -n src/server.jsRepository: Deen-Bridge/dnb-backend
Length of output: 5422
Add a unique { pledge, dueAt } index. processDuePledges uses that pair as the natural key for the upsert, and without a unique constraint concurrent runs can still insert duplicate PledgeCycle documents. Catch the duplicate-key race around the upsert so one collision doesn’t abort the rest of the batch. A compound { status, windowEndsAt } index would also help the lapsed-cycle sweep.
🤖 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 `@src/models/PledgeCycle.js` around lines 3 - 32, The pledgeCycleSchema lacks
the indexes and race handling required by processDuePledges. Add a unique
compound index on pledge and dueAt, add a compound index on status and
windowEndsAt for the lapsed-cycle sweep, and update the processDuePledges upsert
handling to catch and ignore duplicate-key errors so one concurrent collision
does not abort the batch.
| export const computeNextDueAt = (fromDate, cadence, anchorDay = 1) => { | ||
| const date = new Date(fromDate); | ||
| const utcDate = new Date(Date.UTC(date.getUTCFullYear(), date.getUTCMonth(), date.getUTCDate(), 12, 0, 0)); | ||
|
|
||
| if (cadence === "daily") { | ||
| utcDate.setUTCDate(utcDate.getUTCDate() + 1); | ||
| return utcDate; | ||
| } | ||
|
|
||
| if (cadence === "weekly") { | ||
| utcDate.setUTCDate(utcDate.getUTCDate() + 7); | ||
| return utcDate; | ||
| } | ||
|
|
||
| const targetDay = Math.min(anchorDay || 1, 28); | ||
| const year = utcDate.getUTCFullYear(); | ||
| const month = utcDate.getUTCMonth(); | ||
| const nextMonth = month + 1; | ||
| const nextMonthDate = new Date(Date.UTC(year, nextMonth, 1, 12, 0, 0)); | ||
| const daysInMonth = new Date(Date.UTC(year, nextMonth + 1, 0)).getUTCDate(); | ||
| const day = Math.min(targetDay, daysInMonth); | ||
| nextMonthDate.setUTCDate(day); | ||
| return nextMonthDate; | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Monthly anchorDay is silently capped at 28, defeating the days-in-month clamp entirely — breaks end-of-month billing.
Line 23 pre-caps targetDay to Math.min(anchorDay || 1, 28) before the actual days-in-month logic (Lines 27-29) runs. Since daysInMonth is always ≥ 28, Math.min(targetDay, daysInMonth) on Line 29 can never reduce anything further than the value already forced by Line 23 — the "clamp to last day of month" logic is dead code. This means pledges with anchorDay 29, 30, or 31 (all valid per Pledge.js's max: 31) will always bill on the 28th, even in 30/31-day months, and even in leap-year Februaries where the correct clamp would be the 29th.
This is exercised by test/pledgeScheduler.test.js:21-28, which anchors on Jan 31 → Feb, in 2024 (a leap year, Feb has 29 days), and asserts next.getUTCDate() is 28. With the fix below, the correct expected value for that test is 29, not 28 — the test currently encodes the bug rather than the intended end-of-month semantics.
🔧 Proposed fix
- const targetDay = Math.min(anchorDay || 1, 28);
+ const targetDay = anchorDay || 1;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const computeNextDueAt = (fromDate, cadence, anchorDay = 1) => { | |
| const date = new Date(fromDate); | |
| const utcDate = new Date(Date.UTC(date.getUTCFullYear(), date.getUTCMonth(), date.getUTCDate(), 12, 0, 0)); | |
| if (cadence === "daily") { | |
| utcDate.setUTCDate(utcDate.getUTCDate() + 1); | |
| return utcDate; | |
| } | |
| if (cadence === "weekly") { | |
| utcDate.setUTCDate(utcDate.getUTCDate() + 7); | |
| return utcDate; | |
| } | |
| const targetDay = Math.min(anchorDay || 1, 28); | |
| const year = utcDate.getUTCFullYear(); | |
| const month = utcDate.getUTCMonth(); | |
| const nextMonth = month + 1; | |
| const nextMonthDate = new Date(Date.UTC(year, nextMonth, 1, 12, 0, 0)); | |
| const daysInMonth = new Date(Date.UTC(year, nextMonth + 1, 0)).getUTCDate(); | |
| const day = Math.min(targetDay, daysInMonth); | |
| nextMonthDate.setUTCDate(day); | |
| return nextMonthDate; | |
| }; | |
| export const computeNextDueAt = (fromDate, cadence, anchorDay = 1) => { | |
| const date = new Date(fromDate); | |
| const utcDate = new Date(Date.UTC(date.getUTCFullYear(), date.getUTCMonth(), date.getUTCDate(), 12, 0, 0)); | |
| if (cadence === "daily") { | |
| utcDate.setUTCDate(utcDate.getUTCDate() + 1); | |
| return utcDate; | |
| } | |
| if (cadence === "weekly") { | |
| utcDate.setUTCDate(utcDate.getUTCDate() + 7); | |
| return utcDate; | |
| } | |
| const targetDay = anchorDay || 1; | |
| const year = utcDate.getUTCFullYear(); | |
| const month = utcDate.getUTCMonth(); | |
| const nextMonth = month + 1; | |
| const nextMonthDate = new Date(Date.UTC(year, nextMonth, 1, 12, 0, 0)); | |
| const daysInMonth = new Date(Date.UTC(year, nextMonth + 1, 0)).getUTCDate(); | |
| const day = Math.min(targetDay, daysInMonth); | |
| nextMonthDate.setUTCDate(day); | |
| return nextMonthDate; | |
| }; |
🤖 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 `@src/workers/pledgeScheduler.js` around lines 9 - 32, Update computeNextDueAt
so monthly targetDay preserves valid anchorDay values through the days-in-month
clamp; remove the premature cap at 28 while retaining the default fallback for
missing values. Ensure anchor days 29–31 resolve to the lesser of the requested
day and the next month’s actual length, and update the affected leap-year test
expectation from 28 to 29.
1 participant
closes #29
Summary by CodeRabbit
New Features
Tests