Add ResultExt trait for error mapping utilities #320
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds a new public ResultExt trait with map_err_str/map_err_into and exports it; applies that trait across many modules to replace manual error-to-string mappings, collapses several nested if-let checks into combined guards, changes a stream return to Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as Module code
participant ResultExt as ResultExt trait
participant ErrCtor as Target error constructor
Caller->>ResultExt: .map_err_str(TargetError)
Note right of ResultExt: Requires InitialError: Display\nconverts error -> String then F(String)->TargetError
ResultExt-->>Caller: Result<T, TargetError>
Caller->>ErrCtor: propagate/return TargetError
Note over Caller,ErrCtor: Replaces `.map_err(|e| Variant(e.to_string()))`\nand is applied across many modules
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (23)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
🧹 Nitpick comments (3)
rust/crates/cove-util/src/result_ext.rs (3)
29-32: Polish doc wording and consistency.Capitalize the first word and reference trait names consistently.
-/// map an error using Into::into before passing to error constructor +/// Map an error using `Into::into` before passing to the error constructor. -/// shorthand for map_err(|e| f(e.into())) to convert errors using Into trait +/// Shorthand for `map_err(|e| f(e.into()))`, converting with the `Into` trait.
25-27: Clarify generic names to reduce ambiguity.Using
F/Gfor both “function” and “error type” can confuse readers. PreferE2for the new error type andIfor the intermediate type.Example (pairs with the FnOnce change):
- fn map_err_str<F>(...) + fn map_err_str<E2, F>(...) - fn map_err_into<F, G>(...) + fn map_err_into<I, E2, F>(...)Also applies to: 32-34
3-35: Consider a zero‑arg convenience:into_err()Common case: only convert
E: Into<E2>without applying a constructor. Optional sugar:fn into_err<E2>(self) -> Result<T, E2> where E: Into<E2>;Impl:
self.map_err(Into::into).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rust/crates/cove-util/src/result_ext.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test (ubuntu-latest)
- GitHub Check: test (macos-latest)
🔇 Additional comments (2)
rust/crates/cove-util/src/result_ext.rs (2)
42-43: LGTM: core mapping logic is correct.Implementations correctly delegate to
Result::map_errwithto_string()andinto().Also applies to: 49-49
11-12: Fix doctest import path and ensure thiserror is available to doctestsFile: rust/crates/cove-util/src/result_ext.rs (doctest)
-/// use shuttle_common::result_ext::ResultExt; +/// use cove_util::result_ext::ResultExt;
- thiserror is already declared in rust/crates/cove-util/Cargo.toml under [dependencies]; if doctests fail, add it to [dev-dependencies] or replace the #[derive(thiserror::Error)] example with a manual Display impl.
- Optional: re-export or pub-mod the module from rust/crates/cove-util/src/lib.rs (e.g.
pub mod result_ext;orpub use crate::result_ext::ResultExt;) if you want users to import withuse cove_util::ResultExt;.
praveenperera
force-pushed
the
result-ext
branch
from
43aab07 to
26592bb
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
rust/src/manager/wallet_manager.rs (1)
635-639: Potential underflow in confirmations calculation.If block_height > current_height, subtraction underflows (panic in debug, wrap in release).
Apply this fix to guard:
- Ok(current_height - block_height + 1) + if block_height > current_height { + Ok(0) + } else { + Ok(current_height - block_height + 1) + }rust/src/database/wallet_data/label.rs (1)
270-272: Don’t swallow DB errors when upserting txn labels.unwrap_or(None) hides read failures; may cause silent data loss or inconsistencies.
- let current = self.get_txn_label_record(label.ref_).unwrap_or(None); + let current = self.get_txn_label_record(label.ref_)?;
🧹 Nitpick comments (8)
rust/crates/cove-util/src/result_ext.rs (1)
10-11: Fix doctest crate path and add Rust fence for clarity.The example imports from
shuttle_common, which will fail here. Also, mark the code block as Rust for better docs.Apply:
- /// ``` - /// use shuttle_common::result_ext::ResultExt; + /// ```rust + /// use cove_util::result_ext::ResultExt;rust/src/reporting.rs (1)
22-23: Fix typos in user-facing strings.
- “fianlize” → “finalize”
- “uft8” → “utf8”
Apply:
- #[error("failed to fianlize csv: {0}")] + #[error("failed to finalize csv: {0}")] @@ - String::from_utf8(self.into_bytes()).expect("we only create rows with valid uft8 strings") + String::from_utf8(self.into_bytes()).expect("we only create rows with valid utf8 strings")Also applies to: 35-36
rust/src/file_handler.rs (1)
20-21: Fix typo in error message.“Unable to to read file” → “Unable to read file”.
- #[error("Unable to to read file {0}")] + #[error("Unable to read file {0}")]rust/crates/cove-util/src/lib.rs (1)
3-3: Re-export ResultExt at crate root for ergonomics.Add a public re-export in rust/crates/cove-util/src/lib.rs so callers can use
use cove_util::ResultExt as _;. Repository search shows many files currently importuse cove_util::result_ext::ResultExt as _, so this is safe and non-breaking.pub mod result_ext; +pub use result_ext::ResultExt;rust/src/multi_qr.rs (1)
55-56: Fix typo in user-facing error message.“BBQr did not container seed words” → “BBQR did not contain seed words”.
- #[error("BBQr did not container seed words, found: {0}")] + #[error("BBQR did not contain seed words, found: {0}")]rust/src/manager/wallet_manager.rs (1)
853-854: Nit: typo in log message.“swtich” → “switch”.
- error!("trying to swtich to native segwit, but already segwit"); + error!("trying to switch to native segwit, but already segwit");rust/src/database/unsigned_transactions.rs (2)
134-136: Inconsistent error mapping pattern detected.Line 136 still uses the old
map_err(|error| Error::TableAccess(error.to_string()))?pattern while the rest of the file has been updated to usemap_err_str.Apply this diff to maintain consistency:
- let table = read_txn - .open_table(MAIN_TABLE) - .map_err(|error| Error::TableAccess(error.to_string()))?; + let table = read_txn.open_table(MAIN_TABLE).map_err_str(Error::TableAccess)?;
149-151: Another inconsistent error mapping pattern.Similar to the previous comment, line 149-151 uses the old pattern while the rest uses
map_err_str.Apply this diff for consistency:
- let table = read_txn - .open_table(BY_WALLET_TABLE) - .map_err(|error| Error::TableAccess(error.to_string()))?; + let table = read_txn.open_table(BY_WALLET_TABLE).map_err_str(Error::TableAccess)?;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
rust/crates/cove-util/src/lib.rs(1 hunks)rust/crates/cove-util/src/result_ext.rs(1 hunks)rust/src/database/global_cache.rs(2 hunks)rust/src/database/global_flag.rs(3 hunks)rust/src/database/historical_price.rs(6 hunks)rust/src/database/unsigned_transactions.rs(3 hunks)rust/src/database/wallet.rs(4 hunks)rust/src/database/wallet_data/label.rs(6 hunks)rust/src/file_handler.rs(2 hunks)rust/src/manager/import_wallet_manager.rs(3 hunks)rust/src/manager/send_flow_manager.rs(7 hunks)rust/src/manager/wallet_manager.rs(7 hunks)rust/src/manager/wallet_manager/actor.rs(17 hunks)rust/src/multi_qr.rs(4 hunks)rust/src/reporting.rs(2 hunks)rust/src/tap_card/tap_signer_reader.rs(2 hunks)rust/src/wallet.rs(10 hunks)rust/src/wallet_scanner.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- rust/src/wallet_scanner.rs
🧰 Additional context used
🧬 Code graph analysis (6)
rust/src/wallet.rs (1)
rust/src/bdk_store.rs (1)
try_new(23-37)
rust/src/database/wallet.rs (1)
rust/src/database.rs (1)
wallets(56-58)
rust/src/manager/wallet_manager.rs (1)
rust/src/manager/wallet_manager/actor.rs (1)
next_address(665-668)
rust/src/database/global_cache.rs (3)
rust/src/database/global_flag.rs (1)
set(60-76)rust/src/database/unsigned_transactions.rs (1)
set(162-174)rust/src/database/global_config.rs (1)
set(317-337)
rust/src/manager/send_flow_manager.rs (2)
rust/crates/cove-types/src/address.rs (1)
amount(220-222)rust/src/manager/send_flow_manager/state.rs (1)
is_coin_control(77-79)
rust/src/database/unsigned_transactions.rs (2)
rust/src/database/global_cache.rs (2)
get(72-82)set(84-100)rust/src/database/global_flag.rs (2)
get(45-58)set(60-76)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test (ubuntu-latest)
- GitHub Check: test (macos-latest)
🔇 Additional comments (60)
rust/crates/cove-util/src/result_ext.rs (2)
39-55: Trait impl looks solid; ergonomics LGTM.Generics and
FnOncecapture are correct; bodies delegate tomap_errcleanly. No correctness concerns.
13-24: No change needed —thiserroris already declared for the crate. rust/crates/cove-util/Cargo.toml containsthiserror = { workspace = true }, so the doctest example is covered.rust/src/tap_card/tap_signer_reader.rs (1)
18-19: Adoption ofmap_err_stris correct and improves readability.
PsbtSignError(String)maps cleanly viamap_err_str(Error::PsbtSignError). Import scoped as_is consistent with the repo.Also applies to: 259-260
rust/src/reporting.rs (1)
87-88: Good switch tomap_err_str(variant) to simplify error mapping.Passing
CsvCreationError::FinalizeCsvdirectly is the right usage.rust/src/file_handler.rs (2)
3-4: Nice use ofmap_err_strto reduce boilerplate.Both
openandread_to_endmappings are clearer now.Also applies to: 40-41, 44-44
34-41: Handle missing-file errors viaErrorKind::NotFoundrather thanexists()
Replace theexists()check and directopen()mapping with:- if !self.file_path.exists() { - return Err(FileHandlerError::FileNotFound); - } - - let mut file = - std::fs::File::open(&self.file_path).map_err_str(FileHandlerError::OpenFile)?; + use std::io::ErrorKind; + let mut file = match std::fs::File::open(&self.file_path) { + Ok(f) => f, + Err(e) if e.kind() == ErrorKind::NotFound => return Err(FileHandlerError::FileNotFound), + Err(e) => return Err(FileHandlerError::OpenFile(e.to_string())), + };Please manually verify the codebase for any other
.exists()→File::open()patterns to align error handling.rust/src/manager/import_wallet_manager.rs (3)
4-4: Standardize error mapping import looks good.Brings ResultExt into scope without polluting the namespace.
104-105: Use of map_err_str here is correct and reduces boilerplate.Mnemonic parse errors map cleanly into InvalidWordGroup(String).
137-138: Consistent error mapping on wallet creation.map_err_str to WalletImportError(String) is appropriate.
rust/src/multi_qr.rs (4)
17-17: Importing ResultExt for local helpers is correct.
118-120: BBQR add_part uses map_err_str correctly.Good conversion to ParseError(String).
195-197: Consistent map_err_str in add_part().Keeps error handling uniform.
248-249: Seed words parsing mapped via map_err_str is clean.rust/src/manager/wallet_manager.rs (7)
15-16: Import grouping with ResultExt is fine.
240-242: Good: get() error mapped via map_err_str(GetSelectedWalletError).Keeps WalletDoesNotExist path intact.
360-361: Good: fee client errors standardized to FeesError(String).
372-373: CSV creation error mapping standardized.
626-627: Transaction details async mapping is now concise.
649-651: next_address error mapping is correct and consistent.
812-813: fee_rate_options adopts the same standardized mapping.rust/src/database/wallet_data/label.rs (6)
6-6: ResultExt import is appropriate.
258-265: Writes: begin_write/commit now mapped via DatabaseAccess.This aligns with the module’s error model.
296-301: Insert single label: standardized write/commit mapping.
309-315: Bulk insert: standardized write/commit mapping.
365-369: Delete labels: standardized write/commit mapping.
415-418: Reads: begin_read/table open mapped to DatabaseAccess/TableAccess.rust/src/database/wallet.rs (4)
6-7: ResultExt import is fine.
156-161: get_all: map_err_str(ReadError) is correct.
260-264: save_all_wallets: insert/commit mapped to SaveError(String).Clearer and consistent error surface.
292-295: read_table: read/table open mapped to DatabaseAccess/TableAccess.rust/src/database/global_flag.rs (3)
6-7: ResultExt import looks good.
46-49: get(): read/table open mapped to DatabaseAccess/TableAccess; read mapped to Read(String).
62-72: set(): write/table open/insert/commit use map_err_str consistently.Good standardization.
rust/src/wallet.rs (1)
25-25: Adoption of map_err_str across wallet ops is correct.All mapped variants accept String; conversions read cleanly and reduce boilerplate.
Also applies to: 189-193, 252-253, 304-311, 340-343, 366-367, 405-405, 411-412, 477-477, 489-489, 589-589
rust/src/database/historical_price.rs (5)
6-7: ResultExt import is correct.
53-59: get_price_for_block: read/table open and get mapped via map_err_str.
75-84: set_price_for_block: write/table open/insert/commit standardized.
95-99: get_prices_for_blocks: consistent error mapping per get().Also applies to: 106-107
118-128: clear(): standardized mapping; retain() error mapped to DatabaseAccess.rust/src/database/unsigned_transactions.rs (2)
5-6: LGTM! Clean import of the ResultExt trait.The import follows the proper convention with the underscore suffix to indicate it's imported for trait methods only.
118-126: LGTM! Consistent error mapping pattern applied.Good use of
map_err_strfor standardizing error conversion across database operations in thedelete_tx_idmethod.rust/src/database/global_cache.rs (2)
6-7: LGTM! Clean trait import.Good use of the underscore import pattern for the ResultExt trait.
72-100: Excellent consistent error handling refactor!All error mappings in both
getandsetmethods have been properly updated to usemap_err_str, providing a cleaner and more maintainable error handling pattern.rust/src/manager/send_flow_manager.rs (7)
14-15: LGTM! Proper trait import.The ResultExt trait is correctly imported for its extension methods.
519-520: Good simplification of error handling.The
map_err_strusage here makes the error conversion more concise and readable.
1206-1207: Clean error propagation simplification.The double question mark pattern
??withmap_err_stris much cleaner than the previous verbose error mapping.
1265-1266: Good standardization of error handling.The error mapping for
UnableToGetMaxSendis now consistent with the rest of the codebase.
1427-1437: Elegant use of if-let chain with pattern matching!The refactored conditional using
if let Some(amount) = address_with_network.amount && !is_coin_controlis cleaner and more idiomatic than nested conditionals. This leverages Rust's if-let chains feature effectively.
1529-1534: Good flattening of conditional logic.The combined conditional
if matches!(wallet_type, WalletType::Cold | WalletType::XpubOnly) && let Err(e) = manager.save_unsigned_transaction(details.clone())reduces nesting while maintaining the same behavior. The pattern matching withmatches!is idiomatic and clear.
1819-1821: Consistent error handling pattern.Good application of
map_err_strfor both PSBT building and fee extraction errors, maintaining consistency with the rest of the refactor.rust/src/manager/wallet_manager/actor.rs (10)
39-40: LGTM! Trait import added correctly.The ResultExt trait is properly imported to enable the
map_err_strextension method throughout the file.
186-187: Excellent systematic error handling refactor for transaction building!All PSBT finish operations now consistently use
map_err_str(Error::BuildTxError)instead of the verbosemap_err(|error| Error::BuildTxError(error.to_string())). This improves maintainability and readability.Also applies to: 211-212, 250-251
244-245: Good standardization of AddUtxosError handling.The error mapping for UTXO operations is now consistently using
map_err_str, making the error propagation cleaner and more maintainable.Also applies to: 998-999, 1019-1020
343-344: Comprehensive fee calculation error handling improvement!All fee-related error mappings in the fee rate options methods have been systematically updated to use
map_err_str(Error::FeesError). This provides consistent error handling across all fee calculation paths.Also applies to: 347-348, 351-352, 402-403, 406-407, 410-411, 441-442, 445-446, 449-450
646-647: Clean PSBT finalization error handling.The
map_err_str(Error::PsbtFinalizeError)provides a cleaner alternative to the previous string formatting approach.
671-672: Node connection error handling simplified.Good use of
map_err_strfor the node connection check.
682-683: Consistent error handling for wallet operations.Balance retrieval and transaction fetching now use
map_err_strconsistently, improving error propagation clarity.Also applies to: 687-688, 1255-1256, 1261-1262
705-711: Elegant simplification of guard conditions!The refactored if-let chain
if let Some(last_scan) = self.last_scan_finished() && elapsed_secs_since(last_scan) < 15 && !force_scanis much cleaner than nested conditionals. This uses Rust's if-let chains feature effectively to combine multiple conditions in a single readable expression.
812-813: Historical prices and transaction details error handling improved.The error mapping for these operations is now consistent with the rest of the codebase using
map_err_str.Also applies to: 826-827
959-960: Weighted UTXOs error handling standardized.Good application of
map_err_strfor AddUtxosError in the weighted UTXOs retrieval.
praveenperera
force-pushed
the
result-ext
branch
from
26592bb to
aebfcd4
Compare
Summary by CodeRabbit
New Features
Refactor
Public API
Documentation