Skip to content

feat: add serde to peer_id #10

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Apr 24, 2025
Merged

feat: add serde to peer_id #10

merged 4 commits into from
Apr 24, 2025

Conversation

sfroment
Copy link
Owner

Signed-off-by: Sacha Froment [email protected]

Copy link

@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

This pull request introduces serde support for peer ID serialization and deserialization by implementing Serialize and Deserialize for FixedSizePeerID, along with corresponding tests and updates in related modules.

  • Added a new serde module for peer ID encoding/decoding.
  • Renamed PeerID to FixedSizePeerID in several locations to align naming conventions.
  • Updated error handling and tests to use the new Error enum instead of ParsePeerIDError.

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
sf-peer-id/src/serde.rs Added serde implementations and tests for FixedSizePeerID.
sf-peer-id/src/peer_id.rs Updated FixedSizePeerID implementation and I/O functions for serde.
sf-peer-id/src/lib.rs Exposed new serde module and updated public API exports.
sf-peer-id/src/hex.rs Updated error type used in hex conversion functions.
sf-peer-id/src/error.rs Reworked error handling by replacing ParsePeerIDError with Error.
sf-peer-id/Cargo.toml Added serde and unsigned-varint dependencies with updated features.

Copy link

codecov bot commented Apr 24, 2025

Codecov Report

Attention: Patch coverage is 95.26627% with 8 lines in your changes missing coverage. Please review.

Project coverage is 97.63%. Comparing base (dd07e0f) to head (dd2da61).
Report is 5 commits behind head on master.

Files with missing lines Patch % Lines
sf-metrics/src/inmemory/implementation.rs 70.00% 3 Missing ⚠️
sf-peer-id/src/peer_id.rs 96.42% 0 Missing and 2 partials ⚠️
sf-peer-id/src/serde.rs 96.55% 0 Missing and 2 partials ⚠️
sf-peer-id/src/error.rs 97.14% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #10      +/-   ##
==========================================
- Coverage   97.64%   97.63%   -0.02%     
==========================================
  Files          19       20       +1     
  Lines        1572     1688     +116     
  Branches       28       32       +4     
==========================================
+ Hits         1535     1648     +113     
  Misses         20       20              
- Partials       17       20       +3     
Components Coverage Δ
metrics 96.64% <72.72%> (+0.13%) ⬆️
server 98.49% <100.00%> (-0.01%) ⬇️
protocol 98.91% <ø> (ø)
logging ∅ <ø> (∅)
peer-id 97.12% <96.68%> (-0.52%) ⬇️
Files with missing lines Coverage Δ
sf-metrics/src/inmemory/common.rs 98.92% <100.00%> (ø)
sf-peer-id/src/hex.rs 100.00% <100.00%> (ø)
sf-server/src/main.rs 93.33% <ø> (ø)
sf-server/src/peer_handler.rs 100.00% <ø> (ø)
sf-server/src/socket_metadata.rs 100.00% <ø> (ø)
sf-server/src/state.rs 100.00% <100.00%> (ø)
sf-server/src/ws.rs 93.82% <ø> (ø)
sf-peer-id/src/error.rs 97.36% <97.14%> (-2.64%) ⬇️
sf-peer-id/src/peer_id.rs 97.16% <96.42%> (-0.02%) ⬇️
sf-peer-id/src/serde.rs 96.55% <96.55%> (ø)
... and 1 more

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dd07e0f...dd2da61. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@sfroment sfroment force-pushed the dev/ice branch 3 times, most recently from 1e08f72 to 6927790 Compare April 24, 2025 14:15
Signed-off-by: Sacha Froment <[email protected]>
Signed-off-by: Sacha Froment <[email protected]>
@sfroment sfroment requested a review from Copilot April 24, 2025 14:27
Copy link

@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

This PR adds serde support for peer identifiers by introducing serialization and deserialization for FixedSizePeerID and refactoring the associated error handling and naming. Key changes include:

  • Adding a new serde module with implementations for serialization, deserialization, and a custom Visitor.
  • Renaming the primary identifier type from PeerID to FixedSizePeerID with updated methods and error types.
  • Updating tests, Cargo.toml dependencies, and related modules to support the new naming and error handling.

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
sf-peer-id/src/serde.rs Adds serde serialization/deserialization support using a dedicated buffer.
sf-peer-id/src/peer_id.rs Renames PeerID to FixedSizePeerID and updates methods (from_bytes, write, etc.) with new error handling.
sf-peer-id/src/lib.rs Updates re-exports to include the new Error type and FixedSizePeerID.
sf-peer-id/src/hex.rs Switches from ParsePeerIDError to the new Error type for hex conversion.
sf-peer-id/src/error.rs Refactors error handling with an expanded Error enum and related conversions.
sf-peer-id/Cargo.toml Adds serde and unsigned-varint dependencies and updates feature flags.
sf-metrics/src/inmemory/implementation.rs Updates format strings to use new interpolation syntax for metrics output.
sf-metrics/src/inmemory/common.rs Adjusts debug output formatting using new syntax.
Comments suppressed due to low confidence (1)

sf-peer-id/src/peer_id.rs:17

  • [nitpick] The struct is now named 'FixedSizePeerID' but some documentation comments and debug messages still refer to 'PeerID'. Consider updating these comments and outputs for consistency.
pub struct FixedSizePeerID<const S: usize> {

@sfroment sfroment force-pushed the dev/ice branch 2 times, most recently from eb3448e to a4dd563 Compare April 24, 2025 14:53
Signed-off-by: Sacha Froment <[email protected]>
Copy link

@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

This PR introduces serde support for peer IDs and updates various code paths to use the new formatting capture syntax. Key changes include:

  • Updating panic, log, and debug messages to use the new "{variable}" interpolation.
  • Adding serde implementations and corresponding tests for FixedSizePeerID.
  • Refactoring error handling to use a unified Error enum.

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
sf-server/src/ws.rs Updated panic messages to use new formatting capture syntax.
sf-server/src/state.rs Updated logging messages for readability and consistency.
sf-peer-id/src/serde.rs Added serde (de)serialization support for FixedSizePeerID.
sf-peer-id/src/peer_id.rs Renamed PeerID to FixedSizePeerID and updated tests accordingly.
sf-peer-id/src/hex.rs Updated error returns to use the unified Error enum.
sf-peer-id/src/error.rs Refactored error handling with expanded Error variants.
sf-peer-id/Cargo.toml Enabled serde and updated dependency versions.
sf-metrics/* Updated string interpolation in metric formatting and tests.
Comments suppressed due to low confidence (2)

sf-peer-id/src/peer_id.rs:347

  • [nitpick] The test named 'test_peer_id_from_bytes_invalid_length' now uses from_str instead of from_bytes; consider renaming it to better reflect that it validates hex string input.
let result = FixedSizePeerID::<3>::from_str("deadbeef");

sf-server/src/ws.rs:171

  • Ensure that the project's Rust compiler version supports the new formatting capture syntax (e.g., using {e} instead of the traditional "{}" pattern) to avoid potential runtime issues.
panic!("Failed to connect to WS after multiple attempts: {e}");

@sfroment sfroment merged commit f92f978 into master Apr 24, 2025
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant