update executeboosted reward function #65
Conversation
| * @param _proposalId the ID of the proposal | ||
| * @return uint256 executeCallBounty | ||
| */ | ||
| function calcExecuteCallBounty(bytes32 _proposalId) private view returns(uint256) { |
There was a problem hiding this comment.
If there's no special reason not to, please make this public.
There was a problem hiding this comment.
ok. maybe it will be useful to someone.
| * @dev executeBoosted try to execute a boosted or QuietEndingPeriod proposal if it is expired | ||
| * it rewards the msg.sender with P % of the proposal's upstakes upon a successful call to this function. | ||
| * P = t/10 and t = number of blocks passed between the proposal's timeout and a successful call to this function. | ||
| * the function assume an avergae block time of 15 seconds. |
There was a problem hiding this comment.
That seems to me just an unnecesaary complex representation of what you are doing.
Just say "P = t/150, where t is the number of seconds passed since the the proposal's timeout", no need to make assumptions on block times (which you are not making, anyway, this code will run the same whatever the block times are)
There was a problem hiding this comment.
the rewards steps increments on each 15 seconds(which is an average block time).
it is not continuous (step on each second). if we decided to do that we could ,but currently it is not.
There was a problem hiding this comment.
Yes, it increments on each 15 seconds, which is the average block time, so it will miscalculate the amount of blocks mined exactly half of the time.
To me it looks like adding complexity without any good reason at all. Time in the infra is measured everywhere in seconds and not in (approximations of) blocks, so why make an exception here?
There was a problem hiding this comment.
made it a linear function.
see 382daa5
| "proposal state in not Boosted nor QuietEndingPeriod"); | ||
| require(_execute(_proposalId), "proposal need to expire"); | ||
| uint256 expirationCallBountyPercentage = | ||
| uint256 blocksSinceTimeOut = |
There was a problem hiding this comment.
just use seconds, not blocks, as unit of measure (see above)
There was a problem hiding this comment.
it is blocks ,not very accurate calculation but good enough.
with the assumption that an average block time is 15 seconds.
There was a problem hiding this comment.
Does not seem so good to me. If the first block is mined after 14 seconds, then "blocksSinceTimeout = ((14).div(15) = 0.
There was a problem hiding this comment.
yes, the first step is 0.
| proposal.expirationCallBountyPercentage = expirationCallBountyPercentage; | ||
| expirationCallBounty = expirationCallBountyPercentage.mul(proposal.stakes[YES]).div(100); | ||
|
|
||
| proposal.expirationCallBountyPercentage = blocksSinceTimeOut; |
There was a problem hiding this comment.
you write in the docs that the maximum is 10%, but this goes up to 100, it seems to me.
There was a problem hiding this comment.
I see that calcExecuteCallBounty divides by 1000, but please please please do not have a variable called PerCentage that stores ProMille.
There was a problem hiding this comment.
you write in the docs that the maximum is 10%, but this goes up to 100, it seems t
yes, a value of 100 is 10% -
see https://github.com/daostack/infra/blob/execute_boosted/contracts/votingMachines/GenesisProtocolLogic.sol#L76
and
There was a problem hiding this comment.
I know, my objection is against the variable name, not the calculation :-)
There was a problem hiding this comment.
just call it expirationCallBountyProMille and everything is fine :-)
4 participants
P = t/10 and t = number of blocks passed between the proposal's timeout and a successful call to this function. max P is 10 %.
the bug manifest itself when a dao/scheme will try to redeem its downstakes winning from a failed proposals and only if there is already a reward payout for executeboosted call. in some cases it might cause the redeem call to revert so the dao/scheme could not redeem its winning.