Skip to content
New issue

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

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

Already on GitHub? # to your account

fix: infer missing type field for transactions where v is 0x0 or 0x1 #2044

Merged
merged 2 commits into from
Mar 11, 2025

Conversation

carneiro-cw
Copy link
Contributor

@carneiro-cw carneiro-cw commented Mar 11, 2025

PR Type

Enhancement, Bug fix


Description

  • Infer missing type field for transactions

  • Add type 2 inference for v values 0x0/0x1

  • Implement new test cases for type inference

  • Add serde_path_to_error dependency


Changes walkthrough 📝

Relevant files
Enhancement
external_transaction.rs
Enhance transaction type inference and add tests                 

src/eth/primitives/external_transaction.rs

  • Infer type 2 transactions when v is 0x0 or 0x1
  • Add logic to insert "type" field if missing
  • Implement new test cases for type 2 inference
  • Test both v=0x0 and v=0x1 scenarios
  • +51/-0   
    Dependencies
    Cargo.toml
    Add new dependency for error handling                                       

    Cargo.toml

    • Add serde_path_to_error dependency version 0.1.17
    +1/-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.
  • Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Error Handling

    The new code for inferring transaction type doesn't handle potential errors or edge cases. Consider adding error handling for unexpected 'v' values or missing fields.

    if let Some(Value::String(v_value)) = map.get("v") {
        if (v_value == "0x0" || v_value == "0x1") && !map.contains_key("type") {
            map.insert("type".to_string(), Value::String("0x2".to_string()));
        }
    }

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Handle non-string "v" field values

    The current implementation assumes that the "v" field will always be a string.
    Consider handling cases where the "v" field might be of a different type to prevent
    potential runtime errors.

    src/eth/primitives/external_transaction.rs [37-41]

    -if let Some(Value::String(v_value)) = map.get("v") {
    -    if (v_value == "0x0" || v_value == "0x1") && !map.contains_key("type") {
    -        map.insert("type".to_string(), Value::String("0x2".to_string()));
    +match map.get("v") {
    +    Some(Value::String(v_value)) if v_value == "0x0" || v_value == "0x1" => {
    +        if !map.contains_key("type") {
    +            map.insert("type".to_string(), Value::String("0x2".to_string()));
    +        }
         }
    +    Some(Value::Number(v_value)) if v_value == 0 || v_value == 1 => {
    +        if !map.contains_key("type") {
    +            map.insert("type".to_string(), Value::String("0x2".to_string()));
    +        }
    +    }
    +    _ => {}
     }
    Suggestion importance[1-10]: 8

    __

    Why: This suggestion significantly improves the robustness of the code by handling both string and number types for the "v" field. It prevents potential runtime errors and makes the code more flexible in handling different input formats.

    Medium
    Possible issue
    Handle potential map insertion errors

    Consider handling potential errors when inserting the "type" field into the map. The
    current implementation assumes the insertion will always succeed, which may not be
    the case if the map has reached its capacity or if there's a memory allocation
    failure.

    src/eth/primitives/external_transaction.rs [37-41]

     if let Some(Value::String(v_value)) = map.get("v") {
         if (v_value == "0x0" || v_value == "0x1") && !map.contains_key("type") {
    -        map.insert("type".to_string(), Value::String("0x2".to_string()));
    +        map.insert("type".to_string(), Value::String("0x2".to_string()))
    +            .map_err(|e| D::Error::custom(format!("Failed to insert 'type' field: {}", e)))?;
         }
     }
    Suggestion importance[1-10]: 7

    __

    Why: The suggestion addresses a potential error case that was not handled in the original code. It improves error handling and provides more robust code by mapping insertion errors to custom deserialization errors.

    Medium

    @carneiro-cw
    Copy link
    Contributor Author

    Before moving to alloy we used this function to do this.

    @carneiro-cw carneiro-cw enabled auto-merge (squash) March 11, 2025 13:14
    @carneiro-cw carneiro-cw merged commit 6a66101 into main Mar 11, 2025
    40 checks passed
    @carneiro-cw carneiro-cw deleted the fix_infer_missing_type branch March 11, 2025 13:28
    @gabriel-aranha-cw
    Copy link
    Contributor

    Final benchmark:
    Run ID: bench-1868325566

    Git Info:

    Configuration:

    • Target Account Strategy: Default

    Leader Stats:
    RPS Stats: Max: 1999.00, Min: 1234.00, Avg: 1844.54, StdDev: 75.02
    TPS Stats: Max: 1992.00, Min: 1633.00, Avg: 1790.43, StdDev: 85.61

    Follower Stats:
    Imported Blocks/s: Max: 2.00, Min: 1.00, Avg: 1.60, StdDev: 0.49
    Imported Transactions/s: Max: 3955.00, Min: 1134.00, Avg: 2949.23, StdDev: 909.15

    Plots:

    # for free to join this conversation on GitHub. Already have an account? # to comment
    Labels
    None yet
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    3 participants