Constant / view / pure functions still requiring .call()

[sohkai]

Using 0.4.9.

#161 seems like a pretty reasonable thing to do, and it should work, but I'm having trouble with it on aragonOS.

From what I gather, the problem seems to be from truffle test compiling the contracts again. I see the compiler running three times: once for the original, once for the modified, (at this point the modified's ABI is switched out for the original, and I can verify this in the coverageEnv/builds folder), and then again when truffle test is run.

I'm not familiar with how everything is hooked up in truffle, but running through the code, it seems to be that:

1. Running truffle test will compile any necessary files with the test files (it first checks for any updated files, and here it doesn't find any)
2. The compiler is invoked for any necessary files, and then with all the necessary files' dependencies. The only necessary files at this point are the test mocks that we use.
3. Now tests are run, with contract artifacts being created, etc. When creating the contract artifacts in the test, truffle checks if the function in the abi is constant or not, and if so, provides the .call() for us. This last bit is the weird part: I can confirm in the coverageEnv/build folder that the ABIs have been swapped, and that any constant functions are appropriately marked. However, the above check when creating the contract artifacts seems to be failing for a lot of these functions, leading me to think that the re-compile done by truffle test is causing a newer version of the ABI to be used.

I tried to see if not compiling the tests' dependencies would work... but that started giving me parser/compiler errors D:

[cgewecke]

Hi @sohkai, thank you for saying #161 seems reasonable - unfortunately it was the least bad of two bad choices. It also only works if the compilation step runs twice rather than three times.

I think the underlying issue is the placement of solidity mocks in the testing folder. Truffle assumes these are files it should process specially as solidity tests and compiles them on the fly. FWIW Zeppelin (whose engineers proposed this clever ABI rewriting strategy) structured their tests the way you have at Aragon and ultimately had to move their mocks into the root contracts folder and npm ignore them for publication. That work was done in ZS PR 617.

Does that seem like it maps to your case? There's a more extensive discussion here at #146.

[sohkai]

Yes, that does. I just saw another repo do this, and was about to try it myself. Thanks for the heads up, I'll report back on how it goes!

As an aside, let's put something in the docs for this (unless it's already there and I missed something?) :).

[cgewecke]

Agree! I'll do that this morning.

[sohkai]

Trying this now, and it seems to work if you're running JS tests 🎉.

However, for solidity tests, would it still force a third compile?

[cgewecke]

@sohkai Yes, it will recompile anything in the tests directory, including contracts that are imported by solidity tests. Native solidity tests as such are a problem.

This tool is scheduled to be re-written to use an in-memory execution trace instead of an event based instrumentation strategy. We're kind of hoping to get around a bunch of these issues that way - the current approach is increasingly difficult to set up and quite brittle.

[cgewecke]

Updated the docs here. Let me know if you think anything's missing or could be clearer - sorry about the confusion around this.

[sohkai]

Awesome, it looks good! We might not be able to upgrade yet because we do have some solidity test files, so looking forward to the new version :).

[sohkai]

Closing with aaaab91