feat(s4): chain selection engine — multi-chain scoring (STA-136)#128
Merged
Conversation
|
Caution Review failedPull request was closed or merged during review WalkthroughAdds a chain-selection subsystem: domain models and ports for weights, candidates, fees, and health; a ChainSelectionEngine implementing weighted multi-criteria selection (filters by wallet liquidity and chain health); Spring config and fallbacks; JDBC persistence for selection logs; and unit tests + fixtures. Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant Engine as ChainSelectionEngine
participant Health as ChainHealthProvider
participant Fee as ChainFeeProvider
participant WalletRepo as WalletRepository
participant BalanceRepo as WalletBalanceRepository
participant LogRepo as ChainSelectionLogRepository
Client->>+Engine: selectChain(request)
Engine->>+WalletRepo: find wallets for on-ramp
WalletRepo-->>-Engine: wallets
Engine->>+BalanceRepo: fetch balances for wallets
BalanceRepo-->>-Engine: balances
Engine->>Engine: filter candidates by wallet liquidity
loop for each candidate
Engine->>+Health: getHealthScore(chainId)
Health-->>-Engine: healthScore
Engine->>+Fee: estimateFeeUsd(chainId, stablecoin)
Fee-->>-Engine: feeUsd
Engine->>Engine: scoreCandidate(feeUsd, finality, health, weights)
end
Engine->>Engine: resolve selected candidate (preferred or max score)
Engine->>+LogRepo: save(transferId, ChainSelectionResult)
LogRepo-->>-Engine: persisted
Engine-->>-Client: ChainSelectionResult
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 |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Puneethkumarck
force-pushed
the
feature/STA-136-chain-selection-engine
branch
from
af0727d to
a5d7631
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 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/main/java/com/stablecoin/payments/custody/config/FallbackAdaptersConfig.java`:
- Around line 32-49: The fallback beans in FallbackAdaptersConfig (methods
fallbackChainHealthProvider and fallbackChainFeeProvider) are being registered
in prod if live providers are missing; mark them explicitly non-production by
adding a Spring profile restriction (e.g., annotate the methods or the class
with `@Profile`("!prod")) so these `@ConditionalOnMissingBean` fallbacks only load
outside prod, and keep the existing log lines to make activation visible.
In
`@blockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/domain/model/ChainSelectionResult.java`:
- Around line 19-30: The canonical constructor in ChainSelectionResult must
reject inconsistent states: add a check that selectedChain is contained in
candidates (throw IllegalArgumentException if not), and validate candidate
selection consistency by ensuring there is at most one candidate marked as
selected and, if a candidate-level selection flag exists (e.g.,
candidate.isSelected() or candidate.selected()), that the selected candidate
equals selectedChain; keep the existing null/empty checks and continue to copy
candidates via List.copyOf(candidates).
In
`@blockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/domain/model/ChainSelectionWeights.java`:
- Around line 23-37: The ChainSelectionWeights canonical constructor currently
skips NaN/Infinity checks so non-finite values can bypass range and sum
validation; before the existing negative checks for costWeight, speedWeight, and
reliabilityWeight (and before the SUM_TOLERANCE sum check), add
Double.isFinite(...) validations for each of costWeight, speedWeight and
reliabilityWeight and throw an IllegalArgumentException identifying the
offending field if any is not finite so poisoned (NaN/Infinity) weights cannot
propagate.
In
`@blockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/domain/port/ChainFeeProvider.java`:
- Around line 11-18: Update the ChainFeeProvider contract and runtime checks:
change the JavaDoc for ChainFeeProvider.estimateFeeUsd to require and document
that implementations must return a strictly positive, finite double (reject 0,
negative, NaN, or ±Infinity). Additionally, wherever estimateFeeUsd(...) is
consumed (the reciprocal scoring code), add a defensive check that throws an
IllegalStateException or IllegalArgumentException if the returned value is <= 0
or not finite before computing 1.0/fee to fail fast and avoid corrupting
rankings.
In
`@blockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/domain/port/ChainHealthProvider.java`:
- Around line 5-18: The ChainHealthProvider contract currently forces a binary
health score so reliabilityWeight * healthScore never affects selection; either
change the port to return a continuous bounded score (e.g. document and enforce
getHealthScore(ChainId) -> double in [0.0,1.0]) and update all callers to use
that continuous value in the scoring formula, or remove the reliability term
from the weighted scoring calculation wherever reliabilityWeight and
getHealthScore are used; specifically modify the ChainHealthProvider interface
method getHealthScore to accept/return a normalized continuous score and update
any scorer (the code that multiplies reliabilityWeight * healthScore) to use the
new range, or eliminate reliabilityWeight from the scorer to avoid a no-op.
In
`@blockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/domain/port/ChainSelectionLogRepository.java`:
- Around line 12-18: The port method save currently takes both UUID transferId
and ChainSelectionResult, but ChainSelectionResult already contains the
transferId, creating a potential consistency bug; either remove the redundant
transferId parameter from ChainSelectionLogRepository.save and update all
callers to call save(result) (or save(result.transferId(), result) signature ->
save(result) API change), or if you must keep the signature add a fast-fail
validation inside implementations: in ChainSelectionLogRepository.save(UUID
transferId, ChainSelectionResult result) assert
transferId.equals(result.transferId()) and throw an IllegalArgumentException (or
another unchecked exception) if they differ so callers cannot persist mismatched
IDs.
In
`@blockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/domain/service/ChainSelectionEngine.java`:
- Around line 106-109: The stream currently calls
chainHealthProvider.getHealthScore twice (once in the filter and again inside
scoreCandidate), causing duplicate provider calls and inconsistent snapshots;
change the pipeline to compute and attach the health score once per candidate
(e.g., map each config to a tuple/holder containing config and healthScore
immediately after hasWalletWithSufficientBalance), then filter by healthScore >
0 and pass that cached healthScore into scoreCandidate (update scoreCandidate
signature or add a helper that accepts the precomputed health score); apply the
same caching refactor for the other stream block around scoreCandidate at lines
155-164 to avoid repeated getHealthScore calls.
- Around line 187-189: The selection currently uses
candidates.stream().max(Comparator.comparingDouble(ChainCandidate::score)) which
leaves tie-breaking to encounter order (from MVP_CHAINS.values()) and can be
non-deterministic; update the comparator in ChainSelectionEngine to include an
explicit deterministic secondary key — e.g., replace
Comparator.comparingDouble(ChainCandidate::score) with a comparator that does
.thenComparing(...) using a stable attribute (like a lexicographic chainId/name)
or consults an explicit priority map keyed by the chain identifier to break
ties, keeping the same orElseThrow(ChainUnavailableException...) behavior.
In
`@blockchain-custody/blockchain-custody/src/test/java/com/stablecoin/payments/custody/domain/service/ChainSelectionEngineTest.java`:
- Around line 258-274: The test
selectChain_preferredChainUnhealthy_fallsBackToScoring currently only asserts
the result is not CHAIN_BASE, which allows an incorrect fallback to
CHAIN_ETHEREUM; update the assertion after calling sut.selectChain(...) to
assert the deterministic winner (CHAIN_SOLANA) instead of a negative check.
Locate the test method and replace the final
assertThat(result.selectedChain()).isNotEqualTo(CHAIN_BASE) with an assertion
that the selectedChain equals CHAIN_SOLANA, keeping the existing stubs of
chainHealthProvider and chainFeeProvider that make Solana the expected scorer.
- Around line 113-452: The test suite lacks negative-path tests for the public
ChainSelectionEngine.ChainSelectionRequest constructor; add new unit tests in
ChainSelectionEngineTest that exercise its validation branches by asserting that
constructing ChainSelectionRequest with an invalid transferId (null or empty), a
null/invalid stablecoin, and non-positive amounts (zero and negative BigDecimal)
each throw the expected exception (e.g., IllegalArgumentException or the
domain-specific validation exception); reference the nested type
ChainSelectionEngine.ChainSelectionRequest and ensure each test uses
assertThatThrownBy (or your project's preferred assertion) to verify the
message/exception type for transferId, stablecoin, and amount validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1ea3eb8c-3fc7-46b7-8432-cac9ae820c03
📒 Files selected for processing (13)
blockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/application/config/ChainSelectionConfig.javablockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/config/FallbackAdaptersConfig.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/model/ChainCandidate.javablockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/domain/model/ChainSelectionResult.javablockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/domain/model/ChainSelectionWeights.javablockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/domain/port/ChainFeeProvider.javablockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/domain/port/ChainHealthProvider.javablockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/domain/port/ChainSelectionLogRepository.javablockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/domain/service/ChainSelectionEngine.javablockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/infrastructure/persistence/ChainSelectionLogPersistenceAdapter.javablockchain-custody/blockchain-custody/src/test/java/com/stablecoin/payments/custody/domain/service/ChainSelectionEngineTest.javablockchain-custody/blockchain-custody/src/testFixtures/java/com/stablecoin/payments/custody/fixtures/ChainSelectionFixtures.java
Comment on lines
+32
to
+49
| @Bean | ||
| @ConditionalOnMissingBean | ||
| public ChainHealthProvider fallbackChainHealthProvider() { | ||
| log.info("Using fallback ChainHealthProvider (all chains healthy)"); | ||
| return (ChainId chainId) -> 1.0; | ||
| } | ||
|
|
||
| /** | ||
| * Fallback fee provider with realistic defaults: | ||
| * Base=0.01, Ethereum=2.50, Solana=0.005 USD. | ||
| */ | ||
| @Bean | ||
| @ConditionalOnMissingBean | ||
| public ChainFeeProvider fallbackChainFeeProvider() { | ||
| log.info("Using fallback ChainFeeProvider (static fee estimates)"); | ||
| return (ChainId chainId, StablecoinTicker stablecoin) -> | ||
| DEFAULT_FEES.getOrDefault(chainId.value(), 1.0); | ||
| } |
There was a problem hiding this comment.
Make these fallback adapters explicitly non-prod.
@ConditionalOnMissingBean alone will also activate this code in a misconfigured production boot. In that state the selector silently routes with healthScore=1.0 and static fees instead of live provider data.
Suggested fix
import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean;
+import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;
`@Slf4j`
`@Configuration`
+@ConditionalOnProperty(
+ value = "app.custody.chain-selection.fallback-enabled",
+ havingValue = "true"
+)
public class FallbackAdaptersConfig {🤖 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/config/FallbackAdaptersConfig.java`
around lines 32 - 49, The fallback beans in FallbackAdaptersConfig (methods
fallbackChainHealthProvider and fallbackChainFeeProvider) are being registered
in prod if live providers are missing; mark them explicitly non-production by
adding a Spring profile restriction (e.g., annotate the methods or the class
with `@Profile`("!prod")) so these `@ConditionalOnMissingBean` fallbacks only load
outside prod, and keep the existing log lines to make activation visible.
Comment on lines
+19
to
+30
| public ChainSelectionResult { | ||
| if (selectedChain == null) { | ||
| throw new IllegalArgumentException("selectedChain is required"); | ||
| } | ||
| if (candidates == null || candidates.isEmpty()) { | ||
| throw new IllegalArgumentException("candidates must not be empty"); | ||
| } | ||
| if (transferId == null) { | ||
| throw new IllegalArgumentException("transferId is required"); | ||
| } | ||
| candidates = List.copyOf(candidates); | ||
| } |
There was a problem hiding this comment.
Enforce result/candidate consistency in the canonical constructor.
Right now this record accepts impossible states, such as a selectedChain that is absent from candidates or multiple candidates marked as selected. Since this object is the persisted audit payload, those inconsistencies should be rejected at construction time.
Suggested invariant checks
public ChainSelectionResult {
if (selectedChain == null) {
throw new IllegalArgumentException("selectedChain is required");
}
if (candidates == null || candidates.isEmpty()) {
throw new IllegalArgumentException("candidates must not be empty");
}
+ if (candidates.stream().anyMatch(c -> c == null)) {
+ throw new IllegalArgumentException("candidates must not contain null entries");
+ }
+ long selectedCount = candidates.stream().filter(ChainCandidate::selected).count();
+ if (selectedCount != 1) {
+ throw new IllegalArgumentException("candidates must contain exactly one selected entry");
+ }
+ if (candidates.stream().noneMatch(c -> selectedChain.equals(c.chainId()) && c.selected())) {
+ throw new IllegalArgumentException("selectedChain must match the selected candidate");
+ }
if (transferId == null) {
throw new IllegalArgumentException("transferId is required");
}
candidates = List.copyOf(candidates);
}📝 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 ChainSelectionResult { | |
| if (selectedChain == null) { | |
| throw new IllegalArgumentException("selectedChain is required"); | |
| } | |
| if (candidates == null || candidates.isEmpty()) { | |
| throw new IllegalArgumentException("candidates must not be empty"); | |
| } | |
| if (transferId == null) { | |
| throw new IllegalArgumentException("transferId is required"); | |
| } | |
| candidates = List.copyOf(candidates); | |
| } | |
| public ChainSelectionResult { | |
| if (selectedChain == null) { | |
| throw new IllegalArgumentException("selectedChain is required"); | |
| } | |
| if (candidates == null || candidates.isEmpty()) { | |
| throw new IllegalArgumentException("candidates must not be empty"); | |
| } | |
| if (candidates.stream().anyMatch(c -> c == null)) { | |
| throw new IllegalArgumentException("candidates must not contain null entries"); | |
| } | |
| long selectedCount = candidates.stream().filter(ChainCandidate::selected).count(); | |
| if (selectedCount != 1) { | |
| throw new IllegalArgumentException("candidates must contain exactly one selected entry"); | |
| } | |
| if (candidates.stream().noneMatch(c -> selectedChain.equals(c.chainId()) && c.selected())) { | |
| throw new IllegalArgumentException("selectedChain must match the selected candidate"); | |
| } | |
| if (transferId == null) { | |
| throw new IllegalArgumentException("transferId is required"); | |
| } | |
| candidates = List.copyOf(candidates); | |
| } |
🤖 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/domain/model/ChainSelectionResult.java`
around lines 19 - 30, The canonical constructor in ChainSelectionResult must
reject inconsistent states: add a check that selectedChain is contained in
candidates (throw IllegalArgumentException if not), and validate candidate
selection consistency by ensuring there is at most one candidate marked as
selected and, if a candidate-level selection flag exists (e.g.,
candidate.isSelected() or candidate.selected()), that the selected candidate
equals selectedChain; keep the existing null/empty checks and continue to copy
candidates via List.copyOf(candidates).
Comment on lines
+23
to
+37
| public ChainSelectionWeights { | ||
| if (costWeight < 0) { | ||
| throw new IllegalArgumentException("costWeight must be non-negative"); | ||
| } | ||
| if (speedWeight < 0) { | ||
| throw new IllegalArgumentException("speedWeight must be non-negative"); | ||
| } | ||
| if (reliabilityWeight < 0) { | ||
| throw new IllegalArgumentException("reliabilityWeight must be non-negative"); | ||
| } | ||
| double sum = costWeight + speedWeight + reliabilityWeight; | ||
| if (Math.abs(sum - 1.0) > SUM_TOLERANCE) { | ||
| throw new IllegalArgumentException( | ||
| "Weights must sum to ~1.0, but got %s".formatted(sum)); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Java, do comparisons with Double.NaN using < and > return false, and can Math.abs(Double.NaN - 1.0) > 0.001 also evaluate to false?
💡 Result:
Yes.
- In Java,
NaNis unordered, so the numerical comparison operators<,<=,>,>=returnfalseif either operand isNaN. Therefore, bothDouble.NaN < xandDouble.NaN > xevaluate tofalsefor anyx(includingNaN). [1] Double.NaN - 1.0evaluates toNaN, andMath.abs(NaN)returnsNaN. Then the comparisonNaN > 0.001is alsofalse, soMath.abs(Double.NaN - 1.0) > 0.001can indeed evaluate tofalse. [1][2]
Sources: Java Language Specification on NaN comparisons [1], and java.lang.Math.abs behavior with NaN [2].
[1] (jcp.org)
[2] (docs.oracle.com)
Citations:
- 1: https://jcp.org/aboutJava/communityprocess/maintenance/JLS/strictfp-changes.pdf?utm_source=openai
- 2: https://docs.oracle.com/javase/8/docs/api/java/lang/Math.html?utm_source=openai
Validate weights are finite to prevent NaN propagation.
The canonical constructor bypasses NaN validation. In Java, NaN < 0 returns false, and Math.abs(NaN - 1.0) > tolerance also returns false, allowing poisoned weights to propagate through all downstream score calculations.
Add Double.isFinite() checks before range validation:
Suggested fix
public ChainSelectionWeights {
+ if (!Double.isFinite(costWeight)) {
+ throw new IllegalArgumentException("costWeight must be finite");
+ }
+ if (!Double.isFinite(speedWeight)) {
+ throw new IllegalArgumentException("speedWeight must be finite");
+ }
+ if (!Double.isFinite(reliabilityWeight)) {
+ throw new IllegalArgumentException("reliabilityWeight must be finite");
+ }
if (costWeight < 0) {
throw new IllegalArgumentException("costWeight must be non-negative");
}🤖 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/domain/model/ChainSelectionWeights.java`
around lines 23 - 37, The ChainSelectionWeights canonical constructor currently
skips NaN/Infinity checks so non-finite values can bypass range and sum
validation; before the existing negative checks for costWeight, speedWeight, and
reliabilityWeight (and before the SUM_TOLERANCE sum check), add
Double.isFinite(...) validations for each of costWeight, speedWeight and
reliabilityWeight and throw an IllegalArgumentException identifying the
offending field if any is not finite so poisoned (NaN/Infinity) weights cannot
propagate.
Comment on lines
+11
to
+18
| /** | ||
| * Estimates the fee in USD for transferring a stablecoin on the given chain. | ||
| * | ||
| * @param chainId the target chain | ||
| * @param stablecoin the stablecoin to transfer | ||
| * @return estimated fee in USD | ||
| */ | ||
| double estimateFeeUsd(ChainId chainId, StablecoinTicker stablecoin); |
There was a problem hiding this comment.
Require strictly positive, finite fee estimates.
Line 18 feeds a reciprocal scoring term, so 0, negative, or non-finite fees will corrupt ranking instead of failing fast. Tighten the contract here and reject invalid values before scoring.
Suggested contract tightening
/**
* Estimates the fee in USD for transferring a stablecoin on the given chain.
+ * Implementations must return a finite value greater than 0.
*
* `@param` chainId the target chain
* `@param` stablecoin the stablecoin to transfer
- * `@return` estimated fee in USD
+ * `@return` estimated fee in USD, strictly greater than 0
*/🤖 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/domain/port/ChainFeeProvider.java`
around lines 11 - 18, Update the ChainFeeProvider contract and runtime checks:
change the JavaDoc for ChainFeeProvider.estimateFeeUsd to require and document
that implementations must return a strictly positive, finite double (reject 0,
negative, NaN, or ±Infinity). Additionally, wherever estimateFeeUsd(...) is
consumed (the reciprocal scoring code), add a defensive check that throws an
IllegalStateException or IllegalArgumentException if the returned value is <= 0
or not finite before computing 1.0/fee to fail fast and avoid corrupting
rankings.
Comment on lines
+5
to
+18
| /** | ||
| * Port for retrieving the current health score of a blockchain chain. | ||
| * <p> | ||
| * Returns 0.0 for unhealthy/down chains and 1.0 for healthy chains. | ||
| */ | ||
| public interface ChainHealthProvider { | ||
|
|
||
| /** | ||
| * Returns the health score for the given chain. | ||
| * | ||
| * @param chainId the chain to evaluate | ||
| * @return 0.0 (unhealthy) or 1.0 (healthy) | ||
| */ | ||
| double getHealthScore(ChainId chainId); |
There was a problem hiding this comment.
Make health scoring non-binary or remove it from the weighted model.
With the current contract, healthy chains always contribute the same reliability term. Since unhealthy chains are already filtered out before selection, reliabilityWeight * healthScore cannot change the argmax result at all. Either widen this port to a bounded continuous score (for example [0,1]) or drop reliability from the scoring formula.
Suggested contract update
- * Returns 0.0 for unhealthy/down chains and 1.0 for healthy chains.
+ * Returns a finite health score in the range [0.0, 1.0], where 0.0 means
+ * unavailable and higher values indicate better current chain health.
@@
- * `@return` 0.0 (unhealthy) or 1.0 (healthy)
+ * `@return` health score in [0.0, 1.0]📝 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
| /** | |
| * Port for retrieving the current health score of a blockchain chain. | |
| * <p> | |
| * Returns 0.0 for unhealthy/down chains and 1.0 for healthy chains. | |
| */ | |
| public interface ChainHealthProvider { | |
| /** | |
| * Returns the health score for the given chain. | |
| * | |
| * @param chainId the chain to evaluate | |
| * @return 0.0 (unhealthy) or 1.0 (healthy) | |
| */ | |
| double getHealthScore(ChainId chainId); | |
| /** | |
| * Port for retrieving the current health score of a blockchain chain. | |
| * <p> | |
| * Returns a finite health score in the range [0.0, 1.0], where 0.0 means | |
| * unavailable and higher values indicate better current chain health. | |
| */ | |
| public interface ChainHealthProvider { | |
| /** | |
| * Returns the health score for the given chain. | |
| * | |
| * `@param` chainId the chain to evaluate | |
| * `@return` health score in [0.0, 1.0] | |
| */ | |
| double getHealthScore(ChainId chainId); |
🤖 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/domain/port/ChainHealthProvider.java`
around lines 5 - 18, The ChainHealthProvider contract currently forces a binary
health score so reliabilityWeight * healthScore never affects selection; either
change the port to return a continuous bounded score (e.g. document and enforce
getHealthScore(ChainId) -> double in [0.0,1.0]) and update all callers to use
that continuous value in the scoring formula, or remove the reliability term
from the weighted scoring calculation wherever reliabilityWeight and
getHealthScore are used; specifically modify the ChainHealthProvider interface
method getHealthScore to accept/return a normalized continuous score and update
any scorer (the code that multiplies reliabilityWeight * healthScore) to use the
new range, or eliminate reliabilityWeight from the scorer to avoid a no-op.
Comment on lines
+12
to
+18
| /** | ||
| * Saves the chain selection result for the given transfer. | ||
| * | ||
| * @param transferId the transfer for which chain was selected | ||
| * @param result the selection result including all scored candidates | ||
| */ | ||
| void save(UUID transferId, ChainSelectionResult result); |
There was a problem hiding this comment.
Remove the duplicated transfer identifier from the port contract.
ChainSelectionResult already carries transferId, but Line 18 requires callers to pass it again. That opens a consistency bug where the persisted transfer id and the serialized result describe different transfers.
Suggested API simplification
- void save(UUID transferId, ChainSelectionResult result);
+ void save(ChainSelectionResult result);If you keep the current signature, the implementation should at least fail fast when !transferId.equals(result.transferId()).
🤖 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/domain/port/ChainSelectionLogRepository.java`
around lines 12 - 18, The port method save currently takes both UUID transferId
and ChainSelectionResult, but ChainSelectionResult already contains the
transferId, creating a potential consistency bug; either remove the redundant
transferId parameter from ChainSelectionLogRepository.save and update all
callers to call save(result) (or save(result.transferId(), result) signature ->
save(result) API change), or if you must keep the signature add a fast-fail
validation inside implementations: in ChainSelectionLogRepository.save(UUID
transferId, ChainSelectionResult result) assert
transferId.equals(result.transferId()) and throw an IllegalArgumentException (or
another unchecked exception) if they differ so callers cannot persist mismatched
IDs.
Comment on lines
+106
to
+109
| var scoredCandidates = candidateConfigs.stream() | ||
| .filter(config -> hasWalletWithSufficientBalance(config.chainId(), request.stablecoin(), request.amount())) | ||
| .filter(config -> chainHealthProvider.getHealthScore(config.chainId()) > 0) | ||
| .map(config -> scoreCandidate(config, request.stablecoin())) |
There was a problem hiding this comment.
Cache chain health before scoring.
chainHealthProvider is invoked once in the filter and again in scoreCandidate. That doubles provider traffic and can score a different health snapshot than the one used to admit the candidate.
Suggested fix
- var scoredCandidates = candidateConfigs.stream()
- .filter(config -> hasWalletWithSufficientBalance(config.chainId(), request.stablecoin(), request.amount()))
- .filter(config -> chainHealthProvider.getHealthScore(config.chainId()) > 0)
- .map(config -> scoreCandidate(config, request.stablecoin()))
+ var scoredCandidates = candidateConfigs.stream()
+ .filter(config -> hasWalletWithSufficientBalance(config.chainId(), request.stablecoin(), request.amount()))
+ .map(config -> Map.entry(config, chainHealthProvider.getHealthScore(config.chainId())))
+ .filter(entry -> entry.getValue() > 0)
+ .map(entry -> scoreCandidate(entry.getKey(), request.stablecoin(), entry.getValue()))
.toList();
@@
- private ChainCandidate scoreCandidate(ChainConfig config, StablecoinTicker stablecoin) {
+ private ChainCandidate scoreCandidate(ChainConfig config, StablecoinTicker stablecoin, double healthScore) {
double feeUsd = chainFeeProvider.estimateFeeUsd(config.chainId(), stablecoin);
- double healthScore = chainHealthProvider.getHealthScore(config.chainId());
double costScore = feeUsd > 0 ? 1.0 / feeUsd : 0.0;Also applies to: 155-164
🤖 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/domain/service/ChainSelectionEngine.java`
around lines 106 - 109, The stream currently calls
chainHealthProvider.getHealthScore twice (once in the filter and again inside
scoreCandidate), causing duplicate provider calls and inconsistent snapshots;
change the pipeline to compute and attach the health score once per candidate
(e.g., map each config to a tuple/holder containing config and healthScore
immediately after hasWalletWithSufficientBalance), then filter by healthScore >
0 and pass that cached healthScore into scoreCandidate (update scoreCandidate
signature or add a helper that accepts the precomputed health score); apply the
same caching refactor for the other stream block around scoreCandidate at lines
155-164 to avoid repeated getHealthScore calls.
Comment on lines
+187
to
+189
| return candidates.stream() | ||
| .max(Comparator.comparingDouble(ChainCandidate::score)) | ||
| .orElseThrow(() -> new ChainUnavailableException("No candidates to select from")); |
There was a problem hiding this comment.
Make equal-score selection deterministic.
max(comparingDouble(score)) falls back to stream encounter order on ties. Because the candidate stream comes from MVP_CHAINS.values(), an exact tie can resolve differently across JVMs or future refactors.
Suggested fix
- return candidates.stream()
- .max(Comparator.comparingDouble(ChainCandidate::score))
+ return candidates.stream()
+ .max(Comparator.comparingDouble(ChainCandidate::score)
+ .thenComparing(candidate -> candidate.chainId().value()))
.orElseThrow(() -> new ChainUnavailableException("No candidates to select from"));📝 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
| return candidates.stream() | |
| .max(Comparator.comparingDouble(ChainCandidate::score)) | |
| .orElseThrow(() -> new ChainUnavailableException("No candidates to select from")); | |
| return candidates.stream() | |
| .max(Comparator.comparingDouble(ChainCandidate::score) | |
| .thenComparing(candidate -> candidate.chainId().value())) | |
| .orElseThrow(() -> new ChainUnavailableException("No candidates to select from")); |
🤖 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/domain/service/ChainSelectionEngine.java`
around lines 187 - 189, The selection currently uses
candidates.stream().max(Comparator.comparingDouble(ChainCandidate::score)) which
leaves tie-breaking to encounter order (from MVP_CHAINS.values()) and can be
non-deterministic; update the comparator in ChainSelectionEngine to include an
explicit deterministic secondary key — e.g., replace
Comparator.comparingDouble(ChainCandidate::score) with a comparator that does
.thenComparing(...) using a stable attribute (like a lexicographic chainId/name)
or consults an explicit priority map keyed by the chain identifier to break
ties, keeping the same orElseThrow(ChainUnavailableException...) behavior.
Comment on lines
+113
to
+452
| @Test | ||
| void selectChain_baseWinsOnLowCost() { | ||
| // Base: fee=0.01 (cheapest for its finality); Solana fee=0.005 but speed dominates less with default weights | ||
| // Default weights: cost=0.4, speed=0.35, reliability=0.25 | ||
| // Base: 0.4*(1/0.01) + 0.35*(1/12) + 0.25*1.0 = 40.0 + 0.0292 + 0.25 = 40.279 | ||
| // Ethereum: 0.4*(1/2.50) + 0.35*(1/300) + 0.25*1.0 = 0.16 + 0.00117 + 0.25 = 0.411 | ||
| // Solana: 0.4*(1/0.005) + 0.35*(1/5) + 0.25*1.0 = 80.0 + 0.07 + 0.25 = 80.32 | ||
| // Actually Solana wins! Let me adjust fees to make Base win: | ||
| // Make Solana more expensive and Base cheapest with speed factoring in | ||
| var weights = ChainSelectionWeights.builder() | ||
| .costWeight(0.5) | ||
| .speedWeight(0.25) | ||
| .reliabilityWeight(0.25) | ||
| .build(); | ||
| var sut = engineWith(weights); | ||
|
|
||
| stubAllChainsWithSufficientBalance(); | ||
| stubAllChainsHealthy(); | ||
| // Set fees so Base wins: Base cheap with decent speed | ||
| given(chainFeeProvider.estimateFeeUsd(CHAIN_BASE, USDC)).willReturn(0.01); | ||
| given(chainFeeProvider.estimateFeeUsd(CHAIN_ETHEREUM, USDC)).willReturn(2.50); | ||
| given(chainFeeProvider.estimateFeeUsd(CHAIN_SOLANA, USDC)).willReturn(0.02); | ||
|
|
||
| var result = sut.selectChain(aSelectionRequest()); | ||
|
|
||
| var expected = ChainSelectionResult.builder() | ||
| .selectedChain(CHAIN_BASE) | ||
| .transferId(TRANSFER_ID) | ||
| .candidates(result.candidates()) | ||
| .build(); | ||
| assertThat(result).usingRecursiveComparison() | ||
| .ignoringFields("candidates") | ||
| .isEqualTo(expected); | ||
| } | ||
|
|
||
| @Test | ||
| void selectChain_solanaWinsOnSpeed() { | ||
| // Speed weight dominates | ||
| var weights = ChainSelectionWeights.builder() | ||
| .costWeight(0.05) | ||
| .speedWeight(0.9) | ||
| .reliabilityWeight(0.05) | ||
| .build(); | ||
| var sut = engineWith(weights); | ||
|
|
||
| stubAllChainsWithSufficientBalance(); | ||
| stubAllChainsHealthy(); | ||
| stubAllChainsFees(); | ||
|
|
||
| var result = sut.selectChain(aSelectionRequest()); | ||
|
|
||
| // Solana has 5s finality (fastest) → highest speed score | ||
| var expected = ChainSelectionResult.builder() | ||
| .selectedChain(CHAIN_SOLANA) | ||
| .transferId(TRANSFER_ID) | ||
| .candidates(result.candidates()) | ||
| .build(); | ||
| assertThat(result).usingRecursiveComparison() | ||
| .ignoringFields("candidates") | ||
| .isEqualTo(expected); | ||
| } | ||
|
|
||
| @Test | ||
| void selectChain_ethereumWinsForHighValue() { | ||
| // Only Ethereum has sufficient balance for a large transfer | ||
| var weights = ChainSelectionWeights.defaults(); | ||
| var sut = engineWith(weights); | ||
| var highAmount = new BigDecimal("500000.00"); | ||
|
|
||
| // Only Ethereum has sufficient balance | ||
| stubChainWithBalance(CHAIN_ETHEREUM, highAmount, true); | ||
| stubChainWithBalance(CHAIN_BASE, highAmount, false); | ||
| stubChainWithBalance(CHAIN_SOLANA, highAmount, false); | ||
| given(chainHealthProvider.getHealthScore(CHAIN_ETHEREUM)).willReturn(1.0); | ||
| given(chainFeeProvider.estimateFeeUsd(CHAIN_ETHEREUM, USDC)).willReturn(2.50); | ||
|
|
||
| var result = sut.selectChain(aSelectionRequestWithAmount(highAmount)); | ||
|
|
||
| var expected = ChainSelectionResult.builder() | ||
| .selectedChain(CHAIN_ETHEREUM) | ||
| .transferId(TRANSFER_ID) | ||
| .candidates(result.candidates()) | ||
| .build(); | ||
| assertThat(result).usingRecursiveComparison() | ||
| .ignoringFields("candidates") | ||
| .isEqualTo(expected); | ||
| } | ||
|
|
||
| @Test | ||
| void selectChain_unhealthyChainExcluded() { | ||
| var weights = ChainSelectionWeights.defaults(); | ||
| var sut = engineWith(weights); | ||
|
|
||
| stubAllChainsWithSufficientBalance(); | ||
| // Base is unhealthy | ||
| given(chainHealthProvider.getHealthScore(CHAIN_BASE)).willReturn(0.0); | ||
| given(chainHealthProvider.getHealthScore(CHAIN_ETHEREUM)).willReturn(1.0); | ||
| given(chainHealthProvider.getHealthScore(CHAIN_SOLANA)).willReturn(1.0); | ||
| given(chainFeeProvider.estimateFeeUsd(CHAIN_ETHEREUM, USDC)).willReturn(2.50); | ||
| given(chainFeeProvider.estimateFeeUsd(CHAIN_SOLANA, USDC)).willReturn(0.005); | ||
|
|
||
| var result = sut.selectChain(aSelectionRequest()); | ||
|
|
||
| // Base should not be in candidates at all | ||
| assertThat(result.candidates().stream().map(c -> c.chainId().value()).toList()) | ||
| .doesNotContain("base"); | ||
| } | ||
|
|
||
| @Test | ||
| void selectChain_allChainsUnhealthy_throwsException() { | ||
| var weights = ChainSelectionWeights.defaults(); | ||
| var sut = engineWith(weights); | ||
|
|
||
| stubAllChainsWithSufficientBalance(); | ||
| given(chainHealthProvider.getHealthScore(CHAIN_BASE)).willReturn(0.0); | ||
| given(chainHealthProvider.getHealthScore(CHAIN_ETHEREUM)).willReturn(0.0); | ||
| given(chainHealthProvider.getHealthScore(CHAIN_SOLANA)).willReturn(0.0); | ||
|
|
||
| assertThatThrownBy(() -> sut.selectChain(aSelectionRequest())) | ||
| .isInstanceOf(ChainUnavailableException.class) | ||
| .hasMessageContaining("No healthy chain"); | ||
| } | ||
|
|
||
| @Test | ||
| void selectChain_preferredChainSelected() { | ||
| var weights = ChainSelectionWeights.defaults(); | ||
| var sut = engineWith(weights); | ||
|
|
||
| stubAllChainsWithSufficientBalance(); | ||
| stubAllChainsHealthy(); | ||
| stubAllChainsFees(); | ||
|
|
||
| var result = sut.selectChain(aSelectionRequestWithPreferredChain("base")); | ||
|
|
||
| var expected = ChainSelectionResult.builder() | ||
| .selectedChain(CHAIN_BASE) | ||
| .transferId(TRANSFER_ID) | ||
| .candidates(result.candidates()) | ||
| .build(); | ||
| assertThat(result).usingRecursiveComparison() | ||
| .ignoringFields("candidates") | ||
| .isEqualTo(expected); | ||
| } | ||
|
|
||
| @Test | ||
| void selectChain_preferredChainUnhealthy_fallsBackToScoring() { | ||
| var weights = ChainSelectionWeights.defaults(); | ||
| var sut = engineWith(weights); | ||
|
|
||
| stubAllChainsWithSufficientBalance(); | ||
| // Base is unhealthy but is preferred | ||
| given(chainHealthProvider.getHealthScore(CHAIN_BASE)).willReturn(0.0); | ||
| given(chainHealthProvider.getHealthScore(CHAIN_ETHEREUM)).willReturn(1.0); | ||
| given(chainHealthProvider.getHealthScore(CHAIN_SOLANA)).willReturn(1.0); | ||
| given(chainFeeProvider.estimateFeeUsd(CHAIN_ETHEREUM, USDC)).willReturn(2.50); | ||
| given(chainFeeProvider.estimateFeeUsd(CHAIN_SOLANA, USDC)).willReturn(0.005); | ||
|
|
||
| var result = sut.selectChain(aSelectionRequestWithPreferredChain("base")); | ||
|
|
||
| // Base is unhealthy, so it falls back to scoring. Solana should win. | ||
| assertThat(result.selectedChain()).isNotEqualTo(CHAIN_BASE); | ||
| } | ||
|
|
||
| @Test | ||
| void selectChain_insufficientBalance_chainExcluded() { | ||
| var weights = ChainSelectionWeights.defaults(); | ||
| var sut = engineWith(weights); | ||
|
|
||
| // Base has insufficient balance | ||
| stubChainWithBalance(CHAIN_BASE, AMOUNT, false); | ||
| stubChainWithBalance(CHAIN_ETHEREUM, AMOUNT, true); | ||
| stubChainWithBalance(CHAIN_SOLANA, AMOUNT, true); | ||
| given(chainHealthProvider.getHealthScore(CHAIN_ETHEREUM)).willReturn(1.0); | ||
| given(chainHealthProvider.getHealthScore(CHAIN_SOLANA)).willReturn(1.0); | ||
| given(chainFeeProvider.estimateFeeUsd(CHAIN_ETHEREUM, USDC)).willReturn(2.50); | ||
| given(chainFeeProvider.estimateFeeUsd(CHAIN_SOLANA, USDC)).willReturn(0.005); | ||
|
|
||
| var result = sut.selectChain(aSelectionRequest()); | ||
|
|
||
| assertThat(result.candidates().stream().map(c -> c.chainId().value()).toList()) | ||
| .doesNotContain("base"); | ||
| } | ||
|
|
||
| @Test | ||
| void selectChain_logsSelectionResult() { | ||
| var weights = ChainSelectionWeights.defaults(); | ||
| var sut = engineWith(weights); | ||
|
|
||
| stubAllChainsWithSufficientBalance(); | ||
| stubAllChainsHealthy(); | ||
| stubAllChainsFees(); | ||
|
|
||
| var result = sut.selectChain(aSelectionRequest()); | ||
|
|
||
| then(chainSelectionLogRepository).should().save(TRANSFER_ID, result); | ||
| } | ||
|
|
||
| @Test | ||
| void selectChain_allCandidatesScored() { | ||
| var weights = ChainSelectionWeights.defaults(); | ||
| var sut = engineWith(weights); | ||
|
|
||
| stubAllChainsWithSufficientBalance(); | ||
| stubAllChainsHealthy(); | ||
| stubAllChainsFees(); | ||
|
|
||
| var result = sut.selectChain(aSelectionRequest()); | ||
|
|
||
| // All 3 MVP chains should appear as candidates | ||
| assertThat(result.candidates()).hasSize(3); | ||
| } | ||
|
|
||
| @Test | ||
| void selectChain_singleHealthyChain_selectedByDefault() { | ||
| var weights = ChainSelectionWeights.defaults(); | ||
| var sut = engineWith(weights); | ||
|
|
||
| // Only Ethereum is healthy and has balance | ||
| stubChainWithBalance(CHAIN_BASE, AMOUNT, false); | ||
| stubChainWithBalance(CHAIN_ETHEREUM, AMOUNT, true); | ||
| stubChainWithBalance(CHAIN_SOLANA, AMOUNT, false); | ||
| given(chainHealthProvider.getHealthScore(CHAIN_ETHEREUM)).willReturn(1.0); | ||
| given(chainFeeProvider.estimateFeeUsd(CHAIN_ETHEREUM, USDC)).willReturn(2.50); | ||
|
|
||
| var result = sut.selectChain(aSelectionRequest()); | ||
|
|
||
| var expected = ChainSelectionResult.builder() | ||
| .selectedChain(CHAIN_ETHEREUM) | ||
| .transferId(TRANSFER_ID) | ||
| .candidates(result.candidates()) | ||
| .build(); | ||
| assertThat(result).usingRecursiveComparison() | ||
| .ignoringFields("candidates") | ||
| .isEqualTo(expected); | ||
| } | ||
|
|
||
| @Test | ||
| void selectChain_noWalletForChain_chainExcluded() { | ||
| var weights = ChainSelectionWeights.defaults(); | ||
| var sut = engineWith(weights); | ||
|
|
||
| // Base has no wallet at all | ||
| given(walletRepository.findByChainIdAndPurpose(CHAIN_BASE, WalletPurpose.ON_RAMP)) | ||
| .willReturn(List.of()); | ||
| stubChainWithBalance(CHAIN_ETHEREUM, AMOUNT, true); | ||
| stubChainWithBalance(CHAIN_SOLANA, AMOUNT, true); | ||
| given(chainHealthProvider.getHealthScore(CHAIN_ETHEREUM)).willReturn(1.0); | ||
| given(chainHealthProvider.getHealthScore(CHAIN_SOLANA)).willReturn(1.0); | ||
| given(chainFeeProvider.estimateFeeUsd(CHAIN_ETHEREUM, USDC)).willReturn(2.50); | ||
| given(chainFeeProvider.estimateFeeUsd(CHAIN_SOLANA, USDC)).willReturn(0.005); | ||
|
|
||
| var result = sut.selectChain(aSelectionRequest()); | ||
|
|
||
| assertThat(result.candidates().stream().map(c -> c.chainId().value()).toList()) | ||
| .doesNotContain("base"); | ||
| } | ||
|
|
||
| @Test | ||
| void selectChain_scoringFormula_correctCalculation() { | ||
| var weights = ChainSelectionWeights.defaults(); | ||
| var sut = engineWith(weights); | ||
|
|
||
| // Only Base is available to make the test deterministic | ||
| stubChainWithBalance(CHAIN_BASE, AMOUNT, true); | ||
| stubChainWithBalance(CHAIN_ETHEREUM, AMOUNT, false); | ||
| stubChainWithBalance(CHAIN_SOLANA, AMOUNT, false); | ||
| given(chainHealthProvider.getHealthScore(CHAIN_BASE)).willReturn(1.0); | ||
| given(chainFeeProvider.estimateFeeUsd(CHAIN_BASE, USDC)).willReturn(0.01); | ||
|
|
||
| var result = sut.selectChain(aSelectionRequest()); | ||
|
|
||
| // Expected score: 0.4*(1/0.01) + 0.35*(1/12) + 0.25*1.0 | ||
| // = 0.4*100 + 0.35*0.08333 + 0.25 | ||
| // = 40.0 + 0.02917 + 0.25 = 40.27917 | ||
| double expectedScore = 0.4 * (1.0 / 0.01) + 0.35 * (1.0 / 12) + 0.25 * 1.0; | ||
|
|
||
| var expectedCandidate = ChainCandidate.builder() | ||
| .chainId(CHAIN_BASE) | ||
| .feeUsd(0.01) | ||
| .finalitySeconds(12) | ||
| .healthScore(1.0) | ||
| .score(expectedScore) | ||
| .selected(true) | ||
| .build(); | ||
|
|
||
| assertThat(result.candidates()).hasSize(1); | ||
| assertThat(result.candidates().getFirst()).usingRecursiveComparison() | ||
| .withComparatorForType(Double::compare, Double.class) | ||
| .isEqualTo(expectedCandidate); | ||
| } | ||
|
|
||
| @Test | ||
| void selectChain_resultContainsSelectedFlag() { | ||
| var weights = ChainSelectionWeights.defaults(); | ||
| var sut = engineWith(weights); | ||
|
|
||
| stubAllChainsWithSufficientBalance(); | ||
| stubAllChainsHealthy(); | ||
| stubAllChainsFees(); | ||
|
|
||
| var result = sut.selectChain(aSelectionRequest()); | ||
|
|
||
| // Exactly one candidate should have selected=true | ||
| long selectedCount = result.candidates().stream().filter(ChainCandidate::selected).count(); | ||
| assertThat(selectedCount).isEqualTo(1L); | ||
| // The selected candidate's chainId should match result.selectedChain() | ||
| var selectedCandidate = result.candidates().stream() | ||
| .filter(ChainCandidate::selected) | ||
| .findFirst() | ||
| .orElseThrow(); | ||
| assertThat(selectedCandidate.chainId()).isEqualTo(result.selectedChain()); | ||
| } | ||
|
|
||
| @Test | ||
| void selectChain_costWeightZero_speedDominates() { | ||
| // Zero cost weight, high speed weight → fastest chain wins | ||
| var weights = ChainSelectionWeights.builder() | ||
| .costWeight(0.0) | ||
| .speedWeight(0.75) | ||
| .reliabilityWeight(0.25) | ||
| .build(); | ||
| var sut = engineWith(weights); | ||
|
|
||
| stubAllChainsWithSufficientBalance(); | ||
| stubAllChainsHealthy(); | ||
| stubAllChainsFees(); | ||
|
|
||
| var result = sut.selectChain(aSelectionRequest()); | ||
|
|
||
| // Solana has 5s finality (fastest) → highest speed score → should win | ||
| var expected = ChainSelectionResult.builder() | ||
| .selectedChain(CHAIN_SOLANA) | ||
| .transferId(TRANSFER_ID) | ||
| .candidates(result.candidates()) | ||
| .build(); | ||
| assertThat(result).usingRecursiveComparison() | ||
| .ignoringFields("candidates") | ||
| .isEqualTo(expected); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add negative-path coverage for ChainSelectionRequest.
The public ChainSelectionEngine.ChainSelectionRequest constructor validates transferId, stablecoin, and amount, but none of those branches are exercised here.
Suggested tests
+ `@Test`
+ void givenNullTransferId_whenCreatingRequest_thenThrows() {
+ assertThatThrownBy(() -> new ChainSelectionEngine.ChainSelectionRequest(null, USDC, AMOUNT, null))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("transferId");
+ }
+
+ `@Test`
+ void givenNullStablecoin_whenCreatingRequest_thenThrows() {
+ assertThatThrownBy(() -> new ChainSelectionEngine.ChainSelectionRequest(TRANSFER_ID, null, AMOUNT, null))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("stablecoin");
+ }
+
+ `@Test`
+ void givenNonPositiveAmount_whenCreatingRequest_thenThrows() {
+ assertThatThrownBy(() -> new ChainSelectionEngine.ChainSelectionRequest(TRANSFER_ID, USDC, BigDecimal.ZERO, null))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("amount");
+ }🤖 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/domain/service/ChainSelectionEngineTest.java`
around lines 113 - 452, The test suite lacks negative-path tests for the public
ChainSelectionEngine.ChainSelectionRequest constructor; add new unit tests in
ChainSelectionEngineTest that exercise its validation branches by asserting that
constructing ChainSelectionRequest with an invalid transferId (null or empty), a
null/invalid stablecoin, and non-positive amounts (zero and negative BigDecimal)
each throw the expected exception (e.g., IllegalArgumentException or the
domain-specific validation exception); reference the nested type
ChainSelectionEngine.ChainSelectionRequest and ensure each test uses
assertThatThrownBy (or your project's preferred assertion) to verify the
message/exception type for transferId, stablecoin, and amount validation.
Comment on lines
+258
to
+274
| void selectChain_preferredChainUnhealthy_fallsBackToScoring() { | ||
| var weights = ChainSelectionWeights.defaults(); | ||
| var sut = engineWith(weights); | ||
|
|
||
| stubAllChainsWithSufficientBalance(); | ||
| // Base is unhealthy but is preferred | ||
| given(chainHealthProvider.getHealthScore(CHAIN_BASE)).willReturn(0.0); | ||
| given(chainHealthProvider.getHealthScore(CHAIN_ETHEREUM)).willReturn(1.0); | ||
| given(chainHealthProvider.getHealthScore(CHAIN_SOLANA)).willReturn(1.0); | ||
| given(chainFeeProvider.estimateFeeUsd(CHAIN_ETHEREUM, USDC)).willReturn(2.50); | ||
| given(chainFeeProvider.estimateFeeUsd(CHAIN_SOLANA, USDC)).willReturn(0.005); | ||
|
|
||
| var result = sut.selectChain(aSelectionRequestWithPreferredChain("base")); | ||
|
|
||
| // Base is unhealthy, so it falls back to scoring. Solana should win. | ||
| assertThat(result.selectedChain()).isNotEqualTo(CHAIN_BASE); | ||
| } |
There was a problem hiding this comment.
Assert the actual fallback winner.
This setup is deterministic. isNotEqualTo(CHAIN_BASE) still passes if fallback scoring regresses to CHAIN_ETHEREUM, so it does not really protect the scoring path.
Suggested fix
- assertThat(result.selectedChain()).isNotEqualTo(CHAIN_BASE);
+ assertThat(result.selectedChain()).isEqualTo(CHAIN_SOLANA);🤖 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/domain/service/ChainSelectionEngineTest.java`
around lines 258 - 274, The test
selectChain_preferredChainUnhealthy_fallsBackToScoring currently only asserts
the result is not CHAIN_BASE, which allows an incorrect fallback to
CHAIN_ETHEREUM; update the assertion after calling sut.selectChain(...) to
assert the deterministic winner (CHAIN_SOLANA) instead of a negative check.
Locate the test method and replace the final
assertThat(result.selectedChain()).isNotEqualTo(CHAIN_BASE) with an assertion
that the selectedChain equals CHAIN_SOLANA, keeping the existing stubs of
chainHealthProvider and chainFeeProvider that make Solana the expected scorer.
This was referenced Mar 8, 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
ChainSelectionEnginedomain service — weighted scoring model:score = costWeight*(1/fee) + speedWeight*(1/finality) + reliabilityWeight*healthScoreChainSelectionWeightsimmutable record with configurable weights via@ConfigurationProperties(prefix = "app.custody.chain-selection")hasSufficientBalance), chain health (ChainHealthProvider), stablecoin availabilityChainHealthProvider,ChainFeeProvider,ChainSelectionLogRepositoryChainSelectionLogPersistenceAdapter— JdbcTemplate insert with Jackson 3 JSONB serializationFallbackAdaptersConfig(all healthy, static fees)Test plan
Closes STA-136
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests