Skip to content

feat(deposit-address-service): execute v3 refund withdrawals - #3709

Open
amateima wants to merge 1 commit into
feat/deposit-address-service-v3-deposit-executionfrom
feat/deposit-address-service-v3-withdrawal
Open

feat(deposit-address-service): execute v3 refund withdrawals#3709
amateima wants to merge 1 commit into
feat/deposit-address-service-v3-deposit-executionfrom
feat/deposit-address-service-v3-withdrawal

Conversation

@amateima

Copy link
Copy Markdown
Contributor

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

The refund path for a mis_route or intent_refund, and for a
correct_transfer the execute endpoint rejected as below the minimum —
replacing the placeholder throws at the lines they sat on, so the
deposit lock is held across both actions via processUnderLock's
existing try/finally. The parent's exclusion routing is kept: anything
that is not a correct_transfer diverts to the withdraw, so intent_refund
takes the same second hop out to the committed refund address as a
mis_route.

Withdrawals are EVM-only, stricter than the deposit path, and need the
message's withdraw leaf materials; canonicality-then-balance and the
broadcast/reconciliation plumbing are reused unchanged, with
operation: "withdraw" on the pending record. A sign-withdraw 422 is
terminal on the HTTP status alone, recorded as withdraw_failed (code now
optional — the client discards the API's discriminator) and ACKed;
everything else NACKs. The refund deducts gas (deductGasFromRefund:
true), deliberately unlike v1. withdraw_executed is recorded but not yet
published; lifecycle publishing follows before execution is enabled.

Gated by ENABLE_V3_WITHDRAWALS, the same variable the polling bot reads,
defaulting off.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@amateima
amateima force-pushed the feat/deposit-address-service-v3-withdrawal branch from 427658f to fb4601d Compare August 27, 2026 17:19
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