feat(fx): add FrankfurterRateAdapter for free FX rates (STA-206)#233
Merged
Merged
Conversation
Add Frankfurter API adapter as a free alternative to Refinitiv for sandbox/dev use. No API key required. Activated via app.fx.rate-provider=frankfurter. - FrankfurterRateAdapter: RestClient with HTTP/1.1, @CIRCUITBREAKER + @Retry - FrankfurterProperties: @ConfigurationProperties with sensible defaults - FrankfurterRateResponse: package-private ACL DTO - Resilience4j circuit breaker + retry config for frankfurter instance - 8 WireMock-based unit tests (success, empty rates, errors, timeout) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Puneethkumarck
added
service-s6
WalkthroughIntroduces a Frankfurter API rate provider integration with typed configuration, Spring-managed HTTP adapter, response DTO, resilience policies (circuit breaker and retry), and comprehensive WireMock unit tests. Enables switching rate providers via configuration. Changes
Sequence DiagramsequenceDiagram
participant Client
participant CircuitBreaker as CircuitBreaker<br/>(resilience4j)
participant Retry as Retry
participant FrankfurterRateAdapter
participant Frankfurter API
participant Fallback
Client->>CircuitBreaker: getRate(fromCurrency, toCurrency)
alt Circuit Closed
CircuitBreaker->>Retry: permit request
Retry->>FrankfurterRateAdapter: getRate()
FrankfurterRateAdapter->>Frankfurter API: GET /latest?from={from}&to={to}
alt Success (rates present)
Frankfurter API-->>FrankfurterRateAdapter: 200 + rates
FrankfurterRateAdapter->>FrankfurterRateAdapter: parse + validate
FrankfurterRateAdapter-->>Retry: Optional<CorridorRate>
Retry-->>CircuitBreaker: success
CircuitBreaker-->>Client: Optional<CorridorRate>
else API Error or Missing Rates
Frankfurter API-->>FrankfurterRateAdapter: error or empty rates
FrankfurterRateAdapter->>Fallback: exception raised
Fallback-->>Retry: Optional.empty()
Retry-->>CircuitBreaker: fallback result
CircuitBreaker-->>Client: Optional.empty()
end
else Circuit Open
CircuitBreaker->>Fallback: circuit open, skip request
Fallback-->>Client: Optional.empty()
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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)
📝 Coding Plan
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: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@fx-liquidity-engine/fx-liquidity-engine/src/main/java/com/stablecoin/payments/fx/infrastructure/provider/frankfurter/FrankfurterProperties.java`:
- Around line 6-17: Add a separate connectTimeoutMs field to the
FrankfurterProperties record and initialize it in the compact constructor with
its own default (e.g., 5000) instead of reusing readTimeoutMs; update the record
signature to include Integer connectTimeoutMs, update the compact constructor to
check/connectTimeoutMs for null or <=0 and set the default there, and then
update any adapter code that currently uses readTimeoutMs for both timeouts to
reference FrankfurterProperties.connectTimeoutMs for the connect timeout and
FrankfurterProperties.readTimeoutMs for the read timeout.
In
`@fx-liquidity-engine/fx-liquidity-engine/src/main/java/com/stablecoin/payments/fx/infrastructure/provider/frankfurter/FrankfurterRateAdapter.java`:
- Around line 48-59: Change the routine request log in
FrankfurterRateAdapter.getRate from INFO to DEBUG to avoid high-volume logging
during frequent polling; update the log statement that currently reads
log.info("[FRANKFURTER] Fetching rate for {}:{}", fromCurrency, toCurrency) to
use log.debug instead, keeping the existing warning log (log.warn) for missing
responses unchanged so only notable issues remain at higher levels.
- Around line 67-79: Add a brief explanatory code comment in
FrankfurterRateAdapter next to the CorridorRate.builder() usage (or specifically
the ageMs(0) line) that states the Frankfurter API provides only a date (no
timestamp), so true staleness cannot be computed and we set ageMs to 0 as a
pragmatic/sandbox choice; reference the Frankfurter API behavior and note this
limitation for future maintainers and that it should be revisited if a
timestamped provider is used.
- Around line 30-43: Constructor FrankfurterRateAdapter incorrectly uses
properties.readTimeoutMs() for the HTTP connect timeout; add a distinct
connectTimeoutMs to FrankfurterProperties and use properties.connectTimeoutMs()
when building the HttpClient (keep
requestFactory.setReadTimeout(Duration.ofMillis(properties.readTimeoutMs())) for
read timeout) so connect and read timeouts are separated; update
FrankfurterProperties interface/class to expose connectTimeoutMs() and adjust
any configuration bindings.
- Around line 86-91: frankfurterFallback currently always returns
Optional.empty(), hiding provider-down failures; change
frankfurterFallback(String fromCurrency, String toCurrency, Exception ex) so
that when ex indicates the provider is unavailable it throws a distinct
ProviderUnavailableException (include fromCurrency, toCurrency and ex as cause)
instead of returning Optional.empty(), and only return Optional.empty() for
legitimate “no rate” conditions; update the method signature to allow throwing
ProviderUnavailableException (or use an unchecked
ProviderUnavailableRuntimeException) and keep references to CorridorRate,
Optional.empty(), FxQuoteApplicationService and RateRefreshJob in logs/exception
message so callers can detect provider-down vs legitimate empty rates.
In `@fx-liquidity-engine/fx-liquidity-engine/src/main/resources/application.yml`:
- Around line 152-163: The frankfurter retry block uses camelCase keys
(maxAttempts, waitDuration) which is inconsistent with the existing fxRate
block's kebab-case (max-attempts, wait-duration); change frankfurter property
names to kebab-case (max-attempts, wait-duration) to match style and
maintainability, and optionally add the same exponential backoff settings used
by fxRate (e.g., enable exponential-backoff with multiplier/delay settings)
under the frankfurter configuration so retry behavior is consistent with fxRate.
In
`@fx-liquidity-engine/fx-liquidity-engine/src/test/java/com/stablecoin/payments/fx/infrastructure/provider/frankfurter/FrankfurterRateAdapterTest.java`:
- Around line 191-232: The tests currently assert that adapter.getRate("USD",
"EUR") throws a generic Exception which is too broad; update the three tests
(shouldThrowOnServerError, shouldThrowOnClientError, shouldThrowOnReadTimeout)
to assert the specific exception types thrown by the HTTP client used by
FrankfurterRateAdapter — e.g., assert server 500 triggers
HttpServerErrorException (or the adapter's mapped exception), 404 triggers
HttpClientErrorException (or the adapter's mapped exception), and the fixedDelay
timeout triggers ResourceAccessException/ReadTimeoutException depending on the
HTTP client; locate the assertions around adapter.getRate and replace
isInstanceOf(Exception.class) with the appropriate specific exception class
names to improve test precision.
- Around line 38-43: Current unit test directly instantiates
FrankfurterRateAdapter in setUp (using new FrankfurterRateAdapter(new
FrankfurterProperties(...))) which bypasses Spring so `@Retry/`@CircuitBreaker on
FrankfurterRateAdapter aren't exercised; add a separate integration test class
(e.g., FrankfurterRateAdapterIntegrationTest) annotated with `@SpringBootTest`
(and `@AutoConfigureWireMock` or configure WireMock via test properties), autowire
the FrankfurterRateAdapter bean instead of new-ing it, configure the
FrankfurterProperties endpoint to point to the WireMock server, and exercise
failure/retry/circuit-breaker scenarios to validate resilience behavior
end-to-end.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 99b0ce6f-cd05-49b7-8122-1368b241ab8e
📒 Files selected for processing (5)
fx-liquidity-engine/fx-liquidity-engine/src/main/java/com/stablecoin/payments/fx/infrastructure/provider/frankfurter/FrankfurterProperties.javafx-liquidity-engine/fx-liquidity-engine/src/main/java/com/stablecoin/payments/fx/infrastructure/provider/frankfurter/FrankfurterRateAdapter.javafx-liquidity-engine/fx-liquidity-engine/src/main/java/com/stablecoin/payments/fx/infrastructure/provider/frankfurter/FrankfurterRateResponse.javafx-liquidity-engine/fx-liquidity-engine/src/main/resources/application.ymlfx-liquidity-engine/fx-liquidity-engine/src/test/java/com/stablecoin/payments/fx/infrastructure/provider/frankfurter/FrankfurterRateAdapterTest.java
Comment on lines
+6
to
+17
| public record FrankfurterProperties( | ||
| String baseUrl, | ||
| Integer readTimeoutMs | ||
| ) { | ||
| public FrankfurterProperties { | ||
| if (baseUrl == null || baseUrl.isBlank()) { | ||
| baseUrl = "https://api.frankfurter.app"; | ||
| } | ||
| if (readTimeoutMs == null || readTimeoutMs <= 0) { | ||
| readTimeoutMs = 5000; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Properties record is well-structured with sensible defaults.
Compact constructor with null/blank checks and defaults is a clean pattern. Consider adding a separate connectTimeoutMs property—the adapter currently reuses readTimeoutMs for both connect and read timeouts, which conflates two distinct concerns.
♻️ Add separate connect timeout property
`@ConfigurationProperties`(prefix = "app.fx.frankfurter")
public record FrankfurterProperties(
String baseUrl,
- Integer readTimeoutMs
+ Integer readTimeoutMs,
+ Integer connectTimeoutMs
) {
public FrankfurterProperties {
if (baseUrl == null || baseUrl.isBlank()) {
baseUrl = "https://api.frankfurter.app";
}
if (readTimeoutMs == null || readTimeoutMs <= 0) {
readTimeoutMs = 5000;
}
+ if (connectTimeoutMs == null || connectTimeoutMs <= 0) {
+ connectTimeoutMs = 3000;
+ }
}
}📝 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
| public record FrankfurterProperties( | |
| String baseUrl, | |
| Integer readTimeoutMs | |
| ) { | |
| public FrankfurterProperties { | |
| if (baseUrl == null || baseUrl.isBlank()) { | |
| baseUrl = "https://api.frankfurter.app"; | |
| } | |
| if (readTimeoutMs == null || readTimeoutMs <= 0) { | |
| readTimeoutMs = 5000; | |
| } | |
| } | |
| public record FrankfurterProperties( | |
| String baseUrl, | |
| Integer readTimeoutMs, | |
| Integer connectTimeoutMs | |
| ) { | |
| public FrankfurterProperties { | |
| if (baseUrl == null || baseUrl.isBlank()) { | |
| baseUrl = "https://api.frankfurter.app"; | |
| } | |
| if (readTimeoutMs == null || readTimeoutMs <= 0) { | |
| readTimeoutMs = 5000; | |
| } | |
| if (connectTimeoutMs == null || connectTimeoutMs <= 0) { | |
| connectTimeoutMs = 3000; | |
| } | |
| } | |
| } |
🤖 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/infrastructure/provider/frankfurter/FrankfurterProperties.java`
around lines 6 - 17, Add a separate connectTimeoutMs field to the
FrankfurterProperties record and initialize it in the compact constructor with
its own default (e.g., 5000) instead of reusing readTimeoutMs; update the record
signature to include Integer connectTimeoutMs, update the compact constructor to
check/connectTimeoutMs for null or <=0 and set the default there, and then
update any adapter code that currently uses readTimeoutMs for both timeouts to
reference FrankfurterProperties.connectTimeoutMs for the connect timeout and
FrankfurterProperties.readTimeoutMs for the read timeout.
Comment on lines
+30
to
+43
| public FrankfurterRateAdapter(FrankfurterProperties properties) { | ||
| var httpClient = HttpClient.newBuilder() | ||
| .version(Version.HTTP_1_1) | ||
| .connectTimeout(Duration.ofMillis(properties.readTimeoutMs())) | ||
| .build(); | ||
|
|
||
| var requestFactory = new JdkClientHttpRequestFactory(httpClient); | ||
| requestFactory.setReadTimeout(Duration.ofMillis(properties.readTimeoutMs())); | ||
|
|
||
| this.restClient = RestClient.builder() | ||
| .baseUrl(properties.baseUrl()) | ||
| .requestFactory(requestFactory) | ||
| .build(); | ||
| } |
There was a problem hiding this comment.
connectTimeout incorrectly uses readTimeoutMs.
Line 33 sets connectTimeout using properties.readTimeoutMs(). These are distinct concerns: connect timeout guards against slow network handshakes (typically 2-5s), while read timeout guards against slow responses (often longer). Using the same value means either connect is too long or read is too short.
🔧 Separate connect and read timeouts
public FrankfurterRateAdapter(FrankfurterProperties properties) {
var httpClient = HttpClient.newBuilder()
.version(Version.HTTP_1_1)
- .connectTimeout(Duration.ofMillis(properties.readTimeoutMs()))
+ .connectTimeout(Duration.ofMillis(properties.connectTimeoutMs()))
.build();This requires adding connectTimeoutMs to FrankfurterProperties as suggested in that file's review.
🤖 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/infrastructure/provider/frankfurter/FrankfurterRateAdapter.java`
around lines 30 - 43, Constructor FrankfurterRateAdapter incorrectly uses
properties.readTimeoutMs() for the HTTP connect timeout; add a distinct
connectTimeoutMs to FrankfurterProperties and use properties.connectTimeoutMs()
when building the HttpClient (keep
requestFactory.setReadTimeout(Duration.ofMillis(properties.readTimeoutMs())) for
read timeout) so connect and read timeouts are separated; update
FrankfurterProperties interface/class to expose connectTimeoutMs() and adjust
any configuration bindings.
Comment on lines
+48
to
+59
| public Optional<CorridorRate> getRate(String fromCurrency, String toCurrency) { | ||
| log.info("[FRANKFURTER] Fetching rate for {}:{}", fromCurrency, toCurrency); | ||
|
|
||
| var response = restClient.get() | ||
| .uri("/latest?from={from}&to={to}", fromCurrency, toCurrency) | ||
| .retrieve() | ||
| .body(FrankfurterRateResponse.class); | ||
|
|
||
| if (response == null || response.rates() == null || response.rates().isEmpty()) { | ||
| log.warn("[FRANKFURTER] No rate returned for {}:{}", fromCurrency, toCurrency); | ||
| return Optional.empty(); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider DEBUG level for routine request logging.
Line 49 logs at INFO for every rate fetch. In production with frequent polling, this creates significant log volume. Consider DEBUG for the request and reserve INFO for warnings/errors.
♻️ Reduce log verbosity
- log.info("[FRANKFURTER] Fetching rate for {}:{}", fromCurrency, toCurrency);
+ log.debug("[FRANKFURTER] Fetching rate for {}:{}", fromCurrency, toCurrency);📝 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
| public Optional<CorridorRate> getRate(String fromCurrency, String toCurrency) { | |
| log.info("[FRANKFURTER] Fetching rate for {}:{}", fromCurrency, toCurrency); | |
| var response = restClient.get() | |
| .uri("/latest?from={from}&to={to}", fromCurrency, toCurrency) | |
| .retrieve() | |
| .body(FrankfurterRateResponse.class); | |
| if (response == null || response.rates() == null || response.rates().isEmpty()) { | |
| log.warn("[FRANKFURTER] No rate returned for {}:{}", fromCurrency, toCurrency); | |
| return Optional.empty(); | |
| } | |
| public Optional<CorridorRate> getRate(String fromCurrency, String toCurrency) { | |
| log.debug("[FRANKFURTER] Fetching rate for {}:{}", fromCurrency, toCurrency); | |
| var response = restClient.get() | |
| .uri("/latest?from={from}&to={to}", fromCurrency, toCurrency) | |
| .retrieve() | |
| .body(FrankfurterRateResponse.class); | |
| if (response == null || response.rates() == null || response.rates().isEmpty()) { | |
| log.warn("[FRANKFURTER] No rate returned for {}:{}", fromCurrency, toCurrency); | |
| return Optional.empty(); | |
| } |
🤖 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/infrastructure/provider/frankfurter/FrankfurterRateAdapter.java`
around lines 48 - 59, Change the routine request log in
FrankfurterRateAdapter.getRate from INFO to DEBUG to avoid high-volume logging
during frequent polling; update the log statement that currently reads
log.info("[FRANKFURTER] Fetching rate for {}:{}", fromCurrency, toCurrency) to
use log.debug instead, keeping the existing warning log (log.warn) for missing
responses unchanged so only notable issues remain at higher levels.
Comment on lines
+67
to
+79
| var corridorRate = CorridorRate.builder() | ||
| .fromCurrency(fromCurrency) | ||
| .toCurrency(toCurrency) | ||
| .rate(rate) | ||
| .spreadBps(DEFAULT_SPREAD_BPS) | ||
| .feeBps(DEFAULT_FEE_BPS) | ||
| .provider("frankfurter") | ||
| .ageMs(0) | ||
| .build(); | ||
|
|
||
| log.info("[FRANKFURTER] Rate fetched {}:{}={}", fromCurrency, toCurrency, rate); | ||
| return Optional.of(corridorRate); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
ageMs=0 hardcoded—document the limitation.
The Frankfurter API returns only a date, not a timestamp, so actual rate staleness cannot be determined. Hardcoding ageMs=0 is pragmatic for a sandbox provider, but consider adding a brief code comment explaining this limitation for future maintainers.
📝 Add explanatory comment
var corridorRate = CorridorRate.builder()
.fromCurrency(fromCurrency)
.toCurrency(toCurrency)
.rate(rate)
.spreadBps(DEFAULT_SPREAD_BPS)
.feeBps(DEFAULT_FEE_BPS)
.provider("frankfurter")
+ // Frankfurter API provides date only (no timestamp), so ageMs cannot be accurately determined
.ageMs(0)
.build();📝 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
| var corridorRate = CorridorRate.builder() | |
| .fromCurrency(fromCurrency) | |
| .toCurrency(toCurrency) | |
| .rate(rate) | |
| .spreadBps(DEFAULT_SPREAD_BPS) | |
| .feeBps(DEFAULT_FEE_BPS) | |
| .provider("frankfurter") | |
| .ageMs(0) | |
| .build(); | |
| log.info("[FRANKFURTER] Rate fetched {}:{}={}", fromCurrency, toCurrency, rate); | |
| return Optional.of(corridorRate); | |
| } | |
| var corridorRate = CorridorRate.builder() | |
| .fromCurrency(fromCurrency) | |
| .toCurrency(toCurrency) | |
| .rate(rate) | |
| .spreadBps(DEFAULT_SPREAD_BPS) | |
| .feeBps(DEFAULT_FEE_BPS) | |
| .provider("frankfurter") | |
| // Frankfurter API provides date only (no timestamp), so ageMs cannot be accurately determined | |
| .ageMs(0) | |
| .build(); | |
| log.info("[FRANKFURTER] Rate fetched {}:{}={}", fromCurrency, toCurrency, rate); | |
| return Optional.of(corridorRate); | |
| } |
🤖 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/infrastructure/provider/frankfurter/FrankfurterRateAdapter.java`
around lines 67 - 79, Add a brief explanatory code comment in
FrankfurterRateAdapter next to the CorridorRate.builder() usage (or specifically
the ageMs(0) line) that states the Frankfurter API provides only a date (no
timestamp), so true staleness cannot be computed and we set ageMs to 0 as a
pragmatic/sandbox choice; reference the Frankfurter API behavior and note this
limitation for future maintainers and that it should be revisited if a
timestamped provider is used.
Comment on lines
+86
to
+91
| @SuppressWarnings("unused") | ||
| private Optional<CorridorRate> frankfurterFallback(String fromCurrency, String toCurrency, Exception ex) { | ||
| log.error("[FRANKFURTER] Resilience fallback for {}:{} due to {}", | ||
| fromCurrency, toCurrency, ex.getClass().getSimpleName(), ex); | ||
| return Optional.empty(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find usages of RateProvider.getRate and check how empty is handled
ast-grep --pattern 'getRate($_, $_)' | head -50
rg -n -A5 'getRate\(' --type=java -g '!*Test*' -g '!FrankfurterRateAdapter.java'Repository: Puneethkumarck/stablebridge-platform
Length of output: 6316
🏁 Script executed:
#!/bin/bash
# Check FxQuoteApplicationService full method context around getRate call
rg -n -B10 -A15 'rateProvider.getRate' fx-liquidity-engine/fx-liquidity-engine/src/main/java/com/stablecoin/payments/fx/application/service/FxQuoteApplicationService.java | head -60Repository: Puneethkumarck/stablebridge-platform
Length of output: 1561
🏁 Script executed:
#!/bin/bash
# Check RateRefreshJob full method context
rg -n -B5 -A15 'rateProvider.getRate' fx-liquidity-engine/fx-liquidity-engine/src/main/java/com/stablecoin/payments/fx/infrastructure/scheduling/RateRefreshJob.javaRepository: Puneethkumarck/stablebridge-platform
Length of output: 1084
🏁 Script executed:
#!/bin/bash
# Check how FrankfurterRateAdapter.getRate is decorated (circuit breaker, resilience4j)
rg -n -B20 'public Optional<CorridorRate> getRate' fx-liquidity-engine/fx-liquidity-engine/src/main/java/com/stablecoin/payments/fx/infrastructure/provider/frankfurter/FrankfurterRateAdapter.java | head -40Repository: Puneethkumarck/stablebridge-platform
Length of output: 1041
🏁 Script executed:
#!/bin/bash
# Verify frankfurterFallback is used as fallback method in circuit breaker config
rg -n 'frankfurterFallback\|@CircuitBreaker\|@Fallback' fx-liquidity-engine/fx-liquidity-engine/src/main/java/com/stablecoin/payments/fx/infrastructure/provider/frankfurter/FrankfurterRateAdapter.javaRepository: Puneethkumarck/stablebridge-platform
Length of output: 62
Circuit breaker fallback returns Optional.empty() without distinguishing provider-down from legitimate empty rates.
Both FxQuoteApplicationService and RateRefreshJob correctly handle the empty Optional—the former throws RateUnavailableException (critical path), the latter logs and continues (background refresh). However, since the fallback returns the same Optional.empty() regardless of whether the provider is down or legitimately has no rate, callers cannot distinguish these cases. Consider having frankfurterFallback() throw a distinct exception (e.g., ProviderUnavailableException) instead of returning empty, enabling callers to implement provider-specific fallback strategies.
🤖 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/infrastructure/provider/frankfurter/FrankfurterRateAdapter.java`
around lines 86 - 91, frankfurterFallback currently always returns
Optional.empty(), hiding provider-down failures; change
frankfurterFallback(String fromCurrency, String toCurrency, Exception ex) so
that when ex indicates the provider is unavailable it throws a distinct
ProviderUnavailableException (include fromCurrency, toCurrency and ex as cause)
instead of returning Optional.empty(), and only return Optional.empty() for
legitimate “no rate” conditions; update the method signature to allow throwing
ProviderUnavailableException (or use an unchecked
ProviderUnavailableRuntimeException) and keep references to CorridorRate,
Optional.empty(), FxQuoteApplicationService and RateRefreshJob in logs/exception
message so callers can detect provider-down vs legitimate empty rates.
Comment on lines
+152
to
+163
| frankfurter: | ||
| maxAttempts: 3 | ||
| waitDuration: 1s | ||
| retry-exceptions: | ||
| - java.io.IOException | ||
| - java.net.http.HttpTimeoutException | ||
| - java.net.http.HttpConnectTimeoutException | ||
| - org.springframework.web.client.ResourceAccessException | ||
| - org.springframework.web.client.HttpServerErrorException | ||
| ignore-exceptions: | ||
| - org.springframework.web.client.HttpClientErrorException | ||
| - io.github.resilience4j.circuitbreaker.CallNotPermittedException |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Inconsistent property naming style in retry configuration.
The frankfurter retry config uses camelCase (maxAttempts, waitDuration) while the existing fxRate config uses kebab-case (max-attempts, wait-duration). Spring Boot accepts both, but mixing styles in the same file reduces maintainability.
Also, fxRate enables exponential backoff; consider whether frankfurter should too for better retry distribution under load.
♻️ Align naming style with existing config
frankfurter:
- maxAttempts: 3
- waitDuration: 1s
+ max-attempts: 3
+ wait-duration: 1s
+ enable-exponential-backoff: true
+ exponential-backoff-multiplier: 2
retry-exceptions:🤖 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/resources/application.yml`
around lines 152 - 163, The frankfurter retry block uses camelCase keys
(maxAttempts, waitDuration) which is inconsistent with the existing fxRate
block's kebab-case (max-attempts, wait-duration); change frankfurter property
names to kebab-case (max-attempts, wait-duration) to match style and
maintainability, and optionally add the same exponential backoff settings used
by fxRate (e.g., enable exponential-backoff with multiplier/delay settings)
under the frankfurter configuration so retry behavior is consistent with fxRate.
Comment on lines
+38
to
+43
| @BeforeEach | ||
| void setUp() { | ||
| wireMock.resetAll(); | ||
| var properties = new FrankfurterProperties(wireMock.baseUrl(), 2000); | ||
| adapter = new FrankfurterRateAdapter(properties); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for Spring Boot integration tests covering FrankfurterRateAdapter
rg -l '@SpringBootTest' --type=java | xargs -I {} rg -l 'FrankfurterRateAdapter|frankfurter' {} 2>/dev/null || echo "No integration tests found"Repository: Puneethkumarck/stablebridge-platform
Length of output: 106
🏁 Script executed:
# Find and examine FrankfurterRateAdapter class definition
find . -name 'FrankfurterRateAdapter.java' -type f | head -5Repository: Puneethkumarck/stablebridge-platform
Length of output: 226
🏁 Script executed:
# Check for resilience annotations (`@Retry`, `@CircuitBreaker`) in the adapter class
rg -A 30 'class FrankfurterRateAdapter' --type=javaRepository: Puneethkumarck/stablebridge-platform
Length of output: 11352
🏁 Script executed:
# Check if resilience4j or similar resilience framework is in dependencies
rg -i 'resilience|retry|circuit' --type=xml --type=gradle | head -20Repository: Puneethkumarck/stablebridge-platform
Length of output: 62
🏁 Script executed:
# Look at the actual test class to understand its scope
cat -n fx-liquidity-engine/fx-liquidity-engine/src/test/java/com/stablecoin/payments/fx/infrastructure/provider/frankfurter/FrankfurterRateAdapterTest.java | head -100Repository: Puneethkumarck/stablebridge-platform
Length of output: 4709
Unit tests bypass Spring context—resilience annotations won't be exercised.
Direct instantiation of FrankfurterRateAdapter means @Retry and @CircuitBreaker annotations are inactive. This is appropriate for testing adapter logic in isolation, but consider adding a separate @SpringBootTest integration test to verify resilience behavior end-to-end.
🤖 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/provider/frankfurter/FrankfurterRateAdapterTest.java`
around lines 38 - 43, Current unit test directly instantiates
FrankfurterRateAdapter in setUp (using new FrankfurterRateAdapter(new
FrankfurterProperties(...))) which bypasses Spring so `@Retry/`@CircuitBreaker on
FrankfurterRateAdapter aren't exercised; add a separate integration test class
(e.g., FrankfurterRateAdapterIntegrationTest) annotated with `@SpringBootTest`
(and `@AutoConfigureWireMock` or configure WireMock via test properties), autowire
the FrankfurterRateAdapter bean instead of new-ing it, configure the
FrankfurterProperties endpoint to point to the WireMock server, and exercise
failure/retry/circuit-breaker scenarios to validate resilience behavior
end-to-end.
Comment on lines
+191
to
+232
| @Test | ||
| @DisplayName("should throw on server error 500") | ||
| void shouldThrowOnServerError() { | ||
| wireMock.stubFor(get(urlEqualTo("/latest?from=USD&to=EUR")) | ||
| .willReturn(aResponse().withStatus(500))); | ||
|
|
||
| assertThatThrownBy(() -> adapter.getRate("USD", "EUR")) | ||
| .isInstanceOf(Exception.class); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("should throw on client error 404") | ||
| void shouldThrowOnClientError() { | ||
| wireMock.stubFor(get(urlEqualTo("/latest?from=USD&to=EUR")) | ||
| .willReturn(aResponse().withStatus(404))); | ||
|
|
||
| assertThatThrownBy(() -> adapter.getRate("USD", "EUR")) | ||
| .isInstanceOf(Exception.class); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("should throw on read timeout") | ||
| void shouldThrowOnReadTimeout() { | ||
| wireMock.stubFor(get(urlEqualTo("/latest?from=USD&to=EUR")) | ||
| .willReturn(aResponse() | ||
| .withStatus(200) | ||
| .withHeader("Content-Type", "application/json") | ||
| .withFixedDelay(5000) | ||
| .withBody(""" | ||
| { | ||
| "amount": 1.0, | ||
| "base": "USD", | ||
| "date": "2026-03-17", | ||
| "rates": { | ||
| "EUR": 0.86723 | ||
| } | ||
| } | ||
| """))); | ||
|
|
||
| assertThatThrownBy(() -> adapter.getRate("USD", "EUR")) | ||
| .isInstanceOf(Exception.class); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Error handling tests assert on generic Exception.class.
These tests verify that exceptions propagate, but asserting on Exception.class is overly broad. Consider asserting on more specific types (e.g., HttpServerErrorException, HttpClientErrorException, ResourceAccessException) for better test precision. This would catch accidental changes to exception handling behavior.
♻️ Use specific exception types
assertThatThrownBy(() -> adapter.getRate("USD", "EUR"))
- .isInstanceOf(Exception.class);
+ .isInstanceOf(org.springframework.web.client.HttpServerErrorException.class); assertThatThrownBy(() -> adapter.getRate("USD", "EUR"))
- .isInstanceOf(Exception.class);
+ .isInstanceOf(org.springframework.web.client.HttpClientErrorException.class); assertThatThrownBy(() -> adapter.getRate("USD", "EUR"))
- .isInstanceOf(Exception.class);
+ .isInstanceOf(org.springframework.web.client.ResourceAccessException.class);📝 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
| @Test | |
| @DisplayName("should throw on server error 500") | |
| void shouldThrowOnServerError() { | |
| wireMock.stubFor(get(urlEqualTo("/latest?from=USD&to=EUR")) | |
| .willReturn(aResponse().withStatus(500))); | |
| assertThatThrownBy(() -> adapter.getRate("USD", "EUR")) | |
| .isInstanceOf(Exception.class); | |
| } | |
| @Test | |
| @DisplayName("should throw on client error 404") | |
| void shouldThrowOnClientError() { | |
| wireMock.stubFor(get(urlEqualTo("/latest?from=USD&to=EUR")) | |
| .willReturn(aResponse().withStatus(404))); | |
| assertThatThrownBy(() -> adapter.getRate("USD", "EUR")) | |
| .isInstanceOf(Exception.class); | |
| } | |
| @Test | |
| @DisplayName("should throw on read timeout") | |
| void shouldThrowOnReadTimeout() { | |
| wireMock.stubFor(get(urlEqualTo("/latest?from=USD&to=EUR")) | |
| .willReturn(aResponse() | |
| .withStatus(200) | |
| .withHeader("Content-Type", "application/json") | |
| .withFixedDelay(5000) | |
| .withBody(""" | |
| { | |
| "amount": 1.0, | |
| "base": "USD", | |
| "date": "2026-03-17", | |
| "rates": { | |
| "EUR": 0.86723 | |
| } | |
| } | |
| """))); | |
| assertThatThrownBy(() -> adapter.getRate("USD", "EUR")) | |
| .isInstanceOf(Exception.class); | |
| } | |
| `@Test` | |
| `@DisplayName`("should throw on server error 500") | |
| void shouldThrowOnServerError() { | |
| wireMock.stubFor(get(urlEqualTo("/latest?from=USD&to=EUR")) | |
| .willReturn(aResponse().withStatus(500))); | |
| assertThatThrownBy(() -> adapter.getRate("USD", "EUR")) | |
| .isInstanceOf(org.springframework.web.client.HttpServerErrorException.class); | |
| } | |
| `@Test` | |
| `@DisplayName`("should throw on client error 404") | |
| void shouldThrowOnClientError() { | |
| wireMock.stubFor(get(urlEqualTo("/latest?from=USD&to=EUR")) | |
| .willReturn(aResponse().withStatus(404))); | |
| assertThatThrownBy(() -> adapter.getRate("USD", "EUR")) | |
| .isInstanceOf(org.springframework.web.client.HttpClientErrorException.class); | |
| } | |
| `@Test` | |
| `@DisplayName`("should throw on read timeout") | |
| void shouldThrowOnReadTimeout() { | |
| wireMock.stubFor(get(urlEqualTo("/latest?from=USD&to=EUR")) | |
| .willReturn(aResponse() | |
| .withStatus(200) | |
| .withHeader("Content-Type", "application/json") | |
| .withFixedDelay(5000) | |
| .withBody(""" | |
| { | |
| "amount": 1.0, | |
| "base": "USD", | |
| "date": "2026-03-17", | |
| "rates": { | |
| "EUR": 0.86723 | |
| } | |
| } | |
| """))); | |
| assertThatThrownBy(() -> adapter.getRate("USD", "EUR")) | |
| .isInstanceOf(org.springframework.web.client.ResourceAccessException.class); | |
| } |
🤖 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/provider/frankfurter/FrankfurterRateAdapterTest.java`
around lines 191 - 232, The tests currently assert that adapter.getRate("USD",
"EUR") throws a generic Exception which is too broad; update the three tests
(shouldThrowOnServerError, shouldThrowOnClientError, shouldThrowOnReadTimeout)
to assert the specific exception types thrown by the HTTP client used by
FrankfurterRateAdapter — e.g., assert server 500 triggers
HttpServerErrorException (or the adapter's mapped exception), 404 triggers
HttpClientErrorException (or the adapter's mapped exception), and the fixedDelay
timeout triggers ResourceAccessException/ReadTimeoutException depending on the
HTTP client; locate the assertions around adapter.getRate and replace
isInstanceOf(Exception.class) with the appropriate specific exception class
names to improve test precision.
3 tasks
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
FrankfurterRateAdapterimplementingRateProviderport for S6 FX & Liquidity Engineapi.frankfurter.app— no API key required, replaces Refinitiv for sandbox/devapp.fx.rate-provider=frankfurter(@ConditionalOnProperty)@CircuitBreaker+@Retrywith fallback returningOptional.empty()Files
FrankfurterRateAdapter.java— RestClient with HTTP/1.1, read timeout from propertiesFrankfurterProperties.java—@ConfigurationPropertieswith defaults (baseUrl, readTimeoutMs)FrankfurterRateResponse.java— package-private ACL recordFrankfurterRateAdapterTest.java— 8 WireMock tests, single-assert patternapplication.yml— Resilience4j circuit breaker + retry config forfrankfurterTest plan
Closes #203
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Tests