build(deps)!: update prost, tokio, tonic and other dependencies #95
Conversation
lklimek
changed the title
Cargo.toml
Outdated
|
|
||
| [workspace.package] | ||
|
|
||
| rust-version = "1.76" |
There was a problem hiding this comment.
This is "minimal" version supported by the crate, and it supports 1.76.
Bumping this will be a breaking change.
But I have changed the Dockerfiles to ensure everything builds on 1.76.
There was a problem hiding this comment.
Actually, I found out that it doesn't build cleanly on 1.76, updating to 1.80 as you suggested.
lklimek
force-pushed
the
build/deps-update
branch
from
40c917c to
d60c56f
Compare
lklimek
force-pushed
the
build/deps-update
branch
from
97daed9 to
ac6a144
Compare
WalkthroughThe pull request introduces several updates across various Changes
Poem
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (7)
Dockerfile-debian (2)
4-4: LGTM! Consider using a more flexible version constraint.The update to
rust:1.80.0-bookwormaligns with the new Rust version requirement mentioned in the PR objectives. This ensures compatibility with the updated dependencies.Consider using a more flexible version constraint like
rust:1.80-bookwormto automatically get minor updates and security patches while still maintaining the major version requirement.
Line range hint
38-47: Great optimizations for the build process!The updates to the cargo build command introduce several improvements:
- Caching mechanisms for Cargo's registry and git database, which will speed up subsequent builds.
- The
--config net.git-fetch-with-cli=trueoption addresses an ARM build issue.- The
cargo cleancommand at the end helps preserve space on GitHub cache.These changes will significantly improve build performance and efficiency.
Consider adding a comment explaining the purpose of the
cargo cleancommand, as its placement at the end of the build process might not be immediately obvious to all readers.Dockerfile-alpine (2)
3-3: Approved: Base image update looks good.The update to
rust:1.80-alpine3.20aligns with the PR objectives and provides a more recent Alpine version, which is good for security and compatibility.Consider using a more specific Rust version (e.g.,
rust:1.80.0-alpine3.20) to ensure reproducible builds across different environments.
Line range hint
18-26: Approved: Improved protoc installation process.The updated protoc installation process correctly handles different architectures (ARM64 and x86_64) using the BUILDPLATFORM. This change enhances the Dockerfile's compatibility across different build environments.
Consider adding a comment explaining the purpose of the conditional PROTOC_ARCH setting for better readability. For example:
# Set PROTOC_ARCH based on build platform to ensure correct protoc version is downloaded RUN if [[ "$BUILDPLATFORM" == "linux/arm64" ]] ; then export PROTOC_ARCH=aarch_64; else export PROTOC_ARCH=x86_64 ; fi; \ # ... rest of the commandscripts/release.sh (1)
95-97: Improved error handling and version updating process. Consider minor enhancements.The changes improve the script's robustness and maintainability:
- Adding
set -exenhances error handling and debugging.- Simplifying the
sedcommand for Cargo.toml makes it more maintainable.- Updating DEFAULT_VERSION in build.rs ensures consistency.
- Using
cargo fmtmaintains code style consistency.Consider the following minor improvements:
- Check if files exist before modifying them to prevent potential errors:
if [ -f "./Cargo.toml" ]; then sed -i "s/^version = .*/version = \"$rs_tenderdash_abci_version\"/" ./Cargo.toml else echo "Error: Cargo.toml not found" exit 1 fi if [ -f "./proto/build.rs" ]; then sed -i "s/^\s*const DEFAULT_VERSION: &str = \".*\";/const DEFAULT_VERSION: \&str = \"v$td_version\";/" ./proto/build.rs cargo fmt -- ./proto/build.rs else echo "Error: proto/build.rs not found" exit 1 fi
- Consider not suppressing stderr for
cargo fmtto catch potential warnings:cargo fmt -- ./proto/build.rsThese changes will make the script more robust and help catch potential issues earlier.
proto/src/lib.rs (1)
34-34: Approved: Exposing prost crate improves usability but increases API surface.The addition of
pub use prost;makes theprostcrate publicly available to users of this library. This change aligns with the library's purpose and may simplify usage for consumers who need direct access toprostfunctionality.However, be aware that this increases the API surface and creates a tighter coupling with the
prostcrate, which could impact future maintenance and make it harder to change the underlying Protobuf implementation if needed.Consider documenting this change and its implications in the library's documentation to inform users about the availability of
prostand any potential impact on semantic versioning.abci/src/server/codec.rs (1)
Line range hint
168-172: LGTM! Consider enhancing error handling.The updated
decodemethod correctly uses the newly importeddecode_varintfunction. The error handling improvement helps distinguish between partial length delimiters and actual errors, which is a good practice.Consider adding a more specific error type for the case when the source buffer exceeds
MAX_VARINT_LENGTHbut still fails to decode. This could help in debugging potential issues:Err(e) => { if src_len > MAX_VARINT_LENGTH { return Err(Error::InvalidVarint(src_len)); } return Err(e.into()); },This assumes you have an
InvalidVarintvariant in yourErrorenum. If not, you might want to add one.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (14)
- Cargo.toml (1 hunks)
- Dockerfile-alpine (2 hunks)
- Dockerfile-debian (1 hunks)
- abci/Cargo.toml (2 hunks)
- abci/src/lib.rs (1 hunks)
- abci/src/server/codec.rs (3 hunks)
- abci/src/signatures.rs (1 hunks)
- abci/tests/grpc.rs (3 hunks)
- proto-compiler/Cargo.toml (1 hunks)
- proto/Cargo.toml (2 hunks)
- proto/src/lib.rs (1 hunks)
- proto/src/protobuf.rs (2 hunks)
- proto/tests/unit.rs (1 hunks)
- scripts/release.sh (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- Cargo.toml
🔇 Additional comments (32)
proto-compiler/Cargo.toml (4)
9-10: LGTM: Workspace configurations addedThe addition of workspace configurations for
rust-versionandversionis a good practice. This change will help maintain consistency across multiple packages in the workspace and aligns with the PR objectives.
18-18: Verify regex downgrade, other updates look goodThe updates to
ureq(2.9.6 to 2.10) andzip(2.1.3 to 2.2) are minor and align with the PR objectives. However, the downgrade ofregexfrom 1.10.4 to 1.10 is unusual.Could you please provide the rationale for downgrading the
regexcrate? It's important to ensure this doesn't introduce any compatibility issues or remove any needed functionality.To verify the impact of these changes, especially the regex downgrade, please run the following script:
#!/bin/bash # Description: Check for any issues related to the dependency updates # Test: Check for any usage of regex features that might have been removed rg --type rust 'regex::' -C 3 # Test: Run tests to ensure functionality is not broken cargo test # Test: Check for any warnings related to the updated dependencies cargo build 2>&1 | grep -E "warning:.*regex|warning:.*ureq|warning:.*zip"Also applies to: 20-21
23-23: tonic-build update looks good, verify compatibilityThe update of
tonic-buildfrom 0.11.0 to 0.12 aligns with the PR objectives. While this is a minor version update, it could potentially introduce breaking changes. Since it's an optional dependency, it's crucial to ensure that this update doesn't cause any compatibility issues when thegrpcfeature is enabled.To verify compatibility, please run the following script:
16-17: Dependency updates look good, verify compatibilityThe updates to
prost-build(0.12.4 to 0.13) andtempfile(3.10.1 to 3.12) align with the PR objectives. These are minor version updates and should not introduce breaking changes. However, it's important to ensure that these updates don't cause any compatibility issues with the rest of the codebase.To verify compatibility, please run the following script:
Dockerfile-debian (2)
6-6: Excellent improvement to shell execution!The update to
SHELL ["/bin/bash", "-exc"]enhances the build process in two ways:
- The
-eflag ensures the build fails fast if any command encounters an error.- The
-xflag enables command tracing, which aids in debugging by showing each command as it's executed.These changes will make the build process more robust and easier to troubleshoot.
Line range hint
1-47: Overall, excellent updates to the Dockerfile!The changes in this Dockerfile align well with the PR objectives and introduce several improvements:
- Updated base image to a specific Rust version, ensuring compatibility with new dependency requirements.
- Enhanced shell configuration for better error handling and debugging.
- Optimized build process with caching mechanisms and ARM-specific configurations.
These updates will result in a more robust, efficient, and maintainable build process. Great job on these improvements!
Dockerfile-alpine (2)
15-15: Approved: Enhanced shell configuration improves build process.The updated SHELL command with
-excflags improves the build process in two ways:
- The
-eflag ensures the build fails immediately if any command exits with an error, preventing silent failures.- The
-xflag prints each command before execution, which aids in debugging if issues occur.
Line range hint
1-55: Overall: Excellent improvements to the Dockerfile.The changes made to this Dockerfile significantly enhance its functionality and robustness:
- The base image update ensures compatibility with Rust 1.80 and provides a more recent Alpine version.
- The improved shell configuration with
-excflags enhances error handling and debugging during the build process.- The updated protoc installation process now supports both ARM64 and x86_64 architectures, improving cross-platform compatibility.
These improvements align well with the PR objectives and contribute to a more maintainable and reliable build process.
proto/Cargo.toml (6)
22-23: LGTM: Workspace configurations addedThe addition of workspace configurations for
rust-versionandversionpromotes consistency across the project. This is a good practice for maintaining uniformity in a multi-package workspace.
54-55: LGTM: Minor dependency updatesThe updates to bytes, serde, num-traits, and chrono are minor version increments. These updates likely include bug fixes and performance improvements without introducing breaking changes.
Also applies to: 61-61, 68-68
Line range hint
22-73: Summary of Cargo.toml changesThe updates in this file align with the PR objectives, introducing new versions of prost, tonic, and other dependencies. These changes bring potential improvements but also introduce breaking changes that require careful attention.
Key points:
- Workspace configurations have been added for consistency.
- Major version updates to prost (0.13) and tonic (0.12) may introduce breaking changes.
- Other dependencies have been updated to their latest minor versions.
- derive_more has been updated to 1.0 with new features added.
Please ensure that:
- The codebase is compatible with all updated dependencies, especially prost and tonic.
- The project now requires Rust 1.80 as mentioned in the PR objectives.
- All breaking changes are addressed and the codebase is adjusted accordingly.
- New features from derive_more are utilized where appropriate.
These changes will likely improve the project's performance and maintainability, but thorough testing is crucial to ensure everything works as expected with the updated dependencies.
53-53: Verify compatibility with tonic 0.12The update to tonic 0.12 aligns with the PR objectives. As this is a major version update, it may introduce breaking changes.
Please ensure that:
- The codebase is compatible with the new tonic version.
- Any breaking changes are addressed.
Run the following script to check for any usage of deprecated or removed tonic features:
#!/bin/bash # Description: Check for potential breaking changes in tonic usage # Search for tonic usage in the codebase rg --type rust 'use tonic|tonic::'
69-69: Verify compatibility with derive_more 1.0 and new featuresThe update to derive_more 1.0 with additional features is a significant change. While version 1.0 often indicates API stability, it's important to verify compatibility.
Please ensure that:
- The codebase is compatible with derive_more 1.0.
- The new features "from" and "from_str" are properly utilized where needed.
Run the following script to check for derive_more usage and potential areas where the new features could be applied:
#!/bin/bash # Description: Check derive_more usage and potential areas for new features # Search for derive_more usage rg --type rust 'use derive_more|#\[derive\(' # Search for potential areas to use new "from" and "from_str" features rg --type rust 'impl From<.*> for' -g '!**/target/**' rg --type rust 'impl FromStr for' -g '!**/target/**'
50-52: Verify compatibility with prost 0.13The update to prost 0.13 is in line with the PR objectives. However, as this is a major version update, it may introduce breaking changes.
Please ensure that:
- The codebase is compatible with the new prost version.
- Any breaking changes are addressed.
- The project now requires Rust 1.80 as mentioned in the PR objectives.
Run the following script to check for any usage of deprecated or removed prost features:
abci/Cargo.toml (4)
Line range hint
1-85: Summary of Cargo.toml changesThe changes in this file align with the PR objectives of updating dependencies. Key points:
- Workspace configuration has been added, which may impact version management across the project.
- Dependencies have been updated (
bollard,tokio) and removed (prost), which may require compatibility checks.- The
tonicdev-dependency has been commented out, potentially affecting gRPC-related tests.Please ensure all suggested verifications are performed to maintain the project's stability and test coverage.
82-85: Verify impact of removing tonic from dev-dependencies.The
tonicdependency has been commented out in the dev-dependencies section. This suggests that gRPC server tests may no longer be included or have been moved elsewhere.Please clarify the reason for removing the
tonicdependency and confirm that this doesn't negatively impact the project's test coverage. Run the following script to check for any remainingtonicusage:#!/bin/bash # Description: Check for any remaining tonic usage in tests # Test 1: Search for tonic usage in test files rg "use.*tonic" --type rust --glob "**/*test*.rs" # Test 2: Check if there are any gRPC-related tests that might be affected rg "gRPC|grpc" --type rust --glob "**/*test*.rs"If
tonicis still being used or if there are gRPC-related tests, please ensure they are properly addressed or explain why they are no longer needed.
80-80: Dependency versions updated: Verify compatibility and test coverage.The following changes have been made to the dependencies:
bollardupdated from "0.16.1" to "0.17"tokioupdated from "1.37.0" to "1.39"prostdependency has been removedThese updates align with the PR objectives and are generally good for security and feature improvements.
To ensure these changes don't introduce any compatibility issues, please run the following script:
#!/bin/bash # Description: Verify updated dependencies and their impact # Test 1: Check for any breaking changes in bollard 0.17 rg -i "breaking.?changes" $(fd -t f -e md | grep -i "bollard.*changelog") # Test 2: Check for any breaking changes in tokio 1.39 rg -i "breaking.?changes" $(fd -t f -e md | grep -i "tokio.*changelog") # Test 3: Verify that prost is not used anywhere in the codebase if rg "use.*prost" --type rust; then echo "Warning: prost is still being used in the codebase despite being removed from dependencies." fi # Test 4: Run the test suite to ensure everything still works cargo test --all-featuresAdditionally, please ensure that the test coverage is adequate for the updated dependencies, especially for any new features or changed APIs.
Also applies to: 82-82
15-16: Workspace configuration added: Verify project-wide impact.The addition of
rust-version.workspace = trueandversion.workspace = trueindicates that this package is now part of a workspace configuration. This change can help maintain consistency across multiple packages in the project.To ensure this change doesn't unexpectedly affect other parts of the project, please run the following script:
✅ Verification successful
Workspace configuration verified successfully.
The workspace is correctly configured with the
[workspace]section, defined members, and consistent versioning across all packages. No issues were found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify workspace configuration and its impact on other packages # Test 1: Check if a workspace root Cargo.toml exists if [ -f "./Cargo.toml" ]; then echo "Workspace root Cargo.toml found." # Check if it defines workspace members and package version grep -q '\[workspace\]' ./Cargo.toml && grep -q 'members' ./Cargo.toml && grep -q 'package.version' ./Cargo.toml if [ $? -eq 0 ]; then echo "Workspace configuration seems correct." else echo "Warning: Workspace configuration may be incomplete." fi else echo "Warning: No workspace root Cargo.toml found." fi # Test 2: Check for other packages that might be affected echo "Packages that might be affected by the workspace configuration:" fd -t f -e toml | xargs grep -l 'version.workspace = true'Length of output: 574
Script:
#!/bin/bash # Description: Verify workspace configuration and its impact on other packages # Test 1: Check if a workspace root Cargo.toml exists and is correctly configured if [ -f "./Cargo.toml" ]; then echo "Workspace root Cargo.toml found." # Check if it defines [workspace], members, and package.version if grep -q '\[workspace\]' ./Cargo.toml; then echo "[workspace] section found." else echo "Warning: [workspace] section missing in Cargo.toml." fi if grep -q 'members\s*=' ./Cargo.toml; then echo "Workspace members are defined." else echo "Warning: Workspace members not defined." fi if grep -q 'version\s*=' ./Cargo.toml; then echo "Package version is set at workspace level." else echo "Warning: Package version not set at workspace level." fi else echo "Warning: No workspace root Cargo.toml found." fi # Test 2: Identify other Cargo.toml files that might be affected by workspace configuration echo "Checking other Cargo.toml files for workspace version settings..." fd -t f -e toml | while read -r file; do if grep -q 'version.workspace\s*=\s*true' "$file"; then echo "Affected package found: $file" fi doneLength of output: 1199
Script:
#!/bin/bash # Description: Verify workspace members and consistent versioning across the workspace # Test 1: List all workspace members echo "Listing all workspace members defined in the root Cargo.toml:" grep -A 100 '\[workspace\]' Cargo.toml | grep 'members' -A 100 | grep -o '".*"' | tr -d '"' || echo "No members found." # Test 2: Verify each member's Cargo.toml for consistent versioning echo "Verifying consistent versioning across workspace members:" fd -t f -e toml | while read -r file; do if grep -q 'version.workspace\s*=\s*true' "$file"; then echo "$file has workspace versioning enabled." version=$(grep '^version\s*=' "$file" | awk -F'=' '{print $2}' | tr -d ' "') echo " - Version: $version" fi done # Test 3: Check for duplicate or conflicting version declarations echo "Checking for duplicate or conflicting version declarations:" fd -t f -e toml | grep 'version\s*=' | sort | uniq -c | sort -nr || echo "No duplicate or conflicting versions found."Length of output: 1739
proto/src/protobuf.rs (3)
24-24: Approved: Beneficial addition of theCopytraitThe addition of the
Copytrait to theTimestampstruct is a positive change. This allows for more efficient copying ofTimestampinstances, which can lead to performance improvements in scenarios where these objects are frequently copied. The change is safe because the struct only contains primitive types (i64 and i32) which already implementCopy.This modification aligns well with the PR objectives of updating and improving the codebase.
24-24: Summary: Beneficial improvements toTimestampandDurationstructsThe changes made to this file are minimal but impactful:
- Both
TimestampandDurationstructs now implement theCopytrait.- This allows for more efficient copying of these types, potentially improving performance in scenarios where they are frequently copied.
- The changes are safe and don't introduce any breaking changes or potential issues.
These modifications align well with the PR objectives of updating and improving the codebase. They enhance the usability and efficiency of these core types without altering their fundamental behavior.
Also applies to: 49-49
49-49: Approved: Beneficial addition of theCopytraitThe addition of the
Copytrait to theDurationstruct is a positive change. This allows for more efficient copying ofDurationinstances, which can lead to performance improvements in scenarios where these objects are frequently copied. The change is safe because the struct only contains primitive types (i64 and i32) which already implementCopy.This modification aligns well with the PR objectives of updating and improving the codebase.
Note: The
Durationstruct doesn't derive::serde::Deserialize, ::serde::SerializelikeTimestampdoes. This is correct because custom implementations for these traits are provided later in the file.Let's verify the custom serialization implementations:
✅ Verification successful
Verified: Custom serialization implementations confirmed
The custom implementations of
serde::Serializeandserde::Deserializefor theDurationstruct have been successfully verified. This ensures that the omission of these traits in the derive attributes is properly handled by the custom implementations.The addition of the
Copytrait remains beneficial for performance and is safe given thatDurationconsists of primitive types.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify custom serialization implementations for Duration struct # Test: Check for custom Serialize implementation rg --type rust "impl serde::Serialize for Duration" proto/src/protobuf.rs # Test: Check for custom Deserialize implementation rg --type rust "impl<'de> serde::Deserialize<'de> for Duration" proto/src/protobuf.rsLength of output: 255
abci/tests/grpc.rs (5)
21-21: LGTM: Import path updated correctlyThe import statement has been updated to use
tenderdash_proto::tonicinstead oftonicdirectly. This change is consistent with the overall refactoring to use the customtenderdash_protomodule.
61-61: LGTM: Server import updated correctlyThe import for
Serverhas been updated to usetenderdash_proto::tonic::transport::Server, which is consistent with the new import structure. This change correctly shifts the server implementation to thetenderdash_protonamespace.
128-129: LGTM: Method signature updated correctlyThe
echomethod signature has been properly updated to usetenderdash_proto::tonic::Requestandtenderdash_proto::tonic::Response. These changes align with the new import structure and ensure consistency throughout the codebase.
137-141: LGTM: Method signature and return type updated correctlyThe
infomethod signature and return type have been properly updated to usetenderdash_proto::tonic::Request,tenderdash_proto::tonic::Response, andtenderdash_proto::tonic::Status. These changes maintain consistency with the new import structure and ensure proper type usage throughout the implementation.
Line range hint
1-150: Summary: Import paths and type usage updated consistentlyThe changes in this file consistently update the import paths and type usage for the
toniccrate to use thetenderdash_proto::tonicmodule. This refactoring appears to be part of a larger effort to use a customtenderdash_protomodule throughout the codebase.While the changes in this file look good, it's important to ensure consistency across the entire project.
Please run the following script to verify that these changes are consistent across the codebase:
This script will help identify any inconsistencies in the usage of
tenderdash_proto::tonicacross the project, excluding the current file.proto/tests/unit.rs (1)
Line range hint
1-200: Verify test coverage considering recent dependency updatesThe existing tests in this file cover various aspects of protobuf functionality, which is good. However, given the recent updates to dependencies like prost, tokio, and tonic mentioned in the PR objectives, it might be beneficial to verify if the current test suite is still comprehensive.
Consider running the following command to check the test coverage:
This will help ensure that the test suite remains robust after the dependency updates.
abci/src/server/codec.rs (4)
204-204: LGTM! Consistent with import changes.The
encodemethod has been updated to use the newly importedencode_varintfunction. This change is consistent with the import modifications and maintains the same functionality.
Line range hint
1-290: Overall assessment: Changes look good and align with PR objectives.The modifications in this file successfully update the usage of
prostlibrary components as intended. The changes are consistent across the file, including the test module. The code maintains its original functionality while adapting to the new dependency versions.A few minor suggestions have been made to further improve error handling and ensure consistency across the codebase. Please consider these suggestions and ensure all tests are passing after the changes.
212-212: LGTM! Ensure all tests are passing.The test module has been correctly updated to use the new import path for
Message. This change maintains consistency with the main code.To ensure that all tests are passing with the new changes, please run the following command:
#!/bin/bash # Description: Run all tests for the project cargo test --allThis will verify that the changes haven't introduced any regressions in the test suite.
12-15: LGTM! Consider verifying import usage across the codebase.The updated imports align with the PR objective of updating dependencies. The more specific imports from
tenderdash_proto::prostimprove code clarity and reduce potential naming conflicts.To ensure consistency across the codebase, please run the following script to verify the usage of these imports:
abci/src/signatures.rs (1)
26-26: Import ofCanonicalVoteAddedThe import of
CanonicalVotefromtenderdash_proto::typesis necessary for its usage in theSignabletrait implementation. This update ensures thatCanonicalVoteis correctly referenced in the code.
| #[allow(deprecated)] | ||
| #[cfg(feature = "server")] | ||
| pub use server::{start_server, CancellationToken, Server, ServerBuilder, ServerRuntime}; | ||
| pub use tenderdash_proto as proto; | ||
| use tenderdash_proto::prost::{DecodeError, EncodeError}; |
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent Imports Detected
The import of DecodeError and EncodeError in proto/src/error.rs still uses prost::{DecodeError, EncodeError}. To maintain consistency across the codebase, please update this import to tenderdash_proto::prost::{DecodeError, EncodeError}.
🔗 Analysis chain
LGTM! Verify impact on codebase.
The import change from prost to tenderdash_proto::prost for DecodeError and EncodeError looks good and aligns with the PR objectives. This modification likely reflects updates in the dependency structure or aims to maintain consistency across the project.
To ensure this change doesn't introduce any unintended consequences, please run the following script to verify that all usages of DecodeError and EncodeError in the codebase are consistent with this new import:
This script will help identify any inconsistencies in the usage or import of these error types across the project.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of DecodeError and EncodeError from tenderdash_proto::prost
# Test: Search for DecodeError and EncodeError usages
echo "Searching for DecodeError and EncodeError usages:"
rg --type rust -e "DecodeError" -e "EncodeError"
# Test: Verify imports of DecodeError and EncodeError
echo "Verifying imports of DecodeError and EncodeError:"
rg --type rust -e "use .*prost::.*DecodeError" -e "use .*prost::.*EncodeError"
Length of output: 825
| @@ -23,8 +23,7 @@ use std::{ | |||
| }; | |||
|
|
|||
| use bytes::BufMut; | |||
| use prost::Message; | |||
| use tenderdash_proto::types::CanonicalVote; | |||
| use tenderdash_proto::{prost::Message, types::CanonicalVote}; | |||
|
|
|||
| use crate::{ | |||
| proto::types::{ | |||
There was a problem hiding this comment.
Correct the Syntax of the #[deprecated] Attribute
The syntax for the #[deprecated] attribute should include named parameters such as since and note. The correct usage is:
#[deprecated(note = "replaced by calculate_sign_hash() to unify naming between core, platform and tenderdash")]This ensures that the compiler properly recognizes the deprecation and provides informative warnings.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
proto/Cargo.toml (2)
Line range hint
2-23: LGTM! Consider adding a version field.The added package metadata provides comprehensive information about the package, which is excellent for documentation and discoverability. The workspace configurations for
rust-versionandversionare appropriate.Consider adding a
versionfield to the package metadata, even if it's managed by the workspace. This can help with explicit versioning when the package is used outside the workspace context.
Line range hint
28-46: LGTM! Consider enhancing the deprecation notice.The features configuration is well-documented, especially the note about protobuf file regeneration. The deprecation of the
stdfeature in favor ofgrpcis a good practice.To improve clarity, consider enhancing the deprecation notice for the
stdfeature. You could add a comment explaining whygrpcis preferred and when thestdfeature might be removed in future versions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- abci/Cargo.toml (3 hunks)
- proto-compiler/Cargo.toml (1 hunks)
- proto/Cargo.toml (3 hunks)
- proto/tests/unit.rs (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- abci/Cargo.toml
- proto-compiler/Cargo.toml
- proto/tests/unit.rs
🧰 Additional context used
🔇 Additional comments (2)
proto/Cargo.toml (2)
Line range hint
73-79: LGTM! Verify build dependency compatibility.The update to
serde_jsonin dev-dependencies is consistent with the overall dependency updates.Please verify that the unchanged build-dependency on
tenderdash-proto-compileris compatible with the updated dependencies. Run the following script to check for any potential issues:#!/bin/bash # Description: Verify build dependency compatibility # Test: Check if tenderdash-proto-compiler builds successfully with updated dependencies cd ../proto-compiler && cargo check && cd ../proto # Test: Ensure that the proto compilation still works as expected cargo clean && cargo build
50-69: LGTM! Verify compatibility with updated dependencies.The dependency updates align with the PR objectives. The new features added to
derive_more(["from", "from_str"]) may introduce new functionality.Please ensure that the updated dependencies, especially
prostandtonic, are compatible with the rest of the codebase. Run the following script to check for any compatibility issues:
Issue being fixed or feature implemented
New version of libraries like prost, tokio and tonic were released.
What was done?
How Has This Been Tested?
run tests locally
Breaking Changes
Related projects must use matching versions of prost, tokio and tonic.
Rust 1.80 is required.
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Bug Fixes
Dependency Updates
bollard,tokio,prost, andtonic.Documentation
Refactor
Signabletrait to unify naming conventions across implementations.