Make tx max fee reimbursement best effort#968
Open
mswilkison wants to merge 8 commits into
Open
Conversation
mswilkison
changed the title
mswilkison
requested review from
Shadowfiend,
lrsaturnino,
lukasz-zimnoch,
nkuba,
pdyraga and
piotr-roslaniec
as code owners
The flag-doc block in AbstractL1BTCDepositor (solidity and starknet
copies) still described the pre-patch behavior ("Add txMaxFee to the
minted tBTC amount"), which contradicts the post-patch code path that
only adds txMaxFee when the contract balance covers it. The Wormhole V2
variants already carry the corrected wording; this commit mirrors that
phrasing into both abstract copies for documentation congruence with the
implementation in finalizeDeposit.
The V2Base depositor previously had no dedicated test file; coverage relied implicitly on the V2Arbitrum twin's tests. The new file covers the four reimbursement scenarios exercised by finalizeDeposit: - reimburseTxMaxFee is false: contract pays only the base tbtcAmount. - reimburseTxMaxFee is true with balance >= reimbursedAmount: contract pays base + txMaxFee; no skip event. - reimburseTxMaxFee is true with balance one wei short of reimbursedAmount: contract pays only base, emits DepositTxMaxFeeReimbursementSkipped with the available balance. - reimburseTxMaxFee is true with zero balance: contract pays only base, emits the skip event with availableBalance == 0, and the L2 receiver payload is preserved through the skip branch. All assertions read the post-call tBTC allowance to the Wormhole token bridge and the event log; the contract source itself is unchanged.
The existing finalizeDeposit suite only exercised the reimburseTxMaxFee=false path. Three additional contexts now cover the balance-gated branches added by the best-effort patch: - balance covers reimbursedAmount: contract transfers base + txMaxFee and emits no skip event. - balance is one wei below reimbursedAmount: contract transfers only base and emits DepositTxMaxFeeReimbursementSkipped with the available balance. - proxy holds zero tBTC: contract transfers only base, emits the skip event with availableBalance == 0, and the L2 receiver payload survives through the skip branch. Assertions read the post-call tBTC allowance to the Wormhole token bridge plus the event log; contract source is unchanged.
Two NatSpec defects shared by the solidity and starknet copies: - "gas dgasd" garbled token in the initializeDeposit doc block (corrected to "gas refund"). - Empty backticks (`\`\`` contract.) inside the Bitcoin deposit script description where the contract name was meant to be inlined (filled in with `AbstractL1BTCDepositor`). Documentation only; no behavioral change.
External review flagged two gaps in the prior reimbursement-branch tests: 1. The "proxy holds zero tBTC" context was a false positive. In production, a real Wormhole Token Bridge pulls baseTbtcAmount from the proxy via transferFrom during finalizeDeposit; with zero balance that transfer reverts and rolls back the skip event together with the state transition. The smock fake in the unit test does not pull tokens, so the test passed against behavior that does not exist on mainnet. The context is replaced with "balance equals baseTbtcAmount" in both V2Base and V2Arbitrum suites; this matches the real Rafael / Base recipient scenarios where the Bridge sweep delivered base but not the full base + txMaxFee, and the skip event arg availableBalance is now asserted as baseTbtcAmount. 2. The reimburseTxMaxFee=true contexts only asserted the allowance and the presence/absence of the skip event. A regression that transferred the correct amount but emitted a wrong tbtcAmount in DepositFinalized would have slipped through. Every reimbursement-branch context now asserts DepositFinalized with the exact expected initialAmountWei and tbtcAmount (reimbursedAmount for the covered path, baseTbtcAmount for both skip-paths). Contract source is unchanged; only the two new V2 test files are modified.
piotr-roslaniec
approved these changes
May 22, 2026
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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
Updates
AbstractL1BTCDepositor.finalizeDepositso the deposit transaction max fee reimbursement is best-effort instead of mandatory.When
reimburseTxMaxFeeis enabled, the depositor now:depositTxMaxFeereimbursement only if the contract balance can cover the fully reimbursed amountDepositTxMaxFeeReimbursementSkippedwhen the reimbursement is skipped because the shared depositor balance is shortThis keeps finalization live when a small reimbursement shortfall would otherwise block forwarding tBTC to the user's final destination address.
Review follow-ups also:
reimburseTxMaxFee = trueno longer reads as a guaranteed reimbursementRoot cause
The current code always adds
depositTxMaxFeeto the amount forwarded when reimbursement is enabled. If earlier finalizations consume enough of the shared depositor contract balance, a later finalization can have enough tBTC to cover the base deposit amount but not enough to also cover the reimbursement. In that case the final transfer reverts and the depositor waits until unrelated deposits top up the contract.Tradeoff
This favors liveness over exact per-deposit reimbursement. A depositor may miss the
depositTxMaxFeereimbursement when the contract balance is short, but they still receive the base finalized deposit amount immediately instead of being blocked behind later deposits.That means reimbursement is no longer strictly guaranteed by
reimburseTxMaxFee = true; it becomes conditional on available contract balance. The new event makes skipped reimbursements visible for monitoring or possible off-chain reconciliation. If the contract cannot cover even the base amount, finalization can still revert; this PR only prevents the extra reimbursement amount from blocking otherwise-forwardable deposits.Validation
Results:
99 passingforL1BTCDepositorWormhole+L1BTCDepositorWormholeV2Arbitrum149 passingforNativeBTCDepositor+BTCDepositorWormholenpm run formatexits successfully; existing repo warnings remain, with no errors