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

implement a new multiaddress API #198

Open
marten-seemann opened this issue Apr 18, 2023 · 1 comment
Open

implement a new multiaddress API #198

marten-seemann opened this issue Apr 18, 2023 · 1 comment

Comments

@marten-seemann
Copy link
Contributor

Problems with the current API

Major problems:

  • When receiving a multiaddress (e.g. from an address record in the DHT), we immediately try to parse it. If parsing fails, that multiaddress is discarded. Parsing fails if the multiaddress contains any unknown components: if a node hasn’t been updated to understand WebTransport addresses, it won’t be able to parse a WebTransport multiaddress, and won’t be able to forward it to other (WebTransport-enabled) nodes.

Related: multiformats/multiaddr#155

Minor problems:

  • Multiaddr is an interface, and therefore (almost always) allocated on the heap. This creates a lot of GC pressure.
  • Methods are split between packet-level functions (e.g. SplitLast) and methods on the interface (e.g. ValueForProtocol). There’s no (apparent) logic behind this, making use of the API confusing
  • The methods weren’t designed with allocations in mind. One of them (especially Protocols) is the major source of allocations in a Kubo node

Proposal for a new API

Wishlist

  • It is possible to handle multiaddresses with unsupported components
  • Multiaddr is not an interface any more, but a concrete type
  • maybe: Multiaddr can be entirely allocated on the stack
  • Multiaddr are comparable using the == operator. This means they can be used as map keys!
  • Component is a concrete type, also allocated on the stack, and == comparable
@MarcoPolo
Copy link
Contributor

Another annoyance I ran into: Component doesn't implement the Multiaddr interface. So if you collect a bunch into a slice: []Components there's no way to join them all. I'm not sure if we even need the component in the new API, just wanted to mention it as another pain point.

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

No branches or pull requests

3 participants
@MarcoPolo @marten-seemann and others