refactor!: simplify token amount calculations #128
Conversation
Refactor `get_output_amount` and `get_input_amount` to remove tuple outputs and streamline logic. Add mutable variants for methods to handle pool state updates effectively. Update dependency versions, bump SDK to 3.4.0, and enhance README with revised usage and benchmark instructions.
WalkthroughAdjusts glasses Greetings! This pull request represents a systematic upgrade to the Uniswap V3 SDK, incrementing the package version from 3.3.0 to 3.4.0. The modifications primarily involve refactoring pool and trade methods to simplify state management, updating dependency versions, and enhancing documentation. Fascinating, isn't it? Changes
Sequence DiagramsequenceDiagram
participant Pool
participant Trade
Pool->>Pool: Update internal state
Trade->>Pool: Request output/input amount
Pool-->>Trade: Return calculated amount
Possibly related PRs
Suggested reviewers
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: 1
🔭 Outside diff range comments (4)
src/entities/pool.rs (3)
Line range hint
300-347: Don't forget the// TODO: update tick data provider.
Great use of mutable state to reflect real swaps. However, the leftover comment begs for completion.Would you like me to propose a partial solution or open an issue for that?
Line range hint
395-445: A dash of DRY might help.
get_input_amount_mutrepeats logic akin toget_output_amount_mut. Consider small shared utility functions to reduce duplication.- // Entire function logic is duplicated + // Extract shared logic into a helper, called from both get_output_amount_mut & get_input_amount_mut
Line range hint
697-707: Most gratifying test coverage.
Ensures direct correctness for USDC→DAI. An optional check for boundary liquidity conditions could be beneficial.README.md (1)
Line range hint
89-90: The code example requires updating to match the new API!The example still shows the old tuple destructuring pattern which is now as obsolete as Internet Explorer.
Apply this diff to update the example:
- let (amount_out, _pool_after) = pool.get_output_amount(&amount_in, None).unwrap(); + let amount_out = pool.get_output_amount(&amount_in, None).unwrap();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
Cargo.toml(3 hunks)README.md(4 hunks)examples/from_pool_key_with_tick_data_provider.rs(1 hunks)src/entities/pool.rs(8 hunks)src/entities/trade.rs(7 hunks)
🔇 Additional comments (16)
src/entities/pool.rs (5)
257-298: Splendid docstring and streamlined return.
You’ve cleverly removed the redundant updated pool from the returned tuple. This approach is consistent with the new design, minimizing confusion. No immediate concerns.
Line range hint
350-393: Input-amount logic looks symmetrical.
This is a nifty mirror ofget_output_amount. Nothing here contradicts my lofty standards.
Line range hint
709-719: DAI→USDC test direction is apt.
You have validated the complementary scenario. Everything is in order.
Line range hint
721-731: Cross-checking partial liquidity?
While this covers the primary path, you might consider a partial liquidity scenario. Just a suggestion.
Line range hint
733-743: Admirable completeness in test coverage.
Nothing contradictory to see here.src/entities/trade.rs (6)
Line range hint
556-566: Exact input flow is well-defined.
Switchingtoken_amountviapool.get_output_amountis elegantly straightforward. No significant nits.
585-585: Exact output flow is symmetrical.
Employingpool.get_input_amountfor the reverse direction is consistent.
676-676: Skipping insufficient liquidity is logical.
This guard ensures best-route logic remains robust. Well done.
714-714: Neat usage ofwrapped().
This maintains type consistency without rummaging through references.
769-769: Concise approach forOk(amount_in) => ....
Yes, that’s exactly how I'd handle it.
807-807: Wrapping user input stands out.
A fine way to unify references and owned values.Cargo.toml (2)
23-23: Updatedbigdecimalis good.
Keeping dependencies up-to-date helps ensure security and feature completeness.
33-33: Synchronized withuniswap-sdk-core3.4.0.
Harmonizing versions across dependencies is wise. No concerns.README.md (3)
4-5: Bazinga! The badge modifications are spot on!The addition of CodeRabbit badge and the simplified docs.rs badge enhance the repository's professional appearance.
53-53: The version increment follows proper semantic versioning!The bump to 3.4.0 appropriately reflects the breaking changes in the API, as mentioned in the PR title's
refactor!prefix.
128-134: The benchmarking documentation exhibits improved clarity!The addition of specific benchmark execution instructions will assist users in running targeted performance tests. As someone who appreciates precise documentation, I find this modification particularly satisfying.
Refactor
get_output_amountandget_input_amountto remove tuple outputs and streamline logic. Add mutable variants for methods to handle pool state updates effectively. Update dependency versions, bump SDK to 3.4.0, and enhance README with revised usage and benchmark instructions.Summary by CodeRabbit
New Features
Dependencies
bigdecimaldependency to version 0.4.7uniswap-sdk-coreto version 3.4.0Documentation
docs.rslinkRefactor
PoolandTradestruct methods to improve state management and type handling