feat(infra): add sandbox profiles for S1, S6, S7, S10, S13 + env template (STA-213)#245
Merged
Puneethkumarck merged 4 commits intoMar 22, 2026
Merged
Conversation
…late (STA-213) All 10 services now have application-sandbox.yml for real sandbox API testing. Includes .env.sandbox.template with all required env vars and signup URLs, FrankfurterRateAdapterSandboxTest for live API validation, and .env.sandbox in .gitignore to prevent key leaks. Closes #240, Closes #241 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Puneethkumarck
added
enhancement
|
Caution Review failedPull request was closed or merged during review WalkthroughAdds sandbox Spring profiles and a centralized Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env.sandbox.template:
- Around line 31-32: Reorder the two related environment variables so they are
alphabetically sorted: place PERSONA_INQUIRY_TEMPLATE_ID before
PERSONA_SANDBOX_API_KEY in the .env template; this satisfies dotenv-linter and
keeps related keys consistent.
In `@api-gateway-iam/api-gateway-iam/src/main/resources/application-sandbox.yml`:
- Line 12: The YAML default for JWT_PRIVATE_KEY_BASE64 is empty
(private-key-base64: ${JWT_PRIVATE_KEY_BASE64:}), which can lead to cryptic
runtime failures; either provide a documented development-only default value for
JWT_PRIVATE_KEY_BASE64 (clearly marked insecure) or add a fail-fast validation
in your JWT config loader (e.g., JwtProperties / JwtConfig /
loadJwtKey()/getPrivateKeyBase64()) that checks the resolved private-key-base64
and throws a clear exception on startup if it is missing or invalid. Ensure the
validation message references JWT_PRIVATE_KEY_BASE64/private-key-base64 so
developers know which env var to set.
In
`@fx-liquidity-engine/fx-liquidity-engine/src/test/java/com/stablecoin/payments/fx/infrastructure/provider/frankfurter/FrankfurterRateAdapterSandboxTest.java`:
- Around line 90-95: Update the test to assert the specific Spring exception
type instead of a generic Exception: replace the generic assertion in
shouldThrowOnSameCurrencyPair() so that assertThatThrownBy(() ->
adapter.getRate("USD", "USD")) checks for
org.springframework.web.client.HttpClientErrorException (the exception thrown by
RestClient.retrieve) rather than Exception.class; ensure the test imports or
fully qualifies HttpClientErrorException so the assertion targets the specific
failure mode from adapter.getRate.
In `@merchant-iam/merchant-iam/src/main/resources/application-sandbox.yml`:
- Line 12: The sandbox config currently uses an empty default for the JWT
private key ("private-key-base64: ${JWT_PRIVATE_KEY_BASE64:}"), which can hide
missing env vars; change this to fail-fast by either removing the empty default
so the JVM/property binder throws on missing env or add explicit startup
validation that checks the "private-key-base64" property (bound from
JWT_PRIVATE_KEY_BASE64) and aborts with a clear error if unset; locate where
JWT/private-key is read (config class or JwtConfig/Startup initializer) and add
the validation there to match api-gateway-iam behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7bad93d4-967e-40da-94c7-513a621babd7
📒 Files selected for processing (8)
.env.sandbox.template.gitignoreapi-gateway-iam/api-gateway-iam/src/main/resources/application-sandbox.ymlfx-liquidity-engine/fx-liquidity-engine/src/main/resources/application-sandbox.ymlfx-liquidity-engine/fx-liquidity-engine/src/test/java/com/stablecoin/payments/fx/infrastructure/provider/frankfurter/FrankfurterRateAdapterSandboxTest.javaledger-accounting/ledger-accounting/src/main/resources/application-sandbox.ymlmerchant-iam/merchant-iam/src/main/resources/application-sandbox.ymlpayment-orchestrator/payment-orchestrator/src/main/resources/application-sandbox.yml
Comment on lines
+31
to
+32
| PERSONA_SANDBOX_API_KEY=persona_sandbox_your_key_here | ||
| PERSONA_INQUIRY_TEMPLATE_ID=itmpl_your_template_id_here |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor: Consider alphabetical ordering of related keys.
dotenv-linter suggests PERSONA_INQUIRY_TEMPLATE_ID should precede PERSONA_SANDBOX_API_KEY for consistency. Low priority.
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 32-32: [UnorderedKey] The PERSONA_INQUIRY_TEMPLATE_ID key should go before the PERSONA_SANDBOX_API_KEY key
(UnorderedKey)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.env.sandbox.template around lines 31 - 32, Reorder the two related
environment variables so they are alphabetically sorted: place
PERSONA_INQUIRY_TEMPLATE_ID before PERSONA_SANDBOX_API_KEY in the .env template;
this satisfies dotenv-linter and keeps related keys consistent.
|
|
||
| api-gateway-iam: | ||
| jwt: | ||
| private-key-base64: ${JWT_PRIVATE_KEY_BASE64:} |
There was a problem hiding this comment.
Empty default for JWT private key may cause unclear startup failures.
${JWT_PRIVATE_KEY_BASE64:} defaults to empty string. If the developer forgets to set this env var, token signing will fail at runtime with a potentially cryptic error. Consider defaulting to a well-known development-only key (clearly documented as insecure) or fail fast with a validation check.
🤖 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/main/resources/application-sandbox.yml`
at line 12, The YAML default for JWT_PRIVATE_KEY_BASE64 is empty
(private-key-base64: ${JWT_PRIVATE_KEY_BASE64:}), which can lead to cryptic
runtime failures; either provide a documented development-only default value for
JWT_PRIVATE_KEY_BASE64 (clearly marked insecure) or add a fail-fast validation
in your JWT config loader (e.g., JwtProperties / JwtConfig /
loadJwtKey()/getPrivateKeyBase64()) that checks the resolved private-key-base64
and throws a clear exception on startup if it is missing or invalid. Ensure the
validation message references JWT_PRIVATE_KEY_BASE64/private-key-base64 so
developers know which env var to set.
|
|
||
| merchant-iam: | ||
| jwt: | ||
| private-key-base64: ${JWT_PRIVATE_KEY_BASE64:} |
There was a problem hiding this comment.
Same empty default concern as api-gateway-iam.
${JWT_PRIVATE_KEY_BASE64:} defaults empty. Developers forgetting to set this env var will encounter runtime token signing failures. Consider aligning behavior with api-gateway-iam (if you add fail-fast validation there, do the same here).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@merchant-iam/merchant-iam/src/main/resources/application-sandbox.yml` at line
12, The sandbox config currently uses an empty default for the JWT private key
("private-key-base64: ${JWT_PRIVATE_KEY_BASE64:}"), which can hide missing env
vars; change this to fail-fast by either removing the empty default so the
JVM/property binder throws on missing env or add explicit startup validation
that checks the "private-key-base64" property (bound from
JWT_PRIVATE_KEY_BASE64) and aborts with a clear error if unset; locate where
JWT/private-key is read (config class or JwtConfig/Startup initializer) and add
the validation there to match api-gateway-iam behavior.
- Stripe: create PaymentIntent without auto-confirm (ACH needs payment method) - Modulr: remove Bearer prefix (sandbox-token uses raw key), add SCAN/FPS destination support, fix SEPA_CREDIT_TRANSFER scheme, truncate reference to 18 - Circle: funded sandbox via mock wire ($10,000 USD) - Companies House: fix expected status (dissolved, not active) - Solana: switch from public devnet to Alchemy Solana Devnet RPC - Fireblocks: expect 404 for non-existent TX, handle 400 for sandbox assets All 25 sandbox tests passing across 9 providers. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
WireMock test updated to match: raw key auth (no Bearer prefix), SEPA_CREDIT_TRANSFER scheme, truncated reference, SCAN destination fields. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
fiat-on-ramp/fiat-on-ramp/src/test/java/com/stablecoin/payments/onramp/infrastructure/provider/stripe/StripeAdapterSandboxTest.java (1)
39-42:⚠️ Potential issue | 🟠 MajorAdd
@SpringBootTestto this integration test suite.This test explicitly targets the real Stripe test mode API and qualifies as an integration test. Per repository policy, integration tests must use
@SpringBootTestor@Testcontainersfor bootstrap. While the current manualRestClientsetup is functional, adding@SpringBootTestaligns with the required bootstrap pattern and enables proper Spring context management for potential service dependencies and configuration handling.Suggested patch
import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Tag; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.condition.EnabledIfEnvironmentVariable; +import org.springframework.boot.test.context.SpringBootTest; @@ `@Tag`("sandbox") +@SpringBootTest `@EnabledIfEnvironmentVariable`(named = "STRIPE_TEST_SECRET_KEY", matches = "sk_test_.+") `@DisplayName`("Stripe Adapter Sandbox (live test mode)") class StripeAdapterSandboxTest {🤖 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/infrastructure/provider/stripe/StripeAdapterSandboxTest.java` around lines 39 - 42, Add the missing integration-test bootstrap by annotating the StripeAdapterSandboxTest class with `@SpringBootTest` so the Spring context is initialized for this sandbox/integration suite; update the class declaration (StripeAdapterSandboxTest) to include the `@SpringBootTest` annotation (and optionally keep existing `@Tag/`@EnabledIfEnvironmentVariable/@DisplayName) so Spring-managed beans/configuration are available to tests that currently create manual RestClient instances.fiat-off-ramp/fiat-off-ramp/src/test/java/com/stablecoin/payments/offramp/infrastructure/provider/modulr/ModulrPayoutAdapterSandboxTest.java (1)
58-93:⚠️ Potential issue | 🟠 MajorExercise the sandbox profile through Spring Boot.
This class still instantiates
ModulrPayoutAdapterdirectly and hardcodes the sandbox URL, so it never validatesapplication-sandbox.yml, property binding, or the actualsandboxprofile wiring that this PR is adding. Converting it to@SpringBootTestand autowiring the adapter would make this a real integration check.Suggested direction
+import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.context.SpringBootTest; ... +@SpringBootTest(properties = "spring.profiles.active=sandbox") `@Tag`("sandbox") `@EnabledIfEnvironmentVariable`(named = "MODULR_SANDBOX_API_KEY", matches = ".+") `@DisplayName`("Modulr Payout Adapter Sandbox (live sandbox API)") class ModulrPayoutAdapterSandboxTest { - private ModulrPayoutAdapter adapter; - - `@BeforeEach` - void setUp() { - var properties = new ModulrProperties( - "https://api-sandbox.modulrfinance.com", - System.getenv("MODULR_SANDBOX_API_KEY"), - "", - System.getenv("MODULR_SANDBOX_SOURCE_ACCOUNT_ID"), - 15 - ); - adapter = new ModulrPayoutAdapter(properties); - } + `@Autowired` + private ModulrPayoutAdapter adapter;As per coding guidelines, "Integration tests must use
@SpringBootTestor Testcontainers — no mocking of DB".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fiat-off-ramp/fiat-off-ramp/src/test/java/com/stablecoin/payments/offramp/infrastructure/provider/modulr/ModulrPayoutAdapterSandboxTest.java` around lines 58 - 93, The tests in ModulrPayoutAdapterSandboxTest instantiate ModulrPayoutAdapter directly and hardcode sandbox settings so they don't exercise Spring Boot profile binding; change the class to a Spring integration test (annotate with `@SpringBootTest` and activate the "sandbox" profile via `@ActiveProfiles` or properties) and autowire the ModulrPayoutAdapter instance instead of creating it manually, remove any hardcoded sandbox URL/config in the test (use the application-sandbox.yml values), and keep helper methods like aFpsPayoutRequest and the test methods (shouldInitiateFpsPayoutInSandbox, shouldReturnModulrReferenceWithExpectedFormat) unchanged so they run against the real Spring-managed adapter and configuration.
🤖 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/src/test/java/com/stablecoin/payments/custody/infrastructure/provider/fireblocks/FireblocksCustodyAdapterSandboxTest.java`:
- Around line 90-91: The test asserts that
adapter.getTransactionStatus(fabricatedTxId) throws
HttpClientErrorException.NotFound but the FireblocksCustodyAdapter fallbacks
wrap 4xx errors into IllegalStateException; update the failing tests to either
expect IllegalStateException (e.g.,
assertThatThrownBy(...).isInstanceOf(IllegalStateException.class)) or change the
adapter behavior in FireblocksCustodyAdapter (inspect
getTransactionStatusFallback / signAndSubmitFallback) to unwrap and rethrow the
original HttpClientErrorException if you want the raw 4xx to propagate; pick one
approach and make parallel changes for the other tests mentioned (lines 116-121)
so assertions match the actual fallback behavior.
In
`@fiat-off-ramp/fiat-off-ramp/src/main/java/com/stablecoin/payments/offramp/infrastructure/provider/modulr/ModulrPaymentRequest.java`:
- Around line 13-18: Update the Javadoc on ModulrPaymentRequest and its
parameter docs to match the current wire contract: replace the SEPA example
token SEPA_CREDIT with SEPA_CREDIT_TRANSFER and replace the FPS destination
example with SCAN for Faster Payments; also review and update the related param
descriptions for currency, reference, externalReference and destination (lines
covering the parameter block, e.g., the Javadoc for ModulrPaymentRequest) so the
documented enums/strings and destination type examples exactly match the
adapter’s emitted values.
In
`@fiat-off-ramp/fiat-off-ramp/src/main/java/com/stablecoin/payments/offramp/infrastructure/provider/modulr/ModulrPayoutAdapter.java`:
- Around line 68-69: The adapter currently assumes non-FASTER_PAYMENTS rails are
IBAN which can produce invalid Modulr payloads; update resolveDestination and
resolvePermittedScheme in ModulrPayoutAdapter to fail fast: explicitly switch on
request.paymentRail() (e.g., PaymentRail.FASTER_PAYMENTS vs PaymentRail.IBAN)
and throw a clear exception for unsupported rails (PIX, UPI, M_PESA, etc.),
ensure the FASTER_PAYMENTS branch validates presence and format of sortCode and
accountNumber, and ensure the IBAN branch validates an IBAN is present (and
format if available); also apply the same strict validation/early-rejection
logic to the code paths around the 106-123 region so mismatched permittedScheme
+ destination data never produce a Modulr payload.
In
`@fiat-off-ramp/fiat-off-ramp/src/test/java/com/stablecoin/payments/offramp/infrastructure/provider/modulr/ModulrPayoutAdapterTest.java`:
- Around line 145-160: The test currently only asserts the SEPA JSON contract;
add a WireMock body assertion for the GBP Faster Payments path in
ModulrPayoutAdapterTest by extending or adding the stub used by
initiatePayout_gbpFasterPayments() to include withRequestBody(equalToJson(...))
that matches a payload containing "destination.type":"SORT_CODE_ACCOUNT_NUMBER"
(or type used by your fixture), non-null "sortCode" and "accountNumber" fields
and "permittedScheme":"FASTER_PAYMENTS" (or "SCAN" if applicable); update the
test fixture/assertion to use the correct sort-code/account-number example
values used elsewhere so CI validates the faster-payments branch as well.
In
`@fiat-on-ramp/fiat-on-ramp/src/test/java/com/stablecoin/payments/onramp/infrastructure/provider/stripe/StripeAdapterSandboxTest.java`:
- Around line 76-83: The test StripeAdapterSandboxTest currently calls the
Stripe API directly via restClient.post() (the blocks that build requests and
call /v1/payment_intents), which bypasses the production adapter; modify the
test so it invokes StripePspAdapter.initiatePayment(...) instead (or
split/rename the class to indicate it is a raw Stripe smoke test), ensuring the
test exercises the adapter's form construction, resilience behavior, and
response mapping rather than calling restClient.post() directly; update
assertions to validate the adapter's return types and mappings and remove or
repurpose the direct restClient.post() calls accordingly.
---
Outside diff comments:
In
`@fiat-off-ramp/fiat-off-ramp/src/test/java/com/stablecoin/payments/offramp/infrastructure/provider/modulr/ModulrPayoutAdapterSandboxTest.java`:
- Around line 58-93: The tests in ModulrPayoutAdapterSandboxTest instantiate
ModulrPayoutAdapter directly and hardcode sandbox settings so they don't
exercise Spring Boot profile binding; change the class to a Spring integration
test (annotate with `@SpringBootTest` and activate the "sandbox" profile via
`@ActiveProfiles` or properties) and autowire the ModulrPayoutAdapter instance
instead of creating it manually, remove any hardcoded sandbox URL/config in the
test (use the application-sandbox.yml values), and keep helper methods like
aFpsPayoutRequest and the test methods (shouldInitiateFpsPayoutInSandbox,
shouldReturnModulrReferenceWithExpectedFormat) unchanged so they run against the
real Spring-managed adapter and configuration.
In
`@fiat-on-ramp/fiat-on-ramp/src/test/java/com/stablecoin/payments/onramp/infrastructure/provider/stripe/StripeAdapterSandboxTest.java`:
- Around line 39-42: Add the missing integration-test bootstrap by annotating
the StripeAdapterSandboxTest class with `@SpringBootTest` so the Spring context is
initialized for this sandbox/integration suite; update the class declaration
(StripeAdapterSandboxTest) to include the `@SpringBootTest` annotation (and
optionally keep existing `@Tag/`@EnabledIfEnvironmentVariable/@DisplayName) so
Spring-managed beans/configuration are available to tests that currently create
manual RestClient instances.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f528b819-89cc-42bd-b81d-a2a9b897ddf5
📒 Files selected for processing (10)
blockchain-custody/blockchain-custody/src/main/resources/application-sandbox.ymlblockchain-custody/blockchain-custody/src/test/java/com/stablecoin/payments/custody/infrastructure/provider/fireblocks/FireblocksCustodyAdapterSandboxTest.javablockchain-custody/blockchain-custody/src/test/java/com/stablecoin/payments/custody/infrastructure/provider/solana/SolanaRpcAdapterSandboxTest.javafiat-off-ramp/fiat-off-ramp/src/main/java/com/stablecoin/payments/offramp/infrastructure/provider/modulr/ModulrPaymentRequest.javafiat-off-ramp/fiat-off-ramp/src/main/java/com/stablecoin/payments/offramp/infrastructure/provider/modulr/ModulrPayoutAdapter.javafiat-off-ramp/fiat-off-ramp/src/main/resources/application-sandbox.ymlfiat-off-ramp/fiat-off-ramp/src/test/java/com/stablecoin/payments/offramp/infrastructure/provider/modulr/ModulrPayoutAdapterSandboxTest.javafiat-off-ramp/fiat-off-ramp/src/test/java/com/stablecoin/payments/offramp/infrastructure/provider/modulr/ModulrPayoutAdapterTest.javafiat-on-ramp/fiat-on-ramp/src/test/java/com/stablecoin/payments/onramp/infrastructure/provider/stripe/StripeAdapterSandboxTest.javamerchant-onboarding/merchant-onboarding/src/test/java/com/stablecoin/payments/merchant/onboarding/infrastructure/kyb/CompaniesHouseAdapterSandboxTest.java
Comment on lines
+90
to
+91
| assertThatThrownBy(() -> adapter.getTransactionStatus(fabricatedTxId)) | ||
| .isInstanceOf(HttpClientErrorException.NotFound.class); |
There was a problem hiding this comment.
Exception assertions are likely incorrect because adapter fallbacks wrap 4xx into IllegalStateException.
These assertions expect raw HttpClientErrorException to escape, but the adapter’s resilience fallbacks appear to catch and rethrow as IllegalStateException, so both changed tests can fail for the wrong reason.
Proposed test fix aligned to fallback behavior
- assertThatThrownBy(() -> adapter.getTransactionStatus(fabricatedTxId))
- .isInstanceOf(HttpClientErrorException.NotFound.class);
+ assertThatThrownBy(() -> adapter.getTransactionStatus(fabricatedTxId))
+ .isInstanceOf(IllegalStateException.class)
+ .hasMessageContaining("Fireblocks custody unavailable")
+ .hasCauseInstanceOf(HttpClientErrorException.NotFound.class);
@@
- try {
- var result = adapter.signAndSubmit(request);
- assertThat(result.custodyTxId()).isNotBlank();
- } catch (HttpClientErrorException.BadRequest ex) {
- assertThat(ex.getStatusCode().value()).isEqualTo(400);
- }
+ try {
+ var result = adapter.signAndSubmit(request);
+ assertThat(result.custodyTxId()).isNotBlank();
+ } catch (IllegalStateException ex) {
+ assertThat(ex.getCause()).isInstanceOf(HttpClientErrorException.BadRequest.class);
+ }Use this read-only verification to confirm fallback wrapping and check whether 4xx is explicitly ignored in resilience config:
#!/bin/bash
set -euo pipefail
echo "=== Fireblocks adapter resilience annotations and fallbacks ==="
fd 'FireblocksCustodyAdapter.java' -x rg -n -C2 '@Retry|@CircuitBreaker|fallbackMethod|getTransactionStatusFallback|signAndSubmitFallback|throw new IllegalStateException|retrieve\(\)\.body\(' {}
echo
echo "=== Resilience config for fireblocks (ignore/record exceptions) ==="
rg -n -C2 'resilience4j|fireblocks|ignore-exceptions|record-exceptions|retry-exceptions' -g '**/application*.yml' -g '**/application*.yaml' -g '**/application*.properties'Also applies to: 116-121
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@blockchain-custody/blockchain-custody/src/test/java/com/stablecoin/payments/custody/infrastructure/provider/fireblocks/FireblocksCustodyAdapterSandboxTest.java`
around lines 90 - 91, The test asserts that
adapter.getTransactionStatus(fabricatedTxId) throws
HttpClientErrorException.NotFound but the FireblocksCustodyAdapter fallbacks
wrap 4xx errors into IllegalStateException; update the failing tests to either
expect IllegalStateException (e.g.,
assertThatThrownBy(...).isInstanceOf(IllegalStateException.class)) or change the
adapter behavior in FireblocksCustodyAdapter (inspect
getTransactionStatusFallback / signAndSubmitFallback) to unwrap and rethrow the
original HttpClientErrorException if you want the raw 4xx to propagate; pick one
approach and make parallel changes for the other tests mentioned (lines 116-121)
so assertions match the actual fallback behavior.
Comment on lines
+13
to
18
| * @param currency ISO 4217 currency code (e.g., EUR, GBP) | ||
| * @param reference payment reference visible to beneficiary | ||
| * @param externalReference idempotency key / external correlation ID | ||
| * @param destination beneficiary destination details | ||
| * @param permittedScheme payment scheme (e.g., SEPA_CREDIT) | ||
| * @param permittedScheme payment scheme (e.g., SEPA_CREDIT, FPS) | ||
| */ |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Align the Javadoc with the current wire contract.
The adapter now emits SEPA_CREDIT_TRANSFER for SEPA and uses SCAN as the destination type for Faster Payments. The updated docs still use SEPA_CREDIT/FPS examples, which no longer match what this DTO represents on the wire.
Also applies to: 29-37
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@fiat-off-ramp/fiat-off-ramp/src/main/java/com/stablecoin/payments/offramp/infrastructure/provider/modulr/ModulrPaymentRequest.java`
around lines 13 - 18, Update the Javadoc on ModulrPaymentRequest and its
parameter docs to match the current wire contract: replace the SEPA example
token SEPA_CREDIT with SEPA_CREDIT_TRANSFER and replace the FPS destination
example with SCAN for Faster Payments; also review and update the related param
descriptions for currency, reference, externalReference and destination (lines
covering the parameter block, e.g., the Javadoc for ModulrPaymentRequest) so the
documented enums/strings and destination type examples exactly match the
adapter’s emitted values.
Comment on lines
+68
to
69
| var destination = resolveDestination(request); | ||
| var permittedScheme = resolvePermittedScheme(request.paymentRail()); |
There was a problem hiding this comment.
Fail fast on unsupported or mismatched payout rails.
resolveDestination() currently treats every non-FASTER_PAYMENTS rail as IBAN, and the FPS branch never verifies that the bank account actually contains sort-code data. Since PaymentRail also includes PIX, UPI, and M_PESA, this can build invalid outbound Modulr payloads instead of rejecting the request before the external call.
Suggested hardening
+import com.stablecoin.payments.offramp.domain.model.AccountType;
import com.stablecoin.payments.offramp.domain.model.PaymentRail;
...
public PayoutResult initiatePayout(PayoutRequest request) {
log.info("[MODULR] Initiating payout payoutId={} amount={} currency={} rail={}",
request.payoutId(), request.fiatAmount(), request.currency(), request.paymentRail());
+ validateDestination(request);
var destination = resolveDestination(request);
var permittedScheme = resolvePermittedScheme(request.paymentRail());
...
+ private static void validateDestination(PayoutRequest request) {
+ switch (request.paymentRail()) {
+ case SEPA -> {
+ if (request.bankAccount().accountType() != AccountType.IBAN) {
+ throw new PayoutPartnerException(
+ request.payoutId(), "modulr", "SEPA payouts require an IBAN destination");
+ }
+ }
+ case FASTER_PAYMENTS -> {
+ if (request.bankAccount().accountType() != AccountType.SORT_CODE) {
+ throw new PayoutPartnerException(
+ request.payoutId(), "modulr",
+ "Faster Payments require sort code + account number");
+ }
+ }
+ default -> throw new PayoutPartnerException(
+ request.payoutId(), "modulr",
+ "Unsupported payment rail for Modulr: " + request.paymentRail());
+ }
+ }
+
private static ModulrPaymentRequest.ModulrDestination resolveDestination(PayoutRequest request) {
var name = request.partnerIdentifier().partnerName();
return switch (request.paymentRail()) {
+ case SEPA -> ModulrPaymentRequest.ModulrDestination.iban(
+ request.bankAccount().accountNumber(),
+ name);
case FASTER_PAYMENTS -> ModulrPaymentRequest.ModulrDestination.scan(
request.bankAccount().bankCode(),
request.bankAccount().accountNumber(),
name);
- default -> ModulrPaymentRequest.ModulrDestination.iban(
- request.bankAccount().accountNumber(),
- name);
+ default -> throw new IllegalStateException("Unsupported payment rail: " + request.paymentRail());
};
}
private static String resolvePermittedScheme(PaymentRail rail) {
return switch (rail) {
case SEPA -> "SEPA_CREDIT_TRANSFER";
case FASTER_PAYMENTS -> null;
- default -> null;
+ default -> throw new IllegalStateException("Unsupported payment rail: " + rail);
};
}Also applies to: 106-123
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@fiat-off-ramp/fiat-off-ramp/src/main/java/com/stablecoin/payments/offramp/infrastructure/provider/modulr/ModulrPayoutAdapter.java`
around lines 68 - 69, The adapter currently assumes non-FASTER_PAYMENTS rails
are IBAN which can produce invalid Modulr payloads; update resolveDestination
and resolvePermittedScheme in ModulrPayoutAdapter to fail fast: explicitly
switch on request.paymentRail() (e.g., PaymentRail.FASTER_PAYMENTS vs
PaymentRail.IBAN) and throw a clear exception for unsupported rails (PIX, UPI,
M_PESA, etc.), ensure the FASTER_PAYMENTS branch validates presence and format
of sortCode and accountNumber, and ensure the IBAN branch validates an IBAN is
present (and format if available); also apply the same strict
validation/early-rejection logic to the code paths around the 106-123 region so
mismatched permittedScheme + destination data never produce a Modulr payload.
Comment on lines
145
to
160
| .withRequestBody(equalToJson(""" | ||
| { | ||
| "sourceAccountId": "A1100ABCD1", | ||
| "amount": 9500.00, | ||
| "currency": "EUR", | ||
| "reference": "Payout 887adb57-1d2e-4f3a-b5c6-d7e8f9a0b1c2", | ||
| "reference": "887adb57-1d2e-4f3a", | ||
| "externalReference": "887adb57-1d2e-4f3a-b5c6-d7e8f9a0b1c2", | ||
| "destination": { | ||
| "type": "IBAN", | ||
| "iban": "DE89370400440532013000", | ||
| "name": "modulr" | ||
| "name": "modulr", | ||
| "sortCode": null, | ||
| "accountNumber": null | ||
| }, | ||
| "permittedScheme": "SEPA_CREDIT" | ||
| "permittedScheme": "SEPA_CREDIT_TRANSFER" | ||
| } |
There was a problem hiding this comment.
Lock down the FPS request contract in CI.
This assertion now covers the SEPA branch only. The new FASTER_PAYMENTS/SCAN path is still protected only by the tagged live test, so a bad sort-code payload or permittedScheme regression can pass normal CI. Add a WireMock body assertion for initiatePayout_gbpFasterPayments() with a sort-code/account-number fixture.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@fiat-off-ramp/fiat-off-ramp/src/test/java/com/stablecoin/payments/offramp/infrastructure/provider/modulr/ModulrPayoutAdapterTest.java`
around lines 145 - 160, The test currently only asserts the SEPA JSON contract;
add a WireMock body assertion for the GBP Faster Payments path in
ModulrPayoutAdapterTest by extending or adding the stub used by
initiatePayout_gbpFasterPayments() to include withRequestBody(equalToJson(...))
that matches a payload containing "destination.type":"SORT_CODE_ACCOUNT_NUMBER"
(or type used by your fixture), non-null "sortCode" and "accountNumber" fields
and "permittedScheme":"FASTER_PAYMENTS" (or "SCAN" if applicable); update the
test fixture/assertion to use the correct sort-code/account-number example
values used elsewhere so CI validates the faster-payments branch as well.
Comment on lines
+76
to
+83
| var response = restClient.post() | ||
| .uri("/v1/payment_intents") | ||
| .contentType(MediaType.APPLICATION_FORM_URLENCODED) | ||
| .header("Idempotency-Key", UUID.randomUUID().toString()) | ||
| .body(formData) | ||
| .retrieve() | ||
| .body(StripePaymentIntentResponse.class); | ||
|
|
There was a problem hiding this comment.
Sandbox test bypasses the adapter path.
Lines 76-83 and Lines 98-112 call Stripe directly via RestClient, so this suite no longer validates StripePspAdapter.initiatePayment(...) behavior used in production (form construction, resilience annotations, and adapter contract mapping). Please route this sandbox test through the adapter, or rename/scope it explicitly as a raw Stripe API smoke test to avoid false confidence.
Also applies to: 98-112
🤖 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/infrastructure/provider/stripe/StripeAdapterSandboxTest.java`
around lines 76 - 83, The test StripeAdapterSandboxTest currently calls the
Stripe API directly via restClient.post() (the blocks that build requests and
call /v1/payment_intents), which bypasses the production adapter; modify the
test so it invokes StripePspAdapter.initiatePayment(...) instead (or
split/rename the class to indicate it is a raw Stripe smoke test), ensuring the
test exercises the adapter's form construction, resilience behavior, and
response mapping rather than calling restClient.post() directly; update
assertions to validate the adapter's return types and mappings and remove or
repurpose the direct restClient.post() calls accordingly.
… test Address CodeRabbit review: assert HttpClientErrorException instead of generic Exception for 422 same-currency pair error. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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
application-sandbox.ymlfor the 5 missing services (S1, S6, S7, S10, S13) — all 10 services now have sandbox profiles.env.sandbox.templatewith all required env vars, signup URLs, and descriptionsFrankfurterRateAdapterSandboxTest— live API test (4 tests, all pass).env.sandboxto.gitignoreto prevent key leaksServices Updated
app.fx.rate-provider=frankfurter)Test plan
FrankfurterRateAdapterSandboxTest— 4 tests pass against liveapi.frankfurter.appCloses #240, Closes #241
🤖 Generated with Claude Code
Summary by CodeRabbit
Chores
New Features
Tests