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

chore: create storage kind #1883

Merged
merged 3 commits into from
Nov 25, 2024
Merged

chore: create storage kind #1883

merged 3 commits 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 StorageKind enum to allow for future extensibility of storage types
  • Renamed StratusStorageConfig to StorageConfig for clarity and consistency
  • Added storage_kind field to StorageConfig with a default value of "stratus-storage"
  • Implemented FromStr for StorageKind to allow parsing from string representations
  • Updated the init method to check the storage_kind before initializing storage
  • Modified error messages to use Self::VARIANTS for better maintainability
  • Updated import statements and usage of the renamed StorageConfig in src/config.rs

Changes walkthrough 📝

Relevant files
Enhancement
config.rs
Update storage configuration and error handling                   

src/config.rs

  • Renamed StratusStorageConfig to StorageConfig
  • Updated import statement for StorageConfig
  • Modified error message in Environment::from_str to use Self::VARIANTS
  • +5/-5     
    mod.rs
    Implement StorageKind and refactor storage configuration 

    src/eth/storage/mod.rs

  • Introduced StorageKind enum with StratusStorage variant
  • Renamed StratusStorageConfig to StorageConfig and added storage_kind
    field
  • Implemented FromStr for StorageKind
  • Updated init method to check storage_kind
  • +28/-2   

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

    Potential Performance Issue
    The new StorageKind enum is currently only used for a single variant. Consider if this abstraction is necessary at this point, or if it might lead to unnecessary complexity and potential performance overhead.

    Error Handling
    The init method now checks for StorageKind::StratusStorage, but doesn't handle other potential variants. Consider how to handle future storage kinds or if this check is necessary.

    Tracing
    The initialization of the storage in the init method is not logged. Consider adding a tracing event to log the initialization with relevant configurations.

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    General
    Enhance error handling and extensibility by properly matching on all possible StorageKind variants

    The init method in StorageConfig assumes that storage_kind is always StratusStorage.
    Consider handling other potential StorageKind variants to make the code more robust
    and extensible.

    src/eth/storage/mod.rs [122-130]

     pub fn init(&self) -> Result<Arc<StratusStorage>, StratusError> {
         let temp_storage = self.temp_storage.init()?;
         let perm_storage = self.perm_storage.init()?;
    -    let StorageKind::StratusStorage = self.storage_kind;
    -    let storage = StratusStorage::new(temp_storage, perm_storage)?;
    -
    -    Ok(Arc::new(storage))
    +    match self.storage_kind {
    +        StorageKind::StratusStorage => {
    +            let storage = StratusStorage::new(temp_storage, perm_storage)?;
    +            Ok(Arc::new(storage))
    +        },
    +        _ => Err(StratusError::UnsupportedStorageKind(self.storage_kind.to_string())),
    +    }
     }
    Suggestion importance[1-10]: 8

    Why: The suggestion significantly improves error handling and future extensibility by properly matching all StorageKind variants, rather than assuming only StratusStorage. This makes the code more robust and easier to maintain as new storage kinds are added.

    8

    @marcospb19-cw marcospb19-cw enabled auto-merge (squash) November 25, 2024 14:23
    @marcospb19-cw marcospb19-cw merged commit f63fc5c into main Nov 25, 2024
    32 checks passed
    @marcospb19-cw marcospb19-cw deleted the create-storage-kind branch November 25, 2024 14:26
    @gabriel-aranha-cw
    Copy link
    Contributor

    Final benchmark:
    Run ID: bench-1429455026

    Git Info:

    Configuration:

    • Target Account Strategy: Default (Random)

    RPS Stats: Max: 1173.00, Min: 727.00, Avg: 882.98, StdDev: 56.09
    TPS Stats: Max: 1022.00, Min: 739.00, Avg: 855.69, StdDev: 65.05

    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