-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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
Fuzz tampered tests for ERC2771Forwarder
#5258
Conversation
|
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.
Leaving small nits since I'm still not convinced 100% of the Foundry tests. They're still good to go in case you decide to merge before 5.2
test/metatx/ERC2771Forwarder.t.sol
Outdated
}); | ||
} | ||
|
||
function _tamperedExecute( |
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.
this does not execute, it prepares a request, tampers it, and prepare the expected revert ... but it does not execute.
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.
yeah i kinda meant for this function to generate a tampered execute, feel free to rename
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.
I'm getting
FAIL. Reason: unknown cheatcode with selector 0xd5bee9f5; you may have a mismatch between the `Vm` interface (likely in `forge-std`) and the `forge` version;
Is there a missing foundry update ?
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.
Why are testExecuteBatchTamperedValuesZeroReceiver
and testExecuteBatchTamperedValues
different?
One does _tamperedValues
then expect a call, the other replace batchRequestDatas
entirely and relies on the expectRevert
in tamperedExecute
.
I'd like to understand and clean all that, but I'm not able to run it locally :/
@Amxx those two functions are a bit different as they test for two different cases:
To get the latest version and be able to run the tests, do |
@cairoeth I rewrote the test quite heavily. IMO that is more readable. Let me know what you think. |
looks great, thanks! |
Fixes #4896 and helps #4894
PR Checklist
npx changeset add
)