refactor: add derive_more and adjust imports
#86
Conversation
Integrated `derive_more` for `Deref` attribute across various structs and adjusted the core module imports accordingly. This helps in simplifying the code and reducing redundancy.
WalkthroughThe pull request introduces 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: 1
🧹 Outside diff range and nitpick comments (11)
Cargo.toml (1)
12-12: LGTM! Consider specifying a minimum version instead.The addition of the
derive_moredependency with the "deref" feature aligns well with the PR objectives. This will indeed simplify the implementation of theDereftrait across multiple structs in the codebase.Consider using a minimum version specification instead of an exact version:
-derive_more = { version = "1.0.0", features = ["deref"] } +derive_more = { version = "^1.0.0", features = ["deref"] }This allows for compatible updates within the 1.x.x range, which can include important bug fixes and performance improvements.
src/prelude.rs (1)
Line range hint
1-34: Consider addingDerefimport if widely used.Given that the PR objectives mention integrating the
derive_morecrate and itsDereffunctionality, it might be beneficial to importDerefin this prelude file if it's going to be widely used across the project.If
Derefis now commonly used due to thederive_moreintegration, consider adding it to thecoreimport:pub use core::{cmp::Ordering, num::NonZeroU64, ops::Deref, str::FromStr};This would make
Derefeasily accessible throughout the project without needing additional imports in individual files.src/entities/fractions/currency_amount.rs (9)
Line range hint
13-22: Check for zero denominator innewmethodWhile you've added a check for the numerator exceeding
MAX_UINT256, it's important to ensure that thedenominatoris not zero to prevent a division by zero error.Consider adding a check for a zero denominator:
fn new( currency: T, numerator: impl Into<BigInt>, denominator: impl Into<BigInt>, ) -> Result<Self, Error> { let numerator = numerator.into(); let denominator = denominator.into(); + if denominator.is_zero() { + return Err(Error::DivisionByZero); + } // Ensure the amount does not exceed MAX_UINT256 if numerator.div_floor(&denominator) > *MAX_UINT256 { return Err(Error::MaxUint); } let exponent = currency.decimals(); Ok(FractionBase::new( numerator, denominator, CurrencyMeta { currency, decimal_scale: BigUint::from(10_u64).pow(exponent as u32), }, )) }
Line range hint
30-36: Ensure denominator is not zero infrom_fractional_amountSimilar to the
newmethod, thefrom_fractional_amountfunction should check that thedenominatoris not zero to avoid potential division by zero errors.Apply the same zero-check when calling
Self::new:pub fn from_fractional_amount( currency: T, numerator: impl Into<BigInt>, denominator: impl Into<BigInt>, ) -> Result<Self, Error> { + if denominator.into().is_zero() { + return Err(Error::DivisionByZero); + } Self::new(currency, numerator, denominator) }
Line range hint
38-44: Update documentation formultiplymethodThe
multiplymethod now returns aResult<Self, Error>. It's important to update the method's documentation to reflect the new return type and any possible errors.Add an error section to the doc comment:
/// Multiplication of currency amount by another fractional amount + /// + /// # Errors + /// + /// Returns an error if the resulting amount exceeds `MAX_UINT256`.
Line range hint
46-52: Handle division by zero individemethodWhile the
dividemethod returns aResult, it doesn't currently check if theotherfraction's numerator is zero, which would cause a division by zero error.Add a check for division by zero:
pub fn divide<M: Clone>(&self, other: &impl FractionBase<M>) -> Result<Self, Error> { + if other.numerator().is_zero() { + return Err(Error::DivisionByZero); + } let divided = self.as_fraction() / other.as_fraction(); Self::from_fractional_amount( self.currency.clone(), divided.numerator, divided.denominator, ) }
Line range hint
70-76: Improve error variant for currency mismatch inaddmethodReturning
Error::NotEqualwhen currencies do not match may not clearly convey the issue. Consider using a more descriptive error variant likeError::CurrencyMismatch.Update the error variant:
if !self.currency.equals(&other.currency) { - return Err(Error::NotEqual); + return Err(Error::CurrencyMismatch); }Ensure that the
Errorenum includes theCurrencyMismatchvariant and update any related documentation.
Line range hint
78-84: Consistent error messaging insubtractmethodSimilar to the
addmethod, update the error variant toError::CurrencyMismatchfor clarity when currencies do not match.Apply the same change:
if !self.currency.equals(&other.currency) { - return Err(Error::NotEqual); + return Err(Error::CurrencyMismatch); }
Line range hint
94-101: Document possible errors into_significantmethodThe
to_significantmethod now returnsResult<String, Error>. It's important to update the method's documentation to include information about potential errors.Enhance the documentation:
/// Convert the currency amount to a string with a specified number of significant digits + /// + /// # Errors + /// + /// Returns an error if the conversion fails due to invalid input or computation errors.
Line range hint
103-115: Use appropriate error variant into_fixedmethodWhen
decimal_placesexceeds the currency's decimals, the method returnsError::NotEqual, which may not accurately describe the error. Consider introducing anError::InvalidDecimalPlacesvariant.Update the error handling:
if decimal_places > self.currency.decimals() { - return Err(Error::NotEqual); + return Err(Error::InvalidDecimalPlaces); }Don't forget to add the
InvalidDecimalPlacesvariant to theErrorenum and update any associated documentation.
Line range hint
137-168: Update unit tests to accommodate new error handlingThe changes to return
Resulttypes affect how errors are captured in unit tests. Tests expecting panics should now handleResult::Err.Modify the tests accordingly. For example:
#[test] - #[should_panic(expected = "AMOUNT")] fn test_token_amount_exceeds_max_uint256() { - let _w = CurrencyAmount::from_raw_amount(TOKEN18.clone(), MAX_UINT256.clone() + 1); - assert!(_w.is_ok(), "AMOUNT"); + fn test_token_amount_exceeds_max_uint256() { + let result = CurrencyAmount::from_raw_amount(TOKEN18.clone(), MAX_UINT256.clone() + 1); + assert!(matches!(result, Err(Error::MaxUint)), "Expected MaxUint error"); }Apply similar changes to other tests that are affected by the new error handling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- Cargo.toml (1 hunks)
- src/entities/currency.rs (3 hunks)
- src/entities/fractions/currency_amount.rs (1 hunks)
- src/entities/fractions/fraction.rs (1 hunks)
- src/entities/fractions/price.rs (0 hunks)
- src/prelude.rs (1 hunks)
💤 Files with no reviewable changes (1)
- src/entities/fractions/price.rs
🧰 Additional context used
🔇 Additional comments (11)
Cargo.toml (1)
12-12: Verify the usage ofderive_moreand implementation ofDerefTo ensure that this dependency is being utilized effectively:
- Confirm that
derive_moreis imported and used in the relevant Rust files.- Verify that the
Dereftrait is correctly implemented for the intended structs (e.g.,CurrencyLike,CurrencyAmount,FractionLike).- Check that the core module imports have been adjusted as mentioned in the PR objectives.
Run the following script to verify the usage of
derive_moreand the implementation ofDeref:Please review the output of this script to ensure that
derive_moreis being used correctly and that theDereftrait is implemented as intended.✅ Verification successful
Further Verification Needed: Proper Implementation of
DerefTraitTo ensure that the
Dereftrait is correctly implemented:
- Search for
#[derive(Deref)]annotations in the codebase.- Check for any manual implementations of the
Dereftrait.Please run the following script and review the output:
derive_moreUsage andDerefImplementation Verified
derive_more::Derefis correctly imported in:
src/entities/currency.rssrc/entities/fractions/fraction.rs
Dereftrait is implemented via#[derive(Deref)]in these files.No manual implementations of the
Dereftrait were found.Core module imports have been adjusted as per PR objectives.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of derive_more and implementation of Deref # Test 1: Check for imports of derive_more echo "Checking for imports of derive_more:" rg --type rust 'use derive_more::Deref;' # Test 2: Check for Deref implementations echo "Checking for Deref implementations:" ast-grep --lang rust --pattern $'#[derive(Deref)] struct $STRUCT_NAME { $$$ }' # Test 3: Check for changes in core module imports echo "Checking for changes in core module imports:" rg --type rust '^use (crate::)?core::'Length of output: 1114
Script:
#!/bin/bash # Description: Verify the implementation of the Deref trait # Test 1: Search for #[derive(Deref)] echo "Searching for #[derive(Deref)]:" rg '#\[derive\(Deref\)\]' --type rust # Test 2: Search for manual implementations of Deref echo "Searching for manual implementations of Deref trait:" rg 'impl\s+Deref\s+for\s+' --type rustLength of output: 268
src/prelude.rs (1)
28-28: Approve change, but verify impact of removingops::Div.The removal of
ops::Divfrom the core imports aligns with the PR objectives of adjusting core module imports. However, we should ensure this change doesn't inadvertently break any existing functionality.To verify the impact of this change, please run the following script:
Consider adding a comment explaining why
ops::Divwas removed, e.g.:// Note: ops::Div removed as it's now derived using the `derive_more` crate pub use core::{cmp::Ordering, num::NonZeroU64, str::FromStr};This will help future maintainers understand the reasoning behind this change.
src/entities/currency.rs (4)
3-3: LGTM: Correct import ofDereffromderive_moreThe import is correctly placed and aligns with the PR objectives of integrating the
derive_morecrate for implementing theDereftrait.
13-13: Excellent:Dereftrait automatically derived forCurrencyLikeThe addition of
Derefto the derive macro forCurrencyLikeis a good improvement. This change:
- Automatically implements the
Dereftrait, reducing boilerplate code.- Maintains backwards compatibility by keeping other derived traits.
- Aligns with the PR objectives of simplifying the codebase.
This modification should make the code more maintainable and easier to work with.
28-29: Great:#[deref]attribute correctly applied tometafieldThe addition of the
#[deref]attribute to themetafield is a good change:
- It specifies that the
metafield should be used for theDerefimplementation.- This allows direct access to the underlying metadata type
Mwithout explicit dereferencing.- It's consistent with the
Derefderive macro added to the struct.- This change, combined with the derive macro, effectively replaces the manual
Derefimplementation, as mentioned in the PR summary.This modification simplifies the usage of
CurrencyLikeand makes the code more intuitive.
Line range hint
1-138: Summary: Successful integration ofderive_moreforDerefimplementationThe changes in this file successfully integrate the
derive_morecrate to implement theDereftrait for theCurrencyLikestruct. The modifications include:
- Importing the
Dereftrait fromderive_more.- Adding
Derefto the derive macro forCurrencyLike.- Applying the
#[deref]attribute to themetafield.These changes align well with the PR objectives and should result in:
- Simplified code by removing the need for a manual
Derefimplementation.- Improved readability and maintainability.
- No apparent negative impact on existing functionality.
The implementation looks correct and consistent. Great job on this refactoring!
src/entities/fractions/currency_amount.rs (1)
2-2: Importingcore::ops::DivThe addition of
use core::ops::Div;is appropriate as theDivtrait is utilized in thedividemethod for division operations. Good job ensuring all necessary traits are correctly imported.src/entities/fractions/fraction.rs (4)
4-4: LGTM: Correct inclusion ofDivinopsimportsAdding
Divto theopsimports enables division operations, aligning with the new implementations forFractionLike<M>.
6-6: LGTM: Appropriate use ofderive_more::DerefIntroducing
derive_more::Derefsimplifies the dereferencing ofFractionLike<M>, enhancing code readability.
Line range hint
57-62: LGTM: Addition ofDivtrait bounds enhances arithmetic capabilitiesIncluding
Div<Output = Self>in the trait bounds ofFractionTraitextends the arithmetic operations, enabling division on fractions.
Line range hint
74-74: LGTM: Inclusion ofDivinFractionBasetrait boundsAdding
Div<Output = Self>to theFractionBasetrait ensures that all implementing types support division, maintaining consistency across arithmetic operations.
Integrated
derive_moreforDerefattribute across various structs and adjusted the core module imports accordingly. This helps in simplifying the code and reducing redundancy.Summary by CodeRabbit
New Features
Pricetype for handling currency prices with various methods for creation, multiplication, and conversion.Bug Fixes
Refactor
Tests
Pricestruct and its methods to ensure functionality.