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

feat(swarm): remove deprecated items #3956

Merged
merged 16 commits into from
May 24, 2023
Merged

Conversation

tcoratger
Copy link
Contributor

@tcoratger tcoratger commented May 16, 2023

Description

Related: #3647.

Notes & open questions

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

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.

Some of these are unreleased, please cross check them with docs.rs! :)

swarm/src/behaviour.rs Show resolved Hide resolved
swarm/src/handler.rs Outdated Show resolved Hide resolved
swarm/src/lib.rs Outdated Show resolved Hide resolved
@tcoratger
Copy link
Contributor Author

Some of these are unreleased, please cross check them with docs.rs! :)

@thomaseizinger Indeed, I had missed some on doc.rs, I've added back in the codebase the ones that were not yet available on doc.rs so that users have time to see the deprecation warning before we delete .

On the other hand, external_addresses function from the PollParameters trait should be removed in favor of using libp2p_swarm::ExternalAddresses directly. This induces:

pub struct SwarmPollParameters<'a> {
    local_peer_id: &'a PeerId,
    supported_protocols: &'a [Vec<u8>],
    listened_addrs: Vec<&'a Multiaddr>,
    external_addrs: &'a Addresses,
}

to become (if I'm correct)

pub struct SwarmPollParameters<'a> {
    local_peer_id: &'a PeerId,
    supported_protocols: &'a [Vec<u8>],
    listened_addrs: Vec<&'a Multiaddr>,
    external_addrs: &'a ExternalAddresses,
}

And in my opinion the same thing applies for external_addrs in the Swarm<TBehaviour> structure, which induces a lot of changes on all the methods used (add, remove...) on the addresses. Should these changes be made here? Or is it too much for this deprecation PR? Or am I going in the wrong direction?

@thomaseizinger
Copy link
Contributor

Or am I going in the wrong direction?

Slightly yes. The ExternalAddresses struct should be used instead of PollParameters::external_addresses.

Check how ListenAddresses is used for some inspiration :)

@tcoratger
Copy link
Contributor Author

Or am I going in the wrong direction?

Slightly yes. The ExternalAddresses struct should be used instead of PollParameters::external_addresses.

Check how ListenAddresses is used for some inspiration :)

@thomaseizinger Sure, then my concern was about this part:

let score = params
  .external_addresses()
  .find_map(|r| (r.addr == address).then_some(r.score))
  .unwrap_or(AddressScore::Finite(0));

that has to be modified to something like:

let score = self
  .external_addresses
  .iter()
  .find_map(|r| (r == &address).then_some(r.score));

But this is not possible I think because there is no score element inside the type returned by the iter function of impl ExternalAddresses. I saw the small comment // TODO: Fix once we report AddressScore through FromSwarm event. so that I imagine this score line was supposed to be deleted in favor of catching the address score from the FromSwarm event but I wasn't able to find the corresponding implementation, did I miss something here?

@mxinden
Copy link
Member

mxinden commented May 18, 2023

@tcoratger AddressScore is likely going away with #3954. That would solve the issue above, right?

@tcoratger
Copy link
Contributor Author

@mxinden Yes right, so maybe we are waiting for this PR to merge, it will make things easier here because there will be no more to deal with this problem and there is no point in finding a solution here if AddressScore is deleted by the PR that you quote :)

@thomaseizinger thomaseizinger added this to the v0.52.0 milestone May 23, 2023
@mergify
Copy link
Contributor

mergify bot commented May 24, 2023

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

@tcoratger tcoratger marked this pull request as ready for review May 24, 2023 09:06
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.

Awesome, thanks for updating the PR!

One question.

swarm/src/lib.rs Outdated Show resolved Hide resolved
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.

Two additional questions (apply to all doctest changes)

libp2p/src/tutorials/ping.rs Outdated Show resolved Hide resolved
libp2p/src/tutorials/ping.rs Outdated Show resolved Hide resolved
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.

Awesome, thank you!

@mergify mergify bot merged commit 6329d74 into libp2p:master May 24, 2023
@tcoratger tcoratger deleted the swarm-deprecated branch May 25, 2023 07:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants