Refactor and optimize SwapMath
#280
Conversation
|
Please can you merge main so we can re-review the gas improvements 🙏 |
# Conflicts: # .forge-snapshots/simple swap.snap # .forge-snapshots/swap against liquidity with native token.snap # .forge-snapshots/swap against liquidity.snap # .forge-snapshots/swap with hooks.snap # .forge-snapshots/swap with native.snap # src/libraries/SwapMath.sol # test/SwapMath.spec.ts # test/__snapshots__/PoolManager.gas.spec.ts.snap # test/__snapshots__/PoolManager.spec.ts.snap # test/__snapshots__/SwapMath.spec.ts.snap
…InputAmountTakenAsFee`
@hensha256 Done. As stated before, I had to modify |
Replace inline assembly with native Solidity for `amountRemainingAbs` calculation in `computeSwapStep` function. The previous implementation was not intuitive and easy to read. This change simplifies the code and improves readability while maintaining the exact functionality.
In `SwapMath.sol`, we reduced redundancy by storing the value of `feePips` into `_feePips` and using it throughout the function instead of casting `feePips` multiple times. This refactoring improved efficiency and reduced bytecode size, leading to reduced gas cost for function calls.
The declaration and assignment of `amountRemainingAbs` has been moved inside the relevant if-else blocks in the SwapMath.sol file. This change makes the code more concise and clear by limiting the scope and use of `amountRemainingAbs` to where it is needed.
… the target price for `exactIn` to achieve equivalence with V3
src/libraries/SwapMath.sol
Outdated
| sqrtPriceNextX96 = sqrtPriceTargetX96; | ||
| } else { | ||
| // cap the output amount to not exceed the remaining output amount | ||
| amountOut = uint256(amountRemaining); | ||
| sqrtPriceNextX96 = SqrtPriceMath.getNextSqrtPriceFromOutput( | ||
| sqrtPriceCurrentX96, liquidity, uint256(amountRemaining), zeroForOne |
There was a problem hiding this comment.
is it cheaper to pass in amountOut to prevent double-casting amountRemaining?
There was a problem hiding this comment.
Casting between uint256 and int256 shouldn't matter.
There was a problem hiding this comment.
was gonna ask same thing - even if it doesn't make a difference I think the code reads better with using the variable we just defined above imo
There was a problem hiding this comment.
Passing amountOut to getNextSqrtPriceFromOutput.
src/libraries/SwapMath.sol
Outdated
| sqrtPriceNextX96 = sqrtPriceTargetX96; | ||
| } else { | ||
| // cap the output amount to not exceed the remaining output amount | ||
| amountOut = uint256(amountRemaining); | ||
| sqrtPriceNextX96 = SqrtPriceMath.getNextSqrtPriceFromOutput( | ||
| sqrtPriceCurrentX96, liquidity, uint256(amountRemaining), zeroForOne |
There was a problem hiding this comment.
was gonna ask same thing - even if it doesn't make a difference I think the code reads better with using the variable we just defined above imo
Related Issue
Which issue does this pull request resolve?
Obscure control flow related to
SwapMath.Description of changes
Improvements to the
SwapMathlibrary have been implemented. A new function,getSqrtRatioTarget, written inYulusing bitwise operations, has been introduced to replace a nested ternary operation withinPool.swap:( params.zeroForOne ? step.sqrtPriceNextX96 < params.sqrtPriceLimitX96 : step.sqrtPriceNextX96 > params.sqrtPriceLimitX96 ) ? params.sqrtPriceLimitX96 : step.sqrtPriceNextX96which basically was doing
Furthermore, the
computeSwapStepfunction has been refactored, resulting in a cleaner control flow and significant gas savings. The gas savings achieved range from 10% to 28%, as can be seen inSwapMath.spec.ts.snap. In conjunction with the changes introduced in pull request #258, the maximum savings oncomputeSwapStepcan reach up to 40%.All tests pass except for
SwapMath::#computeSwapStep::entire input amount taken as fee, whereamountInchanges from0to9, andfeeAmountchanges from10to1.I wonder if "entire input amount taken as fee when output is zero" is an intended feature and the invariant
amountIn == 0 && feeAmount == 10should be kept. Adjusting the invariant toamountIn + feeAmount == 10appears to be acceptable and reasonable.