feat(stellar): add fee-bump sponsorship - #50
Conversation
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughFee sponsorship is added for Stellar payments and donations through validated fee-bump transactions, configurable spend limits, persisted sponsorship metadata, and an authenticated sponsorship status endpoint. ChangesFee sponsorship flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant PaymentController
participant FeeSponsorService
participant FeeSponsorDailySpend
participant StellarServer
participant Transaction
Client->>PaymentController: Submit signed XDR with requestSponsorship
PaymentController->>FeeSponsorService: Validate and prepare fee bump
FeeSponsorService->>FeeSponsorDailySpend: Reserve sponsorship stroops
FeeSponsorService->>StellarServer: Submit signed fee-bump transaction
StellarServer-->>FeeSponsorService: Return transaction hash and charged fee
FeeSponsorService->>FeeSponsorDailySpend: Reconcile reservation
FeeSponsorService-->>PaymentController: Return sponsorship result
PaymentController->>Transaction: Persist sponsorship metadata
PaymentController-->>Client: Return submission response
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
src/models/FeeSponsorDailySpend.js (1)
3-10: 🚀 Performance & Scalability | 🔵 TrivialSchema looks solid and the fields map exactly to the atomic
findOneAndUpdatereservation infeeSponsorService.js(dateKey,totalStroops,perUser.<userKey>). ThedateKeyunique index correctly supports the daily lookup/reservation path, and I appreciate that you deliberately avoided a TTL here — these accounting rows shouldn't be auto-deleted mid-cap-window.One operational note for later: since there's (rightly) no TTL, one document accrues per UTC day forever. That's tiny data, but a periodic archival/cleanup job for rows older than your retention window keeps the collection tidy without risking active-day records.
🤖 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/FeeSponsorDailySpend.js` around lines 3 - 10, No code change is required for this review; retain the current feeSponsorDailySpendSchema design, including the unique dateKey index and absence of a TTL.src/config/validateEnv.js (1)
31-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a fail-fast check for enabled-but-unconfigured sponsorship.
Treating all five as optional is right — the feature is opt-in. But there's a gap: if
FEE_SPONSOR_ENABLED="true"whileFEE_SPONSOR_SECRETis unset, validation still passes (the production-only warning at Line 79 is easy to miss), and the failure only surfaces later as a runtime503 sponsor_unavailablefromsponsorKeypair(). Failing fast at boot makes the misconfiguration obvious to operators instead of silently disabling sponsorship in prod.♻️ Suggested conditional validation (add inside
validateEnv, after the required-vars check)if (process.env.FEE_SPONSOR_ENABLED === "true" && !process.env.FEE_SPONSOR_SECRET) { logger.error( "❌ FEE_SPONSOR_ENABLED is 'true' but FEE_SPONSOR_SECRET is not set." ); process.exit(1); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/config/validateEnv.js` around lines 31 - 35, Update validateEnv after the required-vars check to fail fast when FEE_SPONSOR_ENABLED is exactly "true" but FEE_SPONSOR_SECRET is unset: log a clear configuration error through logger and exit with status 1. Keep the five sponsorship variables optional when sponsorship is disabled.
🤖 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 444-450: Standardize the sponsorship responses across all listed
sites to the { success, message, data } envelope: in paymentController.js lines
444-450 and donationController.js lines 172-178, move code and canSubmitNormally
into data; in paymentController.js lines 641-645, place the sponsorship status
under data and provide a consistent message.
- Around line 436-464: Update the sponsored submission flow around
submitSponsoredTransaction in src/controllers/stellar/paymentController.js
(lines 436-464) to durably persist a sponsorship attempt/idempotency key before
the external call, reconcile ambiguous outcomes by the inner transaction hash
before allowing fallback, and handle subsequent database failures without
leaving the payment unrecoverable. Apply the same durable-attempt persistence
and unknown-outcome reconciliation to the corresponding sponsored submission
flow in src/controllers/stellar/donationController.js (lines 166-192).
- Line 410: Validate requestSponsorship with an explicit boolean check before
selecting the sponsorship flow in both
src/controllers/stellar/paymentController.js at lines 410-410 and
src/controllers/stellar/donationController.js at lines 139-139; return a 400
response for any non-boolean value while preserving normal handling for true and
false.
In `@src/models/Transaction.js`:
- Around line 16-19: Update the sponsorFeeStroops field in the Transaction model
to represent non-negative integer stroops safely, using the project’s validated
decimal-string/BigInt-compatible convention rather than an unrestricted Number
with only min: 0. Propagate the same representation and validation through
related services and controllers, ensuring all Stellar amount calculations use
integer stroops or BigInt.
In `@src/services/stellar/feeSponsorService.js`:
- Around line 40-43: Update sameAmount to parse both Stellar amount values into
exact integer stroops, using BigInt or the existing precise amount parser, and
compare those integers directly. Remove the Number(...).toFixed(7) normalization
so distinct high-value payments cannot round to the same comparison value.
---
Nitpick comments:
In `@src/config/validateEnv.js`:
- Around line 31-35: Update validateEnv after the required-vars check to fail
fast when FEE_SPONSOR_ENABLED is exactly "true" but FEE_SPONSOR_SECRET is unset:
log a clear configuration error through logger and exit with status 1. Keep the
five sponsorship variables optional when sponsorship is disabled.
In `@src/models/FeeSponsorDailySpend.js`:
- Around line 3-10: No code change is required for this review; retain the
current feeSponsorDailySpendSchema design, including the unique dateKey index
and absence of a TTL.
🪄 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: dad3ce6c-ad5b-4dd8-a053-0f0df837d19b
📒 Files selected for processing (10)
.env.examplesrc/config/validateEnv.jssrc/controllers/stellar/donationController.jssrc/controllers/stellar/paymentController.jssrc/models/FeeSponsorDailySpend.jssrc/models/Transaction.jssrc/routes/stellar/paymentRoutes.jssrc/services/stellar/feeSponsorService.jssrc/services/stellar/stellarService.jstest/feeSponsor.test.js
|
Addressed all five actionable findings in commit 9568232:
Validation: 38 tests passed across 5 suites; syntax and diff checks are clean. @coderabbitai review |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/models/Transaction.js (1)
26-30: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winOptional: make
sponsorshipAttemptKeya partial unique index to harden idempotency.The staged submission flow uses
sponsorshipAttemptKey(the inner tx hash) as the idempotency key, but today the guarantee lives only in controller logic. A DB-level partial unique index would prevent two rows from ever claiming the same sponsored inner hash even under a race, which is the kind of belt-and-suspenders worth having on a payment path. Keep it partial/sparse since most documents leave this field unset.transactionSchema.index( { sponsorshipAttemptKey: 1 }, { unique: true, partialFilterExpression: { sponsorshipAttemptKey: { $type: "string" } } } );🤖 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/Transaction.js` around lines 26 - 30, Update the transaction schema around sponsorshipAttemptKey to add a partial unique index on sponsorshipAttemptKey, applying uniqueness only when the field contains a string. Preserve the existing field definition and ensure documents without sponsorshipAttemptKey remain allowed.Source: Path instructions
src/controllers/stellar/paymentController.js (1)
448-464: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftAvoid holding the MongoDB transaction open across the Horizon reconcile call.
The
elsebranch was carefully written tocommitTransaction()before the externalsubmitPreparedSponsoredTransactioncall — good, because MongoDB multi-document transactions have atransactionLifetimeLimitSeconds(default 60s) and hold locks while open. The reconcile-prior branch breaks that discipline: it callsreconcileSponsoredTransaction(...)(a Horizon HTTP fetch of up to 200 records) while the transaction started at the top of the handler is still open, and then keeps that same transaction open through the laterverifyPaymentOperationsnetwork call up to the final commit. Under slow Horizon responses this can trip the transaction lifetime limit and abort a legitimate sponsored recovery, or hold locks longer than necessary.Recommend committing/ending the read transaction before the external reconcile call and re-opening a fresh transaction afterward (as the
elsebranch already does), so external I/O never runs inside an open DB transaction.
src/controllers/stellar/paymentController.js#L448-L464: commit/end the transaction beforereconcileSponsoredTransaction(transaction.sponsorshipAttemptKey), then re-startTransaction()before continuing to the submitted/verify steps.src/controllers/stellar/donationController.js#L178-L194: apply the same restructuring aroundreconcileSponsoredTransaction(donation.sponsorshipAttemptKey).🤖 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 448 - 464, In src/controllers/stellar/paymentController.js lines 448-464, commit/end the current read transaction before calling reconcileSponsoredTransaction, then start a fresh transaction before the subsequent submitted/verify flow. Apply the same restructuring around reconcileSponsoredTransaction in src/controllers/stellar/donationController.js lines 178-194; ensure no external Horizon or verification network call runs while a MongoDB transaction is open.
🤖 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 730-742: Restrict getSponsorshipStatus to administrators by
applying the existing admin authorization guard in the route configuration
before this handler runs. Keep protect for authentication, ensure non-admin
users are rejected, and preserve the handler’s current response behavior for
authorized requests.
---
Nitpick comments:
In `@src/controllers/stellar/paymentController.js`:
- Around line 448-464: In src/controllers/stellar/paymentController.js lines
448-464, commit/end the current read transaction before calling
reconcileSponsoredTransaction, then start a fresh transaction before the
subsequent submitted/verify flow. Apply the same restructuring around
reconcileSponsoredTransaction in src/controllers/stellar/donationController.js
lines 178-194; ensure no external Horizon or verification network call runs
while a MongoDB transaction is open.
In `@src/models/Transaction.js`:
- Around line 26-30: Update the transaction schema around sponsorshipAttemptKey
to add a partial unique index on sponsorshipAttemptKey, applying uniqueness only
when the field contains a string. Preserve the existing field definition and
ensure documents without sponsorshipAttemptKey remain allowed.
🪄 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: 48ab9f53-13b9-4de3-b8b8-a63eba18258d
📒 Files selected for processing (6)
src/config/validateEnv.jssrc/controllers/stellar/donationController.jssrc/controllers/stellar/paymentController.jssrc/models/Transaction.jssrc/services/stellar/feeSponsorService.jstest/feeSponsor.test.js
🚧 Files skipped from review as they are similar to previous changes (3)
- src/config/validateEnv.js
- test/feeSponsor.test.js
- src/services/stellar/feeSponsorService.js
| export const getSponsorshipStatus = async (req, res) => { | ||
| try { | ||
| res.status(200).json({ | ||
| success: true, | ||
| message: "Fee sponsorship status retrieved", | ||
| data: { sponsorship: await getFeeSponsorStatus() }, | ||
| }); | ||
| } catch (error) { | ||
| logger.error("Fee sponsorship status error:", error); | ||
| const status = error instanceof FeeSponsorshipError ? error.status : 500; | ||
| res.status(status).json({ success: false, message: error.message, data: null }); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect how the sponsorship status route is protected
fd paymentRoutes.js --exec cat -n {}Repository: Deen-Bridge/dnb-backend
Length of output: 1219
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the auth middleware and the route mount chain
printf '\n### authMiddleware.js\n'
fd authMiddleware.js --exec sh -c 'cat -n "$1"' sh {}
printf '\n### routes mounting references\n'
rg -n "paymentRoutes|/stellar|sponsorship/status|router\.use\(protect\)|admin" src --glob '!**/node_modules/**'Repository: Deen-Bridge/dnb-backend
Length of output: 5859
Restrict /sponsorship/status to admins
router.use(protect) only authenticates users; it doesn’t add any role check, so any logged-in user can read the sponsor account public key, XLM balance, and daily spend. Add an admin guard before exposing this treasury 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 730 - 742,
Restrict getSponsorshipStatus to administrators by applying the existing admin
authorization guard in the route configuration before this handler runs. Keep
protect for authentication, ensure non-admin users are rejected, and preserve
the handler’s current response behavior for authorized requests.
Source: Path instructions
1 participant
Summary
Testing
test/feeSponsor.test.js: 11 passedCloses #30
Summary by CodeRabbit
feeCharged.