Feature anyhow removal #459
Conversation
WalkthroughThis pull request refactors error handling across the codebase by removing the dependency on the Changes
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 0
🧹 Nitpick comments (3)
src/bitcoin_price.rs (1)
43-51: Enhanced return type for get_price method.The method has been improved to return
Result<f64, MostroError>instead ofOption<f64>, which provides better error information when price retrieval fails. The error handling is properly implemented using map_err and ok_or.Consider adding a more specific error message when a currency is not found, such as including the currency code in the error:
- .ok_or(MostroInternalErr(ServiceError::NoAPIResponse)) + .ok_or(MostroInternalErr(ServiceError::NoAPIResponse(format!("Price for currency '{}' not found", currency))))This would require updating the ServiceError enum to accept a String for the NoAPIResponse variant.
src/db.rs (2)
590-591: Remove debug print statements.These println statements appear to be leftover debugging code that should be removed or replaced with proper logging.
- println!("solver_pubkey: {}", solver_pubkey); - println!("order_id: {}", order_id); + // Use tracing module for logging if needed + tracing::debug!("Checking if solver {} is assigned to order {}", solver_pubkey, order_id);
22-624: Consider extracting the common error mapping pattern into a helper function.Throughout the file, there's a repetitive pattern of mapping SQLx errors to
MostroInternalErr. Creating a helper function would reduce duplication and make the code more maintainable.You could create a helper function like this:
fn map_db_err<T>(result: Result<T, sqlx::Error>) -> Result<T, MostroError> { result.map_err(|e| MostroInternalErr(ServiceError::DbAccessError(e.to_string()))) }Then use it in your code:
- let mut conn = pool - .acquire() - .await - .map_err(|e| MostroInternalErr(ServiceError::DbAccessError(e.to_string())))?; + let mut conn = map_db_err(pool.acquire().await)?;And for query execution:
- .await - .map_err(|e| MostroInternalErr(ServiceError::DbAccessError(e.to_string())))?; + .await + .pipe(map_db_err)?;The
.pipe()method would require thetapcrate or similar function composition utility, but this approach would significantly reduce code duplication.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
Cargo.toml(1 hunks)src/app.rs(1 hunks)src/app/add_invoice.rs(0 hunks)src/app/admin_add_solver.rs(0 hunks)src/app/rate_user.rs(0 hunks)src/app/take_buy.rs(0 hunks)src/app/take_sell.rs(0 hunks)src/bitcoin_price.rs(2 hunks)src/cli.rs(2 hunks)src/cli/settings.rs(8 hunks)src/db.rs(34 hunks)src/lightning/mod.rs(0 hunks)src/main.rs(1 hunks)src/messages.rs(1 hunks)src/scheduler.rs(2 hunks)src/util.rs(1 hunks)
💤 Files with no reviewable changes (6)
- src/app/take_sell.rs
- src/app/take_buy.rs
- src/app/add_invoice.rs
- src/app/rate_user.rs
- src/lightning/mod.rs
- src/app/admin_add_solver.rs
✅ Files skipped from review due to trivial changes (1)
- src/main.rs
🔇 Additional comments (21)
Cargo.toml (1)
53-54: LGTM! The patch configuration is a good approach for local development.The PR successfully removes the
anyhowdependency and adds a local path reference tomostro-corein the patch section. This setup is beneficial for development as it allows you to work with a local version of the core library while implementing the error handling changes.src/app.rs (1)
356-356: Fixed error handling to dereference theMostroErrorcorrectly.The code properly dereferences the error with
*errwhen passing it to themanage_errorsfunction. This change is necessary after switching fromanyhow::Resultto the customMostroErrortype.src/cli.rs (3)
5-5: LGTM! Appropriate import for the new error handling approach.This import properly replaces the previous
anyhow::Resultdependency with the project-specific error type.
32-32: Function signature updated to use the custom error type.The return type now explicitly specifies
MostroErroras the error type, which enhances clarity and type safety.
36-36: Function return values properly wrapped withOk.The return values are now properly wrapped with
Ok()to match the new function signature. This ensures type consistency throughout the error handling chain.Also applies to: 38-38
src/messages.rs (2)
1-1: LGTM! Appropriate import for the new error handling approach.This import properly replaces the previous
anyhow::Resultdependency with the project-specific error type.
7-7: Function signature updated to use the custom error type.The return type now explicitly specifies
MostroErroras the error type, which enhances clarity and type safety.src/scheduler.rs (4)
12-13: Appropriate import of custom error types.Good addition of imports for the custom MostroError and ServiceError types that are part of the PR's main goal to replace anyhow.
235-235: Updated function signature correctly.The function signature has been properly updated to return
Result<(), MostroError>instead ofanyhow::Result<()>, which aligns with the PR objective of removing anyhow dependency.
240-244: Improved error handling with specific error types.Error handling has been properly refactored to use the custom error type hierarchy. This provides more specific error information compared to the previous anyhow implementation.
248-252: Consistent error handling pattern.The error handling for key retrieval follows the same pattern as the database connection error handling, maintaining consistency throughout the codebase.
src/util.rs (1)
542-542: Updated return type for invoice_subscribe function.The function signature has been properly changed from
anyhow::Result<()>toResult<(), MostroError>to align with the PR objective of removing anyhow dependency.src/bitcoin_price.rs (3)
1-2: Proper imports for custom error types.Good addition of the necessary imports for MostroError and ServiceError to implement the anyhow removal strategy.
23-31: Enhanced error handling in update_prices method.The method's return type has been correctly changed to
Result<(), MostroError>and HTTP request error handling has been improved with specific error types, making debugging and error handling more effective.
36-39: Consistent error handling for RwLock operations.Error handling for RwLock write operations follows the same pattern as other error handling in the file, converting lock errors to ServiceError::IOError.
src/cli/settings.rs (5)
3-4: Appropriate imports for custom error types.Good addition of the necessary imports to implement the anyhow removal strategy across the codebase.
50-54: Updated error type for TryFrom implementation.The error type for the Database TryFrom implementation has been correctly updated to use MostroError, aligning with the PR objective.
70-74: Consistent error type updates across all TryFrom implementations.All TryFrom implementations for the different configuration structs (Lightning, Nostr, Mostro) have been updated consistently to use MostroError as the error type.
Also applies to: 84-88, 107-111
173-173: Updated function signature for init_default_dir.The function signature has been properly updated to return
Result<PathBuf, MostroError>instead of using anyhow's Result type.
187-189: Enhanced error handling with specific error types.The error handling in init_default_dir has been improved to use map_err with specific ServiceError variants (EnvVarError, IOError), providing better context for troubleshooting when errors occur.
Also applies to: 204-213
src/db.rs (1)
22-73: LGTM! Good error handling refactoring.The removal of anyhow and introduction of the more specific
MostroErrortype improves the API of this module by making the error types explicit in the function signatures.
@grunch
removed anyhow dependency.
Summary by CodeRabbit
Chores
anyhowdependency to streamline the project.Refactor
MostroErrorfor enhanced error specificity.