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

fix: inconsistencies and tests #876

Merged
merged 9 commits into from
May 21, 2024
Merged

fix: inconsistencies and tests #876

merged 9 commits into from
May 21, 2024

Conversation

gabriel-aranha-cw
Copy link
Contributor

@gabriel-aranha-cw gabriel-aranha-cw commented May 20, 2024

This PR is focused on finishing up the e2e WebSocket and Subscription tests:

  1. Fix the unsupported subscription error code;
  2. Correctly serialize the block header for the newHeads event into ethers_core::types::Block as EthersBlock for compatibility with HardHat e2e tests;
  3. Re enable newHeads events fields validation tests;
  4. Create newPendingTransactions tests cases.

Below the previous and new example payloads of the newHeads event:
Previous:

{
    "jsonrpc": "2.0",
    "method": "eth_subscription",
    "params": {
        "subscription": "B96i2Iay",
        "result": {
            "number": "0x7",
            "hash": "0xea2e640cf9cf85178466ebb2f721ea6b3ec88def0a8c3d3f7d31e775eed05347",
            "transactions_root": "0x56e81f171bcc55a6ff8345e692c0f86e5b48e01b996cadc001622fb5e363b421",
            "gas_used": "0x0",
            "gas_limit": "0x0",
            "bloom": "0x00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000",
            "timestamp": 1716237906,
            "parent_hash": "0xc3751ea2572cb6b4f061af1127a67eaded2cfc191f2a18d69000bbe2e98b680a",
            "author": "0x0000000000000000000000000000000000000000",
            "extra_data": "0x",
            "miner": "0x0000000000000000000000000000000000000000",
            "difficulty": "0x0",
            "receipts_root": "0x56e81f171bcc55a6ff8345e692c0f86e5b48e01b996cadc001622fb5e363b421",
            "uncle_hash": "0x1dcc4de8dec75d7aab85b567b6ccd41ad312451b948a7413f0a142fd40d49347",
            "size": "0x0",
            "state_root": "0x56e81f171bcc55a6ff8345e692c0f86e5b48e01b996cadc001622fb5e363b421",
            "total_difficulty": "0x0",
            "nonce": "0x0000000000000000"
        }
    }
}

New:

{
    "jsonrpc": "2.0",
    "method": "eth_subscription",
    "params": {
        "subscription": "8HRddspf",
        "result": {
            "hash": "0xea2e640cf9cf85178466ebb2f721ea6b3ec88def0a8c3d3f7d31e775eed05347",
            "parentHash": "0xc3751ea2572cb6b4f061af1127a67eaded2cfc191f2a18d69000bbe2e98b680a",
            "sha3Uncles": "0x1dcc4de8dec75d7aab85b567b6ccd41ad312451b948a7413f0a142fd40d49347",
            "miner": "0x00000000000000000000000000000000000000ff",
            "stateRoot": "0x0000000000000000000000000000000000000000000000000000000000000000",
            "transactionsRoot": "0x56e81f171bcc55a6ff8345e692c0f86e5b48e01b996cadc001622fb5e363b421",
            "receiptsRoot": "0x56e81f171bcc55a6ff8345e692c0f86e5b48e01b996cadc001622fb5e363b421",
            "number": "0x7",
            "gasUsed": "0x0",
            "gasLimit": "0x5f5e100",
            "extraData": "0x",
            "logsBloom": "0x00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000",
            "timestamp": "0x664bb3e7",
            "difficulty": "0x0",
            "totalDifficulty": "0x0",
            "sealFields": [],
            "uncles": [],
            "transactions": [],
            "size": null,
            "mixHash": null,
            "nonce": "0x0000000000000000",
            "baseFeePerGas": "0x0"
        }
    }
}

Copy link

github-actions bot commented May 20, 2024

PR Review 🔍

(Review updated until commit bf4f574)

⏱️ Estimated effort to review [1-5]

3, because the PR involves changes across multiple files including Rust and TypeScript, affecting error handling, serialization, and test cases. Understanding the context and ensuring the changes meet the requirements would require a moderate level of effort.

🧪 Relevant tests

Yes

⚡ Possible issues

Possible Bug: The use of .unwrap() in the From implementation for SubscriptionMessage in block_header.rs could cause a panic if serialization fails. Consider handling this error more gracefully.

🔒 Security concerns

No

Code feedback:
relevant filesrc/eth/primitives/block_header.rs
suggestion      

Replace .unwrap() with error handling to prevent potential runtime panics if serialization fails. Consider using ? to propagate the error or handle it explicitly. [important]

relevant lineSelf::from_json(ðers_block).unwrap()

relevant filesrc/eth/rpc/rpc_server.rs
suggestion      

Ensure that the error message for unsupported subscription kinds includes sufficient diagnostic information. Consider adding more context or logging at this point. [medium]

relevant line.reject(rpc_invalid_params_error(format!("unsupported subscription kind: {}", kind)))

relevant filee2e/test/e2e-json-rpc.test.ts
suggestion      

The test case for 'newPendingTransactions' should verify more properties of the result to ensure the response structure is as expected, not just the presence of fields. [medium]

relevant lineexpect(params).to.have.property('result').that.is.an('string');

relevant filee2e/test/e2e-json-rpc.test.ts
suggestion      

Consider adding cleanup or reset logic before each test to ensure that the state from previous tests does not affect the current test. This can be done using hooks like beforeEach. [medium]

relevant lineawait sendReset();

@gabriel-aranha-cw gabriel-aranha-cw changed the title [IN PROGRESS] Fix inconsistencies fix: inconsistencies May 20, 2024
Copy link

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Score
Possible bug
Improve error handling by replacing unwrap() with error propagation

Consider handling the potential error from from_json method gracefully instead of
unwrapping directly. This will prevent runtime panics if the conversion fails.

src/eth/primitives/block_header.rs [199]

-Self::from_json(&ethers_block).unwrap()
+Self::from_json(&ethers_block).map_err(|e| e.to_string())?
 
Suggestion importance[1-10]: 9

Why: This suggestion addresses a potential runtime panic by replacing unwrap() with proper error handling, which is crucial for robust and safe code.

9
Possible issue
Add error handling for message serialization in the error creation function

Ensure that the message parameter in rpc_invalid_params_error function is serialized
properly to avoid runtime errors. Consider adding error handling for serialization
failure.

src/eth/rpc/rpc_parser.rs [48]

-ErrorObjectOwned::owned(INVALID_PARAMS_CODE, INVALID_PARAMS_MSG, Some(message))
+serde_json::to_value(message).map(|msg| ErrorObjectOwned::owned(INVALID_PARAMS_CODE, INVALID_PARAMS_MSG, Some(msg))).map_err(|e| e.to_string())?
 
Suggestion importance[1-10]: 8

Why: Adding error handling for message serialization enhances the robustness of the code by preventing potential runtime errors during serialization.

8
Enhancement
Enhance logging by including error details in the structured log

Use structured logging to include the error details when rejecting a subscription due to
an unsupported kind.

src/eth/rpc/rpc_server.rs [416-417]

-tracing::warn!(%kind, "unsupported subscription kind");
-pending.reject(rpc_invalid_params_error(format!("unsupported subscription kind: {}", kind))).await;
+let error_msg = format!("unsupported subscription kind: {}", kind);
+tracing::warn!(%kind, %error_msg, "subscription rejection");
+pending.reject(rpc_invalid_params_error(error_msg)).await;
 
Suggestion importance[1-10]: 7

Why: This suggestion improves the logging mechanism by providing more detailed information, which can be useful for debugging and monitoring.

7
Maintainability
Standardize assertion style for consistency in tests

Ensure consistent use of assertion styles in tests to improve readability and
maintainability.

e2e/test/e2e-json-rpc.test.ts [194]

-expect(response.error.code).eq(-32602);
+expect(response.error.code).to.equal(-32602);
 
Suggestion importance[1-10]: 6

Why: Ensuring consistent use of assertion styles improves code readability and maintainability, although it is a minor improvement.

6

@gabriel-aranha-cw gabriel-aranha-cw changed the title fix: inconsistencies fix: inconsistencies and tests May 21, 2024
@gabriel-aranha-cw gabriel-aranha-cw marked this pull request as ready for review May 21, 2024 12:54
@gabriel-aranha-cw gabriel-aranha-cw requested a review from a team as a code owner May 21, 2024 12:54
Copy link

Persistent review updated to latest commit bf4f574

Copy link

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Score
Possible bug
Replace unwrap() with error handling to prevent runtime panics

Replace the use of unwrap() with proper error handling to prevent potential panics at
runtime.

src/eth/primitives/block_header.rs [199]

-Self::from_json(&ethers_block).unwrap()
+Self::from_json(&ethers_block).map_err(|e| e.to_string())?
 
Suggestion importance[1-10]: 9

Why: Replacing unwrap() with proper error handling is crucial to prevent potential runtime panics, which improves the robustness and reliability of the code.

9
Possible issue
Add error handling for the reject method to ensure robustness

Ensure proper error handling for the reject method to handle potential failures during the
rejection process.

src/eth/rpc/rpc_server.rs [423]

-pending.reject(rpc_invalid_params_error(format!("unsupported subscription kind: {}", kind))).await;
+pending.reject(rpc_invalid_params_error(format!("unsupported subscription kind: {}", kind))).await?;
 
Suggestion importance[1-10]: 8

Why: Adding error handling for the reject method ensures robustness by handling potential failures during the rejection process, which is important for reliable error management.

8
Enhancement
Enhance error messages for better debugging and user feedback

Consider adding a more descriptive error message to enhance debugging and user feedback.

src/eth/rpc/rpc_parser.rs [48]

-ErrorObjectOwned::owned(INVALID_PARAMS_CODE, INVALID_PARAMS_MSG, Some(message))
+ErrorObjectOwned::owned(INVALID_PARAMS_CODE, "Invalid parameters provided: {}", Some(message))
 
Suggestion importance[1-10]: 7

Why: Enhancing error messages can significantly improve debugging and user feedback, making it easier to identify and resolve issues.

7
Best practice
Replace magic numbers with constants for better code clarity and maintainability

Use a constant for the waitTimeInMilliseconds to avoid magic numbers and improve
maintainability.

e2e/test/e2e-json-rpc.test.ts [179]

-const waitTimeInMilliseconds = 40;
+const DEFAULT_WAIT_TIME = 40;
+const waitTimeInMilliseconds = DEFAULT_WAIT_TIME;
 
Suggestion importance[1-10]: 6

Why: Replacing magic numbers with constants improves code clarity and maintainability, though it is a minor enhancement compared to critical bug fixes or error handling improvements.

6

@gabriel-aranha-cw gabriel-aranha-cw merged commit c4ae172 into main May 21, 2024
25 checks passed
@gabriel-aranha-cw gabriel-aranha-cw deleted the fix-inconsistencies branch May 21, 2024 17:14
@gabriel-aranha-cw
Copy link
Contributor Author

closes #719

@mayconamaroCW mayconamaroCW linked an issue May 21, 2024 that may be closed by this pull request
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.

Add content checks to eth_subscribe test cases
3 participants