search

whisperfish / libsignal-service-rs

A Rust version of the libsignal-service-java library for communicating with Signal servers.
https://whisperfish.github.io/libsignal-service-rs/libsignal_service
GNU Affero General Public License v3.0
68 stars 31 forks source link

Refactoring ACI/PNI storage types #275

Closed rubdos closed 9 months ago

rubdos commented 10 months ago

Branch as I go along, with things that I disagree with :'-)

One thing I disagree with, but don't necessarily want to fix here: set_next_prekey_id sounds like a great source of confusion. In RDBMS-based implementations, the next_prekey_id-family is expected to just query the database and return max+1. Setting a value doesn't make any sense, but the naming of the method implies it should need to be a method with a side effect.

codecov[bot] commented 10 months ago

Codecov Report

Attention: 30 lines in your changes are missing coverage. Please review.

Comparison is base (1cc7517) 17.21% compared to head (c5b5662) 17.07%.

:exclamation: Current head c5b5662 differs from pull request most recent head e42688c. Consider uploading reports for the commit e42688c to get more accurate results

Files Patch % Lines
libsignal-service/src/push_service.rs 0.00% 23 Missing :warning:
libsignal-service/src/configuration.rs 0.00% 3 Missing :warning:
libsignal-service/src/service_address.rs 0.00% 3 Missing :warning:
libsignal-service/src/pre_keys.rs 0.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #275 +/- ## ========================================== - Coverage 17.21% 17.07% -0.15% ========================================== Files 38 38 Lines 3073 3099 +26 ========================================== Hits 529 529 - Misses 2544 2570 +26 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

rubdos commented 9 months ago

Let's ship it, I think I had a bit more quality of life improvements to add, but I can do it later! Thanks for this 😄

FYI, this is 100% untested. I probably want to ship some more things, but https://gitlab.com/whisperfish/whisperfish/-/merge_requests/530 is on hold currently until after release :'-)

rubdos commented 9 months ago

(This is more becoming the "let's just implement it damnit" merge request)

rubdos commented 9 months ago

(This is more becoming the "let's just implement it damnit" merge request)

In order to avoid this, I'll cut this PR short on just some "minor" PNI/ACI refactoring, such that https://gitlab.com/whisperfish/whisperfish/-/merge_requests/530#02abb4bfed1d40529ab2bc6cfe9338119e97be98 is also mergable. Continuation in #285.