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)!: remove the magicsock module from the public api #2247

Conversation

divagant-martian
Copy link
Contributor

@divagant-martian divagant-martian commented Apr 26, 2024

Description

magicsock main's component, the MagicSock is not useful on it's own. Having it on our public API forces us to treat as breaking changes modifications that for a MagicEndpoint user are implementation details. To avoid this, this PR makes the following changes:

  • expose all types that are part of the public functions of the MagicSock as pub use
  • explicitly re-export every publicly exposed by magicsock from magic_endpoint (so that the types involved in the prev change can be used from magic_endpoint directly)
  • changes the description of the magic_endpoint module to not include the MagicSock and make it more expressive at the same time.
  • MagicSock had a bunch of methods that were simply methods of Inner re-exposed. Now that MagicSock is private, we can get rid of those all and simply use Deref to Inner without exposing Inner via trait associated types.
  • That being said, renames MagicSock to magicsock::Handle which is what it really is, leaving only the new and close methods, which are the ones that handle creation and destruction of the associated tasks.
  • Misc rename of fields of type MagicSock to msock, which I think aligns better to new naming as is more readable/clear.
  • Simplifies the Timer code. The removed parts were considered used because they were publicly accessible. Becoming private showed parts that were dead code.
  • local_addr no longer returns an error (it seems to be it never did), but it became more clear in the process of accessing the MagicSock(Inner) methods via Deref.
  • MagicEndpoint::local_addr no longer returns an error

Breaking Changes

  • module magicsock is no longer public
  • iroh_net::magicsock::ConnectionType -> iroh_net::magic_endpoint::ConnectionType
  • iroh_net::magicsock::ControlMsg -> iroh_net::magic_endpoint::ControlMsg
  • iroh_net::magicsock::ConnectionInfo -> iroh_net::magic_endpoint::ConnectionInfo
  • iroh_net::magicsock::ConnectionTypeStream -> iroh_net::magic_endpoint::ConnectionTypeStream
  • iroh_net::magicsock::DirectAddrInfo -> iroh_net::magic_endpoint::DirectAddrInfo
  • iroh_net::magicsock::LocalEndpointsStream -> iroh_net::magic_endpoint::LocalEndpointsStream
  • iroh_net::magicsock::MagicSock no longer accessible. Use MagicEndpoint instead.
  • iroh_net::magicsock::Options no longer public. Used internally only.
  • iroh_net::magicsock::PacketSplitIter no longer public. This is an implementation detail of internal use only.
  • iroh_net::magicsock::Timer no longer public. This is an implementation detail of internal use only.
  • iroh_net::magicsock::UdpSocket no longer public. This is an implementation detail of internal use only.

Notes & open questions

ended up using cargo public-api -p iroh-net diff to get what was added and make some of those not-for-public enums and structs private again. While really useful, checking this is very annoying. Hopefully we don't have to do it often.

Change checklist

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

@dignifiedquire dignifiedquire changed the title refactor(iroh_net)!: remove the magicsock module from the public api refactor(iroh-net)!: remove the magicsock module from the public api Apr 26, 2024
@rklaehn
Copy link
Contributor

rklaehn commented Apr 29, 2024

I don't have time for a thorough review, but I very much agree with this, if only to reduce the confusion when talking about MagicEndpoint and MagicSocket.

ended up using cargo public-api -p iroh-net diff to get what was added and make some of those not-for-public enums and structs private again. While really useful, checking this is very annoying. Hopefully we don't have to do it often.

Didn't know this. Nice!

Copy link
Contributor

@dignifiedquire dignifiedquire left a comment

Choose a reason for hiding this comment

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

one change please, other than that LGTM

@dignifiedquire dignifiedquire added this to the v0.15.0 milestone Apr 29, 2024
@divagant-martian divagant-martian added this pull request to the merge queue Apr 29, 2024
Merged via the queue into n0-computer:main with commit 06e0b7b Apr 29, 2024
21 of 22 checks passed
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