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

feat: importer as component #1567

Closed

Conversation

gabriel-aranha-cw
Copy link
Contributor

@gabriel-aranha-cw gabriel-aranha-cw commented Jul 30, 2024

PR Type

Enhancement, Configuration changes


Description

  • Removed StratusMode enum and related logic from the configuration.
  • Added init_with_config methods to ImporterConfig, MinerConfig, and BlockchainClient for initialization with StratusConfig.
  • Updated ConfigChecks implementation to include checks for MinerMode based on the presence of ImporterConfig.
  • Refactored main and run functions to use the new initialization methods.
  • Updated justfile commands to remove --mode flag and use --block-mode instead.

Changes walkthrough 📝

Relevant files
Enhancement
config.rs
Refactor configuration to remove StratusMode and add MinerMode checks

src/config.rs

  • Removed StratusMode enum and related logic.
  • Added checks for MinerMode based on the presence of ImporterConfig.
  • Updated ConfigChecks implementation to reflect new logic.
  • +11/-40 
    importer_config.rs
    Add `init_with_config` method to `ImporterConfig`               

    src/eth/importer/importer_config.rs

  • Added init_with_config method to initialize ImporterConfig with
    StratusConfig.
  • +23/-0   
    miner_config.rs
    Add `init_with_config` method to `MinerConfig`                     

    src/eth/miner/miner_config.rs

  • Added init_with_config method to initialize MinerConfig with
    StratusConfig.
  • Updated MinerMode enum to derive PartialEq.
  • +10/-1   
    blockchain_client.rs
    Add `init_with_config` method to `BlockchainClient`           

    src/infra/blockchain_client/blockchain_client.rs

  • Added init_with_config method to initialize BlockchainClient with
    StratusConfig.
  • +18/-0   
    main.rs
    Refactor main initialization logic to use new config methods

    src/main.rs

  • Refactored main and run functions to remove StratusMode logic.
  • Updated initialization logic to use new init_with_config methods.
  • +4/-37   
    Configuration changes
    justfile
    Update justfile commands to reflect new configuration       

    justfile

  • Updated commands to remove --mode flag and use --block-mode instead.
  • +2/-2     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link

    PR Reviewer Guide 🔍

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

    Missing Logging
    The initialization of core components such as miner, executor, chain, and importer in the run function should be logged with all relevant configurations to ensure proper monitoring and debugging.

    Use spawn_named
    Consider using spawn_named instead of tokio::spawn for better traceability and monitoring of background tasks.

    Missing Error Logging
    When returning an error in the init_with_config method, ensure the original error is logged in a field called reason.

    Missing Error Logging
    When returning an error in the init_with_config method, ensure the original error is logged in a field called reason.

    Copy link

    github-actions bot commented Jul 30, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add a check to ensure self.miner is not None before accessing self.miner.block_mode

    To avoid potential runtime errors, consider adding a check to ensure that self.miner
    is not None before accessing self.miner.block_mode.

    src/config.rs [526-535]

     if self.importer.is_some() {
         // When importer is set, miner mode must be external.
    -    if self.miner.block_mode != MinerMode::External {
    -        return Err(anyhow::anyhow!("miner mode must be external when importer config is set"));
    +    if let Some(miner) = &self.miner {
    +        if miner.block_mode != MinerMode::External {
    +            return Err(anyhow::anyhow!("miner mode must be external when importer config is set"));
    +        }
         }
     } else {
         // When importer is not set, miner mode cannot be external.
    -    if self.miner.block_mode == MinerMode::External {
    -        return Err(anyhow::anyhow!("miner mode must not be external when importer config is not set"));
    +    if let Some(miner) = &self.miner {
    +        if miner.block_mode == MinerMode::External {
    +            return Err(anyhow::anyhow!("miner mode must not be external when importer config is not set"));
    +        }
         }
     }
     
    Suggestion importance[1-10]: 9

    Why: This suggestion addresses a potential runtime error by ensuring that self.miner is not None before accessing its block_mode property, which is crucial for the stability of the code.

    9
    Add a check to handle the case where BlockchainClient::init_with_config returns None

    To ensure that the chain variable is properly initialized, consider adding a check
    to handle the case where BlockchainClient::init_with_config returns None.

    src/main.rs [23]

    -let chain = BlockchainClient::init_with_config(&config).await?;
    +let chain = BlockchainClient::init_with_config(&config).await?.ok_or_else(|| anyhow::anyhow!("BlockchainClient initialization failed"))?;
     
    Suggestion importance[1-10]: 9

    Why: This suggestion addresses a potential issue where BlockchainClient::init_with_config might return None, ensuring that the chain variable is properly initialized and preventing possible runtime errors.

    9
    Enhancement
    Simplify error handling in the init_with_config function using the ? operator

    To improve readability and maintainability, consider using the ? operator to
    simplify error handling in the init_with_config function.

    src/eth/importer/importer_config.rs [43-55]

     if let Some(importer_config) = config.importer.as_ref() {
    -    if let Some(chain) = chain {
    -        return Ok(Some(importer_config.init(
    -            Arc::clone(executor),
    -            Arc::clone(miner),
    -            Arc::clone(storage),
    -            chain,
    -        )?));
    -    } else {
    -        return Err(anyhow::anyhow!("chain is not initialized"));
    -    }
    +    let chain = chain.ok_or_else(|| anyhow::anyhow!("chain is not initialized"))?;
    +    return Ok(Some(importer_config.init(
    +        Arc::clone(executor),
    +        Arc::clone(miner),
    +        Arc::clone(storage),
    +        chain,
    +    )?));
     }
     Ok(None)
     
    Suggestion importance[1-10]: 8

    Why: This suggestion improves code readability and maintainability by simplifying error handling using the ? operator, making the code more concise and easier to understand.

    8
    Best practice
    Rename the init_with_config function to init_with_stratus_config for better clarity

    To improve code readability, consider renaming the init_with_config function to
    init_with_stratus_config to better reflect its purpose.

    src/eth/miner/miner_config.rs [25-31]

    -pub fn init_with_config(&self, config: &StratusConfig, storage: Arc<StratusStorage>) -> anyhow::Result<Arc<Miner>> {
    +pub fn init_with_stratus_config(&self, config: &StratusConfig, storage: Arc<StratusStorage>) -> anyhow::Result<Arc<Miner>> {
         if config.importer.is_some() {
             self.init_external_mode(storage)
         } else {
             self.init(storage)
         }
     }
     
    Suggestion importance[1-10]: 6

    Why: This suggestion improves code readability by providing a more descriptive function name, but it is not crucial for the functionality of the code.

    6

    @gabriel-aranha-cw gabriel-aranha-cw deleted the importer-component-no-mode branch July 30, 2024 18:27
    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