🐛 Fix: stateOverrides persisting after call is finished #1481
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
🦋 Changeset detectedLatest commit: d591949 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 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 |
|
Caution Review failedThe pull request is closed. WalkthroughThis changeset introduces a patch to the 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: 3
🧹 Outside diff range and nitpick comments (1)
.changeset/smart-mice-dream.md (1)
1-5: LGTM! Consider adding more details to the description.The changeset file is correctly formatted and provides the essential information about the patch. The description accurately reflects the bug fix mentioned in the PR objectives.
To improve clarity, consider expanding the description slightly to include the impact of the fix. For example:
- Fixed bug with state overrides persisting after the call was finished + Fixed bug with state overrides persisting after the call was finished, ensuring correct state management and preventing unintended side effects in subsequent operationsThis additional context can help other developers and users better understand the significance of the change.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- .changeset/smart-mice-dream.md (1 hunks)
- packages/actions/src/Call/callHandler.js (2 hunks)
- packages/actions/src/Call/callHandlerOpts.js (0 hunks)
- packages/actions/src/Call/handleStateOverrides.js (1 hunks)
💤 Files with no reviewable changes (1)
- packages/actions/src/Call/callHandlerOpts.js
🧰 Additional context used
🔇 Additional comments (2)
packages/actions/src/Call/callHandler.js (2)
14-14: ImporthandleStateOverridesmoduleThe import statement correctly brings in the
handleStateOverridesfunction from./handleStateOverrides.js.
128-138: EnsurestateOverridesare properly applied to the VM instanceThe addition of
handleStateOverridesintegrates state override handling into the call flow. However, please verify thathandleStateOverridescorrectly applies the state overrides to the VM instance before executing the call. This ensures that any state modifications are effective during EVM execution.
| export async function handleStateOverrides(client, params) { | ||
| if (params.stateOverrideSet) { | ||
| for (const [address, state] of Object.entries(params.stateOverrideSet)) { | ||
| const res = await setAccountHandler(client)({ | ||
| address: /** @type import('@tevm/utils').Address*/ (address), | ||
| ...(state.nonce !== undefined ? { nonce: state.nonce } : {}), | ||
| ...(state.balance !== undefined ? { balance: state.balance } : {}), | ||
| ...(state.code !== undefined ? { deployedBytecode: state.code } : {}), | ||
| ...(state.state !== undefined ? { state: state.state } : {}), | ||
| ...(state.stateDiff !== undefined ? { stateDiff: state.stateDiff } : {}), | ||
| throwOnFail: false, | ||
| }) | ||
| if (res.errors?.length) { | ||
| return { | ||
| errors: [ | ||
| new InvalidParamsError('Invalid state override', { | ||
| cause: /** @type {Error} */ (res.errors.length === 1 ? res.errors[0] : new AggregateError(res.errors)), | ||
| }), | ||
| ], | ||
| } | ||
| } | ||
| } | ||
| } | ||
| return {}; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add unit tests for handleStateOverrides
Unit tests are essential to ensure that the new handleStateOverrides function behaves correctly under various scenarios, including handling valid and invalid state overrides and correctly propagating errors.
Would you like assistance in creating unit tests for this function or opening a GitHub issue to track this task?
| return { | ||
| errors: [ | ||
| new InvalidParamsError('Invalid state override', { | ||
| cause: /** @type {Error} */ (res.errors.length === 1 ? res.errors[0] : new AggregateError(res.errors)), |
There was a problem hiding this comment.
Ensure compatibility of AggregateError with target environments
The use of AggregateError is supported in Node.js 15.0.0 and above. If the project needs to support older Node.js versions, consider an alternative method for aggregating errors to maintain compatibility.
| new InvalidParamsError('Invalid state override', { | ||
| cause: /** @type {Error} */ (res.errors.length === 1 ? res.errors[0] : new AggregateError(res.errors)), | ||
| }), | ||
| ], | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Include the address in error messages for better debugging
Including the address that caused the error can aid in quickly identifying problematic state overrides during debugging.
Apply this diff to modify the error message:
- new InvalidParamsError('Invalid state override', {
+ new InvalidParamsError(`Invalid state override for address ${address}`, {Committable suggestion was skipped due to low confidence.
roninjin10
force-pushed
the
10-13-_bug_fix_stateoverrides_persisting_after_call_is_finished
branch
from
2fa1bf1 to
a3518b7
Compare
roninjin10
force-pushed
the
10-13-_bug_fix_stateoverrides_persisting_after_call_is_finished
branch
from
a3518b7 to
6d4f6a0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/actions/src/Call/handleStateOverrides.js (1)
15-19: Consider removing the console.log statementThe console.log statement on line 18 appears to be used for debugging purposes. Consider removing it before merging this PR to production, or replacing it with a more appropriate logging mechanism if the information is necessary for production environments.
packages/actions/src/Call/callProcedure.spec.ts (1)
77-77: Consider removing the console.log statement or use a more appropriate debugging method.While adding a console.log statement can be helpful for debugging, it's generally not recommended to leave them in the final code, even in test files. This can clutter the test output and potentially hide important information.
Instead, consider the following options:
- If this log is temporary for debugging, remove it before merging the PR.
- If the information is crucial, use a proper assertion to check the error condition.
- If you need to keep the logging for future debugging, consider using a debug library that can be easily enabled/disabled.
Here's a suggestion to replace the console.log with an assertion:
- console.log(response.error) + expect(response.error).toBeUndefined()This way, you're explicitly checking that there's no error, which seems to be the intent of the following line.
packages/actions/src/Call/callHandlerOpts.spec.ts (1)
Line range hint
214-228: LGTM: New test case for block overrides without blobBaseFee.The addition of this test case improves coverage for block override scenarios, specifically when
blobBaseFeeis not provided. This is a valuable test to ensure correct behavior in such cases.Consider adding an assertion to verify that the
blobBaseFeeis not present in theblockOverrideSetobject, to make the test's intention clearer:expect(blockOverrideSet).not.toHaveProperty('blobBaseFee');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- packages/actions/src/Call/callHandler.js (2 hunks)
- packages/actions/src/Call/callHandlerOpts.spec.ts (1 hunks)
- packages/actions/src/Call/callProcedure.spec.ts (1 hunks)
- packages/actions/src/Call/handleStateOverrides.js (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/actions/src/Call/callHandler.js
🧰 Additional context used
🔇 Additional comments (12)
packages/actions/src/Call/handleStateOverrides.js (8)
1-2: LGTM: Import statements are appropriateThe import statements are correctly importing the necessary dependencies for the function.
4-7: LGTM: HandleStateOverridesError typedef is well-definedThe typedef for HandleStateOverridesError is correctly defined as an alias for InvalidParamsError and appropriately marked as @internal.
9-14: LGTM: JSDoc for handleStateOverrides is comprehensiveThe JSDoc for the handleStateOverrides function is well-written, accurately describing the function's parameters and return type.
19-27: LGTM: Effective use of optional chaining and error handlingThis section of the function effectively uses optional chaining to include properties conditionally. Setting
throwOnFailtofalseallows for custom error handling, which is implemented in the subsequent code block. The type casting ofaddressensures type safety.
28-40: LGTM: Robust error handling implementationThe error handling logic in this section is well-implemented. It correctly differentiates between single and multiple errors, using AggregateError when appropriate. The function returns the expected objects based on the presence or absence of errors.
32-32: Ensure AggregateError compatibilityAs mentioned in a previous review, the use of
AggregateErroris supported in Node.js 15.0.0 and above. If this project needs to support older Node.js versions, consider implementing a fallback mechanism for environments whereAggregateErroris not available.To check the current Node.js version requirements for this project, you can run:
#!/bin/bash # Check for Node.js version specification in package.json jq '.engines.node' package.json # Check for .nvmrc file cat .nvmrc 2>/dev/null || echo ".nvmrc not found" # Check for any other files that might specify Node.js version fd -t f '(\.node-version|\.tool-versions|\.node-version-file)$'
15-40: Add unit tests for handleStateOverrides functionAs mentioned in a previous review, it's crucial to add unit tests for the
handleStateOverridesfunction. These tests should cover various scenarios, including:
- Handling valid state overrides
- Handling invalid state overrides
- Correct error propagation
- Edge cases (e.g., empty stateOverrideSet, all possible combinations of state properties)
Adding these tests will ensure the function behaves correctly under different conditions and make future maintenance easier.
Would you like assistance in creating unit tests for this function or opening a GitHub issue to track this task?
1-40: Overall assessment: Well-implemented with minor improvements neededThe
handleStateOverridesfunction is well-structured and implements robust error handling. It makes good use of TypeScript features and follows best practices for handling optional parameters and complex objects.Key points:
- Consider removing or replacing the console.log statement (line 18).
- Ensure AggregateError compatibility with the project's target Node.js versions.
- Add comprehensive unit tests to cover various scenarios and edge cases.
These minor improvements will enhance the function's reliability and maintainability.
packages/actions/src/Call/callProcedure.spec.ts (1)
Line range hint
1-231: Comprehensive test coverage for callProcedure function.The test suite for the
callProcedurefunction is well-structured and covers a wide range of scenarios:
- Basic call
- Call with state override
- Call with block override
- Call with tracing enabled
- Error handling
This comprehensive coverage ensures that the
callProcedurefunction is thoroughly tested across various use cases, which is crucial for maintaining the reliability of the@tevm/actionspackage.packages/actions/src/Call/callHandlerOpts.spec.ts (3)
3-3: LGTM: Import statement updated correctly.The import statement has been updated to include
bytesToHexfrom '@tevm/utils', which is likely used in the updated tests. This change is consistent with the modifications in the test cases.
Line range hint
1-265: Overall assessment of test suite changes.The changes to this test suite have both positive and potentially concerning aspects:
- Positive: Addition of a new test case for block overrides without blobBaseFee, improving coverage in that area.
- Concern: Removal of two state override tests, potentially reducing coverage for state override scenarios.
While the test suite still covers a wide range of scenarios for the
callHandlerOptsfunction, the removal of state override tests warrants further investigation.To ensure that we maintain comprehensive test coverage, please:
- Confirm whether the state override functionality has been moved, removed, or refactored.
- If the functionality still exists, consider adding equivalent tests in the appropriate location.
- If the functionality has been removed, update the documentation to reflect this change.
You can use the following script to search for any remaining state override functionality in the codebase:
#!/bin/bash # Search for state override related code and tests echo "Searching for state override related code:" rg --type typescript "stateOverride" ./packages echo "Searching for state override related tests:" rg --type typescript "state override" ./packages --glob "*spec.ts"Based on the results, we can determine the next steps for maintaining or updating our test coverage.
Line range hint
1-265: Clarify the removal of state override tests.Two test cases related to state overrides have been removed:
- "should handle state overrides correctly"
- "should handle invalid state overrides"
This reduction in test coverage for state override scenarios could potentially leave the
callHandlerOptsfunction vulnerable to regressions or bugs related to state handling.Could you please clarify the reason for removing these tests? If the state override functionality has been moved or refactored, consider adding equivalent tests in the appropriate location to maintain comprehensive test coverage.
To verify the current state of state override handling, you can run the following script:
✅ Verification successful
State override test coverage remains intact.
The removal of the two test cases in
callHandlerOpts.spec.tsdoes not reduce overall test coverage for state overrides, as related tests are present in other modules:
./packages/memory-client/src/test/viem/call.spec.ts./packages/decorators/src/eip1193/JsonRpcSchemaPublic.ts./packages/actions/src/Contract/validateContractParams.spec.ts./packages/actions/src/Call/zCallParams.spec.ts./packages/actions/src/Call/validateCallParams.spec.ts🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for state override related code in the codebase echo "Searching for state override related code:" rg --type typescript "stateOverride" ./packages echo "Searching for state override related tests:" rg --type typescript "state override" ./packages --glob "*spec.ts"Length of output: 472
Script:
#!/bin/bash # Search for state override related code in the codebase echo "Searching for state override related code:" rg --type typescript "stateOverride" ./packages echo "Searching for state override related tests:" rg --type typescript "stateOverride" ./packages --glob "*spec.ts"Length of output: 375
Script:
#!/bin/bash # Search for state override related code in the codebase using 'ts' as the type echo "Searching for state override related code:" rg --type ts "stateOverride" ./packages echo "Searching for state override related tests:" rg --type ts "stateOverride" ./packages --glob "*spec.ts"Length of output: 3499
roninjin10
deleted the
10-13-_bug_fix_stateoverrides_persisting_after_call_is_finished
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
Bug Fixes
New Features