Web: refactor cart.js

Created on 23 Oct 2020  Â·  9Comments  Â·  Source: gitcoinco/web

Description

Evaluate and add more refactor if needed into cart.js

I believe @octavioamu might be able to shed some light on the fixes which could be done

Most helpful comment

We can definitely do that! I'll use a new key for the JSON stores here so they are easily distinguished from existing JSON stores, and will be sure to remove the entry once the contribution is created.

All 9 comments

@mds1 is already on it. He is creating a component for the zksync .

As part of the zkSync refactor, instead of just cleaning up the code and moving it into it's own component, I think we should additionally update our architecture to use zkSync's new batch transfer feature. Using this instead of our "Gitcoin zkSync account" has a lot of benefits:

  • Out code/architecture will be _significantly_ simplified and consequently less buggy with less edge cases to handle
  • Improved security: Users no longer need to trust Gitcoin and they now only login to zkSync
  • Improved UX

    • If the user has funds in zkSync, it's one less signature

    • Batch transfers are now atomic, so we remove the (currently unhandled) scenario where one transaction in the sequence fails.

    • More clear distinction between when a failure is from the Gitcoin side vs. zkSync side

Because of how the current code is bit of a mess resulting from fixing various edge cases and bugs that arose from the current "Gitcoin zkSync account" approach, I don't think migrating to this new architecture will significantly impact development time, but still would like to get everyone's thoughts here.

@apbendi Please add any details if I missed something!

cc @owocki

This is mostly done, just need to add a few UX things and delete some unneeded code in cart.js.

Question: At what point do we want to save Contribution info to database? Here's my thinking of a flow, which would also resolve #7744. Let me know your thoughts:

  1. Immediately before checkout starts, save off a JSON store blob with all the cart data
  2. Saving contributions based on checkout method:

    1. If the user already has funds in zkSync, we wait until after checkout is complete before saving contributions in database. Checkout is fast, and this approach is very simple as we can POST a contribution with its corresponding zkSync tx hash, and not need to worry about matching those up later. (Finding then matching up hashes is what we did last round and it's a little flaky, so I'd prefer not to do this). Transaction validator would be extremely simple and reliable, because we already know all transactions were successful. Emails would be sent after zkSync checkout is complete.

    2. If the user does not have funds in zkSync, it's still easiest to wait until everything is complete for the same reasons as above. This results in the same process/info as above

    3. If the user is doing an L1 checkout, we can save the contributions immediately before receiving a transaction hash, then add the transaction hash to the contributions once we have it. Emails are sent once we receive the transaction hash like we currently do

  3. Clear the JSON blob to indicate the checkout was completed

Questions on this @mds1— for options 2.i and, especially, 2.ii, what happens if the user closes their browser before the contributions complete?

For option 2.iii, how do we currently handle transaction failures? I believe we account for them later, but I forget how. Nothing we're changing here would impact that process, would it?

for options 2.i and, especially, 2.ii, what happens if the user closes their browser before the contributions complete?

  1. There's an L1 deposit, and the user closes the whole browser before that completes: Their cart will still be populated, they'll have funds in zkSync, and they can just re-visit the cart and checkout again, but of course skip the L1 deposit this time. The standard "don't forget to checkout your cart" emails work here.

  2. The user closes the window after the L2 checkout completes but before Gitcoin registers it: The zSync library polls every 500ms for the transaction hashes, so they'd have to close the Gitcoin tab (or the whole window) pretty quickly after checkout is complete. In this case we don't have the user's transaction hashes, but we can try to guess which ones they are by looking through all their zkSync transactions. I think this will be pretty rare, and I'd like to avoid matching up hashes like this if possible.

  3. The user only closes the Gitcoin tab, but not the zkSync tab, before the transfers complete: The zkSync page will throw an error and prevent completing checkout

For option 2.iii, how do we currently handle transaction failures? I believe we account for them later, but I forget how. Nothing we're changing here would impact that process, would it?

The backend transaction validator parses the transaction receipt and marks the transaction as failed in the database

Let me know your thoughts on all this!

Immediately before checkout starts, save off a JSON store blob with all the cart data

  • So when you check out starts , it means when user clicks on L1 / zksync checkout button -> we fire API which would create subscription + contribution object (with state as pending)
  • Once process is complete -> another API call would fire with the txn hash to update those contributions
  • Like @apbendi asked -> if you close the browsers before second API call frires ,the subscription / contribution objects would remain and would be in pending / failed state ? (how would it switch over to failed state)

@danlipert + @owocki

So when you check out starts , it means when user clicks on L1 / zksync checkout button -> we fire API which would create subscription + contribution object (with state as pending)

We could do it that way also. The reason I suggested the JSON store way is partly because right now for L1 checkouts we dump the data in the JSON store as backup, then create the contribution/subscription objects _after_ we get the transaction hash.

So I was thinking we do a similar process for L2 checkouts also. But with L2, as soon as we get the transaction hash we know the contributions were successful, which is nice.

The other reason I suggested the JSON store way is because there's two downsides I see to the "create contributions early" approach:

  1. It's more work to implement, since it's less similar to the existing approach
  2. This approach actually becomes more problematic if the user doesn't complete a checkout. Because now we have to also keep track of their "in progress" contributions, and match them up when they re-visit cart to finish checkout. With the JSON store approach of lazily creating contributions as late as possible, we don't have that issue

Is there a benefit to creating contributions that don't have transaction hashes, then updating them once we do have hashes? There might be something I'm missing!

Like @apbendi asked -> if you close the browsers before second API call frires ,the subscription / contribution objects would remain and would be in pending / failed state ? (how would it switch over to failed state)

Exactly, and that is basically downside 2 above. I don't think there's an easy answer, which is why I prefer creating the contributions/subscriptions as late as possible (only once we have a transaction hash). Because when the contributions/subscriptions don't have transaction hashes, it gets complex if, say, the user modifies their cart before finishing checkout. Now we have add logic to remove or edit certain contributions. Even if they don't edit their cart, we have to implement logic that looks at an item in the cart and matches it an incomplete contribution

Hmm it does make sense going the JSON store way even though it's ugly data !
Could we ensure we remove the entry from JSON store -> when we create a contribution so that way it's evident that
all entries in JSON store are pending contributions ?

We can definitely do that! I'll use a new key for the JSON stores here so they are easily distinguished from existing JSON stores, and will be sure to remove the entry once the contribution is created.

Was this page helpful?
0 / 5 - 0 ratings

Related issues

kziemianek picture kziemianek  Â·  3Comments

frankchen07 picture frankchen07  Â·  4Comments

mbeacom picture mbeacom  Â·  4Comments

wizzfile picture wizzfile  Â·  3Comments

christianbundy picture christianbundy  Â·  3Comments