⚡ Feat: Optimize executeCall #1497
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
|
WalkthroughThe changes to the Changes
Poem
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (1)
packages/actions/src/Call/executeCall.js (1)
46-46: TODO indicates incomplete implementation needing attentionThe comment on line 46 suggests that the current approach is a temporary workaround and that
forkTransport.sendshould be used when it's available.Would you like me to open a GitHub issue to track the implementation of
forkTransport.send? This will ensure that the code is updated accordingly once the method becomes available.
| if (getStorage()?.length) { | ||
| return | ||
| } |
There was a problem hiding this comment.
Premature function exit inside asynchronous operation may skip storage updates
The return statement within the storage retrieval promise may cause the asynchronous function to exit early, potentially skipping the processing of remaining storage keys. This can lead to incomplete storage caching and inconsistent state.
Consider revising the control flow to ensure all storage keys are processed. If the intention is to skip the rest of the current promise execution, ensure that other promises continue executing as expected.
| } | ||
| for (const contract of accessListRes.result?.accessList ?? []) { | ||
| for (const storageKey of contract.storageKeys) { | ||
| const getStorage = () => vm.stateManager._baseState.caches.storage.get(createAddress(contract.address), hexToBytes(storageKey, { size: 32 })) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Accessing internal properties may lead to maintenance issues
The code accesses internal properties _baseState and caches of vm.stateManager, which are intended for internal use and may change in future versions of the library. Relying on internal implementation details can introduce maintenance challenges and compatibility issues.
Consider using the public API methods provided by vm.stateManager to interact with the storage. For example, use vm.stateManager.getContractStorage and vm.stateManager.putContractStorage for accessing and modifying contract storage.
| if (accessListRes.error) { | ||
| client.logger.error('Unable to get an access list', accessListRes.error) | ||
| } |
There was a problem hiding this comment.
Error handling may be insufficient when access list retrieval fails
Currently, if accessListRes.error exists, the code logs an error but continues execution. If the subsequent code relies on the access list being available, this might lead to unintended behavior or runtime errors.
Consider adding error handling to manage this scenario appropriately, such as:
- Returning early with an error response.
- Providing a fallback mechanism if the access list is critical for execution.
- Ensuring that subsequent code checks for the existence of
accessListRes.resultbefore proceeding.
| if (getStorage()?.length) { | ||
| return | ||
| } |
There was a problem hiding this comment.
Improper use of return inside a loop may cause premature function exit
Using return inside the inner for loop will exit the entire asynchronous function, not just skip to the next iteration. If the goal is to skip to the next iteration when getStorage()?.length is truthy, continue should be used instead of return.
Apply this diff to correct the control flow:
- if (getStorage()?.length) {
- return
- }
+ if (getStorage()?.length) {
+ continue
+ }Committable suggestion skipped: line range outside the PR's diff.
| ; (async () => { | ||
| if (!client.forkTransport) return | ||
| const blockTag = await client.getVm() | ||
| .then(vm => vm.blockchain.blocksByTag.get('forked')) | ||
| .then(block => block !== undefined ? numberToHex(block.header.number) : 'latest') | ||
| // TODO this is wrong we should use forkTransport.send but it doesn't exist yet | ||
| /** | ||
| * @type {import('../eth/EthJsonRpcResponse.js').EthCreateAccessListJsonRpcResponse} | ||
| */ | ||
| const accessListRes = await client.forkTransport?.request({ | ||
| method: 'eth_createAccessList', | ||
| params: [ | ||
| { | ||
| to: evmInput.to, | ||
| gas: evmInput.gasLimit, | ||
| // gasPrice: evmInput.gasPrice, not necessary to set this | ||
| value: evmInput.value, | ||
| data: evmInput.data, | ||
| }, | ||
| blockTag, | ||
| ], | ||
| }) | ||
| if (accessListRes.error) { | ||
| client.logger.error('Unable to get an access list', accessListRes.error) | ||
| } | ||
| for (const contract of accessListRes.result?.accessList ?? []) { | ||
| for (const storageKey of contract.storageKeys) { | ||
| const getStorage = () => vm.stateManager._baseState.caches.storage.get(createAddress(contract.address), hexToBytes(storageKey, { size: 32 })) | ||
| if (getStorage()?.length) { | ||
| return | ||
| } | ||
| /** | ||
| * @type {import('../eth/EthJsonRpcResponse.js').EthCreateAccessListJsonRpcResponse} | ||
| */ | ||
| client.forkTransport.request({ | ||
| method: 'eth_getStorageAt', | ||
| params: [contract.address, storageKey, blockTag] | ||
| }).then(async storage => { | ||
| const vm = await client.getVm() | ||
| // THIS MUST BE SYNC TO INITIALLY SET STORAGE OR RACE CONDITIONS WILL EXIST!!! | ||
| // This can be cleaned up with a state lock later | ||
| if (getStorage()?.length) { | ||
| return | ||
| } | ||
| vm.stateManager._baseState.caches.storage | ||
| .put( | ||
| createAddress(contract.address), | ||
| hexToBytes(storageKey, { size: 32 }), | ||
| hexToBytes(/** @type {import('viem').Hex}*/(storage)) | ||
| ) | ||
| }) | ||
| } | ||
| } | ||
| })() |
There was a problem hiding this comment.
Potential race conditions due to unawaited asynchronous function
The asynchronous function starting at line 41 is invoked without awaiting its completion. Since it's not awaited, the main execution flow continues without ensuring that the storage fetching and caching operations have completed. This can lead to race conditions where the required storage data may not be available when needed.
Consider awaiting the asynchronous function to ensure proper execution order:
- ; (async () => {
+ await (async () => {Alternatively, if the intention is to perform these operations in parallel, ensure that the rest of the code does not depend on the outcome of this asynchronous function.
📝 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.
| ; (async () => { | |
| if (!client.forkTransport) return | |
| const blockTag = await client.getVm() | |
| .then(vm => vm.blockchain.blocksByTag.get('forked')) | |
| .then(block => block !== undefined ? numberToHex(block.header.number) : 'latest') | |
| // TODO this is wrong we should use forkTransport.send but it doesn't exist yet | |
| /** | |
| * @type {import('../eth/EthJsonRpcResponse.js').EthCreateAccessListJsonRpcResponse} | |
| */ | |
| const accessListRes = await client.forkTransport?.request({ | |
| method: 'eth_createAccessList', | |
| params: [ | |
| { | |
| to: evmInput.to, | |
| gas: evmInput.gasLimit, | |
| // gasPrice: evmInput.gasPrice, not necessary to set this | |
| value: evmInput.value, | |
| data: evmInput.data, | |
| }, | |
| blockTag, | |
| ], | |
| }) | |
| if (accessListRes.error) { | |
| client.logger.error('Unable to get an access list', accessListRes.error) | |
| } | |
| for (const contract of accessListRes.result?.accessList ?? []) { | |
| for (const storageKey of contract.storageKeys) { | |
| const getStorage = () => vm.stateManager._baseState.caches.storage.get(createAddress(contract.address), hexToBytes(storageKey, { size: 32 })) | |
| if (getStorage()?.length) { | |
| return | |
| } | |
| /** | |
| * @type {import('../eth/EthJsonRpcResponse.js').EthCreateAccessListJsonRpcResponse} | |
| */ | |
| client.forkTransport.request({ | |
| method: 'eth_getStorageAt', | |
| params: [contract.address, storageKey, blockTag] | |
| }).then(async storage => { | |
| const vm = await client.getVm() | |
| // THIS MUST BE SYNC TO INITIALLY SET STORAGE OR RACE CONDITIONS WILL EXIST!!! | |
| // This can be cleaned up with a state lock later | |
| if (getStorage()?.length) { | |
| return | |
| } | |
| vm.stateManager._baseState.caches.storage | |
| .put( | |
| createAddress(contract.address), | |
| hexToBytes(storageKey, { size: 32 }), | |
| hexToBytes(/** @type {import('viem').Hex}*/(storage)) | |
| ) | |
| }) | |
| } | |
| } | |
| })() | |
| await (async () => { | |
| if (!client.forkTransport) return | |
| const blockTag = await client.getVm() | |
| .then(vm => vm.blockchain.blocksByTag.get('forked')) | |
| .then(block => block !== undefined ? numberToHex(block.header.number) : 'latest') | |
| // TODO this is wrong we should use forkTransport.send but it doesn't exist yet | |
| /** | |
| * @type {import('../eth/EthJsonRpcResponse.js').EthCreateAccessListJsonRpcResponse} | |
| */ | |
| const accessListRes = await client.forkTransport?.request({ | |
| method: 'eth_createAccessList', | |
| params: [ | |
| { | |
| to: evmInput.to, | |
| gas: evmInput.gasLimit, | |
| // gasPrice: evmInput.gasPrice, not necessary to set this | |
| value: evmInput.value, | |
| data: evmInput.data, | |
| }, | |
| blockTag, | |
| ], | |
| }) | |
| if (accessListRes.error) { | |
| client.logger.error('Unable to get an access list', accessListRes.error) | |
| } | |
| for (const contract of accessListRes.result?.accessList ?? []) { | |
| for (const storageKey of contract.storageKeys) { | |
| const getStorage = () => vm.stateManager._baseState.caches.storage.get(createAddress(contract.address), hexToBytes(storageKey, { size: 32 })) | |
| if (getStorage()?.length) { | |
| return | |
| } | |
| /** | |
| * @type {import('../eth/EthJsonRpcResponse.js').EthCreateAccessListJsonRpcResponse} | |
| */ | |
| client.forkTransport.request({ | |
| method: 'eth_getStorageAt', | |
| params: [contract.address, storageKey, blockTag] | |
| }).then(async storage => { | |
| const vm = await client.getVm() | |
| // THIS MUST BE SYNC TO INITIALLY SET STORAGE OR RACE CONDITIONS WILL EXIST!!! | |
| // This can be cleaned up with a state lock later | |
| if (getStorage()?.length) { | |
| return | |
| } | |
| vm.stateManager._baseState.caches.storage | |
| .put( | |
| createAddress(contract.address), | |
| hexToBytes(storageKey, { size: 32 }), | |
| hexToBytes(/** @type {import('viem').Hex}*/(storage)) | |
| ) | |
| }) | |
| } | |
| } | |
| })() |
Description
Concise description of proposed changes
Testing
Explain the quality checks that have been done on the code changes
Additional Information
Your ENS/address:
Summary by CodeRabbit
New Features
Bug Fixes