Optimize envelope visitors by Unpacked versions of envelopes

[insipx]

this deserializes envelopes once, allowing visitors to return borrowed data from the network and should reduce cloning, esp in sort methods.

an experiment which uses hands-off claude to tackle a small tech debt issue

Note

Replace packed envelope types with Unpacked variants across envelope visitors and endpoints

• Introduces new Unpacked* types (unpacked_envelopes.rs) that mirror the protobuf OriginatorEnvelope, UnsignedOriginatorEnvelope, and PayerEnvelope structs but decode nested messages inline rather than storing raw bytes.
• Updates the EnvelopeVisitor trait and all extractor implementations to accept UnpackedOriginatorEnvelope, UnpackedUnsignedOriginatorEnvelope, and UnpackedPayerEnvelope instead of packed types, eliminating per-visit prost::Message::decode calls.
• Changes endpoint Output types for QueryEnvelope, QueryEnvelopes, and SubscribeEnvelopes to return UnpackedQueryEnvelopesResponse / UnpackedSubscribeEnvelopesResponse so callers receive already-decoded envelopes.
• Propagates unpacked types through query methods, stream type aliases, and the d14n_compat subscription decoder in xmtp_mls.
• Behavioral Change: wire-format compatibility between packed and unpacked types is achieved by re-encoding then decoding; existing corrupted-bytes error paths in SubscribeEnvelopesResponse tests are removed because nested messages are now held as structs.

^{Macroscope summarized 11e9168.}

[claude]

Claude finished @insipx's task —— View job

---

Code Review — March 17, 2026, 20:57 UTC

This is a well-designed performance optimization that eliminates redundant deserialization. The wire-format compatibility approach is clever and the changes are thorough. I found a few potential issues worth addressing:

---

🐛 Potential Bug: Silent Conversion Failures

Location: crates/xmtp_api_d14n/src/protocol/impls/protocol_envelopes.rs:305-310

The TryFrom conversion is called but errors are silently converted to None:

let unpacked = self
    .originator_envelope
    .as_ref()
    .map(UnpackedOriginatorEnvelope::try_from)
    .transpose()
    .map_err(ConversionError::from)?;
unpacked.as_ref().accept(visitor)?;

If TryFrom fails due to malformed bytes, it becomes a ConversionError, which may be appropriate. However, the None case still calls .accept() on Option<&UnpackedOriginatorEnvelope>, which will trigger visit_none() instead of propagating a decode error. Consider whether this is the intended behavior or if missing envelopes should be errors.

---

⚠️ Performance Consideration: Double Encoding/Decoding

Location: crates/xmtp_proto/src/types/unpacked_envelopes.rs:91-93

The TryFrom implementation re-encodes then decodes:

fn try_from(packed: &OriginatorEnvelope) -> Result<Self, Self::Error> {
    Self::decode(packed.encode_to_vec().as_slice())
}

This is necessary for wire-format compatibility, but it adds overhead when converting from packed to unpacked. Since the PR's goal is to deserialize once at the network boundary, ensure this conversion only happens where needed (e.g., compatibility layers, not hot paths). From reviewing the usage, it appears this is only used in get_newest_envelope_response::Response which is likely not a hot path. ✅

---

🔍 Missing Error Context

Location: crates/xmtp_proto/src/types/unpacked_envelopes.rs:84-94

The TryFrom implementation returns a bare prost::DecodeError. Consider wrapping this in a more descriptive error that indicates it's a packed→unpacked conversion failure, which would help debugging:

type Error = ConversionError;  // Instead of prost::DecodeError

fn try_from(packed: &OriginatorEnvelope) -> Result<Self, Self::Error> {
    Self::decode(packed.encode_to_vec().as_slice())
        .map_err(|e| ConversionError::Decode(
            "failed to convert packed to unpacked envelope".into(), 
            e
        ))
}

This would provide better error context in logs when debugging envelope issues.

---

✅ Strengths

1. Wire-format compatibility is elegant — using the same tag numbers with different types (bytes vs message) is a clever use of protobuf's wire format.

2. Comprehensive test coverage — The roundtrip tests in unpacked_envelopes.rs:157-171 and visitor tests in protocol_envelopes.rs:658-896 are thorough.

3. Zero behavioral changes — The PR maintains the same public API and behavior while improving internal efficiency.

4. Consistent updates — All 11+ extractor implementations were updated uniformly to use unpacked types.

5. Proper lifetimes — The EnvelopeVisitor<'env> trait correctly uses lifetimes to allow borrowing from unpacked envelopes, enabling zero-copy extraction in many cases.

---

📋 Minor Observations

1. Test data limitations — Most tests use minimal mock data (e.g., vec![1,2,3]) which won't catch issues with real MLS protocol messages. Consider adding at least one test with actual serialized MLS data.

2. Documentation — The module docs in unpacked_envelopes.rs:1-15 are excellent and explain the wire-format compatibility clearly. Consider adding a similar explanation to the PR description for future reference.

---

🔒 Security Assessment

No security concerns identified. The unpacked types use the same protobuf decoder (prost::Message::decode) as the packed versions, so there's no new attack surface. The change is purely structural optimization.

---

📊 Test Coverage Assessment

✅ Wire-format compatibility tested (lines 157-186)
✅ Visitor pattern tested across all payload types (lines 742-786)
✅ Edge cases tested (empty payloads, None options) (lines 817-843)
✅ Integration tests in subscriptions (mod.rs:702-767)
⚠️ Missing: Tests with intentionally malformed nested envelopes to verify error handling

---

Summary

This is a solid performance optimization with no major issues. The main suggestions are improving error context and verifying the behavior when envelope conversion fails. The code quality is high and the approach is sound.

---

[macroscopeapp]

Approvability

Verdict: Approved

This is an internal optimization that introduces 'Unpacked' envelope types to avoid repeated protobuf decode calls when traversing nested envelopes. The wire format remains compatible, all changes are within the author's ownership domain, and the PR includes comprehensive tests demonstrating wire-format compatibility.

^{You can customize Macroscope's approvability policy. Learn more.}

[codecov]

Codecov Report

❌ Patch coverage is 92.94872% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.51%. Comparing base (95bf2a8) to head (14bda83).
⚠️ Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
...tp_api_d14n/src/protocol/impls/envelope_visitor.rs	50.00%	6 Missing ⚠️
crates/xmtp_api_d14n/src/protocol/macros.rs	0.00%	3 Missing ⚠️
...tes/xmtp_api_d14n/src/protocol/extractors/bytes.rs	0.00%	1 Missing ⚠️
..._api_d14n/src/protocol/impls/protocol_envelopes.rs	94.44%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3340      +/-   ##
==========================================
- Coverage   82.51%   82.51%   -0.01%     
==========================================
  Files         376      377       +1     
  Lines       51267    51292      +25     
==========================================
+ Hits        42304    42322      +18     
- Misses       8963     8970       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.