feat: add native wsteth support#246
Conversation
|
Visit the preview URL for this PR (updated for commit 7ec2240): https://staging-zksync-dapp-wallet-v2--pr246-tx-nikola-txfusio-3ifj5qui.web.app (expires Tue, 15 Jul 2025 18:26:59 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: a25831e6058958ccabf0f806505b5b8e7241b178 |
JackHamer09
left a comment
There was a problem hiding this comment.
tldr: code is just ai generated and the quality is really poor
| const eraWalletStore = useZkSyncWalletStore(); | ||
| const { eraNetwork } = useZkSyncProviderStore(); | ||
| const { captureException } = useSentryLogger(); | ||
| const chain = eraNetwork.key === "mainnet" ? mainnet : sepolia; |
There was a problem hiding this comment.
this doesn't make a lot of sense to me.. Why not use l1 network config of selected network?
| fee: DepositFeeValues | ||
| ) => { | ||
| // Create signer | ||
| const { ethereum } = window as any; |
There was a problem hiding this comment.
This would only support injected wallets... You should use connected wallet instead of creating new one
| const browserProvider = new BrowserProvider(ethereum); | ||
| const isMainnet = eraNetwork.key === "mainnet" || eraNetwork.id === 324; | ||
| const networkType = isMainnet ? types.Network.Mainnet : types.Network.Sepolia; |
There was a problem hiding this comment.
this looks like AI generated code which doesn't make too much sense
| const allowance = await signer.getAllowanceL1(transaction.tokenAddress, transaction.bridgeAddress); | ||
|
|
||
| if (allowance < BigInt(transaction.amount)) { | ||
| const approveTx = await signer.approveERC20(transaction.tokenAddress, transaction.amount, { |
There was a problem hiding this comment.
Allowance should be done properly and handled on the UI, there is already logic for it
|
I didn't use AI for any of the mentioned parts - those were basically examples for how we did it previously, but I agree with the comments and I agree that it could be polished! Updated based on the comments:
|
There was a problem hiding this comment.
-
Please check this PR with some improvements #253
-
See the comments, left some feedback about general logic of token addresses
-
I'm getting a bunch of errors in
views/transactions/Deposit.vue. MostlyProperty 'l2Address' does not exist on type 'Token'.. I tried rebuilding everything and installing latest dependencies.
I left the allowance part - the reason I added it for the handleCustomBridgeDeposit function is because before, when calling wallet.deposit, we pass the approveBaseERC20 as true
It was a hacky way of fixing deposit for those chains that use custom erc20 as base token I think. Normally, and in this case as well, you should do it correctly through the UI, so that user understands what's going on and which transaction they are signing. Or is this already handled?
I used it because in some cases in other projects, I found it that some functions that expect object params, expect either an object with a parameter or with no parameter, and even if we pass "undefined" as a param, the function will take it as a param that exists, and potentially throw an error - so I just adopted the style that if I pass the object with potentially undefined params, I don't pass the params that would be undefined. In these cases it doesn't make a difference, so I agree with you to keep it simplified.
|
|
🎉 This PR is included in version 1.44.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Tested withdraw, deposit, and transfer for the native wstETH token, as well as the bridged (erc20) one, also tested ETH token and others to not mess up the previous flow.
The custom gas price fetching and custom withdraw tx creation had to be added since newer version of the SDK works in a different way for bridge addresses and it didn't work with native wstETH address
The balances/tokens map had to be updated - previously it used L1 address as the key, but I had to change it so that it uses the L2 address as the key - since we want to keep backward compatibility with the previous bridged (non-native non-Lido) wstETH token, so we want to support both, but they have the same L1 address, so we couldn't use the L1 address as the key
This PR fixes #238