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: initial redis support (only for debug purposes) - part 1 #1555

Merged
merged 2 commits into from
Jul 29, 2024
Merged

Conversation

dinhani-cw
Copy link
Contributor

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

PR Type

Enhancement, Configuration changes


Description

  • Enhanced BlockNumber struct by adding derive_more::Display and removing the manual implementation of Display.
  • Introduced a new redis module in the storage.
  • Added RedisPermanentStorage to the permanent storage options and included a new configuration option perm-storage-url.
  • Created a new module for Redis permanent storage and implemented RedisPermanentStorage with methods for block, transaction, account, and slot operations.
  • Added redis dependency to Cargo.toml.
  • Updated docker-compose.yaml to include Redis and Redis Commander services.

Changes walkthrough 📝

Relevant files
Enhancement
block_number.rs
Enhance `BlockNumber` struct with `derive_more::Display` 

src/eth/primitives/block_number.rs

  • Added derive_more::Display to BlockNumber struct.
  • Removed manual implementation of Display for BlockNumber.
  • +16/-8   
    mod.rs
    Introduce Redis module in storage                                               

    src/eth/storage/mod.rs

    • Added redis module.
    +1/-0     
    permanent_storage.rs
    Add Redis support to permanent storage configuration         

    src/eth/storage/permanent_storage.rs

  • Added RedisPermanentStorage to permanent storage options.
  • Introduced perm-storage-url configuration option.
  • +13/-0   
    mod.rs
    Create Redis permanent storage module                                       

    src/eth/storage/redis/mod.rs

    • Created module for Redis permanent storage.
    +3/-0     
    redis_permanent.rs
    Implement RedisPermanentStorage with necessary methods     

    src/eth/storage/redis/redis_permanent.rs

  • Implemented RedisPermanentStorage with methods for block, transaction,
    account, and slot operations.
  • +238/-0 
    Dependencies
    Cargo.toml
    Add Redis dependency to Cargo.toml                                             

    Cargo.toml

    • Added redis dependency.
    +1/-0     
    Configuration changes
    docker-compose.yaml
    Add Redis services to docker-compose                                         

    docker-compose.yaml

    • Added Redis and Redis Commander services.
    +24/-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 changed the title feat: initial redis support (only for tests) feat: initial redis support (only for debug purposes) Jul 29, 2024
    @dinhani-cw dinhani-cw changed the title feat: initial redis support (only for debug purposes) feat: initial redis support (only for debug purposes) - part 1 Jul 29, 2024
    Copy link

    PR Reviewer Guide 🔍

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

    Core Component Initialization
    The initialization of RedisPermanentStorage is not logged. Ensure that the initialization is logged with all relevant configurations for better traceability.

    Error Handling
    The conn function uses log_and_err! macro for logging errors. Ensure that the original error is logged in a field called reason for consistency.

    Dynamic Field Logging
    In multiple places, dynamic fields are being formatted into log messages. Instead, add these dynamic fields as tracing fields to ensure structured logging.

    Unwrap Usage
    The new function uses unwrap when opening a Redis client. Use expect with a meaningful message instead to handle potential errors gracefully.

    Copy link

    github-actions bot commented Jul 29, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Improve type conversion clarity and safety in the read_mined_block_number method

    The read_mined_block_number method currently converts the usize value directly into
    a BlockNumber. This can be improved by explicitly converting the usize to U64 before
    creating the BlockNumber for better clarity and type safety.

    src/eth/storage/redis/redis_permanent.rs [67]

    -Ok(Some(value)) => Ok(value.into()),
    +Ok(Some(value)) => Ok(BlockNumber(U64::from(value))),
     
    Suggestion importance[1-10]: 9

    Why: This suggestion enhances type safety and clarity by explicitly converting usize to U64 before creating BlockNumber. It is a best practice that improves code reliability.

    9
    Enhancement
    Simplify error handling in the conn method by using the ? operator

    The conn method currently logs an error and returns it using log_and_err!. It would
    be more idiomatic to use the ? operator directly in the match statement to propagate
    the error, simplifying the code.

    src/eth/storage/redis/redis_permanent.rs [41-44]

    -match self.client.get_connection() {
    -    Ok(conn) => Ok(conn),
    -    Err(e) => log_and_err!(reason = e, "failed to get redis connection"),
    -}
    +self.client.get_connection().map_err(|e| log_and_err!(reason = e, "failed to get redis connection"))
     
    Suggestion importance[1-10]: 8

    Why: The suggestion simplifies the error handling logic, making the code more idiomatic and easier to read. It correctly addresses the existing code and provides a clear improvement.

    8
    Performance
    Optimize the save_accounts method by using collect directly into a Vec

    The save_accounts method can be optimized by using collect directly into a Vec
    instead of collect_vec, which is more idiomatic and avoids an unnecessary dependency
    on itertools.

    src/eth/storage/redis/redis_permanent.rs [175-182]

    -let redis_accounts = accounts
    +let redis_accounts: Vec<_> = accounts
         .into_iter()
         .map(|acc| {
             let key = format!("account::{}", acc.address);
             let value = to_json_string(&acc);
             (key, value)
         })
    -    .collect_vec();
    +    .collect();
     
    Suggestion importance[1-10]: 8

    Why: This suggestion removes an unnecessary dependency on itertools and makes the code more idiomatic by using collect directly into a Vec. It is a valid and useful optimization.

    8
    Optimize the save_block method by using extend instead of push in the loop

    The save_block method can be optimized by using extend instead of push in the loop
    to add multiple elements to redis_values, which can improve performance slightly.

    src/eth/storage/redis/redis_permanent.rs [88-93]

    -for tx in &block.transactions {
    +redis_values.extend(block.transactions.iter().map(|tx| {
         let tx_key = format!("tx::{}", tx.input.hash);
         let tx_value = to_json_string(&tx);
    -    redis_values.push((tx_key, tx_value));
    -}
    +    (tx_key, tx_value)
    +}));
     
    Suggestion importance[1-10]: 7

    Why: The suggestion provides a minor performance improvement by using extend instead of push in the loop. It is contextually accurate and optimizes the code slightly.

    7

    @dinhani-cw dinhani-cw enabled auto-merge (squash) July 29, 2024 22:31
    @dinhani-cw dinhani-cw merged commit 36effeb into main Jul 29, 2024
    33 checks passed
    @dinhani-cw dinhani-cw deleted the redis branch July 29, 2024 22:35
    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