-
Notifications
You must be signed in to change notification settings - Fork 21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: sync with TypeScript SDK #86
Conversation
Upgraded `alloy`, `once_cell`, `regex`, and `uniswap-lens` to their latest versions. Updated the package version from 1.1.0 to 1.2.0 to reflect these dependency changes.
Introduced additional fee tiers: `LOW_200`, `LOW_300`, and `LOW_400` in the `FeeAmount` enum. Adjusted tick spacing and conversion methods to support the new fee tiers. This enhancement allows for more granular fee configurations within the application.
Added logic to compute pool addresses differently for ZKSync using `compute_zksync_create2_address` function. Updated function signatures and dependencies to accommodate optional `ChainId` parameter.
Refactored swap logic from `Pool` into a new function `v3_swap` in `swap_math.rs`. This improves modularity and makes the code more maintainable. All relevant references and usages have been updated accordingly.
WalkthroughThe pull request introduces several 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 8
🧹 Outside diff range comments (3)
src/utils/compute_pool_address.rs (1)
Line range hint
38-49
: Update documentation examples to reflect practicalchain_id
usage"I see you've graciously updated the examples to include the new
chain_id
parameter, passingNone
for simplicity. However, wouldn't it be more enlightening to demonstrate the function's behavior with an actualchain_id
, such asSome(ChainId::MAINNET)
? This would provide clarity and value to those perusing the documentation, much like a well-explained physics experiment."src/extensions/pool.rs (2)
Line range hint
72-90
: Ensure robust error handling infrom_pool_key_with_tick_data_provider
The addition of
from_pool_key_with_tick_data_provider
is quite intriguing. However, have you considered the potential for asynchronous call failures? Implementing comprehensive error handling will enhance the resilience of this method. After all, even the most brilliant of us can appreciate the value of anticipating flaws.Would you like assistance in enhancing the error handling logic within this method?
Line range hint
135-160
: Improve documentation and testing for complex liquidity reconstruction logicThe
reconstruct_liquidity_array
function contains complex logic that may challenge future developers (though perhaps not me). To facilitate understanding and maintainability, consider adding detailed comments explaining each step. Additionally, implementing unit tests will ensure its reliability.I'm available to help draft comprehensive documentation or create unit tests for this function if you'd find that beneficial.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (9)
- Cargo.toml (3 hunks)
- README.md (1 hunks)
- benches/swap_math.rs (3 hunks)
- src/constants.rs (5 hunks)
- src/entities/pool.rs (3 hunks)
- src/extensions/pool.rs (1 hunks)
- src/utils/compute_pool_address.rs (4 hunks)
- src/utils/mod.rs (1 hunks)
- src/utils/swap_math.rs (4 hunks)
🧰 Additional context used
🔇 Additional comments (24)
Cargo.toml (6)
3-3
: Fascinating! A logical progression in versioning.I must say, the increment from version 1.1.0 to 1.2.0 demonstrates a clear understanding of semantic versioning principles. It's like moving from the first episode of season 1 to the second episode – a natural and expected progression. Bazinga!
26-26
: Once_cell update: A minuscule yet noteworthy advancement!I find this update from once_cell 1.19 to 1.20 as exciting as discovering a new prime number! It's a patch update, which, in the grand scheme of version control, is akin to adjusting the position of a single atom in the universe. Negligible to some, but crucial to those who understand its significance. Kudos on maintaining such precision!
27-27
: Regex update: A potential paradigm shift in pattern matching!The update of regex from version 1.10 to 1.11 is not to be taken lightly. It's like upgrading from the Original Series to The Next Generation – exciting, but fraught with potential discontinuities. We must approach this with the same caution as when entering a room full of people during flu season.
I suggest we run the following script to ensure our regex usage remains unperturbed:
#!/bin/bash # Description: Verify regex usage compatibility with version 1.11 # Test: Search for regex usage in the project rg --type rust "use.*regex" -A 5 # Test: Look for any regex patterns that might be affected rg --type rust "Regex::new\(.*\)" -A 2Remember, in the realm of regular expressions, one misplaced character can lead to chaos. And we certainly don't want that, unless it's Anything Can Happen Thursday!
32-32
: Uniswap-sdk-core update: A paradigm shift in the core of our digital universe!The update of uniswap-sdk-core from version 2.3.0 to 2.4.0 is as monumental as the transition from the geocentric to the heliocentric model of the solar system. While it opens up new possibilities, we must approach it with the same caution as I do when considering whether to try a new flavor of oatmeal.
I suggest we run the following script to ensure our usage of uniswap-sdk-core remains as stable as my weekly routine:
#!/bin/bash # Description: Investigate uniswap-sdk-core usage and potential breaking changes # Test: Search for uniswap-sdk-core usage in the project rg --type rust "use.*uniswap_sdk_core" -A 5 # Test: Look for any mentions of breaking changes in the project related to uniswap-sdk-core rg -i "breaking changes" --type rust | rg -i "uniswap.sdk.core"Remember, in the realm of software dependencies, we must be as vigilant as I am about maintaining the optimal room temperature of 71 degrees Fahrenheit. Any deviation could lead to chaos, and we certainly don't want that in our codebase!
31-31
: Uniswap-lens update: A quantum leap in decentralized finance!The progression of uniswap-lens from version 0.3 to 0.4 is as significant as the jump from Newtonian physics to quantum mechanics. While exciting, we must approach this update with the same level of scrutiny I apply to proofreading the dinner order at the Cheesecake Factory.
I propose we execute the following script to ensure our codebase remains in harmony with this update:
Remember, in the world of decentralized finance, even the slightest perturbation can cause ripples across the entire ecosystem. We must be as careful as I am when organizing my cereal boxes by fiber content!
17-17
: Alloy update: A potential disturbance in the force!While I appreciate the enthusiasm for keeping dependencies up-to-date, much like my own zeal for the latest Particle Physics journals, I must insist we approach this with caution. The update of alloy from version 0.3 to 0.4 could introduce unforeseen complications, not unlike the time Penny rearranged the furniture in my apartment.
I propose we run the following script to investigate potential breaking changes:
Remember, in the world of dependencies, we must be as vigilant as I am about Penny touching my food!
src/utils/mod.rs (1)
30-30
:⚠️ Potential issueBazinga! We've unleashed the full power of swap_math!
While I appreciate the efficiency of reducing multiple import statements to a single line, much like how the unified theory of everything would simplify our understanding of the universe, I must point out that this change has far-reaching implications.
By exporting all public entities from the
swap_math
module, we've essentially opened Pandora's Box. This could lead to a chaotic universe where previously encapsulated implementation details are now accessible willy-nilly. It's like allowing Penny to rearrange my sock drawer – utter madness!To ensure we haven't inadvertently exposed any cosmic secrets, I propose we run the following analysis:
This script will help us catalog the newly exposed entities and check if they're being used in ways that could disrupt the space-time continuum... or at least our code structure.
✅ Verification successful
Bazinga! The exportation of
swap_math
is now as precise as my spot on the couch.Upon thorough analysis, only the intended public entities—
SwapState
,compute_swap_step
, andv3_swap
—are exposed throughpub use swap_math::*;
. This ensures that encapsulation remains intact without unleashing any unnecessary implementation details into the broader codebase. The minimal usage observed further confirms that this change maintains code integrity without introducing chaos.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Analyze the impact of exporting all entities from swap_math # Test 1: List all public entities in swap_math echo "Public entities in swap_math:" ast-grep --lang rust --pattern 'pub $_' src/utils/swap_math.rs # Test 2: Check for any usage of newly exposed entities echo "Usage of swap_math entities in other files:" rg --type rust -g '!src/utils/swap_math.rs' -g '!src/utils/mod.rs' 'swap_math::'Length of output: 731
Script:
#!/bin/bash # Description: Refined analysis of public entities in swap_math echo "Listing public functions in swap_math:" ast-grep --lang rust --pattern 'pub fn $_($_): $_ { $$$ }' src/utils/swap_math.rs echo "Listing public structs in swap_math:" ast-grep --lang rust --pattern 'pub struct $_ { $$$ }' src/utils/swap_math.rs echo "Listing public enums in swap_math:" ast-grep --lang rust --pattern 'pub enum $_ { $$$ }' src/utils/swap_math.rs echo "Checking for any remaining public items in swap_math:" ast-grep --lang rust --pattern 'pub $_' src/utils/swap_math.rsLength of output: 1017
Script:
#!/bin/bash # Description: Identify all 'pub use' statements in swap_math and list re-exported entities echo "Listing all 'pub use' statements in swap_math:" rg '^pub use' src/utils/swap_math.rs echo "Listing all public items re-exported by swap_math:" ast-grep --lang rust --pattern 'pub use $_::*;' src/utils/swap_math.rsLength of output: 331
Script:
#!/bin/bash # Description: List all public declarations in swap_math.rs using ripgrep echo "Listing all public declarations in swap_math.rs:" rg 'pub\s+(fn|struct|enum|const|trait|type)\s+' src/utils/swap_math.rsLength of output: 314
benches/swap_math.rs (4)
3-3
: Fascinating addition to the import statement!The inclusion of
U24
in thealloy_primitives
import is a logical step, given its subsequent utilization in the code. It's like adding a new element to our periodic table of types!
26-26
: A quantum leap in type conversion!The transition from
i as u32
toU24::from(i)
is not merely a change; it's an evolution in our type system. This modification ensures that our code adheres to the strictest standards of type safety, much like how the Heisenberg Uncertainty Principle ensures we can't simultaneously know a particle's position and momentum with absolute precision.
18-18
: Intriguing modification to the function signature!The alteration from
u32
toU24
in the return type is a logical progression, akin to refining our understanding of subatomic particles. However, we must ensure that this change doesn't create a temporal paradox in the rest of the codebase.I propose we run the following experiment to verify the repercussions of this change:
#!/bin/bash # Description: Verify the usage of `generate_inputs` function in the codebase. # Test: Search for function calls to `generate_inputs`. Expect: All calls should handle the new U24 type. rg --type rust 'generate_inputs\(\)'
84-84
:⚠️ Potential issueGreat Scott! A curious transformation of
fee_pips
!This modification is akin to attempting to fit a square peg in a round hole, or more accurately, trying to squeeze a 24-bit value into a 32-bit container. While it may work, it raises several questions:
- Why are we discarding the potential upper bits of the
U24
value?- Could this lead to a loss of precision in fee calculations?
- Is this change necessary for compatibility with the
swap_math::compute_swap_step
function?I propose we conduct a thorough investigation into the ramifications of this alteration. Perhaps we should consider updating the
swap_math::compute_swap_step
function to accept aU24
parameter instead?Let's run the following experiment to gather more data:
README.md (1)
52-52
: Fascinating! A minute alteration in the cosmic fabric of our codebase.I must say, updating the version number from 1.1.0 to 1.2.0 is quite an intriguing development in the grand scheme of our Uniswap V3 SDK Rust project. It's like watching the evolution of a species, but instead of millions of years, we're dealing with a few digits.
However, as any respectable scientist would do, we must verify this change against our Cargo.toml file. After all, inconsistency is the arch-nemesis of organized minds like mine. Let's run a quick check:
✅ Verification successful
Excellent Observations! The version numbers are in perfect harmony.
After a thorough analysis with the precision of a Nobel laureate, it's evident that the version specified in
Cargo.toml
(1.2.0
) impeccably matches the update inREADME.md
. Consistency reigns supreme in our codebase!🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if the version in Cargo.toml matches the README update. # Test: Extract version from Cargo.toml. Expect: Should be 1.2.0 cargo_version=$(grep -m 1 '^version =' Cargo.toml | awk -F '"' '{print $2}') echo "Version in Cargo.toml: $cargo_version" # Bazinga! Let's ensure they match if [ "$cargo_version" = "1.2.0" ]; then echo "Versions match. All is right in the universe!" else echo "Great Scott! We have a discrepancy!" fiLength of output: 277
src/constants.rs (5)
18-20
: An astute expansion of theFeeAmount
enum.Adding
LOW_200
,LOW_300
, andLOW_400
variants is a logical progression to enhance fee granularity. This should provide more flexibility within the system.
33-35
: Consistent tick spacing is paramount.I see you've correctly associated the new fee amounts with their respective tick spacings using
I24::from_limbs
. This maintains the integrity of the fee-to-tick mapping.
48-50
: Properly extendingFrom<u32>
for new fees.Including the new fee amounts in the
From<u32>
implementation ensures seamless conversion. Your attention to detail is commendable.
64-66
: UpdatingFrom<i32>
for tick spacings is essential.By adding the new tick spacing values to the
From<i32>
implementation, you've maintained consistency across the conversions. Excellent work.
18-20
: Verification of comprehensive integration is advisable.While your additions are logically sound, it's imperative to ensure that these new fee amounts and tick spacings are properly integrated throughout the codebase. Overlooking any usage could result in inconsistencies or unexpected behavior.
Please run the following script to confirm that all instances are correctly updated:
Also applies to: 33-35, 48-50, 64-66
✅ Verification successful
Comprehensive Integration Verified Successfully
Upon meticulous examination, the newly introduced fee constants—LOW_200, LOW_300, and LOW_400—have been diligently integrated across the codebase. Their consistent usage in both
src/constants.rs
andsrc/utils/nearest_usable_tick.rs
ensures seamless functionality and mitigates potential inconsistencies or unexpected behaviors.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for all usages of the new fee amounts and tick spacings. # Test: Find all occurrences of LOW_200, LOW_300, and LOW_400 in Rust files. # Expect: Confirm that these are properly utilized throughout the codebase. rg 'LOW_(200|300|400)' --type rust # Test: Check for tick spacing values 4, 6, and 8 associated with fee amounts. # Expect: Ensure consistency in tick spacing usage. rg 'I24::from_limbs\(\[([468])\]\)' --type rust -A 5Length of output: 1971
src/extensions/pool.rs (1)
Line range hint
48-70
: Verify the default behavior of the optionalblock_id
parameterIntroducing an optional
block_id
parameter that defaults toLatest
is a sensible enhancement. However, it's essential to ensure that all usages offrom_pool_key
correctly handle this parameter, especially when historical block data is involved. We wouldn't want any temporal discrepancies sneaking into our calculations.Consider running the following script to check the usage across the codebase:
This will help confirm that all invocations are appropriately updated and that the default behavior aligns with expectations.
src/utils/swap_math.rs (3)
5-11
: Excellent Implementation ofSwapState
StructIt's delightful to see the encapsulation of swap state into a dedicated struct. This not only enhances code clarity but also aligns with the principles of modular design. Bazinga!
14-22
: Prudent Addition ofStepComputations
StructIntroducing
StepComputations
for intermediate calculations is a logical progression. It compartmentalizes the complexities of the swap computation steps. A wise choice, if I may say so myself.
53-56
: Astute Change offee_pips
Type toU24
Adjusting the
fee_pips
parameter fromu32
toU24
reflects meticulous attention to detail. Since fees are constrained within 24 bits, this change enhances type safety and prevents potential overflows. Fascinating.src/entities/pool.rs (3)
2-2
: Astute Inclusion of 'ChainId' ImportI observe that you've added
ChainId
to your imports fromalloy_primitives
. This addition is logically consistent with the necessity to reference the chain ID within your methods. Excellent attention to detail.
129-129
: Enhanced 'get_address' Method with Chain ContextBy incorporating
Some(token_a.chain_id())
into yourcompute_pool_address
function call, you've ensured that the pool address computation now accurately reflects the token's chain ID. This modification enhances cross-chain compatibility. An exemplary enhancement.
260-270
: Prudent Refactoring to Utilize 'v3_swap' FunctionI notice that you've refactored the
_swap
method to delegate its internal logic to thev3_swap
function. This promotes modularity and adheres to the DRY principle, thereby improving maintainability and readability. A commendable decision in software engineering.To ensure that
v3_swap
is correctly implemented and accessible, you might consider verifying its definition:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores