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.

Sign up for GitHub

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

Jump to bottom

CLR cart updates #6978

Merged
merged 9 commits into from
Jun 26, 2020
Merged

CLR cart updates #6978

merged 9 commits into from
Jun 26, 2020

Conversation

mds1
Copy link
Contributor

@mds1 mds1 commented Jun 24, 2020 •
edited
Loading

Builds on the changes from #6966. Closes #6959

NOTE: This branch has not been thoroughly tested. Pending issues:

  • It looks like the get_amount endpoint throws if USDC is an input?
  • Similarly, for an input of DAI (or any stablecoin) it returns a token name of USDT—added logic to handle this
  • Prediction curves return empty arrays when testing against the Rinkeby grants

image

cc @thelostone-mc @owocki

@owocki
Copy link
Contributor

owocki commented Jun 24, 2020 via email

@codecov
Copy link

codecov bot commented Jun 24, 2020 •
edited
Loading

Codecov Report

Merging #6978 into stable will increase coverage by 0.00%.
The diff coverage is 34.78%.

Impacted file tree graph

@@           Coverage Diff           @@
##           stable    #6978   +/-   ##
=======================================
  Coverage   26.46%   26.46%           
=======================================
  Files         299      299           
  Lines       28829    28845   +16     
  Branches     4226     4228    +2     
=======================================
+ Hits         7630     7635    +5     
- Misses      20928    20939   +11     
  Partials      271      271
Impacted Files Coverage Δ
app/dashboard/admin.py 64.73% <ø> (ø)
.../dashboard/management/commands/cleanup_db_space.py 0.00% <ø> (ø)
app/dashboard/router.py 38.69% <ø> (ø)
app/dashboard/views.py 10.55% <0.00%> (ø)
app/kudos/admin.py 50.96% <0.00%> (-1.00%) ⬇️
app/kudos/utils.py 19.71% <0.00%> (-0.08%) ⬇️
app/marketing/mails.py 10.94% <ø> (ø)
app/grants/views.py 18.64% <25.00%> (-0.07%) ⬇️
app/dashboard/models.py 49.39% <33.33%> (-0.02%) ⬇️
app/kudos/models.py 50.62% <42.85%> (-0.18%) ⬇️
... and 2 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 897e1f6...d93d544. Read the comment docs.

@mds1 mds1 force-pushed the clr-cart-updates branch from d93d544 to 95e2e9f Compare June 24, 2020 23:43
@mds1
Copy link
Contributor Author

mds1 commented Jun 24, 2020

Thanks @owocki, just cherry-picked it to this branch and added a fix for handling stablecoins.

Haven't tested the end-to-end flow since it seems the CLR curves return empty arrays for the default Rinkeby grants. But I did test by temporarily replacing line 774 from

const clr_prediction_curve_2d = grant.grant_clr_prediction_curve;

to

const clr_prediction_curve_2d = [
  [ 0.0, 1577.1156943411, 0.0 ],
  [ 1.0, 1629.4037561848, 52.2880618436975 ],
  [ 10.0, 1706.22286476493, 129.107170423827 ],
  [ 100.0, 1842.66035063668, 265.544656295572 ],
  [ 1000.0, 2020.7280581732, 443.612363832099 ],
  [ 10000.0, 2183.80886558997, 606.693171248862 ]
];

and it did work as expected

@mds1 mds1 marked this pull request as ready for review June 24, 2020 23:45
@thelostone-mc thelostone-mc mentioned this pull request Jun 25, 2020
Closed
@danlipert
Copy link
Contributor

this is still a WIP branch right?

@mds1
Copy link
Contributor Author

mds1 commented Jun 25, 2020

@danlipert This should be good to merge, though I'd like to get someone else to give it a brief test to confirm it works as expected. Reason why is just because I wasn't able to test the full e2e flow:

Haven't tested the end-to-end flow since it seems the CLR curves return empty arrays for the default Rinkeby grants. But I did test by temporarily replacing line 774 from ...

I think @thelostone-mc may have tested this before approving but I'm not 100% sure

To summarize the changes in this branch:

  • Adds API endpoint to fetch CLR prediction curve
  • Cart uses that endpoint to get updated curves every time /grants/cart is loaded
  • Reduce the number of calls the cart page sends to the get_amount endpoint. This lets the cart compute CLR match amounts faster (this was slow when there's a large number of items in the cart)

@danlipert
Copy link
Contributor

@mds1 Ah I think I understand - to get the prediction curves calculated in your local you have to run the management command to compute them after setting up a couple of example donations. Set up some grants, at least 2 users, and have the 2 users donate to at least 2 different grants. Then run the management command like so:
docker-compose exec web python3 app/manage.py estimate_clr tech rinkeby - replace tech with whatever category the grants are set up under. Once this runs you should have some populated curves. Let me know if im misunderstanding the issue tho

@mds1
Copy link
Contributor Author

mds1 commented Jun 25, 2020

Got it, that makes sense. Thanks. I'll be able to test that out in a few hours and will update you here afterwards

@thelostone-mc
Copy link
Member

Did test it out !
Added like 15 grants to see if it slowed down
Seems all chill except the sync_amount being called twice

Screenshot 2020-06-26 at 5 13 40 AM

@thelostone-mc thelostone-mc mentioned this pull request Jun 25, 2020
Closed
@thelostone-mc
Copy link
Member

Reason why we have to do that

Matt SolomonToday at 5:24 AM
@thelostone-mc The reason the endpoint is called twice is just an artifact of the cart architecture—any time an item in the cart is changed, or when the cart is loaded, we run through the watch code to make sure it's up to date.
So when the page is first loaded, we read the list of grants from local storage which triggers the watcher. Then we fetch the updated CLR curves which again triggers the watcher

@mds1 mds1 force-pushed the clr-cart-updates branch from 95e2e9f to b364440 Compare June 26, 2020 02:14
@mds1
Copy link
Contributor Author

mds1 commented Jun 26, 2020

This PR now also includes the changes from #6996

One final issue to resolve before merging: In the side cart, if I add all 3 rinkeby grants and use "Apply to all", the grant_donation_amount field of "Go Fund My Test Grant" gets deleted somehow....

@mds1 mds1 mentioned this pull request Jun 26, 2020
Closed
@thelostone-mc
Copy link
Member

One final issue to resolve before merging: In the side cart, if I add all 3 rinkeby grants and use "Apply to all", the grant_donation_amount field of "Go Fund My Test Grant" gets deleted somehow....

Tested it with 6 mainnet grants and it seems all chill

@mds1
Copy link
Contributor Author

mds1 commented Jun 26, 2020

Tested it with 6 mainnet grants and it seems all chill

This isn't the first time that specific grant has given me issues, so there might just be something wrong with it or my config on rinkeby. So I think should be good to merge

@thelostone-mc thelostone-mc merged commit 092b2be into gitcoinco:stable Jun 26, 2020
This was referenced Jun 26, 2020
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thelostone-mc thelostone-mc thelostone-mc approved these changes

@danlipert danlipert Awaiting requested review from danlipert

@frankchen07 frankchen07 Awaiting requested review from frankchen07

@octavioamu octavioamu Awaiting requested review from octavioamu

@PixelantDesign PixelantDesign Awaiting requested review from PixelantDesign

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mds1 @owocki @danlipert @thelostone-mc