Adding node dependencies includes a lot of unnecessary contracts

[sohkai]

In our use case, one of our contract packages depends on another contract package. However, copying this dependency into the coverage environment results in a ridiculous number of extra contracts being instrumented, due to the inclusion of the dependency's node modules.

The full list of contracts being instrumented in that repo: https://gist.github.com/sohkai/459dc9ab1b3a410e9af6ab9ae07a196c.

It would be great if we had a way to target certain folders or files in the dependencies for instrumentation, and a way to skip the rest of node_modules.

[cgewecke]

Oh my, that list is enormous. Which repo are you trying to cover? I'd just like an example to test a solution against.

@bingen Submitted a PR here the other day that lets you target specific org namespaced folders / subdirectories in the copyPackages option. Does that help with this problem at all?

[sohkai]

Not quite, the list is that large because the right org packages are being copied :).

This would be a separate way to specifically target contracts in dependencies for instrumentation and ignore the rest.

The above list corresponds to aragon-apps/apps/finance, which depends on aragon-apps/apps/vault as well as aragonOS.

[sohkai]

It turns out that our particular set of issues with coverage being recursively instrumented was fixed in https://github.com/aragon/aragon-apps/pull/863 by blacklisting specific dependencies in the skip list (e.g. see example change).

This was probably due to some confusion on our side about how copyPackages and the skip list works.

[cgewecke]

@sohkai Sweet ( it's definitely super confusing / kind of a nightmare. )

Was looking over the coverage runs in aragon-apps...

• Agent: Believe these have been failing because of assembly staticcall in the sig validation logic. This may be fixed in the new client (6.4.5).
• Also see testrpc-sc explicitly listed as a dep in some of your apps and it might make sense to delegate that to solidity-coverage. We now use a pre-release postfix on the package (ex: 6.4.5-sc.3) so we can patch while we track the ganache base version. Is semver weird about that?
• Payroll looks like it has compilation failures stemming from blacklisting the ppf contracts. Instrumentation speeds on 0.6.x are better and it's possible you could blacklist less without being destroyed by it.

[sohkai]

Thanks for taking a look into them @cgewecke!

Agent: Believe these have been failing because of assembly staticcall in the sig validation logic. This may be fixed in the new client (6.4.5).

I believe these Agent tests are still failing with solidity-coverage 0.6.1, using the new client, but I will try updating them to sc.3.

Also see testrpc-sc explicitly listed as a dep in some of your apps and it might make sense to delegate that to solidity-coverage. We now use a pre-release postfix on the package (ex: 6.4.5-sc.3) so we can patch while we track the ganache base version. Is semver weird about that?

This is more of a lerna thing than anything else; we need to list it separately because otherwise lerna won't copy over the included binary into the individual package, and running npx testrpc-sc doesn't work :(.

[cgewecke]

@sohkai Yes 6.4.5-sc.3 might actually make a difference, I saw some staticcall stuff clear up on the bancorprotocol contracts with that change yesterday.

lerna won't copy over the included binary into the individual package,

Oooh. I didn't know that, thanks.

[cgewecke]

@sohkai One last thing (tangentially) that might interest you...

Maker engineer Kris Kaczor has built a cool CI service called codechecks which eth-gas-reporter now has an integration with. It's kind of like coveralls for gas - it shows a pull request's gas consumption changes relative to its target branch in the Github UI and comes with a diff report. (It's free :)).

There's setup/config info here if this is something you could use.

[sohkai]

@sohkai Yes 6.4.5-sc.3 might actually make a difference, I saw some staticcall stuff clear up on the bancorprotocol contracts with that change yesterday.

Can confirm wit sc.3 that these staticcall tests are working for us now as well 👍.

Will check out the gas check in CI! That looks super cool :). Quick question on it; how well does it support monorepo tests? Should it try and pipe the results of each separate package together, kind of like what happens with lcov?

[cgewecke]

This should be a non-issue with newly published 0.7.0. We're no longer copying any packages & the tool runs at the same layer as the project root.

However if you run into any weird issues using Lerna with the new version, please just lmk.

A monorepo called Sabiler has been able to run the new version without serious problems so hopefully everything's ok.