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

Docs: Improve Accessors and Manager contracts documentation #507

Merged
merged 6 commits into from
Feb 16, 2023
Merged

Docs: Improve Accessors and Manager contracts documentation #507

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
40307ef
Improve SimulateTxAccessor docs
mmv08 Feb 14, 2023
dd47954
improve FallbackManager docs
mmv08 Feb 14, 2023
c5414c5
add GuardManager docs
mmv08 Feb 14, 2023
1be7046
managers documentation wip
mmv08 Feb 15, 2023
ea319fd
module manager docs
mmv08 Feb 15, 2023
cc348ed
add ownermanager docs
mmv08 Feb 15, 2023
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
26 changes: 24 additions & 2 deletions contracts/accessors/SimulateTxAccessor.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,20 +3,42 @@ pragma solidity >=0.7.0 <0.9.0;

import "../base/Executor.sol";

/// @title Simulate Transaction Accessor - can be used with StorageAccessible to simulate Safe transactions
/// @author Richard Meissner - <[email protected]>
/**
* @title Simulate Transaction Accessor.
* @notice Can be used with StorageAccessible to simulate Safe transactions.
Copy link
Member

Choose a reason for hiding this comment

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

should this be @dev?
image

Copy link
Member Author

@mmv08 mmv08 Feb 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I had an internal debate on this one. I reasoned that the end user of this particular contract is a developer, so notice is the way to go. Also, the docs mention that @dev is for extra details and we have none

* @author Richard Meissner - @rmeissner
*/
contract SimulateTxAccessor is Executor {
address private immutable accessorSingleton;

constructor() {
accessorSingleton = address(this);
}

/**
* @notice Modifier to make a function callable via delegatecall only.
* If the function is called via a regular call, it will revert.
*/
modifier onlyDelegateCall() {
require(address(this) != accessorSingleton, "SimulateTxAccessor should only be called via delegatecall");
_;
}

/**
* @notice Simulates a Safe transaction and returns the used gas, success boolean and the return data.
* @dev Executes the specified operation {Call, DelegateCall} and returns operation-specific data.
* Has to be called via delegatecall.
* This returns the data equal to `abi.encode(uint256(etimate), bool(success), bytes(returnData))`.
* Specifically, the returndata will be:
* `estimate:uint256 || success:bool || returnData.length:uint256 || returnData:bytes`.
* @param to Destination address .
* @param value Native token value.
* @param data Data payload.
* @param operation Operation type {Call, DelegateCall}.
* @return estimate Gas used.
* @return success Success boolean value.
* @return returnData Return data.
*/
function simulate(
address to,
uint256 value,
Expand Down
20 changes: 12 additions & 8 deletions contracts/base/Executor.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,19 @@
pragma solidity >=0.7.0 <0.9.0;
import "../common/Enum.sol";

/// @title Executor - A contract that can execute transactions
/// @author Richard Meissner - <[email protected]>
/**
* @title Executor - A contract that can execute transactions
* @author Richard Meissner - @rmeissner
*/
contract Executor {
/// @dev Executes either a delegatecall or a call with provided parameters
/// @param to Destination address.
/// @param value Ether value.
/// @param data Data payload.
/// @param operation Operation type.
/// @return success boolean flag indicating if the call succeeded
/**
* @notice Executes either a delegatecall or a call with provided parameters.
* @param to Destination address.
* @param value Ether value.
* @param data Data payload.
* @param operation Operation type.
* @return success boolean flag indicating if the call succeeded.
*/
function execute(
address to,
uint256 value,
Expand Down
21 changes: 15 additions & 6 deletions contracts/base/FallbackManager.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,20 @@ pragma solidity >=0.7.0 <0.9.0;

import "../common/SelfAuthorized.sol";

/// @title Fallback Manager - A contract that manages fallback calls made to this contract
/// @author Richard Meissner - <[email protected]>
/**
* @title Fallback Manager - A contract managing fallback calls made to this contract
* @author Richard Meissner - @rmeissner
*/
contract FallbackManager is SelfAuthorized {
event ChangedFallbackHandler(address handler);

// keccak256("fallback_manager.handler.address")
bytes32 internal constant FALLBACK_HANDLER_STORAGE_SLOT = 0x6c9a6c4a39284e37ed1cf53d337577d14212a4870fb976a4366c693b939918d5;

/**
* @notice Internal function to set the fallback handler.
* @param handler contract to handle fallback calls.
*/
function internalSetFallbackHandler(address handler) internal {
bytes32 slot = FALLBACK_HANDLER_STORAGE_SLOT;
// solhint-disable-next-line no-inline-assembly
Expand All @@ -19,15 +25,18 @@ contract FallbackManager is SelfAuthorized {
}
}

/// @dev Allows to add a contract to handle fallback calls.
/// Only fallback calls without value and with data will be forwarded.
/// This can only be done via a Safe transaction.
/// @param handler contract to handle fallback calls.
/**
* @notice Set Fallback Handler to `handler` for the Safe.
* @dev Only fallback calls without value and with data will be forwarded.
* This can only be done via a Safe transaction.
* @param handler contract to handle fallback calls.
*/
function setFallbackHandler(address handler) public authorized {
internalSetFallbackHandler(handler);
emit ChangedFallbackHandler(handler);
}

// @notice Forwards all calls to the fallback handler if set. Returns 0 if no handler is set.
// solhint-disable-next-line payable-fallback,no-complex-fallback
fallback() external {
bytes32 slot = FALLBACK_HANDLER_STORAGE_SLOT;
Expand Down
22 changes: 18 additions & 4 deletions contracts/base/GuardManager.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,15 +31,22 @@ abstract contract BaseGuard is Guard {
}
}

/// @title Guard Manager - A contract that manages transaction guards which perform pre and post-checks on execution by multisig owners
/// @author Richard Meissner - <[email protected]>
/**
* @title Guard Manager - A contract managing transaction guards which perform pre and post-checks on Safe transactions.
* @author Richard Meissner - @rmeissner
*/
contract GuardManager is SelfAuthorized {
event ChangedGuard(address guard);

// keccak256("guard_manager.guard.address")
bytes32 internal constant GUARD_STORAGE_SLOT = 0x4a204f620c8c5ccdca3fd54d003badd85ba500436a431f0cbda4f558c93c34c8;

/// @dev Set a guard that checks transactions before execution
/// @param guard The address of the guard to be used or the 0 address to disable the guard
/**
* @dev Set a guard that checks transactions before execution
* This can only be done via a Safe transaction.
* @notice Set Transaction Guard `guard` for the Safe.
* @param guard The address of the guard to be used or the 0 address to disable the guard
*/
function setGuard(address guard) external authorized {
if (guard != address(0)) {
require(Guard(guard).supportsInterface(type(Guard).interfaceId), "GS300");
Expand All @@ -52,6 +59,13 @@ contract GuardManager is SelfAuthorized {
emit ChangedGuard(guard);
}

/**
* @dev Internal method to retrieve the current guard
* We do not have a public method because we're short on bytecode size limit,
* to retrieve the guard address, one can use `getStorageAt` from `StorageAccessible` contract
* with the slot `GUARD_STORAGE_SLOT`
* @return guard The address of the guard
*/
function getGuard() internal view returns (address guard) {
bytes32 slot = GUARD_STORAGE_SLOT;
// solhint-disable-next-line no-inline-assembly
Expand Down
111 changes: 71 additions & 40 deletions contracts/base/ModuleManager.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,14 @@ import "../common/Enum.sol";
import "../common/SelfAuthorized.sol";
import "./Executor.sol";

/// @title Module Manager - A contract that manages modules that can execute transactions via this contract
/// @author Stefan George - <[email protected]>
/// @author Richard Meissner - <[email protected]>
/**
* @title Module Manager - A contract managing Safe modules
* @notice Modules are pluggable extensions to the Safe that can be added to the Safe by the owner.
Modules are a security risk since they can execute arbitrary transactions, so only trusted
and audited modules should be added to the Safe.
* @author Stefan George - @Georgi87
* @author Richard Meissner - @rmeissner
*/
contract ModuleManager is SelfAuthorized, Executor {
event EnabledModule(address module);
event DisabledModule(address module);
Expand All @@ -17,6 +22,12 @@ contract ModuleManager is SelfAuthorized, Executor {

mapping(address => address) internal modules;

/**
* @notice Setup function sets the initial storage of the contract.
* Optionally executes a delegate call to another contract to setup the modules.
* @param to Optional destination address of call to execute.
* @param data Optional data of call to execute.
*/
function setupModules(address to, bytes memory data) internal {
require(modules[SENTINEL_MODULES] == address(0), "GS100");
modules[SENTINEL_MODULES] = SENTINEL_MODULES;
Expand All @@ -27,10 +38,11 @@ contract ModuleManager is SelfAuthorized, Executor {
}
}

/// @dev Allows to add a module to the whitelist.
/// This can only be done via a Safe transaction.
/// @notice Enables the module `module` for the Safe.
/// @param module Module to be whitelisted.
/**
* @notice Enables the module `module` for the Safe.
* @dev This can only be done via a Safe transaction.
* @param module Module to be whitelisted.
*/
function enableModule(address module) public authorized {
// Module address cannot be null or sentinel.
require(module != address(0) && module != SENTINEL_MODULES, "GS101");
Expand All @@ -41,11 +53,12 @@ contract ModuleManager is SelfAuthorized, Executor {
emit EnabledModule(module);
}

/// @dev Allows to remove a module from the whitelist.
/// This can only be done via a Safe transaction.
/// @notice Disables the module `module` for the Safe.
/// @param prevModule Module that pointed to the module to be removed in the linked list
/// @param module Module to be removed.
/**
* @notice Disables the module `module` for the Safe.
* @dev This can only be done via a Safe transaction.
* @param prevModule Previous module in the modules linked list.
* @param module Module to be removed.
*/
function disableModule(address prevModule, address module) public authorized {
// Validate module address and check that it corresponds to module index.
require(module != address(0) && module != SENTINEL_MODULES, "GS101");
Expand All @@ -55,11 +68,15 @@ contract ModuleManager is SelfAuthorized, Executor {
emit DisabledModule(module);
}

/// @dev Allows a Module to execute a Safe transaction without any further confirmations.
/// @param to Destination address of module transaction.
/// @param value Ether value of module transaction.
/// @param data Data payload of module transaction.
/// @param operation Operation type of module transaction.
/**
* @notice Execute `operation` (0: Call, 1: DelegateCall) to `to` with `value` (Native Token)
* @dev Function is virtual to allow overriding for L2 singleton to emit an event for indexing.
* @param to Destination address of module transaction.
* @param value Ether value of module transaction.
* @param data Data payload of module transaction.
* @param operation Operation type of module transaction.
* @return success Boolean flag indicating if the call succeeded.
*/
function execTransactionFromModule(
address to,
uint256 value,
Expand All @@ -74,11 +91,15 @@ contract ModuleManager is SelfAuthorized, Executor {
else emit ExecutionFromModuleFailure(msg.sender);
}

/// @dev Allows a Module to execute a Safe transaction without any further confirmations and return data
/// @param to Destination address of module transaction.
/// @param value Ether value of module transaction.
/// @param data Data payload of module transaction.
/// @param operation Operation type of module transaction.
/**
* @notice Execute `operation` (0: Call, 1: DelegateCall) to `to` with `value` (Native Token) and return data
* @param to Destination address of module transaction.
* @param value Ether value of module transaction.
* @param data Data payload of module transaction.
* @param operation Operation type of module transaction.
* @return success Boolean flag indicating if the call succeeded.
* @return returnData Data returned by the call.
*/
function execTransactionFromModuleReturnData(
address to,
uint256 value,
Expand All @@ -102,19 +123,23 @@ contract ModuleManager is SelfAuthorized, Executor {
}
}

/// @dev Returns if an module is enabled
/// @return True if the module is enabled
/**
* @notice Returns if an module is enabled
* @return True if the module is enabled
*/
function isModuleEnabled(address module) public view returns (bool) {
return SENTINEL_MODULES != module && modules[module] != address(0);
}

/// @dev Returns array of modules.
/// If all entries fit into a single page, the next pointer will be 0x1.
/// If another page is present, next will be the last element of the returned array.
/// @param start Start of the page. Has to be a module or start pointer (0x1 address)
/// @param pageSize Maximum number of modules that should be returned. Has to be > 0
/// @return array Array of modules.
/// @return next Start of the next page.
/**
* @notice Returns an array of modules.
* If all entries fit into a single page, the next pointer will be 0x1.
* If another page is present, next will be the last element of the returned array.
* @param start Start of the page. Has to be a module or start pointer (0x1 address)
* @param pageSize Maximum number of modules that should be returned. Has to be > 0
* @return array Array of modules.
* @return next Start of the next page.
*/
function getModulesPaginated(address start, uint256 pageSize) external view returns (address[] memory array, address next) {
require(start == SENTINEL_MODULES || isModuleEnabled(start), "GS105");
require(pageSize > 0, "GS106");
Expand All @@ -130,13 +155,15 @@ contract ModuleManager is SelfAuthorized, Executor {
moduleCount++;
}

// Because of the argument validation we can assume that
// the `currentModule` will always be either a module address
// or sentinel address (aka the end). If we haven't reached the end
// inside the loop, we need to set the next pointer to the last element
// because it skipped over to the next module which is neither included
// in the current page nor won't be included in the next one
// if you pass it as a start.
/**
Because of the argument validation we can assume that
the `currentModule` will always be either a module address
or sentinel address (aka the end). If we haven't reached the end
inside the loop, we need to set the next pointer to the last element
because it skipped over to the next module which is neither included
in the current page nor won't be included in the next one
if you pass it as a start.
*/
if (next != SENTINEL_MODULES) {
next = array[moduleCount - 1];
}
Expand All @@ -147,8 +174,12 @@ contract ModuleManager is SelfAuthorized, Executor {
}
}

/// @dev Returns true if `account` is a contract.
/// @param account The address being queried
/**
* @notice Returns true if `account` is a contract.
* @dev This function will return false if invoked during the constructor of a contract,
* as the code is not actually created until after the constructor finishes.
* @param account The address being queried
*/
function isContract(address account) internal view returns (bool) {
uint256 size;
// solhint-disable-next-line no-inline-assembly
Expand Down
Loading