Voting: Allow to modify required support #458
Conversation
Snapshot it for each vote, create role for it and add tests.
| @@ -85,6 +88,21 @@ contract Voting is IForwarder, AragonApp { | |||
| voteTime = _voteTime; | |||
| } | |||
|
|
|||
| /** | |||
| * @notice Change required support to `(_supportRequiredPct - _supportRequiredPct % 10^16) / 10^14`% | |||
There was a problem hiding this comment.
I've never understood this radspec calculation (here and elsewhere) 😅. Why not just supportRequiredPct / 10^14?
There was a problem hiding this comment.
It was supposed to round to 2 decimals, but I think it's wrong, 14 and 16 should be reverted.
There was a problem hiding this comment.
For some reason I changed it in commit 6bb186b 🤔
There was a problem hiding this comment.
Whatever it's the good solution, will fix it in another PR as it affects to other functions here and Survey too.
There was a problem hiding this comment.
Another helper function for radspec :)
apps/voting/test/voting.js
Outdated
|
|
||
| assert.isTrue(isOpen, 'vote should be open') | ||
| assert.isFalse(isExecuted, 'vote should not be executed') | ||
| assert.equal(creator, holder50, 'creator should be correct') | ||
| assert.equal(snapshotBlock, await getBlockNumber() - 1, 'snapshot block should be correct') | ||
| assert.deepEqual(supportRequired, neededSupport, 'required support should be app required support') |
There was a problem hiding this comment.
Seems weird to need the deepEqual in the tests (here and elsewhere), since both supportRequired and minQuorum are numbers.
Address PR #458 comments.
apps/voting/test/voting.js
Outdated
| assert.equal((await voting.supportRequiredPct()).toString(), neededSupport.add(1).toString(), 'should have changed required support') | ||
| }) | ||
|
|
||
| it('fails changing required supoprt lower than minimum acceptance quorum', async () => { |
| @@ -251,6 +271,7 @@ contract Voting is IForwarder, AragonApp { | |||
| vote_.metadata = _metadata; | |||
| vote_.snapshotBlock = getBlockNumber() - 1; // avoid double voting in this very block | |||
| vote_.totalVoters = token.totalSupplyAt(vote_.snapshotBlock); | |||
| vote_.supportRequiredPct = supportRequiredPct; | |||
There was a problem hiding this comment.
🚨 The cached vote_.supportRequiredPct is currently not being used to check whether a vote can be executed in canExecute().
aragon-apps/apps/voting/contracts/Voting.sol
Line 210 in 97ca6b0
There was a problem hiding this comment.
Good catch, thanks!! I see that the test for it below is wrong too.
apps/voting/test/voting.js
Outdated
| await timeTravel(votingTime + 1) | ||
|
|
||
| const state = await voting.getVote(voteId) | ||
| assert.equal(state[5].toNumber(), neededSupport.toNumber(), 'required support in vote should stay equal') |
apps/voting/test/voting.js
Outdated
| // it will succeed | ||
|
|
||
| await voting.vote(voteId, true, true, { from: holder50 }) | ||
| await voting.vote(voteId, true, true, { from: holder19 }) |
There was a problem hiding this comment.
This test is not actually testing the logic we want to test, as both holders are voting YES, the support for the proposal is 100%.
Because we are not properly using the cached supportRequiredPct for the canExecute calculation.
We should add a NO vote by holder31, which would make the support of the proposal be 69, and would make this test fail until the issue with canExecute is fixed
Address PR #458 comments.
Address PR aragon#458 comments.
Address PR aragon#458 comments.
Voting: Allow to modify required support
Snapshot it for each vote, create role for it and add tests.
Needed for #270.
Users can always upgrade their Voting apps to modify how the supporting threshold is calculated if they wanted to modify their behaviour. This directly exposes that functionality to avoid convoluted situations like that. Also useful when changing the size or required threshold in multisig scenarios.