Add assert cheatcodes#729
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
| assert.NotNil(t, assertionTest.callSequence, "expected call sequence to exist") | ||
|
|
||
| // Get the last call in the sequence (the one that triggered the assertion failure) | ||
| lastCall := (*assertionTest.callSequence)[len(*assertionTest.callSequence)-1] |
There was a problem hiding this comment.
I thought callSequence would be the sequence we tried to execute, so the first one in the sequence could be the one that triggered the assertion failure, rather than the last one. If so, that could lead to these tests failing nondeterministically
Not sure how to test whether I'm right or not...
| contract TestContract { | ||
| CheatCodes cheats = CheatCodes(0x7109709ECfa91a80626fF3989D68f67F5b1DD12D); | ||
|
|
||
| function test_assertTrue_passes() public { |
There was a problem hiding this comment.
Since these are all meant to not get an assertion failure, maybe we should group these into one big function so that we can be sure everything gets hit? Right now we might miss one because it doesn't get randomly generated in any of the sequences
dguido
left a comment
There was a problem hiding this comment.
Detailed review - the core implementation is solid and follows existing patterns well, but there are a few critical issues to address.
Must Fix
1. Error Swallowing in cheatCodePanicData
returnData, _ := panicMethod.Inputs.Pack(panicCodeBig) // Error swallowedIf packing fails, returnData will be nil and the fuzzer returns corrupted panic data. Please handle the error:
returnData, err := panicMethod.Inputs.Pack(panicCodeBig)
if err != nil {
logging.GlobalLogger.Panic("Failed to pack panic code for assertion cheatcode", err)
}2. Test Reliability Issue
As @samalws-tob noted, the test relies on the last call being the failing assertion, but fuzzer-generated sequences are randomized. Consider checking any call in the sequence for panic code 0x01, not just the last one.
Recommended Improvements
3. Performance: Cache Panic Method
Creating a new abi.Method on every assertion failure is inefficient. Consider creating it once at initialization:
var panicMethod = abi.NewMethod("Panic", "Panic", abi.Function, "", false, false,
abi.Arguments{{Type: typeUint256}}, abi.Arguments{})4. Add Documentation
The cheatCodePanicData function lacks a doc comment explaining its purpose.
5. Defensive Nil Checks
Consider adding nil checks for comparison inputs:
if left == nil || right == nil {
return nil, cheatCodeRevertData([]byte("assertLt: nil value provided"))
}Overall the implementation demonstrates good understanding of the codebase architecture. Once these issues are fixed, this will be ready to merge.
- Cache panicMethod outside closure to avoid recreating on every failure - Add error handling for panic code packing (was being swallowed) - Add documentation with Solidity panic code reference link - Fix test to check all calls in sequence for panic code 0x01 (failing assertion may not be last call due to randomized fuzzing) - Consolidate test functions from 21 to 4 comprehensive functions Co-Authored-By: Claude Opus 4.5 <[email protected]>
Co-Authored-By: Claude Opus 4.5 <[email protected]>
Add
assert*cheatcodes:assertTrue,assertFalse,assertEq(6 type variants),assertNotEq(6 type variants), andassertLt/Le/Gt/Ge(8 comparison operators). Tested each, most assertions returnPanic(0x01)on failure (assertFalseandassertTruedon't, I wasn't able to figure out why)Testing:
TestAssertCheatCodesPass: Verifies passing assertions work correctlyTestAssertCheatCodesFail: Verifies failing assertions are detected and return panic code 0x01TODO:
assertTrueandassertFalse