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

Remove data request-response code. #3747

Merged
merged 3 commits into from
Oct 11, 2024
Merged

Remove data request-response code. #3747

merged 3 commits into from
Oct 11, 2024

Conversation

twittner
Copy link
Contributor

Follow-up to #3740 which removes the remaining bits of data request-response code.

Follow-up to #3740 which removes the remaining bits of data
request-response code.
@twittner twittner marked this pull request as ready for review October 10, 2024 15:07
@twittner twittner requested a review from bfish713 as a code owner October 10, 2024 15:07
@@ -160,8 +139,6 @@ pub enum NetworkEventInternal {
GossipEvent(Box<GossipEvent>),
/// a direct message event
DMEvent(libp2p::request_response::Event<Vec<u8>, Vec<u8>>),
/// a request response event
RequestResponseEvent(libp2p::request_response::Event<Request, Response>),
Copy link
Contributor

Choose a reason for hiding this comment

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

I know the enum doesn't derive Serialize, but I'm a bit worried that removing variants might be a breaking libp2p change somehow. Does this definitely not get serialized somehow at any point?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good question, but no it should not be. These events should be events that are created from messages we receive. Also libp2p should be good at handling nodes running different protocols sets of protocols (as long as each protocol they are both running match)

@twittner twittner merged commit df63ec3 into main Oct 11, 2024
35 of 36 checks passed
@twittner twittner deleted the tw/more-rm-req-res branch October 11, 2024 19:21
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.

4 participants