feat(proto)!: serde feature to enable serde for all messages #98
Conversation
proto/Cargo.toml
Outdated
| @@ -31,7 +31,7 @@ all-features = true | |||
| # | |||
| # Sometimes cleaning the build cache with `cargo clean` might be necessary to solve | |||
| # issues related to outdated generated files. | |||
| default = ["grpc"] | |||
| default = ["grpc", "serde"] | |||
There was a problem hiding this comment.
Are you sure we want to have it by default? I feel like we don't.
There was a problem hiding this comment.
Disabling this by default would break backwards-compatibility.
And it's required by Platform.
There was a problem hiding this comment.
So we supported serde in previous versions by default without feature? Platform could use feature if we need this in case we dump requests
There was a problem hiding this comment.
removed serde from default features
WalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
Suggested reviewers
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: 7
🧹 Outside diff range and nitpick comments (11)
proto/Cargo.toml (1)
57-59: LGTM: Serde dependency added correctly, consider version alignmentThe addition of
serdeas an optional dependency is well-implemented and consistent with the newserdefeature. Good job on disabling default features and only enabling the "derive" feature, as this helps minimize unnecessary code.One suggestion:
Consider checking if the specific version (1.0.208) aligns with other parts of the project or related dependencies. It might be beneficial to use a version specifier that allows for compatible updates (e.g., "^1.0.208") unless there's a specific reason for pinning to this exact version.
abci/Cargo.toml (1)
Line range hint
1-85: Consider updating documentation for deprecatedstdfeatureWhile not directly related to the serde changes, I noticed that the
stdfeature is marked as deprecated in favor ofgrpc. It might be beneficial to update the package documentation to reflect this change and guide users towards using thegrpcfeature instead.proto-compiler/src/lib.rs (1)
100-107: Consider using a logging framework for better control.The addition of logging statements for type and field attributes is helpful for debugging and understanding the compilation process. However, using
println!directly might lead to cluttered output, especially if there are many attributes.Consider using a proper logging framework like
logortracing. This would allow for more flexible control over log levels and output formatting. Here's an example of how you could refactor this:+ use log::{info, debug}; - println!("[info] => Adding type attribute: {:?}", type_attribute); + debug!("Adding type attribute: {:?}", type_attribute); - println!("[info] => Adding field attribute: {:?}", field_attribute); + debug!("Adding field attribute: {:?}", field_attribute);This change would allow users to control the verbosity of the output using log levels, making it easier to manage in different environments (e.g., development vs. production).
proto/tests/unit.rs (1)
138-141: LGTM: New serde test for ConsensusParamsThe addition of this test function is excellent. It verifies the serde functionality for
ConsensusParams, which aligns with the PR objectives. The conditional compilation ensures it only runs when the "serde" feature is enabled.Consider adding more assertions to verify other fields of the
ConsensusParamsstruct, not just theconsensus_version. This would provide more comprehensive coverage of the deserialization process.proto-compiler/src/constants.rs (1)
46-47: LGTM with a minor suggestion.The change to make
SERIALIZEDconditionally deriveDeserializeandSerializewhen the "serde" feature is enabled is good. It allows for more flexible use of serialization.Consider adding a brief comment explaining the purpose of this constant and why it's now
pub(crate).abci/src/application.rs (3)
196-215: LGTM! Consider adding a note about potential output differences.The
serialize!macro is well-designed and provides a good abstraction for conditional serialization based on theserdefeature. It will help maintain consistency across the codebase.Consider adding a comment noting that the output format might differ slightly between the
serdeand non-serdecases. This could be important for consumers of the serialized data.
226-229: LGTM! Consider adding comments for complex serializations.The changes to
serialize_response_for_loggingimprove consistency and comprehensiveness of the logged data. The use of theserialize!macro simplifies the code and makes it more maintainable.Consider adding brief comments explaining the purpose of complex serializations, especially for fields like
tx_recordsandtx_results. This would help future maintainers understand the rationale behind the serialization choices.Also applies to: 243-250, 270-276
Line range hint
309-334: LGTM! Consider handling potentialNonecases more explicitly.The changes to
validator_set_update_to_stringimprove consistency in serialization and provide more comprehensive logging of validator set updates. The use of theserialize!macro enhances code maintainability.Consider handling the
Nonecase more explicitly at the beginning of the function, rather than usingunwrap_orat the end. This could improve readability and make the function's behavior more obvious. For example:fn validator_set_update_to_string(validator_set_update: Option<&ValidatorSetUpdate>) -> String { match validator_set_update { None => "None".to_string(), Some(update) => { // existing serialization logic here } } }proto/src/protobuf.rs (3)
Line range hint
69-78: Potential Integer Overflow inDuration::serializeMethodThe calculation
self.seconds * 1_000_000_000 + self.nanos as i64in theserializemethod could cause an integer overflow whenself.secondsis near the maximum or minimum value ofi64.Consider using checked arithmetic to prevent overflow:
let total_nanos = self.seconds.checked_mul(1_000_000_000) .and_then(|sec_nanos| sec_nanos.checked_add(self.nanos as i64)) .ok_or_else(|| serde::ser::Error::custom("Duration value overflow"))?; serializer.serialize_i64(total_nanos)This ensures that any overflow is detected and an appropriate error is returned during serialization.
Line range hint
106-113: Inconsistent Serialization and Deserialization Formats forDurationIn the
serializemethod,Durationis serialized as ani64usingserializer.serialize_i64(total_nanos), but in thedeserializemethod, it expects a string viadeserializer.deserialize_str(DurationVisitor). This mismatch can lead to deserialization errors.To maintain consistency, adjust the deserialization to match the serialization format. Modify the
deserializemethod to usedeserialize_i64:deserializer.deserialize_i64(DurationVisitor)And update the
DurationVisitorto implement thevisit_i64method:fn visit_i64<E>(self, value: i64) -> Result<Duration, E> where E: serde::de::Error, { let seconds = value / 1_000_000_000; let nanos = (value % 1_000_000_000) as i32; Ok(Duration { seconds, nanos }) }This ensures that the serialization and deserialization processes are aligned.
Line range hint
81-105: Add Error Handling for Value Ranges inDurationVisitorWhen converting from
i128toi64andi32in theDurationVisitor, there is a risk of truncation or overflow if the values exceed the valid ranges forsecondsandnanos.Implement checks to verify that the parsed values are within the acceptable ranges:
fn visit_i128<E>(self, value: i128) -> Result<Duration, E> where E: serde::de::Error, { if value > i64::MAX as i128 * 1_000_000_000 + 999_999_999 || value < i64::MIN as i128 * 1_000_000_000 - 999_999_999 { return Err(E::custom("Duration value out of range")); } let seconds = (value / 1_000_000_000) as i64; let nanos = (value % 1_000_000_000) as i32; Ok(Duration { seconds, nanos }) }This addition ensures that any out-of-range values are correctly handled during deserialization.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
- abci/Cargo.toml (2 hunks)
- abci/src/application.rs (6 hunks)
- proto-compiler/src/constants.rs (2 hunks)
- proto-compiler/src/lib.rs (2 hunks)
- proto/Cargo.toml (1 hunks)
- proto/src/lib.rs (1 hunks)
- proto/src/prelude.rs (2 hunks)
- proto/src/protobuf.rs (4 hunks)
- proto/src/serializers/timestamp.rs (1 hunks)
- proto/src/time.rs (1 hunks)
- proto/tests/unit.rs (2 hunks)
🧰 Additional context used
🔇 Additional comments (24)
proto/src/prelude.rs (3)
14-15: Conditional export oftonicaligns with PR objectives.The addition of
pub use tonic;under thegrpcfeature flag is consistent with the PR's goal of implementing serialization features. This change enhances modularity by makingtonicavailable only when thegrpcfeature is enabled.
17-17: Confirm the renaming ofFromMilisandToMilistraits.The re-export of
FromMillisandToMillisfromcrate::timealigns with the PR objectives of renaming these traits. However, we should verify that the original traits have been properly deprecated and that all usages have been updated.Let's check for any remaining uses of the old trait names:
#!/bin/bash # Description: Check for any remaining uses of the old trait names # Expected result: No occurrences of FromMilis or ToMilis rg --type rust "FromMilis|ToMilis"If any occurrences are found, they should be updated to use the new trait names.
3-3: Consider the implications of allowing unused imports at the crate level.The change from
#[allow(unused_imports)]to#![allow(unused_imports)]expands the scope of this attribute from module-level to crate-level. While this may suppress warnings more broadly, it could potentially hide useful information about truly unused imports across the entire crate.To assess the impact, let's check for unused imports across the crate:
Consider reverting to module-level allow if there are no legitimate unused imports in other modules.
proto/Cargo.toml (3)
49-50: LGTM: Serde feature added as optionalThe addition of the
serdefeature is well-implemented and aligns with the PR objectives. It's good to see that it's not included in the default features, addressing the concerns raised in previous discussions. This change provides optional serialization/deserialization support, which will be beneficial for debugging purposes as mentioned in the PR objectives.
52-52: LGTM: Bytes dependency updated correctlyThe update to the
bytesdependency is correct. Removing theserdefeature frombytesensures that serialization support forbytesis only enabled when the newserdefeature is activated. This change is consistent with making serialization optional and aligns well with the overall objectives of the PR.
49-59: Overall: Well-implemented changes for serde supportThe changes in this file effectively implement the
serdefeature as described in the PR objectives. The new feature is correctly added as optional, addressing previous concerns about default inclusion. Dependencies have been updated appropriately to support this feature. These changes will enable serialization and deserialization for debugging purposes while maintaining backwards compatibility.Great job on implementing these changes! The code looks clean and well-structured.
abci/Cargo.toml (2)
43-43: LGTM: Newserdefeature addedThe addition of the
serdefeature aligns well with the PR objectives. It's correctly implemented as an optional feature, which maintains backwards compatibility. The dependency ontenderdash-proto/serdesuggests that the core serialization logic is implemented in thetenderdash-protocrate, which is a good separation of concerns.
58-58: LGTM:serde_jsondependency updated and made optionalThe
serde_jsondependency has been updated from version 1.0.115 to 1.0.128, which is a minor version bump and shouldn't introduce breaking changes. Making it optional is consistent with the newserdefeature and supports the PR objectives by providing JSON serialization capabilities when needed.proto-compiler/src/lib.rs (3)
131-131: LGTM! Please provide more context on the compilation method change.The change from
compile_with_configtocompile_protos_with_configlooks good and likely provides more flexibility in the gRPC compilation process. However, it would be helpful to understand the rationale behind this change.Could you please provide more information on:
- The specific benefits of using
compile_protos_with_configovercompile_with_config?- Any potential impacts on existing functionality or performance?
- Whether this change requires updates to documentation or dependent code?
Line range hint
1-153: Overall, the changes look good and align with the PR objectives.The modifications to implement serde serialization, improve logging, and enhance the compilation process are well-implemented. Here's a summary of the key points:
- The addition of the
SERIALIZEDtype attribute successfully implements serde for all message types.- The new logging statements improve visibility into the compilation process, though they could benefit from using a proper logging framework.
- The change in the gRPC compilation method likely provides more flexibility, but more context would be helpful.
To further improve this PR:
- Consider implementing the logging framework suggestion.
- Provide more information about the rationale behind changing the gRPC compilation method.
- Ensure that comprehensive testing has been done to verify that these changes don't introduce any unintended side effects, especially considering the global application of the
SERIALIZEDattribute.Great work on implementing these features! The changes will significantly enhance the project's serialization capabilities.
98-98: LGTM! Verify the impact of the global type attribute.The addition of the
SERIALIZEDtype attribute for all types aligns with the PR objective of implementing serde serialization. However, given its broad application, it's crucial to ensure this doesn't have any unintended side effects.To verify the impact, please run the following script:
proto/tests/unit.rs (1)
6-6: LGTM: Import cleanupThe removal of
ConsensusParamsfrom the imports is consistent with the changes mentioned in the PR objectives. This cleanup helps maintain a clean and relevant import list.proto/src/lib.rs (4)
47-47: LGTM: Addition of serializers moduleThe conditional inclusion of the
serializersmodule based on theserdefeature flag is a good practice. It aligns with the PR objective of implementing serde serialization for all message types while keeping the library flexible.
Line range hint
1-265: Summary of changes and points for attentionThe changes in this file generally align well with the PR objectives, particularly the addition of the
serializersmodule for implementing serde serialization. However, there are a couple of points that need attention:
- The purpose and contents of the newly added
timemodule need to be clarified.- It would be helpful to confirm whether the re-export of the
preludemodule is a new addition or a relocation of an existing re-export.These clarifications will help in fully understanding the scope and impact of the changes in this PR.
To get a comprehensive view of all changes in this file, please run the following command:
#!/bin/bash # Description: Show all changes in the file git diff HEAD~1 proto/src/lib.rs
49-49: Please provide more information about thetimemoduleThe addition of the
timemodule wasn't explicitly mentioned in the PR objectives. Could you please provide more information about its purpose and contents? Also, consider if this module should be conditionally compiled like theserializersmodule.To understand the contents and purpose of the
timemodule, please run the following command:
52-52: LGTM: Re-export of prelude moduleThe re-export of the
preludemodule is a good practice, making commonly used items easily accessible. However, could you clarify if this is a new addition or just a relocation of an existing re-export? This information will help in understanding the scope of changes in this PR.To verify if this is a new addition or relocation, please run the following command:
proto-compiler/src/constants.rs (6)
48-48: LGTM!The change to make
TYPE_TAGconditionally apply theserdeattribute when the "serde" feature is enabled is consistent with the previous change and provides good flexibility.
50-61: LGTM! Consistent application of conditional compilation.The changes to
QUOTED,QUOTED_WITH_DEFAULT,DEFAULT,HEXSTRING,BASE64STRING,VEC_BASE64STRING, andOPTIONALconstants are consistent and well-implemented. The conditional compilation for the "serde" feature allows for flexible use of these serialization attributes.
65-78: LGTM! Consistent application of conditional compilation.The changes to
NULLABLEVECARRAY,NULLABLE,ALIAS_POWER_QUOTED,RENAME_EDPUBKEY,RENAME_SECPPUBKEY,RENAME_SRPUBKEY,RENAME_DUPLICATEVOTE, andRENAME_LIGHTCLIENTATTACKconstants are consistent and well-implemented. The conditional compilation for the "serde" feature allows for flexible use of these serialization attributes and renaming options.
82-85: LGTM! Consistent application of conditional compilation.The changes to
ALIAS_VALIDATOR_POWER_QUOTED,ALIAS_TOTAL_VOTING_POWER_QUOTED,ALIAS_TIMESTAMP, andALIAS_PARTSconstants are consistent and well-implemented. The conditional compilation for the "serde" feature allows for flexible use of these aliasing attributes.
94-98: LGTM! New type attributes added. Please clarify their implications.The additions to
CUSTOM_TYPE_ATTRIBUTESlook good. The new entries forBlockIDFlag,Request.value, andResponse.valueseem to enhance type conversions and derivations.Could you please clarify:
- Why
BlockIDFlagnow needs to deriveFromPrimitiveandToPrimitive?- What are the implications of deriving
FromforRequest.valueandResponse.value?These explanations would help in understanding the broader impact of these changes on the codebase.
Line range hint
1-200: Overall, the changes look good and improve flexibility.The modifications to
proto-compiler/src/constants.rsconsistently apply conditional compilation for the "serde" feature across various serialization-related constants. This approach enhances the flexibility of the codebase by allowing conditional serialization when the "serde" feature is enabled.Key points:
- Consistent use of
#[cfg_attr(feature = "serde", ...)]across all relevant constants.- Addition of new type attributes in
CUSTOM_TYPE_ATTRIBUTESfor improved type conversions and derivations.- The changes maintain good code organization and readability.
Consider adding brief comments explaining the purpose of key constants and the rationale behind making some of them
pub(crate).abci/src/application.rs (2)
295-303: LGTM! Improved consistency and comprehensiveness.The changes to
exec_tx_results_to_stringenhance the consistency of serialization across the codebase and provide more comprehensive logging of transaction results. The use of theserialize!macro simplifies the code and improves maintainability.
Line range hint
1-579: Overall, the changes improve serialization consistency and logging comprehensiveness.The introduction of the
serialize!macro and the updates to various serialization functions enhance the codebase in several ways:
- Improved consistency in serialization across different response types.
- Enhanced maintainability through the use of a centralized serialization macro.
- More comprehensive logging of ABCI responses and related data structures.
These changes should make debugging and monitoring of the ABCI application easier and more effective.
| #[derive(Clone, Copy, PartialEq, ::prost::Message)] | ||
| #[cfg_attr(feature = "serde", derive(::serde::Serialize, ::serde::Deserialize))] | ||
| #[cfg_attr( | ||
| feature = "serde", | ||
| serde( | ||
| from = "crate::serializers::timestamp::Rfc3339", | ||
| into = "crate::serializers::timestamp::Rfc3339" | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Align Serialization Strategies Between Timestamp and Duration
While Timestamp uses serde(from = "crate::serializers::timestamp::Rfc3339", into = "crate::serializers::timestamp::Rfc3339"), Duration implements custom serialization and deserialization logic. This difference might lead to inconsistent behavior and increased maintenance effort.
Consider implementing similar from and into conversions for Duration, or ensure that both structs use consistent serialization approaches. For example, you could create a DurationRfc3339 adapter in crate::serializers::duration and adjust the attributes accordingly:
#[cfg_attr(
feature = "serde",
serde(
from = "crate::serializers::duration::DurationRfc3339",
into = "crate::serializers::duration::DurationRfc3339"
)
)]This promotes consistency and simplifies serialization handling across your types.
There was a problem hiding this comment.
fixing this is too big task, out of scope of this PR.
There was a problem hiding this comment.
@lklimek, understood. Thanks for the clarification.
(｡•́︿•̀｡)
✏️ Learnings added
Learnt from: lklimek
PR: dashpay/rs-tenderdash-abci#98
File: proto/src/protobuf.rs:24-31
Timestamp: 2024-10-10T13:18:57.488Z
Learning: Fixing the inconsistency in serialization approaches between `Timestamp` and `Duration` is considered a large task and out of scope for the current PR.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
proto/src/error.rs (1)
15-19: LGTM! Consider adding a doc comment for the new variant.The new
TimeConversionerror variant is well-implemented and consistent with the existing code structure. It provides a clear error message format that includes the reason for the time conversion failure.Consider adding a brief doc comment for the
TimeConversionvariant to explain when this error might occur. This would improve the documentation and make it easier for other developers to understand when this error might be raised. For example:/// Error occurred during time conversion operations. TimeConversion { reason: String } | e | { format!("error converting time: {}", e.reason) },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- proto/src/error.rs (1 hunks)
- proto/src/time.rs (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- proto/src/time.rs
🧰 Additional context used
Issue being fixed or feature implemented
To export requests and responses for debugging purposes, we need all types to implement
serdeserialization and deserialization.What was done?
Updated proto and proto-compiler to derive serde on all messages.
Defined new feature,
serde, that guards serde features.How Has This Been Tested?
Run tests
Breaking Changes
serdefeature introduced totenderdash-proto(disabled by default).Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
serdefeature for enhanced serialization support.serializersandtime.Bug Fixes
TimestampandDurationstructs.Tests
ConsensusParams.