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: stratus_reset instead of debug_setHead #1560

Merged
merged 1 commit into from
Jul 30, 2024
Merged

feat: stratus_reset instead of debug_setHead #1560

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, Tests


Description

  • Replaced debug_setHead with stratus_reset and hardhat_reset in the RPC server.
  • Renamed reset_at to reset in various storage implementations and added #[cfg(feature = "dev")] attribute.
  • Updated end-to-end tests to use stratus_reset instead of debug_setHead.
  • Simplified sendReset function by removing conditional check for isStratus.
  • Updated patch files to replace debug_setHead with stratus_reset.

Changes walkthrough 📝

Relevant files
Enhancement
8 files
rpc_server.rs
Replace debug_setHead with stratus_reset and hardhat_reset

src/eth/rpc/rpc_server.rs

  • Replaced debug_setHead with stratus_reset and hardhat_reset
  • Removed debug_set_head function
  • Added stratus_reset function
  • +9/-14   
    inmemory_permanent.rs
    Rename reset_at to reset with dev feature flag                     

    src/eth/storage/inmemory/inmemory_permanent.rs

  • Renamed reset_at to reset
  • Added #[cfg(feature = "dev")] attribute to reset function
  • +2/-1     
    permanent_storage.rs
    Rename reset_at to reset with dev feature flag                     

    src/eth/storage/permanent_storage.rs

  • Renamed reset_at to reset
  • Added #[cfg(feature = "dev")] attribute to reset function
  • +2/-1     
    redis_permanent.rs
    Rename reset_at to reset with dev feature flag                     

    src/eth/storage/redis/redis_permanent.rs

  • Renamed reset_at to reset
  • Added #[cfg(feature = "dev")] attribute to reset function
  • +2/-1     
    rocks_permanent.rs
    Rename reset_at to reset with dev feature flag                     

    src/eth/storage/rocks/rocks_permanent.rs

  • Renamed reset_at to reset
  • Added #[cfg(feature = "dev")] attribute to reset function
  • +2/-1     
    stratus_storage.rs
    Update reset function to remove block number parameter     

    src/eth/storage/stratus_storage.rs

  • Renamed reset function to remove block number parameter
  • Added #[cfg(feature = "dev")] attribute to reset function
  • +3/-2     
    rpc.ts
    Simplify sendReset function                                                           

    e2e/test/helpers/rpc.ts

    • Removed conditional check for isStratus in sendReset function
    +1/-5     
    brlc-yield-streamer.patch
    Replace debug_setHead with stratus_reset in patch               

    e2e/cloudwalk-contracts/patches/brlc-yield-streamer.patch

    • Replaced debug_setHead with stratus_reset in patch
    +6/-6     
    Tests
    2 files
    e2e-json-rpc.test.ts
    Replace debug_setHead with stratus_reset in tests               

    e2e/test/automine/e2e-json-rpc.test.ts

    • Replaced debug_setHead with stratus_reset in tests
    +3/-6     
    e2e-json-rpc.test.ts
    Replace debug_setHead with stratus_reset in tests               

    e2e/test/external/e2e-json-rpc.test.ts

    • Replaced debug_setHead with stratus_reset in tests
    +3/-4     

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

    Logging and Monitoring
    The stratus_reset function should include logging to ensure the reset operation is monitored. Additionally, consider using spawn_named for any asynchronous tasks to improve traceability.

    Tracing Fields
    The stratus_health function should include relevant identifiers as span fields for better traceability in logs.

    Error Logging
    The stratus_reset function should log the original error in a field called reason to provide more context in case of failures.

    Copy link

    github-actions bot commented Jul 30, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Maintainability
    Remove the redundant registration of the same method

    The stratus_reset method is registered twice, which is redundant. It should be
    registered only once to avoid potential conflicts or confusion.

    src/eth/rpc/rpc_server.rs [153-154]

    -module.register_blocking_method("hardhat_reset", stratus_reset)?;
     module.register_blocking_method("stratus_reset", stratus_reset)?;
     
    Suggestion importance[1-10]: 10

    Why: Removing the redundant registration of the stratus_reset method is crucial to avoid potential conflicts and confusion, ensuring the code is clean and maintainable.

    10
    Possible issue
    Ensure the reset function works correctly in both environments

    The sendReset function should include the stratus_reset method to ensure it works
    correctly in both environments.

    e2e/test/helpers/rpc.ts [226]

    -await send("hardhat_reset");
    +if (isStratus) {
    +    await send("stratus_reset");
    +} else {
    +    await send("hardhat_reset");
    +}
     
    Suggestion importance[1-10]: 9

    Why: Ensuring the sendReset function works correctly in both environments is crucial for the reliability of the tests, making this a significant improvement.

    9
    Modify the method to accept a block number parameter for resetting

    The reset method should accept a BlockNumber parameter to match the trait definition
    and ensure it can reset to a specific block number.

    src/eth/storage/stratus_storage.rs [480]

    -pub fn reset(&self) -> Result<(), StratusError> {
    +pub fn reset(&self, number: BlockNumber) -> Result<(), StratusError> {
     
    Suggestion importance[1-10]: 8

    Why: Modifying the method to accept a block number parameter is important for functionality, ensuring the method aligns with the trait definition and can reset to a specific block number.

    8
    Best practice
    Rename the method to maintain consistency with the trait definition

    The reset method should be renamed to reset_at to maintain consistency with the
    method name used in the trait definition and other implementations.

    src/eth/storage/inmemory/inmemory_permanent.rs [283]

    -fn reset(&self, block_number: BlockNumber) -> anyhow::Result<()> {
    +fn reset_at(&self, block_number: BlockNumber) -> anyhow::Result<()> {
     
    Suggestion importance[1-10]: 7

    Why: Renaming the method to maintain consistency with the trait definition improves code readability and maintainability, although it is not critical for functionality.

    7

    @dinhani-cw dinhani-cw enabled auto-merge (squash) July 30, 2024 02:15
    @dinhani-cw dinhani-cw merged commit 34cbbb7 into main Jul 30, 2024
    34 checks passed
    @dinhani-cw dinhani-cw deleted the reset branch July 30, 2024 02:18
    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