Multi-asset settlement: asset registry and per-asset trustlines beyond hardcoded USDC (closes #60) - #78
Conversation
- Add network-aware asset registry (src/config/assets.js) covering USDC, EURC (issuers verified against Circle's official docs), and native XLM. USDC stays the registry default so existing behavior is unchanged. - Refactor stellarService.js to be asset-parametric: buildPaymentTransaction, buildReversePaymentTransaction, buildSep7Uri, and verifyPaymentOperations now accept an assetCode; getAccountBalance/preflightPayment report per-asset balances and trustlines. hasUsdcTrustline/USDC/USDC_ISSUER kept as back-compat wrappers derived from the registry. - Add currency to Transaction (widened enum + assetIssuer), Course, and Book models, all defaulting to USDC so existing rows need no migration. - paymentController.js rejects items priced in an unsupported asset with a 400 naming supported codes, and threads the item's currency through the direct-payment flow (preflight, XDR build, SEP-7 URI, Transaction record, on-chain verification). - walletController.js now surfaces per-asset trustline status via the already-updated getAccountBalance. Scope notes: - Path payments (buildPathPaymentTransaction, getQuote) intentionally stay USDC-settled; settling a path payment in a chosen asset is issue Deen-Bridge#27's mechanism, not this one. - Space.js was left untouched: Space isn't part of the purchase flow in paymentController.js, so a currency field there would be inert. Tests: added src/config/assets.test.js (registry resolution per network) and src/services/stellar/multiAsset.test.js (native-asset payment building, non-USDC trustline check, USDC default path unchanged). Updated stellarPaymentController.test.js and stellarService.test.js for the new call signature/messages/shape.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe PR adds a network-aware Stellar asset registry, multi-asset and path-payment support, asynchronous payment verification, expanded transaction schemas, and refund, dispute, and arbitration handlers. ChangesStellar payments and refund lifecycle
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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: 5
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
services/emails/sendMail.js (1)
18-30: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winError log message is stale now that
sendTemplateis shared.The
catchblock always logs"Error sending otp email:", butsendTemplateis now used for both OTP and receipt sends. A failed receipt send will be mislabeled as an OTP failure in logs, complicating triage.🩹 Proposed fix
} catch (error) { - logger.error("Error sending otp email:", error.response?.data || error.message); + logger.error(`Error sending ${templateId} email:`, error.response?.data || error.message); throw error; }🤖 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 `@services/emails/sendMail.js` around lines 18 - 30, Update the catch block in sendTemplate to use a generic email-send error message rather than labeling every failure as an OTP failure. Preserve the existing error details and rethrow behavior for both OTP and receipt sends.
🟠 Major comments (23)
src/controllers/stellar/walletController.js-38-41 (1)
38-41: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject nonexistent Stellar accounts or correct this comment.
getAccountBalancereturns{ exists: false }for Horizon 404s, butconnectWalletstill persists the wallet because it never checksaccountInfo.exists. This comment promises account verification while the endpoint can connect an uninitialized/nonexistent account.Proposed fix
const accountInfo = await getAccountBalance(publicKey); + if (!accountInfo.exists) { + return res.status(400).json({ + success: false, + message: "Stellar account does not exist", + }); + }🤖 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/walletController.js` around lines 38 - 41, Update connectWallet around the getAccountBalance call to check accountInfo.exists and reject nonexistent Stellar accounts before persisting the wallet; otherwise revise the nearby verification comment to accurately describe the behavior. Preserve the existing flow for accounts confirmed to exist.src/services/stellar/stellarTomlService.js-6-18 (1)
6-18: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPublish the asset registry rather than a USDC-only TOML list.
The payment layer supports USDC, EURC, and native XLM, but this endpoint advertises only USDC. Clients discovering assets through SEP-1 therefore cannot select the newly supported currencies.
src/services/stellar/stellarTomlService.js#L6-L18: derive[[CURRENCIES]]entries from the shared network-aware registry, including the correct native-asset representation.test/stellarToml.test.js#L60-L68: assert every registry asset is published with its network-specific fields, not just USDC.🤖 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/stellarTomlService.js` around lines 6 - 18, Replace the USDC-only CURRENCIES definition in stellarTomlService.js with entries derived from the shared network-aware asset registry, including the correct native XLM representation and network-specific fields for USDC and EURC. Update test/stellarToml.test.js to verify every registry asset is published with its expected network-specific fields.src/services/stellar/stellarTomlService.js-33-46 (1)
33-46: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winValidate both Stellar keys with the checksum-aware StrKey helper.
STELLAR_PLATFORM_PUBLIC_KEYis written into TOML directly, andSIGNING_KEYis only shape-checked. UseStrKey.isValidEd25519PublicKey(...)for both so checksum-invalidG...values don’t publish broken SEP metadata, and add a regression for that case.🤖 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/stellarTomlService.js` around lines 33 - 46, Update the key validation in the Stellar TOML generation flow to use StrKey.isValidEd25519PublicKey for both STELLAR_PLATFORM_PUBLIC_KEY and SIGNING_KEY after trimming their values, while preserving omission of missing or invalid keys. Add a regression test covering checksum-invalid G-prefixed values and verify neither key is emitted.src/controllers/books/recommendedBooksController.js-12-18 (2)
12-18: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMake the interest cache key collision-safe.
join(",")is not an injective encoding:["art,science"]and["art", "science"]produce the same key, so one request can receive another request’s recommendations. Use validated values with a collision-safe canonical encoding, such asJSON.stringify([...new Set(interests)].sort()).🤖 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/books/recommendedBooksController.js` around lines 12 - 18, The recommended-books cache key built in the hasInterests branch is collision-prone because comma-joining interests can represent different arrays identically. Use validated interests, remove duplicates, sort them, and serialize the canonical array with a collision-safe encoding such as JSON.stringify while preserving the popular-books key for empty interests.
12-18: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winValidate and bound
interestsbefore using it.
Array.isArrayonly validates the container; arbitrary elements and unbounded arrays reach MongoDB’s$inquery and the Redis key. Reject non-empty strings, cap count and length, and normalize once before building both the query and key. Otherwise malformed requests can produce 500s or amplify database/cache work.As per path instructions,
src/**/*.jsendpoints must validate request input.Also applies to: 33-35
🤖 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/books/recommendedBooksController.js` around lines 12 - 18, The recommended-books endpoint must validate and bound interests before using it in MongoDB queries or Redis keys. In the controller’s interests handling, reject non-empty non-string elements, enforce maximum item count and string length, and normalize the validated values once; reuse that normalized collection for both the query and cacheKey construction, while preserving the popular-books path for absent or empty interests.Source: Path instructions
src/controllers/books/recommendedBooksController.js-49-49 (1)
49-49: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRestore the standard response envelope.
This currently emits
{ success, books, message }or{ success, recommended, message }; it never includesdata. Wrap the branch-specific result underdatawhile keepingmessagetop-level.As per path instructions, changed
src/**/*.jsendpoints must return{ success, message, data }.Proposed fix
- res.status(200).json({ success: true, ...payload }); + const { message, ...data } = payload; + res.status(200).json({ success: true, message, data });🤖 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/books/recommendedBooksController.js` at line 49, Update the response construction in the recommended books controller to preserve the standard `{ success, message, data }` envelope: keep success and message top-level, and move the branch-specific payload fields such as books or recommended under data. Ensure both response branches use this structure.Source: Path instructions
test/app.test.js-20-20 (1)
20-20: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRecognize all loopback MongoDB hosts before using an external database.
Checking only whether the URI contains
"localhost"lets127.0.0.1and::1connect to a developer’s local database, while hostnames such asmongo-localhost.example.comcan be incorrectly treated as local. Parse the URI hostname and explicitly classify loopback addresses to preserve test isolation.🤖 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 `@test/app.test.js` at line 20, Update the MongoDB environment check around process.env.MONGO_URI to parse the URI hostname and classify only actual loopback hosts, including localhost, 127.0.0.1, and ::1, as local. Use the parsed hostname rather than substring matching so names such as mongo-localhost.example.com are treated as external.test/app.test.js-21-30 (1)
21-30: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winShorten the remote MongoDB probe here.
test/app.test.js:19-30can still spend the whole 30sbeforeAllbudget waiting on driver server selection before it falls back toMongoMemoryServer. Use a smallserverSelectionTimeoutMSfor this connect attempt, or make remote test DBs an explicit opt-in.🤖 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 `@test/app.test.js` around lines 21 - 30, Update the remote mongoose.connect attempt in beforeAll to use a short serverSelectionTimeoutMS, so an unavailable MONGO_URI quickly falls through to MongoMemoryServer.create(); preserve the existing 30-second overall hook timeout and fallback behavior.src/controllers/books/bookController.js-232-236 (1)
232-236: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winGenerate the Cloudinary URL with raw/authenticated options.
private_download_url(book.filePublicId, "raw", ...)only sets the format; it still needsresource_type: "raw"andtype: "authenticated"to match how the file is uploaded, or the preview redirect can point at the wrong asset. Add coverage for the redirect path too.🤖 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/books/bookController.js` around lines 232 - 236, Update the private_download_url call in the book file redirect flow to include resource_type "raw" and type "authenticated" alongside the existing raw format and expiration. Add coverage for the bookController redirect path, verifying the generated URL uses these upload-matching options.src/utils/fileValidation.js-16-19 (1)
16-19: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winThe magic-byte bypass and the test that depends on it need to move together.
Thanks for adding content-signature validation — that's a real security upgrade over trusting MIME headers. The problem is where the test seam was placed:
validateMagicBytesshort-circuits onprocess.env.NODE_ENV === "test"and substitutes a"fake"substring check, which means the production security control for every upload path (avatars, thumbnails, book files) is disabled by an environment variable. Any environment whereNODE_ENVistestwill accept an HTML/SVG polyglot renamedbook.pdf. The upload test asserts against that shortcut rather than against real detection, so the two have to change in one commit.
src/utils/fileValidation.js#L16-L19: remove theNODE_ENV === "test"branch and let tests control the dependency instead — accept the detector as an injectable third parameter defaulting tofileTypeFromBuffer, or have the suitejest.spyOnthe module the way it already stubscloudinary.test/bookUpload.test.js#L67-L80: stop relying on the"fake pdf text file"substring. Assert the 400 using a payload that genuinely fails signature detection, and replace the minimalvalidPdfBytes/validImageBytesstubs (Lines 14-15) with small real PDF/JPEG fixtures — the 12-byte JFIF header in particular may not satisfyfile-typeonce real detection runs.🤖 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/utils/fileValidation.js` around lines 16 - 19, Remove the NODE_ENV test bypass from validateMagicBytes and let tests control fileTypeFromBuffer through injection or the existing module-stubbing pattern. In test/bookUpload.test.js lines 67-80, stop relying on the “fake” substring and assert rejection using content that genuinely fails signature detection; replace the minimal validPdfBytes and validImageBytes fixtures at lines 14-15 with small valid PDF and JPEG fixtures, including a complete JFIF structure.src/controllers/courses/courseController.js-250-263 (1)
250-263: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
hasInterestsis computed but never used to guard the query.Nice touch adding an explicit cache key here since
cacheMiddlewareis GET-only. One gap though: on therecommended:nonebranch,interestsisundefined(or a non-array), andCourse.find({ category: { $in: undefined } })will throw a Mongoose cast error rather than returning an empty list — the caller gets a 500 from thecatchblock instead of a meaningful response. Worse, the sentinel key already tells us the intent: no interests means nothing to recommend, so the DB round-trip is unnecessary.Also worth noting
[...interests].sort()doesn't dedupe or normalize case, so["Tech","tech"]and["tech","Tech"]produce different keys for the same logical query — cheap to normalize while you're here.🐛 Proposed fix: short-circuit the no-interests case
const hasInterests = Array.isArray(interests) && interests.length > 0; + if (!hasInterests) { + return res.status(200).json({ success: true, recommended: [] }); + } + // This endpoint is POST (interests come in the body), so the shared // cacheMiddleware (GET-only) can't key off req.query - cache explicitly here instead. - const cacheKey = hasInterests - ? `${CACHE_KEYS.COURSES}recommended:${[...interests].sort().join(",")}` - : `${CACHE_KEYS.COURSES}recommended:none`; + const normalized = [...new Set(interests.map((i) => String(i).toLowerCase()))].sort(); + const cacheKey = `${CACHE_KEYS.COURSES}recommended:${normalized.join(",")}`; const recommended = await getCacheOrSet( cacheKey, () => Course.find({ category: { $in: interests } }), CACHE_TTL.COURSES );🤖 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/courses/courseController.js` around lines 250 - 263, Update the recommendation flow around hasInterests and cacheKey to return an empty recommendation list immediately when interests is missing, non-array, or empty, avoiding Course.find with an invalid $in value and the unnecessary cache/database call. For valid interests, normalize them consistently (including case) and deduplicate before generating the cache key and using the same normalized values in the Course.find query.src/utils/fileValidation.js-1-1 (1)
1-1: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winfile-type@22 does not fit this project’s Node floor
src/utils/fileValidation.js:1
file-type@22requires Node 22+, but this repo targets Node 20 in CI/README and 18+ in CONTRIBUTING. Either raise the runtime floor everywhere or pinfile-typeto a compatible major, or uploads will break on the supported environment.🤖 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/utils/fileValidation.js` at line 1, The file-type dependency used by fileValidation must support the project’s declared Node 18/20 runtime floor. Pin file-type to a compatible major in the dependency configuration, or consistently raise the documented and CI runtime requirements instead of leaving file-type@22 incompatible with supported environments.src/controllers/notificationController.js-20-46 (1)
20-46: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRace condition:
initRedisSubscribercan be entered concurrently, spawning duplicate subscriptions.
isSubscriberInitis only set totrueafterawait subClient.connect()andawait subClient.subscribe(...)complete. It is called from bothsseNotifications(line 122, on every new connection) anddispatchSSENotification(line 75, on every publish) without any synchronization. If two calls race before the first sets the flag, both proceed past theif (isSubscriberInit) return;guard, each duplicating the Redis client and subscribing tonotifications:sseagain. Under real traffic (e.g., many users reconnecting after a deploy) this leaks duplicate Redis connections and causes every notification to be delivered multiple times to the same SSE client.🔒 Proposed fix using an in-flight promise lock
let isSubscriberInit = false; +let subscriberInitPromise = null; export const initRedisSubscriber = async () => { if (isSubscriberInit) return; + if (subscriberInitPromise) return subscriberInitPromise; if (!isRedisReady()) return; - try { - const mainClient = getRedisClient(); - if (!mainClient) return; - - const subClient = mainClient.duplicate(); - await subClient.connect(); - - await subClient.subscribe("notifications:sse", (message) => { - try { - const { recipientId, notification } = JSON.parse(message); - deliverLocalSSENotification(recipientId, notification); - } catch (err) { - logger.error("Error handling Redis SSE pub/sub message:", err); - } - }); - - isSubscriberInit = true; - logger.info("✅ Redis SSE Pub/Sub subscriber initialized"); - } catch (err) { - logger.error("Failed to initialize Redis SSE subscriber:", err); - } + subscriberInitPromise = (async () => { + try { + const mainClient = getRedisClient(); + if (!mainClient) return; + + const subClient = mainClient.duplicate(); + await subClient.connect(); + + await subClient.subscribe("notifications:sse", (message) => { + try { + const { recipientId, notification } = JSON.parse(message); + deliverLocalSSENotification(recipientId, notification); + } catch (err) { + logger.error("Error handling Redis SSE pub/sub message:", err); + } + }); + + isSubscriberInit = true; + logger.info("✅ Redis SSE Pub/Sub subscriber initialized"); + } catch (err) { + logger.error("Failed to initialize Redis SSE subscriber:", err); + } finally { + subscriberInitPromise = null; + } + })(); + return subscriberInitPromise; };🤖 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/notificationController.js` around lines 20 - 46, Prevent concurrent initialization in initRedisSubscriber by adding an in-flight promise lock that concurrent callers await instead of creating additional Redis clients or subscriptions. Set and clear the lock across the full connect/subscribe operation, while preserving the existing isSubscriberInit guard and error logging.test/stellarPaymentController.test.js-16-53 (1)
16-53: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdd
hasTrustlineto the Stellar service mock.src/services/stellar/multiAsset.test.jsimportshasTrustlinefromstellarService.js, but this factory only stubshasUsdcTrustline; a worker that links both suites can fail withdoes not provide an export named hasTrustline.🤖 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 `@test/stellarPaymentController.test.js` around lines 16 - 53, Add a jest mock entry for the hasTrustline export in the stellarService mock factory alongside hasUsdcTrustline, ensuring multiAsset.test.js can resolve the complete service API without changing existing mock behavior.test/refund.test.js-143-144 (1)
143-144: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winThis fixture shape is why the revocation assertions pass.
buyer.purchasedCourses = [course._id]stores bare ObjectIds, butsrc/models/User.jsdeclares document arrays of{ courseId, purchaseDate }. Seeding the real shape ([{ courseId: course._id }]) makes these assertions catch the filter bug flagged insrc/controllers/stellar/refundController.jsLines 267-292 instead of hiding it. Worth adding a book-refund case and a non-USDC currency case too, since the multi-asset path is entirely untested here.Also applies to: 289-297
🤖 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 `@test/refund.test.js` around lines 143 - 144, Update the refund test fixtures around the buyer setup to use the User model’s document shape, assigning purchasedCourses entries as objects with courseId rather than bare ObjectIds. Add coverage for book refunds and a non-USDC currency through the multi-asset refund path, ensuring revocation assertions exercise the filtering behavior in the refund controller.src/services/stellar/stellarService.js-678-695 (1)
678-695: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUnsupported
assetCodeturns into a permanently retried "transient" failure.
getAssetConfigreturnsnullfor a code that isn't in the registry, andmatchesAssetthen dereferencesassetConfig.issuer. TheTypeErroris swallowed by the outercatch, which reportstransient: true— so the verification job keeps retrying a request that can never succeed. Callers pass persisted values (transaction.currency || "USDC"inpaymentController.jsLine 631), so a legacy or renamed currency string reaches here.🐛 Proposed fix: fail fast, non-transient
const verification = await verifyTransaction(txHash); const assetConfig = getAssetConfig(assetCode, NETWORK); + if (!assetConfig) { + return { + verified: false, + reason: `Unsupported asset code: ${assetCode}`, + }; + } + if (!verification.exists) {🤖 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/stellarService.js` around lines 678 - 695, Update verifyPaymentOperations to handle a null result from getAssetConfig before matchesAsset accesses assetConfig. Return a failed verification with a non-transient result and a clear unsupported-asset reason, preventing the outer catch from classifying invalid asset codes as transient retries; preserve existing behavior for valid configurations.src/services/stellar/stellarService.js-420-448 (1)
420-448: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winNative XLM payments: the spend amount isn't stacked on top of reserve + fees.
For
XLMthe same balance funds the payment and the base reserve/fee, but the two checks are independent:availableStroops >= requiredStroopsandxlmAvailable >= minReserve + fee. A wallet with 10 XLM, 1 subentry (1.5 XLM reserve) paying 9.9 XLM passes both checks and then fails on submission withtx_insufficient_balance— exactly the bounce preflight exists to prevent.🛡️ Proposed fix: include the native spend in the reserve floor
const minReserveStroops = (2n + BigInt(summary.subentryCount)) * BASE_RESERVE_STROOPS; const feeStroops = BigInt(StellarSdk.BASE_FEE) * BigInt(operationCount); const xlmAvailableStroops = toStroops(summary.xlmBalance); + // Native payments are funded from the same XLM balance as reserve + fees. + const nativeSpendStroops = assetConfig.issuer ? 0n : requiredStroops; - if (xlmAvailableStroops < minReserveStroops + feeStroops) { + if ( + xlmAvailableStroops < + minReserveStroops + feeStroops + nativeSpendStroops + ) {🤖 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/stellarService.js` around lines 420 - 448, Update the native XLM preflight checks in the source-account validation block so the reserve-and-fee comparison also includes requiredStroops when assetConfig.issuer is absent. Keep issued-asset checks unchanged, and ensure XLM payments only pass when the balance covers the payment amount, minimum reserve, and network fee together.services/emails/sendMail.js-33-43 (1)
33-43: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winNew code logs raw recipient email addresses.
logger.info(\Sending OTP email to: ${email}`)andlogger.info(`Sending receipt for ${receipt.txHash} to: ${receipt.email}`)write user email addresses to logs atinfo` level. This is PII that will end up in log aggregation/retention systems; prefer logging a user/transaction ID instead of the raw address.🤖 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 `@services/emails/sendMail.js` around lines 33 - 43, Replace raw email addresses in the info logs within sendOtpEmail and sendReceiptEmail with a non-PII user or transaction identifier. Preserve the existing sendTemplate arguments and retain the transaction hash only where it is already used for receipt context.Source: Linters/SAST tools
src/controllers/authController.js-92-96 (1)
92-96: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftCookie
secure/sameSitegating solely onNODE_ENV === "production"risks breaking cross-site refresh tokens in any non-production HTTPS deployment.The CORS allow-list confirms the frontend and API run on different origins (
dnb-frontend.vercel.appvs. this API). Cross-site cookies requireSameSite=None; Secure. With this change, any environment whereNODE_ENV !== "production"(staging, preview deploys, QA) getssecure:false, sameSite:"Lax"— browsers won't attach that cookie on cross-site fetch/XHR requests, silently breakingrefreshSession/logoutUserfor those environments even though they're served over HTTPS and cross-site. The previous hardcodedsecure:true, sameSite:"None"worked uniformly for every such deployment.Consider deriving the flag from the actual protocol/deployment context instead (e.g.
req.secureor a dedicatedCOOKIE_SECURE/COOKIE_SAME_SITEenv var), reserving theLax/non-secure fallback specifically for local HTTP development.Also applies to: 354-358, 462-466
🤖 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/authController.js` around lines 92 - 96, Update the cookie option logic in the auth controller’s refresh-token, logout, and related cookie-setting paths instead of gating solely on NODE_ENV. Derive secure/SameSite from the actual HTTPS deployment context or dedicated cookie configuration, ensuring cross-site HTTPS environments use secure:true and sameSite:"None", while retaining the non-secure/Lax fallback only for local HTTP development.src/controllers/authController.js-131-135 (1)
131-135: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftOTP codes are persisted in plaintext job payloads/keys and then exposed via the admin dashboard. The root cause is enqueuing the raw OTP as part of the durable job document; the dashboard's unredacted JSON response is a downstream amplifier of the same exposure.
src/controllers/authController.js#L131-L135: don't put the rawotpvalue into the jobpayloadoridempotencyKey. Look up the OTP inside thesendOtpEmailhandler (e.g. from a short-lived, hashed OTP store/collection keyed byuserId) instead of passing it through the durable queue, and derive the idempotency key fromuserId+ a request-scoped correlation id rather than the OTP itself.src/routes/jobsRoutes.js#L22-L31: project out (or redact) thepayloadfield before returning job listings as JSON, so sensitive job inputs are never surfaced through the dashboard even as a secondary safeguard.🤖 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/authController.js` around lines 131 - 135, Remove the raw OTP from the durable queue data in authController’s enqueue call: have the sendOtpEmail handler retrieve it from a short-lived hashed OTP store keyed by userId, and derive idempotencyKey from userId plus a request-scoped correlation ID. Also update src/routes/jobsRoutes.js lines 22-31 to project out or redact payload before returning job listings, ensuring sensitive inputs are not exposed in the dashboard.src/models/Job.js-3-26 (1)
3-26: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winMissing index for the dashboard's default sort (
createdAt).
jobsRoutes.js'sGET /admin/jobsroute sorts bycreatedAtdescending, but no index covers that field (the compound index only covers{status, runAt}). As theJobcollection grows, this listing query will require an in-memory sort over the whole collection.♻️ Suggested index
jobSchema.index({ status: 1, runAt: 1 }); +jobSchema.index({ createdAt: -1 });As per path instructions, flag missing indexes for new query patterns introduced on
src/models/**.🤖 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/Job.js` around lines 3 - 26, Add a descending index for createdAt in the Job schema, alongside the existing schema indexes, so the GET /admin/jobs default createdAt-descending sort is covered without altering the existing status/runAt index.Source: Path instructions
src/routes/jobsRoutes.js-13-20 (1)
13-20: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winNo rate limiting on the token-protected admin dashboard; token comparison isn't constant-time.
/admin/jobsis mounted outside the/apiprefix (seeapp.jsLine 190), so it likely bypasses any rate limiter scoped to/api. Combined with a plain!==comparison of the bearer token, this route has no throttling against repeated guesses ofJOBS_DASHBOARD_TOKEN. For an admin surface that can enumerate job payloads, add a dedicated rate limiter and a constant-time comparison (crypto.timingSafeEqual).🔒 Suggested hardening
+import rateLimit from "express-rate-limit"; +import crypto from "crypto"; + +const dashboardLimiter = rateLimit({ windowMs: 15 * 60 * 1000, max: 20 }); + router.use((req, res, next) => { const token = process.env.JOBS_DASHBOARD_TOKEN; if (!token) return res.status(404).json({ success: false, message: "Not found" }); - if (req.headers.authorization !== `Bearer ${token}`) { + const provided = Buffer.from(req.headers.authorization || ""); + const expected = Buffer.from(`Bearer ${token}`); + if (provided.length !== expected.length || !crypto.timingSafeEqual(provided, expected)) { return res.status(401).json({ success: false, message: "Unauthorized" }); } next(); }); +router.use(dashboardLimiter);🤖 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/routes/jobsRoutes.js` around lines 13 - 20, Harden the token-protected middleware in the jobs dashboard route by adding a dedicated rate limiter for the `/admin/jobs` surface and replacing the plain authorization comparison with `crypto.timingSafeEqual`. Preserve the existing 404 response when `JOBS_DASHBOARD_TOKEN` is unset and 401 response for invalid credentials, ensuring token buffers are length-safe before constant-time comparison.src/jobs/queue.js-61-77 (1)
61-77: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
duplicateis still wrong here.Job.findOneAndUpdate(..., { upsert: true, new: true })returns the document either way, sojob.attemptsMade > 0only flips after the job is processed. A secondenqueue()for the same still-queued job will be reported asduplicate: false.🐛 Use the upsert result metadata instead
- const job = await Job.findOneAndUpdate( + const result = await Job.findOneAndUpdate( { idempotencyKey: options.idempotencyKey }, { $setOnInsert: { @@ - { upsert: true, new: true, session: options.session } + { upsert: true, new: true, session: options.session, includeResultMetadata: true } ); - return { queued: true, id: job._id, duplicate: job.attemptsMade > 0 }; + return { queued: true, id: result.value._id, duplicate: result.lastErrorObject.updatedExisting };🤖 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/jobs/queue.js` around lines 61 - 77, Update the enqueue flow around Job.findOneAndUpdate to derive duplicate from the upsert result metadata rather than job.attemptsMade. Preserve the queued response and job ID, and ensure a newly inserted job reports duplicate false while an existing idempotency-key match reports duplicate true even before processing.
🟡 Minor comments (12)
src/controllers/stellar/walletController.js-107-109 (1)
107-109: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDocument native XLM separately from issued-asset trustlines.
The service contract exposes native XLM as
xlmBalance; itsbalances/trustlinesmaps are registry-issued-asset data, and XLM cannot have a trustline. Please clarify the response shape so clients do not look fortrustlines.XLMor prompt users to add an impossible XLM trustline.🤖 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/walletController.js` around lines 107 - 109, Clarify the response-shape documentation near the wallet controller balance/trustline fields: identify native XLM exclusively through xlmBalance, and state that balances and trustlines contain only registry-issued assets. Explicitly indicate that XLM has no trustline so clients do not inspect trustlines.XLM or prompt users to add one.QUICK_START.md-73-73 (1)
73-73: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAlign the
SIGNING_KEYguidance with the implementation.This says the value “must remain unset,” but the service intentionally publishes a valid public
G…key. Clarify that it must be a public key—not a secret seed—and when operators should configure it; otherwise the documented setup disables a supported feature.🤖 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 `@QUICK_START.md` at line 73, Update the TOML environment-variable guidance in QUICK_START.md so SIGNING_KEY is described as a public G… key, never a secret seed. State when operators should configure it, aligning the setup instructions with the service’s supported SEP-10 behavior.src/controllers/books/recommendedBooksController.js-12-49 (1)
12-49: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd Jest coverage for
src/controllers/books/recommendedBooksController.js— cover the popular branch, reordered-interest cache keys, cache hit/miss behavior, Redis fallback, and the response envelope.🤖 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/books/recommendedBooksController.js` around lines 12 - 49, Add Jest tests for the recommended-books controller covering the no-interests popular query, interest-based recommendations, deterministic cache keys when interests are reordered, cache hit and miss behavior, Redis fallback, and the `{ success: true, ...payload }` response envelope. Mock Book, getCacheOrSet, and response/request dependencies as needed, and verify query sorting, limits, cache TTL usage, and returned payloads without changing controller behavior.Source: Path instructions
src/controllers/uploadController.js-15-24 (1)
15-24: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winUse the required API response envelope consistently.
The changed responses omit
data, breaking the declared{ success, message, data }API contract. Usedata: nullfor errors and place successful payloads underdata.
src/controllers/uploadController.js#L15-L24: return signature metadata insidedataand includemessage.src/middlewares/authMiddleware.js#L46-L50: adddata: nullto the forbidden response.src/controllers/books/bookController.js#L31-L33: adddata: nullto the validation error.src/controllers/books/bookController.js#L140-L143: adddata: nullto deletion success/error responses.🤖 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/uploadController.js` around lines 15 - 24, Use the required { success, message, data } envelope across all affected responses: in src/controllers/uploadController.js lines 15-24, add a success message and move signature metadata under data; in src/middlewares/authMiddleware.js lines 46-50, add data: null to the forbidden response; in src/controllers/books/bookController.js lines 31-33, add data: null to the validation error; and in lines 140-143, add data: null to both deletion success and error responses.Source: Path instructions
src/controllers/uploadController.js-7-13 (1)
7-13: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winConstrain the direct-upload signature. The signature only binds
timestampandfolder, so any authenticated caller can reuse it to upload any Cloudinary-supported file intodirect-uploads. Sign the intended upload constraints here, or use a preset with explicitallowed_formats/size limits for this workflow.🤖 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/uploadController.js` around lines 7 - 13, Update the signature generation in the upload controller around cloudinary.utils.api_sign_request to bind the direct-upload constraints, including the permitted file formats and size limit, or use a configured upload preset that enforces those allowed_formats and size limits; preserve the existing timestamp and direct-uploads folder constraints.src/routes/uploadRoutes.js-1-9 (1)
1-9: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd Jest coverage for
POST /api/uploads/signature. Cover the unauthenticated401response and the success payload shape for this protected Cloudinary signature route.🤖 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/routes/uploadRoutes.js` around lines 1 - 9, Add Jest coverage for the POST /signature route in the router using the existing protect and generateSignature integrations: verify unauthenticated requests receive 401, and verify an authenticated successful request returns the expected Cloudinary signature payload shape.Source: Path instructions
src/controllers/notificationController.js-221-225 (1)
221-225: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winUn-awaited
dispatchSSENotificationcalls in fan-out loops — attach error handling.Both
createNewCourseNotificationandcreateNewBookNotificationinvokedispatchSSENotification(...)inside aforloop withoutawaiting or attaching.catch(). The surroundingtry/catchonly catches synchronous throws and awaited rejections — a rejected fire-and-forget promise here would become an unhandled rejection. SincedispatchSSENotificationmostly self-catches today, this isn't causing a crash yet, but it's a fragile pattern this file's path instructions specifically call out to flag.🛠️ Suggested fix (applies to both functions)
- const senderPayload = { _id: creatorId, name: creator.name, avatar: creator.avatar }; - for (const notification of createdNotifications) { - const plainNotif = notification.toObject ? notification.toObject() : { ...notification }; - plainNotif.sender = senderPayload; - dispatchSSENotification(plainNotif.recipient, plainNotif); - } + const senderPayload = { _id: creatorId, name: creator.name, avatar: creator.avatar }; + await Promise.allSettled( + createdNotifications.map((notification) => { + const plainNotif = notification.toObject ? notification.toObject() : { ...notification }; + plainNotif.sender = senderPayload; + return dispatchSSENotification(plainNotif.recipient, plainNotif); + }) + );As per path instructions, "Flag missing auth/ownership checks, unvalidated request input, and unhandled promise rejections."
Also applies to: 255-259
🤖 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/notificationController.js` around lines 221 - 225, Update the fan-out loops in createNewCourseNotification and createNewBookNotification so each dispatchSSENotification call is awaited or given explicit rejection handling. Keep the existing try/catch behavior effective by awaiting the dispatch, or attach a .catch handler that records the failure without creating an unhandled rejection.Source: Path instructions
test/notification.test.js-258-280 (1)
258-280: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winTest leaves a 30s
keepAliveinterval running after the test ends.
sseNotificationsschedulessetInterval(..., 30000). SincereqMock.onis a barejest.fn(), the"close"handler is never invoked and the interval is never cleared, leaking an open timer handle past this test. Consider usingjest.useFakeTimers()(and advancing/clearing after assertions) or capturing and manually invoking the"close"callback to clean up.+ beforeEach(() => jest.useFakeTimers({ doNotFake: ["nextTick"] })); + afterEach(() => jest.useRealTimers());Also, this suite doesn't cover the Redis pub/sub relay path (
initRedisSubscriber/dispatchSSENotification) or the connection-cleanup-on-close behavior introduced in this PR — worth adding follow-up coverage. As per path instructions, "New or changed endpoints need Jest coverage."🤖 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 `@test/notification.test.js` around lines 258 - 280, The sseNotifications test leaves its keepAlive interval active and lacks coverage for cleanup and Redis relay behavior. Update the test around sseNotifications to capture and invoke the reqMock "close" callback or use fake timers with explicit cleanup, then add Jest coverage for initRedisSubscriber/dispatchSSENotification and connection cleanup when the request closes.Source: Path instructions
src/controllers/stellar/paymentController.js-300-300 (1)
300-300: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
sendMaxandpatharrive unvalidated from the client.
sendMaxflows intotoStroops()insidebuildPathPaymentTransactionand then into the persistedTransaction.sendMax; a non-numeric or negative value becomes aBigIntconversion throw surfacing as a generic500.pathInputentries are spread based ona.asset_typewithout checking the codes/issuers are well formed. A small shape check here (positive decimal string forsendMax, each path entry eithernativeor{asset_code, asset_issuer}) keeps the failure mode a clean400. The unsupported-asset rejection on 351-359 is a solid addition.As per path instructions: "Flag missing auth/ownership checks, unvalidated request input, and unhandled promise rejections."
Also applies to: 351-359
🤖 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` at line 300, Validate sendMax and pathInput in the payment controller before passing them to buildPathPaymentTransaction: require sendMax to be a positive decimal string, and require every path entry to be native or a well-formed asset_code/asset_issuer object. Return a clean 400 for invalid input, while preserving the existing unsupported-asset rejection.Source: Path instructions
src/controllers/stellar/paymentController.js-104-141 (1)
104-141: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
sendAssetCodeis effectively optional and silently degrades to native XLM.The guard on line 132 short-circuits when
sendAssetCodeis falsy, and line 139 then branches onsendAssetIssueralone — so a request with neither field returns a quote priced in XLM without ever saying so. Please requiresendAssetCodeexplicitly. Same block:itemIdgoes straight intofindById, so a malformed id throws aCastErrorand surfaces as a500rather than a400.🛡️ Proposed validation
+ if (!sendAssetCode) { + return res.status(400).json({ + success: false, + message: "sendAssetCode is required", + }); + } + + if (!mongoose.isValidObjectId(itemId)) { + return res.status(400).json({ + success: false, + message: "Invalid item id", + }); + } + const Model = itemType === "book" ? Book : Course; const item = await Model.findById(itemId);🤖 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 104 - 141, Require a valid, explicit sendAssetCode in getQuote before constructing sendAsset, returning a 400 response when it is missing instead of defaulting to native XLM; retain issuer validation for non-native assets. Also validate itemId before Model.findById and return 400 for malformed IDs, preventing Mongoose CastError responses from becoming 500 errors..env.example-29-33 (1)
29-33: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDuplicate EmailJS keys — old block wasn't removed.
Lines 29-33 redeclare
EMAILJS_PRIVATE_KEY,EMAILJS_PUBLIC_KEY,EMAILJS_SERVICE_ID, andEMAILJS_TEMPLATE_ID, which already exist above at lines 23-26. dotenv-linter flags all four as duplicated keys. Since the second occurrence wins, this is harmless at runtime but confusing for anyone setting up their.envfrom this template. Remove the earlier block (lines 23-26) so only the consolidated one withEMAILJS_RECEIPT_TEMPLATE_IDremains.🤖 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 @.env.example around lines 29 - 33, Remove the earlier duplicate EMAILJS_PRIVATE_KEY, EMAILJS_PUBLIC_KEY, EMAILJS_SERVICE_ID, and EMAILJS_TEMPLATE_ID entries from .env.example, preserving the later consolidated block that also includes EMAILJS_RECEIPT_TEMPLATE_ID.Source: Linters/SAST tools
test/jobs.test.js-55-65 (1)
55-65: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
JOBS_DASHBOARD_TOKENis set but never cleaned up, and no success-path test exists.
process.env.JOBS_DASHBOARD_TOKENis mutated globally at Line 56 with noafterEachreset, risking leakage into other test files sharing the same Jest worker. Also, only the failure cases (missing/wrong token) are asserted — there's no test proving a correct bearer token actually returns 200 with job data.✅ Suggested additions
it("protects the jobs dashboard with a bearer token", async () => { process.env.JOBS_DASHBOARD_TOKEN = "dashboard-secret"; const missing = await request(app).get("/admin/jobs"); const wrong = await request(app) .get("/admin/jobs") .set("Authorization", "Bearer wrong"); + const ok = await request(app) + .get("/admin/jobs") + .set("Authorization", "Bearer dashboard-secret"); expect(missing.statusCode).toBe(401); expect(wrong.statusCode).toBe(401); + expect(ok.statusCode).toBe(200); }); + + afterEach(() => { + delete process.env.JOBS_DASHBOARD_TOKEN; + });🤖 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 `@test/jobs.test.js` around lines 55 - 65, Update the “protects the jobs dashboard with a bearer token” test to restore the original JOBS_DASHBOARD_TOKEN value after execution, including when assertions fail, so the environment does not leak between tests. Add a success-path request using the configured “dashboard-secret” bearer token and assert a 200 response containing job data, while preserving the existing missing and incorrect token assertions.Source: Path instructions
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 887b747e-fe4f-46ad-9986-a4a3a5241a79
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (60)
.env.example.gitignoreQUICK_START.mdREADME.mdapp.jsdocs/caching.mdpackage.jsonserver.jsservices/emails/sendMail.jssrc/config/assets.jssrc/config/assets.test.jssrc/config/validateEnv.jssrc/controllers/authController.jssrc/controllers/books/bookController.jssrc/controllers/books/recommendedBooksController.jssrc/controllers/courses/courseController.jssrc/controllers/notificationController.jssrc/controllers/spaceController.jssrc/controllers/stellar/donationController.jssrc/controllers/stellar/paymentController.jssrc/controllers/stellar/refundController.jssrc/controllers/stellar/walletController.jssrc/controllers/uploadController.jssrc/controllers/userController.jssrc/jobs/handlers.jssrc/jobs/queue.jssrc/middlewares/authMiddleware.jssrc/middlewares/errorHandler.jssrc/middlewares/upload.jssrc/models/Book.jssrc/models/Course.jssrc/models/Job.jssrc/models/Notification.jssrc/models/Refund.jssrc/models/Transaction.jssrc/models/User.jssrc/routes/books/bookRoutes.jssrc/routes/jobsRoutes.jssrc/routes/stellar/paymentRoutes.jssrc/routes/uploadRoutes.jssrc/routes/userRoutes.jssrc/routes/wellKnownRoutes.jssrc/services/stellar/multiAsset.test.jssrc/services/stellar/stellarService.jssrc/services/stellar/stellarTomlService.jssrc/utils/fileValidation.jstest/app.test.jstest/auth.test.jstest/authRoles.test.jstest/bookUpload.test.jstest/jobHandlers.test.jstest/jobs.test.jstest/notification.test.jstest/pathPayments.test.jstest/payout.test.jstest/preflightPayment.test.jstest/refund.test.jstest/stellarPaymentController.test.jstest/stellarService.test.jstest/stellarToml.test.js
- refundController.js: fix access-revocation filter to compare
entry.courseId/bookId against refund.itemId instead of the whole
purchase subdocument (the old cId.toString() comparison never
actually matched real purchase records).
- refundController.js: submitRefund now verifies the actual reverse
payment operation (destination, amount, asset) via
verifyPaymentOperations instead of just checking the transaction
exists and succeeded; buyer is fetched once and reused for both
verification and access revocation.
- refundController.js: buildRefundXdr validates refund.currency
against the asset registry before building the reverse payment
(400 for unsupported currencies instead of a 500 from
resolveAsset), and passes it through as assetCode so a refund
settles in the original payment's asset instead of always USDC.
- paymentController.js: both enqueue() calls (verifyPaymentOnChain,
generateReceipt) are now non-fatal - a queue outage no longer rolls
back an already on-chain-confirmed payment or a verified retrying
status.
- paymentController.js: getQuote and initializePayment's path-payment
branch now reject items priced in a non-USDC currency with a 400,
instead of silently treating item.price as a USDC amount for path
payment settlement.
- test/refund.test.js: fixed fixture to seed purchasedCourses with
the {courseId, purchaseDate} subdocument shape the real purchase
flow uses, matching the corrected filter logic.
…istry-60 # Conflicts: # src/models/Course.js
|
@Times-stack please fix CI |
test/refund.test.js "should submit signed XDR, verify Horizon, and revoke item access atomically" started failing (200 expected, 400 received) after the CodeRabbit fix that made submitRefund verify the actual reverse-payment operation via verifyPaymentOperations instead of just checking transaction exists+successful. The mocked Horizon operations().forTransaction() call returned an empty records array, which satisfied the old looser check but not the new one, which needs a payment record matching the expected destination/amount/asset. Fixed by hoisting buyerWallet to outer describe-scope (it was a beforeEach-local const, but the operations mock is set once in beforeAll and needs to read the current buyerWallet via closure), and giving the mock a payment record matching the refund's actual destination, amount, and currency.
2 participants
Closes #60
Summary
Generalizes the payment stack from USDC-only to a small network-aware
asset registry, so items can be priced and settled in USDC, EURC, or
native XLM (extensible to more registry entries later) while existing
USDC flows keep working unchanged.
What changed
src/config/assets.js(new): network-aware registry — code,issuer, decimals, displayName, isDefault — for
testnet/mainnet.EURC issuer addresses verified against Circle's official docs
(developers.circle.com/stablecoins/eurc-contract-addresses). USDC
stays
isDefault: trueso nothing breaks.stellarService.js:buildPaymentTransaction,buildReversePaymentTransaction,buildSep7Uri, andverifyPaymentOperationsnow accept anassetCode(default USDC).getAccountBalance/preflightPaymentreport per-assetbalances/trustlinesmaps alongside the originalusdcBalance/hasTrustlinefields. NewhasTrustline(publicKey, assetCode);hasUsdcTrustlinekept as a thin back-compat wrapper.USDC/USDC_ISSUERare now derived from the registry rather thanhardcoded, same exported shape.
Transaction.currencyenum widened from["USDC"]tothe full registry (
getSupportedCodes()), plus newassetIssuer.Course.currency/Book.currencyadded, defaulting to"USDC"— nomigration needed for existing rows.
paymentController.js: rejects items priced in an unsupportedasset with a 400 naming supported codes; threads the item's
currencythrough pre-flight, XDR building, SEP-7 URI generation,and the saved
Transactionrecord for the direct-payment path.walletController.js: wallet endpoints now surface per-assettrustline status via the already-updated
getAccountBalance, so theUI can prompt e.g. "add a EURC trustline."
Deliberate scope boundaries
findPaymentPaths,buildPathPaymentTransaction, andgetQuoteare untouched — settlinga path payment in a chosen non-USDC asset is issue [Enhancement] Path payments: pay in any Stellar asset with USDC settlement via path-payment-strict-receive #27's mechanism,
not this one, per the issue description's own coordination note.
Space.jswas left untouched. Looking at the actual purchaseflow in
paymentController.js,Spaceisn't part of it (onlybook/coursego throughinitializePayment), so acurrencyfield there would be inert. Happy to add it if
Spacepurchases areplanned, but didn't want to add dead schema fields.
Tests
src/config/assets.test.js: registry resolution per network(USDC/EURC/XLM issuers, default asset, unsupported-code handling).
src/services/stellar/multiAsset.test.js: native-asset (XLM)payment building, non-USDC (EURC) trustline check, USDC default path
unchanged, multi-asset
getAccountBalanceshape.stellarPaymentController.test.jsandstellarService.test.jsfor the new
verifyPaymentOperationssignature and generalized errormessages/balance shape.
npm testpasses locally aside from failures caused by missinglocal
.env/MongoDB (confirmed CI provides both via.github/workflows/ci.yml, unrelated to this change).Acceptance criteria checklist (from #60)
buildPaymentTransactionbuilds valid payments in any registryasset, including native XLM
end-to-end; Transaction records correct
currency/assetIssuermissing trustline rejected pre-flight
non-USDC trustline check, USDC default path
Summary by CodeRabbit