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

Let's make fewer breaking changes #71

Closed
thomaseizinger opened this issue Dec 14, 2022 · 4 comments
Closed

Let's make fewer breaking changes #71

thomaseizinger opened this issue Dec 14, 2022 · 4 comments
Milestone

Comments

@thomaseizinger
Copy link
Contributor

thomaseizinger commented Dec 14, 2022

A Multiaddr is a core primitive of libp2p and thus an almost guaranteed dependency of everything libp2p. Every breaking change in this library will trigger a cascade of breaking changes across the ecosystem because Multiaddr constantly appears in public type signatures.

Just within our own crates, a breaking change here causes a breaking change in: libp2p-core -> libp2p-swarm -> libp2p-identify (for example) -> libp2p. There are several users of libp2p that build their own libraries on top which continues the chain.

We should do everything possible to:

  • Harden the API against breaking changes
  • Only make them very selectively (4 breaking changes in 1 year is pretty bad)

After a quick survey, I see the following problems:

  • Protocol is not non_exhaustive: There will always be more protocols, we should future proof the API for that.
  • multihash is re-exported and part of the public API despite being < 1.0: This is a problem. A fundamental crate like multiaddr should only have stable dependencies in its API.

I am not sure how to deal with multihash. It is useful to represent Certhash and P2p in a type-safe way. One thing we can do is just not update to the latest version as quickly unless we actually need a specific feature.

Update: After looking at multihash in more detail and opening and issue there, I think the best way forward would be to split multihash into two crates: one for the definition of multihash and one for all the implementations of hash algorithms, the custom derive, etc.

@thomaseizinger
Copy link
Contributor Author

Now that multihash is basically sorted, here is a list of things we should consider for this crate:

  • Remove unsigned_varint from the public API (it is currently exposed through the Error::InvalidUvar variant
  • Consider making Error completely opaque like we did for multihash::Error?
  • Remove from_url module: Whilst useful, it can easily live in a separate crate

@thomaseizinger thomaseizinger added this to the v0.18 milestone May 5, 2023
@mxinden
Copy link
Member

mxinden commented Jun 5, 2023

Remove unsigned_varint from the public API (it is currently exposed through the Error::InvalidUvar variant

I vaguely remember an in-person discussion about this. Given that unsigned_varint is unlikely to change in the near future, do you consider this a hard requirement for v0.18?

Consider making Error completely opaque like we did for multihash::Error?

Error is already non_exhaustive. Again, do you consider this a hard requirement for v0.18?

Remove from_url module: Whilst useful, it can easily live in a separate crate

Given that from_url is behind a feature flag and given that its only dependency (url) is optional, I would consider this low priority. Agreed @thomaseizinger?

@thomaseizinger
Copy link
Contributor Author

thomaseizinger commented Jun 5, 2023

Remove unsigned_varint from the public API (it is currently exposed through the Error::InvalidUvar variant

I vaguely remember an in-person discussion about this. Given that unsigned_varint is unlikely to change in the near future, do you consider this a hard requirement for v0.18?

Consider making Error completely opaque like we did for multihash::Error?

Error is already non_exhaustive. Again, do you consider this a hard requirement for v0.18?

I'd strongly prefer if we make the Error type opaque. That will give us much more flexibility in the kind of changes we can make further down the line without breaking users.

It is not a hard requirement for v0.18.

Remove from_url module: Whilst useful, it can easily live in a separate crate

Given that from_url is behind a feature flag and given that its only dependency (url) is optional, I would consider this low priority. Agreed @thomaseizinger?

Not a hard requirement for v0.18 either but should be done in the next release then. I opened #94 and added it to a new v0.19 milestone.

@thomaseizinger
Copy link
Contributor Author

I think we can close this now :)

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

2 participants