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

refac: put StratusStorage behind a trait #1881

Merged
merged 1 commit into from
Nov 25, 2024

Conversation

marcospb19-cw
Copy link
Contributor

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

PR Type

Enhancement


Description

  • Introduced a new Storage trait in src/eth/storage/mod.rs to define a common interface for storage operations.
  • Implemented the Storage trait for StratusStorage in src/eth/storage/stratus_storage.rs.
  • Added imports for the new Storage trait in multiple files across the project.
  • Moved StratusStorageConfig from stratus_storage.rs to mod.rs.
  • Updated test code in log_filter.rs to use StratusStorage::new_test() instead of creating separate in-memory storages.
  • Removed unnecessary imports and adjusted method visibility in stratus_storage.rs.

Changes walkthrough 📝

Relevant files
Enhancement
10 files
importer_offline.rs
Add Storage trait import                                                                 

src/bin/importer_offline.rs

  • Added import for Storage trait from stratus::eth::storage
+1/-0     
evm.rs
Add Storage trait import                                                                 

src/eth/executor/evm.rs

  • Added import for Storage trait from crate::eth::storage
+1/-0     
executor.rs
Add Storage trait import                                                                 

src/eth/executor/executor.rs

  • Added import for Storage trait from crate::eth::storage
+1/-0     
importer.rs
Add Storage trait import                                                                 

src/eth/follower/importer/importer.rs

  • Added import for Storage trait from crate::eth::storage
+1/-0     
miner.rs
Add Storage trait import                                                                 

src/eth/miner/miner.rs

  • Added import for Storage trait from crate::eth::storage
+1/-0     
log_filter.rs
Update test storage initialization                                             

src/eth/primitives/log_filter.rs

  • Removed imports for InMemoryPermanentStorage and
    InMemoryTemporaryStorage
  • Updated build_filter function to use StratusStorage::new_test()
  • +1/-3     
    log_filter_input.rs
    Add Storage trait import                                                                 

    src/eth/primitives/log_filter_input.rs

    • Added import for Storage trait from crate::eth::storage
    +1/-0     
    rpc_server.rs
    Add Storage trait import                                                                 

    src/eth/rpc/rpc_server.rs

    • Added import for Storage trait from crate::eth::storage
    +1/-0     
    mod.rs
    Introduce Storage trait and move config                                   

    src/eth/storage/mod.rs

  • Introduced Storage trait with method definitions
  • Moved StratusStorageConfig struct and its implementation here
  • Added necessary imports for the new trait and config
  • +113/-5 
    stratus_storage.rs
    Implement Storage trait for StratusStorage                             

    src/eth/storage/stratus_storage.rs

  • Removed StratusStorageConfig and related imports
  • Implemented Storage trait for StratusStorage
  • Moved most public methods to trait implementation
  • +23/-55 

    💡 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: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Trait Implementation
    Verify that the new Storage trait is correctly implemented for StratusStorage and that all required methods are present and properly defined.

    Method Visibility
    Check that the visibility of methods in StratusStorage has been appropriately adjusted to match the new trait implementation.

    Configuration Changes
    Review the changes to StratusStorageConfig and ensure that the configuration is properly set up and initialized.

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    General
    Use a more specific error type for improved error handling and API clarity in the Storage trait

    Consider using a more specific error type instead of StratusError for the Storage
    trait methods. This would provide more precise error handling and improve the API's
    clarity.

    src/eth/storage/mod.rs [39-57]

     pub trait Storage: Sized {
         // -------------------------------------------------------------------------
         // Block number
         // -------------------------------------------------------------------------
     
    -    fn read_block_number_to_resume_import(&self) -> Result<BlockNumber, StratusError>;
    +    fn read_block_number_to_resume_import(&self) -> Result<BlockNumber, StorageError>;
     
    -    fn read_pending_block_header(&self) -> Result<Option<PendingBlockHeader>, StratusError>;
    +    fn read_pending_block_header(&self) -> Result<Option<PendingBlockHeader>, StorageError>;
     
    -    fn read_mined_block_number(&self) -> Result<BlockNumber, StratusError>;
    +    fn read_mined_block_number(&self) -> Result<BlockNumber, StorageError>;
     
    -    fn set_pending_block_number(&self, block_number: BlockNumber) -> Result<(), StratusError>;
    +    fn set_pending_block_number(&self, block_number: BlockNumber) -> Result<(), StorageError>;
     
    -    fn set_pending_block_number_as_next(&self) -> Result<(), StratusError>;
    +    fn set_pending_block_number_as_next(&self) -> Result<(), StorageError>;
     
    -    fn set_pending_block_number_as_next_if_not_set(&self) -> Result<(), StratusError>;
    +    fn set_pending_block_number_as_next_if_not_set(&self) -> Result<(), StorageError>;
     
    -    fn set_mined_block_number(&self, block_number: BlockNumber) -> Result<(), StratusError>;
    +    fn set_mined_block_number(&self, block_number: BlockNumber) -> Result<(), StorageError>;
    Suggestion importance[1-10]: 7

    Why: Using a more specific error type like StorageError instead of StratusError would improve error handling and API clarity. This change enhances type safety and makes it easier for users of the Storage trait to handle errors more precisely.

    7
    Implement the Default trait for StratusStorage to provide a standard way of creating a default instance

    Consider implementing the Default trait for StratusStorage to provide a standard way
    of creating a default instance, which could be useful in testing and initialization
    scenarios.

    src/eth/storage/stratus_storage.rs [36-57]

     impl StratusStorage {
         /// Creates a new storage with the specified temporary and permanent implementations.
         pub fn new(temp: Box<dyn TemporaryStorage>, perm: Box<dyn PermanentStorage>) -> Result<Self, StratusError> {
             let this = Self { temp, perm };
     
             Ok(this)
         }
     
         #[cfg(feature = "dev")]
         pub fn new_test() -> Result<Self, StratusError> {
             use crate::eth::storage::InMemoryPermanentStorage;
             use crate::eth::storage::InMemoryTemporaryStorage;
     
             let perm = Box::new(InMemoryPermanentStorage::default());
             let temp = Box::new(InMemoryTemporaryStorage::default());
     
             Self::new(temp, perm)
         }
     }
     
    +impl Default for StratusStorage {
    +    fn default() -> Self {
    +        Self::new_test().expect("Failed to create default StratusStorage")
    +    }
    +}
    +
    Suggestion importance[1-10]: 5

    Why: Implementing the Default trait for StratusStorage provides a convenient way to create a default instance, which can be useful in testing and initialization scenarios. However, it's a minor improvement that doesn't address any critical issues.

    5

    @marcospb19-cw marcospb19-cw merged commit 2304d01 into main Nov 25, 2024
    34 checks passed
    @marcospb19-cw marcospb19-cw deleted the refac-stratus-storage-into-a-trait branch November 25, 2024 13:24
    @gabriel-aranha-cw
    Copy link
    Contributor

    Final benchmark:
    RPS Stats: Max: 1149.00, Min: 619.00, Avg: 906.42, StdDev: 51.97
    TPS Stats: Max: 1047.00, Min: 770.00, Avg: 878.85, StdDev: 62.70
    Plot: View Plot

    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.

    2 participants