feat(s3): application controllers + CollectionCommandHandler (STA-127)#135
Merged
Merged
Conversation
Add REST controllers and domain command handler for the S3 Fiat On-Ramp service collection order lifecycle. Fix CollectionCommandHandlerTest stubbing to ignore random collectionId/pspTxnId fields. - CollectionRequest DTO with Jakarta validation (fiat-on-ramp-api) - CollectionCommandHandler: initiate (idempotent), get by ID, get by paymentId - CollectionResult wrapper for 201 vs 200 status distinction - CollectionController: POST/GET endpoints for collections + refund - GlobalExceptionHandler: maps domain exceptions to HTTP responses - FiatOnRampClient: Feign client with initiateCollection method - 6 unit tests (interaction-verification, BDD style) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
WalkthroughThis PR introduces collection order lifecycle management, featuring a REST API with idempotent POST/GET endpoints, a domain service orchestrating collection initiation with PSP integration, Feign client binding, centralized exception handling, and result wrapping for 201/200 response differentiation. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Controller as CollectionController
participant Handler as CollectionCommandHandler
participant Repo as CollectionOrderRepository
participant PSP as PspGateway
participant TxnRepo as PspTransactionRepository
participant Pub as CollectionEventPublisher
Client->>Controller: POST /v1/collections (CollectionRequest)
Controller->>Handler: initiateCollection(paymentId, ...)
Handler->>Repo: findByPaymentId(paymentId)
alt Order exists (idempotent replay)
Repo-->>Handler: existing CollectionOrder
Handler-->>Controller: CollectionResult(order, false)
else New order
Repo-->>Handler: empty
Handler->>Handler: create CollectionOrder
Handler->>PSP: initiate(paymentRail, pspId, ...)
PSP-->>Handler: PspInitiation response
Handler->>TxnRepo: save PspTransaction
Handler->>Pub: publishEvent(CollectionInitiatedEvent)
Handler->>Repo: save CollectionOrder
Handler-->>Controller: CollectionResult(order, true)
end
Controller-->>Client: 201 Created / 200 OK (CollectionResponse)
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Suggested Labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@fiat-on-ramp/fiat-on-ramp-api/src/main/java/com/stablecoin/payments/onramp/api/CollectionRequest.java`:
- Line 15: The DTO currently has paymentRailType and senderAccountType as
`@NotBlank` Strings and the controller converts them via valueOf(), causing raw
IllegalArgumentException on invalid values; change the validation so invalid
enum values produce a clear 400. Either (preferred) change CollectionRequest to
use the actual enum types (e.g., PaymentRailType paymentRailType,
SenderAccountType senderAccountType) so Spring will reject bad values before
reaching the controller, or add a custom constraint annotation (e.g.,
`@ValueOfEnum`) on the String fields and implement a ConstraintValidator that
checks against the enum and returns a descriptive violation message; also remove
or stop relying on direct Enum.valueOf() in the controller to avoid throwing
IllegalArgumentException.
In
`@fiat-on-ramp/fiat-on-ramp/src/main/java/com/stablecoin/payments/onramp/application/controller/CollectionController.java`:
- Around line 117-128: toCollectionResponse currently forwards possibly-null
values (pspReference, pspSettledAt, expiresAt) from CollectionOrder into the
CollectionResponse; ensure null-safety by either (A) updating the
CollectionResponse constructor/fields to accept nullable values or Optional
types, or (B) defensively mapping nulls before constructing the response (e.g.,
Optional.ofNullable(order.pspReference()),
Optional.ofNullable(order.pspSettledAt()),
Optional.ofNullable(order.expiresAt()) or sensible default values). Locate the
toCollectionResponse method and the CollectionResponse class/constructor to make
the change so the API model no longer assumes non-null inputs and avoids NPEs.
In
`@fiat-on-ramp/fiat-on-ramp/src/main/java/com/stablecoin/payments/onramp/application/controller/GlobalExceptionHandler.java`:
- Around line 86-98: The handlers in GlobalExceptionHandler (methods
handleIllegalState and handleIllegalArgument) currently return raw
ex.getMessage() which can leak internal details; instead either map these broad
exceptions to domain-specific exceptions (e.g., InvalidCollectionStateException)
and handle those, or sanitize the message before returning to the client by
replacing ex.getMessage() with a generic, non-sensitive message (e.g., "Invalid
request" or "Conflict occurred") when calling ApiError.of, and log the full
exception detail internally (using log.debug/error) for diagnostics; update both
handleIllegalState and handleIllegalArgument to use sanitized messages or new
domain exceptions while keeping ApiError.of usage for structured responses.
In
`@fiat-on-ramp/fiat-on-ramp/src/main/java/com/stablecoin/payments/onramp/domain/service/CollectionCommandHandler.java`:
- Around line 67-77: The PSP is called before the order is persisted, which can
cause duplicate charges if later DB persistence fails; change the flow so
CollectionOrder.initiate(...) is persisted in PENDING state before calling
pspGateway.initiatePayment(...), then apply order = order.initiatePayment() and
order = order.awaitConfirmation(...) and update the persisted record atomically;
additionally ensure the PSP call uses a stable idempotency key (use paymentId
rather than collectionId) when invoking initiatePayment to allow safe retries
and/or add a compensation/reconciliation hook for failures that occur after the
PSP call.
In
`@fiat-on-ramp/fiat-on-ramp/src/test/java/com/stablecoin/payments/onramp/domain/service/CollectionCommandHandlerTest.java`:
- Around line 107-108: The test currently calls
handler.initiateCollection(PAYMENT_ID, CORRELATION_ID, amount, paymentRail, psp,
senderAccount) and ignores its return; update the test to capture the result,
assert result.created() == true, and assert the returned order fields (e.g.,
order.getPaymentId(), order.getAmount(), order.getPaymentRail(), order.getPsp(),
order.getSenderAccount(), and any expected status) match the inputs/expected
values. Apply the same pattern in the idempotency test that calls
initiateCollection (assert created flag and order properties when applicable)
and in the retrieval tests (capture the returned order from the retrieval method
and assert its properties and any created/updated flags match expectations).
Ensure you reference the initiateCollection return type and use its created()
method and the order getters to perform assertions.
- Around line 91-99: The test builds expectedEvent using Instant.now() (new
CollectionInitiatedEvent(..., Instant.now())) which causes timestamp mismatch
when comparing to the handler-produced event; replace the current assertion that
uses eqIgnoring(expectedEvent, "collectionId") with the test utility
eqIgnoringTimestamps(expectedEvent) so the timestamp differences are
ignored—locate the expectedEvent construction and the assertion that references
eqIgnoring(...) and swap to eqIgnoringTimestamps(expectedEvent) (use the
existing eqIgnoringTimestamps helper already present in tests).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a7d38972-5da6-4941-8975-d6c5e4185807
📒 Files selected for processing (7)
fiat-on-ramp/fiat-on-ramp-api/src/main/java/com/stablecoin/payments/onramp/api/CollectionRequest.javafiat-on-ramp/fiat-on-ramp-client/src/main/java/com/stablecoin/payments/onramp/client/FiatOnRampClient.javafiat-on-ramp/fiat-on-ramp/src/main/java/com/stablecoin/payments/onramp/application/controller/CollectionController.javafiat-on-ramp/fiat-on-ramp/src/main/java/com/stablecoin/payments/onramp/application/controller/GlobalExceptionHandler.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/CollectionResult.javafiat-on-ramp/fiat-on-ramp/src/test/java/com/stablecoin/payments/onramp/domain/service/CollectionCommandHandlerTest.java
| @NotNull UUID correlationId, | ||
| @NotNull @Positive BigDecimal amount, | ||
| @NotBlank String currency, | ||
| @NotBlank String paymentRailType, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Enum validation at controller layer throws raw IllegalArgumentException.
paymentRailType and senderAccountType (line 22) are validated as @NotBlank but converted via valueOf() in the controller. Invalid enum values throw IllegalArgumentException, which GlobalExceptionHandler catches and returns 400 with a generic message. Consider using a custom validator or documenting the accepted values in the API contract.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@fiat-on-ramp/fiat-on-ramp-api/src/main/java/com/stablecoin/payments/onramp/api/CollectionRequest.java`
at line 15, The DTO currently has paymentRailType and senderAccountType as
`@NotBlank` Strings and the controller converts them via valueOf(), causing raw
IllegalArgumentException on invalid values; change the validation so invalid
enum values produce a clear 400. Either (preferred) change CollectionRequest to
use the actual enum types (e.g., PaymentRailType paymentRailType,
SenderAccountType senderAccountType) so Spring will reject bad values before
reaching the controller, or add a custom constraint annotation (e.g.,
`@ValueOfEnum`) on the String fields and implement a ConstraintValidator that
checks against the enum and returns a descriptive violation message; also remove
or stop relying on direct Enum.valueOf() in the controller to avoid throwing
IllegalArgumentException.
Comment on lines
+117
to
+128
| private CollectionResponse toCollectionResponse(CollectionOrder order) { | ||
| return new CollectionResponse( | ||
| order.collectionId(), | ||
| order.paymentId(), | ||
| order.status().name(), | ||
| order.paymentRail().rail().name(), | ||
| order.psp().pspName(), | ||
| order.pspReference(), | ||
| order.pspSettledAt(), | ||
| order.createdAt(), | ||
| order.expiresAt()); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider null-safety on optional fields.
pspReference, pspSettledAt, and expiresAt may be null depending on order state. If CollectionResponse requires non-null values for certain fields, consider defensive handling. Current implementation correctly passes nulls for nullable response fields.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@fiat-on-ramp/fiat-on-ramp/src/main/java/com/stablecoin/payments/onramp/application/controller/CollectionController.java`
around lines 117 - 128, toCollectionResponse currently forwards possibly-null
values (pspReference, pspSettledAt, expiresAt) from CollectionOrder into the
CollectionResponse; ensure null-safety by either (A) updating the
CollectionResponse constructor/fields to accept nullable values or Optional
types, or (B) defensively mapping nulls before constructing the response (e.g.,
Optional.ofNullable(order.pspReference()),
Optional.ofNullable(order.pspSettledAt()),
Optional.ofNullable(order.expiresAt()) or sensible default values). Locate the
toCollectionResponse method and the CollectionResponse class/constructor to make
the change so the API model no longer assumes non-null inputs and avoids NPEs.
Comment on lines
+86
to
+98
| @ResponseStatus(CONFLICT) | ||
| @ExceptionHandler(IllegalStateException.class) | ||
| public ApiError handleIllegalState(IllegalStateException ex) { | ||
| log.info("Invalid state transition: {}", ex.getMessage()); | ||
| return ApiError.of("OR-0002", CONFLICT.getReasonPhrase(), ex.getMessage()); | ||
| } | ||
|
|
||
| @ResponseStatus(BAD_REQUEST) | ||
| @ExceptionHandler(IllegalArgumentException.class) | ||
| public ApiError handleIllegalArgument(IllegalArgumentException ex) { | ||
| log.info("Illegal argument: {}", ex.getMessage()); | ||
| return ApiError.of("OR-0001", BAD_REQUEST.getReasonPhrase(), ex.getMessage()); | ||
| } |
There was a problem hiding this comment.
Potential information leakage via generic exception messages.
IllegalStateException and IllegalArgumentException are broad JDK exceptions thrown by frameworks, libraries, and internal code. Exposing ex.getMessage() directly can leak implementation details (class names, internal state, parameter values).
Consider:
- Replace with dedicated domain exceptions for controlled messaging.
- If catching these is necessary, sanitize or replace messages before returning to client.
🛡️ Safer alternative with generic client messages
`@ResponseStatus`(CONFLICT)
`@ExceptionHandler`(IllegalStateException.class)
public ApiError handleIllegalState(IllegalStateException ex) {
- log.info("Invalid state transition: {}", ex.getMessage());
- return ApiError.of("OR-0002", CONFLICT.getReasonPhrase(), ex.getMessage());
+ log.warn("Invalid state transition: {}", ex.getMessage());
+ return ApiError.of("OR-0002", CONFLICT.getReasonPhrase(),
+ "Operation not allowed in current state");
}
`@ResponseStatus`(BAD_REQUEST)
`@ExceptionHandler`(IllegalArgumentException.class)
public ApiError handleIllegalArgument(IllegalArgumentException ex) {
- log.info("Illegal argument: {}", ex.getMessage());
- return ApiError.of("OR-0001", BAD_REQUEST.getReasonPhrase(), ex.getMessage());
+ log.warn("Illegal argument: {}", ex.getMessage());
+ return ApiError.of("OR-0001", BAD_REQUEST.getReasonPhrase(),
+ "Invalid request parameters");
}Alternatively, introduce domain-specific exceptions (e.g., InvalidCollectionStateException) with controlled messages and remove these broad catch-all handlers.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @ResponseStatus(CONFLICT) | |
| @ExceptionHandler(IllegalStateException.class) | |
| public ApiError handleIllegalState(IllegalStateException ex) { | |
| log.info("Invalid state transition: {}", ex.getMessage()); | |
| return ApiError.of("OR-0002", CONFLICT.getReasonPhrase(), ex.getMessage()); | |
| } | |
| @ResponseStatus(BAD_REQUEST) | |
| @ExceptionHandler(IllegalArgumentException.class) | |
| public ApiError handleIllegalArgument(IllegalArgumentException ex) { | |
| log.info("Illegal argument: {}", ex.getMessage()); | |
| return ApiError.of("OR-0001", BAD_REQUEST.getReasonPhrase(), ex.getMessage()); | |
| } | |
| `@ResponseStatus`(CONFLICT) | |
| `@ExceptionHandler`(IllegalStateException.class) | |
| public ApiError handleIllegalState(IllegalStateException ex) { | |
| log.warn("Invalid state transition: {}", ex.getMessage()); | |
| return ApiError.of("OR-0002", CONFLICT.getReasonPhrase(), | |
| "Operation not allowed in current state"); | |
| } | |
| `@ResponseStatus`(BAD_REQUEST) | |
| `@ExceptionHandler`(IllegalArgumentException.class) | |
| public ApiError handleIllegalArgument(IllegalArgumentException ex) { | |
| log.warn("Illegal argument: {}", ex.getMessage()); | |
| return ApiError.of("OR-0001", BAD_REQUEST.getReasonPhrase(), | |
| "Invalid request parameters"); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@fiat-on-ramp/fiat-on-ramp/src/main/java/com/stablecoin/payments/onramp/application/controller/GlobalExceptionHandler.java`
around lines 86 - 98, The handlers in GlobalExceptionHandler (methods
handleIllegalState and handleIllegalArgument) currently return raw
ex.getMessage() which can leak internal details; instead either map these broad
exceptions to domain-specific exceptions (e.g., InvalidCollectionStateException)
and handle those, or sanitize the message before returning to the client by
replacing ex.getMessage() with a generic, non-sensitive message (e.g., "Invalid
request" or "Conflict occurred") when calling ApiError.of, and log the full
exception detail internally (using log.debug/error) for diagnostics; update both
handleIllegalState and handleIllegalArgument to use sanitized messages or new
domain exceptions while keeping ApiError.of usage for structured responses.
Comment on lines
+67
to
+77
| // 2. Create new collection order in PENDING state | ||
| var order = CollectionOrder.initiate(paymentId, correlationId, amount, paymentRail, psp, senderAccount); | ||
|
|
||
| // 3. Call PSP to initiate payment | ||
| var pspResult = pspGateway.initiatePayment(new PspPaymentRequest( | ||
| order.collectionId(), amount, paymentRail, senderAccount, psp.pspName())); | ||
|
|
||
| // 4. Transition: PENDING -> PAYMENT_INITIATED -> AWAITING_CONFIRMATION | ||
| order = order.initiatePayment(); | ||
| order = order.awaitConfirmation(pspResult.pspReference()); | ||
|
|
There was a problem hiding this comment.
Non-idempotent PSP call before persistence risks duplicate payments.
If the PSP call (line 71) succeeds but the transaction fails later (e.g., DB constraint violation at line 102), the payment is initiated at the PSP but not recorded locally. On retry, a new collectionId is generated (line 68), bypassing the paymentId idempotency check because nothing was saved. This could result in duplicate PSP charges.
Consider:
- Persist the order in PENDING state first, then call PSP — rollback semantics become clearer
- Use an idempotency key with the PSP (e.g.,
paymentId) rather thancollectionId - Implement a compensation/reconciliation mechanism
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@fiat-on-ramp/fiat-on-ramp/src/main/java/com/stablecoin/payments/onramp/domain/service/CollectionCommandHandler.java`
around lines 67 - 77, The PSP is called before the order is persisted, which can
cause duplicate charges if later DB persistence fails; change the flow so
CollectionOrder.initiate(...) is persisted in PENDING state before calling
pspGateway.initiatePayment(...), then apply order = order.initiatePayment() and
order = order.awaitConfirmation(...) and update the persisted record atomically;
additionally ensure the PSP call uses a stable idempotency key (use paymentId
rather than collectionId) when invoking initiatePayment to allow safe retries
and/or add a compensation/reconciliation hook for failures that occur after the
PSP call.
Comment on lines
+91
to
+99
| var expectedEvent = new CollectionInitiatedEvent( | ||
| expectedOrder.collectionId(), | ||
| PAYMENT_ID, | ||
| CORRELATION_ID, | ||
| amount.amount(), | ||
| amount.currency(), | ||
| paymentRail.rail().name(), | ||
| psp.pspName(), | ||
| Instant.now()); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if eqIgnoring handles timestamp fields or if there's a dedicated timestamp-ignoring matcher
ast-grep --pattern 'eqIgnoring($_, $_)'
rg -n "eqIgnoring" --type java -A5 -B2Repository: Puneethkumarck/stablebridge-platform
Length of output: 50393
Use eqIgnoringTimestamps() to avoid test flakiness.
Line 113 calls eqIgnoring(expectedEvent, "collectionId"), which only ignores the collectionId field. Since expectedEvent is constructed with Instant.now() at line 91 but the handler generates a fresh timestamp at execution time, the assertion will fail due to timestamp mismatch. Replace with eqIgnoringTimestamps(expectedEvent), which is available in the project's test utilities and already used for this exact purpose elsewhere.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@fiat-on-ramp/fiat-on-ramp/src/test/java/com/stablecoin/payments/onramp/domain/service/CollectionCommandHandlerTest.java`
around lines 91 - 99, The test builds expectedEvent using Instant.now() (new
CollectionInitiatedEvent(..., Instant.now())) which causes timestamp mismatch
when comparing to the handler-produced event; replace the current assertion that
uses eqIgnoring(expectedEvent, "collectionId") with the test utility
eqIgnoringTimestamps(expectedEvent) so the timestamp differences are
ignored—locate the expectedEvent construction and the assertion that references
eqIgnoring(...) and swap to eqIgnoringTimestamps(expectedEvent) (use the
existing eqIgnoringTimestamps helper already present in tests).
Comment on lines
+107
to
+108
| // when | ||
| handler.initiateCollection(PAYMENT_ID, CORRELATION_ID, amount, paymentRail, psp, senderAccount); |
There was a problem hiding this comment.
Missing return value assertions.
The test invokes initiateCollection but discards the result. Verify result.created() == true and that the returned order has expected properties. Same pattern applies to the idempotency test (line 129) and retrieval tests (lines 153, 187).
Proposed fix for this test
// when
- handler.initiateCollection(PAYMENT_ID, CORRELATION_ID, amount, paymentRail, psp, senderAccount);
+ var result = handler.initiateCollection(PAYMENT_ID, CORRELATION_ID, amount, paymentRail, psp, senderAccount);
// then
+ assertThat(result.created()).isTrue();
+ assertThat(result.order().paymentId()).isEqualTo(PAYMENT_ID);
then(collectionOrderRepository).should().save(eqIgnoring(expectedOrder, "collectionId"));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@fiat-on-ramp/fiat-on-ramp/src/test/java/com/stablecoin/payments/onramp/domain/service/CollectionCommandHandlerTest.java`
around lines 107 - 108, The test currently calls
handler.initiateCollection(PAYMENT_ID, CORRELATION_ID, amount, paymentRail, psp,
senderAccount) and ignores its return; update the test to capture the result,
assert result.created() == true, and assert the returned order fields (e.g.,
order.getPaymentId(), order.getAmount(), order.getPaymentRail(), order.getPsp(),
order.getSenderAccount(), and any expected status) match the inputs/expected
values. Apply the same pattern in the idempotency test that calls
initiateCollection (assert created flag and order properties when applicable)
and in the retrieval tests (capture the returned order from the retrieval method
and assert its properties and any created/updated flags match expectations).
Ensure you reference the initiateCollection return type and use its created()
method and the order getters to perform assertions.
This was referenced Mar 9, 2026
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
domain/service/— initiates collection (idempotent by paymentId), PSP call, state transitions (PENDING → PAYMENT_INITIATED → AWAITING_CONFIRMATION), PspTransaction recording, outbox event publishingPOST /v1/collections(201/200 idempotent),GET /v1/collections/{id},GET /v1/collections?paymentId=,POST /v1/collections/{id}/refundsinitiateCollectionmethodTest plan
eqIgnoringmatchersCloses STA-127
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Tests