Skip to content

Commit

Permalink
Merge pull request aragon#458 from aragon/issue270
Browse files Browse the repository at this point in the history 
Voting: Allow to modify required support
  • Loading branch information
@bingen
bingen authored Sep 18, 2018
2 parents 3df5da9 + 7571bd0 commit 17abb2f
Show file tree
Hide file tree
Showing 2 changed files with 72 additions and 12 deletions.
25 changes: 23 additions & 2 deletions apps/voting/contracts/Voting.sol
View file
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`%
* @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 @@ -178,7 +196,7 @@ contract Voting is IForwarder, AragonApp {
}

// Voting is already decided
if (_isValuePct(vote_.yea, vote_.totalVoters, supportRequiredPct)) {
if (_isValuePct(vote_.yea, vote_.totalVoters, vote_.supportRequiredPct)) {
return true;
}

Expand All @@ -189,7 +207,7 @@ contract Voting is IForwarder, AragonApp {
return false;
}
// Has enough support?
if (!_isValuePct(vote_.yea, totalVotes, supportRequiredPct)) {
if (!_isValuePct(vote_.yea, totalVotes, vote_.supportRequiredPct)) {
return false;
}
// Has min quorum?
Expand All @@ -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;
vote_.minAcceptQuorumPct = minAcceptQuorumPct;

emit StartVote(voteId);
Expand Down
59 changes: 49 additions & 10 deletions apps/voting/test/voting.js
View file
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 support lower than minimum acceptance quorum', async () => {
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,13 +196,14 @@ 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(minQuorum, minimumAcceptanceQuorum, 'min quorum should be app min quorum')
assert.equal(supportRequired.toNumber(), neededSupport.toNumber(), 'required support should be app required support')
assert.equal(minQuorum.toNumber(), minimumAcceptanceQuorum.toNumber(), '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')
assert.equal(totalVoters, 100, 'total voters should be 100')
Expand All @@ -195,19 +218,35 @@ 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, false, { from: holder50 })
await voting.vote(voteId, true, false, { from: holder19 })
await voting.vote(voteId, false, false, { from: holder31 })
await timeTravel(votingTime + 1)

const state = await voting.getVote(voteId)
assert.equal(state[5].toNumber(), neededSupport.toNumber(), '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))

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


await voting.vote(voteId, true, true, { from: holder31 })
await timeTravel(votingTime + 1)

const state = await voting.getVote(voteId)
assert.deepEqual(state[5], minimumAcceptanceQuorum, 'acceptance quorum in vote should stay equal')
assert.equal(state[6].toNumber(), minimumAcceptanceQuorum.toNumber(), '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

0 comments on commit 17abb2f

Please sign in to comment.