refactor!: consolidate currency-related traits and add Currency enum
#85
Conversation
Reorganized `BaseCurrency` and `BaseCurrencyCore` traits, encapsulating their functionalities. Introduced `Currency` enum to represent both native and token currencies, providing a unified interface for currency operations. Updated version to 3.0.0 to reflect these breaking changes.
WalkthroughThe pull request includes significant updates to the Changes
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: 3
🧹 Outside diff range and nitpick comments (7)
src/entities/native_currency.rs (1)
Line range hint
1-15: Summary of changes and recommendationsThe changes to
src/entities/native_currency.rsappear to be part of a larger refactoring effort to improve the currency trait hierarchy. The main points to consider are:
- The
NativeCurrencytrait now extendsBaseCurrencyCoreinstead ofBaseCurrency, which may have significant implications across the codebase.- The existing methods
is_nativeandis_tokenseem compatible with the new structure, but this should be verified against theBaseCurrencyCoredefinition.- The implementation for
CurrencyLike<true, M>needs to be checked to ensure it satisfies the requirements ofBaseCurrencyCore.To ensure the consistency and correctness of these changes:
- Review all usages of
NativeCurrencyandBaseCurrencythroughout the codebase.- Update any documentation related to the currency trait hierarchy.
- Consider adding tests to verify that the behavior of
NativeCurrencyimplementations remains consistent with the expected functionality after this change.These steps will help maintain the integrity of the currency system while benefiting from the new, more refined trait structure.
src/entities/ether.rs (1)
Ether does not implement the BaseCurrency trait
- The
Ethertype does not implement theBaseCurrencytrait as expected after recent changes.- Ensure that
impl BaseCurrency for Etheris provided and all required methods are implemented.- Verify that existing test cases cover all aspects of the
BaseCurrencytrait implementation forEther.🔗 Analysis chain
Line range hint
1-6: Verify compatibility of unchanged code with new trait structureWhile the
Ethertype definition, struct methods, and test cases remain unchanged, it's crucial to ensure they are still valid and complete given the introduction of theBaseCurrencytrait. Please verify that:
- The
Ethertype definition still accurately represents the concept of Ether in the new trait hierarchy.- Existing struct methods fulfill all requirements of the new
BaseCurrencytrait.- Test cases cover all aspects of the
BaseCurrencytrait implementation.To assist with this verification, you can run the following script:
This script will help ensure that the
Ethertype fully implements theBaseCurrencytrait and that all methods are adequately tested.Also applies to: 29-78
Further verification needed for BaseCurrency trait implementation and test coverage
To ensure comprehensive verification, please run the following scripts:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the completeness of BaseCurrency trait implementation for Ether # Test 1: Check if all BaseCurrency methods are implemented for Ether echo "Checking BaseCurrency trait implementation for Ether:" ast-grep --lang rust --pattern $'impl BaseCurrency for Ether { $$$ }' # Test 2: Verify test coverage for BaseCurrency methods echo "Checking test coverage for BaseCurrency methods:" rg --type rust -A 5 $'#[test]' src/entities/ether.rsLength of output: 350
Script:
#!/bin/bash # Test 1: Check if BaseCurrency trait is implemented for Ether echo "Verifying BaseCurrency trait implementation for Ether:" ast-grep --lang rust --pattern $'impl BaseCurrency for Ether { $$$ }' # Test 2: List all test functions related to BaseCurrency in ether.rs echo "Listing test cases for BaseCurrency methods in Ether:" rg --type rust '#\[test\]' src/entities/ether.rsLength of output: 407
README.md (1)
Line range hint
1-138: Consider updating README to reflect recent changesWhile the version number has been correctly updated, it might be beneficial to review and potentially update other parts of the README to reflect the significant changes mentioned in the PR objectives. Specifically:
- Consider adding information about the new
Currencyenum and its usage.- Update the example code to demonstrate the use of consolidated currency-related traits, if applicable.
- Add a note about the breaking changes in version 3.0.0 to alert users who might be upgrading.
These additions would provide users with a more comprehensive understanding of the recent changes and how to use the new features.
🧰 Tools
🪛 Markdownlint
19-19: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
src/entities/token.rs (1)
Line range hint
1-324: Summary: Successful transition toBaseCurrencytraitThe changes in this file successfully implement the transition from the
Currencytrait to theBaseCurrencytrait. This refactoring aligns well with the PR objectives to consolidate currency-related traits. The modifications are consistent throughout the file and maintain the existing functionality while potentially allowing for more flexible currency comparisons.However, it's crucial to ensure that this change is reflected consistently across the entire codebase. The verification script provided earlier will help identify any remaining references to the
Currencytrait that might need updating.Consider adding a brief comment in the file explaining the rationale behind the
BaseCurrencytrait and its relationship to other currency-related traits in the system. This would help future developers understand the design decisions made in this refactoring.src/entities/currency.rs (1)
Line range hint
99-127: Update test cases to utilize the newCurrencyenum variantsThe current tests use
EtherandTokendirectly. To fully exercise the newCurrencyenum structure, consider updating the tests to useCurrency::NativeCurrency(Ether::on_chain(chain_id))andCurrency::Token(token_instance). This will ensure that the enum's behavior is thoroughly validated.For example, modify the test as follows:
-assert!(Ether::on_chain(1).equals(&Ether::on_chain(1))); +let currency_a = Currency::NativeCurrency(Ether::on_chain(1)); +let currency_b = Currency::NativeCurrency(Ether::on_chain(1)); +assert!(currency_a.equals(¤cy_b));src/entities/fractions/currency_amount.rs (1)
Line range hint
71-73: Use a more descriptive error variant into_fixedmethod.Currently, when
decimal_placesexceed the currency's decimals, the method returnsError::NotEqual, which might not clearly convey the issue. Consider defining a new error variant, such asError::InvalidDecimalPlaces, to provide clearer feedback to the user about the nature of the error.Apply this diff to introduce a new error variant and update the error handling:
// In your Error enum definition: +/// Error indicating that the provided decimal places exceed the currency's decimals. +InvalidDecimalPlaces, // In the `to_fixed` method: if decimal_places > self.currency.decimals() { - return Err(Error::NotEqual); + return Err(Error::InvalidDecimalPlaces); }src/entities/base_currency.rs (1)
Line range hint
32-39: Prefer returningOption<&str>instead ofOption<&String>In the
BaseCurrencyCoretrait, the methodssymbolandnamereturnOption<&String>. It's more idiomatic in Rust to returnOption<&str>to provide greater flexibility and avoid unnecessary indirection.Apply the following changes:
- fn symbol(&self) -> Option<&String>; + fn symbol(&self) -> Option<&str>; - fn name(&self) -> Option<&String>; + fn name(&self) -> Option<&str>;Ensure that the implementations of these methods return
Option<&str>accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
- Cargo.toml (1 hunks)
- README.md (1 hunks)
- src/entities/base_currency.rs (3 hunks)
- src/entities/currency.rs (2 hunks)
- src/entities/ether.rs (2 hunks)
- src/entities/fractions/currency_amount.rs (1 hunks)
- src/entities/fractions/price.rs (3 hunks)
- src/entities/native_currency.rs (1 hunks)
- src/entities/token.rs (2 hunks)
- src/prelude.rs (1 hunks)
- src/utils/compute_price_impact.rs (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- Cargo.toml
🧰 Additional context used
🔇 Additional comments (21)
src/entities/native_currency.rs (3)
Line range hint
6-13: Verify compatibility of existing methods withBaseCurrencyCoreThe
is_nativeandis_tokenmethods appear to be compatible with the newBaseCurrencyCoresupertrait, as they implement basic currency type checking functionality. However, it's important to ensure that these methods align with the expectations set byBaseCurrencyCore.Please run the following script to check the
BaseCurrencyCoretrait definition:#!/bin/bash # Description: Check the BaseCurrencyCore trait definition echo "Searching for BaseCurrencyCore trait definition:" rg --type rust "trait BaseCurrencyCore" -A 10After reviewing the
BaseCurrencyCoretrait definition, please confirm thatis_nativeandis_tokenare either required byBaseCurrencyCoreor are appropriate extensions forNativeCurrency.
Line range hint
15-15: VerifyCurrencyLike<true, M>implementsBaseCurrencyCoreThe implementation of
NativeCurrencyforCurrencyLike<true, M>remains syntactically valid. However, with the change in theNativeCurrencytrait's supertrait fromBaseCurrencytoBaseCurrencyCore, it's crucial to ensure thatCurrencyLike<true, M>satisfies all the requirements ofBaseCurrencyCore.Please run the following script to check the implementation of
BaseCurrencyCoreforCurrencyLike:#!/bin/bash # Description: Check BaseCurrencyCore implementation for CurrencyLike echo "Searching for BaseCurrencyCore implementation for CurrencyLike:" rg --type rust "impl.*BaseCurrencyCore.*for.*CurrencyLike" -C 5After reviewing the results, please confirm that
CurrencyLike<true, M>properly implementsBaseCurrencyCore. If it doesn't, you may need to add an explicit implementation or adjust the trait bounds onCurrencyLike.
4-4: 🛠️ Refactor suggestionVerify the impact of changing the supertrait to
BaseCurrencyCoreThe change from
BaseCurrencytoBaseCurrencyCoreas the supertrait forNativeCurrencyrepresents a significant alteration in the trait hierarchy. This modification might have far-reaching effects on the codebase, particularly in areas whereNativeCurrencyis used and expected to have all the methods fromBaseCurrency.Please run the following script to check for potential inconsistencies or breaking changes:
Consider updating the documentation for
NativeCurrencyto reflect this change in the trait hierarchy. Also, ensure that any code relying onNativeCurrencyhaving all methods fromBaseCurrencyis updated accordingly.src/prelude.rs (1)
6-7: 🛠️ Refactor suggestionConsider the implications of using wildcard imports.
The changes from specific imports to wildcard imports (
*) forbase_currencyandcurrencymodules have both advantages and potential drawbacks:Pros:
- Simplifies import statements, reducing code verbosity.
- Allows easy access to all public items in these modules without explicitly listing them.
Cons:
- May lead to name conflicts if multiple modules export items with the same name.
- Reduces code readability as it's less clear which specific items are being imported.
- Can potentially expose more of the API than intended, which might complicate future refactoring.
These changes align with the PR objectives of consolidating currency-related traits. However, consider the following:
- Ensure that this change doesn't introduce any name conflicts in the codebase.
- Verify that exposing all public items from these modules is intentional and doesn't break encapsulation principles.
- Consider adding comments to clarify which specific items are commonly used from these modules.
If you decide to keep the wildcard imports, consider adding comments like this for clarity:
// Imports BaseCurrency, BaseCurrencyCore, etc. pub use crate::entities::base_currency::*; // Imports Currency, CurrencyLike, etc. pub use crate::entities::currency::*;To ensure these changes don't introduce conflicts, run the following script:
This will help identify any potential issues arising from the wildcard imports.
src/entities/ether.rs (2)
7-12: Approve the refactoring of currency traitsThe changes to the macro definition and implementation are well-aligned with the PR objectives. The transition from
impl_currencytoimpl_base_currencyand the use ofBaseCurrencyinstead ofCurrencyindicate a more specialized and hierarchical approach to currency representations. This refactoring should lead to improved code organization and clarity.Key improvements:
- Renamed macro reflects its more specific purpose.
- Use of
BaseCurrencytrait aligns with the consolidation of currency-related traits.- Updated
equalsmethod signature ensures type consistency with the new trait hierarchy.These changes contribute to a more robust and maintainable codebase.
28-28: Approve the updated macro usageThe change in macro usage from
impl_currency!toimpl_base_currency!is consistent with the earlier modification to the macro definition. This update ensures that the newimpl_base_currencymacro is correctly utilized for bothEtherand&Ethertypes.This change maintains consistency throughout the file and aligns with the overall refactoring strategy.
src/utils/compute_price_impact.rs (2)
13-13: Approve the trait bound change, but verify its impact.The change from
CurrencytoBaseCurrencyfor the generic type constraints is likely part of a larger refactoring effort to improve the type system. This change appears to be in line with the PR objectives of consolidating currency-related traits.To ensure this change doesn't introduce unintended consequences, please run the following script to check for any other occurrences of the
Currencytrait that might need updating:✅ Verification successful
Trait Bound Change Verified Successfully
The update from
CurrencytoBaseCurrencyin the generic type constraints does not affect other parts of the codebase. No additional usages of theCurrencytrait were found, ensuring the change is safe and isolated.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other occurrences of the Currency trait # that might need updating after this change. # Search for Currency trait usage echo "Searching for Currency trait usage:" rg --type rust "trait.*Currency" -g '!target/' # Search for Currency trait bounds echo "Searching for Currency trait bounds:" rg --type rust "<.*: Currency" -g '!target/'Length of output: 476
Line range hint
14-38: Verify BaseCurrency trait implementation and test validity.The function body remains unchanged, which is good. However, we should ensure that:
- All method calls on
TBaseandTQuotein the function body are still valid with theBaseCurrencytrait.- The concrete types used in the tests (
Ether,token,token_1) implement theBaseCurrencytrait.Please run the following script to verify the
BaseCurrencytrait implementation:If any of these checks fail, you may need to update the trait implementations or the test cases accordingly.
README.md (1)
21-21: Version update reflects significant changesThe update from version 2.4.0 to 3.0.0 correctly reflects the major changes described in the PR objectives, including the consolidation of currency-related traits and the addition of the
Currencyenum. This change follows semantic versioning principles, indicating breaking changes to users.Let's verify if this version update is consistent across the project:
src/entities/fractions/price.rs (4)
26-27: LGTM: Consistent update of type constraints in Price implementation.The change from
CurrencytoBaseCurrencyforTBaseandTQuotein thePriceimplementation is consistent with the earlier update in thePriceMetastruct. This ensures that the entirePricestructure uses the more specificBaseCurrencytrait.To ensure this change doesn't affect any methods within the
Priceimplementation, please run the following check:#!/bin/bash # Description: Check for any methods in Price implementation that might be affected by the trait change echo "Checking for methods in Price implementation that use Currency trait:" rg --type rust -A 10 "impl<TBase, TQuote>.*Price<TBase, TQuote>" src/entities/fractions/price.rs | rg "fn.*Currency"
82-82: LGTM: Consistent update of type constraint in multiply method.The change from
CurrencytoBaseCurrencyforTOtherQuotein themultiplymethod signature is consistent with the earlier updates. This ensures that the method uses the more specificBaseCurrencytrait for its generic parameter.To ensure this change doesn't break any existing usage of the
multiplymethod, please run the following check:#!/bin/bash # Description: Check for any usage of the multiply method that might be affected by the trait change echo "Checking for usage of multiply method:" rg --type rust "\.multiply\(<.*>" -A 5
Line range hint
1-238: Summary: Consistent and appropriate updates to currency-related type constraints.The changes in this file consistently update the type constraints from
CurrencytoBaseCurrencyfor thePriceMetastruct,Priceimplementation, andmultiplymethod. These updates align well with the PR objectives of consolidating currency-related traits and likely provide more appropriate constraints for the price-related structures and methods.To ensure the overall consistency and correctness of these changes, please run the following final verification:
#!/bin/bash # Description: Verify the consistency of BaseCurrency usage and check for any remaining Currency usages echo "Checking for any remaining Currency trait usage in price.rs:" rg --type rust "Currency[^A-Za-z]" src/entities/fractions/price.rs echo "Verifying consistent usage of BaseCurrency in price.rs:" rg --type rust "BaseCurrency" src/entities/fractions/price.rs echo "Checking for any Currency to BaseCurrency changes in other files:" rg --type rust -g "!price.rs" "BaseCurrency"This will help ensure that the changes are consistent throughout the file and identify any potential areas that might need attention in other parts of the codebase.
11-12: LGTM: Type constraint update aligns with PR objectives.The change from
CurrencytoBaseCurrencyforTBaseandTQuotealigns with the PR's goal of consolidating currency-related traits. This update likely provides a more specific and appropriate constraint for thePriceMetastruct.To ensure this change doesn't unintentionally restrict the usage of
PriceMeta, please verify the trait hierarchy:src/entities/token.rs (3)
17-20: LGTM: Macro updated to useBaseCurrencyThe change from
impl_currencytoimpl_base_currencyand the use ofBaseCurrencyinstead ofCurrencyis consistent with the PR objectives to consolidate currency-related traits. This modification enhances the clarity of the trait hierarchy and maintains consistency throughout the implementation.
22-24: LGTM:equalsmethod updated for broader compatibilityThe
equalsmethod signature has been updated to accept&impl BaseCurrency, which is consistent with the earlier changes and the PR objectives. This modification potentially allows for greater flexibility in comparing different currency types that implement theBaseCurrencytrait.
35-35: LGTM: Trait implementation updated toBaseCurrencyThe trait implementation for
Tokenhas been correctly updated to useBaseCurrencyinstead ofCurrency. This change is consistent with the earlier modifications and aligns with the PR objectives.To ensure this change doesn't have unintended consequences, let's verify its impact on the wider codebase:
src/entities/currency.rs (2)
5-7: Implementation ofCurrencyenum enhances code clarityThe introduction of the
Currencyenum with variantsNativeCurrencyandTokenconsolidates the currency representation, simplifying the handling of different currency types within the codebase.
83-88: Verify correctness ofequalsmethod when comparing different currency typesIn the
equalsimplementation,Currency::NativeCurrency(ether)callsether.equals(other). Ensure that theequalsmethod inEthercorrectly handles cases whereothermay be aTokenand returnsfalseas expected to prevent incorrect equality evaluations.You can run the following script to check the
equalsmethod implementations:src/entities/fractions/currency_amount.rs (2)
Line range hint
104-108: Ensure test tokens implementBaseCurrencytrait.With the updated trait bound to
BaseCurrency, verify that the test tokensTOKEN18andTOKEN0implement theBaseCurrencytrait. This ensures that all unit tests remain valid and pass without issues.
9-16: Verify thatBaseCurrencytrait provides required methods used inCurrencyAmount.The change from
CurrencytoBaseCurrencyin the trait bounds affects howCurrencyAmountinteracts with thecurrencyfield. Ensure that all methods called oncurrency, such asdecimals(),equals(), andwrapped(), are available in theBaseCurrencytrait. This is crucial for maintaining the functionality of methods likenew,to_fixed,add,subtract, andwrapped.You can run the following script to confirm that the
BaseCurrencytrait includes the necessary method definitions:✅ Verification successful
Verification Successful:
BaseCurrencytrait includesdecimalsandequalsmethods.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check that `BaseCurrency` defines the required methods. # Expected output: Definitions of `decimals`, `equals`, and `wrapped` methods within `BaseCurrency`. rg --type rust --context 5 'trait BaseCurrency' src/entitiesLength of output: 1490
Script:
#!/bin/bash # Description: Verify that `BaseCurrency` defines `decimals` and `equals` methods. # Search for the `decimals` method within the `BaseCurrency` trait rg --type rust 'trait BaseCurrency' src/entities -A 10 | rg 'fn decimals\(&self\) ->' # Search for the `equals` method within the `BaseCurrency` trait rg --type rust 'trait BaseCurrency' src/entities -A 10 | rg 'fn equals\(&self, _\) ->'Length of output: 176
Script:
#!/bin/bash # Description: List all method definitions in the `BaseCurrency` trait and its supertraits. # List methods in BaseCurrency trait echo "Methods in BaseCurrency trait:" rg --type rust 'trait BaseCurrency' src/entities -A 20 | rg 'fn ' # List methods in BaseCurrencyCore trait echo -e "\nMethods in BaseCurrencyCore trait:" rg --type rust 'trait BaseCurrencyCore' src/entities -A 20 | rg 'fn 'Length of output: 1290
src/entities/base_currency.rs (1)
Line range hint
43-75: Review generic parameters in macro implementationIn the
impl_base_currency_core!macro, the implementation block specifies generic parametersconst IS_NATIVE: bool, M. If$currencyalready includes these generic parameters, declaring them again may be redundant or lead to conflicts.Please verify if the generic parameters
const IS_NATIVE: bool, Mare necessary in theimplblock or if they can be inferred from$currency. Consider simplifying the macro as follows:- impl<const IS_NATIVE: bool, M> BaseCurrencyCore for $currency { + impl BaseCurrencyCore for $currency {
Reorganized
BaseCurrencyandBaseCurrencyCoretraits, encapsulating their functionalities. IntroducedCurrencyenum to represent both native and token currencies, providing a unified interface for currency operations. Updated version to 3.0.0 to reflect these breaking changes.Summary by CodeRabbit
New Features
uniswap-sdk-corefrom 2.4.0 to 3.0.0.BaseCurrencyCoreandBaseCurrencytraits.Currencyenum to simplify currency handling.Bug Fixes
Documentation
README.mdto reflect the new version ofuniswap-sdk-core.Chores
src/prelude.rsfor easier access to public items.