Repository navigation
Withdraw flow updates - #1041
Conversation
✅ Deploy Preview for acre-dapp-testnet ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for acre-dapp-v1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for acre-dapp ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
bb6cbe7 to
283e87b
Compare
Withdrawing to an Ethereum address needs a direct `redeem(shares, receiver, owner)` call, which the contract handle could not express: it only knew how to encode `approveAndCall`, the two-step idiom the Bitcoin path needs because `BitcoinRedeemer` is a third-party contract that must hold an allowance over the user's shares and be told to act in one Safe transaction. Redeeming against our own balance needs neither half of that. OrangeKit executes the call from the user's Safe, so `msg.sender` is the Safe, and the same Safe is passed as `owner`. OpenZeppelin's `_withdraw` only calls `_spendAllowance` when `caller != owner`, so that branch is never taken and no approval is required. Encoding only - nothing calls this yet.
`initializeWithdrawal` assumed the queued redemption design: it encoded `approveAndCall` to `BitcoinRedeemer`, then read a `redemptionRequestId` back out of the transaction receipt so the caller could track a request that settled later. Both halves are now wrong. Redemption is synchronous and complete once the transaction is mined, so there is no request to track, and there are two destinations rather than one. It is replaced by two explicitly named methods instead of one method with a destination flag, so neither path can be reached by accident and each returns only the identifier it actually has: - `initializeTbtcWithdrawal(btcAmount, receiverEvmAddress)` redeems straight to an Ethereum address and returns just the transaction hash. The receiver MUST be an address the user can move funds from - the account's own Safe holds no ETH and cannot relay the tBTC back out. - `initializeBitcoinWithdrawal(btcAmount)` routes shares through `BitcoinRedeemer` to the tBTC Bridge and returns the redemption key, which is the correct identifier when there is no request id and matches how the subgraph keys redemptions. Wallet selection on the Bitcoin path is now sized off `previewRedeem(shares)`, not the gross amount: `redeem` returns assets net of the exit fee, and only that net amount reaches the Bridge, so the gross figure could select a wallet that cannot cover the redemption. `Tbtc.buildRedemptionData` is replaced by `Tbtc.initiateRedemption`, which delegates the encoding to the tBTC SDK's `requestRedemptionWithProxy`. The hand-rolled encoder omitted the redeemer-output-script length prefix and broke real mainnet withdrawals; it also passed a zero wallet public key and an empty main UTXO, which were harmless only while the redeemer ignored them. The live wallet and its main UTXO are now resolved client-side, because the payload really does reach `Bridge.requestRedemption`. The three step callbacks move from `modules/account.ts` to a new `lib/utils/callbacks.ts` in the same change rather than a separate one: `lib/redeemer-proxy.ts` needs them, importing from `modules/` into `lib/` would create an import cycle, and `src/index.ts` star-exports both `./lib/utils` and `./modules/account`, so a commit where both declare these names does not compile.
`findRedemptionRequestIdFromTransaction` existed to recover a queued redemption's request id from the `RedemptionRequested` event in the transaction receipt. Synchronous redemption emits no such request and its only caller is gone, so the method, the `#runner` field it needed to reach the provider, and its tests all go with it. Also drops the `// TODO: set the new mainnet address` above the mainnet artifact, which no longer describes reality, and the stale mock entries on `MockAcreContracts` that only compiled because the `as BitcoinRedeemer` assertion suppresses excess-property checks. Documents what is left: this wrapper needs the deployed address and nothing more. The redemption never calls the redeemer directly - the Safe calls `acreBTC.approveAndCall` and the redeemer picks it up in `receiveApproval` - so the artifact is never used to encode calldata and an ABI change on the redeemer does not ripple into the SDK.
`getEstimatedDuration` quoted 72 hours for a `requested` withdrawal and 6 hours only once it was `pending`. That split existed because a withdrawal waited in the Midas queue for the next NAV update before redemption could start, and the longer estimate covered that wait. Funds now sit on the acreBTC contract, so a request goes straight into the tBTC redemption process - roughly 5 to 7 hours, averaged to 6. The `status` parameter has no remaining effect and is dropped, along with every call site that passed it. The user-facing copy described the same queue and is updated with it: the 14-day withdrawal tooltip, the success-modal wait time, and the pending-withdrawal banner, which also named the NAV update directly.
`BitcoinRedeemerV2` routes through the Midas `WithdrawalQueue`, which stops working once `MidasAllocator.emergencyWithdraw()` moves the shares out and no configuration revives it. `BitcoinRedeemerV3` redeems synchronously, so the Bitcoin path keeps working with the queue unset. Only the deployed address changes here. The SDK never encodes redeemer calldata - the user's Safe calls `acreBTC.approveAndCall(bitcoinRedeemer, shares, redemptionData)` and the redeemer picks it up in `receiveApproval` - so the ABI change between V2 and V3 does not ripple into the SDK. Depends on `withdraw-funds-from-midas-to-acrebtc`, which adds the contract along with its mainnet and sepolia deployment artifacts. Until that lands the `sdk` package does not typecheck.
283e87b to
6a58c1d
Compare
`BitcoinRedeemerV2` routes through the Midas `WithdrawalQueue`, which stops working once `MidasAllocator.emergencyWithdraw()` moves the Midas shares out, and no configuration revives it. `BitcoinRedeemerV3` is the original `BitcoinRedeemer` with `stBTC` swapped for `acreBTC`, so the redemption is synchronous again and needs no allocator shares. Deploy a fresh proxy instead of upgrading, so V2 stays in place as a rollback. `tbtcVault` is set in the initializer, so no follow-up `updateTbtcVault` call is needed.
kkosiorowska
left a comment
kkosiorowska
left a comment
There was a problem hiding this comment.
Quick question about release timing. The commits assume the funds already sit on acreBTC, and redeem pays out only from that balance, so until the Midas exit settles, withdrawals would just revert.
Am I right that we don't need to add anything in the code for this, and we just merge it once the tBTC from Midas has landed on acreBTC and the vault is unpaused? If so, maybe it's worth adding that to the PR description so nobody merges it too early.
| Hex.from(transactionHash), | ||
| ) | ||
| return { transactionHash, redemptionRequestId } | ||
| return { transactionHash } |
There was a problem hiding this comment.
Shouldn't we check the receipt status here? Before, findRedemptionRequestIdFromTransaction did it indirectly. Now we only return the hash, and neither OrangeKit nor the relayer sender checks receipt.status. If the tx reverts (e.g. the vault is paused or the main UTXO changes), the dApp still shows success.
Maybe we could add waitForTransaction + throw if status !== 1 in AcreRelayerTransactionSender? Then it covers both paths (here and redeemer-proxy.ts) in one place.
There was a problem hiding this comment.
IMO not a real issue:
- OrangeKit
sendTransactionreturns whatever hash theTransactionSendergives it. - AcreRelayerTransactionSender send
POSTrequest toacre-api. The relayer:- calls viem
wallet.sendTransaction, which estimates gas first, so a call that would revert (paused vault, stale UTXO) fails before it is broadcast; - then calls
waitForTransactionReceiptand throws "Transaction … reverted on-chain" if status !== "success".
- calls viem
- Both errors come back as HTTP 502.
AcreRelayerTransactionSenderthrows on!response.ok, so the dApp goes toonError, not success.
There was a problem hiding this comment.
Should the optimistic tBTC withdrawal still be added as "pending"? Since it now settles in a single tx (the subgraph marks it Requested + Finalized at once, so it comes back as completed), it looks like the dashboard would briefly show the "Withdrawal in Progress" banner with "~6 hours" until the next refetch. The banner and EstimatedDuration in the history don't get the destination, so they fall back to the Bitcoin estimate. I haven't verified this in the app though.
| throw new Error("Receiver cannot be the zero address") | ||
|
|
||
| const data = this.instance.interface.encodeFunctionData("requestRedeem", [ | ||
| const data = this.instance.interface.encodeFunctionData("redeem", [ |
There was a problem hiding this comment.
Claude flagged this one for me during the review, so I wanted to ask if it's actually a thing.
Withdrawals go through our relayer, and as far as I understand it only sends transactions that are on its allowlist. This PR changes what the withdrawals call:
- tBTC withdrawal:
acreBTC.redeeminstead ofacreBTC.requestRedeem - BTC withdrawal:
acreBTC.approveAndCallnow approvesBitcoinRedeemerV3instead ofBitcoinRedeemerV2
Does the relayer allowlist only check that the call goes to acreBTC, or does it also check the function name and the approved contract? If it's the second, the relayer would reject both withdrawals after release, so we'd need to update the allowlist first.
There was a problem hiding this comment.
Relayer only checks which contract calls, not the function.
The relayer returns the hash only once the transaction is mined successfully, and a tBTC redemption settles in that transaction. The optimistic `pending` activity showed the Bitcoin "~6 hours" banner until the next refetch.
Everything is ready, funds are in |
kkosiorowska
left a comment
kkosiorowska
left a comment
There was a problem hiding this comment.
LGTM 🚀 I only have read access to the repo, so I can't resolve the threads or merge myself. @r-czajkowski, could you resolve them and merge when ready?
3 participants
Replaces the queued withdrawal with two synchronous redemption paths and enables withdrawals.
NOTE: The new contract is required. We add the new contract in a follow-up PR.