-
Notifications
You must be signed in to change notification settings - Fork 39
chore: migrate GPv2Order orderUid tests to Foundry #197
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
Conversation
Co-authored-by: mfw78 <53399572+mfw78@users.noreply.github.com>
bytes32 orderDigest = keccak256("order digest"); | ||
address owner = address(uint160(uint256(keccak256("owner")))); | ||
uint32 validTo = uint32(uint256(keccak256("valid to"))); | ||
bytes memory orderUid = executor.packOrderUidParamsTest(GPv2Order.UID_LENGTH, orderDigest, owner, validTo); |
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 could alternatively use Order.sol
within test/libraries
as can all related incidences of packOrderUidParamsTest
.
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.
It's true that Order.computeOrderUid
is almost duplicate code of packOrderUidParamsTest
. I'd still vote in favor of using packOrderUidParamsTest
here because otherwise we'd need to assume that the Order
library won't change in the future (and in case we wouldn't be testing GPv2Order
anymore but just the custom implementation of Order
.
And I really considered replacing the implementation of Order.computeOrderUid
with just abi.encodePacked(...input parameters)
so that I could use that to test that packOrderUidParamsTest
worked as expected (but I decided not to to avoid the exact problem I mentioned above).
Description
See title. Left an empty test file as a reminder that
GPv2OrderTestInterface
still needs to be removed fromtest/src
.Test Plan
CI.
Related Issues
#117