perf!: refactor types and replace num-bigint with fastnum
#93
Conversation
Replace `num-bigint` and `bigdecimal` with `fastnum` for improved performance and simplified arithmetic operations. Updated `Cargo.toml`, core types, and utility functions to utilize `fastnum`'s features and refined the API for consistency. This commits also includes enhancements in test cases and clean-ups for derived constants and methods.
WalkthroughThis pull request introduces a comprehensive update to the Uniswap SDK Core in Rust, focusing on dependency management, type conversions, and core functionality improvements. The changes include updating the package version to Changes
Suggested reviewers
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 (
|
Updated the `BigDecimal` type alias to `fastnum::D512` for consistency. Modified the `to_big_decimal` function to account for the sign of the input, ensuring proper construction of `BigDecimal` instances.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/utils/mod.rs (2)
5-5: Document the newly introducedtypesmodule.Adding a dedicated
typesmodule is a great step toward better organization. However, consider including inline documentation or Rustdoc on thetypesmodule itself to help future maintainers understand its purpose and usage.
11-11: Avoid wildcard exports for clearer code.Re-exporting everything with
pub use types::*;can lead to name collisions and makes it less clear what is actually available from the module. Explicit imports improve maintainability and clarity.src/utils/sqrt.rs (1)
39-40: Potential Floating Precision Pitfall in Tests
While casting tof64and usingsqrt().floor()up to 999 likely works fine, increasing the test range without special handling of large numbers may introduce floating-point inaccuracies. Consider a purely integer-based comparison for larger test ranges.src/entities/token.rs (1)
12-14: Changedbuy_fee_bpsandsell_fee_bpsto Non-Optional
ReplacingOption<BigUint>withu64removes nullable overhead and clarifies that fees must always have a numeric value. This simplifies logic across the board, but verify that zero truly represents “no fee” in all contexts.src/entities/fractions/currency_amount.rs (1)
148-149: Retrieving numerator and denominator by value.
Obtaining the values by value instead of reference may have a small copying overhead, but it simplifies usage. Ensure that this design decision is safe given your performance goals.src/entities/fractions/fraction.rs (1)
87-90: ReturningBigIntby value innumerator/denominator.
While returning by value simplifies usage, it may incur an overhead for copies of large integers. Evaluate if returning references is more optimal for performance in tight loops.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
Cargo.toml(1 hunks)README.md(1 hunks)src/constants.rs(1 hunks)src/entities/fractions/currency_amount.rs(9 hunks)src/entities/fractions/fraction.rs(14 hunks)src/entities/fractions/price.rs(3 hunks)src/entities/token.rs(8 hunks)src/examples/token_example.rs(1 hunks)src/lib.rs(1 hunks)src/utils/compute_zksync_create2_address.rs(1 hunks)src/utils/mod.rs(1 hunks)src/utils/sqrt.rs(3 hunks)src/utils/types.rs(1 hunks)
🔇 Additional comments (43)
src/utils/sqrt.rs (4)
2-2: Use offastnum::i512Import Looks Appropriate
No issues found with importingi512from thefastnumcrate.
12-27: Manual Newton's Method for Integer Square Root Appears Correct
The step-by-step approximation and early returns for special cases look correct. This implementation is straightforward, and negative inputs return an error. Be mindful of performance if this is repeatedly called on very large integers, but overall this is a solid approach.
49-50: 2^i Test Logic is Proper
Raising 2 to power i and verifying its square root ensures a solid large-integer test case.
56-57: Validation AgainstMAX_UINT256
Ensuring that sqrt of the maximum 256-bit integer yields the expected result (2^128 - 1) demonstrates robust coverage of edge cases.src/utils/types.rs (3)
7-9: Constructing BigDecimal from BigInt
UsingBigDecimal::from_partswith a defaultContextis concise. Confirm that the default context meets precision and rounding needs; for most use cases, it's sufficient, but specialized scenarios might require explicit precision.
13-15: Unwrapping onfrom_le_slice
SinceU256is always 32 bytes, this unwrap should never fail. Nevertheless, note that any unexpected usage outsideU256invariants would panic here. For library code, consider if there's ever a scenario it could be misused.
19-21: BigInt Conversion From U256
Creating aBigIntfrom a U256 by first converting toBigUintis logical. No issues found.src/examples/token_example.rs (1)
15-16: Using Zero Fees Instead of Optional Fields
Switching fromNoneto0aligns with the newTokenMetasignature. This is consistent with a simpler, always-present fee model.src/lib.rs (1)
52-55: Updated Type Aliases tofastnum
Replacing the older numeric types withfastnum::I512,fastnum::U512, andfastnum::UD512is consistent with the PR’s performance-focused objectives. Verify that any downstream code depending onbigdecimalornum-bigintfunctionalities has been fully migrated.src/entities/token.rs (8)
7-7: AllowingCopyinTokenMeta
Making the structCopyis generally safe when it holds only numeric and address fields. Confirm that there's no future plan to store heap-allocated data or references here.
65-66: Constructor Updates for Mandatory Fees
Switching parameters tou64ensures no missing fees. Any advanced fee structure can be handled with more explicit design if needed.
138-139: Macro Defaults Buy/Sell Fee to 0
This macro usage enforces the new non-optional fee model. Straightforward and consistent.
149-150: Macro Defaults (Cont’d)
Identical pattern for defaulting fees to zero, robust for typical usage.
160-161: Macro Assignment of Fees to 0
Expected usage aligns with the struct’s updated definition, no concerns here.
171-172: Continuing the Macro Change
Consistent repetition of zero-fee default in the macro. Remains clear and uniform.
182-183: Exact Same Macro Pattern for Named Token
Again, sets buy and sell fees to zero by default, reflecting the new design choice.
193-194: Final Macro Variation
Same approach, finalizing the zero-fee macro usage across all expansions.src/entities/fractions/currency_amount.rs (9)
3-3: Adoptingfastnum::i512import.
Great move towards usingfastnum::i512for larger integer arithmetic. This aligns well with the PR's objective of replacingnum-bigint.
14-14: Switchingdecimal_scaletoBigInt.
UsingBigIntfor the scale factor is consistent with the updated approach across the codebase. This change helps maintain a uniform numeric type strategy.
28-28: Floor division to avoid overflow.
Verifyingnumerator.div_floor(denominator) > MAX_UINT256ensures an overflow boundary check. This logic appears correct, but confirm that fractional cases aren’t incorrectly handled downstream.
37-37: Usingi512!(10).pow(...).
ReplacingBigUint::from(10).pow(exponent)withi512!(10).pow(...)is consistent with the newfastnum-based approach and appears correct.
83-84: Constructing BigDecimal fromquotientanddecimal_scale.
These lines properly convert the raw quotient and scale into decimal form. The direct usage ofto_big_decimal()is clear and avoids unnecessary clones.
119-119: Fraction-based to_significant/to_fixed calculations.
UsingFraction::newwithdecimal_scaleto compute precision-limited outputs is a neat approach. Make sure any rounding edge cases are covered in tests (especially near boundaries).Also applies to: 136-136
195-195: Boundary test withMAX_UINT256.
Creating a currency amount at the maximum boundary is a solid test for overflow. Good coverage.
202-202: Overflow test scenarios.
These lines test amounts just aboveMAX_UINT256. Ensuring an error is thrown or the operation panics as expected is crucial.Also applies to: 209-209
216-218: Verifying numerator exceedingMAX_UINT256.
This is a valid test for partial fractions where the numerator is slightly aboveMAX_UINT256. Good to confirm we can handle intermediate values safely.src/entities/fractions/fraction.rs (9)
9-9: Importingfastnum::i512.
This aligns with the overall library switch, ensuring consistency in large-integer operations.
26-26: Default denominator toi512!(1).
Switching the default fromBigInt::from(1)toi512!(1)neatly integrates the new crate.
103-103: Refactored remainder/invert/to_decimal.
These lines remove unnecessary clones by returningBigIntdirectly. This is cleaner, though confirm usage patterns to ensure minimal overhead.Also applies to: 111-111, 117-117
132-138: Refined significant digits logic.
Introducing.with_rounding_mode(rounding_strategy)and carefully computinginteger_digitsis a good approach. Ensure edge rounding cases get tested (e.g., borderline half-ups).
147-148: Fixed decimal rounding.
Using explicit rounding mode for decimal places keeps the logic predictable. Good improvement for controlling precision.
155-155:as_fractionnow constructs a simpleFraction.
Returning a freshFraction::new(...)avoids references to self. This improves code clarity.
182-183: Direct numeric field return.
Removing references innumerator/denominatoraligns with standard Rust patterns in the rest of the code.Also applies to: 188-189
197-197: Comparison using cross multiplication.
This pattern is standard for fraction equality and ordering checks. No immediate issues found.Also applies to: 217-217
241-241: Arithmetic operations with cross multiplication.
The approach for fraction addition, subtraction, multiplication, and division is correct. The cross multiplication logic is well-known, but watch out for potential overflows if the multiplied parts can exceed current bounds.Also applies to: 256-256, 262-263, 283-283, 298-298, 304-305, 331-332, 357-358
src/constants.rs (1)
28-29: Transitioning to a compile-timeMAX_UINT256.
Replacing the lazy_static approach withpub const MAX_UINT256is a solid change for performance and memory usage. This also avoids runtime initialization overhead.Cargo.toml (3)
3-3: Bumping version to4.0.0-rc.
Signifying a major release candidate aligns nicely with the breaking changes fromnum-biginttofastnum.
13-13: Addingfastnumdependency.
This aligns with the PR's goal of replacingnum-bigint. Confirm that disabling default features and enablinglibmmeets all your needs (e.g., no missing features that might cause issues).
19-19: Introducing a default feature array and extendingstd.
Providing an empty default feature set is a sensible approach to keep the crate lightweight, while extending thestdfeature with"fastnum/std"ensures correct integration.Also applies to: 20-20
src/utils/compute_zksync_create2_address.rs (1)
35-35: LGTM! Ownership semantics improvedThe change to pass
bytesdirectly tokeccak256instead of&bytesis correct as it aligns better with Rust's ownership model, sincekeccak256takes ownership of its input.src/entities/fractions/price.rs (3)
2-2: LGTM! Aligned with performance objectivesThe addition of
fastnum::i512import aligns with the PR's objective of replacingnum-bigintwith more performant alternatives.
39-40: LGTM! Improved scalar calculationsThe change from
BigInttoi512for power calculations is a good optimization:
- Fixed-size integer operations can be more performant than arbitrary-precision
- The range of i512 is more than sufficient for token decimal calculations (typically ≤ 18)
74-75: LGTM! Removed unnecessary clonesGood optimization by removing unnecessary
.clone()calls since the values can be moved directly into the new Price constructor.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/lib.rs(1 hunks)src/utils/types.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/utils/types.rs
🔇 Additional comments (2)
src/lib.rs (2)
52-55: Verify numerical precision requirements with new fixed-size types.The switch from arbitrary-precision types (
num-bigint::BigInt,bigdecimal::BigDecimal) to fixed-size types (fastnum::I512,fastnum::D512) could impact numerical operations. While this change should improve performance, we need to ensure that 512 bits provide sufficient precision for all SDK use cases.Run this script to analyze numeric literal usage and verify precision requirements:
✅ Verification successful
Fixed-size types provide sufficient precision for the codebase
Based on the analysis of numeric literals and operations in the codebase:
- The largest explicit numeric value found is
340282366920938463463374607431768211455insrc/utils/sqrt.rs, which fits within I512/U512.- Most large numbers in the codebase are Ethereum addresses (40-character hex strings), which are well within the 512-bit capacity.
- BigInt operations found in the code:
- Powers of 2 calculations in sqrt.rs
- Operations with MAX_UINT256 in currency_amount.rs
All these operations are within the 512-bit range.The switch to fixed-size 512-bit types from fastnum appears safe as:
- No arbitrary-precision arithmetic patterns were found
- All numeric literals and operations are well within the 512-bit capacity
- Ethereum-related calculations (addresses, amounts, etc.) naturally fit within these bounds
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for large numeric literals that might exceed I512/U512 capacity # Search for numeric literals in the codebase rg -n '(?:\b\d{20,}\b|0x[0-9a-fA-F]{40,}\b)' --type rust # Search for BigInt/BigUint/BigDecimal operations that might rely on arbitrary precision ast-grep --pattern 'BigInt::from($$$)' --lang rust ast-grep --pattern 'BigUint::from($$$)' --lang rust ast-grep --pattern 'BigDecimal::from($$$)' --lang rustLength of output: 2966
52-55: Verify rounding mode compatibility.Ensure that all rounding modes previously used with
bigdecimal::RoundingModehave equivalent options infastnum::decimal::RoundingModeto maintain consistent behavior.Run this script to check rounding mode usage:
✅ Verification successful
Rounding modes are compatible and properly mapped
The verification shows that the rounding modes are compatible between the old and new implementations:
The old
Roundingenum has three variants:
RoundDownRoundHalfUp(default)RoundUpThese are correctly mapped to
fastnum::decimal::RoundingModein theto_rounding_strategyfunction:
Rounding::RoundDown→RoundingMode::DownRounding::RoundHalfUp→RoundingMode::HalfUpRounding::RoundUp→RoundingMode::UpThe mapping preserves the same rounding behavior across the codebase, maintaining consistent functionality in both
FractionandCurrencyAmountimplementations.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for rounding mode usage in the codebase # Search for RoundingMode usage ast-grep --pattern 'RoundingMode::$_' --lang rustLength of output: 318
Script:
#!/bin/bash # Let's check the enum definition of the old Rounding type ast-grep --pattern 'enum Rounding {$$$}' --lang rust # Also check the fastnum RoundingMode definition rg -A10 'pub enum RoundingMode'Length of output: 88
Script:
#!/bin/bash # Let's check for any imports or uses of the old rounding type rg -B2 -A2 'use.*Rounding' # And check the file where we found the rounding mode usage cat src/entities/fractions/fraction.rsLength of output: 13425
Script:
#!/bin/bash # Let's check for the old Rounding enum definition and any imports rg -B2 -A2 'enum Rounding' # Also check for any other files that might use the Rounding enum rg -l 'Rounding::'Length of output: 396
Script:
#!/bin/bash # Let's see the complete Rounding enum definition cat src/constants.rs # And check how it's used in currency_amount.rs cat src/entities/fractions/currency_amount.rsLength of output: 10123
|
Merge this fucking one first. |
Replace
num-bigintandbigdecimalwithfastnumfor improved performance and simplified arithmetic operations. UpdatedCargo.toml, core types, and utility functions to utilizefastnum's features and refined the API for consistency. This commits also includes enhancements in test cases and clean-ups for derived constants and methods.Summary by CodeRabbit
New Features
fastnumlibrary.Breaking Changes
4.0.0-rc.Improvements
Dependency Changes
bigdecimal,num-bigint, andnum-integer.fastnumdependency.