Skip to content
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

refactor!: upgrade thiserror and refine error handling #88

Merged
merged 1 commit into from
Nov 6, 2024

Conversation

shuhuiluo
Copy link
Collaborator

@shuhuiluo shuhuiluo commented Nov 6, 2024

Upgraded thiserror version and made specific error messages clearer. Adjusted error variants and assertions to provide more meaningful feedback, enhancing clarity in debug and runtime scenarios.

Summary by CodeRabbit

  • New Features

    • Updated the package version to 3.1.0, enhancing overall functionality.
  • Bug Fixes

    • Improved error handling for currency operations, providing clearer error messages for issues like currency mismatch and overflow.
    • Enhanced error clarity in mathematical functions, specifying the type of error encountered.
  • Refactor

    • Simplified the error handling mechanism in the sorted_insert function, improving usability.
  • Documentation

    • Updated documentation for the Error enum to reflect new error types and their purposes.

Upgraded `thiserror` version and made specific error messages clearer. Adjusted error variants and assertions to provide more meaningful feedback, enhancing clarity in debug and runtime scenarios.
@shuhuiluo shuhuiluo requested a review from malik672 November 6, 2024 05:21
Copy link

coderabbitai bot commented Nov 6, 2024

Walkthrough

The pull request includes modifications to the Cargo.toml file, updating the package version and changing the thiserror dependency. It also enhances error handling across several files, specifically in the CurrencyAmount, FractionBase, Price, and utility functions, by renaming error types and improving the specificity of error messages. The sorted_insert function's return type is simplified, and the error handling in the sqrt function is clarified. Overall, these changes aim to improve clarity and usability in error reporting and function behavior.

Changes

File Path Change Summary
Cargo.toml - Updated package version from 3.0.0 to 3.1.0.
- Changed thiserror dependency from 1.0 to 2, added default-features = false.
- Updated feature declaration for std from ["thiserror"] to ["thiserror/std"].
src/entities/fractions/currency_amount.rs - Changed error in new from Error::MaxUint to Error::UintOverflow.
- Updated errors in add and subtract from Error::NotEqual to Error::CurrencyMismatch.
- Modified error in to_fixed from Error::NotEqual to Error::Invalid("DECIMALS").
src/entities/fractions/fraction.rs - Updated error in to_significant from Error::Invalid to Error::Invalid("SIGNIFICANT_DIGITS").
src/entities/fractions/price.rs - Changed error in multiply and quote from Error::NotEqual to Error::CurrencyMismatch.
src/error.rs - Renamed MaxUint to UintOverflow, NotEqual to CurrencyMismatch.
- Updated Invalid variant to accept a static string.
- Simplified error messages for ChainIdMismatch and EqualAddresses.
src/utils/sorted_insert.rs - Changed return type from Result<Option<T>, Error> to Option<T>.
- Simplified error handling with assertions instead of returning errors.
src/utils/sqrt.rs - Updated error in sqrt from Error::Invalid to Error::Invalid("NEGATIVE").

Possibly related PRs

Suggested reviewers

  • malik672

Poem

In the meadow where bunnies play,
Changes hop in a bright array.
Errors refined, clarity's cheer,
With each new version, we hold dear.
Let's celebrate with a joyful leap,
For code that's cleaner, and promises to keep! 🐇✨


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (8)
src/utils/sqrt.rs (1)

Line range hint 22-49: Add test coverage for negative input case.

While the test coverage is comprehensive for valid inputs, there's no test verifying the error handling for negative inputs. Consider adding a test like this:

#[test]
fn test_sqrt_negative() {
    let negative = BigInt::from(-1);
    match sqrt(&negative) {
        Err(Error::Invalid(msg)) => assert_eq!(msg, "NEGATIVE"),
        _ => panic!("Expected Error::Invalid(\"NEGATIVE\") for negative input"),
    }
}
src/error.rs (3)

12-14: Consider enhancing the documentation with an example.

The rename to UintOverflow and the specific mention of U256::MAX in the error message are good improvements.

Consider adding an example to the documentation to show when this error might occur:

-/// Triggers when it tries to exceed [`alloy_primitives::U256::MAX`].
+/// Triggers when a numeric operation would exceed [`alloy_primitives::U256::MAX`].
+///
+/// # Example
+/// This error occurs in operations like currency amount multiplication where
+/// the result would exceed the maximum allowed value.

20-22: Consider adding validation and documentation for Invalid variant.

While the addition of a &'static str parameter allows for more specific error messages, consider:

  1. Adding documentation about expected format/content of error messages
  2. Creating constants for common error messages to ensure consistency
/// Common error messages for the Invalid variant
pub mod error_messages {
    pub const SIGNIFICANT_DIGITS: &str = "SIGNIFICANT_DIGITS";
    // Add other common error messages here
}

43-55: Consider adding negative test cases.

While the current tests verify the error messages correctly, consider adding tests for:

  1. Invalid error messages (empty strings, very long messages)
  2. Different chain ID combinations
  3. Edge cases for currency mismatches
#[test]
fn test_chain_id_mismatch_combinations() {
    let cases = [(0, 1), (u64::MAX, 0), (u64::MAX, u64::MAX - 1)];
    for (id1, id2) in cases {
        let error = Error::ChainIdMismatch(id1, id2);
        assert_eq!(error.to_string(), format!("chain IDs do not match: {} and {}", id1, id2));
    }
}
src/utils/sorted_insert.rs (3)

11-16: Consider the implications of switching from Results to panics.

The change from Result<Option<T>, Error> to Option<T> with assertions represents a significant shift in error handling strategy. While this simplifies the API, it also means that invalid inputs will now cause panics instead of returnable errors.

This is a breaking change that:

  • Makes the function more ergonomic for valid inputs
  • Treats max_size validation as a programming error rather than a runtime error
  • May require updates to error handling in consuming code

Consider whether these conditions truly represent programming errors (that should panic) rather than runtime errors (that should return Result). If these could be runtime conditions, consider keeping the Result return type for more graceful error handling.


56-59: Enhance the array length validation test.

While the test correctly verifies the panic message, it could be more thorough by testing with different array and max sizes.

Consider adding more test cases:

#[test]
#[should_panic(expected = "array length cannot exceed max_size")]
fn test_length_greater_than_max_size_variants() {
    // Test with different array sizes exceeding max_size
    let mut arr1 = vec![1, 2, 3];
    sorted_insert(&mut arr1, 1, 2, cmp);

    let mut arr2 = vec![1, 2, 3, 4, 5];
    sorted_insert(&mut arr2, 1, 3, cmp);
}

Line range hint 3-5: Update documentation to reflect panic conditions.

The function documentation should be updated to explicitly mention the panic conditions for invalid inputs.

Consider updating the documentation:

 /// Given an array of items sorted by `comparator`, insert an item into its sort index and constrain
 /// the size to `maxSize` by removing the last item
+///
+/// # Panics
+/// 
+/// This function will panic if:
+/// - `max_size` is zero
+/// - The length of `items` exceeds `max_size`
src/entities/fractions/currency_amount.rs (1)

126-126: Consider enhancing the error message with more context.

While the change from NotEqual to Invalid("DECIMALS") is an improvement, the error message could be more specific about why the decimals are invalid.

Consider using a more descriptive error message that includes the actual and maximum allowed decimal places:

-return Err(Error::Invalid("DECIMALS"));
+return Err(Error::Invalid(&format!(
+    "Requested {decimal_places} decimals exceeds currency's {max} decimals",
+    max = self.currency.decimals()
+)));
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between f94c2a1 and b040056.

📒 Files selected for processing (7)
  • Cargo.toml (2 hunks)
  • src/entities/fractions/currency_amount.rs (4 hunks)
  • src/entities/fractions/fraction.rs (1 hunks)
  • src/entities/fractions/price.rs (2 hunks)
  • src/error.rs (1 hunks)
  • src/utils/sorted_insert.rs (3 hunks)
  • src/utils/sqrt.rs (1 hunks)
🔇 Additional comments (14)
Cargo.toml (3)

3-3: LGTM: Version bump follows semantic versioning.

The version increment from 3.0.0 to 3.1.0 appropriately reflects the thiserror upgrade and error handling improvements.


23-23: LGTM: Feature declaration correctly updated.

The std feature now properly includes thiserror/std, which is the correct way to enable std features when default-features is disabled.


20-20: Verify the impact of thiserror v2 upgrade.

The upgrade from thiserror 1.0 to 2.0 is a major version change that could introduce breaking changes. The addition of default-features = false is good for no_std compatibility.

Let's verify the changes:

✅ Verification successful

Let me check the specific error implementations to better understand the impact.


The upgrade to thiserror v2 is safe for this codebase

The error implementation in this codebase is straightforward and only uses basic thiserror features that are stable across versions:

  • Simple enum with static error messages
  • Basic parameter interpolation with numbered placeholders
  • Standard derive macros alongside thiserror

The usage patterns are fully compatible with thiserror v2, and the default-features = false flag is appropriate for the no_std support shown in the test configuration.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for potential breaking changes in error handling

# Test 1: Look for thiserror derive macro usage
rg -l '#\[derive\(.*Error.*\)\]'

# Test 2: Check for custom error implementations
rg -A 3 'impl.*Error.*for'

# Test 3: Look for error type definitions
ast-grep --pattern 'pub enum $name {
  $$$
}'

Length of output: 124


Script:

#!/bin/bash
# Get the content of error.rs to analyze the error types and their implementations
cat src/error.rs

# Also check for any thiserror macro usage patterns
rg -p '#\[derive\(.*Error.*\)\]' src/error.rs -A 5

Length of output: 1985

src/utils/sqrt.rs (1)

14-14: LGTM! Error message improvement is clear and descriptive.

The change from a generic Invalid error to Error::Invalid("NEGATIVE") provides better context for debugging and aligns well with the PR's objective of improving error handling clarity.

src/error.rs (3)

1-2: LGTM! Documentation and derive attributes are well structured.

The documentation comment clearly describes the purpose of these error types, and the derive attributes are properly consolidated.


5-9: LGTM! Error messages are clear and informative.

The error messages for chain ID mismatch and equal addresses provide precise information about the error condition.


16-18: LGTM! The rename improves domain clarity.

The rename from NotEqual to CurrencyMismatch better reflects the domain-specific nature of this error.

src/utils/sorted_insert.rs (1)

22-22: LGTM! Core logic changes are consistent with new error handling.

The changes to return values align well with the simplified return type while preserving the original sorting behavior.

Also applies to: 33-33

src/entities/fractions/price.rs (2)

86-86: LGTM! Improved error clarity in multiply method.

The change from Error::NotEqual to Error::CurrencyMismatch provides a more descriptive and specific error message when the quote currency doesn't match the other price's base currency.


105-105: LGTM! Improved error clarity in quote method.

The change from Error::NotEqual to Error::CurrencyMismatch provides a more descriptive and specific error message when the input currency amount's currency doesn't match the base currency. This maintains consistency with the error handling in the multiply method.

src/entities/fractions/currency_amount.rs (3)

28-28: LGTM! Improved error naming clarity.

The rename from MaxUint to UintOverflow better describes the error condition, making it more intuitive for developers to understand the issue.


91-91: LGTM! Enhanced domain-specific error clarity.

The rename from NotEqual to CurrencyMismatch provides better context by explicitly indicating the nature of the mismatch, making it immediately clear that the error relates to incompatible currencies.


101-101: LGTM! Consistent error handling with add operation.

The error type change maintains consistency with the add method, ensuring symmetric error handling for both arithmetic operations.

src/entities/fractions/fraction.rs (1)

126-126: LGTM! Consider making the error message even more descriptive.

The change from a generic Invalid error to Invalid("SIGNIFICANT_DIGITS") improves error clarity. However, consider making it even more descriptive by including the invalid value, e.g., "SIGNIFICANT_DIGITS_MUST_BE_POSITIVE".

Let's verify consistent error message usage across the codebase:

✅ Verification successful

Error messages follow a consistent pattern across the codebase

The verification shows that all Error::Invalid instances use concise, uppercase static strings as error messages:

  • "NEGATIVE" for negative number validation
  • "DECIMALS" for decimal places validation
  • "SIGNIFICANT_DIGITS" for significant digits validation

The current error message style aligns well with the codebase's conventions, so making it more verbose isn't necessary.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for consistent error message patterns in Invalid errors
# Expected: All Invalid errors should use descriptive static strings

# Search for Error::Invalid usage
rg "Error::Invalid\(" -A 1

Length of output: 537

@malik672 malik672 merged commit bd5c71a into master Nov 6, 2024
3 checks passed
@shuhuiluo shuhuiluo deleted the thiserror branch November 6, 2024 07:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants