Merge pull request #537 from keep-network/log-refactor

DepositLog refactor

- Fix `TBTCDepositToken` shadow by removing `TBTCSystem`'s copy
   - Now `TBTCSystem` uses the one inherited from `DepositLog`.
- Remove return value from logging functions.
   - Return values were not used. 
- Logging functions now revert if `approvedToLog` fails. 
   - Returning true/false allowed for non-halting logging fail. Events would
     not be emitted but the `Deposit` contract would continue to function. It
     seems that this is not a real benefit. If `approvedToLog` fails, the logging
     function is not called by the `Deposit` contract and it makes sense to revert.

solidity/contracts/DepositLog.sol

@@ -104,61 +104,53 @@ contract DepositLog {
     // AUTH
     //
 
-    /// @notice             Checks if an address is an allowed logger
+    /// @notice             Checks if an address is an allowed logger.
     /// @dev                checks tbtcDepositToken to see if the caller represents
     ///                     an existing deposit.
-    ///                     We don't require this, so deposits are not bricked if the system borks
-    /// @param  _caller     The address of the calling contract
-    /// @return             True if approved, otherwise false
-    /* solium-disable-next-line no-empty-blocks */
+    ///                     We don't require this, so deposits are not bricked if the system borks.
+    /// @param  _caller     The address of the calling contract.
+    /// @return             True if approved, otherwise false.
     function approvedToLog(address _caller) public view returns (bool) {
         return tbtcDepositToken.exists(uint256(_caller));
     }
 
-    /// @notice               Sets the tbtcDepositToken contract.
-    /// @dev                  The contract is used by `approvedToLog` to check if the
-    ///                       caller is a Deposit contract. This should only be called once.
-    /// @param  _tbtcDepositToken  tbtcDepositToken contract.
-    function setTbtcDepositToken(TBTCDepositToken _tbtcDepositToken) public {
-        require(
-            address(tbtcDepositToken) == address(0),
-            "tbtcDepositToken is already set"
-        );
-        tbtcDepositToken = _tbtcDepositToken;
-    }
-
     //
     // Logging
     //
 
-    /// @notice               Fires a Created event
-    /// @dev                  We append the sender, which is the deposit contract that called
-    /// @param  _keepAddress  The address of the associated keep
-    /// @return               True if successful, else revert
-    function logCreated(address _keepAddress) public returns (bool) {
-        if (!approvedToLog(msg.sender)) return false;
+    /// @notice               Fires a Created event.
+    /// @dev                  We append the sender, which is the deposit contract that called.
+    /// @param  _keepAddress  The address of the associated keep.
+    /// @return               True if successful, else revert.
+    function logCreated(address _keepAddress) public {
+        require(
+            approvedToLog(msg.sender),
+            "Caller is not approved to log events"
+        );
         emit Created(msg.sender, _keepAddress, block.timestamp);
-        return true;
     }
 
-    /// @notice                 Fires a RedemptionRequested event
-    /// @dev                    This is the only event without an explicit timestamp
-    /// @param  _requester      The ethereum address of the requester
-    /// @param  _digest         The calculated sighash digest
-    /// @param  _utxoSize       The size of the utxo in sat
+    /// @notice                 Fires a RedemptionRequested event.
+    /// @dev                    This is the only event without an explicit timestamp.
+    /// @param  _requester      The ethereum address of the requester.
+    /// @param  _digest         The calculated sighash digest.
+    /// @param  _utxoSize       The size of the utxo in sat.
     /// @param  _redeemerOutputScript The redeemer's length-prefixed output script.
-    /// @param  _requestedFee   The requester or bump-system specified fee
-    /// @param  _outpoint       The 36 byte outpoint
-    /// @return                 True if successful, else revert
+    /// @param  _requestedFee   The requester or bump-system specified fee.
+    /// @param  _outpoint       The 36 byte outpoint.
+    /// @return                 True if successful, else revert.
     function logRedemptionRequested(
         address _requester,
         bytes32 _digest,
         uint256 _utxoSize,
         bytes memory _redeemerOutputScript,
         uint256 _requestedFee,
         bytes memory _outpoint
-    ) public returns (bool) {
-        if (!approvedToLog(msg.sender)) return false;
+    ) public {
+        require(
+            approvedToLog(msg.sender),
+            "Caller is not approved to log events"
+        );
         emit RedemptionRequested(
             msg.sender,
             _requester,
@@ -168,125 +160,139 @@ contract DepositLog {
             _requestedFee,
             _outpoint
         );
-        return true;
     }
 
-    /// @notice         Fires a GotRedemptionSignature event
-    /// @dev            We append the sender, which is the deposit contract that called
-    /// @param  _digest signed digest
-    /// @param  _r      signature r value
-    /// @param  _s      signature s value
-    /// @return         True if successful, else revert
+    /// @notice         Fires a GotRedemptionSignature event.
+    /// @dev            We append the sender, which is the deposit contract that called.
+    /// @param  _digest signed digest.
+    /// @param  _r      signature r value.
+    /// @param  _s      signature s value.
     function logGotRedemptionSignature(bytes32 _digest, bytes32 _r, bytes32 _s)
         public
-        returns (bool)
     {
-        if (!approvedToLog(msg.sender)) return false;
+        require(
+            approvedToLog(msg.sender),
+            "Caller is not approved to log events"
+        );
         emit GotRedemptionSignature(
             msg.sender,
             _digest,
             _r,
             _s,
             block.timestamp
         );
-        return true;
     }
 
-    /// @notice     Fires a RegisteredPubkey event
-    /// @dev        We append the sender, which is the deposit contract that called
-    ///             returns false if not approved, to prevent accidentally halting Deposit
-    /// @return     True if successful, else false
+    /// @notice     Fires a RegisteredPubkey event.
+    /// @dev        We append the sender, which is the deposit contract that called.
     function logRegisteredPubkey(
         bytes32 _signingGroupPubkeyX,
         bytes32 _signingGroupPubkeyY
-    ) public returns (bool) {
-        if (!approvedToLog(msg.sender)) return false;
+    ) public {
+        require(
+            approvedToLog(msg.sender),
+            "Caller is not approved to log events"
+        );
         emit RegisteredPubkey(
             msg.sender,
             _signingGroupPubkeyX,
             _signingGroupPubkeyY,
             block.timestamp
         );
-        return true;
     }
 
-    /// @notice     Fires a SetupFailed event
-    /// @dev        We append the sender, which is the deposit contract that called
-    ///             returns false if not approved, to prevent accidentally halting Deposit
-    /// @return     True if successful, else false
-    function logSetupFailed() public returns (bool) {
-        if (!approvedToLog(msg.sender)) return false;
+    /// @notice     Fires a SetupFailed event.
+    /// @dev        We append the sender, which is the deposit contract that called.
+    function logSetupFailed() public {
+        require(
+            approvedToLog(msg.sender),
+            "Caller is not approved to log events"
+        );
         emit SetupFailed(msg.sender, block.timestamp);
-        return true;
     }
 
-    /// @notice     Fires a FraudDuringSetup event
-    /// @dev        We append the sender, which is the deposit contract that called
-    ///             returns false if not approved, to prevent accidentally halting Deposit
-    /// @return     True if successful, else false
-    function logFraudDuringSetup() public returns (bool) {
-        if (!approvedToLog(msg.sender)) return false;
+    /// @notice     Fires a FraudDuringSetup event.
+    /// @dev        We append the sender, which is the deposit contract that called.
+    function logFraudDuringSetup() public {
+        require(
+            approvedToLog(msg.sender),
+            "Caller is not approved to log events"
+        );
         emit FraudDuringSetup(msg.sender, block.timestamp);
-        return true;
     }
 
-    /// @notice     Fires a Funded event
-    /// @dev        We append the sender, which is the deposit contract that called
-    ///             returns false if not approved, to prevent accidentally halting Deposit
-    /// @return     True if successful, else false
-    function logFunded() public returns (bool) {
-        if (!approvedToLog(msg.sender)) return false;
+    /// @notice     Fires a Funded event.
+    /// @dev        We append the sender, which is the deposit contract that called.
+    function logFunded() public {
+        require(
+            approvedToLog(msg.sender),
+            "Caller is not approved to log events"
+        );
         emit Funded(msg.sender, block.timestamp);
-        return true;
     }
 
-    /// @notice     Fires a CourtesyCalled event
-    /// @dev        We append the sender, which is the deposit contract that called
-    ///             returns false if not approved, to prevent accidentally halting Deposit
-    /// @return     True if successful, else false
-    function logCourtesyCalled() public returns (bool) {
-        if (!approvedToLog(msg.sender)) return false;
+    /// @notice     Fires a CourtesyCalled event.
+    /// @dev        We append the sender, which is the deposit contract that called.
+    function logCourtesyCalled() public {
+        require(
+            approvedToLog(msg.sender),
+            "Caller is not approved to log events"
+        );
         emit CourtesyCalled(msg.sender, block.timestamp);
-        return true;
     }
 
-    /// @notice             Fires a StartedLiquidation event
-    /// @dev                We append the sender, which is the deposit contract that called
-    /// @param _wasFraud    True if liquidating for fraud
-    /// @return             True if successful, else revert
-    function logStartedLiquidation(bool _wasFraud) public returns (bool) {
-        if (!approvedToLog(msg.sender)) return false;
+    /// @notice             Fires a StartedLiquidation event.
+    /// @dev                We append the sender, which is the deposit contract that called.
+    /// @param _wasFraud    True if liquidating for fraud.
+    function logStartedLiquidation(bool _wasFraud) public {
+        require(
+            approvedToLog(msg.sender),
+            "Caller is not approved to log events"
+        );
         emit StartedLiquidation(msg.sender, _wasFraud, block.timestamp);
-        return true;
     }
 
     /// @notice     Fires a Redeemed event
-    /// @dev        We append the sender, which is the deposit contract that called
-    ///             returns false if not approved, to prevent accidentally halting Deposit
-    /// @return     True if successful, else false
-    function logRedeemed(bytes32 _txid) public returns (bool) {
-        if (!approvedToLog(msg.sender)) return false;
+    /// @dev        We append the sender, which is the deposit contract that called.
+    function logRedeemed(bytes32 _txid) public {
+        require(
+            approvedToLog(msg.sender),
+            "Caller is not approved to log events"
+        );
         emit Redeemed(msg.sender, _txid, block.timestamp);
-        return true;
     }
 
     /// @notice     Fires a Liquidated event
-    /// @dev        We append the sender, which is the deposit contract that called
-    ///             returns false if not approved, to prevent accidentally halting Deposit
-    /// @return     True if successful, else false
-    function logLiquidated() public returns (bool) {
-        if (!approvedToLog(msg.sender)) return false;
+    /// @dev        We append the sender, which is the deposit contract that called.
+    function logLiquidated() public {
+        require(
+            approvedToLog(msg.sender),
+            "Caller is not approved to log events"
+        );
         emit Liquidated(msg.sender, block.timestamp);
-        return true;
     }
 
     /// @notice     Fires a ExitedCourtesyCall event
-    /// @dev        We append the sender, which is the deposit contract that called
-    ///             returns false if not approved, to prevent accidentally halting Deposit
-    /// @return     True if successful, else false
-    function logExitedCourtesyCall() public returns (bool) {
-        if (!approvedToLog(msg.sender)) return false;
+    /// @dev        We append the sender, which is the deposit contract that called.
+    function logExitedCourtesyCall() public {
+        require(
+            approvedToLog(msg.sender),
+            "Caller is not approved to log events"
+        );
         emit ExitedCourtesyCall(msg.sender, block.timestamp);
-        return true;
+    }
+
+    /// @notice               Sets the tbtcDepositToken contract.
+    /// @dev                  The contract is used by `approvedToLog` to check if the
+    ///                       caller is a Deposit contract. This should only be called once.
+    /// @param  _tbtcDepositTokenAddress  The address of the tbtcDepositToken.
+    function setTbtcDepositToken(TBTCDepositToken _tbtcDepositTokenAddress)
+        internal
+    {
+        require(
+            address(tbtcDepositToken) == address(0),
+            "tbtcDepositToken is already set"
+        );
+        tbtcDepositToken = _tbtcDepositTokenAddress;
     }
 }

solidity/contracts/system/TBTCSystem.sol

@@ -49,7 +49,6 @@ contract TBTCSystem is Ownable, ITBTCSystem, DepositLog {
     uint256 pausedTimestamp;
     uint256 constant pausedDuration = 10 days;
 
-    TBTCDepositToken public tbtcDepositToken;
     IBTCETHPriceFeed public  priceFeed;
     IBondedECDSAKeepVendor public keepVendor;
     IRelay public relay;
@@ -102,7 +101,6 @@ contract TBTCSystem is Ownable, ITBTCSystem, DepositLog {
         uint256 _keepSize
     ) external onlyOwner {
         require(!_initialized, "already initialized");
-        tbtcDepositToken = _tbtcDepositToken;
         keepVendor = _keepVendor;
         _vendingMachine.setExternalAddresses(
             _tbtcToken,

solidity/test/DepositFactoryTest.js

@@ -1,5 +1,5 @@
 const {deployAndLinkAll} = require("./helpers/testDeployer.js")
-const {contract, web3} = require("@openzeppelin/test-environment")
+const {contract, web3, accounts} = require("@openzeppelin/test-environment")
 const {states} = require("./helpers/utils.js")
 const {BN, constants, expectRevert} = require("@openzeppelin/test-helpers")
 const {ZERO_ADDRESS} = constants
@@ -174,6 +174,7 @@ describe("DepositFactory", async function() {
     it("is not affected by state changes to master", async () => {
       const keep = await ECDSAKeepStub.new()
 
+      await tbtcDepositToken.forceMint(accounts[0], testDeposit.address)
       await testDeposit.createNewDeposit(
         tbtcSystemStub.address,
         tbtcToken.address,

solidity/test/DepositUtilsTest.js

@@ -72,6 +72,10 @@ describe("DepositUtils", async function() {
     beneficiary = accounts[2]
 
     feeRebateToken.forceMint(beneficiary, web3.utils.toBN(testDeposit.address))
+    tbtcDepositToken.forceMint(
+      beneficiary,
+      web3.utils.toBN(testDeposit.address),
+    )
 
     await testDeposit.createNewDeposit(
       tbtcSystemStub.address,
@@ -283,6 +287,10 @@ describe("DepositUtils", async function() {
 
       beneficiary = accounts[2]
 
+      tbtcDepositToken.forceMint(
+        beneficiary,
+        web3.utils.toBN(testDeposit.address),
+      )
       feeRebateToken.forceMint(
         beneficiary,
         web3.utils.toBN(testDeposit.address),