Skip to content

Commit

Permalink
[SC-463] Refactor to simplify into 1 contract and interface (#19)
Browse files Browse the repository at this point in the history 
* refactor to simplify into 1 contract and interface

* refactor guardian to be part of access control, also queue becomes a separate role

* formatting
  • Loading branch information
@hexonaut
hexonaut authored Jun 14, 2024
1 parent 02c3abc commit 4336079
Show file tree
Hide file tree
Showing 12 changed files with 238 additions and 354 deletions.
13 changes: 5 additions & 8 deletions README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,11 @@ Executors serve as admins of the bridged domain instances of the protocol. <br>T
![Architecture Diagram](/diagram.png)
## 🤝 Contribution Guidelines
In order to add governance relay infrastructure for a new domain, perform the following steps:
1. Go to [XChain Helpers](https://github.com/marsfoundation/xchain-helpers) repository and add:
1. A domain helper abstracting away the process of passing messages between host domain and your bridged domain.
2. A helper function to the XChainForwarders library abstracting away host domain interactions with the bridge.
2. Add `BridgeExecutor` to the `/src/executors` directory. Follow currently used naming convention.
3. Add a new test file for your domain to the `/test` directory. Inherit `CrosschainTestBase` and add tests specific to your domain to the test suite. All of the tests have to pass. Follow linting and naming convention used in other test files.
4. Use proper labeling for your open PR (always set adequate priority and status)
5. Get an approving review from at least one of three designated reviewers - **@hexonaut**, **@lucas-manuel** or **@barrutko**
6. Enjoy governance messages being passed through the bridge to your domain! 🎉
1. Go to [XChain Helpers](https://github.com/marsfoundation/xchain-helpers) repository and complete the process for adding a new domain.
2. Add a new test file for your domain to the `/test` directory. Inherit `CrosschainTestBase` and add tests specific to your domain to the test suite. All of the tests have to pass. Follow linting and naming convention used in other test files.
3. Use proper labeling for your open PR (always set adequate priority and status)
4. Get an approving review from at least one of three designated reviewers - **@hexonaut**, **@lucas-manuel** or **@barrutko**
5. Enjoy governance messages being passed through the bridge to your domain! 🎉

***
*The IP in this repository was assigned to Mars SPC Limited in respect of the MarsOne SP*
220 changes: 92 additions & 128 deletions src/executors/BridgeExecutorBase.sol → src/Executor.sol
View file
Original file line number Diff line number Diff line change
@@ -1,30 +1,30 @@
// SPDX-License-Identifier: AGPL-3.0
pragma solidity ^0.8.10;

import { Address } from "lib/openzeppelin-contracts/contracts/utils/Address.sol";
import { AccessControl } from "openzeppelin-contracts/contracts/access/AccessControl.sol";
import { Address } from "openzeppelin-contracts/contracts/utils/Address.sol";

import { IExecutorBase } from 'src/interfaces/IExecutorBase.sol';
import { IExecutor } from './interfaces/IExecutor.sol';

/**
* @title BridgeExecutorBase
* @title Executor
* @author Aave
* @notice Abstract contract that implements basic executor functionality
* @dev It does not implement an external `queue` function. This should instead be done in the inheriting
* contract with proper access control
* @notice Executor which queues up message calls and executes them after an optional delay
*/
abstract contract BridgeExecutorBase is IExecutorBase {
contract Executor is IExecutor, AccessControl {

using Address for address;

bytes32 public constant SUBMISSION_ROLE = keccak256('SUBMISSION_ROLE');
bytes32 public constant GUARDIAN_ROLE = keccak256('GUARDIAN_ROLE');

// Minimum allowed grace period, which reduces the risk of having an actions set expire due to network congestion
uint256 constant MINIMUM_GRACE_PERIOD = 10 minutes;

// Time between queuing and execution
uint256 private _delay;
// Time after the execution time during which the actions set can be executed
uint256 private _gracePeriod;
// Address with the ability of canceling actions sets
address private _guardian;

// Number of actions sets
uint256 private _actionsSetCounter;
Expand All @@ -33,44 +33,91 @@ abstract contract BridgeExecutorBase is IExecutorBase {
// Map of queued actions (actionHash => isQueued)
mapping(bytes32 => bool) private _queuedActions;

/**
* @dev Only guardian can call functions marked by this modifier.
**/
modifier onlyGuardian() {
if (msg.sender != _guardian) revert NotGuardian();
_;
}

/**
* @dev Only this contract can call functions marked by this modifier.
**/
modifier onlyThis() {
if (msg.sender != address(this)) revert OnlyCallableByThis();
_;
}

/**
* @dev Constructor
*
* @param delay The delay before which an actions set can be executed
* @param gracePeriod The time period after a delay during which an actions set can be executed
* @param guardian The address of the guardian, which can cancel queued proposals (can be zero)
*/
constructor(
uint256 delay,
uint256 gracePeriod,
address guardian
uint256 gracePeriod
) {
if (
gracePeriod < MINIMUM_GRACE_PERIOD
) revert InvalidInitParams();

_updateDelay(delay);
_updateGracePeriod(gracePeriod);
_updateGuardian(guardian);

_setRoleAdmin(SUBMISSION_ROLE, DEFAULT_ADMIN_ROLE);
_setRoleAdmin(GUARDIAN_ROLE, DEFAULT_ADMIN_ROLE);

_grantRole(DEFAULT_ADMIN_ROLE, msg.sender);
_grantRole(DEFAULT_ADMIN_ROLE, address(this)); // Necessary for self-referential calls to change configuration
}

/// @inheritdoc IExecutor
function queue(
address[] memory targets,
uint256[] memory values,
string[] memory signatures,
bytes[] memory calldatas,
bool[] memory withDelegatecalls
) external override onlyRole(SUBMISSION_ROLE) {
if (targets.length == 0) revert EmptyTargets();
uint256 targetsLength = targets.length;
if (
targetsLength != values.length ||
targetsLength != signatures.length ||
targetsLength != calldatas.length ||
targetsLength != withDelegatecalls.length
) revert InconsistentParamsLength();

uint256 actionsSetId = _actionsSetCounter;
uint256 executionTime = block.timestamp + _delay;
unchecked {
++_actionsSetCounter;
}

for (uint256 i = 0; i < targetsLength; ) {
bytes32 actionHash = keccak256(
abi.encode(
targets[i],
values[i],
signatures[i],
calldatas[i],
executionTime,
withDelegatecalls[i]
)
);
if (isActionQueued(actionHash)) revert DuplicateAction();
_queuedActions[actionHash] = true;
unchecked {
++i;
}
}

ActionsSet storage actionsSet = _actionsSets[actionsSetId];
actionsSet.targets = targets;
actionsSet.values = values;
actionsSet.signatures = signatures;
actionsSet.calldatas = calldatas;
actionsSet.withDelegatecalls = withDelegatecalls;
actionsSet.executionTime = executionTime;

emit ActionsSetQueued(
actionsSetId,
targets,
values,
signatures,
calldatas,
withDelegatecalls,
executionTime
);
}

/// @inheritdoc IExecutorBase
/// @inheritdoc IExecutor
function execute(uint256 actionsSetId) external payable override {
if (getCurrentState(actionsSetId) != ActionsSetState.Queued) revert OnlyQueuedActions();

Expand Down Expand Up @@ -98,8 +145,8 @@ abstract contract BridgeExecutorBase is IExecutorBase {
emit ActionsSetExecuted(actionsSetId, msg.sender, returnedData);
}

/// @inheritdoc IExecutorBase
function cancel(uint256 actionsSetId) external override onlyGuardian {
/// @inheritdoc IExecutor
function cancel(uint256 actionsSetId) external override onlyRole(GUARDIAN_ROLE) {
if (getCurrentState(actionsSetId) != ActionsSetState.Queued) revert OnlyQueuedActions();

ActionsSet storage actionsSet = _actionsSets[actionsSetId];
Expand All @@ -123,57 +170,47 @@ abstract contract BridgeExecutorBase is IExecutorBase {
emit ActionsSetCanceled(actionsSetId);
}

/// @inheritdoc IExecutorBase
function updateGuardian(address guardian) external override onlyThis {
_updateGuardian(guardian);
}

/// @inheritdoc IExecutorBase
function updateDelay(uint256 delay) external override onlyThis {
/// @inheritdoc IExecutor
function updateDelay(uint256 delay) external override onlyRole(DEFAULT_ADMIN_ROLE) {
_updateDelay(delay);
}

/// @inheritdoc IExecutorBase
function updateGracePeriod(uint256 gracePeriod) external override onlyThis {
/// @inheritdoc IExecutor
function updateGracePeriod(uint256 gracePeriod) external override onlyRole(DEFAULT_ADMIN_ROLE) {
if (gracePeriod < MINIMUM_GRACE_PERIOD) revert GracePeriodTooShort();
_updateGracePeriod(gracePeriod);
}

/// @inheritdoc IExecutorBase
/// @inheritdoc IExecutor
function executeDelegateCall(address target, bytes calldata data)
external
payable
override
onlyThis
onlyRole(DEFAULT_ADMIN_ROLE)
returns (bytes memory)
{
return target.functionDelegateCall(data);
}

/// @inheritdoc IExecutorBase
/// @inheritdoc IExecutor
function receiveFunds() external payable override {}

/// @inheritdoc IExecutorBase
/// @inheritdoc IExecutor
function getDelay() external view override returns (uint256) {
return _delay;
}

/// @inheritdoc IExecutorBase
/// @inheritdoc IExecutor
function getGracePeriod() external view override returns (uint256) {
return _gracePeriod;
}

/// @inheritdoc IExecutorBase
function getGuardian() external view override returns (address) {
return _guardian;
}

/// @inheritdoc IExecutorBase
/// @inheritdoc IExecutor
function getActionsSetCount() external view override returns (uint256) {
return _actionsSetCounter;
}

/// @inheritdoc IExecutorBase
/// @inheritdoc IExecutor
function getActionsSetById(uint256 actionsSetId)
external
view
Expand All @@ -183,7 +220,7 @@ abstract contract BridgeExecutorBase is IExecutorBase {
return _actionsSets[actionsSetId];
}

/// @inheritdoc IExecutorBase
/// @inheritdoc IExecutor
function getCurrentState(uint256 actionsSetId) public view override returns (ActionsSetState) {
if (_actionsSetCounter <= actionsSetId) revert InvalidActionsSetId();
ActionsSet storage actionsSet = _actionsSets[actionsSetId];
Expand All @@ -198,16 +235,11 @@ abstract contract BridgeExecutorBase is IExecutorBase {
}
}

/// @inheritdoc IExecutorBase
/// @inheritdoc IExecutor
function isActionQueued(bytes32 actionHash) public view override returns (bool) {
return _queuedActions[actionHash];
}

function _updateGuardian(address guardian) internal {
emit GuardianUpdate(_guardian, guardian);
_guardian = guardian;
}

function _updateDelay(uint256 delay) internal {
emit DelayUpdate(_delay, delay);
_delay = delay;
Expand All @@ -218,74 +250,6 @@ abstract contract BridgeExecutorBase is IExecutorBase {
_gracePeriod = gracePeriod;
}

/**
* @notice Queue an ActionsSet
* @dev If a signature is empty, calldata is used for the execution, calldata is appended to signature otherwise
* @param targets Array of targets to be called by the actions set
* @param values Array of values to pass in each call by the actions set
* @param signatures Array of function signatures to encode in each call (can be empty)
* @param calldatas Array of calldata to pass in each call (can be empty)
* @param withDelegatecalls Array of whether to delegatecall for each call
**/
function _queue(
address[] memory targets,
uint256[] memory values,
string[] memory signatures,
bytes[] memory calldatas,
bool[] memory withDelegatecalls
) internal {
if (targets.length == 0) revert EmptyTargets();
uint256 targetsLength = targets.length;
if (
targetsLength != values.length ||
targetsLength != signatures.length ||
targetsLength != calldatas.length ||
targetsLength != withDelegatecalls.length
) revert InconsistentParamsLength();

uint256 actionsSetId = _actionsSetCounter;
uint256 executionTime = block.timestamp + _delay;
unchecked {
++_actionsSetCounter;
}

for (uint256 i = 0; i < targetsLength; ) {
bytes32 actionHash = keccak256(
abi.encode(
targets[i],
values[i],
signatures[i],
calldatas[i],
executionTime,
withDelegatecalls[i]
)
);
if (isActionQueued(actionHash)) revert DuplicateAction();
_queuedActions[actionHash] = true;
unchecked {
++i;
}
}

ActionsSet storage actionsSet = _actionsSets[actionsSetId];
actionsSet.targets = targets;
actionsSet.values = values;
actionsSet.signatures = signatures;
actionsSet.calldatas = calldatas;
actionsSet.withDelegatecalls = withDelegatecalls;
actionsSet.executionTime = executionTime;

emit ActionsSetQueued(
actionsSetId,
targets,
values,
signatures,
calldatas,
withDelegatecalls,
executionTime
);
}

function _executeTransaction(
address target,
uint256 value,
Expand Down
Loading

0 comments on commit 4336079

Please sign in to comment.