feat(sdk): add transport feature to dapi-grpc for types-only consumers - #4344
feat(sdk): add transport feature to dapi-grpc for types-only consumers#4344PastaPastaPasta wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThe PR adds an explicit ChangesTransport Feature Cut
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
b184fa1 to
49e8a05
Compare
|
⛔ Blockers found — Opus deferred (commit 74991e4) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4344 +/- ##
=========================================
Coverage 87.62% 87.62%
=========================================
Files 2704 2704
Lines 345206 345206
=========================================
Hits 302473 302473
Misses 42733 42733
🚀 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.
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.)
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']
2 participants
Issue being fixed or feature implemented
dapi-grpcunconditionally builds tonic with its native transport stack (channel + TLS roots) on non-wasm targets, so any consumer of just the message types or the proof-verification layer drags hyper/tokio/rustls/ring/tower into its dependency tree even when it never opens a connection.drive-proof-verifieris the concrete case: a fully synchronous crate that pays for a networking stack it cannot use. For embedders that vendor and audit every dependency (Dash Core's platform GUI — PastaPastaPasta/dash#67 — plus explorers and other own-transport clients), that's ~120 crates of pure supply-chain overhead.The wasm target already proves the crate works with codegen-only tonic; this makes the same cut expressible on native.
First slice of #4335, split out per its "happy to split" note — this is the prerequisite commit the rest of that series builds on.
What was done?
transportcargo feature ondapi-grpccarrying tonic'schannel/transport/tls-*features (the same client/server-style feature splittenderdash-protoalready has). The base tonic dependency drops tocodegen. It is deliberately not a default: cargo features are not target-scoped, so a default-on transport would force tonic's transport stack onto wasm32 consumers riding defaults (thepastaclaw's blocker — fixed).build.rsdrives tonic-build'sbuild_transportfromCARGO_FEATURE_TRANSPORT, never on wasm32.rs-dapi-client(target-scoped to non-wasm; its wasm side usestonic-web-wasm-client),dash-sdk(via itsdefaultfeature),drive-abciviaserver(which now impliestransport).wasm-sdkand other wasm consumers need no changes — defaults are wasm-safe.drive-proof-verifier's native tree orwasm-sdk's wasm32 tree;drive-proof-verifierjoins the nightly per-feature check matrix (the matrix enumerates features fromCargo.toml, so the newtransportfeature is covered there automatically).drive-proof-verifieritself needs zero changes — its standalone tree just 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 viatokio-stream(so the tree is transport-free, not tokio-free — documented as such).Types-only consumption:
How Has This Been Tested?
cargo check -p dapi-grpcwith defaults (native), with--no-default-features --features core,platform,client(native types-only), and forwasm32-unknown-unknown— all clean on top of currentv4.2-dev.cargo check -p drive-proof-verifier --lockedandcargo check -p rs-dapi-client;cargo treeconfirms no hyper/rustls/tower in the verifier's tree.cargo fmt --check --allclean;Cargo.lockuntouched (features don't affect resolution).Breaking Changes
None for SDK users (
dash-sdkenablesdapi-grpc/transportin its defaults) or any in-repo consumer. External native consumers depending ondapi-grpcdirectly with default features and calling generatedconnect()methods must addfeatures = ["transport"]— the failure mode is a clear missing-method error naming the gap, versus the previous state where external wasm consumers got an unbuildable tokio/mio graph with no indication why. wasm consumers are strictly better off: defaults now build on wasm32.Checklist:
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests