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

Change type of Peer.EndPoint to DnsEndPoint #166

Merged
merged 2 commits into from
Apr 4, 2019

Conversation

longfin
Copy link
Member

@longfin longfin commented Apr 1, 2019

This PR changes Peer.EndPoint's type to DnsEndPoint to resolve #165.

In this process, I've found a problem that the node behind the NAT can't connect properly, but I will treat it as a separate issue.

@longfin longfin requested review from dahlia, ipdae and earlbread April 1, 2019 06:39
dahlia
dahlia previously approved these changes Apr 1, 2019
earlbread
earlbread previously approved these changes Apr 1, 2019
@codecov
Copy link

codecov bot commented Apr 1, 2019

Codecov Report

Merging #166 into master will decrease coverage by 3.1%.
The diff coverage is 38.46%.

@@            Coverage Diff             @@
##           master     #166      +/-   ##
==========================================
- Coverage   87.04%   83.93%   -3.11%     
==========================================
  Files          71       71              
  Lines        3226     3243      +17     
==========================================
- Hits         2808     2722      -86     
- Misses        418      521     +103
Impacted Files Coverage Δ
Libplanet/Net/Peer.cs 85.18% <100%> (+0.56%) ⬆️
Libplanet/Net/Swarm.cs 81.54% <29.41%> (-5.47%) ⬇️
Libplanet/Net/IceServer.cs 0% <0%> (-100%) ⬇️
Libplanet/Net/IceServerException.cs 0% <0%> (-100%) ⬇️
Libplanet/Net/NetworkStreamProxy.cs 0% <0%> (-80.77%) ⬇️

@longfin longfin dismissed stale reviews from earlbread and dahlia via 452b28c April 1, 2019 06:56
@longfin
Copy link
Member Author

longfin commented Apr 1, 2019

@dahlia @earlbread I've amend the commit to cover Dns.GetHostAddressesAsync() in unittest. PTAL.

@longfin
Copy link
Member Author

longfin commented Apr 1, 2019

I've append another bug fix.

earlbread
earlbread previously approved these changes Apr 1, 2019
CHANGES.md Outdated Show resolved Hide resolved
CHANGES.md Outdated Show resolved Hide resolved
CHANGES.md Outdated Show resolved Hide resolved
@@ -75,7 +76,7 @@ public partial class Swarm : ICollection<Peer>, IDisposable
public Swarm(
PrivateKey privateKey,
TimeSpan dialTimeout,
IPAddress ipAddress = null,
string host = null,
Copy link
Contributor

Choose a reason for hiding this comment

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

What if an invalid address is passed? Does the constructor throw an exception, or merely Swarm crashes at unspecified moments?

Copy link
Member Author

Choose a reason for hiding this comment

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

Neither. Swarm will advertise the address as its endpoint, and as a result will be isolated from the network.

Do we need to check connectability? (It's probably hard on the constructor, and it's possible in StartAsync()).

Copy link
Contributor

Choose a reason for hiding this comment

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

Although it would good to have basic validations on the given ipAddress, at this point we necessarily need that IMHO.

Copy link
Member Author

Choose a reason for hiding this comment

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

host can be an IP address, but it can also be any host name. so I think it is a bit tricky to check here statically.

Libplanet/Net/Swarm.cs Outdated Show resolved Hide resolved
Libplanet/Net/Swarm.cs Show resolved Hide resolved
@longfin longfin self-assigned this Apr 3, 2019
@longfin longfin merged commit 790fcc8 into planetarium:master Apr 4, 2019
limebell pushed a commit to limebell/libplanet that referenced this pull request Jul 7, 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.

Change type of Peer.EndPoint to enable DNS lookup
3 participants