Skip to content

Conversation

@ericneil-sanc
Copy link
Contributor

@ericneil-sanc ericneil-sanc commented Jan 6, 2026

PR Scope

Please title your PR according to the following types and scopes following conventional commits:

  • fix(SDK name): will trigger a patch version
  • chore(<type>): will not trigger any release and should be used for internal repo changes
  • <type>(public): will trigger a patch version for non-code changes (e.g. README changes)
  • feat(SDK name): will trigger a minor version
  • feat(breaking): will trigger a major version for a breaking change

Description

Uses pathInput for v4-sdk's midPrice() function, in the initial value of the reduce() call

How Has This Been Tested?

[e.g. Manually, E2E tests, unit tests, Storybook]

Are there any breaking changes?

[e.g. Type definitions, API definitions]

If there are breaking changes, please ensure you bump the major version Bump the major version (by using the title feat(breaking): ...), post a notice in #eng-sdks, and explicitly notify all Uniswap Labs consumers of the SDK.

(Optional) Feedback Focus

[Specific parts of this PR you'd like feedback on, or that reviewers should pay closer attention to]

(Optional) Follow Ups

[Things that weren't addressed in this PR, ways you plan to build on this work, or other ways this work could be extended]


✨ Claude-Generated Content

Description

Fixes a bug in the Route.midPrice getter where this.input was incorrectly used instead of this.pathInput when determining the initial price direction in the reduce function.
The pathInput property contains the normalized currency that matches the pool's currencies (e.g., WETH), while input may be a native currency (e.g., ETH) that doesn't match the pool's currency0 or currency1. This caused incorrect price calculations when swapping native currencies through WETH pools.

Changes

  • Updated midPrice getter in sdks/v4-sdk/src/entities/route.ts to use this.pathInput instead of this.input when comparing against this.pools[0].currency0 (line 80)
  • Added unit tests for ETH input with WETH pool scenarios:
    • Single pool: ETH -> currency3 via WETH/currency3 pool
    • Multi-pool: ETH -> currency0 via WETH/currency3 and currency3/currency0 pools

How Has This Been Tested?

Unit tests added to verify midPrice calculations when using native currency (ETH) as input with WETH pools.

Are there any breaking changes?

No - this is a bug fix that corrects the price calculation behavior.

@ericneil-sanc ericneil-sanc requested a review from a team as a code owner January 6, 2026 00:05
@github-actions
Copy link

github-actions bot commented Jan 6, 2026

🤖 Claude Code Review

Review complete

Summary

This PR fixes a bug in the midPrice calculation for routes where the input currency is native (e.g., ETH) but the first pool uses the wrapped version (e.g., WETH).

The bug: In the midPrice getter, the initial accumulator compared this.pools[0].currency0 against this.input. When the input is ETH but the pool contains WETH, this comparison would fail to find a match, leading to incorrect price calculation direction.

The fix: Change the comparison from this.input to this.pathInput, which correctly represents the currency as it appears in the pool (WETH when ETH is provided and the pool uses WETH).

This is consistent with how the rest of the Route class works - pathInput is specifically designed to hold the "equivalent or wrapped/unwrapped input to match pool" as documented on line 17.

Verification

  • All 24 route tests pass
  • New tests added specifically cover the ETH → WETH pool scenario with both single and multi-pool routes
  • The fix is minimal and targeted

💡 Want a fresh review? Add a comment containing @request-claude-review to trigger a new review at any time.

github-actions[bot]
github-actions bot previously approved these changes Jan 6, 2026
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

📋 Review verdict: APPROVE

👆 The main review comment above is the source of truth for this PR review. It is automatically updated on each review cycle, so always refer to it for the most current feedback.

This formal review submission is for the verdict only.

@github-actions github-actions bot dismissed their stale review January 6, 2026 01:22

Superseded by new review after PR update

github-actions[bot]
github-actions bot previously approved these changes Jan 6, 2026
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

📋 Review verdict: APPROVE

👆 The main review comment above is the source of truth for this PR review. It is automatically updated on each review cycle, so always refer to it for the most current feedback.

This formal review submission is for the verdict only.

@graphite-app graphite-app bot requested a review from a team January 6, 2026 01:28
@graphite-app
Copy link

graphite-app bot commented Jan 6, 2026

Graphite Automations

"Request reviewers once CI passes on sdks monorepo" took an action on this PR • (01/06/26)

2 reviewers were added and 1 assignee was added to this PR based on Siyu Jiang (See-You John)'s automation.

@github-actions github-actions bot dismissed their stale review January 6, 2026 19:50

Superseded by new review after PR update

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

📋 Review verdict: APPROVE

👆 The main review comment above is the source of truth for this PR review. It is automatically updated on each review cycle, so always refer to it for the most current feedback.

This formal review submission is for the verdict only.

@xrsv xrsv merged commit b7bafe1 into main Jan 6, 2026
9 checks passed
@xrsv xrsv deleted the fix-weth-v4-start branch January 6, 2026 21:14
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.

4 participants