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

fix: return connected address in RPC get_peers #2052

Merged
merged 2 commits into from
May 18, 2020

Conversation

keroro520
Copy link
Contributor

The RPC get_peers miss the peer connected address. Hence it may be empty addresses returned for inbound peers. e.g.

      {
         "addresses" : [],
         "version" : "0.29.0 (a6733e6 2020-02-26)",
         "node_id" : "QmPoPDhGPbNfAuDM1kmQFyY8cocC2SXipi6gYV6BmAj2w7",
         "is_outbound" : false
      }

This PR:

  • Includes connected address in the returning addresses
  • Uses the score recorded in peer_store as the returning peer score

@keroro520 keroro520 requested review from quake, jjyr and a team April 30, 2020 05:35
@doitian
Copy link
Member

doitian commented May 6, 2020

Is it possible to add test cases to cover more branches instead of just relying on the RPC documentation test?

@doitian doitian added this to 👀 Awaiting review in CKB - Pull Requests May 7, 2020
@keroro520
Copy link
Contributor Author

Is it possible to add test cases to cover more branches instead of just relying on the RPC documentation test?

Adding integration tests can cover more detail. It seems hard to do it in unit tests.

CKB - Pull Requests automation moved this from 👀 Awaiting review to ✅ Reviewer approved May 18, 2020
@quake
Copy link
Member

quake commented May 18, 2020

bors r=quake,doitian

@bors
Copy link
Contributor

bors bot commented May 18, 2020

Build succeeded:

  • continuous-integration/travis-ci/push

@bors bors bot merged commit 4a0da61 into nervosnetwork:develop May 18, 2020
CKB - Pull Requests automation moved this from ✅ Reviewer approved to Done May 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants