Skip to content

fix(deposit-address): include logIndex in dedup keys so sibling transfers in one tx all sweep - #3748

Open
amateima wants to merge 4 commits into
masterfrom
fix/deposit-address-logindex-dedup
Open

fix(deposit-address): include logIndex in dedup keys so sibling transfers in one tx all sweep#3748
amateima wants to merge 4 commits into
masterfrom
fix/deposit-address-logindex-dedup

Conversation

@amateima

Copy link
Copy Markdown
Contributor

Summary

When a single transaction delivers multiple token transfers to the same deposit address, the bot swept only the first one. The rest were silently skipped — every retry hit the "already executed" guard — and once the indexer's 15-minute serving window closed, they became invisible to the bot permanently, requiring a manual sweep. This happened in production: one Ethereum tx carried two 10 USDC transfers to the same address, and the second breached the fill-latency SLA before being swept by hand.

The root cause is that the bot's duplicate-detection keys identified a transfer by transaction hash (plus deposit address), which cannot tell two transfers in the same transaction apart. This PR adds the transfer's logIndex — its position within the transaction — to those keys, so every transfer gets its own identity. Sibling transfers in one tx now each get swept (or refunded), on both the v1 and v3 paths, for deposits and withdraws alike.

Technical details

Two dedup keys were logIndex-blind, and the two transfers collided on both:

  1. getDepositKey() (src/utils/DepositAddressUtils.ts) was depositAddress:transactionHash. It guards the in-flight locks (observedExecutedDeposits, observedExecutedWithdraws) and the Redis-persisted executedWithdrawKeys / terminallySkippedWithdrawKeys / refundOnlyDepositKeys. Since forEachAsync processes a poll's messages concurrently, the losing sibling hit the winner's freshly-added in-flight key and returned.
  2. executedDepositTxHashes (DepositAddressHandler) keyed on the bare transactionHash, persisted to Redis. After the winner executed, every redelivery of the loser matched this guard until the indexer window closed and the row stopped being served.

Changes:

  • getDepositKey() is now depositAddress:transactionHash:logIndex. This single change fixes all five sets keyed on it. Erc20Transfer.logIndex is a required field on every message version (native transfers get a synthetic one), so no data-availability concerns.
  • executedDepositTxHashesexecutedDepositKeys: stores depositKeys instead of bare txHashes. The prune loop in evaluateDepositAddresses now reuses the depositKeysFromIndexer set, and the redundant bare-txHash set is deleted.
  • The Redis key name (deposit-address:executed:*) is unchanged: old-format entries cannot false-positive against the composite format and are pruned on the first poll.
  • Updated the stale cross-reference in deposit-address-service/message.ts (transferId() docstring described the old collision; that service already keys on chainId:txHash:logIndex and needs no change).
  • Tests: handler tests now build expected keys via getDepositKey instead of literals; added the incident-shape assertion that two messages differing only in logIndex produce distinct keys.

Accepted trade-offs:

  • One-time migration window: on first deploy, persisted old-format entries (all four Redis sets) miss the new-format lookups and are pruned on the first poll. Rows still inside the indexer's 15-min window fall through to the existing guards — the on-chain balance check for deposit/withdraw paths (funds already moved → balance below amount → skip), and API re-rejection for refund-only / terminally-skipped keys (one wasted API call). No transition shim.
  • Concurrent sibling sweeps: with distinct keys, sibling transfers execute concurrently within one poll. Balance checks compare per-transfer amount against total balance, so all pass — the desired outcome. Worst case on v1, two siblings race the deploy tx and one reverts; that path already releases its in-flight key and retries next poll.

Testing

  • test/DepositAddressUtils.ts: key-format assertions updated; new test that two transfers in one tx (differing only in logIndex) produce different keys.
  • Full deposit-address suite green (91 passing), tsc and eslint clean.

🤖 Generated with Claude Code

…fers in one tx all sweep

One transaction can carry multiple transfers to the same deposit address,
but both dedup guards collapsed them into one: getDepositKey was
depositAddress:txHash and the Redis-persisted executed set keyed on the
bare txHash, so only the first transfer was swept and the rest were
silently skipped until the indexer window closed.

getDepositKey is now depositAddress:txHash:logIndex, which fixes every
set keyed on it (in-flight deposit/withdraw locks, executed/skipped/
refund-only withdraw sets) on both v1 and v3 paths. The executed-deposits
set now stores depositKeys instead of bare txHashes, letting the prune
loop reuse the depositKey set built from indexer messages.

Old-format Redis entries self-prune on the first poll; rows still inside
the indexer window fall through to the existing on-chain balance checks.

Co-Authored-By: Claude Fable 5 <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: 8c422c2fd6

ℹ️ 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 thread src/deposit-address/DepositAddressHandler.ts
@amateima

Copy link
Copy Markdown
Contributor Author

@codex review

@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: e52d0c0046

ℹ️ 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 thread src/deposit-address/DepositAddressHandler.ts
…e depositKey

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@amateima

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Nice work!

Reviewed commit: 035fa35596

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

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