Merge pull request #135 from gnosis/feature/issue_133

Closes #133: Added events + required refactoring

contracts/GnosisSafe.sol

@@ -18,7 +18,7 @@ contract GnosisSafe
     using SafeMath for uint256;
 
     string public constant NAME = "Gnosis Safe";
-    string public constant VERSION = "1.0.0";
+    string public constant VERSION = "1.1.0";
 
     //keccak256(
     //    "EIP712Domain(address verifyingContract)"
@@ -35,7 +35,19 @@ contract GnosisSafe
     //);
     bytes32 public constant SAFE_MSG_TYPEHASH = 0x60b3cbf8b4a223d68d641b3b6ddf9a298e7f33710cf3d3a9d1146b5a6150fbca;
 
-    event ExecutionFailed(bytes32 txHash);
+    event ApproveHash(
+        bytes32 indexed approvedHash,
+        address indexed owner
+    );
+    event SignMsg(
+        bytes32 indexed msgHash
+    );
+    event ExecutionFailure(
+        bytes32 txHash, uint256 payment
+    );
+    event ExecutionSuccess(
+        bytes32 txHash, uint256 payment
+    );
 
     uint256 public nonce;
     bytes32 public domainSeparator;
@@ -104,29 +116,66 @@ 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));
-        }
-
         // We transfer the calculated tx costs to the tx.origin to avoid sending it to intermediate contracts that have made calls
+        uint256 payment = 0;
         if (gasPrice > 0) {
-            handlePayment(gasUsed, baseGas, gasPrice, gasToken, refundReceiver);
+            payment = handlePayment(gasUsed, baseGas, gasPrice, gasToken, refundReceiver);
         }
+        if (success) emit ExecutionSuccess(txHash, payment);
+        else emit ExecutionFailure(txHash, payment);
     }
 
     function handlePayment(
@@ -137,15 +186,18 @@ contract GnosisSafe
         address payable refundReceiver
     )
         private
+        returns (uint256 payment)
     {
         // solium-disable-next-line security/no-tx-origin
         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
+            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");
         } else {
-            require(transferToken(gasToken, receiver, gasUsed.add(baseGas).mul(gasPrice)), "Could not pay gas costs with token");
+            payment = gasUsed.add(baseGas).mul(gasPrice);
+            require(transferToken(gasToken, receiver, payment), "Could not pay gas costs with token");
         }
     }
 
@@ -258,6 +310,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 +321,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,8 @@ import "../common/SelfAuthorized.sol";
 /// @author Richard Meissner - <richard@gnosis.pm>
 contract FallbackManager is SelfAuthorized {
 
+    event IncomingTransaction(address from, uint256 value);
+
     // keccak256("fallback_manager.handler.address")
     bytes32 internal constant FALLBACK_HANDLER_STORAGE_SLOT = 0x6c9a6c4a39284e37ed1cf53d337577d14212a4870fb976a4366c693b939918d5;
 
@@ -33,7 +35,10 @@ contract FallbackManager is SelfAuthorized {
         payable
     {
         // Only calls without value and with data will be forwarded
-        if (msg.value > 0 || msg.data.length == 0) return;
+        if (msg.value > 0 || msg.data.length == 0) {
+            emit IncomingTransaction(msg.sender, msg.value);
+            return;
+        }
         bytes32 slot = FALLBACK_HANDLER_STORAGE_SLOT;
         address handler;
         // solium-disable-next-line security/no-inline-assembly

contracts/base/ModuleManager.sol

@@ -12,6 +12,8 @@ contract ModuleManager is SelfAuthorized, Executor {
 
     event EnabledModule(Module module);
     event DisabledModule(Module module);
+    event ExecutionFromModuleSuccess(address indexed module);
+    event ExecutionFromModuleFailure(address indexed module);
 
     address public constant SENTINEL_MODULES = address(0x1);
 
@@ -72,6 +74,8 @@ 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());
+        if (success) emit ExecutionFromModuleSuccess(msg.sender);
+        else emit ExecutionFromModuleFailure(msg.sender);
     }
 
     /// @dev Returns array of modules.

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

@@ -9,7 +9,7 @@ const ProxyFactory = artifacts.require("./ProxyFactory.sol")
 const MockContract = artifacts.require('./MockContract.sol')
 const MockToken = artifacts.require('./Token.sol')
 
-contract('GnosisSafe via create2', function(accounts) {
+contract('GnosisSafe deployment via create2', function(accounts) {
 
     const CALL = 0
 
@@ -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/gnosisSafeDeploymentViaTx.js

@@ -9,7 +9,7 @@ const GnosisSafe = artifacts.require("./GnosisSafe.sol")
 const MockContract = artifacts.require('./MockContract.sol');
 const MockToken = artifacts.require('./Token.sol');
 
-contract('GnosisSafe Trustless Deployment', function(accounts) {
+contract('GnosisSafe trustless deployment', function(accounts) {
 
     const CALL = 0
 

test/gnosisSafeManagement.js

@@ -7,7 +7,7 @@ const ProxyFactory = artifacts.require("./ProxyFactory.sol")
 const MockContract = artifacts.require('./MockContract.sol')
 const MockToken = artifacts.require('./Token.sol')
 
-contract('GnosisSafe', function(accounts) {
+contract('GnosisSafe owner and module management', function(accounts) {
 
     let gnosisSafe
     let gnosisSafeMasterCopy

test/gnosisSafeSignatureTypes.js

@@ -4,7 +4,7 @@ const GnosisSafe = artifacts.require("./GnosisSafe.sol")
 const ProxyFactory = artifacts.require("./ProxyFactory.sol")
 
 
-contract('GnosisSafe Without Refund', function(accounts) {
+contract('GnosisSafe without refund', function(accounts) {
 
     let gnosisSafe
     let executor = accounts[8]

test/gnosisSafeTransactionExecution.js

@@ -7,7 +7,7 @@ const ProxyFactory = artifacts.require("./ProxyFactory.sol")
 const MockContract = artifacts.require('./MockContract.sol')
 const MockToken = artifacts.require('./Token.sol')
 
-contract('GnosisSafe', function(accounts) {
+contract('GnosisSafe with refunds', function(accounts) {
 
     let gnosisSafe
     let lw

test/multiSend.js

@@ -131,12 +131,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'
+            'ExecutionFailure', gnosisSafe.address, true, 'execTransaction send multiple transactions'
         )
+        assert.equal(0, event.args.payment)
     })
 
     it('single fail should fail all', async () => {
@@ -155,12 +156,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'
+            'ExecutionFailure', gnosisSafe.address, true, 'execTransaction send multiple transactions'
         )
+        assert.equal(0, event.args.payment)
         assert.equal(await gnosisSafe.getThreshold(), 1)
     })
 })

test/utils/execution.js

@@ -4,6 +4,7 @@ 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)
     switch(hexValue) {
         case "0x": return 0
         case "00": return 4
@@ -18,22 +19,26 @@ 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
     let signatureCost = signatureCount * (68 + 2176 + 2176 + 6000) // (array count (3 -> r, s, v) + ecrecover costs) * signature count
     let payload = safe.contract.execTransaction.getData(
         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)
+    let baseGasEstimate = estimateBaseGasCosts(payload) + signatureCost + (nonce > 0 ? 5000 : 20000)
+    baseGasEstimate += 1500 // 1500 -> hash generation costs
+    baseGasEstimate += 1000 // 1000 -> Event emission
+    return baseGasEstimate + 32000; // 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,10 +91,16 @@ 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 eventName = (txFailed) ? 'ExecutionFailure' : 'ExecutionSuccess'
+    let event = utils.checkTxEvent(tx, eventName, 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)
+    if (txGasEstimate > 0) {
+        let maxPayment = (baseGasEstimate + txGasEstimate) * gasPrice
+        console.log("    User paid", event.args.payment.toNumber(), "after signing a maximum of", maxPayment)
+        assert.ok(maxPayment >= event.args.payment, "Should not pay more than signed")
+    } else {
+        console.log("    User paid", event.args.payment.toNumber())
     }
     return tx
 }