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

Add explicit getters for TimelockAuthorizer #1298

Merged
merged 8 commits into from
May 20, 2022
Merged

Add explicit getters for TimelockAuthorizer #1298

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
4f69fd7
refactor: make root storage variable private
TomAFrench May 12, 2022
14a2595
refactor: make `delayerPerActionId` storage variable private
TomAFrench May 12, 2022
1f0dd9b
refactor: make scheduledExecutions storage variable private
TomAFrench May 12, 2022
c3fcaf0
refactor: make isPermissionGranted storage variable private
TomAFrench May 12, 2022
70b6aab
style: linting
TomAFrench May 12, 2022
0d95703
docs: fix typo
TomAFrench May 20, 2022
4d55d4a
refactor: use getRootTransferDelay getter rather than using variable …
TomAFrench May 20, 2022
df2fb94
refactor: rename isPermissionGranted to isPermissionGrantedOnTarget
TomAFrench May 20, 2022
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
93 changes: 65 additions & 28 deletions pkg/vault/contracts/authorizer/TimelockAuthorizer.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
// along with this program. If not, see <http://www.gnu.org/licenses/>.

pragma solidity ^0.7.0;
pragma experimental ABIEncoderV2;

import "@balancer-labs/v2-interfaces/contracts/solidity-utils/helpers/BalancerErrors.sol";
import "@balancer-labs/v2-interfaces/contracts/solidity-utils/helpers/IAuthentication.sol";
Expand Down Expand Up @@ -77,10 +78,10 @@ contract TimelockAuthorizer is IAuthorizer, IAuthentication {
IAuthentication private immutable _vault;
uint256 private immutable _rootTransferDelay;

address public root;
ScheduledExecution[] public scheduledExecutions;
mapping(bytes32 => bool) public isPermissionGranted;
mapping(bytes32 => uint256) public delaysPerActionId;
address private _root;
ScheduledExecution[] private _scheduledExecutions;
mapping(bytes32 => bool) private _isPermissionGranted;
mapping(bytes32 => uint256) private _delaysPerActionId;

/**
* @dev Emitted when a new execution `scheduledExecutionId` is scheduled.
Expand Down Expand Up @@ -127,7 +128,7 @@ contract TimelockAuthorizer is IAuthorizer, IAuthentication {
IAuthentication vault,
uint256 rootTransferDelay
) {
root = admin;
_root = admin;
_vault = vault;
_executor = new TimelockExecutor();
_rootTransferDelay = rootTransferDelay;
Expand All @@ -148,7 +149,7 @@ contract TimelockAuthorizer is IAuthorizer, IAuthentication {
* @dev Returns true if `account` is the root.
*/
function isRoot(address account) public view returns (bool) {
return account == root;
return account == _root;
}

/**
Expand All @@ -172,6 +173,13 @@ contract TimelockAuthorizer is IAuthorizer, IAuthentication {
return address(_executor);
}

/**
* @dev Returns the root address.
*/
function getRoot() external view returns (address) {
return _root;
}

/**
* @dev Returns the action ID for function selector `selector`.
*/
Expand All @@ -186,6 +194,13 @@ contract TimelockAuthorizer is IAuthorizer, IAuthentication {
return keccak256(abi.encodePacked(actionId, how));
}

/**
* @dev Returns the execution delay for action `actionId`.
*/
function getActionIdDelay(bytes32 actionId) external view returns (uint256) {
return _delaysPerActionId[actionId];
}

/**
* @dev Returns the permission ID for action `actionId`, account `account` and target `where`.
*/
Expand All @@ -197,6 +212,21 @@ contract TimelockAuthorizer is IAuthorizer, IAuthentication {
return keccak256(abi.encodePacked(actionId, account, where));
}

/**
* @dev Returns true if `account` has the permission defined by action `actionId` and target `where`.
* This function is specific for the strict permission defined by the tuple `(actionId, where)`, `account` may also
* hold the global permission for the action `actionId` which would allow them to perform this action on `where`.
* For this reason, it's recommended to use `hasPermission` if checking whether `account` is allowed to perform
* a given action.
*/
function isPermissionGrantedOnTarget(
bytes32 actionId,
address account,
address where
) external view returns (bool) {
return _isPermissionGranted[permissionId(actionId, account, where)];
}

/**
* @dev Returns true if `account` is allowed to perform action `actionId` in target `where`.
*/
Expand All @@ -206,8 +236,8 @@ contract TimelockAuthorizer is IAuthorizer, IAuthentication {
address where
) public view returns (bool) {
return
isPermissionGranted[permissionId(actionId, account, where)] ||
isPermissionGranted[permissionId(actionId, account, EVERYWHERE)];
_isPermissionGranted[permissionId(actionId, account, where)] ||
_isPermissionGranted[permissionId(actionId, account, EVERYWHERE)];
}

/**
Expand Down Expand Up @@ -241,7 +271,7 @@ contract TimelockAuthorizer is IAuthorizer, IAuthentication {
address where
) public view override returns (bool) {
return
(delaysPerActionId[actionId] > 0) ? account == address(_executor) : hasPermission(actionId, account, where);
_delaysPerActionId[actionId] > 0 ? account == address(_executor) : hasPermission(actionId, account, where);
}

/**
Expand All @@ -266,13 +296,20 @@ contract TimelockAuthorizer is IAuthorizer, IAuthentication {
return _canPerformOrWhatever(REVOKE_ACTION_ID, account, where, actionId);
}

/**
* @dev Returns the scheduled execution `scheduledExecutionId`.
*/
function getScheduledExecution(uint256 scheduledExecutionId) external view returns (ScheduledExecution memory) {
return _scheduledExecutions[scheduledExecutionId];
}

/**
* @dev Returns true if execution `scheduledExecutionId` can be executed.
* Only true if it is not already executed or cancelled, and if the execution delay has passed.
*/
function canExecute(uint256 scheduledExecutionId) external view returns (bool) {
require(scheduledExecutionId < scheduledExecutions.length, "ACTION_DOES_NOT_EXIST");
ScheduledExecution storage scheduledExecution = scheduledExecutions[scheduledExecutionId];
require(scheduledExecutionId < _scheduledExecutions.length, "ACTION_DOES_NOT_EXIST");
ScheduledExecution storage scheduledExecution = _scheduledExecutions[scheduledExecutionId];
return
!scheduledExecution.executed &&
!scheduledExecution.cancelled &&
Expand All @@ -284,7 +321,7 @@ contract TimelockAuthorizer is IAuthorizer, IAuthentication {
* @dev Sets the root address to `newRoot`.
*/
function setRoot(address newRoot) external onlyExecutor {
TomAFrench marked this conversation as resolved.
Show resolved Hide resolved
root = newRoot;
_root = newRoot;
emit RootSet(newRoot);
}

Expand All @@ -299,18 +336,18 @@ contract TimelockAuthorizer is IAuthorizer, IAuthentication {
bytes32 actionId = getActionId(this.setRoot.selector);
bytes32 scheduleRootChangeActionId = getActionId(SCHEDULE_DELAY_ACTION_ID, actionId);
bytes memory data = abi.encodeWithSelector(this.setRoot.selector, newRoot);
return _scheduleWithDelay(scheduleRootChangeActionId, address(this), data, _rootTransferDelay, executors);
return _scheduleWithDelay(scheduleRootChangeActionId, address(this), data, getRootTransferDelay(), executors);
}

/**
* @dev Sets a new delay `delay` for action `actionId`.
*/
function setDelay(bytes32 actionId, uint256 delay) external onlyExecutor {
bytes32 setAuthorizerActionId = _vault.getActionId(IVault.setAuthorizer.selector);
bool isAllowed = actionId == setAuthorizerActionId || delay <= delaysPerActionId[setAuthorizerActionId];
bool isAllowed = actionId == setAuthorizerActionId || delay <= _delaysPerActionId[setAuthorizerActionId];
require(isAllowed, "DELAY_EXCEEDS_SET_AUTHORIZER");

delaysPerActionId[actionId] = delay;
_delaysPerActionId[actionId] = delay;
emit ActionDelaySet(actionId, delay);
}

Expand All @@ -330,7 +367,7 @@ contract TimelockAuthorizer is IAuthorizer, IAuthentication {
// critical, as otherwise it'd be possible to execute an action with a delay shorter than its current one
// by first changing it to a smaller (or zero) value.

uint256 actionDelay = delaysPerActionId[actionId];
uint256 actionDelay = _delaysPerActionId[actionId];
bytes32 scheduleDelayActionId = getActionId(SCHEDULE_DELAY_ACTION_ID, actionId);
bytes memory data = abi.encodeWithSelector(this.setDelay.selector, actionId, newDelay);
return _scheduleWithDelay(scheduleDelayActionId, address(this), data, actionDelay, executors);
Expand All @@ -354,8 +391,8 @@ contract TimelockAuthorizer is IAuthorizer, IAuthentication {
* @dev Executes a scheduled action `scheduledExecutionId`.
*/
function execute(uint256 scheduledExecutionId) external returns (bytes memory result) {
require(scheduledExecutionId < scheduledExecutions.length, "ACTION_DOES_NOT_EXIST");
ScheduledExecution storage scheduledExecution = scheduledExecutions[scheduledExecutionId];
require(scheduledExecutionId < _scheduledExecutions.length, "ACTION_DOES_NOT_EXIST");
ScheduledExecution storage scheduledExecution = _scheduledExecutions[scheduledExecutionId];
require(!scheduledExecution.executed, "ACTION_ALREADY_EXECUTED");
require(!scheduledExecution.cancelled, "ACTION_ALREADY_CANCELLED");

Expand All @@ -376,8 +413,8 @@ contract TimelockAuthorizer is IAuthorizer, IAuthentication {
* @dev Cancels a scheduled action `scheduledExecutionId`.
*/
function cancel(uint256 scheduledExecutionId) external {
require(scheduledExecutionId < scheduledExecutions.length, "ACTION_DOES_NOT_EXIST");
ScheduledExecution storage scheduledExecution = scheduledExecutions[scheduledExecutionId];
require(scheduledExecutionId < _scheduledExecutions.length, "ACTION_DOES_NOT_EXIST");
ScheduledExecution storage scheduledExecution = _scheduledExecutions[scheduledExecutionId];

require(!scheduledExecution.executed, "ACTION_ALREADY_EXECUTED");
require(!scheduledExecution.cancelled, "ACTION_ALREADY_CANCELLED");
Expand Down Expand Up @@ -499,8 +536,8 @@ contract TimelockAuthorizer is IAuthorizer, IAuthentication {
address where
) private {
bytes32 permission = permissionId(actionId, account, where);
if (!isPermissionGranted[permission]) {
isPermissionGranted[permission] = true;
if (!_isPermissionGranted[permission]) {
_isPermissionGranted[permission] = true;
emit PermissionGranted(actionId, account, where);
}
}
Expand All @@ -511,8 +548,8 @@ contract TimelockAuthorizer is IAuthorizer, IAuthentication {
address where
) private {
bytes32 permission = permissionId(actionId, account, where);
if (isPermissionGranted[permission]) {
isPermissionGranted[permission] = false;
if (_isPermissionGranted[permission]) {
_isPermissionGranted[permission] = false;
emit PermissionRevoked(actionId, account, where);
}
}
Expand All @@ -523,7 +560,7 @@ contract TimelockAuthorizer is IAuthorizer, IAuthentication {
bytes memory data,
address[] memory executors
) private returns (uint256 scheduledExecutionId) {
uint256 delay = delaysPerActionId[actionId];
uint256 delay = _delaysPerActionId[actionId];
require(delay > 0, "CANNOT_SCHEDULE_ACTION");
return _scheduleWithDelay(actionId, where, data, delay, executors);
}
Expand All @@ -535,13 +572,13 @@ contract TimelockAuthorizer is IAuthorizer, IAuthentication {
uint256 delay,
address[] memory executors
) private returns (uint256 scheduledExecutionId) {
scheduledExecutionId = scheduledExecutions.length;
scheduledExecutionId = _scheduledExecutions.length;
emit ExecutionScheduled(actionId, scheduledExecutionId);

// solhint-disable-next-line not-rely-on-time
uint256 executableAt = block.timestamp + delay;
bool protected = executors.length > 0;
scheduledExecutions.push(ScheduledExecution(where, data, false, false, protected, executableAt));
_scheduledExecutions.push(ScheduledExecution(where, data, false, false, protected, executableAt));

bytes32 executeActionId = getActionId(EXECUTE_ACTION_ID, bytes32(scheduledExecutionId));
for (uint256 i = 0; i < executors.length; i++) {
Expand Down Expand Up @@ -569,7 +606,7 @@ contract TimelockAuthorizer is IAuthorizer, IAuthentication {
// If there is a delay defined for the granular action ID, then the sender must be the authorizer (scheduled
// execution)
bytes32 granularActionId = getActionId(actionId, how);
if (delaysPerActionId[granularActionId] > 0) {
if (_delaysPerActionId[granularActionId] > 0) {
return account == address(_executor);
}

Expand Down
20 changes: 10 additions & 10 deletions pkg/vault/test/authorizer/TimelockAuthorizer.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1925,7 +1925,7 @@ describe('TimelockAuthorizer', () => {
it('schedules a delay change', async () => {
const id = await authorizer.scheduleDelayChange(action, delay, [], { from: admin });

const scheduledExecution = await authorizer.scheduledExecutions(id);
const scheduledExecution = await authorizer.getScheduledExecution(id);
expect(scheduledExecution.executed).to.be.false;
expect(scheduledExecution.data).to.be.equal(expectedData);
expect(scheduledExecution.where).to.be.equal(authorizer.address);
Expand Down Expand Up @@ -1959,7 +1959,7 @@ describe('TimelockAuthorizer', () => {
it('schedules a delay change', async () => {
const id = await authorizer.scheduleDelayChange(action, delay, [], { from: admin });

const scheduledExecution = await authorizer.scheduledExecutions(id);
const scheduledExecution = await authorizer.getScheduledExecution(id);
expect(scheduledExecution.executed).to.be.false;
expect(scheduledExecution.data).to.be.equal(expectedData);
expect(scheduledExecution.where).to.be.equal(authorizer.address);
Expand Down Expand Up @@ -2073,7 +2073,7 @@ describe('TimelockAuthorizer', () => {
it('schedules a non-protected execution', async () => {
const id = await schedule();

const scheduledExecution = await authorizer.scheduledExecutions(id);
const scheduledExecution = await authorizer.getScheduledExecution(id);
expect(scheduledExecution.executed).to.be.false;
expect(scheduledExecution.data).to.be.equal(data);
expect(scheduledExecution.where).to.be.equal(where.address);
Expand All @@ -2093,7 +2093,7 @@ describe('TimelockAuthorizer', () => {
const receipt = await authorizer.execute(id);
expectEvent.inReceipt(await receipt.wait(), 'ExecutionExecuted', { scheduledExecutionId: id });

const scheduledExecution = await authorizer.scheduledExecutions(id);
const scheduledExecution = await authorizer.getScheduledExecution(id);
expect(scheduledExecution.executed).to.be.true;

expect(await vault.getAuthorizer()).to.be.equal(newAuthorizer.address);
Expand All @@ -2116,7 +2116,7 @@ describe('TimelockAuthorizer', () => {
it('schedules the requested execution', async () => {
const id = await schedule();

const scheduledExecution = await authorizer.scheduledExecutions(id);
const scheduledExecution = await authorizer.getScheduledExecution(id);
expect(scheduledExecution.executed).to.be.false;
expect(scheduledExecution.data).to.be.equal(data);
expect(scheduledExecution.where).to.be.equal(where.address);
Expand All @@ -2140,7 +2140,7 @@ describe('TimelockAuthorizer', () => {
const receipt = await authorizer.execute(id, { from: executors[0] });
expectEvent.inReceipt(await receipt.wait(), 'ExecutionExecuted', { scheduledExecutionId: id });

const scheduledExecution = await authorizer.scheduledExecutions(id);
const scheduledExecution = await authorizer.getScheduledExecution(id);
expect(scheduledExecution.executed).to.be.true;

expect(await vault.getAuthorizer()).to.be.equal(newAuthorizer.address);
Expand Down Expand Up @@ -2251,7 +2251,7 @@ describe('TimelockAuthorizer', () => {
it('executes the action', async () => {
await authorizer.execute(id, { from });

const scheduledExecution = await authorizer.scheduledExecutions(id);
const scheduledExecution = await authorizer.getScheduledExecution(id);
expect(scheduledExecution.executed).to.be.true;

expect(await vault.getAuthorizer()).to.be.equal(newAuthorizer.address);
Expand Down Expand Up @@ -2314,7 +2314,7 @@ describe('TimelockAuthorizer', () => {

await authorizer.execute(id);

const scheduledExecution = await authorizer.scheduledExecutions(id);
const scheduledExecution = await authorizer.getScheduledExecution(id);
expect(scheduledExecution.executed).to.be.true;

expect(await vault.getAuthorizer()).to.be.equal(newAuthorizer.address);
Expand Down Expand Up @@ -2364,7 +2364,7 @@ describe('TimelockAuthorizer', () => {
it('cancels the action', async () => {
await authorizer.cancel(id, { from });

const scheduledExecution = await authorizer.scheduledExecutions(id);
const scheduledExecution = await authorizer.getScheduledExecution(id);
expect(scheduledExecution.cancelled).to.be.true;
});

Expand Down Expand Up @@ -2434,7 +2434,7 @@ describe('TimelockAuthorizer', () => {

const id = await authorizer.scheduleRootChange(grantee, [], { from: admin });

const scheduledExecution = await authorizer.scheduledExecutions(id);
const scheduledExecution = await authorizer.getScheduledExecution(id);
expect(scheduledExecution.executed).to.be.false;
expect(scheduledExecution.data).to.be.equal(expectedData);
expect(scheduledExecution.where).to.be.equal(authorizer.address);
Expand Down
6 changes: 3 additions & 3 deletions pvt/helpers/src/models/authorizer/TimelockAuthorizer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,10 +61,10 @@ export default class TimelockAuthorizer {
}

async delay(action: string): Promise<BigNumberish> {
return this.instance.delaysPerActionId(action);
return this.instance.getActionIdDelay(action);
}

async scheduledExecutions(
async getScheduledExecution(
id: BigNumberish
): Promise<{
executed: boolean;
Expand All @@ -74,7 +74,7 @@ export default class TimelockAuthorizer {
data: string;
where: string;
}> {
return this.instance.scheduledExecutions(id);
return this.instance.getScheduledExecution(id);
}

async canPerform(action: string, account: Account, where: Account): Promise<boolean> {
Expand Down