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

temporary commit: add importer-offline storage #1884

Closed
wants to merge 0 commits into from

Conversation

marcospb19-cw
Copy link
Contributor

@marcospb19-cw marcospb19-cw commented Nov 25, 2024

PR Type

Enhancement


Description

  • Introduced a new ImporterOfflineStorage implementation for the Storage trait.
  • Refactored multiple components (EVM, Executor, Miner, Importer, RPC) to use Arc<dyn Storage> instead of Arc<StratusStorage>.
  • Updated Storage trait to be Send + Sync + 'static.
  • Modified StorageConfig to support initializing different storage implementations.
  • Added ImporterOfflineStorage as a new variant to the StorageKind enum.
  • Updated various config structs and methods to work with the new dynamic storage trait.

Changes walkthrough 📝

Relevant files
Enhancement
13 files
evm.rs
Refactor EVM to use dynamic Storage trait                               

src/eth/executor/evm.rs

  • Changed StratusStorage to dyn Storage in Evm::new and RevmSession::new
    methods
  • Updated storage field type in RevmSession struct to Arc
  • +3/-3     
    executor.rs
    Update Executor to use dynamic Storage trait                         

    src/eth/executor/executor.rs

  • Updated storage parameter type from Arc to Arc in
    Evms::spawn and evm_loop functions
  • Changed storage field type in Executor struct to Arc
  • Modified Executor::new method to accept Arc instead of
    Arc
  • +4/-4     
    executor_config.rs
    Refactor ExecutorConfig to use dynamic Storage                     

    src/eth/executor/executor_config.rs

  • Added import for Storage trait
  • Modified ExecutorConfig::init method to accept Arc
    instead of Arc
  • +2/-1     
    importer.rs
    Update Importer to use dynamic Storage trait                         

    src/eth/follower/importer/importer.rs

  • Changed storage field type in Importer struct to Arc
  • Updated Importer::new method to accept Arc instead of Arc

  • +2/-2     
    importer_config.rs
    Refactor ImporterConfig to use dynamic Storage                     

    src/eth/follower/importer/importer_config.rs

  • Added import for Storage trait
  • Modified ImporterConfig::init and init_follower methods to accept
    Arc instead of Arc
  • +3/-2     
    miner.rs
    Update Miner to use dynamic Storage trait                               

    src/eth/miner/miner.rs

  • Changed storage field type in Miner struct to Arc
  • Updated Miner::new method to accept Arc instead of Arc
  • +2/-2     
    miner_config.rs
    Refactor MinerConfig to use dynamic Storage                           

    src/eth/miner/miner_config.rs

  • Added import for Storage trait
  • Modified MinerConfig::init and init_with_mode methods to accept
    Arc instead of Arc
  • +3/-2     
    log_filter.rs
    Update log filter to use dynamic Storage                                 

    src/eth/primitives/log_filter.rs

  • Added import for Storage trait
  • Modified build_filter function to use Arc instead of Arc
  • +3/-2     
    log_filter_input.rs
    Refactor LogFilterInput to use dynamic Storage                     

    src/eth/primitives/log_filter_input.rs

    • Changed parse method parameter type from &Arc<StratusStorage> to &Arc<dyn Storage>
    +1/-1     
    rpc_context.rs
    Update RpcContext to use dynamic Storage trait                     

    src/eth/rpc/rpc_context.rs

  • Added import for Storage trait
  • Changed storage field type in RpcContext struct to Arc
  • +2/-1     
    rpc_server.rs
    Refactor RPC server to use dynamic Storage                             

    src/eth/rpc/rpc_server.rs

    • Modified serve_rpc function to accept Arc<dyn Storage> instead of Arc<StratusStorage>
    +1/-1     
    importer_offline_storage.rs
    Implement ImporterOfflineStorage                                                 

    src/eth/storage/importer_offline_storage.rs

  • Added new file implementing ImporterOfflineStorage struct
  • Implemented Storage trait for ImporterOfflineStorage
  • Added methods for block number management, account and slot
    operations, and block operations
  • +546/-0 
    mod.rs
    Update Storage module with new ImporterOfflineStorage       

    src/eth/storage/mod.rs

  • Added importer_offline_storage module
  • Modified Storage trait to be Send + Sync + 'static
  • Updated StorageConfig::init to return Arc
  • Added ImporterOfflineStorage variant to StorageKind enum
  • +12/-5   

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Potential Performance Issue
    The new Storage trait is now Send + Sync + 'static, which might impact performance due to dynamic dispatch. Consider evaluating the performance impact of this change.

    Error Handling
    The save_block_batch method doesn't use the same error handling pattern as other methods. Consider using the timed wrapper and proper error logging for consistency.

    Logging Improvement
    Some methods in the ImporterOfflineStorage implementation use tracing::debug! for important operations. Consider using tracing::info! for significant state changes or important operations.

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Properly handle the new ImporterOfflineStorage variant in the storage initialization

    The ImporterOfflineStorage variant is added to the StorageKind enum, but the init
    method doesn't properly handle this new variant. Update the init method to create an
    ImporterOfflineStorage instance when this variant is selected.

    src/eth/storage/mod.rs [128-131]

     Ok(match self.storage_kind {
         StorageKind::StratusStorage => Arc::new(StratusStorage::new(temp_storage, perm_storage)?),
    -    StorageKind::ImporterOfflineStorage => Arc::new(StratusStorage::new(temp_storage, perm_storage)?),
    +    StorageKind::ImporterOfflineStorage => Arc::new(importer_offline_storage::ImporterOfflineStorage::new(temp_storage, perm_storage)?),
     })
    Suggestion importance[1-10]: 9

    Why: The suggestion correctly identifies that the new ImporterOfflineStorage variant is not properly handled in the init method. It proposes a fix that creates the appropriate storage type based on the selected variant, which is crucial for the correct functioning of the new storage option.

    9

    @marcospb19-cw marcospb19-cw force-pushed the feat-add-importer-offline-storage branch from 1687ca4 to f63fc5c Compare November 25, 2024 16:40
    @marcospb19-cw marcospb19-cw deleted the feat-add-importer-offline-storage branch December 5, 2024 04:50
    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