Skip to content

fix(TransactionClient): don't resubmit repriced (already-mined) replacements#3504

Open
droplet-rl wants to merge 2 commits into
masterfrom
droplet/fix-transaction-replaced-repriced
Open

fix(TransactionClient): don't resubmit repriced (already-mined) replacements#3504
droplet-rl wants to merge 2 commits into
masterfrom
droplet/fix-transaction-replaced-repriced

Conversation

@droplet-rl

@droplet-rl droplet-rl commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

Problem

Follow-up to #3503, addressing the Codex P1 raised on that PR (which had already merged ~2 minutes before the review landed).

#3503 opted inventory rebalance transfers into TransactionClient._submit's confirmation loop (ensureConfirmation: true). That loop's TRANSACTION_REPLACED handler resubmits under a fresh nonce on any replacement:

case ethers.errors.TRANSACTION_REPLACED:
  ...
  return this._submit(txn, { nonce: null, maxTries: maxTries - 1 });

ethers (v5) classifies a same-nonce replacement via error.cancelled:

  • reason: "repriced"cancelled === false — a gas speed-up of the same logical transaction. The transfer has already mined under a new hash.
  • reason: "cancelled" / "replaced"cancelled === true — the original did not take effect.

In the repriced case the current code resubmits a transfer that already landed → double-send (e.g. a duplicated inventory rebalance). This affects every ensureConfirmation caller (rebalancer, refiller, finalizers, dataworker), not just the inventory path.

Fix

In the TRANSACTION_REPLACED branch, return the mined replacement when cancelled === false; only resubmit on genuine cancellations/replacements (cancelled === true). Returning error.replacement also hands the caller the actual mined tx (correct hash) rather than the original, now-replaced one.

A repriced replacement can still mine and revert. ethers attaches the replacement's mined receipt to the error, and waitForTransaction does not throw on revert, so receipt.status === 0 would otherwise be reported as a confirmed success. The cancelled === false branch now checks error.receipt.status and, on a reverted replacement, throws (mirroring the existing CALL_EXCEPTION failure path) so callers don't record a reverted rebalance/finalization as confirmed. (Codex P2.)

Testing

  • tsc --noEmit, eslint, prettier: clean
  • test/TransactionClient.ts — 12 passing, including:
    • existing Resubmits on TRANSACTION_REPLACED (genuine replacement → still resubmits),
    • Does not resubmit a repriced (already-mined) replacement (regression guard: wait() called once, mined replacement returned, no resubmit), and
    • new Treats a reverted repriced replacement as a failure (replacement receipt status === 0 → surfaced as a failure, no resubmit).

Docs

No README/AGENTS updates needed — this behavior is documented only in the in-code comment, which is updated here.

🤖 Generated with Claude Code

…cements

The ensureConfirmation TRANSACTION_REPLACED handler resubmitted under a fresh
nonce on *any* replacement. ethers reports a same-nonce speed-up of the *same*
logical transaction as TRANSACTION_REPLACED with cancelled === false; in that
case the transaction has already mined under a new hash, so resubmitting
double-sends (e.g. duplicating an inventory rebalance transfer). Return the
mined replacement instead, and only resubmit on genuine cancellations or
replacements (cancelled === true), where the original did not take effect.

Surfaced by the Codex review on #3503 (P1) and flagged there as a follow-up.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 48f761feb8

ℹ️ 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".

Comment on lines +173 to +178
if (cancelled === false && isDefined(replacement)) {
this.logger.warn({
...common,
message: `Transaction on ${chain} was repriced and already mined; using replacement.`,
});
return replacement;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve failed replacement receipts as failures

When a repriced replacement is mined but reverts, ethers still reports TRANSACTION_REPLACED with cancelled === false and includes the replacement receipt; this branch returns the replacement as a successful TransactionResponse without checking that receipt status. In that scenario an ensureConfirmation caller records a failed on-chain operation as successful instead of following the existing CALL_EXCEPTION failure path, so reverted rebalances/finalizations can be skipped by the bot.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — fixed in 1073374. ethers' waitForTransaction (used internally to fetch the replacement's receipt) doesn't throw on revert, so a repriced replacement that mined with status === 0 was being returned as a confirmed success.

The cancelled === false branch now inspects error.receipt.status: a reverted replacement (status === 0) is thrown, mirroring the CALL_EXCEPTION failure path above, so ensureConfirmation callers no longer record a reverted rebalance/finalization as successful. A successful replacement (status === 1, or an older receipt with no status) is still returned. Added a regression test ("Treats a reverted repriced replacement as a failure").

A repriced replacement (cancelled === false) can mine and revert. ethers
attaches the replacement's mined receipt to the TRANSACTION_REPLACED error,
and waitForTransaction does not throw on revert, so receipt.status === 0 was
previously returned as a confirmed success. Check the receipt status and, when
it reverted, throw (mirroring the CALL_EXCEPTION path) so callers don't record
a reverted rebalance/finalization as confirmed.

Adds a regression test; updates the existing repriced-success test to carry a
status === 1 receipt.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@droplet-rl

Copy link
Copy Markdown
Contributor Author

Thanks @codex — addressed the P2 in 1073374.

A repriced replacement (cancelled === false) can mine and revert. Since ethers' internal waitForTransaction doesn't throw on revert, the replacement's receipt.status === 0 was being returned as a confirmed success. The cancelled === false branch now inspects error.receipt.status and throws on a reverted replacement (mirroring the CALL_EXCEPTION failure path), so ensureConfirmation callers no longer record a reverted rebalance/finalization as successful. A successful replacement is still returned as before. Added a regression test and updated the PR description.

@codex please re-review the updated diff.

@droplet-rl

Copy link
Copy Markdown
Contributor Author

Addressed the Codex review on #3504 (commit 1073374).

The single inline P2 ("Preserve failed replacement receipts as failures") was valid: a repriced replacement (cancelled === false) can mine and revert. ethers attaches the replacement's mined receipt to the TRANSACTION_REPLACED error, and its internal waitForTransaction doesn't throw on revert, so receipt.status === 0 was being returned as a confirmed success — letting an ensureConfirmation caller record a reverted rebalance/finalization as successful.

Fix: the cancelled === false branch now inspects error.receipt.status and, on a reverted replacement (status === 0), throws — mirroring the existing CALL_EXCEPTION failure path. A successful replacement (status === 1, or an older receipt with no status) is still returned as before.

  • Added regression test "Treats a reverted repriced replacement as a failure" (excluded from results, no resubmit); updated the existing repriced-success test to carry a status === 1 receipt. 12 passing.
  • tsc --noEmit, eslint, prettier: clean.
  • Replied on the inline thread and posted a top-level comment tagging @codex for re-review; updated the PR description.

@droplet-rl

Copy link
Copy Markdown
Contributor Author

🔎 View trace

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. More of your lovely PRs please.

Reviewed commit: 107337401c

ℹ️ 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".

@droplet-rl

Copy link
Copy Markdown
Contributor Author

Thanks @codex — re-review on 107337401c came back clean (P2 addressed). Nothing further to change here; this is ready for a maintainer's look.

@droplet-rl

Copy link
Copy Markdown
Contributor Author

🔎 View trace

@chatgpt-codex-connector

Copy link
Copy Markdown

Summary

  • No code changes were made. The trigger says the re-review on 107337401c came back clean and that nothing further is needed, so I did not create a new commit or PR.
  • No README/AGENTS updates are needed because there is no behavior, configuration, interface, or runtime-flow change in this follow-up. AGENTS.mdL13-L20

Testing / Checks

  • pwd && find .. -name AGENTS.md -print && git status --short
  • cat AGENTS.md && git diff -- yarn.lock | sed -n '1,120p'
  • nl -ba AGENTS.md | sed -n '1,24p'

Note: git status --short shows an existing modified yarn.lock; I did not alter or commit it.

View task →

@droplet-rl

Copy link
Copy Markdown
Contributor Author

Confirmed — no further action. The PR diff is limited to src/clients/TransactionClient.ts and test/TransactionClient.ts; the modified yarn.lock is a local working-tree artifact and is not part of this branch/commit. PR stands as-is, ready for a maintainer's review.

@droplet-rl

Copy link
Copy Markdown
Contributor Author

🔎 View trace

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant