Require constant, view, and pure methods be invoked by `.call`

[cgewecke]

With 0.3.5 we've begun using a testrpc that implements the Byzantium fork. Unfortunately we're also pushing up against the limits of solidity-coverage's current design and there were some new bugs discovered while doing the upgrade:

• highest working pragma is 0.4.18. Above that you will get compilation errors complaining that the non-state-changing nature of constant and view methods has been violated.
• pure modifier compiles but returns a transaction object rather than a value. If your tests check / use that value they will fail.

[area]

This is because of a Truffle convenience that people have got used to, not Solidity/EVM, and has a workaround (though will require a modest buy-in from users).

Normally, with the contract objects floating around truffle, if you call a function that is going to have an effect on the blockchain, you do something like contract.setVariable(), and you get a transaction object back. If you want to get the return value back from a function, then what you're meant to do is contract.getVariable.call(), which does not broadcast a transaction, and returns the value. However, Truffle is trying to be convenient, and in the case of a function being marked as constant/pure/view, will map contract.getVariable() on to contract.getVariable.call(), and then people can just do contract.getVariable(). This is because Truffle is assuming you will never want to broadcast a transaction that has no effect on the blockchain. If we remove those modifiers in preprocessing, we get the behaviour you've described if we're just calling contract.getVariable() in our tests.

TLDR: For function calls where you're testing against return variables, everyone should always be using contract.function.call().

EDIT: If I'm right, this change probably requires 0.4.0 (i.e. a version bump signifying a change in behaviour).

[cgewecke]

Yes, agree - thanks for pressing for this. TBH I failed to register this was an issue at all when we did the view work a few weeks ago. And agree that .call is a better pattern anyway.

[cgewecke]

@area Closing, re-open if you wish. Thanks again.

[frangio]

Can you explain why it will be needed to add .call()? It's pretty inconvenient, and I'd like to help think of an alternative solution.

[cgewecke]

@frangio Thank you, that would be wonderful. Also agree that this is annoying and ideally solidity-coverage should be agnostic about how these methods are invoked.

Quick overview of how SC currently works:

1. We instrument solidity by parsing the code and injecting solidity events into the text files at key measurement points like lines, conditional branches, statements, etc.
2. We compile the instrumented code and run the tests on a hacked vm that inspects every event as it passes through the LOG opcode and writes the coverage events to file
3. We translate the collected data into the format Istanbul requires and generate a report.

With 0.4.18 0.5.0 solidity will not compile methods marked as view or pure if they contain events. The only way we see of getting around this is to strip the modifiers off pre-compilation and require that those methods be invoked by .call. If you can see a better approach we'd be happy to use it.

Additionally: we have near term plans to refactor this project and abandon the event injection strategy because its quirks and bugs are piling up in an unsustainable fashion.

@area Has proposed that we use testrpc's new debug_traceTransaction endpoint to track code execution in memory. There's the beginning of a discussion at #111 and #150 about how we might do this but it looks like handling .call will continue to be one of the key technical challenges we face.

If you (or anyone you know) is interested in working on this, we're open! We need new everything.

Thanks again @frangio.

[spalladino]

With 0.4.18 solidity will not compile methods marked as view or pure if they contain events. The only way we see of getting around this is to strip the modifiers off pre-compilation and require that those methods be invoked by .call. If you can see a better approach we'd be happy to use it.

Would it be possible to...?
1- Compile the contracts before instrumentation and store the generated ABIs (which should have the required annotations, so pure/view functions are always handled via call by the client library)
2- Remove the annotations, instrument and compile
3- Overwrite the ABIs generated in step (2) with the ones from step (1)

If I understand correctly, truffle contracts (or whichever library is used for calls) should just check the ABI for constant flags to determine whether to use call or send. Also, function SHAs are unaffected by the constant/pure/view modifiers, and changing the underlying binary, if it contains the same functions, should also work. Am I missing something?

[frangio]

An alternative that doesn't involve compiling twice would be to compile without annotations, and then manually modify the generated schemas setting the "constant" field of pure/view methods to true.

[cgewecke]

@spalladino @frangio Yes! Smart!! The first idea is good and the single compilation refinement is even better.

However, wondering if you'd be open to a short term (4 weeks?) compromise. This is the kind of change we prefer to test E2E on our Zeppelin fork because it adds several steps to the execution sequence and invariably there are issues we fail to catch with unit tests. If you temporarily go to ^0.4.17, merge the PR with the new testrpc and use solidity-coverage 0.3.5 that should require no code changes on your part, allow you to make lots of solidity updates in Zeppelin (bar pure), and buy us time to make this change / test it adequately. At the moment we can't test against Zeppelin because it's pre-fork.

If that doesn't work for you, I understand - no worries. The context for us is that this kind of fix is like giving antibiotics to a patient who needs a heart transplant. We probably need to do it to keep them alive - we also need to get moving on the main event.

Open to other ideas, any suggestions. . .

[frangio]

@cgewecke We're actually about to upgrade to the latest Truffle with Solidity 0.4.18. We're considering removing coverage temporarily (OpenZeppelin/openzeppelin-contracts#573), but 4 weeks until it's fixed might be too long. 😕

[cgewecke]

@frangio Well it's not the end of the world - just do whatever you think is best in the interim. As long as you upgrade to Byzantium we'll have a way to sanity check the implementation whenever it gets done. If someone wants to open a PR here they're welcome to as well.

[cgewecke]

@frangio This likely makes no difference for your case because you probably want to use pure normally, but I have misidentified the solc version that actually enforces the view/constant visibility restrictions. It looks like it will be v 0.5.0 - we have been testing those cases with the experimental pragma. . . . very sorry for the confusion here.

People who don't need to invoke their pure functions like contract.iAmPure() can use 0.4.18 and solidity-coverage 0.3.5.

[area]

The branch fix/callNotRequired contains my swing at fixing this 'properly'. During the original parsing of the contracts done to get the source mapping, we record which functions are marked as constant, pure, or view. We then compile the contracts with all of those modifiers stripped, as before, and set the constant parameter in the ABI to true for the functions we recorded as constant.

Two tests fail to pass on that branch, and I can't spot why. I'm hoping @cgewecke can come to my rescue, there. They are

  1) app config with test command options string: should run test
  2) app config racing test command: should run test after testrpc has started

They appear to time out.

Other issues with the branch that should be discussed:

• This integrates us even more closely with Truffle. I have included a compileCommand option, but it looks for the JSON files corresponding to the contracts in the ./build/contracts/ directory, which is obviously Truffle specific. Even potentially more problematic, the JSON files generated by truffle are do not contain just the ABI, but the abi is just one property in the object. Do we know of anyone using solidity-coverage without truffle?
• Other related properties in the ABI are ignored currently, specifically payable and stateMutability. We should change these too to guard against changes to Truffle's behavior in the future, but I'm unsure currently what the valid combinations are. The Solidity Documentation has what we need, though.

I have not opened a PR yet, but if you want to move discussion there, I'm happy to do that too.

[cgewecke]

@area Thank you, that looks good to me. If you open a PR I'll fix those tests and will update our Zeppelin fork to their latest / make sure it passes.

[Edit] - My view re: other ABI properties is that the constant fix is likely good for a while. We can probably safely watch them and see if they're going to break us, respond, etc.

[Edit II] - I don't know of any non-truffle users except Melonport, and they're moving to DappHub which is non-testrpc anyway.

[cgewecke]

@spalladino @area

Ironically, while this ABI rewrite idea will work for many projects, it's running into problems with Zeppelin. truffle compile only targets the contents of the contracts folder. Zeppelin tests much of their code base indirectly via .sol mocks stored in the test directory. These only get compiled on truffle test (which assumes they are solidity tests). TLDR; we can't capture/modify the necessary Zeppelin ABI's in a separate step.

Example of the ABI modification strategy failing on recent Zeppelin in Travis.

Possible I'm overlooking an obvious workaround here - in any case would welcome ideas from you about the best way forward. Or a correction if I've misunderstood what's wrong.

Some possibilities:

• create a "barrel" file at Zeppelin in the contracts folder which imports all the mocks so they get picked up by compile. (We'll need to modify our code a little on this end to make this work, but it's do-able).
• Move the mocks into the contracts folder at Zeppelin and explicitly exclude them from coverage.
• Fork solidity-coverage and implement a custom compilation strategy for Zeppelin's case using Truffle components / build options.
• Capitulate to the temporary 0.3.5 solution suggested above, invoke pure by call. (FWIW - this is my least favorite - ABI modification is probably an improvement to the current state of things for most users).

[frangio]

Interesting.

(which assumes they are solidity tests)

Are you sure this is true? I don't think it assumes this, because Solidity test contracts need to be named like Test*. My understanding is that it compiles them when it sees the artifacts.require. Would it be possible to hook onto that perhaps?

[frangio]

Nevermind, what I said about artifacts.require doesn't seem to be true. 😛

[frangio]

I think we can move the mocks into the contracts/ folder and make sure they're not included in the NPM package. What do you think @spalladino?

[cgewecke]

@frangio Thanks, moving mocks to contracts is good here anyway - we'd just keep the current single-compilation solution and publish. Leaving this open for a bit for any other views....

[Edit - actually I think we'll still need to compile twice and substitute the original ABI, or have a dedicated instrumentation sweep for these mods].

[spalladino]

How about manually creating a symlink to test within contracts right before running the coverage step?

[cgewecke]

Very sorry for the delay getting this done. In the end this has been implemented using @spalladino's original idea - ABI swapping. Unfortunately was not able to get a symlink to work on our Zeppelin fork. Happy to discuss this further in our Gitter though.

Closing here and will make additional notes at Zeppelin.

Thank you @frangio @spalladino for thinking about this and coming up with a good solution :)
Thanks you @area for working on this :)

[spalladino]

That's great @cgewecke! And please, don't apologise about delays - getting this work done out of your own free time is awesome. Let's follow up in OpenZeppelin/openzeppelin-contracts#575 as you say.