Convenience methods for ServiceId

[rubdos]

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]

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]

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 whisperfish/libsignal-service-rs#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]

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]

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]

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

[rubdos]

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!