diff --git a/contracts/accessors/SimulateTxAccessor.sol b/contracts/accessors/SimulateTxAccessor.sol index 97d5d68da..f1d2b61d6 100644 --- a/contracts/accessors/SimulateTxAccessor.sol +++ b/contracts/accessors/SimulateTxAccessor.sol @@ -16,7 +16,7 @@ contract SimulateTxAccessor is Executor { } /** - * @dev Modifier to make a function callable via delegatecall only. + * @notice Modifier to make a function callable via delegatecall only. * If the function is called via a regular call, it will revert. */ modifier onlyDelegateCall() { diff --git a/contracts/base/Executor.sol b/contracts/base/Executor.sol index 522487bbf..521cfa576 100644 --- a/contracts/base/Executor.sol +++ b/contracts/base/Executor.sol @@ -8,7 +8,7 @@ import "../common/Enum.sol"; */ contract Executor { /** - * @dev Executes either a delegatecall or a call with provided parameters. + * @notice Executes either a delegatecall or a call with provided parameters. * @param to Destination address. * @param value Ether value. * @param data Data payload. diff --git a/contracts/base/ModuleManager.sol b/contracts/base/ModuleManager.sol index 65fd2daa7..96f28a1b1 100644 --- a/contracts/base/ModuleManager.sol +++ b/contracts/base/ModuleManager.sol @@ -23,8 +23,8 @@ contract ModuleManager is SelfAuthorized, Executor { mapping(address => address) internal modules; /** - * @notice Setup function sets initial storage of the contract. - * Optionally can make a delegate call to another contract to setup the 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. */ diff --git a/contracts/base/OwnerManager.sol b/contracts/base/OwnerManager.sol index b012022d9..c849287c4 100644 --- a/contracts/base/OwnerManager.sol +++ b/contracts/base/OwnerManager.sol @@ -2,9 +2,13 @@ pragma solidity >=0.7.0 <0.9.0; import "../common/SelfAuthorized.sol"; -/// @title OwnerManager - Manages a set of owners and a threshold to perform actions. -/// @author Stefan George - -/// @author Richard Meissner - +/** + * @title OwnerManager - Manages Safe owners and a threshold to authorize transactions. + * @dev Uses a linked list to store the owners because the code generate by the solidity compiler + * is more efficient than using a dynamic array. + * @author Stefan George - @Georgi87 + * @author Richard Meissner - @rmeissner + */ contract OwnerManager is SelfAuthorized { event AddedOwner(address owner); event RemovedOwner(address owner); @@ -16,9 +20,11 @@ contract OwnerManager is SelfAuthorized { uint256 internal ownerCount; uint256 internal threshold; - /// @dev Setup function sets initial storage of contract. - /// @param _owners List of Safe owners. - /// @param _threshold Number of required confirmations for a Safe transaction. + /** + * @notice Sets the initial storage of the contract. + * @param _owners List of Safe owners. + * @param _threshold Number of required confirmations for a Safe transaction. + */ function setupOwners(address[] memory _owners, uint256 _threshold) internal { // Threshold can only be 0 at initialization. // Check ensures that setup function can only be called once. @@ -43,11 +49,12 @@ contract OwnerManager is SelfAuthorized { threshold = _threshold; } - /// @dev Allows to add a new owner to the Safe and update the threshold at the same time. - /// This can only be done via a Safe transaction. - /// @notice Adds the owner `owner` to the Safe and updates the threshold to `_threshold`. - /// @param owner New owner address. - /// @param _threshold New threshold. + /** + * @notice Adds the owner `owner` to the Safe and updates the threshold to `_threshold`. + * @dev This can only be done via a Safe transaction. + * @param owner New owner address. + * @param _threshold New threshold. + */ function addOwnerWithThreshold(address owner, uint256 _threshold) public authorized { // Owner address cannot be null, the sentinel or the Safe itself. require(owner != address(0) && owner != SENTINEL_OWNERS && owner != address(this), "GS203"); @@ -61,12 +68,13 @@ contract OwnerManager is SelfAuthorized { if (threshold != _threshold) changeThreshold(_threshold); } - /// @dev Allows to remove an owner from the Safe and update the threshold at the same time. - /// This can only be done via a Safe transaction. - /// @notice Removes the owner `owner` from the Safe and updates the threshold to `_threshold`. - /// @param prevOwner Owner that pointed to the owner to be removed in the linked list - /// @param owner Owner address to be removed. - /// @param _threshold New threshold. + /** + * @notice Removes the owner `owner` from the Safe and updates the threshold to `_threshold`. + * @dev This can only be done via a Safe transaction. + * @param prevOwner Owner that pointed to the owner to be removed in the linked list + * @param owner Owner address to be removed. + * @param _threshold New threshold. + */ function removeOwner(address prevOwner, address owner, uint256 _threshold) public authorized { // Only allow to remove an owner, if threshold can still be reached. require(ownerCount - 1 >= _threshold, "GS201"); @@ -81,12 +89,13 @@ contract OwnerManager is SelfAuthorized { if (threshold != _threshold) changeThreshold(_threshold); } - /// @dev Allows to swap/replace an owner from the Safe with another address. - /// This can only be done via a Safe transaction. - /// @notice Replaces the owner `oldOwner` in the Safe with `newOwner`. - /// @param prevOwner Owner that pointed to the owner to be replaced in the linked list - /// @param oldOwner Owner address to be replaced. - /// @param newOwner New owner address. + /** + * @notice Replaces the owner `oldOwner` in the Safe with `newOwner`. + * @dev This can only be done via a Safe transaction. + * @param prevOwner Owner that pointed to the owner to be replaced in the linked list + * @param oldOwner Owner address to be replaced. + * @param newOwner New owner address. + */ function swapOwner(address prevOwner, address oldOwner, address newOwner) public authorized { // Owner address cannot be null, the sentinel or the Safe itself. require(newOwner != address(0) && newOwner != SENTINEL_OWNERS && newOwner != address(this), "GS203"); @@ -102,10 +111,11 @@ contract OwnerManager is SelfAuthorized { emit AddedOwner(newOwner); } - /// @dev Allows to update the number of required confirmations by Safe owners. - /// This can only be done via a Safe transaction. - /// @notice Changes the threshold of the Safe to `_threshold`. - /// @param _threshold New threshold. + /** + * @notice Changes the threshold of the Safe to `_threshold`. + * @dev This can only be done via a Safe transaction. + * @param _threshold New threshold. + */ function changeThreshold(uint256 _threshold) public authorized { // Validate that threshold is smaller than number of owners. require(_threshold <= ownerCount, "GS201"); @@ -115,16 +125,26 @@ contract OwnerManager is SelfAuthorized { emit ChangedThreshold(threshold); } + /** + * @notice Returns the number of required confirmations for a Safe transaction aka the threshold. + * @return Threshold number. + */ function getThreshold() public view returns (uint256) { return threshold; } + /** + * @notice Returns if `owner` is an owner of the Safe. + * @return Boolean if owner is an owner of the Safe. + */ function isOwner(address owner) public view returns (bool) { return owner != SENTINEL_OWNERS && owners[owner] != address(0); } - /// @dev Returns array of owners. - /// @return Array of Safe owners. + /** + * @notice Returns a list of Safe owners. + * @return Array of Safe owners. + */ function getOwners() public view returns (address[] memory) { address[] memory array = new address[](ownerCount); diff --git a/test/accessors/SimulateTxAccessor.spec.ts b/test/accessors/SimulateTxAccessor.spec.ts index d2f557a58..c573a7b4a 100644 --- a/test/accessors/SimulateTxAccessor.spec.ts +++ b/test/accessors/SimulateTxAccessor.spec.ts @@ -51,11 +51,8 @@ describe("SimulateTxAccessor", async () => { expect(await hre.ethers.provider.getCode(accessor.address)).to.be.eq(code); }); - it.only("simulate call", async () => { + it("simulate call", async () => { const { safe, accessor, simulator } = await setupTests(); - console.log("safe address: ", safe.address); - console.log("simulator address: ", simulator.address); - console.log("accessor address: ", accessor.address); const tx = buildContractCall(safe, "getOwners", [], 0); const simulationData = accessor.interface.encodeFunctionData("simulate", [tx.to, tx.value, tx.data, tx.operation]); const acccessibleData = await simulator.callStatic.simulate(accessor.address, simulationData);