Feature/stellar gifting - #54
Conversation
- Apply cacheMiddleware to GET endpoints for books, courses, users, spaces, and search routes with appropriate TTLs - Add cache invalidation middleware to POST, PUT, DELETE operations to ensure cache consistency when data changes - Add Redis environment variables to validateEnv.js optional vars - Create .env.example documenting all environment variables including Redis configuration options Closes Deen-Bridge#2
feat: configure Redis cache layer for production use
WalkthroughAdds resilient Horizon networking, claimable-balance payment fallback, Stellar gift and claim APIs, GiftClaim persistence, recipient access grants, and scheduled expiry handling with Jest coverage. ChangesStellar payment and gifting flows
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant GiftRoutes
participant GiftController
participant HorizonClient
participant GiftClaim
Client->>GiftRoutes: initialize or submit gift
GiftRoutes->>GiftController: authenticated gift request
GiftController->>HorizonClient: build, submit, and verify Stellar transaction
HorizonClient-->>GiftController: transaction result and balance ID
GiftController->>GiftClaim: save gift state and access grant
GiftController-->>Client: gift or claim response
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Biome (2.5.3)src/controllers/stellar/paymentController.jsFile contains syntax errors that prevent linting: Line 420: Expected a catch clause but instead found 'const'.; Line 483: Expected a semicolon or an implicit semicolon after a statement, but found none; Line 492: Expected a statement but instead found 'finally'.; Line 495: Expected a statement but instead found '}'. 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: 3
🧹 Nitpick comments (5)
app.js (1)
193-198: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove this
importto the top with the others, and consider a shutdown hook.Two small things (both optional, welcome-to-open-source style nits):
- ESM hoists this
import, so it works, but placing it mid-file after route wiring makes the module harder to scan — grouping it with the other imports at the top keeps intent obvious.startGiftSweepJob()registers asetIntervalthat nothing clears. On graceful shutdown (or repeated startup in some harnesses) the timer keeps the event loop alive and sweeps can overlap. Exposing a stop hook (clearInterval) wired into your SIGTERM/SIGINT handling would tidy that up.🤖 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 `@app.js` around lines 193 - 198, Move the startGiftSweepJob import to the existing top-level import block in app.js. Update the gift sweep job API to expose a stop hook that clears its interval, then invoke that hook from the existing SIGTERM/SIGINT shutdown handling so graceful shutdown cannot leave the timer running.src/services/stellar/horizonClient.js (1)
154-184: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueNice safe-submit design; two small resilience notes.
The single-resubmit-via-
verifyFnlogic is the right call for non-idempotent submits — good defensive thinking. Two things worth considering (both optional):
- Submit-mode failures never call
recordFailure(endpoint), so a persistently failing submit endpoint won't trip its circuit breaker the way read-mode failures do.- If
maxRetrieswere ever0, a resubmit path (attempt++; continue) would exit the loop and letexecute()resolve toundefined. The runtime default is3, so it's not reachable today, but a guardedreturn/throw at the end of the loop would future-proof it.🤖 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/services/stellar/horizonClient.js` around lines 154 - 184, Update the submit-mode branch in execute so retriable submit failures call recordFailure(endpoint) before retry or rethrow, matching read-mode circuit-breaker accounting. Also ensure the retry loop cannot fall through and resolve undefined when maxRetries is zero, by adding an explicit terminal return or throw after the loop while preserving the single verifyFn resubmit behavior.src/models/GiftClaim.js (1)
70-72: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAdd an index for the expiry-sweep query. 🗂️
sweepExpiredGiftsrunsGiftClaim.find({ status: "open", claimExpiryDate: { $lt: now } })every 10 minutes, but the only indexes arerecipient+statusandsender+status. Neither serves this predicate, so the sweep will collection-scan and get slower asGiftClaimgrows. A compound index on{ status: 1, claimExpiryDate: 1 }keeps it efficient. As per path instructions: flag "missing indexes for new query patterns."🗂️ Proposed index
giftClaimSchema.index({ recipient: 1, status: 1 }); giftClaimSchema.index({ sender: 1, status: 1 }); +// Supports the periodic expired-gift sweep query +giftClaimSchema.index({ status: 1, claimExpiryDate: 1 });🤖 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/GiftClaim.js` around lines 70 - 72, Add a compound index on giftClaimSchema for status and claimExpiryDate, ordered as { status: 1, claimExpiryDate: 1 }, to support the predicate used by sweepExpiredGifts while preserving the existing list-endpoint indexes.Source: Path instructions
src/controllers/stellar/giftController.js (2)
216-243: 🩺 Stability & Availability | 🔵 TrivialMove the Stellar network call out of the open Mongo transaction. ⏱️
session.startTransaction()opens at line 217-218, butsubmitTransaction(signedXdr)(plus the follow-upverifyTransaction/resolveBalanceIdHorizon round-trips) runs while that transaction is still open. Since Horizon calls now carry retries/timeouts, the Mongo transaction can stay open for many seconds and risks hittingtransactionLifetimeLimitSeconds(60s default) and holding write locks — the same pattern also appears insubmitClaimandsrc/controllers/stellar/paymentController.jssubmitPayment. Consider: do the on-chain submit+verify first (no session), then open a short transaction only for the DB writes.🤖 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/giftController.js` around lines 216 - 243, Restructure submitGift so submitTransaction and all follow-up Horizon calls, including verifyTransaction and resolveBalanceId, execute before session.startTransaction. Start a short Mongo transaction only after the Stellar workflow succeeds, then perform the required GiftClaim writes and commit; preserve the existing failure responses and status updates without holding the session during network calls. Apply the same separation to submitClaim and paymentController’s submitPayment.
194-202: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider aligning gift responses with the project's
{ success, message, data }shape. 📦These handlers return ad-hoc top-level fields (
giftId,variant,payment,gift,sent,received,xdr) instead of nesting underdata. It works, but as per path instructions this API expects "consistent response shapes ({ success, message, data })", and clients/tests are easier to write against a uniform envelope. This is pervasive acrossinitializeGift,submitGift,getGifts,getGift,initializeClaim, andsubmitClaim, so it's a low-priority cleanup rather than a blocker.🤖 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/giftController.js` around lines 194 - 202, Update the response bodies in initializeGift, submitGift, getGifts, getGift, initializeClaim, and submitClaim to use the consistent { success, message, data } envelope, moving existing ad-hoc fields such as giftId, variant, payment, gift, sent, received, and xdr under data while preserving their values and existing success behavior.Source: Path instructions
🤖 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/paymentController.js`:
- Around line 327-335: Strengthen verification in both settlement flows: in
src/controllers/stellar/paymentController.js lines 327-335, update the
claimable_balance branch to call verifyClaimableBalanceOp with result.hash,
transaction.creatorWallet, and transaction.amount before setting verified or
resolving balanceId; in src/controllers/stellar/giftController.js lines 252-269,
replace the direct_payment verifyTransaction check with verifyPaymentOperations
using the expected split from initializeGift, including applyPlatformFee based
on destinationPublicKey versus PLATFORM_WALLET_PUBLIC_KEY, and remove the dead
comment block.
In `@src/middlewares/errorHandler.js`:
- Around line 82-87: Hoist the AllEndpointsOpenError check above the NODE_ENV
development/production split in errorHandler, return the standard response shape
with success, status, message, and the existing network-unavailable code, and
remove both duplicated branches at src/middlewares/errorHandler.js lines 82-87
and 99-104.
In `@src/models/GiftClaim.js`:
- Around line 65-68: Replace the virtual itemTypeModel getter in giftClaimSchema
with a persisted itemTypeModel schema path that refPath can resolve during
populate("itemId"). Ensure it is populated consistently from the existing
itemType values, mapping book to Book and course to Course, and update any
related schema configuration without relying on the virtual.
---
Nitpick comments:
In `@app.js`:
- Around line 193-198: Move the startGiftSweepJob import to the existing
top-level import block in app.js. Update the gift sweep job API to expose a stop
hook that clears its interval, then invoke that hook from the existing
SIGTERM/SIGINT shutdown handling so graceful shutdown cannot leave the timer
running.
In `@src/controllers/stellar/giftController.js`:
- Around line 216-243: Restructure submitGift so submitTransaction and all
follow-up Horizon calls, including verifyTransaction and resolveBalanceId,
execute before session.startTransaction. Start a short Mongo transaction only
after the Stellar workflow succeeds, then perform the required GiftClaim writes
and commit; preserve the existing failure responses and status updates without
holding the session during network calls. Apply the same separation to
submitClaim and paymentController’s submitPayment.
- Around line 194-202: Update the response bodies in initializeGift, submitGift,
getGifts, getGift, initializeClaim, and submitClaim to use the consistent {
success, message, data } envelope, moving existing ad-hoc fields such as giftId,
variant, payment, gift, sent, received, and xdr under data while preserving
their values and existing success behavior.
In `@src/models/GiftClaim.js`:
- Around line 70-72: Add a compound index on giftClaimSchema for status and
claimExpiryDate, ordered as { status: 1, claimExpiryDate: 1 }, to support the
predicate used by sweepExpiredGifts while preserving the existing list-endpoint
indexes.
In `@src/services/stellar/horizonClient.js`:
- Around line 154-184: Update the submit-mode branch in execute so retriable
submit failures call recordFailure(endpoint) before retry or rethrow, matching
read-mode circuit-breaker accounting. Also ensure the retry loop cannot fall
through and resolve undefined when maxRetries is zero, by adding an explicit
terminal return or throw after the loop while preserving the single verifyFn
resubmit behavior.
🪄 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: 7659d852-2f63-4926-a499-05449483fb88
📒 Files selected for processing (19)
.env.exampleapp.jssrc/config/validateEnv.jssrc/config/validateEnv.test.jssrc/controllers/stellar/giftController.jssrc/controllers/stellar/paymentController.jssrc/jobs/sweepExpiredGifts.jssrc/middlewares/errorHandler.jssrc/models/GiftClaim.jssrc/models/Transaction.jssrc/routes/stellar/giftRoutes.jssrc/services/stellar/claimableBalanceService.jssrc/services/stellar/horizonClient.jssrc/services/stellar/horizonClient.test.jssrc/services/stellar/stellarService.jstest/controllers/stellar/giftController.test.jstest/jobs/sweepExpiredGifts.test.jstest/models/GiftClaim.test.jstest/services/stellar/claimableBalanceService.test.js
|
@TheBigWealth89 Please fix conflicts |
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 (2)
src/controllers/stellar/paymentController.js (2)
280-372: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
settlementModeis destructured asconstbut reassigned in the fallback branch.Line 326 declares
settlementModewithconst, but line 387 (settlementMode = "claimable_balance";) reassigns it. This throwsTypeError: Assignment to constant variableevery time the fallback path is actually needed — i.e., precisely when the creator lacks a USDC trustline, which is the scenario this whole PR is built to handle.🐛 Proposed fix
- const { item, creator, destinationPublicKey, settlementMode } = resolved; + const { item, creator, destinationPublicKey } = resolved; + let { settlementMode } = resolved;🤖 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/paymentController.js` around lines 280 - 372, Change the settlementMode binding in initializePayment from const to a reassignable declaration, since the fallback branch later assigns "claimable_balance". Preserve the resolved settlement mode for normal paths and allow the fallback path to update it without throwing.
373-472: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winDangling merge-conflict leftover crashes every
initializePaymentcall.After the trustline/settlement-mode switch (373-394) fully builds
paymentTx, an extra unconditional block runs at 396-418:
- It reads
preflight.ok—preflightis never declared anywhere in this function, so this throwsReferenceError: preflight is not defined(ES modules are always strict) before a single request can succeed.- It unconditionally re-invokes
buildPaymentTransaction(...), overwriting whateverpaymentTxthe switch above produced — so even ifpreflightexisted, the claimable-balance fallback branch's result would be silently discarded.- It assigns to
sep7Uriwithout ever declaring it (let/const), which is itself aReferenceErrorin strict mode.- Later, the response payload (468-472) references
sep7UriandisPathPayment, neither of which exists anywhere else in the file.pathInput(destructured at line 286) and the importedbuildPathPaymentTransaction(line 9) are also unused, confirming the path-payment branch this leftover code belonged to was never reconnected.This matches the maintainer comment asking to resolve merge conflicts — this reads exactly like an unresolved merge where the old path-payment/preflight/SEP-7 logic was left dangling next to the new trustline-fallback switch. As written,
initializePaymentcannot return a successful response for any input.🐛 Minimal fix to remove the dead/broken block
} - if (!preflight.ok) { - await session.abortTransaction(); - return res.status(400).json({ - success: false, - message: "Payment failed pre-flight safety checks", - reasons: preflight.reasons, - }); - } - - paymentTx = await buildPaymentTransaction({ - sourcePublicKey: buyer.stellarWallet.publicKey, - destinationPublicKey, - amount: item.price.toString(), - memo, - applyPlatformFee: settlementMode === "direct", - }); - - sep7Uri = buildSep7Uri({ - destination: destinationPublicKey, - amount: item.price.toString(), - memo, - }); - }And drop the now-undefined references in the response payload:
payment: { xdr: paymentTx.xdr, networkPassphrase: paymentTx.networkPassphrase, expectedHash: paymentTx.hash, }, - ...(sep7Uri && { sep7Uri }), - ...(isPathPayment && { - pathPaymentNote: - "Path payment XDR provided. SEP-7 URI is not available for path payments; use the XDR signing flow.", - }),If SEP-7 URI generation (for the non-fallback path) and path-payment support (
sendAsset/sendMax/pathinputs,buildPathPaymentTransaction) are still intended features, they need to be properly reintegrated into the surviving trustline/settlement-mode switch rather than left as orphaned code.🤖 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/paymentController.js` around lines 373 - 472, Remove the dangling preflight and unconditional payment rebuild block from initializePayment, including its undeclared sep7Uri assignment, so the trustline/settlement-mode switch remains the sole builder of paymentTx and preserves claimable-balance fallback behavior. Remove response fields referencing undefined sep7Uri and isPathPayment; do not reintroduce path-payment or SEP-7 logic unless it is properly integrated with the existing inputs and builders.
♻️ Duplicate comments (1)
src/controllers/stellar/paymentController.js (1)
561-600: 🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy liftClaimable-balance verification still doesn't check amount/claimant — unresolved from a prior review round.
This branch only checks
verification.exists && verification.successful; it never confirms the created balance's amount and claimant matchtransaction.amount/transaction.creatorWallet. A buyer can submit a completely different, cheap/self-directed transaction that succeeds on-chain and still getverified = truehere, sincesignedXdris never compared against the server-built XDR. This is the same gap raised in a previous review round (then at lines 327-335 in the old numbering) and it remains unaddressed at the current lines 564-572.🔒 Suggested fix
if (transaction.settlement === "claimable_balance") { - const verification = await verifyTransaction(result.hash); - if (!verification.exists || !verification.successful) { + const verification = await verifyClaimableBalanceOp( + result.hash, + transaction.creatorWallet, + transaction.amount + ); + if (!verification.exists || !verification.successful || !verification.matches) { verified = false; failureReason = "On-chain verification failed"; } else {🤖 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/paymentController.js` around lines 561 - 600, Update the claimable_balance branch in the payment verification flow to validate that the successfully verified balance matches transaction.amount and transaction.creatorWallet as claimant, rather than relying only on verification.exists and verification.successful. Reuse the server-built transaction/XDR validation path where available, and set verified to false with the existing failureReason handling whenever the amount, claimant, or transaction authenticity does not match.
🤖 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 `@src/controllers/stellar/paymentController.js`:
- Around line 280-372: Change the settlementMode binding in initializePayment
from const to a reassignable declaration, since the fallback branch later
assigns "claimable_balance". Preserve the resolved settlement mode for normal
paths and allow the fallback path to update it without throwing.
- Around line 373-472: Remove the dangling preflight and unconditional payment
rebuild block from initializePayment, including its undeclared sep7Uri
assignment, so the trustline/settlement-mode switch remains the sole builder of
paymentTx and preserves claimable-balance fallback behavior. Remove response
fields referencing undefined sep7Uri and isPathPayment; do not reintroduce
path-payment or SEP-7 logic unless it is properly integrated with the existing
inputs and builders.
---
Duplicate comments:
In `@src/controllers/stellar/paymentController.js`:
- Around line 561-600: Update the claimable_balance branch in the payment
verification flow to validate that the successfully verified balance matches
transaction.amount and transaction.creatorWallet as claimant, rather than
relying only on verification.exists and verification.successful. Reuse the
server-built transaction/XDR validation path where available, and set verified
to false with the existing failureReason handling whenever the amount, claimant,
or transaction authenticity does not match.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5b911e1e-dd1d-4969-ad43-404227969dc3
📒 Files selected for processing (4)
app.jssrc/controllers/stellar/paymentController.jssrc/models/Transaction.jssrc/services/stellar/stellarService.js
🚧 Files skipped from review as they are similar to previous changes (3)
- app.js
- src/models/Transaction.js
- src/services/stellar/stellarService.js
3 participants
Add Stellar Claimable Balances & Gifting Flow
Resolves: #43
Description
This PR introduces the highly requested "Stellar Gifting" functionality, allowing users to securely purchase books and courses on behalf of other users without the platform ever taking custody of funds.
It accomplishes this via strict, client-signed Stellar XDR envelopes. Depending on the creator's wallet state, this system seamlessly dynamically routes payments as either a standard
paymentoperation or as a trustline-freecreate_claimable_balanceoperation.Key Changes
1. New
GiftClaimData ModelGiftClaimschema to track the lifecycle of a gift (pending_signature,open,claimed,reclaimed,expired).TransactionTTL logic; relies on a rigorousclaimExpiryDatechecked by an interval worker.2. Core Gifting API (
/api/stellar/gifts)POST /initialize: Validates the recipient's wallet, ensures they don't already own the item, checks for duplicate pending gifts, and builds the correct unsigned XDR variant based on the creator's USDC trustline status.POST /submit: Accepts the signed XDR, performs immediate strict on-chain validation via Horizon, and officially grants the item access to the recipient (not the payer).POST /claim/initialize&POST /claim/submit: Handles the recipient claiming aclaimable_balancegift, verifying that the caller is the legitimate creator of the item, and processing the on-chain claim.GET /&GET /:id: Allows users to fetch their sent/received gifts and queries Horizon for live status ofopenclaimable balances.3. Expiry Sweep Background Job
sweepExpiredGifts.jsinterval worker that scans the database foropengifts that are strictly past theirclaimExpiryDate.getClaimableBalance) to avoid race conditions. If the balance was genuinely reclaimed or expired on-chain, it permanently locks the local DB state toexpired.4.
claimable_balanceFallback for Normal Purchases/api/stellar/payment/initializepath to gracefully handle cases where a creator lacks a USDC trustline during a standard purchase.create_claimable_balancefallback path, tagging the API response withfallback: "claimable_balance".Transactionlogic to persistbalanceIdand correctly verify claimable balance settlements upon submission.Security & Verification
claimableBalanceService,giftController, andsweepExpiredGiftsfully mocking the Horizon client to ensure zero network requests during CI/CD.Testing Instructions
feature/stellar-gifting.npm test.Summary by CodeRabbit
/api/stellar/giftsfor initializing, submitting, listing, and fetching gifts..env.example.503 NETWORK_UNAVAILABLEresponse.