Context
Surfaced by CodeRabbit during review of #711. The panic paths are pre-existing (inherited from the original `ManagedCoreAccount::from_*_account` constructors that #711 split). Both `ManagedCoreFundsAccount` and `ManagedCoreKeysAccount` carry the same `expect(...)` fallback today.
Repo coding guideline: "Avoid `unwrap()` and `expect()` in library code; use proper error types (e.g. `thiserror`) and never panic in library code."
Problem
The constructors look like:
pub fn from_account(account: &super::super::Account) -> Self {
let key_source = address_pool::KeySource::Public(account.account_xpub);
let managed_type = ManagedAccountType::from_account_type(
account.account_type,
account.network,
&key_source,
)
.unwrap_or_else(|_| {
let no_key_source = address_pool::KeySource::NoKeySource;
ManagedAccountType::from_account_type(
account.account_type,
account.network,
&no_key_source,
)
.expect("Should succeed with NoKeySource") // <-- panics
});
Self::new(managed_type, account.network, account.is_watch_only)
}
If `from_account_type` errors even with `NoKeySource` (e.g. an account variant we forgot to handle), the constructor aborts the caller's process instead of returning a structured error.
Suggested fix
Convert all three constructors (`from_account`, `from_bls_account`, `from_eddsa_account`) on both `ManagedCoreFundsAccount` and `ManagedCoreKeysAccount` to return `Result<Self, Error>` and propagate the underlying `crate::error::Error` from `ManagedAccountType::from_account_type`.
Touches every callsite, including the FFI surface in `key-wallet-ffi`. Worth doing in one focused PR.
Files
- `key-wallet/src/managed_account/managed_core_funds_account.rs`
- `key-wallet/src/managed_account/managed_core_keys_account.rs`
- (call sites discovered via `cargo check`)
Original review thread: #711 (review comment 3187368652)
Context
Surfaced by CodeRabbit during review of #711. The panic paths are pre-existing (inherited from the original `ManagedCoreAccount::from_*_account` constructors that #711 split). Both `ManagedCoreFundsAccount` and `ManagedCoreKeysAccount` carry the same `expect(...)` fallback today.
Repo coding guideline: "Avoid `unwrap()` and `expect()` in library code; use proper error types (e.g. `thiserror`) and never panic in library code."
Problem
The constructors look like:
If `from_account_type` errors even with `NoKeySource` (e.g. an account variant we forgot to handle), the constructor aborts the caller's process instead of returning a structured error.
Suggested fix
Convert all three constructors (`from_account`, `from_bls_account`, `from_eddsa_account`) on both `ManagedCoreFundsAccount` and `ManagedCoreKeysAccount` to return `Result<Self, Error>` and propagate the underlying `crate::error::Error` from `ManagedAccountType::from_account_type`.
Touches every callsite, including the FFI surface in `key-wallet-ffi`. Worth doing in one focused PR.
Files
Original review thread: #711 (review comment 3187368652)