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.

Sign up for GitHub

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

Voting: Allow to modify required support #458

Merged
merged 3 commits into from
Sep 18, 2018
Merged

Voting: Allow to modify required support #458

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b5a282c
Voting: Allow to modify required support
Sep 17, 2018
97ca6b0
Voting: remove unnecessary use of deepEqual in tests
Sep 18, 2018
b468b83
Voting: fix bug with required support
Sep 18, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions apps/voting/contracts/Voting.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ contract Voting is IForwarder, AragonApp {
using SafeMath64 for uint64;

bytes32 public constant CREATE_VOTES_ROLE = keccak256("CREATE_VOTES_ROLE");
bytes32 public constant MODIFY_SUPPORT_ROLE = keccak256("MODIFY_SUPPORT_ROLE");
bytes32 public constant MODIFY_QUORUM_ROLE = keccak256("MODIFY_QUORUM_ROLE");

uint256 public constant PCT_BASE = 10 ** 18; // 0% = 0; 1% = 10^16; 100% = 10^18
Expand All @@ -28,6 +29,7 @@ contract Voting is IForwarder, AragonApp {
address creator;
uint64 startDate;
uint256 snapshotBlock;
uint256 supportRequiredPct;
uint256 minAcceptQuorumPct;
uint256 yea;
uint256 nay;
Expand All @@ -50,6 +52,7 @@ contract Voting is IForwarder, AragonApp {
event StartVote(uint256 indexed voteId);
event CastVote(uint256 indexed voteId, address indexed voter, bool supports, uint256 stake);
event ExecuteVote(uint256 indexed voteId);
event ChangeSupportRequired(uint256 supportRequiredPct);
event ChangeMinQuorum(uint256 minAcceptQuorumPct);

modifier voteExists(uint256 _voteId) {
Expand Down Expand Up @@ -85,6 +88,21 @@ contract Voting is IForwarder, AragonApp {
voteTime = _voteTime;
}

/**
* @notice Change required support to `(_supportRequiredPct - _supportRequiredPct % 10^16) / 10^14`%
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've never understood this radspec calculation (here and elsewhere) 😅. Why not just supportRequiredPct / 10^14?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was supposed to round to 2 decimals, but I think it's wrong, 14 and 16 should be reverted.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some reason I changed it in commit 6bb186b 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whatever it's the good solution, will fix it in another PR as it affects to other functions here and Survey too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another helper function for radspec :)

* @param _supportRequiredPct New required support
*/
function changeSupportRequiredPct(uint256 _supportRequiredPct)
external
authP(MODIFY_SUPPORT_ROLE, arr(_supportRequiredPct, supportRequiredPct))
{
require(minAcceptQuorumPct <= _supportRequiredPct);
require(_supportRequiredPct <= PCT_BASE);
supportRequiredPct = _supportRequiredPct;

emit ChangeSupportRequired(_supportRequiredPct);
}

/**
* @notice Change minimum acceptance quorum to `(_minAcceptQuorumPct - _minAcceptQuorumPct % 10^16) / 10^14`%
* @param _minAcceptQuorumPct New acceptance quorum
Expand Down Expand Up @@ -210,6 +228,7 @@ contract Voting is IForwarder, AragonApp {
address creator,
uint64 startDate,
uint256 snapshotBlock,
uint256 supportRequired,
uint256 minAcceptQuorum,
uint256 yea,
uint256 nay,
Expand All @@ -224,6 +243,7 @@ contract Voting is IForwarder, AragonApp {
creator = vote_.creator;
startDate = vote_.startDate;
snapshotBlock = vote_.snapshotBlock;
supportRequired = vote_.supportRequiredPct;
minAcceptQuorum = vote_.minAcceptQuorumPct;
yea = vote_.yea;
nay = vote_.nay;
Expand Down Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚨 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

if (!_isValuePct(vote_.yea, totalVotes, supportRequiredPct)) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, thanks!! I see that the test for it below is wrong too.

vote_.minAcceptQuorumPct = minAcceptQuorumPct;

emit StartVote(voteId);
Expand Down
55 changes: 47 additions & 8 deletions apps/voting/test/voting.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ contract('Voting App', accounts => {
let votingBase, daoFact, voting, token, executionTarget

let APP_MANAGER_ROLE
let CREATE_VOTES_ROLE, MODIFY_QUORUM_ROLE
let CREATE_VOTES_ROLE, MODIFY_SUPPORT_ROLE, MODIFY_QUORUM_ROLE

const votingTime = 1000
const root = accounts[0]
Expand All @@ -47,6 +47,7 @@ contract('Voting App', accounts => {
// Setup constants
APP_MANAGER_ROLE = await kernelBase.APP_MANAGER_ROLE()
CREATE_VOTES_ROLE = await votingBase.CREATE_VOTES_ROLE()
MODIFY_SUPPORT_ROLE = await votingBase.MODIFY_SUPPORT_ROLE()
MODIFY_QUORUM_ROLE = await votingBase.MODIFY_QUORUM_ROLE()
})

Expand All @@ -61,6 +62,7 @@ contract('Voting App', accounts => {
voting = Voting.at(receipt.logs.filter(l => l.event == 'NewAppProxy')[0].args.proxy)

await acl.createPermission(ANY_ADDR, voting.address, CREATE_VOTES_ROLE, root, { from: root })
await acl.createPermission(ANY_ADDR, voting.address, MODIFY_SUPPORT_ROLE, root, { from: root })
await acl.createPermission(ANY_ADDR, voting.address, MODIFY_QUORUM_ROLE, root, { from: root })
})

Expand Down Expand Up @@ -144,12 +146,32 @@ contract('Voting App', accounts => {
assert.equal(voteId, 0, 'voting should have been created')
})

it('can change required support', async () => {
const receipt = await voting.changeSupportRequiredPct(neededSupport.add(1))
const events = receipt.logs.filter(x => x.event == 'ChangeSupportRequired')

assert.equal(events.length, 1, 'should have emitted ChangeSupportRequired event')
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 () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: supoprt -> support

return assertRevert(async () => {
await voting.changeSupportRequiredPct(minimumAcceptanceQuorum.minus(1))
})
})

it('fails changing required support to greater than 100%', async () => {
return assertRevert(async () => {
await voting.changeSupportRequiredPct(pct16(101))
})
})

it('can change minimum acceptance quorum', async () => {
const receipt = await voting.changeMinAcceptQuorumPct(1)
const events = receipt.logs.filter(x => x.event == 'ChangeMinQuorum')

assert.equal(events.length, 1, 'should have emitted ChangeMinQuorum event')
assert.equal(await voting.minAcceptQuorumPct(), 1, 'should have change acceptance quorum')
assert.equal(await voting.minAcceptQuorumPct(), 1, 'should have changed acceptance quorum')
})

it('fails changing minimum acceptance quorum to zero', async () => {
Expand All @@ -174,12 +196,13 @@ contract('Voting App', accounts => {
})

it('has correct state', async () => {
const [isOpen, isExecuted, creator, startDate, snapshotBlock, minQuorum, y, n, totalVoters, execScript] = await voting.getVote(voteId)
const [isOpen, isExecuted, creator, startDate, snapshotBlock, supportRequired, minQuorum, y, n, totalVoters, execScript] = await voting.getVote(voteId)

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')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems weird to need the deepEqual in the tests (here and elsewhere), since both supportRequired and minQuorum are numbers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree.

assert.deepEqual(minQuorum, minimumAcceptanceQuorum, 'min quorum should be app min quorum')
assert.equal(y, 0, 'initial yea should be 0')
assert.equal(n, 0, 'initial nay should be 0')
Expand All @@ -195,6 +218,22 @@ contract('Voting App', accounts => {
})
})

it('changing required support does not affect vote required support', async () => {
await voting.changeSupportRequiredPct(pct16(70))

// With previous required support at 50%, vote should be approved
// with new quorum at 70% it shouldn't have, but since min quorum is snapshotted
// it will succeed

await voting.vote(voteId, true, true, { from: holder50 })
await voting.vote(voteId, true, true, { from: holder19 })
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

await timeTravel(votingTime + 1)

const state = await voting.getVote(voteId)
assert.deepEqual(state[5], neededSupport, 'required support in vote should stay equal')
await voting.executeVote(voteId) // exec doesn't fail
})

it('changing min quorum doesnt affect vote min quorum', async () => {
await voting.changeMinAcceptQuorumPct(pct16(50))

Expand All @@ -207,7 +246,7 @@ contract('Voting App', accounts => {
await timeTravel(votingTime + 1)

const state = await voting.getVote(voteId)
assert.deepEqual(state[5], minimumAcceptanceQuorum, 'acceptance quorum in vote should stay equal')
assert.deepEqual(state[6], minimumAcceptanceQuorum, 'acceptance quorum in vote should stay equal')
await voting.executeVote(voteId) // exec doesn't fail
})

Expand All @@ -216,7 +255,7 @@ contract('Voting App', accounts => {
const state = await voting.getVote(voteId)
const voterState = await voting.getVoterState(voteId, holder31)

assert.equal(state[7], 31, 'nay vote should have been counted')
assert.equal(state[8], 31, 'nay vote should have been counted')
assert.equal(voterState, VOTER_STATE.NAY, 'holder31 should have nay voter status')
})

Expand All @@ -226,8 +265,8 @@ contract('Voting App', accounts => {
await voting.vote(voteId, true, true, { from: holder31 })
const state = await voting.getVote(voteId)

assert.equal(state[6], 31, 'yea vote should have been counted')
assert.equal(state[7], 0, 'nay vote should have been removed')
assert.equal(state[7], 31, 'yea vote should have been counted')
assert.equal(state[8], 0, 'nay vote should have been removed')
})

it('token transfers dont affect voting', async () => {
Expand All @@ -236,7 +275,7 @@ contract('Voting App', accounts => {
await voting.vote(voteId, true, true, { from: holder31 })
const state = await voting.getVote(voteId)

assert.equal(state[6], 31, 'yea vote should have been counted')
assert.equal(state[7], 31, 'yea vote should have been counted')
assert.equal(await token.balanceOf(holder31), 0, 'balance should be 0 at current block')
})

Expand Down