feat(s4): transfer monitor — block indexer + confirmation polling (STA-142)#145
Merged
Merged
Conversation
Puneethkumarck
added
phase-3
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds per-chain chain/tokens config and resolution, a system UTC Clock bean, transfer/ balance monitor properties and scheduled jobs, new domain ports and services for balance sync and transfer monitoring (including resubmission/confirmation flows), persistence bulk retrieval, state-machine resubmission APIs, fixtures, and extensive unit tests. Changes
Sequence DiagramssequenceDiagram
participant Job as BalanceSyncJob
participant Handler as BalanceSyncCommandHandler
participant BalanceRepo as WalletBalanceRepository
participant WalletRepo as WalletRepository
participant RPC as ChainRpcProvider
participant DB as Persistence
rect rgba(200,200,255,0.5)
Job->>Handler: syncAllBalances()
end
Handler->>BalanceRepo: findAll()
BalanceRepo-->>Handler: List<WalletBalance>
loop per balance
Handler->>WalletRepo: findById(walletId)
alt wallet found
WalletRepo-->>Handler: Wallet
Handler->>RPC: getLatestBlockNumber(chain)
RPC-->>Handler: latestBlock
alt new blocks exist
Handler->>RPC: getTokenBalance(chain,address,tokenContract)
RPC-->>Handler: onChainBalance
Handler->>BalanceRepo: save(updated)
BalanceRepo->>DB: persist
else no new blocks
Handler->>Handler: skip
end
else wallet missing
Handler->>Handler: log & skip
end
end
Handler->>Handler: log summary
sequenceDiagram
participant Job as TransferMonitorJob
participant Handler as TransferMonitorCommandHandler
participant TransferRepo as ChainTransferRepository
participant RPC as ChainRpcProvider
participant Clock as Clock
participant Custody as CustodyEngine
participant BalanceRepo as WalletBalanceRepository
participant Publisher as TransferEventPublisher
participant EventsDB as TransferLifecycleEventRepository
rect rgba(200,255,200,0.5)
Job->>Handler: monitorPendingTransfers()
end
loop status in SUBMITTED,CONFIRMING,RESUBMITTING
Handler->>TransferRepo: findByStatus(status)
TransferRepo-->>Handler: transfers
loop per transfer
alt SUBMITTED
Handler->>RPC: getTransactionReceipt(txHash)
alt receipt found
Handler->>Handler: processSubmitted(receipt)
alt success
Handler->>TransferRepo: save(CONFIRMING)
Handler->>Handler: checkConfirmations(...)
else reverted
Handler->>TransferRepo: save(FAILED)
Handler->>Publisher: publishFailedEvent()
Handler->>EventsDB: save(failure)
end
else no receipt
Handler->>Clock: now()
Clock-->>Handler: ts
alt timeout exceeded
Handler->>TransferRepo: save(RESUBMITTING)
else continue
end
end
else CONFIRMING
Handler->>RPC: getTransactionReceipt(txHash)
alt sufficient confirmations
Handler->>TransferRepo: save(CONFIRMED)
Handler->>BalanceRepo: debit wallet balance
Handler->>Publisher: publishConfirmedEvent()
Handler->>EventsDB: save(confirmation)
else log progress
end
else RESUBMITTING
alt attempts < max
Handler->>Custody: claimResubmission & sign
Custody-->>Handler: SignResult
Handler->>Custody: submitSignedTx
Custody-->>Handler: txHash
Handler->>TransferRepo: save(updated)
else attempts >= max
Handler->>TransferRepo: save(FAILED)
Handler->>Publisher: publishFailedEvent()
Handler->>EventsDB: save(failure)
end
end
end
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
🚥 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 |
…A-142) Add transfer monitoring system with two scheduled jobs and domain services: - TransferMonitorCommandHandler: monitors SUBMITTED/CONFIRMING/RESUBMITTING transfers, handles confirmation counting, stuck detection (>120s), resubmission via CustodyEngine, and max-attempt failure (3 attempts) - BalanceSyncCommandHandler: syncs all wallet balances from on-chain RPC data - TransferMonitorJob: @scheduled thin delegator polling every 15s - BalanceSyncJob: @scheduled thin delegator syncing every 30s - Domain ports: TransferMonitorProperties, ChainConfirmationProperties - Config: ChainMonitorConfig, TransferMonitorConfig, ClockConfig - 14 unit tests covering confirm, resubmit, max attempts, stuck detection, failed receipt, balance sync, and error resilience Closes STA-142 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Puneethkumarck
force-pushed
the
feature/STA-142-s4-transfer-monitor-v2
branch
from
9324d25 to
ea55bff
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/application/config/ChainMonitorConfig.java`:
- Around line 23-26: The getMinConfirmations method in ChainMonitorConfig
currently defaults missing or invalid entries to 1, which hides config errors;
change it to fail-fast by validating the chains map lookup and the retrieved
props.minConfirmations() (reject null / missing chainId and non-positive values)
and throw a clear configuration exception (e.g., IllegalArgumentException or a
custom ConfigException) instead of returning 1; update all similar normalization
sites (the other branch around lines 38-40) so
TransferMonitorCommandHandler.checkConfirmations() receives a legitimate
positive confirmation count and mis-typed or missing chain IDs raise an explicit
error during startup or config load.
In
`@blockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/application/scheduler/BalanceSyncJob.java`:
- Around line 16-24: BalanceSyncJob currently calls
balanceSyncCommandHandler.syncAllBalances() from syncBalances() on every
instance; add cluster coordination so only one replica runs the job (e.g., use a
distributed lock or leader election). Modify syncBalances() to acquire a
cluster-safe lock or verify leadership before invoking
balanceSyncCommandHandler.syncAllBalances() (for example integrate a library
such as ShedLock or Spring Cloud leader election and use a lock/leadership check
in syncBalances()), ensure the lock is time-bounded and released/failsafe, and
add logging around lock acquisition/failure so non-leader instances skip running
syncAllBalances().
In
`@blockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/config/FallbackAdaptersConfig.java`:
- Around line 140-145: The fallback TransferEventPublisher bean
(fallbackTransferEventPublisher) must not be active in production; update the
bean so it's only registered for non-production/dev/test environments or behind
an explicit opt-in property. Add a guard such as `@Profile`("!prod") or
`@ConditionalOnProperty`(name="app.enable-fallback-transfer-event-publisher",
havingValue="true", matchIfMissing=false") to the fallbackTransferEventPublisher
method, or instead remove `@ConditionalOnMissingBean` and throw an
IllegalStateException during startup when TransferEventPublisher is absent so
TransferMonitorCommandHandler cannot silently log away terminal transfer events
in production.
In
`@blockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/domain/port/ChainConfirmationProperties.java`:
- Around line 8-14: The current
ChainConfirmationProperties.getMinConfirmations(String chainId) must not
silently default unknown chains to 1; change the contract and implementations so
the method fails fast for unknown/mismatched chain IDs (e.g., throw a
checked/unchecked exception or return an Optional/OptionalInt) instead of
returning 1, and update callers—particularly
TransferMonitorCommandHandler.checkConfirmations—to handle the failure path
explicitly (reject/abort the transfer or surface an error) rather than treating
a missing entry as a 1-confirmation success.
In
`@blockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/domain/service/BalanceSyncCommandHandler.java`:
- Around line 17-20: The class-level `@Transactional` on BalanceSyncCommandHandler
causes a single DB transaction to span the entire polling loop and RPC calls;
remove or stop using the class-level transaction and instead start a new short
transaction only when persisting each balance result. Concretely, remove the
`@Transactional` annotation from the handler (or the long-running handler method),
and wrap the per-balance persistence step (e.g., the method that updates/saves
balance rows) in its own `@Transactional` method or execute it via
TransactionTemplate/PlatformTransactionManager so RPC calls happen outside any
transaction and each balance save uses its own short transaction.
- Around line 66-70: The on-chain lookup in BalanceSyncCommandHandler uses
chainRpcProvider.getTokenBalance(chainId, address, null) which causes all
WalletBalance records to read the same default asset; instead resolve the token
contract identifier from the WalletBalance/stablecoin via balance.stablecoin()
(or a resolver helper) and pass that contract/address into getTokenBalance so
each walletId+stablecoin pair queries its specific token contract rather than
null.
- Around line 35-59: The current BalanceSyncCommandHandler uses
walletBalanceRepository.findAll() and then walletRepository.findById(...) per
balance and calls chainRpcProvider.getLatestBlockNumber(balance.chainId()) per
row causing a full-table scan and N+1 lookups; change this to page through
balances (use a pageable/offset loop) and fetch balances already joined with
wallet data (include wallet address in the balances query) so you don't call
walletRepository.findById(balance.walletId()) for each item, and group or cache
chain IDs so you call chainRpcProvider.getLatestBlockNumber(chainId) once per
chain (or once per loop) instead of per balance. Ensure loops use a fixed page
size, use a repository method that returns balances with wallet info (or a DTO),
and replace per-balance walletRepository.findById and per-balance latestBlock
calls accordingly in BalanceSyncCommandHandler.
In
`@blockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/domain/service/TransferMonitorCommandHandler.java`:
- Around line 181-196: The current flow calls custodyEngine.signAndSubmit(...)
before persisting the resubmission, which risks double-submission if the
subsequent chainTransferRepository.save(...) or
lifecycleEventRepository.save(...) fails; change the sequence to persist an
idempotency/resubmission marker first (e.g., call transfer.resubmit(...) to
produce a resubmission state or token and immediately persist that via
chainTransferRepository.save(resubmitted) and optionally
lifecycleEventRepository.save(TransferLifecycleEvent.record(...,"RESUBMISSION_CLAIM")))
before invoking custodyEngine.signAndSubmit(signRequest), then update the
persisted record with the returned txHash and final "SUBMITTED" lifecycle event;
alternatively implement idempotency in CustodyEngine.signAndSubmit keyed by
transfer.transferId() so repeated calls are safe.
- Around line 132-139: processConfirming currently only logs and returns when
chainRpcProvider.getTransactionReceipt(transfer.chainId(), transfer.txHash()) is
null, which lets CONFIRMING transfers stall forever; update processConfirming
(for ChainTransfer) to, after a short grace window, treat a missing receipt as a
candidate for resubmission by checking how long the transfer has been in
CONFIRMING (e.g., an existing timestamp on transfer or transfer.updatedAt) and
then re-entering the existing resubmission/max-attempt path (increment attempt
count or call the existing resubmit/enqueue logic) instead of silently
returning; ensure the grace-window value and attempt increment follow the same
policy used by your other resubmission logic so transfers eventually move out of
CONFIRMING.
- Around line 41-42: The class-level transaction on
TransferMonitorCommandHandler causes the entire monitorPendingTransfers() loop
to commit partially when exceptions are swallowed; change isolation so each
transfer is handled in its own transaction by annotating the per-transfer
handlers processSubmitted, processConfirming, and processResubmitting with
`@Transactional`(propagation = Propagation.REQUIRES_NEW) (or refactor to use an
outbox + after-commit hook) so failures trigger rollback for that transfer and
do not affect others; also ensure operations that must persist before external
side-effects are reordered in processResubmitting so you persist the transfer
state (and/or write an outbox event) before calling
custodyEngine.signAndSubmit(), or make signAndSubmit() idempotent to avoid lost
state when persistence fails, and ensure confirmDebitBalance() and
publishConfirmedEvent() are executed inside those per-transfer transactions so
their failures roll back the single transfer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 07ad42ce-a39c-45f7-aed0-cd25b5e848be
📒 Files selected for processing (15)
blockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/application/config/ChainMonitorConfig.javablockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/application/config/ClockConfig.javablockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/application/config/TransferMonitorConfig.javablockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/application/scheduler/BalanceSyncJob.javablockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/application/scheduler/TransferMonitorJob.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/port/ChainConfirmationProperties.javablockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/domain/port/TransferMonitorProperties.javablockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/domain/port/WalletBalanceRepository.javablockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/domain/service/BalanceSyncCommandHandler.javablockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/domain/service/TransferMonitorCommandHandler.javablockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/infrastructure/persistence/WalletBalancePersistenceAdapter.javablockchain-custody/blockchain-custody/src/test/java/com/stablecoin/payments/custody/domain/service/BalanceSyncCommandHandlerTest.javablockchain-custody/blockchain-custody/src/test/java/com/stablecoin/payments/custody/domain/service/TransferMonitorCommandHandlerTest.javablockchain-custody/blockchain-custody/src/testFixtures/java/com/stablecoin/payments/custody/fixtures/TransferMonitorFixtures.java
Comment on lines
+16
to
+24
| @Component | ||
| @RequiredArgsConstructor | ||
| @ConditionalOnProperty(name = "app.balance.sync.enabled", havingValue = "true", matchIfMissing = true) | ||
| public class BalanceSyncJob { | ||
|
|
||
| private final BalanceSyncCommandHandler balanceSyncCommandHandler; | ||
|
|
||
| @Scheduled(fixedDelayString = "${app.balance.sync.interval-ms:30000}") | ||
| public void syncBalances() { |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add cluster coordination if this service can run more than one replica.
Without leader election or a distributed scheduler lock, every instance will execute syncAllBalances() every 30 seconds. That multiplies RPC traffic and can create concurrent writes against the same balance rows. Use a cluster-safe scheduler mechanism if this job is intended to run once per environment.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@blockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/application/scheduler/BalanceSyncJob.java`
around lines 16 - 24, BalanceSyncJob currently calls
balanceSyncCommandHandler.syncAllBalances() from syncBalances() on every
instance; add cluster coordination so only one replica runs the job (e.g., use a
distributed lock or leader election). Modify syncBalances() to acquire a
cluster-safe lock or verify leadership before invoking
balanceSyncCommandHandler.syncAllBalances() (for example integrate a library
such as ShedLock or Spring Cloud leader election and use a lock/leadership check
in syncBalances()), ensure the lock is time-bounded and released/failsafe, and
add logging around lock acquisition/failure so non-leader instances skip running
syncAllBalances().
Comment on lines
+140
to
+145
| @Bean | ||
| @ConditionalOnMissingBean | ||
| public TransferEventPublisher fallbackTransferEventPublisher() { | ||
| log.info("Using fallback TransferEventPublisher (log only)"); | ||
| return event -> log.warn("[FALLBACK-EVENT] Published event: {}", event); | ||
| } |
There was a problem hiding this comment.
Gate the fallback event publisher to non-production only.
If the real TransferEventPublisher is missing, this bean turns terminal transfer events into WARN logs only. TransferMonitorCommandHandler publishes confirmed/failed events after state transitions, so a bad production wiring here will silently desync downstream payment state. Restrict this fallback behind a dev/test profile/property, or fail startup when the publisher is absent.
🤖 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 140 - 145, The fallback TransferEventPublisher bean
(fallbackTransferEventPublisher) must not be active in production; update the
bean so it's only registered for non-production/dev/test environments or behind
an explicit opt-in property. Add a guard such as `@Profile`("!prod") or
`@ConditionalOnProperty`(name="app.enable-fallback-transfer-event-publisher",
havingValue="true", matchIfMissing=false") to the fallbackTransferEventPublisher
method, or instead remove `@ConditionalOnMissingBean` and throw an
IllegalStateException during startup when TransferEventPublisher is absent so
TransferMonitorCommandHandler cannot silently log away terminal transfer events
in production.
Comment on lines
+8
to
+14
| /** | ||
| * Returns the minimum confirmations required for a given chain. | ||
| * | ||
| * @param chainId the chain identifier (e.g., "base", "ethereum", "solana") | ||
| * @return minimum confirmations, defaults to 1 if chain is unknown | ||
| */ | ||
| int getMinConfirmations(String chainId); |
There was a problem hiding this comment.
Do not default unknown chains to one confirmation.
This contract makes a missing/mismatched chain entry look valid with a threshold of 1. In TransferMonitorCommandHandler.checkConfirmations(), that would confirm the transfer and debit the wallet after a single block. Unknown chains should fail fast or be rejected explicitly.
🤖 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/ChainConfirmationProperties.java`
around lines 8 - 14, The current
ChainConfirmationProperties.getMinConfirmations(String chainId) must not
silently default unknown chains to 1; change the contract and implementations so
the method fails fast for unknown/mismatched chain IDs (e.g., throw a
checked/unchecked exception or return an Optional/OptionalInt) instead of
returning 1, and update callers—particularly
TransferMonitorCommandHandler.checkConfirmations—to handle the failure path
explicitly (reject/abort the transfer or surface an error) rather than treating
a missing entry as a 1-confirmation success.
Comment on lines
+35
to
+59
| var balances = walletBalanceRepository.findAll(); | ||
|
|
||
| if (balances.isEmpty()) { | ||
| log.debug("No wallet balances to sync"); | ||
| return; | ||
| } | ||
|
|
||
| log.info("Starting balance sync for {} wallet balances", balances.size()); | ||
|
|
||
| var successCount = 0; | ||
| var failureCount = 0; | ||
|
|
||
| for (var balance : balances) { | ||
| try { | ||
| var wallet = walletRepository.findById(balance.walletId()); | ||
|
|
||
| if (wallet.isEmpty()) { | ||
| log.warn("Wallet not found for balanceId={} walletId={} — skipping sync", | ||
| balance.balanceId(), balance.walletId()); | ||
| failureCount++; | ||
| continue; | ||
| } | ||
|
|
||
| var latestBlock = chainRpcProvider.getLatestBlockNumber(balance.chainId()); | ||
|
|
There was a problem hiding this comment.
This scheduler path is a full-table scan plus N+1 wallet lookups every 30s.
findAll() followed by walletRepository.findById(...) per balance will degrade quickly as the balance table grows, and you also re-fetch the latest block per balance instead of per chain. This needs paging/batching and a query shape that already includes the wallet address.
🤖 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/BalanceSyncCommandHandler.java`
around lines 35 - 59, The current BalanceSyncCommandHandler uses
walletBalanceRepository.findAll() and then walletRepository.findById(...) per
balance and calls chainRpcProvider.getLatestBlockNumber(balance.chainId()) per
row causing a full-table scan and N+1 lookups; change this to page through
balances (use a pageable/offset loop) and fetch balances already joined with
wallet data (include wallet address in the balances query) so you don't call
walletRepository.findById(balance.walletId()) for each item, and group or cache
chain IDs so you call chainRpcProvider.getLatestBlockNumber(chainId) once per
chain (or once per loop) instead of per balance. Ensure loops use a fixed page
size, use a repository method that returns balances with wallet info (or a DTO),
and replace per-balance walletRepository.findById and per-balance latestBlock
calls accordingly in BalanceSyncCommandHandler.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
blockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/domain/port/ChainConfirmationProperties.java (1)
8-14:⚠️ Potential issue | 🟠 MajorUnknown chains must not default to one confirmation.
Line 12 bakes in a fail-open contract. A missing or mismatched chain ID can be treated as a one-confirmation success, which is unsafe for transfer finality. Make unknown chains fail fast instead, and force callers to handle that error path explicitly.
🤖 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/ChainConfirmationProperties.java` around lines 8 - 14, The current getMinConfirmations(String chainId) returns a primitive int and treats unknown chains as 1; change the contract so callers must handle unknown chains explicitly by making getMinConfirmations return an OptionalInt (or Optional<Integer>) instead of int, update the ChainConfirmationProperties interface signature accordingly, and update all implementations and callers of ChainConfirmationProperties.getMinConfirmations to handle the empty OptionalInt (e.g., throw a domain-specific UnknownChainException or fail fast) rather than assuming a default of 1.blockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/config/FallbackAdaptersConfig.java (1)
140-145:⚠️ Potential issue | 🟠 MajorRestrict the fallback event publisher to non-production only.
@ConditionalOnMissingBeanalone still makes this log-only publisher eligible in production. If the real outbox/Kafka adapter is missing, confirmed/failed transfer events are silently dropped after state transitions. Gate this bean behind a non-prod profile/property, or fail startup whenTransferEventPublisheris absent.🤖 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 140 - 145, The fallback TransferEventPublisher bean fallbackTransferEventPublisher() in FallbackAdaptersConfig is currently annotated only with `@ConditionalOnMissingBean` and therefore can be used in production, silently dropping events; restrict it to non-production by gating it with a profile or explicit property: annotate fallbackTransferEventPublisher() with `@Profile`("!prod") (or add `@ConditionalOnProperty`(name="app.fallback-transfer-publisher.enabled", havingValue="true", matchIfMissing=false)) so the log-only publisher is only created in non-prod, or alternatively remove `@ConditionalOnMissingBean` and throw a BeanCreationException at startup when no TransferEventPublisher bean is present in production environments to fail fast.
🤖 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/application/config/TransferMonitorConfig.java`:
- Around line 16-23: The constructor TransferMonitorConfig should stop silently
coercing non-positive primitives; instead bind defaults only when values are
absent (null) and explicitly reject non-positive values by throwing an
IllegalArgumentException. Change handling in the TransferMonitorConfig
constructor so that if resubmitTimeoutS or maxAttempts is null you set them to
their defaults (120 and 3), but if they are non-null and <= 0 you throw an
exception with a clear message referencing the field (resubmitTimeoutS or
maxAttempts); update any parameter types or callers as needed to allow
nullability so absence is distinguishable from an explicit zero.
In
`@blockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/domain/port/WalletBalanceRepository.java`:
- Around line 19-20: The current unbounded findAll() on WalletBalanceRepository
must be replaced with a bounded, cursored/paginated fetch to avoid full-table
scans and RPC fan-out; add a method on WalletBalanceRepository such as
findBalancesDueForRefresh(int limit, `@Nullable` String cursor) or
findDueForRefresh(Pageable page) that returns a finite page (and next-cursor
token) of WalletBalance records requiring refresh, deprecate/remove findAll(),
update the caller(s) to loop using the limit/cursor until exhausted (or stop
after configured max per sync window), and ensure the repository implementation
only selects balances whose refresh timestamp or state indicates they are due
rather than returning the entire table.
In
`@blockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/domain/service/TransferMonitorCommandHandler.java`:
- Around line 144-165: In checkConfirmations, avoid casting the block difference
directly to int; compute the difference as a long (latestBlock -
receiptBlockNumber), clamp it to a non-negative value and to Integer.MAX_VALUE
before casting to int to prevent overflow or negative confirmations, then use
that safe int for currentConfirmations and the subsequent logic (references:
checkConfirmations, latestBlock, receiptBlockNumber, currentConfirmations).
- Around line 201-211: confirmDebitBalance silently no-ops when
walletBalanceRepository.findByWalletIdAndStablecoinForUpdate returns empty,
which can hide reconciliation gaps; change the method to handle the missing
WalletBalance explicitly by using ifPresentOrElse (or orElseThrow) on the
Optional returned from findByWalletIdAndStablecoinForUpdate: in the present
branch keep the existing logic (call balance.confirmDebit(transfer.amount()),
save via walletBalanceRepository.save(updated), and info-log the confirmation),
and in the empty branch either log a WARN with identifying transfer details
(transfer.fromWalletId(), transfer.stablecoin().ticker(), transfer.amount(),
transfer.id()) and throw a runtime/checked exception (or a domain-specific
exception) to fail the confirmation so callers cannot treat it as successful.
In
`@blockchain-custody/blockchain-custody/src/test/java/com/stablecoin/payments/custody/domain/service/BalanceSyncCommandHandlerTest.java`:
- Around line 104-106: Update the negative assertions in
BalanceSyncCommandHandlerTest: replace the specific save matcher assertions
(then(walletBalanceRepository).should(never()).save(eqIgnoringTimestamps(balance)))
with a blanket verification that no save occurred
(then(walletBalanceRepository).should(never()).save(any())) or use
then(walletBalanceRepository).shouldHaveNoMoreInteractions() to ensure nothing
was persisted; additionally, in the "no-new-blocks" test add an explicit
verification that chainRpcProvider.getTokenBalance(...) is never invoked
(then(chainRpcProvider).should(never()).getTokenBalance(...)). Apply the same
changes at the other occurrence around lines 134-135 so both skip-path tests
assert no saves and no token-balance RPC calls.
In
`@blockchain-custody/blockchain-custody/src/test/java/com/stablecoin/payments/custody/domain/service/TransferMonitorCommandHandlerTest.java`:
- Around line 269-317: Add a new unit test in the ConfirmingTransfers nested
class that simulates the receipt disappearing
(chainRpcProvider.getTransactionReceipt returns null) and the latest block
number advancing past the reorg timeout, then call
handler.monitorPendingTransfers() and assert the transfer transitions to
RESUBMITTING: set up
chainTransferRepository.findByStatus(TransferStatus.CONFIRMING) to return the
confirming transfer, stub chainRpcProvider.getTransactionReceipt to return null
and getLatestBlockNumber to a value > RECEIPT_BLOCK + reorgThreshold, invoke
handler.monitorPendingTransfers(), and verify chainTransferRepository.save(...)
was called with a transfer having status RESUBMITTING (and that
transferEventPublisher.publish(...) was invoked with the corresponding
TransferResubmittedEvent); reference the test helpers
aConfirmingTransferOnBase(), chainRpcProvider.getTransactionReceipt,
getLatestBlockNumber, handler.monitorPendingTransfers(), and
TransferResubmittedEvent to locate code.
---
Duplicate comments:
In
`@blockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/config/FallbackAdaptersConfig.java`:
- Around line 140-145: The fallback TransferEventPublisher bean
fallbackTransferEventPublisher() in FallbackAdaptersConfig is currently
annotated only with `@ConditionalOnMissingBean` and therefore can be used in
production, silently dropping events; restrict it to non-production by gating it
with a profile or explicit property: annotate fallbackTransferEventPublisher()
with `@Profile`("!prod") (or add
`@ConditionalOnProperty`(name="app.fallback-transfer-publisher.enabled",
havingValue="true", matchIfMissing=false)) so the log-only publisher is only
created in non-prod, or alternatively remove `@ConditionalOnMissingBean` and throw
a BeanCreationException at startup when no TransferEventPublisher bean is
present in production environments to fail fast.
In
`@blockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/domain/port/ChainConfirmationProperties.java`:
- Around line 8-14: The current getMinConfirmations(String chainId) returns a
primitive int and treats unknown chains as 1; change the contract so callers
must handle unknown chains explicitly by making getMinConfirmations return an
OptionalInt (or Optional<Integer>) instead of int, update the
ChainConfirmationProperties interface signature accordingly, and update all
implementations and callers of ChainConfirmationProperties.getMinConfirmations
to handle the empty OptionalInt (e.g., throw a domain-specific
UnknownChainException or fail fast) rather than assuming a default of 1.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e505e003-c388-42e9-8485-f41768c78e6e
📒 Files selected for processing (15)
blockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/application/config/ChainMonitorConfig.javablockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/application/config/ClockConfig.javablockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/application/config/TransferMonitorConfig.javablockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/application/scheduler/BalanceSyncJob.javablockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/application/scheduler/TransferMonitorJob.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/port/ChainConfirmationProperties.javablockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/domain/port/TransferMonitorProperties.javablockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/domain/port/WalletBalanceRepository.javablockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/domain/service/BalanceSyncCommandHandler.javablockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/domain/service/TransferMonitorCommandHandler.javablockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/infrastructure/persistence/WalletBalancePersistenceAdapter.javablockchain-custody/blockchain-custody/src/test/java/com/stablecoin/payments/custody/domain/service/BalanceSyncCommandHandlerTest.javablockchain-custody/blockchain-custody/src/test/java/com/stablecoin/payments/custody/domain/service/TransferMonitorCommandHandlerTest.javablockchain-custody/blockchain-custody/src/testFixtures/java/com/stablecoin/payments/custody/fixtures/TransferMonitorFixtures.java
Comment on lines
+16
to
+23
| public TransferMonitorConfig { | ||
| if (resubmitTimeoutS <= 0) { | ||
| resubmitTimeoutS = 120; | ||
| } | ||
| if (maxAttempts <= 0) { | ||
| maxAttempts = 3; | ||
| } | ||
| } |
There was a problem hiding this comment.
Do not silently coerce invalid monitor settings.
With primitive components, an explicit 0 or negative value is indistinguishable from “missing” and gets rewritten to 120 / 3. That hides operator mistakes in retry and timeout settings. Bind defaults only for absence, then reject non-positive values explicitly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@blockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/application/config/TransferMonitorConfig.java`
around lines 16 - 23, The constructor TransferMonitorConfig should stop silently
coercing non-positive primitives; instead bind defaults only when values are
absent (null) and explicitly reject non-positive values by throwing an
IllegalArgumentException. Change handling in the TransferMonitorConfig
constructor so that if resubmitTimeoutS or maxAttempts is null you set them to
their defaults (120 and 3), but if they are non-null and <= 0 you throw an
exception with a clear message referencing the field (resubmitTimeoutS or
maxAttempts); update any parameter types or callers as needed to allow
nullability so absence is distinguishable from an explicit zero.
Comment on lines
+19
to
+20
|
|
||
| List<WalletBalance> findAll(); |
There was a problem hiding this comment.
Replace findAll() with a bounded sync fetch contract.
This puts balance sync on a full-table scan plus full RPC fan-out every 30 seconds. As balance volume grows, this becomes an unbounded hot path that can overrun the scheduler window and hammer RPC providers. Prefer pagination/cursoring or a query that returns only balances due for refresh.
🤖 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/WalletBalanceRepository.java`
around lines 19 - 20, The current unbounded findAll() on WalletBalanceRepository
must be replaced with a bounded, cursored/paginated fetch to avoid full-table
scans and RPC fan-out; add a method on WalletBalanceRepository such as
findBalancesDueForRefresh(int limit, `@Nullable` String cursor) or
findDueForRefresh(Pageable page) that returns a finite page (and next-cursor
token) of WalletBalance records requiring refresh, deprecate/remove findAll(),
update the caller(s) to loop using the limit/cursor until exhausted (or stop
after configured max per sync window), and ensure the repository implementation
only selects balances whose refresh timestamp or state indicates they are due
rather than returning the entire table.
Comment on lines
+144
to
+165
| private void checkConfirmations(ChainTransfer transfer, long receiptBlockNumber, | ||
| BigDecimal gasUsed, BigDecimal effectiveGasPrice) { | ||
| var latestBlock = chainRpcProvider.getLatestBlockNumber(transfer.chainId()); | ||
| var currentConfirmations = (int) (latestBlock - receiptBlockNumber); | ||
| var minConfirmations = chainConfirmationProperties.getMinConfirmations(transfer.chainId().value()); | ||
|
|
||
| if (currentConfirmations >= minConfirmations) { | ||
| var confirmed = transfer.confirm(receiptBlockNumber, currentConfirmations, gasUsed, effectiveGasPrice); | ||
| chainTransferRepository.save(confirmed); | ||
| lifecycleEventRepository.save( | ||
| TransferLifecycleEvent.record(confirmed.transferId(), "CONFIRMED")); | ||
|
|
||
| confirmDebitBalance(confirmed); | ||
| publishConfirmedEvent(confirmed); | ||
|
|
||
| log.info("Transfer {} CONFIRMED at block {} with {} confirmations", | ||
| transfer.transferId(), receiptBlockNumber, currentConfirmations); | ||
| } else { | ||
| log.debug("Transfer {} has {} of {} required confirmations", | ||
| transfer.transferId(), currentConfirmations, minConfirmations); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Potential integer overflow on block difference calculation.
(int) (latestBlock - receiptBlockNumber) can overflow if latestBlock is extremely large or receiptBlockNumber is unexpectedly negative. For EVM chains this is unlikely but a sanity check would be defensive.
Defensive bounds check
private void checkConfirmations(ChainTransfer transfer, long receiptBlockNumber,
BigDecimal gasUsed, BigDecimal effectiveGasPrice) {
var latestBlock = chainRpcProvider.getLatestBlockNumber(transfer.chainId());
- var currentConfirmations = (int) (latestBlock - receiptBlockNumber);
+ var confirmationDiff = latestBlock - receiptBlockNumber;
+ var currentConfirmations = (int) Math.max(0, Math.min(confirmationDiff, Integer.MAX_VALUE));
var minConfirmations = chainConfirmationProperties.getMinConfirmations(transfer.chainId().value());🤖 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/TransferMonitorCommandHandler.java`
around lines 144 - 165, In checkConfirmations, avoid casting the block
difference directly to int; compute the difference as a long (latestBlock -
receiptBlockNumber), clamp it to a non-negative value and to Integer.MAX_VALUE
before casting to int to prevent overflow or negative confirmations, then use
that safe int for currentConfirmations and the subsequent logic (references:
checkConfirmations, latestBlock, receiptBlockNumber, currentConfirmations).
Comment on lines
+201
to
+211
| private void confirmDebitBalance(ChainTransfer transfer) { | ||
| var balanceOpt = walletBalanceRepository.findByWalletIdAndStablecoinForUpdate( | ||
| transfer.fromWalletId(), transfer.stablecoin()); | ||
|
|
||
| balanceOpt.ifPresent(balance -> { | ||
| var updated = balance.confirmDebit(transfer.amount()); | ||
| walletBalanceRepository.save(updated); | ||
| log.info("Confirmed debit of {} {} from wallet {}", | ||
| transfer.amount(), transfer.stablecoin().ticker(), transfer.fromWalletId()); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Silent no-op when balance record is missing.
ifPresent quietly skips debit if WalletBalance not found. A confirmed transfer without balance adjustment is a reconciliation gap — consider logging at WARN or failing the confirmation.
Proposed fix
private void confirmDebitBalance(ChainTransfer transfer) {
var balanceOpt = walletBalanceRepository.findByWalletIdAndStablecoinForUpdate(
transfer.fromWalletId(), transfer.stablecoin());
- balanceOpt.ifPresent(balance -> {
+ if (balanceOpt.isEmpty()) {
+ log.warn("Balance not found for confirmed transfer {} — wallet={} stablecoin={}",
+ transfer.transferId(), transfer.fromWalletId(), transfer.stablecoin().ticker());
+ return;
+ }
+
+ balanceOpt.ifPresent(balance -> {
var updated = balance.confirmDebit(transfer.amount());
walletBalanceRepository.save(updated);
log.info("Confirmed debit of {} {} from wallet {}",
transfer.amount(), transfer.stablecoin().ticker(), transfer.fromWalletId());
});
}🤖 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/TransferMonitorCommandHandler.java`
around lines 201 - 211, confirmDebitBalance silently no-ops when
walletBalanceRepository.findByWalletIdAndStablecoinForUpdate returns empty,
which can hide reconciliation gaps; change the method to handle the missing
WalletBalance explicitly by using ifPresentOrElse (or orElseThrow) on the
Optional returned from findByWalletIdAndStablecoinForUpdate: in the present
branch keep the existing logic (call balance.confirmDebit(transfer.amount()),
save via walletBalanceRepository.save(updated), and info-log the confirmation),
and in the empty branch either log a WARN with identifying transfer details
(transfer.fromWalletId(), transfer.stablecoin().ticker(), transfer.amount(),
transfer.id()) and throw a runtime/checked exception (or a domain-specific
exception) to fail the confirmation so callers cannot treat it as successful.
Comment on lines
+104
to
+106
| // then | ||
| then(chainRpcProvider).shouldHaveNoInteractions(); | ||
| then(walletBalanceRepository).should(never()).save(eqIgnoringTimestamps(balance)); |
There was a problem hiding this comment.
Strengthen the negative assertions in these skip-path tests.
These checks only prove that save(...) was never called with the specific matcher. A mutated balance could still be persisted and both tests would pass. Assert no save at all (never().save(any()) / no more interactions) and, in the no-new-blocks case, explicitly verify getTokenBalance(...) is never invoked. As per coding guidelines, "Assert on meaningful values — avoid assertTrue(result != null)".
Also applies to: 134-135
🤖 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/BalanceSyncCommandHandlerTest.java`
around lines 104 - 106, Update the negative assertions in
BalanceSyncCommandHandlerTest: replace the specific save matcher assertions
(then(walletBalanceRepository).should(never()).save(eqIgnoringTimestamps(balance)))
with a blanket verification that no save occurred
(then(walletBalanceRepository).should(never()).save(any())) or use
then(walletBalanceRepository).shouldHaveNoMoreInteractions() to ensure nothing
was persisted; additionally, in the "no-new-blocks" test add an explicit
verification that chainRpcProvider.getTokenBalance(...) is never invoked
(then(chainRpcProvider).should(never()).getTokenBalance(...)). Apply the same
changes at the other occurrence around lines 134-135 so both skip-path tests
assert no saves and no token-balance RPC calls.
…on isolation, fail-fast config (STA-142) Fixes 6 review findings from PR #145: 1. **Resubmission crash safety** — persist claim before calling custody engine (claimResubmission → signAndSubmit → confirmResubmission) to prevent double-submission on crash between custody call and save. 2. **Per-transfer transaction isolation** — remove class-level @transactional, wrap each processTransfer() in TransactionTemplate(REQUIRES_NEW) so failures in one transfer don't affect others. 3. **Null token contract** — add TokenContractResolver domain port to resolve token contract addresses per chain/stablecoin; BalanceSyncCommandHandler now passes the correct contract to getTokenBalance() instead of null. 4. **Fail-fast for unknown chains** — ChainConfirmationProperties throws IllegalStateException for unconfigured chains instead of silently defaulting to 1 confirmation. ChainProperties rejects non-positive minConfirmations. 5. **BalanceSync transaction scope** — remove class-level @transactional so RPC calls happen outside any DB transaction; each save runs in its own transaction. 6. **CONFIRMING stall protection** — if receipt disappears beyond a configurable grace window (confirmingTimeoutS, default 300s), transfer is marked for resubmission instead of stalling indefinitely. Added CONFIRMING → RESUBMITTING state transition for reorg recovery. Also gates fallback TransferEventPublisher behind @ConditionalOnProperty to prevent silent event loss in production. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…mandHandlerTest Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
blockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/domain/model/ChainTransfer.java (1)
244-256: 🧹 Nitpick | 🔵 TrivialConsider documenting deprecation of
resubmit()in favor of crash-safe pattern.With
claimResubmission()+confirmResubmission()providing crash safety, the originalresubmit()method becomes the non-crash-safe alternative. Consider deprecating or documenting when to use each approach to prevent misuse.🤖 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/ChainTransfer.java` around lines 244 - 256, Mark the non-crash-safe resubmit method as deprecated and update its Javadoc to point callers to the crash-safe claimResubmission() + confirmResubmission() flow: add the `@Deprecated` annotation to ChainTransfer.resubmit(String) and a brief Javadoc explaining it's the non-crash-safe alternative, when (if ever) it may still be used, and instructs to use claimResubmission() followed by confirmResubmission() for crash-safe behavior; also update any internal callers to prefer the new flow where applicable.
♻️ Duplicate comments (4)
blockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/config/FallbackAdaptersConfig.java (1)
143-145:⚠️ Potential issue | 🟠 MajorDisable the fallback publisher by default.
matchIfMissing = truestill activates this log-onlyTransferEventPublisherwhenever production wiring forgets the property, so terminal transfer events can be silently downgraded to WARN logs instead of being published. SinceTransferMonitorCommandHandlerrelies on this port for confirmation/failure propagation, this should be explicit opt-in only.Suggested change
`@Bean` `@ConditionalOnMissingBean` `@ConditionalOnProperty`(name = "app.transfer.event-publisher.fallback-enabled", - havingValue = "true", matchIfMissing = true) + havingValue = "true", matchIfMissing = false) public TransferEventPublisher fallbackTransferEventPublisher() { log.info("Using fallback TransferEventPublisher (log only)"); return event -> log.warn("[FALLBACK-EVENT] Published event: {}", event); }🤖 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 143 - 145, The fallback, log-only TransferEventPublisher is being enabled by default because the `@ConditionalOnProperty` on the bean in FallbackAdaptersConfig uses matchIfMissing = true; change it to matchIfMissing = false so the fallback publisher is opt-in only (i.e., only active when app.transfer.event-publisher.fallback-enabled=true). Locate the bean definition for the fallback TransferEventPublisher in class FallbackAdaptersConfig and update the `@ConditionalOnProperty` annotation accordingly.blockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/domain/service/TransferMonitorCommandHandler.java (2)
252-261:⚠️ Potential issue | 🟠 MajorDo not silently confirm when the balance row is missing.
If this Optional is empty, the transfer is already saved as
CONFIRMEDand the event is published, but the reserved balance is never reconciled. Throw here so the transfer transaction rolls back instead of leaving a reconciliation gap.Fail fast on a missing balance row
private void confirmDebitBalance(ChainTransfer transfer) { - var balanceOpt = walletBalanceRepository.findByWalletIdAndStablecoinForUpdate( - transfer.fromWalletId(), transfer.stablecoin()); - - balanceOpt.ifPresent(balance -> { - var updated = balance.confirmDebit(transfer.amount()); - walletBalanceRepository.save(updated); - log.info("Confirmed debit of {} {} from wallet {}", - transfer.amount(), transfer.stablecoin().ticker(), transfer.fromWalletId()); - }); + var balance = walletBalanceRepository.findByWalletIdAndStablecoinForUpdate( + transfer.fromWalletId(), transfer.stablecoin()) + .orElseThrow(() -> new IllegalStateException( + "Missing balance for transferId=" + transfer.transferId())); + + var updated = balance.confirmDebit(transfer.amount()); + walletBalanceRepository.save(updated); + log.info("Confirmed debit of {} {} from wallet {}", + transfer.amount(), transfer.stablecoin().ticker(), transfer.fromWalletId()); }🤖 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/TransferMonitorCommandHandler.java` around lines 252 - 261, In confirmDebitBalance, do not silently skip when walletBalanceRepository.findByWalletIdAndStablecoinForUpdate(...) returns empty; instead throw a runtime exception (e.g., IllegalStateException or a custom TransferReconciliationException) that includes the transfer id/fromWalletId/stablecoin/timestamp so the enclosing transaction rolls back and the missing balance is noticed; keep the existing happy-path (call balance.confirmDebit(transfer.amount()), save, and log) but add an explicit else path that throws using the transfer details to make failure audible and traceable.
106-109:⚠️ Potential issue | 🔴 CriticalThe resubmission claim is still inside the custody transaction.
The
TransactionTemplateopened inmonitorPendingTransfers()spans the claim save,signAndSubmit(), and the final save. If custody succeeds and the later persistence step fails, Spring rolls the earlier claim back too, and the next poll can submit the transfer again. Commit the claim in its own transaction before calling custody, or makeCustodyEngineidempotent ontransferId.Also applies to: 222-247
🤖 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/TransferMonitorCommandHandler.java` around lines 106 - 109, The custody submission is happening inside the same TransactionTemplate started by monitorPendingTransfers() so if the final persistence after signAndSubmit() fails the earlier claim save is rolled back and the transfer can be resubmitted; move the claim persistence into its own transaction before calling CustodyEngine.signAndSubmit() (e.g., commit the claim save in a separate transaction or call transactionTemplate.execute for only the save step) or alternatively make CustodyEngine.signAndSubmit()/CustodyEngine implementations idempotent by checking transferId to ensure duplicate submissions are ignored; update processTransfer(transfer) to persist the claim in a separate transaction prior to calling signAndSubmit() and then perform the remaining saves in the outer transaction.blockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/domain/service/BalanceSyncCommandHandler.java (1)
36-37:⚠️ Potential issue | 🟠 MajorThis 30s polling path still does a full scan plus N+1 lookups.
findAll()loads the entire balance table,walletRepository.findById(...)adds one DB hit per row, andgetLatestBlockNumber(...)adds one RPC hit per row. Page balances, fetch wallet address with the balance query, and cache latest block per chain for the run.Also applies to: 49-60
🤖 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/BalanceSyncCommandHandler.java` around lines 36 - 37, The syncAllBalances() path in BalanceSyncCommandHandler currently calls walletBalanceRepository.findAll() and then walletRepository.findById(...) and getLatestBlockNumber(...) per balance causing full-table load and N+1 DB/RPC hits; change it to page through balances instead of findAll(), modify the balance query (or repository method) to join or select the wallet address together so you don't need walletRepository.findById(...) for each row, and add a per-run cache (e.g. a Map<chainId, Long>) to store the result of getLatestBlockNumber(chainId) so you only call the RPC once per chain; apply the same pagination/join/caching approach to the similar logic used in the other sync method (lines 49-60) in this class.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@blockchain-custody/blockchain-custody/src/test/java/com/stablecoin/payments/custody/domain/service/BalanceSyncCommandHandlerTest.java`:
- Line 10: Remove the unused import TokenContractResolver from the test file;
locate the import statement for TokenContractResolver at the top of
BalanceSyncCommandHandlerTest and delete it (the test already uses the fixture
method defaultTokenContractResolver(), so no replacement is needed), then run
./gradlew spotlessApply to format and ensure Spotless passes.
In
`@blockchain-custody/blockchain-custody/src/test/java/com/stablecoin/payments/custody/domain/service/TransferMonitorCommandHandlerTest.java`:
- Around line 441-449: The test currently verifies interactions but not
ordering; update TransferMonitorCommandHandlerTest to use a Mockito InOrder for
chainTransferRepository, custodyEngine and lifecycleEventRepository and assert
the sequence: first
chainTransferRepository.save(eqIgnoringTimestamps(expectedClaimed)), then
custodyEngine.signAndSubmit(eqIgnoringTimestamps(signRequest)), then
chainTransferRepository.save(eqIgnoringTimestamps(expectedResubmitted)), and
finally lifecycleEventRepository.save(...) calls
(SUBMITTED/RESUBMISSION_CLAIMED) in the same InOrder so the claim-before-submit
crash-safety ordering is enforced.
In
`@blockchain-custody/blockchain-custody/src/testFixtures/java/com/stablecoin/payments/custody/fixtures/TransferMonitorFixtures.java`:
- Around line 84-89: The defaultTokenContractResolver fixture always returns
USDC_BASE_CONTRACT regardless of (chainId, stablecoin), which masks incorrect
lookups; change defaultTokenContractResolver to use a keyed Map of (chainId,
stablecoin) -> contract (populate with expected test pairs like the USDC entry)
and have the resolver lookup the map and throw an exception (or assert) for
unknown keys so tests fail on wrong chain/token resolution; update references to
TokenContractResolver and USDC_BASE_CONTRACT in the fixture to perform map-based
lookup and error-on-miss behavior.
---
Outside diff comments:
In
`@blockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/domain/model/ChainTransfer.java`:
- Around line 244-256: Mark the non-crash-safe resubmit method as deprecated and
update its Javadoc to point callers to the crash-safe claimResubmission() +
confirmResubmission() flow: add the `@Deprecated` annotation to
ChainTransfer.resubmit(String) and a brief Javadoc explaining it's the
non-crash-safe alternative, when (if ever) it may still be used, and instructs
to use claimResubmission() followed by confirmResubmission() for crash-safe
behavior; also update any internal callers to prefer the new flow where
applicable.
---
Duplicate comments:
In
`@blockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/config/FallbackAdaptersConfig.java`:
- Around line 143-145: The fallback, log-only TransferEventPublisher is being
enabled by default because the `@ConditionalOnProperty` on the bean in
FallbackAdaptersConfig uses matchIfMissing = true; change it to matchIfMissing =
false so the fallback publisher is opt-in only (i.e., only active when
app.transfer.event-publisher.fallback-enabled=true). Locate the bean definition
for the fallback TransferEventPublisher in class FallbackAdaptersConfig and
update the `@ConditionalOnProperty` annotation accordingly.
In
`@blockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/domain/service/BalanceSyncCommandHandler.java`:
- Around line 36-37: The syncAllBalances() path in BalanceSyncCommandHandler
currently calls walletBalanceRepository.findAll() and then
walletRepository.findById(...) and getLatestBlockNumber(...) per balance causing
full-table load and N+1 DB/RPC hits; change it to page through balances instead
of findAll(), modify the balance query (or repository method) to join or select
the wallet address together so you don't need walletRepository.findById(...) for
each row, and add a per-run cache (e.g. a Map<chainId, Long>) to store the
result of getLatestBlockNumber(chainId) so you only call the RPC once per chain;
apply the same pagination/join/caching approach to the similar logic used in the
other sync method (lines 49-60) in this class.
In
`@blockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/domain/service/TransferMonitorCommandHandler.java`:
- Around line 252-261: In confirmDebitBalance, do not silently skip when
walletBalanceRepository.findByWalletIdAndStablecoinForUpdate(...) returns empty;
instead throw a runtime exception (e.g., IllegalStateException or a custom
TransferReconciliationException) that includes the transfer
id/fromWalletId/stablecoin/timestamp so the enclosing transaction rolls back and
the missing balance is noticed; keep the existing happy-path (call
balance.confirmDebit(transfer.amount()), save, and log) but add an explicit else
path that throws using the transfer details to make failure audible and
traceable.
- Around line 106-109: The custody submission is happening inside the same
TransactionTemplate started by monitorPendingTransfers() so if the final
persistence after signAndSubmit() fails the earlier claim save is rolled back
and the transfer can be resubmitted; move the claim persistence into its own
transaction before calling CustodyEngine.signAndSubmit() (e.g., commit the claim
save in a separate transaction or call transactionTemplate.execute for only the
save step) or alternatively make CustodyEngine.signAndSubmit()/CustodyEngine
implementations idempotent by checking transferId to ensure duplicate
submissions are ignored; update processTransfer(transfer) to persist the claim
in a separate transaction prior to calling signAndSubmit() and then perform the
remaining saves in the outer transaction.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c301f576-3bc6-467f-86ea-f4de15e084e8
📒 Files selected for processing (14)
blockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/application/config/ChainMonitorConfig.javablockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/application/config/TransferMonitorConfig.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/model/ChainTransfer.javablockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/domain/port/ChainConfirmationProperties.javablockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/domain/port/TokenContractResolver.javablockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/domain/port/TransferMonitorProperties.javablockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/domain/service/BalanceSyncCommandHandler.javablockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/domain/service/TransferMonitorCommandHandler.javablockchain-custody/blockchain-custody/src/main/resources/application.ymlblockchain-custody/blockchain-custody/src/test/java/com/stablecoin/payments/custody/domain/model/ChainTransferTest.javablockchain-custody/blockchain-custody/src/test/java/com/stablecoin/payments/custody/domain/service/BalanceSyncCommandHandlerTest.javablockchain-custody/blockchain-custody/src/test/java/com/stablecoin/payments/custody/domain/service/TransferMonitorCommandHandlerTest.javablockchain-custody/blockchain-custody/src/testFixtures/java/com/stablecoin/payments/custody/fixtures/TransferMonitorFixtures.java
Comment on lines
+38
to
+51
| public String resolveContract(ChainId chainId, StablecoinTicker stablecoin) { | ||
| var props = chains.get(chainId.value()); | ||
| if (props == null) { | ||
| throw new IllegalStateException( | ||
| "Missing chain config for chainId=%s. Configured chains: %s" | ||
| .formatted(chainId.value(), chains.keySet())); | ||
| } | ||
| var contracts = props.tokenContracts(); | ||
| if (contracts == null || !contracts.containsKey(stablecoin.ticker())) { | ||
| throw new IllegalStateException( | ||
| "No token contract configured for chain=%s stablecoin=%s" | ||
| .formatted(chainId.value(), stablecoin.ticker())); | ||
| } | ||
| return contracts.get(stablecoin.ticker()); |
There was a problem hiding this comment.
Reject blank token-contract values at startup.
containsKey() treats USDC: "" as configured, so resolveContract() can return an empty contract and the sync path will send it to the RPC provider. Validate tokenContracts values as non-blank in ChainProperties and fail fast.
Fail-fast validation
public ChainProperties {
if (minConfirmations <= 0) {
throw new IllegalArgumentException(
"minConfirmations must be positive, got: " + minConfirmations);
}
if (avgFinalityS <= 0) {
avgFinalityS = 12;
}
if (tokenContracts == null) {
tokenContracts = Map.of();
}
+ if (tokenContracts.values().stream().anyMatch(v -> v == null || v.isBlank())) {
+ throw new IllegalArgumentException("tokenContracts must not contain blank values");
+ }
}Also applies to: 64-74
Comment on lines
+441
to
+449
| // then — claim persisted first, then resubmission | ||
| then(chainTransferRepository).should().save(eqIgnoringTimestamps(expectedClaimed)); | ||
| then(custodyEngine).should().signAndSubmit(eqIgnoringTimestamps(signRequest)); | ||
| then(chainTransferRepository).should().save(eqIgnoringTimestamps(expectedResubmitted)); | ||
| then(lifecycleEventRepository).should().save( | ||
| eqIgnoring(TransferLifecycleEvent.record(transfer.transferId(), "RESUBMISSION_CLAIMED"), "eventId")); | ||
| then(lifecycleEventRepository).should().save( | ||
| eqIgnoring(TransferLifecycleEvent.record(transfer.transferId(), "SUBMITTED"), "eventId")); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and read the test file
git ls-files | grep -i "TransferMonitorCommandHandlerTest.java"Repository: Puneethkumarck/stablebridge-platform
Length of output: 217
🏁 Script executed:
# Read the test file to understand the context around lines 441-449
wc -l blockchain-custody/blockchain-custody/src/test/java/com/stablecoin/payments/custody/domain/service/TransferMonitorCommandHandlerTest.javaRepository: Puneethkumarck/stablebridge-platform
Length of output: 221
🏁 Script executed:
# Read the specific test method and surrounding context
sed -n '400,500p' blockchain-custody/blockchain-custody/src/test/java/com/stablecoin/payments/custody/domain/service/TransferMonitorCommandHandlerTest.javaRepository: Puneethkumarck/stablebridge-platform
Length of output: 4743
🏁 Script executed:
# Check the handler implementation to understand transaction context
sed -n '1,100p' blockchain-custody/blockchain-custody/src/test/java/com/stablecoin/payments/custody/domain/service/TransferMonitorCommandHandlerTest.javaRepository: Puneethkumarck/stablebridge-platform
Length of output: 5529
🏁 Script executed:
# Also search for the handler class itself
git ls-files | grep -i "TransferMonitorCommandHandler.java" | grep -v TestRepository: Puneethkumarck/stablebridge-platform
Length of output: 213
🏁 Script executed:
# Read the handler implementation to understand transaction semantics
wc -l blockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/domain/service/TransferMonitorCommandHandler.javaRepository: Puneethkumarck/stablebridge-platform
Length of output: 217
🏁 Script executed:
# Search for the monitorPendingTransfers method in the handler
rg -A 50 "monitorPendingTransfers" blockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/domain/service/TransferMonitorCommandHandler.javaRepository: Puneethkumarck/stablebridge-platform
Length of output: 2406
🏁 Script executed:
# Find the processResubmitting method
rg -A 30 "processResubmitting" blockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/domain/service/TransferMonitorCommandHandler.javaRepository: Puneethkumarck/stablebridge-platform
Length of output: 3211
🏁 Script executed:
# Read the rest of processResubmitting to see the complete flow
rg -A 50 "Step 2: Call custody engine" blockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/domain/service/TransferMonitorCommandHandler.javaRepository: Puneethkumarck/stablebridge-platform
Length of output: 2215
Enforce call sequence with InOrder for crash-safety validation.
The handler explicitly documents that the claim-before-submit ordering is a crash-recovery checkpoint: after custodyEngine.signAndSubmit() fails, the incremented attempt count persists, allowing idempotent retry with the same nonce. Your current assertions only verify the interactions occurred, not their sequence. With Mockito's then().should(), this test still passes if signAndSubmit() executes before persisting the claimed transfer, masking a regression in the documented crash-safety guarantee.
Suggested assertion shape
+import org.mockito.InOrder;
+import static org.mockito.Mockito.inOrder;
...
- then(chainTransferRepository).should().save(eqIgnoringTimestamps(expectedClaimed));
- then(custodyEngine).should().signAndSubmit(eqIgnoringTimestamps(signRequest));
- then(chainTransferRepository).should().save(eqIgnoringTimestamps(expectedResubmitted));
+ InOrder inOrder = inOrder(chainTransferRepository, custodyEngine);
+ inOrder.verify(chainTransferRepository).save(eqIgnoringTimestamps(expectedClaimed));
+ inOrder.verify(custodyEngine).signAndSubmit(eqIgnoringTimestamps(signRequest));
+ inOrder.verify(chainTransferRepository).save(eqIgnoringTimestamps(expectedResubmitted));🤖 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/TransferMonitorCommandHandlerTest.java`
around lines 441 - 449, The test currently verifies interactions but not
ordering; update TransferMonitorCommandHandlerTest to use a Mockito InOrder for
chainTransferRepository, custodyEngine and lifecycleEventRepository and assert
the sequence: first
chainTransferRepository.save(eqIgnoringTimestamps(expectedClaimed)), then
custodyEngine.signAndSubmit(eqIgnoringTimestamps(signRequest)), then
chainTransferRepository.save(eqIgnoringTimestamps(expectedResubmitted)), and
finally lifecycleEventRepository.save(...) calls
(SUBMITTED/RESUBMISSION_CLAIMED) in the same InOrder so the claim-before-submit
crash-safety ordering is enforced.
Comment on lines
+84
to
+89
| /** | ||
| * Default token contract resolver mapping USDC to test contract addresses. | ||
| */ | ||
| public static TokenContractResolver defaultTokenContractResolver() { | ||
| return (chainId, stablecoin) -> USDC_BASE_CONTRACT; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Make the resolver fixture input-sensitive.
Returning one constant contract for every (chainId, stablecoin) means tests using this helper cannot catch a wrong chain/token lookup. Back this with a keyed map and throw on unknown pairs so the fixture exercises the real resolution behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@blockchain-custody/blockchain-custody/src/testFixtures/java/com/stablecoin/payments/custody/fixtures/TransferMonitorFixtures.java`
around lines 84 - 89, The defaultTokenContractResolver fixture always returns
USDC_BASE_CONTRACT regardless of (chainId, stablecoin), which masks incorrect
lookups; change defaultTokenContractResolver to use a keyed Map of (chainId,
stablecoin) -> contract (populate with expected test pairs like the USDC entry)
and have the resolver lookup the map and throw an exception (or assert) for
unknown keys so tests fail on wrong chain/token resolution; update references to
TokenContractResolver and USDC_BASE_CONTRACT in the fixture to perform map-based
lookup and error-on-miss behavior.
This was referenced Mar 9, 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
@Scheduleddelegators (15s / 30s polling intervals)TransferMonitorProperties,ChainConfirmationProperties) keep domain layer clean of application config dependenciesTest plan
TransferMonitorCommandHandlerTest— 9 tests (confirm, not-enough-confirmations, stuck detection, no-stuck, revert, resubmit, max-attempts, no-transfers)BalanceSyncCommandHandlerTest— 5 tests (sync, wallet-not-found, no-new-blocks, RPC-failure-resilience, no-balances)./gradlew :blockchain-custody:blockchain-custody:checkpasses (unit + integration + jacoco)Closes STA-142
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests