Flip the sign of deltas #481
Conversation
|
I realize this is potentially a can of worms w swap math + backwards compatibility, but does a negative amountSpecified make more sense to denote amountIn now (negative for the user)? |
Co-authored-by: hensha256 <henshawalice@gmail.com>
| @@ -200,8 +200,8 @@ library SqrtPriceMath { | |||
| { | |||
| unchecked { | |||
| return liquidity < 0 | |||
There was a problem hiding this comment.
Are we sure we don't want to switch liquidityDelta sign if we are changing amountSpecified sign? Now they don't match right?
IE liquidityDelta is positive, we return negative delta because user owes $ to pool and if liquidityDelta is negative (user is taking liquidity out of the pool) we return a positive delta because the user can now claim it back from the pool...
There was a problem hiding this comment.
Right the argument is that liquidityDelta is the user's delta of liquidity units not token units. When they pass in liquidityDelta = 100 its because they want their liquidity balance to increase by 100 (which means sending tokens into the contract). So liquidityDelta is in line with amountSpecified now... for both of them:
- positive: the balance of the thing in question goes up
- negative: the balance of the thing in question goes down
There was a problem hiding this comment.
great, makes sense! i think im in agreement w that framing then
| } | ||
|
|
||
| if (exactIn && sqrtRatioNextX96 != sqrtRatioTargetX96) { | ||
| // we didn't reach the target, so take the remainder of the maximum input as fee | ||
| feeAmount = uint256(amountRemaining) - amountIn; | ||
| feeAmount = uint256(-amountRemaining) - amountIn; |
There was a problem hiding this comment.
I think I'm not 100% understanding this line. If we have't reached the target the feeAmount is the amount left to swap minus the amount already swapped?
Also why does this case only happen in the exactIn case?
There was a problem hiding this comment.
If we have't reached the target the feeAmount is the amount left to swap minus the amount already swapped?
If we havent reached the target (which is the essentially maximum slippage where the trade has to stop), that means that all of the amountRemaining has successfully be traded into output tokens. The case this happens, the function follows the following flow...
- calculate
amountRemainingLessFeewhich is just the amount to be traded, minus the LP fee - calculates the
sqrtRatioNextX96that does get reached by this amount of input tokens - calculate the exact
amountInneeded to trade to that next sqrtRatio usinggetAmount0Delta. Due to rounding of calculations this could be slightly different to theamountRemainingLessFeethat it was going to trade before - now the true LP fee amount needs to be calculated, which is the difference between the total amount to be traded
amountRemaining(which is now negative so needs to be flipped), andamountInwhich is the amount thats actually being swapped. This means that the fee is rounded in the LP's favour.
Also why does this case only happen in the exactIn case?
LP fees are taken on input tokens. For exactIn, the calculation makes sure that the final amountIn including fees is amountRemaining. For non-exactIn the fee amount can just be calculated as feePips% of the amountIn, again rounded in the LP's favour. Theres not a need to make sure that the total feeAmount + amountIn == amountRemaining because its not exactIn so the "exact" part stops mattering
| @@ -249,7 +249,7 @@ contract SqrtPriceMathTestTest is Test, GasSnapshot { | |||
| assertEq(amount0, 0); | |||
| } | |||
|
|
|||
| function test_getAmount0Delta_returns0_1Amount1ForPriceOf1To1_21() public { | |||
| function test_getAmount0Delta_1Amount1ForPriceOf1To1_21() public { | |||
There was a problem hiding this comment.
hm I know not your change but what is 1Amount1? Not really sure what this is testing/what the change to the name means
There was a problem hiding this comment.
It tests getAmount0Delta, with an amount1 of 1, and a price of SQRT_RATIO_121_100.
I removed the return0 because it doesnt test that (i think it was copied from the test name above)
closes #467
The current set up is counter intuitive.
After this pull request: