Skip to content

review fixes#427

Merged
liquid-8 merged 2 commits intouniswap-python:dev/v4-finfrom
liquid-8:uniswap4_RC
Mar 18, 2026
Merged

review fixes#427
liquid-8 merged 2 commits intouniswap-python:dev/v4-finfrom
liquid-8:uniswap4_RC

Conversation

@liquid-8
Copy link
Copy Markdown
Member

No description provided.

@greptile-apps
Copy link
Copy Markdown

greptile-apps bot commented Mar 18, 2026

Greptile Summary

This PR cleans up the Uniswap4 class by removing the now-redundant version parameter (and all the if self.version == 4 / else: raise ValueError branches), fixing several real bugs, and tidying up observability. It also correctly relocates pre-commit to dev-dependencies in pyproject.toml.

Key changes:

  • pre-commit moved to dev-dependencies — it is a developer tooling package and had no business being installed in production environments.
  • version parameter and guards removedUniswap4 is a v4-only class; the version checks were dead code and added noise.
  • Default max_slippage reduced from 0.1 (10%) to 0.01 (1%) — a safer default that reduces the risk of unexpected slippage losses for users who do not explicitly set this value.
  • ETH exact-output swap value bug fixed_token_to_token_swap_output was incorrectly sending qty (the exact output amount) as the msg.value for ETH swaps; it now correctly sends amount_in_max.
  • print()logger.info() — logging is now routed through the module logger, respecting application log configuration.
  • approve() address comparison fixed_addr_to_str(token) != ETH_ADDRESS now correctly handles AddressLike (bytes) inputs that would never have equaled the string ETH_ADDRESS.
  • Stale :param version: docstring — the version parameter was removed from the signature but its docstring entry remains on line 77.
  • Silent exception drop in get_token — the bare except Exception: swallows the root-cause exception; prefer raise InvalidToken(address) from e to preserve the original traceback.

Confidence Score: 4/5

  • Safe to merge after addressing the stale docstring and re-adding exception chaining in get_token; no critical regressions introduced.
  • The bulk of changes are straightforward cleanups (removing dead version-guard branches) and genuine bug fixes. The two remaining issues are minor: a stale docstring and a swallowed exception that loses diagnostic context. No new logic errors were introduced.
  • Pay close attention to uniswap/uniswap4.py around the __init__ docstring (line 77) and get_token exception handling (line 1432).

Important Files Changed

Filename Overview
uniswap/uniswap4.py Major cleanup of the Uniswap4 class: removes version parameter and all associated if self.version == 4 guards (the class is v4-only), lowers default max_slippage from 10% to 1%, fixes an ETH value bug in exact-output swaps (ether_amount = amount_in_max instead of qty), replaces print() calls with logger.info(), and fixes the address-to-string comparison in approve(). Two minor issues remain: a stale :param version: docstring entry and a swallowed exception in get_token.
pyproject.toml Correctly moves pre-commit from [tool.poetry.dependencies] (production) to [tool.poetry.dev-dependencies], where it belongs as a development tooling dependency.

Sequence Diagram

sequenceDiagram
    participant Caller
    participant Uniswap4
    participant Permit2
    participant ERC20
    participant Router

    Note over Uniswap4: __init__: version param removed,<br/>max_slippage default 0.1→0.01

    Caller->>Uniswap4: approve(token)
    alt token is NOT ETH (fixed: uses _addr_to_str comparison)
        Uniswap4->>ERC20: approve(permit2_address, max_approval)
        ERC20-->>Uniswap4: tx
        Note over Uniswap4: sleep(7s)
    end
    Uniswap4->>Permit2: approve(token, router_address, max_approval, expiration)
    Permit2-->>Uniswap4: tx
    Uniswap4-->>Caller: tx hash

    Caller->>Uniswap4: _token_to_token_swap_output(input=ETH, qty, qtycap)
    Note over Uniswap4: amount_in_max = (1 + slippage) * qtycap<br/>BUG FIX: ether_amount = amount_in_max (was qty)
    Uniswap4->>Router: execute(commands, [exact_out_params], deadline)<br/>value = amount_in_max
    Router-->>Uniswap4: tx hash
    Uniswap4-->>Caller: tx hash
Loading

Last reviewed commit: "review fixes"

@@ -76,7 +75,7 @@ def __init__(
:param provider: Can be optionally set to a Web3 provider URI. If none set, will fall back to the PROVIDER environment variable, or web3 if set.
:param web3: Can be optionally set to a custom Web3 instance.
:param version: Which version of the Uniswap contracts to use.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Stale :param version: docstring entry

The version parameter was removed from __init__ in this PR, but its :param version: docstring entry on line 77 was left behind. This dangling doc entry will confuse users and tool-generated documentation.

Suggested change
:param version: Which version of the Uniswap contracts to use.

(Simply delete the line — no replacement needed.)

Comment on lines +1432 to 1433
except Exception:
raise InvalidToken(address)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Original exception context is silently dropped

The previous code logged a logger.warning(...) with the original exception before re-raising InvalidToken. That log was removed here, and the bare except Exception: clause means the root cause (e.g., network error, ABI mismatch, contract revert) is now completely invisible to the caller and to logs.

Prefer chaining exceptions so the original traceback is preserved:

Suggested change
except Exception:
raise InvalidToken(address)
except Exception as e:
raise InvalidToken(address) from e

@liquid-8 liquid-8 merged commit 293add0 into uniswap-python:dev/v4-fin Mar 18, 2026
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.

1 participant