feat(proto)!: separate client and server feature #118
Conversation
WalkthroughThe pull request introduces a comprehensive reorganization of gRPC-related features and dependencies across multiple packages in the Tenderdash project. The changes focus on separating gRPC server and client functionalities, updating feature flags, and modifying module compilation conditions. Key modifications include renaming gRPC-related enum variants, updating Cargo.toml configurations, and adjusting conditional compilation directives to provide more granular control over gRPC server and client components. Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
⏰ Context from checks skipped due to timeout of 90000ms (4)
🔇 Additional comments (5)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Can you tell us more about this PR? What is the overall purpose, etc? |
First of all, this is internal library used by dash platform only. For WASM, we only build client bindings, as we don't need server code (which doesn't work anyway). We also exclude some transport logic that is handled by separate library. This PR just cleans up some Cargo features and logic around code generation, to make it easier to use only what's needed in WASM. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
abci/Cargo.toml (1)
38-39: Consider adding migration instructions for the deprecated feature.While the deprecation of
stdin favor ofgrpcis clear, consider adding a comment with migration instructions or linking to documentation.-# std is deprecated, use "grpc" instead +# std is deprecated, use "grpc" instead. +# Migration guide: https://github.com/dashpay/tenderdash/wiki/migrating-from-std-to-grpcproto/Cargo.toml (2)
39-46: Enhance feature documentation and deprecation noticesThe feature documentation could be improved:
- Add version numbers to deprecation notices
- Provide migration examples
- Make comment formatting consistent (periods)
Apply this diff to improve the documentation:
-# Enable standard library support; DEPRECATED - use `client` and/or `server` instead +# Enable standard library support. +# DEPRECATED since 0.x.x: Use `client` and/or `server` features instead. +# Example: tenderdash-proto = { version = "0.x", features = ["client"] } std = ["client"] -# Enable gRPC support using tonic; internal, consider using `server` or `client` instead +# Enable gRPC support using tonic. +# INTERNAL: Use `server` or `client` features instead. grpc = ["client", "dep:tonic", "tonic/codegen", "tonic/prost"] -# Build gRPC server using tonic. Includes `client` feature. +# Build gRPC server using tonic. Includes `client` feature server = ["client", "tonic/transport"] -# Build minimal gRPC client using tonic, without transport +# Build minimal gRPC client using tonic, without transport. client = ["tenderdash-proto-compiler/grpc", "prost/std", "grpc"]
37-46: Highlight the build.rs warning in the documentationThe warning about build.rs and feature changes is crucial. Consider making it more prominent to prevent issues:
Add this warning to:
- The README.md file
- The crate documentation
- The changelog for this breaking change
This will help users understand potential pitfalls when switching between features or upgrading versions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
abci/Cargo.toml(1 hunks)abci/src/lib.rs(1 hunks)abci/src/server/generic.rs(1 hunks)proto-compiler/Cargo.toml(0 hunks)proto-compiler/src/constants.rs(2 hunks)proto-compiler/src/lib.rs(1 hunks)proto/Cargo.toml(2 hunks)proto/build.rs(1 hunks)proto/src/.gitignore(1 hunks)proto/src/lib.rs(2 hunks)proto/src/time.rs(1 hunks)
💤 Files with no reviewable changes (1)
- proto-compiler/Cargo.toml
✅ Files skipped from review due to trivial changes (1)
- proto/src/.gitignore
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test-all
🔇 Additional comments (17)
proto/build.rs (2)
16-19: LGTM! Clear documentation of build behavior.The added comment effectively explains that building both server and client components is safe, even if they're not used in lib.rs.
20-25: Verify feature flag dependencies.The conditional compilation directives correctly separate server and client builds. However, we should verify that all necessary dependencies are properly configured in Cargo.toml.
✅ Verification successful
Feature flag dependencies are correctly configured ✅
The conditional compilation in
build.rsis properly supported by the feature flag hierarchy inCargo.toml:
- Server feature includes client capabilities and transport
- Client feature provides minimal gRPC client support
- Tonic dependency is properly gated with appropriate features for each use case
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify feature flag configurations in Cargo.toml files # Check for proper feature definitions and dependencies # Search for feature definitions echo "Checking feature definitions in Cargo.toml files:" rg -A 5 '^\[features\]' # Search for feature usage in dependencies echo "Checking feature usage in dependencies:" rg -A 5 'features\s*=\s*\['Length of output: 4592
Script:
#!/bin/bash # Get the complete features section from proto/Cargo.toml echo "Features section from proto/Cargo.toml:" rg -A 20 '^\[features\]' proto/Cargo.toml # Check if tonic is properly gated by features echo -e "\nTonic dependency configuration:" rg 'tonic.*=' proto/Cargo.tomlLength of output: 1283
abci/src/lib.rs (2)
42-44: LGTM! Improved error handling structure.The removal of
#[from]attributes in favor of explicit type specifications enhances clarity and control over error conversions.
53-65: Well-structured manual implementations for no_std compatibility.The manual implementations of
Fromtraits forEncodeErrorandDecodeErrorare correctly implemented and documented. This change improves compatibility with no_std environments while maintaining clean error conversion paths.proto/src/time.rs (2)
9-11: LGTM! Improved documentation clarity.The added note about resolution differences between protobuf timestamp and milliseconds is helpful for developers.
3-5: Verify format module usage across features.The conditional compilation ensures the format module is only included when necessary. However, we should verify that this doesn't break any existing functionality.
✅ Verification successful
Conditional compilation of format module is correctly implemented
The format module is properly isolated and only imported in the nostd context. No conflicts or breaking changes were found in server/client feature paths. The timestamp formatting functionality remains intact through specialized handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify format module usage # Check if format module is used in server/client feature contexts # Search for format module usage echo "Checking format module usage:" rg -p "use.*format" # Search for potential timestamp formatting calls echo "Checking timestamp formatting calls:" rg -p "format!.*time"Length of output: 593
Script:
#!/bin/bash # Check format module contents and direct references echo "Checking format module contents:" rg -p "mod format|pub mod format" echo "Checking direct format module references:" rg -p "format::" --type rust # Check feature-gated code blocks echo "Checking feature-gated blocks:" ast-grep --pattern 'cfg(feature = $_)'Length of output: 344
abci/src/server/generic.rs (1)
Line range hint
99-106: LGTM! Simplified type constraints.The removal of the unnecessary lifetime parameter
'afrom theServerimplementation forGenericServeris a good simplification. The implementation maintains all necessary trait bounds (Send + Sync + 'static) while reducing complexity.proto-compiler/src/lib.rs (2)
127-138: LGTM! Clean separation of server configuration.The GrpcServer configuration properly enables all required components (client, server, transport) while maintaining feature gates.
139-149: LGTM! Well-optimized client configuration for WASM.The GrpcClient configuration correctly disables server and transport components while keeping client functionality, which aligns with WASM target requirements.
proto/src/lib.rs (3)
6-6: LGTM! Documentation style fix.Allowing clippy::doc_lazy_continuation improves documentation readability without affecting functionality.
37-39: LGTM! Clean module organization.The conditional compilation logic properly separates the modules based on features, with clear boundaries between server, client, and nostd implementations.
Also applies to: 42-49
51-58: LGTM! Well-structured re-exports.The re-export logic maintains a clear precedence (server > client > nostd) while ensuring only one implementation is exposed.
proto-compiler/src/constants.rs (2)
16-19: LGTM! Clear enum variants.The GenerationMode enum variants clearly represent their purposes with good documentation.
35-42: LGTM! Descriptive Display implementation.The Display trait implementation provides clear and consistent string representations of the generation modes.
abci/Cargo.toml (1)
35-35: LGTM! Proper feature dependency.Adding grpc to server feature ensures all required functionality is available.
proto/Cargo.toml (2)
55-55: LGTM: Optimized tonic dependency configurationGood optimization! Disabling default features reduces the dependency footprint and aligns with making the crate more WASM-friendly.
37-37: Breaking change: Default feature changed to "server"This change modifies the default behavior from including just gRPC support to including both client and server features. While this aligns with the PR objectives, it could impact existing users.
Let's verify the impact on existing dependencies:
Consider:
- Adding a migration guide in the README
- Documenting the new feature flags in the crate documentation
✅ Verification successful
Breaking change impact is minimal, documentation updates recommended
The change to default features appears safe as:
- No dependencies found relying on default features
- The
abcipackage explicitly opts out of defaults withdefault-features = falseStill recommended:
- Document the feature flag changes in CHANGELOG.md
- Update crate documentation to explain the new feature structure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for dependencies on tenderdash-proto without explicit feature selection rg --type toml -A 5 'tenderdash-proto = .*(?!features)'Length of output: 315
Script:
#!/bin/bash # Find all Cargo.toml files that depend on tenderdash-proto rg --type toml -l "tenderdash-proto" # For each found file, show the dependency specification and surrounding context rg --type toml -A 5 -B 2 'tenderdash-proto = .*'Length of output: 651
Script:
#!/bin/bash # Extract the tenderdash-proto dependency configuration from proto-compiler rg --type toml -A 5 -B 2 'tenderdash-proto = .*' proto-compiler/Cargo.tomlLength of output: 76
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
proto/src/error.rs (1)
3-6: LGTM! Consider updating module documentationThe change from
grpctostdfeature for error-related imports is semantically correct. Consider updating the module documentation to mention the feature flag requirements.Add a note about feature requirements:
//! This module defines the various errors that be raised during Protobuf //! conversions. +//! +//! When the `std` feature is enabled, this module uses types from the standard +//! library. Otherwise, it uses types from the core library.proto/Cargo.toml (1)
62-62: Consider pinning tonic version more strictly.Since this is a breaking change that relies heavily on tonic's feature flags, consider using a more strict version requirement (e.g.,
=0.12.3) to prevent potential compatibility issues with minor version updates.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
abci/Cargo.toml(1 hunks)proto/Cargo.toml(2 hunks)proto/src/error.rs(1 hunks)proto/src/lib.rs(2 hunks)proto/src/prelude.rs(1 hunks)proto/src/protobuf.rs(1 hunks)proto/src/serializers/from_str.rs(1 hunks)proto/src/serializers/part_set_header_total.rs(1 hunks)proto/src/serializers/time_duration.rs(1 hunks)proto/src/serializers/timestamp.rs(1 hunks)proto/tests/unit.rs(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- proto/tests/unit.rs
- proto/src/serializers/timestamp.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- abci/Cargo.toml
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test-all
- GitHub Check: build (alpine)
🔇 Additional comments (8)
proto/src/prelude.rs (1)
14-14: LGTM! Verify tonic dependency configurationThe change from
grpctostdfeature for tonic re-export is semantically correct since tonic requires the standard library. This aligns with the PR objective to deprecate thegrpcfeature.Let's verify the tonic dependency configuration:
✅ Verification successful
Verified: Tonic dependency and feature flags are properly configured
The change to
#[cfg(feature = "std")]is correct and well-structured:
- The
stdfeature enables tonic through theclientfeature- This aligns with deprecating the
grpcfeature- The dependency hierarchy ensures proper feature resolution
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if tonic is properly configured in Cargo.toml rg -l 'tonic.*=.*' | while read -r file; do echo "=== $file ===" rg 'tonic.*=.*' "$file" doneLength of output: 408
Script:
#!/bin/bash # Check feature definitions in Cargo.toml files for file in proto/Cargo.toml proto-compiler/Cargo.toml; do echo "=== $file ===" rg '^\[features\]' -A 10 "$file" doneLength of output: 908
Script:
#!/bin/bash # Get complete feature definitions and tonic-related dependencies from proto/Cargo.toml echo "=== Complete proto/Cargo.toml ===" cat proto/Cargo.tomlLength of output: 2781
proto/src/serializers/time_duration.rs (1)
2-5: LGTM! Appropriate feature flag for Duration typeThe change from
grpctostdfeature for Duration import is semantically correct, as it properly reflects the choice between std and core libraries.proto/src/serializers/from_str.rs (1)
4-7: LGTM! Appropriate feature flag for trait importsThe change from
grpctostdfeature for FromStr and Display trait imports is semantically correct, as it properly reflects the choice between std and core libraries.proto/Cargo.toml (1)
37-52: LGTM! Clear separation of features for better granularity.The new feature structure effectively separates client and server capabilities, which is crucial for WASM support. The deprecation notices and feature dependencies are well-organized.
Let's verify the impact of these feature changes on dependent crates:
✅ Verification successful
Feature changes are safe to proceed
The verification shows that:
- Dependencies are already using the new feature flags (
server,std)- No external usage of the deprecated
grpcfeature was found- The only
grpcfeature reference is in the internal proto-compiler crate🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for direct dependencies on the deprecated 'grpc' feature rg --type rust -A 3 'tenderdash-proto.*features.*=.*["'']grpc'Length of output: 62
Script:
#!/bin/bash # Find all Cargo.toml files and check for tenderdash-proto dependencies fd Cargo.toml --type f --exec sh -c 'echo "=== {}:" && cat {} | grep -A 5 "tenderdash-proto"' # Also search for any direct references to the features in rust files rg --type rust -e 'tenderdash.proto.*features' -e '#\[cfg\(feature.*grpc'Length of output: 1886
proto/src/lib.rs (2)
3-6: LGTM! Improved feature flag organization.The change from
grpctostdfor no_std configuration is more semantically correct, and the added clippy allowance is appropriate for documentation.
37-58: Well-structured conditional compilation for module separation.The module organization clearly separates client and server functionality while maintaining compatibility through re-exports. This is crucial for WASM support where server functionality is not needed.
Let's verify the module organization:
✅ Verification successful
Module separation is correctly implemented and verified.
The codebase maintains clean separation between server, client, and no_std implementations through feature flags, with no unexpected cross-dependencies. The organization supports WASM compatibility by properly isolating server functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any potential cross-dependencies between client and server modules rg -A 3 'use (crate::)?(tenderdash_grpc|tenderdash_grpc_client)' --type rustLength of output: 392
Script:
#!/bin/bash # Check module definitions and their contents echo "=== Checking module definitions ===" fd -e rs -x head -n 20 {} echo -e "\n=== Checking for any tonic/grpc related imports ===" rg "use tonic|use grpc" --type rustLength of output: 20579
proto/src/serializers/part_set_header_total.rs (1)
9-11: LGTM! Consistent feature flag usage for imports.The change from
grpctostdfeature flag for conditional imports is consistent with the new feature organization.proto/src/protobuf.rs (1)
6-8: LGTM! Consistent feature flag usage for imports.The change from
grpctostdfeature flag for conditional imports aligns with the new feature organization.
Issue being fixed or feature implemented
When building for WASM32 (eg. Javascript) targets, some tonic features, like channels, are not available.
We need to refactor this library a bit to allow building rs-tenderdash-abci without these features.
We do this by separating client and server-side code to
clientandserverfeatures .New approach defines the following features for tenderdash-proto crate:
When built with --no-default-features, only data types are generated.
What was done?
tenderdash-protoHow Has This Been Tested?
GHA + tested as part of dash platform wasm work (branch
feat/wasm-dapi-sdk-client, rev 0a34d38106999a2a3da0b87f7b7403fb3e6d3f4d).Breaking Changes
stdis deprecated, andgrpcis marked as internal; useserverorclientinsteadChecklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
Release Notes
Configuration Changes
Error Handling Improvements
Module Management
Dependency Updates