Fix/ownable error - Silent transferOwnership Failure

[pooleja]

Fixes #319

The transfer ownership call in the Ownable contract had a silent failure if the "to" address was not set in the transferOwnership() call. This fix adds a specific require() call so that any callers of the function will be notified that they passed an incorrect error.

[coveralls]

Coverage decreased (-0.09%) to 83.421% when pulling 6d565ef on pooleja:fix/ownable_error into c3a30e9 on OpenZeppelin:master.

[maraoz]

@pooleja test coverage decreased, can you check that please? You can get the test coverage locally by running npm run coverage

[pooleja]

Ownable.sol says it is at 100% still:
https://coveralls.io/builds/12483052/source?filename=contracts%2Fownership%2FOwnable.sol

That is the only contract code touched.

I am wondering if the code coverage counts lines without actual code, like { or } lines? If so, this was reduced by 2 lines of brackets for code fomatting.

[maraoz]

Ah, sorry. ACK!

[frangio]

@cgewecke Do you have any hint as to what caused the coverage decrease?

Let's merge this anyway.

[cgewecke]

@frangio Interesting - it looks like it's because the old tests hit both branches of an if statement raising the overall value of branch coverage for the whole project, and coveralls is averaging everything together.

From Istanbul's report in the CI (the second number is the branch totals):

A previous build had these values:

This build looks like this:

Also . . . I'm seeing coverage crash occasionally in the CI here on a weird database error, so if you see really bad numbers that could also be a cause. Re-running on Travis could clear it.

We're upgrading to testrpc 4.x over the weekend which might help smooth things out.

[jakub-wojciechowski]

Guys @pooleja @frangio ,
I think there is a slightly bigger problem with this new test. If we comment the line:

function transferOwnership(address newOwner) onlyOwner {
    //require(newOwner != address(0));
    owner = newOwner;
  }

All test will pass successfully.

We need to check both paths of execution by failing when there is no error thrown at all:

it('should guard ownership against stuck state', async function() {
    let originalOwner = await ownable.owner();
    try {
      await ownable.transferOwnership(null, {from: originalOwner});
    } catch(error) {
      return assertJump(error);
    }
    assert.fail()
  });

[frangio]

Good catch @jakub-wojciechowski.

@pooleja Can you fix the test?

[pooleja]

Yep, I will take care of that. Good catch.

[pooleja]

Added the assert.fail() call to ensure there is no regression so it will be caught if the check for invalid address is removed.

[coveralls]

Coverage decreased (-0.09%) to 83.421% when pulling 64787b1 on pooleja:fix/ownable_error into c3a30e9 on OpenZeppelin:master.