Support Solidity 0.5.x

[area]

Based on the leapdao fork, with a nod to #311 as well. Also trying to get dependencies up-to-date.

This branch is currently published under the beta tag; you can try it out by npm install --save --dev solidity-coverage@beta. Bug reports would be very welcome before I release properly. You must be using solidity 0.5.1 or higher (0.5.0 will not work).

[codecov-io]

Codecov Report

Merging #318 into master will decrease coverage by 3.3%.
The diff coverage is 94.73%.

@@            Coverage Diff             @@
##           master     #318      +/-   ##
==========================================
- Coverage   95.38%   92.07%   -3.31%     
==========================================
  Files           6        6              
  Lines         368      366       -2     
  Branches       84       79       -5     
==========================================
- Hits          351      337      -14     
- Misses         17       29      +12

Impacted Files	Coverage Δ
lib/instrumentSolidity.js	100% <100%> (ø)	⬆️
lib/coverageMap.js	100% <100%> (ø)	⬆️
lib/preprocessor.js	92.85% <91.89%> (-2.6%)	⬇️
lib/parse.js	82% <93.75%> (-9.18%)	⬇️
lib/instrumenter.js	92.53% <96%> (-2.92%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7e18028...7ee0170. Read the comment docs.

[hiddentao]

Noticed one issue. If I have a line within a function like this:

       address payable owner = msg.sender;

It seems to fail with:

TypeError: Cannot read property 'expressionList' of null
    at ASTBuilder.FunctionCall (/Users/ram/dev/clients/noblocknoparty/contracts/node_modules/solidity-parser-antlr/dist/ASTBuilder.js:272:17)

I debugged and found that it's the coverage-generated line just before that source line which fails. Specifically, it's unable to fetch function call arguments:

emit __CoverageDeployer('/home/user/dev/contracts/Deployer.sol',31);
emit __StatementCoverageDeployer('/home/user/dev/contracts/Deployer.sol',1);  // <-- this line fails!
address payable owner = msg.sender;

[area]

Hm, I'm not able to reproduce it with a contract with just that line in... can you provide more context?

[hiddentao]

@area Sure.

Here is the full code:

pragma solidity ^0.5.4;

import './zeppelin/lifecycle/Destructible.sol';
import './Conference.sol';

contract Deployer is Destructible {
    event NewParty(
        address indexed deployedAddress,
        address indexed deployer
    );

    function deploy(
        string calldata _name,
        uint256 _deposit,
        uint _limitOfParticipants,
        uint _coolingPeriod
    ) external {
       address payable owner = msg.sender;

        Conference c = new Conference(
            _name,
            _deposit,
            _limitOfParticipants,
            _coolingPeriod,
            owner
        );

        emit NewParty(address(c), owner);
    }
}

I'm fairly sure the code for Destructible and Conference aren't relevant to this issue, but in case you want to see them the full code is also at https://github.com/wearekickback/contracts/tree/master/contracts

[hiddentao]

Ah, just an additional note that the code above is new code that's not yet tested in the wild - I'm trying to upgrade our contracts to 0.5+, hence why I'm trying out this beta version of solidity-coverage. Thanks for the working on this, btw :)

[hiddentao]

I've re-coded it to get it working (I removed the offending line and simply used msg.sender directly). I had to also fix the web3 ABI decoder as a final step (see #319). It's all good now, though it would still be nice to fix this issue.

[vdrg]

@area It looks like the API for this package has changed. I got it working as follows:

Suggested change

	const SolidityCoder = require('web3-eth-abi');
	const { AbiCoder } = require('web3-eth-abi');
	const SolidityCoder = AbiCoder();

[vdrg]

Oh I guess this is related to @hiddentao's comment. I'm not sure if this is the only fix needed, but for me it is generating the reports correctly.
Also, are there plans to update testrpc-sc? There are things like eth_signTypedData that are present in ganache but not in the sc fork.

[area]

Hm, this change definitely breaks for me, with TypeError: AbiCoder is not a function - indeed AbiCoder is not defined at all. Are you working with the beta release or this branch?

I agree an update for testrpc-sc would be good. I'm going to concentrate on this solidity 0.5.x support for now though.

[hiddentao]

Beta release 46 changes the API, hence why the current code breaks if you install the latest web3 abi beta. Unfortunately the web3 team's current release versioning usage breaks semantic versioning. Hence why my PR to fix the dependency version specifically.

[area]

Oh derp, I understand now. I could have sworn your PR fixed the version of web3, not web3-eth-abi.

[area]

@hiddentao I have seen the behaviour locally, but am unable to get a test to fail along those lines, which is quite frustrating. I agree this should be fixed though, so will be holding off a non-beta release until I track it down.

[area]

@hiddentao It looks like your error is something to do with eth-gas-reporter. It can't deal with an instrumented contract with that line in it, for whatever reason (even though it is valid solidity - it parses fine for coverage, obviously). Updating the version of solidity-parser-antlr in eth-gas-reporter solved the issue for me.

[area]

So the reason the setup transaction reverted is because ZOS are very cutting-edge, and have already introduced the CREATE2 opcode in their assembly (which was new in Constantinople), which the modified ganache we were using didn't support. The only way to fix that was to bring the modified ganache up-to-date... which I have done!

I have published a new beta which should work for you, @JirkaChadima. I think you also need to add zos to the copyPackages array in .solcover.js, as well as the test command you have already tried. See also the note below about --allowUnlimitedContractSize, which applies to this repository as well. Note that the increased gas use is expected, and is discussed in the FAQs

For everyone else:

• testrpc-sc should now be on par feature wise with [email protected] (cc: @vdrg who was asking after this). It is likely there are some small technical differences between them, beyond the deliberate ones, but I would be surprised if they manifested in any significant way. If they do, please let me know. This is because I based the custom ethereumjs-vm-sc off of the most recent ethereumjs-vm, which isn't necessarily used in ganache-cli.
• If you are using a custom script to start testrpc-sc, you now need to add --allowUnlimitedContractSize to the arguments passed to testrpc-sc. You do not need to do this if you are using the testrpcOptions parameter in .solcover.js.

[JirkaChadima]

You, sir, are awesome! Outstanding work and it's now running as expected in our repo. Thank you very much!

[vibern0]

Hello @area
It's been a while since I said I would get back with the problem I found, sorry for that.
Now the code is open-source (you can see here) and I'm having a new problem which is already reported ParserError: extraneous input '{' expecting ')'
This problem only happens with beta.5. In previous versions, the error is There was a problem instrumenting ./coverageEnv/contracts/FixidityLib.sol: TypeError: Cannot read property 'stop' of null
Anything I can help with? I'm available to fix it.

Thanks

[fernandomg]

Hello @area, thank you very much for the effort put into this project.

I'm facing an issue with this contract: https://github.com/GoodDollar/GoodContracts/blob/master/contracts/BancorFormula.sol

The error:

test-solidity-coverage on  master [✘] is 📦 v1.0.0 via ⬢ v10.15.3 
➜ npm run coverage

> [email protected] coverage /.../test-solidity-coverage
> solidity-coverage

Generating coverage environment
Running: truffle compile  
(this can take a few seconds)...

Compiling your contracts...
===========================
> Compiling ./contracts/BancorFormula.sol
> Compiling ./contracts/ExpArray.sol
> Artifacts written to /.../test-solidity-coverage/coverageEnv/build/contracts
> Compiled successfully using:
   - solc: 0.5.3+commit.10d17f24.Emscripten.clang

Instrumenting  ./coverageEnv/contracts/BancorFormula.sol
Cleaning up...
There was a problem instrumenting ./coverageEnv/contracts/BancorFormula.sol: ParserError: missing ';' at '}' (133:36)
Exiting without generating coverage...

To reproduce:

git clone [email protected]:fernandomg/test-solidity-coverage
cd test-solidity-coverage
npm i && npm run coverage

Thanks!

[mrice32]

Hey @area, thanks for updating everything!

We're currently using the beta and we've been running into a compilation issue related to calling internal functions from a constructor (I think this used to be an issue in solc). The following contract fails to compile after being instrumented:

pragma solidity ^0.5.0;

pragma experimental ABIEncoderV2;

library TestLib {
    struct State {
        uint integer;
    }

    function internalFunction(State storage state) internal {
        state.integer = 5;
    }
}


contract Test {
    using TestLib for TestLib.State;
    TestLib.State state;

    constructor() public {
        state.internalFunction();
    }

    function getInteger() public returns (uint) {
        return state.integer;
    }
}

The error that the compiler gives is:

InternalCompilerError: Assembly exception for bytecode

We've noticed that moving the internal call to a public method seems to avoid the issue. The following contract compiles fine after being instrumented:

pragma solidity ^0.5.0;

pragma experimental ABIEncoderV2;

library TestLib {
    struct State {
        uint integer;
    }

    function internalFunction(State storage state) internal {
        state.integer = 5;
    }
}


contract Test {
    using TestLib for TestLib.State;
    TestLib.State state;

    constructor() public {
        callInternalFunction();
    }

    function getInteger() public returns (uint) {
        return state.integer;
    }

    function callInternalFunction() public {
        state.internalFunction();
    }
}

Two other things to note:

• We have noticed that this only happens when ABIEncoderV2 is enabled.
• These contracts all compile (without instrumentation) with the most recent version of solc.

cc @ptare @rawfalafel @rosalindstengle

[daniel-iobuilders]

Hey @area,
I'm using the lates beta, but seem to have a bit of a strange behavior. The code coverage report shows a function with visibility internal as not covered, even though is it called several times during the tests.

The respective code can be found here: https://github.com/IoBuilders/holdable-token/tree/5a7bfac9bd55ffe91724fbeb06da4cfab5818255

After executing npm install, npm run coverage can be excuted to create the code coverage report. The report shows line 218 as not covered. This is not correct, as it is the only line of its function and the function is called several times. Not sure if it is important, but the function is a getter for block.timestamp.

Thanks for any hints, how I could correct the code coverage report to be created correctly.

[cgewecke]

@daniel-iobuilders Is this the function?

function _getBlockTimeStamp() internal view returns (uint256) {
        /* solium-disable-next-line security/no-block-members */
        return block.timestamp;
    }

It's possible the method is getting optimized out by solc. Is there a reason why you've encapsulated block.timestamp that way? Can the method be replaced by a direct reference to the variable?

[daniel-iobuilders]

@daniel-iobuilders Is this the function?

function _getBlockTimeStamp() internal view returns (uint256) {
        /* solium-disable-next-line security/no-block-members */
        return block.timestamp;
    }

It's possible the method is getting optimized out by solc. Is there a reason why you've encapsulated block.timestamp that way? Can the method be replaced by a direct reference to the variable?

The encapsulation is done to overwrite it in the mock implementation of the contract and use it in the unit tests to manipulate the returned block timestamp. The smart contract has a concept of expiration and in order to test if the expiration logic I need to manipulate the returned timestamp.

I've deactivated the optimizer in truffle-config.js, but get the same result. I will do some more tests to check if it could be related to the compiler.

[cgewecke]

@daniel-iobuilders, re:

use it in the unit tests to manipulate the returned block timestamp. The smart contract has a concept of expiration and in order to test if the expiration logic I need to manipulate the returned timestamp.

There is an alternative way of doing this with ganache which has extension RPC endpoints that let you increase the block time. OpenZeppelin publishes a library of test helpers you might find helpful for this purpose.

[yxliang01]

Is there a way to keep both the support of Solidity 0.5 and 0.4? While the newer version of Solidity is mandatory, would be great if the old stable version 0.4 can be retained until 0.6 is out.

[cgewecke]

@yxliang01

Is there a way to keep both the support of Solidity 0.5 and 0.4? While the newer version of Solidity is mandatory, would be great if the old stable version 0.4 can be retained until 0.6 is out.

Does this mean you'd like a version of the stable which has a newer ganache client?

[yxliang01]

Not only that. @cgewecke The PR description says that the modification in this PR breaks support of Solidity 0.4 at all. This indicates that upon merging this PR, all subsequent improvements to solidity-coverage can't be applied to projects requires solc. What I proposed is that don't break Solidity 0.4 support until support for Solidity 0.6 is ready.

[cgewecke]

@yxliang01 I've just created a branch from the current master called v5-stable where dependency updates for a solidity v4 compatible version will be possible. But I don't think we can provide backwards compatibility for more substantive improvements there.

This PR is getting merged today because it is the improvement that's necessary.

Let's continue discussion about v5 at #331.

[cgewecke]

This is fantastic. Thanks @area and @pinkiebell.

Merging. Am going to add some CI stuff in separate PR and publish shortly.

[fabianorodrigo]

@cgewecke , this merge you're doing will end up with a new tag release and is going to be update on npmjs.org? My doubt is: when you finish, I can just execute 'npm install solidity-coverage' to get the merged package?
Thanks

[cgewecke]

@fabianorodrigo Yes. It's not published to npm yet, but will ping this PR when it is.

[fabianorodrigo]

Based on the leapdao fork, with a nod to #311 as well. Also trying to get dependencies up-to-date.

This branch is currently published under the beta tag; you can try it out by npm install --save --dev solidity-coverage@beta. Bug reports would be very welcome before I release properly. You must be using solidity 0.5.1 or higher (0.5.0 will not work).

I received the following message when I tried to run solidity-coverage@beta on the project http://github.com/leapdao/solEVM-enforcer

> Artifacts written to /home/xpto/leapdao/solEVM-enforcer/coverageEnv/build/contracts
> Compiled successfully using:
   - solc: 0.5.2+commit.1df8f40c.Emscripten.clang

Instrumenting  ./coverageEnv/contracts/Enforcer.sol
Instrumenting  ./coverageEnv/contracts/EthereumRuntime.sol
Instrumenting  ./coverageEnv/contracts/EVMConstants.sol
Instrumenting  ./coverageEnv/contracts/EVMRuntime.sol
Cleaning up...
There was a problem instrumenting ./coverageEnv/contracts/EVMRuntime.sol: ParserError: no viable alternative at input 'els{' (97:17)
Exiting without generating coverage...

[cgewecke]

@fabianorodrigo That bug was fixed on master by #333. Am running the solEVM-enforcer tests rn on my box and they look ok (they instrumented fine and have passed a few hundred tests without problems).

Hoping to publish 6.0 today or tomorrow, just waiting to see if the parser is going to get a new version tonight.

[cgewecke]

@fabianorodrigo 0.6.0 is published.