Add pre-flight payment safety checks with SEP-29 memo detection - #68
Conversation
Add preflightPayment() to stellarService.js, validating destination existence, USDC trustline, source balance/reserve, and SEP-29 memo-required status before an unsigned XDR is built. Wire it into initializePayment (400 with structured reasons, no Transaction row created on failure) and expose it standalone via POST /api/stellar/payment/preflight so the frontend can surface blocking issues before prompting the wallet to sign. Centralize the platform's DNB-<TYPE>-<id8> memo construction into a single helper shared by both call sites.
WalkthroughChangesThe Stellar payment flow adds structured preflight validation for destination existence, USDC trustlines, source balances, XLM reserve coverage, and SEP-29 memo requirements. Shared destination and memo helpers are used by both the preflight endpoint and payment initialization. Stellar payment preflight
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant paymentRoutes
participant getPaymentPreflight
participant preflightPayment
participant Horizon
Client->>paymentRoutes: POST /preflight
paymentRoutes->>getPaymentPreflight: Invoke authenticated handler
getPaymentPreflight->>preflightPayment: Validate payment details
preflightPayment->>Horizon: Load source and destination accounts
Horizon-->>preflightPayment: Return account data
preflightPayment-->>getPaymentPreflight: Return validation verdict
getPaymentPreflight-->>Client: Return preflight response
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR adds a “pre-flight” safety layer to Stellar payments so the backend (and a new API endpoint) can detect common failure modes (missing destination, missing USDC trustline, insufficient balances/reserve, SEP-29 memo-required) before building an unsigned XDR and prompting a wallet signature.
Changes:
- Added
preflightPayment()+ SEP-29config.memo_requireddetection (isMemoRequired) to return a structured{ ok, reasons, warnings }verdict. - Added a new protected endpoint
POST /api/stellar/payment/preflightto let the frontend fetch blocking issues ahead of signing. - Gated
initializePayment(non-path payments) on pre-flight checks and centralized purchase memo formatting viabuildPurchaseMemo().
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/services/stellar/stellarService.js |
Adds preflight validation logic, SEP-29 memo-required detection, and shared parsing helpers for account balances/reserve calculations. |
src/controllers/stellar/paymentController.js |
Adds a preflight controller endpoint, shared destination-resolution helper, centralized memo construction, and preflight gating in the initialize flow. |
src/routes/stellar/paymentRoutes.js |
Exposes the new /preflight payment route. |
test/preflightPayment.test.js |
Adds unit tests covering reason codes, SEP-29 behavior, reserve math, and multi-reason collection using mocked Horizon calls. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Build the payment transaction (single op full amount for platform collect, split for direct if fee configured) | ||
| const isPathPayment = sendAssetInput && sendMax; |
| const Model = itemType === "book" ? Book : Course; | ||
| const populateField = itemType === "book" ? "author" : "createdBy"; | ||
|
|
||
| const query = Model.findById(itemId).populate(populateField, "stellarWallet name"); | ||
| const item = session ? await query.session(session) : await query; |
| // Stellar protocol base reserve is 0.5 XLM per ledger entry (2 base entries | ||
| // per account, plus one more per subentry: trustlines, offers, signers, data). | ||
| const BASE_RESERVE_STROOPS = 5000000n; | ||
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/services/stellar/stellarService.js (1)
340-416: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSolid preflight implementation — verified the reserve/fee math by hand against the test cases.
Promise.allfor parallel account loads, BigInt stroops throughout (no floats), and collecting all reasons instead of short-circuiting all check out. This satisfies the non-custodial/BigInt requirements forsrc/services/stellar/**.One gap worth flagging:
preflightPaymentassumes the source is paying in USDC (SOURCE_INSUFFICIENT_BALANCE/SOURCE_INSUFFICIENT_RESERVEare computed againstsummary.usdcBalance). If this is ever called for a different source asset (e.g. from the path-payment flow), these checks would be meaningless or misleading. CurrentlypaymentController.jscorrectly avoids calling this for path payments — but that means path payments get no preflight validation, including the asset-agnostic destination checks (existence/trustline/memo-required). See the corresponding comment onpaymentController.js.🤖 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 340 - 416, Update preflightPayment to separate asset-agnostic destination validation from source-asset-specific balance and reserve checks, so callers using non-USDC assets do not receive misleading USDC insufficiency reasons. Preserve the existing destination account, trustline, and memo checks, and adjust the calling flow as needed to run those checks for path payments without applying USDC-specific source validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/controllers/stellar/paymentController.js`:
- Around line 211-273: Extract the duplicated fee-split and preflightPayment
invocation from getPaymentPreflight and initializePayment into a shared
runPreflight helper. Have the helper accept the buyer wallet,
destinationPublicKey, item, memo, and settlementMode, preserve the existing
amount and operationCount behavior, and update both handlers to use it.
- Around line 211-273: The new getPaymentPreflight endpoint lacks
controller-level Jest coverage. Add endpoint tests that mock
resolvePaymentDestination and preflightPayment, covering invalid itemType,
missing Stellar wallet, free items, and a successful response asserting the 200
status and preflight payload shape; ensure the tests use the project’s
--experimental-vm-modules-compatible setup.
- Around line 401-420: Ensure the isPathPayment branch also performs
destination-side preflight validation before buildPathPaymentTransaction and
wallet signing. Refactor preflightPayment or extract a destination-only
validation helper so checks such as missing accounts, trustlines, and required
memos apply without assuming a USDC source asset; preserve the existing
source-asset checks for non-path payments and abort the session with the
existing failure response when validation fails.
---
Nitpick comments:
In `@src/services/stellar/stellarService.js`:
- Around line 340-416: Update preflightPayment to separate asset-agnostic
destination validation from source-asset-specific balance and reserve checks, so
callers using non-USDC assets do not receive misleading USDC insufficiency
reasons. Preserve the existing destination account, trustline, and memo checks,
and adjust the calling flow as needed to run those checks for path payments
without applying USDC-specific source validation.
🪄 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: b64b3bd2-60e5-48f9-b1a4-a307ffeacf18
📒 Files selected for processing (4)
src/controllers/stellar/paymentController.jssrc/routes/stellar/paymentRoutes.jssrc/services/stellar/stellarService.jstest/preflightPayment.test.js
| export const getPaymentPreflight = async (req, res) => { | ||
| try { | ||
| const buyerId = req.user._id; | ||
| const { itemType, itemId } = req.body; | ||
|
|
||
| if (!["book", "course"].includes(itemType)) { | ||
| return res.status(400).json({ | ||
| success: false, | ||
| message: "Invalid item type. Must be 'book' or 'course'", | ||
| }); | ||
| } | ||
|
|
||
| const buyer = await User.findById(buyerId); | ||
| if (!buyer?.stellarWallet?.publicKey) { | ||
| return res.status(400).json({ | ||
| success: false, | ||
| message: "Please connect your Stellar wallet first", | ||
| }); | ||
| } | ||
|
|
||
| const resolved = await resolvePaymentDestination({ itemType, itemId }); | ||
| if (resolved.error) { | ||
| return res.status(resolved.error.status).json({ | ||
| success: false, | ||
| message: resolved.error.message, | ||
| }); | ||
| } | ||
|
|
||
| const { item, destinationPublicKey, settlementMode } = resolved; | ||
|
|
||
| if (!item.price || item.price === 0) { | ||
| return res.status(400).json({ | ||
| success: false, | ||
| message: "This item is free, no payment required", | ||
| }); | ||
| } | ||
|
|
||
| const memo = buildPurchaseMemo(itemType, itemId); | ||
| const feeSplitPreview = | ||
| settlementMode === "direct" ? calculateFeeSplit(item.price) : null; | ||
|
|
||
| const preflight = await preflightPayment({ | ||
| sourcePublicKey: buyer.stellarWallet.publicKey, | ||
| destinationPublicKey, | ||
| amount: item.price.toString(), | ||
| memo, | ||
| operationCount: feeSplitPreview ? 2 : 1, | ||
| }); | ||
|
|
||
| res.status(200).json({ | ||
| success: true, | ||
| preflight, | ||
| }); | ||
| } catch (error) { | ||
| logger.error("Payment preflight error:", error); | ||
| res.status(500).json({ | ||
| success: false, | ||
| message: "Failed to run payment pre-flight checks", | ||
| error: | ||
| process.env.NODE_ENV === "development" ? error.message : undefined, | ||
| }); | ||
| } | ||
| }; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Endpoint logic looks correct, but this handler and initializePayment's new preflight block (Lines 401-410) are byte-for-byte duplicated logic.
Both compute feeSplitPreview and call preflightPayment with the identical shape. If the operationCount logic or preflight params ever need a fix (e.g. adding sendAsset support), it's easy to update one call site and forget the other, silently reintroducing the exact race this PR is trying to close.
Consider extracting a shared helper, e.g.:
♻️ Suggested extraction
const runPreflight = async ({ buyerWallet, destinationPublicKey, item, memo, settlementMode }) => {
const feeSplitPreview =
settlementMode === "direct" ? calculateFeeSplit(item.price) : null;
return preflightPayment({
sourcePublicKey: buyerWallet,
destinationPublicKey,
amount: item.price.toString(),
memo,
operationCount: feeSplitPreview ? 2 : 1,
});
};Then call runPreflight(...) from both getPaymentPreflight and initializePayment.
🤖 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 211 - 273, Extract
the duplicated fee-split and preflightPayment invocation from
getPaymentPreflight and initializePayment into a shared runPreflight helper.
Have the helper accept the buyer wallet, destinationPublicKey, item, memo, and
settlementMode, preserve the existing amount and operationCount behavior, and
update both handlers to use it.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
No Jest coverage found for the new getPaymentPreflight endpoint.
The only tests supplied (test/preflightPayment.test.js) exercise stellarService.preflightPayment/isMemoRequired directly, not the controller/route. Given this endpoint drives frontend payment gating, an endpoint-level test (mocking resolvePaymentDestination/preflightPayment, asserting the 400s for invalid itemType, missing wallet, free item, and the 200 success shape) would close the gap.
As per path instructions, "New or changed endpoints need Jest coverage (the runner uses --experimental-vm-modules)."
🤖 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 211 - 273, The new
getPaymentPreflight endpoint lacks controller-level Jest coverage. Add endpoint
tests that mock resolvePaymentDestination and preflightPayment, covering invalid
itemType, missing Stellar wallet, free items, and a successful response
asserting the 200 status and preflight payload shape; ensure the tests use the
project’s --experimental-vm-modules-compatible setup.
Source: Path instructions
| const feeSplitPreview = | ||
| settlementMode === "direct" ? calculateFeeSplit(item.price) : null; | ||
|
|
||
| const preflight = await preflightPayment({ | ||
| sourcePublicKey: buyer.stellarWallet.publicKey, | ||
| destinationPublicKey, | ||
| amount: item.price.toString(), | ||
| memo, | ||
| operationCount: feeSplitPreview ? 2 : 1, | ||
| }); | ||
|
|
||
| if (!preflight.ok) { | ||
| await session.abortTransaction(); | ||
| return res.status(400).json({ | ||
| success: false, | ||
| message: "Payment failed pre-flight safety checks", | ||
| reasons: preflight.reasons, | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Preflight gate correctly blocks and aborts on failure — but it only runs for the non-path-payment branch.
isPathPayment (Line 373) transactions skip preflightPayment entirely and go straight to buildPathPaymentTransaction. That means for path payments, none of the destination checks (DESTINATION_ACCOUNT_MISSING, DESTINATION_NO_TRUSTLINE, DESTINATION_MEMO_REQUIRED) — which are asset-agnostic and would apply regardless of the source asset — are validated before the wallet is asked to sign. Issue #59 explicitly calls for pre-flight validation "before unsigned XDR creation and wallet signing," without carving out an exception for path payments.
I understand preflightPayment's source-side checks (USDC balance/reserve) can't apply as-is to path payments since the source asset may not be USDC — but the destination-side checks could still run. Worth either:
- Splitting
preflightPaymentso destination-only checks can be invoked independently of source-asset assumptions, or - Explicitly documenting that path payments intentionally rely solely on the submit-time result-code backstop for now.
Right now the fallback (submit-time result-code handling) does still catch this at signing/submission time, which limits the blast radius — but the wallet will still be asked to sign a doomed-to-fail transaction, which is exactly what this PR set out to prevent.
🤖 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 401 - 420, Ensure
the isPathPayment branch also performs destination-side preflight validation
before buildPathPaymentTransaction and wallet signing. Refactor preflightPayment
or extract a destination-only validation helper so checks such as missing
accounts, trustlines, and required memos apply without assuming a USDC source
asset; preserve the existing source-asset checks for non-path payments and abort
the session with the existing failure response when validation fails.
Summary
Closes #59.
preflightPayment()tostellarService.js: runs destination existence, USDC trustline, source USDC balance, source XLM reserve/fee, and SEP-29config.memo_requiredchecks in parallel and returns a structured{ ok, reasons, warnings }verdict instead of throwing.isMemoRequired()reading the destination account'sdata_attr["config.memo_required"]entry per SEP-29.initializePaymentonpreflightPayment: on failure it aborts the Mongo transaction, creates noTransactionrow, and returns 400 with the structured reason codes. The happy path is unchanged.POST /api/stellar/payment/preflight(getPaymentPreflight) so the frontend can show blocking issues before prompting the wallet to sign.DNB-<TYPE>-<id8>memo construction into onebuildPurchaseMemo()helper shared by both call sites, with the memo convention documented inline.result_codeshandling insubmitTransactionis left as-is as a backstop for races.Test plan
test/preflightPayment.test.js— 14 tests covering each reason code individually, SEP-29 pass/fail, reserve math with extra operations (fee split), and multi-reason collection, all withserver.loadAccountmocked (no live Horizon calls).pathPayments.test.js/payout.test.js/auth.test.jsreproduce identically on the base commit and are unrelated environment issues, not caused by this diff).Summary by CodeRabbit
New Features
Bug Fixes
Tests