require explicit conversions to and from address::DeviceId

[cosmicexplorer]

Problem

See https://github.com/signalapp/libsignal-client/pull/286/files#r625970592 from #286 for motivation. We currently use u32 to mean different things in different places, including in the .device_id() method of address::ProtocolAddress. We would prefer instead to require an explicit .into() from an intermediate struct that looks like:

#[derive(Copy, Clone)]
pub struct DeviceId(u32);
impl From<u32> for DeviceId { ... }
impl From<DeviceId> for u32 { ... }

So note that this is not part of the documentation push in #285, but rather a separate type of refactoring change.

Solution

(all of the below also have docstrings)

1. Create address::DeviceId wrapping u32.
2. Modify the existing definition of storage::prekey::PreKeyId wrapping u32.
3. Modify the existing definition of storage::signed_prekey::SignedPreKeyId wrapping u32.
4. Apply .into() calls to convert to and from u32 as necessary to make tests pass.

Result

We don't accidentally mistake a device id for a chain counter!

[jrose-signal]

I think you've made a very strong case for why strong types are a good idea. ;-)

[cosmicexplorer]

@jrose-signal I've split up this change into three logically separate commits to make it easier to review.

[jrose-signal]

Looks like the split up commits aren't quite split up right anyway, so I just reviewed this all together.

[jrose-signal]

Maybe pre_key_id.map(|id| id.into()).unwrap_or(0)? I know it's both equivalent and a little longer, but it does make it clear that "0" is a placeholder rather than a real pre-key with ID #0.

[cosmicexplorer]

That makes much more sense!

[jrose-signal]

Looks like this missed the rebase.

[jrose-signal]

Still shows up in at least one other spot!

[jrose-signal]

I double-checked and the message versions for SenderKeyMessage and SenderKeyDistributionMessage should be the same, as should SignalMessage and PreKeySignalMessage, but they are two different message families and so they should not be the same as each other. I'll put in a PR to record the SK(D)M version in the stored state like we do for 1-1 sessions.

[jrose-signal]

(That was #293.)

[cosmicexplorer]

Thanks so much for fixing the SenderKeyState dissonance in #293! In another branch I haven't pushed I had been wondering what to do with that.

[jrose-signal]

This is looking pretty good, but let's leave out some of the more complicated ones: MessageVersion (and the HKDF changes), and SignedPreKeyTimestamp

[cosmicexplorer]

Was having lots of difficulty making the bridge tests pass and would probably like your assistance on those changes independently, so have reverted the RegistrationId changes and stuffed them into https://github.com/cosmicexplorer/libsignal-client/compare/type-safety-over-ambiguously-named-fields...TYPE-SAFETY-V2?expand=1 for a followup PR.

[jrose-signal]

This one's pretty straightforward, so I picked it up first. Only a few comments.

[jrose-signal]

Looks like this missed the rebase.

[cosmicexplorer]

Ok, one of the CI runs is failing reliably, and I can't reproduce it locally. I'm running cargo +stable test --workspace --verbose -- --include-ignored, but that command succeeds for me. It doesn't look like this test is failing on main, so I'm wondering what I'm doing wrong. The beginning of the errors in https://github.com/signalapp/libsignal/runs/7399386113?check_suite_focus=true look like:

 ---- src/support/mod.rs - support::bridge_get (line 162) stdout ----
error: cannot find macro `bridge_get` in this scope
  --> src/support/mod.rs:172:1
   |
12 | bridge_get!(Foo::bar as GetBar -> &str, ffi = "foo_get_bar", jni = "Foo_GetBar", node = "Foo_GetBar");
   | ^^^^^^^^^^

[cosmicexplorer]

Hm, it looks like that exact failure is happening for all the other PRs I have open right now as well. It seems like the CI might be wonky.

[jrose-signal]

Ah, right, because doctests now get run on macro docs! We have a fix for this privately (because we updated our pinned nightly). Don't worry about it here; since I'll be cherry-picking things to the private branch, I'll make sure it works there before merging and closing the public branch.

[jrose-signal]

Cherry-picked as ec3c2d3, will be in the next release!