refactor(infra): enforce remaining coding standards across platform (STA-249)#270
Merged
Merged
Conversation
Three small style-rule cleanups across the platform: - Replace LoggerFactory.getLogger with @slf4j in ExternalApiLoggingInterceptor. - Drop 5 leftover block comments from switch-default and no-op lambda bodies in webhook validators, OFAC parser, and the nonce manager. - Use var for ~20 obvious local declarations across ledger, compliance, custody, fiat-on-ramp, merchant-iam, merchant-onboarding, payment-orchestrator, and a few test files (PermissionTest, JournalCommandHandlerTest, DevCustodyAdapterTest). No behavior change. Verified with compileJava + compileTestJava on every touched module.
Remove ~135 step-by-step narration and section-divider comments across 29 production files. Code now stands on its own; the remaining 47 comments document non-obvious WHY (Temporal saga authority, JPA orphanRemoval ordering, advisory-lock semantics, PII masking rationale, dev-mode keypair notes, etc.). Heaviest cleanups: PayoutCommandHandler (-13), TransferCommandHandler (-12), RefundCommandHandler (-12), TracingConfig (-13 architecture explainer), RiskScoringService (-9 Factor 1-8 labels), CollectionCommandHandler (-7), PaymentEventConsumer (-7), MerchantTeam (-6 Invariant labels), LockService, MerchantOnboardingWorkflowImpl, PaymentWorkflowImpl, FxRateLockApplicationService. Verified: ./gradlew compileJava + compileTestJava BUILD SUCCESSFUL.
…A-249) Eliminate any()/eq()/anyString()/anyLong()/anyInt()/anyBoolean()/ anyList() from 30 unit test files across api-gateway-iam (14), payment-orchestrator (6), fx-liquidity-engine (3), merchant-onboarding (3), merchant-iam (2), compliance-travel-rule (1), ledger-accounting (1). ~180 matcher usages removed. Stubs now take the literal values the SUT actually passes; save verifications use eqIgnoringTimestamps / eqIgnoring from platform-test TestUtils. Where runtime-generated values genuinely cannot be pre-constructed (workflowId from generated paymentId, Duration computed at revoke time) we use typed argThat(lambda) -- the same escape hatch eqIgnoring itself uses. Verified: 1,467 unit tests pass across all 7 affected modules.
…rs (STA-249) Controllers now call CommandHandler / QueryHandler directly per CLAUDE.md "Controller → CommandHandler directly — no intermediate service layer". compliance-travel-rule: delete ComplianceCheckApplicationService (thin pass-through). ComplianceCheckController and CustomerRiskProfileController inject ComplianceCheckCommandHandler + ComplianceCheckResponseMapper directly. Tests rewritten to mock the handler. fx-liquidity-engine: promote FxQuoteApplicationService, FxRateLockApplicationService, and LiquidityPoolApplicationService to FxQuoteCommandHandler, FxRateLockCommandHandler, and LiquidityPoolQueryHandler under domain/service/. Handlers now take primitives and return domain objects (+ idempotency records), so the hexagonal rule domain-may-not-depend-on-application stays green. Controllers own response mapping via FxResponseMapper (added CorridorSnapshot overload). New per-handler unit tests preserve intent from the removed ApplicationService tests. Dead-code removal: drop orphaned ApplicationService classes left from STA-243 — ApiKeyApplicationService, AuthApplicationService, OAuthClientApplicationService, MerchantApplicationService (api-gateway-iam) and MerchantApplicationService (merchant-onboarding). Confirmed zero references in main or test before deletion. Net: -1505 lines. 13 files deleted, 13 modified, 6 new handlers/tests. Verified: compliance-travel-rule (268 tests), fx-liquidity-engine (202 tests), full compileJava + compileTestJava BUILD SUCCESSFUL.
Puneethkumarck
added
infra
WalkthroughRemoved multiple application-service classes and inlined comment cleanup across services; controllers were rewired to domain command/query handlers and response mappers; several new domain command handlers and query handlers were added (FX); tests were tightened to use strict argument matching and value-based verifications; many local variables switched to Changes
Sequence Diagram(s)mermaid mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
…ollapse (STA-249) Code-review follow-up — the initial handler-collapse commit (aa89f6e) added FxQuoteCommandHandler and FxRateLockCommandHandler alongside the old @service classes instead of replacing them, and left ApiKeyApplicationService in place despite the PR description claiming it was deleted. Net effect before this commit: three @Service-annotated classes were still being instantiated by Spring component scan with no callers, and two duplicate test files (FxQuoteApplicationServiceTest, FxRateLockApplicationServiceTest) still exercised them. Deletes: - api-gateway-iam/.../application/service/ApiKeyApplicationService.java - fx-liquidity-engine/.../application/service/FxQuoteApplicationService.java - fx-liquidity-engine/.../application/service/FxRateLockApplicationService.java - fx-liquidity-engine/.../test/.../application/service/FxQuoteApplicationServiceTest.java - fx-liquidity-engine/.../test/.../application/service/FxRateLockApplicationServiceTest.java Verified: api-gateway-iam + fx-liquidity-engine unit tests BUILD SUCCESSFUL (rerun-tasks). Rule 3 now fully resolved.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
ledger-accounting/ledger-accounting/src/main/java/com/stablecoin/payments/ledger/domain/service/JournalCommandHandler.java (1)
86-99:⚠️ Potential issue | 🟠 MajorFail fast when a
BalanceUpdatekey is missing to avoid opaque NPEs.Both write paths dereference
updatewithout null checks. IfbalanceCalculatorreturns an incomplete map, this fails late with a non-diagnostic NPE.Proposed hardening patch
@@ - var update = balanceUpdates.get(key); + var update = balanceUpdates.get(key); + if (update == null) { + throw new IllegalStateException("Missing balance update for key: " + key); + } @@ - var update = updates.get(key); + var update = updates.get(key); + if (update == null) { + throw new IllegalStateException("Missing persisted balance update for key: " + key); + } balanceRepository.save(new AccountBalance(Also applies to: 117-123
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ledger-accounting/ledger-accounting/src/main/java/com/stablecoin/payments/ledger/domain/service/JournalCommandHandler.java` around lines 86 - 99, In JournalCommandHandler, both places that read from the balanceUpdates map before building JournalEntry (the var update = balanceUpdates.get(key) used for constructing new JournalEntry) must fail fast when update is null: add an explicit null-check after retrieving update and throw a clear IllegalStateException (or similar runtime exception) that includes the missing key (e.g. account key/accountCode) and transactionId/correlation context, so callers get a diagnostic error rather than an NPE; apply the same check and message to the second occurrence around the lines constructing the other JournalEntry.api-gateway-iam/api-gateway-iam/src/test/java/com/stablecoin/payments/gateway/iam/domain/service/MerchantCommandHandlerTest.java (1)
299-313:⚠️ Potential issue | 🟠 MajorRemove hardcoded secret literal from test fixtures.
"raw-secret"should not be embedded in tests; use generated fixture data and reuse the variable in stubs/assertions.As per coding guidelines, "No hardcoded secrets or real PII in test fixtures".Suggested patch
+ var rawSecret = UUID.randomUUID().toString(); given(oauthClientCommandHandler.create( merchantId, "Acme Corp Default Client", List.of("payments:read"), List.of("client_credentials"))) - .willReturn(new OAuthClientCommandHandler.CreateOAuthClientResult(oauthClient, "raw-secret")); + .willReturn(new OAuthClientCommandHandler.CreateOAuthClientResult(oauthClient, rawSecret)); @@ var expectedEvent = new OAuthClientProvisionedEvent( - clientId, merchantId, "raw-secret", "Acme Corp Default Client", + clientId, merchantId, rawSecret, "Acme Corp Default Client", List.of("payments:read"), List.of("client_credentials"), createdAt);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api-gateway-iam/api-gateway-iam/src/test/java/com/stablecoin/payments/gateway/iam/domain/service/MerchantCommandHandlerTest.java` around lines 299 - 313, The test embeds a hardcoded secret ("raw-secret") when stubbing OAuthClientCommandHandler.create and when asserting the published OAuthClientProvisionedEvent; replace the literal with a generated fixture variable (e.g., oauthClientSecret) and use that same variable in the willReturn(new OAuthClientCommandHandler.CreateOAuthClientResult(oauthClient, oauthClientSecret)) call and in the expectedEvent construction used with eventPublisher.publish to avoid hardcoded secrets; update any fixture setup that produces the secret so the test reuses it consistently.merchant-iam/merchant-iam/src/main/java/com/stablecoin/payments/merchant/iam/infrastructure/persistence/adapter/RoleRepositoryAdapter.java (1)
45-47:⚠️ Potential issue | 🟡 MinorRemove unused stub implementation.
The
countActiveUsersByRoleIdmethod is never invoked—role deletion validation happens directly inMerchantTeam.deleteCustomRole()via in-memory user stream filtering (MerchantTeam.java:128-134). Delete this stub or implement it if future use is planned.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@merchant-iam/merchant-iam/src/main/java/com/stablecoin/payments/merchant/iam/infrastructure/persistence/adapter/RoleRepositoryAdapter.java` around lines 45 - 47, Remove the unused stub method countActiveUsersByRoleId(UUID) from RoleRepositoryAdapter since role deletion validation is done in-memory in MerchantTeam.deleteCustomRole; delete the method signature and body and clean up any now-unused imports or references in RoleRepositoryAdapter to keep the class compiling.compliance-travel-rule/compliance-travel-rule/src/test/java/com/stablecoin/payments/compliance/domain/service/ComplianceCheckCommandHandlerTest.java (1)
217-242:⚠️ Potential issue | 🟡 MinorUse
InOrderto verify provider execution sequence.The test name
shouldInvokeAllProvidersInOrderdeclares an intent to verify cross-mock invocation order, but sequentialthen(...).should()verifications only confirm each method was called—not their relative sequence. Mockito requires explicitInOrderverification to enforce strict ordering across mocks. For compliance audit trails, execution order matters.Suggested fix
+import static org.mockito.Mockito.inOrder; +import org.mockito.InOrder; ... void shouldInvokeAllProvidersInOrder() { // ... setup ... handler.initiateCheck(...); - then(kycProvider).should().verify(senderId, recipientId); - then(sanctionsProvider).should().screen(senderId, recipientId); - then(amlProvider).should().analyze(senderId, recipientId); - then(travelRuleProvider).should().transmit(argThat((TravelRulePackage p) -> p != null)); - then(checkRepository).should().save(argThat((ComplianceCheck c) -> c != null)); - then(eventPublisher).should().publish(argThat(o -> o instanceof ComplianceCheckPassed)); + InOrder inOrder = inOrder(kycProvider, sanctionsProvider, amlProvider, + travelRuleProvider, checkRepository, eventPublisher); + inOrder.verify(kycProvider).verify(senderId, recipientId); + inOrder.verify(sanctionsProvider).screen(senderId, recipientId); + inOrder.verify(amlProvider).analyze(senderId, recipientId); + inOrder.verify(travelRuleProvider).transmit(argThat((TravelRulePackage p) -> p != null)); + inOrder.verify(checkRepository).save(argThat((ComplianceCheck c) -> c != null)); + inOrder.verify(eventPublisher).publish(argThat(o -> o instanceof ComplianceCheckPassed));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@compliance-travel-rule/compliance-travel-rule/src/test/java/com/stablecoin/payments/compliance/domain/service/ComplianceCheckCommandHandlerTest.java` around lines 217 - 242, The test should assert the cross-mock invocation order instead of independent calls: create a Mockito InOrder for kycProvider, sanctionsProvider, amlProvider, travelRuleProvider, checkRepository, and eventPublisher, invoke handler.initiateCheck(...) as before, then use inOrder.verify(...) on those mocks in the expected sequence (verify kycProvider.verify(...), then sanctionsProvider.screen(...), then amlProvider.analyze(...), then travelRuleProvider.transmit(...), then checkRepository.save(...), then eventPublisher.publish(...)); remove the separate then(...).should() calls or replace them with the inOrder verifications so the test enforces strict ordering.payment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/domain/workflow/PaymentWorkflowImpl.java (1)
300-307:⚠️ Potential issue | 🔴 CriticalAvoid hard-coded transfer destination in workflow execution path.
Line 305 and Line 306 hard-code recipient wallet and chain for every payment. Unless this is an explicit treasury-routing requirement, this can misroute funds and bypass corridor/recipient-specific settlement logic. Use validated destination data from workflow input (or controlled config) instead of literals.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@payment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/domain/workflow/PaymentWorkflowImpl.java` around lines 300 - 307, PaymentWorkflowImpl currently passes hard-coded recipient and chain into chainTransferActivity.submitTransfer via new ChainTransferRequest (the literal "0x742d35..." and "base"); change it to use validated destination fields from the workflow input or a controlled routing config instead: extract the destination address and chain from the incoming request (e.g., request.destination()/request.getRecipientAddress() and request.chain()/request.getDestinationChain()) or, if routing must be centralized, read them from a validated RoutingConfig, perform validation/sanitization, and pass those variables into the ChainTransferRequest along with existing values (request.paymentId(), request.correlationId(), fxResult.targetAmount()); ensure no hard-coded fallbacks except for a safe, auditable config-derived default and add validation errors/early-fail if destination is missing or invalid before calling chainTransferActivity.submitTransfer.fx-liquidity-engine/fx-liquidity-engine/src/test/java/com/stablecoin/payments/fx/infrastructure/messaging/PaymentEventConsumerTest.java (1)
143-158:⚠️ Potential issue | 🟡 MinorAssert outbox silence on the non-publishing paths.
These branches never verify that
eventPublisherstays untouched, and the failed no-lock case also does not assertpoolRepositorystays untouched. Extra side effects after an early return would still pass this suite.Also applies to: 178-192, 213-228
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fx-liquidity-engine/fx-liquidity-engine/src/test/java/com/stablecoin/payments/fx/infrastructure/messaging/PaymentEventConsumerTest.java` around lines 143 - 158, The test branch "noLockFound" in PaymentEventConsumerTest (and the similar branches around lines 178-192, 213-228) doesn't assert that eventPublisher and poolRepository remain untouched; update the tests that exercise consumer.onPaymentCompleted to also verify no interactions with eventPublisher (then(eventPublisher).shouldHaveNoInteractions()) and with poolRepository where appropriate (then(poolRepository).shouldHaveNoInteractions()), using the same Mockito/BDD style as the existing assertions so early returns cannot hide unexpected side effects.fx-liquidity-engine/fx-liquidity-engine/src/test/java/com/stablecoin/payments/fx/application/controller/FxQuoteControllerTest.java (1)
143-245:⚠️ Potential issue | 🟡 MinorMissing test coverage for DELETE /v1/fx/lock/{lockId} endpoint.
The controller has a
releaseLockendpoint (lines 61-65 ofFxQuoteController.java) that delegates torateLockCommandHandler.releaseLock(lockId). No test verifies this delegation or the 204 No Content response.Do you want me to generate test cases for the
releaseLockendpoint?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fx-liquidity-engine/fx-liquidity-engine/src/test/java/com/stablecoin/payments/fx/application/controller/FxQuoteControllerTest.java` around lines 143 - 245, Add unit tests covering the DELETE /v1/fx/lock/{lockId} endpoint: verify that controller.releaseLock(lockId) delegates to rateLockCommandHandler.releaseLock(lockId) and returns HttpStatus.NO_CONTENT (204) on success, and add tests to assert that exceptions thrown by rateLockCommandHandler.releaseLock(lockId) are propagated (e.g., QuoteNotFoundException). Use the existing test pattern in FxQuoteControllerTest (nested LockRate class style) to create a new nested test class or methods that call controller.releaseLock(lockId) and use Mockito given(...).willThrow(...) / willDoNothing() and AssertJ assertions to check response status and exception propagation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@api-gateway-iam/api-gateway-iam/src/test/java/com/stablecoin/payments/gateway/iam/application/security/AuditLogFilterTest.java`:
- Around line 88-99: Tests duplicate AuditLogEntry.builder() setup across
multiple cases; extract a shared helper method (e.g., expectedAuditEntry(...))
in AuditLogFilterTest to build the common AuditLogEntry and return it so
assertions reuse it. Implement a private static method that accepts the varying
values (merchantId, action, resource, sourceIp, statusCode, authMethod,
clientId) and returns the built AuditLogEntry; then replace the inline builder
blocks in the tests with calls to expectedAuditEntry(...) and keep the existing
then(auditLogRepository).should().save(eqIgnoring(expectedEntry, "logId",
"occurredAt")); assertion to avoid duplication and simplify future changes.
- Around line 123-139: The test shouldNotFailWhenRepositoryThrows in
AuditLogFilterTest currently only asserts that filterChain.doFilter was called;
add an explicit verification that auditLogRepository.save(...) was invoked (with
an AuditLogEntry matching merchantId, action "POST", resource "/v1/payments",
etc.) so the exception-handling path is exercised. After calling
filter.doFilterInternal(request, response, filterChain), add a
then(auditLogRepository).should().save(...) using eqIgnoring or an argument
matcher that ignores "logId" and "occurredAt" to assert save was attempted
despite the repository throwing.
In
`@fx-liquidity-engine/fx-liquidity-engine/src/main/java/com/stablecoin/payments/fx/domain/service/FxRateLockCommandHandler.java`:
- Around line 60-64: FxRateLockCommandHandler already performs the domain-level
liquidity validation using pool.hasSufficientLiquidity(...) and throws
InsufficientLiquidityException, so remove the duplicate liquidity check from
LockService.lockRate() to avoid redundant validation and the less-specific
IllegalStateException; update LockService.lockRate() to assume validation has
been done by FxRateLockCommandHandler (or delegate to it) and keep only logic
that performs the lock itself, leaving InsufficientLiquidityException handling
in the domain handler (references: FxRateLockCommandHandler,
pool.hasSufficientLiquidity, InsufficientLiquidityException,
LockService.lockRate).
In
`@fx-liquidity-engine/fx-liquidity-engine/src/main/java/com/stablecoin/payments/fx/domain/service/LiquidityPoolQueryHandler.java`:
- Line 24: CorridorSnapshot is currently an inner record of
LiquidityPoolQueryHandler but is used by other layers; extract it into a new
top-level DTO/projection (e.g., public record CorridorSnapshot(LiquidityPool
pool, CorridorRate rate)) placed in the domain/application boundary package,
remove the nested declaration from LiquidityPoolQueryHandler, update
imports/usages across the codebase to reference the new top-level
CorridorSnapshot type, and ensure the new type is public so consumers compile
without depending on the service implementation.
In
`@fx-liquidity-engine/fx-liquidity-engine/src/test/java/com/stablecoin/payments/fx/domain/service/FxQuoteCommandHandlerTest.java`:
- Around line 46-99: Add tests covering stale-rate behavior: one test (e.g.,
shouldUseProviderWhenCachedRateIsStale) should mock rateCache.get("USD","EUR")
to return a corridor rate marked stale, mock rateProvider.getRate("USD","EUR")
to return a fresh corridorRate, then assert handler.getQuote("USD","EUR",
amount) returns the expected quote from quoteService.createQuote and that
rateCache.put("USD","EUR", corridorRate) was called; another test (e.g.,
shouldThrowWhenBothCachedAndProviderRatesAreStale) should mock rateCache.get to
return a stale rate and rateProvider.getRate to return Optional.empty or a stale
rate and assert handler.getQuote throws RateUnavailableException containing
"USD:EUR". Ensure you reference handler.getQuote, rateCache, rateProvider,
quoteService.createQuote, quoteRepository.save and RateUnavailableException when
locating code to change.
In
`@fx-liquidity-engine/fx-liquidity-engine/src/test/java/com/stablecoin/payments/fx/domain/service/FxRateLockCommandHandlerTest.java`:
- Around line 65-213: Add a new nested test class for releaseLock and cover the
four branches of handler.releaseLock(UUID): (1) when
lockRepository.findById(lockId) returns empty assert LockNotFoundException and
no interactions with poolRepository/eventPublisher; (2) when the lock exists but
is not active assert LockNotActiveException and no further interactions; (3)
successful release: mock lockRepository.findById to return an active lock, mock
poolRepository.findByCorridor to return the pool, verify lockRepository.save and
poolRepository.save are called with updated entities and that
eventPublisher.publish(...) is invoked with the expected FxRateReleased event;
(4) pool not found: mock poolRepository.findByCorridor to return empty and
assert PoolNotFoundException and no save/publish calls. Use handler.releaseLock,
lockRepository.findById, poolRepository.findByCorridor, lockRepository.save,
poolRepository.save and eventPublisher.publish to locate and assert behavior.
In
`@ledger-accounting/ledger-accounting/src/test/java/com/stablecoin/payments/ledger/infrastructure/messaging/LedgerOutboxEventPublisherTest.java`:
- Around line 55-56: The test uses eqIgnoring(expectedEvent, "eventId") which
currently skips all Instant fields so completedAt/detectedAt aren't asserted;
update LedgerOutboxEventPublisherTest to stop relying on eqIgnoring to validate
timestamps: keep a loose check for eventId but capture the published event (via
ArgumentCaptor or a custom argThat) and explicitly assert completedAt and
detectedAt against expectedEvent (or within a small tolerance) to ensure
timestamp mapping is verified when calling outbox.schedule(...). Reference
helpers: eqIgnoring, expectedEvent, outbox.schedule, eventId, completedAt,
detectedAt.
In
`@merchant-iam/merchant-iam/src/test/java/com/stablecoin/payments/merchant/iam/domain/team/MerchantTeamServiceTest.java`:
- Around line 134-140: In MerchantTeamServiceTest, strengthen the weak argThat
checks by asserting semantic values: replace argThat((Instant i) -> i != null)
with a comparison against the persisted invitation expiry (capture the saved
Invitation via an ArgumentCaptor or match the same Instant used to persist) and
replace publish(argThat(Objects::nonNull)) with an argThat that inspects the
published event object (verify merchantId, user email/name, and
invitationId/token match expected values). Apply the same approach to the other
occurrence noted (lines ~459-463) so both the saved Invitation and the published
event are validated for expected field values instead of non-null only.
In
`@merchant-onboarding/merchant-onboarding/src/test/java/com/stablecoin/payments/merchant/onboarding/application/controller/KybWebhookControllerTest.java`:
- Line 52: The test currently asserts no interactions with kybProvider on the
invalid-signature branch but doesn't assert the Temporal client; add an
assertion to ensure workflowClient also had no interactions. In
KybWebhookControllerTest (the invalid-signature test), after
then(kybProvider).shouldHaveNoInteractions(); add
then(workflowClient).shouldHaveNoInteractions(); (or an equivalent
verifyNoMoreInteractions(workflowClient)) so the test guarantees zero downstream
side effects from invalid signatures.
In
`@payment-orchestrator/payment-orchestrator/src/test/java/com/stablecoin/payments/orchestrator/application/controller/PaymentControllerTest.java`:
- Around line 59-69: The repeated given(...) stubbing for
commandHandler.initiatePayment in PaymentControllerTest should be extracted into
a private helper to avoid duplication; add a method (e.g.,
stubInitiatePaymentOrReturn or whenInitiatePaymentWillReturn) inside the test
class that accepts the expected return value (initiateResult) and performs the
given(commandHandler.initiatePayment(...)).willReturn(...) call using the same
argThat predicates (IDEMPOTENCY_KEY, SENDER_ID, RECIPIENT_ID,
SOURCE_AMOUNT_VALUE, SOURCE_CURRENCY, TARGET_CURRENCY, SOURCE_COUNTRY,
TARGET_COUNTRY), then replace both verbatim blocks in the tests with a single
call to that helper to centralize the matcher logic and keep tests DRY.
In
`@payment-orchestrator/payment-orchestrator/src/test/java/com/stablecoin/payments/orchestrator/domain/service/PaymentCommandHandlerTest.java`:
- Around line 145-147: Replace the weak assertion that only checks
save(existingPayment) with two stronger checks: verify that paymentRepository
was queried for the idempotency key (call the repository method used in the
code, e.g., findByIdempotencyKey or findByIdempotencyKeyOrWhatever, with
existingPayment.getIdempotencyKey()) and then assert
then(paymentRepository).shouldHaveNoMoreInteractions() to ensure no other saves
or persists happened; keep the existing eventPublisher and workflowClient
no-interaction assertions. This uses paymentRepository and existingPayment as
anchors to locate and tighten the test.
---
Outside diff comments:
In
`@api-gateway-iam/api-gateway-iam/src/test/java/com/stablecoin/payments/gateway/iam/domain/service/MerchantCommandHandlerTest.java`:
- Around line 299-313: The test embeds a hardcoded secret ("raw-secret") when
stubbing OAuthClientCommandHandler.create and when asserting the published
OAuthClientProvisionedEvent; replace the literal with a generated fixture
variable (e.g., oauthClientSecret) and use that same variable in the
willReturn(new OAuthClientCommandHandler.CreateOAuthClientResult(oauthClient,
oauthClientSecret)) call and in the expectedEvent construction used with
eventPublisher.publish to avoid hardcoded secrets; update any fixture setup that
produces the secret so the test reuses it consistently.
In
`@compliance-travel-rule/compliance-travel-rule/src/test/java/com/stablecoin/payments/compliance/domain/service/ComplianceCheckCommandHandlerTest.java`:
- Around line 217-242: The test should assert the cross-mock invocation order
instead of independent calls: create a Mockito InOrder for kycProvider,
sanctionsProvider, amlProvider, travelRuleProvider, checkRepository, and
eventPublisher, invoke handler.initiateCheck(...) as before, then use
inOrder.verify(...) on those mocks in the expected sequence (verify
kycProvider.verify(...), then sanctionsProvider.screen(...), then
amlProvider.analyze(...), then travelRuleProvider.transmit(...), then
checkRepository.save(...), then eventPublisher.publish(...)); remove the
separate then(...).should() calls or replace them with the inOrder verifications
so the test enforces strict ordering.
In
`@fx-liquidity-engine/fx-liquidity-engine/src/test/java/com/stablecoin/payments/fx/application/controller/FxQuoteControllerTest.java`:
- Around line 143-245: Add unit tests covering the DELETE /v1/fx/lock/{lockId}
endpoint: verify that controller.releaseLock(lockId) delegates to
rateLockCommandHandler.releaseLock(lockId) and returns HttpStatus.NO_CONTENT
(204) on success, and add tests to assert that exceptions thrown by
rateLockCommandHandler.releaseLock(lockId) are propagated (e.g.,
QuoteNotFoundException). Use the existing test pattern in FxQuoteControllerTest
(nested LockRate class style) to create a new nested test class or methods that
call controller.releaseLock(lockId) and use Mockito given(...).willThrow(...) /
willDoNothing() and AssertJ assertions to check response status and exception
propagation.
In
`@fx-liquidity-engine/fx-liquidity-engine/src/test/java/com/stablecoin/payments/fx/infrastructure/messaging/PaymentEventConsumerTest.java`:
- Around line 143-158: The test branch "noLockFound" in PaymentEventConsumerTest
(and the similar branches around lines 178-192, 213-228) doesn't assert that
eventPublisher and poolRepository remain untouched; update the tests that
exercise consumer.onPaymentCompleted to also verify no interactions with
eventPublisher (then(eventPublisher).shouldHaveNoInteractions()) and with
poolRepository where appropriate
(then(poolRepository).shouldHaveNoInteractions()), using the same Mockito/BDD
style as the existing assertions so early returns cannot hide unexpected side
effects.
In
`@ledger-accounting/ledger-accounting/src/main/java/com/stablecoin/payments/ledger/domain/service/JournalCommandHandler.java`:
- Around line 86-99: In JournalCommandHandler, both places that read from the
balanceUpdates map before building JournalEntry (the var update =
balanceUpdates.get(key) used for constructing new JournalEntry) must fail fast
when update is null: add an explicit null-check after retrieving update and
throw a clear IllegalStateException (or similar runtime exception) that includes
the missing key (e.g. account key/accountCode) and transactionId/correlation
context, so callers get a diagnostic error rather than an NPE; apply the same
check and message to the second occurrence around the lines constructing the
other JournalEntry.
In
`@merchant-iam/merchant-iam/src/main/java/com/stablecoin/payments/merchant/iam/infrastructure/persistence/adapter/RoleRepositoryAdapter.java`:
- Around line 45-47: Remove the unused stub method
countActiveUsersByRoleId(UUID) from RoleRepositoryAdapter since role deletion
validation is done in-memory in MerchantTeam.deleteCustomRole; delete the method
signature and body and clean up any now-unused imports or references in
RoleRepositoryAdapter to keep the class compiling.
In
`@payment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/domain/workflow/PaymentWorkflowImpl.java`:
- Around line 300-307: PaymentWorkflowImpl currently passes hard-coded recipient
and chain into chainTransferActivity.submitTransfer via new ChainTransferRequest
(the literal "0x742d35..." and "base"); change it to use validated destination
fields from the workflow input or a controlled routing config instead: extract
the destination address and chain from the incoming request (e.g.,
request.destination()/request.getRecipientAddress() and
request.chain()/request.getDestinationChain()) or, if routing must be
centralized, read them from a validated RoutingConfig, perform
validation/sanitization, and pass those variables into the ChainTransferRequest
along with existing values (request.paymentId(), request.correlationId(),
fxResult.targetAmount()); ensure no hard-coded fallbacks except for a safe,
auditable config-derived default and add validation errors/early-fail if
destination is missing or invalid before calling
chainTransferActivity.submitTransfer.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 042f5388-157e-4150-9956-eeb29852b516
📒 Files selected for processing (101)
api-gateway-iam/api-gateway-iam/src/main/java/com/stablecoin/payments/gateway/iam/application/controller/ApiKeyController.javaapi-gateway-iam/api-gateway-iam/src/main/java/com/stablecoin/payments/gateway/iam/application/security/UserJwtAuthenticationFilter.javaapi-gateway-iam/api-gateway-iam/src/main/java/com/stablecoin/payments/gateway/iam/application/service/ApiKeyApplicationService.javaapi-gateway-iam/api-gateway-iam/src/main/java/com/stablecoin/payments/gateway/iam/application/service/AuthApplicationService.javaapi-gateway-iam/api-gateway-iam/src/main/java/com/stablecoin/payments/gateway/iam/application/service/MerchantApplicationService.javaapi-gateway-iam/api-gateway-iam/src/main/java/com/stablecoin/payments/gateway/iam/application/service/OAuthClientApplicationService.javaapi-gateway-iam/api-gateway-iam/src/test/java/com/stablecoin/payments/gateway/iam/application/controller/ApiKeyControllerTest.javaapi-gateway-iam/api-gateway-iam/src/test/java/com/stablecoin/payments/gateway/iam/application/controller/AuthControllerTest.javaapi-gateway-iam/api-gateway-iam/src/test/java/com/stablecoin/payments/gateway/iam/application/controller/MerchantControllerTest.javaapi-gateway-iam/api-gateway-iam/src/test/java/com/stablecoin/payments/gateway/iam/application/controller/OAuthClientControllerTest.javaapi-gateway-iam/api-gateway-iam/src/test/java/com/stablecoin/payments/gateway/iam/application/security/AuditLogFilterTest.javaapi-gateway-iam/api-gateway-iam/src/test/java/com/stablecoin/payments/gateway/iam/application/security/RateLimitFilterTest.javaapi-gateway-iam/api-gateway-iam/src/test/java/com/stablecoin/payments/gateway/iam/domain/service/ApiKeyCommandHandlerTest.javaapi-gateway-iam/api-gateway-iam/src/test/java/com/stablecoin/payments/gateway/iam/domain/service/ApiKeyServiceTest.javaapi-gateway-iam/api-gateway-iam/src/test/java/com/stablecoin/payments/gateway/iam/domain/service/AuthCommandHandlerTest.javaapi-gateway-iam/api-gateway-iam/src/test/java/com/stablecoin/payments/gateway/iam/domain/service/AuthServiceTest.javaapi-gateway-iam/api-gateway-iam/src/test/java/com/stablecoin/payments/gateway/iam/domain/service/MerchantCommandHandlerTest.javaapi-gateway-iam/api-gateway-iam/src/test/java/com/stablecoin/payments/gateway/iam/domain/service/MerchantServiceTest.javaapi-gateway-iam/api-gateway-iam/src/test/java/com/stablecoin/payments/gateway/iam/domain/service/OAuthClientCommandHandlerTest.javaapi-gateway-iam/api-gateway-iam/src/test/java/com/stablecoin/payments/gateway/iam/domain/service/OAuthClientServiceTest.javablockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/domain/service/ChainSelectionEngine.javablockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/domain/service/TransferCommandHandler.javablockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/infrastructure/persistence/NonceManagerPersistenceAdapter.javablockchain-custody/blockchain-custody/src/test/java/com/stablecoin/payments/custody/infrastructure/provider/dev/DevCustodyAdapterTest.javacompliance-travel-rule/compliance-travel-rule/src/main/java/com/stablecoin/payments/compliance/application/controller/ComplianceCheckController.javacompliance-travel-rule/compliance-travel-rule/src/main/java/com/stablecoin/payments/compliance/application/controller/CustomerRiskProfileController.javacompliance-travel-rule/compliance-travel-rule/src/main/java/com/stablecoin/payments/compliance/application/service/ComplianceCheckApplicationService.javacompliance-travel-rule/compliance-travel-rule/src/main/java/com/stablecoin/payments/compliance/domain/model/ComplianceCheck.javacompliance-travel-rule/compliance-travel-rule/src/main/java/com/stablecoin/payments/compliance/domain/service/RiskScoringService.javacompliance-travel-rule/compliance-travel-rule/src/main/java/com/stablecoin/payments/compliance/infrastructure/provider/chainalysis/ChainalysisAmlAdapter.javacompliance-travel-rule/compliance-travel-rule/src/main/java/com/stablecoin/payments/compliance/infrastructure/provider/ofacsdn/SdnXmlParser.javacompliance-travel-rule/compliance-travel-rule/src/test/java/com/stablecoin/payments/compliance/application/controller/ComplianceCheckControllerTest.javacompliance-travel-rule/compliance-travel-rule/src/test/java/com/stablecoin/payments/compliance/application/controller/CustomerRiskProfileControllerTest.javacompliance-travel-rule/compliance-travel-rule/src/test/java/com/stablecoin/payments/compliance/application/service/ComplianceCheckApplicationServiceTest.javacompliance-travel-rule/compliance-travel-rule/src/test/java/com/stablecoin/payments/compliance/domain/service/ComplianceCheckCommandHandlerTest.javafiat-off-ramp/fiat-off-ramp/src/main/java/com/stablecoin/payments/offramp/domain/service/PayoutCommandHandler.javafiat-off-ramp/fiat-off-ramp/src/main/java/com/stablecoin/payments/offramp/infrastructure/persistence/entity/PayoutOrderEntity.javafiat-off-ramp/fiat-off-ramp/src/main/java/com/stablecoin/payments/offramp/infrastructure/provider/modulr/ModulrWebhookSignatureValidator.javafiat-on-ramp/fiat-on-ramp/src/main/java/com/stablecoin/payments/onramp/domain/service/CollectionCommandHandler.javafiat-on-ramp/fiat-on-ramp/src/main/java/com/stablecoin/payments/onramp/domain/service/RefundCommandHandler.javafiat-on-ramp/fiat-on-ramp/src/main/java/com/stablecoin/payments/onramp/infrastructure/provider/stripe/StripePspAdapter.javafiat-on-ramp/fiat-on-ramp/src/main/java/com/stablecoin/payments/onramp/infrastructure/provider/stripe/StripeSignatureValidator.javafx-liquidity-engine/fx-liquidity-engine/src/main/java/com/stablecoin/payments/fx/application/controller/CorridorController.javafx-liquidity-engine/fx-liquidity-engine/src/main/java/com/stablecoin/payments/fx/application/controller/FxQuoteController.javafx-liquidity-engine/fx-liquidity-engine/src/main/java/com/stablecoin/payments/fx/application/controller/LiquidityPoolController.javafx-liquidity-engine/fx-liquidity-engine/src/main/java/com/stablecoin/payments/fx/application/mapper/FxResponseMapper.javafx-liquidity-engine/fx-liquidity-engine/src/main/java/com/stablecoin/payments/fx/application/service/FxQuoteApplicationService.javafx-liquidity-engine/fx-liquidity-engine/src/main/java/com/stablecoin/payments/fx/application/service/FxRateLockApplicationService.javafx-liquidity-engine/fx-liquidity-engine/src/main/java/com/stablecoin/payments/fx/application/service/LiquidityPoolApplicationService.javafx-liquidity-engine/fx-liquidity-engine/src/main/java/com/stablecoin/payments/fx/domain/model/FxQuote.javafx-liquidity-engine/fx-liquidity-engine/src/main/java/com/stablecoin/payments/fx/domain/service/FxQuoteCommandHandler.javafx-liquidity-engine/fx-liquidity-engine/src/main/java/com/stablecoin/payments/fx/domain/service/FxRateLockCommandHandler.javafx-liquidity-engine/fx-liquidity-engine/src/main/java/com/stablecoin/payments/fx/domain/service/LiquidityPoolQueryHandler.javafx-liquidity-engine/fx-liquidity-engine/src/main/java/com/stablecoin/payments/fx/domain/service/LockService.javafx-liquidity-engine/fx-liquidity-engine/src/main/java/com/stablecoin/payments/fx/infrastructure/messaging/PaymentEventConsumer.javafx-liquidity-engine/fx-liquidity-engine/src/test/java/com/stablecoin/payments/fx/application/controller/CorridorControllerTest.javafx-liquidity-engine/fx-liquidity-engine/src/test/java/com/stablecoin/payments/fx/application/controller/FxQuoteControllerTest.javafx-liquidity-engine/fx-liquidity-engine/src/test/java/com/stablecoin/payments/fx/application/controller/LiquidityPoolControllerTest.javafx-liquidity-engine/fx-liquidity-engine/src/test/java/com/stablecoin/payments/fx/application/service/FxQuoteApplicationServiceTest.javafx-liquidity-engine/fx-liquidity-engine/src/test/java/com/stablecoin/payments/fx/application/service/FxRateLockApplicationServiceTest.javafx-liquidity-engine/fx-liquidity-engine/src/test/java/com/stablecoin/payments/fx/application/service/LiquidityPoolApplicationServiceTest.javafx-liquidity-engine/fx-liquidity-engine/src/test/java/com/stablecoin/payments/fx/domain/service/FxQuoteCommandHandlerTest.javafx-liquidity-engine/fx-liquidity-engine/src/test/java/com/stablecoin/payments/fx/domain/service/FxRateLockCommandHandlerTest.javafx-liquidity-engine/fx-liquidity-engine/src/test/java/com/stablecoin/payments/fx/domain/service/LiquidityPoolQueryHandlerTest.javafx-liquidity-engine/fx-liquidity-engine/src/test/java/com/stablecoin/payments/fx/infrastructure/messaging/PaymentEventConsumerTest.javaledger-accounting/ledger-accounting/src/main/java/com/stablecoin/payments/ledger/domain/model/LedgerTransaction.javaledger-accounting/ledger-accounting/src/main/java/com/stablecoin/payments/ledger/domain/model/ReconciliationRecord.javaledger-accounting/ledger-accounting/src/main/java/com/stablecoin/payments/ledger/domain/service/AccountingRules.javaledger-accounting/ledger-accounting/src/main/java/com/stablecoin/payments/ledger/domain/service/BalanceCalculator.javaledger-accounting/ledger-accounting/src/main/java/com/stablecoin/payments/ledger/domain/service/JournalCommandHandler.javaledger-accounting/ledger-accounting/src/main/java/com/stablecoin/payments/ledger/infrastructure/messaging/LedgerEventConsumer.javaledger-accounting/ledger-accounting/src/test/java/com/stablecoin/payments/ledger/domain/service/JournalCommandHandlerTest.javaledger-accounting/ledger-accounting/src/test/java/com/stablecoin/payments/ledger/infrastructure/messaging/LedgerOutboxEventPublisherTest.javamerchant-iam/merchant-iam/src/main/java/com/stablecoin/payments/merchant/iam/application/controller/ErrorCodes.javamerchant-iam/merchant-iam/src/main/java/com/stablecoin/payments/merchant/iam/domain/team/MerchantTeam.javamerchant-iam/merchant-iam/src/main/java/com/stablecoin/payments/merchant/iam/domain/team/model/MerchantUser.javamerchant-iam/merchant-iam/src/main/java/com/stablecoin/payments/merchant/iam/infrastructure/persistence/adapter/RoleRepositoryAdapter.javamerchant-iam/merchant-iam/src/test/java/com/stablecoin/payments/merchant/iam/domain/team/AuthServiceTest.javamerchant-iam/merchant-iam/src/test/java/com/stablecoin/payments/merchant/iam/domain/team/MerchantTeamServiceTest.javamerchant-iam/merchant-iam/src/test/java/com/stablecoin/payments/merchant/iam/domain/team/model/core/PermissionTest.javamerchant-onboarding/merchant-onboarding/src/main/java/com/stablecoin/payments/merchant/onboarding/application/config/SecurityConfig.javamerchant-onboarding/merchant-onboarding/src/main/java/com/stablecoin/payments/merchant/onboarding/application/controller/ErrorCodes.javamerchant-onboarding/merchant-onboarding/src/main/java/com/stablecoin/payments/merchant/onboarding/application/controller/KybWebhookController.javamerchant-onboarding/merchant-onboarding/src/main/java/com/stablecoin/payments/merchant/onboarding/application/service/MerchantApplicationService.javamerchant-onboarding/merchant-onboarding/src/main/java/com/stablecoin/payments/merchant/onboarding/infrastructure/kyb/OnfidoKybAdapter.javamerchant-onboarding/merchant-onboarding/src/main/java/com/stablecoin/payments/merchant/onboarding/infrastructure/temporal/workflow/MerchantOnboardingWorkflowImpl.javamerchant-onboarding/merchant-onboarding/src/test/java/com/stablecoin/payments/merchant/onboarding/application/controller/KybWebhookControllerTest.javamerchant-onboarding/merchant-onboarding/src/test/java/com/stablecoin/payments/merchant/onboarding/domain/merchant/MerchantCommandHandlerTest.javamerchant-onboarding/merchant-onboarding/src/test/java/com/stablecoin/payments/merchant/onboarding/infrastructure/temporal/workflow/MerchantOnboardingWorkflowTest.javapayment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/domain/service/PaymentCommandHandler.javapayment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/domain/workflow/PaymentWorkflowImpl.javapayment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/infrastructure/activity/ComplianceCheckActivityImpl.javapayment-orchestrator/payment-orchestrator/src/test/java/com/stablecoin/payments/orchestrator/application/controller/PaymentControllerMvcTest.javapayment-orchestrator/payment-orchestrator/src/test/java/com/stablecoin/payments/orchestrator/application/controller/PaymentControllerTest.javapayment-orchestrator/payment-orchestrator/src/test/java/com/stablecoin/payments/orchestrator/domain/service/PaymentCommandHandlerTest.javapayment-orchestrator/payment-orchestrator/src/test/java/com/stablecoin/payments/orchestrator/domain/workflow/PaymentWorkflowTest.javapayment-orchestrator/payment-orchestrator/src/test/java/com/stablecoin/payments/orchestrator/infrastructure/activity/ComplianceCheckActivityImplTest.javapayment-orchestrator/payment-orchestrator/src/test/java/com/stablecoin/payments/orchestrator/infrastructure/activity/FxLockActivityImplTest.javaplatform-infra/src/main/java/com/stablecoin/payments/platform/infrastructure/http/ExternalApiLoggingInterceptor.javaplatform-infra/src/main/java/com/stablecoin/payments/platform/infrastructure/messaging/AbstractOutboxEventPublisher.javaplatform-infra/src/main/java/com/stablecoin/payments/platform/infrastructure/tracing/TracingConfig.java
💤 Files with no reviewable changes (32)
- fx-liquidity-engine/fx-liquidity-engine/src/main/java/com/stablecoin/payments/fx/domain/model/FxQuote.java
- fx-liquidity-engine/fx-liquidity-engine/src/main/java/com/stablecoin/payments/fx/application/service/FxQuoteApplicationService.java
- compliance-travel-rule/compliance-travel-rule/src/main/java/com/stablecoin/payments/compliance/domain/model/ComplianceCheck.java
- fiat-off-ramp/fiat-off-ramp/src/main/java/com/stablecoin/payments/offramp/infrastructure/persistence/entity/PayoutOrderEntity.java
- fx-liquidity-engine/fx-liquidity-engine/src/main/java/com/stablecoin/payments/fx/application/service/FxRateLockApplicationService.java
- platform-infra/src/main/java/com/stablecoin/payments/platform/infrastructure/messaging/AbstractOutboxEventPublisher.java
- ledger-accounting/ledger-accounting/src/main/java/com/stablecoin/payments/ledger/domain/service/AccountingRules.java
- merchant-onboarding/merchant-onboarding/src/main/java/com/stablecoin/payments/merchant/onboarding/application/controller/ErrorCodes.java
- ledger-accounting/ledger-accounting/src/main/java/com/stablecoin/payments/ledger/infrastructure/messaging/LedgerEventConsumer.java
- api-gateway-iam/api-gateway-iam/src/main/java/com/stablecoin/payments/gateway/iam/application/security/UserJwtAuthenticationFilter.java
- platform-infra/src/main/java/com/stablecoin/payments/platform/infrastructure/tracing/TracingConfig.java
- blockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/domain/service/TransferCommandHandler.java
- merchant-iam/merchant-iam/src/main/java/com/stablecoin/payments/merchant/iam/application/controller/ErrorCodes.java
- fiat-on-ramp/fiat-on-ramp/src/main/java/com/stablecoin/payments/onramp/domain/service/CollectionCommandHandler.java
- compliance-travel-rule/compliance-travel-rule/src/main/java/com/stablecoin/payments/compliance/domain/service/RiskScoringService.java
- fx-liquidity-engine/fx-liquidity-engine/src/main/java/com/stablecoin/payments/fx/infrastructure/messaging/PaymentEventConsumer.java
- merchant-onboarding/merchant-onboarding/src/main/java/com/stablecoin/payments/merchant/onboarding/infrastructure/kyb/OnfidoKybAdapter.java
- payment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/infrastructure/activity/ComplianceCheckActivityImpl.java
- merchant-onboarding/merchant-onboarding/src/main/java/com/stablecoin/payments/merchant/onboarding/application/config/SecurityConfig.java
- merchant-iam/merchant-iam/src/main/java/com/stablecoin/payments/merchant/iam/domain/team/MerchantTeam.java
- fiat-off-ramp/fiat-off-ramp/src/main/java/com/stablecoin/payments/offramp/domain/service/PayoutCommandHandler.java
- fiat-on-ramp/fiat-on-ramp/src/main/java/com/stablecoin/payments/onramp/domain/service/RefundCommandHandler.java
- fx-liquidity-engine/fx-liquidity-engine/src/main/java/com/stablecoin/payments/fx/domain/service/LockService.java
- compliance-travel-rule/compliance-travel-rule/src/test/java/com/stablecoin/payments/compliance/application/service/ComplianceCheckApplicationServiceTest.java
- fx-liquidity-engine/fx-liquidity-engine/src/test/java/com/stablecoin/payments/fx/application/service/LiquidityPoolApplicationServiceTest.java
- api-gateway-iam/api-gateway-iam/src/main/java/com/stablecoin/payments/gateway/iam/application/service/AuthApplicationService.java
- api-gateway-iam/api-gateway-iam/src/main/java/com/stablecoin/payments/gateway/iam/application/service/OAuthClientApplicationService.java
- compliance-travel-rule/compliance-travel-rule/src/main/java/com/stablecoin/payments/compliance/application/service/ComplianceCheckApplicationService.java
- fx-liquidity-engine/fx-liquidity-engine/src/main/java/com/stablecoin/payments/fx/application/service/LiquidityPoolApplicationService.java
- merchant-onboarding/merchant-onboarding/src/main/java/com/stablecoin/payments/merchant/onboarding/infrastructure/temporal/workflow/MerchantOnboardingWorkflowImpl.java
- api-gateway-iam/api-gateway-iam/src/main/java/com/stablecoin/payments/gateway/iam/application/service/MerchantApplicationService.java
- merchant-onboarding/merchant-onboarding/src/main/java/com/stablecoin/payments/merchant/onboarding/application/service/MerchantApplicationService.java
Comment on lines
+60
to
+64
| if (!pool.hasSufficientLiquidity(quote.targetAmount())) { | ||
| throw InsufficientLiquidityException.forCorridor( | ||
| quote.fromCurrency(), quote.toCurrency(), | ||
| quote.targetAmount(), pool.availableBalance()); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Duplicate liquidity check exists in LockService.
pool.hasSufficientLiquidity() is checked here and again in LockService.lockRate() (context snippet 1, lines 27-31). The domain exception thrown here (InsufficientLiquidityException) is more appropriate than IllegalStateException from LockService, so this early validation is reasonable. However, consider removing the check from LockService to avoid redundancy.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@fx-liquidity-engine/fx-liquidity-engine/src/main/java/com/stablecoin/payments/fx/domain/service/FxRateLockCommandHandler.java`
around lines 60 - 64, FxRateLockCommandHandler already performs the domain-level
liquidity validation using pool.hasSufficientLiquidity(...) and throws
InsufficientLiquidityException, so remove the duplicate liquidity check from
LockService.lockRate() to avoid redundant validation and the less-specific
IllegalStateException; update LockService.lockRate() to assume validation has
been done by FxRateLockCommandHandler (or delegate to it) and keep only logic
that performs the lock itself, leaving InsufficientLiquidityException handling
in the domain handler (references: FxRateLockCommandHandler,
pool.hasSufficientLiquidity, InsufficientLiquidityException,
LockService.lockRate).
| private final LiquidityPoolRepository poolRepository; | ||
| private final RateCache rateCache; | ||
|
|
||
| public record CorridorSnapshot(LiquidityPool pool, CorridorRate rate) {} |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Move CorridorSnapshot out of LiquidityPoolQueryHandler.
This record is now consumed outside the handler, so the application layer depends on a concrete service type instead of a stable projection. Extract it to a dedicated top-level DTO/projection at the domain/application boundary.
As per coding guidelines, fx-liquidity-engine/**: Hexagonal architecture compliance required.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@fx-liquidity-engine/fx-liquidity-engine/src/main/java/com/stablecoin/payments/fx/domain/service/LiquidityPoolQueryHandler.java`
at line 24, CorridorSnapshot is currently an inner record of
LiquidityPoolQueryHandler but is used by other layers; extract it into a new
top-level DTO/projection (e.g., public record CorridorSnapshot(LiquidityPool
pool, CorridorRate rate)) placed in the domain/application boundary package,
remove the nested declaration from LiquidityPoolQueryHandler, update
imports/usages across the codebase to reference the new top-level
CorridorSnapshot type, and ensure the new type is public so consumers compile
without depending on the service implementation.
Comment on lines
+46
to
+99
| @Nested | ||
| @DisplayName("getQuote") | ||
| class GetQuote { | ||
|
|
||
| @Test | ||
| void shouldCreateQuoteFromCachedRate() { | ||
| var amount = new BigDecimal("10000.00"); | ||
| var corridorRate = aUsdEurRate(); | ||
| var quote = anActiveQuote(); | ||
|
|
||
| given(rateCache.get("USD", "EUR")).willReturn(Optional.of(corridorRate)); | ||
| given(quoteService.createQuote("USD", "EUR", amount, corridorRate)).willReturn(quote); | ||
| given(quoteRepository.save(quote)).willReturn(quote); | ||
|
|
||
| var result = handler.getQuote("USD", "EUR", amount); | ||
|
|
||
| assertThat(result) | ||
| .usingRecursiveComparison() | ||
| .isEqualTo(quote); | ||
|
|
||
| then(rateProvider).shouldHaveNoInteractions(); | ||
| } | ||
|
|
||
| @Test | ||
| void shouldCreateQuoteFromProviderWhenCacheMisses() { | ||
| var amount = new BigDecimal("10000.00"); | ||
| var corridorRate = aUsdEurRate(); | ||
| var quote = anActiveQuote(); | ||
|
|
||
| given(rateCache.get("USD", "EUR")).willReturn(Optional.empty()); | ||
| given(rateProvider.getRate("USD", "EUR")).willReturn(Optional.of(corridorRate)); | ||
| given(quoteService.createQuote("USD", "EUR", amount, corridorRate)).willReturn(quote); | ||
| given(quoteRepository.save(quote)).willReturn(quote); | ||
|
|
||
| var result = handler.getQuote("USD", "EUR", amount); | ||
|
|
||
| assertThat(result) | ||
| .usingRecursiveComparison() | ||
| .isEqualTo(quote); | ||
|
|
||
| then(rateCache).should().put("USD", "EUR", corridorRate); | ||
| } | ||
|
|
||
| @Test | ||
| void shouldThrowWhenNoRateAvailable() { | ||
| var amount = new BigDecimal("10000.00"); | ||
| given(rateCache.get("USD", "EUR")).willReturn(Optional.empty()); | ||
| given(rateProvider.getRate("USD", "EUR")).willReturn(Optional.empty()); | ||
|
|
||
| assertThatThrownBy(() -> handler.getQuote("USD", "EUR", amount)) | ||
| .isInstanceOf(RateUnavailableException.class) | ||
| .hasMessageContaining("USD:EUR"); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add a stale-rate scenario for getQuote().
The new suite covers cache hit/miss, but it does not prove what happens when the cached or provider rate is stale. Please add one case that locks down the expected reject/fallback behavior before a quote is created.
As per coding guidelines, fx-liquidity-engine/**: Quote expiry and staleness must be enforced.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@fx-liquidity-engine/fx-liquidity-engine/src/test/java/com/stablecoin/payments/fx/domain/service/FxQuoteCommandHandlerTest.java`
around lines 46 - 99, Add tests covering stale-rate behavior: one test (e.g.,
shouldUseProviderWhenCachedRateIsStale) should mock rateCache.get("USD","EUR")
to return a corridor rate marked stale, mock rateProvider.getRate("USD","EUR")
to return a fresh corridorRate, then assert handler.getQuote("USD","EUR",
amount) returns the expected quote from quoteService.createQuote and that
rateCache.put("USD","EUR", corridorRate) was called; another test (e.g.,
shouldThrowWhenBothCachedAndProviderRatesAreStale) should mock rateCache.get to
return a stale rate and rateProvider.getRate to return Optional.empty or a stale
rate and assert handler.getQuote throws RateUnavailableException containing
"USD:EUR". Ensure you reference handler.getQuote, rateCache, rateProvider,
quoteService.createQuote, quoteRepository.save and RateUnavailableException when
locating code to change.
12 fixes addressing CodeRabbit review comments: Production: - merchant-iam: delete unused countActiveUsersByRoleId stub from RoleRepository + adapter (role deletion validates in-memory via MerchantTeam.deleteCustomRole). Tests: - MerchantCommandHandlerTest: replace hardcoded "raw-secret" literal with UUID.randomUUID() reused between stub and expected event. - ComplianceCheckCommandHandlerTest: use Mockito InOrder over all six providers/repo/publisher to enforce cross-mock invocation sequence (shouldInvokeAllProvidersInOrder). - PaymentEventConsumerTest: assert shouldHaveNoInteractions() on eventPublisher and poolRepository for all early-return branches (idempotentConsumed, noLockFound, idempotentExpired, noLockFoundFailed). - FxQuoteControllerTest: new @nested ReleaseLock with success (204) + LockNotFoundException propagation. - AuditLogFilterTest: extract shared expectedAuditEntry(...) helper; add missing then(auditLogRepository).should().save(...) assertion in shouldNotFailWhenRepositoryThrows so exception-handling path is actually exercised. - FxRateLockCommandHandlerTest: new @nested ReleaseLock with 4 tests — LockNotFoundException, skip-when-already-expired, happy path with liquidity return, pool-missing graceful handling. - MerchantTeamServiceTest: replace argThat(Instant i -> i != null) with ArgumentCaptor + isAfter(now) semantic check; replace publish(argThat( Objects::nonNull)) with eqIgnoring on a fully-built expected event. - KybWebhookControllerTest: add then(workflowClient).shouldHaveNoInteractions() to invalid-signature test. - PaymentCommandHandlerTest: strengthen idempotency-replay assertion — verify findByIdempotencyKey was called + shouldHaveNoMoreInteractions(). - PaymentControllerTest: extract private stubInitiatePayment helper to DRY up repeated given(commandHandler.initiatePayment(...)) blocks. - LedgerOutboxEventPublisherTest: switch from eqIgnoring to ArgumentCaptor so completedAt/detectedAt Instants are actually asserted (TestUtils.eqIgnoring type-ignores all Instants by design — not suitable for timestamp verification). Not fixed, documented in PR reply: - JournalCommandHandler NPE hardening: CLAUDE.md forbids validation for scenarios that cannot happen; BalanceUpdate key is guaranteed by BalanceCalculator contract. - PaymentWorkflowImpl hard-coded wallet/chain: pre-existing sandbox placeholder from STA-243, out of scope for STA-249 cleanup. - LockService duplicate liquidity check: pre-existing duplication inherited from deleted FxRateLockApplicationService; requires touching LockService which is out of scope. Follow-up ticket. - CorridorSnapshot extraction to top-level: style preference. Inner record is the minimal surface area for an internal projection. Verified: ./gradlew test -x :phase2-integration-tests:test -x :phase3-integration-tests:test BUILD SUCCESSFUL (128 tasks).
Owner
Author
CodeRabbit review response — commit
|
| # | Comment | File | Fix |
|---|---|---|---|
| 1 | Unused countActiveUsersByRoleId stub |
RoleRepositoryAdapter.java:45-47 + interface |
Deleted from both interface and adapter. |
| 2 | Hardcoded "raw-secret" in tests |
MerchantCommandHandlerTest.java:299-313 |
Replaced with var rawSecret = UUID.randomUUID().toString(). |
| 3 | Sequential then().should() doesn't verify ordering |
ComplianceCheckCommandHandlerTest.java:217-242 |
Switched to Mockito InOrder over all 6 mocks. |
| 4 | Early-return branches don't assert no side effects | PaymentEventConsumerTest.java (3 branches) |
Added then(eventPublisher).shouldHaveNoInteractions() + poolRepository where applicable. |
| 5 | Missing tests for DELETE /v1/fx/lock/{lockId} |
FxQuoteControllerTest.java |
Added @Nested ReleaseLock with success (204) + LockNotFoundException propagation. |
| 6a | AuditLogEntry.builder() duplication |
AuditLogFilterTest.java:88-99 |
Extracted expectedAuditEntry(...) helper used by all 4 tests. |
| 6b | shouldNotFailWhenRepositoryThrows doesn't verify save was attempted |
AuditLogFilterTest.java:123-139 |
Added then(auditLogRepository).should().save(...) assertion. |
| 7 | No test coverage for releaseLock handler |
FxRateLockCommandHandlerTest.java |
Added @Nested ReleaseLock with 4 tests: not-found, already-expired, happy-path, pool-missing graceful. |
| 8 | Weak argThat((Instant i) -> i != null) |
MerchantTeamServiceTest.java:134,459 |
Replaced with ArgumentCaptor<Instant> + isAfter(now) semantic check + full expected event via eqIgnoring. |
| 9 | Invalid-signature test misses workflowClient |
KybWebhookControllerTest.java:52 |
Added then(workflowClient).shouldHaveNoInteractions(). |
| 10 | Weak idempotency-replay assertion | PaymentCommandHandlerTest.java:145-147 |
Added findByIdempotencyKey verification + shouldHaveNoMoreInteractions(). |
| 11 | Repeated stubbing in PaymentControllerTest |
PaymentControllerTest.java:59-69 |
Extracted private stubInitiatePayment(InitiateResult) helper. |
| 12 | eqIgnoring skips Instant fields |
LedgerOutboxEventPublisherTest.java:55-56 |
Great catch. Our TestUtils.eqIgnoring type-ignores Instant.class by design. Refactored this test to ArgumentCaptor + explicit assertions on completedAt / detectedAt. Shared helper left unchanged because ~30 other tests rely on the timestamp-ignoring behavior. |
⏭ Out of scope — follow-up tickets (3)
| # | Comment | Rationale |
|---|---|---|
| 13 | PaymentWorkflowImpl.java:300-307 hard-coded wallet/chain |
Pre-existing sandbox placeholder from STA-243, untouched by this PR's diff. Will be replaced when S4 blockchain-custody service wires real recipient addresses. Logged as a follow-up. |
| 14 | FxRateLockCommandHandler.java:64 duplicate liquidity check in LockService.lockRate() |
Pre-existing duplication inherited verbatim from the now-deleted FxRateLockApplicationService. Removing the check in LockService is a behavioral change to untouched code — out of STA-249 cleanup scope. Will open a follow-up to consolidate the domain invariant. |
| 15 | CorridorSnapshot should be top-level |
Style preference. Keeping it nested inside LiquidityPoolQueryHandler minimizes surface area for an internal projection used by exactly one mapper method. Happy to revisit if it grows more consumers. |
🚫 Will not fix (1)
| # | Comment | Rationale |
|---|---|---|
| 16 | JournalCommandHandler.java:86-99 — fail fast on missing BalanceUpdate |
CLAUDE.md explicitly says "Don't add error handling, fallbacks, or validation for scenarios that can't happen. Trust internal code and framework guarantees." The balanceUpdates map is built by BalanceCalculator.computeBalances() iterating request.entries() and keying via balanceKey(accountCode, currency) — the read side uses the same iterator and same keying function. The key is guaranteed present by the calculator's contract. Adding a null check would violate the rule I'm cleaning up in this PR. |
✅ Verified, no action needed (2)
| # | Comment | Finding |
|---|---|---|
| 17 | FxQuoteCommandHandlerTest.java:46-99 — add stale-rate tests |
The handler has no stale-detection logic — staleness is a RateCache implementation concern. From the handler's perspective a stale cache returns Optional.empty() and triggers provider fetch, which is already covered by shouldCreateQuoteFromProviderWhenCacheMisses. The second suggestion (both cache + provider return empty) is already covered by shouldThrowWhenNoRateAvailable. |
| 18 | (CodeRabbit latest review) dbb9556 |
"No actionable comments were generated in the recent review. 🎉" |
Verification
./gradlew test -x :phase2-integration-tests:test -x :phase3-integration-tests:test — BUILD SUCCESSFUL (128 actionable tasks, all unit tests green).
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
compliance-travel-rule/compliance-travel-rule/src/test/java/com/stablecoin/payments/compliance/domain/service/ComplianceCheckCommandHandlerTest.java (1)
337-361:⚠️ Potential issue | 🟠 MajorAdd explicit fail-closed test for sanctions provider exceptions.
This class validates sanctions-hit behavior, but it does not assert the exception path for
sanctionsProvider.screen(...). For this service, the error path should be explicitly covered to ensure rejection, persistence of FAILED state, and failure-event publication.As per coding guidelines, "Sanctions screening must be fail-closed (reject on error, not pass)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@compliance-travel-rule/compliance-travel-rule/src/test/java/com/stablecoin/payments/compliance/domain/service/ComplianceCheckCommandHandlerTest.java` around lines 337 - 361, Add a new test in ComplianceCheckCommandHandlerTest that simulates sanctionsProvider.screen(...) throwing an exception and asserts the pipeline fail-closed behavior: stub given(sanctionsProvider.screen(senderId, recipientId)).willThrow(new RuntimeException("...")); call handler.initiateCheck(paymentId, senderId, recipientId, ...); verify the returned/ persisted ComplianceCheck has status ComplianceCheckStatus.FAILED and overallResult OverallResult.FAILED by configuring checkRepository.save(...) to return its argument and asserting usingRecursiveComparison on paymentId/status/overallResult; also assert amlProvider has no interactions and that the failure event is published via the event publisher (e.g. verify(eventPublisher).publish(...)/publishEvent(...)). Ensure the test uses UUIDs like the existing tests and mirrors setup of shouldStopOnSanctionsHit for consistency.
♻️ Duplicate comments (1)
payment-orchestrator/payment-orchestrator/src/test/java/com/stablecoin/payments/orchestrator/domain/service/PaymentCommandHandlerTest.java (1)
154-165:⚠️ Potential issue | 🟡 MinorAssert the duplicate-save fallback explicitly.
Line 164 stubs
paymentRepository.save(...)to throw, but this test never proves that the save was actually attempted or that the handler performed the retry lookup after the conflict. As written, it can still pass if the implementation does two idempotency lookups and skips the concurrent-duplicate branch entirely. Add repository interaction assertions for thesave(...)call and the post-conflict lookup/no-more-interactions.Also applies to: 181-182
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@payment-orchestrator/payment-orchestrator/src/test/java/com/stablecoin/payments/orchestrator/domain/service/PaymentCommandHandlerTest.java` around lines 154 - 165, Test currently stubs paymentRepository.save(...) to throw a DataIntegrityViolationException but never asserts that save was invoked or that the handler retried the idempotency lookup; update the test to explicitly verify paymentRepository.save(...) was called with the expected Payment (use the same eqIgnoring matcher), then verify paymentRepository.findByIdempotencyKey(IDEMPOTENCY_KEY) was called again after the exception and assert no further interactions with paymentRepository; reference the existing symbols paymentRepository.save, paymentRepository.findByIdempotencyKey, Payment.initiate, and anInitiatedPayment when adding these interaction verifications so the test proves the duplicate-save fallback path is exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@compliance-travel-rule/compliance-travel-rule/src/test/java/com/stablecoin/payments/compliance/domain/service/ComplianceCheckCommandHandlerTest.java`:
- Around line 110-112: Replace permissive argThat matchers used in the mock
setups for travelRuleProvider.transmit and checkRepository.save with
ArgumentCaptor-based assertions: capture the TravelRulePackage passed to
travelRuleProvider.transmit (use
ArgumentCaptor.forClass(TravelRulePackage.class)) and assert using
usingRecursiveComparison().comparingOnlyFields("checkId","originatorVasp","beneficiaryVasp","protocolRef");
capture the ComplianceCheck passed to checkRepository.save
(ArgumentCaptor.forClass(ComplianceCheck.class)) and assert
usingRecursiveComparison().comparingOnlyFields("paymentId","senderId","recipientId","status");
update all occurrences (including the setups around travelRuleProvider.transmit
and checkRepository.save) to return the same stubbing results but validate the
captured objects instead of only checking non-null with argThat.
- Around line 754-757: The test ComplianceCheckCommandHandlerTest should assert
that no downstream providers are touched when a DuplicatePaymentException
triggers the early exit; add then(sanctionsProvider).shouldHaveNoInteractions(),
then(amlProvider).shouldHaveNoInteractions(),
then(profileRepository).shouldHaveNoInteractions(),
then(eventPublisher).shouldHaveNoInteractions(), and
then(travelRuleProvider).shouldHaveNoInteractions() alongside the existing
kycProvider and checkRepository assertions in the duplicate-payment early-exit
test so any refactor that accidentally runs compliance checks before duplicate
validation will fail the test.
---
Outside diff comments:
In
`@compliance-travel-rule/compliance-travel-rule/src/test/java/com/stablecoin/payments/compliance/domain/service/ComplianceCheckCommandHandlerTest.java`:
- Around line 337-361: Add a new test in ComplianceCheckCommandHandlerTest that
simulates sanctionsProvider.screen(...) throwing an exception and asserts the
pipeline fail-closed behavior: stub given(sanctionsProvider.screen(senderId,
recipientId)).willThrow(new RuntimeException("...")); call
handler.initiateCheck(paymentId, senderId, recipientId, ...); verify the
returned/ persisted ComplianceCheck has status ComplianceCheckStatus.FAILED and
overallResult OverallResult.FAILED by configuring checkRepository.save(...) to
return its argument and asserting usingRecursiveComparison on
paymentId/status/overallResult; also assert amlProvider has no interactions and
that the failure event is published via the event publisher (e.g.
verify(eventPublisher).publish(...)/publishEvent(...)). Ensure the test uses
UUIDs like the existing tests and mirrors setup of shouldStopOnSanctionsHit for
consistency.
---
Duplicate comments:
In
`@payment-orchestrator/payment-orchestrator/src/test/java/com/stablecoin/payments/orchestrator/domain/service/PaymentCommandHandlerTest.java`:
- Around line 154-165: Test currently stubs paymentRepository.save(...) to throw
a DataIntegrityViolationException but never asserts that save was invoked or
that the handler retried the idempotency lookup; update the test to explicitly
verify paymentRepository.save(...) was called with the expected Payment (use the
same eqIgnoring matcher), then verify
paymentRepository.findByIdempotencyKey(IDEMPOTENCY_KEY) was called again after
the exception and assert no further interactions with paymentRepository;
reference the existing symbols paymentRepository.save,
paymentRepository.findByIdempotencyKey, Payment.initiate, and anInitiatedPayment
when adding these interaction verifications so the test proves the
duplicate-save fallback path is exercised.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: c7cd4e5f-9cdb-4aff-bd4d-25312c443ca6
📒 Files selected for processing (13)
api-gateway-iam/api-gateway-iam/src/test/java/com/stablecoin/payments/gateway/iam/application/security/AuditLogFilterTest.javaapi-gateway-iam/api-gateway-iam/src/test/java/com/stablecoin/payments/gateway/iam/domain/service/MerchantCommandHandlerTest.javacompliance-travel-rule/compliance-travel-rule/src/test/java/com/stablecoin/payments/compliance/domain/service/ComplianceCheckCommandHandlerTest.javafx-liquidity-engine/fx-liquidity-engine/src/test/java/com/stablecoin/payments/fx/application/controller/FxQuoteControllerTest.javafx-liquidity-engine/fx-liquidity-engine/src/test/java/com/stablecoin/payments/fx/domain/service/FxRateLockCommandHandlerTest.javafx-liquidity-engine/fx-liquidity-engine/src/test/java/com/stablecoin/payments/fx/infrastructure/messaging/PaymentEventConsumerTest.javaledger-accounting/ledger-accounting/src/test/java/com/stablecoin/payments/ledger/infrastructure/messaging/LedgerOutboxEventPublisherTest.javamerchant-iam/merchant-iam/src/main/java/com/stablecoin/payments/merchant/iam/domain/team/RoleRepository.javamerchant-iam/merchant-iam/src/main/java/com/stablecoin/payments/merchant/iam/infrastructure/persistence/adapter/RoleRepositoryAdapter.javamerchant-iam/merchant-iam/src/test/java/com/stablecoin/payments/merchant/iam/domain/team/MerchantTeamServiceTest.javamerchant-onboarding/merchant-onboarding/src/test/java/com/stablecoin/payments/merchant/onboarding/application/controller/KybWebhookControllerTest.javapayment-orchestrator/payment-orchestrator/src/test/java/com/stablecoin/payments/orchestrator/application/controller/PaymentControllerTest.javapayment-orchestrator/payment-orchestrator/src/test/java/com/stablecoin/payments/orchestrator/domain/service/PaymentCommandHandlerTest.java
💤 Files with no reviewable changes (1)
- merchant-iam/merchant-iam/src/main/java/com/stablecoin/payments/merchant/iam/domain/team/RoleRepository.java
Comment on lines
+110
to
112
| given(travelRuleProvider.transmit(argThat((TravelRulePackage p) -> p != null))).willReturn("tr-ref-123"); | ||
| given(checkRepository.save(argThat((ComplianceCheck c) -> c != null))) | ||
| .willAnswer(invocation -> invocation.getArgument(0)); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate permissive non-null argThat matchers in this test class
rg -n 'argThat\(\(.*\)\s*->\s*.*!=\s*null\)' compliance-travel-rule/compliance-travel-rule/src/test/java/com/stablecoin/payments/compliance/domain/service/ComplianceCheckCommandHandlerTest.javaRepository: Puneethkumarck/stablebridge-platform
Length of output: 2798
🏁 Script executed:
#!/bin/bash
# Find TravelRulePackage class definition
fd -t f 'TravelRulePackage.java' compliance-travel-rule/Repository: Puneethkumarck/stablebridge-platform
Length of output: 210
🏁 Script executed:
#!/bin/bash
# Find ComplianceCheck class definition
fd -t f 'ComplianceCheck.java' compliance-travel-rule/Repository: Puneethkumarck/stablebridge-platform
Length of output: 208
🏁 Script executed:
#!/bin/bash
# Check for existing ArgumentCaptor usage in test files
rg -n 'ArgumentCaptor' compliance-travel-rule/ -t java --max-count 10Repository: Puneethkumarck/stablebridge-platform
Length of output: 2264
🏁 Script executed:
#!/bin/bash
# Read TravelRulePackage domain class
cat -n compliance-travel-rule/compliance-travel-rule/src/main/java/com/stablecoin/payments/compliance/domain/model/TravelRulePackage.javaRepository: Puneethkumarck/stablebridge-platform
Length of output: 734
🏁 Script executed:
#!/bin/bash
# Read ComplianceCheck domain class
cat -n compliance-travel-rule/compliance-travel-rule/src/main/java/com/stablecoin/payments/compliance/domain/model/ComplianceCheck.javaRepository: Puneethkumarck/stablebridge-platform
Length of output: 15299
🏁 Script executed:
#!/bin/bash
# Show one example of ArgumentCaptor usage in context (around line 145)
sed -n '140,155p' compliance-travel-rule/compliance-travel-rule/src/test/java/com/stablecoin/payments/compliance/domain/service/ComplianceCheckCommandHandlerTest.javaRepository: Puneethkumarck/stablebridge-platform
Length of output: 808
Replace permissive argThat matchers with ArgumentCaptor to validate compliance-critical fields.
Current matchers check only non-null status, skipping audit-trail fields. Use ArgumentCaptor.forClass() with usingRecursiveComparison().comparingOnlyFields() to assert on paymentId, senderId, recipientId, status in ComplianceCheck; and checkId, originatorVasp, beneficiaryVasp, protocolRef in TravelRulePackage. This pattern is already established at line 145+ in this test file.
Affected lines: 110–112, 138–139, 170–171, 202, 231–233, 243–245, and 17 instances total.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@compliance-travel-rule/compliance-travel-rule/src/test/java/com/stablecoin/payments/compliance/domain/service/ComplianceCheckCommandHandlerTest.java`
around lines 110 - 112, Replace permissive argThat matchers used in the mock
setups for travelRuleProvider.transmit and checkRepository.save with
ArgumentCaptor-based assertions: capture the TravelRulePackage passed to
travelRuleProvider.transmit (use
ArgumentCaptor.forClass(TravelRulePackage.class)) and assert using
usingRecursiveComparison().comparingOnlyFields("checkId","originatorVasp","beneficiaryVasp","protocolRef");
capture the ComplianceCheck passed to checkRepository.save
(ArgumentCaptor.forClass(ComplianceCheck.class)) and assert
usingRecursiveComparison().comparingOnlyFields("paymentId","senderId","recipientId","status");
update all occurrences (including the setups around travelRuleProvider.transmit
and checkRepository.save) to return the same stubbing results but validate the
captured objects instead of only checking non-null with argThat.
Comment on lines
+754
to
757
| then(kycProvider).shouldHaveNoInteractions(); | ||
| then(checkRepository).should().findByPaymentId(paymentId); | ||
| then(checkRepository).shouldHaveNoMoreInteractions(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n compliance-travel-rule/compliance-travel-rule/src/test/java/com/stablecoin/payments/compliance/domain/service/ComplianceCheckCommandHandlerTest.java | sed -n '740,770p'Repository: Puneethkumarck/stablebridge-platform
Length of output: 1659
🏁 Script executed:
find compliance-travel-rule -name "ComplianceCheckCommandHandler.java" -not -path "*Test*"Repository: Puneethkumarck/stablebridge-platform
Length of output: 224
🏁 Script executed:
cat -n compliance-travel-rule/compliance-travel-rule/src/main/java/com/stablecoin/payments/compliance/domain/service/ComplianceCheckCommandHandler.java | head -100Repository: Puneethkumarck/stablebridge-platform
Length of output: 5475
🏁 Script executed:
cat -n compliance-travel-rule/compliance-travel-rule/src/main/java/com/stablecoin/payments/compliance/domain/service/ComplianceCheckCommandHandler.java | sed -n '48,110p'Repository: Puneethkumarck/stablebridge-platform
Length of output: 3122
🏁 Script executed:
cat -n compliance-travel-rule/compliance-travel-rule/src/test/java/com/stablecoin/payments/compliance/domain/service/ComplianceCheckCommandHandlerTest.java | sed -n '1,100p'Repository: Puneethkumarck/stablebridge-platform
Length of output: 5849
🏁 Script executed:
cat -n compliance-travel-rule/compliance-travel-rule/src/main/java/com/stablecoin/payments/compliance/domain/service/ComplianceCheckCommandHandler.java | sed -n '85,140p'Repository: Puneethkumarck/stablebridge-platform
Length of output: 2859
Add no-interaction assertions for all downstream providers in duplicate-payment early-exit.
The test only blocks kycProvider and checkRepository interactions. Since DuplicatePaymentException is thrown before the compliance pipeline executes (line 55 in handler), add shouldHaveNoInteractions() assertions for sanctionsProvider, amlProvider, profileRepository, eventPublisher, and travelRuleProvider to prevent regression where refactoring might inadvertently execute compliance checks before duplicate validation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@compliance-travel-rule/compliance-travel-rule/src/test/java/com/stablecoin/payments/compliance/domain/service/ComplianceCheckCommandHandlerTest.java`
around lines 754 - 757, The test ComplianceCheckCommandHandlerTest should assert
that no downstream providers are touched when a DuplicatePaymentException
triggers the early exit; add then(sanctionsProvider).shouldHaveNoInteractions(),
then(amlProvider).shouldHaveNoInteractions(),
then(profileRepository).shouldHaveNoInteractions(),
then(eventPublisher).shouldHaveNoInteractions(), and
then(travelRuleProvider).shouldHaveNoInteractions() alongside the existing
kycProvider and checkRepository assertions in the duplicate-payment early-exit
test so any refactor that accidentally runs compliance checks before duplicate
validation will fail the test.
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.
Summary
Close the remaining gaps against
CLAUDE.md/docs/CODING_STANDARDS.mdthat were missed in the STA-243 sweep: strip leftover narration comments, remove generic Mockito matchers, promote obvious local variables tovar, replaceLoggerFactory.getLoggerwith@Slf4j, drop residual block comments, and collapse the*ApplicationServiceintermediate layer so controllers callCommandHandler/QueryHandlerdirectly. Also deletes 8 orphaned / duplicate*ApplicationServiceclasses (+ 2 duplicate test files) left behind by STA-243 or introduced in error during the handler-collapse step.Related Issue
Tracks local issue STA-249 (standards cleanup —
tasks/issues.mdseries, Linear exhausted).Type of Change
What Changed
Five commits on the branch:
19280ff—@Slf4j, drop block comments, prefervarplatform-infra/ExternalApiLoggingInterceptornow uses Lombok@Slf4jinstead ofLoggerFactory.getLogger./* */block comments from switch-default and no-op lambda bodies in Stripe / Modulr webhook validators, OFAC SDN parser, andNonceManagerPersistenceAdapter.varacross ledger, compliance, custody, fiat-on-ramp, merchant-iam, merchant-onboarding, payment-orchestrator, plusPermissionTest,JournalCommandHandlerTest,DevCustodyAdapterTest.1f48e78— drop narration comments across servicesPayoutCommandHandler(−13),TransferCommandHandler(−12),RefundCommandHandler(−12),TracingConfig(−13 architecture explainer),RiskScoringService(−9 Factor labels),CollectionCommandHandler,PaymentEventConsumer,MerchantTeam(−6 Invariant labels),LockService,MerchantOnboardingWorkflowImpl,PaymentWorkflowImpl,FxRateLockApplicationService.orphanRemovalordering,pg_advisory_xact_locksemantics, PII masking rationale, dev-mode keypair note, chain reorg grace window, crash/resubmit idempotency invariants.d411ccd— replace Mockito generic matchers with literal valuesany(),anyString(),anyLong(),anyInt(),anyBoolean(),anyList(), andeq(...)from 30 unit test files.save(...)verifications useeqIgnoringTimestamps/eqIgnoringfromplatform-test/TestUtils.Durationcomputed at revoke time) we use typedargThat(lambda)— the same escape hatcheqIgnoringitself is built on.aa89f6e— collapse ApplicationService layer into CommandHandlersComplianceCheckApplicationService(thin pass-through);ComplianceCheckControllerandCustomerRiskProfileControllernow injectComplianceCheckCommandHandler+ComplianceCheckResponseMapperdirectly. Tests rewritten to mock the handler.FxQuoteCommandHandler,FxRateLockCommandHandler, andLiquidityPoolQueryHandlerunderdomain/service/. Handlers take primitives and return domain objects (+ idempotency-state records), soHexagonalArchitectureRules.domainShouldNotDependOnApplicationstays green. Controllers own response mapping viaFxResponseMapper(added aCorridorSnapshotoverload). DeletedLiquidityPoolApplicationService+ its test. New per-handler unit tests preserve intent.*ApplicationServiceclasses left over from STA-243 —AuthApplicationService,OAuthClientApplicationService,MerchantApplicationService(api-gateway-iam), andMerchantApplicationService(merchant-onboarding).dbb9556— delete orphan ApplicationService classes missed in collapse (code-review follow-up)@Serviceclasses instead of replacing them, and leftApiKeyApplicationServicein place despite the description claiming deletion. Self-review caught this.ApiKeyApplicationService.java(api-gateway-iam),FxQuoteApplicationService.java,FxRateLockApplicationService.java, and their two duplicate test filesFxQuoteApplicationServiceTest.java/FxRateLockApplicationServiceTest.java.−683lines; Rule 3 now fully resolved across all services.Total removed by the handler-collapse work: 8
*ApplicationServiceclasses (1 compliance pass-through, 3 fx duplicates/orphans, 4 api-gateway dead code) + 1 merchant-onboarding dead service + 2 duplicate test files + 1 corresponding ApplicationService test for compliance. Net effect across the branch: ~100 files touched, final diff approximately +1,787 / −2,464.How Was It Tested
FxQuoteCommandHandlerTest,FxRateLockCommandHandlerTest,LiquidityPoolQueryHandlerTest)Verification commands actually run on this branch:
./gradlew compileJava compileTestJava— BUILD SUCCESSFUL (all modules)./gradlew test -x :phase2-integration-tests:test -x :phase3-integration-tests:test— BUILD SUCCESSFUL, 128 actionable tasks, zero failures./gradlew :api-gateway-iam:api-gateway-iam:test :fx-liquidity-engine:fx-liquidity-engine:test --rerun-tasks— BUILD SUCCESSFULBreaking Changes
N/A — public HTTP API surfaces on all 5 refactored controllers are unchanged (same paths, same request/response types). Internal class renames in
fx-liquidity-engineare not part of any published client.Security Considerations
Checklist
./gradlew spotlessCheckpasses (spotlessApply ran automatically during each commit via pre-commit hook)CHANGELOG.mdupdated — N/A, no user-facing change@Serviceduplicates during review, fixed indbb9556)Out of Scope / Known
@Autowiredfield injection in@SpringBootTestretry-test classes (22 uses across 11 files) was deliberately left alone. TheCLAUDE.mdrule reads as production-style and these are Spring's standard test-slice idioms. Can be converted to@TestConstructor(autowireMode = ALL)in a follow-up if the team wants.FxQuoteCommandHandler.getQuote(read-then-write pattern on cache miss) was inherited from the oldFxQuoteApplicationServiceand not fixed here. File a separate ticket if it matters.LiquidityPoolQueryHandler.CorridorSnapshot.rateis intentionally nullable (handled byFxResponseMapper:95); worth adding@Nullablein a follow-up for documentation.Summary by CodeRabbit