Added events + required refactoring

contracts/GnosisSafe.sol

@@ -35,7 +35,21 @@ contract GnosisSafe
     //);
     bytes32 public constant SAFE_MSG_TYPEHASH = 0x60b3cbf8b4a223d68d641b3b6ddf9a298e7f33710cf3d3a9d1146b5a6150fbca;
 
-    event ExecutionFailed(bytes32 txHash);
+    // TODO: what should be indexed (8 gas per data on non-indexed => 256 gas per word VS 375 gas indexed word)
+    event ApproveHash(
+        bytes32 indexed approvedHash,
+        address indexed owner
+    );
+    event SignMsg(
+        bytes32 indexed msgHash
+    );
+    event Execution(
+        bytes32 indexed txHash, bool indexed success, address indexed to, // topics
+        uint256 value, bytes data, Enum.Operation operation
+    );
+    event ExecutionPayment(
+        address token, uint256 value
+    );
 
     uint256 public nonce;
     bytes32 public domainSeparator;
@@ -104,25 +118,60 @@ contract GnosisSafe
         bytes calldata signatures
     )
         external
-        returns (bool success)
+        returns (bool)
     {
+        bytes32 txHash = checkTransaction(
+            to, value, data, operation, // Transaction info
+            safeTxGas, baseGas, gasPrice, gasToken, refundReceiver, // Payment info
+            signatures
+        );
+        require(gasleft() >= safeTxGas, "Not enough gas to execute safe transaction");
+        return executeTransaction(
+            txHash, to, value, data, operation, // Transaction info
+            safeTxGas, baseGas, gasPrice, gasToken, refundReceiver // Payment info
+        );
+    }
+
+    function checkTransaction(
+        address to,
+        uint256 value,
+        bytes memory data,
+        Enum.Operation operation,
+        uint256 safeTxGas,
+        uint256 baseGas,
+        uint256 gasPrice,
+        address gasToken,
+        address payable refundReceiver,
+        bytes memory signatures
+    ) private returns (bytes32 txHash) {
         bytes memory txHashData = encodeTransactionData(
             to, value, data, operation, // Transaction info
             safeTxGas, baseGas, gasPrice, gasToken, refundReceiver, // Payment info
             nonce
         );
         // Increase nonce and execute transaction.
         nonce++;
-        checkSignatures(keccak256(txHashData), txHashData, signatures, true);
-        require(gasleft() >= safeTxGas, "Not enough gas to execute safe transaction");
+        txHash = keccak256(txHashData);
+        checkSignatures(txHash, txHashData, signatures, true);
+    }
+
+    function executeTransaction(
+        bytes32 txHash,
+        address to,
+        uint256 value,
+        bytes memory data,
+        Enum.Operation operation,
+        uint256 safeTxGas,
+        uint256 baseGas,
+        uint256 gasPrice,
+        address gasToken,
+        address payable refundReceiver
+    ) private returns (bool success) {
         uint256 gasUsed = gasleft();
         // If no safeTxGas has been set and the gasPrice is 0 we assume that all available gas can be used
         success = execute(to, value, data, operation, safeTxGas == 0 && gasPrice == 0 ? gasleft() : safeTxGas);
         gasUsed = gasUsed.sub(gasleft());
-        if (!success) {
-            emit ExecutionFailed(keccak256(txHashData));
-        }
-
+        emit Execution(txHash, success, to, value, data, operation);
         // We transfer the calculated tx costs to the tx.origin to avoid sending it to intermediate contracts that have made calls
         if (gasPrice > 0) {
             handlePayment(gasUsed, baseGas, gasPrice, gasToken, refundReceiver);
@@ -142,10 +191,14 @@ contract GnosisSafe
         address payable receiver = refundReceiver == address(0) ? tx.origin : refundReceiver;
         if (gasToken == address(0)) {
             // For ETH we will only adjust the gas price to not be higher than the actual used gas price
+            uint256 payment = gasUsed.add(baseGas).mul(gasPrice < tx.gasprice ? gasPrice : tx.gasprice);
             // solium-disable-next-line security/no-send
-            require(receiver.send(gasUsed.add(baseGas).mul(gasPrice < tx.gasprice ? gasPrice : tx.gasprice)), "Could not pay gas costs with ether");
+            require(receiver.send(payment), "Could not pay gas costs with ether");
+            emit ExecutionPayment(gasToken, payment);
         } else {
-            require(transferToken(gasToken, receiver, gasUsed.add(baseGas).mul(gasPrice)), "Could not pay gas costs with token");
+            uint256 payment = gasUsed.add(baseGas).mul(gasPrice);
+            require(transferToken(gasToken, receiver, payment), "Could not pay gas costs with token");
+            emit ExecutionPayment(gasToken, payment);
         }
     }
 
@@ -258,6 +311,7 @@ contract GnosisSafe
     {
         require(owners[msg.sender] != address(0), "Only owners can approve a hash");
         approvedHashes[msg.sender][hashToApprove] = 1;
+        emit ApproveHash(hashToApprove, msg.sender);
     }
 
     /**
@@ -268,7 +322,9 @@ contract GnosisSafe
         external
         authorized
     {
-        signedMessages[getMessageHash(_data)] = 1;
+        bytes32 msgHash = getMessageHash(_data);
+        signedMessages[msgHash] = 1;
+        emit SignMsg(msgHash);
     }
 
     /**

contracts/base/FallbackManager.sol

@@ -6,6 +6,10 @@ import "../common/SelfAuthorized.sol";
 /// @author Richard Meissner - <[email protected]>
 contract FallbackManager is SelfAuthorized {
 
+    event IncomingTransaction(
+        address from, uint256 value, bool hasData
+    );
+
     // keccak256("fallback_manager.handler.address")
     bytes32 internal constant FALLBACK_HANDLER_STORAGE_SLOT = 0x6c9a6c4a39284e37ed1cf53d337577d14212a4870fb976a4366c693b939918d5;
 
@@ -32,6 +36,7 @@ contract FallbackManager is SelfAuthorized {
         external
         payable
     {
+        emit IncomingTransaction(msg.sender, msg.value, msg.data.length != 0);
         // Only calls without value and with data will be forwarded
         if (msg.value > 0 || msg.data.length == 0) return;
         bytes32 slot = FALLBACK_HANDLER_STORAGE_SLOT;

contracts/base/ModuleManager.sol

@@ -12,6 +12,11 @@ contract ModuleManager is SelfAuthorized, Executor {
 
     event EnabledModule(Module module);
     event DisabledModule(Module module);
+    // TODO: what should be indexed
+    event ExecutionFromModule(
+        bool indexed success, address indexed module, address indexed to, // topics
+        uint256 value, bytes data, Enum.Operation operation
+    );
 
     address public constant SENTINEL_MODULES = address(0x1);
 
@@ -72,6 +77,7 @@ contract ModuleManager is SelfAuthorized, Executor {
         require(msg.sender != SENTINEL_MODULES && modules[msg.sender] != address(0), "Method can only be called from an enabled module");
         // Execute transaction without further confirmations.
         success = execute(to, value, data, operation, gasleft());
+        emit ExecutionFromModule(success, msg.sender, to, value, data, operation);
     }
 
     /// @dev Returns array of modules.

package.json

@@ -18,7 +18,7 @@
     "preversion": "npm run restore",
     "truffle-zos-merge": "node scripts/merge_zos.js",
     "restore": "rm -rf build && npx truffle compile && npm run truffle-zos-merge && npx tnt cB",
-    "install": "cd $INIT_CWD && npm explore truffle -- npm install [email protected].7"
+    "install": "cd $INIT_CWD && npm explore truffle -- npm install [email protected].11"
   },
   "repository": {
     "type": "git",

test/createCall.js

@@ -60,7 +60,7 @@ contract('CreateCall', function(accounts) {
 
         let creationData = createCall.contract.performCreate.getData(0, compileContract.data)
         let testContract = utils.getParamFromTxEvent(
-            await safeUtils.executeTransaction(lw, gnosisSafe, 'create test contract', [lw.accounts[0], lw.accounts[1]], createCall.address, 0, creationData, DELEGATECALL, executor),
+            await safeUtils.executeTransaction(lw, gnosisSafe, 'create test contract', [lw.accounts[0], lw.accounts[1]], createCall.address, 0, creationData, DELEGATECALL, executor, { extraGas: 15000 }),
             'ContractCreation', 'newContract', gnosisSafe.address, TestContract, 'executeTransaction CREATE with value'
         )
 
@@ -82,7 +82,7 @@ contract('CreateCall', function(accounts) {
 
         let creationData = createCall.contract.performCreate.getData(web3.toWei(1, 'ether'), compileContract.data)
         let testContract = utils.getParamFromTxEvent(
-            await safeUtils.executeTransaction(lw, gnosisSafe, 'create test contract', [lw.accounts[0], lw.accounts[1]], createCall.address, 0, creationData, DELEGATECALL, executor),
+            await safeUtils.executeTransaction(lw, gnosisSafe, 'create test contract', [lw.accounts[0], lw.accounts[1]], createCall.address, 0, creationData, DELEGATECALL, executor, { extraGas: 15000 }),
             'ContractCreation', 'newContract', gnosisSafe.address, TestContract, 'executeTransaction CREATE'
         )
 
@@ -106,7 +106,7 @@ contract('CreateCall', function(accounts) {
 
         let creationData = createCall.contract.performCreate2.getData(0, compileContract.data, salt)
         let testContract = utils.getParamFromTxEvent(
-            await safeUtils.executeTransaction(lw, gnosisSafe, 'create test contract', [lw.accounts[0], lw.accounts[1]], createCall.address, 0, creationData, DELEGATECALL, executor),
+            await safeUtils.executeTransaction(lw, gnosisSafe, 'create test contract', [lw.accounts[0], lw.accounts[1]], createCall.address, 0, creationData, DELEGATECALL, executor, { extraGas: 15000 }),
             'ContractCreation', 'newContract', gnosisSafe.address, TestContract, 'executeTransaction CREATE2'
         )
 
@@ -129,7 +129,7 @@ contract('CreateCall', function(accounts) {
 
         let creationData = createCall.contract.performCreate2.getData(web3.toWei(1, 'ether'), compileContract.data, salt)
         let testContract = utils.getParamFromTxEvent(
-            await safeUtils.executeTransaction(lw, gnosisSafe, 'create test contract', [lw.accounts[0], lw.accounts[1]], createCall.address, 0, creationData, DELEGATECALL, executor),
+            await safeUtils.executeTransaction(lw, gnosisSafe, 'create test contract', [lw.accounts[0], lw.accounts[1]], createCall.address, 0, creationData, DELEGATECALL, executor, { extraGas: 15000 }),
             'ContractCreation', 'newContract', gnosisSafe.address, TestContract, 'executeTransaction CREATE2 with value'
         )
 

test/gnosisSafeDeploymentViaCreate2.js

@@ -75,7 +75,7 @@ contract('GnosisSafe via create2', function(accounts) {
         // Estimate safe creation costs
         let gnosisSafeData = await gnosisSafeMasterCopy.contract.setup.getData([lw.accounts[0], lw.accounts[1], lw.accounts[2]], 2, 0, "0x", 0, 0, 0, 0)
         let creationNonce = new Date().getTime()
-        let estimate = (await proxyFactory.createProxyWithNonce.estimateGas(gnosisSafeMasterCopy.address, gnosisSafeData, creationNonce)) + 9000
+        let estimate = (await proxyFactory.createProxyWithNonce.estimateGas(gnosisSafeMasterCopy.address, gnosisSafeData, creationNonce)) + 14000
         let creationData = await getCreationData(0, estimate * gasPrice, creationNonce)
 
         // User funds safe

test/multiSend.js

@@ -91,12 +91,13 @@ contract('MultiSend', function(accounts) {
         let data = await multiSend.contract.multiSend.getData(nestedTransactionData)
         let transactionHash = await gnosisSafe.getTransactionHash(multiSend.address, 0, data, DELEGATECALL, 0, 0, 0, 0, 0, nonce)
         let sigs = utils.signTransaction(lw, [lw.accounts[0]], transactionHash)
-        utils.checkTxEvent(
+        let event = utils.checkTxEvent(
             await gnosisSafe.execTransaction(
                 multiSend.address, 0, data, DELEGATECALL, 0, 0, 0, 0, 0, sigs
             ),
-            'ExecutionFailed', gnosisSafe.address, true, 'execTransaction send multiple transactions'
+            'Execution', gnosisSafe.address, true, 'execTransaction send multiple transactions'
         )
+        assert.equal(false, event.args.success)
     })
 
     it('single fail should fail all', async () => {
@@ -115,12 +116,13 @@ contract('MultiSend', function(accounts) {
         let data = await multiSend.contract.multiSend.getData(nestedTransactionData)
         let transactionHash = await gnosisSafe.getTransactionHash(multiSend.address, 0, data, DELEGATECALL, 0, 0, 0, 0, 0, nonce)
         let sigs = utils.signTransaction(lw, [lw.accounts[0]], transactionHash)
-        utils.checkTxEvent(
+        let event = utils.checkTxEvent(
             await gnosisSafe.execTransaction(
                 multiSend.address, 0, data, DELEGATECALL, 0, 0, 0, 0, 0, sigs
             ),
-            'ExecutionFailed', gnosisSafe.address, true, 'execTransaction send multiple transactions'
+            'Execution', gnosisSafe.address, true, 'execTransaction send multiple transactions'
         )
+        assert.equal(false, event.args.success)
         assert.equal(await gnosisSafe.getThreshold(), 1)
     })
 })

test/utils/execution.js

@@ -4,6 +4,8 @@ const BigNumber = require('bignumber.js');
 const GAS_PRICE = web3.toWei(100, 'gwei')
 
 let baseGasValue = function(hexValue) {
+    // TODO: adjust for Istanbul hardfork (https://eips.ethereum.org/EIPS/eip-2028)
+    // TODO: this should include the event costs
     switch(hexValue) {
      case "0x": return 0
      case "00": return 4
@@ -18,6 +20,7 @@ let baseGasValue = function(hexValue) {
  }
 
 let estimateBaseGas = function(safe, to, value, data, operation, txGasEstimate, gasToken, refundReceiver, signatureCount, nonce) {
+    // TODO: adjust for Istanbul hardfork (https://eips.ethereum.org/EIPS/eip-2028)
     // numbers < 256 are 192 -> 31 * 4 + 68
     // numbers < 65k are 256 -> 30 * 4 + 2 * 68
     // For signature array length and baseGasEstimate we already calculated the 0 bytes so we just add 64 for each non-zero byte
@@ -26,14 +29,16 @@ let estimateBaseGas = function(safe, to, value, data, operation, txGasEstimate,
         to, value, data, operation, txGasEstimate, 0, GAS_PRICE, gasToken, refundReceiver, "0x"
     )
     let baseGasEstimate = estimatebaseGasCosts(payload) + signatureCost + (nonce > 0 ? 5000 : 20000) + 1500 // 1500 -> hash generation costs
-    return baseGasEstimate + 32000; // Add aditional gas costs (e.g. base tx costs, transfer costs)
+    // TODO: reduce to 32k again and adjust basecost calc for events
+    return baseGasEstimate + 37000; // Add aditional gas costs (e.g. base tx costs, transfer costs)
 }
 
 let executeTransactionWithSigner = async function(signer, safe, subject, accounts, to, value, data, operation, executor, opts) {
     let options = opts || {}
     let txFailed = options.fails || false
     let txGasToken = options.gasToken || 0
     let refundReceiver = options.refundReceiver || 0
+    let extraGas = options.extraGas || 0
 
     // Estimate safe transaction (need to be called with from set to the safe address)
     let txGasEstimate = 0
@@ -49,7 +54,7 @@ let executeTransactionWithSigner = async function(signer, safe, subject, account
     }
     let nonce = await safe.nonce()
 
-    let baseGasEstimate = estimateBaseGas(safe, to, value, data, operation, txGasEstimate, txGasToken, refundReceiver, accounts.length, nonce)
+    let baseGasEstimate = estimateBaseGas(safe, to, value, data, operation, txGasEstimate, txGasToken, refundReceiver, accounts.length, nonce) + extraGas
     console.log("    Base Gas estimate: " + baseGasEstimate)
 
     let gasPrice = GAS_PRICE
@@ -86,11 +91,11 @@ let executeTransactionWithSigner = async function(signer, safe, subject, account
     let tx = await safe.execTransaction(
         to, value, data, operation, txGasEstimate, baseGasEstimate, gasPrice, txGasToken, refundReceiver, sigs, {from: executor, gas: estimate + txGasEstimate + 10000, gasPrice: options.txGasPrice || gasPrice}
     )
-    let events = utils.checkTxEvent(tx, 'ExecutionFailed', safe.address, txFailed, subject)
-    if (txFailed) {
-        let transactionHash = await safe.getTransactionHash(to, value, data, operation, txGasEstimate, baseGasEstimate, gasPrice, txGasToken, refundReceiver, nonce)
-        assert.equal(transactionHash, events.args.txHash)
-    }
+    let event = utils.checkTxEvent(tx, 'Execution', safe.address, true, subject)
+    let transactionHash = await safe.getTransactionHash(to, value, data, operation, txGasEstimate, baseGasEstimate, gasPrice, txGasToken, refundReceiver, nonce)
+    assert.equal(transactionHash, event.args.txHash)
+    assert.equal(!txFailed, event.args.success)
+    // TODO: test all events params
     return tx
 }