[REFACTOR] 에스크로 결제 모듈에 Repository·Facade 계층 도입#48
Merged
Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughEscrow-payments 모듈을 Mongoose 모델 직접 사용에서 EscrowPaymentRepository 및 UserFacade 추상화로 전면 전환하고, 서비스/프로세서/스케줄러와 테스트들을 repository/facade 기반으로 리팩토링했습니다. 변경 사항Repository 계층 및 Facade 추상화
테스트 헬퍼 리팩토링
모듈 통합
서비스 계층 마이그레이션
Processor & Scheduler 마이그레이션
테스트 스위트 업데이트
주요 리뷰 포인트
🎯 리뷰 난이도: 🎯 4 (Complex) | ⏱️ ~45분 Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
src/modules/escrow-payments/escrow-create.processor.ts (3)
209-231:⚠️ Potential issue | 🟠 Major | ⚡ Quick winpost-flight 재시도 백오프 — 마지막 시도에서 sleep 누락 등 동작은 OK, 단
markEscrowednull 의미 재확인 필요
markEscrowed가null을 반환하는 경우 "다른 경로가 상태를 변경한 동시성 충돌" 로 주석되어 있습니다. 그런데 line 220-225 에서 3회째에null이 반환되면Error("Post-flight DB update did not match")를 던지는데, 동시성 충돌이라면 이미 다른 경로(예: 복구 스케줄러)가 ESCROWED 로 만들었을 가능성이 있고 이때는 성공 처리가 맞습니다. 현재는 실패로 분류되어 Bull 이 재시도 → 결국rollbackAllEscrows까지 트리거될 수 있습니다.null반환 시 현재 상태를 한 번 더 조회하여 ESCROWED 이면 성공으로 간주하는 분기를 두는 편이 안전합니다.♻️ 제안 패치(아이디어)
result = await this.escrowPaymentRepo.markEscrowed( paymentId, escrowId, sequence, txHash, ); if (result) break; - // null 반환 = 다른 경로가 상태를 변경한 경우 (동시성 충돌) - if (attempt === 3) { + // null 반환 = 다른 경로가 상태를 변경한 경우 (동시성 충돌) + // 이미 ESCROWED 라면 성공으로 간주 + const current = await this.escrowPaymentsService.getEscrowStatus( + paymentId, escrowId, + ); + if (current.status === "ESCROWED") { + this.logger.warn( + `Post-flight saw ESCROWED via reconciliation: paymentId=${paymentId} escrowId=${escrowId}`, + ); + return; + } + if (attempt === 3) { this.logger.error( `Post-flight DB update missed for paymentId=${paymentId} escrowId=${escrowId}; recovery scheduler must reconcile via condition lookup`, ); throw new Error("Post-flight DB update did not match"); }🤖 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/modules/escrow-payments/escrow-create.processor.ts` around lines 209 - 231, The retry logic treats a final null from escrowPaymentRepo.markEscrowed(paymentId, escrowId, sequence, txHash) as an error, but null may mean another path already set the record to ESCROWED; update the last-attempt branch so that when markEscrowed returns null on the final attempt you re-query the current escrow payment state (e.g., via escrowPaymentRepo.findById or a status lookup) and if the status equals ESCROWED consider the operation successful (break/return) otherwise log the mismatch with this.logger.error and throw the original Error to trigger retry/rollback; keep the existing backoff/wait behavior for non-final attempts and preserve throwing on true error exceptions.
186-206:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win중복 로그 발견 — XRPL 제출 후의 로그가 제출 전 로그와 동일합니다
Line 186-188 에서 이미 "Submitting EscrowCreate: ..." 를 한 번 출력하고, Line 202-205 에서 같은 메시지를 또 출력합니다. 후자는 제출 성공 후의 로그여야 자연스럽습니다(예: txHash, sequence 포함). 운영 트레이스에서 "제출 시도 → 성공" 의 흐름이 흐려져 사고 시 디버깅이 어려워질 수 있어, 메시지를 사후 로그에 맞게 바꾸거나 제거 권장합니다.
📝 제안 패치
- this.logger.log( - `Submitting EscrowCreate: buyer=${buyerUser.wallet.address} ` + - `seller=${sellerUser.wallet.address} amount=${escrow.amountXrp} ${currency}`, - ); - + this.logger.log( + `EscrowCreate submitted: paymentId=${paymentId} escrowId=${escrowId} ` + + `txHash=${txHash} sequence=${sequence}`, + );🤖 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/modules/escrow-payments/escrow-create.processor.ts` around lines 186 - 206, The duplicate "Submitting EscrowCreate" log should be fixed so the post-submit message reflects success and includes transaction details: after calling xrplService.createEscrow (where txHash and sequence are set), update the second logger.log to either be removed or changed to a post-submit success message that includes txHash and sequence (and retains buyerUser.wallet.address, sellerUser.wallet.address, escrow.amountXrp, currency) so traces show "attempt" vs "submitted" clearly; ensure revertSubmitting on escrowPaymentRepo remains in the catch block unchanged.
169-180:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winpreflight 실패 시 던지는 예외의 상태값 정확성 확인
preflight가null/false인 경우, "다른 호출자가 이미 PENDING_ESCROW → SUBMITTING 으로 바꿨다" 라는 의미일 가능성이 큽니다. 그런데 예외 메시지는expected=PENDING_ESCROW, actual=${escrow.status}로 던지는데,escrow.status는 line 129 에서 읽어온 오래된 메모리 상태(여전히 PENDING_ESCROW) 입니다. 따라서 예외 메시지가 항상expected=PENDING_ESCROW, actual=PENDING_ESCROW로 찍혀 디버깅을 방해합니다. 실제 현재 상태를 다시 조회하거나 메시지를 "concurrent preflight lost" 로 명확히 바꾸는 게 좋습니다.🤖 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/modules/escrow-payments/escrow-create.processor.ts` around lines 169 - 180, The current code throws InvalidEscrowItemStatusException with escrow.status that was read earlier (stale) when this.escrowPaymentRepo.preflight(...) returns falsy; update the handler to either re-load the latest escrow state before throwing (e.g., fetch current escrow status via the escrow repository and use that value in the exception) or change the exception message to a deterministic concurrent-failure message like "concurrent preflight lost"; specifically modify the preflight failure branch that checks preFlighted, using the fresh status (or the new static message) when constructing InvalidEscrowItemStatusException instead of the previously cached escrow.status.src/modules/escrow-payments/escrow-submit-recovery.scheduler.ts (3)
134-142:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
cancelSubmittingEscrow와rollbackAllEscrows사이의 비원자성 — 부분 실패 시 좀비 상태
cancelSubmittingEscrow가 성공한 직후rollbackAllEscrows호출 전에 프로세스가 죽으면, 해당 escrow 만 CANCELLED 이고 payment 자체는 PROCESSING/ACTIVE 로 남아 다른 escrow 가 그대로 살아있는 좀비 상태가 됩니다. 다음 cron 틱에서도findStuckSubmitting만 본다면 잡히지 않고, 해당 payment 가 영영 종료되지 않을 수 있습니다. 두 작업을 하나의 repository 메서드(또는 동일 세션 트랜잭션)로 합치거나, payment 상태 기준의 별도 회수 스케줄러를 두는 것을 고려해 주세요.🤖 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/modules/escrow-payments/escrow-submit-recovery.scheduler.ts` around lines 134 - 142, The current two-step sequence (escrowPaymentRepo.cancelSubmittingEscrow(paymentId, escrowId) then escrowPaymentsService.rollbackAllEscrows(paymentId)) is non-atomic and can leave a zombie payment if the process dies between calls; combine them into one atomic operation or add a recovery path. Replace the two calls in cancelSubmittingEscrowAndRollback with a single repository-level transactional method (e.g., escrowPaymentRepo.cancelSubmittingAndRollbackAll(paymentId, escrowId)) that marks the specific escrow CANCELLED and runs the rollbackAllEscrows logic inside the same DB transaction/session, or alternatively implement a separate recovery scheduler that scans payments in PROCESSING/ACTIVE (use findStuckSubmitting semantics) and forces rollback when all SUBMITTING escrows are handled; update usages of cancelSubmittingEscrow and rollbackAllEscrows accordingly.
89-93:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
status !== "SUBMITTING"분기에서 무조건 "recovered" 반환은 로그를 오해 유발다른 경로에서 이미 CANCELLED 로 전이된 경우에도 "recovered" 를 반환합니다. 호출자(line 62-70)는 이 값을 보고 "Recovered SUBMITTING escrow" 로 로그를 남기므로 운영 중 사실과 다른 메시지가 찍힙니다. 가벼운 수정이지만 사후 부검 시 헷갈리기 쉬워 분기 보강을 권장합니다.
📝 제안 패치
- if (escrow.status !== "SUBMITTING") return "recovered"; // 이미 다른 경로로 처리됨 + if (escrow.status !== "SUBMITTING") { + // 다른 경로에서 이미 종료 상태로 전이됨 + return escrow.status === "CANCELLED" ? "cancelled" : "recovered"; + }🤖 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/modules/escrow-payments/escrow-submit-recovery.scheduler.ts` around lines 89 - 93, The code currently returns "recovered" whenever escrow.status !== "SUBMITTING", which causes misleading "Recovered SUBMITTING escrow" logs even for CANCELLED items; update the branch after calling escrowPaymentsService.getEscrowStatus(paymentId, escrowId) to explicitly check for escrow.status === "CANCELLED" and return a distinct value (e.g., "cancelled") in that case, and return a different, accurate token (e.g., "skipped" or keep "recovered" only for the intended case) for other non-SUBMITTING statuses so the caller can log the correct outcome; modify the conditional around the escrow variable (the result of getEscrowStatus) accordingly.
107-127:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
markEscrowednull 반환 시markActive평가 누락 가능성결제가 여러 에스크로를 포함할 때,
markEscrowed가 null을 반환하면 (라인 115) 즉시 반환되어 같은 payment의 다른 에스크로들이 이미 모두 ESCROWED/RELEASED/CANCELLED 상태인 경우에도markActive가 호출되지 않을 수 있습니다. 이는 동시성 문제로 인해 에스크로 상태가 변경된 후 업데이트가 실패할 때 발생합니다. 결과적으로 결제가 PROCESSING 상태에 영구히 고착될 수 있으므로, null 케이스에서도 payment를 다시 조회하여 모든 에스크로의 상태를 확인한 후 필요하면markActive를 호출하는 것이 안전합니다.🤖 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/modules/escrow-payments/escrow-submit-recovery.scheduler.ts` around lines 107 - 127, When xrplResult exists but this.escrowPaymentRepo.markEscrowed(...) returns null, you must still re-check the payment's escrow statuses and call this.escrowPaymentRepo.markActive(paymentId) if all escrows are in ESCROWED/RELEASED/CANCELLED; update the block handling xrplResult to, on null result, fetch the payment's escrows (via escrowPaymentRepo or an existing fetch method), evaluate the same allEscrowed predicate against the fresh data, and call markActive(paymentId) when appropriate before returning "recovered"; reference markEscrowed, markActive, escrowPaymentRepo, paymentId, escrowId and xrplResult to locate and modify the logic.src/modules/escrow-payments/escrow-payments.service.ts (1)
266-321:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
releaseEscrow복구 경로 부재로 인한 상태 불일치 위험XRPL 트랜잭션이 레저에 확정된 후 응답 수신 실패 시 DB가
ESCROWED로 롤백되는 구조적 문제가 있습니다. SUBMITTING 단계에는EscrowSubmitRecoveryScheduler로 reconciliation을 수행하지만, RELEASING 단계는 유사한 복구 메커니즘이 없습니다.구체적으로:
finishEscrow가submitAndWait()로 트랜잭션 확정을 기다리지만, 네트워크 오류로 응답 손실 시 DB는ESCROWED로 롤백- 재시도 시 동일 sequence로 EscrowFinish를 다시 제출하려 하면 XRPL에서
tefPAST_SEQ실패- 결과: XRPL은 funds released 상태지만 DB는
ESCROWED로 기록되어 회계 불일치SUBMITTING 복구 패턴을 참고하여 RELEASING 상태에 대한 reconciler 추가를 검토해주세요 (condition 기반으로 finishEscrow 제출 여부를 XRPL에서 확인하는 방식).
🤖 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/modules/escrow-payments/escrow-payments.service.ts` around lines 266 - 321, releaseEscrow can roll back to ESCROWED after a lost response even though XRPL may have applied the EscrowFinish; stop reverting to ESCROWED on catch and implement a RELEASING recovery flow: when finishEscrow (xrplService.finishEscrow) fails with network/timeout, leave escrow.status as "RELEASING", persist payment, enqueue a new EscrowReleaseRecoveryScheduler (analogous to EscrowSubmitRecoveryScheduler) that uses escrow.xrplSequence or escrow.condition and xrplService methods (e.g., a new xrplService.checkEscrowReleased or ledger lookup) to reconcile whether the escrow was already released and then mark EscrowItem status to "RELEASED" and set txHashRelease/releasedAt, or if not released safely retry finishEscrow only after confirming sequence/condition to avoid tefPAST_SEQ; update releaseEscrow error handling to persist the RELEASING state and schedule the reconciler instead of setting status back to "ESCROWED".
🧹 Nitpick comments (11)
src/modules/escrow-payments/__tests__/initiate.spec.ts (1)
43-46: ⚡ Quick win트랜잭션 세션 검증을 구체화하세요.
expect.any(Object)는 너무 광범위합니다. 실제로 Mongoose 세션 객체가 전달되는지 확인하려면 더 구체적인 matcher를 사용하는 것이 좋습니다.♻️ 개선 제안
expect(ctx.escrowPaymentRepo.markProcessing).toHaveBeenCalledWith( PAYMENT_ID.toString(), - expect.any(Object), + expect.objectContaining({ + // Mongoose session의 특징적인 속성 검증 + id: expect.any(String), + }), );또는 session이 중요하지 않다면 명시적으로 표현:
expect(ctx.escrowPaymentRepo.markProcessing).toHaveBeenCalledWith( PAYMENT_ID.toString(), - expect.any(Object), + expect.anything(), // session (any transaction object) );🤖 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/modules/escrow-payments/__tests__/initiate.spec.ts` around lines 43 - 46, The test currently uses expect.any(Object) when asserting ctx.escrowPaymentRepo.markProcessing was called and should be made more specific: replace expect.any(Object) with a matcher that verifies a Mongoose session (e.g., import ClientSession from 'mongoose' and use expect.any(ClientSession)) so the call to ctx.escrowPaymentRepo.markProcessing(PAYMENT_ID.toString(), ...) asserts a real session is passed; alternatively, if the session value isn't important, make the intent explicit by using expect.anything() or a clearer comment and adjust the matcher accordingly.src/modules/escrow-payments/escrow-payments-crud.service.ts (3)
130-150: 🏗️ Heavy liftRepository 추상화 누수 —
$or/$in/ObjectId가 여전히 서비스에 남아 있음이번 PR의 목적이 “DB 쿼리와 비즈니스 로직 분리”인데,
findAll은 여전히 Mongoose 전용 연산자($or,$in)와Types.ObjectId변환을 서비스 내부에서 조립하고 있습니다. 이렇게 되면
- 추후 status 그룹/필터 정책이 바뀔 때 Repository와 서비스 양쪽을 모두 손봐야 하고,
- 테스트도 결국 “mongoose 같은 필터 객체”를 그대로 검증하게 되어 PR이 노린 “쿼리 체인 mock 제거”의 효용이 반감됩니다.
도메인 의도를 인자로 받는 Repository API(예:
findPageByParticipant(userId, { status?, group?, page, limit }))로 옮기는 편을 권장합니다. 그러면 서비스는 ObjectId/연산자에 대해 알 필요가 없어집니다.♻️ 제안 방향
- const uid = new Types.ObjectId(userId); - const filter: Record<string, any> = { - $or: [{ buyerId: uid }, { sellerId: uid }], - }; - if (status) { - filter.status = status; - } else if (group === "ongoing") { - filter.status = { $in: ["PENDING_APPROVAL", "APPROVED", "PROCESSING", "ACTIVE"] }; - } else if (group === "done") { - filter.status = { $in: ["COMPLETED", "CANCELLED"] }; - } - const skip = (page - 1) * limit; - const [data, total] = await Promise.all([ - this.escrowPaymentRepo.findMany(filter, skip, limit), - this.escrowPaymentRepo.countDocuments(filter), - ]); + const { data, total } = await this.escrowPaymentRepo.findPageByParticipant( + userId, + { status, group, page, limit }, + );🤖 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/modules/escrow-payments/escrow-payments-crud.service.ts` around lines 130 - 150, The service currently constructs DB-specific filters (using Types.ObjectId, $or, $in and the local filter variable) and calls this.escrowPaymentRepo.findMany / countDocuments; move that logic into the repository by adding a domain-oriented method like escrowPaymentRepo.findPageByParticipant(userId, { status?, group?, page, limit }) which will convert userId to ObjectId, build the $or/$in clauses and return the paged result (data and total) so the service no longer references Types.ObjectId, $or, $in or the filter object; update the service code to call findPageByParticipant and remove the filter construction and the two repo calls.
60-60: ⚡ Quick win
as any캐스팅 다수 — Repository/Facade 반환 타입을 정리해 제거 권장
(seller as any)._id,(buyer as any)._id,(item: any),(doc as any).buyerId등 동일 패턴이 반복됩니다. 현재 타입 단언으로 가려진 부분에서_id나buyerId같은 필드명이 오타/리네이밍될 경우 컴파일 타임에 잡히지 않아 회귀 사고로 이어질 수 있습니다.Repository는
LeanEscrowPayment같은 명시 타입, Facade는LeanUser류 타입을 반환하도록 정리하면as any를 자연스럽게 걷어낼 수 있고, 이후 다른 서비스/스케줄러 마이그레이션(PR 스택)에서도 타입 안전성을 유지할 수 있습니다.Also applies to: 73-73, 153-153, 161-166
🤖 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/modules/escrow-payments/escrow-payments-crud.service.ts` at line 60, Multiple occurrences cast values to `any` (e.g., `(seller as any)._id`, `(buyer as any)._id`, `(item: any)`, `(doc as any).buyerId`) which hides missing/renamed fields; update the repository/facade return types and local variable types instead of using `as any`: define/consume explicit lean types like `LeanUser` for `seller`/`buyer` and `LeanEscrowPayment` (or similar) for `doc`/`item`, change the repository/facade method signatures to return those types, and then remove all `as any` casts (affecting symbols in this file such as sellerId assignment, buyerId usages, item accessors and the methods that fetch `seller`/`buyer`/`doc`); ensure the `. _id` and `buyerId` properties are present on the new interfaces so the compiler enforces correctness.
91-119: ⚡ Quick winRepository 추상화 설계 —
.toObject()호출로 인한 Mongoose 메서드 직접 노출Repository 패턴의 목표는 서비스 계층이 Mongoose 문서 API(
toObject()등)에 의존하지 않도록 격리하는 것입니다. 현재 코드는escrowPaymentRepo.create()가 MongooseDocument를 반환한 후 서비스에서 곧바로.toObject()를 호출하고 있어 이 원칙이 깨져 있습니다.개선 방향: Repository의
create()메서드가 일관되게 POJO(또는 매핑된 DTO)를 반환하도록 개선하면, 서비스에서.toObject()호출을 제거할 수 있습니다. 현재findByIdLean(),findMany()메서드들은 이미 POJO를 반환하는데,create()만 Document를 반환하고 있어 불일치합니다.♻️ 예시 — Repository 메서드 일관성 확보
Repository의
create()메서드를 다음과 같이 수정:async create(data: Record<string, any>): Promise<EscrowPayment> { const doc = await new this.model(data).save(); return doc.toObject(); // Repository 내부에서만 변환 }그러면 서비스는:
const saved = await this.escrowPaymentRepo.create({...}); return this.mapToResponse(saved, userId); // .toObject() 제거🤖 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/modules/escrow-payments/escrow-payments-crud.service.ts` around lines 91 - 119, The service currently calls .toObject() after awaiting escrowPaymentRepo.create(...), leaking Mongoose Document behavior into the service; update the repository so create(...) returns a POJO (like findByIdLean() and findMany() do) by converting the saved document to a plain object inside the repository implementation, then remove the .toObject() call in the service (so mapToResponse(saved, userId) receives a POJO). Ensure escrowPaymentRepo.create keeps the same signature but returns an EscrowPayment POJO and adjust any callers accordingly.src/modules/escrow-payments/escrow-payments.module.ts (1)
33-34: 💤 Low valueRepository와 Facade의 exports 누락으로 재사용 불가
EscrowPaymentRepository와UserFacade를 providers에만 등록하고 exports하지 않으면, 다른 모듈(예: admin, analytics)에서 에스크로 데이터 접근이 필요할 때 동일한 추상화를 재사용할 수 없습니다.현재 escrow 모듈 내부에서만 사용한다면 문제없지만, 향후 확장성을 고려하면:
📦 재사용 가능성 개선안
], + exports: [EscrowPaymentRepository, UserFacade], }) export class EscrowPaymentsModule {}🤖 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/modules/escrow-payments/escrow-payments.module.ts` around lines 33 - 34, The module currently registers EscrowPaymentRepository and UserFacade only in providers but does not export them, preventing reuse by other modules; update the EscrowPaymentsModule to add EscrowPaymentRepository and UserFacade to the module's exports array (alongside existing providers like EscrowPaymentsService/EscrowPaymentsController if needed) so other modules can import and reuse these abstractions.src/modules/escrow-payments/__tests__/helpers.ts (1)
107-112: ⚡ Quick winClientSession mock의 불완전한 API 커버리지
실제 Mongoose ClientSession은
abortTransaction,commitTransaction,inTransaction등의 메서드도 제공하지만, 현재 mock은withTransaction,endSession만 구현되어 있습니다. 트랜잭션 롤백 시나리오를 테스트할 수 없습니다.🧪 테스트 커버리지 개선안
const session = { withTransaction: jest .fn() .mockImplementation((fn: () => Promise<void>) => fn()), endSession: jest.fn().mockResolvedValue(undefined), + abortTransaction: jest.fn().mockResolvedValue(undefined), + commitTransaction: jest.fn().mockResolvedValue(undefined), + inTransaction: jest.fn().mockReturnValue(false), };🤖 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/modules/escrow-payments/__tests__/helpers.ts` around lines 107 - 112, The ClientSession mock used in tests only implements withTransaction and endSession, missing other Mongoose ClientSession methods (abortTransaction, commitTransaction, inTransaction) which prevents testing rollback scenarios; update the mock `session` object (the same one providing `withTransaction` and `endSession`) to include `abortTransaction`, `commitTransaction`, and an `inTransaction` boolean (or getter) and adjust `withTransaction`'s mockImplementation to set `inTransaction` true before invoking the passed function, set it false after success and call `commitTransaction`, and on rejection call `abortTransaction` and reset `inTransaction`, so tests can assert rollback/commit behavior.src/modules/escrow-payments/repositories/user.facade.ts (1)
25-36: ⚡ Quick winprojection 필드 선택이 lean() 결과에 미치는 영향
{ _id: 1, name: 1, wallet: 1 }projection은 명시된 필드만 반환하지만,wallet객체 전체를 포함하므로wallet.seed같은 민감한 하위 필드도 포함될 수 있습니다. Facade의 목적상 필요한 필드만 명시하는 것이 안전합니다.🔒 보안 강화 제안
return this.model .findOne( { "wallet.address": address, type }, - { _id: 1, name: 1, wallet: 1 }, + { _id: 1, name: 1, "wallet.address": 1, "wallet.publicKey": 1 }, ) .lean() .exec();필요한 wallet 하위 필드만 명시적으로 선택.
🤖 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/modules/escrow-payments/repositories/user.facade.ts` around lines 25 - 36, The findByWalletAddressAndType method currently projects the entire wallet object which will include sensitive subfields (e.g., wallet.seed); update the projection in that method to only include the specific wallet subfields you actually need (for example "wallet.address" and any other non-sensitive fields) instead of "wallet", so change the projection object used in this.model.findOne(...) to explicitly list allowed wallet fields (e.g., { _id: 1, name: 1, "wallet.address": 1 }) and remove any sensitive keys.src/modules/escrow-payments/repositories/escrow-payment.repository.ts (1)
142-170: ⚡ Quick winmarkEscrowed의 중복 필드 업데이트 검증 부재
동일한 escrow에 대해
markEscrowed가 여러 번 호출되면xrplSequence,txHashCreate등이 덮어써질 수 있습니다. XRPL 블록체인 메타데이터는 불변이어야 하는데 검증 로직이 없습니다.🔒 멱등성 보장 개선안
escrows: { $elemMatch: { _id: new Types.ObjectId(escrowId), status: "SUBMITTING", + xrplSequence: { $exists: false }, // 이미 설정된 경우 업데이트 방지 }, },또는 호출자에서 멱등성 보장 후 호출하도록 문서화.
🤖 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/modules/escrow-payments/repositories/escrow-payment.repository.ts` around lines 142 - 170, markEscrowed가 동일한 escrow에 대해 여러번 호출되면 xrplSequence/txHashCreate가 덮어써질 수 있으니 업데이트 조건에 기존 메타데이터가 비어 있는 경우에만 실행되도록 쿼리를 강화하세요: 현재 markEscrowed 함수의 findOneAndUpdate 조회조건(escrows.$elemMatch) 에 escrows 항목이 xrplSequence 또는 txHashCreate가 존재하지 않거나 null인 조건(예: _id: new Types.ObjectId(escrowId), status: "SUBMITTING", xrplSequence: { $exists: false } 또는 xrplSequence: null, txHashCreate: { $exists: false } 등)을 추가해서 이미 설정된 경우 업데이트가 실패해 null을 반환하도록 하고, 필요한 경우 호출자에게 멱등성 실패를 명확히 전달하도록 처리하세요.src/modules/escrow-payments/escrow-payments.service.ts (1)
144-174: 💤 Low value트랜잭션 흐름 자체는 LGTM, 단
initiated!non-null 단언은 명시적 가드 권장
withTransaction콜백이 성공해야만initiated가 설정되고, 실패 시catch에서 던지므로return initiated!는 런타임상 안전합니다. 다만 미래의 리팩토링이 콜백 내return흐름을 바꾸면 silent NPE 로 이어질 수 있으니, 트랜잭션 직후 한 줄 가드를 두는 편이 안전합니다.♻️ 제안 패치
} finally { await session.endSession(); } + if (!initiated) throw new PaymentInitiationFailedException(); this.logger.log( `Payment ${paymentId} initiated — ${escrowIds.length} escrow(s) queued`, ); - return initiated!; + return initiated;🤖 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/modules/escrow-payments/escrow-payments.service.ts` around lines 144 - 174, The code relies on the transaction callback to always set initiated, but the final `return initiated!` uses a non-null assertion that could become unsafe after future refactors; after the `session.withTransaction(...)` completes and before returning, add an explicit guard like `if (!initiated) throw new PaymentInitiationFailedException()` (or rethrow `PaymentNotApprovedForPayException` as appropriate) so that `initiated` is validated; update the block around `initiated`, `escrowPaymentRepo.markProcessing`, and `withTransaction` to perform this null-check and throw a clear exception instead of relying on `!`.src/modules/escrow-payments/escrow-cancel.scheduler.ts (2)
41-47: 💤 Low value복호화된 buyer seed 가 함수 스코프 전체에 남아있음
decryptedSeed와buyerWallet.seed(평문) 가 escrow 루프 전체 기간 동안 메모리에 유지됩니다. CANCELLING escrow 가 여러 건이면 그만큼 평문 노출 구간이 길어지고, 예외가 발생해 catch 까지 도달하면 더 오래 살아남습니다. 사용 후 명시적으로buyerWallet.seed = ""로 초기화하거나, 가능하면 escrow 단위로 seed 를 다시 가져와 즉시 폐기하는 방식이 해외 B2B 결제 보안 관점에서 권장됩니다.🤖 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/modules/escrow-payments/escrow-cancel.scheduler.ts` around lines 41 - 47, The decrypted buyer seed (decryptedSeed / buyerWallet.seed) is kept in memory for the entire escrow loop; fetch and decrypt the seed only inside the per-escrow processing block and ensure it is zeroed out immediately after use. Constrain xrplService.decrypt(...) to the smallest scope (inside the loop handling each escrow), use the resulting buyerWallet only for the transaction, and in a finally block set buyerWallet.seed = "" and overwrite/clear decryptedSeed (or avoid storing it in a separate variable) so plaintext seed does not persist across iterations or until catch handling.
49-69: 🏗️ Heavy lift스케줄러의 전체 문서 저장으로 인한 동시성 위험
루프 내
escrowPaymentRepo.save(payment)호출이 메모리상 전체 문서를 덮어씁니다. 같은 payment에 대해EscrowCreateProcessor.markEscrowed()또는EscrowPaymentsService.releaseEscrow()등이 동시 실행되면, 이 스케줄러가 가진 stale 상태로 다른 escrow 항목들이 되돌려질 수 있습니다.Repository 레이어는 이미 상태 전이마다
findOneAndUpdate()로 부분 업데이트하는 패턴을 따르고 있습니다 (markEscrowed,cancelSubmittingEscrow,preflight등). 일관성을 위해 아래와 같은 메서드를 Repository에 추가하고 스케줄러에서 사용하기를 권장합니다:async markCancelled( paymentId: string, escrowId: string, ): Promise<void> { await this.model .findOneAndUpdate( { _id: paymentId, escrows: { $elemMatch: { _id: new Types.ObjectId(escrowId), status: "CANCELLING", }, }, }, { $set: { "escrows.$.status": "CANCELLED" } }, ) .exec(); }그 후 스케줄러 59줄의
save(payment)대신 위 메서드를 호출하면, 동시 접근 시에도 데이터 일관성이 보장됩니다.🤖 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/modules/escrow-payments/escrow-cancel.scheduler.ts` around lines 49 - 69, The scheduler currently calls escrowPaymentRepo.save(payment) inside the loop (see escrowPaymentRepo.save(payment), payment._id, escrow.xrplSequence), which can overwrite the whole payment document and cause race conditions with concurrent operations like EscrowCreateProcessor.markEscrowed() or EscrowPaymentsService.releaseEscrow(); add a repository method markCancelled(paymentId: string, escrowId: string) that performs a targeted findOneAndUpdate with an $elemMatch on escrows._id and status "CANCELLING" and sets escrows.$.status to "CANCELLED", then replace the save(payment) call in the loop with a call to escrowPaymentRepo.markCancelled(payment._id.toString(), escrow._id.toString()) so only the single escrow element is updated atomically.
🤖 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/modules/escrow-payments/__tests__/helpers.ts`:
- Line 124: The startSession mock must match the repository signature: make
startSession return a Promise like Mongoose 9.x does by replacing the
synchronous mock with an async-resolving mock (use
jest.fn().mockResolvedValue(session) for the startSession mock so it returns a
Promise that resolves to session), or if the repo intentionally kept a
synchronous signature then change the mock to
jest.fn().mockReturnValue(session); update the startSession mock in the test
helpers to use the correct form to match the repository implementation.
In `@src/modules/escrow-payments/escrow-payments-crud.service.ts`:
- Around line 177-181: The method findUserByWalletAddress currently forces a
UserDocument via "as UserDocument" despite UserFacade.findByWalletAddress
returning a lean POJO (User | null), causing a type and runtime mismatch; fix by
removing the "as UserDocument" assertion and make the method's return type match
the facade (e.g., change Promise<UserDocument> to Promise<User> or
Promise<LeanDocument<User>>), or alternatively have the facade provide a
hydrated variant (e.g., add findByWalletAddressHydrated) and call that from
findUserByWalletAddress; ensure WalletUserNotFoundException remains thrown when
null is returned.
In `@src/modules/escrow-payments/escrow-payments.module.ts`:
- Line 25: 현재 escrow-payments.module.ts에서 { name: User.name, schema: UserSchema
}로 User 스키마를 직접 등록해 Mongoose discriminator(UserBuyer/UserSeller) 충돌 및 레지스트리 불일치
위험이 있습니다; UserFacade.findByWalletAddressAndType()가 type 필드(디스크리미네이터)에 의존하므로
해결하려면 escrow-payments.module.ts의 imports에서 UsersModule을 추가하고 User 스키마 직접 등록 항목을
제거하여 기존 UsersModule의 discriminator 설정을 재사용하거나, 만약 직접 등록이 불가피하면 UsersModule과 동일한
discriminator 설정(UserBuyer/UserSeller)을 동기화하도록 변경하세요 (참조 심볼: User.name,
UserSchema, UsersModule, UserFacade.findByWalletAddressAndType, EscrowPayment,
EscrowPaymentSchema).
In `@src/modules/escrow-payments/repositories/escrow-payment.repository.ts`:
- Around line 100-120: The preflight method currently performs an atomic
findOneAndUpdate via this.model.findOneAndUpdate(...).exec() but returns null
silently when the update fails (e.g., escrow not in PENDING_ESCROW), which masks
concurrency/state errors; update the repository method preflight to check the
result of the exec() call and, if null, throw a descriptive Error (or a custom
error class) that includes the paymentId and escrowId and a short reason (e.g.,
"preflight failed: escrow not in PENDING_ESCROW or already submitting") so
callers (e.g., escrow-create.processor) can catch/handle it instead of
proceeding on a silent null.
- Around line 94-121: The preflight method's findOneAndUpdate call returns the
pre-update document because it lacks the new: true option; update the preflight
function to pass { new: true } (consistent with markEscrowed and markProcessing)
into the findOneAndUpdate options so it returns the updated
EscrowPaymentDocument after setting escrows.$ fields and submittingAt.
- Around line 58-66: Add a compound index on the nested escrows fields to avoid
collection scans in findStuckSubmitting; in the schema that defines
EscrowPaymentSchema (the EscrowPayment schema creation), add an index for
{"escrows.status": 1, "escrows.submittingAt": 1} (i.e., call
EscrowPaymentSchema.index for escrows.status and escrows.submittingAt) after the
EscrowPaymentSchema is created so the findStuckSubmitting method (which queries
escrows.$elemMatch with status "SUBMITTING" and submittingAt <$lt cutoff) can
use the index.
- Around line 68-70: The startSession method lacks an explicit return type;
update the signature of startSession to return Promise<ClientSession> and ensure
you import ClientSession from mongoose, e.g., change startSession() { return
this.model.db.startSession(); } to a signature with the explicit return type (no
other code change needed) so the method is type-safe when calling
this.model.db.startSession().
---
Outside diff comments:
In `@src/modules/escrow-payments/escrow-create.processor.ts`:
- Around line 209-231: The retry logic treats a final null from
escrowPaymentRepo.markEscrowed(paymentId, escrowId, sequence, txHash) as an
error, but null may mean another path already set the record to ESCROWED; update
the last-attempt branch so that when markEscrowed returns null on the final
attempt you re-query the current escrow payment state (e.g., via
escrowPaymentRepo.findById or a status lookup) and if the status equals ESCROWED
consider the operation successful (break/return) otherwise log the mismatch with
this.logger.error and throw the original Error to trigger retry/rollback; keep
the existing backoff/wait behavior for non-final attempts and preserve throwing
on true error exceptions.
- Around line 186-206: The duplicate "Submitting EscrowCreate" log should be
fixed so the post-submit message reflects success and includes transaction
details: after calling xrplService.createEscrow (where txHash and sequence are
set), update the second logger.log to either be removed or changed to a
post-submit success message that includes txHash and sequence (and retains
buyerUser.wallet.address, sellerUser.wallet.address, escrow.amountXrp, currency)
so traces show "attempt" vs "submitted" clearly; ensure revertSubmitting on
escrowPaymentRepo remains in the catch block unchanged.
- Around line 169-180: The current code throws InvalidEscrowItemStatusException
with escrow.status that was read earlier (stale) when
this.escrowPaymentRepo.preflight(...) returns falsy; update the handler to
either re-load the latest escrow state before throwing (e.g., fetch current
escrow status via the escrow repository and use that value in the exception) or
change the exception message to a deterministic concurrent-failure message like
"concurrent preflight lost"; specifically modify the preflight failure branch
that checks preFlighted, using the fresh status (or the new static message) when
constructing InvalidEscrowItemStatusException instead of the previously cached
escrow.status.
In `@src/modules/escrow-payments/escrow-payments.service.ts`:
- Around line 266-321: releaseEscrow can roll back to ESCROWED after a lost
response even though XRPL may have applied the EscrowFinish; stop reverting to
ESCROWED on catch and implement a RELEASING recovery flow: when finishEscrow
(xrplService.finishEscrow) fails with network/timeout, leave escrow.status as
"RELEASING", persist payment, enqueue a new EscrowReleaseRecoveryScheduler
(analogous to EscrowSubmitRecoveryScheduler) that uses escrow.xrplSequence or
escrow.condition and xrplService methods (e.g., a new
xrplService.checkEscrowReleased or ledger lookup) to reconcile whether the
escrow was already released and then mark EscrowItem status to "RELEASED" and
set txHashRelease/releasedAt, or if not released safely retry finishEscrow only
after confirming sequence/condition to avoid tefPAST_SEQ; update releaseEscrow
error handling to persist the RELEASING state and schedule the reconciler
instead of setting status back to "ESCROWED".
In `@src/modules/escrow-payments/escrow-submit-recovery.scheduler.ts`:
- Around line 134-142: The current two-step sequence
(escrowPaymentRepo.cancelSubmittingEscrow(paymentId, escrowId) then
escrowPaymentsService.rollbackAllEscrows(paymentId)) is non-atomic and can leave
a zombie payment if the process dies between calls; combine them into one atomic
operation or add a recovery path. Replace the two calls in
cancelSubmittingEscrowAndRollback with a single repository-level transactional
method (e.g., escrowPaymentRepo.cancelSubmittingAndRollbackAll(paymentId,
escrowId)) that marks the specific escrow CANCELLED and runs the
rollbackAllEscrows logic inside the same DB transaction/session, or
alternatively implement a separate recovery scheduler that scans payments in
PROCESSING/ACTIVE (use findStuckSubmitting semantics) and forces rollback when
all SUBMITTING escrows are handled; update usages of cancelSubmittingEscrow and
rollbackAllEscrows accordingly.
- Around line 89-93: The code currently returns "recovered" whenever
escrow.status !== "SUBMITTING", which causes misleading "Recovered SUBMITTING
escrow" logs even for CANCELLED items; update the branch after calling
escrowPaymentsService.getEscrowStatus(paymentId, escrowId) to explicitly check
for escrow.status === "CANCELLED" and return a distinct value (e.g.,
"cancelled") in that case, and return a different, accurate token (e.g.,
"skipped" or keep "recovered" only for the intended case) for other
non-SUBMITTING statuses so the caller can log the correct outcome; modify the
conditional around the escrow variable (the result of getEscrowStatus)
accordingly.
- Around line 107-127: When xrplResult exists but
this.escrowPaymentRepo.markEscrowed(...) returns null, you must still re-check
the payment's escrow statuses and call
this.escrowPaymentRepo.markActive(paymentId) if all escrows are in
ESCROWED/RELEASED/CANCELLED; update the block handling xrplResult to, on null
result, fetch the payment's escrows (via escrowPaymentRepo or an existing fetch
method), evaluate the same allEscrowed predicate against the fresh data, and
call markActive(paymentId) when appropriate before returning "recovered";
reference markEscrowed, markActive, escrowPaymentRepo, paymentId, escrowId and
xrplResult to locate and modify the logic.
---
Nitpick comments:
In `@src/modules/escrow-payments/__tests__/helpers.ts`:
- Around line 107-112: The ClientSession mock used in tests only implements
withTransaction and endSession, missing other Mongoose ClientSession methods
(abortTransaction, commitTransaction, inTransaction) which prevents testing
rollback scenarios; update the mock `session` object (the same one providing
`withTransaction` and `endSession`) to include `abortTransaction`,
`commitTransaction`, and an `inTransaction` boolean (or getter) and adjust
`withTransaction`'s mockImplementation to set `inTransaction` true before
invoking the passed function, set it false after success and call
`commitTransaction`, and on rejection call `abortTransaction` and reset
`inTransaction`, so tests can assert rollback/commit behavior.
In `@src/modules/escrow-payments/__tests__/initiate.spec.ts`:
- Around line 43-46: The test currently uses expect.any(Object) when asserting
ctx.escrowPaymentRepo.markProcessing was called and should be made more
specific: replace expect.any(Object) with a matcher that verifies a Mongoose
session (e.g., import ClientSession from 'mongoose' and use
expect.any(ClientSession)) so the call to
ctx.escrowPaymentRepo.markProcessing(PAYMENT_ID.toString(), ...) asserts a real
session is passed; alternatively, if the session value isn't important, make the
intent explicit by using expect.anything() or a clearer comment and adjust the
matcher accordingly.
In `@src/modules/escrow-payments/escrow-cancel.scheduler.ts`:
- Around line 41-47: The decrypted buyer seed (decryptedSeed / buyerWallet.seed)
is kept in memory for the entire escrow loop; fetch and decrypt the seed only
inside the per-escrow processing block and ensure it is zeroed out immediately
after use. Constrain xrplService.decrypt(...) to the smallest scope (inside the
loop handling each escrow), use the resulting buyerWallet only for the
transaction, and in a finally block set buyerWallet.seed = "" and
overwrite/clear decryptedSeed (or avoid storing it in a separate variable) so
plaintext seed does not persist across iterations or until catch handling.
- Around line 49-69: The scheduler currently calls
escrowPaymentRepo.save(payment) inside the loop (see
escrowPaymentRepo.save(payment), payment._id, escrow.xrplSequence), which can
overwrite the whole payment document and cause race conditions with concurrent
operations like EscrowCreateProcessor.markEscrowed() or
EscrowPaymentsService.releaseEscrow(); add a repository method
markCancelled(paymentId: string, escrowId: string) that performs a targeted
findOneAndUpdate with an $elemMatch on escrows._id and status "CANCELLING" and
sets escrows.$.status to "CANCELLED", then replace the save(payment) call in the
loop with a call to escrowPaymentRepo.markCancelled(payment._id.toString(),
escrow._id.toString()) so only the single escrow element is updated atomically.
In `@src/modules/escrow-payments/escrow-payments-crud.service.ts`:
- Around line 130-150: The service currently constructs DB-specific filters
(using Types.ObjectId, $or, $in and the local filter variable) and calls
this.escrowPaymentRepo.findMany / countDocuments; move that logic into the
repository by adding a domain-oriented method like
escrowPaymentRepo.findPageByParticipant(userId, { status?, group?, page, limit
}) which will convert userId to ObjectId, build the $or/$in clauses and return
the paged result (data and total) so the service no longer references
Types.ObjectId, $or, $in or the filter object; update the service code to call
findPageByParticipant and remove the filter construction and the two repo calls.
- Line 60: Multiple occurrences cast values to `any` (e.g., `(seller as
any)._id`, `(buyer as any)._id`, `(item: any)`, `(doc as any).buyerId`) which
hides missing/renamed fields; update the repository/facade return types and
local variable types instead of using `as any`: define/consume explicit lean
types like `LeanUser` for `seller`/`buyer` and `LeanEscrowPayment` (or similar)
for `doc`/`item`, change the repository/facade method signatures to return those
types, and then remove all `as any` casts (affecting symbols in this file such
as sellerId assignment, buyerId usages, item accessors and the methods that
fetch `seller`/`buyer`/`doc`); ensure the `. _id` and `buyerId` properties are
present on the new interfaces so the compiler enforces correctness.
- Around line 91-119: The service currently calls .toObject() after awaiting
escrowPaymentRepo.create(...), leaking Mongoose Document behavior into the
service; update the repository so create(...) returns a POJO (like
findByIdLean() and findMany() do) by converting the saved document to a plain
object inside the repository implementation, then remove the .toObject() call in
the service (so mapToResponse(saved, userId) receives a POJO). Ensure
escrowPaymentRepo.create keeps the same signature but returns an EscrowPayment
POJO and adjust any callers accordingly.
In `@src/modules/escrow-payments/escrow-payments.module.ts`:
- Around line 33-34: The module currently registers EscrowPaymentRepository and
UserFacade only in providers but does not export them, preventing reuse by other
modules; update the EscrowPaymentsModule to add EscrowPaymentRepository and
UserFacade to the module's exports array (alongside existing providers like
EscrowPaymentsService/EscrowPaymentsController if needed) so other modules can
import and reuse these abstractions.
In `@src/modules/escrow-payments/escrow-payments.service.ts`:
- Around line 144-174: The code relies on the transaction callback to always set
initiated, but the final `return initiated!` uses a non-null assertion that
could become unsafe after future refactors; after the
`session.withTransaction(...)` completes and before returning, add an explicit
guard like `if (!initiated) throw new PaymentInitiationFailedException()` (or
rethrow `PaymentNotApprovedForPayException` as appropriate) so that `initiated`
is validated; update the block around `initiated`,
`escrowPaymentRepo.markProcessing`, and `withTransaction` to perform this
null-check and throw a clear exception instead of relying on `!`.
In `@src/modules/escrow-payments/repositories/escrow-payment.repository.ts`:
- Around line 142-170: markEscrowed가 동일한 escrow에 대해 여러번 호출되면
xrplSequence/txHashCreate가 덮어써질 수 있으니 업데이트 조건에 기존 메타데이터가 비어 있는 경우에만 실행되도록 쿼리를
강화하세요: 현재 markEscrowed 함수의 findOneAndUpdate 조회조건(escrows.$elemMatch) 에 escrows
항목이 xrplSequence 또는 txHashCreate가 존재하지 않거나 null인 조건(예: _id: new
Types.ObjectId(escrowId), status: "SUBMITTING", xrplSequence: { $exists: false }
또는 xrplSequence: null, txHashCreate: { $exists: false } 등)을 추가해서 이미 설정된 경우 업데이트가
실패해 null을 반환하도록 하고, 필요한 경우 호출자에게 멱등성 실패를 명확히 전달하도록 처리하세요.
In `@src/modules/escrow-payments/repositories/user.facade.ts`:
- Around line 25-36: The findByWalletAddressAndType method currently projects
the entire wallet object which will include sensitive subfields (e.g.,
wallet.seed); update the projection in that method to only include the specific
wallet subfields you actually need (for example "wallet.address" and any other
non-sensitive fields) instead of "wallet", so change the projection object used
in this.model.findOne(...) to explicitly list allowed wallet fields (e.g., {
_id: 1, name: 1, "wallet.address": 1 }) and remove any sensitive keys.
🪄 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
Run ID: bcc1b3d7-08c2-4f5c-90bc-349962876c95
📒 Files selected for processing (15)
src/modules/escrow-payments/__tests__/approval.spec.tssrc/modules/escrow-payments/__tests__/create.spec.tssrc/modules/escrow-payments/__tests__/helpers.tssrc/modules/escrow-payments/__tests__/initiate.spec.tssrc/modules/escrow-payments/__tests__/query.spec.tssrc/modules/escrow-payments/__tests__/recovery-scheduler.spec.tssrc/modules/escrow-payments/__tests__/xrpl.spec.tssrc/modules/escrow-payments/escrow-cancel.scheduler.tssrc/modules/escrow-payments/escrow-create.processor.tssrc/modules/escrow-payments/escrow-payments-crud.service.tssrc/modules/escrow-payments/escrow-payments.module.tssrc/modules/escrow-payments/escrow-payments.service.tssrc/modules/escrow-payments/escrow-submit-recovery.scheduler.tssrc/modules/escrow-payments/repositories/escrow-payment.repository.tssrc/modules/escrow-payments/repositories/user.facade.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves #47
개요
서비스·스케줄러 계층이 Mongoose 모델에 직접 의존하고 있어
쿼리 로직이 비즈니스 로직과 섞이고, 테스트에서도 복잡한 쿼리 체인을
직접 목킹해야 하는 문제가 있었습니다.
변경 내용
신규 파일
repositories/escrow-payment.repository.ts— 에스크로 결제 관련 모든 DB 접근 집중 (findById, markProcessing, preflight, markEscrowed 등)repositories/user.facade.ts— UserModule 의존성을 단일 진입점으로 래핑리팩토링
EscrowPaymentsService,EscrowPaymentsCrudService,EscrowCreateProcessor,EscrowCancelScheduler,EscrowSubmitRecoveryScheduler)에서@InjectModel제거 → 레포지토리·파사드 주입으로 교체$elemMatch,$set,findOneAndUpdate등)이 서비스에서 사라지고 레포지토리 내부로 이동테스트
makeEscrowPaymentModelMock/makeUserModelMock(쿼리 체인 목) 제거makeEscrowPaymentRepoMock/makeUserFacadeMock으로 교체 — 단순mockResolvedValue기반Summary by CodeRabbit
릴리스 노트
내부 개선
테스트 개선
성능/운영