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

Add zkSync Checkout #7314

Merged

Conversation

mds1
Copy link
Contributor

@mds1 mds1 commented Sep 2, 2020

This is currently a draft PR, but the core functionality has been implemented so I think this is a good time for an initial review

Instructions for testing

  1. zkSync has test tokens for Rinkeby that anyone can mint. The full list of supported Rinkeby tokens can be found here, but only a subset have been added to Gitcoin. Mint yourself the below tokens:

    • USDT, 6 decimals, located at 0x3b00ef435fa4fcff5c209a37d1f3dcff37c705ad
    • LINK, 18 decimals, located at 0x4da8d0795830f75be471f072a034d42c369b5d0a
    • You'll see I added Rinkeby USDC to the economy model, but for some reason Gitcoin doesn't seem to recognize that, so just use the above two
  2. Then, add some grants to your cart

  3. Choose any mix of ETH, USDT, and LINK for your grants. For now, don't use token amounts below 0.5 to ensure the amount is packable. (We'll handle this more robustly later. For more info see here and here in the zkSync docs.)

  4. Click "Checkout with zkSync" and follow the steps

cc @danlipert @octavioamu @thelostone-mc @apbendi @owocki

@mds1
Copy link
Contributor Author

mds1 commented Sep 2, 2020

The checkout modal shown in these Figma mockups has now been implemented, so you no longer have to rely on console messages to see status updates

@mds1 mds1 changed the base branch from master to round-7-integration September 7, 2020 22:33
@codecov
Copy link

codecov bot commented Sep 7, 2020

Codecov Report

Merging #7314 into round-7-integration will decrease coverage by 0.08%.
The diff coverage is 11.25%.

Impacted file tree graph

@@                   Coverage Diff                   @@
##           round-7-integration    #7314      +/-   ##
=======================================================
- Coverage                26.47%   26.38%   -0.09%     
=======================================================
  Files                      303      306       +3     
  Lines                    29996    30211     +215     
  Branches                  4430     4461      +31     
=======================================================
+ Hits                      7942     7972      +30     
- Misses                   21779    21969     +190     
+ Partials                   275      270       -5     
Impacted Files Coverage Δ
app/grants/urls.py 100.00% <ø> (ø)
app/grants/models.py 43.09% <5.00%> (-2.44%) ⬇️
app/grants/views.py 15.46% <15.78%> (+0.09%) ⬆️
app/economy/tx.py 17.82% <50.00%> (ø)
app/retail/emails.py 20.95% <0.00%> (-0.12%) ⬇️
app/grants/clr.py 0.00% <0.00%> (ø)
app/quests/views.py 16.22% <0.00%> (ø)
...rketing/management/commands/no_applicants_email.py 0.00% <0.00%> (ø)
...eting/management/commands/assemble_leaderboards.py 39.73% <0.00%> (ø)
app/grants/management/commands/analytics_clr.py 0.00% <0.00%> (ø)
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ba34f93...7751bc7. Read the comment docs.

@mds1
Copy link
Contributor Author

mds1 commented Sep 8, 2020

Hey @thelostone-mc and @apbendi, this PR has most of the core functionality already, so I think it would be good to get a first round of reviews now. Current functionality that was added:

  1. Main zkSync checkout flow. See the description above for instructions on how to test this
  2. JSONStore is used to indicate whether a user was interrupted during zkSync checkout. Upon visiting the cart, we check this, and do not let them do another checkout until they finish the first checkout. Currently the proper modal will show up, but the "finish checkout" functionality is not yet finished
    • You can test this feature by confirming the tx in step 3 then immediately closing/refreshing the page. You should also be warned before closing the page if you've confirmed the tx in step 3
  3. Transaction validator was updated to not mark contributions as complete until the zkSync transfer was done on L2. Can test similarly to above

Adds endpoint to look for dropped and replaced transactions to help with this
- Update tx processing section of zkSync checkout modal
- Update grant detauls page to show Etherscan and zkScan links
- Add note about supported L2 account types on new grant page
@mds1
Copy link
Contributor Author

mds1 commented Sep 9, 2020

@thelostone-mc New changes just pushed:

  • zkSync modal can now only be closed by pressing the "X", to prevent accidental closures
  • zkSync checkout can be resumed when interrupted by visiting the carts page. This should support cases were the deposit tx was dropped and replaced
  • Add "Transaction Processing Info", "Grant Creation", and "Grant Details" updates of these Figma mockups

@mds1
Copy link
Contributor Author

mds1 commented Sep 9, 2020

Additional updates. This is probably a good final set of changes for this PR:

  • Batch deposit address was updated since that is now deployed on mainnet
  • If the user only is checking out with 1 token, they use the regular zkSync contract instead of our batch deposit contract. This saves them about 15k gas
  • Does not allow zkSync checkout with unsupported tokens

@mds1 mds1 marked this pull request as ready for review September 9, 2020 15:45
Copy link
Member

@thelostone-mc thelostone-mc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks gooood

// Generate deposit payload ------------------------------------------------------------------
console.log('Generating deposit payload...');
const deposits = []; // array of arrays, passed into batckZkSyncDepositContract.deposit(...)
let overrides = { gasLimit: '1000000' }; // TODO improve gas estimate
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any thoughts on how we'd improve this ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's two situations to consider:

  1. User is only checking out with one token, e.g. only DAI
  2. User is checking out with 2 or more tokens, e.g. DAI and ETH

For case 1, zkSync checkout only supports [ 'ETH', 'DAI', 'USDC', 'USDT', 'LINK', 'WBTC', 'PAN' ], so we can find mainnet deposits using those tokens (or just make some ourselves if we can't find any) and use those for the estimates.

For case 2, it gets a little tricker. Ganache fork of mainnet seemed to be returning wrong gas estimates when I tested earlier. If I can figure out why and how to make those more accurate, that is the easiest approach, and we can just test every combination and go from there. If that approach won't work, I need to think about this more and am open to ideas! 🙂

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for improving the gas limits pls. its very scary to see a 1mm gas estimate as an end user

@thelostone-mc thelostone-mc merged commit c50e4a0 into gitcoinco:round-7-integration Sep 9, 2020
thelostone-mc added a commit that referenced this pull request Sep 14, 2020
* Add zkSync Checkout (#7314)

* Integrate minimal working ETH-only zkSync checkout

* Refactor L1 checkout a bit to enable more code re-use with zkSync

* Adds full working checkout flow using localstorage, supports ERC20 + ETH

* Improve checkout UX by adding modal whehn using zkSync

* Warn user before closing or reloading if they started zkSync checkout

* Switch to deterministic from ephemeral wallet, fix allowance bug

Allowance bug info: If user already had sufficient allowance, tokens were not transferred

* Pull out logic into functions to facilitate re-sending on interrupted deposits

* zkSync checkout now POSTs to database and shows success message on completion

* Handle zkSync checkout interruptions

* Fix ESLint errors, add some try/catch blocks

* Fix bug in tx validator that prevented testing on rinkeby

Also some prep and cleanup for tx validator update

* Update tx validator to handle zkSync transfers

* Tweak: Show metamask popup on zkSync checkout prompts

* Only allow zkSync modal to be closed with a button to prevent accidental closures when checking out

* zkSync checkout can now be resumed if interrupted

Adds endpoint to look for dropped and replaced transactions to help with this

* Various UI/UX updates based on Figma mockups

- Update tx processing section of zkSync checkout modal
- Update grant detauls page to show Etherscan and zkScan links
- Add note about supported L2 account types on new grant page

* Update address of batch deposit contract

* Reduce gas by only using batch deposit contract when necessary, regular zkSync contract otherwise

* Now throws error if user tries to use zkSync checkout with unsupported token

* add list view for grants and migrate to vue (#7336)

* add list view for grants and migrate to vue

* hide idle grants by default

* Add contributions on grants

* add my grants

* Minor fix

* Implement follow in vuejs

* remove duplicated ids

Co-authored-by: Aditya Anand M C <aditya.anandmc@gmail.com>

* fixes #7223

* zkSync updates part 2 (#7348)

* Checks and validates cart amounts to ensure they are packable

* Add support for additional deposits. After donations are complete, all leftover balances are sent to user's web3 address

* Update appearance of zkSync modal to match new designs

* Add additional checkout flow to support users who already have a zkSync balance

* Update detail.js

Co-authored-by: Aditya Anand M C <aditya.anandmc@gmail.com>

* updated dates, just in case we got it wrong with db migration

* round up fixes

* fix for the anonymous user issue

* fix up clr info on cards

* show contribution button only if user has logged in

* update icon

* hide browse grants button

* rounding issue

* reload page on switch type/category

* fix grants without round

* fix favorite

* fix component initialization

* eslint fix

* fix grant activity

* sloppy fix

* a few fixes

* add pagination

* readd stuff

* switch btwn rinkeby+mainnet &show sybil info

* Fix my grants and add team members (#7354)

* Grants Round 7 Trust Bonus Improvements (#7353)

* Show SMS on trust bonus page w/ conditional verify button or status

* Add placeholder for twitter verification & handle SMS verification via cart link

* Conditionally show Trust Bonus banner in cart

* Include link in cart trust bonus button + bonus screen touchups

* Increase the size of the BrightID QR Code

* Fix linting indentation error

* fix for loading sidecart

* debounce search

* remove already loaded scripts

* move filter "my grants"

* zkSync updates part 3 (#7356)

* Update tx validator to support zkSync transfers when user already has funds on L2

* Add loading screen to zkSync modal

* Improve gas estimates, recommend checkout approach to user, limit zkSync to 100 contributions

* Change zkSync fees to be additive, show which tokens in cart don't support zkSync

* Fix so all zksync gas limit estimtes are strings

* Bug fixes to cart and tx validator

* Update cart copy and add zkSync details to GRANTS.md

* Update GRANTS.md

* adds grant funding info; per lefteris feedback

* adds grant funding info; per lefteris feedback

* fix for sms verification

* ads trust tab coming soon stuff

* stopgap for #7131 (comment)

* new grants header

* zkSync bug fixes (#7368)

* Fix zkScan URLs

* Ensure default network is mainnet

* Impove how tokens are fetched, close zksync modal on 'insufficient balance' errors

* Bug fix: prevents users from checking out when donation amounts + fees exceed their L1 wallet balance

* matic banner

* round 7 integration; adds brightid events to calenndar

* udpates img

* styles for banners and grid fixes

* Additional cart improvements (#7374)

* Add gas estimation for approval txs

* Fixes for Argent and BN, CSS tweak for mobile

* helper script to re-add team members that were removed by #7319

* updates grant scripts

* adds learn more link

* adds brightID info to sybilscore

* iexact

Co-authored-by: Matt <matt@mattsolomon.dev>
Co-authored-by: Miguel Angel Gordián <miguel@gordian.dev>
Co-authored-by: owocki <ksowocki@gmail.com>
Co-authored-by: octavioamu <octavioamuchastegui@gmail.com>
Co-authored-by: Ben DiFrancesco <ben@scopelift.co>
zoek1 pushed a commit to zoek1/web-1 that referenced this pull request Sep 14, 2020
* Integrate minimal working ETH-only zkSync checkout

* Refactor L1 checkout a bit to enable more code re-use with zkSync

* Adds full working checkout flow using localstorage, supports ERC20 + ETH

* Improve checkout UX by adding modal whehn using zkSync

* Warn user before closing or reloading if they started zkSync checkout

* Switch to deterministic from ephemeral wallet, fix allowance bug

Allowance bug info: If user already had sufficient allowance, tokens were not transferred

* Pull out logic into functions to facilitate re-sending on interrupted deposits

* zkSync checkout now POSTs to database and shows success message on completion

* Handle zkSync checkout interruptions

* Fix ESLint errors, add some try/catch blocks

* Fix bug in tx validator that prevented testing on rinkeby

Also some prep and cleanup for tx validator update

* Update tx validator to handle zkSync transfers

* Tweak: Show metamask popup on zkSync checkout prompts

* Only allow zkSync modal to be closed with a button to prevent accidental closures when checking out

* zkSync checkout can now be resumed if interrupted

Adds endpoint to look for dropped and replaced transactions to help with this

* Various UI/UX updates based on Figma mockups

- Update tx processing section of zkSync checkout modal
- Update grant detauls page to show Etherscan and zkScan links
- Add note about supported L2 account types on new grant page

* Update address of batch deposit contract

* Reduce gas by only using batch deposit contract when necessary, regular zkSync contract otherwise

* Now throws error if user tries to use zkSync checkout with unsupported token
zoek1 pushed a commit to zoek1/web-1 that referenced this pull request Sep 14, 2020
* Integrate minimal working ETH-only zkSync checkout

* Refactor L1 checkout a bit to enable more code re-use with zkSync

* Adds full working checkout flow using localstorage, supports ERC20 + ETH

* Improve checkout UX by adding modal whehn using zkSync

* Warn user before closing or reloading if they started zkSync checkout

* Switch to deterministic from ephemeral wallet, fix allowance bug

Allowance bug info: If user already had sufficient allowance, tokens were not transferred

* Pull out logic into functions to facilitate re-sending on interrupted deposits

* zkSync checkout now POSTs to database and shows success message on completion

* Handle zkSync checkout interruptions

* Fix ESLint errors, add some try/catch blocks

* Fix bug in tx validator that prevented testing on rinkeby

Also some prep and cleanup for tx validator update

* Update tx validator to handle zkSync transfers

* Tweak: Show metamask popup on zkSync checkout prompts

* Only allow zkSync modal to be closed with a button to prevent accidental closures when checking out

* zkSync checkout can now be resumed if interrupted

Adds endpoint to look for dropped and replaced transactions to help with this

* Various UI/UX updates based on Figma mockups

- Update tx processing section of zkSync checkout modal
- Update grant detauls page to show Etherscan and zkScan links
- Add note about supported L2 account types on new grant page

* Update address of batch deposit contract

* Reduce gas by only using batch deposit contract when necessary, regular zkSync contract otherwise

* Now throws error if user tries to use zkSync checkout with unsupported token
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.

4 participants