Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

enha: more serde tests #1568

Merged
merged 1 commit into from
Jul 30, 2024
Merged

enha: more serde tests #1568

merged 1 commit into from
Jul 30, 2024

Conversation

dinhani-cw
Copy link
Contributor

@dinhani-cw dinhani-cw commented Jul 30, 2024

PR Type

Enhancement, Tests


Description

  • Enhanced multiple structs (CallInput, EcdsaRs, EcdsaV, EvmExecutionMetrics, DateTimeNow) with additional derives including PartialEq, Eq, fake::Dummy, and serde::Serialize/Deserialize.
  • Implemented Dummy<Faker> for EcdsaRs and EcdsaV.
  • Added gen_test_serde! macros for several structs to generate serde tests.
  • Updated fake dependency in Cargo.toml to include the chrono feature.

Changes walkthrough 📝

Relevant files
Enhancement
call_input.rs
Enhance CallInput struct with additional derives and serde attributes

src/eth/primitives/call_input.rs

  • Added PartialEq, Eq, fake::Dummy, and serde::Serialize derives to
    CallInput.
  • Renamed fields using serde attributes.
  • +6/-3     
    ecdsa_rs.rs
    Enhance EcdsaRs struct with additional derives and dummy
    implementation

    src/eth/primitives/ecdsa_rs.rs

  • Added DebugAsJson, PartialEq, Eq, serde::Serialize, and
    serde::Deserialize derives to EcdsaRs.
  • Implemented Dummy for EcdsaRs.
  • Extended gen_newtype_from macro for additional types.
  • +12/-4   
    ecdsa_v.rs
    Enhance EcdsaV struct with additional derives and dummy implementation

    src/eth/primitives/ecdsa_v.rs

  • Added DebugAsJson, PartialEq, Eq, serde::Serialize, and
    serde::Deserialize derives to EcdsaV.
  • Implemented Dummy for EcdsaV.
  • Extended gen_newtype_from macro for additional types.
  • +12/-4   
    execution_metrics.rs
    Enhance `EvmExecutionMetrics` struct with additional derives

    src/eth/primitives/execution_metrics.rs

  • Added PartialEq, Eq, fake::Dummy, and serde::Deserialize derives to
    EvmExecutionMetrics.
  • +1/-1     
    now.rs
    Enhance `DateTimeNow` struct with additional derives         

    src/eth/primitives/now.rs

  • Added DebugAsJson, PartialEq, Eq, fake::Dummy, and serde::Deserialize
    derives to DateTimeNow.
  • +2/-1     
    Tests
    mod.rs
    Add serde test generation for multiple structs                     

    src/eth/primitives/mod.rs

    • Added gen_test_serde! macros for multiple structs.
    +5/-6     
    Dependencies
    Cargo.toml
    Update `fake` dependency to include `chrono` feature         

    Cargo.toml

    • Added chrono feature to fake dependency.
    +1/-1     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Tracing Fields
    Ensure that any function instrumented with tracing::instrument records identifiers as span fields if none are present.

    Tracing Fields
    Ensure that any function instrumented with tracing::instrument records identifiers as span fields if none are present.

    Tracing Fields
    Ensure that any function instrumented with tracing::instrument records identifiers as span fields if none are present.

    Tracing Fields
    Ensure that any function instrumented with tracing::instrument records identifiers as span fields if none are present.

    Tracing Fields
    Ensure that any function instrumented with tracing::instrument records identifiers as span fields if none are present.

    @dinhani-cw dinhani-cw enabled auto-merge (squash) July 30, 2024 18:11
    Copy link

    github-actions bot commented Jul 30, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Ensure EvmExecutionMetrics can be deserialized with missing fields by adding serde(default) to the derive attributes

    The #[derive(...)] attribute for EvmExecutionMetrics should include serde(default)
    to ensure that deserialization does not fail if any fields are missing, providing
    default values instead.

    src/eth/primitives/execution_metrics.rs [3-4]

    -#[derive(DebugAsJson, Clone, Copy, Default, PartialEq, Eq, derive_more::Add, derive_more::AddAssign, fake::Dummy, serde::Serialize, serde::Deserialize)]
    +#[derive(DebugAsJson, Clone, Copy, Default, PartialEq, Eq, derive_more::Add, derive_more::AddAssign, fake::Dummy, serde::Serialize, serde::Deserialize, serde(default))]
     pub struct EvmExecutionMetrics {
     
    Suggestion importance[1-10]: 9

    Why: Adding serde(default) ensures robustness in deserialization by providing default values for missing fields, which is a best practice for data handling.

    9
    Performance
    Avoid serializing None values for the value field to reduce the serialized output size

    The #[serde(rename = "value", default)] attribute on the value field should include
    skip_serializing_if = "Option::is_none" to avoid serializing None values, which can
    reduce the size of the serialized output.

    src/eth/primitives/call_input.rs [13-14]

    -#[serde(rename = "value", default)]
    +#[serde(rename = "value", default, skip_serializing_if = "Option::is_none")]
     pub value: Wei,
     
    Suggestion importance[1-10]: 8

    Why: This suggestion improves performance by reducing the size of the serialized output when the value field is None, which is a beneficial optimization.

    8
    Enhancement
    Simplify the dummy_with_rng implementation for EcdsaRs by using rand::random to generate the U256 value

    The dummy_with_rng implementation for EcdsaRs can be simplified by using the
    rand::random function to generate the U256 value directly, which makes the code more
    concise and readable.

    src/eth/primitives/ecdsa_rs.rs [19-21]

    -fn dummy_with_rng<R: rand::prelude::Rng + ?Sized>(_: &Faker, rng: &mut R) -> Self {
    -    Self(U256([rng.next_u64(), rng.next_u64(), rng.next_u64(), rng.next_u64()]))
    +fn dummy_with_rng<R: rand::prelude::Rng + ?Sized>(_: &Faker, _: &mut R) -> Self {
    +    Self(rand::random())
     }
     
    Suggestion importance[1-10]: 7

    Why: Simplifying the dummy_with_rng implementation makes the code more concise and readable, which is a good enhancement for maintainability.

    7
    Simplify the dummy_with_rng implementation for EcdsaV by using rand::random to generate the U64 value

    The dummy_with_rng implementation for EcdsaV can be simplified by using the
    rand::random function to generate the U64 value directly, which makes the code more
    concise and readable.

    src/eth/primitives/ecdsa_v.rs [19-20]

    -fn dummy_with_rng<R: rand::prelude::Rng + ?Sized>(_: &Faker, rng: &mut R) -> Self {
    -    Self::from(rng.next_u64())
    +fn dummy_with_rng<R: rand::prelude::Rng + ?Sized>(_: &Faker, _: &mut R) -> Self {
    +    Self::from(rand::random::<u64>())
     }
     
    Suggestion importance[1-10]: 7

    Why: Simplifying the dummy_with_rng implementation makes the code more concise and readable, which is a good enhancement for maintainability.

    7

    @dinhani-cw dinhani-cw merged commit 4c6b781 into main Jul 30, 2024
    34 checks passed
    @dinhani-cw dinhani-cw deleted the serde branch July 30, 2024 18:15
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    None yet
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant