Remove traits and refactor tests#185
Conversation
dknopik
left a comment
There was a problem hiding this comment.
Very good. I am very happy about this solution! :)
LGTM, minor notes below.
There was a problem hiding this comment.
Pull Request Overview
This pull request refactors the codebase by removing legacy traits and simplifying the logic for handling network state and message reception while fixing the committee info computation for the validator role. Key changes include:
- Removal of traits (NetworkStateService, ValidatorService, and the old MessageReceiver) and replacing them with more specific implementations.
- Updates to various modules (database, network, client, and message_receiver) to align with the new design, including changes in error handling and dependency management.
- Introduction of a dedicated CommitteeInfo structure in ssv_types to better encapsulate committee information.
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| anchor/database/src/state.rs | Refactored network state handling with new helper functions for committee info computation. |
| anchor/message_receiver/src/lib.rs | Simplified the MessageReceiver interface and removed the testing module from public exports. |
| anchor/network/src/network.rs | Updated the Network struct to use a concrete Arc and removed generic parameters. |
| anchor/common/ssv_types/src/committee.rs | Added CommitteeInfo for grouping committee members with validator indices. |
| anchor/message_receiver/src/manager.rs | Renamed ManagerMessageReceiver to MessageReceiver and updated the receive method signature accordingly. |
| anchor/network/Cargo.toml | Updated dependencies to remove redundant features and ensure workspace consistency. |
| anchor/client/src/lib.rs | Refactored to use the updated MessageReceiver and network state handling from the database. |
| anchor/database/src/lib.rs | Removed WatchableNetworkState re-export and now re-export only NetworkState. |
| anchor/common/ssv_types/src/partial_sig.rs | Added PartialEq and Eq derives to PartialSignatureKind for better value comparisons. |
| anchor/common/ssv_types/src/lib.rs | Updated re-exports to include CommitteeInfo. |
| anchor/message_receiver/src/testing.rs | Removed the testing module related to the legacy MessageReceiver interface. |
Comments suppressed due to low confidence (2)
anchor/message_receiver/src/manager.rs:23
- [nitpick] The name 'MessageReceiver' is quite generic; consider renaming it to better reflect its role in managing and dispatching messages.
pub struct MessageReceiver {
anchor/network/src/network.rs:162
- [nitpick] The explicit clone of 'message_receiver' may be redundant if its 'receive' method can operate on a shared reference; review if cloning is necessary to avoid potential performance overhead.
if let Err(err) = self.message_receiver.clone().receive(propagation_source, message_id, message) {
# Conflicts: # anchor/client/src/lib.rs # anchor/message_receiver/src/lib.rs # anchor/message_receiver/src/testing.rs # anchor/message_validator/src/lib.rs # anchor/network/Cargo.toml # anchor/network/src/network.rs
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the code by removing several traits (NetworkStateService, ValidatorService, and MessageReceiver) and simplifying the related test implementations while also fixing the committee info computation for the validator role. Key changes include:
- Removing unused traits and simplifying state and message receiver handling.
- Refactoring functions in the state module to better compute committee and validator data.
- Updating Cargo.toml dependencies and related module re-exports to reflect the removal of traits.
Reviewed Changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| anchor/database/src/state.rs | Refactored network state handling and committee info functions. |
| anchor/message_receiver/src/lib.rs | Removed testing module and updated error handling using thiserror. |
| anchor/network/src/network.rs | Removed generic MessageReceiver; adjusted instantiation to use Arc. |
| anchor/network/Cargo.toml | Removed message_validator dependency from workspace and updated message_receiver usage. |
| anchor/common/ssv_types/src/committee.rs | Added CommitteeInfo struct and re-export changes. |
| anchor/common/ssv_types/src/msgid.rs | Changed the conversion implementation from MessageId to VariableList. |
| anchor/message_receiver/src/manager.rs | Updated MessageReceiver struct and implementation to remove generics. |
| anchor/client/src/lib.rs | Updated references to ManagerMessageReceiver and adjusted validator initialization. |
| anchor/database/src/lib.rs | Removed WatchableNetworkState re-exports in favor of using NetworkState directly. |
| anchor/common/ssv_types/src/partial_sig.rs | Added PartialEq and Eq derives to PartialSignatureKind enum. |
| anchor/common/ssv_types/src/consensus.rs | Minor cleanup in spacing. |
Comments suppressed due to low confidence (1)
anchor/common/qbft/src/lib.rs:119
- Typographical error: 'misisng' should be 'missing' in the warning message.
warn!("Data misisng for last prepared value");
|
nice lgtm |
Issue Addressed
This is mostly a refactoring related to #161
Proposed Changes
Removes traits (NetworkStateService, ValidatorService, and MessageReceiver) and simplifies the code.
Fixes the committee info computation for the validator role (sorry for the misleading title).