Cannot run coverage on imported contracts from node_modules

[alecps]

Currently, you can only run coverage on custom contracts located in the /contracts directory and not on any npm installed contracts that are inherited by the custom contracts. Some developers need to demonstrate full test coverage for all smart contracts used in their projects, including those in node_modules. However, it is also considered bad practice to move npm installed contracts out of node modules and into the /contracts directory alongside custom contracts. There should be an option to configure .solcover.js to instrument all inherited contracts including those in node_modules.

I've suggested a solution for this in PR #375

[alcuadrado]

This is an interesting suggestion.

Some developers need to demonstrate full test coverage for all smart contracts used in their projects

Can you give more info on this? I'm just curious.

[PaulRBerg]

Just like @alcuadrado, I'm also curious to find out more about your use case.

As far as I know, everyone expects the files under "node_modules" to be production-ready. That is, no tests or coverage should be run against them.

It is also considered bad practice to move npm installed contracts out of node modules

Yes, that is bad practice.

Update

It seems Alec left more comments on the PR page at #375.

[alecps]

I agree that the files under node_modules are expected to be production ready. However, I have encountered requests from mangers to demonstrate full coverage on all contracts, and needed to fork the tool in order to satisfy those requests. It's probably not a feature every dev will need, so if it's a pain to add it feel free to close

[cgewecke]

@AlecSchaefer In defense of your proposal - nyc (the JS coverage tool) has a special option to allow this so it's a valid use case in JS world.

My original feeling was that if there were coverage gaps in the dependencies ideally one would open PRs addressing that deficit at the source. But I can see how that might not be acceptable (or convenient) for some risk regimes.

In some cases it might be quite difficult - dappsys is an example.

@AlecSchaefer @PaulRBerg Do you either of you have a link / citation for the judgement that it's bad practice to maintain local versions of the deps? Not questioning correctness of this view but it would be nice to have an authoritative reference in the thread. Maybe the principle has some flexibility.

[PaulRBerg]

Do you either of you have a link / citation for the judgement that it's bad practice to maintain local versions of the deps?

Not off the top of my head, but I do have indirect proof. The node ecosystem is increasingly moving towards a node_module-less world (see npx, yarn pnp), hence in the future it won't even be possible to move a dependency out of "node_modules" because there will be no "node_modules".

However, forking and installing from there is totally fine. Having your changes on GitHub is a much better and cohesive way of maintaining diffs. But yeah this is not a hard and fast rule and it might be just an anecdote from me and @alcuadrado.

[alcuadrado]

Just to avoid confusion: I think adding this option is a good idea. At Nomic Labs we conduct smart contract security audits, so I empathize with such a strict testing requirement, and that was the reason for my curiosity.

[alecps]

The best authoritative citation I could find saying that it's bad practice to move contracts out of node_modules is in Zeppelin's public audit of Augur. In the "Medium severity" section, the audit recommends to Install OpenZeppelin via NPM. Their reasoning is partly because of a licensing issue, but they also point out that maintaining dependency contracts locally "makes it difficult and error-prone to update to a more recent version," and to, "Consider following the recommended way to use OpenZeppelin contracts, which is via the zeppelin-solidity NPM package." This advice is echoed in subsequent comments as well. I can also add that I've heard the same advice during security audits.

[cgewecke]

@AlecSchaefer Excellent, thank you.

[barakman]

@cgewecke: So where does this issue stand then?

I am getting TypeError: Overriding function changes state mutability from "view" to "nonpayable". when my contract imports from a preinstalled package (i.e., imports a contract located under node_modules).

I can resolve the problem by copying that contract (from under the node_modules folder) into my "local" contracts folder, and then importing it from there instead (i.e., via an import statement which starts with ./).

But it makes very little sense to resolve the problem this way.

BTW, the problem appears to be solidity-coverage removing the view in that function's declaration.

Now, I know that it has to be done in order to instrument the function and allow it to emit events.

What I don't understand is, how come this problem is resolved when the import is from my "local" contracts folder instead of from the "remote" node_modules folder.

Thanks

[cgewecke]

What I don't understand is, how come this problem is resolved when the import is from my "local" contracts folder instead of from the "remote" node_modules folder.

Unfortunately I also don't understand this. Tbh 6.x has been substantially rewritten in 7.x because the strategy of modifying the state mutability modifiers to permit Event statements has been brittle.

The most common failure comes from Truffle running compile outside of solidity-coverage's direct control. When you execute - you should see only two compilation cycles. If there's a third right before test executes, it means that Truffle's profiler is (incorrectly) detecting that more things need to be compiled and destroying the work solidity-coverage has just done.

Your description reminds me a little of #371. Is it possible you have identically named contracts in your inheritance tree, one in the contracts folder, one being imported from node_modules?

[barakman]

@cgewecke : Thank you for your very quick response.
No, that's not my case (as far as I can tell; hopefully I'm not missing anything here).
I actually experience two different types of failures under two different setups (haven't yet minimized those setups to be posted here).

One type is what I've described above - the view classifier is omitted, thus overriding function changes state mutability from "view" to "nonpayable".

The other one is even more severe IMO:

For that same function, no compilation error is issued, but when called from my Truffle test, instead of returning the expected value, it returns a tx-receipt object, which is exactly what you'd expect from a non-constant function.

Now, again, I'm not sure how SC handles those constant functions in general (I mean, I understand that it changes them to non-constant, but I don't know how they "behave" like constant functions when called from a Truffle test).

But the two error types that I've mentioned above seem to be two sides of the same problem - a function which has changed from constant to non-constant (with the only difference being that in one case we get a compilation error and in the other case we get a runtime error).

Here is a "kinda-MVP" for the first scenario (compilation error) in case you'd like to investigate it:
https://drive.google.com/open?id=1Y808UoA8F5San5GQJqJ63v8seyeYzQ12

Use:

• npm install
• npm test 1 in order to run truffle test
• npm test 2 in order to run solidity-coverage

P.S.:
As you can see, I also had to add a "dummy" contract (Importer.sol) in order to force Truffle to create an artifact required by one of the tests (otherwise, that test fails upon attempting to instantiate the required contract).
The more I think about it, the less I'm inclined to find a solution to this mess (which appears in both Truffle and SC); instead, I'm starting to think of copying the required contracts from the node_modules folder into my contracts folder at the end of npm install.

Thanks again :)

[cgewecke]

@barakman I will check out the example. People are often alarmed when they see the modifiers removed, but behind the scenes the ABI's are rewritten so that truffle calls those methods instead of sending them as transactions. What you've described is consistent with the 'unwelcome third compilation' phenomenon, where Truffle undoes the ABI modification and all of that work goes awry. You get a receipt instead of a value, etc.

The more I think about it, the less I'm inclined to find a solution to this mess (which appears in both Truffle and SC); instead, I'm starting to think of copying the required contracts from the node_modules folder into my contracts folder at the end of npm install.

Oh yes, this is a sound approach.

[barakman]

@cgewecke:
I've checked this, and found that your 'third-compilation' conjecture doesn't seem to be the case on my setup (i.e., in both cases, there appear to be exactly two compilations).

The difference appears to be that the Skipping instrumentation of step is applied on contracts located under my contracts folder, but not on contracts located under the node_modules/some_package folder.

I'm going to look into that now (in file ./node_modules/solidity-coverage/lib/app.js)...
Sorry, that's just the result of me changing the skipFiles configuration of solidity-coverage.

But the fact is, taking a working project and adding even just an empty contract which imports from the preinstalled package, already yields a mess.
In this case, it does indeed add a third compilation, though not from the runCompileCommand function (not really sure where this one is triggered from).
And this time, the end result is not a compilation error of overriding function changed from "view" to "nonpayable", but a runtime error due to the fact that the function returns a receipt instead of the expected value.
Oh, it's triggered by the runTestCommand function, which calls truffle-test, which in turn detects that it needs to recompile a few contracts (I guess it determines that some of the JSON files aren't up-to-date).