feat(sdk): add transport feature to dapi-grpc for types-only consumers - #4344
Conversation
📝 WalkthroughWalkthroughThe PR adds an explicit ChangesTransport Feature Cut
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Cargo
participant dapi_grpc_build_rs
participant GeneratedTonicCode
Cargo->>dapi_grpc_build_rs: Provide CARGO_FEATURE_TRANSPORT
dapi_grpc_build_rs->>GeneratedTonicCode: Generate transport for non-wasm targets
dapi_grpc_build_rs->>GeneratedTonicCode: Disable transport for wasm32
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
b184fa1 to
49e8a05
Compare
|
✅ Final review complete — no blockers (commit 759e6dc) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4344 +/- ##
=========================================
Coverage 87.80% 87.80%
=========================================
Files 2641 2641
Lines 336510 336510
=========================================
Hits 295468 295468
Misses 41042 41042
🚀 New features to boost your workflow:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The transport-free native configuration works, but making transport a target-independent default breaks the previously supported default WASM configuration of the public dapi-grpc crate. The types-only documentation also overstates the dependency reduction by claiming Tokio is absent when tonic's codegen graph still includes it.
Source: reviewer backends gpt-5.6-sol (Codex general and Codex rust-quality); final verifier backend gpt-5.6-sol (Codex); orchestration-only openclaw-agent/cliproxy/gpt-5.6-sol (not reviewer evidence).
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— rust-quality (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking | 🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/dapi-grpc/Cargo.toml`:
- [BLOCKING] packages/dapi-grpc/Cargo.toml:17: Default features now make dapi-grpc fail to compile on WASM
Cargo features are not target-scoped, so adding `transport` to the crate's defaults forwards tonic's channel, transport, TLS, and ring features to the active wasm32 tonic dependency as well as the native one. The target check in `build.rs` only prevents generation of `connect()` methods; it cannot disable dependency features already selected by Cargo. At this exact head, the wasm32 dependency graph contains `dapi-grpc -> tonic -> tokio -> mio` together with hyper, rustls, and ring, and the default wasm32 check attempts to compile those unsupported dependencies. The base revision enabled transport features only under `cfg(not(target_arch = "wasm32"))`, so default WASM consumers did not have this graph. Updating `wasm-sdk` repairs one in-repository consumer but leaves external WASM consumers using `dapi-grpc` defaults broken. Preserve WASM-safe public defaults by activating transport through a genuinely non-WASM mechanism, or remove it from the defaults and have native network consumers enable it explicitly.
- [SUGGESTION] packages/dapi-grpc/Cargo.toml:29-33: Types-only feature documentation incorrectly promises no Tokio dependency
The transport-free graph still contains `tonic -> tokio-stream -> tokio`, which is also acknowledged in the PR description as a sync-only Tokio slice. Claiming there is "no hyper/tokio in the dependency tree" is therefore inaccurate and can mislead embedders using this feature specifically to audit their dependency closure. Document that the networking and TLS transport stack is disabled while tonic's codegen dependencies, including its minimal Tokio slice, remain.
|
|
||
| [features] | ||
| default = ["core", "platform", "client"] | ||
| default = ["core", "platform", "client", "transport"] |
There was a problem hiding this comment.
🔴 Blocking: Default features now make dapi-grpc fail to compile on WASM
Cargo features are not target-scoped, so adding transport to the crate's defaults forwards tonic's channel, transport, TLS, and ring features to the active wasm32 tonic dependency as well as the native one. The target check in build.rs only prevents generation of connect() methods; it cannot disable dependency features already selected by Cargo. At this exact head, the wasm32 dependency graph contains dapi-grpc -> tonic -> tokio -> mio together with hyper, rustls, and ring, and the default wasm32 check attempts to compile those unsupported dependencies. The base revision enabled transport features only under cfg(not(target_arch = "wasm32")), so default WASM consumers did not have this graph. Updating wasm-sdk repairs one in-repository consumer but leaves external WASM consumers using dapi-grpc defaults broken. Preserve WASM-safe public defaults by activating transport through a genuinely non-WASM mechanism, or remove it from the defaults and have native network consumers enable it explicitly.
source: ['codex']
There was a problem hiding this comment.
Valid — fixed in 74991e4. You're right that cargo features aren't target-scoped and the build.rs gate only affects codegen, not dependency selection. transport is no longer a default feature: defaults are now wasm-safe (verified: cargo check -p dapi-grpc --target wasm32-unknown-unknown with defaults builds), the wasm-sdk workaround is reverted, and native networked consumers enable the feature explicitly (dash-sdk carries it in its own defaults, so SDK users are unchanged).
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Resolved in this update — Default features now make dapi-grpc fail to compile on WASM no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| # Networked tonic client: `connect()` on generated clients, TLS roots. Without | ||
| # this feature the crate provides message types and transport-generic client | ||
| # stubs only — no hyper/tokio in the dependency tree. Types-only consumers | ||
| # (proof verification, embedders with their own transport) build with | ||
| # `default-features = false, features = ["platform", "client"]`. |
There was a problem hiding this comment.
🟡 Suggestion: Types-only feature documentation incorrectly promises no Tokio dependency
The transport-free graph still contains tonic -> tokio-stream -> tokio, which is also acknowledged in the PR description as a sync-only Tokio slice. Claiming there is "no hyper/tokio in the dependency tree" is therefore inaccurate and can mislead embedders using this feature specifically to audit their dependency closure. Document that the networking and TLS transport stack is disabled while tonic's codegen dependencies, including its minimal Tokio slice, remain.
| # Networked tonic client: `connect()` on generated clients, TLS roots. Without | |
| # this feature the crate provides message types and transport-generic client | |
| # stubs only — no hyper/tokio in the dependency tree. Types-only consumers | |
| # (proof verification, embedders with their own transport) build with | |
| # `default-features = false, features = ["platform", "client"]`. | |
| # Networked tonic client: `connect()` on generated clients, TLS roots. Without | |
| # this feature the crate provides message types and transport-generic client | |
| # stubs only. Tonic's codegen graph still includes tokio-stream and a minimal | |
| # Tokio slice, but its hyper/rustls transport stack is not enabled. Types-only | |
| # consumers build with | |
| # `default-features = false, features = ["platform", "client"]`. |
source: ['codex']
There was a problem hiding this comment.
Also valid — the comment overpromised. Reworded in 74991e4 along the lines you suggested: the transport/TLS stack is disabled, while tonic's codegen graph keeps tokio-stream and a minimal Tokio slice.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Resolved in this update — Types-only feature documentation incorrectly promises no Tokio dependency no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
49e8a05 to
74991e4
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The two prior findings are fixed: dapi-grpc defaults are now transport-free, and the documentation accurately describes the remaining minimal Tokio dependency. One new blocker remains because dash-sdk's target-independent defaults directly re-enable tonic's native transport stack on WASM, even though native builds already receive that feature through rs-dapi-client's target-scoped dependency.
Source: Codex reviewers gpt-5.6-sol (general and rust-quality); final verifier gpt-5.6-sol (Codex). Orchestration-only: openclaw-agent/cliproxy/gpt-5.6-sol (not reviewer evidence).
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— rust-quality (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-sdk/Cargo.toml`:
- [BLOCKING] packages/rs-sdk/Cargo.toml:75-81: Dash SDK defaults still enable native transport on WASM
Cargo features are not target-scoped, so `dapi-grpc/transport` in dash-sdk's default feature set enables tonic's channel, transport, TLS, and ring features when dash-sdk is built for `wasm32-unknown-unknown`. The dependency graph attributes these features directly to `dapi-grpc feature "transport" -> dash-sdk feature "default"`, and `cargo check -p dash-sdk --target wasm32-unknown-unknown --locked` reaches mio's explicit `This wasm target is unsupported by mio` error while also attempting to compile ring. The build-script target check only suppresses generated `connect()` methods and cannot remove dependency features already selected by Cargo. The new CI assertion misses this configuration because wasm-sdk depends on dash-sdk with `default-features = false`. Remove the forwarding from dash-sdk's defaults; native dash-sdk builds already receive `dapi-grpc/transport` through rs-dapi-client's non-WASM dependency, while WASM builds will remain transport-free.
| "mocks", | ||
| "offline-testing", | ||
| "dapi-grpc/client", | ||
| "dapi-grpc/transport", | ||
| "token_reward_explanations", | ||
| ] |
There was a problem hiding this comment.
🔴 Blocking: Dash SDK defaults still enable native transport on WASM
Cargo features are not target-scoped, so dapi-grpc/transport in dash-sdk's default feature set enables tonic's channel, transport, TLS, and ring features when dash-sdk is built for wasm32-unknown-unknown. The dependency graph attributes these features directly to dapi-grpc feature "transport" -> dash-sdk feature "default", and cargo check -p dash-sdk --target wasm32-unknown-unknown --locked reaches mio's explicit This wasm target is unsupported by mio error while also attempting to compile ring. The build-script target check only suppresses generated connect() methods and cannot remove dependency features already selected by Cargo. The new CI assertion misses this configuration because wasm-sdk depends on dash-sdk with default-features = false. Remove the forwarding from dash-sdk's defaults; native dash-sdk builds already receive dapi-grpc/transport through rs-dapi-client's non-WASM dependency, while WASM builds will remain transport-free.
| "mocks", | |
| "offline-testing", | |
| "dapi-grpc/client", | |
| "dapi-grpc/transport", | |
| "token_reward_explanations", | |
| ] | |
| default = [ | |
| "mocks", | |
| "offline-testing", | |
| "dapi-grpc/client", | |
| "token_reward_explanations", | |
| ] |
source: ['codex']
There was a problem hiding this comment.
Resolved in this update — Dash SDK defaults still enable native transport on WASM no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
dapi-grpc unconditionally built tonic with its native transport stack
(channel + TLS roots) on non-wasm targets, so any consumer of the message
types or proof-verification layers (drive-proof-verifier) dragged
hyper/rustls and the tokio networking stack into its dependency tree even
when it never opens a connection. The wasm target already proves the crate
works with codegen-only tonic.
Add an opt-in 'transport' cargo feature carrying tonic's
channel/transport/tls features, mirroring the client/server feature split
tenderdash-proto already has. It is deliberately NOT a default feature:
cargo features are not target-scoped, so a default-on transport would force
tonic's transport stack onto wasm32 consumers riding defaults, where it
does not build. build.rs drives tonic-build's build_transport from
CARGO_FEATURE_TRANSPORT (never on wasm32). Native networked consumers
enable it explicitly: rs-dapi-client (target-scoped to non-wasm), dash-sdk
(default feature, so SDK users are unchanged), and drive-abci via server
(which now implies transport). wasm-sdk and other wasm consumers need no
changes.
drive-proof-verifier needs no changes and its standalone tree drops from
407 to 339 crates: hyper, h2, rustls, ring, tower and the rest of the
transport stack disappear; what remains of tonic's codegen core is a
sync-only tokio slice via tokio-stream.
Types-only consumption is simply the default; embedders with their own
transport depend on:
dapi-grpc = { default-features = false, features = ["platform", "client"] }
(default-features = false remains advisable for wasm and keeps the feature
set explicit.)
74991e4 to
759e6dc
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The transport split is correct: dapi-grpc defaults are transport-free, native rs-dapi-client builds enable transport through a target-specific dependency, and the prior dash-sdk WASM blocker is fixed. The existing JS build already compiles the new dapi-grpc WASM code-generation path, but the newly added dependency leak guards can silently pass when cargo tree fails, so one CI suggestion remains.
Source: reviewers gpt-5.6-sol (Codex general and rust-quality); final verifier gpt-5.6-sol (Codex). Orchestration-only: openclaw-agent/cliproxy/gpt-5.6-sol (not reviewer evidence).
Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— rust-quality (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `.github/workflows/tests-rs-workspace.yml`:
- [SUGGESTION] .github/workflows/tests-rs-workspace.yml:205-218: Make dependency leak guards fail when cargo tree fails
Each inverse cargo tree query suppresses stderr and treats an empty grep result as proof that the dependency is absent. That also treats cargo tree errors as success. For example, an inverse query becomes ambiguous when multiple versions of a package such as rustls are present, so a future transport leak can pass this guard precisely when the graph contains multiple versions. Generate the complete dependency tree as a separate command, allowing Cargo failures to stop the step, and then search its package lines.
| for banned in hyper rustls tower; do | ||
| if cargo tree -p drive-proof-verifier -e normal -i "$banned" 2>/dev/null | grep -q .; then | ||
| echo "::error::$banned leaked into drive-proof-verifier's dependency tree" | ||
| exit 1 | ||
| fi | ||
| done | ||
| for banned in hyper rustls tower mio; do | ||
| for wasm_package in dash-sdk wasm-sdk; do | ||
| if cargo tree -p "$wasm_package" --target wasm32-unknown-unknown -e normal -i "$banned" 2>/dev/null | grep -q .; then | ||
| echo "::error::$banned leaked into $wasm_package's wasm32 dependency tree" | ||
| exit 1 | ||
| fi | ||
| done | ||
| done |
There was a problem hiding this comment.
🟡 Suggestion: Make dependency leak guards fail when cargo tree fails
Each inverse cargo tree query suppresses stderr and treats an empty grep result as proof that the dependency is absent. That also treats cargo tree errors as success. For example, an inverse query becomes ambiguous when multiple versions of a package such as rustls are present, so a future transport leak can pass this guard precisely when the graph contains multiple versions. Generate the complete dependency tree as a separate command, allowing Cargo failures to stop the step, and then search its package lines.
| for banned in hyper rustls tower; do | |
| if cargo tree -p drive-proof-verifier -e normal -i "$banned" 2>/dev/null | grep -q .; then | |
| echo "::error::$banned leaked into drive-proof-verifier's dependency tree" | |
| exit 1 | |
| fi | |
| done | |
| for banned in hyper rustls tower mio; do | |
| for wasm_package in dash-sdk wasm-sdk; do | |
| if cargo tree -p "$wasm_package" --target wasm32-unknown-unknown -e normal -i "$banned" 2>/dev/null | grep -q .; then | |
| echo "::error::$banned leaked into $wasm_package's wasm32 dependency tree" | |
| exit 1 | |
| fi | |
| done | |
| done | |
| verifier_tree="$(mktemp)" | |
| cargo tree --locked -p drive-proof-verifier -e normal --prefix none > "$verifier_tree" | |
| for banned in hyper rustls tower; do | |
| if grep -Eq "^${banned} v[0-9]" "$verifier_tree"; then | |
| echo "::error::$banned leaked into drive-proof-verifier's dependency tree" | |
| exit 1 | |
| fi | |
| done | |
| rm "$verifier_tree" | |
| for wasm_package in dash-sdk wasm-sdk; do | |
| wasm_tree="$(mktemp)" | |
| cargo tree --locked -p "$wasm_package" --target wasm32-unknown-unknown -e normal --prefix none > "$wasm_tree" | |
| for banned in hyper rustls tower mio; do | |
| if grep -Eq "^${banned} v[0-9]" "$wasm_tree"; then | |
| echo "::error::$banned leaked into $wasm_package's wasm32 dependency tree" | |
| exit 1 | |
| fi | |
| done | |
| rm "$wasm_tree" | |
| done |
source: ['codex']
3 participants
Issue being fixed or feature implemented
dapi-grpcunconditionally built tonic with its native channel and TLS transport stack, so consumers that only need generated message types or proof verification also pulled hyper, rustls, ring, and tower into their dependency trees.drive-proof-verifieris a concrete synchronous consumer that does not open network connections.This is the transport-feature prerequisite extracted from #4335. It lets embedders such as Dash Core use Platform proof verification without inheriting a networking implementation they do not use.
What was done?
transportfeature todapi-grpcfor tonic channel, transport, and TLS support.dapi-grpcdefaults transport-free because Cargo features are additive and cannot be target-scoped.build.rsemit tonic transport helpers only whentransportis enabled and never forwasm32.rs-dapi-client's non-wasm target dependency.serverfeature implytransport, preserving the DAPI server build.drive-proof-verifier's native dependency tree;dash-sdkandwasm-sdkwasm32 dependency trees.dapi-grpcto the nightly per-feature package matrix.Types-only consumers can use:
How Has This Been Tested?
Validated after rebasing onto current
v4.2-dev:cargo fmt --check --alldapi-grpcchecksdrive-proof-verifier,rs-dapi-client, and defaultdash-sdkchecksdash-sdkincludes transport while wasm32dash-sdkdoes notThe corrected full #4335 series is rebased and validated separately on top of this prerequisite.
Breaking Changes
SDK users and in-repository consumers retain native transport through the appropriate native client or server dependency.
External native consumers that depend directly on
dapi-grpcand call generatedconnect()methods must explicitly enablefeatures = ["transport"]. wasm32 consumers can now use the default feature set without pulling an incompatible native transport graph.Checklist:
Summary by CodeRabbit
New Features
Tests