More limit order E2E tests #780
Conversation
| /// quote. | ||
| /// quote. The fee is denoted in the sell token. |
There was a problem hiding this comment.
Making sure I'm not missing anything: is this comment correct in all cases?
crates/e2e/tests/e2e/limit_orders.rs
Outdated
| // Fund settlement contract | ||
| tx!( | ||
| solver_account, | ||
| token_a.mint(contracts.gp_settlement.address(), to_wei(1010)) | ||
| ); |
There was a problem hiding this comment.
If we only want to avoid the rounding issue mentioned in the PR description we would only ever have to fund the settlement contract with a single wei per order with 0 fees. Using 1010 ETH seems a bit excessive. 😅
Note that we only still run into this issue because the single wei of surplus_fee hardcoded at the moment is so small that it still causes numerical issues (adjusting the buy_token price for such a small fee doesn't change the buy_token price at all leaving again no fee to cover the NaiveSolver rounding issue). For this specific test it be enough to not fund the settlement contract and instead hard code a fee of 2 wei (big enough to change the adjusted price).
However, to be on safe side and never have to worry about this thing it's probably best to be a bit more generous with the hardcoded surplus_fee (maybe 1_000?). This value is already unrealistically small but at least it should be big enough to never cause numerical issues, making it unnecessary to ever fund the settlement contract for an e2e test.
There was a problem hiding this comment.
I don't know the details of this calculation, so I only updated the tests to match the values that I got when they failed. If you can verify this in some meaningful way, please do.
| quoter: Arc::new(FixedFeeQuoter { | ||
| quoter, | ||
| fee: 1_000.into(), | ||
| }), |
There was a problem hiding this comment.
I would actually like to see this quote fees using the baseline quoter.
Its still not 100% clear to me exactly where the rounding issue is coming from.
One concern that I have is that it actually comes from the clearing price calculation related to the limit prices and not the Naive solver. And, if we use this fixed fee, it might be harder to catch regressions over there.
There was a problem hiding this comment.
Its still not 100% clear to me exactly where the rounding issue is coming from.
What part of the explanation do you not find convincing?
Note that when the test still set a surplus_fee of 0 the settlement contract was short of exactly 1 wei of the token swapped on uniswap. That's the exact behavior explained in the linked PR.
I went a bit more into the details for the 0 surplus_fee case here in case you missed that.
I guess technically it's still possible that there is an error in the price adjustment for limit orders but I'm fairly certain the issues we experienced before were caused by the NaiveSolver.
And, if we use this fixed fee, it might be harder to catch regressions over there.
Not sure how a variable fee would help with that case.
Fixes #769.
Adds a test for settling two limit orders and a mix of 1 limit order and 1 market order.
@MartinquaXD figured out that our tests were failing due to this, in particular:
All credit for the detective work goes to him. I fixed the issue in our test setup by funding the settlement contract and also making it so that surplus fees for limit orders are always 1 when testing.