add swap to price tests #904
Conversation
saucepoint
left a comment
saucepoint
left a comment
There was a problem hiding this comment.
fuzz test where the starting pool price is fuzzed would be cool -- but this looks good enough to me haha
| assertEq(manager.protocolFeesAccrued(currency1), expectedProtocolFee); | ||
| } | ||
|
|
||
| function test_swap_toLiquidity_fromMinPrice() public { |
There was a problem hiding this comment.
maybe worth naming this one withLiquidity and the other one withoutLiquidity?
There was a problem hiding this comment.
I think toLiquidity is accurate because this test traverses the swap until reaching nonzero liquidity. The relevant limit is the output amount, whereas in the other test the relevant limit is the price.
There was a problem hiding this comment.
(can still include withoutLiquidity in the other test name)
test/PoolManager.t.sol
Outdated
| modifyLiquidityRouter.modifyLiquidity(_key, params, ZERO_BYTES); | ||
|
|
||
| // zeroForOne=false to swap higher | ||
| IPoolManager.SwapParams memory swapParams = IPoolManager.SwapParams(false, 1, TickMath.MAX_SQRT_PRICE - 1); |
There was a problem hiding this comment.
If you instead used -1 for amountSpecified, I think this test could be more precise about the final price:
assertEq(sqrtPriceX96, TickMath.getSqrtPriceAtTick(-10));
test/PoolManager.t.sol
Outdated
| (sqrtPriceX96,,,) = manager.getSlot0(_key.toId()); | ||
|
|
||
| // The swap pushes the price to the sqrtPriceLimit. | ||
| assertEq(TickMath.getTickAtSqrtPrice(SQRT_PRICE_1_4), TickMath.getTickAtSqrtPrice(sqrtPriceX96)); |
There was a problem hiding this comment.
It's weird that this is testing by tick rather than by price. Shouldn't this test instead verify that the limit price is equal to the final price?
assertEq(SQRT_PRICE_1_4, sqrtPriceX96);
| } else { | ||
| // the price is between the position, so let's just swap down | ||
| zeroForOne = true; | ||
| sqrtPriceX96Limit = TickMath.MIN_SQRT_PRICE + 1; |
There was a problem hiding this comment.
I think this is the case that's failing the fuzz test
|
|
||
| modifyLiquidityRouter.modifyLiquidity(_key, params, ZERO_BYTES); | ||
|
|
||
| IPoolManager.SwapParams memory swapParams = IPoolManager.SwapParams(zeroForOne, 1, sqrtPriceX96Limit); |
There was a problem hiding this comment.
The failure happens because the price does move slightly. It's not ideal for the test to fail probabilistically. It fails because the getTickAtSqrtPrice flips from 0 to -1.
I think that case would pass if this amountSpecified was -1, because that amount would become fee and not move the price.
There was a problem hiding this comment.
Yeah I meant to change to -1 thanks!
There was a problem hiding this comment.
To prevent such probabilistic failures, consider adjusting the amountSpecified parameter to -1, right?
This adjustment ensures that the amount becomes a fee, preventing unintended price movements and enhancing the test's reliability.
|
I guess, it's better to extract the common setup code into a separate helper function. |
| IPoolManager.ModifyLiquidityParams memory params = IPoolManager.ModifyLiquidityParams({ | ||
| tickLower: -10, | ||
| tickUpper: 10, | ||
| liquidityDelta: 100000000000e18, |
There was a problem hiding this comment.
better to use underscores(_) for readability.
suggestion: 100_000_000_000e18
rv64m
left a comment
rv64m
left a comment
There was a problem hiding this comment.
🔍 Code Review Summary
The code review confirms that the pull request successfully adds three new tests to test/PoolManager.t.sol. These tests correctly validate swap behavior under specific edge conditions, such as swapping to a liquidity boundary or in a pool without liquidity. The changes enhance the robustness of the test suite, and no issues or risks were identified.
📋 Reviewed Changes
- test/PoolManager.t.sol: Added three new test functions (
test_swap_toLiquidity_fromMinPrice,test_fuzz_swap_toLiquidity,test_swap_toPrice_fromMaxPrice_withoutLiquidity) to verify swap behavior when hitting liquidity boundaries and in pools with no liquidity.
🤔 Review Quality Assessment
The review provides comprehensive coverage of the changes, which were limited to the addition of new test cases. The analysis correctly identified the purpose and verified the implementation of these tests. Confidence in the review quality is high, as the scope was well-defined and the findings are accurate. No gaps were identified.
Related Issue
Which issue does this pull request resolve?
Description of changes