Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add emitFreeLogs option - Used for debugging/coverage #378

Merged
merged 2 commits into from
Nov 1, 2018

Conversation

Agusx1211
Copy link
Contributor

@Agusx1211 Agusx1211 commented Nov 1, 2018

Some tools for debugging and creating coverage reports of contracts relay on adding events before compiling and testing, this is approach was proven to work on the solidity-coverage project https://github.com/sc-forks/solidity-coverage

By doing this, they elevate the cost of all the calls, and they can't cover views and pure functions, because the EVM will throw when those functions try to emit an event.

This emitFreeLogs option allows instantiating an EVM instance without this limitation, enabling any contract to emit an unlimited quantity of events without modifying the block gas limit, and also to emit events during STATICCALLS.

Note:
Like allowUnlimitedContractSize, this (emitFreeLogs) option should only be used when debugging, any transaction calling the LOG opcode when this is enabled will behave differently than in the real EVM used on Ethereum mainnet and testnet.

Related:
sc-forks/solidity-coverage#234
OpenZeppelin/openzeppelin-contracts#876 (comment)

@holgerd77
Copy link
Member

If this goes on we might very well refactor this at some time, do a breaking change and then maybe put this under a generalized opts.debugOptions array option or something, for now this is ok though and I am open for it and think this is a useful addition, haven't digged deeper into the implementation though. Let me know once this is ready for review, also drop a note how urgent would be a release on this once merged.

@Agusx1211
Copy link
Contributor Author

If this goes on we might very well refactor this at some time, do a breaking change and then maybe put this under a generalized opts.debugOptions array option or something

Maybe it will be a good idea to be able to feed the lib with a custom opFns.js; this will allow any debugger or tool to create or modify the opcodes needed.

haven't digged deeper into the implementation though. Let me know once this is ready for review,

I believe the implementation is ready with these few modifications, about the code, is not pretty, my first idea was to do the check inside opFns.js, but that is a static file, and can't be configurable witout a major refactor.

So the implementation has two parts:

staticcalls: There is no way to tell to opFns.js that the log opcode should behave differently, so runCode.js changes runState.static to false when applying a log opcode, then it sets it back to the original state.

gas to 0: I added a flag freeLogs to lookupOpInfo, this should modify the gas cost of log to 0 when the flag is set to true, the swap could be moved to runCode.js if that's cleanner.

also drop a note how urgent would be a release on this once merged.

About that, I am not in charge of solidity-coverage, but a lot of projects are using it, OpenZeppelin being the biggest one.

They currently are instantiating a ganache instance with "unlimited" block gas limit; this workaround is only enough for solving the gas problem. For the staticcalls they are just advising to ignore the coverage of those contracts, that is a dangerous solution, and more if we are testing contracts like OpenZeppelin, which are used by a lot of projects.

As soon as this is merged, we should be able to see coverage of staticcalls on solidity-coverage; so is not urgent, but it will make a lot of contracts tests more robust.

@holgerd77
Copy link
Member

Also not sure about the best way to approach this, but could you come up with a test case on this? Probably in index.js of API tests?

@holgerd77
Copy link
Member

Thanks for the excessive explanation.

Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants