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: add endpoint to check node mode #1637

Merged
merged 11 commits into from
Aug 13, 2024
Merged

Conversation

gabriel-aranha-cw
Copy link
Contributor

@gabriel-aranha-cw gabriel-aranha-cw commented Aug 13, 2024

PR Type

Enhancement, Tests


Description

  • Added a new RPC endpoint stratus_mode to check the current node mode (leader or follower)
  • Implemented ModeString trait in globals.rs to convert NodeMode enum to string representation
  • Updated rpc_server.rs to register and implement the new stratus_mode method
  • Added e2e test in leader-follower.test.ts to validate the new endpoint for both leader and follower nodes
  • Improved code organization and type safety by using a trait for mode string conversion

Changes walkthrough 📝

Relevant files
Enhancement
rpc_server.rs
Add new RPC endpoint for node mode                                             

src/eth/rpc/rpc_server.rs

  • Added new RPC method stratus_mode to check node mode
  • Imported ModeString trait from globals
  • Registered stratus_mode method in the RPC module
  • +10/-0   
    globals.rs
    Implement ModeString trait for NodeMode                                   

    src/globals.rs

  • Defined new ModeString trait
  • Implemented ModeString for NodeMode enum
  • Added as_str method to convert node mode to string
  • +13/-0   
    Tests
    leader-follower.test.ts
    Add e2e test for node mode validation                                       

    e2e/cloudwalk-contracts/integration/test/leader-follower.test.ts

  • Added new test case to validate node modes for leader and follower
  • Implemented checks for both Stratus Leader and Follower node modes
  • +14/-0   

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link

    github-actions bot commented Aug 13, 2024

    PR Reviewer Guide 🔍

    (Review updated until commit 269c275)

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Tracing Improvement
    The new stratus_mode function should be instrumented with tracing to log when it's called.

    Error Handling
    The stratus_mode function should use expect instead of unwrapping the result of GlobalState::get_node_mode().

    @gabriel-aranha-cw gabriel-aranha-cw marked this pull request as ready for review August 13, 2024 17:40
    Copy link

    Persistent review updated to latest commit 269c275

    Copy link

    github-actions bot commented Aug 13, 2024

    PR Code Suggestions ✨

    Latest suggestions up to 269c275

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Implement the Display trait for more idiomatic string conversion

    Consider implementing Display trait for NodeMode instead of creating a custom
    ModeString trait. This would provide a more idiomatic way to convert the enum to a
    string.

    src/globals.rs [85-96]

    -pub trait ModeString {
    -    fn as_str(&self) -> &'static str;
    -}
    +use std::fmt;
     
    -impl ModeString for NodeMode {
    -    fn as_str(&self) -> &'static str {
    +impl fmt::Display for NodeMode {
    +    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
             match self {
    -            NodeMode::Leader => "leader",
    -            NodeMode::Follower => "follower",
    +            NodeMode::Leader => write!(f, "leader"),
    +            NodeMode::Follower => write!(f, "follower"),
             }
         }
     }
     
    Suggestion importance[1-10]: 8

    Why: Implementing the Display trait is more idiomatic in Rust and provides better integration with standard library functions. It's a significant improvement in code quality and maintainability.

    8
    Maintainability
    Extract mode validation logic into a separate function for better code organization

    Consider extracting the mode validation logic into a separate function to improve
    code readability and reusability.

    e2e/cloudwalk-contracts/integration/test/leader-follower.test.ts [19-26]

    +async function validateNodeMode(providerUrl: string, expectedMode: string) {
    +    updateProviderUrl(providerUrl);
    +    const modeResponse = await sendWithRetry("stratus_mode", []);
    +    expect(modeResponse.mode).to.equal(expectedMode);
    +}
    +
     // Check Stratus Leader node mode
    -updateProviderUrl("stratus");
    -const leaderMode = await sendWithRetry("stratus_mode", []);
    -expect(leaderMode.mode).to.equal("leader");
    +await validateNodeMode("stratus", "leader");
     
     // Check Stratus Follower node mode
    -updateProviderUrl("stratus-follower");
    -const followerMode = await sendWithRetry("stratus_mode", []);
    -expect(followerMode.mode).to.equal("follower");
    +await validateNodeMode("stratus-follower", "follower");
     
    Suggestion importance[1-10]: 7

    Why: Extracting the validation logic improves code readability and reusability. It's a good refactoring that enhances the test structure and maintainability.

    7
    Best practice
    Use a more descriptive test name to improve test clarity

    Consider using a more descriptive test name that clearly indicates what is being
    validated.

    e2e/cloudwalk-contracts/integration/test/leader-follower.test.ts [17]

    -it("Validate node modes for leader and follower", async function () {
    +it("should correctly report 'leader' and 'follower' modes for respective nodes", async function () {
     
    Suggestion importance[1-10]: 6

    Why: A more descriptive test name improves clarity and helps in understanding the test's purpose. While beneficial, it's a relatively minor improvement to the overall code quality.

    6
    Use a constant for JSON keys to improve maintainability

    Consider using a constant or an enum for the "mode" key in the JSON response. This
    would make the code more maintainable and less prone to typos.

    src/eth/rpc/rpc_server.rs [321-323]

    +const MODE_KEY: &str = "mode";
     Ok(json!({
    -    "mode": mode
    +    MODE_KEY: mode
     }))
     
    Suggestion importance[1-10]: 5

    Why: Using a constant for the JSON key is a good practice, but the current implementation is simple and clear. The improvement is minor and doesn't significantly impact maintainability.

    5

    Previous suggestions

    Suggestions up to commit 29ceb01
    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Extract repeated logic into a separate function to improve code reusability and readability

    Consider extracting the mode checking logic into a separate function to improve code
    reusability and readability. This will make the test more maintainable and easier to
    extend in the future.

    e2e/cloudwalk-contracts/integration/test/leader-follower.test.ts [19-26]

    -// Check Stratus Leader node mode
    -updateProviderUrl("stratus");
    -const leaderMode = await sendWithRetry("stratus_mode", []);
    -expect(leaderMode.mode).to.equal("leader");
    +async function checkNodeMode(providerUrl: string, expectedMode: string) {
    +  updateProviderUrl(providerUrl);
    +  const modeResponse = await sendWithRetry("stratus_mode", []);
    +  expect(modeResponse.mode).to.equal(expectedMode);
    +}
     
    -// Check Stratus Follower node mode
    -updateProviderUrl("stratus-follower");
    -const followerMode = await sendWithRetry("stratus_mode", []);
    -expect(followerMode.mode).to.equal("follower");
    +await checkNodeMode("stratus", "leader");
    +await checkNodeMode("stratus-follower", "follower");
     
    Suggestion importance[1-10]: 8

    Why: This suggestion significantly improves code readability and maintainability by reducing duplication and making the test more modular.

    8
    Error handling
    Add error handling for RPC calls to improve test robustness and error reporting

    Consider adding error handling for the sendWithRetry function calls. This will make
    the test more robust and provide better feedback if there are issues with the RPC
    calls.

    e2e/cloudwalk-contracts/integration/test/leader-follower.test.ts [20-26]

    -const leaderMode = await sendWithRetry("stratus_mode", []);
    -expect(leaderMode.mode).to.equal("leader");
    +try {
    +  const leaderMode = await sendWithRetry("stratus_mode", []);
    +  expect(leaderMode.mode).to.equal("leader");
     
    -// Check Stratus Follower node mode
    -updateProviderUrl("stratus-follower");
    -const followerMode = await sendWithRetry("stratus_mode", []);
    -expect(followerMode.mode).to.equal("follower");
    +  // Check Stratus Follower node mode
    +  updateProviderUrl("stratus-follower");
    +  const followerMode = await sendWithRetry("stratus_mode", []);
    +  expect(followerMode.mode).to.equal("follower");
    +} catch (error) {
    +  throw new Error(`Failed to check node modes: ${error.message}`);
    +}
     
    Suggestion importance[1-10]: 7

    Why: Adding error handling is a good practice that improves the robustness of the test and provides better feedback in case of failures.

    7
    Maintainability
    Use constants for string literals to improve maintainability and reduce potential errors

    Consider using a constant or an enum for the mode strings ("leader", "follower") to
    avoid potential typos and improve maintainability. This approach also centralizes
    the string definitions, making it easier to update them if needed.

    src/eth/rpc/rpc_server.rs [319-322]

    +const LEADER_MODE: &str = "leader";
    +const FOLLOWER_MODE: &str = "follower";
    +
     let mode = match GlobalState::get_node_mode() {
    -    NodeMode::Leader => "leader",
    -    NodeMode::Follower => "follower",
    +    NodeMode::Leader => LEADER_MODE,
    +    NodeMode::Follower => FOLLOWER_MODE,
     };
     
    Suggestion importance[1-10]: 6

    Why: Using constants for string literals is a good practice for maintainability and reducing typos, but the current implementation is already clear and concise.

    6
    Best practice
    Use exhaustive pattern matching for enum variants to improve code robustness

    Consider using a match expression with exhaustive pattern matching instead of the
    current if-else structure. This approach ensures that all possible variants of
    NodeMode are handled, making the code more robust and future-proof.

    src/eth/rpc/rpc_server.rs [319-322]

     let mode = match GlobalState::get_node_mode() {
         NodeMode::Leader => "leader",
         NodeMode::Follower => "follower",
    +    _ => "unknown",
     };
     
    Suggestion importance[1-10]: 3

    Why: The suggestion is valid but unnecessary. The current code already handles all existing variants of NodeMode, and adding a catch-all pattern could hide future enum additions.

    3

    @dinhani-cw
    Copy link
    Contributor

    Instead of creating an endpoint only for exposing the mode, create a endpoint where Status can expose all internal state. Mode can be one of the information it exposes.

    src/globals.rs Outdated Show resolved Hide resolved
    @gabriel-aranha-cw gabriel-aranha-cw enabled auto-merge (squash) August 13, 2024 20:28
    @gabriel-aranha-cw gabriel-aranha-cw merged commit 2555162 into main Aug 13, 2024
    32 checks passed
    @gabriel-aranha-cw gabriel-aranha-cw deleted the create-mode-endpoint branch August 13, 2024 20:32
    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