Add whitelists and guardians roles #29
Conversation
Pull Request Test Coverage Report for Build 11447656675Details
💛 - Coveralls |
contracts/CompoundGovernor.sol
Outdated
| /// @param _newWhitelistGuardian The address of the new whitelistGuardian. | ||
| /// @dev Only the executor (timelock) can call this function. | ||
| function setWhitelistGuardian(address _newWhitelistGuardian) public { | ||
| if (msg.sender != _executor()) { |
There was a problem hiding this comment.
Should I use the stricter _checkGovernance instead which checks if a proposal has actually passed beyond checking the msg.sender?
There was a problem hiding this comment.
@garyghayrat good call, that seems idiomatic. let me know if you want clarification on the implications.
garyghayrat
changed the title
| event WhitelistGuardianSet(address oldGuardian, address newGuardian); | ||
|
|
||
| /// @notice Emitted when the proposalGuardian is set | ||
| /// @notice Emitted when the proposal guardian is set or updated. |
There was a problem hiding this comment.
Nit: duplicate line to above?
|
|
||
| /// @notice Account which has the ability to cancel proposals. This privilege expires at the given expiration | ||
| /// timestamp. | ||
| ProposalGuardian public proposalGuardian; |
There was a problem hiding this comment.
Nit: not that the compiler cares, but easier reading if a struct is defined before it is referenced.
There was a problem hiding this comment.
Moved it up in 42b14e9
There was a problem hiding this comment.
great start @garyghayrat -- i was imagining that the propose/cancel functionality would be changed here to work with the whitelist. were you thinking of adding it separately? I see you mention #28, but that issue looks like it's specifically for threshold-motivated cancellation
contracts/CompoundGovernor.sol
Outdated
| /// @param _newWhitelistGuardian The address of the new whitelistGuardian. | ||
| /// @dev Only the executor (timelock) can call this function. | ||
| function setWhitelistGuardian(address _newWhitelistGuardian) public { | ||
| if (msg.sender != _executor()) { |
There was a problem hiding this comment.
@garyghayrat good call, that seems idiomatic. let me know if you want clarification on the implications.
garyghayrat
force-pushed
the
feat/add-whitelists-and-guardians
branch
from
dad40b8 to
77964d3
Compare
| contract CompoundGovernorSetWhitelistAccountExpirationTest is ProposalTest { | ||
| function testFuzz_WhitelistAnAccountAsTimelock(address _account, uint256 _expiration) public { | ||
| vm.prank(TIMELOCK_ADDRESS); | ||
| governor.setWhitelistAccountExpiration(_account, _expiration); |
There was a problem hiding this comment.
just making sure i understand, we're not passing a proposal here because we don't use _checkGovernance here, correct?
There was a problem hiding this comment.
you raise a good point actually. The current check we have is
if (msg.sender != _executor() && msg.sender != whitelistGuardian) {
revert Unauthorized("Not timelock or guardian", msg.sender);
}I can change it to be like
if (msg.sender != whitelistGuardian) {
_checkGovernance();
}But then we will only get the error GovernorOnlyExecutor, instead one that indicates that the msg.sender can also be a whitelistGuardian. What do you think?
There was a problem hiding this comment.
i'm fine leaving this as is
* Add whitelist accounts and whitelist guardian * Add `proposalGuardian` * Update natspec, function visibility, and test * Assume caller is not proxy admin in tests * Use `_checkGovernance` * Use `@notice` in natspec * Rename test and assume any caller * Assume _caller is not PROXY_ADMIN_ADDRESS * Move `PROPOSAL_GUARDIAN_EXPIRY` into test helper
closes #10. The rest of the logic will be addressed in #28.