Refactor error types into domain-specific hierarchy #126
Conversation
WalkthroughRefactors error handling across core and CLI: the monolithic KeepError is split into domain-specific enums (StorageError, CryptoError, KeyError, FrostError, NetworkError, PolicyError); From conversions added; call sites across keep-core and keep-cli updated to construct and propagate the new error variants without changing public function signatures. Changes
Sequence Diagram(s)Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
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
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (2)keep-cli/src/commands/frost_network.rs (3)
keep-cli/src/commands/serve.rs (2)
🔇 Additional comments (9)
✏️ Tip: You can disable this entire section by setting Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
keep-cli/src/commands/bitcoin.rs (1)
213-224: UseKeepError::Inputinstead ofFrostError::InvalidNetworkfor Bitcoin network validation.
FrostErroris semantically tied to the FROST threshold signature protocol (error codes E4xxx). UsingFrostError::InvalidNetworkfor CLI input validation conflates two unrelated concerns and produces misleading error messages. The codebase providesKeepError::Inputfor exactly this purpose—validating user-provided arguments. Replace the error with:_ => Err(KeepError::Input(format!( "invalid Bitcoin network '{}' (valid: mainnet, testnet, signet, regtest)", s )).into()),keep-core/src/hidden/header.rs (1)
1-307: Runcargo fmtto fix formatting issues.The CI pipeline indicates formatting differences in this file. Run
cargo fmtbefore merging.keep-core/src/frost/share.rs (1)
1-134: Runcargo fmtto fix formatting issues.The CI pipeline indicates formatting differences in this file.
keep-core/src/frost/transport.rs (1)
1-391: Runcargo fmtto fix formatting issues.The CI pipeline indicates formatting differences in this file.
🤖 Fix all issues with AI agents
In @keep-core/src/frost/dealer.rs:
- Around line 155-163: Formatting in the match arm that constructs the Err uses
a line that exceeds the formatter width; run cargo fmt to reflow the long return
Err(...) line (or manually break the long FrostError::Protocol(...) call into
multiple lines) so the match arm fits within the line length limit—look for the
match on bytes.len() and the usage of pubkey and FrostError::Protocol to locate
and reformat the block.
In @keep-core/src/frost/signing.rs:
- Around line 184-199: The function signature_bytes in signing.rs is
misformatted; run rustfmt to apply canonical formatting (e.g., run `cargo fmt`)
so the method's let-else, chaining and block indentation match project style;
ensure the file is saved after formatting and commit the changes referencing
signature_bytes for reviewers.
In @keep-core/src/hidden/header.rs:
- Around line 108-110: Change the StorageError::UnsupportedVersion variant to
take a u16 instead of u8 in the StorageError enum (update its definition in
error.rs to UnsupportedVersion(u16)), then update both call sites that currently
cast the parsed version with as u8 (in the header handling logic and the storage
validation logic) to pass the raw u16 value instead; ensure any pattern matches
or error constructions (e.g., StorageError::UnsupportedVersion(...)) and tests
are updated to expect a u16 payload.
In @keep-core/src/hidden/volume.rs:
- Around line 285-286: The CI failure is due to formatting of the multi-line
matches! patterns (e.g., the expression matching
KeepError::Crypto(CryptoError::InvalidPassword) |
KeepError::Crypto(CryptoError::DecryptionFailed)) in
keep-core/src/hidden/volume.rs (also similar occurrences around the ranges
370-371 and 401-402); run cargo fmt to auto-fix and ensure each matches! arm is
formatted per rustfmt (or manually reformat the multi-line matches! into a
rustfmt-friendly layout) so the patterns and vertical bar align correctly and
pass cargo fmt --check.
🧹 Nitpick comments (9)
keep-core/src/error.rs (1)
273-301: Consider preserving structured error info for redb errors.All redb error types are converted to
StorageError::Database(String)via.to_string(). This loses the ability to programmatically distinguish betweenDatabaseError,TransactionError,TableError,StorageError, andCommitErrorat theStorageErrorlevel.If downstream code needs to handle these differently (e.g., retry on transient transaction errors vs. fail on corruption), consider using separate variants or an enum within
Database.keep-cli/src/signer/handler.rs (3)
200-206: Redundant.into()onKeepErrorvariant.
KeepError::EventIdFailedis already the expected error type forResult<_, KeepError>, so the.into()call is unnecessary.Suggested fix
let event_id = frost_event .id - .ok_or_else(|| KeepError::EventIdFailed.into())?; + .ok_or(KeepError::EventIdFailed)?;
227-232: Same redundant.into()pattern in local FROST path.Same issue as above—
KeepError::EventIdFailedandKeepError::InvalidSignaturedon't need.into().Suggested fix
let event_id = frost_event .id - .ok_or_else(|| KeepError::EventIdFailed.into())?; + .ok_or(KeepError::EventIdFailed)?; let sig_bytes = signer.sign(event_id.as_bytes())?; let sig = nostr_sdk::secp256k1::schnorr::Signature::from_slice(&sig_bytes) - .map_err(|e| KeepError::InvalidSignature(e.to_string()))?; + .map_err(|e| KeepError::InvalidSignature(e.to_string()))?;
253-257:FrostError::SigningFailedused for non-FROST signing path.This code path handles regular Nostr keypair signing (non-FROST), but the error is mapped to
FrostError::SigningFailed. This is semantically misleading—consider using a more general error variant likeKeepError::InvalidSignatureor adding aCryptoError::SigningFailedvariant.Suggested fix
unsigned_event .sign(&keys) .await - .map_err(|e| FrostError::SigningFailed(e.to_string()))? + .map_err(|e| KeepError::InvalidSignature(e.to_string()))?keep-cli/src/signer/frost_signer.rs (1)
24-26: ConsiderInvalidConfigfor "no shares provided" error."No shares provided" is a configuration/setup issue rather than a protocol error.
FrostError::Protocoltypically indicates runtime protocol violations. UsingFrostError::InvalidConfig("No shares provided")would be more semantically consistent with lines 40-47.Suggested fix
if shares.is_empty() { - return Err(FrostError::Protocol("No shares provided".to_string()).into()); + return Err(FrostError::InvalidConfig("No shares provided".to_string()).into()); }keep-core/src/hidden/header.rs (1)
258-261: Inconsistent error wrapping pattern.This explicitly wraps
StorageErrorinKeepError::Storage(...), while all other error sites in this file use.into()for the conversion. This inconsistency could confuse future maintainers.Suggested fix for consistency
if bytes.len() < Self::COMPACT_SIZE { - return Err(KeepError::Storage(StorageError::HeaderSizeMismatch { + return Err(StorageError::HeaderSizeMismatch { expected: Self::COMPACT_SIZE, actual: bytes.len(), - })); + }.into()); }keep-core/src/frost/transport.rs (2)
115-120: Minor inconsistency in error conversion pattern.Line 117 relies on
?to convertFrostErrorwhile line 119 explicitly calls.into()insidemap_err. Both work but the pattern is inconsistent.Align the patterns
pub fn to_bech32(&self) -> Result<String> { let json = self.to_json()?; let hrp = Hrp::parse(SHARE_HRP).map_err(|e| FrostError::Protocol(format!("Invalid HRP: {e}")))?; bech32::encode::<Bech32m>(hrp, json.as_bytes()) - .map_err(|e| FrostError::Protocol(format!("Bech32 encoding failed: {e}")).into()) + .map_err(|e| FrostError::Protocol(format!("Bech32 encoding failed: {e}"))) }
202-216: Inconsistent error conversion in payload/session parsing.Line 204 uses
.into()insidemap_errwhile line 209 doesn't, despite both being hex decode errors in the same struct.Align the patterns
pub fn payload_bytes(&self) -> Result<Vec<u8>> { hex::decode(&self.payload) - .map_err(|_| FrostError::Protocol("Invalid payload hex".into()).into()) + .map_err(|_| FrostError::Protocol("Invalid payload hex".into())) }keep-cli/src/commands/frost.rs (1)
284-287: Consider using a dedicatedPolicyErrorvariant for Warden policy denials.The PR introduces
PolicyErrorin the error hierarchy, but these Warden policy-related errors still useKeepError::Runtime. For consistency with the domain-specific error taxonomy, consider usingPolicyErrorvariants here.Also applies to: 310-310, 324-324
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
keep-cli/src/commands/agent.rskeep-cli/src/commands/bitcoin.rskeep-cli/src/commands/enclave.rskeep-cli/src/commands/frost.rskeep-cli/src/commands/frost_hardware.rskeep-cli/src/commands/frost_network.rskeep-cli/src/commands/mod.rskeep-cli/src/commands/serve.rskeep-cli/src/commands/vault.rskeep-cli/src/server.rskeep-cli/src/signer/frost_signer.rskeep-cli/src/signer/handler.rskeep-cli/src/warden.rskeep-core/src/crypto.rskeep-core/src/error.rskeep-core/src/frost/coordinator.rskeep-core/src/frost/dealer.rskeep-core/src/frost/share.rskeep-core/src/frost/signing.rskeep-core/src/frost/transport.rskeep-core/src/hidden/header.rskeep-core/src/hidden/volume.rskeep-core/src/keyring.rskeep-core/src/keys.rskeep-core/src/lib.rskeep-core/src/storage.rs
🧰 Additional context used
🧬 Code graph analysis (10)
keep-core/src/frost/coordinator.rs (2)
keep-cli/src/signer/frost_signer.rs (4)
new(19-57)new(80-85)sign(63-70)sign(95-100)keep-core/src/frost/dealer.rs (2)
new(18-33)new(64-66)
keep-cli/src/commands/frost_hardware.rs (2)
keep-core/src/frost/share.rs (1)
pubkey_package(90-93)keep-core/src/frost/transport.rs (1)
session_id_bytes(207-216)
keep-cli/src/signer/handler.rs (1)
keep-core/src/crypto.rs (8)
from_slice(242-253)new(30-46)new(101-115)new(159-166)new(218-225)encrypt(335-348)decrypt(233-240)decrypt(350-360)
keep-cli/src/commands/serve.rs (3)
keep-cli/src/signer/frost_signer.rs (2)
new(19-57)new(80-85)keep-core/src/lib.rs (1)
data_key(365-367)keep-core/src/storage.rs (1)
data_key(252-254)
keep-core/src/frost/transport.rs (1)
keep-core/src/frost/share.rs (2)
decrypt(123-133)key_package(85-88)
keep-cli/src/commands/vault.rs (2)
keep-core/src/lib.rs (1)
data_key(365-367)keep-core/src/storage.rs (1)
data_key(252-254)
keep-core/src/lib.rs (1)
keep-core/src/storage.rs (1)
data_key(252-254)
keep-core/src/frost/dealer.rs (1)
keep-core/src/frost/share.rs (1)
key_package(85-88)
keep-cli/src/commands/frost.rs (2)
keep-cli/src/signer/frost_signer.rs (2)
new(19-57)new(80-85)keep-core/src/frost/transport.rs (2)
from_json(110-113)from_json(197-200)
keep-core/src/frost/signing.rs (4)
keep-cli/src/signer/frost_signer.rs (4)
new(19-57)new(80-85)sign(63-70)sign(95-100)keep-core/src/frost/coordinator.rs (2)
new(35-45)aggregate(156-171)keep-core/src/frost/dealer.rs (1)
new(18-33)keep-core/src/frost/share.rs (1)
key_package(85-88)
🪛 GitHub Actions: CI
keep-core/src/hidden/header.rs
[error] 1-999: cargo fmt --check failed. Formatting differences detected in hidden/header.rs. Run 'cargo fmt' to fix code style issues.
keep-core/src/frost/share.rs
[error] 1-999: cargo fmt --check failed. Formatting differences detected in frost/share.rs. Run 'cargo fmt' to fix code style issues.
keep-core/src/frost/transport.rs
[error] 1-999: cargo fmt --check failed. Formatting differences detected in frost/transport.rs. Run 'cargo fmt' to fix code style issues.
keep-core/src/frost/dealer.rs
[error] 1-999: cargo fmt --check failed. Formatting differences detected in frost/dealer.rs. Run 'cargo fmt' to fix code style issues.
keep-core/src/frost/signing.rs
[error] 1-999: cargo fmt --check failed. Formatting differences detected in frost/signing.rs. Run 'cargo fmt' to fix code style issues.
keep-core/src/hidden/volume.rs
[error] 1-999: cargo fmt --check failed. Formatting differences detected in hidden/volume.rs. Run 'cargo fmt' to fix code style issues.
🔇 Additional comments (153)
keep-core/src/error.rs (7)
5-46: Well-structured StorageError with clear error codes.The StorageError enum is well-designed with unique error codes (E1xxx series) and appropriate variants for storage-related failures. The
#[non_exhaustive]attribute is a good choice for future extensibility.
48-86: CryptoError covers the crypto domain comprehensively.Good coverage of cryptographic failure modes with descriptive error codes (E2xxx series). The variants align well with the usage patterns in
keep-core/src/crypto.rs.
88-117: KeyError variants appropriately scoped for key management.The E3xxx series covers key lifecycle operations. The distinction between
NotFound,AlreadyExists, and format errors (InvalidNsec,InvalidNpub) is clear.
119-157: FrostError provides granular FROST protocol error handling.The E4xxx series covers DKG, signing, session management, and threshold validation. The
InsufficientSignersvariant withneeded/availablefields provides actionable debugging info.
159-194: NetworkError and PolicyError complete the domain coverage.NetworkError (E5xxx) and PolicyError (E6xxx) appropriately separate network transport concerns from policy/authorization concerns.
221-271: KeepError as composite wrapper is well-structured.The use of
#[error(transparent)]for domain-specific errors allows clean error propagation while maintaining the ability to match on specific domains. The E9xxx series for miscellaneous runtime errors is appropriate.
38-39: Potential conflictingFrom<std::io::Error>implementations.
StorageError::Iouses#[from] std::io::Error(line 39), and there's also a manualFrom<std::io::Error> for KeepError(lines 303-307). While this compiles because they target different types, it creates two conversion paths:
std::io::Error→StorageError::Io(via#[from])std::io::Error→KeepError::Storage(StorageError::Io(_))(via manual impl)This is intentional and correct—the manual impl enables
?to convert directly toKeepErrorin functions returningResult<T, KeepError>. Just ensure this dual-path behavior is understood by contributors.Also applies to: 303-307
keep-cli/src/signer/handler.rs (4)
10-10: Appropriate error type imports for the handler.The expanded import correctly pulls in all domain-specific error types needed for granular error handling in the signer pathways.
77-91: Error mapping in handle_connect is semantically correct.The error types appropriately distinguish between FROST pubkey issues (
FrostError::InvalidFrostPubkey), key availability (KeyError::NoSigningKey), key format (KeyError::InvalidPubkey), and key mismatch (KeyError::PubkeyMismatch).
288-318: NIP-44 encrypt handler uses consistent error taxonomy.The error mapping for permission denial (
PolicyError), missing key (KeyError::NoSigningKey), invalid secret (CryptoError::InvalidSecretKey), and NIP-44 operation failure (KeepError::Nip44) is appropriate.
354-385: NIP-04 handlers mirror NIP-44 pattern correctly.Consistent error handling between NIP-04 and NIP-44 paths.
keep-core/src/crypto.rs (6)
13-13: Clean import of domain-specific error type.Importing
CryptoErrordirectly allows the module to construct specific variants without prefixing.
159-173: SecretVec RAM encryption/decryption errors are appropriately specific.Using
CryptoError::RamEncryptionFailedandCryptoError::RamDecryptionFailedclearly indicates the failure occurred in the memory-security layer rather than general cryptographic operations.
218-248: SecretKey error handling covers all failure modes.
RamEncryptionFailedfor key creationRamDecryptionFailedfor key accessInvalidKeyLengthwith expected/actual for slice constructionThis provides actionable diagnostics for debugging key-related issues.
268-289: Key derivation errors preserve underlying cause.
CryptoError::KeyDerivation(e.to_string())captures the Argon2 error details, which is helpful for diagnosing parameter or memory issues during password hashing.
317-332: EncryptedData::from_bytes validates minimum length.
CryptoError::DataTooShort { min }clearly communicates the validation failure. This is a good defensive check against truncated or malformed ciphertext.
335-360: Encrypt/decrypt functions use clear error variants.
CryptoError::Encryption("cipher failed")for encryption failuresCryptoError::DecryptionFailedfor decryption failures (no string since it's typically wrong key or corruption)This distinction is appropriate—encryption failures are rare and indicate implementation issues, while decryption failures are common (wrong password).
keep-cli/src/commands/mod.rs (2)
16-26: Input errors correctly classified asKeepError::Input.Using
KeepError::Inputfor password read failures is semantically appropriate—these are user input/interaction errors, not runtime or I/O errors.
28-50: Consistent error handling across input functions.All three input functions (
get_password,get_password_with_confirm,get_confirm) now use the sameKeepError::Inputpattern with descriptive messages.keep-cli/src/signer/frost_signer.rs (4)
6-6: Focused import of FrostError for the FROST signer module.Importing only
FrostError(rather than all error types) reflects this module's single-domain focus.
24-36: FrostSigner validation uses appropriate error variants.
FrostError::Protocolfor no shares (though see note below)FrostError::InsufficientSignerswith structuredneeded/availablefieldsThe structured
InsufficientSignersvariant provides actionable diagnostics.
38-49: Config validation errors are correctly classified.
FrostError::InvalidConfigis appropriate for share/threshold mismatches detected during signer construction.
95-100: NetworkFrostSigner signing error correctly maps to FrostError.Unlike the non-FROST path in
handler.rs, this is genuinely FROST-based signing viaKfpNode, soFrostError::SigningFailedis the correct error type.keep-cli/src/commands/agent.rs (5)
7-7: LGTM!Import correctly adds
KeyErrorto support the new domain-specific error hierarchy.
21-24: LGTM!Using
KeepError::NotImplementedwith a static string for unsupported MCP in hidden mode is appropriate for this feature gate.
35-38: LGTM!Replacing the generic key-not-found error with
KeyError::NotFoundaligns with the domain-specific error hierarchy and provides better error classification.
50-62: LGTM!The runtime and session creation errors are appropriately mapped to
KeepError::Runtimewith descriptive messages. The error handling flow is correct.
72-84: LGTM!I/O errors (read, write, flush) are consistently mapped to
KeepError::Runtime, which is appropriate for operational failures in the MCP command flow.keep-core/src/keys.rs (5)
8-8: LGTM!Import correctly adds
CryptoErrorandKeyErrorfor granular error classification in key operations.
34-36: LGTM!
CryptoError::InvalidSecretKeyis the appropriate error type forSigningKey::from_bytesfailures, clearly indicating a cryptographic validation issue.
45-54: LGTM!The
from_nsecfunction now usesKeyError::InvalidNsecconsistently for all validation failures (decode, HRP mismatch, length mismatch). This provides uniform error handling for malformed nsec inputs.
83-87: LGTM!Using
CryptoError::InvalidSigningKeyfor signing failures correctly categorizes this as a cryptographic error distinct from key format errors.
148-157: LGTM!The
npub_to_bytesfunction mirrors the validation pattern infrom_nsec, usingKeyError::InvalidNpubconsistently for all npub validation failures.keep-cli/src/commands/bitcoin.rs (7)
6-6: LGTM!Import correctly adds domain-specific error types (
FrostError,KeyError) for granular error classification.
27-36: LGTM!Error handling for key lookup and signer creation is appropriate:
KeyError::NotFoundfor missing keys andKeepError::Runtimefor signer initialization failures.
46-49: LGTM!
KeepError::Runtimeis appropriate for address derivation failures from the Bitcoin signer.
70-85: LGTM!Consistent use of
KeyError::NotFoundfor key lookup andKeepError::Runtimefor signer and descriptor operations.
98-100: LGTM!
KeepError::Runtimeis appropriate for internal descriptor generation failures.
121-150: LGTM!The PSBT signing flow correctly uses
KeyError::NotFoundfor key lookup andKeepError::Runtimefor file I/O, PSBT parsing, signing, and output writing operations.
164-178: LGTM!The analyze command correctly maps file I/O, PSBT parsing, signer creation, and analysis errors to
KeepError::Runtime.keep-cli/src/commands/enclave.rs (8)
6-6: LGTM!Import correctly adds
KeyErrorfor key-related error handling.
107-116: LGTM!Enclave attestation and PCR parsing errors are correctly mapped to
KeepError::Runtimefor operational failures.
166-171: LGTM!Using
KeepError::Runtimefor invalid pubkey length from enclave response is appropriate—this is an operational/protocol error rather than user input error.
193-203: LGTM!Consistent use of
KeepError::Runtimefor key generation and pubkey length validation errors from the enclave.
231-232: LGTM!Using
KeepError::Inputfor invalid message hex is semantically correct—this is a user input validation error.
280-282: LGTM!
KeepError::Runtimeis appropriate for enclave signing operation failures.
320-334: LGTM!Error handling distinguishes appropriately between:
KeyError::NotFoundfor missing vault keyKeepError::Runtimefor stdin I/O errorsKeepError::Inputfor invalid hex formatThis classification aids debugging by indicating whether the error is a missing resource, operational failure, or invalid input.
376-378: LGTM!
KeepError::Runtimeis appropriate for enclave key import failures.keep-cli/src/warden.rs (7)
8-8: LGTM!Import correctly adds
PolicyErrorfor policy/Warden-related error handling.
88-99: LGTM!Using
PolicyError::HttpClientfor reqwest client builder failures is appropriate—this clearly indicates a client setup issue rather than a request-time failure.
110-128: LGTM!Error handling correctly distinguishes between:
PolicyError::WardenRequestfor HTTP send/status failuresPolicyError::InvalidResponsefor JSON deserialization failuresThe error messages include relevant context (status code, body text).
199-201: LGTM!
PolicyError::ApprovalTimeoutclearly signals a local timeout waiting for approval, distinct from server-side workflow timeouts.
214-228: Double.into()call creates unnecessary indirection.On lines 215-216 and 224-228,
PolicyError::WardenRequest(...)is constructed and then.into()is called. However, themap_errclosure already returns a type that should convert properly. The patternPolicyError::...(...).into()suggests convertingPolicyErrorto another error type (likelyKeepError), but wrapped in a closure that itself returns aResult<_, PolicyError>.Looking at line 214-216:
let response = req.send().await.map_err(|e| { PolicyError::WardenRequest(format!("Workflow status check: {}", e)).into() })?;The
.into()here converts toKeepError. This is fine and consistent with other usages in the file.
236-238: LGTM!
PolicyError::InvalidResponsefor status JSON parsing failure is consistent with the evaluate method's error handling.
240-265: LGTM!The
PolicyError::WorkflowFailedvariant withstatusanddetailsfields provides excellent debugging context for terminal workflow states. The match arms correctly handle:
APPROVED/REJECTED: Return success with booleanTIMED_OUT/CANCELLED: Return error with specific statusPENDING: Continue polling- Unknown status: Return error with the actual status value
This comprehensive handling prevents silent failures from unexpected server states.
keep-core/src/frost/coordinator.rs (2)
14-14: LGTM!The import change from
KeepErrortoFrostErroraligns with the PR's domain-specific error hierarchy.
51-187: Error handling migration looks correct.The consistent use of
FrostError::Protocolfor all FROST protocol-related failures maintains clear domain separation. The error messages are descriptive and include relevant context (e.g., threshold counts, underlying error details).keep-core/src/frost/share.rs (2)
70-77: LGTM!The error handling for key package serialization is appropriate. The
FrostError::Protocolvariant correctly categorizes these as protocol-level failures.
85-93: The suggested fix is incomplete and would not compile.Lines 72 and 76 use
.map_err(...)?(with?operator after), while lines 87 and 92 use.map_err(...).into()(explicit conversion inside). Both patterns are valid:
- Pattern A (lines 72, 76): Error wrapped in
map_err, then converted by?- Pattern B (lines 87, 92): Error wrapped and converted in one step with
.into()The proposed diff removes only the
.into()without adding?, which would returnResult<T, FrostError>instead of the requiredResult<T, KeepError>, causing a type mismatch. To align with pattern A, you'd need to add?at the end of the chain:pub fn key_package(&self) -> Result<KeyPackage> { KeyPackage::deserialize(&self.key_package_bytes) - .map_err(|e| FrostError::Protocol(format!("Failed to deserialize key package: {e}")).into()) + .map_err(|e| FrostError::Protocol(format!("Failed to deserialize key package: {e}")))? } pub fn pubkey_package(&self) -> Result<PublicKeyPackage> { PublicKeyPackage::deserialize(&self.pubkey_package_bytes) - .map_err(|e| FrostError::Protocol(format!("Failed to deserialize pubkey package: {e}")).into()) + .map_err(|e| FrostError::Protocol(format!("Failed to deserialize pubkey package: {e}")))? }Likely an incorrect or invalid review comment.
keep-cli/src/commands/frost_hardware.rs (3)
6-6: LGTM!The import correctly brings in both
FrostErrorandKeepErrorvariants, enabling appropriate domain-specific error handling for this module that bridges hardware operations with FROST protocol logic.
21-29: Good separation of connection vs operation errors.The distinction between
HardwareConnectionfor initial connection failures andHardwareOperationwith operation context for subsequent failures provides clear error categorization for debugging.
113-131: Appropriate use of FrostError for Frost-specific serialization.The serialization of Frost key material (secret share, verifying share, group key) correctly uses
FrostError::Protocolsince these are FROST protocol-level operations, distinct from the hardware communication layer.keep-core/src/frost/transport.rs (2)
7-7: LGTM!The import correctly brings in
FrostErrorfor this transport layer that handles FROST share serialization and messaging.
238-275: LGTM!The animated frame parsing has good error handling with descriptive messages for each failure mode (empty frames, missing index, missing data, invalid hex, invalid UTF-8).
keep-core/src/keyring.rs (5)
7-7: LGTM!Import statement correctly includes
KeyErrorto support the new error hierarchy.
61-67: LGTM!Error handling in
load_keycorrectly wrapsKeyError::KeyringFullandKeyError::AlreadyExistswithinKeepError::Key(), maintaining consistency with the new error hierarchy.
103-109: LGTM!Error handling in
set_primarycorrectly returnsKeepError::Key(KeyError::NotFound(...))when the key doesn't exist.
111-121: LGTM!Error handling in
removeis consistent withset_primary, using the same wrappedKeyError::NotFoundvariant.
188-191: LGTM!Test assertion correctly updated to match the new nested error variant pattern
KeepError::Key(KeyError::KeyringFull(_)).keep-cli/src/server.rs (6)
10-10: LGTM!Import statement correctly includes
NetworkErrorandKeyErrorfor the refined error handling.
58-62: LGTM!Relay connection errors are correctly mapped to
NetworkError::RelayAdd, providing clear context for debugging connection issues.
147-151: LGTM!Subscription failures are appropriately mapped to
NetworkError::Subscribe.
193-196: LGTM!Runtime notification handler errors are correctly mapped to
KeepError::Runtimewith descriptive context.
210-244: LGTM!NIP-46 event handling errors are well-categorized:
- Decryption →
NetworkError::Decrypt- JSON parsing →
NetworkError::Parse- Encryption →
NetworkError::Encrypt- Event signing →
NetworkError::SignRequestThis provides granular error classification for debugging protocol issues.
42-54: Error conversions are handled correctly by thiserror.The
KeepErrorenum inkeep-core/src/error.rsuses the#[from]attribute on itsKeyError(line 231) andNetworkError(line 237) variants. Thethiserrorcrate automatically generatesFromtrait implementations for these, enabling the implicit conversion via the?operator. No explicit wrapping is required.Likely an incorrect or invalid review comment.
keep-core/src/frost/dealer.rs (4)
7-7: LGTM!Import correctly updated to use
FrostErrorfor domain-specific error handling.
18-33: LGTM!Configuration validation errors appropriately use
FrostError::InvalidConfigwith descriptive messages for threshold constraints.
68-100: LGTM!Key generation and
KeyPackageconversion errors are consistently mapped toFrostError::Protocol, aligning with the pattern inshare.rs(per the relevant code snippet at lines 84-87).
103-144: LGTM!Key splitting logic mirrors the generation path with consistent
FrostError::Protocolerror mapping for signing key deserialization and split operation failures.keep-cli/src/commands/vault.rs (14)
8-8: LGTM!Import correctly updated to include
KeyErrorfor key-related error handling.
33-37: LGTM!Input validation error for password length correctly uses
KeepError::Inputinstead of a generic error variant.
48-51: LGTM!Password read failure appropriately mapped to
KeepError::Runtimewith descriptive context.
53-63: LGTM!Hidden password validation errors consistently use
KeepError::Inputfor user input issues.
149-149: LGTM!Data key access correctly returns
KeyError::Lockedwhen the vault is not unlocked, converted via.into().
188-188: LGTM!Consistent with
cmd_generate_outer- usesKeyError::Lockedfor locked vault state.
263-263: LGTM!Import operation correctly checks for locked state using
KeyError::Locked.
304-304: LGTM!Consistent locked state handling in hidden import path.
475-479: LGTM!Key lookup failures correctly return
KeyError::NotFoundwith the key name for debugging.
509-515: LGTM!Export from outer volume correctly handles both not-found and locked error cases with appropriate
KeyErrorvariants.
552-558: LGTM!Hidden export path mirrors outer volume handling with consistent error types.
597-601: LGTM!Delete operation correctly returns
KeyError::NotFoundwhen key doesn't exist.
631-636: LGTM!Outer volume delete path uses consistent
KeyError::NotFoundhandling.
666-671: LGTM!Hidden volume delete path maintains consistency with other delete operations.
keep-core/src/storage.rs (14)
10-10: LGTM!Import correctly updated to include
CryptoError,KeyError, andStorageErrorfor comprehensive error handling.
79-86: LGTM!Header validation errors appropriately use
StorageError::InvalidMagicandStorageError::UnsupportedVersionfor clear error classification.
122-125: LGTM!Storage creation correctly returns
StorageError::AlreadyExistswith path context when the vault already exists.
167-181: LGTM!Storage open validates both path existence (
StorageError::NotFound) and header size (StorageError::HeaderSizeMismatch) with appropriate error variants and context.
200-228: LGTM!Unlock method correctly:
- Returns
CryptoError::RateLimitedwith remaining seconds when rate-limited- Checks for
CryptoError::DecryptionFailedto record failed attempts- Propagates the original error for proper error classification
256-274: LGTM!
store_keycorrectly uses explicitKeepError::Key(KeyError::Locked)for locked state checks on bothdata_keyanddbaccess.
276-293: LGTM!
load_keycorrectly returnsKeyError::NotFoundwith hex-encoded ID when the key doesn't exist.
295-317: LGTM!
list_keysmaintains consistent locked state checking pattern.
319-336: LGTM!
delete_keycorrectly checks for key existence and returnsKeyError::NotFoundif deletion fails.
342-362: LGTM!Share storage operations follow the same locked state checking pattern as key operations.
364-386: LGTM!
list_sharesmaintains consistency withlist_keysfor locked state handling.
388-410: LGTM!
delete_shareprovides a descriptive error message including the share identifier when not found.
498-506: LGTM!Test assertions correctly updated to match the new nested error variant pattern
KeepError::Crypto(CryptoError::DecryptionFailed)andKeepError::Crypto(CryptoError::RateLimited(_)).
525-529: LGTM!Rate limit reset test correctly verifies
CryptoError::DecryptionFailedbehavior after successful unlock.keep-cli/src/commands/frost_network.rs (10)
7-7: LGTM - Updated imports align with new error hierarchy.The import now includes
FrostErrorandNetworkErroralongsideKeepErrorandResult, enabling domain-specific error handling throughout this module.
49-50: Consistent runtime error handling across all tokio runtime creations.All
Runtime::new()calls consistently map toKeepError::Runtime, which is appropriate since runtime initialization is a general infrastructure concern rather than domain-specific.Also applies to: 131-132, 245-246, 290-291, 437-438, 760-761, 1085-1086, 1245-1246
55-57: LGTM - FROST node and protocol errors correctly mapped toFrostError::Protocol.Network node creation and protocol-level operations appropriately use
FrostError::Protocolfor failures, maintaining clear domain boundaries.Also applies to: 137-140, 143-145, 293-295, 336-339
209-217: Hardware operation errors consistently useKeepError::HardwareOperation.Hardware signer operations (ping, frost_sign, get_share_pubkey, etc.) uniformly map errors to
KeepError::HardwareOperationwith operation name and reason, providing good debuggability.Also applies to: 398-401, 611-614, 725-728, 1202-1205, 1209-1215
225-229: LGTM - Configuration validation errors useFrostError::InvalidConfig.Threshold and participant validation errors consistently use
FrostError::InvalidConfig, which is semantically appropriate for configuration-time validation failures.Also applies to: 425-429, 704-708, 712-716, 1059-1063, 1067-1072
444-447: LGTM - Network operations correctly useNetworkErrorvariants.Relay add, publish, sign request, fetch, and encrypt operations appropriately map to domain-specific
NetworkErrorvariants (RelayAdd,Publish,SignRequest,Fetch,Encrypt), providing clear categorization of network-related failures.Also applies to: 476-477, 482-483, 515-516, 521-522, 648-649, 654-655, 769-770, 791-792, 797-798, 908-908, 920-921, 925-926, 1096-1096, 1141-1141, 1146-1147, 1255-1255, 1284-1284, 1289-1290
406-408: LGTM - FROST-specific errors use appropriateFrostErrorvariants.Commitment failures use
FrostError::CommitmentFailed, timeouts useFrostError::Timeout, and protocol violations useFrostError::Protocol. This provides clear error categorization for FROST operations.Also applies to: 414-417, 541-541, 820-820, 950-950, 1227-1227
735-741: LGTM - DKG operations useFrostError::DkgFailedwith stage context.All DKG-related errors consistently use
FrostError::DkgFailed { stage, reason }, providing clear indication of which DKG phase failed and why.Also applies to: 745-751, 844-853, 875-881, 888-897, 1016-1022
254-256: LGTM - Not-implemented features useKeepError::NotImplemented.Features not available (Warden without feature flag, network event signing) correctly return
KeepError::NotImplementedwith descriptive messages.Also applies to: 682-682
321-326: TheFrostError::InsufficientSignersvariant is properly defined with both fields typed asu16. All usages across the codebase—infrost_signer.rs,frost.rs,frost_network.rs,lib.rs, andsigning.rs—consistently assign values with appropriate type casting. No field type inconsistencies exist.Likely an incorrect or invalid review comment.
keep-cli/src/commands/serve.rs (5)
10-10: LGTM - Updated imports for new error hierarchy.The import now includes
FrostErrorandKeyErrorfor domain-specific error handling in FROST and key-related operations.
58-59: Consistent runtime error handling.All
Runtime::new()failures consistently map toKeepError::Runtime, maintaining uniformity across the module.Also applies to: 123-124, 260-261, 358-359
63-65: LGTM - FROST node operations useFrostError::Protocol.Node creation and announcement errors appropriately use
FrostError::Protocol, consistent with the error hierarchy used infrost_network.rs.Also applies to: 67-69
227-227: LGTM - TUI errors consistently mapped toKeepError::Runtime.TUI execution failures are appropriately categorized as runtime errors since they relate to terminal/UI runtime rather than domain-specific operations.
Also applies to: 323-323, 421-421
245-245: TheKeyError::Locked.into()conversion is properly supported.The
#[from]attribute on theKeyvariant in theKeepErrorenum (keep-core/src/error.rs:231) automatically generates theFrom<KeyError> for KeepErrorimplementation.KeyError::Lockedexists (line 107), and the conversion is valid and will compile correctly.keep-cli/src/commands/frost.rs (8)
6-6: LGTM - Updated imports for domain-specific errors.The import includes
FrostErrorandKeyErrorfor appropriate error categorization in FROST operations.
42-43: LGTM - Empty shares result usesFrostError::Protocol.When
frost_generateorfrost_splitreturns no shares, this is correctly treated as a protocol-level error since it indicates an unexpected internal state.Also applies to: 92-93
221-222: LGTM - Input handling errors appropriately categorized.Read failures use
KeepError::Runtimefor I/O issues, and input size violations useKeepError::Inputfor validation errors. This distinction is semantically correct.Also applies to: 228-231
350-351: LGTM - Input validation and feature errors appropriately categorized.Invalid hex input uses
KeepError::Input, runtime initialization usesKeepError::Runtime, and missing Warden feature usesKeepError::NotImplemented.Also applies to: 355-356, 362-364
382-383: LGTM - Missing shares useKeyError::NotFound.When no shares are found for a group, using
KeyError::NotFoundis semantically appropriate as it indicates a missing key resource.
393-398: LGTM - Insufficient signers usesFrostError::InsufficientSigners.This correctly uses the structured error variant with
neededandavailablefields, consistent with usage in other modules.
449-451: LGTM - Interactive signing round 1 errors use appropriateFrostErrorvariants.Commitment serialization uses
FrostError::Protocol, read errors useKeepError::Runtime, session ID mismatches useFrostError::InvalidSessionId, and commitment deserialization usesFrostError::CommitmentFailed.Also applies to: 477-478, 481-482, 486-487, 489-490, 498-499
522-523: LGTM - Interactive signing round 2 and aggregation errors appropriately categorized.Read errors, session ID validation, signature share handling, and aggregation all use appropriate
FrostErrorvariants (Protocol,InvalidSessionId,SigningFailed).Also applies to: 526-527, 531-532, 534-535, 543-544, 547-548, 550-551
keep-core/src/frost/signing.rs (6)
16-16: LGTM - Simplified imports to domain-specific types only.The import now uses only
FrostErrorandResult, removing the unusedKeepErrorimport since all errors in this module are FROST-specific.
84-100: LGTM - Commitment generation errors useFrostError::Protocol.Invalid session state and duplicate commitment generation correctly return protocol-level errors, as these represent violations of the FROST protocol state machine.
102-118: LGTM - Commitment addition errors appropriately categorized.State validation and duplicate detection correctly use
FrostError::Protocol, maintaining clear protocol semantics.
124-143: Consistent error variant usage for signature share generation.State validation uses
FrostError::Protocol, missing nonces useFrostError::Protocol, and signing failures useFrostError::SigningFailed. This provides clear differentiation between protocol state errors and cryptographic operation failures.
145-178: LGTM - Signature share addition and aggregation errors.Duplicate detection and state validation use
FrostError::Protocol, and aggregation failures correctly propagate as protocol errors with the underlying cause.
206-267: LGTM -sign_with_local_shareserror handling migration.All error conditions appropriately use
FrostErrorvariants:
- Empty shares →
FrostError::Protocol- Insufficient signers →
FrostError::InsufficientSignerswith structured fields- Missing nonces →
FrostError::Protocol- Signing failures →
FrostError::SigningFailed- Aggregation/serialization failures →
FrostError::Protocolkeep-core/src/lib.rs (7)
25-25: LGTM - Updated imports for domain-specific error types.The import includes
FrostError,KeyError, andStorageErrorfor the new hierarchical error system.
78-80: LGTM - Locked state errors consistently useKeepError::Key(KeyError::Locked).All methods requiring an unlocked state consistently check and return the wrapped
KeyError::Lockedvariant, providing clear error categorization.Also applies to: 110-111, 177-178, 200-201, 244-245, 256-257, 273-274, 292-293, 302-303
117-118: LGTM - Duplicate key usesKeepError::Key(KeyError::AlreadyExists).The
import_nsecmethod correctly returnsKeyError::AlreadyExistswhen attempting to import a key that already exists in the keyring.
206-207: LGTM - Not found errors useKeepError::Key(KeyError::NotFound).Missing keys, shares, and groups consistently use
KeyError::NotFoundwith descriptive messages.Also applies to: 234-235, 281-282, 313-314
319-324: LGTM - Insufficient signers usesKeepError::Frost(FrostError::InsufficientSigners).The
frost_signmethod correctly wraps theFrostError::InsufficientSignersvariant underKeepError::Frost, maintaining the hierarchical structure.
355-356: LGTM - Internal data key access returns wrappedKeyError::Locked.The
get_data_keymethod correctly returnsKeepError::Key(KeyError::Locked)when the vault is not unlocked, with updated documentation.Also applies to: 362-362
371-374: LGTM - Home directory error usesKeepError::Storage(StorageError::HomeNotFound).The
default_keep_pathfunction correctly uses the storage-specific error variant for missing home directory, aligning with the domain-specific error hierarchy.keep-core/src/hidden/volume.rs (10)
41-41: LGTM!The import correctly brings in the new domain-specific error types required for the error hierarchy refactoring.
76-101: LGTM!The storage-related error conversions are appropriate:
AlreadyExistsfor path collisionVolumeSizeOverflowfor arithmetic overflow protectionVolumeTooSmallwith named fields for clear error reporting
215-218: LGTM!
StorageError::NotFoundis the correct domain-specific error for a missing vault path.
274-291: LGTM!The rate limiting and authentication error handling correctly uses the new error hierarchy:
CryptoError::RateLimitedfor throttling- Pattern matching against wrapped
KeepError::Crypto(...)variants for recording failures
323-333: LGTM!Returning
CryptoError::InvalidPasswordfor both decryption failure and checksum mismatch is correct—this maintains the documented security property that a wrong hidden password produces the same error as "no hidden volume."
354-416: LGTM!The
unlock_hidden()andunlock()methods consistently apply the new error hierarchy with proper rate limiting checks and authentication error handling.
446-451: LGTM!
KeepError::Key(KeyError::Locked)is the correct error for operations attempted on a locked volume.
618-620: LGTM!
KeyError::NotFoundwith the hex-encoded key ID provides clear error context for debugging while using the appropriate domain-specific error type.Also applies to: 633-635
543-545: LGTM!
StorageError::VolumeFullcorrectly indicates when the hidden volume's data area is exhausted.
919-965: LGTM!Test assertions correctly verify the new error hierarchy structure, checking for both the wrapper (
KeepError::Crypto) and the specific variant (CryptoError::RateLimited,CryptoError::InvalidPassword).
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 (1)
keep-core/src/storage.rs (1)
75-86: Same truncation issue:version as u8.Like in
header.rs, this castsu16tou8, potentially losing information for versions > 255. This should be consistent with whatever decision is made forheader.rs.
🤖 Fix all issues with AI agents
In @keep-core/src/hidden/header.rs:
- Around line 103-110: The UnsupportedVersion error currently takes a u8 which
causes truncation when a u16 version >255 is passed; update the enum/variant
definition StorageError::UnsupportedVersion in error.rs to accept a u16 instead
of u8, then remove the casts to u8 where that variant is constructed (e.g., in
hidden::header.rs where local variable version: u16 is passed, and in
keep-core/src/storage.rs around the previous line 85) so you pass the raw u16
version value into UnsupportedVersion. Ensure all call sites and pattern matches
referencing UnsupportedVersion are updated to use u16.
In @keep-core/src/hidden/volume.rs:
- Around line 92-94: Replace the unnecessary closure using ok_or_else for the
unit variant: where required_min is computed from
DATA_START_OFFSET.checked_add(hidden_size), change the call from ok_or_else(||
StorageError::VolumeSizeOverflow) to ok_or(StorageError::VolumeSizeOverflow) so
clippy no longer flags the unnecessary closure for the unit enum variant.
🧹 Nitpick comments (2)
keep-core/src/hidden/header.rs (1)
262-268: Inconsistent error wrapping pattern.Here
KeepError::Storage(StorageError::HeaderSizeMismatch {...})is used explicitly, while elsewhere in this fileStorageError::InvalidMagic.into()is used. Consider using.into()consistently for cleaner code:- return Err(KeepError::Storage(StorageError::HeaderSizeMismatch { - expected: Self::COMPACT_SIZE, - actual: bytes.len(), - })); + return Err(StorageError::HeaderSizeMismatch { + expected: Self::COMPACT_SIZE, + actual: bytes.len(), + } + .into());keep-core/src/lib.rs (1)
235-241: Consider usingFrostErrorfor missing FROST shares.The error for a missing share currently uses
KeyError::NotFound, but since this is FROST-specific functionality,FrostErrormight be more semantically appropriate for domain consistency. This would align better with the error taxonomy where FROST-related issues useFrostError.That said, if shares are conceptually treated as key material throughout the codebase,
KeyErrorcould be intentional.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
keep-core/src/frost/dealer.rskeep-core/src/frost/share.rskeep-core/src/frost/signing.rskeep-core/src/frost/transport.rskeep-core/src/hidden/header.rskeep-core/src/hidden/volume.rskeep-core/src/keyring.rskeep-core/src/keys.rskeep-core/src/lib.rskeep-core/src/storage.rs
🧰 Additional context used
🧬 Code graph analysis (4)
keep-core/src/frost/signing.rs (1)
keep-core/src/frost/dealer.rs (2)
new(18-35)new(66-68)
keep-core/src/frost/transport.rs (2)
keep-core/src/crypto.rs (3)
derive_key(268-289)decrypt(233-240)decrypt(350-360)keep-core/src/frost/share.rs (2)
decrypt(125-135)key_package(85-89)
keep-core/src/frost/share.rs (1)
keep-cli/src/server.rs (1)
error(425-431)
keep-core/src/lib.rs (1)
keep-core/src/storage.rs (1)
data_key(252-254)
🪛 GitHub Actions: CI
keep-core/src/storage.rs
[error] 1-1: cargo clippy failed: could not compile keep-core due to 16 previous errors
keep-core/src/frost/signing.rs
[error] 29-29: value assigned to session_id is never read
[error] 31-31: value assigned to message is never read
[error] 33-33: value assigned to threshold is never read
[error] 35-35: value assigned to state is never read
[error] 38-38: value assigned to commitments is never read
[error] 40-40: value assigned to signature_shares is never read
[error] 45-45: value assigned to signature is never read
[error] 33-33: value assigned to threshold is never read
[error] 35-35: value assigned to state is never read
[error] 38-38: value assigned to commitments is never read
[error] 40-40: value assigned to signature_shares is never read
[error] 1-1: cargo clippy failed: could not compile keep-core due to 16 previous errors
keep-core/src/hidden/header.rs
[error] 1-1: cargo clippy failed: could not compile keep-core due to 16 previous errors
keep-core/src/keyring.rs
[error] 1-1: cargo clippy failed: could not compile keep-core due to 16 previous errors
keep-core/src/frost/transport.rs
[error] 1-1: cargo clippy failed: could not compile keep-core due to 16 previous errors
keep-core/src/frost/share.rs
[error] 58-58: value assigned to metadata is never read
[error] 61-61: value assigned to pubkey_package_bytes is never read
[error] 1-1: cargo clippy failed: could not compile keep-core due to 16 previous errors
keep-core/src/lib.rs
[error] 1-1: cargo clippy failed: could not compile keep-core due to 16 previous errors
keep-core/src/keys.rs
[error] 1-1: cargo clippy failed: could not compile keep-core due to 16 previous errors
keep-core/src/hidden/volume.rs
[error] 92-92: unnecessary closure used to substitute value for Option::None
[error] 1-1: cargo clippy failed: could not compile keep-core due to 16 previous errors
keep-core/src/frost/dealer.rs
[error] 1-1: cargo clippy failed: could not compile keep-core due to 16 previous errors
🔇 Additional comments (57)
keep-core/src/keys.rs (5)
8-8: LGTM: Import updated for new error hierarchy.The import now uses domain-specific
CryptoErrorandKeyErrortypes, aligning with the PR's goal of decomposing the monolithicKeepError.
35-36: LGTM: Appropriate error type for invalid secret key.Mapping
SigningKey::from_bytesfailure toCryptoError::InvalidSecretKeyis semantically correct for this cryptographic operation.
83-91: LGTM: Sign operation usesCryptoError::InvalidSigningKey.The error mapping is appropriate since this is a cryptographic signing operation failure.
148-162: LGTM:npub_to_byteserror handling mirrorsfrom_nsec.Consistent use of
KeyError::InvalidNpubfor all npub validation failures.
45-58: LGTM: Consistent error handling infrom_nsec.All validation failures (decode error, wrong HRP, wrong length) correctly map to
KeyError::InvalidNsec. The.into()conversion is valid becauseKeepErrorincludes aKey(#[from] KeyError)variant in the enum definition, which automatically implementsFrom<KeyError>forKeepErrorvia the#[from]attribute.keep-core/src/keyring.rs (4)
7-7: LGTM: Import updated forKeyError.Adding
KeyErrorto imports enables the domain-specific key-related error wrapping.
61-67: LGTM: Key operation errors properly wrapped.
KeyringFullandAlreadyExistsare correctly wrapped inKeepError::Key(KeyError::...), providing a clear error hierarchy.
103-113: LGTM:set_primaryandremoveuse consistent error wrapping.Both methods use
KeepError::Key(KeyError::NotFound(...))for missing keys, maintaining consistency.
190-193: LGTM: Test updated to match new error structure.The test correctly expects the wrapped error variant
KeepError::Key(KeyError::KeyringFull(_)).keep-core/src/hidden/header.rs (3)
6-6: LGTM: Import updated forStorageError.The import enables domain-specific storage error handling for header parsing.
131-166: LGTM: Field-specific error messages for header parsing.Using
StorageError::InvalidHeader { field: "..." }provides clear diagnostics for which header field failed to parse. Thetry_into()conversions for fixed-size arrays are infallible given the slice lengths, but the error handling is defensive.
290-307: LGTM: Hidden header field parsing errors.Consistent use of
StorageError::InvalidHeaderwith specific field names for the hidden header's compact format.keep-core/src/hidden/volume.rs (10)
41-41: LGTM: Imports updated for new error types.Adding
CryptoError,KeyError, andStorageErrorenables the domain-specific error handling throughout this file.
75-101: LGTM: Storage error types for volume creation validation.
AlreadyExists,VolumeSizeOverflow, andVolumeTooSmallare appropriateStorageErrorvariants for these storage-level validation failures.
215-218: LGTM:StorageError::NotFoundfor missing path.Correct error type for a file-system level "not found" condition.
269-294: LGTM: Rate limiting and authentication error handling.The
unlock_outermethod correctly:
- Returns
CryptoError::RateLimitedwhen rate limit is exceeded- Records failure only on authentication-related errors (
InvalidPasswordorDecryptionFailed)
326-336: LGTM: Hidden volume decryption errors.Returning
CryptoError::InvalidPasswordfor both decryption failure and checksum mismatch maintains plausible deniability by not revealing whether a hidden volume exists.
385-425: LGTM: Combined unlock with proper error classification.The
is_auth_errorhelper correctly identifies authentication failures, and the method returnsCryptoError::InvalidPasswordwhen both volumes fail authentication.
455-471: LGTM:KeyError::Lockedfor locked state.Using
KeepError::Key(KeyError::Locked)consistently when operations are attempted on a locked volume is semantically correct.
569-572: LGTM:StorageError::VolumeFullfor capacity exceeded.Appropriate error type when the hidden volume cannot accommodate more data.
654-656: LGTM:KeyError::NotFoundfor missing keys.Consistent use of
KeepError::Key(KeyError::NotFound(...))when a key ID is not found in the storage.Also applies to: 674-677
965-1000: LGTM: Tests updated for new error types.The test assertions correctly use
matches!with the new wrapped error variants likeKeepError::Crypto(CryptoError::RateLimited(_))andKeepError::Crypto(CryptoError::InvalidPassword).keep-core/src/storage.rs (9)
10-10: LGTM: Imports updated for domain-specific error types.The import enables
CryptoError,KeyError, andStorageErrorusage throughout the storage module.
122-125: LGTM:StorageError::AlreadyExistsfor existing path.Correct domain-specific error for attempting to create storage at an existing path.
167-181: LGTM: Storage validation errors.
NotFoundandHeaderSizeMismatchare appropriateStorageErrorvariants for these validation checks duringopen().
195-228: LGTM: Unlock flow with rate limiting and error handling.The unlock method correctly:
- Checks rate limit and returns
CryptoError::RateLimited- Records failure only on
DecryptionFailed(authentication failure)- Records success on successful unlock
256-262: LGTM:KeyError::Lockedfor locked storage operations.Consistently returns
KeepError::Key(KeyError::Locked)when attempting operations on locked storage.
279-292: LGTM:load_keyerror handling.Returns
KeyError::Lockedwhen storage is locked andKeyError::NotFoundwhen the key ID doesn't exist.
328-342: LGTM:delete_keyerror handling.Correctly checks for locked state first, then returns
KeyError::NotFoundif the key wasn't in the database.
403-422: LGTM: Share deletion error handling.The
delete_sharemethod correctly usesKeyError::NotFoundwith a descriptive message for missing shares.
505-527: LGTM: Rate limiting tests updated.Tests correctly assert the new error variants
CryptoError::DecryptionFailedandCryptoError::RateLimited.keep-core/src/lib.rs (5)
25-25: LGTM! Import updated for new error hierarchy.The import correctly brings in the new domain-specific error types (
FrostError,KeyError,StorageError) alongsideKeepErrorandResult.
78-81: LGTM! Consistent error handling for locked state.The error migration from
KeepError::LockedtoKeepError::Key(KeyError::Locked)is applied consistently across all methods that check the unlock state.
369-377: LGTM! Data key access error handling updated.The
get_data_keymethod correctly returns the new hierarchical error type, and the docstring is updated to reflect the change. The error semantics are appropriate since a missing data key indicates the Keep is locked.
380-385: LGTM! Storage error for path resolution.The
default_keep_pathfunction correctly usesStorageError::HomeNotFoundfor the missing home directory case, which is semantically appropriate as a storage/filesystem concern.
326-331: LGTM! Well-structured error with explicit fields.The
FrostError::InsufficientSignersvariant withneededandavailablefields is a good design choice. It makes the error information programmatically accessible rather than embedding it in a formatted string.keep-core/src/frost/share.rs (4)
11-11: LGTM! Import updated for FrostError.The import correctly switches from
KeepErrortoFrostErrorfor protocol-level error handling in this module.
70-76: LGTM! Consistent serialization error handling.Both
key_packageandpubkey_packageserialization failures correctly map toFrostError::Protocolwith descriptive error messages.
85-95: LGTM! Deserialization errors properly converted.The
.into()calls correctly convertFrostError::Protocolto the crate'sResulterror type. This follows the error hierarchy pattern where domain-specific errors implementInto<KeepError>.
55-62: Pipeline warnings about unused fields are false positives.Both
metadataandpubkey_package_bytesare actively read in the struct's methods:metadatais accessed at lines 103, 119, and 131;pubkey_package_bytesis deserialized at line 92 and cloned at lines 121 and 133. The "never read" warnings likely originate from the compilation context or test code and do not reflect actual usage in the production code.keep-core/src/frost/transport.rs (6)
7-7: LGTM! Import updated for FrostError.The import correctly brings in
FrostErrorfor protocol-level error handling in the transport module.
31-33: LGTM! Serialization error handling.The key package serialization error correctly maps to
FrostError::Protocolwith a descriptive message.
49-89: LGTM! Comprehensive validation with consistent error handling.The
to_sharemethod has thorough validation for version, salt/nonce/ciphertext lengths, and group pubkey. All validation failures consistently useFrostError::Protocolwith theINVALID_SHAREconstant for generic cases and more specific messages where appropriate (e.g., version mismatch).
107-142: LGTM! JSON and Bech32 error handling.The JSON serialization/deserialization and Bech32 encoding/decoding methods consistently use
FrostError::Protocolwith descriptive error messages that include the underlying error details.
195-219: LGTM! FrostMessage error handling.The
FrostMessagemethods consistently useFrostError::Protocolfor JSON and hex decoding errors. Thesession_id_bytesmethod properly validates both hex decoding and length constraints.
246-283: LGTM! Animated frames parsing with comprehensive error handling.The
from_animated_framesmethod properly validates each step of the parsing process with appropriateFrostError::Protocolerrors for empty input, invalid JSON, missing fields, invalid hex, and UTF-8 decoding failures.keep-core/src/frost/dealer.rs (5)
7-7: LGTM! Import updated for FrostError.The import correctly brings in
FrostErrorfor FROST-specific error handling.
17-35: LGTM! Configuration validation with InvalidConfig variant.The
ThresholdConfig::newmethod appropriately usesFrostError::InvalidConfigfor all configuration validation errors. This semantic distinction fromFrostError::Protocol(used for runtime errors) is a good design choice.
70-103: LGTM! Key generation error handling.The
generatemethod correctly usesFrostError::Protocolfor runtime operation failures (key generation and KeyPackage conversion), with error messages that include the underlying error details.
106-148: LGTM! Key split error handling.The
split_existingmethod follows the same error handling pattern asgenerate, consistently usingFrostError::Protocolfor runtime operation failures.
151-170: LGTM! Group pubkey extraction with format handling.The
extract_group_pubkeyfunction properly handles both 33-byte (compressed) and 32-byte (x-only) public key formats. The explicit match with a descriptive error for unexpected lengths is a good defensive practice.keep-core/src/frost/signing.rs (6)
16-16: LGTM! Import updated for FrostError.The import correctly brings in
FrostErrorfor signing-related error handling.
84-121: LGTM! Commitment phase error handling.The commitment methods consistently use
FrostError::Protocolfor session state violations and duplicate commitment errors. Error messages clearly indicate the issue.
127-183: LGTM! Signature phase error handling with appropriate variants.The signature share methods make good semantic distinctions:
FrostError::Protocolfor workflow/state errorsFrostError::SigningFailedfor cryptographic signing failures- Session state correctly transitions to
Failedon aggregation errors
189-204: LGTM! Clean refactoring of signature_bytes.The method uses the
let-elsepattern for cleaner early return handling. Serialization and length validation errors appropriately useFrostError::Protocol.
211-272: LGTM! Comprehensive error handling in sign_with_local_shares.The function uses appropriate error variants:
FrostError::Protocolfor no shares providedFrostError::InsufficientSignerswith structuredneeded/availablefields for threshold violationsFrostError::SigningFailedfor cryptographic signing errorsFrostError::Protocolfor aggregation and serialization errors
26-46: AllSigningSessionstruct fields are legitimately used in methods—the clippy warnings are false positives.The fields marked with
#[zeroize(skip)](session_id, message, threshold, state, commitments, signature_shares, signature) are all read through getter methods or accessed directly in field operations. Thesession_id()method returns a reference to the field;state()returns the state value;commitments()andsignature_shares()return references; andsignatureis read via thesignature_bytes()method. If clippy warnings persist, suppress them locally using#[allow(unused_assignments)]on individual fields or the struct itself.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @keep-core/src/frost/coordinator.rs:
- Around line 17-18: The #[allow(unused_assignments)] attribute on the Zeroize
derive is incorrect; remove that attribute and apply the same fix used in
signing.rs: stop silencing the wrong lint and instead add the precise allow or
suppression that targets the generated zeroize code (mirror the exact change
from signing.rs for the #[derive(Zeroize, ZeroizeOnDrop)] use). Update the
attribute on the derive for the coordinator (the #[derive(Zeroize,
ZeroizeOnDrop)] block) to match the corrected lint suppression used in
signing.rs.
In @keep-core/src/frost/signing.rs:
- Around line 26-27: The attribute on the item with #[derive(ZeroizeOnDrop)] is
wrong: replace #[allow(unused_assignments)] with #[allow(dead_code)] because the
ZeroizeOnDrop derive causes fields to be read only for side effects (zeroing)
which triggers dead_code warnings; update the attribute adjacent to the
ZeroizeOnDrop derive accordingly so the compiler suppresses dead_code rather
than unused_assignments.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
keep-core/src/frost/coordinator.rskeep-core/src/frost/share.rskeep-core/src/frost/signing.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- keep-core/src/frost/share.rs
🧰 Additional context used
🧬 Code graph analysis (1)
keep-core/src/frost/signing.rs (3)
keep-core/src/frost/coordinator.rs (2)
new(36-46)aggregate(157-172)keep-core/src/frost/share.rs (3)
new(26-43)new(66-84)key_package(86-90)keep-core/src/frost/dealer.rs (2)
new(18-35)new(66-68)
🪛 GitHub Actions: CI
keep-core/src/frost/coordinator.rs
[error] 21-21: value assigned to session_id is never read
[error] 23-23: value assigned to message is never read
[error] 25-25: value assigned to threshold is never read
[error] 27-27: value assigned to commitments is never read
[error] 29-29: value assigned to signature_shares is never read
[error] 31-31: value assigned to our_identifier is never read
[error] 30-30: value assigned to session_id is never read
[error] 32-32: value assigned to message is never read
[error] 34-34: value assigned to threshold is never read
[error] 36-36: value assigned to state is never read
[error] 39-39: value assigned to commitments is never read
[error] 41-41: value assigned to signature_shares is never read
[error] 46-46: value assigned to signature is never read
keep-core/src/frost/signing.rs
[error] 30-30: value assigned to session_id is never read
[error] 32-32: value assigned to message is never read
[error] 34-34: value assigned to threshold is never read
[error] 36-36: value assigned to state is never read
[error] 39-39: value assigned to commitments is never read
[error] 41-41: value assigned to signature_shares is never read
[error] 46-46: value assigned to signature is never read
🔇 Additional comments (15)
keep-core/src/frost/signing.rs (8)
16-16: LGTM - Import change aligns with error refactoring.The import correctly brings in
FrostErrorandResultfrom the crate's error module, consistent with the domain-specific error hierarchy introduced by this PR.
85-104: LGTM - Error handling in generate_commitment.The error paths correctly use
FrostError::Protocolfor invalid session states. The control flow and nonce/commitment generation logic remain unchanged.
106-122: LGTM - Error handling in add_commitment.Duplicate commitment and invalid state errors are properly mapped to
FrostError::Protocol. State transition logic toCollectingSharesis preserved.
128-149: LGTM - Error handling in generate_signature_share.Error variants are appropriately differentiated:
FrostError::Protocolfor state/nonce issuesFrostError::SigningFailedfor cryptographic signing failuresThis distinction aids debugging by categorizing error sources.
151-184: LGTM - Error handling in add_signature_share.Aggregation failures correctly transition the session to
SessionState::Failedand returnFrostError::Protocol. The logic for collecting shares and triggering aggregation is preserved.
190-205: LGTM - Refactored signature_bytes with cleaner pattern matching.The
let Some(...) elsepattern improves readability. Error handling for serialization and length validation correctly usesFrostError::Protocol.
212-226: LGTM - Appropriate use of FrostError::InsufficientSigners.Using a specific error variant with
neededandavailablefields provides better diagnostics than a generic protocol error. This aligns with the PR's goal of introducing domain-specific error types.
246-268: LGTM - Consistent error handling in sign_with_local_shares.The function correctly uses:
FrostError::Protocolfor missing noncesFrostError::SigningFailedfor signing failuresFrostError::Protocolfor aggregation and serialization errorsThis matches the patterns established in
SigningSessionmethods.keep-core/src/frost/coordinator.rs (7)
14-14: LGTM - Import change aligns with error refactoring.Consistent with the pattern in
signing.rs.
52-71: LGTM - Error handling in add_local_share.Serialization errors for commitments correctly use
FrostError::Protocol. The nonce and commitment generation logic is preserved.
73-91: LGTM - Error handling in add_remote_commitment.Validation errors for message type, session ID, commitment deserialization, and identifier parsing all correctly use
FrostError::Protocolwith descriptive messages.
97-131: LGTM - Error handling in generate_signature_share.Error paths for insufficient commitments, missing identifier/nonces, and signing failures all use
FrostError::Protocol. The signing flow is preserved.
133-151: LGTM - Error handling in add_remote_signature_share.Consistent with
add_remote_commitment- usesFrostError::Protocolfor validation and deserialization errors.
157-172: LGTM - Error handling in aggregate.The aggregation error path matches the pattern in
signing.rsand the relevant code snippet from this file's context.
174-188: LGTM - Error handling in aggregate_to_bytes.Serialization and length validation errors correctly use
FrostError::Protocol. The pattern matchessignature_bytesinsigning.rs.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @keep-cli/src/commands/serve.rs:
- Around line 63-69: The current mapping collapses all keep_frost_net errors
into FrostError::Protocol for both KfpNode::new() and node.announce(); update
the error handling to distinguish transport/connectivity errors from protocol
errors by matching the FrostNetError variants returned by KfpNode::new and
node.announce: introduce a new FrostError::Network (or FrostError::Transport)
variant and map FrostNetError::Transport and Timeout (or other network-related
variants) to that, while keeping serialization/deserialization or protocol-level
errors mapped to FrostError::Protocol; adjust the .map_err closures around
KfpNode::new() and node.announce() to perform this match and return the
appropriate FrostError variant instead of always using FrostError::Protocol.
🧹 Nitpick comments (3)
keep-cli/src/signer/handler.rs (2)
77-89: Inconsistent error type for FROST pubkey conversions.Lines 77 and 81 correctly use
FrostError::InvalidFrostPubkeyfor FROST group pubkey conversions. However, inhandle_sign_event(line 187 and 212), the samePublicKey::from_slice(signer.group_pubkey())operation usesKeyError::InvalidPubkeyinstead.For consistency, consider using
FrostError::InvalidFrostPubkeyfor all FROST group pubkey conversions, reservingKeyError::InvalidPubkeyfor non-FROST key operations (like line 86).
196-199: Consider moving signature-related errors to domain-specific types.
KeepError::EventIdFailedandKeepError::InvalidSignatureremain as genericKeepErrorvariants. For consistency with the domain-specific error hierarchy introduced in this PR, consider whether these should be moved toFrostErrororCryptoErrorvariants.Also applies to: 221-224
keep-cli/src/commands/frost_network.rs (1)
892-892: Consider usingCryptoErrorfor encryption failures.
NetworkError::Encryptis used for NIP-44 encryption failures during DKG. Since encryption is a cryptographic operation rather than a network operation,CryptoErrormight be more semantically appropriate. However, since the encryption occurs in the context of preparing network messages, this is a reasonable design choice.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
keep-cli/src/commands/frost.rskeep-cli/src/commands/frost_hardware.rskeep-cli/src/commands/frost_network.rskeep-cli/src/commands/serve.rskeep-cli/src/commands/vault.rskeep-cli/src/signer/handler.rs
🧰 Additional context used
🧬 Code graph analysis (5)
keep-cli/src/commands/frost.rs (2)
keep-core/src/frost/dealer.rs (2)
new(18-35)new(66-68)keep-core/src/frost/transport.rs (2)
from_json(112-115)from_json(200-203)
keep-cli/src/commands/serve.rs (4)
keep-core/src/frost/share.rs (2)
new(27-44)new(66-84)keep-core/src/frost/dealer.rs (2)
new(18-35)new(66-68)keep-core/src/lib.rs (1)
data_key(375-377)keep-core/src/storage.rs (1)
data_key(252-254)
keep-cli/src/commands/vault.rs (2)
keep-core/src/lib.rs (1)
data_key(375-377)keep-core/src/storage.rs (1)
data_key(252-254)
keep-cli/src/commands/frost_network.rs (2)
keep-core/src/frost/share.rs (2)
new(27-44)new(66-84)keep-core/src/frost/dealer.rs (3)
new(18-35)new(66-68)generate(70-104)
keep-cli/src/commands/frost_hardware.rs (5)
keep-cli/src/server.rs (1)
error(425-431)keep-core/src/frost/share.rs (1)
pubkey_package(92-96)keep-core/src/frost/transport.rs (1)
session_id_bytes(210-219)keep-core/src/frost/coordinator.rs (1)
session_id(48-50)keep-core/src/frost/signing.rs (1)
session_id(77-79)
🔇 Additional comments (34)
keep-cli/src/commands/vault.rs (6)
8-8: LGTM!Import correctly adds
KeyErrorto support the new domain-specific error types used throughout this file.
33-63: LGTM!Good categorization of errors:
KeepError::Inputfor validation failures (password length, mismatch)KeepError::Runtimefor IO failures (password read)This improves error handling granularity and aligns with the domain-specific error hierarchy.
149-149: LGTM!Using
KeyError::Lockedwhendata_key()returnsNoneis semantically correct—the vault hasn't been unlocked yet. This pattern is applied consistently acrosscmd_generate_outer,cmd_generate_hidden,cmd_import_outer, andcmd_import_hidden.
475-478: LGTM!
KeyError::NotFound(name.into())provides meaningful error context when a key lookup fails. This pattern is applied consistently across all export and delete commands.
508-516: LGTM!Correct error flow:
KeyError::NotFoundwhen the key doesn't exist,KeyError::Lockedwhen the vault data key is unavailable. Same pattern correctly applied incmd_export_hidden.
631-635: TheFrom<KeyError>implementation is correctly in place via the#[from]attribute onKeepError::Key(line 231 in keep-core/src/error.rs), which automatically generates the conversion. The?operator at line 635 will properly convertKeyErrortoKeepError. The code is correct and will compile as expected.keep-cli/src/signer/handler.rs (5)
10-10: LGTM!The expanded imports for domain-specific error types align well with the refactoring goals of this PR.
114-114: LGTM!Using
PolicyError::PermissionDeniedfor permission-related errors is appropriate and improves error categorization.
240-246: LGTM!The error mappings for secret key validation (
CryptoError::InvalidSecretKey) and signing failures (FrostError::SigningFailed) are appropriate for their contexts.
278-307: LGTM!The
handle_nip44_encryptfunction correctly uses:
PolicyError::PermissionDeniedfor permission checksKeyError::NoSigningKeyfor missing keysCryptoError::InvalidSecretKeyfor invalid secret keysKeepError::Nip44for NIP-44 specific errorsThe error taxonomy is consistent and appropriate.
340-369: LGTM!The NIP-04 handlers follow the same consistent pattern as NIP-44, using appropriate domain-specific error types.
keep-cli/src/commands/frost_network.rs (7)
7-7: LGTM!The import of
FrostError,NetworkErroralong withKeepErrorandResultsets up the domain-specific error types used throughout this file.
49-57: LGTM!Appropriate error categorization:
KeepError::Runtimefor tokio runtime creation failuresFrostError::Protocolfor FROST node initialization errors
209-229: LGTM!Hardware-related errors are properly categorized:
KeepError::HardwareConnectionfor connection failuresKeepError::HardwareOperationwith structuredopandreasonfields for operation failuresFrostError::InvalidConfigfor threshold validationThis provides clear error context for debugging hardware signer issues.
321-327: LGTM!Using
FrostError::InsufficientSignerswithneededandavailablefields provides actionable error information when peer discovery fails.
406-422: LGTM!The commitment flow uses appropriate error types:
FrostError::CommitmentFailedfor commitment generation errorsFrostError::Protocolfor nonce reuse detection (security-critical)KeepError::Runtimefor nonce tracking failures
443-447: LGTM!Network operations correctly use
NetworkErrorvariants:
NetworkError::RelayAddfor relay connection errorsNetworkError::SignRequestfor event signing errorsNetworkError::Publishfor event publishing errorsThis provides clear categorization of network-related failures.
735-745: LGTM!DKG errors use
FrostError::DkgFailedwith structuredstageandreasonfields, providing clear context for which DKG stage failed and why.keep-cli/src/commands/frost.rs (6)
6-6: LGTM!The import of
FrostErrorandKeyErroralongsideKeepErrorenables the domain-specific error handling throughout this file.
42-44: LGTM!Using
FrostError::Protocolfor the edge case wherefrost_generateorfrost_splitreturns an empty shares vector is appropriate, as this indicates an unexpected protocol-level failure.Also applies to: 92-94
222-232: LGTM!The input handling correctly uses:
KeepError::Runtimefor I/O read errorsKeepError::Inputfor size limit violationsThis provides clear distinction between system errors and user input errors.
382-399: LGTM!Appropriate error types for share validation:
KeyError::NotFoundfor missing shares (key-related error)FrostError::InsufficientSignerswith structured fields for threshold violationsThe structured
InsufficientSignerserror withneededandavailablefields provides actionable diagnostic information.
449-491: LGTM!The interactive signing round 1 uses appropriate error types:
FrostError::Protocolfor serialization errorsKeepError::Runtimefor I/O read errorsFrostError::InvalidSessionIdfor session mismatchFrostError::CommitmentFailedfor commitment deserializationThe error categorization clearly distinguishes protocol errors from runtime/I/O errors.
497-552: LGTM!The interactive signing round 2 and aggregation phase correctly use:
FrostError::SigningFailedfor signing errorsFrostError::Protocolfor serialization/validation errorsFrostError::InvalidSessionIdfor session mismatchesThe error handling is comprehensive and consistent with the domain-specific error hierarchy.
keep-cli/src/commands/frost_hardware.rs (6)
6-6: LGTM!The import of
FrostErroralongsideKeepErrorenables domain-specific error handling for FROST protocol errors in hardware operations.
21-29: LGTM!The ping command uses appropriate error categorization:
KeepError::HardwareConnectionfor connection failuresKeepError::HardwareOperationwith structuredopandreasonfields for ping failuresThis provides clear debugging context for hardware signer issues.
43-54: LGTM!Consistent error handling pattern for hardware list operations, matching the structure used in
cmd_frost_hardware_ping.
113-129: LGTM!Using
FrostError::Protocolfor FROST-specific serialization errors (secret share, verifying share, group key) is appropriate as these are protocol-level data format errors.
145-164: LGTM!The import flow correctly categorizes errors:
KeepError::HardwareConnectionfor connectionKeepError::HardwareOperationwithop: "ping"andop: "import_share"for respective operationsThis maintains consistency with the error patterns established elsewhere in the file.
209-235: LGTM!The hardware sign command uses appropriate error types:
KeepError::Inputfor session_id hex validation (user input error)KeepError::HardwareConnectionfor connection failuresKeepError::HardwareOperationwithop: "frost_sign"for signing failuresThe error categorization is consistent and provides clear context for each failure mode.
keep-cli/src/commands/serve.rs (4)
10-10: LGTM!Import statement correctly expanded to include
FrostErrorandKeyError, both of which are used in the refactored error handling paths below.
58-59: LGTM — consistent runtime error categorization.The mapping from tokio runtime creation errors to
KeepError::Runtimeis appropriate and applied consistently across all occurrences in this file (lines 59, 124, 227, 261, 323, 359, 421).
245-245: LGTM — correct domain-specific error type for locked key state.Using
KeyError::Lockedinstead ofKeepError::Lockedproperly categorizes this as a key-related error, aligning with the hierarchical error structure. The same pattern at line 343 is equally correct.
227-228: LGTM!TUI execution errors appropriately categorized as
KeepError::Runtime. The pattern is consistent across all three command functions.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Refactors flat KeepError enum into hierarchical error system with StorageError, CryptoError, KeyError, FrostError, NetworkError, and PolicyError types. Each error has unique codes for debugging.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.