chore: migrate GPv2Signing recoverOrderSigner tests to Foundry #207
Conversation
…t-signing-set-pre-signature
Co-authored-by: mfw78 <53399572+mfw78@users.noreply.github.com>
…st-signing-recover-order-from-trade
…rate-test-signing-calldata-manipulation
…e-test-signing-recoverOrderSigner
| function test_reverts_for_invalid_signing_schemes() public { | ||
| vm.expectRevert(); | ||
| executor.recoverOrderSignerTest( | ||
| defaultOrder(), Sign.Signature({scheme: GPv2Signing.Scheme(uint8(42)), data: hex""}) |
There was a problem hiding this comment.
This test isn't achieving it's actual desired outcome as attempting to convert uint8(42) into GPv2Signing.Scheme causes a revert with a panic at this specific point in the test before there's an actual revert on recoverOrderSignerTest, so it seems that this would require a low-level call to by-pass the solidity type checking.
There was a problem hiding this comment.
Huh, you're absolutely right, great find!
My expectation was that expectRevert catches a revert in the next call, but it just catches any revert that follows! So this is a successful test:
vm.expectRevert("test revert");
revert("test revert");Also, there are valid tests that cannot be compiled, for example:
vm.expectRevert("test revert");
revert("test revert");
return;Which fails compilation with:
Warning (5740): Unreachable code.
--> test/GPv2Signing/RecoverOrderSigner.t.sol:54:9:
|
54 | return;
There was a problem hiding this comment.
I thought a bit about it and it's hard to meaningfully simulate this. We are testing a library, so we can't call something and see a revert because all of this is handled at a compiler level. So I ended up simplifying the test to just revert if a function tries to internally generate an invalid signing scheme. WDYT?
There was a problem hiding this comment.
Do we think that this value adds? I'd argue that because it can be tested in solidity, and the uint8 conversion of the enum is bound by types within the compiler, it's safe for us to remove this test entirely.
There was a problem hiding this comment.
Fair enough. Test has been removed and PR description has been updated.
…rate-test-signing-calldata-manipulation
…e-test-signing-recoverOrderSigner
Description
See title.
test_reverts_for_invalid_signing_schemeshas not been migrated because the compiler itself enforces that the signing scheme is valid and there's no way that I know of to force the compiler to give the library a value that doesn't fit the expected input type.Test Plan
CI.
Related Issues
#120