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

Redeemer would best not check DAO funds when there is a CrExt.rewarder #708

Open
dkent600 opened this issue Jan 15, 2020 · 0 comments
Open

Comments

@dkent600
Copy link
Contributor

dkent600 commented Jan 15, 2020

Change Request

Please in the case where the CrExt has a rewarder, remove the check in Redeemer.contributionRewardExtRedeem for DAO-available ETH and external tokens, thus allowing all rewards to always be "primed" for redemption via suggestion.redeem.

Motivation

In dApps such as Alchemy, we don't want the user to annoyingly waste their funds and time attempting to redeem submissions that revert. Thus we want to be able to efficiently determine in advance whether a suggestion.redeem is going revert, and we want to be able to clearly and simply explain to the user why they can't redeem.

suggestion.redeem will revert in the following scenarios:

  1. when any one or more of the rewards have not yet been successfully "primed" for redeeming by redeeming at the CrExt proposal level.
  2. when a DAO has not sufficient funds to pay a reward (ETH or ext token), either at the time of item 1, or in the current moment.
  3. when the rewards have already been successfully redeemed via the suggestion

Lets assume we are using the Redeemer contract to perform item 1 using Redeemer.redeemFromCRExt, and that item 2 is true at the time we call Redeemer.redeemFromCRExt.

Then we try suggestion.redeem. The result will be a revert due to item 2 having been true at the time of item 1 when Redeemer.redeemFromCRExt was invoked and therefore one or more rewards will not have been "primed" for redeeming.

How can we know in advance that this is going to happen? We can call suggestion.redeem in advance and tell the user it is reverting for possibly either of 1 or 2, which is going to be quite a long message to the user because we can't really eliminate any of the possibilities. "You cannot yet redeem because either a) the submission rewards have not yet been enabled by redeeming on the Competition proposal, or b) the DAO had insufficient funds to pay a reward (ETH or external tokens) at the time you attempted to redeem on the Competition proposal. Please try again to redeem on the Competition proposal." Ouch, for multiple reasons.

Moreover, it is less than optimal to have to call in advance of the user clicking a Redeem button. We would prefer to wait until they click, only then issuing the call and the error message. Better UX is to explain in advance why they can't redeem.

This could all be simpler with the following change to the Redeemer contract: In just the case where the CrExt has a rewarder, remove the check for DAO-available ETH and external tokens, thus allowing all rewards to always be "primed" for redemption via suggestion.redeem.

Then reporting on item 1 is easy by checking whether any one of these CrExt proposal props are non-zero: nativeTokenRewardLeft, reputationChangeLeft, ethRewardLeft, externalTokenRewardLeft (combined with checking item 3).

Reporting on item 2 becomes clear because we only care about DAO balances in the current moment.

Item 3 is easy in any case as we can check the Redeem event (which I believe the subgraph does when hydrating competitionSuggestion.redeemedAt..

So with this change we can be quite clear in the code and efficiently explain in advance to the user why a suggestion cannot at the current time be redeemed.

Moreover, from the point of view of a best practice architectural design pattern, it scopes and simplifies the implications of redeeming via rewarder to code in the rewarder and CrExt contracts, where those implications belong (thus in principle reducing the risk of problems like this)

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

No branches or pull requests

1 participant