feat(deposit-address-service): per-transfer lock and durable state - #3678
Merged
Conversation
amateima
requested review from
bmzig,
dijanin-brat,
mrice32,
nicholaspai and
pxrl
as code owners
August 6, 2026 19:10
9 tasks
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 094f8b0c3c
ℹ️ 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".
amateima
force-pushed
the
feat/deposit-address-service-lock-state
branch
from
August 10, 2026 09:15
094f8b0 to
d06c6eb
Compare
Base automatically changed from
feat/deposit-address-service-message-contract
to
master
August 10, 2026 11:20
amateima
force-pushed
the
feat/deposit-address-service-lock-state
branch
from
August 10, 2026 11:20
d06c6eb to
db7f9fa
Compare
amateima
force-pushed
the
feat/deposit-address-service-lock-state
branch
3 times, most recently
from
August 11, 2026 22:09
b39a154 to
5066eea
Compare
amateima
force-pushed
the
feat/deposit-address-service-lock-state
branch
from
August 24, 2026 11:16
5066eea to
f57cb94
Compare
pxrl
approved these changes
Aug 24, 2026
amateima
force-pushed
the
feat/deposit-address-service-lock-state
branch
from
August 27, 2026 17:19
f57cb94 to
6c45599
Compare
amateima
force-pushed
the
feat/deposit-address-service-lock-state
branch
from
September 3, 2026 22:19
6c45599 to
2d3af9d
Compare
pxrl
approved these changes
Sep 4, 2026
The two Redis keys the design turns on, plus the receipt classifier. Not wired: there is nothing to lock or record until the execution paths exist. The keys are separate because their lifetimes are opposites. The lock must expire, so a consumer that dies does not block a transfer forever; the state must not, because a broadcast_pending record has to outlive anything that could still land on-chain. Merging them would reintroduce the problem the design exists to remove. Both are built on acquireLock/releaseLock/get/set/del, already on RedisCacheInterface — no new primitive, which is what #3648 was criticised for. Guards worth naming: - clearRevertedBroadcast deletes only if the record is still the transaction it was told about. A stale reconciliation of h1 must not delete a newer pending h2 or a terminal outcome, which would unblock a transfer mid-sweep. Read-then- check suffices because callers hold the lock; it would need to be atomic only if that stopped being true. - recordBroadcast refuses to overwrite a terminal outcome — the one transition that loses funds-safety information. - Every write throws unless Redis replies OK. Awaiting a confirmation while believing a broadcast hash is durable is the unrecoverable direction. - A present-but-unparseable record throws rather than reading as absent: it may describe a transfer already swept, so the transfer stays blocked. - The lock token is a uuid generated inside the store and never crosses the boundary, so no caller can supply or reuse one. classifyReceipt returns facts, not actions. A receipt with no explicit status of 0 reads as confirmed: ethers leaves status undefined on some chains, and guessing confirmed can strand a deposit for manual recovery whereas guessing reverted would re-broadcast one that already landed. Only the first is reversible. Skipped a schemaVersion field: superstruct type() already tolerates unknown keys, so add one when a second version exists. Skipped the full transition table — the other rows need a caller to bypass the state read every path begins with. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…s failures Two review findings on the store. recordTerminal was unconditional, so a worker whose lock had lapsed could write its own transaction's outcome over a newer pending record, and withdraw_failed could erase a broadcast that may still land. canReplace now allows a terminal write only over an absent record, a pending record for that same transaction, or an identical outcome. The expected hash is read from the terminal record itself rather than passed in, so a mismatched one cannot be supplied. The premise is reachable, contrary to first appearances: the 480s application deadline is enforced by application code, and nothing threads it into TransactionClient, which accepts no AbortSignal. Its confirmation loop is `++nTries < maxTries` at 10 tries with a 24s mainnet timeout, and the TIMEOUT branch resubmits by recursing with maxTries - 1 — so a worker can be occupied far longer than the 600s lock TTL. Redis rejections now become TransientDependencyError at the store boundary. Previously the raw client error escaped, and since an unrecognised throw is treated as alerting, an ordinary Redis outage would page on every delivery rather than take the debug-level retry path that error already documents for exactly this case. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
amateima
force-pushed
the
feat/deposit-address-service-lock-state
branch
from
September 7, 2026 13:21
2d3af9d to
60301f0
Compare
amateima
added a commit
that referenced
this pull request
Sep 7, 2026
Part of #3663. **Stacked on #3678** — review that first; this diff is against its branch. The first PR here that moves funds: guards, the execute request, broadcast, resolving the outcome against the chain, terminal state, lock release. *Review focus: guard-for-guard parity with `initiateDepositV3`.* `DepositAddressHandler.ts` is untouched and `EXECUTION_ENABLED` defaults `false`, so nothing here can move funds until PR 7. **Two optional fields on `AugmentedTransaction`**, both defaulting to today's behaviour so every existing caller is byte-identical: - `onBroadcast` — records `broadcast_pending` the moment a hash exists, *before* the confirmation wait, and again on every hash change. `sendAndConfirmTransaction` can't do this: it submits and confirms in one call and returns `undefined` with no hash on failure. - `maxTries` — bounds that wait. The hard-coded default of 10 is `M(M+1)/2` = 55 waits, ~22 min on mainnet, outliving both the deadline and the lock. This uses 4. **The outcome comes from the chain, not the exception.** `submit()` flattens revert, exhausted retries and RPC failure into one untyped `Error`, so `resolvePendingTransaction` reads the receipt instead. A revert is the only case that clears the record; every "no receipt" reason is retained. **Lock TTL 600s → 900s**, asserted at startup as `lockTtl >= deadline + confirmBudget` — the deadline check bounds when a broadcast *begins*, not when its confirmation ends. **Deliberately not here:** the `MetadataEmitted` pre-submission scan, nonce bookkeeping (that's `TransactionClient`'s), the send-adjacent lock check, and address/hash validation. Each is reasoned out in #3663's Scope, with the residuals stated. ## Rebased onto #3678, and one behavioural change Master's #3736 landed while this was open and changed the service's own message contract: `intent_refund` now routes to withdraw instead of being dropped. Resolving textually would have reinstated the drop and regressed a production fix, so master's behaviour wins and this branch's shape wins — the classification passes through untranslated and the handler diverts on `!== "correct_transfer"`. `assertActionableClassification` is deleted: with all three classifications actionable it had no work left. Note #3651 also landed, which appends behind in-flight nonces instead of colliding on `"latest"`. That materially narrows the nonce race described in #3663's Scope 5 — I'll correct that entry. ## Verification `yarn tsc --build --force`, `yarn lint`, **164 passing** across the service and `TransactionClient` suites. `DepositAddressService.deposit.ts` drives the real Express boundary, faking only the chain, quote-api and submission client — and its fake invokes `onBroadcast` where the real client does. Several tests were verified to fail without their fix rather than merely pass with it. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: droplet-rl <284132418+droplet-rl@users.noreply.github.com>
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.
Part of #3663 — PR 3 of nine. Stacked on #3670, which is stacked on #3668. Bases retarget automatically as each merges.
What
The two Redis keys the design turns on, plus the receipt classifier.
src/deposit-address-service/transferState.ts.They are separate because their lifetimes are opposites: the lock must expire so a dead consumer does not block a transfer forever; the state must not, because a
broadcast_pendingrecord has to outlive anything that could still land on-chain. Merging them reintroduces the problem the design exists to remove.Built entirely on
acquireLock/releaseLock/get/set/del, all already onRedisCacheInterface— no new Redis primitive, which is what #3648 was criticised for.Guards worth reviewing
clearRevertedBroadcast(id, expectedTxHash)deletes only if the record is still that transaction. A stale reconciliation ofh1must not delete a newer pendingh2or a terminal outcome — that would unblock a transfer mid-sweep, which is the exact failure this project exists to prevent. Read-then-check is sufficient because callers hold the lock; it would need to be atomic only if that stopped being true.recordBroadcastrefuses to overwrite a terminal outcome — the one transition that loses funds-safety information."OK".setcan answerundefined, and awaiting a confirmation while believing a broadcast hash is durable is the unrecoverable direction.CorruptTransferStateErroris retriable but distinct from a Redis outage — it clears only when an operator repairs or deletes the key.acquireLock(id)generates a uuid and returns aTransferLockhandle, so no caller can pass amessageIdor reuse a token. A message id could not serve anyway: Pub/Sub redelivers the same message.classifyReceiptreturns facts, not actions"confirmed" | "reverted" | "unresolved". Whether each means ACK, NACK, clear or persist is retry policy and belongs with the orchestration that also holds the lock — so this file has no opinion on it.A receipt with no explicit
status === 0reads as confirmed. ethers leavesstatusundefined on some chains; guessing confirmed can strand a deposit for manual recovery, whereas guessing reverted would re-broadcast one that already landed. Only the first is reversible.Deliberately skipped
schemaVersion. Superstructtype()already tolerates unknown keys, so today's records stay readable. Add one when there is a second version — a version number does not help with the case it would not cover anyway (a field whose meaning changes).Testing
RELAYER_TEST=true yarn hardhat test "test/DepositAddressService*.ts"→ 79 passing (20 new).Covers all four
classifyReceiptbranches,SET NXexclusivity, token-checked release, internally-generated and differing tokens, the lock TTL, round-trip of every status with its TTL, idempotent terminal rewrite, refusal to downgrade a terminal, unacknowledged writes, corrupt and out-of-range records, and that a stale clear cannot delete either a newer pending hash or a terminal outcome.yarn buildreports only the three pre-existingpoolRebalanceLeafCounterrors.yarn lintclean.Note this touches
errors.ts(three new classes) and the module README, both introduced in #3668.🤖 Generated with Claude Code