feat(s4): business tests — 10 E2E multi-chain transfer flows (STA-145)#151
Merged
Conversation
|
Caution Review failedPull request was closed or merged during review WalkthroughAdds a business/E2E test base class and a comprehensive TransferLifecycleTest that exercises multi-chain transfer submission, controllable adapters (custody, chain RPC/health/fee), monitor-driven confirmations, DB/outbox interactions, idempotency, resubmission and failure scenarios. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@blockchain-custody/blockchain-custody/src/business-test/java/com/stablecoin/payments/custody/TransferLifecycleTest.java`:
- Around line 345-349: In TransferLifecycleTest, replace the fragile COUNT(*)
assertion that uses jdbcTemplate and paymentId with a query that selects the
custody_outbox_record rows for record_key = paymentId.toString() (fetching
event_type/status and payload, ordered by created_at), then assert the actual
sequence and contents of emitted events (e.g., "submitted" then "confirmed") and
verify payload fields rather than just row count; apply the same change pattern
to the other occurrences mentioned (around the blocks at lines ~398-401,
~440-443, ~536-539) so each test validates event types and payload values using
jdbcTemplate.queryForList and explicit assertions instead of COUNT checks.
- Around line 59-74: The static mutable maps and counter (RECEIPT_MAP,
BLOCK_NUMBER_MAP, HEALTH_SCORE_MAP, FEE_MAP, TOKEN_BALANCE_MAP, TX_COUNTER)
create shared global state across tests and make the suite flaky under parallel
JUnit execution; change them to per-test state by removing static and
initializing them in resetControllableState() or a `@BeforeEach` method (or use
ThreadLocal instances) so each test gets its own ConcurrentHashMap/AtomicLong
instances, and ensure any code referencing these symbols (e.g., in
resetControllableState()) uses the instance fields rather than class-level
static fields.
- Around line 618-639: The test currently only verifies attempt_count > 1 but
doesn't assert that a resubmission occurred on-chain; capture the original
txHash after createForwardTransfer (e.g., call getTxHash(transferId) before
making it "stuck"), then after calling
transferMonitorCommandHandler.monitorPendingTransfers() twice capture the new
txHash and assert it differs from the original; additionally verify the custody
invocation was called a second time (mock/spy on the custody client used by the
resubmission flow) with the new txHash or verify the system generated a new
on-chain submission for transferId to prove an actual resubmit rather than just
incrementing attempt_count.
- Around line 227-266: The JSON fixtures always use the same placeholder
recipient, so update forwardTransferJson and returnTransferJson to produce
chain-specific toWalletAddress values: in forwardTransferJson (and mirror in
returnTransferJson) branch on preferredChain (e.g., "SOLANA" => use a
Solana-shaped base58 address, otherwise EVM-style => use a valid 0x + 40-hex
character address) and when preferredChain is null omit the preferredChain field
and default to an EVM-style hex address; also make returnTransferJson handle a
null preferredChain the same way (only include "preferredChain" in the JSON when
not null) so the tests exercise Solana vs EVM address serialization/validation
paths.
- Around line 99-128: The controllable adapters currently ignore chain context
and silently fall back to defaults; update controllableChainRpcProvider's
getTransactionReceipt and getTokenBalance to include chainId in the lookup
(e.g., use a composite key or nested map using chainId.value() plus
txHash/address) and fail fast (throw or return null/raise assertion) when the
exact chain+key is not seeded instead of returning a default; likewise make
controllableChainHealthProvider and controllableChainFeeProvider stop returning
happy-path defaults from HEALTH_SCORE_MAP and FEE_MAP and instead require an
explicit entry for chainId.value() (or fail fast) so tests will surface
wrong-chain or unseeded lookups (references: controllableChainRpcProvider,
getTransactionReceipt, getTokenBalance, BLOCK_NUMBER_MAP, RECEIPT_MAP,
TOKEN_BALANCE_MAP, controllableChainHealthProvider, HEALTH_SCORE_MAP,
controllableChainFeeProvider, FEE_MAP).
- Around line 293-301: The confirmation tests only exercise monotonic block
advancement; add a reorg scenario in TransferLifecycleTest that simulates a
previously seen receipt disappearing or changing height and the chain head
moving backwards so the monitor's reorg handling is exercised. Modify or add a
test that uses configureConfirmedReceipt(txHash, blockNumber) to create an
initial confirmed receipt, then use BLOCK_NUMBER_MAP and setLatestBlock(chainId,
newLowerBlock) together with mutating RECEIPT_MAP (remove the txHash entry or
replace it with a receipt at a different lower block) to simulate the reorg,
then assert the monitor/reactor code observes the loss/height change (e.g.,
transitions the transfer back to unconfirmed or triggers the reorg handler).
Locate and update tests referencing configureConfirmedReceipt, setLatestBlock,
RECEIPT_MAP and BLOCK_NUMBER_MAP so the reorg branch is covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 793741d3-73bc-47fa-9daf-88017d86ab4f
📒 Files selected for processing (2)
blockchain-custody/blockchain-custody/src/business-test/java/com/stablecoin/payments/custody/AbstractBusinessTest.javablockchain-custody/blockchain-custody/src/business-test/java/com/stablecoin/payments/custody/TransferLifecycleTest.java
Comment on lines
+99
to
+128
| public ChainRpcProvider controllableChainRpcProvider() { | ||
| return new ChainRpcProvider() { | ||
| @Override | ||
| public TransactionReceipt getTransactionReceipt(ChainId chainId, String txHash) { | ||
| return RECEIPT_MAP.get(txHash); | ||
| } | ||
|
|
||
| @Override | ||
| public long getLatestBlockNumber(ChainId chainId) { | ||
| var counter = BLOCK_NUMBER_MAP.get(chainId.value()); | ||
| return counter != null ? counter.get() : 1000L; | ||
| } | ||
|
|
||
| @Override | ||
| public BigDecimal getTokenBalance(ChainId chainId, String address, String tokenContract) { | ||
| return TOKEN_BALANCE_MAP.getOrDefault(address, new BigDecimal("500000")); | ||
| } | ||
| }; | ||
| } | ||
|
|
||
| @Bean | ||
| @Primary | ||
| public ChainHealthProvider controllableChainHealthProvider() { | ||
| return chainId -> HEALTH_SCORE_MAP.getOrDefault(chainId.value(), 1.0); | ||
| } | ||
|
|
||
| @Bean | ||
| @Primary | ||
| public ChainFeeProvider controllableChainFeeProvider() { | ||
| return (chainId, stablecoin) -> FEE_MAP.getOrDefault(chainId.value(), defaultFee(chainId)); |
There was a problem hiding this comment.
Tighten the controllable adapters; they currently hide wrong-chain calls.
getTransactionReceipt() and getTokenBalance() ignore part of the lookup key, and the health/fee/balance providers fall back to happy-path defaults. A regression that queries the wrong chain or forgets to seed a lookup will still pass these “multi-chain” tests.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@blockchain-custody/blockchain-custody/src/business-test/java/com/stablecoin/payments/custody/TransferLifecycleTest.java`
around lines 99 - 128, The controllable adapters currently ignore chain context
and silently fall back to defaults; update controllableChainRpcProvider's
getTransactionReceipt and getTokenBalance to include chainId in the lookup
(e.g., use a composite key or nested map using chainId.value() plus
txHash/address) and fail fast (throw or return null/raise assertion) when the
exact chain+key is not seeded instead of returning a default; likewise make
controllableChainHealthProvider and controllableChainFeeProvider stop returning
happy-path defaults from HEALTH_SCORE_MAP and FEE_MAP and instead require an
explicit entry for chainId.value() (or fail fast) so tests will surface
wrong-chain or unseeded lookups (references: controllableChainRpcProvider,
getTransactionReceipt, getTokenBalance, BLOCK_NUMBER_MAP, RECEIPT_MAP,
TOKEN_BALANCE_MAP, controllableChainHealthProvider, HEALTH_SCORE_MAP,
controllableChainFeeProvider, FEE_MAP).
Comment on lines
+227
to
+266
| private String forwardTransferJson(UUID paymentId, UUID correlationId, String preferredChain) { | ||
| if (preferredChain != null) { | ||
| return """ | ||
| { | ||
| "paymentId": "%s", | ||
| "correlationId": "%s", | ||
| "transferType": "FORWARD", | ||
| "stablecoin": "USDC", | ||
| "amount": "1000.00", | ||
| "toWalletAddress": "0xRecipientAddress1234567890abcdef", | ||
| "preferredChain": "%s" | ||
| } | ||
| """.formatted(paymentId, correlationId, preferredChain); | ||
| } | ||
| return """ | ||
| { | ||
| "paymentId": "%s", | ||
| "correlationId": "%s", | ||
| "transferType": "FORWARD", | ||
| "stablecoin": "USDC", | ||
| "amount": "1000.00", | ||
| "toWalletAddress": "0xRecipientAddress1234567890abcdef" | ||
| } | ||
| """.formatted(paymentId, correlationId); | ||
| } | ||
|
|
||
| private String returnTransferJson(UUID paymentId, UUID correlationId, | ||
| UUID parentTransferId, String preferredChain) { | ||
| return """ | ||
| { | ||
| "paymentId": "%s", | ||
| "correlationId": "%s", | ||
| "transferType": "RETURN", | ||
| "parentTransferId": "%s", | ||
| "stablecoin": "USDC", | ||
| "amount": "1000.00", | ||
| "toWalletAddress": "0xRecipientAddress1234567890abcdef", | ||
| "preferredChain": "%s" | ||
| } | ||
| """.formatted(paymentId, correlationId, parentTransferId, preferredChain); |
There was a problem hiding this comment.
These fixtures don’t exercise chain-specific address handling.
All requests use the same placeholder recipient, so the Solana flow never submits a Solana-shaped address and the EVM flows are not using valid hex addresses either. That creates false positives around address validation and serialization in exactly the path this suite is meant to protect.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@blockchain-custody/blockchain-custody/src/business-test/java/com/stablecoin/payments/custody/TransferLifecycleTest.java`
around lines 227 - 266, The JSON fixtures always use the same placeholder
recipient, so update forwardTransferJson and returnTransferJson to produce
chain-specific toWalletAddress values: in forwardTransferJson (and mirror in
returnTransferJson) branch on preferredChain (e.g., "SOLANA" => use a
Solana-shaped base58 address, otherwise EVM-style => use a valid 0x + 40-hex
character address) and when preferredChain is null omit the preferredChain field
and default to an EVM-style hex address; also make returnTransferJson handle a
null preferredChain the same way (only include "preferredChain" in the JSON when
not null) so the tests exercise Solana vs EVM address serialization/validation
paths.
Comment on lines
+293
to
+301
| private void configureConfirmedReceipt(String txHash, long blockNumber) { | ||
| RECEIPT_MAP.put(txHash, new TransactionReceipt( | ||
| txHash, blockNumber, true, | ||
| new BigDecimal("21000"), new BigDecimal("25"), 0)); | ||
| } | ||
|
|
||
| private void setLatestBlock(String chainId, long blockNumber) { | ||
| BLOCK_NUMBER_MAP.computeIfAbsent(chainId, k -> new AtomicLong(0)).set(blockNumber); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add a reorg scenario to the confirmation section.
Every confirmation path here is monotonic: once a receipt appears, the latest block only moves forward. The monitor still needs a business test where a previously seen receipt disappears or shifts block height, otherwise reorg handling remains unprotected. As per coding guidelines, "Block confirmation polling must handle reorgs".
Also applies to: 305-437
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@blockchain-custody/blockchain-custody/src/business-test/java/com/stablecoin/payments/custody/TransferLifecycleTest.java`
around lines 293 - 301, The confirmation tests only exercise monotonic block
advancement; add a reorg scenario in TransferLifecycleTest that simulates a
previously seen receipt disappearing or changing height and the chain head
moving backwards so the monitor's reorg handling is exercised. Modify or add a
test that uses configureConfirmedReceipt(txHash, blockNumber) to create an
initial confirmed receipt, then use BLOCK_NUMBER_MAP and setLatestBlock(chainId,
newLowerBlock) together with mutating RECEIPT_MAP (remove the txHash entry or
replace it with a receipt at a different lower block) to simulate the reorg,
then assert the monitor/reactor code observes the loss/height change (e.g.,
transitions the transfer back to unconfirmed or triggers the reorg handler).
Locate and update tests referencing configureConfirmedReceipt, setLatestBlock,
RECEIPT_MAP and BLOCK_NUMBER_MAP so the reorg branch is covered.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
blockchain-custody/blockchain-custody/src/business-test/java/com/stablecoin/payments/custody/TransferLifecycleTest.java (1)
106-110: 🧹 Nitpick | 🔵 TrivialSilent fallback in
getLatestBlockNumbermay mask test setup errors.Returning
1000Lwhen a chain isn't explicitly configured allows tests to pass even ifBLOCK_NUMBER_MAPwasn't properly seeded for that chain. Consider throwing or returning a sentinel that causes obvious test failure.🔧 Suggested improvement
`@Override` public long getLatestBlockNumber(ChainId chainId) { var counter = BLOCK_NUMBER_MAP.get(chainId.value()); - return counter != null ? counter.get() : 1000L; + if (counter == null) { + throw new IllegalStateException("Test setup error: BLOCK_NUMBER_MAP not seeded for chain " + chainId.value()); + } + return counter.get(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@blockchain-custody/blockchain-custody/src/business-test/java/com/stablecoin/payments/custody/TransferLifecycleTest.java` around lines 106 - 110, The getLatestBlockNumber implementation silently falls back to 1000L when BLOCK_NUMBER_MAP lacks an entry, hiding misconfigured tests; update getLatestBlockNumber (referencing BLOCK_NUMBER_MAP and ChainId) to fail fast by throwing an informative RuntimeException (or returning a clearly invalid sentinel such as Long.MIN_VALUE) when counter is null so missing test setup is obvious and tests fail loudly.
🤖 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/business-test/java/com/stablecoin/payments/custody/TransferLifecycleTest.java`:
- Around line 546-569: The two nested test scenarios InsufficientBalance and
AllChainsDrained (tests like shouldRejectTransferWhenBalanceInsufficient and the
test at 720-743) cover overlapping behavior (all chains drained -> 503);
consolidate them by turning them into a single parameterized test or make them
assert distinct behavior: keep one as the "all chains drained" case that sets
available_balance and blockchain_balance to near-zero for every chain and
expects BC-1002, and change the other to exercise preferred-chain fallback by
setting only the preferred chain (e.g., "base") drained while others have
sufficient balance and then assert success/fallback behavior; update the test
names and JUnit annotations (e.g., `@ParameterizedTest` or `@DisplayName`)
accordingly and ensure the setup code that updates wallet_balances and the
request payloads (forwardTransferJson usage) reflect the chosen inputs for each
case.
- Around line 196-203: The seeded Ethereum wallet address used when inserting
the ON_RAMP wallet (variable ethOnRampId in TransferLifecycleTest and the INSERT
VALUES block) contains non-hex characters ("eth"); replace that value with a
valid 0x-prefixed 40-hex-character address (e.g., only 0-9 and a-f) so that
insertBalanceAndNonce(ethOnRampId, "ethereum") and any EVM address validation in
production will be tested correctly.
- Around line 270-275: The extractField helper currently parses JSON by string
search and will throw StringIndexOutOfBoundsException if the field is missing;
replace its implementation to parse jsonResponse using Jackson's ObjectMapper
(e.g., readTree) and safely retrieve the field via JsonNode#path or `#get`,
returning null or throwing a clear exception when absent. Update the
extractField(String jsonResponse, String fieldName) function to create an
ObjectMapper, parse the JSON into a JsonNode, check node.has(fieldName) or
node.path(fieldName).isMissingNode(), and then return
node.get(fieldName).asText() (or null) to avoid index errors and provide robust
parsing.
- Around line 304-312: The current assertOutboxContainsEvents method is
order-insensitive and can falsely pass when duplicates exist because it uses
anyMatch for each expected type; change the assertion to compare the actual
outboxTypes collection against the expectedEventTypes using a strict unordered
equality assertion (e.g.,
assertThat(outboxTypes).containsExactlyInAnyOrder(expectedEventTypes)) and, if
record_type contains fully-qualified names, map/transform outboxTypes to their
simple names before asserting (all within assertOutboxContainsEvents).
---
Duplicate comments:
In
`@blockchain-custody/blockchain-custody/src/business-test/java/com/stablecoin/payments/custody/TransferLifecycleTest.java`:
- Around line 106-110: The getLatestBlockNumber implementation silently falls
back to 1000L when BLOCK_NUMBER_MAP lacks an entry, hiding misconfigured tests;
update getLatestBlockNumber (referencing BLOCK_NUMBER_MAP and ChainId) to fail
fast by throwing an informative RuntimeException (or returning a clearly invalid
sentinel such as Long.MIN_VALUE) when counter is null so missing test setup is
obvious and tests fail loudly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ac941ac8-6687-452e-b041-a2c56e1d5c48
📒 Files selected for processing (1)
blockchain-custody/blockchain-custody/src/business-test/java/com/stablecoin/payments/custody/TransferLifecycleTest.java
Comment on lines
+196
to
+203
| // Ethereum ON_RAMP wallet | ||
| var ethOnRampId = UUID.randomUUID(); | ||
| jdbcTemplate.update( | ||
| """ | ||
| INSERT INTO wallets (wallet_id, chain_id, address, address_checksum, tier, purpose, custodian, vault_account_id, stablecoin, is_active) | ||
| VALUES (?, 'ethereum', '0xeth1111222233334444555566667777888899990000', '0xeth1111222233334444555566667777888899990000', 'HOT', 'ON_RAMP', 'dev', 'dev-vault-eth-onramp', 'USDC', true) | ||
| """, ethOnRampId); | ||
| insertBalanceAndNonce(ethOnRampId, "ethereum"); |
There was a problem hiding this comment.
Seeded Ethereum address contains invalid hex characters.
The address 0xeth1111222233334444555566667777888899990000 includes non-hex characters (eth). If the production code validates EVM addresses, this test data could bypass validation or mask bugs. Use a valid 40-character hex address.
🔧 Suggested fix
- VALUES (?, 'ethereum', '0xeth1111222233334444555566667777888899990000', '0xeth1111222233334444555566667777888899990000', 'HOT', 'ON_RAMP', 'dev', 'dev-vault-eth-onramp', 'USDC', true)
+ VALUES (?, 'ethereum', '0xaaaa111122223333444455556666777788889999', '0xaaaa111122223333444455556666777788889999', 'HOT', 'ON_RAMP', 'dev', 'dev-vault-eth-onramp', 'USDC', true)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@blockchain-custody/blockchain-custody/src/business-test/java/com/stablecoin/payments/custody/TransferLifecycleTest.java`
around lines 196 - 203, The seeded Ethereum wallet address used when inserting
the ON_RAMP wallet (variable ethOnRampId in TransferLifecycleTest and the INSERT
VALUES block) contains non-hex characters ("eth"); replace that value with a
valid 0x-prefixed 40-hex-character address (e.g., only 0-9 and a-f) so that
insertBalanceAndNonce(ethOnRampId, "ethereum") and any EVM address validation in
production will be tested correctly.
Comment on lines
+270
to
+275
| private static String extractField(String jsonResponse, String fieldName) { | ||
| var pattern = "\"" + fieldName + "\":\""; | ||
| var start = jsonResponse.indexOf(pattern) + pattern.length(); | ||
| var end = jsonResponse.indexOf("\"", start); | ||
| return jsonResponse.substring(start, end); | ||
| } |
There was a problem hiding this comment.
extractField will throw StringIndexOutOfBoundsException on missing fields.
If the field isn't present in the response, indexOf returns -1, and the subsequent substring operations fail with an unhelpful stack trace. Use Jackson's ObjectMapper for robust JSON parsing.
🔧 Suggested fix using ObjectMapper
+ `@Autowired`
+ private com.fasterxml.jackson.databind.ObjectMapper objectMapper;
+
- private static String extractField(String jsonResponse, String fieldName) {
- var pattern = "\"" + fieldName + "\":\"";
- var start = jsonResponse.indexOf(pattern) + pattern.length();
- var end = jsonResponse.indexOf("\"", start);
- return jsonResponse.substring(start, end);
+ private String extractField(String jsonResponse, String fieldName) throws Exception {
+ var node = objectMapper.readTree(jsonResponse);
+ var value = node.get(fieldName);
+ if (value == null || value.isNull()) {
+ throw new AssertionError("Field '" + fieldName + "' not found in response: " + jsonResponse);
+ }
+ return value.asText();
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@blockchain-custody/blockchain-custody/src/business-test/java/com/stablecoin/payments/custody/TransferLifecycleTest.java`
around lines 270 - 275, The extractField helper currently parses JSON by string
search and will throw StringIndexOutOfBoundsException if the field is missing;
replace its implementation to parse jsonResponse using Jackson's ObjectMapper
(e.g., readTree) and safely retrieve the field via JsonNode#path or `#get`,
returning null or throwing a clear exception when absent. Update the
extractField(String jsonResponse, String fieldName) function to create an
ObjectMapper, parse the JSON into a JsonNode, check node.has(fieldName) or
node.path(fieldName).isMissingNode(), and then return
node.get(fieldName).asText() (or null) to avoid index errors and provide robust
parsing.
Comment on lines
+304
to
+312
| private void assertOutboxContainsEvents(UUID paymentId, String... expectedEventTypes) { | ||
| var outboxTypes = jdbcTemplate.queryForList( | ||
| "SELECT record_type FROM custody_outbox_record WHERE record_key = ? ORDER BY id", | ||
| String.class, paymentId.toString()); | ||
| assertThat(outboxTypes).hasSize(expectedEventTypes.length); | ||
| for (var expectedType : expectedEventTypes) { | ||
| assertThat(outboxTypes).anyMatch(type -> type.contains(expectedType)); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
assertOutboxContainsEvents assertion logic is order-insensitive and allows duplicates to satisfy multiple expectations.
The anyMatch loop can pass with ["A", "A"] when expecting ["A", "B"] since it only checks existence. Use containsExactlyInAnyOrder for stricter validation.
🔧 Suggested fix
private void assertOutboxContainsEvents(UUID paymentId, String... expectedEventTypes) {
var outboxTypes = jdbcTemplate.queryForList(
"SELECT record_type FROM custody_outbox_record WHERE record_key = ? ORDER BY id",
String.class, paymentId.toString());
- assertThat(outboxTypes).hasSize(expectedEventTypes.length);
- for (var expectedType : expectedEventTypes) {
- assertThat(outboxTypes).anyMatch(type -> type.contains(expectedType));
- }
+ assertThat(outboxTypes)
+ .hasSize(expectedEventTypes.length)
+ .containsExactlyInAnyOrder(expectedEventTypes);
}If record_type values don't exactly match the expected strings (e.g., they're fully-qualified class names), adjust to extract the simple name or use containsSubsequence with proper ordering.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@blockchain-custody/blockchain-custody/src/business-test/java/com/stablecoin/payments/custody/TransferLifecycleTest.java`
around lines 304 - 312, The current assertOutboxContainsEvents method is
order-insensitive and can falsely pass when duplicates exist because it uses
anyMatch for each expected type; change the assertion to compare the actual
outboxTypes collection against the expectedEventTypes using a strict unordered
equality assertion (e.g.,
assertThat(outboxTypes).containsExactlyInAnyOrder(expectedEventTypes)) and, if
record_type contains fully-qualified names, map/transform outboxTypes to their
simple names before asserting (all within assertOutboxContainsEvents).
Comment on lines
+546
to
+569
| // ── Scenario 6: Insufficient Balance ──────────────────────────────── | ||
|
|
||
| @Nested | ||
| @DisplayName("6. Insufficient Balance — reject with 503") | ||
| class InsufficientBalance { | ||
|
|
||
| @Test | ||
| @DisplayName("should reject transfer when all chains have insufficient balance") | ||
| void shouldRejectTransferWhenBalanceInsufficient() throws Exception { | ||
| var paymentId = UUID.randomUUID(); | ||
| var correlationId = UUID.randomUUID(); | ||
|
|
||
| // Drain all wallet balances | ||
| jdbcTemplate.update( | ||
| "UPDATE wallet_balances SET available_balance = 0.00000001, blockchain_balance = 0.00000001"); | ||
|
|
||
| // Submit -> 503 (no chain has sufficient balance, ChainUnavailableException) | ||
| mockMvc.perform(post("/v1/transfers") | ||
| .contentType(MediaType.APPLICATION_JSON) | ||
| .content(forwardTransferJson(paymentId, correlationId, null))) | ||
| .andExpect(status().isServiceUnavailable()) | ||
| .andExpect(jsonPath("$.code", is("BC-1002"))); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Scenarios 6 and 10 test overlapping conditions.
Both InsufficientBalance (null chain) and AllChainsDrained (preferred "base") verify that drained balances return 503. Consider consolidating into a single parameterized test or differentiating the scenarios more clearly (e.g., one tests fallback behavior when preferred chain is drained).
Also applies to: 720-743
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@blockchain-custody/blockchain-custody/src/business-test/java/com/stablecoin/payments/custody/TransferLifecycleTest.java`
around lines 546 - 569, The two nested test scenarios InsufficientBalance and
AllChainsDrained (tests like shouldRejectTransferWhenBalanceInsufficient and the
test at 720-743) cover overlapping behavior (all chains drained -> 503);
consolidate them by turning them into a single parameterized test or make them
assert distinct behavior: keep one as the "all chains drained" case that sets
available_balance and blockchain_balance to near-zero for every chain and
expects BC-1002, and change the other to exercise preferred-chain fallback by
setting only the preferred chain (e.g., "base") drained while others have
sufficient balance and then assert success/fallback behavior; update the test
names and JUnit annotations (e.g., `@ParameterizedTest` or `@DisplayName`)
accordingly and ensure the setup code that updates wallet_balances and the
request payloads (forwardTransferJson usage) reflect the chosen inputs for each
case.
Add business tests for the S4 Blockchain & Custody service covering all 10 acceptance criteria: Base/Ethereum/Solana happy paths with chain-specific confirmations, chain selection fallback on unhealthy Base, RETURN compensation flow, insufficient balance rejection, idempotency, stuck resubmission recovery, and balance sync after on-chain confirmation. Tests use controllable @TestConfiguration adapters for CustodyEngine, ChainRpcProvider, ChainHealthProvider, and ChainFeeProvider, with TestContainers PostgreSQL + Kafka and real Spring context. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Replace weak outbox COUNT(*) >= 2 assertions with record_type verification - Add txHash comparison in stuck resubmission test to verify new hash differs - Use chain-aware composite keys in controllable RPC adapter (chainId:txHash) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Puneethkumarck
force-pushed
the
feature/STA-145-s4-business-tests
branch
from
43cc6f9 to
9ee748c
Compare
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
@TestConfigurationwith@Primarybeans for CustodyEngine, ChainRpcProvider, ChainHealthProvider, ChainFeeProviderTest Scenarios
Test plan
Closes STA-145
🤖 Generated with Claude Code
Summary by CodeRabbit