Skip to content

fix: validate withdraw destinations before review instead of after co… - #35

Open
kriss39 wants to merge 1 commit into
p-11:mainfrom
kriss39:fix/withdraw-destination-validation
Open

kriss39 wants to merge 1 commit into
p-11:mainfrom
kriss39:fix/withdraw-destination-validation

Conversation

@kriss39

@kriss39 kriss39 commented Sep 13, 2026

Copy link
Copy Markdown

…nfirm

The withdraw flow accepted any string as a Bitcoin destination (bip122 validation was a no-op), so a 0x or testnet address reached the review step. Worse, the review summary swallowed every fee-estimation failure into "Unable to estimate": libqc's estimateEmptyVaultFee throws InvalidDestinationAddressError for a bad address and NoWithdrawableAssetsError when the balance is below the dust limit after fees, and both were logged and discarded. The user was shown the asset list, an enabled confirm button and a fee that "could not be estimated", then hit a hard failure from emptyVault on the error screen - "no assets available to withdraw" right after being shown the assets.

  • Add a cheap Bitcoin mainnet shape check (legacy base58, bech32, bech32m) on the address step so obvious mistakes are caught inline; libqc stays the authoritative validator.
  • Map typed estimation failures to two new summary states, invalid-destination and insufficient-for-fee, which block confirm and explain why. Unknown estimation failures still degrade to a confirmable review with an unknown fee, as before.
  • Move destination validation into lib/withdraw-destination.ts so it can be unit tested without mounting the screen.

Why

  • Explain the reason for this pull request.
  • What problem is it solving?
  • Any relevant background or context?

How

  • Describe the changes made.
  • What approach was used?
  • Any key design decisions or implementation details?

Security / Environment Variables (if applicable)

  • Note any changes that affect security (new dependencies, communication with external services, data encryption, authentication).
  • Note any new environment variables that will need to be set.
  • Describe potential impacts or additional setup needed.

Testing

  • Describe how you tested your changes.
  • List any additional testing steps needed for review.

…nfirm

The withdraw flow accepted any string as a Bitcoin destination
(`bip122` validation was a no-op), so a `0x` or testnet address reached
the review step. Worse, the review summary swallowed every fee-estimation
failure into "Unable to estimate": libqc's `estimateEmptyVaultFee` throws
`InvalidDestinationAddressError` for a bad address and
`NoWithdrawableAssetsError` when the balance is below the dust limit
after fees, and both were logged and discarded. The user was shown the
asset list, an enabled confirm button and a fee that "could not be
estimated", then hit a hard failure from `emptyVault` on the error
screen - "no assets available to withdraw" right after being shown the
assets.

- Add a cheap Bitcoin mainnet shape check (legacy base58, bech32,
  bech32m) on the address step so obvious mistakes are caught inline;
  libqc stays the authoritative validator.
- Map typed estimation failures to two new summary states,
  `invalid-destination` and `insufficient-for-fee`, which block confirm
  and explain why. Unknown estimation failures still degrade to a
  confirmable review with an unknown fee, as before.
- Move destination validation into `lib/withdraw-destination.ts` so it
  can be unit tested without mounting the screen.

This branch has not been deployed

No deployments
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