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?