CCQ: EVM Library and Demo Contract #3425
Conversation
bruce-riley
requested review from
a5-pickle,
gator-boi and
kcsongor
as code owners
There was a problem hiding this comment.
Hopefully you don't feel like I'm nitpicking you to death, but here we go sir...
Please use forge fmt to make the spacing and style consistent.
It would be really nice to ship demo contracts good test coverage as these are the contracts people are going to copy and paste if I understand correctly.
Also, in QueryDemo.sol, the assertions that cause reverts are mostly all require(condition, "string explaining the problem") whereas in QueryResponse.sol, they're mostly custom errors. When dealing with the custom errors, solidity uses the first 4 bytes of the selector hash for the custom errors. When using the require syntax with a string, the entire string stays in memory and has to be returned. So using custom errors tends to be much more efficient gas wise and often also results in cleaner code. Whichever style you choose to use, try to be somewhat consistent where it makes sense.
Lastly, the parsing bits could really use some fuzz tests to ensure things work as expected. If you haven't done that or would like to pair on it I'm more than happy to lend a hand. It's basically the exact same as foundry unit tests, but the unit test functions have arguments (of the things you want to fuzz) and you can use some of the cheatcodes like vm.assume() to guide the fuzzer into being useful.
bruce-riley
force-pushed
the
CCQ/EVM_Library_and_Demo_contract
branch
from
20ce73e
to
e3ea06d
Compare
- Context is generally a worthless import - Ownable is totally unused and a worthless import
To follow standard forge testing practice, you use the same filename for a test contract as for the file it is testing, but make the extension .t.sol.
By moving the structs out of the abstract contract, you can use them directly in any contract that imports QueryResponse.sol *without* requiring that contract to inherit from the QueryResponse contract. This seems to work much better with forge's coverage tooling.
Instead of the test contract inheriting directly from the QueryResponse contract, it instantiates an instance of it and uses that for the testing. This seems to work much nicer with the forge coverage tools. With this commit, the coverage for QueryResponse shows 85.1% line coverage, 100% function coverage, and 50% branch coverage.
If you don't do this, in the tests you have to do:
vm.expectRevert(bytes4(keccak256("UnsupportedQueryType()")));
Whereas with this change, you can simply import the contract and do:
vm.expectRevert(UnsupportedQueryType.selector);
* Gas bad * Natspec updates * Some address(0) checks in the constructor
* Use custom errors in place of require() to match the error handling style overall of QueryResponse.
bruce-riley
force-pushed
the
CCQ/EVM_Library_and_Demo_contract
branch
from
47a182c
to
634c549
Compare
This PR adds the EVM on chain library used to parse query responses.
This PR is not dependent on any of the other CCQ PRs and can be merged as soon as it is approved.