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: move to alloy pt3 #1989

Merged
merged 7 commits into from
Jan 30, 2025
Merged

chore: move to alloy pt3 #1989

merged 7 commits into from
Jan 30, 2025

Conversation

gabriel-aranha-cw
Copy link
Contributor

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

PR Type

Enhancement


Description

  • Replace Ethers types with Alloy types

  • Update block and header conversions

  • Remove unused Ethers-related imports

  • Add new Alloy type aliases


Changes walkthrough 📝

Relevant files
Enhancement
alias.rs
Update type aliases for Alloy migration                                   

src/alias.rs

  • Added AlloyBlockEthersTransaction type alias
  • Removed EthersBlockVoid and EthersBlockEthersTransaction type aliases
  • Kept EthersBlockExternalTransaction and EthersTransaction aliases
  • +1/-2     
    block.rs
    Refactor Block struct to use Alloy types                                 

    src/eth/primitives/block.rs

  • Changed EthersBlockEthersTransaction to AlloyBlockEthersTransaction
  • Updated to_json_rpc_with_full_transactions method
  • Modified From implementation for AlloyBlockEthersTransaction
  • +9/-8     
    block_header.rs
    Migrate BlockHeader to use Alloy types                                     

    src/eth/primitives/block_header.rs

  • Removed Ethers-related imports and type aliases
  • Deleted From implementation for EthersBlock
  • Updated From implementation for SubscriptionMessage
  • Added AlloyBlockVoid import
  • +3/-59   

    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
    Copy link
    Contributor Author

    /benchmark

    Copy link

    github-actions bot commented Jan 30, 2025

    PR Reviewer Guide 🔍

    (Review updated until commit 1c65966)

    Here are some key observations to aid the review process:

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

    Potential Performance Issue

    The to_json_rpc_with_full_transactions method creates a new AlloyBlockEthersTransaction and then immediately converts it to JSON. Consider optimizing this process to avoid unnecessary object creation.

    pub fn to_json_rpc_with_full_transactions(self) -> JsonValue {
        let alloy_block: AlloyBlockEthersTransaction = self.into();
        to_json_value(alloy_block)
    }
    Unused Import

    The Bytes import from crate::eth::primitives::Bytes appears to be unused in this file. Consider removing it if it's not needed.

    use crate::eth::primitives::Bytes;

    Copy link

    github-actions bot commented Jan 30, 2025

    PR Code Suggestions ✨

    Latest suggestions up to 1c65966
    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    General
    Use consistent transaction types

    Consider using alloy_rpc_types_eth::Transaction instead of
    ethers_core::types::Transaction for consistency with the Alloy library.

    src/eth/primitives/block.rs [18]

    -pub type AlloyBlockEthersTransaction = alloy_rpc_types_eth::Block<ethers_core::types::Transaction>;
    +pub type AlloyBlockAlloyTransaction = alloy_rpc_types_eth::Block<alloy_rpc_types_eth::Transaction>;
    Suggestion importance[1-10]: 7

    Why: The suggestion promotes consistency by using Alloy types throughout, which can improve code clarity and reduce potential confusion. However, it's not a critical change, hence the moderate score.

    7
    Consistent transaction type usage

    Consider updating the transactions field to use alloy_rpc_types_eth::Transaction
    instead of EthersTransaction for consistency.

    src/eth/primitives/block.rs [133]

    -let transactions: Vec<EthersTransaction> = block.transactions.into_iter().map_into().collect();
    +let transactions: Vec<alloy_rpc_types_eth::Transaction> = block.transactions.into_iter().map_into().collect();
    Suggestion importance[1-10]: 6

    Why: This suggestion aligns with the first one, promoting consistency in transaction types. It's a valid improvement for code coherence, but not a critical change, thus the moderate score.

    6

    Previous suggestions

    Suggestions up to commit 1c65966
    CategorySuggestion                                                                                                                                    Score
    General
    Use consistent Alloy types

    Consider using alloy_rpc_types_eth::Transaction instead of
    ethers_core::types::Transaction for consistency with the Alloy types being used
    elsewhere in the codebase.

    src/eth/primitives/block.rs [18]

    -pub type AlloyBlockEthersTransaction = alloy_rpc_types_eth::Block<ethers_core::types::Transaction>;
    +pub type AlloyBlockAlloyTransaction = alloy_rpc_types_eth::Block<alloy_rpc_types_eth::Transaction>;
    Suggestion importance[1-10]: 7

    Why: The suggestion promotes consistency by using Alloy types throughout the codebase, which can improve maintainability and reduce potential confusion. However, it's not addressing a critical issue, hence the moderate score.

    7
    Consistently use Alloy types

    Consider updating the From implementation for AlloyBlockEthersTransaction to use
    Alloy types consistently, replacing EthersTransaction with AlloyTransaction.

    src/eth/primitives/block.rs [130-139]

    -impl From<Block> for AlloyBlockEthersTransaction {
    +impl From<Block> for AlloyBlockAlloyTransaction {
         fn from(block: Block) -> Self {
    -        let alloy_block: AlloyBlockEthersTransaction = block.header.into();
    -        let transactions: Vec<EthersTransaction> = block.transactions.into_iter().map_into().collect();
    +        let alloy_block: AlloyBlockAlloyTransaction = block.header.into();
    +        let transactions: Vec<AlloyTransaction> = block.transactions.into_iter().map_into().collect();
     
             Self {
                 transactions: BlockTransactions::Full(transactions),
                 ..alloy_block
             }
         }
     }
    Suggestion importance[1-10]: 6

    Why: This suggestion aims to improve consistency by using Alloy types throughout the implementation. While it's a valid suggestion for code consistency, it doesn't address a critical issue or bug, hence the moderate score.

    6

    @gabriel-aranha-cw gabriel-aranha-cw marked this pull request as ready for review January 30, 2025 12:34
    Copy link

    Persistent review updated to latest commit 1c65966

    @gabriel-aranha-cw
    Copy link
    Contributor Author

    Benchmark:
    Run ID: bench-2800059263

    Git Info:

    Configuration:

    • Target Account Strategy: Default

    RPS Stats: Max: 1903.00, Min: 1182.00, Avg: 1702.09, StdDev: 74.77
    TPS Stats: Max: 1897.00, Min: 1540.00, Avg: 1652.06, StdDev: 87.69

    Plot: View Plot

    @gabriel-aranha-cw gabriel-aranha-cw enabled auto-merge (squash) January 30, 2025 12:41
    @gabriel-aranha-cw
    Copy link
    Contributor Author

    gabriel-aranha-cw commented Jan 30, 2025

    #1963

    @gabriel-aranha-cw gabriel-aranha-cw enabled auto-merge (squash) January 30, 2025 19:37
    @gabriel-aranha-cw gabriel-aranha-cw merged commit f2a5195 into main Jan 30, 2025
    36 of 37 checks passed
    @gabriel-aranha-cw gabriel-aranha-cw deleted the chore-move-to-alloy-pt3 branch January 30, 2025 19:37
    @gabriel-aranha-cw
    Copy link
    Contributor Author

    Final benchmark:
    Run ID: bench-773274308

    Git Info:

    Configuration:

    • Target Account Strategy: Default

    RPS Stats: Max: 1885.00, Min: 869.00, Avg: 1662.22, StdDev: 81.56
    TPS Stats: Max: 1826.00, Min: 1425.00, Avg: 1614.48, StdDev: 83.10

    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