Fix/uniswap tickmath rounding [WIP don't merge]#73
Conversation
📝 WalkthroughWalkthroughThe PR widens protocol price and tick bounds, updates tick square-root rounding, adjusts boundary tests, and centralizes maker price-to-tick conversion and validation in ChangesProtocol tick and price bounds
Maker order validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/client/trades.rs`:
- Around line 99-106: Validate that price_lower is strictly less than
price_upper before converting or aligning ticks in the surrounding trade range
logic. Return the existing invalid-range error for equal or reversed prices,
while preserving the current tick-bound checks; add regression coverage for
equal and reversed prices that fall within a single tick.
In `@src/convert.rs`:
- Around line 590-593: Update the minimum-price test around
price_to_sqrt_price_x96(1e-6) to assert the conversion succeeds and its value
equals MIN_SQRT_PRICE_X96, ensuring rounding produces the protocol minimum
rather than a lower truncated value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bd98caa7-cf10-44cb-b3f1-16c001f62de2
📒 Files selected for processing (4)
src/client/trades.rssrc/constants.rssrc/convert.rssrc/math/tick.rs
| let tick_lower = align_tick_down(price_to_tick(price_lower)?, TICK_SPACING); | ||
| let tick_upper = align_tick_up(price_to_tick(price_upper)?, TICK_SPACING); | ||
|
|
||
| if tick_lower < MIN_TICK || tick_upper > MAX_TICK || tick_lower >= tick_upper { | ||
| return Err(ValidationError::InvalidTickRange { | ||
| lower: tick_lower, | ||
| upper: tick_upper, | ||
| }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate caller price ordering before widening ticks.
Equal—or reversed—prices that round to the same unaligned tick can become align_down(t) < align_up(t) and pass this check, opening an unintended range. Reject price_lower >= price_upper before alignment, and add a regression test using equal and reversed prices within one tick.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/client/trades.rs` around lines 99 - 106, Validate that price_lower is
strictly less than price_upper before converting or aligning ticks in the
surrounding trade range logic. Return the existing invalid-range error for equal
or reversed prices, while preserving the current tick-bound checks; add
regression coverage for equal and reversed prices that fall within a single
tick.
| // 1e-6 is the protocol's minimum starting price. | ||
| let result = price_to_sqrt_price_x96(1e-6); | ||
| assert!(result.is_ok()); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Make the minimum-price conversion round up and assert the protocol value.
At 1e-6, even ideal scaling produces floor(Q96 / 1000), while MIN_SQRT_PRICE_X96 is one unit higher; Q96 % 1000 = 336. The current test therefore passes while the produced value is below the factory minimum.
Proposed fix
- Ok((scaled_int * Q96) / BIGINT_1E6)
+ Ok((scaled_int * Q96 + BIGINT_1E6 - U256::from(1u8)) / BIGINT_1E6)-use crate::constants::{MAX_SQRT_PRICE_X96, Q96_PRECISION};
+use crate::constants::{MAX_SQRT_PRICE_X96, MIN_SQRT_PRICE_X96, Q96_PRECISION};
fn price_very_small_works() {
- let result = price_to_sqrt_price_x96(1e-6);
- assert!(result.is_ok());
+ assert_eq!(
+ price_to_sqrt_price_x96(1e-6).unwrap(),
+ MIN_SQRT_PRICE_X96
+ );
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // 1e-6 is the protocol's minimum starting price. | |
| let result = price_to_sqrt_price_x96(1e-6); | |
| assert!(result.is_ok()); | |
| } | |
| // 1e-6 is the protocol's minimum starting price. | |
| assert_eq!( | |
| price_to_sqrt_price_x96(1e-6).unwrap(), | |
| MIN_SQRT_PRICE_X96 | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/convert.rs` around lines 590 - 593, Update the minimum-price test around
price_to_sqrt_price_x96(1e-6) to assert the conversion succeeds and its value
equals MIN_SQRT_PRICE_X96, ensuring rounding produces the protocol minimum
rather than a lower truncated value.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Tests