fix: replace expect()/unwrap() panics with error propagation#562
fix: replace expect()/unwrap() panics with error propagation#562PastaPastaPasta wants to merge 25 commits intov1.0-devfrom
Conversation
…t.rs data loading
…n with Result Changed dapi_address_list() and insight_api_uri() to return Result instead of panicking. Updated initialize_sdk() to propagate errors via Result<Sdk, String>. Updated callers and tests accordingly.
Replaced 13 .expect() calls in AppContext::new() with match blocks
that log errors and return None. Covers SPV/RPC provider init,
5 system data contract loads, cookie path, CoreClient creation, and
DB wallet queries. Also replaced panic!("unsupported network") in
default_platform_version() with safe fallback, and changed
initialize_sdk callers in update_core_rpc_config to propagate errors.
…cessing In received_asset_lock_finality(), replaced two .expect() calls with proper ? error propagation: credit_outputs.first() now returns an error if empty, and Address::from_script() maps its error to a descriptive rusqlite error. Malformed asset lock data no longer panics.
Replace unimplemented!("marketplace settings") with a UI label so
users see an informational message instead of a crash. Also reverts
read_or_recover/write_or_recover calls that were inadvertently
introduced during cherry-pick back to standard lock calls.
📝 WalkthroughWalkthroughWidespread replacement of panic-prone unwrap/expect/unimplemented! with Result/Option-based handling, mapped errors, guarded fallbacks, and warnings across app initialization, SDK/context creation, backend tasks, DB layers, and many UI time computations; several constructors (notably AppState and SDK) became fallible. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
PastaPastaPasta
requested review from
Copilot and
lklimek
and removed request for
lklimek
There was a problem hiding this comment.
Pull request overview
This PR hardens the application by removing panic-based error handling (expect/unwrap/panic!/unreachable!/unimplemented!) across the app, database, context initialization, SDK/config parsing, and UI, replacing it with Result/Option propagation, logging, and safe fallbacks to avoid crashing the GUI on malformed data or misconfiguration.
Changes:
- Convert several constructors/helpers (notably
AppState::new()andinitialize_sdk()) to returnResultand update call sites accordingly. - Replace many
SystemTime“time went backwards” panics with.unwrap_or_default()in UI screens and background tasks. - Improve DB/data parsing robustness by propagating errors instead of panicking; add warnings + safe defaults for unexpected stored values.
Reviewed changes
Copilot reviewed 55 out of 55 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/kittest/wallets_screen.rs | Updates harness construction to handle AppState::new() returning Result. |
| tests/kittest/startup.rs | Same: uses expect(...) on AppState::new() result in tests. |
| tests/kittest/network_chooser.rs | Same: updates test harness app initialization for new Result API. |
| tests/kittest/identities_screen.rs | Same: updates kittest harness initialization. |
| tests/kittest/create_asset_lock_screen.rs | Same: updates kittest harness initialization. |
| tests/e2e/wallet_flows.rs | Same: updates e2e harness initialization. |
| tests/e2e/navigation.rs | Same: updates e2e harness initialization. |
| src/ui/wallets/send_screen.rs | Replaces SystemTime panic with .unwrap_or_default(). |
| src/ui/tools/transition_visualizer_screen.rs | Replaces SystemTime panic with .unwrap_or_default() in status timing. |
| src/ui/tokens/update_token_config.rs | Replaces unimplemented!() with a non-crashing UI label. |
| src/ui/tokens/unfreeze_tokens_screen.rs | Replaces SystemTime panic with .unwrap_or_default() in status timing. |
| src/ui/tokens/transfer_tokens_screen.rs | Replaces SystemTime panic with .unwrap_or_default() in status timing. |
| src/ui/tokens/set_token_price_screen.rs | Replaces SystemTime panic with .unwrap_or_default() in status timing. |
| src/ui/tokens/resume_tokens_screen.rs | Replaces unwrap()/panic-prone SystemTime usage with .unwrap_or_default(). |
| src/ui/tokens/pause_tokens_screen.rs | Replaces unwrap()/panic-prone SystemTime usage with .unwrap_or_default(). |
| src/ui/tokens/mint_tokens_screen.rs | Replaces SystemTime panic with .unwrap_or_default() in status timing. |
| src/ui/tokens/freeze_tokens_screen.rs | Replaces SystemTime panic with .unwrap_or_default() in status timing. |
| src/ui/tokens/direct_token_purchase_screen.rs | Replaces SystemTime panic with .unwrap_or_default() in status timing. |
| src/ui/tokens/destroy_frozen_funds_screen.rs | Replaces SystemTime panic with .unwrap_or_default() in status timing. |
| src/ui/tokens/claim_tokens_screen.rs | Replaces unwrap()/panic-prone SystemTime usage with .unwrap_or_default(). |
| src/ui/tokens/burn_tokens_screen.rs | Replaces SystemTime panic with .unwrap_or_default() in status timing. |
| src/ui/network_chooser_screen.rs | Replaces SystemTime panic with .unwrap_or_default() for periodic checks. |
| src/ui/identities/withdraw_screen.rs | Replaces SystemTime panic with .unwrap_or_default() in status timing. |
| src/ui/identities/transfer_screen.rs | Replaces SystemTime panic with .unwrap_or_default() in status timing. |
| src/ui/identities/register_dpns_name_screen.rs | Replaces SystemTime panic with .unwrap_or_default() in status timing. |
| src/ui/identities/keys/add_key_screen.rs | Replaces SystemTime panic with .unwrap_or_default() in status timing. |
| src/ui/identities/add_existing_identity_screen.rs | Replaces SystemTime panic with .unwrap_or_default() in status timing. |
| src/ui/contracts_documents/update_contract_screen.rs | Replaces unwrap() on SystemTime with .unwrap_or_default() for elapsed display. |
| src/ui/contracts_documents/register_contract_screen.rs | Same: safer time handling for elapsed display. |
| src/ui/contracts_documents/group_actions_screen.rs | Replaces SystemTime panic with .unwrap_or_default() in status timing. |
| src/ui/contracts_documents/document_action_screen.rs | Replaces unwrap() on SystemTime with .unwrap_or_default() in multiple status paths. |
| src/ui/contracts_documents/contracts_documents_screen.rs | Replaces SystemTime panic with .unwrap_or_default() in query timing. |
| src/ui/contracts_documents/add_contracts_screen.rs | Replaces SystemTime panic with .unwrap_or_default() in status timing. |
| src/sdk_wrapper.rs | Changes initialize_sdk() to return Result and propagates build/config errors. |
| src/main.rs | Updates eframe app creation to propagate AppState::new() errors via ?. |
| src/database/wallet.rs | Replaces multiple panics with error propagation while decoding/parsing DB wallet data. |
| src/database/scheduled_votes.rs | Handles unexpected DB values safely (warn + default) instead of unreachable!(). |
| src/database/initialization.rs | Converts migration failure panic into a returned rusqlite error. |
| src/database/contested_names.rs | Propagates identifier parse errors; replaces time panics with .unwrap_or_default(). |
| src/database/contacts.rs | Avoids unwrap_or_default() on row.get(...) errors; propagates DB read errors instead. |
| src/context/transaction_processing.rs | Propagates errors for malformed asset lock transactions instead of panicking. |
| src/context/mod.rs | Makes AppContext::new() fail gracefully with logging; updates SDK init and network fallback behavior. |
| src/config.rs | Changes parsing helpers to return Result and updates unit tests accordingly (incomplete). |
| src/components/core_p2p_handler.rs | Replaces unsupported-network panic with a returned error. |
| src/backend_task/platform_info.rs | Converts formatting helpers to return Result and propagates errors instead of panicking. |
| src/backend_task/identity/top_up_identity.rs | Avoids panic when rebuilding transitions for debugging; returns richer error info. |
| src/backend_task/identity/register_identity.rs | Propagates identity creation errors; avoids panic in debug transition recreation. |
| src/backend_task/dashpay/payments.rs | Replaces SystemTime unwrap with .unwrap_or_default() for IDs/timestamps. |
| src/backend_task/dashpay/contacts.rs | Avoids panics on invalid identifiers; skips with optional warnings. |
| src/backend_task/contract.rs | Avoids panicking on missing token config; warns and skips invalid entries. |
| src/backend_task/contested_names/vote_on_dpns_name.rs | Propagates missing document type error instead of panicking. |
| src/backend_task/contested_names/query_dpns_vote_contenders.rs | Propagates missing document type error instead of panicking. |
| src/backend_task/contested_names/query_dpns_contested_resources.rs | Replaces multiple unwraps with safe handling + logging; improves async channel/semaphore error handling. |
| src/app_dir.rs | Replaces unimplemented!() for unsupported networks with a returned io::Error. |
| src/app.rs | Makes AppState::new() return Result; replaces hard exits/panics with propagated errors and graceful ZMQ fallback. |
Comments suppressed due to low confidence (1)
src/config.rs:487
NetworkConfig::insight_api_uri()now returnsResult<Uri, String>, so this unit test annotated with#[should_panic]will no longer panic and will fail. Update the test to assert that an empty URL returnsErr(and optionally check the error message) instead of expecting a panic.
#[test]
#[should_panic(expected = "invalid insight API URL")]
fn test_insight_api_uri_empty_panics() {
let config = make_network_config("https://127.0.0.1:443", "", 9998);
let _uri = config.insight_api_uri();
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ExtendedPubKey::decode(&master_ecdsa_bip44_account_0_epk_bytes).map_err(|e| { | ||
| rusqlite::Error::InvalidParameterName(format!( | ||
| "Failed to decode ExtendedPubKey: {}", | ||
| e | ||
| )) | ||
| })?; |
There was a problem hiding this comment.
These parse/decode failures are being mapped to rusqlite::Error::InvalidParameterName, which is intended for SQL parameter binding issues and can be misleading for callers/logging. Consider using a more appropriate rusqlite error variant (e.g., FromSqlConversionFailure with the relevant column index/type) to represent corrupted row data.
There was a problem hiding this comment.
Audit note (Claude Code): This comment is still unresolved.
Both ExtendedPubKey::decode (line 444) and seed hash conversion (line 451) still use rusqlite::Error::InvalidParameterName. The identical fix (switching to FromSqlConversionFailure) was applied in contested_names.rs but was not carried over to wallet.rs.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/ui/tokens/update_token_config.rs (1)
774-774:⚠️ Potential issue | 🟠 MajorRemaining
.expect()on line 774 can still panic at runtime.This PR's objective is to replace panic-prone calls with error propagation or safe fallbacks.
self.signing_key.clone().expect("Signing key must be set")will panic ifsigning_keyisNone. Consider guarding the button click or showing an error message instead.Suggested fix
- signing_key: self.signing_key.clone().expect("Signing key must be set"), + signing_key: match self.signing_key.clone() { + Some(key) => key, + None => { + self.error_message = Some("No signing key selected".to_string()); + return action; + } + },src/database/contacts.rs (1)
101-113:⚠️ Potential issue | 🟡 MinorInconsistent NULL handling with
load_contact_private_info.
load_contact_private_info(lines 76-78) now safely handles NULLnickname/notescolumns viaOption<String>, but this function still uses directrow.get::<_, String>which will returnInvalidColumnTypeerror on NULL values. Since the schema allows NULLs for these columns, apply the same pattern here for consistency.Proposed fix
.query_map(params![owner_identity_id.to_buffer().to_vec()], |row| { Ok(ContactPrivateInfo { owner_identity_id: row.get(0)?, contact_identity_id: row.get(1)?, - nickname: row.get(2)?, - notes: row.get(3)?, + nickname: row.get::<_, Option<String>>(2)?.unwrap_or_default(), + notes: row.get::<_, Option<String>>(3)?.unwrap_or_default(), is_hidden: row.get::<_, i32>(4)? != 0, }) })?src/backend_task/contested_names/query_dpns_contested_resources.rs (1)
30-31:⚠️ Potential issue | 🟡 MinorPre-existing bug:
retriescounter resets on every outer-loop iteration.
retriesis initialized to0at line 46 inside the paginationloop. When a retryable error triggerscontinue(line 115), control returns to line 33, re-executing line 46 and resettingretriesto0. The retry limit ofMAX_RETRIESis therefore never reached.Not introduced by this PR, but worth fixing separately — move
retriesbefore the outerloopor use an inner retry loop.Proposed fix
const MAX_RETRIES: usize = 3; + let mut retries = 0; let mut start_at_value = None; let mut names_to_be_updated = Vec::new(); loop { ... - // Initialize retry counter - let mut retries = 0; let contested_resources = match ContestedResource::fetch_many(sdk, query.clone()).awaitAlso applies to: 46-47, 101-116
src/database/contested_names.rs (1)
493-503:⚠️ Potential issue | 🟡 MinorRemaining
expect()/unwrap()calls ininsert_or_update_contenderspanic on malformed contender data.Four panic sites remain that process untrusted network data:
- Line 495:
.expect("expect a contender document deserialization")- Line 497:
.as_ref().unwrap().clone()- Line 501:
.expect("expected name")- Line 503:
.unwrap()onas_str()These should be converted to proper error propagation with
map_err/ok_orand?for consistency with the rest of the database layer and to avoid panics on malformed DPNS data.src/config.rs (1)
483-487:⚠️ Potential issue | 🟠 MajorUpdate the "empty insight URL" test to expect Err instead of panic
insight_api_uri()returnsResult<Uri, String>, so the#[should_panic]test will fail. Update it to assertis_err().🔧 Suggested fix
- #[test] - #[should_panic(expected = "invalid insight API URL")] - fn test_insight_api_uri_empty_panics() { + #[test] + fn test_insight_api_uri_empty_returns_error() { let config = make_network_config("https://127.0.0.1:443", "", 9998); - let _uri = config.insight_api_uri(); + assert!(config.insight_api_uri().is_err()); }src/ui/contracts_documents/update_contract_screen.rs (1)
275-285:⚠️ Potential issue | 🟡 MinorUse
saturating_subto guard against underflow in elapsed time calculationsThe
unwrap_or_default()call returnsDuration::ZEROifSystemTime::now()fails (e.g., clock issues), resulting innow = 0. Whenstart_timeis captured at a real timestamp,now - start_timewill underflow—panicking in debug mode and wrapping to a huge value in release mode. Usesaturating_subto safely handle this edge case.This affects the elapsed time calculations in:
- Line 281:
FetchingNoncematch arm- Line 293:
Broadcastingmatch arm- Line 305:
ProofErrormatch arm🔧 Suggested fix
- let elapsed = now - start_time; + let elapsed = now.saturating_sub(start_time);Apply the same change to the other two locations.
🤖 Fix all issues with AI agents
In `@src/database/contested_names.rs`:
- Around line 77-86: The code is using rusqlite::Error::InvalidParameterName for
column conversion failures; replace those with
rusqlite::Error::FromSqlConversionFailure carrying the column index, expected
Type::Blob, and Box::new(e) when Identifier::from_bytes fails (e.g., in the
map/transposed blocks that call Identifier::from_bytes), so the error
semantically represents a failed column-to-type conversion; also update all six
occurrences in this file to this pattern (matching other files like wallet.rs),
and in insert_or_update_contenders replace the .expect() calls that parse
documents and names with proper error propagation (return a
rusqlite::Error::FromSqlConversionFailure or convert the parse error into the
function's Result error) instead of panicking so callers receive errors rather
than aborting.
In `@src/ui/identities/withdraw_screen.rs`:
- Around line 596-600: The subtraction computing elapsed_seconds (now -
start_time) can underflow if
SystemTime::now().duration_since(UNIX_EPOCH).unwrap_or_default().as_secs()
returns 0; change the subtraction to use saturating_sub (i.e., let
elapsed_seconds = now.saturating_sub(start_time)) to avoid panics/wraps, and
apply the same safeguard where the start time is written/stored earlier (the
block around lines 261–264 that sets start_time) so that any subsequent
elapsed-time calculations also use saturating_sub and/or log a trace when now
defaults to 0 to aid diagnosis.
🟡 Minor comments (21)
src/backend_task/platform_info.rs-270-270 (1)
270-270:⚠️ Potential issue | 🟡 Minor
saturating_sub(0)is a no-op.
daily_withdrawal_limit.saturating_sub(0)always equalsdaily_withdrawal_limit. The comment notes the 24-hour amount isn't available, but this line is misleading — "Remaining Today" will always show the full daily limit.Consider either tracking the actual 24h withdrawn amount or labeling this as the daily limit directly to avoid confusing users.
src/ui/identities/keys/add_key_screen.rs-659-663 (1)
659-663:⚠️ Potential issue | 🟡 MinorUnsigned subtraction can still panic if
now < start_time.With
unwrap_or_default(), if the second call defaults to0whilestart_timeholds a real timestamp,now - start_timewill underflow and panic — defeating the purpose of removingexpect(). Usesaturating_subfor consistency with the defensive intent of this PR.Proposed fix
- let elapsed_seconds = now - start_time; + let elapsed_seconds = now.saturating_sub(*start_time);src/ui/tokens/destroy_frozen_funds_screen.rs-615-620 (1)
615-620:⚠️ Potential issue | 🟡 MinorPotential u64 underflow on
now - start_time.If
SystemTime::now().duration_since(UNIX_EPOCH)fails and defaults to 0, thennowis0whilestart_timeholds a real epoch timestamp. The subtractionnow - start_timewill panic in debug mode (overflow) or silently wrap in release mode.Use
saturating_subto guard against this:Proposed fix
- let elapsed = now - start_time; + let elapsed = now.saturating_sub(*start_time);src/ui/tokens/set_token_price_screen.rs-1143-1149 (1)
1143-1149:⚠️ Potential issue | 🟡 MinorPotential u64 underflow on
now - start_time.If
SystemTime::now().duration_since(UNIX_EPOCH)returnsErrhere (defaultingnowto 0) but succeeded whenstart_timewas recorded at line 731, the subtraction on line 1148 will panic due to unsigned integer underflow.Use
saturating_subto be consistent with the defensive intent of theunwrap_or_defaultchange.Proposed fix
- let elapsed = now - start_time; + let elapsed = now.saturating_sub(*start_time);src/ui/identities/add_existing_identity_screen.rs-454-458 (1)
454-458:⚠️ Potential issue | 🟡 Minor
unwrap_or_default()is a good replacement — but protect the subtraction at Line 1119.The four
unwrap_or_default()changes are a clear improvement overexpect(). However, ifnowever falls back to0(defaultDuration) whilestart_timeholds a real timestamp, Line 1119'snow - start_timewill underflow (u64subtraction). Usesaturating_subto keep this safe end-to-end.Proposed fix
- let elapsed_seconds = now - start_time; + let elapsed_seconds = now.saturating_sub(*start_time);Also applies to: 656-660, 813-817, 1115-1119
src/ui/tokens/mint_tokens_screen.rs-700-704 (1)
700-704:⚠️ Potential issue | 🟡 Minor
u64subtraction underflow whenunwrap_or_default()yields zero.If
SystemTime::now().duration_since(UNIX_EPOCH)fails here (returningDuration::ZERO→now = 0), butstart_timewas set to a real timestamp on line 302-306, thennow - start_timewill panic in debug or silently wrap in release. Usesaturating_subto avoid this.Proposed fix
- let elapsed = now - start_time; + let elapsed = now.saturating_sub(*start_time);src/ui/contracts_documents/register_contract_screen.rs-267-272 (1)
267-272:⚠️ Potential issue | 🟡 MinorGuard elapsed time against underflow.
If the clock moves backward (or defaults to zero),now - start_timecan panic or wrap; use saturating_sub instead.🔧 Suggested fix
- let elapsed = now - start_time; + let elapsed = now.saturating_sub(start_time);Also applies to: 278-283
src/ui/tokens/resume_tokens_screen.rs-518-524 (1)
518-524:⚠️ Potential issue | 🟡 MinorGuard elapsed time against underflow with correct syntax.
If the clock moves backward,now - start_timecan underflow and wrap to a large incorrect value. Use saturating subtraction to return 0 instead.🔧 Suggested fix
- let elapsed = now - start_time; + let elapsed = now.saturating_sub(start_time);src/ui/tokens/direct_token_purchase_screen.rs-640-645 (1)
640-645:⚠️ Potential issue | 🟡 MinorGuard elapsed time against underflow.
A backward clock can causenow - start_timeto underflow and panic. Usesaturating_sub()instead.🔧 Suggested fix
- let elapsed = now - start_time; + let elapsed = now.saturating_sub(start_time);src/ui/tokens/transfer_tokens_screen.rs-544-549 (1)
544-549:⚠️ Potential issue | 🟡 MinorGuard elapsed time against underflow using saturating arithmetic.
If the system clock is adjusted backward,nowcan be less thanstart_time, causing integer underflow and displaying incorrect elapsed time. Usesaturating_subinstead.🔧 Suggested fix
- let elapsed_seconds = now - start_time; + let elapsed_seconds = now.saturating_sub(*start_time);src/app_dir.rs-55-69 (1)
55-69:⚠️ Potential issue | 🟡 MinorRequire a devnet name when resolving the cookie path for Devnet networks.
When
Network::Devnetis used with aNoneor emptydevnet_name, the current code silently uses an empty string, which results in the cookie path pointing to the base.dashcoredirectory instead of a network-specific subdirectory. This masks misconfiguration errors and should be rejected.🔧 Suggested fix
- Network::Devnet => devnet_name.as_deref().unwrap_or(""), + Network::Devnet => devnet_name + .as_deref() + .filter(|s| !s.is_empty()) + .ok_or_else(|| { + io::Error::new(io::ErrorKind::InvalidInput, "Devnet name is required") + })?,src/ui/contracts_documents/add_contracts_screen.rs-369-374 (1)
369-374:⚠️ Potential issue | 🟡 MinorGuard elapsed time against underflow with correct syntax.
If the system clock goes backward,now - start_timecan panic; usenow.saturating_sub(start_time).🔧 Suggested fix
- let elapsed_seconds = now - start_time; + let elapsed_seconds = now.saturating_sub(start_time);src/ui/tools/transition_visualizer_screen.rs-265-270 (1)
265-270:⚠️ Potential issue | 🟡 MinorGuard elapsed time against underflow.
A backward clock (or zeroed duration) can makenow - start_timewrap; usesaturating_subto prevent this.🔧 Suggested fix
- let elapsed_seconds = now - start_time; + let elapsed_seconds = now.saturating_sub(start_time);src/ui/tokens/freeze_tokens_screen.rs-268-271 (1)
268-271:⚠️ Potential issue | 🟡 MinorHarden elapsed calculation against system time failures
At line 618,
now - start_timecan underflow if the secondSystemTime::now()call fails and returns 0 viaunwrap_or_default(). Usesaturating_subto safely handle this case.Suggested fix
- let elapsed = now - start_time; + let elapsed = now.saturating_sub(start_time);src/ui/tokens/burn_tokens_screen.rs-265-268 (1)
265-268:⚠️ Potential issue | 🟡 MinorGuard elapsed time against underflow when
nowdefaults to 0If the clock jumps before UNIX_EPOCH after
start_timeis set,nowbecomes 0 andnow - start_timecan underflow in debug mode (panic) or wrap in release mode. Prefersaturating_subfor elapsed calculations.✅ Suggested fix
- let elapsed = now - start_time; + let elapsed = now.saturating_sub(start_time);Also applies to: 656-659
src/ui/tokens/claim_tokens_screen.rs-206-209 (1)
206-209:⚠️ Potential issue | 🟡 MinorUse
saturating_subto prevent underflow in elapsed calculationWhen
SystemTime::now()fails,unwrap_or_default()yields 0. The subsequent subtractionnow - start_timeunderflows ifstart_timewas captured successfully, resulting in incorrect elapsed values.✅ Suggested fix
- let elapsed = now - start_time; + let elapsed = now.saturating_sub(start_time);Also applies to: 567-570
src/ui/tokens/pause_tokens_screen.rs-210-213 (1)
210-213:⚠️ Potential issue | 🟡 MinorPrevent elapsed-time underflow from clock rollback
When
SystemTime::now().duration_since(UNIX_EPOCH)fails due to a clock rollback,unwrap_or_default()returns 0, makingelapsed = now - start_timeunderflow ifstart_timewas set to a positive value earlier. Usesaturating_subto handle this safely:Suggested fix
- let elapsed = now - start_time; + let elapsed = now.saturating_sub(start_time);Also applies to: 519-523
src/ui/contracts_documents/group_actions_screen.rs-563-567 (1)
563-567:⚠️ Potential issue | 🟡 MinorUse saturating subtraction for elapsed time to prevent underflow
When
duration_since(UNIX_EPOCH)fails (e.g., system clock issues),unwrap_or_default()returnsDuration::ZERO, resulting innow = 0. Subtracting a non-zerostart_timefrom this causes an underflow. Usesaturating_subto safely handle this edge case, consistent with the pattern used in other UI screens likesend_screen.rs.Suggested fix
- let elapsed = now - start_time; + let elapsed = now.saturating_sub(*start_time);Also applies to: 598–601
src/ui/contracts_documents/document_action_screen.rs-383-387 (1)
383-387:⚠️ Potential issue | 🟡 MinorUse saturating_sub for elapsed time calculations to prevent underflow
When
SystemTime::now()defaults to a duration of 0 (viaunwrap_or_default()on error), the elapsed calculation can underflow. Applysaturating_subto each elapsed computation.✅ Suggested fix (apply to each elapsed calculation)
let elapsed = SystemTime::now() .duration_since(UNIX_EPOCH) .unwrap_or_default() .as_secs() - - start; + .saturating_sub(start);Applies to: 465–469, 523–528, 589–594, 939–943, 948–952
src/ui/contracts_documents/contracts_documents_screen.rs-212-215 (1)
212-215:⚠️ Potential issue | 🟡 MinorUse saturating_sub for elapsed time calculation to prevent potential underflow
If the first
SystemTime::now()call succeeds but a later call fails and returnsunwrap_or_default()(Duration of 0 → 0 seconds), the subtraction0 - start_timewill underflow. Usesaturating_subto handle this edge case defensively.🔧 Suggested fix
let time_elapsed = SystemTime::now() .duration_since(UNIX_EPOCH) .unwrap_or_default() .as_secs() - - start_time; + .saturating_sub(start_time);Also applies to: lines 391-408, 424-428
src/ui/tokens/unfreeze_tokens_screen.rs-270-274 (1)
270-274:⚠️ Potential issue | 🟡 MinorUse
saturating_subto prevent elapsed time underflowWhen
unwrap_or_default()returns0due toSystemTime::duration_since()failure, subtractingstart_timecan cause integer underflow. This results in a panic in debug mode or wrapped value in release mode. Usesaturating_subto clamp at zero instead.🔧 Suggested fix
- let elapsed = now - start_time; + let elapsed = now.saturating_sub(*start_time);Also applies to: 603-609
🧹 Nitpick comments (12)
src/ui/identities/register_dpns_name_screen.rs (1)
561-566: Usesaturating_subto guard against underflow.
now - start_timeon line 566 is unsigned arithmetic. Ifnowever resolves to 0 (via the new default) whilestart_timeholds a real timestamp, this will panic in debug or silently wrap in release. Usingsaturating_subis a trivial hardening that keeps the no-panic guarantee consistent.Proposed fix
- let elapsed_seconds = now - start_time; + let elapsed_seconds = now.saturating_sub(*start_time);src/backend_task/dashpay/contacts.rs (1)
309-315: Nit: log message includes length that is always 32.
decrypt_to_user_idreturns[u8; 32], sodecrypted_id.len()is invariantly 32. Consider logging a hex snippet of the bytes instead for more useful diagnostics:Suggested improvement
let Ok(contact_id) = Identifier::from_bytes(&decrypted_id) else { tracing::warn!( - "Failed to parse decrypted contact ID (length {}), skipping contact info entry", - decrypted_id.len() + "Failed to parse decrypted contact ID (bytes: {}), skipping contact info entry", + hex::encode(&decrypted_id) ); continue; };src/ui/network_chooser_screen.rs (1)
840-840: Remaining.expect()panics in UI code contradict PR objectives.Lines 840 and 860 still use
.expect("Expected to save db settings"), which can panic the UI thread if the DB write fails. Given this PR's goal of eliminating panic-prone calls, consider propagating or logging these errors instead.Suggested fix
- self.save().expect("Expected to save db settings"); + if let Err(e) = self.save() { + tracing::error!("Failed to save db settings: {e}"); + }Apply the same pattern at both lines 840 and 860.
Also applies to: 860-860
src/backend_task/platform_info.rs (1)
244-246: InconsistentAddress::from_scripterror handling between the two format functions.In
format_withdrawal_documents_with_daily_limit(Line 244), a failed address parse gracefully degrades to"Invalid Address: ...", while informat_withdrawal_documents_to_bare_info(Line 318) and in theRecentlyCompletedWithdrawalsbranch (Line 618), the same failure propagates as an error and aborts the entire listing.Consider aligning these: the graceful-degradation approach is arguably better UX for display-only formatting, since one bad address shouldn't prevent showing the rest of the withdrawals.
Also applies to: 318-319
src/ui/tokens/destroy_frozen_funds_screen.rs (2)
197-199: Remaining.expect()is inconsistent with the PR's goal.
load_local_qualified_identities().expect("Identities not loaded")can still panic. Consider propagating the error or falling back to an emptyVecwith an error message, consistent with the approach taken elsewhere in this PR.
306-306: Remaining.expect("No key selected")can panic on a user action.This is reached from
confirmation_ok()which is triggered by user interaction. Ifselected_keyisNone, this panics. Consider returning an error to the UI instead (e.g., setself.error_messageand returnAppAction::None), consistent with the PR's intent.src/ui/identities/add_existing_identity_screen.rs (1)
59-59: Pre-existingexpect()inload_testnet_nodes_from_ymlis inconsistent with this PR's goal.This function already uses
.ok()?for file I/O but then panics on malformed YAML via.expect("expected proper yaml"). Since this PR is specifically about removing panic-prone calls, consider returningNonehere too (e.g.,.ok()?).Proposed fix
fn load_testnet_nodes_from_yml(file_path: &str) -> Option<TestnetNodes> { let file_content = fs::read_to_string(file_path).ok()?; - serde_yaml_ng::from_str(&file_content).expect("expected proper yaml") + serde_yaml_ng::from_str(&file_content).ok() }src/backend_task/identity/register_identity.rs (1)
295-297: Remaining.expect()oncreate_identifier()could be converted to?for consistency with this PR.This is a data-dependent fallible call (not a lock-poisoning unwrap), so it's a natural candidate for the same treatment applied elsewhere in this PR.
Suggested fix
- let identity_id = asset_lock_proof - .create_identifier() - .expect("expected to create an identifier"); + let identity_id = asset_lock_proof + .create_identifier() + .map_err(|e| format!("Failed to create identifier from asset lock proof: {}", e))?;src/backend_task/contract.rs (1)
126-126:unwrap_or_default()ontoken_idsilently produces a zeroed identifier.If
token_id()returnsNone, the resultingTokenInfowill contain a default (all-zeros)Identifier, which could be confusing downstream (e.g., displayed in the UI or used in lookups). Consider matching onNoneand either skipping the token (consistent with the error handling above) or logging a warning.src/context/transaction_processing.rs (1)
243-254: Userusqlite::Error::UserFunctionErrorfor domain-level errors instead ofInvalidParameterName.
InvalidParameterNameis semantically meant for SQL parameter binding issues. Using it for "no credit outputs" or "address parsing failure" is misleading and inconsistent with the precedent insrc/database/wallet.rs, which convertsWalletErrorviaUserFunctionError(Box::new(err)).Suggested fix
- let first = payload.credit_outputs.first().ok_or_else(|| { - rusqlite::Error::InvalidParameterName( - "Asset lock transaction has no credit outputs".to_string(), - ) - })?; + let first = payload.credit_outputs.first().ok_or_else(|| { + rusqlite::Error::UserFunctionError(Box::from( + "Asset lock transaction has no credit outputs", + )) + })?; - let address = - Address::from_script(&first.script_pubkey, self.network).map_err(|e| { - rusqlite::Error::InvalidParameterName(format!( - "Failed to derive address from asset lock credit output script: {e}" - )) - })?; + let address = + Address::from_script(&first.script_pubkey, self.network).map_err(|e| { + rusqlite::Error::UserFunctionError(Box::from(format!( + "Failed to derive address from asset lock credit output script: {e}" + ))) + })?;src/database/contested_names.rs (1)
361-364: Silent error suppression differs from the rest of the file's approach.Here
.ok()swallows thefrom_byteserror, treating corruptawarded_tobytes asNone. The practical effect is benign — it forces a needless UPDATE that writes the correct value — but it's inconsistent with the other sites in this file that propagate conversion errors.A brief
tracing::warn!on the.ok()path would help diagnose corrupt DB data without changing the control flow.src/context/mod.rs (1)
604-612: Catch-all arm silently defaults unknown networks toPLATFORM_V11.The
_ => &PLATFORM_V11arm replaces what was likely apanic!/unreachable!. This is safer, but note that if a newNetworkvariant is introduced with a different platform version, this will silently apply the wrong version. The existing TODO on line 605 already tracks the longer-term fix (reading from the SDK at runtime). Consider adding atracing::warn!in the catch-all so unexpected variants don't go unnoticed.🔧 Optional: log a warning for unrecognized network variants
This can't be a
const fnif you add logging, so you'd need to decide whether theconstqualifier is more valuable here. An alternative is a code comment.- _ => &PLATFORM_V11, + other => { + // If we reach here, a new Network variant was added without updating this match. + // Defaulting to PLATFORM_V11 as a safe fallback. + tracing::warn!(?other, "Unknown network variant, defaulting to PLATFORM_V11"); + &PLATFORM_V11 + }
lklimek
left a comment
lklimek
left a comment
There was a problem hiding this comment.
See comments, ones prefixed with [HIGH] etc. are from Claude, others are mine.
Most important:
- We silently ignore errors, without notifying user that something went wrong. See https://github.com/dashpay/dash-evo-tool/pull/562/changes#r2797870053
- Copilot and rabbit also gave good feedback, please address AI's feedback before assigning ticket to me
- Tests red.
src/app.rs
Outdated
| if let Some(ctx) = self.testnet_app_context.as_ref() { | ||
| ctx | ||
| } else { | ||
| tracing::warn!("Testnet app context not available, falling back to mainnet"); |
There was a problem hiding this comment.
[HIGH] Silent mainnet fallback can cause fund loss
When the user selects Testnet/Devnet/Regtest but the corresponding AppContext is None (failed to initialize), this silently falls back to the mainnet context. The user believes they are on Testnet but all operations — wallet transactions, identity registration, token transfers — silently target mainnet with real funds.
The tracing::warn! is only visible in the log file, not in the UI.
Suggestion: Return Result or Option from current_app_context() so callers can display a visible error. Alternatively, prevent the user from selecting a network whose context failed to initialize (e.g., grey out the option in the network chooser). The same applies to Devnet (line 713), Regtest (line 719), and the catch-all (line 727).
There was a problem hiding this comment.
Audit note (Claude Code): This comment is still unresolved.
The log level was upgraded from warn! to error! and messages now include a "BUG:" prefix, which improves debuggability. However, the core concern remains: if this fallback triggers, the user silently operates on mainnet with no UI indication. The tracing::error! is only visible in log files.
The reviewer's suggestion to return Result/Option from current_app_context() or to prevent selecting unavailable networks (e.g. grey out the option) has not been implemented.
src/backend_task/platform_info.rs
Outdated
| Address::from_script(&output_script, network).expect("expected an address"); | ||
| format!( | ||
| let address = Address::from_script(&output_script, network) | ||
| .map_err(|e| format!("Failed to parse withdrawal address: {}", e))?; |
There was a problem hiding this comment.
[MEDIUM] Inconsistent error handling — one bad address aborts entire withdrawal display
format_withdrawal_documents_to_bare_info uses .map_err()? here, meaning one malformed address aborts the entire withdrawal list. Compare with format_withdrawal_documents_with_daily_limit (line 246) which gracefully degrades:
.unwrap_or_else(|e| format!("Invalid Address: {}", e))Consider using the same graceful approach so one bad document doesn't prevent showing the rest of the withdrawals.
There was a problem hiding this comment.
Audit note (Claude Code): This comment is still unresolved.
Line 618-622 still uses map_err(|e| format\!(...))?, meaning one malformed address aborts the entire withdrawal list. The two helper functions in the same file (format_withdrawal_documents_with_daily_limit at line 246 and format_withdrawal_documents_to_bare_info at line 318-320) both use the soft fallback pattern unwrap_or_else(|e| format\!("Invalid Address: {}", e)). The inconsistency remains.
Audit SummaryReviewed by: Claude Code with a 4-agent team:
The PR accomplishes its stated goal — replacing ~40 Key concern: silent mainnet fallback (HIGH)The most significant issue is the Issues outside the diff[HIGH] #[test]
fn test_insight_api_uri_empty_returns_error() {
let config = make_network_config("https://127.0.0.1:443", "", 9998);
assert!(config.insight_api_uri().is_err());
}[LOW] Inconsistent log level for avatar fetch failures. Positive observations
Findings Summary
Inline comments are posted as a separate pending review for in-diff findings. |
- [HIGH] Prevent silent mainnet fallback by guarding change_network() against switching to unavailable network contexts, and validate saved network on startup. Upgrade fallback logging to error level. - [HIGH] Fix incorrect should_panic test for insight_api_uri() which now returns Result instead of panicking. - [HIGH] Replace .ok() with proper error propagation for awarded_to Identifier parsing in contested_names.rs insert_or_update_name_contest. - [MEDIUM] Make withdrawal address error handling consistent between format_withdrawal_documents_to_bare_info and _with_daily_limit. - [MEDIUM] Restore panic for unsupported network in const fn default_platform_version to prevent silent misconfiguration. - [LOW] Add tracing::warn for Identifier::from_bytes failure in contacts.rs mutual contact matching for consistency. - [LOW] Downgrade avatar fetch failure log from error to warn in contact_profile_viewer.rs for consistency. - Replace remaining seed_hash .expect() panics with map_err in wallet.rs. - Use saturating_sub for elapsed time calculation in withdraw_screen.rs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/ui/identities/withdraw_screen.rs (1)
320-328:⚠️ Potential issue | 🟠 Major
refresh()still contains two.unwrap()calls that can panic.Given this PR's goal of eliminating
unwrap/expectpanics, these two calls are inconsistent:
load_local_qualified_identities().unwrap()— will panic if the DB query fails..find(...).unwrap()— will panic if the identity was deleted or is otherwise missing.A safer approach: propagate a user-visible error or silently keep the stale identity.
Suggested safer fallback
fn refresh(&mut self) { - self.identity = self - .app_context - .load_local_qualified_identities() - .unwrap() - .into_iter() - .find(|identity| identity.identity.id() == self.identity.identity.id()) - .unwrap(); - self.max_amount = self.identity.identity.balance(); + match self.app_context.load_local_qualified_identities() { + Ok(identities) => { + if let Some(updated) = identities + .into_iter() + .find(|i| i.identity.id() == self.identity.identity.id()) + { + self.identity = updated; + self.max_amount = self.identity.identity.balance(); + } else { + tracing::warn!("Identity no longer found during refresh"); + } + } + Err(e) => { + tracing::warn!("Failed to refresh identities: {}", e); + } + } }Based on learnings: "Error handling refactoring is needed across the Dash-EVO-Tool codebase, particularly to avoid panics with
.expect()and instead propagate errors properly."src/database/contested_names.rs (1)
502-512:⚠️ Potential issue | 🟠 MajorRemaining
.expect()/.unwrap()calls ininsert_or_update_contendersstill panic on data issues.Lines 504, 506, 510, and 512 use
.expect()and.unwrap()for document deserialization and field extraction. These are exactly the kind of panics this PR aims to eliminate. A malformed contender document or missing"label"field would crash the application.Proposed fix
- let deserialized_contender = contender - .try_to_contender(dpns_domain_document_type, app_context.platform_version()) - .expect("expect a contender document deserialization"); - - let document = deserialized_contender.document().as_ref().unwrap().clone(); - - let name = document - .get("label") - .expect("expected name") - .as_str() - .unwrap(); + let deserialized_contender = contender + .try_to_contender(dpns_domain_document_type, app_context.platform_version()) + .map_err(|e| { + rusqlite::Error::InvalidParameterName(format!( + "Failed to deserialize contender document: {}", + e + )) + })?; + + let document = deserialized_contender + .document() + .as_ref() + .ok_or_else(|| { + rusqlite::Error::InvalidParameterName( + "Contender has no document".to_string(), + ) + })? + .clone(); + + let name = document + .get("label") + .ok_or_else(|| { + rusqlite::Error::InvalidParameterName( + "Document missing 'label' field".to_string(), + ) + })? + .as_str() + .ok_or_else(|| { + rusqlite::Error::InvalidParameterName( + "'label' field is not a string".to_string(), + ) + })?;Based on learnings: "Error handling refactoring is needed across the Dash-EVO-Tool (DET) codebase, particularly to avoid panics with
.expect()and instead propagate errors properly using the?operator."src/database/wallet.rs (1)
864-870:⚠️ Potential issue | 🟡 MinorRemaining
.expect()on seed hash conversion in identity loading.This is the only seed hash conversion in
get_walletsthat still panics. All other seed hash conversions (lines 445, 525, 710, 805, 908) were converted tomap_err. This one was missed.Proposed fix
- let wallet_seed_hash_array: [u8; 32] = wallet_seed_hash - .try_into() - .expect("Seed hash should be 32 bytes"); + let wallet_seed_hash_array: [u8; 32] = wallet_seed_hash.try_into().map_err(|_| { + rusqlite::Error::InvalidParameterName( + "Seed hash should be 32 bytes".to_string(), + ) + })?;
🤖 Fix all issues with AI agents
In `@src/backend_task/platform_info.rs`:
- Line 270: The expression using daily_withdrawal_limit.saturating_sub(0) is a
no-op and incorrectly shows the full daily limit as "Remaining Today"; update
the code that computes or displays this value (the usage around
daily_withdrawal_limit and the dash_to_credits! conversion) to either remove the
"Remaining Today" output when 24h usage data is unavailable or replace it with a
clear placeholder/annotation such as "(24h usage data unavailable)"; ensure you
adjust any formatting logic that depends on daily_withdrawal_limit so it no
longer misleadingly subtracts zero.
- Around line 618-622: The Address::from_script call in the withdrawal
formatting code currently uses map_err(...)? which aborts on a single bad
address; change it to the soft-fallback pattern used in
format_withdrawal_documents_with_daily_limit and
format_withdrawal_documents_to_bare_info by replacing the map_err + ? handling
around Address::from_script(&output_script, self.network) so that it returns a
string like format!("Invalid Address: {}", e) (or similar) instead of
propagating the error, ensuring the document list still renders even if one
address is invalid.
🧹 Nitpick comments (5)
src/ui/identities/withdraw_screen.rs (1)
247-271: Two.expect()calls remain inshow_confirmation_popup.Lines 249 and 270 both use
.expect("Withdrawal amount should be present"). While these are partially guarded by the UI (the button is only enabled whenwithdrawal_amount.is_some()), a defensive approach would be more consistent with the PR's intent:Suggested safer handling
- "Are you sure you want to withdraw {} to {}", - self.withdrawal_amount - .as_ref() - .expect("Withdrawal amount should be present"), + "Are you sure you want to withdraw {} to {}", + self.withdrawal_amount + .as_ref() + .map(|a| a.to_string()) + .unwrap_or_else(|| "unknown".to_string()),For line 270, return an error status instead of panicking:
- let credits = self - .withdrawal_amount - .as_ref() - .expect("Withdrawal amount should be present") - .value() as u128; + let Some(amount) = self.withdrawal_amount.as_ref() else { + self.withdraw_from_identity_status = WithdrawFromIdentityStatus::ErrorMessage( + "No withdrawal amount specified".to_string(), + ); + self.confirmation_dialog = None; + return AppAction::None; + }; + let credits = amount.value() as u128;src/backend_task/platform_info.rs (1)
456-504: One bad document aborts the entire queued-withdrawal display.Because both
format_withdrawal_documents_with_daily_limitandformat_withdrawal_documents_to_bare_infousecollect::<Result<Vec<String>, String>>()?, a single corrupt or incomplete document will cause the entire view to return an error. For a read-only display of multiple independent documents, consider usingfilter_mapto skip/log bad entries and still show the valid ones.src/database/wallet.rs (2)
438-449: Inconsistentrusqlite::Errorvariant usage within this file.Lines 438-443 use
InvalidParameterNamefor data conversion errors, while lines 534-538 correctly useFromSqlConversionFailurefor the same category of error (converting DB data to a Rust type). These two patterns coexist in the same function (get_wallets), making the error handling inconsistent.Consider standardizing on
FromSqlConversionFailurefor all data conversion failures (ExtendedPubKey decoding, seed hash conversion, address parsing, etc.) since that's the semantically correct variant.InvalidParameterNameis for SQL named-parameter binding mismatches.
592-604: Non-idiomatic error skip pattern.
row.is_err()followed byrow?on the same binding is valid but unnecessarily indirect. A more idiomatic approach:Suggested fix
for row in address_rows { - if row.is_err() { - continue; - } - let ( + let Ok(( seed_array, address, derivation_path, balance, path_reference, path_type, total_received, - ) = row?; + )) = row else { + continue; + };src/app.rs (1)
1026-1029: Remaining.unwrap()onSystemTime— same pattern replaced with.unwrap_or_default()in 26 other files.This is an existing line not touched in this PR, but it's the exact same
duration_since(UNIX_EPOCH).unwrap()pattern being systematically replaced elsewhere. Worth addressing for consistency.Suggested fix
let current_time = SystemTime::now() .duration_since(SystemTime::UNIX_EPOCH) - .unwrap() + .unwrap_or_default() .as_millis() as u64;
src/backend_task/platform_info.rs
Outdated
| @@ -264,68 +268,74 @@ fn format_withdrawal_documents_with_daily_limit( | |||
| total_amount as f64 / (dash_to_credits!(1) as f64), | |||
| daily_withdrawal_limit as f64 / (dash_to_credits!(1) as f64), | |||
| daily_withdrawal_limit.saturating_sub(0) as f64 / (dash_to_credits!(1) as f64), // We don't have 24h amount | |||
There was a problem hiding this comment.
saturating_sub(0) is a no-op — "Remaining Today" always equals the daily limit.
This line subtracts zero, so it always displays the full daily limit as "remaining", which is misleading to users. The comment acknowledges the 24h amount isn't available. Consider either removing the "Remaining Today" line entirely or adding a note like "(24h usage data unavailable)".
🤖 Prompt for AI Agents
In `@src/backend_task/platform_info.rs` at line 270, The expression using
daily_withdrawal_limit.saturating_sub(0) is a no-op and incorrectly shows the
full daily limit as "Remaining Today"; update the code that computes or displays
this value (the usage around daily_withdrawal_limit and the dash_to_credits!
conversion) to either remove the "Remaining Today" output when 24h usage data is
unavailable or replace it with a clear placeholder/annotation such as "(24h
usage data unavailable)"; ensure you adjust any formatting logic that depends on
daily_withdrawal_limit so it no longer misleadingly subtracts zero.
There was a problem hiding this comment.
Audit note (Claude Code): This comment is still unresolved.
A TODO comment was added (// TODO: subtract actual 24h withdrawal amount when available) but the "Remaining Today" line still displays the full daily limit, which is misleading to users. Consider either removing the "Remaining Today" line or annotating the display with "(24h usage data unavailable)" until actual usage data is available.
| let address = | ||
| Address::from_script(&output_script, self.network) | ||
| .expect("expected an address"); | ||
| format!( | ||
| .map_err(|e| { | ||
| format!("Failed to parse withdrawal address: {}", e) | ||
| })?; |
There was a problem hiding this comment.
Inconsistent address error handling: hard fail here vs. soft fallback in helpers.
In format_withdrawal_documents_with_daily_limit (Line 244-246) and format_withdrawal_documents_to_bare_info (Line 318-320), an invalid address is handled with unwrap_or_else(|e| format!("Invalid Address: {}", e)) — the document is still displayed. Here, map_err + ? aborts the entire withdrawal list for a single bad address.
Pick one strategy. The soft fallback used in the helpers is more resilient for a display-only view.
Proposed fix: align with the soft-fallback pattern
let address =
Address::from_script(&output_script, self.network)
- .map_err(|e| {
- format!("Failed to parse withdrawal address: {}", e)
- })?;
+ .map(|addr| addr.to_string())
+ .unwrap_or_else(|e| format!("Invalid Address: {}", e));🤖 Prompt for AI Agents
In `@src/backend_task/platform_info.rs` around lines 618 - 622, The
Address::from_script call in the withdrawal formatting code currently uses
map_err(...)? which aborts on a single bad address; change it to the
soft-fallback pattern used in format_withdrawal_documents_with_daily_limit and
format_withdrawal_documents_to_bare_info by replacing the map_err + ? handling
around Address::from_script(&output_script, self.network) so that it returns a
string like format!("Invalid Address: {}", e) (or similar) instead of
propagating the error, ensuring the document list still renders even if one
address is invalid.
There was a problem hiding this comment.
Audit note (Claude Code): This comment is still unresolved.
Same issue as the lklimek comment above. The suggested fix (.map(|addr| addr.to_string()).unwrap_or_else(|e| format\!("Invalid Address: {}", e))) was not applied. Line 618-622 still uses hard-fail map_err + ?.
…aturating_sub(0) no-op Replace InvalidParameterName with FromSqlConversionFailure in contested_names.rs for all Identifier byte conversion errors, since these are column data conversion failures, not SQL parameter binding mismatches. Also remove the no-op saturating_sub(0) in platform_info.rs withdrawal display. Includes formatting fixes from cargo +nightly fmt. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/database/wallet.rs (4)
862-864:⚠️ Potential issue | 🟠 MajorMissed
.expect()— this will still panic on corrupted data.Every other
seed_hash.try_into()in this file was converted to.map_err(|_| rusqlite::Error::InvalidParameterName(...)), but this one in the identity-loading closure was missed. A corrupted or truncatedwalletcolumn in theidentitytable would crash the application.Proposed fix
- let wallet_seed_hash_array: [u8; 32] = wallet_seed_hash - .try_into() - .expect("Seed hash should be 32 bytes"); + let wallet_seed_hash_array: [u8; 32] = wallet_seed_hash.try_into().map_err(|_| { + rusqlite::Error::InvalidParameterName( + "Seed hash should be 32 bytes".to_string(), + ) + })?;Based on learnings: "Error handling refactoring is needed across the Dash-EVO-Tool (DET) codebase, particularly to avoid panics with
.expect()and instead propagate errors properly using the?operator."🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@src/database/wallet.rs` around lines 862 - 864, Replace the panic-causing .expect() on wallet_seed_hash.try_into() inside the identity-loading closure by mapping the conversion error to a rusqlite::Error and propagating it with ?; specifically change the wallet_seed_hash.try_into().expect(...) usage (where wallet_seed_hash_array and wallet_seed_hash are defined) to wallet_seed_hash.try_into().map_err(|_| rusqlite::Error::InvalidParameterName("invalid wallet seed hash length".into()))? so the closure returns a rusqlite::Result instead of panicking.
590-602:⚠️ Potential issue | 🟡 MinorSilent error swallowing — address row errors are skipped without logging.
When
row.is_err(), the error is silently discarded. This can hide data corruption or parsing issues that the operator should be aware of. At minimum, log before continuing.Also note the pattern is slightly odd: checking
is_err()then callingrow?afterward is redundant — if you reach line 602,rowis guaranteedOk, so?is a no-op unwrap.Proposed fix
for row in address_rows { - if row.is_err() { - continue; - } let ( seed_array, address, derivation_path, balance, path_reference, path_type, total_received, - ) = row?; + ) = match row { + Ok(v) => v, + Err(e) => { + tracing::warn!("Skipping wallet address row due to error: {}", e); + continue; + } + };🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@src/database/wallet.rs` around lines 590 - 602, The loop over address_rows currently swallows errors by continuing when row.is_err() and then uses row? redundantly; change it to explicitly match or use if let Ok((seed_array, address, derivation_path, balance, path_reference, path_type, total_received)) = row { ... } else { let err = row.unwrap_err(); log the error (including err) via the module's logger (e.g., error! or process_logger) with context like "failed to read address row" and continue } so errors are logged and the redundant is_err()/row? pattern is removed.
916-941:⚠️ Potential issue | 🟡 MinorInconsistent error handling: parse failures silently skip, but network validation aborts all.
Lines 917-919 use
if let Ok(...)chains that silently skip rows where the row extraction or address parsing fails. But line 921-928 propagatesrequire_networkerrors via?, aborting the entire wallet load.This means a malformed address string is silently ignored, but an address that parses but fails network validation kills the whole
get_wallets()call. Consider making these consistent — either skip-and-log both, or propagate both.🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@src/database/wallet.rs` around lines 916 - 941, The code currently skips rows when row extraction or Address::<NetworkUnchecked>::from_str fails but aborts the whole get_wallets() when address.require_network(...) returns Err; make the behavior consistent by not using ? there: replace the address.require_network(...).map_err(...)? call with a match or if let that logs the validation error (including address_str and the error) and continues the for loop so malformed or wrong-network addresses are skipped rather than aborting; ensure you still compute canonical_address via Wallet::canonical_address(&address_checked, *network) and insert into wallet.platform_address_info as before, keeping references to platform_rows, wallets_map and platform_address_info the same.
438-463:⚠️ Potential issue | 🟡 MinorReplace
InvalidParameterNamewithFromSqlConversionFailurefor data conversion errors, and convert the remaining.expect()at line 862.Throughout this file,
rusqlite::Error::InvalidParameterNameis used for errors when decoding/converting data read from the database (e.g.,ExtendedPubKey::decode,seed_hash.try_into(),Address::from_str(),deserialize()calls). This is semantically incorrect—InvalidParameterNameis for SQL parameter binding failures, not column value conversion failures.
rusqlite::Error::FromSqlConversionFailure(column_index, sql_type, error)is the appropriate variant. Align all ~19 occurrences with the pattern already used elsewhere in the file (see lines withType::Text,Type::Integer).Additionally, line 862-864 still has
.expect("Seed hash should be 32 bytes")in production code, which contradicts the PR's goal of eliminating panics:Remaining .expect() to convert
let wallet_seed_hash_array: [u8; 32] = wallet_seed_hash .try_into() .expect("Seed hash should be 32 bytes");Convert to:
let wallet_seed_hash_array: [u8; 32] = wallet_seed_hash.try_into().map_err(|_| { rusqlite::Error::FromSqlConversionFailure( 1, rusqlite::types::Type::Blob, Box::new(std::io::Error::new(std::io::ErrorKind::InvalidData, "Seed hash should be 32 bytes")), ) })?;🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@src/database/wallet.rs` around lines 438 - 463, Replace inappropriate rusqlite::Error::InvalidParameterName usages used for data conversion failures (e.g., around ExtendedPubKey::decode, seed_hash.try_into(), Address::from_str(), deserialize() calls) with rusqlite::Error::FromSqlConversionFailure supplying the column index, the expected rusqlite::types::Type (e.g., Type::Blob or Type::Text/Integer as appropriate), and Box::new(the underlying error) to match the pattern used elsewhere; also change the remaining panic in the wallet_seed_hash_array conversion (the .expect("Seed hash should be 32 bytes") around wallet_seed_hash.try_into()) to map_err(...) returning rusqlite::Error::FromSqlConversionFailure with Type::Blob and an io::Error describing "Seed hash should be 32 bytes". Ensure all ~19 occurrences follow this pattern and use the same error construction style found in other parts of the file.
🤖 Fix all issues with AI agents
Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@src/app.rs`:
- Around line 719-762: current_app_context silently falls back to mainnet on
unreachable branches which risks running the UI on mainnet without user notice;
change current_app_context to return a Result<&Arc<AppContext>, NetworkFallback>
(or include an accompanying Option/enum indicating a fallback) so callers are
notified when a fallback occurred, and populate that error/indicator when the
code currently logs the "BUG:" cases (use the existing fields chosen_network,
mainnet_app_context, testnet_app_context, devnet_app_context, local_app_context
to detect the fallback). Update callers to surface the error to the UI (or set
chosen_network appropriately at a mutable callsite) so the user can be informed
instead of silently operating on mainnet.
In `@src/database/contested_names.rs`:
- Around line 19-179: Both get_all_contested_names and
get_ongoing_contested_names duplicate the large row-mapping closure; extract
that logic into a shared helper (e.g., map_contested_row) that accepts a
rusqlite::Row (or &Row), a mutable reference to the contested_name_map
(HashMap<String, ContestedName>), and contest_duration (and any small context
like network if needed) and returns rusqlite::Result<()>; move identifier
parsing (Identifier::from_bytes), state computation (locked/awarded/created_at
-> ContestState), and contestant construction into that helper, then call it
from each query’s query_map closure to avoid duplication and centralize error
handling and mapping logic for get_all_contested_names and
get_ongoing_contested_names.
In `@src/database/wallet.rs`:
- Around line 660-667: The UTXO address parsing uses
Address::from_str(...).assume_checked() and skips the network validation done
for wallet addresses by check_address_for_network(); update the UTXO loading
path to parse the string into an Address, call
check_address_for_network(&address) (or the same validation helper used at line
566) and propagate an error if the network mismatches before calling
.assume_checked(), ensuring the mapped rusqlite::Error includes the validation
failure context.
- Around line 862-864: Replace the panic-causing .expect() on
wallet_seed_hash.try_into() inside the identity-loading closure by mapping the
conversion error to a rusqlite::Error and propagating it with ?; specifically
change the wallet_seed_hash.try_into().expect(...) usage (where
wallet_seed_hash_array and wallet_seed_hash are defined) to
wallet_seed_hash.try_into().map_err(|_|
rusqlite::Error::InvalidParameterName("invalid wallet seed hash
length".into()))? so the closure returns a rusqlite::Result instead of
panicking.
- Around line 590-602: The loop over address_rows currently swallows errors by
continuing when row.is_err() and then uses row? redundantly; change it to
explicitly match or use if let Ok((seed_array, address, derivation_path,
balance, path_reference, path_type, total_received)) = row { ... } else { let
err = row.unwrap_err(); log the error (including err) via the module's logger
(e.g., error! or process_logger) with context like "failed to read address row"
and continue } so errors are logged and the redundant is_err()/row? pattern is
removed.
- Around line 916-941: The code currently skips rows when row extraction or
Address::<NetworkUnchecked>::from_str fails but aborts the whole get_wallets()
when address.require_network(...) returns Err; make the behavior consistent by
not using ? there: replace the address.require_network(...).map_err(...)? call
with a match or if let that logs the validation error (including address_str and
the error) and continues the for loop so malformed or wrong-network addresses
are skipped rather than aborting; ensure you still compute canonical_address via
Wallet::canonical_address(&address_checked, *network) and insert into
wallet.platform_address_info as before, keeping references to platform_rows,
wallets_map and platform_address_info the same.
- Around line 438-463: Replace inappropriate
rusqlite::Error::InvalidParameterName usages used for data conversion failures
(e.g., around ExtendedPubKey::decode, seed_hash.try_into(), Address::from_str(),
deserialize() calls) with rusqlite::Error::FromSqlConversionFailure supplying
the column index, the expected rusqlite::types::Type (e.g., Type::Blob or
Type::Text/Integer as appropriate), and Box::new(the underlying error) to match
the pattern used elsewhere; also change the remaining panic in the
wallet_seed_hash_array conversion (the .expect("Seed hash should be 32 bytes")
around wallet_seed_hash.try_into()) to map_err(...) returning
rusqlite::Error::FromSqlConversionFailure with Type::Blob and an io::Error
describing "Seed hash should be 32 bytes". Ensure all ~19 occurrences follow
this pattern and use the same error construction style found in other parts of
the file.
🧹 Nitpick comments (2)
🤖 Fix all nitpicks with AI agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@src/database/contested_names.rs`: - Around line 19-179: Both get_all_contested_names and get_ongoing_contested_names duplicate the large row-mapping closure; extract that logic into a shared helper (e.g., map_contested_row) that accepts a rusqlite::Row (or &Row), a mutable reference to the contested_name_map (HashMap<String, ContestedName>), and contest_duration (and any small context like network if needed) and returns rusqlite::Result<()>; move identifier parsing (Identifier::from_bytes), state computation (locked/awarded/created_at -> ContestState), and contestant construction into that helper, then call it from each query’s query_map closure to avoid duplication and centralize error handling and mapping logic for get_all_contested_names and get_ongoing_contested_names. In `@src/database/wallet.rs`: - Around line 660-667: The UTXO address parsing uses Address::from_str(...).assume_checked() and skips the network validation done for wallet addresses by check_address_for_network(); update the UTXO loading path to parse the string into an Address, call check_address_for_network(&address) (or the same validation helper used at line 566) and propagate an error if the network mismatches before calling .assume_checked(), ensuring the mapped rusqlite::Error includes the validation failure context.src/database/contested_names.rs (1)
19-179: Consider extracting the shared row-mapping logic.
get_all_contested_namesandget_ongoing_contested_nameshave nearly identical ~80-line row-mapping closures (identifier parsing, state computation, contestant construction). The only difference is the SQLWHEREclause and parameters. Extracting the common row-processing logic into a helper would reduce the surface area for divergence and make the new error-handling patterns easier to maintain in one place.Also applies to: 181-349
🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@src/database/contested_names.rs` around lines 19 - 179, Both get_all_contested_names and get_ongoing_contested_names duplicate the large row-mapping closure; extract that logic into a shared helper (e.g., map_contested_row) that accepts a rusqlite::Row (or &Row), a mutable reference to the contested_name_map (HashMap<String, ContestedName>), and contest_duration (and any small context like network if needed) and returns rusqlite::Result<()>; move identifier parsing (Identifier::from_bytes), state computation (locked/awarded/created_at -> ContestState), and contestant construction into that helper, then call it from each query’s query_map closure to avoid duplication and centralize error handling and mapping logic for get_all_contested_names and get_ongoing_contested_names.src/database/wallet.rs (1)
660-667: UTXO addresses skip network validation unlike wallet addresses.At line 566, wallet addresses go through
check_address_for_network(), but UTXO addresses at line 667 use.assume_checked()without network validation. If the DB contains a UTXO with an address for the wrong network, it would be silently accepted.This is likely safe since UTXOs are filtered by network in SQL and were validated at insertion time, but flagging the inconsistency.
🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@src/database/wallet.rs` around lines 660 - 667, The UTXO address parsing uses Address::from_str(...).assume_checked() and skips the network validation done for wallet addresses by check_address_for_network(); update the UTXO loading path to parse the string into an Address, call check_address_for_network(&address) (or the same validation helper used at line 566) and propagate an error if the network mismatches before calling .assume_checked(), ensuring the mapped rusqlite::Error includes the validation failure context.
| pub fn current_app_context(&self) -> &Arc<AppContext> { | ||
| // Note: change_network() guards against switching to unavailable networks, | ||
| // so the fallback branches below should never be reached in practice. | ||
| // They exist as defense-in-depth and log at error level to aid debugging. | ||
| match self.chosen_network { | ||
| Network::Dash => &self.mainnet_app_context, | ||
| Network::Testnet => self.testnet_app_context.as_ref().expect("expected testnet"), | ||
| Network::Devnet => self.devnet_app_context.as_ref().expect("expected devnet"), | ||
| Network::Regtest => self.local_app_context.as_ref().expect("expected local"), | ||
| _ => todo!(), | ||
| Network::Testnet => { | ||
| if let Some(ctx) = self.testnet_app_context.as_ref() { | ||
| ctx | ||
| } else { | ||
| tracing::error!( | ||
| "BUG: Testnet app context not available but network is set to Testnet. Falling back to mainnet to avoid crash." | ||
| ); | ||
| &self.mainnet_app_context | ||
| } | ||
| } | ||
| Network::Devnet => { | ||
| if let Some(ctx) = self.devnet_app_context.as_ref() { | ||
| ctx | ||
| } else { | ||
| tracing::error!( | ||
| "BUG: Devnet app context not available but network is set to Devnet. Falling back to mainnet to avoid crash." | ||
| ); | ||
| &self.mainnet_app_context | ||
| } | ||
| } | ||
| Network::Regtest => { | ||
| if let Some(ctx) = self.local_app_context.as_ref() { | ||
| ctx | ||
| } else { | ||
| tracing::error!( | ||
| "BUG: Local/Regtest app context not available but network is set to Regtest. Falling back to mainnet to avoid crash." | ||
| ); | ||
| &self.mainnet_app_context | ||
| } | ||
| } | ||
| _ => { | ||
| tracing::error!( | ||
| "BUG: Unknown network variant {:?} in current_app_context. Falling back to mainnet to avoid crash.", | ||
| self.chosen_network | ||
| ); | ||
| &self.mainnet_app_context | ||
| } | ||
| } |
There was a problem hiding this comment.
Defense-in-depth mainnet fallback is reasonable, but consider surfacing the error to the user.
The "BUG:" prefix and error! log level are appropriate since these branches should be unreachable given the guards in change_network() and AppState::new(). However, if this fallback does trigger, the user would silently operate on mainnet with no UI indication — only a log entry they're unlikely to see.
Consider whether these error paths should also set self.chosen_network = Network::Dash (they can't here since &self is immutable) or whether the caller should be notified. This aligns with the audit finding H1 (silent mainnet fallback risk). The current approach is acceptable as defense-in-depth, but a future improvement could surface this to the UI.
🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@src/app.rs` around lines 719 - 762, current_app_context silently falls back
to mainnet on unreachable branches which risks running the UI on mainnet without
user notice; change current_app_context to return a Result<&Arc<AppContext>,
NetworkFallback> (or include an accompanying Option/enum indicating a fallback)
so callers are notified when a fallback occurred, and populate that
error/indicator when the code currently logs the "BUG:" cases (use the existing
fields chosen_network, mainnet_app_context, testnet_app_context,
devnet_app_context, local_app_context to detect the fallback). Update callers to
surface the error to the UI (or set chosen_network appropriately at a mutable
callsite) so the user can be informed instead of silently operating on mainnet.
There was a problem hiding this comment.
Audit note (Claude Code): This comment is still unresolved.
Same concern as the lklimek comment on current_app_context(). The defense-in-depth approach with error! logging is reasonable, but no UI-visible notification has been added. If the fallback triggers, the user would silently operate on mainnet.
Keep the more descriptive warning message from the PR branch that includes the contested_name context.
…nflict merge: resolve v1.0-dev conflict in scheduled_votes.rs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/database/wallet.rs (3)
595-607:⚠️ Potential issue | 🟡 MinorSilent error swallowing is inconsistent with the rest of
get_wallets.Steps 4–8 propagate row errors via
row?, but step 3 silently skips them withcontinue. A corrupted or invalid address row will be silently dropped, potentially hiding data issues. This is especially inconsistent given the function's doc comment about stopping on the first corrupted blob (line 412–415).Proposed fix — propagate consistently
for row in address_rows { - if row.is_err() { - continue; - } let ( seed_array, address, derivation_path, balance, path_reference, path_type, total_received, ) = row?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/database/wallet.rs` around lines 595 - 607, The loop over address_rows in get_wallets silently skips Err rows (using if row.is_err() { continue; }) which is inconsistent with the function's error propagation; remove the manual is_err check and let the row? operator propagate errors for the tuple deconstruction (seed_array, address, derivation_path, balance, path_reference, path_type, total_received) so any corrupted/invalid address row returns an Err instead of being dropped; update the loop that iterates address_rows and any related control flow to rely on row? for consistent error handling.
790-790:⚠️ Potential issue | 🟡 MinorDuplicate step number in trace log.
Line 779 is "step 7" and line 790 is also "step 7". Should be "step 8" here (subsequent steps at lines 854 and 906 are already labeled 8 and 9).
Fix
- tracing::trace!("step 7: load wallet transactions for each wallet"); + tracing::trace!("step 8: load wallet transactions for each wallet");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/database/wallet.rs` at line 790, The trace log in src/database/wallet.rs incorrectly repeats "step 7" — update the tracing::trace! call that currently reads "step 7: load wallet transactions for each wallet" to "step 8: load wallet transactions for each wallet" so step numbering is sequential with the later "step 8" and "step 9" logs; modify the string literal passed to tracing::trace! accordingly.
866-869:⚠️ Potential issue | 🟠 MajorRemaining
.expect()not converted to error propagation.Line 869 still has
.expect("Seed hash should be 32 bytes")which will panic on corrupted data, contradicting this PR's goal. Every other seed hash conversion in this file was converted to.map_err(...).Proposed fix
- let wallet_seed_hash_array: [u8; 32] = wallet_seed_hash - .try_into() - .expect("Seed hash should be 32 bytes"); + let wallet_seed_hash_array: [u8; 32] = wallet_seed_hash + .try_into() + .map_err(|_| { + rusqlite::Error::InvalidParameterName( + "Seed hash should be 32 bytes".to_string(), + ) + })?;Based on learnings: "Error handling refactoring is needed across the Dash-EVO-Tool (DET) codebase, particularly to avoid panics with
.expect()and instead propagate errors properly using the?operator."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/database/wallet.rs` around lines 866 - 869, Replace the panic-causing .expect on converting wallet_seed_hash into wallet_seed_hash_array with proper error propagation: change the expression that creates wallet_seed_hash_array (the .try_into() call on wallet_seed_hash) to map the conversion error into the function's error type and propagate it with ? (e.g., .try_into().map_err(|e| /* convert to appropriate error: WalletError/DbError */ )?), so the code returns an Err instead of panicking; update the error mapping to use the same error variant pattern used elsewhere in wallet.rs for seed hash conversions to keep consistency.
🧹 Nitpick comments (3)
src/ui/network_chooser_screen.rs (1)
889-889: Pre-existing.expect()calls remain in this file.Lines 889, 909, and 959 still use
self.save().expect("Expected to save db settings"), which can panic if the DB write fails. These aren't part of this diff, but since the PR's goal is eliminating panic-prone calls, consider converting them to log-and-warn in a follow-up.// e.g. replace: self.save().expect("Expected to save db settings"); // with: if let Err(e) = self.save() { tracing::error!("Failed to save db settings: {e}"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/network_chooser_screen.rs` at line 889, Replace the panic-prone calls to self.save().expect("Expected to save db settings") (occurring in methods that call save()) with non-panicking error handling: call self.save(), match or if-let on the Result, and log the error via tracing::error! (or tracing::warn!) with a descriptive message including the error (e.g., "Failed to save db settings: {e}"), then continue without panicking; ensure you update all occurrences (the repeated self.save() calls in this module) and keep the save() call semantics otherwise unchanged.src/ui/identities/withdraw_screen.rs (1)
264-288: Two.expect()calls remain inshow_confirmation_popup.Lines 266 and 287 still use
.expect("Withdrawal amount should be present"). While they're guarded by the UI (the Withdraw button is only enabled whenwithdrawal_amount.is_some()), this is inconsistent with the PR's goal of eliminating panics. Consider using early-return with an error status instead.Proposed fix
- format!( - "Are you sure you want to withdraw {} to {}", - self.withdrawal_amount - .as_ref() - .expect("Withdrawal amount should be present"), - message_address - ), + { + let Some(amount) = self.withdrawal_amount.as_ref() else { + self.withdraw_from_identity_status = WithdrawFromIdentityStatus::ErrorMessage( + "No withdrawal amount specified".to_string(), + ); + self.confirmation_dialog = None; + return AppAction::None; + }; + format!( + "Are you sure you want to withdraw {} to {}", + amount, message_address + ) + },And similarly for line 284-288:
- let credits = self - .withdrawal_amount - .as_ref() - .expect("Withdrawal amount should be present") - .value() as u128; + let Some(amount) = self.withdrawal_amount.as_ref() else { + self.withdraw_from_identity_status = WithdrawFromIdentityStatus::ErrorMessage( + "No withdrawal amount specified".to_string(), + ); + self.confirmation_dialog = None; + return AppAction::None; + }; + let credits = amount.value() as u128;Based on learnings: "Error handling refactoring is needed across the Dash-EVO-Tool (DET) codebase, particularly to avoid panics with
.expect()and instead propagate errors properly."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/identities/withdraw_screen.rs` around lines 264 - 288, In show_confirmation_popup, remove the two .expect("Withdrawal amount should be present") calls on self.withdrawal_amount and instead early-return if the amount is missing: use if let Some(amount) = self.withdrawal_amount.as_ref() { ... } to capture amount.value() for credits and proceed to set self.withdraw_from_identity_status = WithdrawFromIdentityStatus::WaitingForResult(now); otherwise clear self.confirmation_dialog (and optionally set an error/idle status) and return so the UI cannot panic when withdrawal_amount is None.src/database/wallet.rs (1)
443-448:InvalidParameterNameis semantically misleading for data corruption errors.Throughout this file,
rusqlite::Error::InvalidParameterNameis used to wrap deserialization/validation failures. This error variant is intended for SQL parameter binding issues, not data integrity errors. Consider usingFromSqlConversionFailure(as done elsewhere in this file, e.g., line 537) or the customWalletError→UserFunctionErrorpath already defined at line 1249, which would provide a more accurate error surface.Not blocking since the pattern is consistent within this PR, but it may confuse callers trying to distinguish parameter errors from data corruption.
Also applies to: 450-452, 464-468
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/database/wallet.rs` around lines 443 - 448, Replace uses of rusqlite::Error::InvalidParameterName wrapping deserialization/validation failures (e.g., the ExtendedPubKey::decode call on master_ecdsa_bip44_account_0_epk_bytes) with a semantically appropriate error variant such as rusqlite::Error::FromSqlConversionFailure or route the failure into the crate's WalletError → UserFunctionError path used elsewhere (see the WalletError/UserFunctionError usage pattern), preserving the original error message and source; update the map_err closures around ExtendedPubKey::decode and the other occurrences noted (lines near the other decode/validation sites) to construct FromSqlConversionFailure with the offending value/type and Box::new(original_error) or convert into WalletError::UserFunctionError as appropriate so callers can distinguish data corruption from SQL parameter errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/database/wallet.rs`:
- Around line 595-607: The loop over address_rows in get_wallets silently skips
Err rows (using if row.is_err() { continue; }) which is inconsistent with the
function's error propagation; remove the manual is_err check and let the row?
operator propagate errors for the tuple deconstruction (seed_array, address,
derivation_path, balance, path_reference, path_type, total_received) so any
corrupted/invalid address row returns an Err instead of being dropped; update
the loop that iterates address_rows and any related control flow to rely on row?
for consistent error handling.
- Line 790: The trace log in src/database/wallet.rs incorrectly repeats "step 7"
— update the tracing::trace! call that currently reads "step 7: load wallet
transactions for each wallet" to "step 8: load wallet transactions for each
wallet" so step numbering is sequential with the later "step 8" and "step 9"
logs; modify the string literal passed to tracing::trace! accordingly.
- Around line 866-869: Replace the panic-causing .expect on converting
wallet_seed_hash into wallet_seed_hash_array with proper error propagation:
change the expression that creates wallet_seed_hash_array (the .try_into() call
on wallet_seed_hash) to map the conversion error into the function's error type
and propagate it with ? (e.g., .try_into().map_err(|e| /* convert to appropriate
error: WalletError/DbError */ )?), so the code returns an Err instead of
panicking; update the error mapping to use the same error variant pattern used
elsewhere in wallet.rs for seed hash conversions to keep consistency.
---
Nitpick comments:
In `@src/database/wallet.rs`:
- Around line 443-448: Replace uses of rusqlite::Error::InvalidParameterName
wrapping deserialization/validation failures (e.g., the ExtendedPubKey::decode
call on master_ecdsa_bip44_account_0_epk_bytes) with a semantically appropriate
error variant such as rusqlite::Error::FromSqlConversionFailure or route the
failure into the crate's WalletError → UserFunctionError path used elsewhere
(see the WalletError/UserFunctionError usage pattern), preserving the original
error message and source; update the map_err closures around
ExtendedPubKey::decode and the other occurrences noted (lines near the other
decode/validation sites) to construct FromSqlConversionFailure with the
offending value/type and Box::new(original_error) or convert into
WalletError::UserFunctionError as appropriate so callers can distinguish data
corruption from SQL parameter errors.
In `@src/ui/identities/withdraw_screen.rs`:
- Around line 264-288: In show_confirmation_popup, remove the two
.expect("Withdrawal amount should be present") calls on self.withdrawal_amount
and instead early-return if the amount is missing: use if let Some(amount) =
self.withdrawal_amount.as_ref() { ... } to capture amount.value() for credits
and proceed to set self.withdraw_from_identity_status =
WithdrawFromIdentityStatus::WaitingForResult(now); otherwise clear
self.confirmation_dialog (and optionally set an error/idle status) and return so
the UI cannot panic when withdrawal_amount is None.
In `@src/ui/network_chooser_screen.rs`:
- Line 889: Replace the panic-prone calls to self.save().expect("Expected to
save db settings") (occurring in methods that call save()) with non-panicking
error handling: call self.save(), match or if-let on the Result, and log the
error via tracing::error! (or tracing::warn!) with a descriptive message
including the error (e.g., "Failed to save db settings: {e}"), then continue
without panicking; ensure you update all occurrences (the repeated self.save()
calls in this module) and keep the save() call semantics otherwise unchanged.
lklimek
left a comment
lklimek
left a comment
There was a problem hiding this comment.
Not all comments are resolved. I manually checked some of them, and they are still there. See comments.
3 participants
Summary
.expect()/.unwrap()/panic!()/unreachable!()/unimplemented!()calls across backend, database, context, and UI layers with proper error propagation or safe fallbacksResultinstead of panicking.expect()calls replaced with match blocks that log errors and returnNone, so a misconfigured network context fails gracefully instead of taking down the whole appdapi_address_list(),insight_api_uri(), andinitialize_sdk()now returnResultinstead of panicking on bad inputSystemTime::now().duration_since(UNIX_EPOCH).expect()replaced with.unwrap_or_default()across 26 files;unimplemented!()in marketplace settings replaced with informational labelMotivation
The application had numerous panic-prone code paths that could crash the entire GUI if any upstream data was malformed, a network was misconfigured, or a database query returned unexpected results. This PR systematically eliminates these crash vectors by:
Result/Optionfrom fallible functionsmatchblocks withtracing::error!logging before returningNone.unwrap_or_default()for non-critical timestamp operationsunreachable!()with safe default branchesTest plan
cargo buildpassescargo clippy --all-features --all-targets -- -D warningspasses cleancargo +nightly fmt --allapplied🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
UI
Tests