Add more v4 tests#456
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev/v4-fin #456 +/- ##
===============================================
+ Coverage 36.77% 70.45% +33.68%
===============================================
Files 12 12
Lines 2238 2244 +6
===============================================
+ Hits 823 1581 +758
+ Misses 1415 663 -752 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Greptile SummaryThis PR fixes several bugs in the v4 swap pipeline (double-encoding of multi-hop routes, wrong token argument in
Confidence Score: 3/5Several correctness issues flagged in earlier review rounds remain unaddressed — most notably the inflated gas budget in drop_txn and the stale-nonce risk in update_last_nonce — and a new assertion in test_get_position_info is structurally broken due to a hex-prefix mismatch. The swap double-encoding fix and the get_price_output token-argument fix are genuine improvements, and the test suite is now far more substantive than before. However, drop_txn still uses self.gas_limit instead of the required 21 000 gas for a plain ETH transfer, update_last_nonce still queries 'latest' rather than 'pending' (risking nonce reuse), and test_get_position_info's pool-ID comparison will always fail because hex() produces a '0x'-prefixed string while bytes.hex() does not. These pre-existing unresolved findings keep confidence below the midpoint. uniswap/uniswap4.py (drop_txn gas budget, update_last_nonce block specifier) and tests/test_uniswap4.py (test_get_position_info pool-ID assertion) need attention before merge. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["make_swap_input / make_swap_output"] --> B{route is None?}
B -- Yes --> C["token_to_token_swap_exact_input/output\n(single-hop, uses pool_key directly)"]
B -- No --> D["token_to_token_swap_input/output\n(now encodes route internally)"]
D --> E["encode_path_keys_input/output\n(@staticmethod)"]
E --> F["_build_and_send_tx\n(custom_nonce passed through)"]
C --> F
F --> G{custom_nonce is None?}
G -- Yes --> H["last_nonce = tx nonce + 1"]
G -- No --> I["last_nonce unchanged\n(caller must call update_last_nonce)"]
H --> J["send_raw_transaction"]
I --> J
Reviews (17): Last reviewed commit: "Add the rest of pool data tests" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| "token_id, token0_decimals, token1_decimals", | ||
| [(TOKEN_ID, ETH_DECIMALS, USDC_DECIMALS)], | ||
| ) | ||
| def test_get_position_value( | ||
| self, | ||
| client: Uniswap4, | ||
| token_id: int, | ||
| token0_decimals: int, | ||
| token1_decimals: int, | ||
| ): | ||
| result = client.get_position_value(token_id, token0_decimals, token1_decimals) | ||
| assert result | ||
|
|
||
| @pytest.mark.parametrize( | ||
| "transaction_hash", |
There was a problem hiding this comment.
hex(int_value) in Python always returns a string prefixed with "0x" (e.g. "0x3f4a..."), while bytes.hex() returns plain lowercase hex with no prefix (e.g. "3f4a..."). The slice comparison truncated_pool_id_str == pool_id_str[:len(truncated_pool_id_str)] will therefore never be True — the first two characters ("0x") in truncated_pool_id_str can never match the leading hex digits of pool_id_str. Strip the prefix with [2:] when building truncated_pool_id_str.
| "gas": int(self.gas_limit), | ||
| "chainId": int(self.w3.eth.chain_id), | ||
| } | ||
| signed_txn = self.w3.eth.account.sign_transaction( | ||
| dict( | ||
| chainId=int(self.w3.net.version), | ||
| nonce=self.last_nonce if custom_nonce is None else custom_nonce, | ||
| gasPrice=Web3.to_wei(gas_price, "gwei"), | ||
| gas=int(21000), | ||
| to=Web3.to_checksum_address(address_to), | ||
| value=Web3.to_wei(0, "wei"), | ||
| ), | ||
| transaction_dict_legacy, | ||
| self.private_key, | ||
| ) | ||
| # This one is for post-Merge transactions | ||
| transaction_dict = { | ||
| "type": 2, | ||
| "nonce": self.w3.eth.get_transaction_count(self.address) | ||
| if custom_nonce is None | ||
| else custom_nonce, | ||
| "from": _addr_to_str(self.address), | ||
| "to": _addr_to_str(address_to), | ||
| "value": Web3.to_wei(0, "wei"), | ||
| "maxFeePerGas": Web3.to_wei(int(gas_price), "gwei"), | ||
| "maxPriorityFeePerGas": Web3.to_wei(priority_fee, "gwei"), | ||
| "gas": int(self.gas_limit), |
There was a problem hiding this comment.
drop_txn now uses self.gas_limit instead of the required 21 000 gas for a plain ETH transfer
A zero-value ETH transfer unconditionally consumes exactly 21 000 gas. Validators require that balance ≥ value + gas_limit × max_fee. Using self.gas_limit (typically a large contract-interaction budget, e.g. 6 M gas) inflates the pre-execution balance check: at a modest 50 gwei the node requires 6 000 000 × 50 gwei = 0.3 ETH just to admit the transaction. This makes drop_txn most likely to fail precisely when it is needed most — when the account is running low. Both transaction_dict_legacy and transaction_dict carry this regression and should be restored to "gas": 21_000.
| @pytest.mark.parametrize( | ||
| "token0, token1, qty, pool_key, hook_data, route", | ||
| [ | ||
| ( | ||
| ETH_ADDRESS, | ||
| USDC_ADDRESS, | ||
| 1000 * ONE_USDC, | ||
| eth_usdc_poolkey, | ||
| b"", | ||
| None, | ||
| ), | ||
| ( | ||
| USDC_ADDRESS, | ||
| ETH_ADDRESS, | ||
| ONE_ETH, | ||
| eth_usdc_poolkey, | ||
| b"", | ||
| None, | ||
| ), | ||
| ( | ||
| ETH_ADDRESS, | ||
| USDT_ADDRESS, | ||
| ONE_ETH, | ||
| None, | ||
| None, | ||
| None, | ||
| None, | ||
| [ | ||
| usdc_usdt_poolkey, | ||
| eth_usdc_poolkey, | ||
| ], | ||
| ), | ||
| ( | ||
| USDT_ADDRESS, | ||
| ETH_ADDRESS, | ||
| 1000 * ONE_USDT, | ||
| None, | ||
| None, | ||
| None, | ||
| None, | ||
| [ | ||
| eth_usdc_poolkey, | ||
| usdc_usdt_poolkey, | ||
| ], | ||
| ), | ||
| ], | ||
| ) | ||
| def test_make_swap_output( |
There was a problem hiding this comment.
test_make_swap_output has the same parametrize mismatch as test_make_swap_input. The decorator names 6 fields ("token0, token1, qty, pool_key, hook_data, route"), but the function signature has a 7th parameter custom_nonce that isn't covered. Cases 3–4 each provide 8 values — the extra None, None at positions 4–5 shifts route to position 8, creating a tuple-length mismatch that causes pytest to raise a ValueError and refuse to collect those parametrize cases entirely.
| @pytest.mark.parametrize( | |
| "token0, token1, qty, pool_key, hook_data, route", | |
| [ | |
| ( | |
| ETH_ADDRESS, | |
| USDC_ADDRESS, | |
| 1000 * ONE_USDC, | |
| eth_usdc_poolkey, | |
| b"", | |
| None, | |
| ), | |
| ( | |
| USDC_ADDRESS, | |
| ETH_ADDRESS, | |
| ONE_ETH, | |
| eth_usdc_poolkey, | |
| b"", | |
| None, | |
| ), | |
| ( | |
| ETH_ADDRESS, | |
| USDT_ADDRESS, | |
| ONE_ETH, | |
| None, | |
| None, | |
| None, | |
| None, | |
| [ | |
| usdc_usdt_poolkey, | |
| eth_usdc_poolkey, | |
| ], | |
| ), | |
| ( | |
| USDT_ADDRESS, | |
| ETH_ADDRESS, | |
| 1000 * ONE_USDT, | |
| None, | |
| None, | |
| None, | |
| None, | |
| [ | |
| eth_usdc_poolkey, | |
| usdc_usdt_poolkey, | |
| ], | |
| ), | |
| ], | |
| ) | |
| def test_make_swap_output( | |
| @pytest.mark.parametrize( | |
| "token0, token1, qty, pool_key, hook_data, route, custom_nonce", | |
| [ | |
| ( | |
| ETH_ADDRESS, | |
| USDC_ADDRESS, | |
| 1000 * ONE_USDC, | |
| eth_usdc_poolkey, | |
| b"", | |
| None, | |
| None, | |
| ), | |
| ( | |
| USDC_ADDRESS, | |
| ETH_ADDRESS, | |
| ONE_ETH, | |
| eth_usdc_poolkey, | |
| b"", | |
| None, | |
| None, | |
| ), | |
| ( | |
| ETH_ADDRESS, | |
| USDT_ADDRESS, | |
| ONE_ETH, | |
| None, | |
| None, | |
| [ | |
| usdc_usdt_poolkey, | |
| eth_usdc_poolkey, | |
| ], | |
| None, | |
| ), | |
| ( | |
| USDT_ADDRESS, | |
| ETH_ADDRESS, | |
| 1000 * ONE_USDT, | |
| None, | |
| None, | |
| [ | |
| eth_usdc_poolkey, | |
| usdc_usdt_poolkey, | |
| ], | |
| None, | |
| ), | |
| ], | |
| ) | |
| def test_make_swap_output( |
1 participant
No description provided.