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: ensure all primitives implement DebugAsJson #1570

Merged
merged 2 commits into from
Jul 30, 2024
Merged

Conversation

dinhani-cw
Copy link
Contributor

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

PR Type

Enhancement, Tests


Description

  • Implemented DebugAsJson for various Ethereum primitive structs and enums.
  • Removed custom Debug implementations where applicable.
  • Added serde_debug_json test macro to ensure JSON serialization matches Debug output.

Changes walkthrough 📝

Relevant files
Enhancement
23 files
address.rs
Implement `DebugAsJson` for `Address` struct                         

src/eth/primitives/address.rs

  • Added DebugAsJson derive to Address struct.
+2/-1     
block_header.rs
Implement `DebugAsJson` for `BlockHeader` struct                 

src/eth/primitives/block_header.rs

  • Added DebugAsJson derive to BlockHeader struct.
+2/-1     
block_number.rs
Implement `DebugAsJson` for `BlockNumber` struct                 

src/eth/primitives/block_number.rs

  • Added DebugAsJson derive to BlockNumber struct.
+2/-1     
bytes.rs
Implement `DebugAsJson` for `Bytes` struct and remove custom `Debug`

src/eth/primitives/bytes.rs

  • Added DebugAsJson derive to Bytes struct.
  • Removed custom Debug implementation for Bytes.
  • +2/-8     
    call_input.rs
    Implement `DebugAsJson` for `CallInput` struct                     

    src/eth/primitives/call_input.rs

    • Added DebugAsJson derive to CallInput struct.
    +3/-1     
    chain_id.rs
    Implement `DebugAsJson` for `ChainId` struct                         

    src/eth/primitives/chain_id.rs

    • Added DebugAsJson derive to ChainId struct.
    +2/-1     
    code_hash.rs
    Implement `DebugAsJson` for `CodeHash` struct                       

    src/eth/primitives/code_hash.rs

    • Added DebugAsJson derive to CodeHash struct.
    +2/-1     
    difficulty.rs
    Implement `DebugAsJson` for `Difficulty` struct                   

    src/eth/primitives/difficulty.rs

    • Added DebugAsJson derive to Difficulty struct.
    +2/-1     
    execution_result.rs
    Implement `DebugAsJson` for `ExecutionResult` enum             

    src/eth/primitives/execution_result.rs

    • Added DebugAsJson derive to ExecutionResult enum.
    +3/-1     
    execution_value_change.rs
    Implement DebugAsJson for ValueState and Debug for
    ExecutionValueChange

    src/eth/primitives/execution_value_change.rs

  • Added DebugAsJson derive to ValueState enum.
  • Added Debug implementation for ExecutionValueChange.
  • +52/-38 
    gas.rs
    Implement `DebugAsJson` for `Gas` struct                                 

    src/eth/primitives/gas.rs

    • Added DebugAsJson derive to Gas struct.
    +2/-1     
    hash.rs
    Implement `DebugAsJson` for `Hash` struct                               

    src/eth/primitives/hash.rs

    • Added DebugAsJson derive to Hash struct.
    +2/-1     
    index.rs
    Implement `DebugAsJson` for `Index` struct                             

    src/eth/primitives/index.rs

    • Added DebugAsJson derive to Index struct.
    +2/-1     
    log.rs
    Implement `DebugAsJson` for `Log` struct                                 

    src/eth/primitives/log.rs

    • Added DebugAsJson derive to Log struct.
    +3/-1     
    log_mined.rs
    Implement `DebugAsJson` for `LogMined` struct                       

    src/eth/primitives/log_mined.rs

    • Added DebugAsJson derive to LogMined struct.
    +2/-1     
    log_topic.rs
    Implement `DebugAsJson` for `LogTopic` struct                       

    src/eth/primitives/log_topic.rs

    • Added DebugAsJson derive to LogTopic struct.
    +2/-1     
    miner_nonce.rs
    Implement `DebugAsJson` for `MinerNonce` struct                   

    src/eth/primitives/miner_nonce.rs

    • Added DebugAsJson derive to MinerNonce struct.
    +2/-1     
    nonce.rs
    Implement `DebugAsJson` for `Nonce` struct                             

    src/eth/primitives/nonce.rs

    • Added DebugAsJson derive to Nonce struct.
    +2/-9     
    size.rs
    Implement `DebugAsJson` for `Size` struct                               

    src/eth/primitives/size.rs

    • Added DebugAsJson derive to Size struct.
    +2/-1     
    slot_index.rs
    Implement DebugAsJson for SlotIndex struct and remove custom Debug

    src/eth/primitives/slot_index.rs

  • Added DebugAsJson derive to SlotIndex struct.
  • Removed custom Debug implementation for SlotIndex.
  • +2/-8     
    slot_value.rs
    Implement `DebugAsJson` for `SlotValue` struct                     

    src/eth/primitives/slot_value.rs

    • Added DebugAsJson derive to SlotValue struct.
    +2/-1     
    unix_time.rs
    Implement `DebugAsJson` for `UnixTime` struct                       

    src/eth/primitives/unix_time.rs

    • Added DebugAsJson derive to UnixTime struct.
    +2/-1     
    wei.rs
    Implement `DebugAsJson` for `Wei` struct                                 

    src/eth/primitives/wei.rs

    • Added DebugAsJson derive to Wei struct.
    +14/-1   
    Tests
    1 files
    ext.rs
    Add `serde_debug_json` test macro                                               

    src/ext.rs

  • Added serde_debug_json test macro to verify JSON serialization matches
    Debug output.
  • +8/-0     

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

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

    PR Reviewer Guide 🔍

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

    Code Smell
    The Debug implementation for ExecutionValueChange uses to_json_string(self) which may not be efficient. Consider using DebugAsJson directly for consistency and potential performance improvements.

    Test Enhancement
    The new serde_debug_json test macro is a good addition. Ensure that it is applied to all relevant types to maintain consistency in testing JSON serialization and Debug output.

    Copy link

    github-actions bot commented Jul 30, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Testing
    Ensure the gen_test_serde macro includes a test for DebugAsJson output consistency

    The gen_test_serde macro should include a test to verify that the DebugAsJson
    implementation produces the same output as the serde_json serialization.

    src/ext.rs [304-317]

     macro_rules! gen_test_serde {
         ($type:ty) => {
             paste::paste! {
                 #[test]
                 pub fn [<serde_debug_json_ $type:snake>]() {
                     let original = <fake::Faker as fake::Fake>::fake::<$type>(&fake::Faker);
                     let encoded_json = serde_json::to_string(&original).unwrap();
                     let encoded_debug = format!("{:?}", original);
                     assert_eq!(encoded_json, encoded_debug);
                 }
     
                 #[test]
                 pub fn [<serde_json_ $type:snake>]() {
                     // encode
    +                let original = <fake::Faker as fake::Fake>::fake::<$type>(&fake::Faker);
    +                let encoded = serde_json::to_string(&original).unwrap();
    +                let decoded: $type = serde_json::from_str(&encoded).unwrap();
    +                assert_eq!(original, decoded);
    +            }
    +        }
    +    };
    +}
     
    Suggestion importance[1-10]: 9

    Why: This suggestion improves the robustness of the testing framework by verifying that the DebugAsJson implementation produces the same output as the serde_json serialization, which is crucial for consistency.

    9
    Consistency
    Include DebugAsJson trait in the Debug implementation for consistency

    The Debug implementation for ExecutionValueChange should also include the
    DebugAsJson trait to ensure consistency with the other structs and enums in the PR.

    src/eth/primitives/execution_value_change.rs [21-27]

     impl<T> Debug for ExecutionValueChange<T>
     where
    -    T: PartialEq + serde::Serialize,
    +    T: PartialEq + serde::Serialize + DebugAsJson,
     {
         fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
             f.write_str(&to_json_string(self))
         }
     }
     
    Suggestion importance[1-10]: 8

    Why: This suggestion ensures consistency with the other structs and enums in the PR by including the DebugAsJson trait, which is a logical and beneficial improvement.

    8
    Enhancement
    Add a Default implementation for ExecutionValueChange

    Add a Default implementation for ExecutionValueChange to ensure it can be
    instantiated with default values, which is useful for testing and initialization.

    src/eth/primitives/execution_value_change.rs [13-19]

     pub struct ExecutionValueChange<T>
     where
         T: PartialEq + serde::Serialize,
     {
         original: ValueState<T>,
         modified: ValueState<T>,
     }
     
    +impl<T> Default for ExecutionValueChange<T>
    +where
    +    T: PartialEq + serde::Serialize,
    +{
    +    fn default() -> Self {
    +        Self {
    +            original: ValueState::NotSet,
    +            modified: ValueState::NotSet,
    +        }
    +    }
    +}
    +
    Suggestion importance[1-10]: 7

    Why: Adding a Default implementation is useful for testing and initialization, enhancing the usability of the ExecutionValueChange struct.

    7

    @dinhani-cw dinhani-cw merged commit e953835 into main Jul 30, 2024
    33 checks passed
    @dinhani-cw dinhani-cw deleted the test-debug branch July 30, 2024 19:13
    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