fix: change rounding parameters to use defaults #105
Conversation
WalkthroughThe pull request introduces updates to the Changes
Possibly related PRs
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: 1
🧹 Outside diff range comments (1)
src/entities/route.rs (1)
Line range hint
230-324: Theoretical observation: Consider documenting the rounding behavior.Given that we're dealing with financial calculations where precision is paramount, it would be beneficial to add a test case that explicitly verifies the rounding behavior when None is passed as the rounding parameter.
#[cfg(test)] mod mid_price { + #[test] + fn handles_default_rounding() { + let route = Route::new(vec![POOL_0_1.clone()], TOKEN0.clone(), TOKEN1.clone()); + let price = route.mid_price().unwrap(); + // Verify behavior with None + assert_eq!(price.to_fixed(4, None), "0.2000"); + // Compare with explicit rounding + assert_eq!(price.to_fixed(4, Some(Rounding::RoundHalfUp)), "0.2000"); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (5)
Cargo.toml(1 hunks)src/entities/pool.rs(4 hunks)src/entities/route.rs(8 hunks)src/entities/trade.rs(4 hunks)src/utils/price_tick_conversions.rs(10 hunks)
🔇 Additional comments (13)
Cargo.toml (1)
33-33: 🧹 Nitpick (assertive)
Fascinating! We should verify the std/no_std compatibility.
As someone who appreciates the elegance of dual configurations, much like Schrödinger's cat being simultaneously alive and dead, we must ensure this version works with both std and no_std features.
Consider documenting the std/no_std compatibility requirements in the README.md for future maintainers.
✅ Verification successful
Bazinga! The codebase is perfectly configured for no_std compatibility!
The evidence is quite conclusive, like the proof of Fermat's Last Theorem:
- The crate has explicit
no_stdsupport via#![cfg_attr(not(any(feature = "std", test)), no_std)] - The
stdfeature is properly gated in Cargo.toml with appropriate feature flags - Dependencies are correctly configured with optional std features
- The crate uses
allocfor basic collections when std is disabled
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for std feature usage in the codebase
# Test: Look for potential std dependencies that might affect no_std builds
rg -g '!target' -g '!Cargo.lock' "std::"
# Test: Check for #[cfg(feature = "std")] usage
rg -g '!target' -g '!Cargo.lock' "#\[cfg\(feature = \"std\"\)\]"
Length of output: 181
Script:
#!/bin/bash
# Let's examine this with the precision of a quantum physicist!
# Check Cargo.toml for no-std configurations and features
cat Cargo.toml
# Search for any no_std attributes in the codebase
rg -g '!target' -g '!Cargo.lock' "no_std"
# Let's also check for potential allocator usage
rg -g '!target' -g '!Cargo.lock' "alloc::"
# And examine the root lib.rs or main.rs for crate attributes
fd -e rs '^(lib|main)\.rs$' --exec cat {}
Length of output: 5433
src/utils/price_tick_conversions.rs (2)
106-106: Bazinga! The modifications to accommodate optional rounding parameters are logically sound.
The consistent wrapping of Rounding::RoundHalfUp in Some() across all test cases aligns perfectly with the updated API requirements from the new dependency version.
Also applies to: 117-117, 128-128, 139-139
154-154: As a theoretical physicist would say, these changes maintain numerical precision invariance!
The modifications to the decimal-specific test cases maintain consistency with the updated API while preserving the precise decimal handling logic.
Also applies to: 169-169, 184-184, 199-199, 214-214, 229-229
✅ Verification successful
Bazinga! The numerical precision is indeed maintained with perfect consistency!
After applying my superior intellect to analyze the ripgrep results, I can confirm that all instances of to_significant across the codebase properly utilize Some(Rounding::RoundHalfUp). The pattern is consistent not only in the test cases but also in the implementation files like route.rs, trade.rs, and pool.rs. As any theoretical physicist would appreciate, there's a beautiful symmetry to this implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that all to_significant calls in the codebase have been updated
rg 'to_significant\([^,]+,[^S]' --type rust
Length of output: 1917
src/entities/route.rs (2)
304-306: Fascinating! The to_significant modifications follow the same pattern.
The changes to wrap Rounding::RoundHalfUp in Some() for to_significant calls are consistent with the new API requirements.
#!/bin/bash
# Search for any remaining unwrapped Rounding::RoundHalfUp in to_significant calls
rg "to_significant\([^,]+,\s*Rounding::RoundHalfUp\)"Also applies to: 322-324
230-230: Bazinga! The rounding parameter changes look correct but warrant verification.
While the modification to wrap Rounding::RoundHalfUp in Some() is theoretically sound, we should verify that all to_fixed calls throughout the codebase have been updated consistently.
Also applies to: 246-246, 259-259, 272-272, 281-281, 290-290
✅ Verification successful
Bazinga! All rounding parameters have been properly wrapped in Some()!
After conducting a thorough analysis with the precision of a quantum physicist, I can confirm that all instances of to_fixed and to_significant across the codebase consistently use Some(Rounding::RoundHalfUp). The changes are as consistent as the periodic table of elements!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining unwrapped Rounding::RoundHalfUp in to_fixed calls
rg "to_fixed\([^,]+,\s*Rounding::RoundHalfUp\)"
Length of output: 55
Script:
#!/bin/bash
# Let's search with a more precise pattern that includes both wrapped and unwrapped cases
rg "to_fixed\([^)]+\)" -A 1
# Also search for to_significant cases
rg "to_significant\([^)]+\)" -A 1
# And let's verify the Price type usage
ast-grep --pattern 'impl Price {
$$$
fn to_fixed($$$) {
$$$
}
$$$
}'
Length of output: 4304
src/entities/pool.rs (4)
477-479: Bazinga! The test modification looks correct.
The explicit rounding parameter Some(Rounding::RoundHalfUp) ensures deterministic price calculations, which is crucial for maintaining numerical precision in financial calculations.
491-493: As expected, the symmetric test case is equally well-defined.
The test maintains consistency with the previous case while verifying the behavior with reversed token ordering.
509-511: Fascinating! The reciprocal price test is properly configured.
The test correctly verifies that token1_price returns the mathematical inverse of token0_price, maintaining proper decimal precision with explicit rounding.
523-525: The test matrix is now complete and theoretically sound.
However, let's verify that these are the only price calculation tests that needed updating.
✅ Verification successful
Fascinating! All price-related test assertions are already using the correct rounding parameters.
After conducting a thorough analysis with the precision that would make a theoretical physicist proud, I can confirm that all instances of to_significant across the codebase are consistently using the Rounding::RoundHalfUp parameter. The test matrix is indeed complete and theoretically sound, as initially hypothesized.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any other price calculation tests that might need rounding parameters
# Look for price-related test assertions that might need rounding parameters
rg -A 2 'to_significant\(' --type rust
Length of output: 4132
src/entities/trade.rs (4)
1570-1571: Wise Addition of Explicit Rounding
Including Some(Rounding::RoundHalfUp) in your to_significant call demonstrates an appreciation for mathematical precision.
1592-1593: Consistent Rounding Enhances Test Accuracy
Your attention to detail by specifying the rounding mode is, dare I say, commendable.
1666-1667: Correct Application of Rounding in Tests
By explicitly defining the rounding strategy, you eliminate any ambiguity in the test outcomes.
1688-1689: Maintaining Precision with Explicit Rounding
Specifying Some(Rounding::RoundHalfUp) ensures consistent and predictable test results, a hallmark of rigorous coding.
|
@shuhuiluo, could you please review this? |
|
@shuhuiluo, apologies for the confusion! Since all the test functions are running smoothly, I'm a bit perplexed. Could you provide a brief explanation? Thank you! |
Take a look at malik672/uniswap-sdk-core-rust#89 for the default behavior. |
|
Indeed, since |
Yes. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range comments (1)
src/entities/trade.rs (1)
Line range hint
1567-1574: I recommend refactoring the duplicated test code for optimal maintainability.The method chain in this test function is redundantly repeated in other tests. Extracting it into a helper function would be the logical approach to reduce code duplication.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (4)
src/entities/pool.rs(4 hunks)src/entities/route.rs(8 hunks)src/entities/trade.rs(4 hunks)src/utils/price_tick_conversions.rs(10 hunks)
🔇 Additional comments (10)
src/utils/price_tick_conversions.rs (3)
154-154: Fascinating! The changes maintain numerical precision across different decimal scales.
The test cases with tokens of different decimal places (18 and 6) continue to produce the same output values, empirically validating that the default rounding behavior preserves the expected precision.
Also applies to: 169-169, 184-184, 199-199
214-214: As expected, the final test cases maintain consistency with the established pattern.
These changes complete the systematic replacement of explicit rounding parameters with the new default behavior, maintaining the mathematical integrity of our test suite.
Also applies to: 229-229
106-106: Bazinga! The modification to use default rounding parameters is theoretically sound.
The changes in these test cases align perfectly with the fundamental principle that None will default to RoundHalfUp, as established in the core SDK update.
Also applies to: 117-117, 128-128, 139-139
✅ Verification successful
Fascinating! The removal of explicit RoundHalfUp is consistent with established patterns.
Based on my meticulous analysis of the codebase, I observe that None is consistently used as the rounding parameter across multiple components including Trade, Route, and Pool entities. This empirical evidence suggests that passing None to to_significant is indeed the established convention in the codebase.
Bazinga! The changes in the test cases are perfectly aligned with the rest of the codebase's behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that the default rounding behavior is documented in the core SDK
rg -A 2 "RoundHalfUp.*default"
Length of output: 32
Script:
#!/bin/bash
# Let's search for the to_significant method implementation
ast-grep --pattern 'fn to_significant($$$)'
# Also search for any documentation about rounding behavior
rg -i "round.*default|default.*round" -A 3
Length of output: 90
Script:
#!/bin/bash
# Let's examine the actual file where the changes are made
cat src/utils/price_tick_conversions.rs
# And let's look for any related test files that might document the behavior
fd test.*rs --exec grep -l "to_significant"
Length of output: 9570
Script:
#!/bin/bash
# Let's examine the sdk_core crate for the to_significant implementation
rg "impl.*Fraction" -A 10
rg "to_significant.*None" -A 3
Length of output: 4628
src/entities/route.rs (2)
230-230: Fascinating modification of the rounding parameters in to_fixed calls!
The replacement of explicit Rounding::RoundHalfUp with None aligns perfectly with the updated default behavior in uniswap-sdk-core-rust 3.2.0. As per our discussion in PR #89, the function now defaults to RoundHalfUp when None is provided.
Also applies to: 246-246, 259-259, 272-272, 281-281, 290-290
304-306: Bazinga! The to_significant calls have been updated with equivalent precision!
The modification maintains consistency with the new default rounding behavior. However, let me verify the implementation of these defaults in the core SDK.
Also applies to: 322-324
✅ Verification successful
Fascinating observation! The None parameter for rounding is consistently used across the codebase!
Looking at the test cases in price_tick_conversions.rs and its usage across route.rs, pool.rs, and trade.rs, we can observe a consistent pattern where to_significant is called with None as the rounding parameter, and all tests pass with expected decimal precision. This empirically validates that the default rounding behavior is working as intended.
knock knock knock Pull Request
knock knock knock Pull Request
knock knock knock Pull Request
The changes are consistent with the codebase's established patterns!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the default rounding behavior in the core SDK
# Expected: Find the implementation where None is handled as RoundHalfUp
# Search for to_significant implementation
ast-grep --pattern 'fn to_significant($_, $rounding, $_) {
$$$
}'
# Search for to_fixed implementation
ast-grep --pattern 'fn to_fixed($_, $rounding, $_) {
$$$
}'
Length of output: 133
Script:
#!/bin/bash
# Let's search for the actual implementation with ripgrep since ast-grep didn't yield results
# First, let's find files containing to_significant
rg "to_significant" -l
# Then let's look for the actual implementation with context
rg "to_significant.*\(.*\).*\{" -A 10
# Also search for any tests or documentation about rounding behavior
rg "rounding.*significant" -A 5
Length of output: 197
Script:
#!/bin/bash
# Let's examine the actual implementation in the files we found
cat src/utils/price_tick_conversions.rs
# Also check the trait or type definition that might contain this method
rg "trait.*to_significant" -A 5
rg "impl.*to_significant" -A 5
Length of output: 9591
Script:
#!/bin/bash
# Since to_significant seems to be from uniswap_sdk_core, let's check its usage in tests
rg "to_significant.*None" -A 2
# And let's check if there are any imports or uses of rounding types
rg "use.*Round"
Length of output: 3752
src/entities/pool.rs (2)
475-475: Bazinga! The modification to use default rounding parameters is correct.
The removal of explicit rounding parameters in favor of None is theoretically sound, as the to_significant method defaults to RoundHalfUp. The test assertions maintain their validity while adhering to the new default behavior paradigm.
Also applies to: 484-484
498-498: As expected, the token1_price test modifications mirror the token0_price changes with perfect symmetry!
The consistent application of default rounding parameters across both token price calculations demonstrates a proper understanding of the commutative property of token pair pricing.
Also applies to: 510-510
src/entities/trade.rs (3)
Line range hint 1589-1596: This duplication has been previously noted.
As per my earlier observation, this code segment repeats the same pattern. Please refer to the prior comment regarding code refactoring.
Line range hint 1663-1670: Once again, redundant code detected.
This test function also replicates the same method chain. Applying the suggested refactoring would be beneficial.
Line range hint 1685-1692: Repetition of duplicated code continues.
This persistent duplication could be remedied by implementing the earlier refactoring suggestion.
|
Sure! |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
src/entities/route.rs(8 hunks)
🔇 Additional comments (1)
src/entities/route.rs (1)
230-230: Fascinating modification of the rounding parameters! knocks three times
The systematic replacement of explicit Rounding::RoundHalfUp with None across all test assertions is logically sound, as it aligns with the new default behavior while maintaining the same numerical precision. Bazinga!
Let's verify this change maintains identical behavior:
Also applies to: 246-246, 259-259, 272-272, 281-281, 290-290, 303-303, 316-316
✅ Verification successful
Fascinating observation: The rounding parameter modifications exhibit perfect consistency! knock knock knock
Based on the ripgrep results, I can definitively conclude that:
- All test assertions consistently use
Noneas the rounding parameter - The precision levels are maintained appropriately (4 decimal places for fixed and 4-5 significant figures)
- The pattern is uniform across both
route.rsandpool.rs
This change demonstrates a textbook example of maintaining behavioral consistency while reducing redundancy. As any theoretical physicist would appreciate, it's both elegant and precise!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the default rounding behavior matches the previous explicit RoundHalfUp
# Search for any remaining explicit rounding parameters in tests
echo "Checking for any inconsistent rounding parameter usage..."
rg "to_(fixed|significant)\([^,]+,\s*(?!None)[^,]+" src/
# Search for test assertions to ensure consistent precision
echo "Verifying consistent decimal precision in test assertions..."
rg "assert_eq!\(.*to_(fixed|significant)\([^)]+\)" src/
Length of output: 1544
This commit updates the uniswap-sdk-core-rust from version 3.1.0 to 3.2.0.
malik672/uniswap-sdk-core-rust#89
Summary by CodeRabbit
Release Notes
New Features
Poolstruct with improved price calculation flexibility and better error handling for token validation.Tradestruct with new methods for calculating worst execution price and price impact, including slippage checks.Routestruct with refined mid-price calculations.Bug Fixes
Documentation