search

multiformats / rust-multiaddr

multiaddr implementation in rust
https://crates.io/crates/multiaddr
Other
86 stars 45 forks source link

fix: Rename `Protocol::WebRTC` to `Protocol::WebRTCDirect` #78

Closed mxinden closed 1 year ago

mxinden commented 1 year ago

See https://github.com/multiformats/multiaddr/pull/150#issuecomment-1468791586 for context.

@melekes @thomaseizinger can you give this a review?

mxinden commented 1 year ago

Could we first release a version where we accept parsing as /webrtc-direct and not rename the variant, thus making this a non-breaking change?

On first thought this is a great idea. Though I am not sure there are no hidden surprises with this strategy. E.g. the guarantee of round-trip parsing (/webrtc-direct -> binary -> /webrtc-direct) no longer holds.

Breaking change:

Non-breaking change:

Thoughts @thomaseizinger?

mxinden commented 1 year ago

Friendly ping @thomaseizinger. What are your thoughts here?

thomaseizinger commented 1 year ago

Friendly ping @thomaseizinger. What are your thoughts here?

Didn't submit my comment :man_facepalming:

thomaseizinger commented 1 year ago

Could we first release a version where we accept parsing as /webrtc-direct and not rename the variant, thus making this a non-breaking change?

On first thought this is a great idea. Though I am not sure there are no hidden surprises with this strategy. E.g. the guarantee of round-trip parsing (/webrtc-direct -> binary -> /webrtc-direct) no longer holds. Breaking change:

  • Avoid long-term confusion, i.e. get it over with quickly
  • Needs a breaking change in libp2p

Non-breaking change:

  • Safes us and our users a lot of work
  • Might be a footgun
  • Could also default to /webrtc-direct from binary to text already now
  • Takes time to fully finish, thus less time between now and the introduction of browser-to-browser /webrtc

Thoughts @thomaseizinger?

What we'd need to work out is how exactly we are going to introduce the other /webrtc protocol after this.

We definitely need to log a deprecation warning if we are now adding the 2nd representation.

Then in a breaking change, we can rename the variant and add the "new" /webrtc protocol.

How big is the footgun actually?

thomaseizinger commented 1 year ago

How big is the footgun actually?

The only problem I can think of is that in the "breaking" change where we add the new (browser-to-browser) /webrtc protocol, users of multiaddr might not realise that the WebRTC protocol semantically changes. However, afaik the only user of this is us in libp2p-webrtc and we know to be careful here.

Ideally, semantic breaking changes are also compile-time breaking changes but I'd say with sufficient documentation, we can also deviate from that. I've pushed https://github.com/multiformats/rust-multihash/pull/272 forward a lot so I'd love if the next breaking change can already include that (and thus be the last one in a while).

thomaseizinger commented 1 year ago

If we move forward with this, it would have to be done as a patch-release from 0.17.0 because master already contains breaking changes.

mxinden commented 1 year ago

Pull request for the non-breaking change: https://github.com/multiformats/rust-multiaddr/pull/84

mxinden commented 1 year ago

With https://github.com/multiformats/rust-multiaddr/pull/84 released as v0.17.1, I updated this pull request, making the breaking change of renaming Protocol::WebRTC to Protocol::WebRTCDirect and removal of the deprecated /webrtc string representation.

mxinden commented 1 year ago

I seem to only be able to request a review from one of the two of you.

DougAnderson444 commented 1 year ago

@mxinden when is this v0.18 planned for release so that we can use the new naming without patches?

thomaseizinger commented 1 year ago

I'd first like to get a release of rust-multihash out: https://github.com/multiformats/rust-multihash/milestone/1

thomaseizinger commented 1 year ago

I'd first like to get a release of rust-multihash out: multiformats/rust-multihash/milestone/1

I've created a milestone for rust-multiaddr too so we can easily keep track of this: https://github.com/multiformats/rust-multiaddr/milestone/3