fix: zksyncos withdrawals and finalization#296
fix: zksyncos withdrawals and finalization#296JackHamer09 wants to merge 3 commits intozksyncos-alphafrom
Conversation
JackHamer09
commented
Nov 13, 2025
JackHamer09
commented
- Fix withdrawals, withdrawal finalization status, and finalization
- Uses new SDK for all of these actions
- Curretntly finalization gas limit is hardcoded since finalization estimation methods are missing
|
Visit the preview URL for this PR (updated for commit 4c8e131):
(expires Thu, 20 Nov 2025 17:47:24 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 9fec89135cb37683b693c65a4a317d1b3c49cd82 |
JackHamer09
changed the title
dutterbutter
left a comment
dutterbutter
left a comment
There was a problem hiding this comment.
I suppose you could maintain createEthersClient / createEthersSdk similar to wallet store instead of initializing as frequent but more of an improvement suggestion.
| const quote = await sdk.withdrawals.quote({ | ||
| to: params!.to, | ||
| token: params!.tokenAddress, | ||
| amount: 1n, // TODO: estimation fails if we pass actual user balance |
There was a problem hiding this comment.
Why would you pass user balance here and not the provided amount the user wants to withdrawal?
There was a problem hiding this comment.
I think it worked on previous protocol version, we were basically estimating max possible fee by passing max possible amount. This would allow to not have to recalculate on amount value change (slightly better UX, less waiting). But not it fails because it needs value to be (balance - total fee).
| const transaction = await sdk.withdrawals.finalize(transactionInfo.value!.transactionHash as Hash); | ||
| if (!transaction.receipt) { | ||
| throw new Error("Finalization transaction failed"); | ||
| } |
There was a problem hiding this comment.
I suggest using the try-variant (tryFinalize) here but more a preference.
| completed: isFinalized, | ||
| withdrawalFinalizationAvailable: isFinalized, | ||
| completed: status.phase === "FINALIZED", | ||
| withdrawalFinalizationAvailable: ["FINALIZE_FAILED", "READY_TO_FINALIZE"].includes(status.phase), |
There was a problem hiding this comment.
Is including FINALIZE_FAILED intentional here?
There was a problem hiding this comment.
Do you know if its failed its possible to retry if it has failed before?
There was a problem hiding this comment.
Do you know if its failed its possible to retry if it has failed before
If finalizeDeposit tx failed you can try again, the status of it will remain READY_TO_FINALIZE.
| export const L2_BASE_TOKEN_ADDRESS = utils.ETH_ADDRESS_IN_CONTRACTS as Address; | ||
| export const L2_NATIVE_TOKEN_VAULT_ADDRESS = utils.L2_NATIVE_TOKEN_VAULT_ADDRESS as Address; | ||
| export const L2_ASSET_ROUTER_ADDRESS = utils.L2_ASSET_ROUTER_ADDRESS as Address; |
There was a problem hiding this comment.
You can get these address from the sdk as well fyi
| @@ -115,15 +117,15 @@ export default ( | |||
|
|
|||
| const [price, limit] = await Promise.all([ | |||
There was a problem hiding this comment.
Why do we need price? For zksync os chains we are 1559 complaint which does not use price? Is this just here for legacy?
There was a problem hiding this comment.
Yeah, legacy code. I really didn't have time to rewrite everything properly.
2 participants
