Skip to content
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

[2/n] Introduce node-protocol #1242

Merged
merged 1 commit into from
Mar 4, 2024
Merged

[2/n] Introduce node-protocol #1242

merged 1 commit into from
Mar 4, 2024

Conversation

AhmedSoliman
Copy link
Contributor

@AhmedSoliman AhmedSoliman commented Feb 28, 2024

Copy link

github-actions bot commented Feb 28, 2024

Test Results

117 files  ±0  117 suites  ±0   10m 24s ⏱️ +7s
102 tests ±0  102 ✅ ±0  0 💤 ±0  0 ❌ ±0 
261 runs  ±0  261 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 22f97d2. ± Comparison against base commit c3f26cd.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@tillrohrmann tillrohrmann left a comment

Choose a reason for hiding this comment

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

Nice work @AhmedSoliman. I do understand now what you mean with directly reusing simple protobuf types. I think it makes sense how you used them to avoid adding more boilerplate. I also learned about prost_build::Config::enum_attribute which is really cool :-)

+1 for merging this PR.

crates/core/src/network_sender.rs Outdated Show resolved Hide resolved
crates/node-protocol/Cargo.toml Outdated Show resolved Hide resolved
crates/node-protocol/Cargo.toml Outdated Show resolved Hide resolved
Copy link
Contributor

@tillrohrmann tillrohrmann left a comment

Choose a reason for hiding this comment

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

Great work @AhmedSoliman. +1 for merging.


anyhow = { workspace = true }
arc-swap = { workspace = true }
async-trait = { workspace = true }
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need this dependency in this version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's used in a MessageRouter up in this stack of PRs.

/// If `to` is a NodeID with generation, then it's guaranteed that messages will be sent to
/// this particular generation, otherwise, it'll be routed to the latest generation available
/// in nodes configuration at the time of the call. This might return
/// [[`NetworkSendError::OldPeerGeneration`]] if the node is is not the latest generation.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// [[`NetworkSendError::OldPeerGeneration`]] if the node is is not the latest generation.
/// [[`NetworkSendError::OldPeerGeneration`]] if the node is not the latest generation.


impl WireSerde for MetadataMessage {
fn encode(&self, _protocol_version: ProtocolVersion) -> Result<Bytes, CodecError> {
// serialize message to bytes
Copy link
Contributor

Choose a reason for hiding this comment

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

should we add an assert stating that _protocol_version is actually BINCODE?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure this'd be particularly useful, but we can check if the version is "at least" BINCODE. I can add such check(s) in a future follow up as needed.

@AhmedSoliman AhmedSoliman merged commit 408487a into main Mar 4, 2024
4 checks passed
@AhmedSoliman AhmedSoliman deleted the pr1242 branch March 4, 2024 16:07
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.

2 participants