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

Improve performance on cart load #8702

Merged
merged 1 commit into from
Apr 1, 2021
Merged

Improve performance on cart load #8702

merged 1 commit into from
Apr 1, 2021

Conversation

octavioamu
Copy link
Contributor

@octavioamu octavioamu commented Mar 26, 2021

Description
  • Renew cart data from the server
  • Create "slim=true" parameter on endpoint to fetch from payload model (use to fetch all the grant data and not update the cart)
  • Change logic to avoid "decimals" error
  • Remove timeout/delay waiting 5s on web3 and instead use events resulting on non blockers onload.
  • Avoid doing zksync / eth operations when in others chains
  • Fix ZEC not getting filled on sidecart by missing "else"
  • Remove dead code and reorder files.
Refers/Fixes
Testing

A lot of testing with diff chains and fixing consoles errors.
Improve the load performance

VID_20210325_181551.mp4

Comment on lines +1423 to +1426
// Make sure none have empty currencies, and if they do default to 0.001 ETH. This is done
// to prevent the cart from getting stuck loading if a currency is empty
updatedGrant[grantIndex]['grant_donation_currency'] = grant.grant_donation_currency ? grant.grant_donation_currency : 'ETH';
updatedGrant[grantIndex]['grant_donation_amount'] = grant.grant_donation_amount ? grant.grant_donation_amount : '0.001';
Copy link
Contributor

Choose a reason for hiding this comment

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

PR looks great! Just one small comment here: This should probably vary by tenant, right? It seems right now we set the default value to 0.001 ETH regardless of the grant's tenant

Copy link
Contributor Author

@octavioamu octavioamu Apr 1, 2021

Choose a reason for hiding this comment

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

only if not defined but I fixed the others areas to always be defined already, so probably this part will be deleted on some point is just a double check for security for now

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.

code wise lgtm

Copy link
Contributor

@gdixon gdixon left a comment

Choose a reason for hiding this comment

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

Awesome! 🚀 LGTM!!

@gdixon gdixon merged commit 64f3b0b into master Apr 1, 2021
iRhonin pushed a commit to iRhonin/web that referenced this pull request Apr 23, 2021
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.

5 participants