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

Kademlia.Peer.t is a record instead of a pair #1466

Merged
merged 4 commits into from Jan 18, 2019

Conversation

Projects
None yet
2 participants
@psteckler
Copy link
Contributor

psteckler commented Jan 18, 2019

Peer.t was a pair, where the first item was a Host_and_port.t, and the second item was an int representing a port. So there were two ports, and there was nothing in the data structure to indicate the roles of those two ports.

In this PR, Peer.t is a record, where the host and two ports each gets a field.

Eventually, there should be peer types for the host and discovery port, and host and communication port. But that's a refinement.

This is a step to fixing #1367. The code that passes a preferred peer to get_ancestry remains broken, this is a step towards fixing that.

  • Tests were added for the new behavior
  • All tests pass (CI will check this if you didn't)
  • Does this close issues? List them:
    Closes #1366

Paul Steckler and others added some commits Jan 18, 2019

@cmr
Copy link
Contributor

cmr left a comment

Overall, looks good. See one comment about the new TODO.

(Envelope.Incoming.wrap
~data:
staged_ledger_aux
(* TODO: this isn't really a discovery port, how to handle? *)

This comment has been minimized.

@cmr

cmr Jan 18, 2019

Contributor

As in, the envelope doesn't want a discovery port, or the peer doesn't have a real discovery port? The peer should be fine I think? I think the port maybe shouldn't even be in the envelope, but that's work for another day... Does anything need to be done with this TODO to land this PR?

This comment has been minimized.

@psteckler

psteckler Jan 18, 2019

Author Contributor

The sender doesn't have a real discovery port. Eventually, the goal should be to have sender in envelopes be a Peer.t (as in #1367), but that's beyond this PR.

Nothing needs to be done for the TODO in this PR.

psteckler added some commits Jan 18, 2019

@cmr

cmr approved these changes Jan 18, 2019

@psteckler psteckler merged commit b5a75bb into master Jan 18, 2019

10 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: build-macos Your tests passed on CircleCI!
Details
ci/circleci: build_public Your tests passed on CircleCI!
Details
ci/circleci: build_withsnark Your tests passed on CircleCI!
Details
ci/circleci: test-all_sig_integration_tests Your tests passed on CircleCI!
Details
ci/circleci: test-all_stake_integration_tests Your tests passed on CircleCI!
Details
ci/circleci: test-unit-test Your tests passed on CircleCI!
Details
ci/circleci: test-withsnark Your tests passed on CircleCI!
Details
ci/circleci: tracetool Your tests passed on CircleCI!
Details

@psteckler psteckler deleted the fix/peer-type-as-record branch Jan 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment