From b2e36314cb32e9be87e35d732d16335f1ec45051 Mon Sep 17 00:00:00 2001 From: pooleja Date: Thu, 20 Jul 2017 10:51:57 -0700 Subject: [PATCH 1/4] Add requirement for address to not be 0 and throw error --- contracts/ownership/Ownable.sol | 5 +-- test/Ownable.js | 70 ++++++++++++++++----------------- 2 files changed, 37 insertions(+), 38 deletions(-) diff --git a/contracts/ownership/Ownable.sol b/contracts/ownership/Ownable.sol index d6366479f..fcb3ef91f 100644 --- a/contracts/ownership/Ownable.sol +++ b/contracts/ownership/Ownable.sol @@ -33,9 +33,8 @@ contract Ownable { * @param newOwner The address to transfer ownership to. */ function transferOwnership(address newOwner) onlyOwner { - if (newOwner != address(0)) { - owner = newOwner; - } + require(newOwner != address(0)); + owner = newOwner; } } diff --git a/test/Ownable.js b/test/Ownable.js index 0ad2fffcb..45714c81b 100644 --- a/test/Ownable.js +++ b/test/Ownable.js @@ -1,45 +1,45 @@ -'use strict'; -const assertJump = require('./helpers/assertJump'); +'use strict' +const assertJump = require('./helpers/assertJump') -var Ownable = artifacts.require('../contracts/ownership/Ownable.sol'); +var Ownable = artifacts.require('../contracts/ownership/Ownable.sol') -contract('Ownable', function(accounts) { - let ownable; +contract('Ownable', function (accounts) { + let ownable - beforeEach(async function() { - ownable = await Ownable.new(); - }); + beforeEach(async function () { + ownable = await Ownable.new() + }) - it('should have an owner', async function() { - let owner = await ownable.owner(); - assert.isTrue(owner !== 0); - }); + it('should have an owner', async function () { + let owner = await ownable.owner() + assert.isTrue(owner !== 0) + }) - it('changes owner after transfer', async function() { - let other = accounts[1]; - await ownable.transferOwnership(other); - let owner = await ownable.owner(); + it('changes owner after transfer', async function () { + let other = accounts[1] + await ownable.transferOwnership(other) + let owner = await ownable.owner() - assert.isTrue(owner === other); - }); + assert.isTrue(owner === other) + }) - it('should prevent non-owners from transfering', async function() { - const other = accounts[2]; - const owner = await ownable.owner.call(); - assert.isTrue(owner !== other); + it('should prevent non-owners from transfering', async function () { + 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); + await ownable.transferOwnership(other, {from: other}) + } catch (error) { + assertJump(error) } - }); + }) - it('should guard ownership against stuck state', async function() { - let originalOwner = await ownable.owner(); - await ownable.transferOwnership(null, {from: originalOwner}); - let newOwner = await ownable.owner(); - - assert.equal(originalOwner, newOwner); - }); - -}); + it('should guard ownership against stuck state setting owner as 0x0 address', async function () { + let originalOwner = await ownable.owner() + try { + await ownable.transferOwnership(null, {from: originalOwner}) + } catch (error) { + assertJump(error) + } + }) +}) From 6d565ef841c07ccd9e740ac6a1be55f089fb021d Mon Sep 17 00:00:00 2001 From: pooleja Date: Thu, 20 Jul 2017 10:58:16 -0700 Subject: [PATCH 2/4] Fix auto-formatting --- test/Ownable.js | 67 +++++++++++++++++++++++++------------------------ 1 file changed, 34 insertions(+), 33 deletions(-) diff --git a/test/Ownable.js b/test/Ownable.js index 45714c81b..3d8be3be3 100644 --- a/test/Ownable.js +++ b/test/Ownable.js @@ -1,45 +1,46 @@ -'use strict' -const assertJump = require('./helpers/assertJump') +'use strict'; +const assertJump = require('./helpers/assertJump'); -var Ownable = artifacts.require('../contracts/ownership/Ownable.sol') +var Ownable = artifacts.require('../contracts/ownership/Ownable.sol'); -contract('Ownable', function (accounts) { - let ownable +contract('Ownable', function(accounts) { + let ownable; - beforeEach(async function () { - ownable = await Ownable.new() - }) + beforeEach(async function() { + ownable = await Ownable.new(); + }); - it('should have an owner', async function () { - let owner = await ownable.owner() - assert.isTrue(owner !== 0) - }) + it('should have an owner', async function() { + let owner = await ownable.owner(); + assert.isTrue(owner !== 0); + }); - it('changes owner after transfer', async function () { - let other = accounts[1] - await ownable.transferOwnership(other) - let owner = await ownable.owner() + it('changes owner after transfer', async function() { + let other = accounts[1]; + await ownable.transferOwnership(other); + let owner = await ownable.owner(); - assert.isTrue(owner === other) - }) + assert.isTrue(owner === other); + }); - it('should prevent non-owners from transfering', async function () { - const other = accounts[2] - const owner = await ownable.owner.call() - assert.isTrue(owner !== other) + it('should prevent non-owners from transfering', async function() { + 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) + await ownable.transferOwnership(other, {from: other}); + } catch(error) { + assertJump(error); } - }) + }); - it('should guard ownership against stuck state setting owner as 0x0 address', async function () { - let originalOwner = await ownable.owner() + it('should guard ownership against stuck state', async function() { + let originalOwner = await ownable.owner(); try { - await ownable.transferOwnership(null, {from: originalOwner}) - } catch (error) { - assertJump(error) + await ownable.transferOwnership(null, {from: originalOwner}); + } catch(error) { + assertJump(error); } - }) -}) + }); + +}); \ No newline at end of file From 58fdb956b582d0d6d28dc20d0c43b96b0e4bae27 Mon Sep 17 00:00:00 2001 From: pooleja Date: Fri, 21 Jul 2017 23:04:50 -0700 Subject: [PATCH 3/4] Add assert to prevent regression --- test/Ownable.js | 1 + 1 file changed, 1 insertion(+) diff --git a/test/Ownable.js b/test/Ownable.js index 3d8be3be3..57fcba40e 100644 --- a/test/Ownable.js +++ b/test/Ownable.js @@ -38,6 +38,7 @@ contract('Ownable', function(accounts) { let originalOwner = await ownable.owner(); try { await ownable.transferOwnership(null, {from: originalOwner}); + assert.fail() } catch(error) { assertJump(error); } From 64787b1ac57702b7b16dd311394b94b7a8123a1c Mon Sep 17 00:00:00 2001 From: pooleja Date: Fri, 21 Jul 2017 23:07:50 -0700 Subject: [PATCH 4/4] Add semicolon to match coding standards --- test/Ownable.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/Ownable.js b/test/Ownable.js index 57fcba40e..9c66d4826 100644 --- a/test/Ownable.js +++ b/test/Ownable.js @@ -38,7 +38,7 @@ contract('Ownable', function(accounts) { let originalOwner = await ownable.owner(); try { await ownable.transferOwnership(null, {from: originalOwner}); - assert.fail() + assert.fail(); } catch(error) { assertJump(error); }