Forked test showcasing the use of the helper interface #92
Conversation
Closes #91. We update the helper to use the math proposed in #91 to compute the buy amount. As CoW AMMs are deprecated anyway, only the helper interface matters for computing order prices. Note that the changes are untested until #92 is merged. If there is interest for it, the helper will be redeployed.
| assertGt(ammOrder.sellAmount, ammUsdcInitialBalance * 2 / 100); | ||
| assertLt(ammOrder.sellAmount, ammUsdcInitialBalance * 3 / 100); | ||
| assertGt(ammOrder.buyAmount, ammWethInitialBalance * 2 / 100); | ||
| assertLt(ammOrder.buyAmount, ammWethInitialBalance * 3 / 100); |
There was a problem hiding this comment.
uber nit: could use assertApproxEqRel
There was a problem hiding this comment.
could this test do an exact expectation?
i find 2 paths for this:
- used forked balances, explain them, explain the math and do an
assertEqwith a value extracted from the test itself (e.g. 3045.12321321) - replace forked balances with mocked ones (e.g. pool initial balance is
30.000:10), calculate the expected outcome, do anassertApproxEqRelwith a very tight tolerance
There was a problem hiding this comment.
uber nit: could use assertApproxEqRel
b836965 ✔️
could this test do an exact expectation?
There has been quite a lot of internal discussion on this so the logic used to be somewhat open when the test was written. But I believe the changes resulting from #94 will make everything easier: it just means that the order should use the lowest possible buy amount for the AMM. Will follow up if I get a satisfactory confirmations (maybe @acanidio-econ can chime in?).
There was a problem hiding this comment.
Given a sell amount, the (limit) buy amount should be set to the smallest possible buy amount. Unfortunately, I am not familiar with the code much. If I understand correctly, a constant product pool should (up to rounding) return buy_amount = ammWethInitialBalance * sell_amount / (ammUsdcInitialBalance - sell_amount).
For general pools, it should return whatever the pool would give as out amount to a user when the in amount from the user is fixed, in a setting with zero fees.
| // Because of how we changed the price, we expect to buy USDC | ||
| assertEq(address(ammOrder.sellToken), address(USDC)); | ||
| assertEq(address(ammOrder.buyToken), address(WETH)); |
There was a problem hiding this comment.
this comment is misleading, if the pool is expected to BUY USDC, shouldn't buyToken be USDC?
There was a problem hiding this comment.
You're right, the comment here is incorrect. I changed it so that now it should be clearer why one is the sell token and why the other is the buy token.
|
isn't |
|
The test is working as expected. The discussion on more precise checks on the amounts is still open, but I don't see it as a blocker for merging. Feel free to continue the discussion here. |
Addresses issue in #92 (comment). The helper now returns a full signature for a CoW Swap order. ### Test plan For the legacy CoW AMM: note that the forked test still works after these changes.
This PR adds a minimal test showing how the helper contract can be used to generate valid AMM orders.
The helper contract hardcodes the contracts used in the various chains, so I have no choice but using a forked test.
I used the legacy path because there's no real CoW AMM with meaningful funds that isn't legacy.
Note that this test won't work once we migrated funds from the AMM used in the test. This doesn't matter as this test will have lost its utility at that point.
How to test
CI (but note that forked tests with the chosen endpoint are very unreliable, about 1 out of 10 runs have some network error).
Look at the code to determine that we test everything we want from the interface.