feat(s4): application controllers + TransferCommandHandler (STA-143)#147
Merged
Puneethkumarck merged 3 commits intoMar 9, 2026
Merged
Conversation
Puneethkumarck
added
phase-3
REST API layer and domain command handler for blockchain transfer orchestration:
- TransferController: POST /v1/transfers (202/200 idempotent), GET /v1/transfers/{id}, GET /v1/wallets/{id}/balance
- TransferCommandHandler: chain selection → balance reservation → nonce → custody signing → submit pipeline
- GlobalExceptionHandler: BC-XXXX error codes (1001 insufficient balance, 1003 not found, 1004 signing, 1005 wallet)
- CustodyOutboxEventPublisher + CustodyOutboxHandler: Namastack outbox for TransferSubmittedEvent
- TransferRequest DTO with jakarta validation, TransferResult wrapper, Feign client update
- 21 new tests (11 handler + 10 controller), 396 total S4 tests green
Closes STA-143
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Puneethkumarck
force-pushed
the
feature/STA-143-s4-controllers-transfer-command-handler
branch
from
874d0c2 to
a840c87
Compare
|
Caution Review failedPull request was closed or merged during review WalkthroughAdds a complete transfer submission flow: API model (TransferRequest), Feign client method, REST controller, TransferCommandHandler orchestration, domain exceptions/results, outbox-based event publishing and Kafka handler, global exception mapping, and comprehensive unit & controller tests. Changes
Sequence DiagramsequenceDiagram
participant Client
participant TransferController
participant TransferCommandHandler
participant ChainSelectionEngine
participant WalletRepository
participant CustodyEngine
participant Outbox
participant KafkaPublisher
Client->>TransferController: POST /v1/transfers (TransferRequest)
TransferController->>TransferCommandHandler: initiateTransfer(...)
TransferCommandHandler->>TransferCommandHandler: Check idempotency by paymentId
alt Idempotent replay
TransferCommandHandler-->>TransferController: TransferResult(created=false)
else New transfer
TransferCommandHandler->>ChainSelectionEngine: selectChain(...)
ChainSelectionEngine-->>TransferCommandHandler: ChainSelectionResult
TransferCommandHandler->>WalletRepository: findActiveWallet(...)
WalletRepository-->>TransferCommandHandler: Wallet
TransferCommandHandler->>TransferCommandHandler: Reserve balance (pessimistic lock)
TransferCommandHandler->>CustodyEngine: signAndSubmit(chainTransfer)
alt Signing success
CustodyEngine-->>TransferCommandHandler: txHash
TransferCommandHandler->>Outbox: schedule(TransferSubmittedEvent)
Outbox-->>TransferCommandHandler: scheduled
else Signing failure
CustodyEngine-->>TransferCommandHandler: exception
TransferCommandHandler->>TransferCommandHandler: release reserved balance
TransferCommandHandler-->>TransferController: throw CustodySigningException
end
TransferCommandHandler->>TransferCommandHandler: persist transfer state
TransferCommandHandler-->>TransferController: TransferResult(created=true)
end
Outbox->>KafkaPublisher: publish(event,key)
KafkaPublisher-->>Outbox: ack
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 |
- Remove assertThat on handler return values — use interaction verification only - Fix single-assert pattern in getWalletBalance test (build expected + recursive compare) - Move aSelectionResult() factory to ChainSelectionFixtures (shared testFixtures) - Add ERROR_CODE constant to ChainUnavailableException, use in GlobalExceptionHandler - Fix inline FQNs in WalletBalanceDetails record (Wallet, List imports) - Fix inline java.time.Instant FQN in TransferController Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@blockchain-custody/blockchain-custody-api/src/main/java/com/stablecoin/payments/custody/api/TransferRequest.java`:
- Around line 24-26: The amount field in TransferRequest currently uses
`@NotBlank` and `@DecimalMin` on a String which yields misleading errors for
non-numeric input; add a `@Pattern`(regexp="^\\d+(\\.\\d+)?$", message="amount
must be a valid decimal number") on the amount field (keeping the existing
`@NotBlank` and `@DecimalMin`) so malformed values (e.g., "abc") produce a clear
format validation error; update the annotation list on the amount property in
TransferRequest accordingly.
- Around line 28-29: The TransferRequest DTO's toWalletAddress field only has
`@NotBlank`; add basic format validation to reject obvious garbage by applying a
validation constraint (e.g., a `@Pattern` or a custom validator) on the
TransferRequest.toWalletAddress field that enforces sensible character and
length rules for blockchain addresses (e.g., hex prefix optional, allowed hex
chars and length range, or alphanumeric constraints for non-hex chains); ensure
the new validator message is descriptive and note in the change that downstream
chain-specific validation must still run before submission (so keep downstream
validation intact).
In
`@blockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/application/controller/GlobalExceptionHandler.java`:
- Around line 87-93: The handler handleCustodySigning in GlobalExceptionHandler
currently logs ex.getMessage(), which may leak custody-sensitive details; change
the logging to avoid raw exception messages by logging only a sanitized summary
such as the exception class name and a short safe context (e.g.
CustodySigningException or ex.getClass().getSimpleName()) or a fixed message via
log.error("Custody signing error: {}", ex.getClass().getSimpleName()) and ensure
the ApiError returned (from ApiError.of using
CustodySigningException.ERROR_CODE) does not include any sensitive details from
ex.getMessage().
In
`@blockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/application/controller/TransferController.java`:
- Around line 109-116: The estimatedConfirmationSeconds method currently
hardcodes chain times; replace it by injecting a configuration-backed
Map<String,Integer> (e.g., a `@ConfigurationProperties` class like
ChainConfirmationsProperties with a Map<String,Integer> confirmations) into
TransferController and change estimatedConfirmationSeconds(String chainId) to
look up chainId in that map (with a sensible default/fallback when absent).
Add/define the properties prefix and YAML/props entries for existing chains,
validate/convert values to Integer as needed, and update any callers to use the
new lookup method in TransferController.
In
`@blockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/domain/service/TransferCommandHandler.java`:
- Around line 194-202: The catch in releaseBalance currently swallows all
exceptions from reservedBalance.release(...) or
walletBalanceRepository.save(...), risking permanent locked funds; change it to
surface failures by removing silent swallowing and either rethrowing the caught
exception (or wrap and throw a domain-specific exception) from releaseBalance so
the transaction fails, and/or invoke a compensation/alert mechanism (e.g.,
enqueue a retry/compensation job or call an alerting service) when
walletBalanceRepository.save(...) fails; locate the releaseBalance method in
TransferCommandHandler and adjust error handling around reservedBalance.release
and walletBalanceRepository.save accordingly.
In
`@blockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/infrastructure/messaging/CustodyOutboxEventPublisher.java`:
- Around line 31-45: The resolveKey method currently uses reflection and
String.valueOf(...) which converts null to the string "null", catches broad
Exception types, and discards the first exception; update resolveKey to (1)
explicitly look up and invoke the accessor methods "paymentId" and then
"transferId" and check the returned value for null (throw
IllegalArgumentException if accessor returns null rather than producing the
literal "null"), (2) catch and handle specific reflection exceptions
(NoSuchMethodException, IllegalAccessException, InvocationTargetException),
unwrap InvocationTargetException to surface the underlying cause, and (3) when
falling back from the paymentId attempt to transferId, add the first exception
(e1) as a suppressed exception to the final IllegalArgumentException so
diagnostics include both failures; keep the method name resolveKey and the same
failure message but include suppressed/underlying exceptions for clarity.
In
`@blockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/infrastructure/messaging/CustodyOutboxHandler.java`:
- Around line 27-35: The catch-all in CustodyOutboxHandler around
kafkaTemplate.send(...).get(...) swallows InterruptedException; update the
exception handling to catch InterruptedException separately, call
Thread.currentThread().interrupt() to restore the thread's interrupt status, log
the interruption (referencing kafkaTemplate.send,
event.getClass().getSimpleName(), topic, key), and then rethrow/wrap the
InterruptedException as the cause in the RuntimeException (as currently done for
other exceptions) so interruption is not lost; keep the existing broad Exception
catch for other failures but ensure InterruptedException handling is prioritized
and preserves the original exception as the cause.
In
`@blockchain-custody/blockchain-custody/src/test/java/com/stablecoin/payments/custody/application/controller/TransferControllerTest.java`:
- Around line 52-173: Add a test in TransferControllerTest that mocks
transferCommandHandler.initiateTransfer to throw new
ChainUnavailableException(...) and asserts the controller returns HTTP 503 and
error code "BC-1002"; specifically create a test similar to existing ones (e.g.,
shouldReturn503ForChainUnavailable) that calls mockMvc.post("/v1/transfers")
with the same JSON payload used in other tests, uses
given(transferCommandHandler.initiateTransfer(...)).willThrow(new
ChainUnavailableException(...)), and asserts status().isServiceUnavailable() and
jsonPath("$.code").value("BC-1002").
In
`@blockchain-custody/blockchain-custody/src/testFixtures/java/com/stablecoin/payments/custody/fixtures/ChainSelectionFixtures.java`:
- Around line 90-96: The fixture aBaseSelectionResult() creates a
non-deterministic transferId with UUID.randomUUID(), causing inconsistent test
behavior; change it to use the deterministic TRANSFER_ID constant (same as used
by aSelectionRequest()) when building the ChainSelectionResult (i.e., replace
the transferId builder argument to use TRANSFER_ID) so all fixtures share the
same stable ID.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: bc5f67bf-94fe-45a9-a277-23e2d289db83
📒 Files selected for processing (17)
blockchain-custody/blockchain-custody-api/src/main/java/com/stablecoin/payments/custody/api/TransferRequest.javablockchain-custody/blockchain-custody-client/src/main/java/com/stablecoin/payments/custody/client/BlockchainCustodyClient.javablockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/application/controller/GlobalExceptionHandler.javablockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/application/controller/TransferController.javablockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/domain/exception/ChainUnavailableException.javablockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/domain/exception/CustodySigningException.javablockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/domain/exception/InsufficientBalanceException.javablockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/domain/exception/TransferNotFoundException.javablockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/domain/exception/WalletNotFoundException.javablockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/domain/model/TransferResult.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/messaging/CustodyOutboxEventPublisher.javablockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/infrastructure/messaging/CustodyOutboxHandler.javablockchain-custody/blockchain-custody/src/test/java/com/stablecoin/payments/custody/application/controller/TransferControllerTest.javablockchain-custody/blockchain-custody/src/test/java/com/stablecoin/payments/custody/domain/service/TransferCommandHandlerTest.javablockchain-custody/blockchain-custody/src/testFixtures/java/com/stablecoin/payments/custody/fixtures/ChainSelectionFixtures.javablockchain-custody/blockchain-custody/src/testFixtures/java/com/stablecoin/payments/custody/fixtures/TestUtils.java
Comment on lines
+28
to
+29
| @NotBlank(message = "toWalletAddress is required") | ||
| String toWalletAddress, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
toWalletAddress lacks format validation.
Currently accepts any non-blank string. Multi-chain address validation is complex, but a basic length/character constraint could catch obvious garbage early. Verify downstream validation handles this before submission.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@blockchain-custody/blockchain-custody-api/src/main/java/com/stablecoin/payments/custody/api/TransferRequest.java`
around lines 28 - 29, The TransferRequest DTO's toWalletAddress field only has
`@NotBlank`; add basic format validation to reject obvious garbage by applying a
validation constraint (e.g., a `@Pattern` or a custom validator) on the
TransferRequest.toWalletAddress field that enforces sensible character and
length rules for blockchain addresses (e.g., hex prefix optional, allowed hex
chars and length range, or alphanumeric constraints for non-hex chains); ensure
the new validator message is descriptive and note in the change that downstream
chain-specific validation must still run before submission (so keep downstream
validation intact).
Comment on lines
+109
to
+116
| private Integer estimatedConfirmationSeconds(String chainId) { | ||
| return switch (chainId) { | ||
| case "base" -> 12; | ||
| case "ethereum" -> 300; | ||
| case "solana" -> 5; | ||
| default -> null; | ||
| }; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider externalizing chain confirmation times to configuration.
Hardcoded values work for now, but these will need updating when adding new chains or if block times change. A Map<String, Integer> from @ConfigurationProperties would be more maintainable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@blockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/application/controller/TransferController.java`
around lines 109 - 116, The estimatedConfirmationSeconds method currently
hardcodes chain times; replace it by injecting a configuration-backed
Map<String,Integer> (e.g., a `@ConfigurationProperties` class like
ChainConfirmationsProperties with a Map<String,Integer> confirmations) into
TransferController and change estimatedConfirmationSeconds(String chainId) to
look up chainId in that map (with a sensible default/fallback when absent).
Add/define the properties prefix and YAML/props entries for existing chains,
validate/convert values to Integer as needed, and update any callers to use the
new lookup method in TransferController.
Comment on lines
+31
to
+45
| private String resolveKey(Object event) { | ||
| try { | ||
| var method = event.getClass().getMethod("paymentId"); | ||
| return String.valueOf(method.invoke(event)); | ||
| } catch (Exception e1) { | ||
| try { | ||
| var method = event.getClass().getMethod("transferId"); | ||
| return String.valueOf(method.invoke(event)); | ||
| } catch (Exception e2) { | ||
| throw new IllegalArgumentException( | ||
| "Event class missing accessor for 'paymentId' or 'transferId': " | ||
| + event.getClass().getName(), e2); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Reflection-based key resolution has null-safety and exception-handling gaps.
String.valueOf(method.invoke(event))returns"null"if the accessor returnsnull, which would produce a problematic outbox key.- Catching generic
Exceptionmasks distinct failure modes (e.g.,InvocationTargetExceptionwrapping a runtime exception thrown by the accessor vs.NoSuchMethodException). e1is discarded when attempting thetransferIdfallback—consider adding it as a suppressed exception for diagnostics.
Proposed improvement for null-safety and exception clarity
private String resolveKey(Object event) {
+ Object key = tryInvoke(event, "paymentId");
+ if (key == null) {
+ key = tryInvoke(event, "transferId");
+ }
+ if (key == null) {
+ throw new IllegalArgumentException(
+ "Event class missing non-null accessor for 'paymentId' or 'transferId': "
+ + event.getClass().getName());
+ }
+ return key.toString();
+}
+
+private Object tryInvoke(Object event, String methodName) {
try {
- var method = event.getClass().getMethod("paymentId");
- return String.valueOf(method.invoke(event));
- } catch (Exception e1) {
- try {
- var method = event.getClass().getMethod("transferId");
- return String.valueOf(method.invoke(event));
- } catch (Exception e2) {
- throw new IllegalArgumentException(
- "Event class missing accessor for 'paymentId' or 'transferId': "
- + event.getClass().getName(), e2);
- }
+ var method = event.getClass().getMethod(methodName);
+ return method.invoke(event);
+ } catch (NoSuchMethodException e) {
+ return null;
+ } catch (ReflectiveOperationException e) {
+ throw new IllegalArgumentException("Failed to invoke " + methodName + " on " + event.getClass().getName(), e);
}
}📝 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
| private String resolveKey(Object event) { | |
| try { | |
| var method = event.getClass().getMethod("paymentId"); | |
| return String.valueOf(method.invoke(event)); | |
| } catch (Exception e1) { | |
| try { | |
| var method = event.getClass().getMethod("transferId"); | |
| return String.valueOf(method.invoke(event)); | |
| } catch (Exception e2) { | |
| throw new IllegalArgumentException( | |
| "Event class missing accessor for 'paymentId' or 'transferId': " | |
| + event.getClass().getName(), e2); | |
| } | |
| } | |
| } | |
| private String resolveKey(Object event) { | |
| Object key = tryInvoke(event, "paymentId"); | |
| if (key == null) { | |
| key = tryInvoke(event, "transferId"); | |
| } | |
| if (key == null) { | |
| throw new IllegalArgumentException( | |
| "Event class missing non-null accessor for 'paymentId' or 'transferId': " | |
| event.getClass().getName()); | |
| } | |
| return key.toString(); | |
| } | |
| private Object tryInvoke(Object event, String methodName) { | |
| try { | |
| var method = event.getClass().getMethod(methodName); | |
| return method.invoke(event); | |
| } catch (NoSuchMethodException e) { | |
| return null; | |
| } catch (ReflectiveOperationException e) { | |
| throw new IllegalArgumentException("Failed to invoke " + methodName + " on " + event.getClass().getName(), e); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@blockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/infrastructure/messaging/CustodyOutboxEventPublisher.java`
around lines 31 - 45, The resolveKey method currently uses reflection and
String.valueOf(...) which converts null to the string "null", catches broad
Exception types, and discards the first exception; update resolveKey to (1)
explicitly look up and invoke the accessor methods "paymentId" and then
"transferId" and check the returned value for null (throw
IllegalArgumentException if accessor returns null rather than producing the
literal "null"), (2) catch and handle specific reflection exceptions
(NoSuchMethodException, IllegalAccessException, InvocationTargetException),
unwrap InvocationTargetException to surface the underlying cause, and (3) when
falling back from the paymentId attempt to transferId, add the first exception
(e1) as a suppressed exception to the final IllegalArgumentException so
diagnostics include both failures; keep the method name resolveKey and the same
failure message but include suppressed/underlying exceptions for clarity.
- Remove silent exception swallowing in releaseBalance — let failure propagate to fail the transaction and prevent permanent balance lock - Handle InterruptedException separately in CustodyOutboxHandler — restore thread interrupt status before rethrowing - Sanitize custody signing error log — log class name instead of message which may contain vault IDs or provider details - Add @pattern validation on TransferRequest.amount for clear format error on non-numeric input - Add 503 ChainUnavailableException controller test (BC-1002) - Use deterministic TRANSFER_ID in aBaseSelectionResult() fixture Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This was referenced Mar 15, 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
POST /v1/transfers(202 Accepted / 200 OK idempotent),GET /v1/transfers/{id},GET /v1/wallets/{id}/balancesubmitTransfer()Test plan
Closes STA-143
🤖 Generated with Claude Code
Summary by CodeRabbit