Skip to content

Commit

Permalink
chore: fix pr comments from ermyas
Browse files Browse the repository at this point in the history
  • Loading branch information
sujithsomraaj committed Aug 28, 2023
1 parent ef8a957 commit 2e07a09
Show file tree
Hide file tree
Showing 5 changed files with 73 additions and 36 deletions.
8 changes: 4 additions & 4 deletions src/MultiMessageReceiver.sol
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,8 @@ contract MultiMessageReceiver is IMultiMessageReceiver, ExecutorAware, Initializ
INITIALIZER
////////////////////////////////////////////////////////////////*/

/// @notice sets the initial paramters
function initialize(address[] calldata _receiverAdapters, uint64 _quorum, address _governaneTimelock)
/// @notice sets the initial parameters
function initialize(address[] calldata _receiverAdapters, uint64 _quorum, address _governanceTimelock)
external
initializer
{
Expand All @@ -84,12 +84,12 @@ contract MultiMessageReceiver is IMultiMessageReceiver, ExecutorAware, Initializ
revert Error.INVALID_QUORUM_THRESHOLD();
}

if (_governaneTimelock == address(0)) {
if (_governanceTimelock == address(0)) {
revert Error.ZERO_GOVERNANCE_TIMELOCK();
}

quorum = _quorum;
governanceTimelock = _governaneTimelock;
governanceTimelock = _governanceTimelock;
}

/*/////////////////////////////////////////////////////////////////
Expand Down
70 changes: 47 additions & 23 deletions src/controllers/GovernanceTimelock.sol
Original file line number Diff line number Diff line change
Expand Up @@ -14,17 +14,18 @@ contract GovernanceTimelock is IGovernanceTimelock {
uint256 public constant MINIMUM_DELAY = 2 days;
uint256 public constant MAXIMUM_DELAY = 14 days;

uint256 public constant MINIMUM_EXECUTION_PERIOD = 1 days;
uint256 public constant MAXIMUM_EXECUTION_PERIOD = 3 days;

/*/////////////////////////////////////////////////////////////////
STATE VARIABLES
////////////////////////////////////////////////////////////////*/
uint256 public txCounter;
uint256 public delay = MINIMUM_DELAY;

/// @dev the admin should be multi-message receiver
address public multiMessageReceiver;
uint256 public execPeriod = MINIMUM_EXECUTION_PERIOD;
address public admin; /// @dev the admin is the one allowed to schedule events

mapping(uint256 txId => ScheduledTransaction) public scheduledTransaction;
mapping(uint256 txId => bool executed) public isExecuted;

/*/////////////////////////////////////////////////////////////////
MODIFIERS
Expand All @@ -38,10 +39,10 @@ contract GovernanceTimelock is IGovernanceTimelock {
_;
}

/// @notice A modifier used for restricting caller to mma receiver contract
modifier onlyMultiMessageReceiver() {
if (msg.sender != multiMessageReceiver) {
revert Error.CALLER_NOT_MULTI_MESSAGE_RECEIVER();
/// @notice A modifier used for restricting caller to admin contract
modifier onlyAdmin() {
if (msg.sender != admin) {
revert Error.CALLER_NOT_ADMIN();
}
_;
}
Expand All @@ -50,13 +51,14 @@ contract GovernanceTimelock is IGovernanceTimelock {
CONSTRUCTOR
////////////////////////////////////////////////////////////////*/

/// @param _multiMessageReceiver is the address of multiMessageReceiver
constructor(address _multiMessageReceiver) {
if (_multiMessageReceiver == address(0)) {
/// @param _admin is the address of admin contract that schedule txs
constructor(address _admin) {
if (_admin == address(0)) {
revert Error.ZERO_ADDRESS_INPUT();
}

multiMessageReceiver = _multiMessageReceiver;
admin = _admin;
emit AdminUpdated(address(0), _admin);
}

/*/////////////////////////////////////////////////////////////////
Expand All @@ -67,7 +69,7 @@ contract GovernanceTimelock is IGovernanceTimelock {
function scheduleTransaction(address _target, uint256 _value, bytes memory _data)
external
override
onlyMultiMessageReceiver
onlyAdmin
{
if (_target == address(0)) {
revert Error.INVALID_TARGET();
Expand All @@ -76,10 +78,11 @@ contract GovernanceTimelock is IGovernanceTimelock {
/// increment tx counter
++txCounter;
uint256 eta = block.timestamp + delay;
uint256 expiry = eta + execPeriod;

scheduledTransaction[txCounter] = ScheduledTransaction(_target, _value, _data, eta);
scheduledTransaction[txCounter] = ScheduledTransaction(_target, _value, _data, eta, expiry, false);

emit TransactionScheduled(txCounter, _target, _value, _data, eta);
emit TransactionScheduled(txCounter, _target, _value, _data, eta, expiry);
}

/// @inheritdoc IGovernanceTimelock
Expand All @@ -89,27 +92,32 @@ contract GovernanceTimelock is IGovernanceTimelock {
revert Error.INVALID_TX_ID();
}

ScheduledTransaction storage transaction = scheduledTransaction[txId];

/// @dev checks if tx is already executed;
if (isExecuted[txId]) {
if (transaction.isExecuted) {
revert Error.TX_ALREADY_EXECUTED();
}

ScheduledTransaction memory transaction = scheduledTransaction[txId];

/// @dev checks timelock
if (transaction.eta < block.timestamp) {
revert Error.TX_TIMELOCKED();
revert Error.TX_EXPIRED();
}

isExecuted[txId] = true;
/// @dev checks if tx within execution period
if(block.timestamp > transaction.expiry) {
revert Error.TX_EXPIRED();
}

transaction.isExecuted = true;

(bool status,) = transaction.target.call(transaction.data);

if (!status) {
revert Error.EXECUTION_FAILS_ON_DST();
}

emit TransactionExecuted(txId, transaction.target, transaction.value, transaction.data, transaction.eta);
emit TransactionExecuted(txId, transaction.target, transaction.value, transaction.data, transaction.eta, transaction.expiry);
}

/// @inheritdoc IGovernanceTimelock
Expand All @@ -128,14 +136,30 @@ contract GovernanceTimelock is IGovernanceTimelock {
emit DelayUpdated(oldDelay, _delay);
}

/// @inheritdoc IGovernanceTimelock
function setExecutionPeriod(uint256 _period) external override onlySelf {
if (_period < MINIMUM_EXECUTION_PERIOD) {
revert Error.INVALID_EXEC_PERIOD_MIN();
}

if (_period > MAXIMUM_EXECUTION_PERIOD) {
revert Error.INVALID_EXEC_PERIOD_MAX();
}

uint256 oldExecPeriod = execPeriod;
execPeriod = _period;

emit ExecutionPeriodUpdated(oldExecPeriod, _period);
}

/// @inheritdoc IGovernanceTimelock
function setAdmin(address _newAdmin) external override onlySelf {
if (_newAdmin == address(0)) {
revert Error.ZERO_TIMELOCK_ADMIN();
}

address oldAdmin = multiMessageReceiver;
multiMessageReceiver = _newAdmin;
address oldAdmin = admin;
admin = _newAdmin;

emit AdminUpdated(oldAdmin, _newAdmin);
}
Expand Down
11 changes: 9 additions & 2 deletions src/interfaces/IGovernanceTimelock.sol
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,17 @@ interface IGovernanceTimelock {
uint256 value;
bytes data;
uint256 eta;
uint256 expiry;
bool isExecuted;
}

/*/////////////////////////////////////////////////////////////////
EVENTS
////////////////////////////////////////////////////////////////*/
event TransactionScheduled(uint256 txId, address target, uint256 value, bytes data, uint256 eta);
event TransactionExecuted(uint256 txId, address target, uint256 value, bytes data, uint256 eta);
event TransactionScheduled(uint256 indexed txId, address target, uint256 value, bytes data, uint256 eta, uint256 expiry);
event TransactionExecuted(uint256 indexed txId, address target, uint256 value, bytes data, uint256 eta, uint256 expiry);

event ExecutionPeriodUpdated(uint256 oldPeriod, uint256 newPeriod);
event DelayUpdated(uint256 oldDelay, uint256 newDelay);
event AdminUpdated(address oldAdmin, address newAdmin);

Expand All @@ -41,6 +44,10 @@ interface IGovernanceTimelock {
/// @dev This can only be invoked by through this timelock contract, thus requiring that an update go through the required time delay first.
function setDelay(uint256 _delay) external;

/// @notice Updates the period after timelock where the transaction can be executed
/// @dev This can only be executed by through this timelock contract, thus requiring that an update go through the required time delay first.
function setExecutionPeriod(uint256 _period) external;

/// @notice Updates the admin.
/// @dev This can only be invoked by through this timelock contract, thus requiring that an update go through the required time delay first.
function setAdmin(address newAdmin) external;
Expand Down
2 changes: 1 addition & 1 deletion src/interfaces/IMultiMessageReceiver.sol
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ interface IMultiMessageReceiver {
event SingleBridgeMsgReceived(
bytes32 indexed msgId, string indexed bridgeName, uint256 nonce, address receiverAdapter
);
event MessageExecuted(bytes32 msgId, address target, uint256 nonce, bytes callData);
event MessageExecuted(bytes32 indexed msgId, address target, uint256 nonce, bytes callData);

/*/////////////////////////////////////////////////////////////////
EXTERNAL FUNCTIONS
Expand Down
18 changes: 12 additions & 6 deletions src/libraries/Error.sol
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,6 @@ library Error {
/// @dev is thrown if deadline is lapsed
error MSG_EXECUTION_PASSED_DEADLINE();

/// @dev is thrown if message is still in timelock period
error MSG_STILL_TIMELOCKED();

/// @dev is thrown if quorum is not reached
error INVALID_QUORUM_FOR_EXECUTION();

Expand All @@ -62,16 +59,16 @@ library Error {
/// @dev is thrown if caller is not governance timelock contract
error CALLER_NOT_GOVERNANCE_TIMELOCK();

/// @dev is thrown if caller is not admin of timelock
error CALLER_NOT_ADMIN();

/*/////////////////////////////////////////////////////////////////
ADAPTER ERRORS
////////////////////////////////////////////////////////////////*/

/// @dev is thrown if caller is not multi message sender
error CALLER_NOT_MULTI_MESSAGE_SENDER();

/// @dev is thrown if caller is not multi message receiver
error CALLER_NOT_MULTI_MESSAGE_RECEIVER();

/// @dev is thrown if sender chain is not allowed on reciever adapter
error INVALID_SENDER_CHAIN_ID();

Expand Down Expand Up @@ -139,6 +136,12 @@ library Error {
/// @dev is thrown if the delay is more than maximum delay
error INVALID_DELAY_MAX();

/// @dev is thrown if the execution period is less than minimum execution period
error INVALID_EXEC_PERIOD_MIN();

/// @dev is thrown if the execution period is more than maximum execution period
error INVALID_EXEC_PERIOD_MAX();

/// @dev is thrown if the new admin is zero
error ZERO_TIMELOCK_ADMIN();

Expand All @@ -153,4 +156,7 @@ library Error {

/// @dev is thrown if timelock period is not over
error TX_TIMELOCKED();

/// @dev is thrown if transaction is expired
error TX_EXPIRED();
}

0 comments on commit 2e07a09

Please sign in to comment.