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.

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

PolicyEnabler mix-in #40

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from
Open
Changes from 3 commits
Commits
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
104 changes: 104 additions & 0 deletions src/libraries/Enableable.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
// SPDX-License-Identifier: MIT
pragma solidity 0.8.15;

import {RolesConsumer} from "src/modules/ROLES/OlympusRoles.sol";

/// @title Enableable
/// @notice This contract is designed to be inherited by contracts that need to be enabled or disabled. It replaces the inconsistent usage of `active` and `locallyActive` state variables across the codebase.
/// @dev A contract that inherits from this contract should use the `whenEnabled` and `whenDisabled` modifiers to gate access to certain functions.
///
/// If custom logic and/or parameters are needed for the enable/disable functions, the inheriting contract can override the `_enable()` and `_disable()` functions. For example, `enable()` could be called with initialisation data that is decoded, validated and assigned in `_enable()`.
abstract contract Enableable is RolesConsumer {
0xJem marked this conversation as resolved.
Show resolved Hide resolved
// ===== STATE VARIABLES ===== //

bool public isEnabled;
bytes32 public constant ROLE = "emergency_shutdown";
0xJem marked this conversation as resolved.
Show resolved Hide resolved

// ===== ERRORS ===== //

error NotDisabled();
error NotEnabled();

// ===== EVENTS ===== //

event Disabled();
event Enabled();

// ===== MODIFIERS ===== //

modifier whenEnabled() {
if (!isEnabled) revert NotEnabled();
_;
}

modifier whenDisabled() {
if (isEnabled) revert NotDisabled();
_;
}

// ===== ENABLEABLE FUNCTIONS ===== //

/// @notice Enable the contract
/// @dev This function performs the following steps:
/// 1. Validates that the caller has `ROLE` ("emergency_shutdown")
0xJem marked this conversation as resolved.
Show resolved Hide resolved
/// 2. Validates that the contract is disabled
/// 3. Calls the implementation-specific `_enable()` function
/// 4. Changes the state of the contract to enabled
/// 5. Emits the `Enabled` event
///
/// @param enableData_ The data to pass to the implementation-specific `_enable()` function
function enable(bytes calldata enableData_) public onlyRole(ROLE) whenDisabled {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is can be pulled into all policies - I wonder if it'd be worth reducing bytecode by having a toggleIsEnabled() instead of distinct enable and disable functions?

The modifier could be also be a single:

    modifier whenEnabledIs(bool condition) {
        if (isEnabled == condition) revert IsEnabledNotMatching(condition);
        _;
    }

Tradeoff with being explicit - so don't mind, just throwing it out there (MonoCooler is kinda chunky which is why i bring up)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel that the simplicity would be lost, and it also adds ambiguity.

e.g. the whenEnabledIs(bool condition) modifier you shared is reverting if isEnabled == condition, but I would have interpreted it the other way - whenEnabledIs(true) would revert if isEnabled != true

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally fair and agree, thought i'd just throw it in the mix.

// Call the implementation-specific enable function
_enable(enableData_);

// Change the state
isEnabled = true;

// Emit the enabled event
emit Enabled();
}

/// @notice Implementation-specific enable function
/// @dev This function is called by the `enable()` function
///
/// The implementing contract can override this function and perform the following:
/// 1. Validate any parameters (if needed) or revert
/// 2. Validate state (if needed) or revert
/// 3. Perform any necessary actions, apart from modifying the `isEnabled` state variable
///
/// @param enableData_ Custom data that can be used by the implementation. The format of this data is
/// left to the discretion of the implementation.
function _enable(bytes calldata enableData_) internal virtual {}

/// @notice Disable the contract
/// @dev This function performs the following steps:
/// 1. Validates that the caller has `ROLE` ("emergency_shutdown")
/// 2. Validates that the contract is enabled
/// 3. Calls the implementation-specific `_disable()` function
/// 4. Changes the state of the contract to disabled
/// 5. Emits the `Disabled` event
///
/// @param disableData_ The data to pass to the implementation-specific `_disable()` function
function disable(bytes calldata disableData_) public onlyRole(ROLE) whenEnabled {
// Call the implementation-specific disable function
_disable(disableData_);

// Change the state
isEnabled = false;

// Emit the disabled event
emit Disabled();
}

/// @notice Implementation-specific disable function
/// @dev This function is called by the `disable()` function.
///
/// The implementing contract can override this function and perform the following:
/// 1. Validate any parameters (if needed) or revert
/// 2. Validate state (if needed) or revert
/// 3. Perform any necessary actions, apart from modifying the `isEnabled` state variable
///
/// @param disableData_ Custom data that can be used by the implementation. The format of this data is
/// left to the discretion of the implementation.
function _disable(bytes calldata disableData_) internal virtual {}
}
Loading