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

Different protocol version of peer handling #185

Merged
merged 6 commits into from Apr 10, 2019

Conversation

Projects
None yet
4 participants
@earlbread
Copy link
Member

commented Apr 9, 2019

  • Added appProtocolVersion to Peer.
  • Made ignore or remove Peer if the appProtocolVersion is different.
  • Added EventHandler to handle different version of Peer.

Please review the name of classes as well. I'm not sure they are appropriate.

@earlbread earlbread self-assigned this Apr 9, 2019

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

Show resolved Hide resolved Libplanet/Net/Peer.cs
Show resolved Hide resolved Libplanet/Net/Peer.cs Outdated
Show resolved Hide resolved Libplanet/Net/Swarm.cs Outdated
Show resolved Hide resolved Libplanet/Net/Peer.cs Outdated
Show resolved Hide resolved Libplanet/Net/Swarm.cs Outdated

@earlbread earlbread force-pushed the earlbread:protocol-version-handling branch 2 times, most recently from 6774236 to b657df8 Apr 9, 2019

@earlbread

This comment has been minimized.

Copy link
Member Author

commented Apr 9, 2019

I applied review comments and amended a commit. Could you please review this again? @dahlia @kijun @longfin

@earlbread earlbread force-pushed the earlbread:protocol-version-handling branch from b657df8 to b848956 Apr 9, 2019

@codecov

This comment has been minimized.

Copy link

commented Apr 9, 2019

Codecov Report

Merging #185 into master will decrease coverage by 3.59%.
The diff coverage is 69.56%.

@@            Coverage Diff            @@
##           master     #185     +/-   ##
=========================================
- Coverage   87.18%   83.58%   -3.6%     
=========================================
  Files          71       72      +1     
  Lines        3245     3266     +21     
=========================================
- Hits         2829     2730     -99     
- Misses        416      536    +120
Impacted Files Coverage Δ
Libplanet/Net/DifferentProtocolVersionEventArgs.cs 100% <100%> (ø)
Libplanet/Net/Peer.cs 87.09% <100%> (+1.91%) ⬆️
Libplanet/Net/Swarm.cs 80.11% <53.33%> (-7.39%) ⬇️
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/Peer.cs Outdated
Show resolved Hide resolved Libplanet/Net/Swarm.cs

@earlbread earlbread force-pushed the earlbread:protocol-version-handling branch from b848956 to bee46d2 Apr 10, 2019

@earlbread

This comment has been minimized.

Copy link
Member Author

commented Apr 10, 2019

I applied suggestions. Please take a look.

@longfin
Copy link
Member

left a comment

Sorry for late review. I've leave some comments.

Show resolved Hide resolved Libplanet/Net/Swarm.cs Outdated
Show resolved Hide resolved Libplanet/Net/Swarm.cs
@@ -1381,6 +1414,19 @@ CancellationToken cancellationToken
if (pong.AppProtocolVersion != _appProtocolVersion)

This comment has been minimized.

Copy link
@longfin

longfin Apr 10, 2019

Member

Do we still need the AppProtocolVersion in the Pong?

This comment has been minimized.

Copy link
@earlbread

earlbread Apr 10, 2019

Author Member

I think the Pong should have AppProtocolVersion so that the sender can know the version of the Peer.

@earlbread earlbread force-pushed the earlbread:protocol-version-handling branch from bee46d2 to cf71c9e Apr 10, 2019

@earlbread

This comment has been minimized.

Copy link
Member Author

commented Apr 10, 2019

I applied review comments and amended commits. Please take a look.

@dahlia

dahlia approved these changes Apr 10, 2019

@earlbread earlbread merged commit 078efe7 into planetarium:master Apr 10, 2019

3 of 5 checks passed

codecov/patch 69.56% of diff hit (target 87.18%)
Details
codecov/project 83.58% (-3.6%) compared to 6d193ab
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-handling branch Apr 10, 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.