[codex] Improve Uniswap quote routing#1132
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the Uniswap V3 and V4 swapper implementations to support mixed fee tiers in multi-hop routes and improves code quality through the use of iterators and functional patterns. Key changes include the introduction of new_two_hop_with_fees, a more robust route selection mechanism via get_selected_candidate, and updated command building logic for both V3 and V4. Feedback highlights a critical issue where the Universal Router's TRANSFER command is incorrectly used for native ETH fees, along with suggestions to replace unsafe unwrap() calls with proper error handling and to remove redundant type conversions in path building.
| let fee_command = if input_is_native { | ||
| // if input is native ETH, we can transfer directly | ||
| commands.push(UniversalRouterCommand::TRANSFER(Transfer { | ||
| UniversalRouterCommand::TRANSFER(Transfer { | ||
| token: *token_in, | ||
| recipient: fee_recipient, | ||
| value: U256::from(fee), | ||
| })); | ||
| }) |
There was a problem hiding this comment.
The Universal Router's TRANSFER command (opcode 0x05) is designed for ERC20 tokens and does not support native ETH. When input_is_native is true, token_in is Address::ZERO, and this command will likely fail. For native ETH fees, you should use PAY_PORTION with the special ETH sentinel address 0x0000000000000000000000000000000000000001. Since the fee is calculated using fee_options.bps, PAY_PORTION is the standard way to handle this in the Universal Router.
| let fee_command = if input_is_native { | |
| // if input is native ETH, we can transfer directly | |
| commands.push(UniversalRouterCommand::TRANSFER(Transfer { | |
| UniversalRouterCommand::TRANSFER(Transfer { | |
| token: *token_in, | |
| recipient: fee_recipient, | |
| value: U256::from(fee), | |
| })); | |
| }) | |
| let fee_command = if input_is_native { | |
| // if input is native ETH, we use PAY_PORTION with the ETH sentinel | |
| UniversalRouterCommand::PAY_PORTION(PayPortion { | |
| token: Address::from_str("0x0000000000000000000000000000000000000001").unwrap(), | |
| recipient: fee_recipient, | |
| bips: U256::from(fee_options.bps), | |
| }) | |
| } else { |
| if pay_fees { | ||
| let swap_recipient = if unwrap_output_weth { address_this } else { recipient }; | ||
| let swap_commands = if let Some(fee_options) = fee_options { | ||
| let fee_recipient = Address::from_str(fee_options.address.as_str()).unwrap(); |
There was a problem hiding this comment.
Using unwrap() on Address::from_str can cause the service to panic if an invalid fee address is provided in the request options. It is safer to handle this as an error using the existing eth_address::parse_str utility.
| let fee_recipient = Address::from_str(fee_options.address.as_str()).unwrap(); | |
| let fee_recipient = eth_address::parse_str(fee_options.address.as_str())?; |
|
|
||
| if pay_fees { | ||
| let swap_commands = if let Some(fee_options) = fee_options { | ||
| let fee_recipient = Address::from_str(fee_options.address.as_str()).unwrap(); |
There was a problem hiding this comment.
Using unwrap() on Address::from_str can cause the service to panic if an invalid fee address is provided in the request options. It is safer to handle this as an error using the existing eth_address::parse_str utility.
| let fee_recipient = Address::from_str(fee_options.address.as_str()).unwrap(); | |
| let fee_recipient = eth_address::parse_str(fee_options.address.as_str())?; |
| bytes.extend(&fee.to_be_bytes_vec()); | ||
| bytes.extend(token_out.as_slice()); | ||
| Bytes::from(bytes) | ||
| let fee = U24::from(fee_tier.as_u24()).to_be_bytes_vec(); |
| .iter() | ||
| .enumerate() | ||
| .flat_map(|(idx, token_pair)| { | ||
| let fee = U24::from(token_pair.fee_tier.as_u24()).to_be_bytes_vec(); |
Overview
Improves Uniswap v3/v4 quote quality while keeping routing single-route, exact-input, and limited to direct or two-hop paths.
Problem
Several quote and execution paths could return worse or inconsistent user quotes:
use_max_amountquotes did not consistently reserve native fees before quote-data construction.Solution
quote_value_after_reserve_by_chain()in v3/v4 quote paths and carryquote.from_valuethrough quote data, Permit2, native tx value, and approval checks.min_amount_outin route data and use it directly in v3/v4 command builders.UNWRAP_WETH.Validation
cargo test -p swapper uniswap --no-default-featurescargo test -p gem_evm uniswapjust test swappercargo clippy -p swapper -- -D warningscargo clippy -p gem_evm -- -D warningsjust formatgit diff --check