Skip to content

feat(xmtp_api_grpc): bidi_stream transport primitive#3770

Merged
tylerhawkes merged 1 commit into
mainfrom
tyler/xip83-2-bidi-transport
Jun 24, 2026
Merged

feat(xmtp_api_grpc): bidi_stream transport primitive#3770
tylerhawkes merged 1 commit into
mainfrom
tyler/xip83-2-bidi-transport

Conversation

@tylerhawkes

@tylerhawkes tylerhawkes commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

Stack 2/4 of the XIP-83 bidi client lane: #3769#3770#3771#3772.

Adds one primitive to the protocol-agnostic Client trait: bidi_stream(request, path, BoxDynStream<Bytes>) -> http::Response<BytesStream> — outbound stream of encoded protobuf messages in, inbound message stream out. Default implementation errors ("not supported by this transport"), so gRPC-Web/wasm and mocks are untouched; forwarded through the &T/Box/Arc/boxed-client impls. GrpcClient overrides it natively via tonic Grpc::streaming + the existing TransparentCodec, reusing the NonBlocking stream machinery verbatim. build_tonic_request made generic over the body type.

🤖 Generated with Claude Code

Note

Add bidi_stream bidirectional streaming transport primitive to GrpcClient

  • Adds bidi_stream to the Client trait in traits.rs with a default implementation that returns an unretryable error, so existing transports don't break.
  • Implements bidi_stream on GrpcClient (non-wasm only) in client.rs, sending an outbound BoxDynStream<Bytes> and returning an inbound BytesStream over HTTP/2.
  • Makes build_tonic_request generic over the body type to support both unary/server-streaming (Bytes) and bidirectional (stream of frames) use cases.
  • Forwards bidi_stream through blanket impls for &T, &mut T, Arc<T>, and BoxedClient<C>.

Macroscope summarized 74f05a8.

@claude

claude Bot commented Jun 12, 2026

Copy link
Copy Markdown

Claude finished @tylerhawkes's task —— View job


Code Review - June 24, 2026 20:31 UTC

  • Read changed files
  • Analyze code quality and best practices
  • Check for potential bugs or issues
  • Review performance and security considerations
  • Assess test coverage

Findings

Code Quality & Best Practices

  • Clean implementation following existing patterns (mirrors stream method structure)
  • Proper delegation through wrapper types (&T, Box, Arc, BoxedClient)
  • Good use of #[cfg(not(target_arch = "wasm32"))] to limit bidi to native platforms
  • Generic build_tonic_request<B> is elegant and reuses existing code

Potential Issues ⚠️

  1. Missing #[cfg] guard in blanket impls (crates/xmtp_proto/src/traits.rs:199-206, 232-239, 265-272)

    • The blanket Client impls for &T, Box<T>, and Arc<T> unconditionally forward bidi_stream
    • On wasm, this will forward to the trait's default impl (which errors), but it's inconsistent with GrpcClient having an explicit #[cfg(not(target_arch = "wasm32"))]
    • Consider: Either add #[cfg(not(target_arch = "wasm32"))] to these forwarding impls OR document why unconditional forwarding is intentional
  2. Code duplication (crates/xmtp_api_grpc/src/grpc_client/client.rs:196-224)

    • bidi_stream is nearly identical to stream (lines 169-194), differing only in:
      • Body type: BoxDynStream<'static, Bytes> vs Bytes
      • Codec call: client.streaming vs client.server_streaming
    • Consider: Extract shared logic into a helper method to reduce duplication and maintenance burden

Performance & Security

  • Reuses proven NonBlockingStreamRequest + TransparentCodec machinery
  • No obvious security concerns
  • Cloning self and client.inner is consistent with existing stream implementation

Test Coverage ⚠️

  • Codecov reports 14.28% patch coverage (24/28 new lines uncovered)
  • Missing coverage for:
    • bidi_stream happy path execution (client.rs:199-224)
    • Default error path in trait (traits.rs:150-160)
    • Forwarding impls (traits.rs, boxed_client.rs)
  • Stack PR 3/4 or 4/4 likely adds integration tests, but unit-level coverage gap remains

Minor Nit

  • Line 215 (client.rs): Type ascription as crate::streams::ResponseFuture is duplicated from stream — if extracted to a helper, this would be deduplicated

Summary: Solid implementation with no blocking issues. Main concerns are code duplication and test coverage, both addressable in follow-ups if needed for this stack.


@codecov

codecov Bot commented Jun 12, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 14.28571% with 24 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.43%. Comparing base (1e1a195) to head (74f05a8).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
crates/xmtp_api_grpc/src/grpc_client/client.rs 22.22% 14 Missing ⚠️
crates/xmtp_proto/src/traits.rs 0.00% 8 Missing ⚠️
crates/xmtp_proto/src/traits/boxed_client.rs 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3770      +/-   ##
==========================================
- Coverage   84.44%   84.43%   -0.02%     
==========================================
  Files         408      408              
  Lines       59880    59904      +24     
==========================================
+ Hits        50567    50578      +11     
- Misses       9313     9326      +13     

☔ View full report in Codecov by Harness.
📢 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.

@tylerhawkes tylerhawkes force-pushed the tyler/xip83-2-bidi-transport branch from 5329964 to ba3c4de Compare June 12, 2026 21:33
@tylerhawkes tylerhawkes force-pushed the tyler/xip83-1-subscribe-protos branch from 2a66968 to 5030100 Compare June 12, 2026 21:33
@tylerhawkes tylerhawkes force-pushed the tyler/xip83-2-bidi-transport branch from ba3c4de to cf43cdd Compare June 12, 2026 21:59
@tylerhawkes tylerhawkes force-pushed the tyler/xip83-1-subscribe-protos branch from 5030100 to 9eb4ed4 Compare June 12, 2026 21:59
@tylerhawkes tylerhawkes force-pushed the tyler/xip83-2-bidi-transport branch from cf43cdd to 7ef18f0 Compare June 15, 2026 20:20
@tylerhawkes tylerhawkes force-pushed the tyler/xip83-1-subscribe-protos branch from 9eb4ed4 to 21b315c Compare June 15, 2026 20:20
tylerhawkes added a commit that referenced this pull request Jun 16, 2026
…#3769)

**Stack 1/4** of the XIP-83 bidi client lane: #3769#3770#3771#3772.

Regenerated `xmtp.mls.api.v1` from xmtp/proto#337: the bidirectional
`Subscribe` RPC with versioned `SubscribeRequest`/`SubscribeResponse`,
id-based `Mutate` (cursors, `history_only`), `Ping`/`Pong`,
`TopicsLive`, `CATCHUP_COMPLETE`, and STARTED `capabilities`. Purely
additive (+1,896 generated lines); `proto_version` pinned to that
branch's sha — draft until the proto PR merges.
Base automatically changed from tyler/xip83-1-subscribe-protos to main June 16, 2026 20:04
@tylerhawkes tylerhawkes force-pushed the tyler/xip83-2-bidi-transport branch from 7ef18f0 to af6ab11 Compare June 16, 2026 21:04
…efault-unsupported elsewhere)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@tylerhawkes tylerhawkes force-pushed the tyler/xip83-2-bidi-transport branch from af6ab11 to 74f05a8 Compare June 24, 2026 20:29
@tylerhawkes tylerhawkes marked this pull request as ready for review June 24, 2026 21:37
@tylerhawkes tylerhawkes requested a review from a team as a code owner June 24, 2026 21:37
@tylerhawkes tylerhawkes merged commit 5a8a134 into main Jun 24, 2026
43 of 44 checks passed
@tylerhawkes tylerhawkes deleted the tyler/xip83-2-bidi-transport branch June 24, 2026 21:38
@macroscopeapp

macroscopeapp Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Needs human review

This PR introduces a new bidirectional streaming transport primitive (XIP-83), adding new capability to the Client trait. While well-scoped and authored by the code owner, new transport infrastructure warrants human review to validate the design and integration approach.

You can customize Macroscope's approvability policy. Learn more.

tylerhawkes added a commit that referenced this pull request Jun 25, 2026
#3771)

**Stack 3/4** of the XIP-83 bidi client lane: #3769#3770#3771#3772.

Native-only `XmtpMlsBidiStreams` trait in `xmtp_proto::api_client` —
`subscribe_bidi(BoxStream<SubscribeRequest>) ->
Stream<Result<SubscribeResponse>>` — and its `V3Client` implementation:
prost-encode outbound frames over
`bidi_stream("/xmtp.mls.api.v1.MlsApi/Subscribe")`, decode inbound via
the existing `XmtpStream`. Browsers stay on `XmtpMlsStreams` + watchdog
(gRPC-Web cannot full-duplex).

🤖 Generated with [Claude Code](https://claude.com/claude-code)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants