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

refactor(iroh-net)!: Rename Endpoint::my_addr to Endpoint::node_addr #2362

Merged
merged 1 commit into from
Jun 14, 2024

Conversation

flub
Copy link
Contributor

@flub flub commented Jun 14, 2024

Description

This is in line with e.g. Endpoint::node_id.

Breaking Changes

  • Endpoint::my_addr -> Endpoint::node_addr

Notes & open questions

Other networking APIs tend to have something like "local_addr",
e.g. UdpSocket::local_addr in the standard library or
Endpoint::local_addr in Quinn. Sometimes this is because they also
have a "peer_addr" version, e.g. UdpSocket::peer_addr.

In this light perhaps some of our APIs might benefit from using this
for consistency with other API conventions. In this case this would
become Endpoint::local_node_addr.

We already have Endpoint::local_addr which returns the socket
addresses we are bound to. Other candidates would be:

  • Endpoint::home_relay -> Endpoint::local_home_relay
  • Endpoint::node_id -> Endpoint::local_node_id
  • Endpoint::secret_key -> Endpoint::local_secret_key

But, you can already see this fall apart. Because our endpoint is not
the thing that is connected to a peer (that is the Connection) I don't
think it makes sense to use the term "local" in the APIs.

And perhaps Endpoint::local_addr should be changed anyway, while it is
compatible with other usages it is rather out of tone for us because
we have too many kind of addresses (socket address, node address,
direct address, ...?). So perhaps that one is better off as
Endpoint::bound_sockets or so.

Change checklist

  • Self-review.
  • Documentation updates if relevant.
  • [ ] Tests if relevant.
  • All breaking changes documented.

This is in line with e.g. Endpoint::node_id.
@divagant-martian divagant-martian added this pull request to the merge queue Jun 14, 2024
Merged via the queue into main with commit 61d5109 Jun 14, 2024
25 of 26 checks passed
ppodolsky pushed a commit to izihawa/iroh that referenced this pull request Jun 22, 2024
…0-computer#2362)

## Description

This is in line with e.g. Endpoint::node_id.

## Breaking Changes

- Endpoint::my_addr -> Endpoint::node_addr

## Notes & open questions

Other networking APIs tend to have something like "local_addr",
e.g. UdpSocket::local_addr in the standard library or
Endpoint::local_addr in Quinn.  Sometimes this is because they also
have a "peer_addr" version, e.g. UdpSocket::peer_addr.

In this light perhaps some of our APIs might benefit from using this
for consistency with other API conventions.  In this case this would
become Endpoint::local_node_addr.

We already have Endpoint::local_addr which returns the socket
addresses we are bound to.  Other candidates would be:

- Endpoint::home_relay -> Endpoint::local_home_relay
- Endpoint::node_id -> Endpoint::local_node_id
- Endpoint::secret_key -> Endpoint::local_secret_key

But, you can already see this fall apart.  Because our endpoint is not
the thing that is connected to a peer (that is the Connection) I don't
think it makes sense to use the term "local" in the APIs.

And perhaps Endpoint::local_addr should be changed anyway, while it is
compatible with other usages it is rather out of tone for us because
we have too many kind of addresses (socket address, node address,
direct address, ...?).  So perhaps that one is better off as
Endpoint::bound_sockets or so.


## Change checklist

- [x] Self-review.
- [x] Documentation updates if relevant.
- ~~[ ] Tests if relevant.~~
- [x] All breaking changes documented.
@flub flub deleted the flub/endpoint-api-node-addr branch October 4, 2024 08:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants