-
-
Notifications
You must be signed in to change notification settings - Fork 775
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
adds the ability for phantom funding of a grant #5054
Conversation
|
Codecov Report
@@ Coverage Diff @@
## master #5054 +/- ##
=========================================
Coverage ? 30.88%
=========================================
Files ? 220
Lines ? 17752
Branches ? 2446
=========================================
Hits ? 5482
Misses ? 12042
Partials ? 228
Continue to review full report at Codecov.
|
@vs77bb should you take this forward or should i? @danlipert any comments on the existing code before we push it across the finish line? |
app/grants/models.py
Outdated
@@ -900,3 +900,25 @@ class MatchPledge(SuperModel): | |||
def __str__(self): | |||
"""Return the string representation of this object.""" | |||
return f"{self.profile} <> {self.amount} DAI" | |||
|
|||
class PhantomFunding(SuperModel): | |||
"""Define the structure of a subscription agreement.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add something that describes what this is for or why we created it? Looks like this is from a cut-and-paste job ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
app/grants/models.py
Outdated
'grants.Grant', | ||
related_name='phantom_funding', | ||
on_delete=models.CASCADE, | ||
help_text=_('The associated Phantom Funding.'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change help text to something like The grant being Phantom funded
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
app/grants/models.py
Outdated
'dashboard.Profile', | ||
related_name='grant_phantom_funding', | ||
on_delete=models.CASCADE, | ||
help_text=_('The associated Phantom Funding.'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change help text to The user profile creating the Phantom Funding
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
any more feedback? @danlipert -- @vs77bb is pestering me to get this live |
…-add a user to a group based upon a link
@gitcoinco/engineers ?? cc @vs77bb |
|
||
""" | ||
|
||
round_number = models.PositiveIntegerField(blank=True, null=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@frankchen07 this is the new model that will contain info about when people phantom fund.
two ways we can manage this:
- you just build phantom funding into your existing reports
- myself or engineering builds a
finalize_phantom_funding
job that runs once per round, at end of round, that
a. closes out phantom funding for that round
b. makes the phantom contribs into actual contributions (both on chain and in the DB)
c. anything else??
cc @danlipert 4 other ideas
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
I'm assuming the data model reflects this in a way where it's easy to build it into the reports. I think as long as it tell me the contributor, which grant(s) they've committed to and how much, it can be easily integrated into the report at the end of the CLR round.
-
from a product perspective, point number 2 is the trickier one, especially with As a funder I want to see previous contributions to projects and what my expected CLR match is #3663 - since this is part of the product update that vitalik requested (a nifty little feature that tells us in "real" time, how much potential a contribution has in the LR context, and this needs all the contributions, real and phantom in order to be the most accurate). I might be overthinking it in the sense that if the data exists it can be used in the feature, and the
phantom_funding
job is a completely separate "payout" related action at the end.
In a related conversation, Dan suggested that we do some kind of simplified calculation and cache the results so we can give them a range, since doing a live calculation with real data every time someone changes a number in an input box can be performance hell.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- yea it does that (tho 'how much' is inferred by 5/(num_votes_cast_by_contrib)
- hmm.. yeah As a funder I want to see previous contributions to projects and what my expected CLR match is #3663 does make it more complicated. ill add a note there about phantom funding.
agree with @danlipert s thoughts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just 1 small thing and changing the toggle to a POST request to prevent CSRF/etc.
app/grants/views.py
Outdated
round_number = 3 | ||
can_phantom_fund = request.user.is_authenticated and request.user.groups.filter(name='phantom_funders').exists() | ||
phantom_funds = PhantomFunding.objects.filter(profile=request.user.profile, grant=grant, round_number=round_number) | ||
is_phantom_funding_this_grant = can_phantom_fund and request.user.is_authenticated and phantom_funds.exists() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extra auth check here already checked when can_phantom_fund
is created
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updating
app/grants/views.py
Outdated
show_tweet_modal = False | ||
if can_phantom_fund: | ||
active_tab = 'phantom' | ||
if can_phantom_fund and request.GET.get('toggle_phantom_fund'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This we need to turn into a POST request so that it can't be exploited
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updating
@danlipert just fixed both your comments in 70bb11b |
anyone else have feedback? this needs to go in weds release @enginneering |
@thelostone-mc @octavioamu Lets make sure we get this one in Wednesday |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code wise lgtm !
Need to re-indent stuff but yeah
Description
adds the ability for phantom funding of a grant.
this allows anyone who is featured flagged to "signal" that they support a specific project.
so that we can get wider amount of contributions from the community, by skipping all the blockchain-ey parts.
to go live:
Refers/Fixes
vivek was going to write a ticket, i dont think he has yet tho.
Testing
tested it locally.