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

enha: force block changes to redis #1576

Merged
merged 1 commit into from
Jul 30, 2024
Merged

enha: force block changes to redis #1576

merged 1 commit into from
Jul 30, 2024

Conversation

dinhani-cw
Copy link
Contributor

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

PR Type

Enhancement, Bug fix


Description

  • Renamed the is_changed method to is_account_modified in ExecutionAccountChanges to better reflect its purpose.
  • Enhanced Redis storage logic to include block number in account and slot modifications, ensuring forced updates.
  • Updated JSON serialization to include block number for both accounts and slots.
  • Modified logic to only process accounts if they are modified, optimizing storage operations.

Changes walkthrough 📝

Relevant files
Enhancement
execution_account_changes.rs
Rename and update account modification check method           

src/eth/primitives/execution_account_changes.rs

  • Renamed is_changed method to is_account_modified.
  • Updated method documentation to reflect the new name.
  • +2/-7     
    Bug fix
    redis_permanent.rs
    Force block number inclusion in Redis account and slot updates

    src/eth/storage/redis/redis_permanent.rs

  • Added block number to account and slot modifications to force updates.
  • Modified logic to only process accounts if they are modified.
  • Updated JSON serialization to include block number.
  • +26/-18 

    💡 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: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Logging and Monitoring
    The operations involving Redis updates should be properly logged for monitoring purposes. Consider adding structured tracing logs to capture key events and data points, such as the account and slot modifications.

    Use spawn_named
    If any of these operations are intended to run in the background, consider using tokio::spawn_named or tokio::spawn_blocking_named for better traceability and monitoring.

    Use traced_sleep
    If there are any sleep operations involved, consider using traced_sleep to ensure visibility in tracing events.

    Copy link

    github-actions bot commented Jul 30, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Replace unwrap() with proper error handling to avoid potential panics

    To avoid potential panics, replace the use of unwrap() with proper error handling
    when inserting the block number into the account value.

    src/eth/storage/redis/redis_permanent.rs [117-118]

     let mut account_value = to_json_value(&account);
    -account_value.as_object_mut().unwrap().insert("block".to_owned(), to_json_value(block.number()));
    +if let Some(account_obj) = account_value.as_object_mut() {
    +    account_obj.insert("block".to_owned(), to_json_value(block.number()));
    +} else {
    +    // handle the error appropriately
    +}
     
    Suggestion importance[1-10]: 9

    Why: This suggestion addresses a potential runtime panic, which is important for the robustness and reliability of the code.

    9
    Maintainability
    Extract repeated logic into a helper function to improve readability and maintainability

    Extract the repeated logic for adding the block number to the JSON value into a
    helper function to improve code readability and maintainability.

    src/eth/storage/redis/redis_permanent.rs [118-130]

    -account_value.as_object_mut().unwrap().insert("block".to_owned(), to_json_value(block.number()));
    +fn add_block_number(value: &mut serde_json::Value, block_number: u64) {
    +    if let Some(obj) = value.as_object_mut() {
    +        obj.insert("block".to_owned(), to_json_value(block_number));
    +    } else {
    +        // handle the error appropriately
    +    }
    +}
     ...
    -slot_value.as_object_mut().unwrap().insert("block".to_owned(), to_json_value(block.number()));
    +add_block_number(&mut account_value, block.number());
    +...
    +add_block_number(&mut slot_value, block.number());
     
    Suggestion importance[1-10]: 8

    Why: Extracting repeated logic into a helper function enhances code readability and maintainability, which is beneficial for long-term code management.

    8
    Best practice
    Rename the method to maintain naming consistency and conciseness

    Consider renaming the method is_account_modified to is_modified to maintain
    consistency with the previous method naming convention and to make the method name
    more concise.

    src/eth/primitives/execution_account_changes.rs [99-101]

    -pub fn is_account_modified(&self) -> bool {
    +pub fn is_modified(&self) -> bool {
       self.nonce.is_modified() || self.balance.is_modified() || self.bytecode.is_modified()
     }
     
    Suggestion importance[1-10]: 7

    Why: The suggestion improves naming consistency and conciseness, but it is not crucial for functionality.

    7

    @dinhani-cw dinhani-cw enabled auto-merge (squash) July 30, 2024 23:37
    @dinhani-cw dinhani-cw merged commit 42580c0 into main Jul 30, 2024
    34 checks passed
    @dinhani-cw dinhani-cw deleted the fix-redis-3 branch July 30, 2024 23:41
    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