Support block overrides parameter for eth_call & debug_traceCall
#691
Conversation
WalkthroughThis pull request introduces support for block overrides in the Ethereum-like blockchain environment. The changes span multiple files, focusing on enhancing the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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 (
|
m-Peter
force-pushed
the
mpeter/eth-call-block-overrides
branch
from
1baa591 to
a4f893f
Compare
m-Peter
force-pushed
the
mpeter/eth-call-block-overrides
branch
from
a4f893f to
02ce675
Compare
m-Peter
force-pushed
the
mpeter/eth-call-block-overrides
branch
from
02ce675 to
fe53fcc
Compare
m-Peter
requested review from
janezpodhostnik,
peterargue and
zhangchiqing
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (9)
services/requester/blocks_provider.go (2)
68-73: Ensure blockOverrides is never set to a stale reference
While the approach of storing theblockOverridespointer in theBlocksProvideris flexible, consider verifying that updates to the struct instance won't lead to situations where a stale pointer is used. If the lifecycle ofBlocksProvideroutlives certain override references, unexpected behaviors could occur.
77-85: Initialize tracer to nil in constructor for clarity
Even though it's safe to leave tracer as a zero value, explicitly setting it to nil in the return statement can enhance readability and ensure future maintainers understand the initial state.Here's a small illustration:
... return &BlocksProvider{ blocks: blocks, chainID: chainID, + tracer: nil, }api/debug.go (1)
182-182: Suggest passing tracer duringNewBlocksProviderinstantiation
Instead of calling SetTracer on the created instance, consider an optional parameter in the constructor, if appropriate, to reduce the potential for usage without a tracer being set.services/requester/requester.go (4)
116-116: Potential improvement: pass chainID explicitly
You’ve already updated other references, but consider if you want to pass chainID to NewBlocksProvider in this constructor call as well, for consistency.
254-254: Suggest grouping repeated getBlockView calls
You calle.getBlockView(height, nil)in multiple locations (lines 254, 266, 279, 410, 605). If each call shares the same semantics (no block overrides), and is repeated often, consider factoring out a helper method or caching. However, if each call context is unique, remain as is.Also applies to: 266-266, 279-279, 410-410, 605-605
332-332: Assess repeated dry-run calls for performance
Each subsequent call to dryRunTx can be expensive if the system is large. If you notice performance bottlenecks, consider caching partial results or employing a more refined approach to binary search for gas usage.Also applies to: 357-357, 387-387
479-481: Recommend passingblockOverridesto getBlockView as a parameter
You’re already passingblockOverridesbut be aware it’s also stored in the provider. The double path for overrides can be tricky. Possibly unify them: if you always pass overrides as a function argument, storing them in the provider might be unnecessary.tests/e2e_web3js_test.go (1)
43-45: Enhance coverage with negative test cases
The new “test contract call overrides” scenario at lines 43–45 should also test for invalid or partial overrides, ensuring that the system gracefully handles or reports errors if contradictory overrides are passed.tests/web3js/contract_call_overrides_test.js (1)
16-96: Consider refactoring for better maintainability.The test case is thorough but contains repeated patterns that could be extracted into helper functions. Consider:
- Creating a helper function for the override verification pattern
- Defining constants for magic values like '0x2' and '0x674DB1E1'
- Adding comments to explain the expected behavior of each override
Example refactor:
+const BLOCK_NUMBER_OVERRIDE = '0x2' +const BLOCK_TIME_OVERRIDE = '0x674DB1E1' +const RANDOM_OVERRIDE = '0x7914bb5b13bac6f621bc37bbf6e406fbf4472aaaaf17ec2f309a92aca4e27fc0' + +async function verifyOverride(selector, defaultValueFn, override, overrideKey) { + // Check the default value + let call = createCallObject(selector) + let response = await helpers.callRPCMethod( + 'eth_call', + [call, 'latest', null, null] + ) + let defaultValue = await defaultValueFn() + assert.equal(web3.utils.hexToNumber(response.body.result), defaultValue) + + // Apply and verify override + let overrides = {} + overrides[overrideKey] = override + response = await helpers.callRPCMethod( + 'eth_call', + [call, 'latest', null, overrides] + ) + assert.equal(web3.utils.hexToNumber(response.body.result), web3.utils.hexToNumber(override)) +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
api/api.go(2 hunks)api/debug.go(1 hunks)bootstrap/bootstrap.go(1 hunks)services/requester/blocks_provider.go(1 hunks)services/requester/requester.go(14 hunks)tests/e2e_web3js_test.go(1 hunks)tests/web3js/contract_call_overrides_test.js(1 hunks)
🔇 Additional comments (11)
services/requester/blocks_provider.go (3)
21-43: Validate error handling behavior in the block hash retrieval lambda
Within the anonymous function at lines 26–37, any retrieval error for a block or block hash silently succeeds by returning an empty hash. This might confuse downstream code because an empty hash could be interpreted as a legitimate block hash. Consider propagating an error or logging it to avoid silent failures.
Would you like me to generate a shell script to locate all code references that compare block hashes for equality with the zero-hash, to ensure no silent error misinterpretations?
45-47: Check for potential concurrency issues with shared ‘blockOverrides’
Setting the block overrides in line 46 is straightforward. However, if multiple goroutines may call BlockContext() concurrently with different overrides, you may risk concurrency conflicts. Confirm whether each BlocksProvider instance is used in a goroutine-safe way or guard shared writes with proper synchronization mechanisms.
95-108: Consider verifying retrieved block is non-nil
In the event that no block is found for the given height, line 105 returns an error. Just make sure your error message is informative and clarifies that the block was not found at the requested height.
api/debug.go (2)
178-178: Confirm the replacement of replayer.NewBlocksProvider
Line 178 uses the new requester.NewBlocksProvider. Ensure the new blocks provider meets all the same requirements as the old one (e.g., concurrency guarantees, error handling, snapshot creation logic).
183-190: Validate correctness of partial block overrides
When only a subset of block override fields is non-nil, lines 183–190 individually apply the values. This is good. Just ensure that your unit tests confirm that partial overrides do not reset any fields (e.g., time changed but coinbase remains from original block).
services/requester/requester.go (3)
68-68: Highlight new blockOverrides param
The new blockOverrides *ethTypes.BlockOverrides parameter in the Call signature allows flexible block contexts. Make sure to thoroughly test combination overrides (e.g., a changed number with an unchanged time) to confirm it behaves as intended.
98-107: Validate that blocksProvider is properly injected
Renaming blocksProvider from replayer’s to the new logic is consistent with the PR changes. Double-check that all call sites have been updated to reference the new BlocksProvider, especially in concurrency or test contexts.
292-293: Check response for revert or error
Line 294 properly handles revert attempts by checking ErrorCode. The logic is correct, but be mindful of how the internal error is surfaced to the calling code. Make sure that full revert reason data is properly logged to ease debugging.
Also applies to: 294-294
tests/web3js/contract_call_overrides_test.js (1)
1-14: LGTM! Test setup follows best practices.
The setup code properly initializes the test environment and deploys the contract in a before hook, with appropriate status verification.
bootstrap/bootstrap.go (1)
196-196: Verify the BlocksProvider implementation compatibility.
The change from replayer.NewBlocksProvider to requester.NewBlocksProvider needs verification to ensure all required interfaces are implemented correctly.
✅ Verification successful
Based on the search results, I can now provide a final response:
The change from replayer to requester BlocksProvider is compatible
Both implementations provide the core GetSnapshotAt method required by the interface. The requester version adds two additional convenience methods (SetTracer and SetBlockOverrides) while removing the OnBlockReceived method which was specific to the replayer's needs. This is a safe change as the core snapshot functionality is maintained while adapting to the requester's specific requirements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if requester.BlocksProvider implements all methods from replayer.BlocksProvider
ast-grep --pattern 'type BlocksProvider interface {
$$$
}'
Length of output: 62
Script:
#!/bin/bash
# Let's first find the relevant files containing BlocksProvider
rg -l "BlocksProvider"
# Then examine the implementation details
rg "BlocksProvider" -B 2 -A 5
Length of output: 18889
Script:
#!/bin/bash
# Let's check the interface implementation requirements
ast-grep --pattern 'type BlockSnapshotProvider interface {
$$$
}'
# And check for any specific method signatures
rg "type BlockSnapshotProvider" -A 10
Length of output: 109
Script:
#!/bin/bash
# Let's look for the interface definition in types package
rg -l "BlockSnapshotProvider" types/
# And check both implementations to compare their methods
rg "func.*BlocksProvider" services/replayer/blocks_provider.go
rg "func.*BlocksProvider" services/requester/blocks_provider.go
Length of output: 596
api/api.go (1)
542-542: LGTM! Block overrides parameter properly integrated.
The addition of the blockOverrides parameter to the Call method is clean and consistent with the PR objectives.
Also applies to: 579-579
| return blockContext, nil | ||
| } | ||
|
|
||
| type BlocksProvider struct { |
There was a problem hiding this comment.
Can you add a comment when to use this one and when to use replayer.BlocksProvider
| bp.tracer = tracer | ||
| } | ||
|
|
||
| func (bp *BlocksProvider) SetBlockOverrides(blockOverrides *ethTypes.BlockOverrides) { |
There was a problem hiding this comment.
I'm not sure this is safe. It looks like we are using the blocks provider in multiple goroutines.
I would recommend changing this to something like:
func (bp *BlocksProvider) WithBlockOverrides(blockOverrides *ethTypes.BlockOverrides) evmTypes.BlockSnapshotProvider {
return &BlocksProviderWithOverrides{
BlocksProvider: *bp,
blockOverrides: blockOverrides,
}
}
type BlocksProviderWithOverrides struct {
BlocksProvider
blockOverrides *ethTypes.BlockOverrides
}
Closes: #685
Description
The fields that we can support are:
NumberTimeCoinbaseRandomThis allows developers to execute a contract call, at a given block, while overriding the above 4 values.
For contributor use:
masterbranchFiles changedin the Github PR explorerSummary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Tests