Skip to content
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

chore: migrate GPv2Order eip712 tests to Foundry #195

Merged
merged 6 commits into from
Aug 8, 2024
Merged

Conversation

fedgiac
Copy link
Contributor

@fedgiac fedgiac commented Aug 6, 2024

Description

See title.

This part was particularly painful to port to Solidity; most notably, there are a lot of missing quality-of-life features when handling arrays.

Since we don't have ethers anymore, I had to build my own EIP712 encoding library (as recommended by Foundry).
As I could have introduced new bugs in the library, the related tests have become more aggressive (for example, all flag combinations are tested for consistency with the contract). There are also a few sanity checks which have already turned out to be useful during development.

I also sneakily introduced fuzz tests, since they seemed appropriate here.

Sorry for the horrible nested loops, suggestions on how to get rid of them are welcome.

The good news: there's a chance that a few things can be simplified with a future version of the compiler, see comments in code.

Test Plan

CI.

Related Issues

#117

@fedgiac fedgiac requested a review from a team as a code owner August 6, 2024 15:23
@mfw78
Copy link
Contributor

mfw78 commented Aug 7, 2024

With the removal of ethers, I'm not so sure about the efficacy of this test (or doing EIP-712 computation as is). This particular test that's being migrated seems more about ensuring that ethers calculates what is expected by the contract, as opposed to verifying that the contract is implemented correctly for EIP-712.

I think in general, the test mostly re-implements the logic within the contract itself and I'm unsure of the efficacy of the test in general.

@fedgiac
Copy link
Contributor Author

fedgiac commented Aug 7, 2024

This is how we compute the EIP712 hash in the contract. This is how we compute it in the test.
I'd argue that the two implementations are completely different.
Also, having a test for this part is essential as we're doing a very custom implementation of EIP712 with a lot of potential pitfalls. There are also quite a few subtleties happening here, for example the definition of the Order struct is the only place where we actually see which struct is EIP712 signed; we also make the strings for balance names and kind names explicit, which are successfully round-tripped to the expected hash.

I've little doubt that the test overall has very high value, but I agree that it would be better to have the EIP712 layer handled somewhere else rather than implementing it directly. I did quite a bit of research on making this work with Foundry and to me the most credible way out is native Solidity support of EIP712, and I mentioned in the inline comments how a migration could look like.
An alternative I see is writing the tests in another language. This is either keeping ethers for this or bringing up a testing framework for some other language. In the end I thought it's better to do a little extra manual work in Solidity rather than having more moving parts, but there is an argument that I'm underestimating the complexity because of overfamiliarity with EIP712.

test/libraries/Eip712.sol Outdated Show resolved Hide resolved
}

function ALL_FLAGS() internal pure returns (Flags[] memory out) {
uint256 numBools = 2;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little confused as to "numBools" of 2, yet Flags only contains a single boolean parameter?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On further inspection, it seems this is referring to the number of permutations associated with boolean values (and given a single boolean, would correspond to 2 permutations).

Given this, I'd sooner see numBools = 1 and then in the array length declared below, to multiply numBools by 2, with a comment on the out variable reflecting that it contains all permutations of all flags.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Is it better now?

test/GPv2Order/Order.sol Outdated Show resolved Hide resolved
@fedgiac fedgiac merged commit 658b897 into main Aug 8, 2024
9 checks passed
@fedgiac fedgiac deleted the migrate-test-order branch August 8, 2024 07:03
@github-actions github-actions bot locked and limited conversation to collaborators Aug 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants