fix(rebalancer): wait for inventory rebalance tx confirmation#3503
Conversation
InventoryClient.rebalanceInventoryIfNeeded submits cross-chain rebalances via
BaseChainAdapter.sendTokenToTargetChain, which built its AugmentedTransaction
without ensureConfirmation. As a result TransactionClient._submit skipped its
receipt-wait + replacement-handling block and returned the pending tx, so the
bot read { hash } and moved on before the rebalance was mined.
Set ensureConfirmation: true so the rebalance routes through the shared
confirmation loop, which awaits the receipt and is robust to RPC errors like
TRANSACTION_REPLACED (resubmits under a fresh nonce instead of throwing) — the
same idiom already used by the refiller, standalone rebalancer adapters,
dataworker, and finalizers.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Thanks for the review and approval, @pxrl 🙏 I fetched this review's inline comments ( One open item from the PR description still stands if you want it as a follow-up: the shared |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4be95a023e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Block until the rebalance is mined rather than returning a pending tx. TransactionClient's | ||
| // confirmation loop awaits the receipt and is robust to RPC errors like transaction replacement | ||
| // (TRANSACTION_REPLACED), resubmitting under a fresh nonce instead of throwing. | ||
| ensureConfirmation: true, |
There was a problem hiding this comment.
Do not resubmit repriced bridge transactions
Because this line now opts inventory bridge deposits into TransactionClient._submit's confirmation path, any same-nonce speed-up/reprice of the transaction can double-send funds: ethers reports a mined same-calldata replacement as TRANSACTION_REPLACED with reason: "repriced"/cancelled: false, but src/clients/TransactionClient.ts:162-167 unconditionally resubmits under a fresh nonce. In that scenario the original logical bridge transfer has already mined, and the recursive submit sends the same rebalance amount again; this should either treat non-cancelled replacements as success using the provided receipt, or avoid enabling confirmation until that distinction is handled.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — this is a real P1. ethers classifies a repriced replacement (cancelled === false) as a gas speed-up of the same logical transaction, which has already mined under a new hash, so the unconditional resubmit here double-sends the transfer. It affects every ensureConfirmation caller, not just the inventory path this PR added.
This PR merged ~2 minutes before the review landed, so the fix is in a follow-up: #3504. There the TRANSACTION_REPLACED branch returns the mined replacement when cancelled === false and only resubmits on genuine cancellations/replacements (cancelled === true), with a regression test covering both paths.
|
Thanks Codex — the P1 is valid and I've actioned it. This PR had already merged (~22:01Z) about 2 minutes before the review landed (~22:03Z), so I couldn't amend it in place. The fix is in a follow-up PR: #3504. Summary of the issue and fix:
I've replied on the inline thread as well and left #3504 for human review (not self-merging). |
Problem
The inventory rebalancer submits a cross-chain transfer and then immediately moves on without waiting for the transaction to be mined. The flagged Slack run ("Executed Inventory rebalances 📒") reported rebalances to BNB / zkSync / Base as done while the txs were still only submitted.
Trace:
InventoryClient.rebalanceInventoryIfNeeded→sendTokenCrossChain→AdapterManager.sendTokenCrossChain→BaseChainAdapter.sendTokenToTargetChain→submitTransaction.In
sendTokenToTargetChain, theAugmentedTransactionwas built withoutensureConfirmation. SoTransactionClient._submitskips its entireif (txn.ensureConfirmation)block — the one that awaitstxnResponse.wait()and handlesTRANSACTION_REPLACED— and returns the pending tx. The caller then just reads{ hash }and returns.Fix
Set
ensureConfirmation: trueon the rebalance transaction. This routes it through the shared confirmation loop inTransactionClient._submit, which:TRANSACTION_REPLACEDit resubmits under a fresh nonce instead of throwing; onCALL_EXCEPTIONit surfaces the revert; otherwise it retries the wait up tomaxTries.This is the same idiom already used by the refiller, the standalone
src/rebalancer/adapters/*, the dataworker, and the finalizers — this inventory path was the one place that omitted it.Scope
In production
sendTokenToTargetChainis reached only via the inventory rebalancer (AdapterManager.sendTokenCrossChain←InventoryClient), so this does not change behavior for other bots.Testing
tsc --noEmit, eslint, prettier: cleantest/generic-adapters/AdapterManager.SendTokensCrossChain.ts— 6 passingtest/generic-adapters/zkSync.ts— 21 passingNote / possible follow-up
The shared
TRANSACTION_REPLACEDhandler resubmits on any replacement. In the ethersreason: "repriced"case (a same-nonce speed-up of the same logical tx) the replacement was already mined, so a resubmit could double-send. That's pre-existing behavior in the shared handler (affects everyensureConfirmationcaller), so I left it out of this targeted fix — happy to tighten it to inspecterror.cancelled/error.receiptas a separate change if wanted.🤖 Generated with Claude Code