feat(anchor): SEP-24 non-custodial fiat on/off-ramp for USDC - #56
feat(anchor): SEP-24 non-custodial fiat on/off-ramp for USDC #56Cyber-Mitch wants to merge 6 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR adds a non-custodial Stellar SEP-24 anchor integration with SEP-10 authentication, Redis-backed JWT storage, interactive deposit and withdrawal flows, transaction tracking, background polling, route wiring, configuration, documentation, and integration tests. It also adjusts search caching and avatar upload middleware. ChangesStellar anchor integration
Adjacent route maintenance
Estimated code review effort: 5 (Critical) | ~90 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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/services/stellar/stellarService.jsFile contains syntax errors that prevent linting: Line 376: Illegal use of an export declaration not at the top level; Line 378: Illegal use of an export declaration not at the top level; Line 389: Illegal use of an export declaration not at the top level; Line 418: Illegal use of an export declaration not at the top level; Line 512: Illegal use of an export declaration not at the top level; Line 577: Illegal use of an export declaration not at the top level; Line 623: Illegal use of an export declaration not at the top level; Line 677: Illegal use of an export declaration not at the top level; Line 706: Illegal use of an export declaration not at the top level; Line 765: Illegal use of an export declaration not at the top level; Line 778: Illegal use of an export declaration not at the top level; Line 780: Illegal use of an export declaration not at the top level; Line 788: Illegal use of an export declaration not at the top level; Line 796: Illegal use of an export declaration not at the top level; Line 798: Illegal use of an export declaration not at the top level; Line 807: expected 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: 9
🧹 Nitpick comments (12)
src/controllers/stellar/paymentController.js (2)
277-318: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winValidate the path-payment inputs before feeding them into the SDK.
sendMax,pathInput, andsendAssetInput.{code,issuer}come straight from the request body intotoStroops(sendMax)(BigInt parse) andnew StellarSdk.Asset(...). Malformed values throw and fall through to the genericcatchas an opaque 500 instead of a clear 400. A small shape/format check (numericsendMax, valid asset code/issuer) keeps the contract clean and the errors actionable.Note on threat model: since the wallet signs and
sendMaxonly bounds the buyer's own send while the server setsdestAmount, this is an input-hygiene issue rather than a platform-fund risk — so a validation guard is sufficient.As per path instructions: "Flag ... unvalidated request input."
🤖 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 277 - 318, Validate path-payment request inputs before the isPathPayment branch constructs StellarSdk.Asset or calls buildPathPaymentTransaction: require a valid numeric sendMax, a well-formed pathInput array, and valid sendAssetInput code and issuer values. Return a clear client-error response for malformed input, while preserving the existing transaction-building flow for valid values.Source: Path instructions
36-141: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNew
/quoteendpoint needs Jest coverage. This is a brand-new externally reachable endpoint with non-trivial branching (item validation, issuer rules, path lookup, slippage clamping), yet the added tests only exercise theapplySlippage/toStroopshelpers directly — the controller flow itself is untested. A mocked test aroundgetQuotewould lock in the 400/404/200 contracts before merge.Minor secondary note: the success payload uses
{ success, quote }rather than the{ success, message, data }envelope; not blocking given the rest of this controller varies too, just flagging for consistency.As per path instructions: "New or changed endpoints need Jest coverage (the runner uses --experimental-vm-modules) and consistent response shapes ({ success, 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/stellar/paymentController.js` around lines 36 - 141, Add Jest coverage for the externally reachable getQuote controller using mocked models, payment-path lookup, and response objects, covering invalid item types, missing items, free items, issuer validation, no paths, and successful slippage-clamped quotes. Also align the successful response with the established { success, message, data } envelope by placing the quote under data while preserving existing error status behavior.Source: Path instructions
src/models/AnchorTransaction.js (1)
10-64: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueNice schema — precision-safe string amounts and a well-scoped unique index. Two redundant indexes to trim, though.
Thanks for storing amounts as strings and scoping
anchorTransactionIduniqueness perhomeDomain— both are exactly right for SEP‑24.Two single-field indexes are shadowed by the compound indexes you added, so Mongo builds and maintains them for nothing (extra write cost + storage, and Mongoose will emit duplicate-index warnings):
user(Line 10) is already covered as the prefix of{ user: 1, status: 1 }(Line 59).status(Line 38) is already covered as the prefix of{ status: 1, nextPollAt: 1 }(Line 64).
nextPollAt(Line 52) is fine to keep — it isn't a prefix of any compound index.♻️ Drop the shadowed field-level indexes
user: { type: mongoose.Schema.Types.ObjectId, ref: "User", required: true, - index: true, },status: { type: String, required: true, default: "incomplete", - index: true, },🤖 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/AnchorTransaction.js` around lines 10 - 64, Remove the field-level indexes from the user and status schema definitions in AnchorTransaction, while preserving the compound indexes { user: 1, status: 1 } and { status: 1, nextPollAt: 1 }. Keep nextPollAt indexed and leave all other schema behavior unchanged.src/controllers/stellar/anchorController.js (1)
42-43: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueResponse payloads diverge from the documented
{ success, message, data }shape.These endpoints return the payload under ad-hoc keys —
anchorhere, andchallenge,transactions,transaction, and[kind]in the sibling handlers — rather than a consistentdataenvelope. Consolidating on the standard shape keeps clients from special-casing each anchor route. If the surrounding codebase has genuinely settled on these domain keys, feel free to keep them for consistency and treat this as a heads-up.As per path instructions: "consistent response shapes ({ success, 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/stellar/anchorController.js` around lines 42 - 43, Update the response construction in the anchor controller handlers, including the flow containing getAnchorInfo, to use the documented { success, message, data } shape: move anchor, challenge, transactions, transaction, and dynamic kind payloads under data while preserving their existing values and success status. Keep any established message behavior unchanged.Source: Path instructions
src/jobs/anchorPoller.js (1)
96-108: 🚀 Performance & Scalability | 🔵 TrivialThroughput note: one record claimed per tick.
tick()claims and refreshes a single record, and the interval fires everyPOLL_TICK_MS(5s), so the effective drain rate is ~1 transaction / 5s. That's totally fine at low volume, but if a burst of deposits/withdrawals stacks up, freshly-due records will queue behind the cadence. If you expect concurrency, consider claiming a small batch per tick (bounded, e.g.Promise.allover N claims) so the loop scales with load rather than wall-clock ticks. Not a blocker — just flagging before this meets production traffic.🤖 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/anchorPoller.js` around lines 96 - 108, Increase anchor poller throughput by updating tick to claim and refresh a bounded batch of due records per interval instead of only one. Use a small fixed concurrency limit with Promise.all, while preserving the accepting guard and existing error handling in startAnchorPoller.services/emails/sendMail.js (1)
11-17: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winValidate
EMAILJS_PRIVATE_KEYalongside the other required vars.
accessToken: EMAILJS_PRIVATE_KEY(line 23) is required by the EmailJS API but isn't included in the pre-flight guard, so a missing private key surfaces only as a runtime API failure instead of the clear "not configured" error the other three vars get.if ( process.env.NODE_ENV !== "test" && - (!EMAILJS_SERVICE_ID || !templateId || !EMAILJS_PUBLIC_KEY) + (!EMAILJS_SERVICE_ID || !templateId || !EMAILJS_PUBLIC_KEY || !EMAILJS_PRIVATE_KEY) ) { throw new Error("EmailJS environment variables are not configured"); }🤖 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 11 - 17, Update the pre-flight guard in sendTemplate to also require EMAILJS_PRIVATE_KEY alongside EMAILJS_SERVICE_ID, templateId, and EMAILJS_PUBLIC_KEY, while preserving the existing test-environment bypass and configuration error.src/routes/jobsRoutes.js (2)
13-20: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winUse a constant-time comparison for the dashboard bearer token.
req.headers.authorization !== \Bearer ${token}`` is a standard string comparison, which leaks timing information about how many leading characters match. Low practical risk over the network, but cheap to harden:+import { timingSafeEqual } from "crypto"; + +const safeEqual = (a, b) => { + const bufA = Buffer.from(a); + const bufB = Buffer.from(b); + return bufA.length === bufB.length && timingSafeEqual(bufA, bufB); +}; + 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}`) { + if (!safeEqual(req.headers.authorization || "", `Bearer ${token}`)) { return res.status(401).json({ success: false, message: "Unauthorized" }); } next(); });🤖 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, Update the authorization check in the router middleware to compare the provided Bearer token using a constant-time comparison, ensuring length mismatches are handled safely before invoking the comparison. Preserve the existing 404 response for a missing JOBS_DASHBOARD_TOKEN, 401 response for invalid credentials, and next() behavior for valid credentials.
22-36: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMissing test coverage for the authorized path and
/dead.
test/jobs.test.jsonly covers missing/wrong bearer token (401s) for/admin/jobs. There's no test for a successful, authorized request to/admin/jobs, nor any coverage for/admin/jobs/dead.As per path instructions, "New or changed endpoints need Jest coverage" for this Express 5 ESM + Mongoose 8 API.
🤖 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 22 - 36, Add Jest coverage in test/jobs.test.js for an authorized request to /admin/jobs, asserting the successful response and expected job data, and for /admin/jobs/dead, asserting its successful response and dead-job results. Mock or seed Job queries consistently with the existing tests and preserve coverage for the current unauthorized 401 cases.Source: Path instructions
src/models/Job.js (1)
1-28: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAdd indexes for the dashboard's actual query patterns.
src/routes/jobsRoutes.jsrunsJob.find().sort({ createdAt: -1 }).limit(100)andJob.find({ status: "dead" }).sort({ failedAt: -1 }), but this schema has no index supporting either sort — both will require in-memory sorts (or scans) that get slower as the collection grows. Since there's no cleanup/archival ofcompleted/deadjobs, this collection only grows, so add indexes for these access patterns now:jobSchema.index({ status: 1, runAt: 1 }); +jobSchema.index({ createdAt: -1 }); +jobSchema.index({ status: 1, failedAt: -1 });Also worth reconsidering:
name: { ..., index: true }isn't used as a standalone lookup filter anywhere in the reviewed code — if that holds, it's extra write overhead for no query benefit.🤖 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 1 - 28, Add compound indexes to jobSchema for the dashboard queries: createdAt descending for the unfiltered recent-jobs query, and status ascending plus failedAt descending for dead-job queries. Remove the standalone name index from the name field unless another reviewed query depends on it.Source: Path instructions
test/jobs.test.js (1)
12-19: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRestore mutated env vars after tests.
QUEUE_DRIVERandJOBS_DASHBOARD_TOKENare set onprocess.envbut never cleaned up. Since Jest's node environment shares the realprocessobject, these can leak into other spec files run in the same worker and cause order-dependent failures.afterEach(() => { jest.useRealTimers(); + delete process.env.JOBS_DASHBOARD_TOKEN; });and restore/delete
QUEUE_DRIVERin anafterAll/afterEachif it wasn't already set before the suite ran.Also applies to: 55-65
🤖 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 12 - 19, Restore environment state mutated by the jobs test suite: capture the original values of QUEUE_DRIVER and JOBS_DASHBOARD_TOKEN before tests run, then restore each value or delete the variable when it was initially unset in afterEach or afterAll. Update the existing beforeEach/afterEach setup around resetInlineQueueForTests and jest.useRealTimers without changing unrelated test behavior.test/jobHandlers.test.js (1)
35-97: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for the
purchasepath inverifyPaymentOnChain.Every test here uses
type: "donation", sorecordSaleEarningsand theUser/Course$addToSetupdates are never exercised — this is why the confirmed-before-side-effects ordering bug flagged insrc/jobs/handlers.js(lines 32-74) wasn't caught. Consider adding atype: "purchase"fixture with a mockedrecordSaleEarningsrejection to assert that a retry re-attempts the side effects rather than short-circuiting via thestatus === "confirmed"guard. Also consider a test for thesendOtpEmailhandler (missing-user error path).As per path instructions, "New or changed endpoints need Jest coverage... Flag missing auth/ownership checks, unvalidated request input, and unhandled promise rejections" for this Express 5 ESM + Mongoose 8 API; the same rigor applies to critical job-handler paths introduced in this PR.
🤖 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/jobHandlers.test.js` around lines 35 - 97, Add purchase-specific coverage to the verifyPaymentOnChain tests using a type: "purchase" fixture, mock recordSaleEarnings to reject, and assert the job remains retryable while side effects are attempted again rather than being skipped by the confirmed-status guard. Also add coverage for sendOtpEmail’s missing-user error path, using the handler’s existing registration and mocks.Source: Path instructions
src/jobs/queue.js (1)
61-77: 🩺 Stability & Availability | 🔵 TrivialNo recovery path for dead-lettered jobs.
Once a Job reaches
dead(orcompleted), callingenqueue()again with the sameidempotencyKeyis a no-op —$setOnInsertonly applies on insert, andprocessNextonly claimsqueued/retryingjobs. Combined with the read-only/admin/jobs/deaddashboard, a permanently dead-lettered job (e.g. agenerateReceiptjob that exhausted retries during an EmailJS outage) has no operational recovery path short of a manual DB edit. Consider adding an admin "requeue" action (reset status/runAt/attemptsMade on a dead job) so this is a proper CI/ops safety net rather than a silent dead end for payment-adjacent notifications.🤖 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, Add an admin requeue action for dead-lettered jobs, integrated with the existing `/admin/jobs/dead` flow and job-processing symbols such as `processNext`. The action must reset the selected job’s status to `queued`, schedule it for immediate execution, and clear its retry-attempt counter so it can be claimed and retried; preserve idempotency behavior for normal `enqueue()` calls and restrict requeueing to appropriate dead/completed jobs.
🤖 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 `@services/emails/sendMail.js`:
- Around line 33-43: Update the info logs in sendOtpEmail and sendReceiptEmail
to avoid exposing raw recipient email addresses, using the project’s existing
masking or truncation approach if available. Preserve the existing logging
context and email-sending behavior while ensuring only a non-sensitive email
representation is logged.
In `@src/controllers/authController.js`:
- Around line 126-131: Update registerUser to generate a new OTP for each
registration attempt, and use that per-request value when enqueueing
sendOtpEmail. Remove reliance on the module-scope generatedOtp from
emailRoutes.js so separate signups never share the same code.
In `@src/controllers/stellar/anchorController.js`:
- Around line 259-266: Validate and normalize the page and limit values in the
Anchor transaction pagination flow before constructing the Mongo query: ensure
page is a positive integer and clamp limit to a safe maximum while preserving
the default values. Use the normalized values for both skip and limit so invalid
or excessively large requests cannot reach AnchorTransaction.find.
- Around line 202-228: Make trustline construction in the deposit flow
best-effort: wrap getAccountBalance and buildChangeTrustTransaction within the
kind === "deposit" branch in local error handling, log the failure using the
controller’s existing logging pattern, and continue returning the successful
response with url and id. Preserve trustlineXdr when building succeeds, but
swallow failures so they do not reach the outer catch or change the 200
response.
In `@src/controllers/stellar/paymentController.js`:
- Around line 484-507: Update the existing transaction-status guard in
initializePayment to include "retrying" alongside "pending" and "submitted", so
transactions awaiting verifyPaymentOnChain cannot be reinitialized before
confirmation completes.
In `@src/jobs/handlers.js`:
- Around line 32-74: Reorder the success path in the verifyPaymentOnChain
handler so purchase side effects—recordSaleEarnings and User/Course
updates—complete before setting and saving transaction.status to confirmed.
Compute purchase.purchaseDate with a new Date() before the save, then set
confirmedAt, clear failureReason, persist the transaction, and queue the
receipt. Preserve the existing confirmed guard and non-purchase behavior.
In `@src/routes/stellar/anchorRoutes.js`:
- Around line 16-22: Update the controllers used by the anchor routes—getInfo,
requestChallenge, verifyChallenge, initiateDeposit, initiateWithdrawal,
getTransactions, and getTransactionById—to return the consistent top-level shape
{ success, message, data }, placing endpoint-specific values such as anchor
inside data and providing data consistently even when no payload exists. Update
the corresponding endpoint Jest assertions to validate this contract for success
and relevant failure responses.
In `@src/services/stellar/anchorService.js`:
- Around line 86-93: Add a bounded timeout option to every anchor HTTP request
in anchorService, including the /info call and the web-auth GET/POST,
/interactive, and /transaction requests. Use the existing service timeout
configuration or a shared constant consistently across all axios calls,
preserving their current error handling and request behavior.
In `@test/anchorJwtCustody.test.js`:
- Around line 23-28: Update the FAKE_ANCHOR_JWT fixture in
anchorJwtCustody.test.js to sign the token with generated ephemeral test-only
key material instead of the hardcoded secret string, while preserving the
existing exp claim and decode-only behavior.
---
Nitpick comments:
In `@services/emails/sendMail.js`:
- Around line 11-17: Update the pre-flight guard in sendTemplate to also require
EMAILJS_PRIVATE_KEY alongside EMAILJS_SERVICE_ID, templateId, and
EMAILJS_PUBLIC_KEY, while preserving the existing test-environment bypass and
configuration error.
In `@src/controllers/stellar/anchorController.js`:
- Around line 42-43: Update the response construction in the anchor controller
handlers, including the flow containing getAnchorInfo, to use the documented {
success, message, data } shape: move anchor, challenge, transactions,
transaction, and dynamic kind payloads under data while preserving their
existing values and success status. Keep any established message behavior
unchanged.
In `@src/controllers/stellar/paymentController.js`:
- Around line 277-318: Validate path-payment request inputs before the
isPathPayment branch constructs StellarSdk.Asset or calls
buildPathPaymentTransaction: require a valid numeric sendMax, a well-formed
pathInput array, and valid sendAssetInput code and issuer values. Return a clear
client-error response for malformed input, while preserving the existing
transaction-building flow for valid values.
- Around line 36-141: Add Jest coverage for the externally reachable getQuote
controller using mocked models, payment-path lookup, and response objects,
covering invalid item types, missing items, free items, issuer validation, no
paths, and successful slippage-clamped quotes. Also align the successful
response with the established { success, message, data } envelope by placing the
quote under data while preserving existing error status behavior.
In `@src/jobs/anchorPoller.js`:
- Around line 96-108: Increase anchor poller throughput by updating tick to
claim and refresh a bounded batch of due records per interval instead of only
one. Use a small fixed concurrency limit with Promise.all, while preserving the
accepting guard and existing error handling in startAnchorPoller.
In `@src/jobs/queue.js`:
- Around line 61-77: Add an admin requeue action for dead-lettered jobs,
integrated with the existing `/admin/jobs/dead` flow and job-processing symbols
such as `processNext`. The action must reset the selected job’s status to
`queued`, schedule it for immediate execution, and clear its retry-attempt
counter so it can be claimed and retried; preserve idempotency behavior for
normal `enqueue()` calls and restrict requeueing to appropriate dead/completed
jobs.
In `@src/models/AnchorTransaction.js`:
- Around line 10-64: Remove the field-level indexes from the user and status
schema definitions in AnchorTransaction, while preserving the compound indexes {
user: 1, status: 1 } and { status: 1, nextPollAt: 1 }. Keep nextPollAt indexed
and leave all other schema behavior unchanged.
In `@src/models/Job.js`:
- Around line 1-28: Add compound indexes to jobSchema for the dashboard queries:
createdAt descending for the unfiltered recent-jobs query, and status ascending
plus failedAt descending for dead-job queries. Remove the standalone name index
from the name field unless another reviewed query depends on it.
In `@src/routes/jobsRoutes.js`:
- Around line 13-20: Update the authorization check in the router middleware to
compare the provided Bearer token using a constant-time comparison, ensuring
length mismatches are handled safely before invoking the comparison. Preserve
the existing 404 response for a missing JOBS_DASHBOARD_TOKEN, 401 response for
invalid credentials, and next() behavior for valid credentials.
- Around line 22-36: Add Jest coverage in test/jobs.test.js for an authorized
request to /admin/jobs, asserting the successful response and expected job data,
and for /admin/jobs/dead, asserting its successful response and dead-job
results. Mock or seed Job queries consistently with the existing tests and
preserve coverage for the current unauthorized 401 cases.
In `@test/jobHandlers.test.js`:
- Around line 35-97: Add purchase-specific coverage to the verifyPaymentOnChain
tests using a type: "purchase" fixture, mock recordSaleEarnings to reject, and
assert the job remains retryable while side effects are attempted again rather
than being skipped by the confirmed-status guard. Also add coverage for
sendOtpEmail’s missing-user error path, using the handler’s existing
registration and mocks.
In `@test/jobs.test.js`:
- Around line 12-19: Restore environment state mutated by the jobs test suite:
capture the original values of QUEUE_DRIVER and JOBS_DASHBOARD_TOKEN before
tests run, then restore each value or delete the variable when it was initially
unset in afterEach or afterAll. Update the existing beforeEach/afterEach setup
around resetInlineQueueForTests and jest.useRealTimers without changing
unrelated test 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: ad21683c-4bfd-45ec-be6f-0d2c54f632cf
📒 Files selected for processing (35)
.env.exampleREADME.mdapp.jsdocs/anchors.mdserver.jsservices/emails/sendMail.jssrc/config/logger.jssrc/config/validateEnv.jssrc/controllers/authController.jssrc/controllers/stellar/anchorController.jssrc/controllers/stellar/donationController.jssrc/controllers/stellar/paymentController.jssrc/jobs/anchorPoller.jssrc/jobs/handlers.jssrc/jobs/queue.jssrc/models/AnchorTransaction.jssrc/models/Job.jssrc/models/Transaction.jssrc/routes/jobsRoutes.jssrc/routes/stellar/anchorRoutes.jssrc/routes/stellar/paymentRoutes.jssrc/services/stellar/anchorService.jssrc/services/stellar/stellarService.jstest/anchorAuth.test.jstest/anchorDiscovery.test.jstest/anchorInteractive.test.jstest/anchorJwtCustody.test.jstest/anchorJwtStorage.test.jstest/anchorPoller.test.jstest/anchorRoutes.test.jstest/anchorTracking.test.jstest/app.test.jstest/jobHandlers.test.jstest/jobs.test.jstest/pathPayments.test.js
- Make trustline building best-effort in initiateInteractiveFlow
- Clamp/validate pagination params on transaction listing
- Normalize anchor route responses to { success, message, data }
- Add ANCHOR_HTTP_TIMEOUT_MS (10s) to outbound anchor HTTP calls
- Replace hardcoded JWT test secret with per-run random key
anchorInteractive.test.js mocked utils/cache.js with only 3 functions, which replaced the module in Jest's registry for all subsequent imports of that specifier — including transitive imports from bookRoutes.js, courseRoutes.js, etc. pulled in via app.js. Spread the real module's exports into the mock factory, overriding only the functions this suite fakes.
|
@zeemscript Please Review |
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 (9)
src/services/stellar/stellarService.js (3)
292-312: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winClose
buildChangeTrustTransactionbefore declaring subsequent exports.The catch block never rethrows or closes the function, so later
exportdeclarations are parsed inside the catch block and the module cannot load. Add the missingthrowand closing braces.Proposed fix
} catch (error) { logger.error("Error building change trust transaction:", error); + 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 `@src/services/stellar/stellarService.js` around lines 292 - 312, Complete buildChangeTrustTransaction’s catch block by logging the error, rethrowing it, and adding the closing braces before any subsequent exports. Ensure later export declarations remain at module scope and the function propagates failures after logging.Source: Linters/SAST tools
294-296: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUse the resilient
client.executepath for every new Horizon account load.These helpers call the fallback
serverdirectly, unlike the surrounding refactored code. That bypasses endpoint rotation, retries, timeouts, and circuit-breaker behavior for trustline preflight and refund flows.Also applies to: 344-346, 508-510
🤖 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 294 - 296, Update the account-loading calls in the trustline preflight and refund flows, including the sites around loadAccount in the relevant service methods, to use the resilient client.execute path instead of calling server.loadAccount directly. Preserve the existing timed operation and ensure every new Horizon account load receives endpoint rotation, retries, timeouts, and circuit-breaker handling through client.execute.
292-302: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winPin change-trust generation to the verified USDC asset.
The exported helper accepts any asset even though this flow is specifically for the configured USDC trustline. Remove the override or reject assets whose code/issuer do not match
USDCandUSDC_ISSUER; otherwise an untrusted caller could obtain unsigned XDR for an arbitrary issuer. As per path instructions, Stellar flows must keep issuer validation server-side while building only the intended unsigned USDC XDR.🤖 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 292 - 302, Update buildChangeTrustTransaction so callers cannot supply an arbitrary asset: remove the asset override and always build the change-trust operation for the configured USDC asset, with server-side validation that its code and issuer match USDC and USDC_ISSUER before generating unsigned XDR.Source: Path instructions
.env.example (2)
64-65: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRemove the server-side
SIGNING_KEYplaceholder from this non-custodial flow.A private-key environment variable conflicts with the stated design and may lead operators to place Stellar signing secrets in the API environment. Document only the public verification material required by SEP-10, if any. As per PR objectives, this integration must avoid Stellar signing keys and server-side signing.
🤖 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 64 - 65, Remove the server-side SIGNING_KEY placeholder from the SEP-10 configuration section in .env.example. Document only the public verification material required by the non-custodial flow, if any, and do not add or retain environment variables for Stellar private keys or server-side signing.
23-26: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep each environment key in one canonical block.
These lines duplicate existing
EMAILJS_*, Redis, and Jitsi configuration. Duplicate assignments make precedence loader-dependent, so an operator can edit one block while another value silently wins.Also applies to: 71-87
🤖 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 23 - 26, Remove the duplicate EMAILJS_* entries from this block and also remove the duplicated Redis and Jitsi configuration in the referenced section. Retain each environment key only in its existing canonical block, preserving the example values there.src/routes/searchRoutes.js (3)
28-29: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd Jest coverage for both changed Express endpoints.
As per path instructions, changed
src/**/*.jsendpoints require Jest coverage.
src/routes/searchRoutes.js#L28-L29: cover/educatorsresponses, cache separation from/, andq/interest/pagination key variations.src/routes/userRoutes.js#L46-L59: cover owner success, non-owner rejection before upload parsing, and avatar upload behavior.🤖 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/searchRoutes.js` around lines 28 - 29, Add Jest tests for src/routes/searchRoutes.js lines 28-29 covering /educators responses, cache separation from /, and cache-key variations for q, interest, and pagination; add tests for src/routes/userRoutes.js lines 46-59 covering owner success, non-owner rejection before upload parsing, and avatar upload behavior. Use the existing endpoint symbols and test conventions, with no direct production-code changes required.Source: Path instructions
28-29: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAlign the new
/educatorsendpoint with the response contract.
searchEducatorsHandlercurrently returns{ success: false, error }on failures, rather than the required{ success, message, data }shape. Update the handler or add an adapter before exposing this route so clients receive a consistent contract.As per path instructions,
src/**/*.jsendpoints must use the{ success, message, data }response shape.🤖 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/searchRoutes.js` around lines 28 - 29, Update searchEducatorsHandler, or add an adapter in the /educators route before it, so both successful and failed responses consistently use the { success, message, data } shape; replace the failure response’s error field with the appropriate message and data values while preserving existing endpoint behavior.Source: Path instructions
18-29: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winRepair the merged cache-key block and remove the duplicate root route.
searchCacheKeyalready ends at line 14, so lines 18-23 contain a top-levelreturn, producing the reported parse failure. The secondrouter.get("/")is also duplicate and normally shadowed by the first. Merge the pagination/filter logic into one function, remove the stale return, and retain one root route. Use??forpageandlimitso the key matches the controller’s undefined-only defaults.Proposed structure
const searchCacheKey = (req) => { const query = req.query.q || req.query.query || ""; const type = req.query.type || "all"; - return `${CACHE_KEYS.SEARCH}${type}:${query.toLowerCase().trim()}`; -}; - -// Main search endpoint - cached for 5 minutes -router.get("/", cacheMiddleware(CACHE_TTL.SEARCH, searchCacheKey), searchAll); - - const page = req.query.page || 1; - const limit = req.query.limit || 10; + const page = req.query.page ?? 1; + const limit = req.query.limit ?? 10; const filterKeys = ["minPrice", "maxPrice", "free", "category", "minRating", "interest", "sort"]; const filtersStr = filterKeys.map((k) => `${k}=${req.query[k] || ""}`).join("&"); return `${CACHE_KEYS.SEARCH}${req.path}:${type}:${query.toLowerCase().trim()}:page=${page}:limit=${limit}:${filtersStr}`; }; -// Main search endpoint +// Main search endpoint - cached for 5 minutes router.get("/", cacheMiddleware(CACHE_TTL.SEARCH, searchCacheKey), searchAll);🤖 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/searchRoutes.js` around lines 18 - 29, Repair searchCacheKey by moving its pagination and filter-key construction inside the function, using nullish coalescing (??) for page and limit to match controller defaults, and removing the stray top-level return. Remove the duplicate root router.get("/") registration, retaining one root route alongside the educators route.Source: Linters/SAST tools
src/routes/userRoutes.js (1)
49-56: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winUse one declared upload middleware, after the ownership check.
uploadis not imported, so evaluatingupload.single("avatar")throws aReferenceErrorwhile registering the router. The route also parses the same multipart request twice. Remove the undefined/duplicate middleware and run the ownership check before the singleuploadImage.single("avatar")call.Proposed fix
router.put( "/update/:id", protect, - upload.single("avatar"), (req, res, next) => { if (req.user._id.toString() !== req.params.id) { return res.status(403).json({ success: false, message: "Not authorized to update this profile", data: null }); } next(); }, uploadImage.single("avatar"), invalidateCacheMiddleware([`${CACHE_KEYS.USER}*`]), updateUser );🤖 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/userRoutes.js` around lines 49 - 56, In the route middleware chain, remove the undefined `upload.single("avatar")` call and place the ownership-check callback before the sole `uploadImage.single("avatar")` middleware. Ensure the multipart request is parsed exactly once after authorization succeeds.
🤖 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 @.env.example:
- Around line 64-65: Remove the server-side SIGNING_KEY placeholder from the
SEP-10 configuration section in .env.example. Document only the public
verification material required by the non-custodial flow, if any, and do not add
or retain environment variables for Stellar private keys or server-side signing.
- Around line 23-26: Remove the duplicate EMAILJS_* entries from this block and
also remove the duplicated Redis and Jitsi configuration in the referenced
section. Retain each environment key only in its existing canonical block,
preserving the example values there.
In `@src/routes/searchRoutes.js`:
- Around line 28-29: Add Jest tests for src/routes/searchRoutes.js lines 28-29
covering /educators responses, cache separation from /, and cache-key variations
for q, interest, and pagination; add tests for src/routes/userRoutes.js lines
46-59 covering owner success, non-owner rejection before upload parsing, and
avatar upload behavior. Use the existing endpoint symbols and test conventions,
with no direct production-code changes required.
- Around line 28-29: Update searchEducatorsHandler, or add an adapter in the
/educators route before it, so both successful and failed responses consistently
use the { success, message, data } shape; replace the failure response’s error
field with the appropriate message and data values while preserving existing
endpoint behavior.
- Around line 18-29: Repair searchCacheKey by moving its pagination and
filter-key construction inside the function, using nullish coalescing (??) for
page and limit to match controller defaults, and removing the stray top-level
return. Remove the duplicate root router.get("/") registration, retaining one
root route alongside the educators route.
In `@src/routes/userRoutes.js`:
- Around line 49-56: In the route middleware chain, remove the undefined
`upload.single("avatar")` call and place the ownership-check callback before the
sole `uploadImage.single("avatar")` middleware. Ensure the multipart request is
parsed exactly once after authorization succeeds.
In `@src/services/stellar/stellarService.js`:
- Around line 292-312: Complete buildChangeTrustTransaction’s catch block by
logging the error, rethrowing it, and adding the closing braces before any
subsequent exports. Ensure later export declarations remain at module scope and
the function propagates failures after logging.
- Around line 294-296: Update the account-loading calls in the trustline
preflight and refund flows, including the sites around loadAccount in the
relevant service methods, to use the resilient client.execute path instead of
calling server.loadAccount directly. Preserve the existing timed operation and
ensure every new Horizon account load receives endpoint rotation, retries,
timeouts, and circuit-breaker handling through client.execute.
- Around line 292-302: Update buildChangeTrustTransaction so callers cannot
supply an arbitrary asset: remove the asset override and always build the
change-trust operation for the configured USDC asset, with server-side
validation that its code and issuer match USDC and USDC_ISSUER before generating
unsigned XDR.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d8fa1c90-bf95-4c99-964d-7c352cc07aab
📒 Files selected for processing (7)
.env.exampleapp.jssrc/config/validateEnv.jssrc/routes/searchRoutes.jssrc/routes/userRoutes.jssrc/services/stellar/stellarService.jstest/app.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
- src/config/validateEnv.js
- test/app.test.js
zeemscript
left a comment
zeemscript
left a comment
There was a problem hiding this comment.
@Cyber-Mitch fix CI please
okay |
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 (1)
src/services/stellar/stellarService.js (1)
371-373: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winClose and rethrow from the
catchblock.
buildChangeTrustTransactionnever closes itscatch/function and does not rethrow. The module cannot parse, so the server and test suite cannot load this service. Restorethrow error;, close the block, and terminate the exported function before the SEP-29 declarations.Proposed fix
} catch (error) { logger.error("Error building change trust transaction:", error); + throw error; + } +}; + // SEP-29: an account opts into requiring a memo on incoming payments by🤖 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 371 - 373, Complete the catch block in buildChangeTrustTransaction by rethrowing the caught error after logging it, then close the catch and exported function before the SEP-29 declarations so the module parses correctly.Source: Linters/SAST tools
🤖 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/services/stellar/stellarService.js`:
- Around line 371-373: Complete the catch block in buildChangeTrustTransaction
by rethrowing the caught error after logging it, then close the catch and
exported function before the SEP-29 declarations so the module parses correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 39e53461-8269-4150-a0dd-9cfeeae6aaca
📒 Files selected for processing (1)
src/services/stellar/stellarService.js
|
I'm still on it @zeemscript |
2 participants
Closes #46
Non-custodial SEP-24 fiat on/off-ramp for USDC: anchor discovery (SEP-1), SEP-10 client-side challenge relay, interactive deposit/withdrawal, and a status-polling worker — all without the server ever holding a Stellar signing key.
No-server-keys checklist
anchorService.js— anyKeypair.fromSecret/secret referenceanchorService.js/anchorController.js/anchorPoller.js— any.sign()callWebAuth.readChallengeTx); signing happens client-side, XDR returned unsignedbuildChangeTrustTransactionreturns unsigned XDR only, mirrorsbuildPaymentTransaction's existing patternexponlyANCHOR_HOME_DOMAINS/ANCHOR_TOML_CACHE_TTLadded — no secret-shaped env varChallenge-validation rejection matrix
Built on the SDK's own audited
WebAuth.readChallengeTx(anchorService.js:126-153), not hand-rolled checks:SIGNING_KEYmanage_datanames the wrong domainJWT custody
anchor:jwt:{userId}:{homeDomain}, TTL from the token's ownexpclaim (decoded, never verified — it's the anchor's signature, not ours).null→ deposit/withdrawal endpoints return401 {requiresReauth: true}rather than attempting a stale-credential anchor call.Poller
src/jobs/anchorPoller.js: 5s tick (.unref()'d, matchingjobs/queue.js's style), atomicfindOneAndUpdateclaim — restart-safe since every tick reads fresh DB state and the claim prevents double-processing across ticks/instances. Exponential backoff with jitter, capped at 6 attempts. Terminal states excluded from the claim query, so the poller simply stops touching a record — no special-case stop logic. Deliberately not built onjobs/queue.js's Job model, per the issue's "align with, not depend on" instruction.Coordination with #25
#25 (SEP-10 server) is open, assigned elsewhere, no PR yet — nothing to share. Both will import
@stellar/stellar-sdk'sWebAuthnamespace, but the functions don't overlap (readChallengeTxhere vs.buildChallengeTxthere), so there's no helper to extract yet.Tests
102 passing (58 baseline + 44 new), 14 suites, zero live network in any test. Boot check passes both testnet (auto-configured) and mainnet-with-no-allowlist (unconfigured → 503, server still boots).
Acceptance criteria (verbatim from #46)
Found, not fixed — flagging separately
cache.js'sgetCacheOrSetdouble-callsfallbackFn()on error (once in try, once in catch) — pre-existing, errors still propagate correctly, just one wasted extra attempt on genuine failure.devis one commit behindmain(missing a Redis cache-layer merge, though the Redis files themselves already exist indevfrom an earlier merge). Worth amain→devsync — will raise separately with the maintainer./infomock, a missingaxios.getdispatch-by-URL) — both fixed. Flagging since this class of mistake is easy to make with multi-call service functions; worth a second look in review of anchor-adjacent PRs.Feature Implemented and Task Completed
@zeemscript Please Review
Summary by CodeRabbit