Skip to content
GitLab
Projects Groups Topics Snippets
  • /
  • Help
    • Help
    • Support
    • Community forum
    • Submit feedback
    • Contribute to GitLab
  • Register
  • Sign in
  • A Ahad Mobile
  • Project information
    • Project information
    • Activity
    • Labels
    • Members
  • Repository
    • Repository
    • Files
    • Commits
    • Branches
    • Tags
    • Contributor statistics
    • Graph
    • Compare revisions
  • Issues 0
    • Issues 0
    • List
    • Boards
    • Service Desk
    • Milestones
  • Merge requests 0
    • Merge requests 0
  • CI/CD
    • CI/CD
    • Pipelines
    • Jobs
    • Schedules
  • Deployments
    • Deployments
    • Environments
    • Releases
  • Packages and registries
    • Packages and registries
    • Package Registry
    • Terraform modules
  • Monitor
    • Monitor
    • Incidents
  • Analytics
    • Analytics
    • Value stream
    • CI/CD
    • Repository
  • Wiki
    • Wiki
  • Snippets
    • Snippets
  • Activity
  • Graph
  • Create a new issue
  • Jobs
  • Commits
  • Issue Boards
Collapse sidebar
  • holatech
  • Ahad Mobile
  • Merge requests
  • !43

fix(withdraw): stop the withdrawal path losing and duplicating money

  • Review changes

  • Download
  • Patches
  • Plain diff
Merged Khanh Pham requested to merge fix/withdraw-hardening into develop Sep 04, 2026
  • Overview 0
  • Commits 7
  • Pipelines 3
  • Changes 28

Seven defects in the crypto withdrawal path, found tracing a 400 whose body read getaddrinfo EAI_AGAIN sandbox-api.fireblocks.io straight through to the user's screen, plus the four faults that surfaced while testing the fixes end to end on Sepolia.

Money

  • Refund credited one platform fee too much. The refund took amount + platformFee while the debit only ever took amount, so every failed withdrawal handed the user a free fee. Refund and debit now derive from one rounded net amount.

  • A second refund could be written for one withdrawal. Fireblocks redelivers out of order; a stale SUBMITTED landing after FAILED walked the row off its terminal state and re-armed the isFailed && wasNotFailed guard for the next FAILED. Observed crediting a user twice on a local run. A terminal status can no longer be walked backwards, and the refund now asks the ledger whether it has already paid — the same idempotency key hasDepositEntry uses, for the same reason: a status is not evidence that money moved.

  • The provider call ran inside the DB transaction, ahead of every write. A commit that failed after Fireblocks accepted the transfer left funds gone with no row and no debit. Submission is now three phases: reserve, debit and commit; submit with no transaction open; record the outcome. A definite rejection refunds. An ambiguous failure does not — the transfer may have landed, and refunding would be a double-spend.

  • Nothing resolved those ambiguous rows, so there is now a reconciliation job. It settles a parked withdrawal by re-submitting under the original idempotency key, the only way to learn whether the first attempt landed. It refuses to guess: a row whose key cannot be rebuilt, or that has outlived the provider's dedupe window, or that has failed too many times, is escalated to a human and never auto-refunded.

  • The balance was read unlocked and outside the transaction, so two concurrent withdrawals both passed the same check and the second debit overwrote the first. The row is now locked FOR UPDATE and validated inside the reservation.

  • The idempotency key was seeded with Date.now(), so it protected nothing on retry. An optional client requestId now drives a stable key, scoped per user along with the unique index and every lookup — unscoped, one account's id collides into another's withdrawal. A replay of a rejected or under-review reservation is refused rather than answered as success, and an id reused for a different payload is refused rather than silently substituted.

  • amount was validated as a non-empty string, so BigNumber('abc') produced NaN and every comparison against it was false: both the minimum and the balance guard were walked past. Validated now at the DTO and again in the service.

  • Rounding could bill more than was asked. toFixed(6) rounds half-up and the DTO admits eighteen decimal places, so a requested 1.4999999 against a 0.5 fee produced a 1.000000 transfer and a 1.500000 debit. Rounding down can only leave dust with the user.

What the user is told

  • Failures are classified by HTTP status, never by the sentence beside it. Infrastructure failures answer 503 with copy that does not claim the balance is untouched — it usually is not, because the debit happens before the transfer is attempted. Retry is offered only where pressing again can help, and carries the same requestId, persisted so it survives the app dying mid-request.

  • Only COMPLETED shows "Withdraw completed". A withdrawal answered SUBMITTED and showed a tick, then failed seconds later for want of gas in the sending vault.

  • The result screen now finds out what actually happened. It polled nothing before: it rendered a snapshot taken when the HTTP call returned and never read again, so a withdrawal that failed four seconds later sat on "in progress" indefinitely and the user was told neither that it failed nor that their money was back. It now polls GET /fireblocks/transactions/:id every five seconds for three minutes, stopping the moment the status is terminal. It will not treat an unrecognised status as success, will not render failure because the poll itself errored, and will not promise a refund it cannot see in the ledger entries.

  • That endpoint answered 500 to every call and had since it was written — one parameter compared against a uuid column and a varchar column, and Postgres types a parameter by its first use. Nothing had called it until now.

  • A background poll's failures no longer raise the global error modal. Requests can mark themselves backgroundPoll; per-request, not the existing global flag, which a poll on its own timer would use to swallow other requests' errors too.

Verification

  • Backend: 1381 tests pass, yarn build clean, eslint clean on every touched file. (yarn lint fails repo-wide on develop too — pre-existing debt in unrelated modules.)
  • Frontend: 53 suites / 390 tests pass, pnpm type-check and pnpm lint clean.
  • End to end on Sepolia: 3 withdrawals confirmed on chain (4.5 USDC net), 5 failed for want of gas and each refunded exactly once. Ledger balance reconciles to the penny against the destination wallet's on-chain balance.
  • Codex review: six rounds, five P1 and one P2, all addressed.

Migrations

Two, both additive with real down methods: nullable request_id, submission_error, submission_attempts, last_submission_attempt_at; fireblocks_transaction_id becomes nullable; a partial unique index on (user_id, request_id) and a partial index for the reconciliation query.

Not addressed

Same class of bug, different routes — flagged rather than fixed to keep this reviewable: POST /fireblocks/internal-transfer has the same unlocked-balance race; deposits write the balance cache without the lock; FileLogger kills the process when it cannot write its file; no screen in the app shows a withdrawal's status once the result screen is closed.

🤖 Generated with Claude Code

https://claude.ai/code/session_01NY8QddexbUnJUUDLSMdEqF

Assignee
Assign to
Reviewers
Request review from
Time tracking
Source branch: fix/withdraw-hardening