✅ Test(actions): more test coverage #1455
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
|
|
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @roninjin10 and the rest of your teammates on |
WalkthroughThis pull request introduces multiple test suites for various functions related to Ethereum transaction processing, validation, and retrieval. It enhances the codebase by adding comprehensive unit tests for functions such as Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
roninjin10
force-pushed
the
09-24-_white_check_mark_test_actions_more_test_coverage
branch
from
d1d36fe to
d0825d8
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (28)
packages/actions/src/eth/ethAccountsProcedure.spec.ts (2)
17-25: LGTM: Good coverage for requests without id. Consider adding a negative test.This test case effectively complements the first one by covering the scenario where the request doesn't include an 'id'. The expectations are comprehensive and the use of 'as const' ensures type safety.
Consider adding a negative test case to check how the function handles invalid inputs, such as:
- A request with an invalid method name
- A request with an invalid jsonrpc version
This would further improve the robustness of your test suite.
1-26: Great addition to test coverage. Consider expanding edge cases.This new test file for
ethAccountsProcedureis well-structured and provides good coverage for the main scenarios. It aligns well with the PR objective of increasing test coverage.To further enhance the robustness of the test suite, consider:
- Adding negative test cases (as mentioned earlier).
- Testing with different numbers of accounts in
testAccounts.- Verifying the behavior when
testAccountsis empty.These additions would provide more comprehensive coverage and help catch potential edge cases.
packages/actions/src/Mine/processTx.spec.ts (1)
10-28: LGTM with suggestions: Comprehensive test case with room for enhancement.The test case effectively sets up the TEVM environment and processes a transaction. However, consider the following improvements:
- Add more diverse test cases with different transaction parameters to increase coverage.
- Include assertions to verify the transaction details in the receipt (e.g., gas used, to address, value).
- Consider testing edge cases, such as transactions with insufficient gas or invalid recipients.
Here's an example of how you could enhance the assertions:
expect(receipts[0]).toMatchObject({ status: 1, gasUsed: expect.any(BigInt), to: '0x1234567890123456789012345678901234567890', transactionHash: expect.any(String), })Additionally, consider adding a test case for a failing transaction:
it('should process a failing transaction', async () => { // ... setup code ... const tx = TransactionFactory.fromTxData({ to: '0x1234567890123456789012345678901234567890', value: 1000000000000000000000000n, // Very high value to ensure failure gasLimit: 21000n, gasPrice: 10n, }).sign(hexToBytes(PREFUNDED_PRIVATE_KEYS[0])) await processTx(client, tx, blockBuilder, receipts) expect(receipts).toHaveLength(1) expect(receipts[0]).toBeDefined() expect(receipts[0].status).toBe(0) // Expect failure status })packages/actions/src/eth/ethSignProcedure.spec.ts (2)
6-24: LGTM: Well-structured test case with comprehensive assertions.The test case effectively verifies the
ethSignProcedurefunction's behavior for signing a message. It checks both the structure and content of the response, which is a good practice.Consider extracting the expected response into a separate variable for improved readability:
const expectedResponse = { id: 1, jsonrpc: '2.0', method: 'eth_sign', result: await testAccounts[0].signMessage({ message: data }), }; expect(response).toEqual(expectedResponse);This change would make it easier to understand the expected structure at a glance and simplify maintenance if the structure changes in the future.
26-38: LGTM: Well-structured error case test with specific assertion.The test case effectively verifies the error handling of
ethSignProcedurewhen an invalid account is provided. The use ofexpect().rejects.toThrow()is appropriate for testing asynchronous errors.Consider extracting the invalid address into a constant for improved readability and maintainability:
const INVALID_ADDRESS = `0x${'69'.repeat(20)}`; const request = { id: 1, jsonrpc: '2.0', method: 'eth_sign', params: [INVALID_ADDRESS, data], } as const; // ... rest of the test ... await expect(procedure(request)).rejects.toThrow(`Account ${INVALID_ADDRESS} not found`);This change would make the test more self-documenting and easier to update if the invalid address format needs to change in the future.
packages/actions/src/eth/ethGetTransactionReceipt.spec.ts (3)
8-42: LGTM: Comprehensive test for successful transaction receipt retrieval.The test case thoroughly checks the happy path scenario for retrieving a transaction receipt. It covers all the essential steps: creating a transaction, mining a block, and verifying the receipt's properties.
Consider adding an expectation for the
statusfield of the receipt. Currently, it's set toundefined, but it should typically be1for a successful transaction or0for a failed one. You could add:status: 1n, // or 0n if the transaction is expected to failto the
expect(receipt).toMatchObject({...})assertion.
44-54: LGTM: Good test for handling non-existent transaction receipts.This test case effectively verifies that the handler returns
nullwhen a transaction receipt is not found. It's a crucial edge case to cover.Consider using a more flexible approach for generating a non-existent transaction hash. Instead of hardcoding a specific hash, you could generate a random hash that's guaranteed not to exist in the current state. This would make the test more robust against potential changes in hash formats or node behavior. Here's an example:
import { randomBytes } from 'crypto' // ... const nonExistentHash = `0x${randomBytes(32).toString('hex')}` as Hex const receipt = await handler({ hash: nonExistentHash })This approach ensures that the hash is always a valid format and is extremely unlikely to collide with any existing transaction.
1-55: Consider adding more test cases for improved coverage.The current test suite covers the basic successful and unsuccessful scenarios well. To further improve the robustness of the tests, consider adding the following cases:
- Test with a transaction that deploys a contract (check
contractAddressin the receipt).- Test with a transaction that emits events (verify
logsandlogsBloom).- Test error handling (e.g., when the node is not available or returns an error).
- Test with different gas usage scenarios to verify
gasUsedandcumulativeGasUsedcalculations.These additional tests would provide more comprehensive coverage of the
ethGetTransactionReceiptHandlerfunctionality and help catch potential edge cases.packages/actions/src/eth/ethGetTransactionREceiptProcedure.spec.ts (3)
9-53: LGTM: Comprehensive test for successful transaction receipt retrieval.The test case thoroughly covers the scenario where a transaction receipt is successfully retrieved. It simulates the entire process from sending a transaction to mining a block and then fetching the receipt. The assertions are comprehensive and cover all relevant fields of the transaction receipt.
Consider adding an assertion for the
effectiveGasPricefield in the transaction receipt, as it's a standard field in Ethereum transaction receipts:effectiveGasPrice: expect.any(String),This addition would make the test even more comprehensive.
55-77: LGTM: Good test for non-existent transaction receipt.This test case effectively covers the scenario where a transaction receipt is not found. It correctly asserts that the result is null when requesting a non-existent transaction receipt.
To make the test more robust, consider using a randomly generated transaction hash instead of a hardcoded one. This can help prevent any potential false positives if the hardcoded hash accidentally matches a real transaction in the future. Here's a suggestion:
import { randomBytes } from 'crypto' // ... const nonExistentTxHash = `0x${randomBytes(32).toString('hex')}` const request = { // ... params: [nonExistentTxHash], // ... }This change would make the test more resilient to potential future changes in the test environment.
1-78: Great job on the test coverage! Consider adding more edge cases.The test file provides excellent coverage for the main scenarios of the
ethGetTransactionReceiptJsonRpcProcedure. Both the successful retrieval of a transaction receipt and the handling of a non-existent transaction are well tested.To further enhance the test suite, consider adding the following test cases:
- Test with a pending transaction (not yet mined).
- Test with a transaction that has been reverted.
- Test with invalid input parameters (e.g., malformed transaction hash).
- Test concurrent requests for multiple transaction receipts.
These additional tests would cover more edge cases and error scenarios, making the test suite even more robust.
packages/actions/src/eth/ethGetTransactionByHashProcedure.spec.ts (2)
8-62: LGTM: Comprehensive test for successful transaction retrieval.This test case thoroughly covers the happy path scenario for retrieving a transaction. It correctly sets up the environment, creates a transaction, mines a block, and then verifies the retrieved transaction details.
The use of regex for
blockHashexpectation is a good practice for handling dynamic values.Consider adding a brief comment explaining the purpose of
skipBalance: truein the transaction creation for better clarity:const callResult = await callHandler(client)({ createTransaction: true, to, value: 420n, + // Skip balance check for testing purposes skipBalance: true, })
1-90: Great job on the test coverage! Consider adding more edge cases.The test suite for
ethGetTransactionByHashJsonRpcProcedureis well-structured and covers both the success and error scenarios effectively. The tests are comprehensive and follow good testing practices.To further enhance the test coverage, consider adding the following test cases:
- Test with a pending transaction (not yet mined).
- Test with a transaction from a different block (not the latest).
- Test with invalid input formats (e.g., malformed transaction hash).
- Test the behavior when the node is in a specific state (e.g., syncing).
These additional tests would help ensure the robustness of the
ethGetTransactionByHashJsonRpcProcedurefunction across various scenarios.packages/actions/src/eth/ethGetTransactionByBlockHashAndIndexProcedure.spec.ts (3)
9-63: LGTM: Comprehensive test case for successful transaction retrieval.This test case thoroughly covers the scenario where a transaction is successfully retrieved. The setup, execution, and assertions are well-structured and comprehensive.
Consider adding a comment explaining the purpose of
skipBalance: truein thecallHandleroptions. This would improve the readability and understanding of the test setup.
65-95: LGTM: Well-structured test case for transaction not found scenario.This test case effectively covers the scenario where a transaction is not found, ensuring the correct error response is returned.
Consider adding the following test scenarios to improve coverage:
- Test with an invalid block hash.
- Test with an out-of-range index for a block with transactions.
- Test with various edge cases for the index (e.g., negative values, very large values).
These additional scenarios would help ensure the robustness of the
ethGetTransactionByBlockHashAndIndexJsonRpcProcedurefunction.
1-95: Great job on increasing test coverage!This new test file for
ethGetTransactionByBlockHashAndIndexJsonRpcProcedureis well-structured, comprehensive, and aligns perfectly with the PR objective of increasing test coverage. The tests cover both successful and error scenarios, providing a solid foundation for ensuring the reliability of the procedure.To further enhance the test suite:
- Consider implementing property-based testing for more exhaustive input combinations.
- Explore edge cases such as maximum gas limits, unusual transaction types, or interactions with other EVM features.
- If applicable, add integration tests that verify this procedure's interaction with other components of the system.
These additions would further strengthen the robustness of your testing strategy and align with best practices in Ethereum development.
packages/actions/src/eth/ethGetTransactionByBlockNumberAndIndexProcedure.spec.ts (3)
8-62: LGTM: Comprehensive test for successful transaction retrieval.The test case thoroughly covers the scenario of retrieving an existing transaction. It properly sets up the environment, creates a transaction, mines a block, and verifies the retrieved transaction's properties.
Consider adding a comment explaining the significance of the
420nvalue used in the transaction. If it's an arbitrary value for testing, you might want to use a more descriptive constant likeTEST_VALUE_WEI.
64-93: LGTM: Error handling scenario is well-tested.The test case effectively covers the scenario where a transaction is not found. It correctly sets up an empty block and verifies the appropriate error response.
Consider adding a test case for an invalid block number or index to ensure complete error handling coverage.
1-94: LGTM: Well-structured and comprehensive test suite.The test suite effectively covers both successful transaction retrieval and error handling scenarios. The tests are clear, well-organized, and use realistic data to simulate blockchain operations.
Consider adding the following test cases to further improve coverage:
- Retrieving a transaction with a non-zero index.
- Handling of invalid input parameters (e.g., malformed block number or index).
- Edge cases such as retrieving transactions from the genesis block or the latest block.
packages/actions/src/eth/getStorageAtHandler.spec.ts (4)
33-74: LGTM: Comprehensive coverage of block tag scenarios.These test cases effectively cover different block tag scenarios, including 'pending' and a specific block number. The inclusion of block mining in the third test is particularly good for testing historical state retrieval.
Consider adding a test case for an 'earliest' block tag to cover all possible block tag values. This would ensure complete coverage of the API's capabilities.
88-104: LGTM: Good error handling test for storage retrieval.This test case effectively verifies that errors from
getContractStorageare properly handled and propagated. The use of mocking to simulate the error condition is appropriate.Consider adding a more specific error message in the expectation:
).rejects.toThrow('Storage retrieval error')This would ensure that the exact error message is being propagated, providing an extra layer of verification.
106-122: LGTM: Good error handling test for block retrieval.This test case effectively verifies that errors from
blockchain.getBlockByTagare properly handled and propagated. The use of mocking to simulate the error condition is appropriate.Similar to the previous test, consider adding a more specific error message in the expectation:
).rejects.toThrow('Block retrieval error')This would ensure that the exact error message is being propagated, providing an extra layer of verification.
1-123: Great job on the comprehensive test suite!The test suite for
getStorageAtHandleris well-structured and covers a wide range of scenarios, including basic functionality, different block tags, error handling, and edge cases. The use of constants and helper functions enhances readability and maintainability.To further improve the test suite, consider the following suggestions:
- Add a test case for the 'earliest' block tag to ensure complete coverage of all possible block tag values.
- Include boundary tests for extremely large or small storage values and positions.
- Test the function's behavior with invalid input types or formats.
- Consider adding a test for concurrent storage operations to ensure thread safety if applicable.
These additions would provide even more robust coverage and help catch potential edge cases.
packages/actions/src/SetAccount/validateSetAccountParams.spec.ts (3)
26-95: LGTM: Comprehensive coverage of error scenarios.These test cases effectively cover various error scenarios, each focusing on a specific invalid input. The use of specific error type checks ensures that the function is throwing the correct errors for each scenario.
Consider adding a brief comment before each test case to explain the specific scenario being tested. This can improve readability and maintainability of the test suite.
97-114: LGTM: Excellent test for multiple error scenarios.This test case effectively verifies the function's ability to handle and report multiple errors simultaneously. It's a crucial test for ensuring comprehensive error checking.
Consider using
expect.arrayContaining()instead of individualexpect().some()calls. This can make the test more concise and easier to read. For example:expect(errors).toEqual( expect.arrayContaining([ expect.any(InvalidAddressError), expect.any(InvalidNonceError), // ... other error types ]) )
127-133: LGTM: Good test for handling partial input.This test case effectively verifies that the function can handle partial input with only the required 'address' field. It's crucial to ensure that undefined optional fields don't cause errors.
Consider adding more variations of this test case with different combinations of optional fields. This would provide even more comprehensive coverage of partial input scenarios. For example:
it('should handle various combinations of optional fields', () => { const testCases = [ { address: '0x1234567890123456789012345678901234567890' as const }, { address: '0x1234567890123456789012345678901234567890' as const, nonce: 1n }, { address: '0x1234567890123456789012345678901234567890' as const, balance: 100n }, // Add more combinations as needed ] testCases.forEach(params => { const errors = validateSetAccountParams(params) expect(errors).toHaveLength(0) }) })packages/actions/src/eth/EthJsonRpcRequest.ts (2)
Line range hint
255-266: Readonly tuple inEthSignTransactionJsonRpcRequestwith potential for further improvementThe change to use a readonly tuple in
EthSignTransactionJsonRpcRequestis consistent with the previous modifications and improves type safety. However, consider whether the inner object should also be made readonly for complete immutability. This would prevent accidental modifications to the object properties as well.Example:
readonly [{ readonly from: Address readonly to?: Address readonly gas?: Hex // ... other properties }]This suggestion is optional and depends on your team's preferences and use cases.
Line range hint
207-295: Summary of changes: Improved type safety across Ethereum JSON-RPC request typesThe changes in this file consistently apply readonly tuples to various Ethereum JSON-RPC request type definitions. These modifications:
- Enhance type safety by preventing accidental modifications to parameters.
- Align with TypeScript best practices for immutable data structures.
- Contribute to improved test coverage by potentially catching issues related to unintended parameter modifications.
- Maintain consistency across all type definitions, improving code maintainability and readability.
These changes align well with the PR objective of improving test coverage and overall code quality. The consistent application of this pattern across all relevant type definitions is commendable.
Consider applying this pattern of using readonly tuples to other similar type definitions in the project, if not already done, to maintain consistency and further improve type safety across the codebase.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (11)
- packages/actions/src/Mine/processTx.spec.ts (1 hunks)
- packages/actions/src/SetAccount/validateSetAccountParams.spec.ts (1 hunks)
- packages/actions/src/eth/EthJsonRpcRequest.ts (4 hunks)
- packages/actions/src/eth/ethAccountsProcedure.spec.ts (1 hunks)
- packages/actions/src/eth/ethGetTransactionByBlockHashAndIndexProcedure.spec.ts (1 hunks)
- packages/actions/src/eth/ethGetTransactionByBlockNumberAndIndexProcedure.spec.ts (1 hunks)
- packages/actions/src/eth/ethGetTransactionByHashProcedure.spec.ts (1 hunks)
- packages/actions/src/eth/ethGetTransactionREceiptProcedure.spec.ts (1 hunks)
- packages/actions/src/eth/ethGetTransactionReceipt.spec.ts (1 hunks)
- packages/actions/src/eth/ethSignProcedure.spec.ts (1 hunks)
- packages/actions/src/eth/getStorageAtHandler.spec.ts (1 hunks)
Additional comments not posted (27)
packages/actions/src/eth/ethAccountsProcedure.spec.ts (3)
1-3: LGTM: Imports are appropriate and consistent.The imports are well-structured and appropriate for a test file. The use of the '.js' extension in imports is consistent with ECMAScript modules.
5-5: LGTM: Well-structured describe block.The describe block is appropriately named and follows best practices for organizing tests.
6-15: LGTM: Comprehensive test for JSON-RPC response with id.This test case thoroughly checks the structure and content of the JSON-RPC response when the request includes an 'id'. The expectations are comprehensive and the use of 'as const' ensures type safety.
packages/actions/src/Mine/processTx.spec.ts (3)
1-6: LGTM: Imports are well-organized and relevant.The imports are appropriately chosen for the test case, including necessary utilities from the TEVM ecosystem and the Vitest testing framework.
8-9: LGTM: Well-structured test suite.The test suite is properly organized using
describeanditblocks. The async nature of the test case is correctly handled.Also applies to: 29-29
1-29: Overall assessment: Good start with room for improvement.This new test file for
processTxprovides a solid foundation for testing the transaction processing functionality. The test is well-structured and covers the basic happy path scenario. To further enhance the test suite:
- Add more test cases to cover different scenarios and edge cases.
- Enhance assertions to verify more details of the transaction processing.
- Consider testing error handling and failure scenarios.
These improvements will increase the robustness and reliability of the test suite, ensuring better coverage of the
processTxfunction's behavior under various conditions.packages/actions/src/eth/ethSignProcedure.spec.ts (2)
1-3: LGTM: Imports are appropriate for the test file.The import statements are concise and import only the necessary functions and data for the tests.
1-39: Overall, well-structured and effective test suite with room for expansion.This test file for
ethSignProcedureis well-organized and covers essential scenarios. The use of Vitest and appropriate testing patterns demonstrates good testing practices. The suggestions provided for minor improvements and additional test cases will further enhance the robustness and coverage of the test suite.Great job on increasing the test coverage! This contribution will help improve the overall quality and reliability of the project.
packages/actions/src/eth/ethGetTransactionReceipt.spec.ts (1)
1-7: LGTM: Import statements are appropriate and well-structured.The import statements are concise and import only the necessary dependencies. The use of
typefor theHeximport from 'viem' is a good practice for type-only imports.packages/actions/src/eth/ethGetTransactionREceiptProcedure.spec.ts (1)
1-8: LGTM: Imports and test suite setup look good.The import statements cover all necessary dependencies, and the describe block is properly structured for the test suite.
packages/actions/src/eth/ethGetTransactionByHashProcedure.spec.ts (2)
1-7: LGTM: Imports and structure are well-organized.The imports are appropriate for the testing scenario, including the necessary functions from the project and the Vitest testing framework. The overall structure follows good testing practices with a describe block containing individual test cases.
64-90: LGTM: Well-structured test for non-existent transaction scenario.This test case effectively covers the error scenario when attempting to retrieve a non-existent transaction. It correctly sets up the environment with an empty block and verifies the expected error response, including the specific error code and message.
The use of a hardcoded transaction hash for the non-existent transaction is appropriate for this test scenario.
packages/actions/src/eth/ethGetTransactionByBlockHashAndIndexProcedure.spec.ts (2)
1-6: LGTM: Import statements are well-organized and relevant.The import statements are logically ordered and include all necessary dependencies for the test file. Good job on keeping the imports clean and relevant.
8-8: LGTM: Test suite setup is clear and focused.The describe block accurately identifies the function being tested, providing a clear structure for the test cases.
packages/actions/src/eth/ethGetTransactionByBlockNumberAndIndexProcedure.spec.ts (1)
1-7: LGTM: Imports and test structure are well-organized.The import statements cover all necessary dependencies, and the describe block is properly set up for the
ethGetTransactionByBlockNumberAndIndexJsonRpcProceduretests.packages/actions/src/eth/getStorageAtHandler.spec.ts (3)
1-10: LGTM: Imports and constants are well-structured.The imports cover all necessary dependencies for the test suite, and the constants are clearly defined with descriptive names. This setup provides a solid foundation for the test cases.
12-31: LGTM: Well-structured test for basic functionality.This test case effectively covers the primary use case of
getStorageAtHandler. It sets up the necessary state, calls the function with appropriate parameters, and verifies the expected result. The use of constants enhances readability and maintainability.
76-86: LGTM: Good coverage of non-existent storage scenario.This test case effectively verifies the behavior of
getStorageAtHandlerwhen querying non-existent storage. It's crucial to test this edge case to ensure the function behaves correctly in all scenarios.packages/actions/src/SetAccount/validateSetAccountParams.spec.ts (4)
1-10: LGTM: Imports are appropriate and comprehensive.The import statements are well-structured, importing necessary error types, testing utilities, and the function under test. This setup provides a solid foundation for the test suite.
13-24: LGTM: Comprehensive test for valid input.This test case effectively covers the scenario where all input parameters are valid. It's a good practice to start with a positive test case. The use of 'as const' for certain fields enhances type safety.
116-125: LGTM: Good coverage of edge case.This test case effectively covers the scenario where the 'throwOnFail' parameter is invalid. It's important to test such edge cases to ensure comprehensive input validation.
1-134: Overall: Excellent test coverage with room for minor enhancements.This test suite provides comprehensive coverage for the
validateSetAccountParamsfunction. It effectively tests various scenarios including valid input, individual error cases, multiple errors, edge cases, and partial input. The tests are well-structured and use appropriate assertions.To further improve the test suite:
- Consider adding brief comments before each test case to explain the specific scenario being tested.
- In the multiple errors test case, consider using
expect.arrayContaining()for more concise assertions.- Expand the partial input test to cover more combinations of optional fields.
These enhancements would make the test suite even more robust and maintainable.
packages/actions/src/eth/EthJsonRpcRequest.ts (5)
207-207: Improved type safety forEthGetTransactionReceiptJsonRpcRequestThe change to make the tuple parameter readonly is a good improvement. It enhances type safety by preventing accidental modifications to the parameter, which can help catch potential issues in tests and usage of this type.
243-243: Consistent improvement inEthSendTransactionJsonRpcRequestThe modification to use a readonly tuple for the
EthSendTransactionJsonRpcRequesttype is consistent with the previous change. This continues to enhance type safety and aligns with TypeScript best practices for immutable data structures.
248-248: Consistent readonly tuple usage inEthSignJsonRpcRequestThe change to
EthSignJsonRpcRequestfollows the same pattern of using a readonly tuple. This consistency across type definitions is commendable, as it improves maintainability and makes the codebase easier to understand.
277-277: Readonly tuple inEthNewFilterJsonRpcRequestwith serialized contentThe change to use a readonly tuple in
EthNewFilterJsonRpcRequestis consistent with the previous modifications and improves type safety. The use ofSerializeToJson<FilterParams>suggests that the inner structure is already being transformed, which may provide some level of immutability. This combination of readonly tuple and serialization offers good protection against unintended modifications.
295-295: Completion of readonly tuple pattern withEthUninstallFilterJsonRpcRequestThe change to use a readonly tuple in
EthUninstallFilterJsonRpcRequestcompletes the pattern of making all tuple parameters readonly in this file. This consistent approach across all type definitions enhances type safety, prevents accidental modifications to parameters, and aligns with TypeScript best practices for immutable data structures. These changes collectively contribute to a more robust and maintainable codebase.
| describe('ethSignProcedure', () => { | ||
| it('should sign a message', async () => { | ||
| const data = '0x42069' | ||
| const request = { | ||
| id: 1, | ||
| jsonrpc: '2.0', | ||
| method: 'eth_sign', | ||
| params: [testAccounts[0].address, data], | ||
| } as const | ||
|
|
||
| const procedure = ethSignProcedure(testAccounts) | ||
| const response = await procedure(request) | ||
|
|
||
| expect(response).toEqual({ | ||
| id: 1, | ||
| jsonrpc: '2.0', | ||
| method: 'eth_sign', | ||
| result: await testAccounts[0].signMessage({ message: data }), | ||
| }) | ||
| }) | ||
|
|
||
| it("should throw an error if account doesn't exist", async () => { | ||
| const data = '0x42069' | ||
| const request = { | ||
| id: 1, | ||
| jsonrpc: '2.0', | ||
| method: 'eth_sign', | ||
| params: [`0x${'69'.repeat(20)}`, data], | ||
| } as const | ||
|
|
||
| const procedure = ethSignProcedure(testAccounts) | ||
|
|
||
| await expect(procedure(request)).rejects.toThrow('Account 0x6969696969696969696969696969696969696969 not found') | ||
| }) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding more test cases for improved coverage.
The current test suite covers the basic functionality and one error scenario, which is a good start. To enhance the robustness of the tests, consider adding the following test cases:
- Test with different types of messages (e.g., empty string, very long string, special characters).
- Test with different accounts from the
testAccountsarray to ensure consistency across accounts. - Test the behavior when invalid data is provided (e.g., non-hex string).
- Test with edge cases for the
idandjsonrpcfields in the request object.
These additional test cases will help ensure that ethSignProcedure behaves correctly under various conditions and increase confidence in its reliability.
| import { createTevmNode } from '@tevm/node' | ||
| import { describe, expect, it } from 'vitest' | ||
| import { callHandler } from '../Call/callHandler.js' | ||
| import { mineHandler } from '../Mine/mineHandler.js' | ||
| import { ethGetTransactionByBlockNumberAndIndexJsonRpcProcedure } from './ethGetTransactionByBlockNumberAndIndexProcedure.js' | ||
|
|
||
| describe('ethGetTransactionByBlockNumberAndIndexJsonRpcProcedure', () => { | ||
| it('should return the transaction if found', async () => { | ||
| const client = createTevmNode() | ||
| const to = `0x${'69'.repeat(20)}` as const | ||
|
|
||
| // Send a transaction | ||
| const callResult = await callHandler(client)({ | ||
| createTransaction: true, | ||
| to, | ||
| value: 420n, | ||
| skipBalance: true, | ||
| }) | ||
|
|
||
| // Mine a block to include the transaction | ||
| await mineHandler(client)({}) | ||
|
|
||
| // Get the block number | ||
| const block = await client.getVm().then((vm) => vm.blockchain.getCanonicalHeadBlock()) | ||
| const blockNumber = `0x${block.header.number.toString(16)}` as const | ||
|
|
||
| const request = { | ||
| id: 1, | ||
| method: 'eth_getTransactionByBlockNumberAndIndex', | ||
| params: [blockNumber, '0x0'], | ||
| jsonrpc: '2.0', | ||
| } as const | ||
|
|
||
| const procedure = ethGetTransactionByBlockNumberAndIndexJsonRpcProcedure(client) | ||
| const response = await procedure(request) | ||
|
|
||
| expect(response.result?.hash).toEqual(callResult.txHash) | ||
| expect(response).toMatchObject({ | ||
| id: 1, | ||
| jsonrpc: '2.0', | ||
| method: 'eth_getTransactionByBlockNumberAndIndex', | ||
| result: { | ||
| accessList: [], | ||
| blobVersionedHashes: undefined, | ||
| blockHash: expect.stringMatching(/^0x[a-fA-F0-9]{64}$/), | ||
| blockNumber: blockNumber, | ||
| data: '0x', | ||
| from: '0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266', | ||
| gas: '0x5a3c', | ||
| gasPrice: '0x7', | ||
| hash: '0x5e5b342fae6b13548e62c3038078915397ebd2406a8c67afd276e8dc84ebba80', | ||
| maxFeePerBlobGas: undefined, | ||
| maxFeePerGas: '0x7', | ||
| maxPriorityFeePerGas: '0x0', | ||
| nonce: '0x0', | ||
| to: '0x6969696969696969696969696969696969696969', | ||
| transactionIndex: '0x0', | ||
| type: '0x2', | ||
| value: '0x1a4', | ||
| }, | ||
| }) | ||
| }) | ||
|
|
||
| it('should return an error if the transaction is not found', async () => { | ||
| const client = createTevmNode() | ||
|
|
||
| // Mine a block without any transactions | ||
| await mineHandler(client)({}) | ||
|
|
||
| // Get the block number | ||
| const block = await client.getVm().then((vm) => vm.blockchain.getCanonicalHeadBlock()) | ||
| const blockNumber = `0x${block.header.number.toString(16)}` as const | ||
|
|
||
| const request = { | ||
| id: 1, | ||
| method: 'eth_getTransactionByBlockNumberAndIndex', | ||
| params: [blockNumber, '0x0'], | ||
| jsonrpc: '2.0', | ||
| } as const | ||
|
|
||
| const procedure = ethGetTransactionByBlockNumberAndIndexJsonRpcProcedure(client) | ||
| const response = await procedure(request) | ||
|
|
||
| expect(response).toEqual({ | ||
| id: 1, | ||
| jsonrpc: '2.0', | ||
| method: 'eth_getTransactionByBlockNumberAndIndex', | ||
| error: { | ||
| code: -32602, | ||
| message: 'Transaction not found', | ||
| }, | ||
| }) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance code quality with constants and type improvements.
While the code is generally well-written, there are a few areas where it could be improved:
-
Consider replacing magic numbers and strings with named constants. For example:
const TEST_ADDRESS = `0x${'69'.repeat(20)}` as const; const TEST_VALUE_WEI = 420n;
-
The use of
as constis good for type safety, but be cautious of overuse. In some cases, like forrequest, it might be better to define an interface or type for the request structure. -
Consider using more descriptive names for some variables. For example,
blockcould belatestBlockto make its purpose clearer.
These changes will improve code readability and maintainability.
This reverts commit da4a743.
roninjin10
deleted the
09-24-_white_check_mark_test_actions_more_test_coverage
branch
Description
Concise description of proposed changes
Testing
Explain the quality checks that have been done on the code changes
Additional Information
Your ENS/address:
Summary by CodeRabbit
New Features
Bug Fixes
Tests
processTx,validateSetAccountParams,ethAccountsProcedure, and others to cover both successful and error scenarios.