Feat/Deposit Account Pool Service (Stellar Keypair Management for Deposit-Based Payments) - #37
Conversation
|
Warning Review limit reached
Next review available in: 11 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds encrypted Stellar deposit-account persistence and a class-based service for creating, listing, balancing, assigning, and releasing accounts. Updates Prisma schema and migration, environment configuration, encryption utilities, TypeScript/ESLint settings, and unit tests. ChangesDeposit account pool
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Caller
participant DepositAccountService
participant Prisma
participant Horizon
Caller->>DepositAccountService: assignAccount(invoiceId)
DepositAccountService->>Prisma: select unused account
Prisma-->>DepositAccountService: candidate accounts
DepositAccountService->>Prisma: conditionally claim candidate
Prisma-->>DepositAccountService: update count
DepositAccountService-->>Caller: sanitized account
Caller->>DepositAccountService: getAccountBalance(address)
DepositAccountService->>Horizon: loadAccount(address)
Horizon-->>DepositAccountService: balances or not-found
DepositAccountService-->>Caller: balances or empty list
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 1
🧹 Nitpick comments (7)
tests/unit/encryption.test.ts (1)
51-51: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAvoid a hardcoded key-shaped literal as test plaintext.
This string is shaped like a real Stellar secret key, which is why the secret scanner (Betterleaks) flags it. Generate it dynamically instead (as already done for
validKeyHexon line 5) to avoid CI secret-scan noise and remove any ambiguity about whether it's a real credential.🔧 Suggested fix
- const secret = 'SDORW56POGIXY3NZS24EPRP36Y4QUTYJ2E2MVRKVKZ27TXL5N7227W4G'; + const secret = `S${crypto.randomBytes(28).toString('hex').toUpperCase()}`;🤖 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 `@tests/unit/encryption.test.ts` at line 51, Replace the hardcoded key-shaped plaintext assigned to secret with a dynamically generated non-secret value, following the existing validKeyHex generation pattern at the top of the test file; keep the test’s plaintext semantics unchanged while ensuring secret scanners cannot interpret it as a real credential.Source: Linters/SAST tools
prisma/schema.prisma (1)
248-261: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAdd an index to back the
assignAccountquery pattern.
assignAccount()(in the dependent service layer) querieswhere: { inUse: false }ordered bycreatedAton every assignment attempt. Without a matching index this becomes a full table scan as the account pool grows.♻️ Suggested index
model DepositAccount { id String `@id` `@default`(uuid()) address String `@unique` encryptedSecret String invoiceId String? `@unique` inUse Boolean `@default`(false) lastUsedAt DateTime? createdAt DateTime `@default`(now()) updatedAt DateTime `@updatedAt` invoice Invoice? `@relation`(fields: [invoiceId], references: [id]) + + @@index([inUse, createdAt]) }🤖 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 `@prisma/schema.prisma` around lines 248 - 261, Add a composite index on DepositAccount covering inUse and createdAt to support the assignAccount query’s filtering and ordering. Keep the existing fields, constraints, and relation unchanged.src/utils/encryption.ts (2)
1-47: 🔒 Security & Privacy | 🔵 TrivialNo key-rotation strategy for
DEPOSIT_ACCOUNT_ENCRYPTION_KEY.Rotating this key would make all previously-encrypted
DepositAccount.encryptedSecretvalues undecryptable, sincedecrypt()only ever tries the current key. Worth considering a key-version prefix in the ciphertext (or a small keyring) before this reaches production scale, given it protects live Stellar secret keys.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/encryption.ts` around lines 1 - 47, Update getEncryptionKey, encrypt, and decrypt to support key rotation through a small keyring or versioned ciphertext format. Ensure newly encrypted values identify the active key version, while decrypt selects the corresponding current or legacy key so existing DepositAccount.encryptedSecret values remain readable during rotation. Preserve authentication and reject unknown or unavailable key versions clearly.
7-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReading
process.envdirectly bypasses theenvironmentconfig module.
environment.depositAccountEncryptionKeyalready centralizes this value; readingprocess.env.DEPOSIT_ACCOUNT_ENCRYPTION_KEYagain here (rather than relying solely onenvironment) duplicates parsing logic and creates two sources of truth that can drift if the config module's handling changes later. This appears aimed at letting tests mutateprocess.envafter theenvironmentmodule snapshot was taken — consider instead resetting Jest's module registry (jest.resetModules()) between tests so the config module remains the single source of truth.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/encryption.ts` around lines 7 - 19, Update getEncryptionKey to use only environment.depositAccountEncryptionKey and remove the direct process.env.DEPOSIT_ACCOUNT_ENCRYPTION_KEY fallback, preserving the existing validation and Buffer conversion. If tests need to change the configured key between cases, reset the Jest module registry and reload the environment configuration rather than mutating around its snapshot.tests/unit/deposit-account.service.test.ts (1)
167-242: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMissing test for the P2002 (409) conflict path in
assignAccount.The success/race-retry/empty-pool cases are covered, but the unique-constraint conflict branch (service.ts lines 111-117, returning
AppError(409, 'Invoice already has an assigned deposit account')) has no corresponding test — it's a concurrency-safety guarantee called out in the PR objectives.🧪 Suggested test addition
it('throws 409 when invoiceId already has an assigned deposit account (unique constraint)', async () => { const candidate = { id: 'acc-1', address: 'GAAA1', inUse: false }; prismaMock.depositAccount.findMany.mockResolvedValue([candidate]); prismaMock.depositAccount.updateMany.mockRejectedValue({ code: 'P2002' }); await expect(service.assignAccount('inv-dup')).rejects.toMatchObject({ statusCode: 409, message: 'Invoice already has an assigned deposit account', }); });Want me to add this test?
🤖 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 `@tests/unit/deposit-account.service.test.ts` around lines 167 - 242, Add a test in the assignAccount suite covering the unique-constraint conflict path: mock findMany with an available candidate, make updateMany reject with a Prisma P2002 error, and assert assignAccount rejects with statusCode 409 and message “Invoice already has an assigned deposit account.”eslint.config.cjs (1)
30-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBlanket-ignoring
tests/**removes lint coverage for all test code.Excluding the whole directory means unused vars, unsafe
any/type issues, etc. in test files will never be caught going forward. If the motivation is conflicts with patterns like top-levelas anycasts or dynamic ESM imports in test files, a scoped override (relaxing only the offending rules fortests/**) preserves more of the safety net than a full ignore.🤖 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 `@eslint.config.cjs` at line 30, Remove the blanket tests/** entry from the ignore configuration and add a scoped ESLint override for tests/** that relaxes only the specific rules causing test-file conflicts, such as top-level as any casts or dynamic ESM imports, while retaining the remaining lint and type-safety checks.src/services/deposit-account.service.ts (1)
78-120: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid the extra
findUniqueround trip on the success path.After a winning
updateMany(result.count === 1), the new row is fully derivable fromcandidateplus the values just written — no other field is mutated by this service, so re-querying the DB is an avoidable round trip on the account-assignment hot path.♻️ Proposed simplification
if (result.count === 1) { - const updated = await prisma.depositAccount.findUnique({ - where: { id: candidate.id }, - }); - return sanitizeDepositAccount(updated!); + return sanitizeDepositAccount({ + ...candidate, + inUse: true, + invoiceId, + lastUsedAt: now, + }); }Note: this also removes the
updated!non-null assertion, which currently has no fallback if the row were ever missing.🤖 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/deposit-account.service.ts` around lines 78 - 120, In assignAccount, remove the successful-path findUnique call after updateMany returns count 1. Construct the sanitized result directly from candidate, applying the assigned invoiceId, inUse, and lastUsedAt values written by the update, and eliminate the updated non-null assertion while preserving the existing conflict and retry behavior.
🤖 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 `@prisma/schema.prisma`:
- Line 259: The DepositAccount.invoice relation currently allows invoice
deletion to null invoiceId without releasing the account. In
prisma/schema.prisma:259, add an explicit restrictive onDelete strategy to the
invoice relation, then regenerate the corresponding foreign-key constraint in
prisma/migrations/20260728064714_add_deposit_account/migration.sql:40-41 so the
database matches the Prisma schema.
---
Nitpick comments:
In `@eslint.config.cjs`:
- Line 30: Remove the blanket tests/** entry from the ignore configuration and
add a scoped ESLint override for tests/** that relaxes only the specific rules
causing test-file conflicts, such as top-level as any casts or dynamic ESM
imports, while retaining the remaining lint and type-safety checks.
In `@prisma/schema.prisma`:
- Around line 248-261: Add a composite index on DepositAccount covering inUse
and createdAt to support the assignAccount query’s filtering and ordering. Keep
the existing fields, constraints, and relation unchanged.
In `@src/services/deposit-account.service.ts`:
- Around line 78-120: In assignAccount, remove the successful-path findUnique
call after updateMany returns count 1. Construct the sanitized result directly
from candidate, applying the assigned invoiceId, inUse, and lastUsedAt values
written by the update, and eliminate the updated non-null assertion while
preserving the existing conflict and retry behavior.
In `@src/utils/encryption.ts`:
- Around line 1-47: Update getEncryptionKey, encrypt, and decrypt to support key
rotation through a small keyring or versioned ciphertext format. Ensure newly
encrypted values identify the active key version, while decrypt selects the
corresponding current or legacy key so existing DepositAccount.encryptedSecret
values remain readable during rotation. Preserve authentication and reject
unknown or unavailable key versions clearly.
- Around line 7-19: Update getEncryptionKey to use only
environment.depositAccountEncryptionKey and remove the direct
process.env.DEPOSIT_ACCOUNT_ENCRYPTION_KEY fallback, preserving the existing
validation and Buffer conversion. If tests need to change the configured key
between cases, reset the Jest module registry and reload the environment
configuration rather than mutating around its snapshot.
In `@tests/unit/deposit-account.service.test.ts`:
- Around line 167-242: Add a test in the assignAccount suite covering the
unique-constraint conflict path: mock findMany with an available candidate, make
updateMany reject with a Prisma P2002 error, and assert assignAccount rejects
with statusCode 409 and message “Invoice already has an assigned deposit
account.”
In `@tests/unit/encryption.test.ts`:
- Line 51: Replace the hardcoded key-shaped plaintext assigned to secret with a
dynamically generated non-secret value, following the existing validKeyHex
generation pattern at the top of the test file; keep the test’s plaintext
semantics unchanged while ensuring secret scanners cannot interpret it as a real
credential.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1436cfd3-87e8-4fae-a005-3ba82a722a53
📒 Files selected for processing (10)
.env.exampleeslint.config.cjsprisma/migrations/20260728064714_add_deposit_account/migration.sqlprisma/schema.prismasrc/config/environment.tssrc/services/deposit-account.service.tssrc/utils/encryption.tstests/unit/deposit-account.service.test.tstests/unit/encryption.test.tstsconfig.json
|
@codebestia I've resolve the minor data integrity & behavior concern, pls check it out, thanks. |
codebestia
left a comment
codebestia
left a comment
There was a problem hiding this comment.
LGTM!
Nice Implementation.
Thank you for your contribution.
2 participants
Deposit Account Pool Service Summary
The Deposit Account Pool Service manages a pool of Stellar deposit keypairs for deposit-based invoice payments.
Key Changes Made
1. Encryption Utility
AES-256-GCM.DEPOSIT_ACCOUNT_ENCRYPTION_KEYenv variable (32-byte hex-encoded string). Enforces strict length checking and fails fast if missing or invalid.encrypt(plaintext)anddecrypt(ciphertext)using${ivHex}:${authTagHex}:${ciphertextHex}format.2. Environment Configuration
STELLAR_HORIZON_URLandDEPOSIT_ACCOUNT_ENCRYPTION_KEYto .env.example.stellarHorizonUrlanddepositAccountEncryptionKeyto environment.ts.3. Database Schema Migration
DepositAccountmodel in schema.prisma:id: UUID Primary Keyaddress: Unique Stellar public key (G...)encryptedSecret: AES-256-GCM ciphertextinvoiceId: Unique nullable FK toInvoiceinUse: Boolean flag (default false)lastUsedAt: Nullable DateTimecreatedAt&updatedAt: TimestampsInvoicemodel with relation fielddepositAccount DepositAccount?.20260728064714_add_deposit_account.4. Deposit Account Service
Horizon.Server:createAccount(): GeneratesKeypair.random(), encrypts secret seed, persists record, returns secret-free summary. No on-chain transactions submitted.getAllAccounts(): Returns all deposit account summaries. Secrets are never exposed.getAccountBalance(address): Querieshorizon.loadAccount(address). Gracefully returns empty array[]when account is 404 (not yet funded/created on-chain).getAvailableAccounts(): Returns deposit accounts whereinUseisfalse.assignAccount(invoiceId): Safe concurrent account allocation using conditionalupdateMany({ where: { id: candidateId, inUse: false } })with retry loop.releaseAccount(accountId): ClearsinvoiceIdtonulland resetsinUsetofalse, allowing re-assignment to future invoices.5. Automated Tests
Verification Results
Automated Tests
deposit-account.service.test.ts: Passed 11/11 tests.encryption.test.ts: Passed 7/7 tests.Type Check & Formatting
npx tsc --noEmit: Executed cleanly with 0 type errors.npm run format: Prettier formatting applied to all files.Closes #27
Summary by CodeRabbit
New Features
Configuration
Tests