Skip to content

Commit

Permalink
Merge pull request #39 from aragon/feature/add-auth-to-execute
Browse files Browse the repository at this point in the history 
feat: add auth to execute() and update Setup & testing
  • Loading branch information
@novaknole
novaknole authored Nov 1, 2024
2 parents 8e9b423 + a5e7dca commit 427a7f5
Show file tree
Hide file tree
Showing 6 changed files with 236 additions and 18 deletions.
8 changes: 6 additions & 2 deletions packages/contracts/src/MajorityVotingBase.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -229,9 +229,13 @@ abstract contract MajorityVotingBase is
bytes32 public constant UPDATE_VOTING_SETTINGS_PERMISSION_ID =
keccak256("UPDATE_VOTING_SETTINGS_PERMISSION");

/// @notice The ID of the permission required to call the `addAddresses` and `removeAddresses` functions.
/// @notice The ID of the permission required to call the `createProposal` functions.
bytes32 public constant CREATE_PROPOSAL_PERMISSION_ID = keccak256("CREATE_PROPOSAL_PERMISSION");

/// @notice The ID of the permission required to call the `execute` function.
bytes32 public constant EXECUTE_PROPOSAL_PERMISSION_ID =
keccak256("EXECUTE_PROPOSAL_PERMISSION");

/// @notice A mapping between proposal IDs and proposal information.
// solhint-disable-next-line named-parameters-mapping
mapping(uint256 => Proposal) internal proposals;
Expand Down Expand Up @@ -362,7 +366,7 @@ abstract contract MajorityVotingBase is
}

/// @inheritdoc IMajorityVoting
function execute(uint256 _proposalId) public virtual {
function execute(uint256 _proposalId) public virtual auth(EXECUTE_PROPOSAL_PERMISSION_ID) {
if (!_canExecute(_proposalId)) {
revert ProposalExecutionForbidden(_proposalId);
}
Expand Down
9 changes: 8 additions & 1 deletion packages/contracts/src/TokenVoting.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -263,7 +263,14 @@ contract TokenVoting is IMembership, MajorityVotingBase {
votingPower: votingPower
});

if (_tryEarlyExecution && _canExecute(_proposalId)) {
if (!_tryEarlyExecution) {
return;
}

if (
_canExecute(_proposalId) &&
dao().hasPermission(address(this), _voter, EXECUTE_PROPOSAL_PERMISSION_ID, _msgData())
) {
_execute(_proposalId);
}
}
Expand Down
45 changes: 38 additions & 7 deletions packages/contracts/src/TokenVotingSetup.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,13 @@ contract TokenVotingSetup is PluginUpgradeableSetup {
/// @notice The ID of the permission required to call the `upgradeToAndCall` function.
bytes32 internal constant UPGRADE_PLUGIN_PERMISSION_ID = keccak256("UPGRADE_PLUGIN_PERMISSION");

/// @notice The ID of the permission required to call the `execute` function.
bytes32 internal constant EXECUTE_PROPOSAL_PERMISSION_ID =
keccak256("EXECUTE_PROPOSAL_PERMISSION");

/// @notice A special address encoding permissions that are valid for any address `who` or `where`.
address internal constant ANY_ADDR = address(type(uint160).max);

/// @notice The address of the `TokenVoting` base contract.
// solhint-disable-next-line immutable-vars-naming
TokenVoting private immutable tokenVotingBase;
Expand Down Expand Up @@ -175,7 +182,7 @@ contract TokenVotingSetup is PluginUpgradeableSetup {
// Prepare permissions
PermissionLib.MultiTargetPermission[]
memory permissions = new PermissionLib.MultiTargetPermission[](
tokenSettings.addr != address(0) ? 5 : 6
tokenSettings.addr != address(0) ? 6 : 7
);

// Set plugin permissions to be granted.
Expand All @@ -200,7 +207,7 @@ contract TokenVotingSetup is PluginUpgradeableSetup {
permissions[2] = PermissionLib.MultiTargetPermission(
PermissionLib.Operation.GrantWithCondition,
plugin,
address(type(uint160).max), // ANY_ADDR
ANY_ADDR,
preparedSetupData.helpers[0], // VotingPowerCondition
TokenVoting(IMPLEMENTATION).CREATE_PROPOSAL_PERMISSION_ID()
);
Expand All @@ -221,10 +228,18 @@ contract TokenVotingSetup is PluginUpgradeableSetup {
permissionId: SET_METADATA_PERMISSION_ID
});

permissions[5] = PermissionLib.MultiTargetPermission({
operation: PermissionLib.Operation.Grant,
where: plugin,
who: ANY_ADDR,
condition: PermissionLib.NO_CONDITION,
permissionId: EXECUTE_PROPOSAL_PERMISSION_ID
});

if (tokenSettings.addr == address(0)) {
bytes32 tokenMintPermission = GovernanceERC20(token).MINT_PERMISSION_ID();

permissions[5] = PermissionLib.MultiTargetPermission({
permissions[6] = PermissionLib.MultiTargetPermission({
operation: PermissionLib.Operation.Grant,
where: token,
who: _dao,
Expand All @@ -251,7 +266,7 @@ contract TokenVotingSetup is PluginUpgradeableSetup {
address votingPowerCondition = address(new VotingPowerCondition(_payload.plugin));

PermissionLib.MultiTargetPermission[]
memory permissions = new PermissionLib.MultiTargetPermission[](4);
memory permissions = new PermissionLib.MultiTargetPermission[](5);

permissions[0] = PermissionLib.MultiTargetPermission({
operation: PermissionLib.Operation.Revoke,
Expand All @@ -264,7 +279,7 @@ contract TokenVotingSetup is PluginUpgradeableSetup {
permissions[1] = PermissionLib.MultiTargetPermission(
PermissionLib.Operation.GrantWithCondition,
_payload.plugin,
address(type(uint160).max), // ANY_ADDR
ANY_ADDR, // ANY_ADDR
votingPowerCondition,
TokenVoting(IMPLEMENTATION).CREATE_PROPOSAL_PERMISSION_ID()
);
Expand All @@ -285,6 +300,14 @@ contract TokenVotingSetup is PluginUpgradeableSetup {
permissionId: SET_METADATA_PERMISSION_ID
});

permissions[4] = PermissionLib.MultiTargetPermission({
operation: PermissionLib.Operation.Grant,
where: _payload.plugin,
who: ANY_ADDR,
condition: PermissionLib.NO_CONDITION,
permissionId: EXECUTE_PROPOSAL_PERMISSION_ID
});

preparedSetupData.permissions = permissions;
preparedSetupData.helpers = new address[](1);
preparedSetupData.helpers[0] = votingPowerCondition;
Expand All @@ -299,7 +322,7 @@ contract TokenVotingSetup is PluginUpgradeableSetup {
SetupPayload calldata _payload
) external view returns (PermissionLib.MultiTargetPermission[] memory permissions) {
// Prepare permissions.
permissions = new PermissionLib.MultiTargetPermission[](5);
permissions = new PermissionLib.MultiTargetPermission[](6);

// Set permissions to be Revoked.
permissions[0] = PermissionLib.MultiTargetPermission({
Expand Down Expand Up @@ -337,10 +360,18 @@ contract TokenVotingSetup is PluginUpgradeableSetup {
permissions[4] = PermissionLib.MultiTargetPermission({
operation: PermissionLib.Operation.Revoke,
where: _payload.plugin,
who: address(type(uint160).max), // ANY_ADDR
who: ANY_ADDR, // ANY_ADDR
condition: PermissionLib.NO_CONDITION,
permissionId: TokenVoting(IMPLEMENTATION).CREATE_PROPOSAL_PERMISSION_ID()
});

permissions[5] = PermissionLib.MultiTargetPermission({
operation: PermissionLib.Operation.Revoke,
where: _payload.plugin,
who: ANY_ADDR,
condition: PermissionLib.NO_CONDITION,
permissionId: EXECUTE_PROPOSAL_PERMISSION_ID
});
}

/// @notice Unsatisfiably determines if the token is an IVotes interface.
Expand Down
130 changes: 130 additions & 0 deletions packages/contracts/test/10_unit-testing/11_plugin.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import {
import {
TOKEN_VOTING_INTERFACE,
UPDATE_VOTING_SETTINGS_PERMISSION_ID,
EXECUTE_PROPOSAL_PERMISSION_ID,
INITIALIZE_SIGNATURE,
INITIALIZE_SIGNATURE_OLD,
Operation,
Expand Down Expand Up @@ -198,6 +199,11 @@ async function globalFixture(): Promise<GlobalFixtureResult> {
deployer
);

// Grant ANY_ADDR the permission to execute proposals
await dao
.connect(deployer)
.grant(initializedPlugin.address, ANY_ADDR, EXECUTE_PROPOSAL_PERMISSION_ID);

// Grant deployer the permission to update the voting settings
await dao
.connect(deployer)
Expand Down Expand Up @@ -2148,6 +2154,71 @@ describe('TokenVoting', function () {
.to.be.revertedWithCustomError(plugin, 'ProposalExecutionForbidden')
.withArgs(id);
});

it('can not execute even if participation and support are met when caller does not have permission', async () => {
const {
alice,
bob,
carol,
dave,
eve,
frank,
grace,
initializedPlugin: plugin,
dummyMetadata,
dummyActions,
dao,
} = await loadFixture(localFixture);

const endDate = (await time.latest()) + TIME.DAY;
const id = await createProposalId(
plugin.address,
dummyActions,
dummyMetadata
);

// Create a proposal.
await plugin[CREATE_PROPOSAL_SIGNATURE](
dummyMetadata,
dummyActions,
0,
0,
endDate,
VoteOption.None,
false
);

// Vote with enough voters so that the execution criteria are met.
await voteWithSigners(plugin, id, {
yes: [alice, bob, carol], // 30 votes
no: [dave, eve], // 20 votes
abstain: [frank, grace], // 20 votes
});

// Wait until the vote is over.
await time.increaseTo(endDate);

// Check that the proposal can be executed.
expect(await plugin.isSupportThresholdReached(id)).to.be.true;
expect(await plugin.isMinParticipationReached(id)).to.be.true;
expect(await plugin.canExecute(id)).to.equal(true);

// Revoke execute permission from ANY_ADDR
await dao.revoke(
plugin.address,
ANY_ADDR,
EXECUTE_PROPOSAL_PERMISSION_ID
);

await expect(plugin.connect(alice).execute(id))
.to.be.revertedWithCustomError(plugin, 'DaoUnauthorized')
.withArgs(
dao.address,
plugin.address,
alice.address,
EXECUTE_PROPOSAL_PERMISSION_ID
);
});
});

describe('Early Execution', async () => {
Expand Down Expand Up @@ -2646,6 +2717,65 @@ describe('TokenVoting', function () {
.to.be.revertedWithCustomError(plugin, 'ProposalExecutionForbidden')
.withArgs(id);
});

it('record vote correctly without executing even when tryEarlyExecution options is selected', async () => {
const {
alice,
bob,
carol,
dave,
eve,
frank,
grace,
dao,
initializedPlugin: plugin,
dummyMetadata,
dummyActions,
} = await loadFixture(localFixture);

// Create a Proposal.
const endDate = (await time.latest()) + TIME.DAY;
const id = await createProposalId(
plugin.address,
dummyActions,
dummyMetadata
);

await plugin[CREATE_PROPOSAL_SIGNATURE](
dummyMetadata,
dummyActions,
0,
0,
endDate,
VoteOption.None,
false
);

// Vote 40 votes for `Yes`. The proposal can still get defeated if the remaining 60 votes vote for `No`.
await voteWithSigners(plugin, id, {
yes: [alice, bob, carol, dave, eve], // 50 votes
no: [], // 0 votes
abstain: [], // 0 votes
});

// Check that the proposal cannot be early executed and didn't execute yet.
expect((await plugin.getProposal(id)).executed).to.equal(false);
expect(await plugin.canExecute(id)).to.equal(false);

// Revoke execute permission from ANY_ADDR
await dao.revoke(
plugin.address,
ANY_ADDR,
EXECUTE_PROPOSAL_PERMISSION_ID
);

// Vote `Yes` with Frank with `tryEarlyExecution` being turned on.
// The vote is decided now, but proposal should not be executed yet.
await plugin.connect(frank).vote(id, VoteOption.Yes, true);
// Check that the proposal can be executed but didn't execute yet.
expect((await plugin.getProposal(id)).executed).to.equal(false);
expect(await plugin.canExecute(id)).to.equal(true);
});
});

describe('Vote Replacement', async () => {
Expand Down
Loading

0 comments on commit 427a7f5

Please sign in to comment.