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

autorelay: refactor libp2p.EnableAutoRelay option #1866

Closed
Tracked by #1916
MarcoPolo opened this issue Nov 9, 2022 · 7 comments · Fixed by #2022
Closed
Tracked by #1916

autorelay: refactor libp2p.EnableAutoRelay option #1866

MarcoPolo opened this issue Nov 9, 2022 · 7 comments · Fixed by #2022
Assignees
Labels
good first issue Good issue for new contributors

Comments

@MarcoPolo
Copy link
Collaborator

We should split the function into two options, one that requires a list of static relays and one that requires a peer source. Having options, some of which are required, is not a nice API pattern.

Originally posted by @marten-seemann in #1852 (comment)

@MarcoPolo MarcoPolo added the good first issue Good issue for new contributors label Nov 9, 2022
@marten-seemann marten-seemann changed the title Refactor libp2p.EnableAutoRelay option autorelay: refactor libp2p.EnableAutoRelay option Nov 9, 2022
@marten-seemann
Copy link
Contributor

Suggestion: libp2p.EnableAutoRelayWithStaticRelays([]peer.AddrInfo) and libp2p.EnableAutoRelayWithAutoDiscovery(func(ctx context.Context, num int) <-chan peer.AddrInfo)?

I'd be grateful for better naming suggestions :)

@marten-seemann marten-seemann mentioned this issue Nov 22, 2022
35 tasks
@MarcoPolo
Copy link
Collaborator Author

Preference for libp2p.EnableAutoRelayWithPeerSource.

@MarcoPolo MarcoPolo self-assigned this Jan 9, 2023
@p-shahi
Copy link
Member

p-shahi commented Jan 25, 2023

@sukunrt Flagging this for you in case you are interested in taking on more issues. Thanks for all the great contributions up till now, no pressure to take this up however 🙏

@sukunrt
Copy link
Member

sukunrt commented Jan 26, 2023

sure! I'd love to!

@sukunrt
Copy link
Member

sukunrt commented Jan 26, 2023

As I understand.

We want two new functions to configure autorelay

libp2p.EnableAutoRelayWithStaticRelays([]peer.AddrInfo, opts ...autorelay.Option)
libp2p.EnableAutoRelayWithPeerSource(func(ctx context.Context, numPeers int) <-chan peer.AddrInfo, opts ...autorelay.Option)

We would need to add the second options parameter because otherwise there would be no way of providing options like WithCircuitV1Support()

In autorelay.WithPeerSource there is a configurable mininterval for callbacks which is set to 30 seconds in autorelay.WithStaticRelays. Should I allow for setting this minInterval too and make the function:

libp2p.EnableAutoRelayWithPeerSource(func(ctx context.Context, numPeers int) <-chan peer.AddrInfo, minInterval time.Duration, opts ...autorelay.Option)

What should I do with the old function libp2p.EnableAutoRelay? Deprecate it or remove it?

@marten-seemann
Copy link
Contributor

We want two new functions to configure autorelay

libp2p.EnableAutoRelayWithStaticRelays([]peer.AddrInfo, opts ...autorelay.Option)
libp2p.EnableAutoRelayWithPeerSource(func(ctx context.Context, numPeers int) <-chan peer.AddrInfo, opts ...autorelay.Option)

That's correct.

In autorelay.WithPeerSource there is a configurable mininterval for callbacks which is set to 30 seconds in autorelay.WithStaticRelays. Should I allow for setting this minInterval too and make the function:

libp2p.EnableAutoRelayWithPeerSource(func(ctx context.Context, numPeers int) <-chan peer.AddrInfo, minInterval time.Duration, opts ...autorelay.Option)

I'd prefer to have this as a autorelay.Option.

What should I do with the old function libp2p.EnableAutoRelay? Deprecate it or remove it?

If it's not too much of a hassle to keep it around (as a deprecated function), I'd say let's just deprecate it. That will make the upgrade process smoother for our users.

@sukunrt
Copy link
Member

sukunrt commented Jan 27, 2023

Can I change autorelay.WithPeerSource from

func WithPeerSource(f func(ctx context.Context, numPeers int) <-chan peer.AddrInfo, minInterval time.Duration) Option 

to

func WithPeerSource(f func(ctx context.Context, numPeers int) <-chan peer.AddrInfo) Option 

and set a default minInterval with autorelay.defaultConfig

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good issue for new contributors
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants