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

swarm/: Enable advanced dialing requests #2317

Merged
merged 31 commits into from
Nov 15, 2021

Conversation

mxinden
Copy link
Member

@mxinden mxinden commented Oct 29, 2021

Enable advanced dialing requests both on Swarm and via
NetworkBehaviourAction. Users can now trigger a dial with a specific
set of addresses, optionally extended via
NetworkBehaviour::addresses_of_peer. In addition the whole process is
now modelled in a type safe way via the builder pattern.

Example of a NetworkBehaviour requesting a dial to a specific peer
with a set of addresses additionally extended through
NetworkBehaviour::addresses_of_peer:

NetworkBehaviourAction::Dial {
    opts: DialOpts::peer_id(peer_id)
              .condition(PeerCondition::Always)
              .addresses(addresses)
              .extend_addresses_through_behaviour()
              .build(),
    handler,
}

Example of a user requesting a dial to an unknown peer with a single
address via Swarm:

swarm1.dial(
    DialOpts::unknown_peer_id()
        .address(addr2.clone())
        .build()
)

Creating this as a draft for now. I have only updated libp2p-swarm for now.

What do folks think of the interface? (//CC @thomaseizinger as this follows your second suggestion provided in #2249 (comment).)

Enable advanced dialing requests both on `Swarm` and via
`NetworkBehaviourAction`. Users can now trigger a dial with a specific
set of addresses, optionally extended via
`NetworkBehaviour::addresses_of_peer`. In addition the whole process is
now modelled in a type safe way via the builder pattern.

Example of a `NetworkBehaviour` requesting a dial to a specific peer
with a set of addresses additionally extended through
`NetworkBehaviour::addresses_of_peer`:

```rust
NetworkBehaviourAction::Dial {
    opts: DialOpts::peer_id(peer_id)
              .condition(PeerCondition::Always)
              .addresses(addresses)
              .extend_addresses_through_behaviour()
              .build(),
    handler,
}
```

Example of a user requesting a dial to an unknown peer with a single
address via `Swarm`:

```rust
swarm1.dial(
    DialOpts::unknown_peer_id()
        .address(addr2.clone())
        .build()
)
```
Copy link
Contributor

@elenaf9 elenaf9 left a comment

Choose a reason for hiding this comment

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

I really like the new interface, would be very useful to have this feature!

swarm/src/lib.rs Outdated
Comment on lines 1785 to 1792
pub enum DialOpts {
// TODO: How about folding these structs into the enum variants?
WithPeerId(WithPeerId),
WithPeerIdWithAddresses(WithPeerIdWithAddresses),
WithoutPeerIdWithAddress(WithoutPeerIdWithAddress),
}

impl DialOpts {
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
pub enum DialOpts {
// TODO: How about folding these structs into the enum variants?
WithPeerId(WithPeerId),
WithPeerIdWithAddresses(WithPeerIdWithAddresses),
WithoutPeerIdWithAddress(WithoutPeerIdWithAddress),
}
impl DialOpts {
enum Opts {
// TODO: How about folding these structs into the enum variants?
WithPeerId(WithPeerId),
WithPeerIdWithAddresses(WithPeerIdWithAddresses),
WithoutPeerIdWithAddress(WithoutPeerIdWithAddress),
}
pub struct DialOpts {
opts: Opts
}
impl DialOpts {

Consider wrapping the enum in a struct to emphasize that the user should not try to build the variants directly, but instead use the methods.

Copy link
Member Author

Choose a reason for hiding this comment

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

Given the structs WithPeerId, WithPeerIdWithAddresses and WithoutPeerIdWithAddress can not be constructed outside of dial_opts, the user can not build DialOpts themselves. Still I understand that this is not intuitive and that it would be nice to make it clear that the user should go through the methods on DialOpts.

Is wrapping an enum into a struct an idiomatic way in Rust to signal that something should not be constructed directly? I don't recall seeing this in the past. Any prominent examples? I would have just relied on a doc comment on DialOpts.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is wrapping an enum into a struct an idiomatic way in Rust to signal that something should not be constructed directly? I don't recall seeing this in the past.

I would argue it the other way around: I feel like it is very rare and not rust-idiomatic that an enum is not constructed directly. Of course a user would notice that it is not possible once they try to build the inner structs, but it still may cause some confusion. With a struct, the API would make it clearer that it should be constructed via methods, especially when looking at it on docs.rs.

I don't recall seeing this in the past. Any prominent examples?

I feel like this is just following the general api design pattern of wrapping internal/ inner types in a new struct to hide implementation details. Imo this always makes sense when the inner type is not relevant for the user and should never be constructed directly by them, which is the case here.


Alternatively, if you decide to fold the structs into the enum variants as you wrote in your TODO in l.1786:

// TODO: How about folding these structs into the enum variants

the user would be able to build the enum types directly and the builder pattern would not be needed at all.

Copy link
Member Author

Choose a reason for hiding this comment

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

Addressed in ad083e9.

swarm/src/lib.rs Outdated
pub(crate) extend_addresses_through_behaviour: bool,
}

impl WithPeerIdWithAddresses {
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
impl WithPeerIdWithAddresses {
impl WithPeerIdWithAddresses {
pub fn condition(mut self, condition: PeerCondition) -> Self {
self.condition = condition;
self
}

Maybe add this method here as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am a bit reluctant to offer two ways to achieve the same thing. In what use-case would one want to call condition on WithPeerIdWithAddresses and couldn't do so on WithPeerId?

Copy link
Contributor

Choose a reason for hiding this comment

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

I am also a bit undecided in this aspect.
But the DialOpts are trying to follow the syntax of a builder pattern, and with a builder it normally doesn't matter in which order you specify the paramters, so you would expect that DialOpts::peer_id(...).condition(..).addresses(..) and DialOpts::peer_id(...).addresses(..).condition(..) both work.
That being said, the current implementation doesn't strictly follow the builder pattern anyway (at least not the way it is described in the Rust API Guidelines), therefore I also see the point of avoiding duplicated code.

Copy link
Member Author

Choose a reason for hiding this comment

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

Addressed in 26dedbe.

swarm/src/lib.rs Outdated
Comment on lines 1800 to 1802
pub fn unknown_peer_id() -> WithoutPeerId {
WithoutPeerId {}
}
Copy link
Contributor

@elenaf9 elenaf9 Oct 30, 2021

Choose a reason for hiding this comment

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

Suggested change
pub fn unknown_peer_id() -> WithoutPeerId {
WithoutPeerId {}
}
pub fn address(address: Multiaddr) -> WithoutPeerIdWithAddress {
WithoutPeerIdWithAddress { address }
}

I do like the consistent syntax of DialOpts::peer_id(..).addresses() and DialOpts::unknown_peer_id(..).address(..).
But this similar syntax suggests that WithPeerId and WithoutPeerId are alike, which is not the case because WithPeerId can be a "final" state from which you can call build(), whereas WithoutPeerId is only intermediate. Not sure about this, but I feel like it could end up confusing the user, since even though it follows the same naming convention, WithoutPeerId is not a valid DialOpt.

Optionally if we follow the above suggestion, we could keep it consist by also adding

impl WithoutPeerIdWithAddress {
        pub fn peer_id(self, peer_id: PeerId) -> WithPeerIdWithAddresses {
            WithPeerIdWithAddresses {
                peer_id,
                condition: Default::default(),,
                addresses: vec![self.address],
                extend_addresses_through_behaviour: false,
            }
        }
    }

The only problem with this is that without a peer id you only have one address, whereas with an peer id there can be multiple.

Copy link
Member Author

Choose a reason for hiding this comment

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

Say we allowed DialOpts::address(..) instead of DialOpts::unknown_peer_id(..).address(..), would that not confuse a user into thinking that they can then later on add a peer id, i.e. that they can do DialOpts::address(..).peer_id(..)?

The only problem with this is that without a peer id you only have one address, whereas with an peer id there can be multiple.

Yes, this is something that I would like to encode at the type level, thus the differentiation between the two. I am not sure how to model your suggestion without loosing the type safety.

Copy link
Contributor

Choose a reason for hiding this comment

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

Say we allowed DialOpts::address(..) instead of DialOpts::unknown_peer_id(..).address(..), would that not confuse a user into thinking that they can then later on add a peer id, i.e. that they can do DialOpts::address(..).peer_id(..)?

I agree. But with reference to the above discussion: the pattern currently already expects a fixed order in which the parameters are specified, e.g. as written above: in case of a known peer id you have to specify the condition before specifying the addresses. Isn't that kind of the same issue?

Yes, this is something that I would like to encode at the type level, thus the differentiation between the two. I am not sure how to model your suggestion without loosing the type safety.

You are right, the type safety should be kept; DialOpts::address(..).peer_id(..) should not be possible.

@mxinden
Copy link
Member Author

mxinden commented Nov 3, 2021

Thanks @elenaf9 for giving this a review and helping to design this interface. I replied to all of your comments. I am curious what you think.

@mxinden mxinden marked this pull request as ready for review November 9, 2021 13:54
@mxinden
Copy link
Member Author

mxinden commented Nov 9, 2021

@elenaf9 do you want to take another look?

@thomaseizinger would very much appreciate your input here.

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.

Nice work!

Two suggestions but those could also be shipped later!

swarm/src/lib.rs Outdated
/// .build()
/// );
/// ```
pub fn dial(&mut self, opts: DialOpts) -> Result<(), DialError> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Given how common the following is:

DialOpts::unknown_peer_id().address(listen_addr).build()

We could consider accepting impl Into<DialOpts>:

Suggested change
pub fn dial(&mut self, opts: DialOpts) -> Result<(), DialError> {
pub fn dial(&mut self, opts: impl Into<DialOpts>) -> Result<(), DialError> {

and implement:

impl From<MultiAddress> for DialOpts { }

where we do exactly the above.

Copy link
Contributor

Choose a reason for hiding this comment

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

Totally agree we can get rid of a boilerplate code like this. Another option is to accept an impl DialOptsBuilder and call: .dial(DialOpts::.address(listen_addr)).

Copy link
Member Author

Choose a reason for hiding this comment

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

Given how common the following is:

DialOpts::unknown_peer_id().address(listen_addr).build()

We could consider accepting impl Into<DialOpts>:

and implement:

impl From<MultiAddress> for DialOpts { }

where we do exactly the above.

Neat! Thanks for bringing it up @thomaseizinger. I will give this a shot.

Copy link
Member Author

Choose a reason for hiding this comment

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

5e3edb3 implements From<Multiaddr> and changes calls to unknown_peer_id().address(xxx).build() to the address only.

Totally agree we can get rid of a boilerplate code like this. Another option is to accept an impl DialOptsBuilder and call: .dial(DialOpts::.address(listen_addr)).

I think From<Multiaddr> is the more elegant way. Given that the two solve the same issue, I would prefer the earlier over the later suggestion.


impl WithPeerIdWithAddresses {
/// Specify a [`PeerCondition`] for the dial.
pub fn condition(mut self, condition: PeerCondition) -> Self {
Copy link
Contributor

Choose a reason for hiding this comment

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

This API requires users to import one more type: PeerCondition. Might be worth providing 3 functions instead that configure the corresponding condition.

Please excuse if that has been discussed and dismissed already!

Copy link
Member Author

Choose a reason for hiding this comment

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

Modelling 3 exclusive states with 3 methods instead of an enum seems not intuitive to me. Is the additional import of PeerCondition worth the added complexity?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think there is a misunderstanding. I would still model the states internally with an enum. However, nothing stops anyone from doing this:

DialOpts::address(addr).condition(...).condition(...)

It might give people a hint that the condition is overridden but in theory, it could also be implemented in an additive way.
I don't feel strongly about it so happy to keep the enum :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I think there is a misunderstanding. I would still model the states internally with an enum.

No misunderstanding :)

I don't feel strongly about it so happy to keep the enum :)

Will keep the enum for now. Not 100% happy but can't think of a better alternative. We can still further iterate on it. Merging for now.

Copy link
Contributor

@elenaf9 elenaf9 left a comment

Choose a reason for hiding this comment

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

Looks good to me!


// We should still be able to dial now!
swarm2
.dial(DialOpts::peer_id(swarm1_peer_id).build())
Copy link
Contributor

Choose a reason for hiding this comment

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

impl From<PeerId> for DialOpts too?

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 good idea. Done via 09293c4.

@mxinden mxinden merged commit 220f84a into libp2p:master Nov 15, 2021
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.

swarm/: Enable dialing a specific fixed set of addresses for a single peer
4 participants