-
Notifications
You must be signed in to change notification settings - Fork 782
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
Conversation
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 |
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.
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.
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. |
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? |
Thanks for the excessive explanation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
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)