🧪 Test: Add more test coverage to actions #1467
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
🦋 Changeset detectedLatest commit: 47dba07 The changes in this PR will be included in the next version bump. This PR includes changesets to release 18 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughA new 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 (
|
|
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 (19)
.changeset/mean-ghosts-wave.md (1)
6-6: Enhance the change description.While the current description is concise, it could benefit from more details. Consider expanding on:
- The specific scenario where the bug occurred.
- The impact of the bug on the system's behavior.
- How the fix addresses the issue.
This additional information would provide better context for future reference and changelog generation.
.changeset/fair-ghosts-rule.md (1)
5-5: Consider enhancing the description slightly.The description is clear and concise. To make it even more informative, you might consider adding a brief mention of how this new contract will benefit developers or improve the testing process.
Here's a suggested enhancement:
-Added a new BlockReader contract. BlockReader is a contract that can be used to test reading blocks from the evm. Used internally to test block overrides +Added a new BlockReader contract. BlockReader is a contract that can be used to test reading blocks from the EVM. It is used internally to test block overrides, enhancing our ability to simulate and validate block interactions in a controlled testing environment. This addition will improve the robustness of our testing framework for EVM-related functionalities..changeset/tiny-dolls-enjoy.md (1)
5-5: Consider adding a period at the end of the description for consistency.To maintain consistency in punctuation, consider adding a period at the end of the description.
Here's the suggested change:
-Fixed bug in tevm_call json-rpc procedure where deployedBytecode, createTrace and createAccessList were not forwarded to the underlying handler. This bug only affected users using JSON-RPC directly +Fixed bug in tevm_call json-rpc procedure where deployedBytecode, createTrace and createAccessList were not forwarded to the underlying handler. This bug only affected users using JSON-RPC directly.test/test-utils/src/BlockReader.s.sol (2)
5-11: LGTM: Function implementation is correct. Consider adding named returns.The
getBlockInfo()function is well-implemented:
- Correctly marked as
publicandview.- Uses appropriate
blockproperties to retrieve current block information.- Return types match the values being returned.
For improved readability and self-documentation, consider using named returns:
function getBlockInfo() public view returns ( uint256 blockNumber, uint256 timestamp, address coinbase, uint256 baseFee ) { return (block.number, block.timestamp, block.coinbase, block.basefee); }This change would make the function's purpose clearer and provide better documentation for users of this contract.
1-12: Consider adding documentation and expanding functionality.Given that this contract is located in the
test-utilsdirectory and seems intended for testing purposes:
- Add NatSpec documentation to explain the contract's purpose and usage in testing scenarios. For example:
/// @title BlockReader /// @notice A utility contract for reading current block information in tests /// @dev This contract is intended to be used in testing environments to access block properties contract BlockReader { /// @notice Retrieves current block information /// @return blockNumber The current block number /// @return timestamp The current block timestamp /// @return coinbase The current block's coinbase address /// @return baseFee The current block's base fee function getBlockInfo() public view returns (...) { // ... (existing implementation) } }
- Consider expanding the contract's functionality if there are other block-related utilities needed for testing. For example, you could add functions to manipulate block properties in a testing environment (if using a framework that supports this).
These additions would improve the contract's usability and maintainability in your testing suite.
packages/actions/src/requestBulkProcedure.js (1)
Line range hint
1-22: Consider adding tests for the new error handling logic.While the changes improve error handling, they don't directly address the PR objective of increasing test coverage. To align with the PR's goals, consider adding unit tests that specifically cover the new error handling logic. This would ensure that both the happy path and error scenarios are properly tested.
Here are some test scenarios to consider:
- Test with a rejected response that includes both code and message.
- Test with a rejected response that includes only a code.
- Test with a rejected response that includes only a message.
- Test with a rejected response that doesn't include code or message (fallback case).
These tests would validate the robustness of the new error handling implementation.
test/test-utils/src/BlockReader.s.sol.ts (2)
4-4: Consider adding names to the return values for clarity.While the ABI is correct, it could be more informative. Consider adding names to the return values to improve readability and self-documentation.
For example:
-'function getBlockInfo() view returns (uint256, uint256, address, uint256)' +'function getBlockInfo() view returns (uint256 blockNumber, uint256 timestamp, address miner, uint256 difficulty)'This change would make the function's purpose and return values more immediately clear to other developers.
10-13: Enhance the documentation comment with a brief description.While the link to the contract documentation is helpful, it would be beneficial to add a brief description of the
BlockReadercontract's purpose directly in the comment.Consider expanding the comment as follows:
/** + * BlockReader contract provides functionality to retrieve current block information. + * It exposes a single view function `getBlockInfo()` that returns block number, timestamp, miner address, and difficulty. * @see [contract docs](https://tevm.sh/learn/contracts/) for more documentation */This addition would give developers a quick understanding of the contract's functionality without needing to follow the link.
packages/actions/src/DumpState/dumpStateHandler.js (1)
59-65: Improved error handling, but consider some refinements.The new error handling logic enhances error specificity by distinguishing between
BaseErrorand other error types. However, there are a few points to consider:
- Instead of using the
_tagproperty to check forBaseError, consider using theinstanceofoperator for more robust type checking:if (e instanceof BaseError) { // ... }
The TODO comment indicates that error typing needs improvement. Consider addressing this by defining a more specific error type or interface for the errors thrown in this context.
The use of
/**@type {any} */ (e)could mask type-related issues. Try to avoid usinganyand instead define a more specific type for the error. This will improve type safety and make the code more maintainable.packages/actions/src/Call/handleEvmError.spec.ts (1)
125-138: LGTM! Consider minor improvements for robustness.The new test case for handling insufficient balance errors with upfront cost is well-structured and adds value to the test suite. It correctly verifies the error type, cause, and message content.
Consider the following improvements:
- Extract the version number from a constant or configuration file to avoid hardcoding it in the test:
import { VERSION } from '../version' // ... expect(result.message).toMatchInlineSnapshot(` "sender doesn't have enough funds to send tx. The upfront cost is 1000 wei Docs: https://tevm.sh/reference/tevm/errors/classes/insufficientbalanceerror/ Details: sender doesn't have enough funds to send tx. The upfront cost is 1000 wei Version: ${VERSION}" `)
- Add a test to verify that the upfront cost is correctly extracted from the error message:
expect(result.upfrontCost).toBe(1000n)These changes will make the test more maintainable and comprehensive.
packages/actions/src/Deploy/deployHandler.spec.ts (1)
66-87: Excellent addition of error handling test cases!This new test case significantly improves the coverage of the
deployHandlerfunction by testing its behavior with incorrect constructor arguments. It aligns well with the PR objective of enhancing test coverage.A few observations:
- The test covers both incorrect argument type and incorrect number of arguments, which are crucial edge cases.
- The use of
expect().rejects.toThrow(InvalidRequestError)is appropriate for testing asynchronous error throwing.Consider adding a test case for an empty
argsarray, as this is another potential edge case:// Attempt to deploy with empty args array await expect( deploy({ abi: simpleConstructorAbi, bytecode: simpleConstructorBytecode, args: [], }), ).rejects.toThrow(InvalidRequestError)This would further improve the robustness of your error handling tests.
packages/actions/src/eth/ethGetLogsHandler.spec.ts (1)
231-274: Approve the timeout addition with a suggestion for improvement.The addition of a 20-second timeout for this test case is a good practice, especially for tests involving forked networks and historical data retrieval. This change will help prevent false negatives due to network latency or slow data fetching.
However, to improve maintainability and consistency across tests, consider the following suggestion:
Define a constant for the timeout value at the top of the file or in a separate configuration file. This will make it easier to adjust timeouts across multiple tests if needed. For example:
const FORKED_TEST_TIMEOUT = 20_000; // Then use it in the test: it('should work for past blocks in forked mode', async () => { // ... test code ... }, { timeout: FORKED_TEST_TIMEOUT } );This approach will make it easier to maintain consistent timeout values across your test suite.
packages/actions/src/requestProcedure.spec.ts (1)
306-331: LGTM: New test case for unsupported methodsThe new test case for handling unsupported methods is well-structured and comprehensive. It correctly verifies the error code and response structure for an unsupported method call.
Consider using a placeholder or constant for the version number in the error message snapshot (line 324). This will reduce the need for frequent updates to the test when the version changes. For example:
expect(res).toMatchInlineSnapshot(` { ... "message": expect.stringMatching(/UnsupportedMethodError: Unknown method unsupported_method\n\nDocs: https:\/\/tevm\.sh\/reference\/tevm\/errors\/classes\/methodnotfounderror\/\nVersion: \d+\.\d+\.\d+/), ... } `)This approach allows for version changes without breaking the test, as long as the overall structure remains the same.
packages/actions/src/Contract/contractHandler.spec.ts (1)
367-401: LGTM! Consider adding a positive test case.The new test case for handling contract revert errors is well-structured and comprehensive. It correctly sets up the test environment, deploys the contract, and verifies the error handling of the
contractHandlerfor a revert scenario. The error checking is thorough, covering the presence, type, and structure of the error.Consider adding a positive test case alongside this revert test to ensure the
transferFromfunction works correctly when given valid parameters. This would provide a more complete test coverage for the function. For example:it('should successfully execute transferFrom', async () => { const client = createTevmNode() await setAccountHandler(client)({ address: ERC20_ADDRESS, deployedBytecode: ERC20_BYTECODE, }) const from = `0x${'11'.repeat(20)}` as const const to = `0x${'22'.repeat(20)}` as const const amount = 1000n // First, mint some tokens to the 'from' address await contractHandler(client)({ abi: ERC20_ABI, functionName: 'mint', args: [from, amount], to: ERC20_ADDRESS, }) // Then, approve the transfer await contractHandler(client)({ abi: ERC20_ABI, functionName: 'approve', args: [to, amount], from, to: ERC20_ADDRESS, }) // Finally, execute transferFrom const result = await contractHandler(client)({ abi: ERC20_ABI, functionName: 'transferFrom', args: [from, to, amount], from: to, to: ERC20_ADDRESS, }) expect(result.errors).toBeUndefined() // Add more assertions as needed to verify the transfer was successful })This additional test case would complement the existing revert test and provide a more robust test suite for the
transferFromfunctionality.packages/actions/src/requestBulkProcedure.spec.ts (1)
89-142: Excellent addition of mixed success/failure test case.This new test case significantly improves the test coverage by handling both successful and failed requests in a single bulk procedure call. It effectively uses the new address constants and verifies the expected outcomes for both scenarios.
A minor suggestion for improvement:
Consider adding a comment explaining the purpose of using
toMatchInlineSnapshot()for the error message. This can help other developers understand why we're using a snapshot instead of a hardcoded string comparison.expect(res[1].error?.message).toMatchInlineSnapshot(` + // Using snapshot to maintain consistent error messages across test runs "Received an invalid address input: Invalid byte sequence ("gg" in "gggggggggggggggggggggggggggggggggggggggg"). Version: 2.21.1packages/actions/src/Call/callHandlerOpts.js (4)
Line range hint
17-17: Address the TODO comment on error handlingThe comment indicates a need for better error handling in the block retrieval logic. Enhancing the error handling will improve the robustness and reliability of the function.
Would you like assistance in improving the error handling here, or should we open a GitHub issue to track this task?
67-68: Address the known bug regarding block tag supportThe TODO comments mention a known bug and the need to implement better support for block tags in block overrides. Resolving this issue will enhance the functionality and correctness of block handling.
Can I assist in implementing better support for block tags, or should we create a GitHub issue to track this task?
Line range hint
153-154: Simplify the conditional checks forcallerandoriginSince
callerandoriginare always assigned default values when undefined, the conditional checksif (caller)andif (origin)are unnecessary. Removing these checks can simplify the code without affecting functionality.Apply this diff to remove the redundant conditional checks:
-def caller = +const caller = params.caller || params.from || params.origin || (params.createTransaction ? '0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266' : `0x${'00'.repeat(20)}`) -if (caller) { opts.caller = createAddress(caller) -} -def origin = +const origin = params.origin || params.from || (params.createTransaction ? '0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266' : `0x${'00'.repeat(20)}`) -if (origin) { if (params.skipBalance !== undefined) { opts.skipBalance = Boolean(params.skipBalance) } else { opts.skipBalance = caller === `0x${'00'.repeat(20)}` && (params.createTransaction ?? false) === false } opts.origin = createAddress(origin) -}
Line range hint
164-166: Enhance the error message for unsupported transaction creationThe error message
'Creating transactions on past blocks is not currently supported'may not provide enough context for users. Consider providing additional guidance or suggesting alternatives to help users understand the limitation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
packages/actions/src/DumpState/__snapshots__/dumpStateHandler.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (21)
- .changeset/fair-ghosts-rule.md (1 hunks)
- .changeset/mean-ghosts-wave.md (1 hunks)
- .changeset/tiny-dolls-enjoy.md (1 hunks)
- packages/actions/src/Call/callHandler.js (0 hunks)
- packages/actions/src/Call/callHandlerOpts.js (1 hunks)
- packages/actions/src/Call/callHandlerResult.js (0 hunks)
- packages/actions/src/Call/callProcedure.js (1 hunks)
- packages/actions/src/Call/callProcedure.spec.ts (2 hunks)
- packages/actions/src/Call/handleEvmError.spec.ts (1 hunks)
- packages/actions/src/Contract/contractHandler.spec.ts (1 hunks)
- packages/actions/src/Deploy/deployHandler.spec.ts (2 hunks)
- packages/actions/src/DumpState/dumpStateHandler.js (2 hunks)
- packages/actions/src/DumpState/dumpStateHandler.spec.ts (1 hunks)
- packages/actions/src/eth/ethGetLogsHandler.spec.ts (1 hunks)
- packages/actions/src/requestBulkProcedure.js (1 hunks)
- packages/actions/src/requestBulkProcedure.spec.ts (2 hunks)
- packages/actions/src/requestProcedure.spec.ts (2 hunks)
- packages/vm/src/actions/runTx.ts (1 hunks)
- test/test-utils/src/BlockReader.s.sol (1 hunks)
- test/test-utils/src/BlockReader.s.sol.ts (1 hunks)
- test/test-utils/src/index.ts (1 hunks)
💤 Files with no reviewable changes (2)
- packages/actions/src/Call/callHandler.js
- packages/actions/src/Call/callHandlerResult.js
🔇 Additional comments (18)
.changeset/mean-ghosts-wave.md (2)
1-4: LGTM: Changeset structure is correct.The changeset file structure is correct, specifying patch updates for both "@tevm/actions" and "@tevm/vm" packages.
1-6: Verify alignment with PR objectives.The PR objectives mention adding more test coverage to actions, but the changeset describes a bug fix related to block override functionality. There's a potential misalignment between the PR title/objectives and the actual changes described in the changeset.
Please clarify:
- Does this PR include both increased test coverage and a bug fix?
- If so, consider updating the changeset to mention the increased test coverage.
- If not, consider updating the PR title and description to accurately reflect the changes made.
.changeset/fair-ghosts-rule.md (1)
1-5: LGTM! Changeset looks good.The changeset file is correctly formatted and provides clear information about the version update and the new feature added. The minor version bump is appropriate for introducing a new contract.
.changeset/tiny-dolls-enjoy.md (1)
1-5: LGTM! The changeset file is well-structured and informative.The changeset file correctly specifies the package being updated and the type of update (patch). The format adheres to the expected structure for changeset files.
test/test-utils/src/BlockReader.s.sol (1)
1-4: LGTM: Contract structure and license are well-defined.The contract structure follows Solidity best practices:
- SPDX license identifier is correctly placed.
- Solidity version pragma is specified and uses a recent stable version.
- Contract name follows the PascalCase naming convention.
packages/actions/src/requestBulkProcedure.js (2)
Line range hint
1-22: Summary: Improved error handling, but missing test coverageThe changes to
requestBulkProcedure.jsenhance error handling by providing more specific error information. This is a positive improvement that will aid in debugging and error reporting. However, the PR's main objective of increasing test coverage hasn't been addressed in this file.Recommendations:
- Proceed with the improved error handling implementation.
- Add unit tests to cover the new error handling logic.
- Address the TODO comment about moving the error code to
@tevm/errorspackage.- Consider adding type checking for the
response.reasonobject.Overall, while the changes are beneficial, additional work is needed to fully meet the PR's objectives and ensure the robustness of the new implementation.
21-22: Improved error handling looks good, but consider adding type checking.The changes to dynamically set the error code and message based on
response.reasonare a good improvement. They provide more specific error information when available while maintaining backwards compatibility with fallback values.However, consider adding type checking for the
response.reasonobject to ensure it has the expected structure. This can prevent potential runtime errors if the structure ofresponse.reasonchanges in the future.Consider adding type checking like this:
code: (response.reason && typeof response.reason.code === 'number') ? response.reason.code : -32000, message: (response.reason && typeof response.reason.message === 'string') ? response.reason.message : 'UnexpectedBulkRequestError',Let's verify if this error handling pattern is consistent across the codebase:
There's a TODO comment about moving the error code to the
@tevm/errorspackage. Would you like me to create a GitHub issue to track this task?✅ Verification successful
Error handling changes verified successfully.
No other instances of the error handling pattern
code: ..., message: ...were found in the codebase. The updates torequestBulkProcedure.jsimprove error reporting without affecting other parts of the system.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar error handling patterns in other files rg --type javascript 'error:\s*\{.*code:.*message:' -g '!requestBulkProcedure.js'Length of output: 118
Script:
#!/bin/bash # Search for similar error handling patterns in other JavaScript files, excluding requestBulkProcedure.js rg --type js 'error:\s*\{.*code:.*message:' -g '!requestBulkProcedure.js'Length of output: 244
test/test-utils/src/BlockReader.s.sol.ts (3)
1-1: LGTM: Import statement is correct.The import of
createContractfrom '@tevm/contract' is appropriate for creating the contract instance.
2-9: LGTM: Contract definition structure is correct.The
_BlockReaderobject is well-structured with all necessary properties for use withcreateContract.
5-8: Verify bytecode matches the intended contract functionality.The bytecode and deployedBytecode are provided, which is correct. However, it's crucial to ensure that this bytecode accurately represents the intended
BlockReadercontract functionality.To verify the bytecode:
This script checks for the presence of the function selector and block information opcodes in the bytecode. Please review the results to ensure they align with the expected functionality of the
BlockReadercontract.packages/actions/src/DumpState/dumpStateHandler.js (1)
1-1: LGTM: Import statement updated correctly.The addition of
BaseErrorto the import statement is consistent with its usage in the updated error handling logic.packages/actions/src/Call/handleEvmError.spec.ts (1)
Line range hint
1-138: Summary: New test case aligns well with PR objectivesThis new test case for handling insufficient balance errors with upfront cost aligns perfectly with the PR's objective of adding more test coverage to actions. It enhances the robustness of the
handleRunTxErrorfunction's test suite by covering a specific scenario that wasn't previously tested.The addition of this test case contributes to:
- Improved code quality through comprehensive testing.
- Better error handling coverage, particularly for insufficient balance scenarios.
- More detailed error messages that include upfront costs, which can be valuable for debugging and user feedback.
These changes support the overall goal of the PR to enhance test coverage and improve the quality of the codebase.
packages/actions/src/Deploy/deployHandler.spec.ts (1)
Line range hint
1-87: Great improvement in test coverage fordeployHandler!The addition of the new test case for incorrect constructor arguments significantly enhances the robustness of the
deployHandlerfunction. This change aligns perfectly with the PR objective of improving test coverage for actions.Key improvements:
- Error handling for incorrect argument types is now tested.
- Error handling for an incorrect number of arguments is now covered.
- The use of
InvalidRequestErrorensures consistency in error handling across the codebase.These changes will help catch potential issues earlier in the development process and improve the overall reliability of the
deployHandlerfunction.packages/actions/src/requestProcedure.spec.ts (2)
2-2: LGTM: New import for MethodNotFoundErrorThe import for
MethodNotFoundErrorfrom@tevm/errorsis correctly placed and necessary for the new test case.
Line range hint
1-331: Summary: Improved test coverage for requestProcedureThe changes in this file successfully enhance the test coverage for the
requestProcedurefunctionality by adding a new test case for handling unsupported methods. The additions are well-implemented and align perfectly with the PR objective of improving test coverage for actions.Key improvements:
- Added import for
MethodNotFoundErrorto support the new test case.- Implemented a comprehensive test case for verifying the behavior of
requestProcedurewith unsupported methods.These changes contribute to the overall robustness of the codebase by ensuring that error handling for unsupported methods is correctly implemented and tested.
packages/actions/src/requestBulkProcedure.spec.ts (3)
7-8: LGTM: Constants for test addresses.The addition of
VALID_ADDRESSandINVALID_ADDRESSconstants is a good practice. They provide clear, reusable values for testing valid and invalid Ethereum addresses.
13-13: Improved test case naming.The change from "should work" to "should work for successful requests" provides a more descriptive and specific test case name. This improvement enhances the clarity of the test suite.
Line range hint
1-142: Overall improvement in test coverage and clarity.The changes to this file significantly enhance the test suite for the
requestBulkProcedurefunction:
- Addition of
VALID_ADDRESSandINVALID_ADDRESSconstants improves test readability and maintainability.- Renaming the existing test case provides better clarity on its purpose.
- The new test case for mixed successful and failed requests improves overall coverage and verifies important edge cases.
These improvements contribute to a more robust and comprehensive test suite.
| @@ -1,4 +1,5 @@ | |||
| export { getAlchemyUrl } from './getAlchemyUrl.js' | |||
| export { transports } from './transports.js' | |||
| export { SimpleContract } from './SimpleContract.s.sol.js' | |||
| export { BlockReader } from './BlockReader.s.sol.js' | |||
There was a problem hiding this comment.
💡 Codebase verification
❌ Missing Implementation and Tests for BlockReader
The export for BlockReader in test/test-utils/src/index.ts references ./BlockReader.s.sol.js, which does not exist. Additionally, no related test files were found.
- Action Required:
- Implement
BlockReader.s.sol.js. - Add corresponding test files (
BlockReader.test.tsorBlockReader.spec.ts) to ensure proper test coverage.
- Implement
🔗 Analysis chain
LGTM! Consider verifying BlockReader implementation and test coverage.
The addition of the BlockReader export aligns well with the PR objective of increasing test coverage for actions. This change expands the available testing utilities in a consistent manner with the existing codebase structure.
To ensure comprehensive test coverage, please verify the implementation and test coverage of the BlockReader itself. Run the following script to check for related test files:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for the existence and content of BlockReader tests
# Test: Check if BlockReader.s.sol.js exists
if [ -f "./test/test-utils/src/BlockReader.s.sol.js" ]; then
echo "BlockReader.s.sol.js exists."
# Display the content of the file
cat "./test/test-utils/src/BlockReader.s.sol.js"
else
echo "BlockReader.s.sol.js not found. Please ensure it's implemented."
fi
# Test: Search for test files related to BlockReader
echo "Searching for BlockReader test files:"
fd -t f -e test.ts -e spec.ts BlockReader
Length of output: 324
| test('should handle block not found', async () => { | ||
| const client = createTevmNode() | ||
| const { errors } = await dumpStateHandler(client)({ blockTag: 1n, throwOnFail: false }) | ||
| expect(errors).toBeDefined() | ||
| expect(errors).toHaveLength(1) | ||
| expect(errors).toMatchInlineSnapshot(` | ||
| [ | ||
| [UnknownBlock: Block number 1 does not exist | ||
|
|
||
| Docs: https://tevm.sh/reference/tevm/errors/classes/unknownblockerror/ | ||
| Version: 1.1.0.next-73], | ||
| ] | ||
| `) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Good addition of error handling test, consider these improvements:
- Add a comment explaining the use of
1nas a block tag for clarity. - The inline snapshot includes version information, which might require frequent updates. Consider using a more flexible assertion for the error message that doesn't include version-specific details.
- Add a test case to verify the behavior when
throwOnFailis set totrue.
Here's a suggested improvement for points 1 and 2:
test('should handle block not found', async () => {
const client = createTevmNode()
+ // Use 1n as a non-existent block number to trigger the error
const { errors } = await dumpStateHandler(client)({ blockTag: 1n, throwOnFail: false })
expect(errors).toBeDefined()
expect(errors).toHaveLength(1)
- expect(errors).toMatchInlineSnapshot(`
- [
- [UnknownBlock: Block number 1 does not exist
-
- Docs: https://tevm.sh/reference/tevm/errors/classes/unknownblockerror/
- Version: 1.1.0.next-73],
- ]
- `)
+ expect(errors[0]).toBeInstanceOf(Error)
+ expect(errors[0].message).toContain('Block number 1 does not exist')
+ expect(errors[0].message).toContain('https://tevm.sh/reference/tevm/errors/classes/unknownblockerror/')
})For point 3, consider adding a new test case:
test('should throw when block not found and throwOnFail is true', async () => {
const client = createTevmNode()
await expect(dumpStateHandler(client)({ blockTag: 1n, throwOnFail: true })).rejects.toThrow('Block number 1 does not exist')
})| expect(response.id).toBe(request.id as any) | ||
|
|
There was a problem hiding this comment.
Avoid unnecessary casting to any in test assertions
Casting request.id to any in your test assertions may mask potential type inconsistencies. Consider ensuring that the types of response.id and request.id are compatible without the need for casting. This will help maintain strong type safety in your tests.
Also applies to: 146-147, 158-159
| it('should handle a call with block override', async () => { | ||
| const blockReaderAddress = createAddress(1234) | ||
| await setAccountHandler(client)({ | ||
| address: blockReaderAddress.toString(), | ||
| deployedBytecode: BlockReader.deployedBytecode, | ||
| }) | ||
|
|
||
| const request: CallJsonRpcRequest = { | ||
| jsonrpc: '2.0', | ||
| method: 'tevm_call', | ||
| id: 1, | ||
| params: [ | ||
| { | ||
| to: blockReaderAddress.toString(), | ||
| data: encodeFunctionData(BlockReader.read.getBlockInfo()), | ||
| }, | ||
| {}, // No state override | ||
| { | ||
| number: numberToHex(1000n), | ||
| time: numberToHex(1234567890n), | ||
| coinbase: '0x1000000000000000000000000000000000000000', | ||
| baseFee: numberToHex(1n), | ||
| }, | ||
| ], | ||
| } | ||
|
|
||
| const response = await callProcedure(client)(request) | ||
| expect(response.error).toBeUndefined() | ||
| expect(response.result).toBeDefined() | ||
| expect(response.method).toBe('tevm_call') | ||
| expect(response.id).toBe(request.id as any) | ||
|
|
||
| const decodedResult = decodeFunctionResult({ | ||
| abi: BlockReader.read.getBlockInfo.abi, | ||
| data: response.result?.rawData as Hex, | ||
| functionName: 'getBlockInfo', | ||
| }) | ||
| expect(decodedResult).toEqual([1000n, 1234567890n, '0x1000000000000000000000000000000000000000', 1n]) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor common test setup code to improve maintainability
The test cases it('should handle a call with block override', ...) and it('should handle a call with tracing enabled', ...) share similar setup code, such as deploying contracts and initializing requests. Consider refactoring this code into helper functions or using shared fixtures to reduce duplication and enhance maintainability.
Also applies to: 124-179
| // this isn't in the type but it needs to be here or else block overrides will fail | ||
| ...{ stateRoot: block.header.stateRoot }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Include stateRoot in the type definitions
The comment notes that stateRoot isn't in the type definition but is required to prevent block overrides from failing. To ensure type safety and clarity, consider updating the type definitions to include stateRoot.
Apply this change to update the type definitions accordingly.
| ...(request.params[0].data ? { data: request.params[0].data } : {}), | ||
| ...(request.params[0].deployedBytecode ? { deployedBytecode: request.params[0].deployedBytecode } : {}), | ||
| ...(request.params[0].createTrace ? { createTrace: request.params[0].createTrace } : {}), | ||
| ...(request.params[0].createAccessList ? { createAccessList: request.params[0].createAccessList } : {}), |
There was a problem hiding this comment.
Ensure proper validation and sanitization of new request parameters.
The new parameters data, deployedBytecode, createTrace, and createAccessList are being directly passed from request.params[0] into the options for callHandler. To prevent potential security vulnerabilities or unexpected behavior, please ensure that these parameters are properly validated and sanitized before use.
| @@ -382,7 +382,7 @@ const _runTx = | |||
| await vm.evm.journal.cleanup() | |||
|
|
|||
| // Generate the tx receipt | |||
| const gasUsed = opts.blockGasUsed !== undefined ? opts.blockGasUsed : block.header.gasUsed | |||
| const gasUsed = (opts.blockGasUsed !== undefined ? opts.blockGasUsed : block.header.gasUsed) ?? 0n | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify the nullish coalescing usage for clarity
The expression on line 385 can be simplified for better readability. Currently, it uses a ternary operator followed by a nullish coalescing operator:
const gasUsed = (opts.blockGasUsed !== undefined ? opts.blockGasUsed : block.header.gasUsed) ?? 0nSince the ternary operator will return a defined value unless both opts.blockGasUsed and block.header.gasUsed are undefined, the ?? 0n ensures gasUsed is 0n only when both are undefined. You can simplify this expression using the nullish coalescing operator sequentially:
const gasUsed = opts.blockGasUsed ?? block.header.gasUsed ?? 0nThis version checks each value in order and defaults to 0n if both are undefined, making the code more concise and easier to understand.
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
getBlockInfoto retrieve key block information.Bug Fixes
Tests