refactor(deps): drop redundant crates and unused dependency features - #1200
Conversation
Nine crates leave the dependency graph, each replaced by something the workspace already links or by plain core: - typed-builder: bon is already a hard dependency of wacore and covers the one struct pair that used it (wacore/src/iq/groups.rs). - flate2: the only runtime use was inflating the MLow table blobs, which wacore-binary's pooled zlib-rs inflater already does. It stays as a dev-dependency for the compression side of tests. - bytemuck: the three casts in lthash were transmutes between [u8; 16] and [u16; 8] wrapped in a manual big-endian swap. from_le_bytes per lane states the wire endianness once and drops both. - arrayref: split_first_chunk carries the window length in the type. - displaydoc: thiserror was already derived alongside it on every enum, and #[error(...)] is the same format string. - derive_more: the From/Into/TryFrom derives covered newtypes and two repr enums; the hand-written impls are shorter than the attributes were. - uuid: only reachable through Aci/Pni/ServiceId, which are Signal-app identity types with no WhatsApp meaning and no use anywhere in the tree. - iana-time-zone and its two platform backends: chrono's `clock` feature exists to resolve the local timezone, and only Utc is ever used. Feature trims that keep the same crates but compile less of them: hashbrown's `equivalent` and aes-gcm's `bytes` were both enabled and unused. Also removes three dev-dependency entries that restated a normal dependency verbatim, and points waproto's dev serde_json at the workspace pin. BREAKING CHANGE: wacore_libsignal no longer exports Aci, Pni, ServiceId, ServiceIdKind, ServiceIdFixedWidthBinaryBytes or WrongKindOfServiceIdError. The repr-based TryFrom impls on CiphertextMessageType and IdentityChange now fail with wacore_libsignal::core::UnknownDiscriminant instead of derive_more::TryFromReprError.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR narrows dependencies, replaces generated Rust implementations, removes service identifier APIs, changes libsignal error formatting, updates byte and decompression paths, replaces ratchet slicing macros, and migrates group option builders to ChangesWorkspace and library cleanup
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50dbe13863
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
📦 Binary size report
.text per crate
Top movers (cargo-bloat attribution)
Baseline: |
Three shapes changed by accident in the previous commit rather than by decision: - typed-builder's build_method(into) made build() finish into anything the options convert into. bon's finisher is renamed to finish() and build() is hand-written generic over From, so a downstream impl From<GroupCreateOptions> for its own request type still compiles. The reflexive From covers the plain case. - IdentityChange is repr(C) again. Its layout is a public guarantee for FFI, and the hand-written conversion never needed the repr narrowed to u8. - Because repr(C) leaves the discriminant type unspecified, IdentityChange's TryFrom takes isize, matching what the repr-driven derive fell back to. UnknownDiscriminant is generic over the discriminant type to carry both it and CiphertextMessageType's u8.
|
All 63 flagged items are Everything else is green: clippy, rustdoc, fmt, cargo-deny, all four Miri jobs, wasm32 release, E2E, and the CodSpeed suites. Generated by Claude Code |
Merging this PR will degrade performance by 7.2%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | bench_reject_prekey_as_signal |
1.1 µs | 1.2 µs | -7.2% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing claude/audit-optimize-dependencies-uk195m (79c680b) with main (309e95d)
Footnotes
-
2 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
|
@codspeedbot why this is slower? |
Short answer: it isn't. The two revisions produce identical measurements when they run on the same machine — the 7.2% is the reported comparison crossing two different CI runners on a benchmark whose entire cost is 39 cache lines. What the numbers actually say
The instruction term is bit-identical. Everything that moved is the cache/memory model, which is 93% of this benchmark's reported cost. In raw terms the whole delta is 3 last-level cache lines (36 → 39). One line is ~2.4% of this benchmark, so 3 lines is 7.2%. Controlled A/BI built and ran both revisions locally on one machine, same toolchain, same simulation instrument, and pulled the raw callgrind totals for this benchmark:
Identical. Not "within noise" — the same numbers. The miss split shuffles cosmetically (base I ran the whole libsignal suite the same way while I was there. All 21 benchmarks land within ±0.25% between base and head: Where the CI delta came fromThe base run executed on an RecommendationAcknowledge it. Nothing in the diff touches this path, and the local A/B shows the code is neutral. If you want it to stop flapping: the benchmark body is 174 instructions and ~89% of its reported cost is the cold-cache model, so its resolution is ~2.4% per cache line and it will keep moving whenever the binary is relinked or the runner changes. Looping the parse a few dozen times inside Summary
Was this response helpful? React with 👍 or 👎 to help us improve CodSpeedBot. |
|
Pulled the raw instrument breakdown for
The instruction count is identical. Not close, identical. The executed instruction stream for this benchmark did not change, which rules out a code-path change: And those two rows moved by exactly the same factor, 13/12, to the digit. A real regression moves memory accesses and cache misses independently, because it changes what the code touches. A uniform scale factor on both is a change in the model's cost weights, and CodSpeed's own comparison says why: the base ran on an Corroborating: all 216 benchmarks crossed the same runner change and 215 came back unchanged. This one surfaced because at ~1.1 µs it is among the smallest in the suite, so a fixed relative shift in the memory-cost weight clears the threshold here and nowhere else. For completeness, the reject path is No action taken. Happy to acknowledge it on CodSpeed if you'd rather clear the warning. Generated by Claude Code |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wacore/appstate/src/lthash.rs (1)
81-101: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the SIMD test use an independent little-endian oracle.
Lines 181-187 call
perform_pointwise_with_overflowtwice, so asimdbuild executes the SIMD implementation for both values. Use asymmetric lane bytes and compute expected wrapping results directly fromu16::from_le_bytes/to_le_bytes; this will catch the endian regression this change addresses.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wacore/appstate/src/lthash.rs` around lines 81 - 101, Update the SIMD test around the two perform_pointwise_with_overflow calls so it uses asymmetric lane bytes and an independent little-endian oracle rather than invoking the SIMD implementation for both expected values. Compute each expected lane with u16::from_le_bytes, apply the required wrapping operation, convert with to_le_bytes, and compare the implementation result against those independently derived bytes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@wacore/libsignal/src/protocol/storage/traits.rs`:
- Around line 30-49: Assign explicit numeric discriminants to both
IdentityChange variants, setting NewOrUnchanged to 0 and ReplacedExisting to 1.
Keep the existing TryFrom<u8> mapping unchanged and follow the
explicit-discriminant style used by CiphertextMessageType.
---
Outside diff comments:
In `@wacore/appstate/src/lthash.rs`:
- Around line 81-101: Update the SIMD test around the two
perform_pointwise_with_overflow calls so it uses asymmetric lane bytes and an
independent little-endian oracle rather than invoking the SIMD implementation
for both expected values. Compute each expected lane with u16::from_le_bytes,
apply the required wrapping operation, convert with to_le_bytes, and compare the
implementation result against those independently derived bytes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 28c66b3f-94ac-4c8a-b034-25b63fc44010
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (25)
Cargo.tomlstorages/chat-store/Cargo.tomlstorages/sqlite-storage/Cargo.tomlwacore/Cargo.tomlwacore/appstate/Cargo.tomlwacore/appstate/src/lthash.rswacore/libsignal/Cargo.tomlwacore/libsignal/src/core/address.rswacore/libsignal/src/core/curve.rswacore/libsignal/src/core/mod.rswacore/libsignal/src/crypto/aes_cbc.rswacore/libsignal/src/crypto/error.rswacore/libsignal/src/crypto/provider.rswacore/libsignal/src/protocol/error.rswacore/libsignal/src/protocol/identity_key.rswacore/libsignal/src/protocol/mod.rswacore/libsignal/src/protocol/protocol.rswacore/libsignal/src/protocol/ratchet/keys.rswacore/libsignal/src/protocol/state/prekey.rswacore/libsignal/src/protocol/state/signed_prekey.rswacore/libsignal/src/protocol/storage/traits.rswacore/noise/Cargo.tomlwacore/src/iq/groups.rswacore/src/voip/mlow/smpl_tables_blob.rswaproto/Cargo.toml
💤 Files with no reviewable changes (4)
- wacore/appstate/Cargo.toml
- storages/sqlite-storage/Cargo.toml
- wacore/noise/Cargo.toml
- wacore/libsignal/Cargo.toml
| #[derive(Copy, Clone, Debug, Eq, PartialEq)] | ||
| #[repr(u8)] | ||
| pub enum IdentityChange { | ||
| /// The protocol address didn't have an identity key or had the same key. | ||
| NewOrUnchanged, | ||
| /// The new identity key replaced a different key for the protocol address. | ||
| ReplacedExisting, | ||
| } | ||
|
|
||
| impl TryFrom<u8> for IdentityChange { | ||
| type Error = crate::core::UnknownDiscriminant; | ||
|
|
||
| fn try_from(value: u8) -> std::result::Result<Self, Self::Error> { | ||
| match value { | ||
| 0 => Ok(Self::NewOrUnchanged), | ||
| 1 => Ok(Self::ReplacedExisting), | ||
| _ => Err(crate::core::UnknownDiscriminant { value }), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
Pin the discriminants explicitly on IdentityChange.
Correct today because Rust defaults the first variant to 0, but the mapping is implicit — nothing stops a future edit from reordering/inserting a variant and silently breaking the hand-written TryFrom<u8> byte mapping. CiphertextMessageType right next door does this correctly with explicit values; let's be consistent and safe here too. I don't want silent protocol drift on my watch.
🔧 Proposed fix
#[derive(Copy, Clone, Debug, Eq, PartialEq)]
#[repr(u8)]
pub enum IdentityChange {
/// The protocol address didn't have an identity key or had the same key.
- NewOrUnchanged,
+ NewOrUnchanged = 0,
/// The new identity key replaced a different key for the protocol address.
- ReplacedExisting,
+ ReplacedExisting = 1,
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[derive(Copy, Clone, Debug, Eq, PartialEq)] | |
| #[repr(u8)] | |
| pub enum IdentityChange { | |
| /// The protocol address didn't have an identity key or had the same key. | |
| NewOrUnchanged, | |
| /// The new identity key replaced a different key for the protocol address. | |
| ReplacedExisting, | |
| } | |
| impl TryFrom<u8> for IdentityChange { | |
| type Error = crate::core::UnknownDiscriminant; | |
| fn try_from(value: u8) -> std::result::Result<Self, Self::Error> { | |
| match value { | |
| 0 => Ok(Self::NewOrUnchanged), | |
| 1 => Ok(Self::ReplacedExisting), | |
| _ => Err(crate::core::UnknownDiscriminant { value }), | |
| } | |
| } | |
| } | |
| #[derive(Copy, Clone, Debug, Eq, PartialEq)] | |
| #[repr(u8)] | |
| pub enum IdentityChange { | |
| /// The protocol address didn't have an identity key or had the same key. | |
| NewOrUnchanged = 0, | |
| /// The new identity key replaced a different key for the protocol address. | |
| ReplacedExisting = 1, | |
| } | |
| impl TryFrom<u8> for IdentityChange { | |
| type Error = crate::core::UnknownDiscriminant; | |
| fn try_from(value: u8) -> std::result::Result<Self, Self::Error> { | |
| match value { | |
| 0 => Ok(Self::NewOrUnchanged), | |
| 1 => Ok(Self::ReplacedExisting), | |
| _ => Err(crate::core::UnknownDiscriminant { value }), | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wacore/libsignal/src/protocol/storage/traits.rs` around lines 30 - 49, Assign
explicit numeric discriminants to both IdentityChange variants, setting
NewOrUnchanged to 0 and ReplacedExisting to 1. Keep the existing TryFrom<u8>
mapping unchanged and follow the explicit-discriminant style used by
CiphertextMessageType.
2 participants
Summary
Audit of every direct dependency in the workspace, looking for crates whose job something already linked can do, plus features that are enabled but never reach any code. Nine crates leave the dependency graph (471 to 462 in
Cargo.lock) and two enabled-but-unused features are dropped. Every removal is a substitution by something already in the tree; the breaking changes are listed explicitly below and are all in dead or unreferenced surface.Audit
typed-builder(+typed-builder-macro)bon, already a hard dependency ofwacorewacore/src/iq/groups.rsflate2(runtime)wacore_binary::zlib_pool::decompress_zlib_pooled, already zlib-rswacore/src/voip/mlow/smpl_tables_blob.rs:35bytemucku16::from_le_bytes/to_le_bytesper lanewacore/appstate/src/lthash.rs:81arrayrefslice::split_first_chunkwacore/libsignal/src/protocol/ratchet/keys.rsdisplaydocthiserror, already derived on every one of those enumswacore/libsignal/srcderive_more(+derive_more-impl)From/Into/TryFromwacore/libsignal/srcuuidServiceIdtypeswacore/libsignal/src/core/address.rsiana-time-zone(+-haiku,android_system_properties)nowinstead ofclockstorages/chat-storeTwo notes on the interesting ones:
flate2. The only non-test call wasinflate()for the MLow constant tables.wacore-binaryalready talks to zlib-rs directly through the pooled inflater, andwacorealready depends on it, so the wrapper crate bought nothing. It stays as a dev-dependency because tests still need the compression side, which the pool does not expose.bytemuck. The casts were transmutes between the byte array and the lane array, followed by a manualswap_bytespass undercfg!(target_endian = "big").from_le_bytesper lane says the same thing once, and on little-endian lowers to the load the transmute emitted.Things checked and deliberately left alone:
x25519-dalekcould be expressed on top ofcurve25519-dalek(already a direct dependency), butStaticSecretclamping anddiffie_hellmanare load-bearing crypto and the wrapper is thin. Not worth the risk for one crate.scopeguardhas no in-tree equivalent, so replacing it means copying it. Kept.httpinwhatsapp-rust-tokio-transportlooks removable, butClientBuilder::add_headertakeshttp::HeaderNameandhttp::HeaderValue, and tokio-websockets does not re-export them.buffa'sjsonfeature andserde'srcare both load-bearing:jsonsupplies theSerializeimpl forMessageFieldthat the generated waproto code needs,rcsupplies it for theArc<str>fields inwacore/src/store/traits.rs. Both were verified by removing them and watching the build fail.diesel's32-column-tablesis the smallest tier that fits; the widest table (device) has 27 columns.tokio-websockets'randpicks the RNG backend;randis the one already in the tree, so it is the right pick overfastrand/getrandom.aws-lc-rsappears inCargo.lockbutcargo tree -ifinds no path to it under any feature combination, so there is nothing to gate.scheduled-thread-pool,stable_deref_trait,md5,smoothutf8,itoa,portable-atomicandhashbrownare each used directly for a reason that has no substitute in the tree.Breaking changes
All four are in surface with no in-tree call site.
wacore_libsignalno longer exportsAci,Pni,ServiceId,ServiceIdKind,ServiceIdFixedWidthBinaryBytesorWrongKindOfServiceIdError. These are Signal-app account identity with no WhatsApp meaning, and the only reasonuuidwas linked. Nothing to migrate: no call site existed.TryFromforCiphertextMessageTypeandIdentityChangenow fails withwacore_libsignal::core::UnknownDiscriminant<T>instead ofderive_more::TryFromReprError<T>. Migration: match onUnknownDiscriminant { value }. The input types are unchanged (u8andisizerespectively, the latter being what#[try_from(repr)]fell back to underrepr(C)).chrono::Localis no longer reachable through thewhatsapp_rust::chronore-export, sinceLocalis gated behind chrono'sclock. Deliberate:Localnever appears in this crate's API, the re-export exists so consumers can name the types that do, andclockcosts three crates plus a per-OS timezone backend. Migration: depend onchronodirectly withclock.GroupCreateOptionsBuilder::buildandGroupParticipantOptionsBuilder::buildare hand-written rather than derived, preserving the genericbuild<T: From<Options>>() -> Tshape thattyped-builder'sbuild_method(into)produced. bon's own finisher isfinish(). No source change should be needed on either side, andcargo-semver-checksconfirms it:wacoreis in the checked set and comes back clean.Changes
wacore:GroupCreateOptionsandGroupParticipantOptionsbuild withbon::Builder. bon spells a non-Nonedefault only on a member markedrequired, so the four fields defaulting to a present value carryrequiredplus awithclosure that restores the bare-value setter. Setter shapes andbuild()'s signature are unchanged.wacore: MLow table blobs inflate throughwacore-binary's pooled inflater, with a 4 MiB cap (the largest committed blob is 30 KB compressed).wacore-appstate: the SIMD ltHash lane conversion is explicit little-endian, dropping the endianness branch along withbytemuck.wacore-libsignal:displaydocdoc comments became#[error(...)]attributes with the same format strings;CurveErrorderivesthiserror::Errorinstead of hand-implementingstd::error::Error.IdentityChangekeeps#[repr(C)], so its C layout guarantee is untouched.chronomoves fromclocktonowin the root package andchat-store.hashbrown'sequivalentandaes-gcm'sbytesfeatures are dropped. Verified against the resolved graph, not just the manifest:cargo tree -f "{p} | {f}"now showshashbrown v0.17.1with no features andaes-gcm v0.11.0with onlyaes,alloc.async-trait,sqlite-storagetokio,wacore-noisewaproto) are removed, and waproto's devserde_jsonnow uses the workspace pin instead of a loose"1".Cost
Nine crates out of
Cargo.lock, and the binary gets slightly smaller: -1.56 KiB stripped (-0.02%), -1.44 KiB.text, -3.99 KiB allocated, with llvm-lines flat in bothwacoreand the root lib. The real win is compile time and supply-chain surface rather than bytes, since four of the nine were proc macros andflate2was a wrapper over zlib-rs that was already linked.The per-crate attribution in the size report reads oddly (
whatsapp_rust+4.94 KiB againstother deps-5.75 KiB) and should not be read as this PR adding code to the root crate: no file undersrc/changed. That is cargo-bloat re-attributing inlined code to the caller once the crate it came from is gone, which is also whymetrics_exporter_prometheus, a dev-dependency that never linked into this binary, shows up as "removed".On the hot paths nothing gains work: the
bytemuckandarrayrefreplacements are the same machine code on little-endian, and the ltHash change removes a branch.Validation
All green: 3200+ tests across the workspace, plus doctests.
whatsapp-rust-voip-cliis excluded because itscpaltoalsa-syschain needs ALSA headers this environment does not have; it was not touched. Full matrix left to CI.Semver Checks (informational)is red, and it is red on the base branch too: the same job fails at 309e95d, main's tip when this branch was cut (run 30576243450, step "Check API against the last published release"). The workflow reports green there only because the job iscontinue-on-error. All 63 flagged items arewaproto::whatsapp::*generated protobuf types drifting from the published 0.6.0 baseline; this PR touches no.protofile and no generated code, onlywaproto/Cargo.toml's dev-dependency line. Zero findings namewacoreorwacore-binary, the other two crates in the checked set.