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

core: make announced swarm addresses configurable #3948

Merged
merged 1 commit into from
Jul 31, 2017

Conversation

ghost
Copy link

@ghost ghost commented May 31, 2017

Fixes #3613

Overall this is good for CR though.

@ghost ghost added topic/interop Interoperability topic/libp2p Topic libp2p topic/perf Performance topic/repo Topic repo labels May 31, 2017
@ghost ghost requested review from whyrusleeping, kevina and Kubuxu May 31, 2017 00:55
@ghost ghost added status/in-progress In progress and removed topic/perf Performance labels May 31, 2017
hostopts := &ConstructPeerHostOpts{
AddrsFactory: addrsFactory,
DisableNatPortMap: cfg.Swarm.DisableNatPortMap,
}
peerhost, err := hostOption(ctx, n.Identity, n.Peerstore, n.Reporter,
Copy link
Member

Choose a reason for hiding this comment

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

maybe we could also have a ConstructSwarmOpts that holds all these other arguments.

Copy link
Author

Choose a reason for hiding this comment

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

Once libp2p/go-libp2p#197 is merged I'd like to remove ConstructPeerHostOpts and use basichost.HostOpts, and would like to do the same with regard to swarm.

core/core.go Outdated
return func(allAddrs []ma.Multiaddr) []ma.Multiaddr {
var addrs []ma.Multiaddr
if len(annAddrs) > 0 {
addrs = annAddrs[:]
Copy link
Member

Choose a reason for hiding this comment

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

Why the reslice?

Copy link
Author

Choose a reason for hiding this comment

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

yeah good question :) removing that

Copy link
Author

Choose a reason for hiding this comment

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

Oh yeah right. We build annAddrs once and cache it outside the closure, and the reslice is to prevent multiple calls to the closure from getting back the same slice. It's gonna be equal, but not the same.

@whyrusleeping
Copy link
Member

@lgierth what is the next step here?

@ghost ghost force-pushed the feat/addresses-announce branch from f003ad0 to e040009 Compare June 29, 2017 16:51
@ghost
Copy link
Author

ghost commented Jun 29, 2017

This should be good to go -- I rebased, let's see what CI says.

@ghost
Copy link
Author

ghost commented Jun 29, 2017

Oh right this depends on a libp2p update. Will take care of that later today.

@whyrusleeping
Copy link
Member

@lgierth you should be unblocked here now :)

@Kubuxu
Copy link
Member

Kubuxu commented Jul 30, 2017

Please rebase.

@ghost ghost force-pushed the feat/addresses-announce branch 2 times, most recently from 0a621b1 to 5ea91ee Compare July 31, 2017 00:13
@ghost
Copy link
Author

ghost commented Jul 31, 2017

I rebased, and looked at that reslice once more, it's gone now.

Jenkins green

License: MIT
Signed-off-by: Lars Gierth <larsg@systemli.org>
@whyrusleeping whyrusleeping merged commit f686b4c into master Jul 31, 2017
@whyrusleeping whyrusleeping deleted the feat/addresses-announce branch July 31, 2017 18:34
vyzo added a commit that referenced this pull request Jul 31, 2017
integrate #3948

License: MIT
Signed-off-by: vyzo <vyzo@hackzen.org>
@ghost
Copy link
Author

ghost commented Aug 1, 2017

@Kubuxu what was it you committed here, just another rebase?

vyzo added a commit that referenced this pull request Aug 1, 2017
integrate #3948

License: MIT
Signed-off-by: vyzo <vyzo@hackzen.org>
vyzo added a commit that referenced this pull request Aug 16, 2017
integrate #3948

License: MIT
Signed-off-by: vyzo <vyzo@hackzen.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/in-progress In progress topic/interop Interoperability topic/libp2p Topic libp2p topic/repo Topic repo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants