feat(payments): add double-spend validation for escrow funding requests#234
feat(payments): add double-spend validation for escrow funding requests#234MorsH14 wants to merge 3 commits into
Conversation
Implements a Redis distributed lock (SET NX PX) around the /escrow/deposit route to prevent race conditions from funding the same order twice. A Lua script ensures only the token owner can release the lock atomically. - Add EscrowFundingLock interface to validation.ts - New lock.ts: acquireLock / releaseLock with LockServiceError for Redis failures - Deposit route returns 409 ESCROW_FUNDING_CONFLICT for in-flight duplicates and 503 LOCK_SERVICE_UNAVAILABLE when Redis is unreachable - Lock auto-expires via configurable ESCROW_FUNDING_LOCK_TTL_MS (default 30s) - Add ioredis + ioredis-mock dependencies to payments service - 6 unit tests covering concurrent blocking, clean release, wrong-token guard, and TTL auto-expiry Closes DelegoLabs#51 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Organization UI 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:
📝 WalkthroughWalkthroughPayments now uses a Redis-backed escrow funding lock for deposit requests, adds lock acquisition/release helpers and related configuration docs, and includes unit tests covering duplicate requests, release behavior, and TTL expiry. ChangesEscrow Funding Lock
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@MorsH14 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
![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
🤖 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 `@apps/backend/payments/src/lock.ts`:
- Around line 68-74: The lock in getLockRedisClient().set is using a one-shot
TTL that can expire while the deposit flow is still running, allowing concurrent
reacquisition of the same key. Update the locking flow in this section of escrow
funding lock handling to either renew the lease for the duration of the
protected operation or enforce a hard upper bound on the total deposit time that
is guaranteed to stay within the TTL. Make sure the fix is applied around the
lock acquisition and release logic in the lock.ts path that uses ttlMs,
lockToken, and result.
- Around line 2-4: Avoid the top-level import of ioredis-mock in the lock
module, since routes.ts can load it in production and trigger resolution of a
dev-only package before useMock is checked. Update the lock-related setup so
MockRedis is loaded lazily only inside the mock branch (or otherwise made
available at runtime), keeping Redis usage for the real path unchanged and
referencing the existing lock.ts imports and mock-creation path to locate the
fix.
In `@apps/backend/payments/src/routes.ts`:
- Around line 108-123: The deposit flow in routes.ts skips distributed locking
when validated.value.orderId is missing, so concurrent requests can still
double-fund the same order. Update the request validation or handler so a lock
key is always available for deposits: either make orderId required in
validateDepositRequest() and use it in acquireLock(), or derive the lock key
from another required unique field before entering the lock block. Ensure the
conflict response path in the funding handler remains unchanged.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 17bf5eb2-8322-430c-bdee-18e2f4423104
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
apps/backend/payments/README.mdapps/backend/payments/package.jsonapps/backend/payments/src/lock.tsapps/backend/payments/src/routes.tsapps/backend/payments/src/validation.tstests/unit/src/escrow-funding-lock.test.js
| import { Redis } from "ioredis"; | ||
| // @ts-ignore — ioredis-mock is a dev dependency used only in test/mock mode | ||
| import MockRedis from "ioredis-mock"; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## Files\n'
git ls-files 'apps/backend/payments/src/lock.ts' 'apps/backend/payments/src/routes.ts' 'apps/backend/payments/package.json'
printf '\n## lock.ts\n'
cat -n apps/backend/payments/src/lock.ts
printf '\n## routes.ts references to lock\n'
rg -n --context 3 'from .*/lock|from "./lock"|from "\./lock"|lock\.ts|lock' apps/backend/payments/src/routes.ts apps/backend/payments/src -g '!**/node_modules/**'
printf '\n## package.json\n'
cat -n apps/backend/payments/package.json
printf '\n## dependency declarations for ioredis-mock\n'
rg -n '"ioredis-mock"|ioredis-mock' apps/backend/payments/package.json apps/backend/payments -g '!**/node_modules/**'Repository: DelegoLabs/Delego
Length of output: 18306
Avoid a top-level import of ioredis-mock. routes.ts loads this module in production, so Node resolves the mock package at startup even when useMock is false. With ioredis-mock only in devDependencies, a pruned production install will fail before serving requests. Lazy-load it inside the mock branch or move it to runtime dependencies.
🤖 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 `@apps/backend/payments/src/lock.ts` around lines 2 - 4, Avoid the top-level
import of ioredis-mock in the lock module, since routes.ts can load it in
production and trigger resolution of a dev-only package before useMock is
checked. Update the lock-related setup so MockRedis is loaded lazily only inside
the mock branch (or otherwise made available at runtime), keeping Redis usage
for the real path unchanged and referencing the existing lock.ts imports and
mock-creation path to locate the fix.
| const ttlMs = Number( | ||
| process.env.ESCROW_FUNDING_LOCK_TTL_MS ?? DEFAULT_LOCK_TTL_MS | ||
| ); | ||
|
|
||
| let result: string | null; | ||
| try { | ||
| result = await getLockRedisClient().set(key, lockToken, "PX", ttlMs, "NX"); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
A fixed TTL can reopen the critical section before deposit finishes.
This lock is acquired once and never renewed. If the protected deposit path runs longer than ESCROW_FUNDING_LOCK_TTL_MS, Redis will expire the key while the first request is still in flight, and a second request can reacquire it. To actually block duplicate in-flight funding, this needs lease renewal or a documented upper bound that is enforced for the entire operation.
🤖 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 `@apps/backend/payments/src/lock.ts` around lines 68 - 74, The lock in
getLockRedisClient().set is using a one-shot TTL that can expire while the
deposit flow is still running, allowing concurrent reacquisition of the same
key. Update the locking flow in this section of escrow funding lock handling to
either renew the lease for the duration of the protected operation or enforce a
hard upper bound on the total deposit time that is guaranteed to stay within the
TTL. Make sure the fix is applied around the lock acquisition and release logic
in the lock.ts path that uses ttlMs, lockToken, and result.
| // Acquire a distributed lock to prevent double-funding the same order. | ||
| if (validated.value.orderId) { | ||
| lock = await acquireLock(validated.value.orderId); | ||
| if (lock === null) { | ||
| json(res, 409, { | ||
| data: null, | ||
| error: { | ||
| code: "ESCROW_FUNDING_CONFLICT", | ||
| message: | ||
| "A funding request for this order is already in progress", | ||
| }, | ||
| }); | ||
| return; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep outline apps/backend/payments/src/validation.ts --view expanded
rg -n -C3 'validateDepositRequest|orderId' apps/backend/payments/src/validation.ts apps/backend/payments/src/routes.tsRepository: DelegoLabs/Delego
Length of output: 5342
🏁 Script executed:
#!/bin/bash
sed -n '1,260p' apps/backend/payments/src/validation.ts
printf '\n--- routes ---\n'
sed -n '1,220p' apps/backend/payments/src/routes.tsRepository: DelegoLabs/Delego
Length of output: 13642
🏁 Script executed:
#!/bin/bash
sed -n '1,260p' apps/backend/payments/src/validation.ts
printf '\n--- routes ---\n'
sed -n '1,220p' apps/backend/payments/src/routes.tsRepository: DelegoLabs/Delego
Length of output: 13642
🏁 Script executed:
#!/bin/bash
sed -n '1,260p' apps/backend/payments/src/validation.ts
printf '\n--- routes ---\n'
sed -n '1,220p' apps/backend/payments/src/routes.tsRepository: DelegoLabs/Delego
Length of output: 13642
Require a lock key for deposit requests
validateDepositRequest() allows orderId to be omitted, so this lock is skipped and concurrent deposits can still double-fund the same order. Make orderId required here, or derive the lock key from another required unique field.
🤖 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 `@apps/backend/payments/src/routes.ts` around lines 108 - 123, The deposit flow
in routes.ts skips distributed locking when validated.value.orderId is missing,
so concurrent requests can still double-fund the same order. Update the request
validation or handler so a lock key is always available for deposits: either
make orderId required in validateDepositRequest() and use it in acquireLock(),
or derive the lock key from another required unique field before entering the
lock block. Ensure the conflict response path in the funding handler remains
unchanged.
|
Fix the failing ci |
|
Resolve conflicts |
2 participants
Summary
SET NX PX) on the/escrow/depositroute to prevent concurrent requests from double-funding the same order (closes [Payments] Add Double-Spend Validation for Escrow Funding Requests #51)src/lock.tsmodule exportsacquireLock/releaseLockwith a Lua script for atomic token-checked deletion, andLockServiceErrorfor Redis unavailabilityEscrowFundingLockinterface added tosrc/validation.tsper implementation scopeESCROW_FUNDING_CONFLICTfor in-flight duplicate requests and 503LOCK_SERVICE_UNAVAILABLEwhen Redis is unreachableESCROW_FUNDING_LOCK_TTL_MS(default 30 s) so a crash cannot permanently block an orderioredis(prod dep) andioredis-mock(dev dep) to the payments serviceDesign decisions
orderIdpresentorderIdare unaffectedLockServiceError→ 503Test plan
node --test tests/unit/src/escrow-funding-lock.test.js— 6/6 passnode --test tests/unit/src/payments-validation.test.js— 17/17 pass (no regressions)pnpm --filter @delego/payments build— clean TypeScript compilepnpm -r typecheck— all packages passtests/integration/src/payments-escrow.testnet.test.js) requires a live Stellar testnet and deployed escrow contract — not run locally🤖 Generated with Claude Code
Summary by CodeRabbit