Skip to content

Conversation

@hensha256
Copy link
Collaborator

No description provided.

@hensha256 hensha256 requested a review from snreynolds August 14, 2024 18:03
})

describe('Interleaving routes', () => {
describe.only('Interleaving routes', () => {
Copy link
Member

Choose a reason for hiding this comment

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

remove .only

v3AmountIn,
v3AmountOutMin,
encodePathExactInput(v3Tokens),
SOURCE_MSG_SENDER, // the user pays for the input of the v3 trade
Copy link
Member

Choose a reason for hiding this comment

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

in v3 you input the payer?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

its just a boolean (like in v4) called payerIsUser. So SOURCE_MSG_SENDER == true and SOURCE_ROUTER == false

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

doesnt let you pull from any address

Base automatically changed from more-routing-tests to dev August 21, 2024 10:50
@hensha256 hensha256 requested a review from snreynolds August 21, 2024 12:07
Copy link

@rv64m rv64m left a comment

Choose a reason for hiding this comment

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

🔍 Code Review Summary

This pull request introduces a comprehensive and well-structured suite of integration tests for Uniswap V4 and mixed-protocol swaps (V2/V3/V4). The tests are robust, covering a wide range of scenarios including interleaved routes, split trades, and atomic batch executions. The use of a pinned mainnet fork and clear helper abstractions makes the tests both reliable and readable, representing a significant improvement in test coverage for critical new functionality.

📋 Reviewed Changes

  • test/integration-tests/UniswapMixed.test.ts: Reviewed new tests for swaps that combine V2, V3, and V4 protocol legs. Analyzed interleaved, split, and batched execution test cases.
  • test/integration-tests/UniswapV4.test.ts: Reviewed a full suite of tests for new Uniswap V4 functionality within the Universal Router, including single-hop, multi-hop, exact-in, and exact-out swaps, as well as V4-specific actions like TAKE_PORTION.
  • test/integration-tests/shared/mainnetForkHelpers.ts: Reviewed updates to testing helpers, including pinning the mainnet fork to a specific block number (20010000) for test reproducibility.

🤔 Review Quality Assessment

The review provides comprehensive coverage of the new test suites for V4 and mixed-protocol swaps. The analysis correctly identifies the high quality and robustness of the tests, noting the use of mainnet forking and complex, realistic scenarios. The two suggestions provided are valuable, actionable improvements for test completeness and code clarity. Confidence in the review is high; no significant gaps were identified, and the findings accurately reflect the PR's strengths and minor areas for enhancement.

💡 Suggestions Summary

  • Inline Comments: 0 suggestions on modified code lines
  • General Suggestions: 2 suggestions for overall improvements

📝 Additional Suggestions

The following suggestions apply to code outside the current PR diff:

1. 🔴 This test correctly checks the output WETH amount from a split trade but has a commented-out assertion for the input DAI amount. The executeRouter helper provides the necessary balance information for the test runner (bob), so this check can be completed to make the test case more robust by verifying both sides of the trade.

File: test/integration-tests/UniswapMixed.test.ts (Line 684)
Confidence: 95%

      const { daiBalanceBefore, daiBalanceAfter, wethBalanceBefore, wethBalanceAfter, v2SwapEventArgs, v3SwapEventArgs } = await executeRouter(
        planner,
        bob,
        router,
        wethContract,
        daiContract,
        usdcContract
      )
      const { amount1Out: wethOutV2 } = v2SwapEventArgs!
      let { amount1: wethOutV3 } = v3SwapEventArgs!

      expect(daiBalanceBefore.sub(daiBalanceAfter)).to.eq(v2AmountIn.add(v3AmountIn))
      expect(wethBalanceAfter.sub(wethBalanceBefore)).to.eq(wethOutV2.sub(wethOutV3))

2. 🔴 The test pattern of sending tokens directly to a V2 pair and then executing a V2_SWAP_EXACT_IN with amountIn: 0 is powerful but unintuitive. Adding a more descriptive comment would improve the long-term maintainability and readability of this test by explaining the specific router behavior being tested.

File: test/integration-tests/UniswapMixed.test.ts (Line 134)
Confidence: 90%

      // The V2_SWAP_EXACT_IN command supports an amountIn of 0. This is a specific feature for chained swaps
      // where the input tokens are already present in the pair contract from a previous step in the transaction.
      planner.addCommand(CommandType.V2_SWAP_EXACT_IN, [MSG_SENDER, 0, v2AmountOutMin, v2Tokens, SOURCE_MSG_SENDER])

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.

5 participants