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

refactor: common type aliases in alias.rs #1535

Merged
merged 1 commit into from
Jul 24, 2024
Merged

refactor: common type aliases in alias.rs #1535

merged 1 commit into from
Jul 24, 2024

Conversation

dinhani-cw
Copy link
Contributor

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

PR Type

Enhancement


Description

  • Introduced a new module alias.rs to define common type aliases for external crates such as Serde, Ethers, and REVM.
  • Replaced direct imports of external crate types with the newly defined aliases across multiple modules.
  • Improved code readability and maintainability by using type aliases.

Changes walkthrough 📝

Relevant files
Enhancement
24 files
alias.rs
Introduce common type aliases for external crates               

src/alias.rs

  • Added type aliases for external crate types.
  • Included aliases for Serde, Ethers, and REVM types.
  • +35/-0   
    evm.rs
    Use type aliases for REVM types in EVM executor                   

    src/eth/executor/evm.rs

    • Replaced direct imports of REVM types with aliases.
    +2/-2     
    account.rs
    Use type aliases for REVM types in account module               

    src/eth/primitives/account.rs

    • Replaced direct imports of REVM types with aliases.
    +2/-2     
    address.rs
    Use type aliases for REVM types in address module               

    src/eth/primitives/address.rs

    • Replaced direct imports of REVM types with aliases.
    +2/-2     
    block.rs
    Use type aliases for Ethers types in block module               

    src/eth/primitives/block.rs

    • Replaced direct imports of Ethers types with aliases.
    +9/-8     
    block_header.rs
    Use type aliases for Ethers types in block header module 

    src/eth/primitives/block_header.rs

    • Replaced direct imports of Ethers types with aliases.
    +4/-2     
    block_number.rs
    Use type aliases for REVM types in block number module     

    src/eth/primitives/block_number.rs

    • Replaced direct imports of REVM types with aliases.
    +1/-1     
    bytes.rs
    Use type aliases for Ethers and REVM types in bytes module

    src/eth/primitives/bytes.rs

    • Replaced direct imports of Ethers and REVM types with aliases.
    +4/-4     
    external_block.rs
    Use type aliases for Ethers types in external block module

    src/eth/primitives/external_block.rs

    • Replaced direct imports of Ethers types with aliases.
    +7/-7     
    external_receipt.rs
    Use type aliases for Ethers types in external receipt module

    src/eth/primitives/external_receipt.rs

    • Replaced direct imports of Ethers types with aliases.
    +1/-1     
    external_transaction.rs
    Use type aliases for Ethers types in external transaction module

    src/eth/primitives/external_transaction.rs

    • Replaced direct imports of Ethers types with aliases.
    +1/-2     
    log.rs
    Use type aliases for Ethers and REVM types in log module 

    src/eth/primitives/log.rs

    • Replaced direct imports of Ethers and REVM types with aliases.
    +2/-3     
    log_mined.rs
    Use type aliases for Ethers types in log mined module       

    src/eth/primitives/log_mined.rs

    • Replaced direct imports of Ethers types with aliases.
    +1/-1     
    log_topic.rs
    Use type aliases for REVM types in log topic module           

    src/eth/primitives/log_topic.rs

    • Replaced direct imports of REVM types with aliases.
    +1/-1     
    slot_index.rs
    Use type aliases for REVM types in slot index module         

    src/eth/primitives/slot_index.rs

    • Replaced direct imports of REVM types with aliases.
    +1/-1     
    slot_value.rs
    Use type aliases for REVM types in slot value module         

    src/eth/primitives/slot_value.rs

    • Replaced direct imports of REVM types with aliases.
    +1/-1     
    transaction_input.rs
    Use type aliases for Ethers types in transaction input module

    src/eth/primitives/transaction_input.rs

    • Replaced direct imports of Ethers types with aliases.
    +1/-1     
    transaction_mined.rs
    Use type aliases for Ethers types in transaction mined module

    src/eth/primitives/transaction_mined.rs

    • Replaced direct imports of Ethers types with aliases.
    +2/-2     
    transaction_stage.rs
    Use type aliases for Ethers types in transaction stage module

    src/eth/primitives/transaction_stage.rs

    • Replaced direct imports of Ethers types with aliases.
    +2/-3     
    unix_time.rs
    Use type aliases for REVM types in unix time module           

    src/eth/primitives/unix_time.rs

    • Replaced direct imports of REVM types with aliases.
    +2/-1     
    wei.rs
    Use type aliases for REVM types in wei module                       

    src/eth/primitives/wei.rs

    • Replaced direct imports of REVM types with aliases.
    +1/-1     
    blockchain_client.rs
    Use type aliases for Ethers types in blockchain client     

    src/infra/blockchain_client/blockchain_client.rs

    • Replaced direct imports of Ethers types with aliases.
    +5/-5     
    pending_transaction.rs
    Use type aliases for Ethers types in pending transaction 

    src/infra/blockchain_client/pending_transaction.rs

    • Replaced direct imports of Ethers types with aliases.
    +2/-2     
    lib.rs
    Add alias module to library                                                           

    src/lib.rs

    • Added new module for type aliases.
    +1/-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 24, 2024 14:32
    Copy link

    PR Reviewer Guide 🔍

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

    Logging
    Ensure that the fetch_transaction and send_raw_transaction functions properly log the operations and any errors that occur. Consider adding more detailed tracing fields to capture dynamic information.

    Logging
    Ensure that the PendingTxState::GettingTx state properly logs the operations and any errors that occur. Consider adding more detailed tracing fields to capture dynamic information.

    Copy link

    github-actions bot commented Jul 24, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Handle potential errors in the conversion from BlockHeader to SubscriptionMessage

    The impl From for SubscriptionMessage implementation should use expect_infallible
    only if it is guaranteed that the conversion will never fail. Otherwise, consider
    handling the error appropriately.

    src/eth/primitives/block_header.rs [235-239]

     impl From<BlockHeader> for SubscriptionMessage {
         fn from(value: BlockHeader) -> Self {
             let ethers_block = EthersBlockVoid::from(value);
    -        Self::from_json(&ethers_block).expect_infallible()
    +        Self::from_json(&ethers_block).unwrap_or_else(|e| {
    +            tracing::error!("Failed to convert BlockHeader to SubscriptionMessage: {:?}", e);
    +            Default::default()
    +        })
         }
     }
     
    Suggestion importance[1-10]: 9

    Why: This suggestion addresses a potential runtime issue by adding error handling, which improves the robustness and reliability of the code.

    9
    Maintainability
    Alias the new type to maintain consistency with the previous code

    Consider using use crate::alias::EthersTransaction as Transaction; to maintain
    consistency with the previous code and avoid confusion with the original Transaction
    type from ethers_core.

    src/infra/blockchain_client/pending_transaction.rs [16]

    -use crate::alias::EthersTransaction;
    +use crate::alias::EthersTransaction as Transaction;
     
    Suggestion importance[1-10]: 9

    Why: This suggestion maintains consistency with the previous code and avoids confusion with the original Transaction type from ethers_core, which improves code readability and maintainability.

    9
    Add a comment explaining the purpose of the new type alias

    Add a brief comment explaining the purpose of the EthersTransaction alias to improve
    code readability and maintainability.

    src/infra/blockchain_client/pending_transaction.rs [16]

    +// Alias for ethers_core::types::Transaction
     use crate::alias::EthersTransaction;
     
    Suggestion importance[1-10]: 8

    Why: Adding a comment explaining the purpose of the EthersTransaction alias improves code readability and maintainability by providing context for future developers.

    8
    Add a log statement to capture the result of the HTTP request in fetch_transaction

    In the fetch_transaction method, consider adding a log statement to capture the
    result of the HTTP request for better debugging and traceability.

    src/infra/blockchain_client/blockchain_client.rs [162-171]

     pub async fn fetch_transaction(&self, tx_hash: Hash) -> anyhow::Result<Option<EthersTransaction>> {
         tracing::debug!(%tx_hash, "fetching transaction");
     
         let hash = to_json_value(tx_hash);
     
         let result = self
             .http
             .request::<Option<EthersTransaction>, Vec<JsonValue>>("eth_getTransactionByHash", vec![hash])
             .await;
     
    +    tracing::debug!(?result, "result of eth_getTransactionByHash");
    +
         match result {
     
    Suggestion importance[1-10]: 6

    Why: Adding a log statement enhances debugging and traceability, which is beneficial for maintainability, but it is not a critical improvement.

    6
    Enhancement
    Directly initialize the struct instead of using an intermediate variable

    The impl From for EthersBlockEthersTransaction implementation should directly
    initialize the EthersBlockEthersTransaction struct instead of first creating an
    intermediate variable ethers_block.

    src/eth/primitives/block.rs [165-169]

     impl From<Block> for EthersBlockEthersTransaction {
         fn from(block: Block) -> Self {
    -        let ethers_block = EthersBlockEthersTransaction::from(block.header.clone());
             let ethers_block_transactions: Vec<EthersTransaction> = block.transactions.clone().into_iter().map_into().collect();
             Self {
                 transactions: ethers_block_transactions,
     
    Suggestion importance[1-10]: 7

    Why: The suggestion improves code readability and efficiency by removing an unnecessary intermediate variable, but it does not address a critical issue.

    7
    Best practice
    Place the new module in an appropriate order within the module declarations

    Ensure that the alias module is placed in an appropriate order within the module
    declarations to maintain readability and consistency.

    src/lib.rs [1]

    +pub mod config;
     pub mod alias;
    -pub mod config;
     
    Suggestion importance[1-10]: 7

    Why: Ensuring the alias module is placed in an appropriate order within the module declarations maintains readability and consistency, but it is a minor improvement.

    7

    @dinhani-cw dinhani-cw merged commit e9ddf5d into main Jul 24, 2024
    33 checks passed
    @dinhani-cw dinhani-cw deleted the type-alias branch July 24, 2024 14:36
    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