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

Add protocol version and ignore peer if protocol version is different #170

Merged
merged 10 commits into from Apr 3, 2019

Conversation

Projects
None yet
3 participants
@earlbread
Copy link
Member

commented Apr 2, 2019

  • Added network protocol version. #167
  • Made the peers ignore each other if the protocol version is different.

@earlbread earlbread self-assigned this Apr 2, 2019

@earlbread earlbread requested review from dahlia and longfin Apr 2, 2019

@codecov

This comment has been minimized.

Copy link

commented Apr 2, 2019

Codecov Report

Merging #170 into master will decrease coverage by 2.62%.
The diff coverage is 74.5%.

@@            Coverage Diff             @@
##           master     #170      +/-   ##
==========================================
- Coverage   86.93%   84.31%   -2.63%     
==========================================
  Files          70       71       +1     
  Lines        3185     3226      +41     
==========================================
- Hits         2769     2720      -49     
- Misses        416      506      +90
Impacted Files Coverage Δ
Libplanet/Net/Messages/Pong.cs 100% <100%> (+33.33%) ⬆️
Libplanet/Net/Swarm.cs 83% <71.05%> (-3.9%) ⬇️
...planet/Net/DifferentAppProtocolVersionException.cs 71.42% <71.42%> (ø)
Libplanet/Net/IceServer.cs 0% <0%> (-100%) ⬇️
Libplanet/Net/IceServerException.cs 0% <0%> (-100%) ⬇️
Libplanet/Net/NetworkStreamProxy.cs 0% <0%> (-80.77%) ⬇️
Show resolved Hide resolved Libplanet/Net/Swarm.cs Outdated
Show resolved Hide resolved Libplanet/Net/Swarm.cs Outdated

@earlbread earlbread force-pushed the earlbread:protocol-version branch from 6fa9130 to 60e5dc3 Apr 3, 2019

@earlbread earlbread force-pushed the earlbread:protocol-version branch from 60e5dc3 to ba2889d Apr 3, 2019

@earlbread

This comment has been minimized.

Copy link
Member Author

commented Apr 3, 2019

I rebased on the current master. Could you please review again? @longfin @dahlia

@dahlia
Copy link
Member

left a comment

Sorry for my late review. I left some comments.

Show resolved Hide resolved CHANGES.md Outdated
@@ -61,14 +62,16 @@ public partial class Swarm : ICollection<Peer>, IDisposable
IPAddress ipAddress = null,
int? listenPort = null,
DateTimeOffset? createdAt = null,
IEnumerable<IceServer> iceServers = null)
IEnumerable<IceServer> iceServers = null,
int protocolVersion = 1)

This comment has been minimized.

Copy link
@dahlia

dahlia Apr 3, 2019

Member

Since it's not about Libplanet's internals but application's own speak, appProtocolVersion or such name might be better.

And I still it's better not to have the default version, as it's prone to be unaware of this.

namespace Libplanet.Net
{
[Serializable]
public sealed class DifferentProtocolVersionException : Exception

This comment has been minimized.

Copy link
@dahlia

dahlia Apr 3, 2019

Member

Could we have XML comments for docs here?

{
dealer.Dispose();
throw new DifferentProtocolVersionException(
$"Peer protocol version({pong.ProtocolVersion}) is " +

This comment has been minimized.

Copy link
@dahlia

dahlia Apr 3, 2019

Member

We could have rather a well-structured exception type. I mean, how about having a separated properties like DifferentProtocolVersionException.ExpectedVersion and DifferentProtocolVersionException.ActualVersion? This approach is a more programmable way. See also DifferentProtocolVersionException as an example of this approach.

Update CHANGES.md
Co-Authored-By: earlbread <waydi1@gmail.com>

@earlbread earlbread dismissed stale reviews from longfin and longfin via cdb64ae Apr 3, 2019

Show resolved Hide resolved CHANGES.md Outdated
@earlbread

This comment has been minimized.

Copy link
Member Author

commented Apr 3, 2019

I applied the review comments. Could you please review this again? @dahlia @longfin

Update CHANGES.md
Co-Authored-By: earlbread <waydi1@gmail.com>

@earlbread earlbread force-pushed the earlbread:protocol-version branch from 20699bf to ab91eae Apr 3, 2019

@longfin

longfin approved these changes Apr 3, 2019

@dahlia

dahlia approved these changes Apr 3, 2019

@earlbread earlbread merged commit 8931917 into planetarium:master Apr 3, 2019

3 of 5 checks passed

codecov/patch 74.5% of diff hit (target 86.93%)
Details
codecov/project 84.31% (-2.63%) compared to 8abbb26
Details
Travis CI - Pull Request Build Passed
Details
WIP Ready for review
Details
changelog This contains self-describing changelog.
Details

@earlbread earlbread deleted the earlbread:protocol-version branch Apr 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.