fix(database): resolve remaining error-handling inconsistencies - #614
Conversation
📝 WalkthroughWalkthroughThis PR replaces panic-prone unwrap/expect calls with explicit error propagation in database code, standardizes rusqlite error variants to use FromSqlConversionFailure with CorruptedBlobError, adds tests for malformed wallet rows, and changes the "Remaining Today" withdrawal display to indicate 24h usage data is unavailable. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/database/wallet.rs (1)
881-883: Remaining.expect()on wallet seed hash in identity loading path.This
try_into().expect(...)performs the same[u8; 32]conversion that was just fixed withFromSqlConversionFailureon line 454. A corruptedwalletcolumn in theidentitytable would panic here.Suggested fix for consistency
let wallet_seed_hash_array: [u8; 32] = wallet_seed_hash .try_into() - .expect("Seed hash should be 32 bytes"); + .map_err(|_| { + rusqlite::Error::FromSqlConversionFailure( + 1, + rusqlite::types::Type::Blob, + Box::new(CorruptedBlobError( + "Identity wallet seed hash should be 32 bytes".to_string(), + )), + ) + })?;Based on learnings, error handling refactoring is needed across the codebase 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 881 - 883, Replace the panicking conversion of wallet_seed_hash into [u8; 32] in the identity loading path by returning an error instead of calling .expect; change the code that sets wallet_seed_hash_array (currently using wallet_seed_hash.try_into().expect("Seed hash should be 32 bytes")) to use try_into().map_err(|e| FromSqlConversionFailure::new("wallet seed hash", Box::new(e)))? (or the crate-specific FromSqlConversionFailure helper used earlier) and propagate the error with ? so the function (e.g., the identity loading function) returns Result instead of panicking; ensure you reference the same error type/constructor used at the earlier conversion fix around FromSqlConversionFailure to keep behavior consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/database/wallet.rs`:
- Around line 881-883: Replace the panicking conversion of wallet_seed_hash into
[u8; 32] in the identity loading path by returning an error instead of calling
.expect; change the code that sets wallet_seed_hash_array (currently using
wallet_seed_hash.try_into().expect("Seed hash should be 32 bytes")) to use
try_into().map_err(|e| FromSqlConversionFailure::new("wallet seed hash",
Box::new(e)))? (or the crate-specific FromSqlConversionFailure helper used
earlier) and propagate the error with ? so the function (e.g., the identity
loading function) returns Result instead of panicking; ensure you reference the
same error type/constructor used at the earlier conversion fix around
FromSqlConversionFailure to keep behavior consistent.
There was a problem hiding this comment.
Pull request overview
This PR finishes addressing the remaining error-handling inconsistencies tracked in #612 by replacing panic-prone paths and aligning DB conversion errors on rusqlite::Error::FromSqlConversionFailure, plus clarifying a misleading withdrawal display line when 24h usage data isn’t available.
Changes:
- Replaced
InvalidParameterName-based conversion errors inDatabase::get_walletswithFromSqlConversionFailurecarryingCorruptedBlobErrorcontext. - Removed remaining
expect/unwrappanics ininsert_or_update_contendersby propagating typed conversion errors instead. - Updated withdrawal info formatting to display “Remaining Today” as N/A when 24h usage data is unavailable, and added unit tests covering the wallet conversion error variants.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/database/wallet.rs |
Standardizes wallet row conversion failures to FromSqlConversionFailure and adds unit tests validating those variants. |
src/database/contested_names.rs |
Replaces panic paths in contender/document extraction with error propagation using CorruptedBlobError context. |
src/backend_task/platform_info.rs |
Avoids misleading “Remaining Today” output when actual 24h usage can’t be computed. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…tity loading Address CodeRabbit nitpick: the wallet seed hash conversion in the identity loading path (get_wallets) still used .expect() which would panic on corrupted data. Replace with the same FromSqlConversionFailure + CorruptedBlobError pattern used for the other three conversions.
|
Good catch @coderabbitai — addressed the remaining |
|
Tip For best results, initiate chat on the files or code changes.
🧠 Learnings usedIf you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/database/wallet.rs (1)
1313-1411: Three tests cover the three targeted paths correctly; consider adding coverage for the identity seed-hash path.The three new tests exercise each
FromSqlConversionFailurevariant with the right preconditions. One small gap: lines 881–889 (identity wallet seed hash) are also changed in this PR but have no corresponding test.🧪 Suggested test skeleton for the identity seed-hash path
+ #[test] + fn test_get_wallets_invalid_identity_seed_hash_uses_from_sql_conversion_failure() { + let db = create_test_database().expect("Failed to create test database"); + let seed_hash = create_test_seed_hash(); + let valid_epk = create_test_master_epk_bytes(Network::Testnet); + + // Insert a valid wallet row + { + let conn = db.conn.lock().unwrap(); + conn.execute( + "INSERT INTO wallet (seed_hash, encrypted_seed, salt, nonce, + master_ecdsa_bip44_account_0_epk, alias, is_main, uses_password, + password_hint, network, confirmed_balance, unconfirmed_balance, total_balance) + VALUES (?, ?, ?, ?, ?, NULL, 0, 0, NULL, 'testnet', 0, 0, 0)", + rusqlite::params![ + seed_hash.as_slice(), + vec![0u8; 64], + vec![0u8; 16], + vec![0u8; 12], + valid_epk, + ], + ).unwrap(); + // Insert an identity row with a wallet seed_hash blob that is only 31 bytes + conn.execute( + "INSERT INTO identity (data, wallet, wallet_index, network) + VALUES (?, ?, ?, 'testnet')", + rusqlite::params![vec![0u8; 4], vec![0u8; 31], 0u32], + ).unwrap(); + } + + let err = db.get_wallets(&Network::Testnet).expect_err("expected failure"); + match err { + rusqlite::Error::FromSqlConversionFailure(1, rusqlite::types::Type::Blob, _) => {} + _ => panic!("unexpected error variant: {}", err), + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/database/wallet.rs` around lines 1313 - 1411, Add a test that exercises the identity wallet seed-hash failure path by creating a test DB, inserting one valid wallet row (as in existing tests) and then inserting an identity row whose wallet blob is an invalid length (e.g., 31 bytes) so that db.get_wallets(&Network::Testnet) returns rusqlite::Error::FromSqlConversionFailure for the identity seed-hash conversion; mirror naming and assertions from other tests (e.g., call it test_get_wallets_invalid_identity_seed_hash_uses_from_sql_conversion_failure, use create_test_database(), create_test_seed_hash(), create_test_master_epk_bytes(), insert into identity with params![vec![0u8;4], vec![0u8;31], 0u32], then match the error for FromSqlConversionFailure with the correct column index/type).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/database/wallet.rs`:
- Around line 1313-1411: Add a test that exercises the identity wallet seed-hash
failure path by creating a test DB, inserting one valid wallet row (as in
existing tests) and then inserting an identity row whose wallet blob is an
invalid length (e.g., 31 bytes) so that db.get_wallets(&Network::Testnet)
returns rusqlite::Error::FromSqlConversionFailure for the identity seed-hash
conversion; mirror naming and assertions from other tests (e.g., call it
test_get_wallets_invalid_identity_seed_hash_uses_from_sql_conversion_failure,
use create_test_database(), create_test_seed_hash(),
create_test_master_epk_bytes(), insert into identity with params![vec![0u8;4],
vec![0u8;31], 0u32], then match the error for FromSqlConversionFailure with the
correct column index/type).
lklimek
left a comment
lklimek
left a comment
There was a problem hiding this comment.
Clean, focused PR. All three changes are correct:
- contested_names.rs:
expect()/unwrap()panics replaced with properFromSqlConversionFailureerror propagation usingCorruptedBlobErrorcontext - wallet.rs:
InvalidParameterNamemisuse replaced withFromSqlConversionFailureat correct column indices; good test coverage for all three paths - platform_info.rs: Honest "N/A" instead of misleading daily-limit-as-remaining value
Follow-up commit properly addressed CodeRabbit's identity seed-hash nitpick. No security concerns.
Summary
Fixes the remaining error-handling inconsistencies tracked in #612 after #562:
expect/unwrappaths ininsert_or_update_contenderswith typed error propagation (FromSqlConversionFailure+CorruptedBlobErrorcontext).InvalidParameterNameconversions inwallet::get_walletswithFromSqlConversionFailure:seed_hashwrong lengthTests
src/database/wallet.rsto verify theFromSqlConversionFailurevariants for the three wallet conversion paths.cargo fmt --allscripts/safe-cargo.sh test --all-features --workspacescripts/safe-cargo.sh test --doc --all-features --workspacescripts/safe-cargo.sh fmt --all -- --checkscripts/safe-cargo.sh clippy --all-features --all-targets -- -D warningsCloses #612
Summary by CodeRabbit
Bug Fixes
Tests
Validation
What was tested:
cargo fmt --all— formatting checkscripts/safe-cargo.sh clippy --all-features --all-targets -- -D warnings— lint with zero warningsscripts/safe-cargo.sh test --all-features --workspace— full workspace test suite including 3 new unit tests forFromSqlConversionFailurevariantsscripts/safe-cargo.sh test --doc --all-features --workspace— doc testsResults:
ClippyCI check — pass (4m20s)Test SuiteCI check — pass (7m42s)Environment: Local macOS arm64; GitHub Actions CI (ubuntu-latest)