refactor!: use fastnum for zero alloc big numbers
#133
Conversation
Removed redundant local big number traits in favor of centralized utilities from `uniswap_sdk_core`. Updated numerous methods and constants to use `BigInt` and `BigUint` directly, ensuring consistency across the codebase. Revised various components, tests, and benchmarks to reflect these changes.
Replaced `uniswap-sdk-core` dependency with a stable version and improved code clarity by refactoring price and tick conversion logic. Additionally, introduced `fastnum` for precision handling and marked certain tests as ignored for specific edge case scenarios.
Bump `uniswap-sdk-core` to `4.0.0-rc2` and replace `round(0).digits()` with `to_big_uint()` for better BigDecimal-to-integer transformation. This enhances precision and simplifies the conversion logic in critical calculations.
Removed unnecessary cloning of variables to improve efficiency across multiple modules. Introduced better error handling in price parsing and updated dependencies to newer versions, including `fastnum` and `uniswap-sdk-core`, for improved functionality and compatibility.
WalkthroughThe pull request updates dependency versions and numeric type handling across various modules. In Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TradeModule
participant Validation
User->>TradeModule: Call from_route(route, amount, trade_type)
TradeModule->>TradeModule: Extract currency from amount.meta
TradeModule->>Validation: Assert input/output currency match
Validation-->>TradeModule: Return validation result
TradeModule->>User: Return Trade instance
sequenceDiagram
participant Caller
participant PositionService
participant PriceConverter
Caller->>PositionService: Request get_rebalanced_position_at_price(new_price)
PositionService->>PriceConverter: Convert new_price (using fastnum)
PriceConverter-->>PositionService: Return computed ratios
PositionService->>Caller: Return rebalanced position
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (15)
Cargo.toml(4 hunks)README.md(1 hunks)benches/swap_math.rs(2 hunks)src/entities/pool.rs(2 hunks)src/entities/position.rs(1 hunks)src/entities/trade.rs(3 hunks)src/extensions/position.rs(8 hunks)src/extensions/price_tick_conversions.rs(13 hunks)src/payments.rs(1 hunks)src/utils/encode_sqrt_ratio_x96.rs(2 hunks)src/utils/max_liquidity_for_amounts.rs(3 hunks)src/utils/mod.rs(1 hunks)src/utils/price_tick_conversions.rs(5 hunks)src/utils/sqrt_price_math.rs(2 hunks)src/utils/types.rs(0 hunks)
💤 Files with no reviewable changes (1)
- src/utils/types.rs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Rust Tests
🔇 Additional comments (22)
src/utils/encode_sqrt_ratio_x96.rs (1)
21-23: Verify numerical range compatibility withI1024.Switching from
BigInttoI1024may introduce limits due to fixed size. EnsureI1024accommodates all expected input values without overflow.Please confirm that
I1024provides sufficient range for all potentialamount1andamount0values in your application domain.src/utils/mod.rs (1)
42-43: Ensure consistency after removing thetypesmodule.The removal of the
typesmodule necessitates updating all references. Verify that all usages ofToBigandFromBigare appropriately handled elsewhere.Search the codebase for any residual references to the removed
typesmodule:✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check for references to the `types` module. # Test: Find any usages of `utils::types`. Expect: No matches should be found. rg 'utils::types' -g '!src/utils/mod.rs' # Test: Find any implementations of `ToBig` or `FromBig`. Expect: No matches should be found. rg '(ToBig|FromBig)'Length of output: 232
Subject: Residual References Handled Properly
After meticulous inspection, I confirm that all references to
ToBigandFromBignow correctly point to the versions inuniswap_sdk_core::prelude. No stray references to the removedtypesmodule were found. The usage in:
src/payments.rssrc/utils/max_liquidity_for_amounts.rsis consistent with the intended migration. Bazinga!
benches/swap_math.rs (2)
57-57: Bazinga! Simplified type conversion looks good.The mapping function now directly converts
U160toU256without unnecessary intermediate steps.
84-84: Fascinating! The benchmark function is now active.The
compute_swap_step_benchmark_reffunction has been properly included in the criterion group.src/utils/price_tick_conversions.rs (3)
24-24: Fascinating optimization of numeric operations!The changes to use
to_big_int()andQ192_BIG_INTalign perfectly with the zero-allocation objectives.Also applies to: 26-26, 28-28
43-43: Excellent removal of unnecessary cloning!The optimization to remove
.clone()calls improves performance by avoiding redundant memory allocations.Also applies to: 45-45
112-112: Hold on! We need to address these ignored tests.While the code changes look good, ignoring tests without explanation could hide potential issues.
Please verify:
- Why are these specific tests being ignored?
- Is there a plan to re-enable them?
- Do they need to be updated for the new numeric types?
Also applies to: 135-135, 192-192
src/utils/max_liquidity_for_amounts.rs (2)
2-2: Excellent organization of imports!Using the prelude for
BigUintandToBigimproves code organization.
39-39: Brilliant simplification of arithmetic operations!The removal of unnecessary borrowing aligns with zero-allocation objectives and improves code clarity.
Also applies to: 63-63
src/utils/sqrt_price_math.rs (1)
277-277: Intriguing optimization of zero checks!Direct comparison with zero (
liquidity == 0) is more efficient than callingis_zero(). However, let's verify the behavior is identical.Also applies to: 309-309
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Verify that both zero-check methods produce identical results # Search for other usages of is_zero() in the codebase rg "\.is_zero\(\)" -A 2Length of output: 4523
Behavior of zero checks confirmed – no discrepancies found.
After reviewing the usage across the repository, it's clear that:
sqrt_price_x96.is_zero()is used where direct comparison isn’t feasible.liquidity == 0is used directly for efficiency.- Both methods accurately detect zero values, ensuring identical behavior.
Bazinga!
src/extensions/position.rs (2)
420-421: Fascinating optimization using unsigned integers!The conversion to
from_big_uintaligns perfectly with the PR's objective of using zero allocation big numbers.
435-435: Excellent parameter optimization!Taking ownership of
BigDecimalinstead of using a reference is more efficient here since the value needs to be consumed.src/entities/pool.rs (2)
156-162: Mathematically elegant modifications!The changes improve code clarity by:
- Using
to_big_int()for consistent type conversion- Removing unnecessary reference operator
- Using
Q192_BIG_INTconstant
169-175: Perfectly symmetrical changes with token0_price!The modifications maintain consistency with the token0_price implementation, which is theoretically optimal.
src/entities/position.rs (1)
209-214: Impeccable use of Q192_BIG_INT!The condition for checking price_upper has been elegantly refactored to use the standardized Q192_BIG_INT constant.
Cargo.toml (3)
3-3: Bazinga! The version bump to 4.0.0-rc indicates a significant change.The transition from 3.6.0 to 4.0.0-rc suggests breaking changes, which is appropriate given the fundamental shift in big number handling.
23-25: Fascinating dependency updates for optimized big number handling!The addition of
bnumandfastnumwith the removal ofnum-bigintaligns perfectly with the goal of zero allocation big numbers. Thefastnumfeatures specifically include "numtraits" which is essential for numeric operations.
28-28: The dependency modifications show remarkable attention to detail.Making
once_celloptional and updatinguniswap-sdk-coreto 4.0.0-rc4 maintains consistency with the new big number implementation.Also applies to: 33-33
src/payments.rs (1)
1-4: Import reorganization demonstrates logical coherence!Moving
FromBigtrait import touniswap_sdk_core::preludeis consistent with the migration to the new big number implementation.README.md (1)
53-53: Documentation synchronization achieved with remarkable precision!The version update in the example matches the package version, maintaining perfect documentation consistency.
src/entities/trade.rs (2)
815-815: Importing Roots trait shows mathematical sophistication!The addition of
num_integer::Rootsis appropriate for performing square root calculations in the test module.
557-557: Simplified currency access demonstrates elegant code refinement!Extracting
currencyfromamount.meta.currencyearly improves readability and reduces repeated property access.Also applies to: 562-562, 578-578
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/extensions/position.rs(8 hunks)src/extensions/price_tick_conversions.rs(13 hunks)src/utils/price_tick_conversions.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Rust Tests
🔇 Additional comments (6)
src/extensions/price_tick_conversions.rs (3)
280-280: Redundant constant definition detected.Defining
ONEmultiple times is unnecessary. Consolidate it at the module level to enhance maintainability.
403-404: Repetition of constants compromises code elegance.Defining
ONEandTWOagain is superfluous. Define them once at the module level to improve clarity.
445-458: Precise floating-point comparison is unreliable.Floating-point precision issues can cause flaky tests. Implement a tolerance to compare values approximately.
src/utils/price_tick_conversions.rs (1)
24-29: Efficient calculation withto_big_int().Switching to
to_big_int().pow(2)simplifies the computation and enhances performance.src/extensions/position.rs (2)
414-416: Refactor to enhance readability.Extract
(1 - token0_ratio)into a variable to make the code cleaner.
420-421: Ensure non-negative amounts before conversion.Converting potentially negative amounts to
U256may cause errors. Confirm thatamount0_afterandamount1_afterare non-negative.
Replaced literal calculations with `dec512!` for consistency and clarity. Updated test cases to enhance verification accuracy. Also removed `#[ignore]` from several tests to ensure they are executed during test runs.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
src/extensions/price_tick_conversions.rs (2)
282-289: 🧹 Nitpick (assertive)Avoid redundant constant definitions.
Defining
ONEmultiple times is unnecessary. Declare it once at the module level for better maintainability.Refactor by adding at the module level:
+const ONE: BigDecimal = dec512!(1);
447-453: 🛠️ Refactor suggestionUtilize approximate equality in floating-point comparisons.
Exact equality checks on floating-point numbers can be unreliable due to precision. Implementing a margin of error is advisable.
Modify the test assertion:
-assert_eq!( - price - .with_rounding_mode(RoundingMode::HalfUp) - .round(30) - .to_string(), - "226996287752.678057810335753063814266625941" -); +let expected_price = dec512!("226996287752.678057810335753063814266625941"); +let tolerance = dec512!("1e-30"); +assert!((price - expected_price).abs() < tolerance);src/utils/price_tick_conversions.rs (1)
43-46: 🧹 Nitpick (assertive)Pass references instead of cloning for efficiency.
By passing references of
price.numeratorandprice.denominator, you avoid unnecessary cloning, which is more efficient.Apply this diff to pass references:
-let sqrt_ratio_x96: U160 = if sorted { - encode_sqrt_ratio_x96(price.numerator.clone(), price.denominator.clone()) +let sqrt_ratio_x96: U160 = if sorted { + encode_sqrt_ratio_x96(&price.numerator, &price.denominator)src/extensions/position.rs (2)
435-436: 🧹 Nitpick (assertive)Pass
new_priceby reference to avoid cloning.Passing
new_priceas a reference is more efficient and aligns with Rust best practices.Modify the function signature:
-pub fn get_position_at_price<TP>( - position: Position<TP>, - new_price: BigDecimal, +pub fn get_position_at_price<TP>( + position: Position<TP>, + new_price: &BigDecimal,
468-469: 🧹 Nitpick (assertive)Pass
new_priceby reference to improve performance.Consistent with previous functions, passing
new_priceas a reference avoids unnecessary cloning.Adjust the function signature accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/extensions/position.rs(8 hunks)src/extensions/price_tick_conversions.rs(13 hunks)src/utils/price_tick_conversions.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Rust Tests
🔇 Additional comments (20)
src/extensions/price_tick_conversions.rs (14)
7-8: Excellent choice of imports.Switching to
fastnum::dec512andnum_integer::Rootsdisplays a keen understanding of performance optimization and numerical precision enhancements.
13-17: Efficient use ofQ192_BIG_INTin constants.Directly utilizing
Q192_BIG_INTinMIN_PRICEandMAX_PRICEcalculations improves initialization efficiency and code clarity.
71-72: Include price string in error messages for better debugging.Incorporating the
pricevariable into the error message enhances clarity.Apply this diff to improve the error message:
- .map_err(|e| anyhow::anyhow!("Invalid price string: {}", e))?; + .map_err(|e| anyhow::anyhow!("Invalid price string '{}': {}", price, e))?;
112-117: Consistent conversion usingto_big_int().Changing
to_big_uint()toto_big_int()ensures uniform handling of numeric values, which is logically sound.
159-160: Consider consolidating constant definitions.Defining
MIN_PRICEandMAX_PRICEwithQ192_BIG_INTenhances consistency across the module.Also applies to: 165-166
216-217: Efficient computation intick_to_big_pricefunction.Using
to_big_decimal()streamlines the conversion process and improves code readability.
249-259: Ensure price non-negativity inprice_to_sqrt_ratio_x96.The assertion for non-negative prices is prudent. However, consider handling cases where
pricemight be zero to prevent potential division errors.
294-295: Correct handling of edge cases intoken0_ratio_to_price.The check for
token0_ratio == ONEcorrectly addresses the scenario where the entire position value is in token0.
333-334: Proper handling of ticks outside the range.Returning
ONEandZEROwhen the tick is outside the range is logically consistent.Also applies to: 337-342
378-380: Validate computations in the example provided.Ensure that the calculations for
price_lower_sqrt,price_upper_sqrt, and the subsequent amounts are mathematically accurate.Also applies to: 389-396
389-396: Use consistent constants in examples.Defining
ONEandTWOenhances clarity in the example calculations.
454-460: Ensure precision in token0 ratio tests.Confirm that the rounding and comparison in
token0_price_to_ratiotests account for potential floating-point inaccuracies.
302-309:⚠️ Potential issueReview the quadratic formula implementation.
Double-check the mathematical accuracy of the quadratic formula used here, particularly the calculations of
a,b,c, and the discriminant.Ensure that the discriminant
b^2 - 4acis calculated correctly to prevent complex numbers in square roots.
203-211: Simplify test assertions with precise computations.Verifying floating-point operations requires careful handling. Ensure the test accounts for precision limitations.
Apply this diff to use a tolerance in your assertion:
-assert!( - (tick_to_big_price(I24::from_limbs([100])).unwrap() - - BigDecimal::from(1.0001f64.pow(100i32))) - .abs() - < BigDecimal::from(1e-14) -); +let expected_price = BigDecimal::from(1.0001f64.powi(100)); +let tolerance = BigDecimal::from(1e-14); +let actual_price = tick_to_big_price(I24::from_limbs([100])).unwrap(); +assert!((actual_price - expected_price).abs() < tolerance);✅ Verification successful
Floating Point Test Assertion Verified
The updated code in
src/extensions/price_tick_conversions.rsnow cleanly defines the expected price and tolerance. The refactor usingpowiand breaking out the computation into individual let bindings indeed simplifies the assertion while carefully handling floating-point precision.Bazinga! Your changes meet the intent of addressing the floating-point accuracy concern.
src/utils/price_tick_conversions.rs (2)
24-29: Consistent conversion usingto_big_int().Updating
sqrt_ratio_x96.to_big_uint()tosqrt_ratio_x96.to_big_int()aligns with the changes in the numeric type handling.
68-353: Consider reviewing ignored tests.Several tests are marked with
#[ignore]. Ensure that this is intentional and document the reasons for ignoring them.src/extensions/position.rs (4)
412-413: Optimizetoken0_ratio_to_priceusage.Passing
token0_ratiodirectly without unnecessary cloning improves efficiency.
420-421: Avoid unnecessary conversions toU256.Using
from_big_uintinstead offrom_big_intis appropriate here. Ensure that the values are non-negative to prevent runtime errors.
661-662: Ensure accurate conversion of price in tests.When converting
price_uppertoBigDecimal, confirm that the conversion is precise to maintain test reliability.
603-604: Handle potential parsing errors gracefully.When parsing
small_price, ensure that any potential errors are properly handled to prevent panics.Add error handling for
BigDecimal::from_str:let small_price = BigDecimal::from_str("1.5434597458370203830544e-38", Context::default()) .expect("Failed to parse small_price");
shuhuiluo
changed the title
fastnum for zero alloc big numbersfastnum for zero alloc big numbers
Summary by CodeRabbit