Skip to content

Fix/q96 decoding precision [WIP don't merge]#72

Open
rdave8 wants to merge 2 commits into
mainfrom
fix/q96-decoding-precision
Open

Fix/q96 decoding precision [WIP don't merge]#72
rdave8 wants to merge 2 commits into
mainfrom
fix/q96-decoding-precision

Conversation

@rdave8

@rdave8 rdave8 commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Summary by CodeRabbit

  • New Features
    • Expanded supported price boundaries, enabling prices from approximately 0.000001 to 1,000,000.
    • Improved maker order price-range handling with protocol-aligned tick validation.
  • Bug Fixes
    • Improved fixed-point price conversion accuracy, especially for fractional values and extreme price ranges.
    • Added validation to reject invalid or incorrectly ordered maker price bounds.
  • Tests
    • Added coverage for boundary prices, tick alignment, valid ranges, and invalid maker configurations.

@coderabbitai

coderabbitai Bot commented Jul 17, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Protocol price and maker tick bounds are widened to approximately 1e-61e6. Q96 decoding is reworked to preserve integer and fractional components, while maker opening uses centralized aligned-tick validation with expanded boundary tests.

Changes

Protocol bounds and maker validation

Layer / File(s) Summary
Protocol price and tick boundaries
src/constants.rs, src/math/tick.rs
Factory sqrt-price constants and maker tick limits change to the new protocol range, with updated boundary assertions.
Q96 price conversion and boundary coverage
src/convert.rs
price_x96_to_f64 uses quotient and remainder conversion, and tests cover prices from approximately 1e-6 through 1e6.
Maker tick derivation and validation
src/client/trades.rs
open_maker delegates aligned tick calculation and validates protocol bounds, ordering, and invalid price ranges through new unit tests.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

Suggested reviewers: praz314159

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title matches the main change by describing the q96 decoding precision fix, even though it includes extra WIP text.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/q96-decoding-precision

Comment @coderabbitai help to get the list of available commands.

@rdave8 rdave8 changed the title Fix/q96 decoding precision Fix/q96 decoding precision [WIP don't merge] Jul 17, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
src/convert.rs (1)

182-194: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Add a mixed integer-and-fractional Q96 regression test.

Current tests exercise Q96, Q96 / 2, and integral multiples, but not the new reconstruction path with both components present. Add Q96 + Q96 / 2 and assert 1.5.

Proposed test
#[test]
fn price_x96_combines_integer_and_fractional_components() {
    let price = price_x96_to_f64(Q96 + Q96 / U256::from(2u64)).unwrap();
    assert!((price - 1.5).abs() < Q96_PRECISION);
}
🤖 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 182 - 194, Add a regression test for the
price_x96_to_f64 conversion that passes Q96 + Q96 / U256::from(2u64) and
verifies the result is approximately 1.5 using the existing Q96_PRECISION
tolerance. Name the test price_x96_combines_integer_and_fractional_components
alongside the current conversion tests.
🤖 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.

Nitpick comments:
In `@src/convert.rs`:
- Around line 182-194: Add a regression test for the price_x96_to_f64 conversion
that passes Q96 + Q96 / U256::from(2u64) and verifies the result is
approximately 1.5 using the existing Q96_PRECISION tolerance. Name the test
price_x96_combines_integer_and_fractional_components alongside the current
conversion tests.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 34d56beb-7cf8-4ce0-ad12-e2ca503e17aa

📥 Commits

Reviewing files that changed from the base of the PR and between e6aaa24 and 56b1840.

📒 Files selected for processing (4)
  • src/client/trades.rs
  • src/constants.rs
  • src/convert.rs
  • src/math/tick.rs

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