Completed ProtocolSettings review

contracts/modules/ProtocolSettings.sol

@@ -11,16 +11,35 @@ import "../events/ProtocolSettingsEvents.sol";
 import "../openzeppelin/SafeERC20.sol";
 import "../mixins/ProtocolTokenUser.sol";
 
+/**
+ * @title Protocol Settings contract.
+ *
+ * @notice This contract code comes from bZx. bZx is a protocol for tokenized
+ * margin trading and lending https://bzx.network similar to the dYdX protocol.
+ *
+ * This contract contains functions to customize protocol settings.
+ * */
 contract ProtocolSettings is State, ProtocolTokenUser, ProtocolSettingsEvents {
 	using SafeERC20 for IERC20;
 	using SafeMath for uint256;
 
+	/**
+	 * @notice Empty public constructor.
+	 * */
 	constructor() public {}
 
+	/**
+	 * @notice Fallback function is to react to receiving value (rBTC).
+	 * */
 	function() external {
 		revert("fallback not allowed");
 	}
 
+	/**
+	 * @notice Set function selectors on target contract.
+	 *
+	 * @param target The address of the target contract.
+	 * */
 	function initialize(address target) external onlyOwner {
 		_setTarget(this.setPriceFeedContract.selector, target);
 		_setTarget(this.setSwapsImplContract.selector, target);
@@ -50,21 +69,40 @@ contract ProtocolSettings is State, ProtocolTokenUser, ProtocolSettingsEvents {
 		_setTarget(this.setRebatePercent.selector, target);
 	}
 
+	/**
+	 * @notice Set the address of the Price Feed instance.
+	 *
+	 * @param newContract The address of the Price Feed new instance.
+	 * */
 	function setPriceFeedContract(address newContract) external onlyOwner {
 		address oldContract = priceFeeds;
 		priceFeeds = newContract;
 
 		emit SetPriceFeedContract(msg.sender, oldContract, newContract);
 	}
 
+	/**
+	 * @notice Set the address of the asset swapper instance.
+	 *
+	 * @param newContract The address of the asset swapper new instance.
+	 * */
 	function setSwapsImplContract(address newContract) external onlyOwner {
 		address oldContract = swapsImpl;
 		swapsImpl = newContract;
 
 		emit SetSwapsImplContract(msg.sender, oldContract, newContract);
 	}
 
-	function setLoanPool(address[] calldata pools, address[] calldata assets) external onlyOwner {
+	/**
+	 * @notice Set a list of loan pools and its tokens.
+	 *
+	 * @param pools The array of addresses of new loan pool instances.
+	 * @param assets The array of addresses of the corresponding underlying tokens.
+	 * */
+	function setLoanPool(
+		address[] calldata pools,
+		address[] calldata assets
+	) external onlyOwner {
 		require(pools.length == assets.length, "count mismatch");
 
 		for (uint256 i = 0; i < pools.length; i++) {
@@ -85,6 +123,14 @@ contract ProtocolSettings is State, ProtocolTokenUser, ProtocolSettingsEvents {
 		}
 	}
 
+	/**
+	 * @notice Set a list of supported tokens by populating the
+	 *   storage supportedTokens mapping.
+	 *
+	 * @param addrs The array of addresses of the tokens.
+	 * @param toggles The array of flags indicating whether
+	 *   the corresponding token is supported or not.
+	 * */
 	function setSupportedTokens(address[] calldata addrs, bool[] calldata toggles) external onlyOwner {
 		require(addrs.length == toggles.length, "count mismatch");
 
@@ -95,6 +141,11 @@ contract ProtocolSettings is State, ProtocolTokenUser, ProtocolSettingsEvents {
 		}
 	}
 
+	/**
+	 * @notice Set the value of lendingFeePercent storage variable.
+	 *
+	 * @param newValue The new value for lendingFeePercent.
+	 * */
 	function setLendingFeePercent(uint256 newValue) external onlyOwner {
 		require(newValue <= 10**20, "value too high");
 		uint256 oldValue = lendingFeePercent;
@@ -103,6 +154,11 @@ contract ProtocolSettings is State, ProtocolTokenUser, ProtocolSettingsEvents {
 		emit SetLendingFeePercent(msg.sender, oldValue, newValue);
 	}
 
+	/**
+	 * @notice Set the value of tradingFeePercent storage variable.
+	 *
+	 * @param newValue The new value for tradingFeePercent.
+	 * */
 	function setTradingFeePercent(uint256 newValue) external onlyOwner {
 		require(newValue <= 10**20, "value too high");
 		uint256 oldValue = tradingFeePercent;
@@ -111,6 +167,11 @@ contract ProtocolSettings is State, ProtocolTokenUser, ProtocolSettingsEvents {
 		emit SetTradingFeePercent(msg.sender, oldValue, newValue);
 	}
 
+	/**
+	 * @notice Set the value of borrowingFeePercent storage variable.
+	 *
+	 * @param newValue The new value for borrowingFeePercent.
+	 * */
 	function setBorrowingFeePercent(uint256 newValue) external onlyOwner {
 		require(newValue <= 10**20, "value too high");
 		uint256 oldValue = borrowingFeePercent;
@@ -119,6 +180,11 @@ contract ProtocolSettings is State, ProtocolTokenUser, ProtocolSettingsEvents {
 		emit SetBorrowingFeePercent(msg.sender, oldValue, newValue);
 	}
 
+	/**
+	 * @notice Set the value of affiliateFeePercent storage variable.
+	 *
+	 * @param newValue The new value for affiliateFeePercent.
+	 * */
 	function setAffiliateFeePercent(uint256 newValue) external onlyOwner {
 		require(newValue <= 10**20, "value too high");
 		uint256 oldValue = affiliateFeePercent;
@@ -127,6 +193,11 @@ contract ProtocolSettings is State, ProtocolTokenUser, ProtocolSettingsEvents {
 		emit SetAffiliateFeePercent(msg.sender, oldValue, newValue);
 	}
 
+	/**
+	 * @notice Set the value of liquidationIncentivePercent storage variable.
+	 *
+	 * @param newValue The new value for liquidationIncentivePercent.
+	 * */
 	function setLiquidationIncentivePercent(uint256 newValue) external onlyOwner {
 		require(newValue <= 10**20, "value too high");
 		uint256 oldValue = liquidationIncentivePercent;
@@ -135,29 +206,64 @@ contract ProtocolSettings is State, ProtocolTokenUser, ProtocolSettingsEvents {
 		emit SetLiquidationIncentivePercent(msg.sender, oldValue, newValue);
 	}
 
+	/**
+	 * @notice Set the value of the maximum swap spread.
+	 *
+	 * @param newValue The new value for maxDisagreement.
+	 * */
 	function setMaxDisagreement(uint256 newValue) external onlyOwner {
 		maxDisagreement = newValue;
 	}
 
+	/**
+	 * @notice Set the value of the maximum source buffer.
+	 *
+	 * @dev To avoid rounding issues on the swap rate a small buffer is implemented.
+	 *
+	 * @param newValue The new value for the maximum source buffer.
+	 * */
 	function setSourceBuffer(uint256 newValue) external onlyOwner {
 		sourceBuffer = newValue;
 	}
 
+	/**
+	 * @notice Set the value of the swap size limit.
+	 *
+	 * @param newValue The new value for the maximum swap size.
+	 * */
 	function setMaxSwapSize(uint256 newValue) external onlyOwner {
 		uint256 oldValue = maxSwapSize;
 		maxSwapSize = newValue;
 
 		emit SetMaxSwapSize(msg.sender, oldValue, newValue);
 	}
 
+	/**
+	 * @notice Set the address of the feesController instance.
+	 *
+	 * @dev The fee sharing proxy must be the feesController of the
+	 * protocol contract. This allows the fee sharing proxy
+	 * to withdraw the fees.
+	 *
+	 * @param newController The new address of the feesController.
+	 * */
 	function setFeesController(address newController) external onlyOwner {
 		address oldController = feesController;
 		feesController = newController;
 
 		emit SetFeesController(msg.sender, oldController, newController);
 	}
 
-	function withdrawFees(address token, address receiver) external returns (uint256) {
+	/**
+	 * @notice The feesController calls this function to withdraw fees
+	 * from three sources: lending, trading and borrowing.
+	 *
+	 * @param token The address of the token instance.
+	 * @param receiver The address of the withdrawal recipient.
+	 *
+	 * @return The withdrawn amount.
+	 * */
+	 function withdrawFees(address token, address receiver) external returns (uint256) {
 		require(msg.sender == feesController, "unauthorized");
 
 		uint256 lendingBalance = lendingFeeTokensHeld[token];
@@ -190,6 +296,16 @@ contract ProtocolSettings is State, ProtocolTokenUser, ProtocolSettingsEvents {
 		return amount;
 	}
 
+	/**
+	 * @notice The feesController calls this function to withdraw fees
+	 * accrued from lending operations.
+	 *
+	 * @param token The address of the token instance.
+	 * @param receiver The address of the withdrawal recipient.
+	 * @param amount The amount of fees to get, ignored if greater than balance.
+	 *
+	 * @return Whether withdrawal was successful.
+	 * */
 	function withdrawLendingFees(
 		address token,
 		address receiver,
@@ -217,7 +333,17 @@ contract ProtocolSettings is State, ProtocolTokenUser, ProtocolSettingsEvents {
 		return true;
 	}
 
-	function withdrawTradingFees(
+	/**
+	 * @notice The feesController calls this function to withdraw fees
+	 * accrued from trading operations.
+	 *
+	 * @param token The address of the token instance.
+	 * @param receiver The address of the withdrawal recipient.
+	 * @param amount The amount of fees to get, ignored if greater than balance.
+	 *
+	 * @return Whether withdrawal was successful.
+	 * */
+	 function withdrawTradingFees(
 		address token,
 		address receiver,
 		uint256 amount
@@ -244,6 +370,16 @@ contract ProtocolSettings is State, ProtocolTokenUser, ProtocolSettingsEvents {
 		return true;
 	}
 
+	/**
+	 * @notice The feesController calls this function to withdraw fees
+	 * accrued from borrowing operations.
+	 *
+	 * @param token The address of the token instance.
+	 * @param receiver The address of the withdrawal recipient.
+	 * @param amount The amount of fees to get, ignored if greater than balance.
+	 *
+	 * @return Whether withdrawal was successful.
+	 * */
 	function withdrawBorrowingFees(
 		address token,
 		address receiver,
@@ -271,27 +407,61 @@ contract ProtocolSettings is State, ProtocolTokenUser, ProtocolSettingsEvents {
 		return true;
 	}
 
+	/**
+	 * @notice The owner calls this function to withdraw protocol tokens.
+	 *
+	 * @dev Wrapper for ProtocolTokenUser::_withdrawProtocolToken internal function.
+	 *
+	 * @param receiver The address of the withdrawal recipient.
+	 * @param amount The amount of tokens to get.
+	 *
+	 * @return The protocol token address.
+	 * @return Withdrawal success (true/false).
+	 * */
 	function withdrawProtocolToken(address receiver, uint256 amount) external onlyOwner returns (address, bool) {
 		return _withdrawProtocolToken(receiver, amount);
 	}
 
+	/**
+	 * @notice The owner calls this function to deposit protocol tokens.
+	 *
+	 * @param amount The tokens of fees to send.
+	 * */
 	function depositProtocolToken(uint256 amount) external onlyOwner {
+		/// @dev Update local balance
 		protocolTokenHeld = protocolTokenHeld.add(amount);
 
+		/// @dev Send the tokens
 		IERC20(protocolTokenAddress).safeTransferFrom(msg.sender, address(this), amount);
 	}
 
+	/**
+	 * @notice Get a list of loan pools.
+	 *
+	 * @param start The offset.
+	 * @param count The limit.
+	 *
+	 * @return The array of loan pools.
+	 * */
 	function getLoanPoolsList(uint256 start, uint256 count) external view returns (bytes32[] memory) {
 		return loanPoolsSet.enumerate(start, count);
 	}
 
+	/**
+	 * @notice Check whether a token is a pool token.
+	 *
+	 * @dev By querying its underlying token.
+	 *
+	 * @param loanPool The token address to check.
+	 * */
 	function isLoanPool(address loanPool) external view returns (bool) {
 		return loanPoolToUnderlying[loanPool] != address(0);
 	}
 
 	/**
-	 * sets the contract registry address of the SovrynSwap network
-	 * @param registryAddress the address of the registry contract
+	 * @notice Set the contract registry address of the SovrynSwap network.
+	 *
+	 * @param registryAddress the address of the registry contract.
 	 * */
 	function setSovrynSwapContractRegistryAddress(address registryAddress) external onlyOwner {
 		require(Address.isContract(registryAddress), "registryAddress not a contract");
@@ -302,6 +472,11 @@ contract ProtocolSettings is State, ProtocolTokenUser, ProtocolSettingsEvents {
 		emit SetSovrynSwapContractRegistryAddress(msg.sender, oldSovrynSwapContractRegistryAddress, sovrynSwapContractRegistryAddress);
 	}
 
+	/**
+	 * @notice Set the wrBTC contract address.
+	 *
+	 * @param wrbtcTokenAddress The address of the wrBTC contract.
+	 * */
 	function setWrbtcToken(address wrbtcTokenAddress) external onlyOwner {
 		require(Address.isContract(wrbtcTokenAddress), "wrbtcTokenAddress not a contract");
 
@@ -311,6 +486,11 @@ contract ProtocolSettings is State, ProtocolTokenUser, ProtocolSettingsEvents {
 		emit SetWrbtcToken(msg.sender, oldwrbtcToken, wrbtcTokenAddress);
 	}
 
+	/**
+	 * @notice Set the protocol token contract address.
+	 *
+	 * @param _protocolTokenAddress The address of the protocol token contract.
+	 * */
 	function setProtocolTokenAddress(address _protocolTokenAddress) external onlyOwner {
 		require(Address.isContract(_protocolTokenAddress), "_protocolTokenAddress not a contract");
 
@@ -321,8 +501,10 @@ contract ProtocolSettings is State, ProtocolTokenUser, ProtocolSettingsEvents {
 	}
 
 	/**
-	 * @dev set rollover base reward. It should be denominated in wRBTC
-	 */
+	 * @notice Set rollover base reward. It should be denominated in wrBTC.
+	 *
+	 * @param baseRewardValue The base reward.
+	 * */
 	function setRolloverBaseReward(uint256 baseRewardValue) external onlyOwner {
 		require(baseRewardValue > 0, "Base reward is zero");
 
@@ -332,6 +514,11 @@ contract ProtocolSettings is State, ProtocolTokenUser, ProtocolSettingsEvents {
 		emit SetRolloverBaseReward(msg.sender, oldValue, rolloverBaseReward);
 	}
 
+	/**
+	 * @notice Set the fee rebate percent.
+	 *
+	 * @param rebatePercent The fee rebate percent.
+	 * */
 	function setRebatePercent(uint256 rebatePercent) external onlyOwner {
 		require(rebatePercent <= 10**20, "Fee rebate is too high");
 

contracts/modules/SwapsExternal.sol

@@ -122,7 +122,7 @@ contract SwapsExternal is VaultController, SwapsUser {
 	 * @param destToken The address of the destiny token instance.
 	 * @param sourceTokenAmount The amount of source tokens.
 	 *
-	 * @return 
+	 * @return The expected return value.
 	 * */
 	function getSwapExpectedReturn(
 		address sourceToken,