Solidity-coverage: Internal calls from constructor fail to compile

[mrice32]

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

[mrice32]

This seems very similar to this issue on older versions of the Solidity compiler. Not sure what would make it show up again - the above was compiled with 0.5.13.

[cgewecke]

@mrice32

(Assuming you are using the 0.7.0-beta)

This sounds a lot like the reports in #417. There, it's been attributed to solidity-coverage forcing solc to compile without optimization. Do you get the same error if you disable optimization in a regular test run?

[mrice32]

Thanks for the quick response. We're currently using the latest stable release, 0.6.7.

I'll try 1) compiling with the optimizer disabled and 2) a coverage run with the 0.7.0-beta and let you know the results. Thanks!

[cgewecke]

@mrice32 Ah cool ok, just lmk if you run into problems...there are links to the 0.7.0 docs and an upgrade guide at the top of the current README.

[mrice32]

I tried compiling with the optimizer off, and I had no problems, which leads me to believe that this may be a distinct issue from #417

As for the beta, do you suspect that installing it may solve this issue assuming it's not a duplicate of #417? I only ask because the beta may take me a bit longer to set up since we have a bit of custom plumbing built around the 0.6.x. We'll get around to this eventually, but I just don't want to gate solving this issue on installing the beta if it's unrelated. Thanks for your help!

[cgewecke]

@mrice32 Yes I do suspect that (unfortunately). The beta uses a much less problematic instrumentation strategy and if you can compile without optimization normally you shouldn't see that error.

Could I ask what your additional plumbing is like?

[mrice32]

It's not super complicated, it's just these files:

https://github.com/UMAprotocol/protocol/blob/master/ci/coverage.sh
https://github.com/UMAprotocol/protocol/blob/master/common/globalSolcoverConfig.js

I'll need to work through the beta instructions, which will almost certainly get rid of or simplify most of this code.

I'll move to the beta and let you know what I find! Thanks!

[mrice32]

The contracts seem to compile correctly with the beta, which is great news! Thanks for your help.

I've attempted to upgrade our repo to the beta, but I've run into an issue that I can't seem to resolve. I'm not sure if it's a coverage issue or a truffle issue.

This error throws before any tests are run.

> Istanbul reports written to ./coverage/ and ./coverage.json                                                                                                   
> solidity-coverage cleaning up, shutting down ganache server                                          
Error: Error: Error                                                                                                                                                           
    at Object.run (/Users/matt/git/protocol/core/node_modules/solidity-coverage/dist/plugin-assets/truffle.library.js:194599:13)                                     
    at processTicksAndRejections (internal/process/task_queues.js:86:5)
Truffle v5.0.21 (core: 5.0.21)                                                                                                                                       
Node v11.15.0     

Not sure why the error has no description, but I narrowed it to this line in our migrations: https://github.com/UMAprotocol/protocol/blob/285311051c2a0847ac2b0ede399d6e066e6d64c3/core/migrations/10_deploy_tokenized_derivative_creator.js#L31. It's the only place where we link a library, so my guess is it has something to do with linking. Are you aware of any issues related to linking libraries?

Side note: this error does not show up when running tests normally.

[mrice32]

Also, please let me know if you'd prefer that I move this discussion to a new issue since it's unrelated to the title.

[cgewecke]

@mrice32

In your package.json Truffle looks pinned at 5.0.21. Is it possible to upgrade to a newer Truffle version and see if the regular tests work?

Then, if they do - try solidity-coverage again with the newer version. The beta relies on plugin resources that were added after 5.0.31. It tries to handle older Truffle versions but maybe that's not working too well.

This thread is fine! Super into finding and fixing any bugs on the beta.

[mrice32]

Thanks for the tip - I'l make sure I run with >= 5.0.31. I locally upgraded to the newest truffle release 5.1.0, and got the same error:

> Istanbul reports written to ./coverage/ and ./coverage.json
> solidity-coverage cleaning up, shutting down ganache server
Error: Error: Error
    at Object.run (/Users/matt/git/protocol/node_modules/truffle/build/webpack:/packages/migrate/index.js?abc3:92:1)
    at processTicksAndRejections (internal/process/task_queues.js:86:5)
Truffle v5.1.0 (core: 5.1.0)
Node v11.15.0

[cgewecke]

Also @mrice32 if you push your change-set to UMA I'd be happy to take a look and debug locally.

[mrice32]

Sure, here's the branch I'm using: https://github.com/mrice32/protocol/tree/coverage. Sorry if it's a little messy!

[mrice32]

To run coverage you'll need to run the following commands from the top level directory:

npm install
./ci/coverage.sh core

[cgewecke]

Awesome, checking it out rn.

[cgewecke]

Ok! In coverage.sh

- $(npm bin)/truffle run coverage --network coverage
+ $(npm bin)/truffle run coverage --network ci

...gets the tests running. Still debugging though because I see a couple failing.

[cgewecke]

I got this:

But I see this set of errors - not sure what's expected here. Can you clarify?

 1) Contract: Registry
       Initialize roles:
     TypeError: registry.initializeRolesOnce is not a function
      at Context.<anonymous> (test/Registry.js:30:44)
      at process._tickCallback (internal/process/next_tick.js:68:7)

  2) Contract: Store
       Compute fees basic check:
     Error: Invalid number of parameters for "computeRegularFee". Got 4 expected 3!
     at PromiEvent (/Users/cgewecke/code/sc-forks/protocol/node_modules/truffle/build/webpack:/packages/contract/lib/promievent.js?f772:9:1)
      at TruffleContract.computeRegularFee (/Users/cgewecke/code/sc-forks/protocol/node_modules/truffle/build/webpack:/packages/contract/lib/execute.js?9956:121:1)
      at Context.<anonymous> (test/Store.js:35:28)
      at process._tickCallback (internal/process/next_tick.js:68:7)

  3) Contract: Store
       Compute fees at 20%:
     Error: Invalid number of parameters for "computeRegularFee". Got 4 expected 3!
     at PromiEvent (/Users/cgewecke/code/sc-forks/protocol/node_modules/truffle/build/webpack:/packages/contract/lib/promievent.js?f772:9:1)
      at TruffleContract.computeRegularFee (/Users/cgewecke/code/sc-forks/protocol/node_modules/truffle/build/webpack:/packages/contract/lib/execute.js?9956:121:1)
      at Context.<anonymous> (test/Store.js:52:28)
      at process._tickCallback (internal/process/next_tick.js:68:7)

  4) Contract: Voting
       Constructor:
     TypeError: voting.initializeOnce is not a function
      at Context.<anonymous> (test/Voting.js:67:16)
      at process._tickCallback (internal/process/next_tick.js:68:7)

  5) Contract: scripts/Voting.js
       basic case:
     Error: Returned error: VM Exception while processing transaction: revert The current voting round has not ended
     at PromiEvent (/Users/cgewecke/code/sc-forks/protocol/node_modules/truffle/build/webpack:/packages/contract/lib/promievent.js?f772:9:1)
      at TruffleContract.getPrice (/Users/cgewecke/code/sc-forks/protocol/node_modules/truffle/build/webpack:/packages/contract/lib/execute.js?9956:121:1)
      at Context.<anonymous> (test/scripts/Voting.js:90:31)
      at process._tickCallback (internal/process/next_tick.js:68:7)

  6) Contract: scripts/Voting.js
       simultaneous and overlapping votes:
     Error: Returned error: VM Exception while processing transaction: revert The current voting round has not ended
     at PromiEvent (/Users/cgewecke/code/sc-forks/protocol/node_modules/truffle/build/webpack:/packages/contract/lib/promievent.js?f772:9:1)
      at TruffleContract.getPrice (/Users/cgewecke/code/sc-forks/protocol/node_modules/truffle/build/webpack:/packages/contract/lib/execute.js?9956:121:1)
      at Context.<anonymous> (test/scripts/Voting.js:163:31)
      at process._tickCallback (internal/process/next_tick.js:68:7)

  7) Contract: scripts/Voting.js
       Intrinio price:
     Error: Returned error: VM Exception while processing transaction: revert The current voting round has not ended
     at PromiEvent (/Users/cgewecke/code/sc-forks/protocol/node_modules/truffle/build/webpack:/packages/contract/lib/promievent.js?f772:9:1)
      at TruffleContract.getPrice (/Users/cgewecke/code/sc-forks/protocol/node_modules/truffle/build/webpack:/packages/contract/lib/execute.js?9956:121:1)
      at Context.<anonymous> (test/scripts/Voting.js:199:32)
      at process._tickCallback (internal/process/next_tick.js:68:7)

  8) Contract: scripts/Voting.js
       Constant price:
     Error: Returned error: VM Exception while processing transaction: revert The current voting round has not ended
     at PromiEvent (/Users/cgewecke/code/sc-forks/protocol/node_modules/truffle/build/webpack:/packages/contract/lib/promievent.js?f772:9:1)
      at TruffleContract.getPrice (/Users/cgewecke/code/sc-forks/protocol/node_modules/truffle/build/webpack:/packages/contract/lib/execute.js?9956:121:1)
      at Context.<anonymous> (test/scripts/Voting.js:252:32)
      at process._tickCallback (internal/process/next_tick.js:68:7)

  9) Contract: scripts/Voting.js
       Numerator/Denominator:
     Error: Returned error: VM Exception while processing transaction: revert The current voting round has not ended
     at PromiEvent (/Users/cgewecke/code/sc-forks/protocol/node_modules/truffle/build/webpack:/packages/contract/lib/promievent.js?f772:9:1)
      at TruffleContract.getPrice (/Users/cgewecke/code/sc-forks/protocol/node_modules/truffle/build/webpack:/packages/contract/lib/execute.js?9956:121:1)
      at Context.<anonymous> (test/scripts/Voting.js:280:32)
      at process._tickCallback (internal/process/next_tick.js:68:7)

One thing that might be important is to set the network_id param for the launched client - I see that specified in the "ci" network in the truffle config.

In .solcover.js I think you'd put:

module.exports = {
  providerOptions: {
    network_id: 1234
  },
  skipFiles: ["Migrations.sol", "echidna_tests", "test"]
};

[mrice32]

Gotcha. That makes a lot of sense. Just out of curiosity, why can't you just use * for the network id?

I'm guessing those test errors have to do with some of the other stuff that I changed, but I'll dig in and double check. Regardless, I think this is really close to working. I really appreciate you taking the time to help!

[cgewecke]

@mrice32 "*" is very likely fine, was just mentioning it in case there's some network id specific logic in your code. Often people give that a concrete value when they're using it to toggle something on/off.

I really appreciate you taking the time to help!

No problem! Thanks for trying the beta and just ping this ticket if there's more to fix.

Closing for now though because the 0.6.x branch isn't going to get further development.