Competition contract + ContributionRewardExt scheme #697
Conversation
| proposal.reputationChangeLeft = proposal.reputationChangeLeft.sub(_reputation); | ||
| require( | ||
| Controller( | ||
| avatar.owner()).mintReputation(_reputation, _beneficiary, address(avatar))); |
There was a problem hiding this comment.
Should the mint be inside of if (_reputation != 0)?
Should we not burn when if (_reputation < 0)?
There was a problem hiding this comment.
Should the mint be inside of if (_reputation != 0)
we can but not must. I will add that.
Should we not burn when if (_reputation < 0)?
the _reputation here is uint so it cannot be < 0
There was a problem hiding this comment.
can we comment, like we did in redeemReputation, that burn is not currently allowed?
| proposal.reputationChangeLeft = proposal.reputationChangeLeft.sub(_reputation); | ||
| require( | ||
| Controller( | ||
| avatar.owner()).mintReputation(_reputation, _beneficiary, address(avatar))); |
There was a problem hiding this comment.
can we comment, like we did in redeemReputation, that burn is not currently allowed?
|
|
||
| /** | ||
| * @title A scheme for proposing and rewarding contributions to an organization | ||
| * @dev An agent can ask an organization to recognize a contribution and reward |
There was a problem hiding this comment.
can you write a line about how this differs from ContributionReward, and what it's intende purpose is?
There was a problem hiding this comment.
(also.the "Ext", does it stand for "External" or "Extra" or "Extended"?)
There was a problem hiding this comment.
So (after reading the code) this is an extended version of the ContriutionReward contract, with the possibility to assign a rewarder, which, after the contributionreward has been accepted, can then later distribute the assets as it would like.
So perhaps call it "ContributionRewardWithRewarder"?
To me, it seems a bit crazy do stuff all of this logic in a single contract (it is an enormous, complex, contract)
If I understand it correctly, if the rewards were all transferable tokens (i.e. no rep), we could have a much cleaner setup by using our original contribution reward, and transfer the assets to a beneficiary contract that manages the payouts for the competition. But (I think) this will not be possible bc the rights to mint rep are difficult to manage.
(which makes me think - not for this PR, but for arc2.0 - instead of doing all this administration of keeping track fo which schemes have the right to mint which rep, it could perhaps be easier and more secure to work with a "rightToMintRep" token)
There was a problem hiding this comment.
So (after reading the code) this is an extended version of the ContriutionReward contract, with the possibility to assign a rewarder, which, after the contributionreward has been accepted, can then later distribute the assets as it would like.
So perhaps call it "ContributionRewardWithRewarder"?
To me, it seems a bit crazy do stuff all of this logic in a single contract (it is an enormous, complex, contract)
If I understand it correctly, if the rewards were all transferable tokens (i.e. no rep), we could have a much cleaner setup by using our original contribution reward, and transfer the assets to a beneficiary contract that manages the payouts for the competition. But (I think) this will not be possible bc the rights to mint rep are difficult to manage.
(which makes me think - not for this PR, but for arc2.0 - instead of doing all this administration of keeping track fo which schemes have the right to mint which rep, it could perhaps be easier and more secure to work with a "rightToMintRep" token)
We might deprecate the Universal ContribiutionReward soon.When doing that this ContribiutionRewardExt will be re name ContribiutionReward
yes. If a rep was not a req here we could teoreticlyl use a normal cr with the beneficiary the competition scheme / or other
|
|
||
| /** | ||
| * @dev execution of proposals, can only be called by the voting machine in which the vote is held. | ||
| * @param _proposalId the ID of the voting in the voting machine |
| /** | ||
| * @dev redeem rewards for proposal | ||
| * @param _proposalId the ID of the voting in the voting machine | ||
| * @param _whatToRedeem whatToRedeem array: |
There was a problem hiding this comment.
array of boolean values
There was a problem hiding this comment.
seems a bit overengineered, this: what is the point of having so fine of control over what is being redeemed in this funjction, when all the other methods are also calleable separately?
There was a problem hiding this comment.
this is an helper function to enable redeem of all in one tx.
the fine control is needed to avoid reverts for the case the dao does not hold all the rewards of the time of the call .
and there was also a need to maintain the flexibility to call each one of it separately.
| } | ||
| } | ||
|
|
||
| if (suggestions[topSuggestions[smallest]].totalVotes < suggestions[_suggestionId].totalVotes) { |
There was a problem hiding this comment.
So, if the _suggestionId has the same number of votes as the smallest in the topSuggestions, it will not be added to the topSuggestions.
That seems unfair :-)
There was a problem hiding this comment.
- in this case all the suggestions which has the same number of votes will get their split reward divided by their count.
- for this we are maintaining votesToSuggestions
increase GAS_LIMIT to 62000000 due to new openzeppeling-solidity
|
thanks @ben-kaufman @jellegerbrandy @dkent600 for comments and feedback. |
4 participants
No description provided.