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

chore: implement deserializer for external receipt #1975

Merged
merged 8 commits into from
Jan 28, 2025

Conversation

gabriel-aranha-cw
Copy link
Contributor

@gabriel-aranha-cw gabriel-aranha-cw commented Jan 27, 2025

PR Type

Enhancement, Tests


Description

  • Implement custom deserializer for ExternalReceipt

  • Handle both ethers and alloy receipt formats

  • Add effectiveGasPrice and type fields if missing

  • Include unit tests for old and new receipt formats


Changes walkthrough 📝

Relevant files
Enhancement
alias.rs
Remove unused EthersReceipt type alias                                     

src/alias.rs

  • Removed EthersReceipt type alias
+0/-1     
external_receipt.rs
Implement custom ExternalReceipt deserializer with tests 

src/eth/primitives/external_receipt.rs

  • Implemented custom deserializer for ExternalReceipt
  • Added handling for both ethers and alloy receipt formats
  • Included effectiveGasPrice and type fields if missing
  • Added unit tests for old and new receipt deserialization
  • +113/-1 

    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

    github-actions bot commented Jan 27, 2025

    PR Reviewer Guide 🔍

    (Review updated until commit ae43c6d)

    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 custom deserializer for ExternalReceipt might benefit from more robust error handling. Consider adding more specific error messages for different failure scenarios.

    impl<'de> serde::Deserialize<'de> for ExternalReceipt {
        fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
        where
            D: serde::Deserializer<'de>,
        {
            // During migration from ethers to alloy, we need to handle receipts from both libraries.
            // Ethers receipts do not include `effectiveGasPrice` and `type` fields which are
            // required by alloy.
            let mut value = JsonValue::deserialize(deserializer)?;
    
            if let Some(obj) = value.as_object_mut() {
                if !obj.contains_key("effectiveGasPrice") {
                    obj.insert("effectiveGasPrice".to_string(), serde_json::json!("0x0"));
                }
                if !obj.contains_key("type") {
                    obj.insert("type".to_string(), serde_json::json!("0x0"));
                }
            } else {
                return Err(serde::de::Error::custom("ExternalReceipt must be a JSON object, received invalid type"));
            }
    
            let receipt = serde_json::from_value(value).map_err(|e| serde::de::Error::custom(format!("Failed to deserialize ExternalReceipt: {}", e)))?;
    
            Ok(ExternalReceipt(receipt))
        }
    }

    Copy link

    PR Code Suggestions ✨

    @gabriel-aranha-cw gabriel-aranha-cw marked this pull request as ready for review January 27, 2025 19:35
    Copy link

    Persistent review updated to latest commit ae43c6d

    Copy link

    PR Code Suggestions ✨

    @gabriel-aranha-cw
    Copy link
    Contributor Author

    #1963

    @gabriel-aranha-cw gabriel-aranha-cw merged commit 382ed2b into main Jan 28, 2025
    36 checks passed
    @gabriel-aranha-cw gabriel-aranha-cw deleted the chore-impl-deserializer-alloy branch January 28, 2025 11:55
    @gabriel-aranha-cw
    Copy link
    Contributor Author

    Final benchmark:
    Run ID: bench-947880828

    Git Info:

    Configuration:

    • Target Account Strategy: Default

    RPS Stats: Max: 2022.00, Min: 1288.00, Avg: 1834.03, StdDev: 76.15
    TPS Stats: Max: 2039.00, Min: 1624.00, Avg: 1780.39, StdDev: 87.74

    Plot: View Plot

    # 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.

    2 participants