Skip to content

Conversation

@BoogerWooger
Copy link
Contributor

@BoogerWooger BoogerWooger commented Dec 24, 2025

Asymmetric ranges and safe bounds for XYCConcentrateGrowLiquidity instruction tests

Change Summary

Added tests for asymmetric and extreme ranges for XYCConcentrate instruction, functions to select parameters safe from arithmetic underflows and symmetry invariants breaking (they cannot lead to pool draining, but can lead to precision losses in some calculation at corner cases))

Related Issue/Ticket:
PT1-285

@claude
Copy link

claude bot commented Dec 24, 2025

Claude finished @BoogerWooger's task —— View job


Code Review: Asymmetric Ranges and Safe Bounds for XYCConcentrateGrowLiquidity

I've reviewed the new test file test/invariants/ConcentrateAsymmetricRanges.t.sol and here is my comprehensive feedback:

✅ Code Quality and Best Practices

Strengths:

  • Well-structured test file with clear organization and comprehensive documentation
  • Excellent mathematical documentation explaining the safety constraints (|α - β| ≤ αβ)
  • Good use of helper functions to reduce code duplication
  • Clear test naming that describes what each test validates
  • Extensive logging for debugging and understanding test behavior

Areas for improvement:

  • Consider extracting magic numbers (e.g., 1e18, 1e9) into named constants
  • Some functions like _createGrowLiquidityOrder are quite long (80+ lines) - consider splitting into smaller functions

✅ Test Coverage

Excellent coverage of edge cases:

  • Multiple delta ratio scenarios (2.5x, 10x, 50x, 100x, 500x)
  • Symmetric and asymmetric pool configurations (1:1, 2:1, 1:10)
  • Large swap scenarios (50% of balance)
  • Mathematical analysis tests demonstrating the problem space
  • Comparison between safe and unsafe bounds

Test strategy is sound:

  • Tests progressively increase delta ratios to explore boundaries
  • Each test validates invariants with appropriate tolerance levels
  • Analysis functions provide insight into failure modes

⚠️ Potential Issues

  1. Precision loss in calculations (test/invariants/ConcentrateAsymmetricRanges.t.sol:75-77):

    uint256 numerator = maxDeltaRatio * maxDeltaRatio / 1e18;
    uint256 sumPlusOne = maxDeltaRatio + 1e18;
    uint256 denominator = sumPlusOne * sumPlusOne / 1e18;

    The division by 1e18 could lead to precision loss. Consider using a higher precision intermediate calculation or rearranging the formula.

  2. Potential division by zero (test/invariants/ConcentrateAsymmetricRanges.t.sol:102-103):

    deltaRatioA = alpha > 0 ? 1e18 * 1e18 / alpha : 0;
    deltaRatioB = beta > 0 ? 1e18 * 1e18 / beta : 0;

    While protected by ternary operators, the zero return could mask important edge cases. Consider whether these should revert or return a special value.

🔒 Security Considerations

Good security practices observed:

  • Safe bounds calculation prevents arithmetic underflows in the XYCConcentrate instruction
  • Comprehensive validation of price ranges before use
  • Tests verify that unsafe bounds are correctly identified

Recommendations:

  • Consider adding fuzz tests to automatically explore the parameter space
  • Add tests for extreme token decimal scenarios (6, 8, 18 decimals)
  • Consider testing with very small balance values to ensure no division issues

🚀 Performance Considerations

  • The calculateSafePriceBounds function performs multiple multiplications and divisions. For production use, consider gas optimization
  • The extensive logging in tests is appropriate for development but should not be included in production code
  • Consider caching square root calculations if they're performed multiple times with the same inputs

📝 Additional Recommendations

  1. Documentation: Consider adding a README or documentation explaining the mathematical basis for the safety constraints and how to use these functions

  2. Integration tests: While unit tests are comprehensive, consider adding integration tests that demonstrate these safe bounds in a full swap scenario

  3. Invariant testing with Foundry: Consider using Foundry's built-in invariant testing capabilities to automatically explore the state space

Summary

This is a high-quality addition to the test suite that demonstrates a thorough understanding of the concentrated liquidity mechanism and its edge cases. The safe bounds calculation provides valuable protection against arithmetic issues. The test coverage is comprehensive and the code is well-documented. With minor improvements to precision handling and some additional edge case coverage, this will be a robust safeguard for the XYCConcentrate instruction.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants