search

daostack / infra

GNU General Public License v3.0
25 stars 22 forks source link

Protocol 2 0 #31

Closed orenyodfat closed 5 years ago

orenyodfat commented 5 years ago

This pr implement the protocol update according to https://docs.google.com/document/d/1GY3heuJa9kDlcpa0crm5xDBpugUlENZbFQoUuhC7XPQ/edit#heading=h.69b65n1tqwg

fix : #27

orenyodfat commented 5 years ago

Thanks @ben-kaufman

orenyodfat commented 5 years ago

@dkent600 "I question why require true from _execute? Do we want to disincentivize calling this expired if the proposal cannot be executed?" - if it is not expired do not call it..(check before call).

orenyodfat commented 5 years ago

@dkent600 "that is unfortunate for the caller. why does the require need to be there at all?" because it add a level of secure if a condition of rewarding does not met ..especially when this function giving some money back. User should call this function only when the proposal is in boosted and it is expired.

dkent600 commented 5 years ago

@orenyodfat

"that is unfortunate for the caller. why does the require need to be there at all?" because it add a level of secure if a condition of rewarding does not met ..especially when this function giving some money back. . User should call this function only when the proposal is in boosted and it is expired.

_execute will return true or false regardless of whether you put it in a require. You can check that return value in an if. How does is this less secure?

orenyodfat commented 5 years ago

"_execute will return true or false regardless of whether you put it in a require. You can check that return value in an if. How does is this less secure?"
by return false:

by the way , same logic choose by openzeppline while implementing ERC20 specification: https://github.com/OpenZeppelin/openzeppelin-solidity/blob/master/contracts/token/ERC20/ERC20.sol#L136

dkent600 commented 5 years ago

@orenyodfat

It may not be clear to callers that the function can unsuccessfully run. caller must check its return value. bad things may happened otherwise.

The caller can check for the ExpirationCallBounty event, the token Transfer event or check their token balance. (They could also .call the function to see if the expirationCallBounty is > 0.) This pattern is preferable to require(_execute(_proposalId)) because it is optional, does not take the developer by surprise, and does not put callers at risk of losing gas.

function can become unclear as there might be multiple return values . by require:

I don't understand what you're saying by that.

by the way , same logic choose by openzeppline while implementing ERC20 specification: https://github.com/OpenZeppelin/openzeppelin-solidity/blob/master/contracts/token/ERC20/ERC20.sol#L136

Here is the token code we're actually using (from BasicToken.sol):

  function transfer(address _to, uint256 _value) public returns (bool) {
    require(_value <= balances[msg.sender]);
    require(_to != address(0));

    balances[msg.sender] = balances[msg.sender].sub(_value);
    balances[_to] = balances[_to].add(_value);
    emit Transfer(msg.sender, _to, _value);
    return true;
  }

In executeExpiredBoosted you've already got the equalivalent of the token transfer pattern via this line:

require(proposal.state == ProposalState.Boosted);

The token transfer code does not require(_value > 0), which would be the equivalent of require(_execute(_proposalId))

safer since it ensures that there are no side effects that remain.

That is the only part of this that makes sense to me

orenyodfat commented 5 years ago

What ever . No more words . The require are in place .

On Tue, 18 Dec 2018 at 15:43 Doug Kent notifications@github.com wrote:

@orenyodfat https://github.com/orenyodfat

It may not be clear to callers that the function can unsuccessfully run. caller must check its return value. bad things may happened otherwise.

The caller can check for the ExpirationCallBounty event, the token Transfer event or check their token balance. (They could also .call the function to see if the expirationCallBounty is > 0.) This pattern is preferable to require(_execute(_proposalId)) because it is optional, does not take the developer by surprise, and does not put callers at risk of losing gas.

function can become unclear as there might be multiple return values . by require:

I don't understand what you're saying by that.

by the way , same logic choose by openzeppline while implementing ERC20 specification:

https://github.com/OpenZeppelin/openzeppelin-solidity/blob/master/contracts/token/ERC20/ERC20.sol#L136

Here is the token code we're actually using (from BasicToken.sol):

function transfer(address _to, uint256 _value) public returns (bool) { require(_value <= balances[msg.sender]); require(_to != address(0));

balances[msg.sender] = balances[msg.sender].sub(_value);
balances[_to] = balances[_to].add(_value);
emit Transfer(msg.sender, _to, _value);
return true;

}

In executeExpiredBoosted you've already got the equalivalent of the token transfer pattern via this line:

require(proposal.state == ProposalState.Boosted);

The token transfer code does not require(_value > 0), which would be the equivalent of require(_execute(_proposalId))

safer since it ensures that there are no side effects that remain.

That is the only part of this that makes sense to me

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/daostack/infra/pull/31#issuecomment-448225054, or mute the thread https://github.com/notifications/unsubscribe-auth/AFt3nh1u8ZnypYrZR6crzRvv_oY1_zMdks5u6PEAgaJpZM4ZVJp9 .

dkent600 commented 5 years ago

@orenyodfat

safer since it ensures that there are no side effects that remain.

Could have an automated test to ensure that the side effects do not occur.

dkent600 commented 5 years ago

@orenyodfat can we have reason strings on all the reverts?

orenyodfat commented 5 years ago

No . Because if it timeout so it is the same as normal q proposal which is timeout .

On Wed, 19 Dec 2018 at 13:38 Doug Kent notifications@github.com wrote:

@dkent600 commented on this pull request.

In contracts/votingMachines/GenesisProtocolLogic.sol https://github.com/daostack/infra/pull/31#discussion_r242884939:

  • if (proposal.state == ProposalState.PreBoosted) {
  • // solium-disable-next-line security/no-block-members
  • if ((now - proposal.times[2]) >= params.preBoostedVotePeriodLimit) {
  • if (shouldBoost(_proposalId)) {
  • //change proposal mode to Boosted mode.
  • proposal.state = ProposalState.Boosted;
  • // solium-disable-next-line security/no-block-members
  • proposal.times[1] = now;
  • orgBoostedProposalsCnt[proposal.organizationId]++;
  • //add a value to average -> average = average + ((value - average) / nbValues)
  • averageBoostDownstakes = averagesBoostDownstakes[proposal.organizationId];
  • averagesBoostDownstakes[proposal.organizationId] = uint256(int256(averageBoostDownstakes) + ((int216(proposal.stakes[NO])-int216(averageBoostDownstakes)).toReal().div(int216(orgBoostedProposalsCnt[proposal.organizationId]).toReal())).fromReal());
  • }
  • } else { //check the Confidence level is stable
  • if (_score(_proposalId) <= proposal.confidenceThreshold) {
  • proposal.state = ProposalState.Queued;

"no need to check its timeout"?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/daostack/infra/pull/31#discussion_r242884939, or mute the thread https://github.com/notifications/unsubscribe-auth/AFt3nk0hxhxZm9Gw4gDnsRtCBizNcro5ks5u6iUpgaJpZM4ZVJp9 .

orenyodfat commented 5 years ago

Nice to have . Not mandatory

On Wed, 19 Dec 2018 at 13:41 Doug Kent notifications@github.com wrote:

@orenyodfat https://github.com/orenyodfat can we have reason strings on all the reverts?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/daostack/infra/pull/31#issuecomment-448565620, or mute the thread https://github.com/notifications/unsubscribe-auth/AFt3nlOctwjhoD2OiQfi87238i19-FjCks5u6iYGgaJpZM4ZVJp9 .

dkent600 commented 5 years ago

@orenyodfat

Nice to have . Not mandatory [revert strings]

They are very helpful to developers