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 PeerChainState #936

Merged
merged 7 commits into from
Jul 29, 2020
Merged

Conversation

earlbread
Copy link
Contributor

This adds PeerChainStatus struct and Swarm<T>.GetPeerChainStatus() method to check the chain status of the connected peers.

longfin
longfin previously approved these changes Jul 28, 2020
limebell
limebell previously approved these changes Jul 28, 2020
@earlbread earlbread dismissed stale reviews from limebell and longfin via 945d6d3 July 28, 2020 10:19
@codecov
Copy link

codecov bot commented Jul 28, 2020

Codecov Report

Merging #936 into master will increase coverage by 0.02%.
The diff coverage is 89.65%.

@@            Coverage Diff             @@
##           master     #936      +/-   ##
==========================================
+ Coverage   87.97%   88.00%   +0.02%     
==========================================
  Files         263      264       +1     
  Lines       24200    24258      +58     
==========================================
+ Hits        21291    21349      +58     
+ Misses       1504     1502       -2     
- Partials     1405     1407       +2     
Impacted Files Coverage Δ
Libplanet/Net/PeerChainState.cs 45.45% <45.45%> (ø)
Libplanet.Tests/Net/SwarmTest.cs 94.84% <100.00%> (+0.14%) ⬆️
Libplanet/Net/Swarm.cs 86.03% <100.00%> (+0.05%) ⬆️
Libplanet/Store/DefaultStore.cs 86.39% <0.00%> (+0.94%) ⬆️

@earlbread
Copy link
Contributor Author

I changed an Array to a HashSet to fix the flaky test. 945d6d3

Libplanet/Net/Swarm.cs Outdated Show resolved Hide resolved
Copy link
Contributor

@dahlia dahlia left a comment

Choose a reason for hiding this comment

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

How about renaming PeerChainStatus to PeerChainState? Cf. https://english.stackexchange.com/questions/12958/status-vs-state. /cc @planetarium/libplanet

@earlbread
Copy link
Contributor Author

How about renaming PeerChainStatus to PeerChainState? Cf. https://english.stackexchange.com/questions/12958/status-vs-state. /cc @planetarium/libplanet

I think that's good. I'll rename it.

riemannulus
riemannulus previously approved these changes Jul 29, 2020
@earlbread earlbread requested a review from dahlia July 29, 2020 05:52
moreal
moreal previously approved these changes Jul 29, 2020
@limebell
Copy link
Member

Currently, Swarm<T>.DialToExistingPeers returns Task<(BoundPeer, ChainStatus)> as its return type. How about merge PeerChainState struct and ChainStatus (and make it public)? Then we can just use Task<PeerChainState> as the method's return type.

@dahlia dahlia changed the title Add PeerChainStatus Add PeerChainState Jul 29, 2020
Co-authored-by: Hong Minhee (洪 民憙) <hong.minhee@gmail.com>
@earlbread earlbread dismissed stale reviews from moreal and riemannulus via 4073220 July 29, 2020 07:21
@earlbread
Copy link
Contributor Author

Currently, Swarm<T>.DialToExistingPeers returns Task<(BoundPeer, ChainStatus)> as its return type. How about merge PeerChainState struct and ChainStatus (and make it public)? Then we can just use Task<PeerChainState> as the method's return type.

I thought about it at first, but to do that, the current internal Message class should be public. Is it okay?

@limebell
Copy link
Member

Currently, Swarm<T>.DialToExistingPeers returns Task<(BoundPeer, ChainStatus)> as its return type. How about merge PeerChainState struct and ChainStatus (and make it public)? Then we can just use Task<PeerChainState> as the method's return type.

I thought about it at first, but to do that, the current internal Message class should be public. Is it okay?

OH I made mistake, you're right.

@earlbread earlbread merged commit cf0d6e7 into planetarium:master Jul 29, 2020
@earlbread earlbread deleted the peer-chain-status branch July 29, 2020 09:40
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.

6 participants