Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix: correct subscription type mapping in stratus_get_subscriptions #2031

Merged
merged 1 commit into from
Feb 27, 2025

Conversation

gabriel-aranha-cw
Copy link
Contributor

@gabriel-aranha-cw gabriel-aranha-cw commented Feb 27, 2025

PR Type

Bug fix


Description

  • Corrected subscription type mapping in stratus_get_subscriptions

  • Swapped pending_txs and new_heads assignments


Changes walkthrough 📝

Relevant files
Bug fix
rpc_server.rs
Correct subscription type assignment in stratus_get_subscriptions

src/eth/rpc/rpc_server.rs

  • Swapped assignments for pending_txs and new_heads
  • Corrected mapping of subscription types to their respective RPC
    context fields
  • +2/-2     

    Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • @gabriel-aranha-cw gabriel-aranha-cw marked this pull request as ready for review February 27, 2025 15:36
    Copy link

    github-actions bot commented Feb 27, 2025

    PR Reviewer Guide 🔍

    (Review updated until commit 9edd1fb)

    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

    Subscription Mapping

    Verify that the subscription type mapping is now correct. Ensure that pending_txs and new_heads are properly assigned to their respective fields in the response JSON.

    let pending_txs = serde_json::to_value(ctx.subs.pending_txs.read().await.values().collect_vec()).expect_infallible();
    let new_heads = serde_json::to_value(ctx.subs.new_heads.read().await.values().collect_vec()).expect_infallible();

    Copy link

    github-actions bot commented Feb 27, 2025

    PR Code Suggestions ✨

    Latest suggestions up to 9edd1fb
    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Correct subscription type mapping

    Ensure that the subscription types are correctly mapped to their respective fields
    in the JSON response. The current implementation might lead to incorrect data being
    returned for 'newPendingTransactions' and 'newHeads'.

    src/eth/rpc/rpc_server.rs [596-601]

     let pending_txs = serde_json::to_value(ctx.subs.pending_txs.read().await.values().collect_vec()).expect_infallible();
     let new_heads = serde_json::to_value(ctx.subs.new_heads.read().await.values().collect_vec()).expect_infallible();
     ...
     let response = json!({
         "newPendingTransactions": pending_txs,
    +    "newHeads": new_heads,
    Suggestion importance[1-10]: 9

    __

    Why: The suggestion correctly identifies and addresses a critical issue where the 'newHeads' field was missing from the JSON response, which could lead to incorrect data being returned. This fix ensures that both 'newPendingTransactions' and 'newHeads' are properly included in the response.

    High

    Previous suggestions

    Suggestions up to commit 9edd1fb
    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Correct subscription type mapping

    Ensure that the subscription types are correctly mapped to their respective fields
    in the JSON response. The current implementation might lead to incorrect data being
    returned for 'newPendingTransactions' and 'newHeads'.

    src/eth/rpc/rpc_server.rs [596-601]

     let pending_txs = serde_json::to_value(ctx.subs.pending_txs.read().await.values().collect_vec()).expect_infallible();
     let new_heads = serde_json::to_value(ctx.subs.new_heads.read().await.values().collect_vec()).expect_infallible();
     ...
     let response = json!({
         "newPendingTransactions": pending_txs,
    +    "newHeads": new_heads,
    Suggestion importance[1-10]: 9

    __

    Why: The suggestion correctly identifies and addresses a critical issue where the 'newHeads' field was missing from the JSON response, which could lead to incorrect data being returned. This fix ensures that both 'newPendingTransactions' and 'newHeads' are properly included in the response.

    High

    Copy link

    Persistent review updated to latest commit 9edd1fb

    @gabriel-aranha-cw gabriel-aranha-cw enabled auto-merge (squash) February 27, 2025 15:38
    @gabriel-aranha-cw gabriel-aranha-cw merged commit 08644bf into main Feb 27, 2025
    42 checks passed
    @gabriel-aranha-cw gabriel-aranha-cw deleted the fix-get-subs-result branch February 27, 2025 17:22
    @gabriel-aranha-cw
    Copy link
    Contributor Author

    Final benchmark:
    Run ID: bench-1656657815

    Git Info:

    Configuration:

    • Target Account Strategy: Default

    Leader Stats:
    RPS Stats: Max: 2100.00, Min: 1452.00, Avg: 1933.19, StdDev: 79.35
    TPS Stats: Max: 2102.00, Min: 1633.00, Avg: 1876.12, StdDev: 93.08

    Follower Stats:
    Imported Blocks/s: Max: 2.00, Min: 1.00, Avg: 1.46, StdDev: 0.50
    Imported Transactions/s: Max: 4143.00, Min: 1320.00, Avg: 2824.29, StdDev: 968.13

    Plots:

    # for free to join this conversation on GitHub. Already have an account? # to comment
    Labels
    None yet
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants