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

refactor(relay): revise public API to follow naming convention #3238

Merged
merged 34 commits into from
Jan 2, 2023

Conversation

jxs
Copy link
Member

@jxs jxs commented Dec 13, 2022

Description

Continues addressing #2217.

Notes

Per commit review is suggested :)

Links to any relevant issues

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.

Thanks, just one question :)

protocols/relay/CHANGELOG.md Outdated Show resolved Hide resolved
protocols/relay/CHANGELOG.md Show resolved Hide resolved
@mergify
Copy link
Contributor

mergify bot commented Dec 14, 2022

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

jxs and others added 8 commits December 14, 2022 14:38
and use them across the crate. Deprecate the old ones.
and use them across the crate. Deprecate the v2 ones.
Currently, we create a new cache for each workflow run for each crate. That ends up blowing the maximum allowed cache size of 10GB and GitHub deletes the least-recently used cache again. Effectively, this means we don't have any caching.

This patch introduces a cache factory workflow that only runs on master and always _saves_ a new cache. The CI workflow run for pull-requests on the other hand only restore these caches but don't save them.
)

This patch deprecates 3 out of 4 functions on `PollParameters`:

- `local_peer_id`
- `listened_addresses`
- `external_addresses`

The addresses can be obtained by inspecting the `FromSwarm` event. To make this easier, we introduce two utility structs in `libp2p-swarm`:

- `ExternalAddresses`
- `ListenAddresses`

A node's `PeerId` is always known to the caller, thus we can require them to pass it in.

Related: libp2p#3124.
@mergify
Copy link
Contributor

mergify bot commented Dec 14, 2022

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

@mergify
Copy link
Contributor

mergify bot commented Dec 14, 2022

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

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.

Very nice! Thank you!

protocols/relay/src/client.rs Outdated Show resolved Hide resolved
protocols/relay/src/lib.rs Outdated Show resolved Hide resolved
protocols/relay/src/v2.rs Show resolved Hide resolved
protocols/relay/src/lib.rs Outdated Show resolved Hide resolved
- rename RelayTransport in client doc to transport
- make behaviour mod private re-exporting CircuitId
- make client mod private re-exporting pub types.
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 for pushing this forward, some more thoughts!

protocols/relay/src/lib.rs Outdated Show resolved Hide resolved
protocols/relay/src/lib.rs Outdated Show resolved Hide resolved
protocols/relay/src/lib.rs Outdated Show resolved Hide resolved
protocols/relay/src/priv_client.rs Show resolved Hide resolved
- dcutr tests use relay::client instead of just client::
- rename RelayedConnection to RelayedConnection
- improve deprcated wording for types, remove the renamed references.
rename RelayedDial to Dial.
@mergify
Copy link
Contributor

mergify bot commented Dec 20, 2022

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

1 similar comment
@mergify
Copy link
Contributor

mergify bot commented Dec 20, 2022

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

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.

Sorry to add some more comments, I've had another thorough look and I think we can polish this a bit more :)

protocols/relay/CHANGELOG.md Outdated Show resolved Hide resolved
protocols/dcutr/examples/dcutr.rs Outdated Show resolved Hide resolved
protocols/relay/src/priv_client.rs Outdated Show resolved Hide resolved
protocols/relay/src/priv_client.rs Outdated Show resolved Hide resolved
protocols/relay/src/priv_client/transport.rs Outdated Show resolved Hide resolved
protocols/relay/src/priv_client/transport.rs Outdated Show resolved Hide resolved
protocols/relay/src/priv_client/transport.rs Outdated Show resolved Hide resolved
protocols/relay/src/priv_client/transport.rs Outdated Show resolved Hide resolved
protocols/relay/tests/lib.rs Outdated Show resolved Hide resolved
@jxs
Copy link
Member Author

jxs commented Dec 21, 2022

Sorry to add some more comments, I've had another thorough look and I think we can polish this a bit more :)

no, thank you for doing it so we caught things like the wrong PR link in the changelog!

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 more things! :)

protocols/relay/src/lib.rs Outdated Show resolved Hide resolved
protocols/relay/src/priv_client/transport.rs Outdated Show resolved Hide resolved
Comment on lines 384 to 394
#[derive(Debug)]
enum ClientEvent {
Relay(client::Event),
Relay(relay::client::Event),
Ping(ping::Event),
}

impl From<client::Event> for ClientEvent {
fn from(event: client::Event) -> Self {
impl From<relay::client::Event> for ClientEvent {
fn from(event: relay::client::Event) -> Self {
ClientEvent::Relay(event)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

If you are feeling like deleting a bit more code, we could turn on event_process = true (which is the default) and remove this. Can easily be done in a follow-up though.

Copy link
Member Author

Choose a reason for hiding this comment

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

ehe yeah done. Also deleted the ClientEvent and RelayEvent that's what you were meaning right? And also removed from the examples.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes!

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.

Great improvement :)

Can you open an issue to investigate how we can have the CI pipeline complain about deprecated APIs in docs?

@jxs
Copy link
Member Author

jxs commented Dec 24, 2022

Great improvement :)

Can you open an issue to investigate how we can have the CI pipeline complain about deprecated APIs in docs?

opened #3282

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.

Appreciate the work here!

@mxinden mxinden added the send-it label Jan 2, 2023
@mergify mergify bot merged commit 9c96bbb into libp2p:master Jan 2, 2023
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