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.

Sign up for GitHub

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

Jump to bottom

feat(dcutr): add ConnectionId to upgrade success/failed events #4558

Merged
merged 15 commits into from
Oct 20, 2023
Merged

feat(dcutr): add ConnectionId to upgrade success/failed events #4558

merged 15 commits into from
Oct 20, 2023

Conversation

dariusc93
Copy link
Member

Description

Resolves #4519.

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

@dariusc93 dariusc93 changed the title refactor(dcutr): Add ConnectionId to Event refactor(dcutr): Add ConnectionId to Event::DirectConnectionUpgrade{Succeeded,Failed} Sep 26, 2023
@thomaseizinger thomaseizinger changed the title refactor(dcutr): Add ConnectionId to Event::DirectConnectionUpgrade{Succeeded,Failed} feat(dcutr): add ConnectionId to Event::DirectConnectionUpgrade{Succeeded,Failed} Sep 26, 2023
thomaseizinger
thomaseizinger reviewed Sep 26, 2023
View reviewed changes
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Thanks!

Can we expend the test to assert that this is definitely the direct connection? We should be able to do that by recording the address from SwarmEvent::ConnetionEstablished and checking that it is not a relayed address.

protocols/dcutr/CHANGELOG.md Outdated Show resolved Hide resolved
protocols/dcutr/src/behaviour_impl.rs Outdated Show resolved Hide resolved
dariusc93 and others added 3 commits September 26, 2023 00:05
@dariusc93 @thomaseizinger
Update protocols/dcutr/CHANGELOG.md
624983a 
Co-authored-by: Thomas Eizinger <thomas@eizinger.io>
@dariusc93
fix: Use correct connection_id
87c30cc
@dariusc93
Merge branch 'refactor/dcutr-connection-id' of github.com:dariusc93/r…
1600e11 
…ust-libp2p into refactor/dcutr-connection-id
@thomaseizinger thomaseizinger changed the title feat(dcutr): add ConnectionId to Event::DirectConnectionUpgrade{Succeeded,Failed} feat(dcutr): add ConnectionId to upgrade success/failed events Sep 26, 2023
mxinden
mxinden reviewed Sep 27, 2023
View reviewed changes
Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Missing test. Otherwise looks good to me. Thanks.

@thomaseizinger thomaseizinger added this to the v0.53.0 milestone Sep 28, 2023
@mergify
Copy link
Contributor

mergify bot commented Sep 28, 2023

This pull request has merge conflicts. Could you please resolve them @dariusc93? 🙏

@mergify
Copy link
Contributor

mergify bot commented Oct 20, 2023

This pull request has merge conflicts. Could you please resolve them @dariusc93? 🙏

@thomaseizinger thomaseizinger marked this pull request as ready for review October 20, 2023 00:10
@mergify
Copy link
Contributor

mergify bot commented Oct 20, 2023

This pull request has merge conflicts. Could you please resolve them @dariusc93? 🙏

@thomaseizinger thomaseizinger added internal-change Pull requests that make internal changes to crates and thus don't need to include a changelog entry. and removed send-it labels Oct 20, 2023
@mergify mergify bot merged commit 3c00ae8 into libp2p:master Oct 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mxinden mxinden mxinden left review comments

@thomaseizinger thomaseizinger thomaseizinger approved these changes

Assignees
No one assigned
Labels
internal-change Pull requests that make internal changes to crates and thus don't need to include a changelog entry. send-it
Projects
None yet
Milestone
v0.53.0
Development

Successfully merging this pull request may close these issues.

dcutr::Event::DirectConnectionUpgradeSucceeded should include the ConnectionId
3 participants
@dariusc93 @thomaseizinger @mxinden