matter-labs / zksync

zkSync: trustless scaling and privacy engine for Ethereum
https://zksync.io
Apache License 2.0
4.88k stars 2.68k forks source link

requires an ERC20 approve() (and gas) even when a previous one was sufficient #264

Open aspiers opened 3 years ago

aspiers commented 3 years ago

I'm trying to use zkSync to pay for some Gitcoin grant donations. I didn't have enough DAI in my zkSync wallet so I tried to deposit some via the zkSync popup from the https://gitcoin.co/grants/cart workflow. The first approve() transaction was taking too long so I asked Metamask to speed it up. It succeeded, but then the zkSync interface didn't notice.

I thought OK, instead of paying more gas fees for another redundant approve(), I should be able to reuse the previous one if I go directly to https://wallet.zksync.io/deposit. But apparently I cannot deposit any more without first clicking the Unlock toggle, and that requires performing another approve() transaction.

Given that ERC20 defines a read-only allowance(address owner, address spender) → uint256 call to determine how much allowance is remaining, it should be easy to use this to determine the maximum deposit which can be made without requiring a further approve(). In fact, from a quick glance at your codebase it seems that your wallet code already has an isERC20DepositsApproved() function defined, but the only code in the repository which uses that is unit tests. So maybe it's a simple case of wiring that up to the wallet frontend?

StanislavBreadless commented 3 years ago

Hi! Could you please provide the following information:

aspiers commented 3 years ago

I was using Brave with Metamask. Here are the two approvals:

StanislavBreadless commented 3 years ago

Hi! Sorry for the late response.

Did you approve the allowance manually?

The transactions give zkSync allowance for 100 DAI, which is not the default behavior by either checkout or zkSync wallet. By default, they give allowance of 2^256 - 1.

Unfortunately now, both the wallet and the checkout pages will check if your allowance is at least 2^255.

aspiers commented 3 years ago

I used MetaMask and when the popup for the approve() appeared, I went to advanced options and changed the allowance from unlimited to 100 DAI because I don't want to have to trust any third-party with all the DAI in my wallet. This should work fine. Why do the wallet and checkout pages check the allowance against 2^255 rather than against the required amount? It sounds like this is a sister bug of #265.

StanislavBreadless commented 3 years ago

We agree that this is an important feature. We are working on it. Stay tuned!

aspiers commented 3 years ago

Excellent, many thanks!

spartucus commented 3 years ago

Any updates on this?

StanislavBreadless commented 3 years ago

Hey @spartucus! Our team has shipped two contract upgrades: v4 which allowed much cheaper transactions and v5 (which will allow permissionless token listing).

Our team was very busy developing the new features, so we did not have enough time to implement the solution to the issue yet. But allowing ERC20 approve() of the exact size needed is on our roadmap and we will implement it later.

spartucus commented 3 years ago

Wonderful, glad to hear it!

mtahon commented 3 years ago

I am having the same issue, very frustrating. I'm not keen to give an unlimited approval, I always override manually the amount to something I am more comfortable with using MetaMask edit function.

image

aspiers commented 3 years ago

Urgh, here we are at another Gitcoin grants round and I've hit exactly the same problem. The ERC20 approve() for depositing DAI into L2 succeeded but the zkSync checkout process (including https://checkout.zksync.io/) failed to notice, so I'm now stuck mid-workflow with the choice of starting again, wasting more gas on a second, totally unnecessary approve(), or just going back to L1 and ditching zkSync. Unfortunately this UX is super harmful to the success of zkSync :-(

bxpana commented 2 years ago

@aspiers please let us know if this is still an issue you are running into.

aspiers commented 2 years ago

I don't have time to check now, but it will definitely still be an issue unless the code for the approve() process has been deliberately changed to address it. BTW I recently fixed the same issue in another project and it is not at all hard to do. It's harder to add support for https://eips.ethereum.org/EIPS/eip-2612 but that is well worth doing on L1.

bxpana commented 2 years ago

Ok I'll double-check on that and get back to you. Thank you

bxpana commented 1 year ago

@aspiers Wanted to see if you saw this error again with the last few Gitcoin Grant rounds we've had

aspiers commented 1 year ago

I haven't done anything which would have checked that since I last saw it. Have your devs have definitely implemented the feature yet? If so let me know and I'll try to find time to test. If not then they need to - unfortunately it is not the kind of issue which will just fix itself as a side-effect of other code changes.