search

signalapp / libsignal

Home to the Signal Protocol as well as other cryptographic primitives which make Signal possible.
GNU Affero General Public License v3.0
3.62k stars 420 forks source link

Convenience methods for ServiceId #595

Closed rubdos closed 1 week ago

rubdos commented 2 weeks ago

These are some convenience methods that make working with ServiceId and ProtocolAddress a tiny bit more convenient. I haven't been able to find use cases in the libsignal-protocol stack itself, but I haven't looked very thoroughly either.

jrose-signal commented 2 weeks ago

Thanks! We discussed these and we think the aci and pni ones don't really pull their weight (note that Aci and Pni already implement TryFrom<ServiceId>). But we can take the to_protocol_address one, though it probably makes sense to use &self there rather than self.

(There may be a point in the future where ProtocolAddress requires a ServiceId instead of an arbitrary String, which would make this redundant. But that would mean changing a bunch of old tests written in the phone number days in all our app repos, so we're not rushing to do it.)

rubdos commented 2 weeks ago

Thanks! We discussed these and we think the aci and pni ones don't really pull their weight (note that Aci and Pni already implement TryFrom<ServiceId>).

Maybe @gferon wants to chip in on why these are useful; I ported them over from an extension trait in https://github.com/whisperfish/libsignal-service-rs/pull/334

But we can take the to_protocol_address one, though it probably makes sense to use &self there rather than self.

I would argue that as long as ServiceId implements Copy, taking self would be more appropriate. I even think Clippy will emit a warning if you have an inherent &self on it.

I'll happily drop the 4b37cd6 commit from this PR!

(There may be a point in the future where ProtocolAddress requires a ServiceId instead of an arbitrary String, which would make this redundant. But that would mean changing a bunch of old tests written in the phone number days in all our app repos, so we're not rushing to do it.)

I think that makes sense, and this PR could be a first step towards it!

jrose-signal commented 2 weeks ago

Yeah, I can see the Copy thing. 17 bytes is right on the border of "is it a good idea to Copy this?", but apparently it still gets passed in registers, so maybe it's reasonable after all. We do have existing methods on ServiceId that use &self, so if there's a lint it's not on by default.

rubdos commented 1 week ago

17 bytes is right on the border of "is it a good idea to Copy this?"

My mental threshold is somewhere between 16 and 32 bytes, but that's based on nothing really.

but apparently it still gets passed in registers, so maybe it's reasonable after all.

Very device dependent, I'd say. ARMv9 and x86, sure, but I wouldn't necessarily bet on armv7 to always pass this through registers if other variables are hot too.

We do have existing methods on ServiceId that use &self, so if there's a lint it's not on by default.

That's fair enough. I made it &self, that way you can drop Copy whenever you want too, without breaking more API than necessary.

I also dropped the aci/pni methods, because .into() is convenient enough usually.

jrose-signal commented 1 week ago

Cherry-picked into our private branch and will be included in the next release!

rubdos commented 1 week ago

Cherry-picked into our private branch and will be included in the next release!

Great, thanks :-)

I'll try to remove our extension trait altogether, and suggest all users to just use ServiceId::from/Into<ServiceId>::into() instead. Thanks!

jrose-signal commented 1 week ago

Included in v0.62.0!