Increase code coverage (#250)

* Refactor mocks contract structure
* Add solcov ignore on covered lines not reported
* Lint fixes
* Ignore abstract methods in coverage report
* Add unit test for not covered methods

contracts/ERC677BridgeToken.sol

@@ -22,6 +22,7 @@ contract ERC677BridgeToken is IBurnableMintableERC677Token, DetailedERC20, Burna
 
     modifier validRecipient(address _recipient) {
         require(_recipient != address(0) && _recipient != address(this));
+        /* solcov ignore next */
         _;
     }
 

contracts/ERC677BridgeTokenRewardable.sol

@@ -22,11 +22,13 @@ contract ERC677BridgeTokenRewardable is ERC677BridgeToken {
 
     modifier onlyBlockRewardContract() {
         require(msg.sender == blockRewardContract);
+        /* solcov ignore next */
         _;
     }
 
     modifier onlyStakingContract() {
         require(msg.sender == stakingContract);
+        /* solcov ignore next */
         _;
     }
 

contracts/mocks/ERC677BridgeTokenRewardableMock.sol

@@ -1,12 +1,14 @@
 pragma solidity 0.4.24;
 
-import "../../contracts/ERC677BridgeTokenRewardable.sol";
+import "../ERC677BridgeTokenRewardable.sol";
 
 contract ERC677BridgeTokenRewardableMock is ERC677BridgeTokenRewardable {
     constructor(string _name, string _symbol, uint8 _decimals)
         public
         ERC677BridgeTokenRewardable(_name, _symbol, _decimals)
-    {}
+    {
+        // solhint-disable-previous-line no-empty-blocks
+    }
 
     function setBlockRewardContractMock(address _blockRewardContract) public {
         blockRewardContract = _blockRewardContract;

contracts/mocks/ERC677ReceiverTest.sol

@@ -1,6 +1,6 @@
-pragma solidity ^0.4.19;
+pragma solidity 0.4.24;
 
-import "../../contracts/interfaces/ERC677Receiver.sol";
+import "../interfaces/ERC677Receiver.sol";
 
 contract ERC677ReceiverTest is ERC677Receiver {
     address public from;

contracts/mocks/ForeignBridgeV2.sol

@@ -1,6 +1,6 @@
-pragma solidity ^0.4.19;
+pragma solidity 0.4.24;
 
-import "../../contracts/upgradeable_contracts/native_to_erc20/ForeignBridgeNativeToErc.sol";
+import "../upgradeable_contracts/native_to_erc20/ForeignBridgeNativeToErc.sol";
 
 interface OwnableToken {
     function transferOwnership(address) external;

contracts/mocks/NoReturnTransferTokenMock.sol

@@ -25,6 +25,7 @@ contract NoReturnTransferTokenMock {
         return balances[_owner];
     }
 
+    // solhint-disable-next-line no-simple-event-func-name
     function transfer(address _to, uint256 _value) public {
         require(_value <= balances[msg.sender]);
         require(_to != address(0));

contracts/mocks/OldBlockReward.sol

@@ -1,6 +1,6 @@
 pragma solidity 0.4.24;
 
-contract oldBlockReward {
+contract OldBlockReward {
     function bridgesAllowedLength() external view returns (uint256) {
         return 3;
     }

contracts/mocks/RevertFallback.sol

@@ -5,5 +5,7 @@ contract RevertFallback {
         revert();
     }
 
-    function receiveEth() public payable {}
+    function receiveEth() public payable {
+        // solhint-disable-previous-line no-empty-blocks
+    }
 }

contracts/upgradeability/OwnedUpgradeabilityProxy.sol

@@ -27,6 +27,7 @@ contract OwnedUpgradeabilityProxy is UpgradeabilityOwnerStorage, UpgradeabilityP
     */
     modifier onlyUpgradeabilityOwner() {
         require(msg.sender == upgradeabilityOwner());
+        /* solcov ignore next */
         _;
     }
 

contracts/upgradeability/Proxy.sol

@@ -9,6 +9,7 @@ contract Proxy {
     * @dev Tells the address of the implementation where every call will be delegated.
     * @return address of the implementation to which it will be delegated
     */
+    /* solcov ignore next */
     function implementation() public view returns (address);
 
     /**

contracts/upgradeable_contracts/BaseFeeManager.sol

@@ -24,6 +24,7 @@ contract BaseFeeManager is EternalStorage, FeeTypes {
 
     modifier validFee(uint256 _fee) {
         require(_fee < MAX_FEE);
+        /* solcov ignore next */
         _;
     }
 
@@ -45,10 +46,13 @@ contract BaseFeeManager is EternalStorage, FeeTypes {
         return uintStorage[keccak256(abi.encodePacked("foreignFee"))];
     }
 
+    /* solcov ignore next */
     function distributeFeeFromAffirmation(uint256 _fee) external;
 
+    /* solcov ignore next */
     function distributeFeeFromSignatures(uint256 _fee) external;
 
+    /* solcov ignore next */
     function getFeeManagerMode() external pure returns (bytes4);
 
     function random(uint256 _count) internal view returns (uint256) {

contracts/upgradeable_contracts/BasicForeignBridge.sol

@@ -29,6 +29,7 @@ contract BasicForeignBridge is EternalStorage, Validatable, BasicBridge {
         }
     }
 
+    /* solcov ignore next */
     function onExecuteMessage(address, uint256, bytes32) internal returns (bool);
 
     function setRelayedMessages(bytes32 _txHash, bool _status) internal {
@@ -39,5 +40,6 @@ contract BasicForeignBridge is EternalStorage, Validatable, BasicBridge {
         return boolStorage[keccak256(abi.encodePacked("relayedMessages", _txHash))];
     }
 
+    /* solcov ignore next */
     function onFailedMessage(address, uint256, bytes32) internal;
 }

contracts/upgradeable_contracts/BasicHomeBridge.sol

@@ -90,8 +90,10 @@ contract BasicHomeBridge is EternalStorage, Validatable, BasicBridge {
         boolStorage[keccak256(abi.encodePacked("messagesSigned", _hash))] = _status;
     }
 
+    /* solcov ignore next */
     function onExecuteAffirmation(address, uint256, bytes32) internal returns (bool);
 
+    /* solcov ignore next */
     function onSignaturesCollected(bytes) internal;
 
     function numAffirmationsSigned(bytes32 _withdrawal) public view returns (uint256) {
@@ -151,5 +153,6 @@ contract BasicHomeBridge is EternalStorage, Validatable, BasicBridge {
         return Message.requiredMessageLength();
     }
 
+    /* solcov ignore next */
     function onFailedAffirmation(address, uint256, bytes32) internal;
 }

contracts/upgradeable_contracts/BlockRewardFeeManager.sol

@@ -16,5 +16,6 @@ contract BlockRewardFeeManager is BaseFeeManager {
         return IBlockReward(addressStorage[keccak256(abi.encodePacked("blockRewardContract"))]);
     }
 
+    /* solcov ignore next */
     function distributeFeeFromBlockReward(uint256 _fee) internal;
 }

contracts/upgradeable_contracts/Claimable.sol

@@ -6,6 +6,7 @@ import "./Sacrifice.sol";
 contract Claimable {
     modifier validAddress(address _to) {
         require(_to != address(0));
+        /* solcov ignore next */
         _;
     }
 

contracts/upgradeable_contracts/ERC677Bridge.sol

@@ -34,5 +34,6 @@ contract ERC677Bridge is BasicBridge {
         fireEventOnTokenTransfer(_from, _value);
     }
 
+    /* solcov ignore next */
     function fireEventOnTokenTransfer(address _from, uint256 _value) internal;
 }

contracts/upgradeable_contracts/Ownable.sol

@@ -19,6 +19,7 @@ contract Ownable is EternalStorage {
     */
     modifier onlyOwner() {
         require(msg.sender == owner());
+        /* solcov ignore next */
         _;
     }
 

contracts/upgradeable_contracts/Upgradeable.sol

@@ -6,6 +6,7 @@ contract Upgradeable {
     // Avoid using onlyUpgradeabilityOwner name to prevent issues with implementation from proxy contract
     modifier onlyIfUpgradeabilityOwner() {
         require(msg.sender == IUpgradeabilityOwnerStorage(this).upgradeabilityOwner());
+        /* solcov ignore next */
         _;
     }
 }

contracts/upgradeable_contracts/Validatable.sol

@@ -9,6 +9,7 @@ contract Validatable is EternalStorage {
 
     modifier onlyValidator() {
         require(validatorContract().isValidator(msg.sender));
+        /* solcov ignore next */
         _;
     }
 

contracts/upgradeable_contracts/ValidatorsFeeManager.sol

@@ -65,7 +65,9 @@ contract ValidatorsFeeManager is BaseFeeManager {
         }
     }
 
+    /* solcov ignore next */
     function onAffirmationFeeDistribution(address _rewardAddress, uint256 _fee) internal;
 
+    /* solcov ignore next */
     function onSignatureFeeDistribution(address _rewardAddress, uint256 _fee) internal;
 }

contracts/upgradeable_contracts/erc20_to_erc20/BasicForeignBridgeErcToErc.sol

@@ -54,7 +54,9 @@ contract BasicForeignBridgeErcToErc is BasicForeignBridge {
         revert();
     }
 
+    /* solcov ignore next */
     function erc20token() public view returns (ERC20Basic);
 
+    /* solcov ignore next */
     function setErc20token(address _token) internal;
 }

test/erc_to_erc/home_bridge.test.js

@@ -7,7 +7,7 @@ const ERC677BridgeTokenRewardable = artifacts.require('ERC677BridgeTokenRewardab
 const FeeManagerErcToErcPOSDAO = artifacts.require('FeeManagerErcToErcPOSDAO.sol')
 const RewardableValidators = artifacts.require('RewardableValidators.sol')
 const BlockReward = artifacts.require('BlockReward')
-const OldBlockReward = artifacts.require('oldBlockReward')
+const OldBlockReward = artifacts.require('OldBlockReward')
 
 const { expect } = require('chai')
 const { ERROR_MSG, ZERO_ADDRESS, toBN } = require('../setup')

test/erc_to_native/home_bridge.test.js

@@ -2,7 +2,7 @@ const HomeBridge = artifacts.require('HomeBridgeErcToNative.sol')
 const EternalStorageProxy = artifacts.require('EternalStorageProxy.sol')
 const BridgeValidators = artifacts.require('BridgeValidators.sol')
 const BlockReward = artifacts.require('BlockReward')
-const OldBlockReward = artifacts.require('oldBlockReward')
+const OldBlockReward = artifacts.require('OldBlockReward')
 const RewardableValidators = artifacts.require('RewardableValidators.sol')
 const FeeManagerErcToNative = artifacts.require('FeeManagerErcToNative.sol')
 const FeeManagerErcToNativePOSDAO = artifacts.require('FeeManagerErcToNativePOSDAO')
@@ -1663,6 +1663,18 @@ contract('HomeBridge_ERC20_to_Native', async accounts => {
       // When
       await homeBridge.setFeeManagerContract(feeManager.address, { from: owner }).should.be.fulfilled
 
+      // Then
+      const feeManagerMode = await homeBridge.getFeeManagerMode()
+      feeManagerMode.should.be.equals(bothDirectionsModeHash)
+    })
+    it('should be able to get fee manager mode from POSDAO fee manager', async () => {
+      // Given
+      const feeManager = await FeeManagerErcToNativePOSDAO.new()
+      const bothDirectionsModeHash = '0xd7de965f'
+
+      // When
+      await homeBridge.setFeeManagerContract(feeManager.address, { from: owner }).should.be.fulfilled
+
       // Then
       const feeManagerMode = await homeBridge.getFeeManagerMode()
       feeManagerMode.should.be.equals(bothDirectionsModeHash)

test/native_to_erc/home_bridge_test.js

@@ -101,7 +101,64 @@ contract('HomeBridge', async accounts => {
         .should.be.rejectedWith(ERROR_MSG)
       false.should.be.equal(await homeContract.isInitialized())
     })
+    it('can set gas Price ', async () => {
+      // Given
+      await homeContract.initialize(
+        validatorContract.address,
+        '3',
+        '2',
+        '1',
+        gasPrice,
+        requireBlockConfirmations,
+        foreignDailyLimit,
+        foreignMaxPerTx,
+        owner
+      ).should.be.fulfilled
 
+      expect(await homeContract.gasPrice()).to.be.bignumber.equal(gasPrice)
+
+      // When
+      const newGasPrice = web3.utils.toWei('2', 'gwei')
+
+      await homeContract.setGasPrice(newGasPrice, { from: accounts[2] }).should.be.rejectedWith(ERROR_MSG)
+      await homeContract.setGasPrice(0, { from: owner }).should.be.rejectedWith(ERROR_MSG)
+      const { logs } = await homeContract.setGasPrice(newGasPrice, { from: owner }).should.be.fulfilled
+
+      // Then
+      expect(await homeContract.gasPrice()).to.be.bignumber.equal(newGasPrice)
+      expectEventInLogs(logs, 'GasPriceChanged', { gasPrice: newGasPrice })
+    })
+    it('can set Required Block Confirmations', async () => {
+      // Given
+      await homeContract.initialize(
+        validatorContract.address,
+        '3',
+        '2',
+        '1',
+        gasPrice,
+        requireBlockConfirmations,
+        foreignDailyLimit,
+        foreignMaxPerTx,
+        owner
+      ).should.be.fulfilled
+
+      expect(await homeContract.requiredBlockConfirmations()).to.be.bignumber.equal(toBN(requireBlockConfirmations))
+
+      // When
+      const newRequiredBlockConfirmations = 15
+      await homeContract
+        .setRequiredBlockConfirmations(newRequiredBlockConfirmations, { from: accounts[2] })
+        .should.be.rejectedWith(ERROR_MSG)
+      await homeContract.setRequiredBlockConfirmations(0, { from: owner }).should.be.rejectedWith(ERROR_MSG)
+      const { logs } = await homeContract.setRequiredBlockConfirmations(newRequiredBlockConfirmations, { from: owner })
+        .should.be.fulfilled
+
+      // Then
+      expect(await homeContract.requiredBlockConfirmations()).to.be.bignumber.equal(toBN(newRequiredBlockConfirmations))
+      expectEventInLogs(logs, 'RequiredBlockConfirmationChanged', {
+        requiredBlockConfirmations: toBN(newRequiredBlockConfirmations)
+      })
+    })
     it('can be deployed via upgradeToAndCall', async () => {
       const storageProxy = await EternalStorageProxy.new().should.be.fulfilled
       const data = homeContract.contract.methods
@@ -161,12 +218,41 @@ contract('HomeBridge', async accounts => {
       true.should.be.equal(await homeContract.isInitialized())
     })
     it('can transfer ownership', async () => {
+      // Given
+      await homeContract.initialize(
+        validatorContract.address,
+        '3',
+        '2',
+        '1',
+        gasPrice,
+        requireBlockConfirmations,
+        foreignDailyLimit,
+        foreignMaxPerTx,
+        owner
+      ).should.be.fulfilled
+
+      expect(await homeContract.owner()).to.be.equal(owner)
+
+      // When
+      const newOwner = accounts[7]
+
+      await homeContract.transferOwnership(newOwner, { from: accounts[2] }).should.be.rejectedWith(ERROR_MSG)
+      await homeContract.transferOwnership(ZERO_ADDRESS, { from: owner }).should.be.rejectedWith(ERROR_MSG)
+      const { logs } = await homeContract.transferOwnership(newOwner, { from: owner }).should.be.fulfilled
+
+      // Then
+      expect(await homeContract.owner()).to.be.equal(newOwner)
+      expectEventInLogs(logs, 'OwnershipTransferred', { previousOwner: owner, newOwner })
+    })
+    it('can transfer proxyOwnership', async () => {
       const storageProxy = await EternalStorageProxy.new().should.be.fulfilled
       const data = homeContract.contract.methods
         .initialize(validatorContract.address, '3', '2', '1', gasPrice, requireBlockConfirmations, '3', '2', owner)
         .encodeABI()
       await storageProxy.upgradeToAndCall('1', homeContract.address, data).should.be.fulfilled
       await storageProxy.transferProxyOwnership(owner).should.be.fulfilled
+
+      expect(await storageProxy.version()).to.be.bignumber.equal(toBN('1'))
     })
   })
 
@@ -1122,6 +1208,31 @@ contract('HomeBridge', async accounts => {
       const feeManagerMode = await homeBridge.getFeeManagerMode()
       feeManagerMode.should.be.equals(oneDirectionsModeHash)
     })
+    it('should be able to get fee manager mode for both directions', async () => {
+      // Given
+      const feeManager = await FeeManagerNativeToErcBothDirections.new()
+      const bothDirectionsModeHash = '0xd7de965f'
+
+      // When
+      await homeBridge.rewardableInitialize(
+        rewardableValidators.address,
+        oneEther,
+        halfEther,
+        minPerTx,
+        gasPrice,
+        requireBlockConfirmations,
+        foreignDailyLimit,
+        foreignMaxPerTx,
+        owner,
+        feeManager.address,
+        homeFee,
+        foreignFee
+      ).should.be.fulfilled
+
+      // Then
+      const feeManagerMode = await homeBridge.getFeeManagerMode()
+      feeManagerMode.should.be.equals(bothDirectionsModeHash)
+    })
   })
 
   describe('#feeManager_OneDirection_fallback', () => {

test/poa20_test.js

@@ -1,6 +1,6 @@
 const POA20 = artifacts.require('ERC677BridgeToken.sol')
 const NoReturnTransferTokenMock = artifacts.require('NoReturnTransferTokenMock.sol')
-const POA20RewardableMock = artifacts.require('./mockContracts/ERC677BridgeTokenRewardableMock')
+const POA20RewardableMock = artifacts.require('ERC677BridgeTokenRewardableMock')
 const ERC677ReceiverTest = artifacts.require('ERC677ReceiverTest.sol')
 const BlockRewardTest = artifacts.require('BlockReward.sol')
 const StakingTest = artifacts.require('Staking.sol')
@@ -542,6 +542,12 @@ async function testERC677BridgeToken(accounts, rewardable) {
       tokenTransfer2.logs[0].event.should.be.equal('Transfer')
     })
   })
+  describe('#renounceOwnership', () => {
+    it('should not be able to renounce ownership', async () => {
+      await token.renounceOwnership({ from: user }).should.be.rejectedWith(ERROR_MSG)
+      await token.renounceOwnership().should.be.rejectedWith(ERROR_MSG)
+    })
+  })
 }
 
 contract('ERC677BridgeToken', async accounts => {

truffle-config.js

@@ -26,7 +26,7 @@ const provider = new ProviderEngine()
 if (process.env.SOLIDITY_COVERAGE === 'true') {
   global.coverageSubprovider = new CoverageSubprovider(artifactAdapter, defaultFromAddress, {
     isVerbose,
-    ignoreFilesGlobs: ['**/Migrations.sol', '**/node_modules/**', '**/test/**', '**/interfaces/**']
+    ignoreFilesGlobs: ['**/Migrations.sol', '**/node_modules/**', '**/mocks/**', '**/interfaces/**']
   })
   provider.addProvider(global.coverageSubprovider)
   const ganacheSubprovider = new GanacheSubprovider({