feat(deposit-address-service): execute v3 deposits - #3701
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d2f96e949
ℹ️ 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".
72ee5bb to
83e248d
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 83e248d399
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
When a payload supplies a string such as "bogus", "1.5", or an empty string for erc20Transfer.amount, the schema accepts it and the later toBN(erc20Transfer.amount) throws an unrecognized INVALID_ARGUMENT. The app treats unrecognized errors as retriable and alerting, so this deterministic malformed delivery is retried for the full retention period instead of being ACKed as a validation failure; negative strings also pass conversion and reach the API. Refine this field to the unsigned integer formats accepted by the request contract.
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb104a643a
ℹ️ 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".
4aa4104 to
3490759
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3490759d12
ℹ️ 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".
3a91930 to
0b644ef
Compare
0b644ef to
1db6484
Compare
1db6484 to
3703a57
Compare
3703a57 to
a299d31
Compare
The merge-base changed after approval.
a299d31 to
4956330
Compare
Wires parseTransfer and TransferStore into a real handler: guards, the execute request, broadcast, reconciliation against the chain, terminal state, lock release. Two optional fields on AugmentedTransaction, both defaulting to today's behaviour so every existing caller is byte-identical: - onBroadcast fires once a hash exists and before the confirmation wait, and again on every hash change. sendAndConfirmTransaction is unusable here: it submits and confirms in one call and returns undefined with no hash on failure, so the earliest a caller sees a hash is after the wait broadcast_pending exists to survive. Rejections stay swallowed, since the transaction is already on the wire. - maxTries bounds that wait, which was hard-coded to 10 inside _submit. Worst case is M(M+1)/2 waits, so 10 is ~22 minutes on mainnet; the service uses 4. The outcome always comes from the chain. submit() catches _submit's throw and returns an empty array, so submitTransaction flattens revert, exhausted retries and RPC failure into one untyped Error. reconcile() therefore reads the receipt, and the same function serves a redelivery that finds a pending record. broadcast_pending gains from/nonce on EVM, which lets a transaction replaced at its nonce be recognised as permanently dead and re-attempted rather than stranding behind a no-TTL key. Omitted on TVM, where _runTransactionTvm returns nonce 0 unconditionally and the chain has no replacement semantics, so the check would clear a live record. Canonicality is a new guard, ordered before the balance check: the polling bot's balance check claims to cover reorgs but cannot tell a real funding transfer from money that happens to sit at a shared-pot address. An absent funding receipt NACKs rather than ACKs, since it cannot be told apart from our RPC lagging the indexer. Lock TTL 600s -> 900s, asserted at startup as lockTtl >= applicationDeadline + confirmBudget. assertBeforeDeadline bounds when a broadcast begins, not when the confirmation after it ends, so a broadcast at t=479s would otherwise outlive a 600s lock. Withdrawals are not ported: a mis_route, and an execute rejected as below the minimum, both NACK so nothing is discarded. Unreachable before EXECUTION_ENABLED is set. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`parseTransfer` returned `route: "deposit" | "withdraw"`, which was both a rename and a lie. A rename because `correct_transfer` => deposit and `mis_route` => withdraw folds in no rules the indexer had not already stated — it was a second vocabulary for one fact, with a type and a function to maintain. A lie because the deposit-vs-withdraw decision is not knowable at parse time: a `correct_transfer` the execute endpoint rejects as below the minimum becomes a refund withdraw, and that is only known once the API has answered. The design already accepts this — it is why there is no `refund_only` marker. `ParsedTransfer` now carries `transferId` and the message as the indexer stated it. The handler switches on `transferClassification` directly, which is also `processExecution`'s shape. `parseTransfer` keeps the one decision that is genuinely its own: `intent_refund` has no v3 path, so it is rejected before anything downstream needs a branch for it. The honest home for the word stays `BroadcastPendingState.operation`, set at broadcast time to what the transaction actually does. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`reconcileBroadcast` -> `resolvePendingTransaction`, and `reconcile.ts` -> `pendingTransaction.ts`. "Reconcile" is the vaguest verb available and was silent on the two things that matter here: that the chain is the source of truth, and that this is the only place a terminal state is written. The new name instead matches the state machine's own vocabulary end to end — `broadcast_pending` -> `BroadcastPendingState` -> `resolvePendingTransaction` — and at both call sites the reader is already holding a pending record, so it names what is in their hand. It also pairs with the existing inner helper, `resolveMissingReceipt`. `resolve*` is the house prefix; `finaliz*` was unusable, since two bot directories own it. The docstring now states the part no name can carry: it returns only when the transaction confirmed, and throws a typed error for every other outcome. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Removes `from`/`nonce` from `broadcast_pending`, the `replacementTarget` helper, `ReplacedBroadcastError` and the `resolveMissingReceipt` branch. A missing receipt now has exactly one answer: retain and NACK. Nonce management is `TransactionClient`'s job. Its confirmation wait already refuses to resubmit a consumed nonce, and re-notifies `onBroadcast` when it replaces a transaction, so the record follows the live hash while a worker is alive. The check only ever helped when a worker *died* mid-confirm during a nonce collision — a conjunction of two already-accepted residuals — and it paid for that with an extra RPC call, two schema fields, and a chain-family gate that clears a live record whenever it is wrong. That gate was wrong once already: TVM returns `nonce: 0` unconditionally, so every Tron record read as replaced until it was caught. The simplification is larger than the deletion. All five reasons a receipt can be missing — unmined, dropped, replaced, mined behind a lagging RPC node, reorged out — are safe to retain and unsafe to clear, so there was nothing to discriminate in the first place. A revert is the only outcome that clears anything, because a reverted transaction provably moved nothing. Residual, now stated rather than mitigated: a transfer whose worker died mid-confirm stays blocked until an operator clears its Redis key. -146/+70 across six files. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The hash was captured *after* `recordBroadcast` succeeded. Since `TransactionClient` swallows a failing `onBroadcast`, a Redis blip left `pending` undefined, the handler threw at "produced no transaction hash", and `resolvePendingTransaction` was never reached — so a **confirmed sweep went unrecorded**. A redelivery then found no state, and once an unrelated transfer refilled the shared pot the balance guard passed and the old calldata swept the new money. That is the 2026-07-20 incident, reproduced inside the service built to prevent it. Assign before the write, so the confirmed path always reaches `recordTerminal`, which supersedes `broadcast_pending` outright. After submission the write is retried once, where it is not swallowed, but best-effort: throwing there would skip the terminal write and reinstate the same bug whenever Redis stayed down. A `persisted` flag resets per hook entry, since a hash that persisted does not vouch for the replacement that followed it. Two regression tests, both verified to fail without the reorder: the pending write fails twice and the transaction still records `deposit_executed`; and the hook's write fails, the retry lands, and a redelivery has a record to resolve rather than re-executing. This also corrects the issue's Scope, which claimed a successful confirm would write over the gap. It could not — the hash never escaped the closure. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ed ones
`UnsupportedOriginChainError` covered two conditions with opposite
dispositions, and ACKed both.
A chain absent from `RELAYER_ORIGIN_CHAINS` is an operator switch. Flip it
back and the transfer is executable again — the funds sit on the deposit
address the whole time. ACKing destroyed the only Pub/Sub delivery that
could ever sweep them, and the polling bot merely skipped and revisited
the row on its next poll, so it was a parity regression too. Now
`OriginChainDisabledError`, retriable.
An unsupported chain *family* is a property of the code and still ACKs, as
`UnsupportedChainFamilyError`. The family check runs first: a chain that
is both unsupported and unconfigured must ACK, or it would retry every 60s
for the whole retention period over something no operator can fix.
Also rejects a non-numeric `chainId` / `destinationChainId` at the schema.
`Number("bogus")` is NaN, which reached `getProvider(NaN)` as an
unrecognised throw — so a deterministically malformed message both paged
and redelivered forever. Worse, NaN flowed into `transferId`, making the
keys `deposit-address:{lock,state}:NaN:<txHash>:<logIndex>` — two
malformed messages sharing a hash and log index would have collided on one
lock and one state record. Refined on the converted value rather than
pattern-matched, so a legitimate non-decimal encoding still passes.
159 tests.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…its provider `processUnderLock` built the provider first, so for an unsupported family or a chain with no RPC configuration `getProvider` threw a bare `No RPC providers defined` before `assertSupportedOriginChain` ran. The app cannot tell that from a transient fault, so it alerted and redelivered forever instead of the ACK the previous commit was meant to produce. Ordering, not logic — the dispositions were right but unreachable. Both chain guards now run before the provider is built. The reconciliation path deliberately does not gate on them: a transfer on a since-disabled chain still has a transaction on the wire, and leaving it unresolved is the unrecoverable direction. The existing tests could not have caught this, and that is the more useful finding: the injected fake `getProvider` answered for every chain, so it was more forgiving than production and hid the defect. It now throws for any chain but the configured one, and with that both routing tests fail if the guards move back after the provider — verified by reintroducing the old order. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`amount` was an unvalidated string reaching `toBN`, and it hid three
defects rather than one.
`toBN("bogus")` throws an untyped ethers error the app cannot distinguish
from a transient fault, so a deterministically malformed message paged and
then redelivered for the whole retention period. That is the reported one.
`toBN("-1")` does **not** throw — it yields -1, and `onchainBalance.lt(-1)`
is false, so a negative amount passed the balance guard outright. That
guard is the only check between the message and the execute call, so this
is the one that mattered.
`toBN("1.5")` silently truncates to 1, so the guard would have checked a
different number than the one forwarded to the API.
Pinned by shape rather than by attempting the conversion, because of that
last case: unlike `chainId`, the raw string is also sent to the API
verbatim, so the value the guard checks and the value we request have to
be the same one. Decimal and 0x-hex are both accepted — the two encodings
the execute endpoint documents — so no legitimate payload is dropped.
Deliberately not extended to `depositAddress`, `contractAddress` or
`transactionHash`. Those also produce untyped throws on garbage, but they
cannot bypass a guard or corrupt identity, and validating an address
needs cross-field chain context plus TronWeb in a module that is
currently pure. Recorded in the issue's Scope instead.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A second failure mode behind the swallowed onBroadcast hook, sharper than the absent-record one fixed in 83e248d. If the original hash persisted and the client then repriced or resubmitted, a failed write for the replacement leaves Redis naming a transaction that will never mine while `pending` names the live one. `recordTerminal` is then refused, because `canReplace` requires the terminal hash to match the pending record — so a confirmed sweep goes unrecorded and every later delivery resolves the dead hash instead. Permanently, even once Redis recovers. The post-submit retry now backs off across three attempts (~1.25s, bounded by the request deadline) instead of trying once. Deliberately small: it is sized for a blip, and a Redis unreachable for seconds is an outage where waiting longer buys nothing. Still best-effort — throwing would skip `recordTerminal`, which supersedes the pending record and is the stronger write of the two. `canReplace` is untouched. Loosening it would let a worker whose lock lapsed mark a transfer executed over a newer in-flight record, after which nobody resolves that transaction. This is therefore a probability reduction, not a closure, and the residual is recorded in the issue's Scope. The exhausted path now warns that the terminal write will be refused, and names both hashes, so the one outcome that ends in a permanent stall is greppable instead of silent. Tracked by a `recordedTxHash` rather than a boolean, so a stale record is distinguishable from an absent one — they fail differently and only the stale variant defeats `canReplace`. Two tests, both needing more than one retry attempt so the backoff is load-bearing: a replacement that lands on the third attempt records `deposit_executed`, and one that never lands warns and stalls on the original hash. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Master's v3 intent_refund handling (#3736) landed while this branch was open, and it changed the service's own message contract: parseTransfer now routes intent_refund to withdraw rather than dropping it. Resolving the rebase textually would have reinstated the drop and regressed a production fix — two transfers had already stranded funds on it. So master's behaviour wins and this branch's shape wins. The classification is passed through untranslated, and the handler diverts on `!== "correct_transfer"` rather than listing mis_route, so a future classification cannot silently fall through to the deposit path. assertActionableClassification is deleted rather than adjusted: with all three v3 classifications actionable it had no work left, and the struct's enums already rejects an unknown one as a validation failure.
4956330 to
1cac548
Compare
|
@droplet-rl Sanity check the changes applied since last approval - all OK? |
…er in test The below-minimum quote-api test constructed the error as (message, status, code). The constructor is (status, message, code, param), so the test passed only because the handler discriminates on code. tsconfig.json excludes test/, so tsc --build never saw it.
|
Yes — the delta since your approval is a rebase and nothing else. Interdiff is empty. Your approval was on Base drift is two commits: #3678 (squash-merged) and #3766. Every file this PR touches has an identical pre-image blob under the squash, so #3678 merged content-identical to what the branch was built on. #3766 is the only real move: it added the per-chain nonce-backlog threshold table around Re-verified on the new base: One thing I found and fixed (
One note, not a blocker: #3766 raised mainnet's backlog-replace threshold to 10 for exactly this shape of service, so a fresh submission now appends behind up to 9 in-flight nonces instead of replacing at 4. The |
Part of #3663 — PR 5 of the standalone deposit-address service. Stacked on #3701. Executes the v3 refund withdrawal: the path for a `mis_route`, and for a `correct_transfer` the execute endpoint rejected as `AMOUNT_BELOW_MINIMUM`. Replaces PR 4's two placeholder throws at the lines they sat on, so the deposit lock is held across both actions via `processUnderLock`'s existing `try/finally`. `WithdrawRouteNotImplementedError` disappears with them, taking the last identifier carrying the removed `route` vocabulary. **Review focus: one lock held across both actions; terminal 422 handling.** ## What is genuinely new - `executeWithdraw` in `depositHandler.ts`, ported guard-for-guard from `initiateWithdrawV3` per the issue's parity matrix: the `ENABLE_V3_WITHDRAWALS` gate (same env var the polling bot reads, NACK while off), **EVM-only** namespaces (stricter than the deposit path — `assertSupportedNamespace` allows `tron` on TVM chains, so it could not be reused), the withdraw leaf materials check, and a smaller response assertion (`assertValidWithdrawResponse`: signed `chainId` against the **refund** chain `erc20Transfer.chainId`, and the now+60s signature deadline — `assertValidExecuteResponse` deliberately does not carry over). - Terminal classification of a sign-withdraw failure **on the HTTP status alone**, exactly as production's `_getSignedWithdrawV3` does (`isHttpError(err) && err.status === 422`): persist `withdraw_failed`, ACK. Everything else NACKs. No client change; `_postOrThrow` discards the API's error code, so `withdraw_failed.code` becomes `optional(...)` and is unset — every existing PR 3 state test passes unchanged. - `deductGasFromRefund: true`, deliberately unlike v1's full-amount refund — not to be unified in the v1 PR. ## Reused unchanged The lock, both state reads, `assertSupportedOriginChain` (already runs on `erc20Transfer.chainId` before routing), canonicality-then-balance in that order and for the same reason, `onBroadcast` + `maxTries`, the pending-write retry, and `resolvePendingTransaction` (whose `operation: "withdraw"` mapping already existed). `broadcast()` is parameterised over `{operation, to, data, value, message, mrkdwn}` rather than duplicated. One adjustment there: a confirmed **withdraw** is not expected to carry the `MetadataEmitted` provenance event, so the missing-metadata warning is now gated to deposits. ## Not in this PR `withdraw_executed` is recorded but **not published** — lifecycle publishing and its recovery are PR 6, which lands before PR 7 enables execution anywhere. ## Verification - `yarn tsc --build --force` clean; `yarn lint` clean. - 190 passing across the service suites + `TransactionClient` (163 on the base branch; +27 here). - Every new guard and branch was red-checked by reintroducing the bug: the metadata gate, `deductGasFromRefund`, the signed-chainId check, the `operation` label, 422-only terminal classification, the code-gated below-minimum fallthrough, the lock held across both actions (the fake API records the lock token it observed inside each call), the withdraw gate, the leaf `kind` filter, EVM-only strictness, and canonicality-before-balance ordering — each failed exactly the expected tests, then passed on revert. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
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.tsisuntouched and
EXECUTION_ENABLEDdefaultsfalse, so nothing here can move funds until PR 7.Two optional fields on
AugmentedTransaction, both defaulting to today's behaviour so everyexisting caller is byte-identical:
onBroadcast— recordsbroadcast_pendingthe moment a hash exists, before the confirmationwait, and again on every hash change.
sendAndConfirmTransactioncan't do this: it submits andconfirms in one call and returns
undefinedwith no hash on failure.maxTries— bounds that wait. The hard-coded default of 10 isM(M+1)/2= 55 waits, ~22 min onmainnet, outliving both the deadline and the lock. This uses 4.
The outcome comes from the chain, not the exception.
submit()flattens revert, exhaustedretries and RPC failure into one untyped
Error, soresolvePendingTransactionreads the receiptinstead. 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— thedeadline check bounds when a broadcast begins, not when its confirmation ends.
Deliberately not here: the
MetadataEmittedpre-submission scan, nonce bookkeeping (that'sTransactionClient's), the send-adjacent lock check, and address/hash validation. Each is reasonedout 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_refundnow routes to withdraw instead of being dropped. Resolving textually would havereinstated 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".assertActionableClassificationis deleted: with all threeclassifications 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 andTransactionClientsuites.
DepositAddressService.deposit.tsdrives the real Express boundary, faking only the chain,quote-api and submission client — and its fake invokes
onBroadcastwhere the real client does.Several tests were verified to fail without their fix rather than merely pass with it.
🤖 Generated with Claude Code