🧪 Test: Add more test coverage to actions #1466
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
|
|
Warning Rate limit exceeded@roninjin10 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 32 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThis pull request introduces a comprehensive suite of tests for various procedures related to Ethereum transaction handling, including Changes
Possibly related issues
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 (
|
|
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @roninjin10 and the rest of your teammates on |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (22)
packages/actions/src/debug/debugTraceCallProcedure.spec.ts (1)
1-42: Summary: Good start, but room for improvement in test coverage and assertions.This test provides a basic check for the
debugTraceCallJsonRpcProcedure. However, to enhance its effectiveness:
- Address the TODO comment by expanding the test to cover more scenarios or trace actual operations.
- Add explicit assertions for critical values alongside the snapshot.
- Consider adding more test cases to cover different input parameters and edge cases.
These improvements will lead to more robust and maintainable tests for this critical functionality.
packages/actions/src/debug/debugTraceTransactionProcedure.spec.ts (4)
9-17: LGTM: Test setup is well-structured. Consider adding a descriptive comment.The test setup is well-structured, creating a TEVM node with appropriate configuration and deploying a contract. This provides a good foundation for the test.
Consider adding a brief comment explaining the purpose of the
SimpleContractand why it's initialized with the specific address420. This would enhance the readability and maintainability of the test.+// Initialize SimpleContract with a deterministic address for consistent testing const contract = SimpleContract.withAddress(createAddress(420).toString())
18-25: Enhance error handling with more descriptive message.The error handling for a failed transaction is good practice. However, the error message could be more informative to aid in debugging.
Consider enhancing the error message to include more context:
if (!sendTxResult.txHash) { - throw new Error('Transaction failed') + throw new Error('Transaction failed: Unable to obtain transaction hash') }This more descriptive error message will make it easier to diagnose issues if the test fails at this point.
27-53: LGTM: Test execution and assertion are well-structured. Consider additional verifications.The test execution and assertion using an inline snapshot is a good practice, especially for complex objects. This ensures that the structure of the response remains consistent.
Consider adding more specific assertions to verify the content of important fields:
- Check that
failedis false.- Verify that
gasis a non-zero value.- Ensure that
structLogsis an empty array as expected.Example:
expect(result.result.failed).toBe(false); expect(BigInt(result.result.gas)).toBeGreaterThan(0n); expect(result.result.structLogs).toEqual([]);These additional assertions will make the test more robust and catch potential issues that might not be apparent from the snapshot alone.
1-53: Overall, well-structured test with room for expansion.This test file provides a good foundation for testing the
debugTraceTransactionJsonRpcProcedure. It covers the basic happy path scenario effectively.For future improvements, consider:
- Adding tests for error cases (e.g., invalid transaction hash, unsupported tracer).
- Testing with different tracer options to ensure the procedure handles various configurations correctly.
- Verifying the behavior with more complex contracts or transactions to ensure robustness.
These additions would further enhance the test coverage and ensure the procedure works correctly under various scenarios.
packages/actions/src/eth/getCodeHandler.spec.ts (3)
28-42: LGTM: Well-structured test for mainnet fork. Consider enhancing assertions.The test case for fetching code from a mainnet fork is well-implemented. It uses a real contract address and appropriate assertions to verify the fetched code.
Consider enhancing the test with more specific assertions:
- Check for a known bytecode prefix of the Uniswap V2 Router contract.
- Verify the exact length of the fetched bytecode against the known deployed bytecode length.
Example:
expect(code.startsWith('0x60806040')).toBe(true) expect(code.length).toBe(21268) // Replace with actual bytecode lengthThis would make the test more robust against potential issues in the forking mechanism.
44-58: LGTM: Good test for Optimism fork. Consider enhancements for consistency and robustness.The test case for fetching code from an Optimism fork is well-implemented, using a real contract address and appropriate basic assertions.
Consider the following enhancements for consistency and robustness:
- Align the assertions with the suggested improvements for the mainnet fork test.
- Add a comment explaining the significance of the Optimism Bridge contract.
- Consider parameterizing the tests to reduce duplication.
Example of parameterized tests:
describe('Forking tests', () => { const testCases = [ { network: 'mainnet', transport: transports.mainnet, contractAddress: '0x7a250d5630B4cF539739dF2C5dAcb4c659F2488D', contractName: 'Uniswap V2 Router' }, { network: 'optimism', transport: transports.optimism, contractAddress: '0x4200000000000000000000000000000000000010', contractName: 'Optimism Bridge' } ] testCases.forEach(({ network, transport, contractAddress, contractName }) => { it(`should fetch code from ${network} fork`, async () => { const forkedClient = createTevmNode({ fork: { transport } }) const forkedHandler = getCodeHandler(forkedClient) const code = await forkedHandler({ address: contractAddress, blockTag: 'latest' }) expect(code).not.toBe('0x') expect(code.length).toBeGreaterThan(2) // Add more specific assertions here }) }) })This approach would make it easier to add tests for additional networks in the future.
27-59: Overall, good addition of forking tests. Consider further enhancements.The new tests for forking functionality significantly improve the coverage of the
getCodeHandler. They test both mainnet and Optimism networks, which is great for ensuring cross-chain compatibility.To further improve these tests, consider:
- Implementing the suggested parameterization to make the tests more maintainable and extensible.
- Adding more specific assertions as mentioned in previous comments.
- Including edge cases, such as testing with non-existent contracts or at specific historical blocks.
- Adding a test for a case where the code should be fetched from the local state instead of the fork, to ensure the forking logic works correctly in all scenarios.
These enhancements will make the test suite more robust and provide better coverage of potential edge cases.
packages/actions/src/eth/ethSendRawTransactionJsonRpcProcedure.spec.ts (2)
10-39: Well-structured test case for handling a valid signed transaction.The test case effectively covers the happy path for processing a valid signed transaction. It correctly sets up the environment, constructs a transaction with realistic parameters, and verifies the expected outcome.
Consider adding a test assertion to verify that the transaction was actually added to the node's pending transactions or mined into a block. This would provide a more comprehensive check of the procedure's functionality.
// Add after the existing expect statement const pendingTransactions = await client.eth.getPendingTransactions() expect(pendingTransactions).toContainEqual(expect.objectContaining({ hash: signedTx.hash() }))
41-69: Good coverage for legacy transaction handling.This test case effectively demonstrates the procedure's ability to handle legacy transactions (type 0). The use of appropriate transaction parameters and consistent assertion strategy is commendable.
To enhance test coverage, consider adding a test case for an EIP-1559 transaction (type 2) with an access list. This would ensure that the procedure correctly handles all transaction types. Here's a suggested structure:
it('should handle an EIP-1559 transaction with access list', async () => { const client = createTevmNode() const procedure = ethSendRawTransactionJsonRpcProcedure(client) const tx = TransactionFactory.fromTxData( { nonce: '0x00', maxFeePerGas: '0x09184e72a000', maxPriorityFeePerGas: '0x09184e72a000', gasLimit: '0x2710', to: createAddress(`0x${'42'.repeat(20)}`), value: parseEther('1'), data: '0x', accessList: [ { address: createAddress(`0x${'11'.repeat(20)}`), storageKeys: ['0x0000000000000000000000000000000000000000000000000000000000000000'] } ], type: 2, }, { common: tevmDefault.ethjsCommon }, ) // Rest of the test follows the same pattern as existing tests })packages/actions/src/eth/ethSendTransactionHandler.spec.ts (4)
22-44: LGTM: Comprehensive test for simple transaction. Consider additional verification.The test case effectively covers the basic functionality of sending a transaction, including setup, execution, and verification of results. It checks both the transaction hash format and the resulting account balance.
Consider adding a verification step for the sender's balance after the transaction to ensure it's correctly deducted:
const fromAccount = await getAccountHandler(client)({ address: from.toString() }) expect(fromAccount.balance).toBe(parseEther('9'))This would provide a more complete verification of the transaction's effects on both sender and receiver.
46-84: LGTM: Thorough test for contract interaction. Consider error handling scenarios.The test case effectively covers the complex scenario of contract interaction, including setup, execution, and verification of the contract state change. It demonstrates a good understanding of the full process of interacting with a smart contract.
To enhance the robustness of your tests, consider adding scenarios that test error handling. For example:
- Attempt to send a transaction with insufficient balance:
it('should handle insufficient balance', async () => { const from = createAddress('0x1234') await setAccountHandler(client)({ address: from.toString(), balance: parseEther('0.5'), }) await expect(handler({ from: from.toString(), to: createAddress('0x5678').toString(), value: parseEther('1'), })).rejects.toThrow() })
- Attempt to interact with a non-existent contract:
it('should handle non-existent contract', async () => { const from = createAddress('0x1234') const nonExistentContract = createAddress('0x9999') await setAccountHandler(client)({ address: from.toString(), balance: parseEther('10'), }) await expect(handler({ from: from.toString(), to: nonExistentContract.toString(), data: '0x1234567890', })).rejects.toThrow() })These additional tests would help ensure that the
ethSendTransactionHandlercorrectly handles error conditions.
1-85: Good test structure. Consider expanding coverage for comprehensive testing.The test suite is well-structured, covering both simple transactions and contract interactions. The use of
beforeEachfor setup and appropriate assertions throughout the tests is commendable.To achieve more comprehensive coverage, consider the following additions:
Edge cases:
- Transactions with zero value
- Transactions with maximum allowed gas
- Transactions to/from special addresses (e.g., zero address)
Error scenarios:
- Invalid input data
- Gas limit exceeded
- Reverted transactions
Different transaction types:
- EIP-1559 transactions
- Legacy transactions
Contract deployment:
- Test deploying a new contract via transaction
Event emission:
- Verify that appropriate events are emitted during transactions
Example for testing a zero-value transaction:
it('should handle zero-value transactions', async () => { const from = createAddress('0x1234') const to = createAddress('0x5678') await setAccountHandler(client)({ address: from.toString(), balance: parseEther('1'), }) const result = await handler({ from: from.toString(), to: to.toString(), value: 0n, }) expect(result).toMatch(/^0x[a-fA-F0-9]{64}$/) await mineHandler(client)() const toAccount = await getAccountHandler(client)({ address: to.toString() }) expect(toAccount.balance).toBe(0n) })These additions would significantly enhance the robustness and reliability of your test suite.
1-85: Effective use of utilities. Consider creating additional helper functions.The test file makes good use of utility functions and custom handlers, which enhances readability and maintainability. Functions like
createAddress,parseEther, and the custom handlers (setAccountHandler,mineHandler, etc.) provide a clean and consistent way to set up and manipulate the test environment.To further improve the structure and reduce repetition, consider creating additional helper functions:
- Account setup helper:
function setupAccount(address: string, balance: bigint) { return setAccountHandler(client)({ address, balance, }) }
- Transaction sender helper:
async function sendTransaction(from: string, to: string, value: bigint, data?: string) { const result = await handler({ from, to, value, data }) await mineHandler(client)() return result }
- Contract state checker:
async function checkContractState(address: string, expectedValue: bigint) { const { data } = await contractHandler(client)({ to: address, abi: SimpleContract.abi, functionName: 'get', }) expect(data).toBe(expectedValue) }Using these helpers, you could simplify your tests:
it('should handle contract interaction', async () => { const from = createAddress('0x1234') const contractAddress = createAddress('0x5678') await setupAccount(from.toString(), parseEther('10')) await setupAccount(contractAddress.toString(), 0n, SimpleContract.deployedBytecode) const data = encodeFunctionData({ abi: SimpleContract.abi, functionName: 'set', args: [42n], }) const result = await sendTransaction(from.toString(), contractAddress.toString(), 0n, data) expect(result).toMatch(/^0x[a-fA-F0-9]{64}$/) await checkContractState(contractAddress.toString(), 42n) })These helpers would make your tests more concise and easier to read, while also reducing the potential for errors in test setup and verification.
packages/actions/src/eth/ethGetLogsHandler.js (3)
72-72: Simplified error handling approved, consider adding context.The direct error throwing simplifies the code and allows for more accurate error propagation. This change is beneficial for debugging and error tracking.
Consider adding a comment to provide context that this error occurs during a forked operation. This can help maintain the context that was previously implied by the ForkError wrapper.
+ // Error occurred while fetching logs from the forked chain throw error
91-91: Approved: Improved handling of undefined jsonRpcLogs.The use of the nullish coalescing operator (
??) is a good practice for handling potentially undefined values. It ensures thatundefinedis explicitly returned whenjsonRpcLogsis not available, improving type safety and clarity.For even better clarity, consider using an explicit type guard:
- const typedLogs = - /** - * @type {Array<Log> | undefined} - */ - (jsonRpcLogs ?? undefined) + const typedLogs: Array<Log> | undefined = Array.isArray(jsonRpcLogs) ? jsonRpcLogs : undefinedThis approach combines runtime type checking with TypeScript's type system, providing an additional layer of safety.
Line range hint
1-138: Overall improvements in error handling and type safety.The changes made to this file are positive steps towards better error handling and type safety. The simplification of error throwing and the improved handling of undefined values are good practices.
For future improvements, consider:
- Reviewing the error handling strategy throughout the function for consistency.
- Adding more type annotations, especially for complex objects like
params.- Exploring opportunities to use TypeScript's discriminated unions for more precise type checking, particularly around the
filterParamsobject.- Evaluating the TODO comments (e.g., "TODO support EIP-234" and "TODO make this more parallized") to determine if they can be addressed in future updates.
These suggestions could further enhance the robustness and maintainability of the code.
packages/actions/src/eth/getBalanceHandler.spec.ts (1)
57-58: Clarify comment about setting up a forked clientThe comment "// This test assumes you have a way to set up a forked client" may be unnecessary since the code below shows how to set up the forked client. Consider removing or rephrasing it for clarity.
-// This test assumes you have a way to set up a forked clientpackages/actions/src/debug/debugTraceTransactionProcedure.js (2)
Line range hint
52-52: Address the TODO by using@tevm/errorsfor consistent error handlingThe TODO comment on line 52 suggests using
@tevm/errorsfor error handling. Implementing this will standardize error codes and messages across the codebase, improving maintainability and readability.Would you like me to help implement this change or open a GitHub issue to track this task?
Line range hint
43-57: EnsureforkAndCacheBlockloads the state root before proceedingAfter invoking
forkAndCacheBlock, the code proceeds without re-verifying if the state root has been successfully loaded. IfforkAndCacheBlockfails or doesn't load the state root as expected, the subsequent call tosetStateRootmay fail or cause unintended errors. Consider recheckinghasStateRootafterforkAndCacheBlock, or add error handling to ensure that the state root is available before proceeding.packages/actions/src/eth/ethGetLogsHandler.spec.ts (2)
58-70: Optimize Mining Operations Inside the LoopIn the loop where you emit events and mine a block after each transaction, calling
await mineHandler(client)()inside the loop can be inefficient.Suggestion:
Consider moving the mining operation outside the loop to mine a single block after all transactions have been sent. This will improve test performance without affecting the test's outcome.
for (let i = 0; i < 3; i++) { const res = await callHandler(client)({ to: contractAddress, from: from.toString(), data: encodeFunctionData(SimpleContract.write.set(BigInt(i))), createTransaction: true, }) expect(res.logs).toHaveLength(1) - await mineHandler(client)() const { rawData: newValue } = await callHandler(client)({ to: contractAddress, data: encodeFunctionData(SimpleContract.read.get()), }) expect(hexToNumber(newValue)).toBe(i) } +await mineHandler(client)()
Line range hint
37-70: Refactor Duplicate Test Setup CodeThere is repeated code for setting up the client, account, and deploying
SimpleContractacross multiple tests.Recommendation:
Extract the common setup steps into helper functions or
beforeEachhooks to adhere to the DRY (Don't Repeat Yourself) principle. This improves maintainability and readability.Example:
const setupTestEnvironment = async () => { const client = createTevmNode() const from = createAddress(PREFUNDED_ACCOUNTS[0].address) await setupAccount(client, from.toString()) const deployResult = await deployHandler(client)({ abi: SimpleContract.abi, bytecode: SimpleContract.bytecode, args: [0n], from: from.toString(), }) const contractAddress = deployResult.createdAddress as Address await mineHandler(client)() return { client, from, contractAddress } }Then, in your tests:
it('should return logs for a given block range', async () => { const { client, from, contractAddress } = await setupTestEnvironment() // ... rest of the test ... })Also applies to: 93-110, 144-161, 185-204
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
- packages/actions/src/debug/debugTraceCallProcedure.spec.ts (1 hunks)
- packages/actions/src/debug/debugTraceTransactionProcedure.js (2 hunks)
- packages/actions/src/debug/debugTraceTransactionProcedure.spec.ts (1 hunks)
- packages/actions/src/eth/ethGetLogsHandler.js (2 hunks)
- packages/actions/src/eth/ethGetLogsHandler.spec.ts (11 hunks)
- packages/actions/src/eth/ethSendRawTransactionJsonRpcProcedure.spec.ts (1 hunks)
- packages/actions/src/eth/ethSendTransactionHandler.spec.ts (1 hunks)
- packages/actions/src/eth/getBalanceHandler.spec.ts (1 hunks)
- packages/actions/src/eth/getCodeHandler.spec.ts (2 hunks)
🔇 Additional comments (12)
packages/actions/src/debug/debugTraceCallProcedure.spec.ts (2)
1-5: LGTM: Imports are appropriate for the test.The necessary dependencies and the function under test are correctly imported.
9-26: LGTM: Test setup and procedure call are well-structured.The test correctly sets up the client and calls the
debugTraceCallJsonRpcProcedurewith a well-formed JSON-RPC request. The use of helper functions likecreateAddressandparseEtherensures proper formatting of inputs.packages/actions/src/debug/debugTraceTransactionProcedure.spec.ts (1)
1-8: LGTM: Import statements are well-organized and complete.The import statements are logically organized, with external dependencies followed by internal imports. All necessary dependencies for the test are present, and there are no unused imports.
packages/actions/src/eth/getCodeHandler.spec.ts (1)
3-3: LGTM: Import statement addition is appropriate.The new import for
SimpleContractandtransportsfrom@tevm/test-utilsis correctly added and necessary for the new forking tests.packages/actions/src/eth/ethSendRawTransactionJsonRpcProcedure.spec.ts (1)
1-9: Imports and overall structure look good.The necessary dependencies are correctly imported, and the test structure follows best practices using describe and it blocks.
packages/actions/src/eth/ethSendTransactionHandler.spec.ts (1)
1-20: LGTM: Imports and test setup are well-structured.The imports cover all necessary dependencies for the tests, and the
beforeEachblock efficiently sets up the test environment with a new client and handler for each test case.packages/actions/src/eth/getBalanceHandler.spec.ts (5)
23-25: Well-structured test for defaultlatestblock tagThe test correctly validates that the handler fetches the balance from the state manager when no block tag is provided, defaulting to
latest.
28-30: Good check for zero balance on an empty addressThis test ensures that the handler returns
0nfor an address with no balance, which is an important edge case.
54-54: Verify error thrown for non-existent block in non-fork modeEnsure that
NoForkUrlSetErroris the correct error to be thrown when attempting to fetch a balance for a non-existent block in non-fork mode.You can confirm by checking where
NoForkUrlSetErroris defined and if it appropriately represents this error condition.
9-9: Verify the import path ofNoForkUrlSetErrorPlease ensure that
NoForkUrlSetErroris correctly exported from'./getBalanceHandler.js'. If it is defined elsewhere, consider updating the import path to the appropriate module.You can run the following script to verify the export of
NoForkUrlSetError:✅ Verification successful
Verified:
NoForkUrlSetErroris correctly exported from'./getBalanceHandler.js'.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if `NoForkUrlSetError` is exported from `getBalanceHandler.js` # Expected: The export statement for `NoForkUrlSetError` exists in `getBalanceHandler.js` rg --type js "export.*NoForkUrlSetError" ./packages/actions/src/eth/getBalanceHandler.jsLength of output: 137
32-42: Confirm correctness of block numbers in balance retrievalIn the test for fetching balance at specific block numbers, please verify that the block numbers correspond to the correct state changes. There may be an off-by-one error in the block numbering when retrieving balances at
blockTag: 1nandblockTag: 0n.Run the following script to check the block numbers and associated balances:
✅ Verification successful
To ensure that
blockTag: 0nis correctly handled in the balance retrieval, please run the following script:
Block numbers in balance retrieval are correctly handled.
The tests correctly associate
blockTag: 0nwith the initial balance of1andblockTag: 1nwith the updated balance of2. No off-by-one errors were found in the block numbering.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the block numbers and balances # Expected: Balances at block 0 and block 1 match the test expectations echo "Blocks and their balances:" rg --type ts -A 5 "await mineHandler" ./packages/actions/src/eth/getBalanceHandler.spec.tsLength of output: 628
Script:
#!/bin/bash # Description: Verify the usage and correctness of blockTag: 0n # Expected: Balance at blockTag: 0n matches the initial state echo "Checking blockTag: 0n usages and expected balances:" rg --type ts -A 5 "blockTag: 0n" ./packages/actions/src/eth/getBalanceHandler.spec.tsLength of output: 462
packages/actions/src/eth/ethGetLogsHandler.spec.ts (1)
182-182: Verify Expected Block Number in Pending Blocks TestThe assertion on line 182 checks that
logs[0]?.blockNumberis2n.Action Required:
Confirm that the block number should indeed be
2nin this test context. Since the block number for pending transactions might differ depending on the state of the node or previous operations, it may be more reliable to check if the block number is greater than or equal to a specific value or to retrieve the current block number programmatically.- expect(logs[0]?.blockNumber).toBe(2n) + expect(logs[0]?.blockNumber).toBeGreaterThanOrEqual(2n)Alternatively, retrieve the expected block number dynamically:
const currentBlockNumber = await client.getBlockNumber() expect(logs[0]?.blockNumber).toBe(currentBlockNumber)
| import { describe, expect, it } from 'vitest' | ||
| import { debugTraceCallJsonRpcProcedure } from './debugTraceCallProcedure.js' | ||
|
|
||
| // TODO this test kinda sucks because it isn't tracing anything but since the logic is mostly in callHandler which is tested it's fine for now |
There was a problem hiding this comment.
Address the TODO comment to improve test quality.
The TODO comment indicates that the current test might not be comprehensive enough. Consider enhancing the test to cover more scenarios or to trace actual operations.
As a follow-up, could you provide more context on why the current test "kinda sucks"? This information would be helpful in determining how to improve the test coverage.
| expect(result).toMatchInlineSnapshot(` | ||
| { | ||
| "id": 1, | ||
| "jsonrpc": "2.0", | ||
| "method": "debug_traceCall", | ||
| "result": { | ||
| "failed": false, | ||
| "gas": "0x0", | ||
| "returnValue": "0x", | ||
| "structLogs": [], | ||
| }, | ||
| } | ||
| `) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding explicit assertions alongside the snapshot.
While the inline snapshot is useful for capturing the overall structure of the response, it might be beneficial to add explicit assertions for critical values. This would make the test's intentions clearer and easier to maintain.
Consider adding explicit assertions before the snapshot, like this:
expect(result.result.failed).toBe(false)
expect(result.result.gas).toBe('0x0')
expect(result.result.returnValue).toBe('0x')
expect(result.result.structLogs).toEqual([])
// Keep the existing snapshot for overall structure
expect(result).toMatchInlineSnapshot(/* ... */)This approach combines the benefits of snapshot testing with clear, specific assertions.
| import { createAddress } from '@tevm/address' | ||
| import { tevmDefault } from '@tevm/common' | ||
| import { createTevmNode } from '@tevm/node' | ||
| import { BlobEIP4844Transaction, TransactionFactory } from '@tevm/tx' | ||
| import { PREFUNDED_PRIVATE_KEYS, bytesToHex, hexToBytes, parseEther } from '@tevm/utils' | ||
| import { describe, expect, it } from 'vitest' | ||
| import { ethSendRawTransactionJsonRpcProcedure } from './ethSendRawTransactionProcedure.js' | ||
|
|
||
| describe('ethSendRawTransactionJsonRpcProcedure', () => { | ||
| it('should handle a valid signed transaction', async () => { | ||
| const client = createTevmNode() | ||
| const procedure = ethSendRawTransactionJsonRpcProcedure(client) | ||
|
|
||
| const tx = TransactionFactory.fromTxData( | ||
| { | ||
| nonce: '0x00', | ||
| maxFeePerGas: '0x09184e72a000', | ||
| maxPriorityFeePerGas: '0x09184e72a000', | ||
| gasLimit: '0x2710', | ||
| to: createAddress(`0x${'42'.repeat(20)}`), | ||
| value: parseEther('1'), | ||
| data: '0x', | ||
| type: 2, | ||
| }, | ||
| { common: tevmDefault.ethjsCommon }, | ||
| ) | ||
|
|
||
| const signedTx = tx.sign(hexToBytes(PREFUNDED_PRIVATE_KEYS[0])) | ||
| const serializedTx = signedTx.serialize() | ||
|
|
||
| const result = await procedure({ | ||
| jsonrpc: '2.0', | ||
| method: 'eth_sendRawTransaction', | ||
| params: [bytesToHex(serializedTx)], | ||
| id: 1, | ||
| }) | ||
|
|
||
| expect(result.result).toBe(bytesToHex(signedTx.hash())) | ||
| }) | ||
|
|
||
| it('should handle a legacy transaction', async () => { | ||
| const client = createTevmNode() | ||
| const procedure = ethSendRawTransactionJsonRpcProcedure(client) | ||
|
|
||
| const tx = TransactionFactory.fromTxData( | ||
| { | ||
| nonce: '0x00', | ||
| gasPrice: '0x09184e72a000', | ||
| gasLimit: '0x2710', | ||
| to: createAddress(`0x${'42'.repeat(20)}`), | ||
| value: parseEther('1'), | ||
| data: '0x', | ||
| type: 0, | ||
| }, | ||
| { common: tevmDefault.ethjsCommon }, | ||
| ) | ||
|
|
||
| const signedTx = tx.sign(hexToBytes(PREFUNDED_PRIVATE_KEYS[0])) | ||
| const serializedTx = signedTx.serialize() | ||
|
|
||
| const result = await procedure({ | ||
| jsonrpc: '2.0', | ||
| method: 'eth_sendRawTransaction', | ||
| params: [bytesToHex(serializedTx)], | ||
| id: 1, | ||
| }) | ||
|
|
||
| expect(result.result).toBe(bytesToHex(signedTx.hash())) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding more test scenarios for comprehensive coverage.
The current test suite provides a solid foundation for testing the ethSendRawTransactionJsonRpcProcedure. To further enhance the robustness of the tests, consider adding the following scenarios:
- Error handling: Test with an invalid transaction to ensure proper error responses.
- Gas price too low: Verify behavior when the gas price is below the network's minimum.
- Nonce handling: Test with a transaction that has a nonce higher than the account's current nonce.
- Large data payload: Ensure the procedure can handle transactions with large data fields.
- Network congestion simulation: Test behavior when the network is congested (if applicable to your implementation).
Here's a sample structure for an error handling test:
it('should handle an invalid transaction', async () => {
const client = createTevmNode()
const procedure = ethSendRawTransactionJsonRpcProcedure(client)
const invalidTx = '0x invalid transaction data'
const result = await procedure({
jsonrpc: '2.0',
method: 'eth_sendRawTransaction',
params: [invalidTx],
id: 1,
})
expect(result.error).toBeDefined()
expect(result.error?.code).toBe(-32000) // Adjust the error code as per your implementation
expect(result.error?.message).toContain('invalid transaction') // Adjust the error message as needed
})Adding these scenarios will significantly improve the test coverage and ensure the procedure behaves correctly under various conditions.
| const forkedHandler = getBalanceHandler(forkedClient) | ||
|
|
||
| // Use a known address from mainnet with a stable balance | ||
| const vitalikAddress = '0xd8dA6BF26964aF9D7eEd9e03E53415D37aA96045' |
There was a problem hiding this comment.
Security consideration: Use test addresses instead of real user addresses
Using a real user's address (even a public figure) in tests may raise privacy and security concerns. It's advisable to use generated test addresses to avoid any potential issues.
| const forkedClient = createTevmNode({ | ||
| fork: { | ||
| transport: transports.mainnet, | ||
| }, | ||
| }) | ||
| const forkedHandler = getBalanceHandler(forkedClient) | ||
|
|
||
| // Use a known address from mainnet with a stable balance | ||
| const vitalikAddress = '0xd8dA6BF26964aF9D7eEd9e03E53415D37aA96045' | ||
| const balance = await forkedHandler({ address: vitalikAddress, blockTag: 'latest' }) | ||
|
|
||
| expect(balance).toBeGreaterThan(0n) |
There was a problem hiding this comment.
Avoid relying on external mainnet data in tests
Using a real mainnet address like Vitalik's can lead to flaky tests due to changes outside of your control. It's better to mock the forked client or use predefined balances to ensure consistent and reliable tests.
Consider applying this change to mock the balance:
const forkedClient = createTevmNode({
fork: {
transport: transports.mainnet,
},
})
+// Mock the balance for the test address
+await setAccountHandler(forkedClient)({ address: vitalikAddress, balance: parseEther('100') })
const forkedHandler = getBalanceHandler(forkedClient)
// Use a known address with a mocked stable balance
const vitalikAddress = '0xd8dA6BF26964aF9D7eEd9e03E53415D37aA96045'
const balance = await forkedHandler({ address: vitalikAddress, blockTag: 'latest' })
-expect(balance).toBeGreaterThan(0n)
+expect(balance).toEqual(parseEther('100'))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const forkedClient = createTevmNode({ | |
| fork: { | |
| transport: transports.mainnet, | |
| }, | |
| }) | |
| const forkedHandler = getBalanceHandler(forkedClient) | |
| // Use a known address from mainnet with a stable balance | |
| const vitalikAddress = '0xd8dA6BF26964aF9D7eEd9e03E53415D37aA96045' | |
| const balance = await forkedHandler({ address: vitalikAddress, blockTag: 'latest' }) | |
| expect(balance).toBeGreaterThan(0n) | |
| const forkedClient = createTevmNode({ | |
| fork: { | |
| transport: transports.mainnet, | |
| }, | |
| }) | |
| // Mock the balance for the test address | |
| await setAccountHandler(forkedClient)({ address: vitalikAddress, balance: parseEther('100') }) | |
| const forkedHandler = getBalanceHandler(forkedClient) | |
| // Use a known address with a mocked stable balance | |
| const vitalikAddress = '0xd8dA6BF26964aF9D7eEd9e03E53415D37aA96045' | |
| const balance = await forkedHandler({ address: vitalikAddress, blockTag: 'latest' }) | |
| expect(balance).toEqual(parseEther('100')) |
| it('should work for past blocks in forked mode', async () => { | ||
| const client = createTevmNode({ | ||
| fork: { | ||
| transport: transports.optimism, | ||
| blockTag: 125985200n, | ||
| }, | ||
| }) | ||
| const logs = await ethGetLogsHandler(client)({ | ||
| filterParams: { | ||
| address: '0xdC6fF44d5d932Cbd77B52E5612Ba0529DC6226F1', | ||
| fromBlock: 125985142n, | ||
| toBlock: 125985142n, | ||
| topics: [ | ||
| '0x8c5be1e5ebec7d5bd14f71427d1e84f3dd0314c0f7b2291e5b200ac8c7c3b925', | ||
| '0x0000000000000000000000007f26A7572E8B877654eeDcBc4E573657619FA3CE', | ||
| '0x0000000000000000000000007B46fFbC976db2F94C3B3CDD9EbBe4ab50E3d77d', | ||
| ], | ||
| }, | ||
| }) | ||
| expect(logs).toHaveLength(1) | ||
| expect(logs).toMatchInlineSnapshot(` | ||
| [ | ||
| { | ||
| "address": "0xdc6ff44d5d932cbd77b52e5612ba0529dc6226f1", | ||
| "blockHash": "0x6c9355482a6937e44fbfbd1c0c9cc95882e47e80c9b48772699c6a49bad1e392", | ||
| "blockNumber": 125985142n, | ||
| "data": "0x0000000000000000000000000000000000000000000b2f1069a1f95dc7180000", | ||
| "logIndex": 23n, | ||
| "removed": false, | ||
| "topics": [ | ||
| "0x8c5be1e5ebec7d5bd14f71427d1e84f3dd0314c0f7b2291e5b200ac8c7c3b925", | ||
| "0x0000000000000000000000007f26a7572e8b877654eedcbc4e573657619fa3ce", | ||
| "0x0000000000000000000000007b46ffbc976db2f94c3b3cdd9ebbe4ab50e3d77d", | ||
| ], | ||
| "transactionHash": "0x4f0781ec417fecaf44b248fd0b0485dca9fbe78ad836598b65c12bb13ab9ddd4", | ||
| "transactionIndex": 11n, | ||
| }, | ||
| ] | ||
| `) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance Test Robustness for Forked Mode
The test uses hardcoded block numbers and addresses specific to the forked network.
Suggestion:
To improve the test's robustness and maintainability:
- Parametrize Block Numbers and Addresses: Extract the block numbers and addresses into constants or configuration files. This makes it easier to update them if the underlying data changes.
- Check for Empty Logs: Add assertions to handle cases where the logs array might be empty due to changes in the forked chain state.
const FORK_BLOCK_NUMBER = 125985200n
const TEST_BLOCK_NUMBER = 125985142n
const TEST_ADDRESS = '0xdC6fF44d5d932Cbd77B52E5612Ba0529DC6226F1'
const client = createTevmNode({
fork: {
transport: transports.optimism,
blockTag: FORK_BLOCK_NUMBER,
},
})
const logs = await ethGetLogsHandler(client)({
filterParams: {
address: TEST_ADDRESS,
fromBlock: TEST_BLOCK_NUMBER,
toBlock: TEST_BLOCK_NUMBER,
topics: [...],
},
})
expect(logs).not.toHaveLength(0)
expect(logs[0]?.blockNumber).toBe(TEST_BLOCK_NUMBER)This approach enhances clarity and eases future updates.
| // this is actually a bug | ||
| address: createAddress(contractAddress).toString().toLowerCase(), |
There was a problem hiding this comment.
Address Normalization Issue
The comment on line 86 indicates there's a bug related to address formatting. In the assertion on line 87, the address is converted to lowercase using .toLowerCase().
Recommendation:
Ethereum addresses are case-insensitive but can be checksummed for error detection. Instead of converting to lowercase, consider using a standard method for address normalization or validation, such as EIP-55 checksumming.
expect(logs[0]).toMatchObject({
- address: createAddress(contractAddress).toString().toLowerCase(),
+ address: createAddress(contractAddress).toString(),
blockNumber: expect.any(BigInt),
transactionHash: expect.any(String),
})If the logged address is in lowercase, ensure that the comparison accounts for the address format used by the logging system.
Committable suggestion was skipped due to low confidence.
roninjin10
deleted the
09-28-_test_tube_test_add_more_test_coverage_to_actions
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
Release Notes
New Features
debugTraceCallJsonRpcProcedure,debugTraceTransactionJsonRpcProcedure,ethSendRawTransactionJsonRpcProcedure, andethSendTransactionHandler, enhancing testing coverage for transaction debugging and handling.Bug Fixes
ethGetLogsHandlerto streamline error reporting.getBalanceHandlerto ensure accurate balance fetching under various conditions.Documentation
getCodeHandler.Tests