services/requester/blocks_provider.go (2)

68-73: Ensure blockOverrides is never set to a stale reference
While the approach of storing the blockOverrides pointer in the BlocksProvider is flexible, consider verifying that updates to the struct instance won't lead to situations where a stale pointer is used. If the lifecycle of BlocksProvider outlives 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 during NewBlocksProvider instantiation
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 call e.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 passing blockOverrides to getBlockView as a parameter
You’re already passing blockOverrides but 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:

1. Creating a helper function for the override verification pattern
2. Defining constants for magic values like '0x2' and '0x674DB1E1'
3. 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))
+}

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

#!/bin/bash
# Check if requester.BlocksProvider implements all methods from replayer.BlocksProvider
ast-grep --pattern 'type BlocksProvider interface {
  $$$
}'

#!/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

#!/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

#!/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

services/requester/blocks_provider.go (3)

14-17: Consider composition over embedding for blockSnapshot.

The struct embeds *BlocksProvider which could lead to method name collisions if BlocksProvider implements methods that match the evmTypes.BlockSnapshot interface. Consider using composition instead of embedding to make the dependencies explicit.

 type blockSnapshot struct {
-    *BlocksProvider
+    provider *BlocksProvider
     block models.Block
 }

---

26-37: Improve error handling in hash calculation.

The closure silently returns an empty hash on errors. This could mask issues and make debugging difficult. Consider logging the errors or using a different error handling strategy.

 func(n uint64) gethCommon.Hash {
     block, err := bs.blocks.GetByHeight(n)
     if err != nil {
+        bs.provider.logger.Debug().Err(err).Uint64("height", n).Msg("failed to get block by height")
         return gethCommon.Hash{}
     }
     blockHash, err := block.Hash()
     if err != nil {
+        bs.provider.logger.Debug().Err(err).Uint64("height", n).Msg("failed to calculate block hash")
         return gethCommon.Hash{}
     }

     return blockHash
 }

---

68-72: Enhance documentation for BlocksProvider usage.

While the comment explains the endpoints where this provider is used, it would be helpful to:

1. Document why this implementation is separate from replayer.BlocksProvider
2. Explain the key differences between the two implementations
3. Provide guidance on when to use each implementation

services/requester/requester.go (2)

439-447: Consider caching block providers.

Creating a new BlocksProvider instance for each getBlockView call could impact performance. Consider caching providers based on their configuration (height + overrides) or implementing a provider pool.

---

251-251: Consider future block overrides support.

Methods like GetBalance, GetNonce, GetStorageAt, and GetCode currently pass nil for block overrides. Consider if these methods should also support block overrides in the future for consistency.

Also applies to: 263-263, 276-276, 407-407

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

services/requester/requester.go (1)

Line range hint 329-357: Include blockOverrides in EstimateGas method calls.

In the EstimateGas method, when calling dryRunTx, blockOverrides is passed as nil. If blockOverrides are relevant for gas estimation, consider passing them accordingly.

Apply this diff to include blockOverrides:

 result, err = e.dryRunTx(tx, from, height, stateOverrides, nil)
+// Pass blockOverrides if applicable
+result, err = e.dryRunTx(tx, from, height, stateOverrides, blockOverrides)

services/requester/overridable_blocks_provider.go (1)

91-109: Avoid unnecessary allocations in WithTracer and WithBlockOverrides.

The methods WithTracer and WithBlockOverrides create new instances of OverridableBlocksProvider on each call, which can lead to increased memory usage and potential performance issues. Since OverridableBlocksProvider is not modified elsewhere, consider modifying the methods to update the existing instance instead of creating a new one.

Apply this diff to update the methods:

 func (bp *OverridableBlocksProvider) WithTracer(tracer *tracers.Tracer) *OverridableBlocksProvider {
-    return &OverridableBlocksProvider{
-        blocks:         bp.blocks,
-        chainID:        bp.chainID,
-        tracer:         tracer,
-        blockOverrides: bp.blockOverrides,
-    }
+    bp.tracer = tracer
+    return bp
 }

 func (bp *OverridableBlocksProvider) WithBlockOverrides(
     blockOverrides *ethTypes.BlockOverrides,
 ) *OverridableBlocksProvider {
-    return &OverridableBlocksProvider{
-        blocks:         bp.blocks,
-        chainID:        bp.chainID,
-        tracer:         bp.tracer,
-        blockOverrides: blockOverrides,
-    }
+    bp.blockOverrides = blockOverrides
+    return bp
 }

api/debug.go (1)

182-189: Avoid redundant nesting in block override assignments.

The block override assignments can be simplified by directly passing config.BlockOverrides to WithBlockOverrides, as they share the same structure.

Apply this diff to simplify the code:

 if config.BlockOverrides != nil {
     blocksProvider = blocksProvider.WithBlockOverrides(&ethTypes.BlockOverrides{
-        Number:   config.BlockOverrides.Number,
-        Time:     config.BlockOverrides.Time,
-        Coinbase: config.BlockOverrides.Coinbase,
-        Random:   config.BlockOverrides.Random,
+        *config.BlockOverrides
     })
 }

services/requester/requester.go (6)

68-68: Update interface documentation for Call method.

The Call method signature has been updated to include blockOverrides. Update the interface documentation to reflect this change for better clarity.

Apply this diff to update the documentation:

 // Call executes the given signed transaction data on the state for the given EVM block height.
 // Note, this function doesn't make any changes in the state/blockchain and is
 // useful to execute and retrieve values.
 func (e *EVM) Call(
     tx *types.DynamicFeeTx,
     from common.Address,
     height uint64,
     stateOverrides *ethTypes.StateOverride,
+    blockOverrides *ethTypes.BlockOverrides,
 ) ([]byte, error) {

---

Line range hint 251-255: Ensure consistent handling of blockOverrides in GetBalance.

In the GetBalance method, blockOverrides is passed as nil. Consider allowing blockOverrides as a parameter if future use cases require balance retrieval with block overrides.

---

Line range hint 263-267: Ensure consistent handling of blockOverrides in GetNonce.

Similar to GetBalance, consider allowing blockOverrides as a parameter in GetNonce to maintain consistency and accommodate future needs.

---

Line range hint 276-280: Ensure consistent handling of blockOverrides in GetStorageAt.

For consistency across methods, consider allowing blockOverrides as a parameter in GetStorageAt.

---

Line range hint 407-411: Ensure consistent handling of blockOverrides in GetCode.

Consider allowing blockOverrides as a parameter in GetCode to maintain consistency and future flexibility.

---

Line range hint 604-608: Ensure consistent handling of blockOverrides in state validation.

In validateTransactionWithState, getBlockView is called without blockOverrides. If transaction validation requires block overrides, consider passing them.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro