Skip to content

Conversation

@Sunnesoft
Copy link
Collaborator

No description provided.

@claude
Copy link

claude bot commented Jan 19, 2026

Claude finished @Sunnesoft's task —— View job


Pull Request Review

I've analyzed the changes to add pegged swap and extraction instructions to the Aqua router. Here's my comprehensive review:

Summary

This PR adds two new AMM instructions (PeggedSwap and Extruction) to the AquaOpcodes contract. While the implementation appears technically sound, there are several issues that should be addressed.

Code Quality and Best Practices

  1. Typo in PR Title and Code:

    • "extruction" should be "extraction" throughout the codebase
    • This affects: PR title, contract name, function names, and documentation
  2. Missing PR Description:

    • No description provided explaining the purpose and use cases for these new instructions
    • Consider adding context about when to use PeggedSwap vs XYCSwap
  3. Code Organization ✅:

    • Instructions properly added at the end of the opcodes array to maintain backward compatibility
    • Follows existing pattern for imports and contract inheritance

Potential Bugs or Issues

  1. PeggedSwap Implementation ⚠️:

    • The _peggedSwapGrowPriceRange2D function requires both input and output balances to be non-zero (line 105)
    • This could cause issues for new liquidity pools or edge cases
    • Consider if zero-balance initialization should be supported
  2. Extruction Interface Design ⚠️:

    • Having separate interfaces (IExtruction and IStaticExtruction) with identical signatures except for view modifier is confusing
    • Consider using a single interface with proper documentation about when static calls are made

Performance Considerations

  1. Gas Optimization ✅:

    • Gas snapshot shows reasonable gas costs for pegged swap operations (~3-7M gas for various test scenarios)
    • The analytical solution in PeggedSwap avoids iterative solving, which is good for gas efficiency
  2. External Calls in Extruction ⚠️:

    • The _extruction function makes external calls which could be expensive
    • Consider adding gas limits or documentation about expected gas usage

Security Concerns

  1. External Contract Calls 🔴:

    • The Extruction instruction allows arbitrary external contract calls
    • This is a significant security risk if not properly restricted
    • Recommendation: Add an allowlist of approved extraction targets or implement strict validation
  2. Reentrancy Risk 🔴:

    • External calls in _extruction could enable reentrancy attacks
    • No reentrancy guards are visible in the current implementation
    • Consider adding reentrancy protection
  3. Input Validation ✅:

    • Good validation in PeggedSwapArgsBuilder.parse() for parameters
    • Proper bounds checking for linearWidth and rates

Test Coverage

  1. PeggedSwap Tests ✅:

    • Found test files: PeggedSwap.t.sol, PeggedSwapInvariants.t.sol, PeggedFeesInvariants.t.sol
    • Gas snapshot includes comprehensive pegged swap test scenarios
  2. Extruction Tests 🔴:

    • No test files found for the Extruction functionality
    • This is a critical gap given the security implications of external calls
    • Must add tests before merging

Recommendations

  1. Critical:

    • Fix the "extruction" → "extraction" typo throughout
    • Add comprehensive tests for the Extraction functionality
    • Implement security controls for external contract calls (allowlist or validation)
    • Add reentrancy guards
  2. Important:

    • Add PR description explaining the purpose of these new instructions
    • Document gas costs and usage patterns for Extraction
    • Consider consolidating the extraction interfaces
  3. Nice to have:

    • Add inline documentation explaining when to use PeggedSwap vs other swap types
    • Include example usage in the PR description

Conclusion

While the PeggedSwap implementation appears well-designed with good mathematical documentation and test coverage, the Extraction functionality introduces significant security risks that must be addressed. The lack of tests for Extraction and the unrestricted external call capability are blocking issues that should be resolved before merging.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds two new instructions, PeggedSwap and Extruction, to the Aqua router's opcode system. The title contains a spelling error ("extruction" instead of "extraction").

Changes:

  • Added imports for Extruction and PeggedSwap instruction modules
  • Integrated both new instructions into the AquaOpcodes contract inheritance chain
  • Expanded the opcodes array from 35 to 37 entries to accommodate the new instructions

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
src/opcodes/AquaOpcodes.sol Adds two new instructions (PeggedSwap and Extruction) to the router's opcode system by importing them, inheriting from them, and registering their handlers in the opcodes array
.gas-snapshot Gas consumption metrics updated to reflect the impact of adding two new opcodes to the contract

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@Sunnesoft Sunnesoft marked this pull request as ready for review January 19, 2026 13:04
@ifelsedeveloper ifelsedeveloper self-assigned this Jan 19, 2026
@ifelsedeveloper
Copy link
Contributor

Valid case, these instruction in the audit scope and therefore we need to include them into opcodes

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.

3 participants