Merge pull request #147 from fabioberger/refactor/modifiers

Update modifiers to throw and update corresponding tests

contracts/DayLimit.sol

@@ -58,8 +58,9 @@ contract DayLimit {
 
   // simple modifier for daily limit.
   modifier limitedDaily(uint _value) {
-    if (underLimit(_value)) {
-      _;
+    if (!underLimit(_value)) {
+      throw;
     }
+    _;
   }
 }

contracts/ownership/Claimable.sol

@@ -13,8 +13,10 @@ contract Claimable is Ownable {
   address public pendingOwner;
 
   modifier onlyPendingOwner() {
-    if (msg.sender == pendingOwner)
-      _;
+    if (msg.sender != pendingOwner) {
+      throw;
+    }
+    _;
   }
 
   function transferOwnership(address newOwner) onlyOwner {

contracts/ownership/Ownable.sol

@@ -15,9 +15,10 @@ contract Ownable {
   }
 
   modifier onlyOwner() {
-    if (msg.sender == owner) {
-      _;
+    if (msg.sender != owner) {
+      throw;
     }
+    _;
   }
 
   function transferOwnership(address newOwner) onlyOwner {

contracts/ownership/Shareable.sol

@@ -39,9 +39,10 @@ contract Shareable {
 
   // simple single-sig function modifier.
   modifier onlyOwner {
-    if (isOwner(msg.sender)) {
-      _;
+    if (!isOwner(msg.sender)) {
+      throw;
     }
+    _;
   }
 
   // multi-sig function modifier: the operation must have an intrinsic hash in order

test/Claimable.js

 'use strict';
+const assertJump = require('./helpers/assertJump');
 
 var Claimable = artifacts.require('../contracts/ownership/Claimable.sol');
 
@@ -23,17 +24,22 @@ contract('Claimable', function(accounts) {
   });
 
   it('should prevent to claimOwnership from no pendingOwner', async function() {
-    claimable.claimOwnership({from: accounts[2]});
-    let owner = await claimable.owner();
-
-    assert.isTrue(owner !== accounts[2]);
+    try {
+        await claimable.claimOwnership({from: accounts[2]});
+    } catch(error) {
+        assertJump(error);
+    }
   });
 
   it('should prevent non-owners from transfering', async function() {
-    await claimable.transferOwnership(accounts[2], {from: accounts[2]});
-    let pendingOwner = await claimable.pendingOwner();
-
-    assert.isFalse(pendingOwner === accounts[2]);
+    const other = accounts[2];
+    const owner = await claimable.owner.call();
+    assert.isTrue(owner !== other);
+    try {
+        await claimable.transferOwnership(other, {from: other});
+    } catch(error) {
+        assertJump(error);
+    }
   });
 
   describe('after initiating a transfer', function () {

test/DayLimit.js

 'use strict';
+const assertJump = require('./helpers/assertJump');
 
 var DayLimitMock = artifacts.require('helpers/DayLimitMock.sol');
 
@@ -32,9 +33,11 @@ contract('DayLimit', function(accounts) {
     let spentToday = await dayLimit.spentToday();
     assert.equal(spentToday, 8);
 
-    await dayLimit.attemptSpend(3);
-    spentToday = await dayLimit.spentToday();
-    assert.equal(spentToday, 8);
+    try {
+        await dayLimit.attemptSpend(3);
+    } catch(error) {
+        assertJump(error);
+    }
   });
 
   it('should allow spending if daily limit is reached and then set higher', async function() {
@@ -45,7 +48,11 @@ contract('DayLimit', function(accounts) {
     let spentToday = await dayLimit.spentToday();
     assert.equal(spentToday, 8);
 
-    await dayLimit.attemptSpend(3);
+    try {
+        await dayLimit.attemptSpend(3);
+    } catch(error) {
+        assertJump(error);
+    }
     spentToday = await dayLimit.spentToday();
     assert.equal(spentToday, 8);
 
@@ -63,7 +70,11 @@ contract('DayLimit', function(accounts) {
     let spentToday = await dayLimit.spentToday();
     assert.equal(spentToday, 8);
 
-    await dayLimit.attemptSpend(3);
+    try {
+        await dayLimit.attemptSpend(3);
+    } catch(error) {
+        assertJump(error);
+    }
     spentToday = await dayLimit.spentToday();
     assert.equal(spentToday, 8);
 

test/Ownable.js

 'use strict';
+const assertJump = require('./helpers/assertJump');
 
 var Ownable = artifacts.require('../contracts/ownership/Ownable.sol');
 
@@ -23,11 +24,14 @@ contract('Ownable', function(accounts) {
   });
 
   it('should prevent non-owners from transfering', async function() {
-    let other = accounts[2];
-    await ownable.transferOwnership(other, {from: accounts[2]});
-    let owner = await ownable.owner();
-
-     assert.isFalse(owner === other);
+    const other = accounts[2];
+    const owner = await ownable.owner.call();
+    assert.isTrue(owner !== other);
+    try {
+      await ownable.transferOwnership(other, {from: other});
+    } catch(error) {
+      assertJump(error);
+    }
   });
 
   it('should guard ownership against stuck state', async function() {