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: add snapshot tests for external primitives #2007

Merged
merged 9 commits into from
Feb 11, 2025

Conversation

gabriel-aranha-cw
Copy link
Contributor

@gabriel-aranha-cw gabriel-aranha-cw commented Feb 10, 2025

PR Type

Enhancement, Tests


Description

  • Add Dummy trait implementations for external primitives

  • Implement JSON snapshot tests for primitives

  • Add serde derive for PartialEq on structs

  • Import necessary types and traits


Changes walkthrough 📝

Relevant files
Enhancement
6 files
external_block.rs
Implement Dummy trait for ExternalBlock                                   
+61/-1   
external_block_with_receipts.rs
Implement Dummy trait for ExternalBlockWithReceipts           
+18/-1   
external_receipt.rs
Implement Dummy trait for ExternalReceipt                               
+60/-1   
external_receipts.rs
Implement Dummy trait for ExternalReceipts                             
+16/-1   
external_transaction.rs
Implement Dummy trait for ExternalTransaction                       
+54/-1   
ext.rs
Add gen_test_json macro for JSON snapshot tests                   
+52/-0   
Tests
6 files
mod.rs
Add JSON snapshot tests for primitives                                     
+7/-0     
ExternalBlock.json
Add JSON snapshot for ExternalBlock                                           
+46/-0   
ExternalBlockWithReceipts.json
Add JSON snapshot for ExternalBlockWithReceipts                   
+81/-0   
ExternalReceipt.json
Add JSON snapshot for ExternalReceipt                                       
+31/-0   
ExternalReceipts.json
Add JSON snapshot for ExternalReceipts                                     
+157/-0 
ExternalTransaction.json
Add JSON snapshot for ExternalTransaction                               
+18/-0   

Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • @gabriel-aranha-cw gabriel-aranha-cw changed the title Enha add serde tests external primitives enha: add snapshot tests for external primitives Feb 10, 2025
    Copy link

    github-actions bot commented Feb 10, 2025

    PR Reviewer Guide 🔍

    (Review updated until commit 047bb06)

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Tracing Improvement

    The Dummy implementation for ExternalBlock creates a complex object with many fields. Consider adding tracing to log the creation of this dummy object for better visibility during testing.

    impl Dummy<Faker> for ExternalBlock {
        fn dummy_with_rng<R: rand_core::RngCore + ?Sized>(faker: &Faker, rng: &mut R) -> Self {
            let mut addr_bytes = [0u8; 20];
            let mut hash_bytes = [0u8; 32];
            let mut nonce_bytes = [0u8; 8];
            rng.fill_bytes(&mut addr_bytes);
            rng.fill_bytes(&mut hash_bytes);
            rng.fill_bytes(&mut nonce_bytes);
    
            let transaction: ExternalTransaction = faker.fake_with_rng(rng);
    
            let block = alloy_rpc_types_eth::Block {
                header: alloy_rpc_types_eth::Header {
                    hash: B256::from_slice(&hash_bytes),
                    inner: alloy_consensus::Header {
                        parent_hash: B256::from_slice(&hash_bytes),
                        ommers_hash: B256::from_slice(&hash_bytes),
                        beneficiary: alloy_primitives::Address::from_slice(&addr_bytes),
                        state_root: B256::from_slice(&hash_bytes),
                        transactions_root: B256::from_slice(&hash_bytes),
                        receipts_root: B256::from_slice(&hash_bytes),
                        withdrawals_root: Some(B256::from_slice(&hash_bytes)),
                        number: rng.next_u64(),
                        gas_used: rng.next_u64(),
                        gas_limit: rng.next_u64(),
                        extra_data: Bytes::default(),
                        logs_bloom: Bloom::default(),
                        timestamp: rng.next_u64(),
                        difficulty: U256::from(rng.next_u64()),
                        mix_hash: B256::from_slice(&hash_bytes),
                        nonce: B64::from_slice(&nonce_bytes),
                        base_fee_per_gas: Some(rng.next_u64()),
                        blob_gas_used: None,
                        excess_blob_gas: None,
                        parent_beacon_block_root: None,
                        requests_hash: None,
                    },
                    total_difficulty: Some(U256::from(rng.next_u64())),
                    size: Some(U256::from(rng.next_u64())),
                },
                uncles: vec![B256::from_slice(&hash_bytes)],
                transactions: alloy_rpc_types_eth::BlockTransactions::Full(vec![transaction]),
                withdrawals: Some(Withdrawals::default()),
            };
    
            ExternalBlock(block)
        }
    }
    Potential Performance Concern

    The Dummy implementation for ExternalBlockWithReceipts creates a new ExternalReceipt for each transaction. For large blocks, this could be inefficient. Consider using a fixed number of dummy receipts or a more efficient generation method.

    impl Dummy<Faker> for ExternalBlockWithReceipts {
        fn dummy_with_rng<R: rand_core::RngCore + ?Sized>(faker: &Faker, rng: &mut R) -> Self {
            let block = ExternalBlock::dummy_with_rng(faker, rng);
    
            let receipts = match &block.transactions {
                alloy_rpc_types_eth::BlockTransactions::Full(txs) => txs.iter().map(|_| ExternalReceipt::dummy_with_rng(faker, rng)).collect(),
                alloy_rpc_types_eth::BlockTransactions::Hashes(_) => Vec::new(),
                alloy_rpc_types_eth::BlockTransactions::Uncle => Vec::new(),
            };
    
            Self { block, receipts }
        }
    }

    Copy link

    github-actions bot commented Feb 10, 2025

    PR Code Suggestions ✨

    Latest suggestions up to 047bb06
    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Implement Default for ExternalBlockWithReceipts

    Implement the Default trait for ExternalBlockWithReceipts to provide a default
    instance of the struct.

    src/eth/primitives/external_block_with_receipts.rs [11-15]

    -#[derive(Debug, Clone, PartialEq, Deserialize, Serialize)]
    +#[derive(Debug, Clone, PartialEq, Deserialize, Serialize, Default)]
     pub struct ExternalBlockWithReceipts {
         pub block: ExternalBlock,
         pub receipts: Vec<ExternalReceipt>,
     }
    Suggestion importance[1-10]: 5

    __

    Why: Implementing Default for ExternalBlockWithReceipts could be useful for creating instances with default values, potentially simplifying code in some scenarios. However, it's a minor improvement and not critical.

    Low

    Previous suggestions

    Suggestions up to commit 047bb06
    CategorySuggestion                                                                                                                                    Impact
    General
    Implement Default for ExternalBlockWithReceipts

    Implement the Default trait for ExternalBlockWithReceipts to provide a default
    instance of the struct.

    src/eth/primitives/external_block_with_receipts.rs [11-15]

    -#[derive(Debug, Clone, PartialEq, Deserialize, Serialize)]
    +#[derive(Debug, Clone, PartialEq, Deserialize, Serialize, Default)]
     pub struct ExternalBlockWithReceipts {
         pub block: ExternalBlock,
         pub receipts: Vec<ExternalReceipt>,
     }
    Suggestion importance[1-10]: 5

    __

    Why: Implementing Default can be useful for creating instances with default values, which could simplify testing and initialization. However, it's a minor improvement and not critical for functionality.

    Low
    Implement Eq for ExternalReceipt

    Implement the Eq trait for ExternalReceipt to enable full equality comparisons, as
    it already implements PartialEq.

    src/eth/primitives/external_receipt.rs [17-19]

    -#[derive(Debug, Clone, PartialEq, derive_more::Deref, serde::Serialize)]
    +#[derive(Debug, Clone, PartialEq, Eq, derive_more::Deref, serde::Serialize)]
     #[serde(transparent)]
     pub struct ExternalReceipt(#[deref] pub AlloyReceipt);
    Suggestion importance[1-10]: 4

    __

    Why: Adding Eq trait can be beneficial for full equality comparisons, but it's not a critical change. The impact is relatively low as PartialEq is already implemented, which covers most use cases.

    Low

    @gabriel-aranha-cw gabriel-aranha-cw marked this pull request as ready for review February 10, 2025 13:12
    @gabriel-aranha-cw gabriel-aranha-cw enabled auto-merge (squash) February 10, 2025 13:13
    Copy link

    Persistent review updated to latest commit 047bb06

    @gabriel-aranha-cw
    Copy link
    Contributor Author

    #1977

    Copy link
    Contributor

    @arthurmm-cloudwalk arthurmm-cloudwalk left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Cool macro 👍

    @gabriel-aranha-cw gabriel-aranha-cw merged commit b0504ab into main Feb 11, 2025
    40 checks passed
    @gabriel-aranha-cw gabriel-aranha-cw deleted the enha-add-serde-tests-external-primitives branch February 11, 2025 15:51
    @gabriel-aranha-cw
    Copy link
    Contributor Author

    Final benchmark:
    Run ID: bench-917186674

    Git Info:

    Configuration:

    • Target Account Strategy: Default

    RPS Stats: Max: 2099.00, Min: 1490.00, Avg: 1913.87, StdDev: 82.16
    TPS Stats: Max: 2094.00, Min: 1705.00, Avg: 1857.22, StdDev: 96.05

    Plot: View Plot

    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.

    2 participants