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
| // 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.
| // 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
4 participants
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.