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

RoutedHost: Embed the underlying host instead of hiding it #2969

Closed
wants to merge 1 commit into from

Conversation

burdiyan
Copy link
Contributor

When using a libp2p host with routing, the underlying concrete type is RoutedHost. This type wraps the original Host type and hides it into a private field, which makes it hard to make interface type assertions on the host.

E.g. sometimes it's useful to have access to the underlying instance of the IDService, which is exposed by BasicHost as a method IDService() identityIDService, but is inaccessible for type assertions because RoutedHost hides the BasicHost in its private field.

This commit uses type embedding, instead of a private field, to fix it.

Comment on lines 35 to 38
type Routing interface {
FindPeer(context.Context, peer.ID) (peer.AddrInfo, error)
}

Copy link
Member

Choose a reason for hiding this comment

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

Can you deprecate this with a comment saying use routing.PeerRouting

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@MarcoPolo
Copy link
Collaborator

This change seems fine, but I think a better solution to the problem of trying to get the ID service is with Fx options. You could provide them in the constructor with Fx's Populate (#2956) or when you build libp2p with Fx directly (#2962).

@burdiyan
Copy link
Contributor Author

burdiyan commented Oct 9, 2024

This change seems fine, but I think a better solution to the problem of trying to get the ID service is with Fx options. You could provide them in the constructor with Fx's Populate (#2956) or when you build libp2p with Fx directly (#2962).

@MarcoPolo Not sure if I agree it would be better. Requiring people to learn and use Fx seems like an overkill for something that can be done with existing language features. I personally stopped using Fx and now prefer doing things explicitly. But of course exposing more Fx-related flexibilities in Libp2p could be helpful too.

When using a libp2p host with routing, the underlying concrete type is RoutedHost.
This type wraps the original Host type and hides it into a private field,
which makes it hard to make interface type assertions on the host.

E.g. sometimes it's useful to have access to the underlying instance of the IDService,
which is exposed by BasicHost as a method `IDService() identityIDService`, but is inaccessible
for type assertions because RoutedHost hides the BasicHost in its private field.

This commit uses type embedding, instead of a private field to fix it.
@MarcoPolo
Copy link
Collaborator

The problem is that we don't want the Host interfaced type to have to expose all the sub-services a user might want access to (e.g. the identify service). We also don't really want users to assume that the result of creating a libp2p Host will be a BasicHost or any other concrete type. If we did, we would return that type directly. Therefore type assertions here are brittle.

Fx solves this problem in a fairly way. It lets you get a reference to a service without having to expose that service in the Host. It also won't create the service if nothing needs it.

@burdiyan
Copy link
Contributor Author

Are the Fx affordances for doing that in libp2p exposed to users? I haven't seen how could I do that. Anyway, I'm not feeling strongly about this change, so feel free to close it :)

@sukunrt
Copy link
Member

sukunrt commented Oct 10, 2024

I agree with @MarcoPolo about not wanting users to rely on type assertions for access to services like Identify. I still think the routed host should embed and wrap the provided host. Why shouldn't it?

@MarcoPolo
Copy link
Collaborator

You'll be able to get the ID service in the next release. Refer to this test for an example:

func TestGetIDService(t *testing.T) {
var id identify.IDService
host, err := New(
NoTransports,
WithFxOption(fx.Populate(&id)),
)
require.NoError(t, err)
defer host.Close()
require.NotNil(t, id)
}

@MarcoPolo MarcoPolo closed this Oct 22, 2024
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.

3 participants