Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More zkSync fixes #7428

Merged
merged 6 commits into from
Sep 16, 2020
Merged

More zkSync fixes #7428

merged 6 commits into from
Sep 16, 2020

Conversation

mds1
Copy link
Contributor

@mds1 mds1 commented Sep 16, 2020

Current changes

1. POST Request Improvements

Fixes item 2 of #7421, which says:

POST request to save data is not sent until after all transactions, so if the donation tx fails this does not get logged and we don't have the contribution data for matching. Move this POST request to be sent after L1 tx (if applicable) and before the L2 txs are sent

For Flow A (pure zkSync) the POST request is now sent after the initial transfer from the nominal zkSync wallet to the Gitcoin zkSync wallet. This means if transfers fail we'll still have all the contribution data in a JSON Store and saved as contributions/subscriptions

For Flow B (L1 deposit) nothing is changed. The POST request is still sent after the deposit tx is sent, but before it is mined (and therefore before any L2 txs occur)

(Need to do more testing and updates of the tx validator locally, but this should be merged and deployed since there are a lot of other changes)

2. More conservative fees/amounts

Fixes item 1 of #7421, which says:

On some checkouts, the final zkSync transfer (the donation to Gitcoin) fails. This happens because insufficient balance remains

I'm not able to reproduce these locally, so for now we've worked around this by increasing the fee margin. This is analogous to raising the gas limit to ensure L1 txs don't fail

3. Ensure "new account" fee is used

Transfers to new accounts on zkSync are more costly than transfers to existing accounts. Previously we fetched the fee by just pretending we were sending to 0x0000000000000000000000000000000000000001, since no one has sent to this address.

However, this code would "break" and give lower fee estimates if someone ever does send to this address. To remedy this, we now check the fee by generate a random address each time and pretending we're sending to that address. The address space is large enough that we are effectively guaranteed to always have a never-been-used address there

4. Fix for zero DAI donations to Gitcoin

Fixes item 5 of #7421, which says:

When you set the donation amount to zero, it naively tries to send a 0 DAI donation to Gitcoin, and the zkSync fee estimator seems to fail when you specify a zero value transfer. Source: https://twitter.com/YahsinHuang/status/1306238719104413697?s=20

5. Fix for batch zkSync checkouts using ETH

Fixes items 3 and 6 of #7421, which are the same bug:

Returned values aren't valid. Did it run out of gas?

The error was that in this situation we tried to check the ETH balance with balanceOf, which caused the error.

6. Fix for final zkSync transfer

Fixes item 4 of #7421, which says:

When there is too little balance in the zkSync wallet to complete the final transfer back to the nominal wallet, the overflow bug happens so it's marked as incomplete. See image below.

7. Fix extra deposits not being included

This feature was broken in a previous PR, and now is resolved

@mds1 mds1 marked this pull request as draft September 16, 2020 13:37
@mds1 mds1 marked this pull request as ready for review September 16, 2020 17:26
@octavioamu octavioamu merged commit 83e8804 into gitcoinco:stable Sep 16, 2020
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.

3 participants