Skip to content

Conversation

diegomrsantos
Copy link
Member

@diegomrsantos diegomrsantos commented May 29, 2025

Issue Addressed

closes #317

Proposed Changes

Enhance error handling and logging by introducing typed error enums for share parsing, RSA parsing, and execution events, and replacing generic error messages with contextual variants.

  • Added ShareParseError and RsaParseError enums using thiserror for richer error information.
  • Extended ExecutionError with specific builders (invalid_operator_event, invalid_validator_event, etc.) and updated call sites accordingly.
  • Updated parsing and sync logic to use the new error variants and removed or adjusted debug log statements.

@diegomrsantos diegomrsantos self-assigned this May 29, 2025
@diegomrsantos diegomrsantos changed the title improve log for event processor Improve log for event processor May 29, 2025
@diegomrsantos diegomrsantos force-pushed the improve-log-event-processor branch 2 times, most recently from 7f11ba8 to d6942e0 Compare June 3, 2025 08:30
@diegomrsantos diegomrsantos requested a review from Copilot June 3, 2025 08:37
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Enhance error handling and logging by introducing typed error enums for share parsing, RSA parsing, and execution events, and replacing generic error messages with contextual variants.

  • Added ShareParseError and RsaParseError enums using thiserror for richer error information.
  • Extended ExecutionError with specific builders (invalid_operator_event, invalid_validator_event, etc.) and updated call sites accordingly.
  • Updated parsing and sync logic to use the new error variants and removed or adjusted debug log statements.

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.

Show a summary per file
File Description
eth/src/util.rs Introduced ShareParseError, updated parse_shares to return it.
eth/src/sync.rs Switched to ExecutionError::invalid_event builder for errors.
eth/src/network_actions.rs Added StringError wrapper, updated error constructors.
eth/src/event_processor.rs Replaced generic errors with contextual builders; removed logs.
eth/src/error.rs Expanded ExecutionError with new variants and builder methods.
eth/Cargo.toml Added thiserror as a dependency.
common/ssv_types/src/util.rs Introduced RsaParseError, updated parse_rsa to return it.
common/ssv_types/src/operator.rs Updated Operator::new to surface RsaParseError.
common/ssv_types/src/lib.rs Re-exported RsaParseError.
Comments suppressed due to low confidence (3)

anchor/eth/src/util.rs:37

  • The error annotation for EncryptedKeyLength is passing the literal string "actual" instead of the actual field; change the placeholder to {actual} so the runtime value is shown.
#[error("Encrypted key has wrong length: expected {}, got {}", ENCRYPTED_KEY_LENGTH, "actual")]

anchor/eth/src/event_processor.rs:442

  • The error message "Failed to validator cluster" is grammatically incorrect; consider updating it to something like "Failed to delete validator from cluster: {e}".
.map_err(|e| ExecutionError::Database(format!("Failed to validator cluster: {e}"))?);

anchor/eth/src/util.rs:50

  • [nitpick] New error variants in parse_shares (e.g., InvalidLength, PublicKeyCreation, EncryptedKeyLength) are not covered by existing tests; consider adding unit tests for each failure branch.
pub fn parse_shares(

@diegomrsantos diegomrsantos force-pushed the improve-log-event-processor branch from d6942e0 to 67b23f4 Compare June 3, 2025 08:45
@diegomrsantos diegomrsantos force-pushed the improve-log-event-processor branch from 67b23f4 to 9adeafa Compare June 3, 2025 08:52
@diegomrsantos diegomrsantos force-pushed the improve-log-event-processor branch from 4ab15e7 to 1b590f6 Compare June 3, 2025 11:05
@diegomrsantos diegomrsantos marked this pull request as ready for review June 3, 2025 11:19
Copy link
Member

@dknopik dknopik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for looking into this! I added some early thoughts

InvalidOperatorEvent {
operator_id: ssv_types::OperatorId,
owner: alloy::primitives::Address,
message: String,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having a message: String field in here feels like an anti pattern. I'd rather have a non Optional sub error for context

Comment on lines +40 to +45
#[error("Invalid event: {message}")]
InvalidEvent {
message: String,
#[source]
source: Option<Box<dyn std::error::Error + Send + Sync>>,
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of a variant per event type, maybe embed the event into this variant?

Comment on lines +16 to +17
#[source]
source: Option<Box<dyn std::error::Error + Send + Sync>>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is kind of emulating anyhow's context, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think anyhow's context is a message describing what we were trying to do when the function called failed. The source is the error that the function called returned.

@diegomrsantos diegomrsantos marked this pull request as draft June 3, 2025 13:47
@Zacholme7
Copy link
Member

We still planning to keep this or is it closable?

@diegomrsantos
Copy link
Member Author

We still planning to keep this or is it closable?

I wanna do it at some point

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants