chore: do not rely on src libraries in test libraries #210
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
We define a broad set of library functions to make testing the contracts in
src
easier. However, these libraries themselves rely on the tested contracts. This could lead to issues when the tests pass because both the libraries and the files insrc
are broken in the same way. To guarantee that tests are independent of the tested libraries, all use ofsrc
libraries has been removed.What I really wanted
Unfortunately, it's still possible that we forget about this and use some function in, e.g.,
GPv2Order
in the test library, especially since we need to import this library to get related types and constants.I tried to create a Solidity file that reexports everything we need from the libraries without the functions themselves (this would, for example, allow us to define a custom linting rule asserting that there's no import from
src
in any test file) but I was unsuccessful. It was very easy to reexport contracts (e.g.,GPv2Settlement
) and interfaces (e.g.,IERC20
), and it was also easy to reexport constants (e.g.,GPv2Order.KIND_SELL
), The issue was with reexporting type structs: to my understanding, this is impossible in Solidity ≤0.8.26, confirmed by the fact that there is no statement that appears to enable this use case in the language grammar.Since neither reexporting everything but the structs nor reexporting the entire libraries make sense, I decided to keep everything as it is without further reexporting.
Test Plan
Nothing broken in CI.