Skip to content

Comments

Remove traits and refactor tests#185

Merged
jking-aus merged 6 commits intosigp:unstablefrom
diegomrsantos:remove-network-state-trait
Mar 18, 2025
Merged

Remove traits and refactor tests#185
jking-aus merged 6 commits intosigp:unstablefrom
diegomrsantos:remove-network-state-trait

Conversation

@diegomrsantos
Copy link
Member

@diegomrsantos diegomrsantos commented Mar 13, 2025

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).

@diegomrsantos diegomrsantos self-assigned this Mar 13, 2025
@diegomrsantos diegomrsantos marked this pull request as ready for review March 13, 2025 13:43
@diegomrsantos diegomrsantos requested a review from dknopik March 13, 2025 13:44
@diegomrsantos diegomrsantos changed the title Remove network state trait and refactor tests Remove traits and refactor tests Mar 13, 2025
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.

Very good. I am very happy about this solution! :)

LGTM, minor notes below.

@diegomrsantos diegomrsantos added the ready-for-review This PR is ready to be reviewed label Mar 14, 2025
@diegomrsantos diegomrsantos requested a review from Copilot March 15, 2025 14:57
Copy link
Contributor

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

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) {

jking-aus
jking-aus previously approved these changes Mar 18, 2025
Copy link
Member

@jking-aus jking-aus left a comment

Choose a reason for hiding this comment

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

lgtm -- nice work diego, just hit those conflicts and I'll merge

# 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
Copy link
Contributor

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

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");

@jking-aus
Copy link
Member

nice lgtm

@jking-aus jking-aus merged commit 9586330 into sigp:unstable Mar 18, 2025
10 checks passed
@dknopik dknopik removed the ready-for-review This PR is ready to be reviewed label Nov 7, 2025
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