Docker updates to build and run #696
Conversation
Co-authored-by: Leo Zhang <[email protected]>
…e call tracer used during block replay
|
PR now has updates to address various build and architecture issues and an improved README.md. The README now strictly covers only the development and local build/test related concerns, with all other documentation on gateway operations being mastered in the Flow public docs |
![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
🧹 Outside diff range and nitpick comments (2)
Makefile (2)
127-134: LGTM! Consider validating ARCH value.The implementation looks good, but consider validating the ARCH value to prevent invalid architectures.
ifdef GOARCH + @if ! echo "$(GOARCH)" | grep -qE '^(amd64|arm64)$$'; then \ + echo "ERROR: Invalid GOARCH. Supported values: amd64, arm64"; \ + exit 1; \ + fi $(eval ARCH=$(GOARCH)) endif
181-211: Enhance docker-run target documentation.While the implementation is solid, consider adding inline comments to explain the logic flow and variable interactions.
.PHONY: docker-run docker-run: + # Reset CMD_ARGS to ensure clean state $(eval CMD_ARGS :=) + + # Configure container run mode (detached or interactive) ifdef DOCKER_RUN_DETACHED $(eval MODE=-d) endif + + # Configure host port mapping ifdef DOCKER_HOST_PORT $(eval HOST_PORT=$(DOCKER_HOST_PORT)) else $(eval HOST_PORT=8545) endif
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
.github/workflows/build.yml(1 hunks)Makefile(3 hunks)
🧰 Additional context used
🪛 Gitleaks (8.21.2)
Makefile
13-13: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
24-24: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (4)
.github/workflows/build.yml (1)
43-43: LGTM! Docker build command improvements.
The changes improve the Docker build command by:
- Using consistent variable naming (
VERSIONinstead ofGATEWAY_VERSION). - Adding architecture support through the
ARCHbuild argument.
Makefile (3)
88-88: LGTM! Consistent version handling.
The build command now correctly uses IMAGE_TAG for version information, maintaining consistency with the Docker image versioning.
61-63: 🛠️ Refactor suggestion
Make container registry configurable.
The container registry is hardcoded to a specific GCP registry. Consider making it configurable for different environments.
-export CONTAINER_REGISTRY := us-west1-docker.pkg.dev/dl-flow-devex-production/development
+export CONTAINER_REGISTRY ?= ${DOCKER_REGISTRY:-us-west1-docker.pkg.dev/dl-flow-devex-production/development}Likely invalid or redundant comment.
2-7: 🛠️ Refactor suggestion
Add error handling for Git commands.
The Git commands might fail in environments where Git metadata isn't available (e.g., shallow clones in CI/CD).
-SHORT_COMMIT := $(shell git rev-parse --short HEAD)
-BRANCH_NAME:=$(shell git rev-parse --abbrev-ref HEAD | tr '/' '-')
-COMMIT := $(shell git rev-parse HEAD)
-GIT_VERSION := $(shell git describe --tags --abbrev=2 2>/dev/null)
+SHORT_COMMIT := $(shell git rev-parse --short HEAD 2>/dev/null || echo "unknown")
+BRANCH_NAME := $(shell git rev-parse --abbrev-ref HEAD 2>/dev/null | tr '/' '-' || echo "unknown")
+COMMIT := $(shell git rev-parse HEAD 2>/dev/null || echo "unknown")
+GIT_VERSION := $(shell git describe --tags --abbrev=2 2>/dev/null || echo "unknown")Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
Makefile (3)
18-18: Consider disabling CGO for better portability.The codebase shows no direct CGO dependencies. Setting
CGO_ENABLED=0would improve binary portability.-COMPILER_FLAGS := CGO_ENABLED=1 +COMPILER_FLAGS := CGO_ENABLED=0
112-112: Remove duplicate implementation.The start-local-bin target duplicates the implementation of start-local. Consider consolidating these targets.
.PHONY: start-local-bin start-local-bin: start-local - rm -rf db/ - rm -rf metrics/data/ - $(COMPILER_FLAGS) go run cmd/main.go run $(EMULATOR_ARGS)Also applies to: 119-119
189-192: Simplify mount handling.The current mount handling could be simplified by combining the mount setup and database directory configuration.
ifdef DOCKER_MOUNT - $(eval MOUNT=--mount type=bind,src="$(DOCKER_MOUNT)",target=$(DATADIR)) - $(call check_and_append,database-dir,DATADIR) + $(eval CMD_ARGS += --mount type=bind,src="$(DOCKER_MOUNT)",target=$(DATADIR) --database-dir=$(DATADIR)) endif
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
Makefile(3 hunks)
🧰 Additional context used
🪛 Gitleaks (8.21.2)
Makefile
13-13: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
24-24: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (4)
Makefile (4)
30-34: LGTM: Clear version handling logic.
The version handling logic is well-structured with clear precedence between VERSION and IMAGE_TAG variables.
36-54: LGTM: Well-structured helper functions and OS detection.
The check_and_append function provides good error handling, and the OS detection is comprehensive with appropriate compiler flags.
2-7: 🛠️ Refactor suggestion
Add error handling for Git commands.
The Git commands might fail in CI/CD environments where Git metadata isn't available. Consider adding fallback values.
-SHORT_COMMIT := $(shell git rev-parse --short HEAD)
-BRANCH_NAME:=$(shell git rev-parse --abbrev-ref HEAD | tr '/' '-')
-COMMIT := $(shell git rev-parse HEAD)
-GIT_VERSION := $(shell git describe --tags --abbrev=2 2>/dev/null)
+SHORT_COMMIT := $(shell git rev-parse --short HEAD 2>/dev/null || echo "unknown")
+BRANCH_NAME := $(shell git rev-parse --abbrev-ref HEAD 2>/dev/null | tr '/' '-' || echo "unknown")
+COMMIT := $(shell git rev-parse HEAD 2>/dev/null || echo "unknown")
+GIT_VERSION := $(shell git describe --tags --abbrev=2 2>/dev/null || echo "unknown")Likely invalid or redundant comment.
55-57: 🛠️ Refactor suggestion
Make container registry configurable.
The container registry is hardcoded to a specific GCP registry. Consider making it configurable for different environments.
-export CONTAINER_REGISTRY := us-west1-docker.pkg.dev/dl-flow-devex-production/development
+export CONTAINER_REGISTRY ?= ${DOCKER_REGISTRY:-us-west1-docker.pkg.dev/dl-flow-devex-production/development}Likely invalid or redundant comment.
… container tp stop it looking on the container local loopback IP
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
Makefile (5)
18-18: Consider making CGO configurable via environment variable.The
COMPILER_FLAGSis initialized withCGO_ENABLED=1, but this might not be optimal for all build scenarios. Consider making it configurable:-COMPILER_FLAGS := CGO_ENABLED=1 +COMPILER_FLAGS := CGO_ENABLED=$(CGO_ENABLED)
27-28: Make profiler settings configurable.The profiler settings are hardcoded. Consider making them configurable via environment variables:
- --profiler-enabled=true \ - --profiler-port=6060 + --profiler-enabled=$(PROFILER_ENABLED) \ + --profiler-port=$(PROFILER_PORT)
44-53: Enhance OS detection robustness.The OS detection is limited to Darwin and Linux. Consider:
- Adding Windows support
- Handling unknown architectures
- Making architecture configurable via environment variables
ifeq ($(UNAME_S),Darwin) OS := macos - ARCH := arm64 + ARCH ?= $(shell uname -m) $(eval COMPILER_FLAGS += CGO_CFLAGS="-O2 -D__BLST_PORTABLE__") else ifeq ($(UNAME_S),Linux) OS := linux - ARCH := amd64 + ARCH ?= $(shell uname -m) +else ifeq ($(UNAME_S),Windows_NT) + OS := windows + ARCH ?= $(shell uname -m) else $(error Unsupported operating system: $(UNAME_S)) endif
58-58: Document DATADIR variable usage.The
DATADIRvariable is exported but lacks documentation about its purpose and requirements.Add a comment explaining its usage:
+# Directory for persistent data storage in Docker containers export DATADIR := /data
202-202: Make WebSocket and rate limit settings configurable.The WebSocket and rate limit settings are hardcoded. Consider making them configurable:
- $(eval CMD_ARGS += --ws-enabled=true --rate-limit=9999999 --rpc-host=0.0.0.0 --log-level=info) + $(eval CMD_ARGS += --ws-enabled=$(WS_ENABLED) --rate-limit=$(RATE_LIMIT) --rpc-host=$(RPC_HOST) --log-level=$(LOG_LEVEL))Add default values at the top of the Makefile:
+# Default values for Docker run configuration +WS_ENABLED ?= true +RATE_LIMIT ?= 9999999 +RPC_HOST ?= 0.0.0.0 +LOG_LEVEL ?= info
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
Makefile(3 hunks)
🧰 Additional context used
🪛 Gitleaks (8.21.2)
Makefile
13-13: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
24-24: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
dev/flow.json (2)
2-7: Consider using environment variables for network configurationsUsing hardcoded URLs and ports makes it difficult to switch environments and could cause conflicts when running emulator and testing environments simultaneously.
Consider using environment variables:
{ "networks": { - "emulator": "127.0.0.1:3569", - "mainnet": "access.mainnet.nodes.onflow.org:9000", - "testing": "127.0.0.1:3569", - "testnet": "access.devnet.nodes.onflow.org:9000" + "emulator": "${FLOW_EMULATOR_HOST:-127.0.0.1}:${FLOW_EMULATOR_PORT:-3569}", + "mainnet": "${FLOW_MAINNET_HOST:-access.mainnet.nodes.onflow.org}:${FLOW_MAINNET_PORT:-9000}", + "testing": "${FLOW_TESTING_HOST:-127.0.0.1}:${FLOW_TESTING_PORT:-3570}", + "testnet": "${FLOW_TESTNET_HOST:-access.devnet.nodes.onflow.org}:${FLOW_TESTNET_PORT:-9000}" }
1-14: Consider environment-specific configuration filesThe current setup combines all environment configurations in a single file that's included in the Docker build context. This could lead to:
- Accidental deployment of development configurations to production
- Increased attack surface by including unnecessary network configurations
- Difficulty in managing different environment settings
Consider:
- Splitting configurations into environment-specific files:
flow.development.jsonflow.production.jsonflow.testing.json- Using Docker build arguments to copy the appropriate configuration file
- Implementing a configuration validation step in CI/CD pipeline
🧰 Tools
🪛 Gitleaks (8.21.2)
11-11: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
Makefile (2)
36-41: Add documentation for the check_and_append function.Consider adding a comment explaining the function's purpose and parameters:
# Function to check and append required arguments +# Args: +# 1: The flag name to append (without --) +# 2: The environment variable name to check define check_and_append
176-205: Consider adding Docker best practices.The docker-run target could benefit from additional Docker best practices:
- Add health check
- Set resource limits
- Add logging options
$(eval CMD_ARGS += --ws-enabled=true --rate-limit=9999999 --rpc-host=0.0.0.0 --log-level=info) $(call check_and_append,access-node-spork-hosts,ACCESS_NODE_SPORK_HOSTS) - docker run $(MODE) -p $(HOST_PORT):8545 -p 8080:8080 $(MOUNT) "$(CONTAINER_REGISTRY)/evm-gateway:$(IMAGE_TAG)" $(CMD_ARGS) + docker run $(MODE) \ + -p $(HOST_PORT):8545 -p 8080:8080 \ + --health-cmd "curl -f http://localhost:8545" \ + --health-interval=30s \ + --health-timeout=10s \ + --health-retries=3 \ + --memory=4g \ + --memory-swap=4g \ + --cpus=2 \ + --log-driver=json-file \ + --log-opt max-size=10m \ + --log-opt max-file=3 \ + $(MOUNT) \ + "$(CONTAINER_REGISTRY)/evm-gateway:$(IMAGE_TAG)" \ + $(CMD_ARGS)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
Makefile(3 hunks)dev/flow.json(1 hunks)
🧰 Additional context used
🪛 Gitleaks (8.21.2)
Makefile
13-13: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
24-24: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
dev/flow.json
11-11: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (3)
dev/flow.json (1)
8-13:
SECURITY: Remove hardcoded private key from configuration
Exposing private keys in source code is a critical security risk, especially when:
- The configuration file is included in the Docker build context
- The private key is committed to version control
- The key could be exposed in the final Docker image
Let's verify the exposure:
Recommendations:
- Remove the hardcoded key immediately
- Use environment variables or secure secret management:
{
"accounts": {
"emulator-account": {
"address": "${FLOW_ACCOUNT_ADDRESS}",
- "key": "b96ce56f16ef8dc0badfcd2ecc9c4ee972e1f987015974e1f07f39c3f26d5982"
+ "key": "${FLOW_ACCOUNT_PRIVATE_KEY}"
}
}
}- Add
flow.jsonto.gitignore - Provide a
flow.json.exampletemplate without sensitive data - Document the required environment variables in README.md
🧰 Tools
🪛 Gitleaks (8.21.2)
11-11: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
Makefile (2)
30-34: LGTM: Clean version handling implementation.
The version handling logic is clear and follows a good pattern:
- VERSION defaults to SHORT_COMMIT if not set
- IMAGE_TAG defaults to VERSION if not set
44-53: Consider adding Windows support.
The OS detection currently only supports macOS and Linux. If Windows support is planned, consider adding it:
else ifeq ($(UNAME_S),Linux)
OS := linux
ARCH := amd64
+else ifeq ($(UNAME_S),Windows_NT)
+ OS := windows
+ ARCH := amd64
else
$(error Unsupported operating system: $(UNAME_S))
endif
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
Makefile (2)
182-186: Add validation for HOST_PORT range.The HOST_PORT value should be validated to ensure it's within a valid range (1-65535).
ifdef DOCKER_HOST_PORT + @if [ $(DOCKER_HOST_PORT) -lt 1 ] || [ $(DOCKER_HOST_PORT) -gt 65535 ]; then \ + echo "ERROR: DOCKER_HOST_PORT must be between 1 and 65535"; \ + exit 1; \ + fi $(eval HOST_PORT=$(DOCKER_HOST_PORT)) else $(eval HOST_PORT=8545) endif
133-134: Consider making emulator startup wait configurable.The fixed 3-second sleep might not be sufficient for the emulator to fully initialize on all systems.
+EMULATOR_STARTUP_WAIT ?= 3 flow emulator -f dev/flow.json & - sleep 3 + sleep $(EMULATOR_STARTUP_WAIT)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
evm-gateway-arch.svgis excluded by!**/*.svg
📒 Files selected for processing (2)
Makefile(3 hunks)dev/Dockerfile(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- dev/Dockerfile
🧰 Additional context used
🪛 Gitleaks (8.21.2)
Makefile
13-13: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
24-24: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (5)
Makefile (5)
2-7: Add error handling for Git commands.
The Git commands might fail in CI/CD environments where Git metadata isn't available.
11-13: Critical: Remove hardcoded sensitive data.
The emulator configuration contains hardcoded API keys and addresses which pose a security risk.
🧰 Tools
🪛 Gitleaks (8.21.2)
13-13: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
18-18: CGO can be disabled for better portability.
The codebase shows no direct CGO dependencies.
55-57: Make container registry configurable.
The container registry is hardcoded to a specific GCP registry.
112-112: Remove duplication between start-local and start-local-bin targets.
Both targets use identical cleanup steps and EMULATOR_ARGS.
Also applies to: 119-119
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
README.md (3)
13-13: Fix heading hierarchy for better document structure.The heading level jumps from h1 to h3. Maintain consistent heading hierarchy by using h2 for the "Design" section.
-### Design +## Design🧰 Tools
🪛 Markdownlint (0.37.0)
13-13: Expected: h2; Actual: h3
Heading levels should only increment by one level at a time(MD001, heading-increment)
54-56: Enhance local development setup instructions.Add more context about prerequisites and expected behavior:
To run an emulator-based local version for development, with the necessary flags set: +# Ensure Flow CLI and emulator are installed and running first +# Expected output: Gateway should start and connect to the emulator ```bash make start-local-bin<details> <summary>🧰 Tools</summary> <details> <summary>🪛 LanguageTool</summary> [uncategorized] ~54-~54: This expression is usually spelled with a hyphen. Context: ...-evm-gateway run {flags} ``` To run an emulator based local version for development, with the... (BASED_HYPHEN) </details> </details> --- Line range hint `307-311`: **Add security and usage guidelines for debugging features.** The profiling instructions should include usage guidelines and security considerations. ```diff # Debugging +> ⚠️ **Warning**: Profiling features can impact performance and expose sensitive information. +> - Only enable profiling in development or testing environments +> - Ensure profiler endpoints are not publicly accessible +> - Monitor resource usage when profiling is enabled + ## Profiler The EVM Gateway supports profiling via the `pprof` package. To enable profiling, add the following flags to the command line:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
README.md(9 hunks)
🧰 Additional context used
🪛 Markdownlint (0.37.0)
README.md
13-13: Expected: h2; Actual: h3
Heading levels should only increment by one level at a time
(MD001, heading-increment)
26-26: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
79-79: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
99-99: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
145-145: Column: 13
Hard tabs
(MD010, no-hard-tabs)
146-146: Column: 12
Hard tabs
(MD010, no-hard-tabs)
147-147: Column: 10
Hard tabs
(MD010, no-hard-tabs)
148-148: Column: 17
Hard tabs
(MD010, no-hard-tabs)
149-149: Column: 21
Hard tabs
(MD010, no-hard-tabs)
152-152: null
Bare URL used
(MD034, no-bare-urls)
133-133: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
155-155: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
201-201: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
202-202: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
203-203: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
204-204: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
205-205: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
206-206: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
207-207: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
197-197: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
289-289: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
290-290: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
291-291: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
289-289: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
290-290: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
291-291: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
285-285: null
Bare URL used
(MD034, no-bare-urls)
287-287: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🪛 LanguageTool
README.md
[uncategorized] ~54-~54: This expression is usually spelled with a hyphen.
Context: ...-evm-gateway run {flags} ``` To run an emulator based local version for development, with the...
(BASED_HYPHEN)
[uncategorized] ~199-~199: This expression is usually spelled with a hyphen.
Context: ... To use the make target to connect a container based gateway instance to testnet requires th...
(BASED_HYPHEN)
[uncategorized] ~201-~201: Loose punctuation mark.
Context: ...s to be set. * ACCESS_NODE_GRPC_HOST: access.devnet.nodes.onflow.org:9000 * ...
(UNLIKELY_OPENING_PUNCTUATION)
🪛 Gitleaks (8.21.2)
README.md
168-168: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (1)
README.md (1)
201-207:
Remove sensitive information from environment variables documentation.
The documentation contains actual configuration values that should be replaced with placeholders.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~201-~201: Loose punctuation mark.
Context: ...s to be set. * ACCESS_NODE_GRPC_HOST: access.devnet.nodes.onflow.org:9000 * ...
(UNLIKELY_OPENING_PUNCTUATION)
🪛 Markdownlint (0.37.0)
201-201: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
202-202: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
203-203: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
204-204: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
205-205: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
206-206: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
207-207: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
There was a problem hiding this comment.
Actionable comments posted: 24
🧹 Outside diff range and nitpick comments (53)
storage/mocks/mocks.go (1)
28-31: Consider more realistic mock valuesSeveral suggestions to improve the mock data:
- The hash format change from
0x1337%dto0x100%dmight break existing tests- The fixed
TotalGasUsedof 30,000 might not represent realistic scenariosConsider these improvements:
-ReceiptRoot: common.HexToHash(fmt.Sprintf("0x100%d", height)), -TransactionHashRoot: common.HexToHash(fmt.Sprintf("0x200%d", height)), -TotalGasUsed: uint64(30_000), -PrevRandao: common.HexToHash(fmt.Sprintf("0x300%d", height)), +ReceiptRoot: common.HexToHash(fmt.Sprintf("0x1337%d", height)), // Maintain existing format +TransactionHashRoot: common.HexToHash(fmt.Sprintf("0x1338%d", height)), +TotalGasUsed: uint64(30_000 + (height * 1000)), // Variable gas based on height +PrevRandao: common.HexToHash(fmt.Sprintf("0x1339%d", height)),tests/web3js/build_evm_state_test.js (1)
Line range hint
6-236: Consider improving test organization and documentation.While the test comprehensively covers EVM interactions, consider these improvements:
- Break down the large test into smaller, focused sub-tests using
describeblocks- Document the significance of specific block heights (e.g., why block 82?)
- Add comments explaining the test strategy and progression
Example refactor:
describe('EVM State Building', () => { describe('EOA Management', () => { it('should create and fund multiple EOAs', async () => { // EOA creation and funding tests }) it('should handle transfers between EOAs', async () => { // Transfer tests }) }) describe('Contract Interactions', () => { it('should deploy and interact with contracts', async () => { // Contract deployment and interaction tests }) }) describe('Historical State', () => { it('should verify historical balances and nonces', async () => { // Historical state tests }) it('should handle gas estimation at different heights', async () => { // Gas estimation tests with documented block heights }) }) })storage/pebble/storage_test.go (2)
32-32: Consider using pebble.NoSync for test performanceUsing
pebble.Syncin tests ensures durability but might impact test performance. Consider usingpebble.NoSyncfor tests unless strict durability is required for specific test cases.-err = batch.Commit(pebble.Sync) +err = batch.Commit(pebble.NoSync)Also applies to: 50-50, 82-82
Line range hint
1-177: Consider adding batch operation helper functionsThe test file shows repeated patterns for batch operations (create -> defer close -> perform operations -> commit). Consider extracting these into test helper functions to reduce duplication and enforce consistent batch handling patterns.
Example helper function:
func withBatch(t *testing.T, db *Storage, fn func(*pebble.Batch) error) { batch := db.NewBatch() defer func() { require.NoError(t, batch.Close()) }() err := fn(batch) require.NoError(t, err) err = batch.Commit(pebble.NoSync) require.NoError(t, err) }tests/integration_test.go (1)
62-74: Consider extracting common configuration setup to reduce duplication.The configuration blocks across all three test functions share significant similarities. Consider creating a helper function to generate the base configuration with common values, allowing test-specific overrides.
func getTestConfig(t *testing.T, opts ...func(*config.Config)) config.Config { cfg := config.Config{ DatabaseDir: t.TempDir(), AccessNodeHost: "localhost:3569", RPCPort: 8545, RPCHost: "127.0.0.1", FlowNetworkID: "flow-emulator", EVMNetworkID: types.FlowEVMPreviewNetChainID, LogLevel: zerolog.DebugLevel, LogWriter: testLogWriter(), } for _, opt := range opts { opt(&cfg) } return cfg } // Usage example: cfg := getTestConfig(t, func(c *config.Config) { c.Coinbase = eoaTestAccount c.COAAddress = *createdAddr c.COAKeys = keys c.GasPrice = new(big.Int).SetUint64(0) })Also applies to: 167-179, 269-281
services/requester/utils.go (1)
11-12: Enhance function documentation for better clarity.The current documentation is minimal. Consider adding more detailed documentation that includes:
- Purpose and use cases
- Parameter descriptions
- Return value description
- Example usage
-// replaceAddresses replace the addresses based on the network +// replaceAddresses replaces import statements in a Cadence script with network-specific +// contract addresses based on the provided chain ID. +// +// Parameters: +// - script: byte slice containing the Cadence script with import statements +// - chainID: Flow chain ID used to determine the correct contract addresses +// +// Returns: +// - byte slice containing the modified script with replaced addresses +// +// Example: +// input: "import FungibleToken" +// output: "import FungibleToken from 0x1234..."storage/index_test.go (2)
25-25: Correct grammatical error in the commentThe comment should use the correct verb form "conforms".
Apply this diff to correct the grammar:
-// tests that make sure the implementation conform to the interface expected behaviour +// tests that make sure the implementation conforms to the interface expected behaviour
155-162: Optimize batch writes in the loopCreating and committing a new batch in each iteration can be inefficient. Consider using a single batch for all iterations to improve performance.
Apply this change to accumulate writes in a single batch:
-batch := b.DB.NewBatch() -for i := 0; i < 10; i++ { - err := b.Blocks.Store(uint64(i+5), flow.Identifier{byte(i)}, mocks.NewBlock(uint64(10+i)), batch) - b.Require().NoError(err) - - err = batch.Commit(pebble2.Sync) - b.Require().NoError(err) -} +batch := b.DB.NewBatch() +for i := 0; i < 10; i++ { + err := b.Blocks.Store(uint64(i+5), flow.Identifier{byte(i)}, mocks.NewBlock(uint64(10+i)), batch) + b.Require().NoError(err) +} +err := batch.Commit(pebble2.Sync) +b.Require().NoError(err)eth/types/types.go (3)
Line range hint
25-42: Add missing fields toTransactionArgsfor blob transactionsThe
TransactionArgsstruct does not include the new fieldsMaxFeePerBlobGasandBlobVersionedHashesnecessary for handling blob transactions. To support these transactions and ensure consistency with theTransactionstruct, consider adding these fields.Apply the following diff to include the missing fields:
type TransactionArgs struct { From *common.Address `json:"from"` To *common.Address `json:"to"` Gas *hexutil.Uint64 `json:"gas"` GasPrice *hexutil.Big `json:"gasPrice"` MaxFeePerGas *hexutil.Big `json:"maxFeePerGas"` MaxPriorityFeePerGas *hexutil.Big `json:"maxPriorityFeePerGas"` Value *hexutil.Big `json:"value"` Nonce *hexutil.Uint64 `json:"nonce"` // We accept "data" and "input" for backwards-compatibility reasons. // "input" is the newer name and should be preferred by clients. // Issue detail: https://github.com/ethereum/go-ethereum/issues/15628 Data *hexutil.Bytes `json:"data"` Input *hexutil.Bytes `json:"input"` // Introduced by AccessListTxType transaction. AccessList *types.AccessList `json:"accessList,omitempty"` ChainID *hexutil.Big `json:"chainId,omitempty"` + MaxFeePerBlobGas *hexutil.Big `json:"maxFeePerBlobGas,omitempty"` + BlobVersionedHashes []common.Hash `json:"blobVersionedHashes,omitempty"` }
Line range hint
25-42: Consider adding unit tests for updatedTransactionArgsvalidationThe
TransactionArgsstruct and itsValidatemethod have been modified. To maintain robustness, consider adding or updating unit tests that cover the new fields and validation logic, especially for scenarios involving blob transactions.
Line range hint
25-42: Update JSON tags for consistency and clarityEnsure that the JSON tags for all fields in the
TransactionArgsstruct consistently useomitemptywhere appropriate. This helps in omitting empty fields during JSON serialization, reducing payload size, and avoiding potential confusion for clients consuming the API.Apply the following diff for consistency:
MaxFeePerGas *hexutil.Big `json:"maxFeePerGas,omitempty"` MaxPriorityFeePerGas *hexutil.Big `json:"maxPriorityFeePerGas,omitempty"` Value *hexutil.Big `json:"value,omitempty"` Nonce *hexutil.Uint64 `json:"nonce,omitempty"` Data *hexutil.Bytes `json:"data,omitempty"` Input *hexutil.Bytes `json:"input,omitempty"` AccessList *types.AccessList `json:"accessList,omitempty"` ChainID *hexutil.Big `json:"chainId,omitempty"` MaxFeePerBlobGas *hexutil.Big `json:"maxFeePerBlobGas,omitempty"` BlobVersionedHashes []common.Hash `json:"blobVersionedHashes,omitempty"`services/requester/cross-spork_client.go (2)
19-22: Consider simplifying the field name for readabilityThe field
getEventsForHeightRangeLimiterin thesporkClientstruct is quite lengthy and may affect readability. Consider renaming it to something more concise, such aseventsLimiterorrangeLimiter, to improve code clarity.
30-36: Use a context-aware rate limiter to handle context cancellationIn the
GetEventsForHeightRangemethod, the call tos.getEventsForHeightRangeLimiter.Take()may block indefinitely without respecting context cancellation. To prevent potential blocking and improve responsiveness, consider using a context-aware rate limiter that can be canceled via the provided context.storage/register_delta.go (1)
96-98: Wrap error with additional context inallocateSlabIndexIn the
allocateSlabIndexfunction, consider wrapping the error returned fromAccountStatusFromByteswith additional context to improve error traceability.Apply this diff to provide more informative error messages:
- return atree.SlabIndex{}, err + return atree.SlabIndex{}, fmt.Errorf("failed to parse account status from bytes: %w", err)storage/pebble/register_storage.go (4)
65-69: Avoid usingrecoverto handle panics inGetmethodUsing
recoverto handle panics may mask underlying issues and make debugging difficult. It's better to prevent panics or let them propagate if they indicate unrecoverable errors.Consider removing the
deferwithrecoverand handle errors explicitly.- defer func() { - if r := recover(); r != nil { - err = fmt.Errorf("panic: %v", r) - } - }()
71-72: Reuseownervariable to avoid redundant conversionYou have already converted
id.Ownerto an address and stored it in theownervariable. Use this variable to avoid redundant conversion.Apply this diff to optimize the code:
- owner := flow.BytesToAddress([]byte(id.Owner)) - if r.owner != flow.BytesToAddress([]byte(id.Owner)) { + owner := flow.BytesToAddress([]byte(id.Owner)) + if r.owner != owner {
175-176: Correct typo in variable name fromonesComplimenttoonesComplementThe variable
onesComplimentis misspelled. It should beonesComplementas it represents the one's complement of the height.Apply this diff to correct the typo:
- onesCompliment := ^height - lookupKey.encoded = binary.BigEndian.AppendUint64(lookupKey.encoded, onesCompliment) + onesComplement := ^height + lookupKey.encoded = binary.BigEndian.AppendUint64(lookupKey.encoded, onesComplement)
157-157: Fix duplicate word in commentThere's a duplicate word "lookup" in the comment. It should read "The lookup key is used..."
Apply this diff to fix the comment:
- // The lookup lookupKey used to find most recent value for a register. + // The lookup key is used to find the most recent value for a register.services/requester/remote_cadence_arch.go (2)
103-107: Pass context from caller instead of using 'context.Background()'Using
context.Background()inrunCalllimits the ability to control cancellation and timeouts. It's better to pass acontext.Contextparameter from the caller to allow for better flexibility in managing the request lifecycle.Modify the method signature and usage:
-func (rca *RemoteCadenceArch) runCall(input []byte) (*evmTypes.ResultSummary, error) { +func (rca *RemoteCadenceArch) runCall(ctx context.Context, input []byte) (*evmTypes.ResultSummary, error) { ... scriptResult, err := rca.client.ExecuteScriptAtBlockHeight( - context.Background(), + ctx, rca.blockHeight, replaceAddresses(dryRunScript, rca.chainID), []cadence.Value{hexEncodedTx, hexEncodedAddress}, )Update the callers to pass a context:
In
RequiredGas:func (rca *RemoteCadenceArch) RequiredGas(input []byte) uint64 { + ctx := context.TODO() // Replace with appropriate context evmResult, err := rca.runCall(ctx, input)In
Run:func (rca *RemoteCadenceArch) Run(input []byte) ([]byte, error) { + ctx := context.TODO() // Replace with appropriate context key := hex.EncodeToString(crypto.Keccak256(input)) // Existing code... evmResult, err := rca.runCall(ctx, input)Consider accepting a
context.Contextparameter inRunandRequiredGasmethods to propagate the context appropriately.
93-96: Simplify hex string encoding using 'hexutil'When converting byte slices to hex strings, you can use the
hexutilpackage from go-ethereum for convenience and consistency.Apply this diff to use
hexutil.Encode:-import ( - "encoding/hex" - // other imports -) +import ( + "github.com/onflow/go-ethereum/common/hexutil" + // other imports +) ... -hexEncodedTx, err := cadence.NewString(hex.EncodeToString(encodedTx)) +hexEncodedTx, err := cadence.NewString(hexutil.Encode(encodedTx))services/replayer/blocks_provider.go (2)
29-35: Log errors when retrieving block hashes for improved visibilityIn the anonymous function within
BlockContext, errors fromGetByHeightandblock.Hash()are silently ignored by returning an empty hash. Logging these errors can aid in diagnosing issues related to block retrieval and hashing.Implement logging for the errors:
27 func(n uint64) gethCommon.Hash { 28 block, err := bs.blocks.GetByHeight(n) 29 if err != nil { 30 + fmt.Printf("Error retrieving block at height %d: %v\n", n, err) 31 return gethCommon.Hash{} 32 } 33 blockHash, err := block.Hash() 34 if err != nil { 35 + fmt.Printf("Error hashing block at height %d: %v\n", n, err) 36 return gethCommon.Hash{} 37 } 38 return blockHash 39 },Alternatively, use a proper logging mechanism if available.
66-72: Ensure sequential block heights to maintain chain integrityThe
OnBlockReceivedmethod checks for non-sequential block heights but could provide more informative error messages. Including the expected next height enhances clarity.Update the error message to indicate the expected height:
66 if bp.latestBlock != nil && bp.latestBlock.Height != (block.Height-1) { 67 return fmt.Errorf( 68 - "%w: received new block: %d, non-sequential of latest block: %d", 68 + "%w: received new block: %d, expected next block height: %d", 69 models.ErrInvalidHeight, 70 block.Height, 71 - bp.latestBlock.Height, 71 + bp.latestBlock.Height+1, 72 ) 73 }cmd/export/cmd.go (1)
21-21: Provide specific error messages for missing required flagsWhen required flags are missing, indicating which ones are missing can help users correct their input more efficiently.
Modify the error handling to specify missing flags:
if height == 0 || outputDir == "" || registerStoreDir == "" { missingFlags := []string{} if height == 0 { missingFlags = append(missingFlags, "--evm-height") } if outputDir == "" { missingFlags = append(missingFlags, "--output") } if registerStoreDir == "" { missingFlags = append(missingFlags, "--register-store") } return fmt.Errorf("the following flags must be provided: %v", missingFlags) }services/evm/executor.go (4)
133-135: Log errors when retrieving blocks inGetHashFuncErrors from
GetByHeightare currently ignored. Logging these errors can help diagnose issues related to block retrieval.Implement logging for the error:
132 block, err := s.blocks.GetByHeight(n) 133 if err != nil { 134 + s.logger.Error().Err(err).Uint64("height", n).Msg("failed to get block by height") 135 return common.Hash{} 136 }
137-139: Log errors when hashing blocks inGetHashFuncIf an error occurs during block hashing, logging it provides valuable information for troubleshooting.
Add logging for the hashing error:
136 blockHash, err := block.Hash() 137 if err != nil { 138 + s.logger.Error().Err(err).Uint64("height", n).Msg("failed to hash block") 139 return common.Hash{} 140 }
87-89: Handle execution errors with more contextWhen an error occurs during transaction execution, providing additional context can help identify the cause.
Wrap the error with transaction details:
87 if err != nil { 88 - return err 88 + return fmt.Errorf("transaction execution failed for tx %s: %w", tx.Hash(), err) 89 }
94-95: Clarify error message for invalid transactionsRephrasing the error message enhances readability and understanding.
Apply this diff to improve the error message:
93 if res.Invalid() { 94 - return fmt.Errorf("invalid transaction %s: %w", tx.Hash(), res.ValidationError) 94 + return fmt.Errorf("validation failed for transaction %s: %w", tx.Hash(), res.ValidationError) 95 }services/replayer/call_tracer_collector.go (1)
95-193: Refactor panic recovery logic to eliminate duplicationThe deferred functions in
OnTxStart,OnTxEnd,OnEnter,OnExit, andOnLogcontain duplicated panic recovery code. To enhance maintainability and reduce repetition, consider abstracting this logic into a separate helper function that can be reused across these methods.services/ingestion/engine.go (3)
43-43: Consider adding field documentationThe new fields added to the
Enginestruct lack documentation explaining their purpose and usage. Consider adding comments to describe:
blocksProvider: Purpose and interaction with replayerregisterStore: Storage functionality and data formattraces: Tracing capabilitiesreplayerConfig: Configuration options and impacttype Engine struct { *models.EngineStatus subscriber EventSubscriber + // blocksProvider manages block retrieval and caching for replay operations blocksProvider *replayer.BlocksProvider store *pebble.Storage + // registerStore handles persistent storage of EVM register state registerStore *pebble.RegisterStorage blocks storage.BlockIndexer receipts storage.ReceiptIndexer transactions storage.TransactionIndexer + // traces stores execution traces for transaction replay and debugging traces storage.TraceIndexer log zerolog.Logger evmLastHeight *models.SequentialHeight blocksPublisher *models.Publisher[*models.Block] logsPublisher *models.Publisher[[]*gethTypes.Log] collector metrics.Collector + // replayerConfig contains settings for transaction replay operations replayerConfig replayer.Config }Also applies to: 45-45, 49-49, 55-55
139-143: Consider adding retry mechanism for failed event processingThe event processing error handling is currently fatal, which may be too aggressive. Consider implementing a retry mechanism with backoff for transient failures.
err := e.processEvents(events.Events) if err != nil { - e.log.Error().Err(err).Msg("failed to process EVM events") - return err + e.log.Warn().Err(err).Msg("failed to process EVM events, retrying...") + // Add exponential backoff retry logic here + return fmt.Errorf("failed to process events after retries: %w", err) }
383-392: Consider optimizing register entries conversionThe
registerEntriesFromKeyValuefunction could be optimized by pre-allocating the slice with the exact capacity needed.func registerEntriesFromKeyValue(keyValue map[flowGo.RegisterID]flowGo.RegisterValue) []flowGo.RegisterEntry { - entries := make([]flowGo.RegisterEntry, 0, len(keyValue)) + entries := make([]flowGo.RegisterEntry, len(keyValue)) + i := 0 for k, v := range keyValue { - entries = append(entries, flowGo.RegisterEntry{ + entries[i] = flowGo.RegisterEntry{ Key: k, Value: v, - }) + } + i++ } return entries }api/debug.go (2)
45-53: Consider grouping related fields in DebugAPI structThe fields could be organized better by grouping related functionality together using embedded structs.
type DebugAPI struct { + // Storage-related fields registerStore *pebble.RegisterStorage - logger zerolog.Logger tracer storage.TraceIndexer blocks storage.BlockIndexer transactions storage.TransactionIndexer receipts storage.ReceiptIndexer + + // Client and configuration client *requester.CrossSporkClient config config.Config + + // Observability collector metrics.Collector + logger zerolog.Logger }
89-105: Add metrics for trace cache hits/missesConsider adding metrics to track trace cache performance.
if isDefaultCallTracer(config) { + d.collector.TraceAttempt() trace, err := d.tracer.GetTransaction(hash) if err == nil { + d.collector.TraceCacheHit() return trace, nil } if !errors.Is(err, errs.ErrEntityNotFound) { + d.collector.TraceError() d.logger.Error().Err(err).Msgf( "failed to retrieve default call trace for tx: %s", hash, ) return nil, err } + d.collector.TraceCacheMiss() }bootstrap/bootstrap.go (3)
54-54: Consider using functional options pattern for Bootstrap configurationThe Bootstrap struct initialization could benefit from using the functional options pattern for better flexibility and maintainability.
+type BootstrapOption func(*Bootstrap) +func WithMetrics(metrics *flowMetrics.Server) BootstrapOption { + return func(b *Bootstrap) { + b.metrics = metrics + } +} +func WithProfiler(profiler *api.ProfileServer) BootstrapOption { + return func(b *Bootstrap) { + b.profiler = profiler + } +} type Bootstrap struct { logger zerolog.Logger config config.Config client *requester.CrossSporkClient storages *Storages publishers *Publishers collector metrics.Collector server *api.Server metrics *flowMetrics.Server events *ingestion.Engine profiler *api.ProfileServer db *pebbleDB.DB }Also applies to: 63-63
396-404: Enhance database shutdown loggingThe database shutdown logging could be improved to provide more context about the shutdown process.
func (b *Bootstrap) StopDB() { if b.db == nil { return } + b.logger.Info().Msg("Initiating PebbleDB shutdown...") err := b.db.Close() if err != nil { - b.logger.Err(err).Msg("PebbleDB graceful shutdown failed") + b.logger.Error(). + Err(err). + Str("db_path", b.config.DatabaseDir). + Msg("PebbleDB graceful shutdown failed") + } else { + b.logger.Info().Msg("PebbleDB shutdown completed successfully") } }
585-587: Document race condition prevention strategyThe comment about starting the API Server first to avoid races should be expanded to better explain the potential race conditions and mitigation strategy.
- // Start the API Server first, to avoid any races with incoming - // EVM events, that might affect the starting state. + // Start the API Server first to establish a consistent initial state. + // This prevents race conditions where: + // 1. Incoming EVM events could be processed before the API is ready + // 2. Client requests could arrive before the server state is initialized + // 3. Metrics collection could begin before the server is fully operationalservices/ingestion/event_subscriber.go (4)
67-67: Consider making buffer size configurableThe channel buffer size is hardcoded to 1000, which might not be optimal for all scenarios. Consider making it configurable.
+const defaultEventBufferSize = 1000 type RPCEventSubscriber struct { logger zerolog.Logger client *requester.CrossSporkClient chain flowGo.ChainID height uint64 + bufferSize int recovery bool recoveredEvents []flow.Event } func NewRPCEventSubscriber( logger zerolog.Logger, client *requester.CrossSporkClient, chainID flowGo.ChainID, startHeight uint64, + bufferSize int, ) *RPCEventSubscriber { + if bufferSize <= 0 { + bufferSize = defaultEventBufferSize + } logger = logger.With().Str("component", "subscriber").Logger() return &RPCEventSubscriber{ logger: logger, client: client, chain: chainID, height: startHeight, + bufferSize: bufferSize, } } func (r *RPCEventSubscriber) Subscribe(ctx context.Context) <-chan models.BlockEvents { - eventsChan := make(chan models.BlockEvents, 1000) + eventsChan := make(chan models.BlockEvents, r.bufferSize)
233-233: Document the rationale for maxRangeForGetEvents valueThe constant
maxRangeForGetEventsvalue of 249 needs documentation explaining why this specific value was chosen.-const maxRangeForGetEvents = uint64(249) +// maxRangeForGetEvents defines the maximum number of blocks that can be fetched in a single +// GetEventsForHeightRange call. This value is set to 249 to: +// 1. Stay within the Flow node's maximum response size limits +// 2. Optimize network bandwidth usage +// 3. Balance between request frequency and response size +const maxRangeForGetEvents = uint64(249)
282-284: Improve error messages for length/height mismatchesThe error messages for length and height mismatches could be more descriptive by including the actual values.
if len(transactions) != len(blocks) { - return 0, fmt.Errorf("transactions and blocks have different length") + return 0, fmt.Errorf( + "mismatch between transaction count (%d) and block count (%d)", + len(transactions), + len(blocks), + ) } // ... similar changes for other error messages ... if transactions[i].Height != blocks[i].Height { - return 0, fmt.Errorf("transactions and blocks have different height") + return 0, fmt.Errorf( + "height mismatch at index %d: transaction height %d != block height %d", + i, + transactions[i].Height, + blocks[i].Height, + ) }Also applies to: 362-364, 375-377
490-492: Consider adding recovery metricsThe recovery logic should include metrics to track recovery attempts and success rates.
if errors.Is(err, errs.ErrMissingBlock) || r.recovery { + r.collector.RecoveryAttempt("missing_block") + result := r.accumulateEventsMissingBlock(events) + if result.Err == nil { + r.collector.RecoverySuccess("missing_block") + } else { + r.collector.RecoveryFailure("missing_block") + } - return r.accumulateEventsMissingBlock(events) + return result }services/requester/requester.go (2)
46-46: MakeblockGasLimitconfigurable for greater flexibilityThe
blockGasLimitis hardcoded as a constant value120_000_000. To improve flexibility and adaptability to different environments or network conditions, consider makingblockGasLimita configurable parameter via application configuration or environment variables. This change would allow for easier adjustments without modifying the source code.Also applies to: 367-367
356-445: Improve readability by refactoring theEstimateGasmethodThe
EstimateGasfunction is quite lengthy and contains complex logic for gas estimation, including a binary search algorithm. Consider refactoring this method to break it down into smaller, well-named helper functions that encapsulate specific parts of the logic. This will enhance readability and maintainability, making it easier for future developers to understand and modify the code.models/events_test.go (1)
79-97: Simplify redundant test code by utilizing helper functionsThere is similar logic in multiple test cases, such as setting up block events and asserting error messages. Consider creating helper functions to reduce code duplication and improve the clarity of the test cases. This refactoring will make the tests more maintainable and easier to extend in the future.
Also applies to: 312-352
services/ingestion/engine_test.go (1)
629-666: Add comments to explainsetupStorefunction's purpose and workflowThe
setupStorefunction initializes the storage and register storage for the tests, but it lacks explanatory comments. Adding comments to describe the purpose of each step, especially the significance of setting the account status and storage addresses, will improve code readability and help future contributors understand the test setup process.tests/web3js/debug_traces_test.js (1)
183-184: Enhance test coverage by validating additional transaction trace fieldsThe assertions focus on specific fields like
from,to,gas, andvalue. Consider adding assertions for other relevant fields such asnonce,gasPrice, andblockNumberto thoroughly validate the transaction traces. This will ensure that all critical aspects of the transaction are correctly traced and reported.Also applies to: 210-211, 222-223
services/ingestion/mocks/EventSubscriber.go (1)
Line range hint
1-1: Note: This is an auto-generated fileChanges to this file should be made through the source interface definition rather than direct modification.
services/evm/extract_test.go (1)
22-24: Enhance test failure outputThe current diff output only prints index and raw diff. Consider enhancing the error message to be more descriptive about what exactly differs between the states.
- fmt.Printf("Difference %d: %v\n", i, diff) + t.Logf("State difference %d:\n Key: %s\n Expected: %v\n Got: %v\n", + i, diff.Key, diff.Expected, diff.Actual)storage/pebble/keys.go (1)
27-28: Document the register key formatThe
registerKeyMarkeris used in the MVCC comparer but its key format is not documented. Please add documentation explaining the expected format and the significance of the last 8 bytes that are handled specially in the Split function.// registers + // Register keys have format: [registerKeyMarker][custom_key][version_bytes] + // where version_bytes is an 8-byte suffix representing the MVCC version registerKeyMarker = byte(50)models/block.go (1)
77-82: Consider adding version field for future compatibilityThe current approach of using
PrevRandaozero hash to detect legacy blocks might become problematic with future changes. Consider adding an explicit version field.type Block struct { *types.Block + Version uint32 FixedHash gethCommon.Hash TransactionHashes []gethCommon.Hash }tests/e2e_web3js_test.go (1)
92-108: Good improvement: Added block initialization setupThe addition of setup logic ensures proper block initialization before running log filtering tests, preventing potential false negatives.
However, consider extracting the block initialization logic into a helper function as it might be useful for other tests.
+func initializeEmptyBlock(emu emulator.Emulator) error { + res, err := flowSendTransaction( + emu, + `transaction() { + prepare(signer: auth(Storage) &Account) { + let currentBlock = getCurrentBlock() + assert(currentBlock.height > 0, message: "current block is zero") + } + }`, + ) + if err != nil { + return err + } + return res.Error +}api/api.go (1)
30-31: Suggestion: MakeBlockGasLimitconfigurableThe hardcoded constant
BlockGasLimitis set to120_000_000. Consider making this value configurable via theconfig.Configstruct to allow flexibility for different network configurations and future adjustments.services/replayer/config.go (1)
7-12: Suggestion: RenameRootAddrtoRootAddressfor clarityIn the
Configstruct, consider renamingRootAddrtoRootAddressto improve code readability and maintain consistency with common naming conventions.Apply this diff to implement the change:
type Config struct { ChainID flow.ChainID - RootAddr flow.Address + RootAddress flow.Address CallTracerCollector EVMTracer ValidateResults bool }tests/web3js/debug_util_test.js (1)
5-15: Suggestion: Wrap the test case in adescribeblock for better organizationTo enhance readability and organization, consider encapsulating the test within a
describeblock. This helps group related tests and makes the test suite easier to navigate.Example:
describe('Debug Utilities Tests', () => { it('should retrieve flow height', async () => { let response = await helpers.callRPCMethod( 'debug_flowHeightByBlock', ['latest'] ); assert.equal(response.status, 200); assert.isDefined(response.body); let height = response.body.result; assert.equal(height, conf.startBlockHeight); }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumtests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (82)
api/api.go(39 hunks)api/debug.go(2 hunks)api/encode_transaction.go(0 hunks)api/net.go(1 hunks)api/pull.go(5 hunks)api/pull_test.go(1 hunks)api/server.go(2 hunks)api/stream.go(5 hunks)api/utils.go(1 hunks)api/wallet.go(4 hunks)bootstrap/bootstrap.go(14 hunks)cmd/export/cmd.go(1 hunks)cmd/main.go(2 hunks)cmd/run/cmd.go(5 hunks)config/config.go(0 hunks)eth/types/types.go(1 hunks)eth/types/types_test.go(1 hunks)go.mod(7 hunks)metrics/server.go(0 hunks)models/block.go(7 hunks)models/block_test.go(6 hunks)models/events.go(4 hunks)models/events_test.go(9 hunks)models/receipt_test.go(1 hunks)models/transaction.go(3 hunks)models/transaction_test.go(4 hunks)services/evm/executor.go(1 hunks)services/evm/extract.go(1 hunks)services/evm/extract_test.go(1 hunks)services/ingestion/engine.go(7 hunks)services/ingestion/engine_test.go(18 hunks)services/ingestion/event_subscriber.go(1 hunks)services/ingestion/event_subscriber_test.go(10 hunks)services/ingestion/mocks/EventSubscriber.go(1 hunks)services/ingestion/subscriber.go(0 hunks)services/replayer/blocks_provider.go(1 hunks)services/replayer/blocks_provider_test.go(1 hunks)services/replayer/call_tracer_collector.go(1 hunks)services/replayer/config.go(1 hunks)services/requester/cadence/create_coa.cdc(0 hunks)services/requester/cadence/get_balance.cdc(0 hunks)services/requester/cadence/get_code.cdc(0 hunks)services/requester/cadence/get_nonce.cdc(0 hunks)services/requester/cross-spork_client.go(5 hunks)services/requester/pool.go(1 hunks)services/requester/remote_cadence_arch.go(1 hunks)services/requester/remote_state.go(0 hunks)services/requester/remote_state_test.go(0 hunks)services/requester/requester.go(8 hunks)services/requester/requester_test.go(0 hunks)services/requester/utils.go(1 hunks)services/testutils/mock_client.go(3 hunks)services/traces/downloader.go(0 hunks)services/traces/engine.go(0 hunks)services/traces/engine_test.go(0 hunks)services/traces/mocks/Downloader.go(0 hunks)storage/index.go(0 hunks)storage/index_test.go(19 hunks)storage/mocks/AccountIndexer.go(0 hunks)storage/mocks/mocks.go(2 hunks)storage/pebble/accounts.go(0 hunks)storage/pebble/blocks.go(2 hunks)storage/pebble/db.go(1 hunks)storage/pebble/keys.go(3 hunks)storage/pebble/ledger.go(0 hunks)storage/pebble/receipts.go(3 hunks)storage/pebble/register_storage.go(1 hunks)storage/pebble/register_storage_test.go(1 hunks)storage/pebble/storage.go(3 hunks)storage/pebble/storage_test.go(4 hunks)storage/pebble/traces.go(0 hunks)storage/pebble/transactions.go(0 hunks)storage/register_delta.go(1 hunks)storage/register_delta_test.go(1 hunks)tests/e2e_web3js_test.go(3 hunks)tests/go.mod(6 hunks)tests/helpers.go(2 hunks)tests/integration_test.go(5 hunks)tests/web3js/build_evm_state_test.js(2 hunks)tests/web3js/debug_traces_test.js(1 hunks)tests/web3js/debug_util_test.js(1 hunks)tests/web3js/eth_deploy_contract_and_interact_test.js(2 hunks)
⛔ Files not processed due to max files limit (6)
- tests/web3js/eth_get_storage_at_test.js
- tests/web3js/eth_logs_filtering_test.js
- tests/web3js/eth_non_interactive_test.js
- tests/web3js/eth_rate_limit_test.js
- tests/web3js/eth_revert_reason_test.js
- tests/web3js/verify_cadence_arch_calls_test.js
💤 Files with no reviewable changes (21)
- services/requester/cadence/get_nonce.cdc
- storage/index.go
- storage/pebble/traces.go
- api/encode_transaction.go
- storage/pebble/transactions.go
- services/traces/mocks/Downloader.go
- services/traces/engine_test.go
- metrics/server.go
- services/requester/cadence/create_coa.cdc
- services/requester/cadence/get_balance.cdc
- services/traces/downloader.go
- services/traces/engine.go
- config/config.go
- services/requester/cadence/get_code.cdc
- storage/mocks/AccountIndexer.go
- services/requester/remote_state.go
- storage/pebble/accounts.go
- services/requester/requester_test.go
- services/requester/remote_state_test.go
- storage/pebble/ledger.go
- services/ingestion/subscriber.go
✅ Files skipped from review due to trivial changes (1)
- eth/types/types_test.go
🔇 Additional comments (79)
services/requester/pool.go (1)
68-68: Review the backoff strategy changes carefully
The change from a Fibonacci to a constant backoff strategy with different timing parameters raises several concerns:
- The maximum duration has been reduced from 3 minutes to 1 minute, which might cause issues with slow transactions
- Using a constant 1-second retry interval instead of a progressive backoff could be more aggressive on the system
- Less adaptive retry behavior might impact system performance under load
Let's verify the transaction sealing times to ensure the new parameters are appropriate:
Consider the following alternatives:
- Keep the Fibonacci backoff for better adaptivity but with adjusted parameters
- Add monitoring/metrics for transaction sealing times to validate these parameters
- Make these timing parameters configurable to allow tuning based on network conditions
✅ Verification successful
Backoff strategy change appears reasonable
Based on the codebase analysis, the change to a constant backoff with 1-minute maximum duration is appropriate:
- The retry is only used for transaction sealing status checks (
TransactionStatusSealed) - No evidence of long-running transactions requiring more than 1-minute retry window was found
- The constant 1-second interval is consistent with other timeouts in the codebase (e.g., HTTP timeouts are set to 1 second minimum)
- The change simplifies the retry logic while maintaining reasonable bounds for transaction processing
The simpler constant backoff strategy should work well for this specific use case where we're just polling for transaction status.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any documentation or tests related to transaction sealing times
# and backoff configurations to validate the new parameters
# Check for any timing-related constants or configurations
rg -A 3 "time\.(Minute|Second)"
# Look for any transaction sealing time references in tests
rg -A 5 "TransactionStatusSealed"
# Search for any performance test configurations
fd -t f "test.*\.go$" --exec rg -l "performance|timing|duration"
Length of output: 3589
Script:
#!/bin/bash
# Let's search for historical context and any related configurations
# Search for any retry-related configurations or constants
rg -A 3 "retry|backoff|WithMaxDuration"
# Look for any transaction processing metrics or timing configurations
rg -A 3 "transaction.*time|processing.*time|sealing.*time"
# Search for any Flow network or transaction-related configurations
fd -t f "config.*\.go$" --exec rg -l "transaction|timeout|duration"
# Check for any test cases that might validate transaction processing times
ast-grep --pattern 'func Test$_($_) {
$$$
time.Sleep($_)
$$$
}'
Length of output: 5756
storage/mocks/mocks.go (2)
6-6: LGTM: Import addition is appropriate
The addition of the "time" import is correctly placed and necessary for the new Timestamp field.
28-29: Verify the impact of hash format changes
The change in hash formats from 0x1337%d to 0x100%d might affect existing tests.
✅ Verification successful
Hash format changes are safe to proceed
The search results show that:
- The modified hash formats (
0x100%d,0x200%d) are only used within the mocks package itself - No test assertions directly depend on these specific hash formats
- Other test files use different hash patterns (
0xf1%d,0x%d,0xff%d) for their mock data
The changes are isolated to mock data generation and don't impact any test assertions or dependencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test files that might be relying on the specific hash format
rg -l "0x1337" --type go
# Search for specific hash assertions in tests
rg "assert.*0x1337.*" --type go
Length of output: 58
Script:
#!/bin/bash
# Let's search for any usage of these specific hash patterns in the codebase
rg "0x100\d|0x200\d" --type go
# Also search for any test assertions involving these hashes
rg "assert.*Hash.*0x" --type go -A 2
# Look for mock hash generation patterns
rg "HexToHash.*fmt\.Sprintf" --type go
Length of output: 941
tests/web3js/build_evm_state_test.js (1)
159-159: Verify the updated gas estimates and document the differences.
The gas estimates for the store operation have been updated:
- At block 82: 21358n (previously 23823n)
- At latest block: 26811n (previously 29292n)
The significant difference between historical and current block estimates (~5.5k gas) suggests varying state conditions or implementation changes.
Consider documenting:
- Why gas estimates differ between historical and current blocks
- The rationale behind these specific gas values
Also applies to: 168-168
✅ Verification successful
Gas estimates for store operation are correctly updated
The gas estimates have been intentionally updated to reflect the actual gas costs in the EVM implementation:
- Historical block (82): 21358n gas
- Latest block: 26811n gas
The difference in gas costs between historical and latest blocks is expected because:
- The test is verifying
eth_estimateGascorrectness at different block heights - The test uses the same
store(0)operation but in different states of the contract - The gas difference (~5.5k) reflects the state-dependent costs of the operation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for related gas estimate changes or documentation
# Look for other gas estimate changes
rg -A 2 -B 2 "estimateGas.*21358|estimateGas.*26811"
# Search for documentation about gas estimates
rg -i "gas.*(estimate|calculation)" README.md docs/
Length of output: 154
Script:
#!/bin/bash
# Let's try a broader search for gas estimates and related test changes
# Search for gas estimate assertions in test files
rg -A 3 -B 3 "assert.*estimatedGas" tests/
# Look for gas-related constants or configurations
rg -A 2 -B 2 "gas.*=.*\d+n" tests/
# Search for any documentation in the root directory
fd -t f "README|CHANGELOG" -X cat {}
Length of output: 25133
Script:
#!/bin/bash
# Let's examine the test file more closely to understand the context of these gas estimates
# Look for the test case and surrounding context
rg -A 10 -B 10 "assert\.equal\(estimatedGas, 21358n\)" tests/
# Search for any store-related contract interactions
ast-grep --pattern 'store($$$)' tests/
# Look for any gas-related configurations
rg "minGasPrice|gasPrice" tests/
Length of output: 9181
storage/pebble/storage_test.go (1)
8-10: LGTM: Import changes are appropriate
The new imports for config and errors packages are necessary for the enhanced batch operations and error handling.
tests/integration_test.go (1)
85-87: LGTM! Consistent pattern for handling bootstrap completion.
The closure pattern for closing the ready channel is consistently implemented across all test functions, which is a good practice for ensuring proper cleanup.
Also applies to: 184-186, 292-294
tests/helpers.go (3)
163-165: LGTM! Consistent bootstrap completion pattern.
The closure pattern for handling bootstrap completion is well-implemented and consistent with the integration tests.
155-155: Verify the impact of increased rate limit.
The rate limit has been significantly increased from 50 to 500. While this may improve test performance, it's important to verify that this doesn't mask any underlying issues.
✅ Verification successful
Based on the search results, I can now generate the final response:
Rate limit increase is safe and justified
The rate limit increase from 50 to 500 is safe because:
- The rate limit is applied per client and is used to protect against API abuse
- The codebase has proper rate limiting implementation using
go.uber.org/ratelimitlibrary - No rate limit related errors or timeouts were found in the error handling code
- The test file
eth_rate_limit_test.jsspecifically tests the rate limiting behavior - The Makefile shows that tests use a much higher rate limit (9999999) in development
The increase appears to be a reasonable adjustment to improve test performance without compromising the system's stability.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for rate-limiting related issues or timeouts in the codebase
# Search for rate limit related error handling
rg -A 3 "rate.?limit|too.?many.?requests|429"
# Search for potential bottlenecks that might have led to the rate limit increase
ast-grep --pattern 'func $_($$) {
$$$
time.Sleep($$$)
$$$
}'
Length of output: 67342
181-181: Investigate the need for increased test timeout.
The test timeout has been increased from 120s to 150s. This could indicate underlying performance issues that should be investigated rather than just increasing the timeout.
✅ Verification successful
Let me gather more information about the test timeouts and execution times.
Let me search specifically for the test files and their execution times.
Based on the analysis of the test files and sleep operations, I can now provide a final response:
The increased test timeout is justified due to multiple asynchronous operations and intentional delays.
The codebase shows several legitimate reasons for the extended timeout:
- Integration tests contain multiple
time.Sleepoperations ranging from 500ms to 5s for startup and transaction execution - Tests involve multiple blockchain interactions including contract deployments, transactions, and event streaming
- Test scenarios include concurrent transaction submissions and key rotations
- WebSocket subscriptions and event monitoring require additional wait times
- Tests verify state across multiple blocks which requires waiting for block production
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Look for potential performance bottlenecks in tests
# Search for sleep calls that might contribute to test duration
rg -A 2 "time.Sleep"
# Search for test functions that might be taking too long
ast-grep --pattern 'func Test_$_(t *testing.T) {
$$$
}'
Length of output: 1631
Script:
#!/bin/bash
# Search for test files in web3js directory that might be causing long execution times
fd -e js . web3js -x cat {}
# Look for previous timeout values in git history
git log -p -- tests/helpers.go | grep -A 2 "mocha.*timeout"
# Search for any other timeout configurations in the codebase
rg "timeout.*s" -A 2
Length of output: 4360
Script:
#!/bin/bash
# Look for test files that might be causing long execution times
fd -e test.js . tests -x cat {}
# Search for any test setup or teardown that might contribute to long execution times
rg "before|after|beforeEach|afterEach" tests -A 3
# Look for any async test operations that might be time-consuming
rg "await|setTimeout|Promise" tests -A 2
Length of output: 69264
services/requester/utils.go (2)
1-9: LGTM! Package and imports are well-organized.
The package name and imports follow Go conventions and include all necessary dependencies.
13-19: Consider making the contract list configurable and add error handling.
The hardcoded contract list might limit flexibility and reusability. Consider:
- Making the contract list configurable based on the operation being performed
- Adding error handling for invalid chain IDs
Also, verify that these are all the system contracts that need address replacement.
go.mod (2)
11-11: Verify compatibility with preview version of flow-go
The dependency on github.com/onflow/flow-go is using a preview version (v0.38.0-preview.0.4). While this is intentional, it's important to ensure stability.
✅ Verification successful
Let me gather more information about the flow-go dependency usage in the codebase.
Based on the analysis of the codebase, I can now provide a final response:
Preview version usage appears safe and intentional
The flow-go dependency is extensively used throughout the codebase, primarily for EVM-related functionality (emulator, types, events) and core Flow types. The imports pattern shows consistent usage of stable APIs, and the codebase appears well-structured to handle the preview version. The main areas using this dependency are:
- EVM integration (services/evm/*)
- Block processing (services/replayer/*)
- Event handling (services/ingestion/*)
- Storage layer (storage/*)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any known issues with the preview version
# Check release notes and discussions
gh api graphql -f query='
{
repository(owner: "onflow", name: "flow-go") {
release(tagName: "v0.38.0-preview.0.4") {
description
url
}
discussions(first: 5, orderBy: {field: UPDATED_AT, direction: DESC}) {
nodes {
title
url
category {
name
}
}
}
}
}'
Length of output: 406
Script:
#!/bin/bash
# Check how flow-go is imported and used in the codebase
rg -l "github.com/onflow/flow-go" --type go
# Check for any potential breaking changes by examining imports
rg "github.com/onflow/flow-go/" --type go -A 2
# Look for any migration guides or breaking changes in README
fd -e md -x grep -l "flow-go" {}
Length of output: 14526
198-198: Verify security implications of crypto package update
The update to golang.org/x/crypto v0.28.0 includes security fixes. Ensure there are no breaking changes.
tests/go.mod (2)
7-12: LGTM: Versions aligned with root go.mod
The dependency versions are correctly aligned with the root module:
- cadence v1.2.2
- flow-go v0.38.0-preview.0.4
- flow-go-sdk v1.2.3
223-223: Verify compatibility of core dependency updates
Multiple core dependencies have been updated:
- golang.org/x/crypto v0.28.0
- golang.org/x/sys v0.26.0
- golang.org/x/text v0.19.0
These updates should be tested thoroughly as they affect fundamental functionality.
Also applies to: 228-228, 230-230
✅ Verification successful
Based on the gathered information, I can now generate the final response:
Core dependency updates appear to be minor version increments
The changes in the core dependencies are minor version updates:
- crypto: v0.26.0 -> v0.28.0
- sys: v0.23.0 -> v0.26.0
- text: v0.17.0 -> v0.19.0
While these packages are used across multiple files in the codebase, particularly in critical areas like wallet management, authentication, and API handlers, the updates are backward-compatible minor version increments following semantic versioning. The changes don't introduce breaking changes that would affect the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any reported issues with the updated packages
# Search for potential compatibility issues in test files
rg -l "golang.org/x/(crypto|sys|text)" | xargs rg "^func.*Test.*"
Length of output: 67
Script:
#!/bin/bash
# Let's first check what Go files exist in the repository
fd -e go
# Then check the actual usage of these packages in the codebase
rg -A 3 "golang.org/x/(crypto|sys|text)"
# Check go.mod for the previous versions of these dependencies
git show HEAD~1:tests/go.mod | rg "golang.org/x/(crypto|sys|text)"
Length of output: 27442
Script:
#!/bin/bash
# Let's check for any direct usage of these packages in the codebase
rg -l "crypto\."
rg -l "sys\."
rg -l "text\."
# Also check for any test failures or issues reported in recent commits
git log -n 5 --grep="test|fail|issue|break|crash" --pretty=format:"%H %s"
Length of output: 1255
storage/pebble/storage.go (3)
19-23: 🛠️ Refactor suggestion
Update codebase to accommodate the new New function signature
The New function now accepts an existing *pebble.DB instance and no longer returns an error. This change delegates the responsibility of database initialization to the caller, simplifying the Storage struct.
To ensure consistency, verify that all instantiations of Storage have been updated to use the new New function signature:
✅ Verification successful
Let me verify the usage of the old vs new signature by checking the actual database initialization code.
All usages of pebble.New are consistent with the new signature
The codebase has been properly updated to accommodate the new New function signature. Both instances of pebble.New usage in services/ingestion/engine_test.go and bootstrap/bootstrap.go correctly initialize a *pebble.DB instance first using pebble.OpenDB and then pass it to pebble.New along with a logger. No instances of the old signature were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all instantiations of `pebble.Storage` and check for the new `New` function usage.
# Search for calls to the old `New` function signature
rg --type go 'pebble\.New\(\s*".*",'
# Search for error handling that may no longer be necessary
rg --type go 'pebble\.New\(.+\)\s*,\s*err'
# Search for instances where a `*pebble.DB` is passed to `pebble.New`
rg --type go 'pebble\.New\(\s*db,\s*'
Length of output: 253
Script:
#!/bin/bash
# Check how the pebble.DB instance is initialized before being passed to pebble.New
rg -B 5 -A 2 --type go 'pebble\.New\(db,'
Length of output: 995
33-33:
Ensure batch is not nil when calling set
The set method now directly uses batch.Set without checking if batch is nil. If batch can be nil, this change could lead to a nil pointer dereference.
Verify that all calls to set provide a non-nil *pebble.Batch instance:
If any calls pass nil as batch, they should be updated to provide a valid batch or handle the case where batch is nil.
62-62: Confirm that switching to NewBatch does not impact batch operations
The NewBatch method now returns a standard *pebble.Batch instead of an indexed batch created with NewIndexedBatch(). If any batch operations rely on indexing features, this change could affect functionality.
Check if the codebase relies on indexed batch features:
If indexed features are required, consider reverting to using NewIndexedBatch() or adjusting the code accordingly.
✅ Verification successful
Let me generate additional scripts to verify the batch operations being performed.
This will help us understand:
- How the batches are used in practice
- What operations are expected from the batch interface/type
Let me generate one final verification to check for the batch interface and its usage.
This will help us understand the actual batch operations being used in the implementation.
Switching to NewBatch is safe - no indexed operations are used
Based on the verification results, the batches are only used for basic operations (Set, Delete, Commit, Close) and not for any indexed operations (Get, Iter, First, Last, Seek). The batch usage patterns show:
- Batches are primarily used for atomic writes in test and implementation files
- Only basic batch operations are used:
batch.Set(),batch.Commit(), andbatch.Close() - No evidence of indexed batch operations being used anywhere in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Identify if indexed batch features are used with batches created by `NewBatch`.
# Find all uses of `NewBatch()`
rg --type go '\.NewBatch\('
# Search for indexed operations on batches
rg --type go 'batch\.(Get|Iter|First|Last|Seek)\('
Length of output: 2351
Script:
#!/bin/bash
# Let's examine how these batches are actually used in the codebase
# Look for batch operations in test files
rg --type go -B 2 -A 2 'batch :=.*NewBatch\(\)' storage/
# Look for the interface or type definition of batch
rg --type go -B 5 -A 5 'type.*Batch'
Length of output: 9855
Script:
#!/bin/bash
# Check for batch operations in the actual implementation files
rg --type go -B 3 -A 3 'func.*Batch' storage/pebble/
rg --type go -B 3 -A 3 'batch\.(Set|Delete|Commit|Close)' storage/pebble/
Length of output: 8269
eth/types/types.go (4)
Line range hint 181-196: Ensure tx.To() is safely handled to avoid nil pointer dereference
In the NewTransaction function, tx.To() can return nil for contract creation transactions. Although the current code checks if t := tx.To(); t != nil before dereferencing, ensure that all usages of to later in the function handle the nil case properly to prevent potential nil pointer dereferences.
Line range hint 222-234: Validate transaction type handling for new blob transaction types
The logic in NewTransaction sets additional fields based on tx.Type(). Verify that all new transaction types, especially those involving blobs (types.BlobTxType), are correctly handled and that the fields MaxFeePerBlobGas and BlobVersionedHashes are appropriately set.
Line range hint 279-292: Confirm correct handling of blob transaction receipts in MarshalReceipt
In the MarshalReceipt function, additional fields blobGasUsed and blobGasPrice are added to the receipt when tx.Type() == types.BlobTxType. Ensure that these fields are correctly populated in the Receipt structure and that their inclusion is accurate for blob transactions.
1-1: Verify package rename from api to types across the codebase
The package name has been changed from api to types. Ensure that all references to this package are updated throughout the codebase to prevent import errors and maintain consistency.
Run the following script to find any leftover references to the old package name:
storage/pebble/receipts.go (1)
116-123: Verify change in error handling in getByBlockHeight method
The getByBlockHeight method now returns an empty slice of receipts instead of an error when errs.ErrEntityNotFound occurs. Please ensure that all callers and downstream code handle this change appropriately and do not rely on the error for control flow.
Run the following script to identify all usages of getByBlockHeight and check for proper error handling:
✅ Verification successful
Let me gather more context about the error handling in the caller code.
Let me check how this method is used in the broader context by looking for its public caller GetByBlockHeight.
Change in error handling for getByBlockHeight is safe and properly handled
The change in error handling is safe because:
- The test cases explicitly verify the new behavior, expecting no error and an empty slice for non-existing block heights
- All callers properly handle both the error case and empty slice case:
services/logs/filter.gochecks for errors explicitly- Tests verify the empty slice scenario with
s.Require().Len(retReceipt, 0)
- The interface documentation in
storage/index.gocorrectly specifies the expected behavior
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of 'getByBlockHeight' to verify error handling.
rg 'getByBlockHeight' -A 3
Length of output: 737
Script:
#!/bin/bash
# Get more context around the first usage to understand error handling
rg 'getByBlockHeight' -B 5 -A 10 storage/pebble/receipts.go
Length of output: 1091
Script:
#!/bin/bash
# Find usages of the public GetByBlockHeight method
rg "GetByBlockHeight" -B 2 -A 5
Length of output: 4365
storage/pebble/blocks.go (2)
202-219: Addition of batch parameter enhances batch processing consistency
The InitHeights method now accepts a batch *pebble.Batch parameter, aligning it with other methods that utilize batch operations. This improves consistency and efficiency in batch processing.
256-264: Special handling for testnet blocks is appropriately implemented
The added logic in the getBlock method correctly addresses the known issue with block hash calculations for specific testnet block heights. The adjustment ensures that the ParentBlockHash is set correctly, preventing potential discrepancies.
services/requester/cross-spork_client.go (2)
234-250: Verified height range validation in event retrieval
The GetEventsForHeightRange method in CrossSporkClient correctly checks that the start and end heights are within the same spork client. This validation prevents inconsistencies when retrieving events and ensures that cross-spork queries are handled appropriately.
252-266: Proper aggregation of errors when closing clients
The Close method effectively aggregates errors from all spork clients using multierror. This approach ensures that all errors encountered during the closure of clients are reported without losing individual error details.
services/testutils/mock_client.go (2)
18-20: Maintain consistency with existing function pointers
The addition of GetEventsForHeightRangeFunc maintains the pattern of using function pointers for mock implementations. This approach provides flexibility in tests and keeps the mock client consistent with existing methods.
44-51: Correct delegation in GetEventsForHeightRange method
The GetEventsForHeightRange method correctly delegates to GetEventsForHeightRangeFunc if it is not nil, otherwise it calls the embedded Client's method. This ensures that the mock client can use custom behavior when needed while defaulting to standard behavior.
storage/pebble/register_storage_test.go (2)
11-91: Comprehensive tests for RegisterStorage functionality
The test cases in Test_RegisterIndex effectively cover various scenarios, including getting non-existent registers, owner mismatches, and storing registers. The use of t.Parallel() enhances test performance.
93-160: Thorough testing of StorageSnapshot behavior
The Test_StorageSnapshot function adequately tests caching behavior, including caching of nil values and repeated retrievals, ensuring efficiency and correctness.
storage/register_delta_test.go (1)
16-213: Extensive testing of RegisterDelta with storage integration
The Test_RegisterDeltaWithStorage function provides a comprehensive suite of tests for RegisterDelta, covering setting values, getting values, overwriting, unsetting, commits, and error scenarios. This ensures robust functionality of the register delta mechanism.
services/requester/remote_cadence_arch.go (2)
124-138: Ensure detailed error messages in 'parseResult'
In the parseResult function, errors are wrapped with additional context. Ensure that the error messages provide sufficient detail for debugging and use %w to wrap errors for unwrapping.
79-92: Specify the 'From' address in the transaction
In the transaction creation within runCall, the 'From' address is not specified. Depending on the downstream processing, this might cause issues if a valid sender address is required. Consider specifying a 'From' address in the transaction to ensure compatibility.
Run the following script to verify whether the missing 'From' address affects transaction execution:
api/utils.go (1)
170-172: Ensure proper handling of nil values for GasTipCap and GasFeeCap
In encodeTxFromArgs, if args.MaxPriorityFeePerGas or args.MaxFeePerGas are nil, casting them directly to *big.Int will result in nil values for GasTipCap and GasFeeCap. Confirm that types.DynamicFeeTx accepts nil for these fields, or consider assigning default values when they are nil to prevent potential runtime issues.
models/events.go (1)
213-247: Good implementation of event decoding and sorting
The addition of NewMultiBlockEvents and NewSingleBlockEvents, along with the sorting logic in NewSingleBlockEvents, effectively handles event decoding for multiple and single Flow blocks. The sorting ensures events are processed in the correct order.
cmd/run/cmd.go (2)
Line range hint 33-89: Improved error handling and graceful shutdown logic
The refactoring from Run to RunE enhances error handling by allowing errors to be returned instead of causing abrupt termination. The introduction of synchronization primitives and proper context cancellation ensures a graceful shutdown process.
247-249: Verify the impact of changing cfg from a pointer to a value
Changing cfg from *config.Config{} to config.Config{} alters how the configuration is passed throughout the application. Ensure that all functions and methods interacting with cfg accommodate this change and that there are no unintended copies or mutations due to the change from a pointer to a value.
services/ingestion/engine.go (2)
7-8: LGTM: Import added for Flow Go model
The addition of the Flow Go model import is appropriate for using Flow-specific types.
121-122: Verify event subscription error handling
The event subscription logic should handle potential race conditions between checking readiness and actual subscription.
models/events_test.go (1)
Line range hint 18-262: Ensure comprehensive error checks in event handling tests
The test functions TestNewSingleBlockEvents and TestNewMultiBlockEvents have been updated to cover various scenarios. Verify that all possible edge cases are tested, including unexpected input types and missing data. Adding more assertions for different error conditions will strengthen the test suite and ensure the reliability of event processing logic.
tests/web3js/debug_traces_test.js (1)
74-74: Ensure compatibility of BigInt usage with Web3.js methods
Passing BigInt literals, such as 100n and 1500, to contract.methods.store() may not be supported in all versions of Web3.js. Verify that the Web3.js version in use supports BigInt arguments. If not, consider converting BigInt values to strings or numbers to maintain compatibility.
Also applies to: 476-476
cmd/main.go (1)
6-6: LGTM: Clean addition of export command
The addition of the export command follows the established pattern for Cobra commands and maintains consistent error handling.
Also applies to: 27-27
api/net.go (1)
12-12: Consider performance implications of config value copying
Changing config from pointer to value type means the entire config struct will be copied when creating new NetAPI instances. While this provides better encapsulation and thread safety, it could impact performance if:
- The config struct is large
- Many
NetAPIinstances are created frequently
Let's check the size and usage of the Config struct:
Also applies to: 15-19
✅ Verification successful
Based on the results, I can now generate the final response:
Config value copying is acceptable in this context
The Config struct is used infrequently (only during API initialization) and primarily contains scalar types and small structs. The only NetAPI instance creation occurs in api/api.go during service setup. Given this usage pattern, the performance impact of copying the config struct is negligible.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check Config struct size and usage patterns
ast-grep --pattern 'type Config struct {
$$$
}'
# Check for NetAPI instance creation patterns
rg -A 2 'NewNetAPI\('
Length of output: 4862
services/ingestion/mocks/EventSubscriber.go (1)
18-28: Verify impact of removing height parameter from Subscribe interface
The removal of the height parameter from the Subscribe method represents a significant change in the event subscription interface. This could affect:
- Event replay capabilities
- Ability to subscribe from specific heights
- Existing consumers of the EventSubscriber interface
Let's analyze the impact:
✅ Verification successful
Based on the shell script results, I can now generate the final response:
Removal of height parameter from Subscribe is intentional and safe
The interface change aligns with the implementation details found in the codebase:
- Height tracking is handled internally by the subscriber (services/ingestion/event_subscriber.go)
- Event replay and height-specific functionality are maintained through internal state
- Only one consumer (engine.go) uses the Subscribe method, and it's already adapted to the new interface
The mock changes correctly reflect the simplified interface while preserving all the required functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find all implementations and usages of EventSubscriber interface
ast-grep --pattern 'interface EventSubscriber {
$$$
}'
# Find all Subscribe method calls
rg -A 3 '\.Subscribe\(ctx'
# Look for potential height-related functionality in event handling
rg -A 5 'height.*event'
Length of output: 4879
api/pull_test.go (1)
63-63: Verify config modification behavior
The change from pointer to value type for config means modifications to the config within the test won't affect the original config object. Ensure this is the intended behavior.
✅ Verification successful
Let me gather more context about the config usage pattern to verify if the change from pointer to value type is safe.
Let me check one more thing to verify if there are any modifications to the config after initialization in the test file.
Based on the gathered evidence, I can now provide the final response:
Change from pointer to value type is safe
The config is only used for reading the FilterExpiry value in the test and is not modified after initialization. The main API implementation also only reads config values without modifying them. This change is safe and follows good practice of using value types when mutation is not needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any places where the config is modified after initialization
rg -p "api\.config\." --type go
Length of output: 384
Script:
#!/bin/bash
# Check how config is used in the test file and related files
rg -p "config\." api/pull_test.go -A 2
# Check the config struct definition and initialization
ast-grep --pattern 'type Config struct {
$$$
}'
# Look for any test helper functions that might modify config
rg -p "func.*config" --type go
Length of output: 5293
Script:
#!/bin/bash
# Check the test file content to see how config is used
cat api/pull_test.go
# Look for any config modifications in the API implementation
ast-grep --pattern 'api.config = $_'
Length of output: 1556
storage/pebble/db.go (3)
15-31: LGTM! Well-configured database options
The database options are well-tuned with appropriate documentation. The configuration shows good consideration for:
- Compaction thresholds
- Memory table settings
- Concurrent compactions
33-50: LGTM! Solid level configuration with performance optimizations
Good implementation of:
- Optimized block sizes for performance
- Bloom filters for efficient prefix seeks
- Progressive scaling of target file sizes
12-13: Consider increasing the cache size for production use
The current cache size of 1MB (1 << 20) might be too small for production workloads. Consider making this configurable based on the environment and available system resources.
Let's check if there are any configuration files that might need to be updated:
api/wallet.go (2)
70-71: LGTM! Improved type safety with ethTypes
Good refactoring to use ethTypes package for transaction-related types. This change:
- Improves type safety
- Maintains consistency with Ethereum types
- Makes the code more maintainable
Also applies to: 121-121, 131-131
22-22: Verify the impact of config field type change
Changing config from pointer to value type means each WalletAPI instance will have its own copy of the config. Ensure this is intentional and doesn't affect config updates propagation.
Let's check for other config usages:
✅ Verification successful
Config field type change is safe to proceed
The change from pointer to value type for the config field is safe because:
- The config is only read within WalletAPI methods (never modified)
- It's used for accessing the wallet key for signing operations
- Each instance having its own config copy is acceptable as the config is treated as immutable within the WalletAPI scope
The change aligns with Go's best practices of not using pointers for immutable configurations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for config field usage patterns
ast-grep --pattern 'config *config.Config'
Length of output: 44
Script:
#!/bin/bash
# Let's try a different approach to find config usage patterns
# First, let's find all references to config fields and methods
rg -l "config\." --type go
# Then, let's look for config type declarations and assignments
rg "config\s+(?:\*)?config\.Config" --type go
# Also search for the WalletAPI struct definition and usages
rg "type\s+WalletAPI\s+struct" --type go -A 5
Length of output: 1699
Script:
#!/bin/bash
# Let's check how config is used within WalletAPI methods
rg "w\.config\." api/wallet.go -B 2 -A 2
# Also check for any direct config field assignments or modifications
rg "\.config\s*=" api/wallet.go -B 2 -A 2
# Let's also check the config package to understand if it's designed to be copied
fd -e go -x rg "type\s+Config\s+struct" {} -A 10 \; -x rg "Clone|Copy" {} \;
Length of output: 1277
models/block.go (2)
49-50: LGTM! Improved FixedHash field type
Good change from *string to gethCommon.Hash:
- Eliminates nil checks
- Better type safety
- Proper RLP encoding support
110-113: LGTM! Improved error handling
Good addition of proper error handling for ToBytes operation in the Hash method.
api/stream.go (2)
24-24: Good change: Config field type promotes immutability
Converting config from pointer to value type is a good practice as it ensures the configuration remains immutable during the StreamAPI's lifetime.
Line range hint 134-144: Consider adding validation for block header fields
While the block header construction looks correct, consider adding validation for critical fields like timestamps and gas limits to ensure they fall within acceptable ranges.
models/block_test.go (2)
35-42: Good addition: Enhanced block hash verification
The addition of hash verification strengthens the test by ensuring the decoded block maintains hash integrity.
77-82: Good refactor: Simplified FixedHash handling
The change from pointer to value type for FixedHash aligns with the codebase's move towards value types and simplifies the code.
Also applies to: 91-91
tests/e2e_web3js_test.go (1)
31-41: Good addition: New test cases enhance coverage
The addition of tests for Cadence architecture calls, transaction traces, and debug utilities improves the test coverage of critical functionality.
models/transaction.go (3)
170-175: LGTM: Function signature enhancement
The updated signature properly adds the TransactionEventPayload return value while maintaining the existing return types.
178-182: LGTM: Error handling update
Error handling has been properly updated to include nil for the new TransactionEventPayload return value.
243-243: LGTM: Return statement update
The return statement correctly includes the txEvent alongside existing return values.
services/replayer/blocks_provider_test.go (6)
280-297: LGTM: Well-structured test setup helper
The setupBlocksDB helper function properly initializes the test environment with temporary directory and necessary configurations.
299-308: LGTM: Tracer initialization helper
The newCallTracer helper function correctly sets up the call tracer with appropriate configuration.
28-70: LGTM: Sequential block reception test
Comprehensive test coverage for block reception scenarios:
- Tests block reception without latest block
- Validates non-sequential block error handling
- Verifies sequential block processing
72-100: LGTM: Block context validation
Thorough validation of block context properties including chain ID, block number, timestamp, and other critical parameters.
102-206: LGTM: GetHashFunc behavior testing
Comprehensive test coverage for hash function behavior:
- Tests heights >= latest block height
- Validates 256 block height range behavior
- Handles missing blocks appropriately
208-278: LGTM: Snapshot retrieval testing
Complete test coverage for snapshot retrieval:
- Tests latest block snapshots
- Validates historic block snapshots
- Properly handles missing historic blocks
services/ingestion/event_subscriber_test.go (3)
46-46: LGTM: Updated subscriber initialization
Properly updated to use the new RPCEventSubscriber with correct parameter ordering.
Also applies to: 48-48
208-211: LGTM: Enhanced assertions
Improved test assertions using require.Equal for more immediate failure detection.
408-409: LGTM: Updated mock setup
Mock setup correctly updated to handle height range parameters consistently.
Also applies to: 416-417, 420-421
tests/web3js/eth_deploy_contract_and_interact_test.js (3)
238-257: LGTM: State override implementation for eth_call
The implementation correctly tests state overrides by:
- Setting up the override with a specific state diff
- Verifying the response status and body
- Validating the expected result
259-298: LGTM: State override implementation for eth_estimateGas
The implementation properly tests gas estimation with state overrides by:
- Testing with a non-zero initial state
- Testing with a zero initial state
- Verifying the gas estimates differ due to state initialization costs
224-224: Verify updated gas estimates
The gas estimates have been reduced:
- From 23977n to 21510n
- From 27398n to 25052n
These changes appear to reflect more accurate gas calculations, but it would be good to verify the reason for the reduction.
Also applies to: 236-236
✅ Verification successful
Gas estimates are correct and expected
The gas estimates in the test file are for different scenarios:
- 21510n is for a call to
sum(100, 200)at block height '0x1' - 25052n is for the same call at 'latest' block height
- The difference is expected as the gas costs vary based on the block height due to state differences
The test is verifying that gas estimation works correctly with different block heights and state overrides, which is a valid test case. The gas estimates are deterministic and part of the test's expected behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any related gas calculation changes
rg -A 3 "gas.*estimate" --type go
Length of output: 35
Script:
#!/bin/bash
# Let's check the test file and surrounding context for gas estimates
rg -B 2 -A 2 "21510n|23977n|25052n|27398n" tests/web3js/eth_deploy_contract_and_interact_test.js
Length of output: 324
Script:
#!/bin/bash
# Let's also check git history for these specific changes
git log -p -- tests/web3js/eth_deploy_contract_and_interact_test.js | rg -B 2 -A 2 "21510n|23977n|25052n|27398n"
Length of output: 0
Script:
#!/bin/bash
# Let's check for any gas-related configurations or changes in the codebase
rg -l "gas" tests/web3js/
Length of output: 840
Script:
#!/bin/bash
# Let's check the test file content to understand the context of these gas estimates
cat tests/web3js/eth_deploy_contract_and_interact_test.js
Length of output: 12139
Script:
#!/bin/bash
# Let's also check if there are any recent changes to the EVM implementation that might affect gas calculation
rg -l "evm.*gas" .
Length of output: 39
api/server.go (1)
56-56: LGTM: Config type change improves immutability
Changing config from pointer to value type is a good improvement because:
- It prevents accidental modifications to the config
- Ensures thread-safety in concurrent operations
- Follows Go best practices for small structs
Also applies to: 69-69
api/pull.go (2)
132-132: LGTM: Consistent config type change
The change from pointer to value type for config aligns with the changes in server.go, maintaining consistency across the codebase.
Also applies to: 143-143
18-18: LGTM: Improved type consistency with ethTypes
The changes improve type consistency by:
- Adding proper import for ethTypes
- Using ethTypes.Transaction instead of local Transaction type
- Using ethTypes.NewTransactionResult for consistent type handling
Also applies to: 439-439, 468-468
api/api.go (2)
927-930: Approved: Correct calculation of gasUsedRatio in FeeHistory
Calculating gasUsedRatio using block.TotalGasUsed and BlockGasLimit provides accurate information for fee history results.
227-230: Approved: Including StartingBlock in SyncStatus
Updating the SyncStatus to include StartingBlock enhances the synchronization information provided to clients, improving transparency of the node's syncing progress.
models/receipt_test.go (1)
13-13: Verify: Confirm that ignoring the additional return value is appropriate
The decodeTransactionEvent function now returns an extra value, which is being ignored in this test case. Ensure that this additional return value is not required for the assertions and that ignoring it does not impact the validity of the test.
Handle different spork hosts depending on testnet or mainnet
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
Makefile (1)
191-194: Validate mount path before binding.The mount handling should validate that the source path exists to prevent runtime errors.
Consider adding validation:
ifdef DOCKER_MOUNT + ifeq ($(wildcard $(DOCKER_MOUNT)),) + $(error Mount path $(DOCKER_MOUNT) does not exist) + endif $(eval MOUNT=--mount type=bind,src="$(DOCKER_MOUNT)",target=$(DATADIR)) $(call check_and_append,database-dir,DATADIR) endif
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Makefile(3 hunks)
🧰 Additional context used
🪛 Gitleaks (8.21.2)
Makefile
14-14: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
25-25: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (4)
Makefile (4)
2-7: Previous review comments still apply.
The following issues from previous reviews are still valid:
- Git commands need error handling
- Sensitive data (API keys, addresses) should not be hardcoded
Also applies to: 12-14
56-58: Previous review comment about configurable container registry still applies.
113-113: Previous review comment about duplicate code in start targets still applies.
Also applies to: 120-120
11-11:
Initialize MAINNET_ACCESS_NODE_SPORK_HOSTS with proper value.
The MAINNET_ACCESS_NODE_SPORK_HOSTS variable is empty, which could cause issues when running in mainnet mode.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
Makefile (2)
10-10: Consider making testnet host configurableThe testnet access node host is hardcoded. Consider making it configurable through environment variables to support different environments and facilitate testing.
-TESTNET_ACCESS_NODE_SPORK_HOSTS := access-001.devnet51.nodes.onflow.org:9000 +TESTNET_ACCESS_NODE_SPORK_HOSTS ?= ${FLOW_TESTNET_ACCESS_NODE_SPORK_HOSTS:-access-001.devnet51.nodes.onflow.org:9000}
215-215: Document the purpose of exposed portsThe docker-run target exposes ports 8545 and 8080, but their purposes are not documented in the comments. Consider adding documentation about:
- Port 8545: JSON-RPC endpoint
- Port 8080: Purpose unclear, needs documentation
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Makefile(3 hunks)
🧰 Additional context used
🪛 Gitleaks (8.21.2)
Makefile
14-14: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
25-25: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (2)
Makefile (2)
31-35: LGTM! Clean version handling implementation.
The version handling logic is well-implemented, allowing for command-line overrides while providing sensible defaults.
38-42: LGTM! Well-implemented argument validation.
The check_and_append function provides robust validation of required arguments with clear error messages.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
Makefile (2)
10-11: Consider making access node spork hosts configurable via environment variables.The access node spork hosts are hardcoded for both testnet and mainnet. This could make it difficult to switch environments or update endpoints.
-TESTNET_ACCESS_NODE_SPORK_HOSTS := access-001.devnet51.nodes.onflow.org:9000 -MAINNET_ACCESS_NODE_SPORK_HOSTS := access-001.mainnet25.nodes.onflow.org:9000 +TESTNET_ACCESS_NODE_SPORK_HOSTS ?= ${FLOW_TESTNET_SPORK_HOSTS:-access-001.devnet51.nodes.onflow.org:9000} +MAINNET_ACCESS_NODE_SPORK_HOSTS ?= ${FLOW_MAINNET_SPORK_HOSTS:-access-001.mainnet25.nodes.onflow.org:9000}
38-42: Enhance error messages in check_and_append function.The error message could be more descriptive to help users understand the expected format and possible values.
define check_and_append $(if $($(2)),\ $(eval CMD_ARGS += --$(1)=$($(2))),\ - $(error ERROR: $(2) ENV variable is required)) + $(error ERROR: $(2) ENV variable is required. Please set $(2) to configure $(1). See README.md for valid values.)) endef
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Makefile(3 hunks)
🧰 Additional context used
🪛 Gitleaks (8.21.2)
Makefile
14-14: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
25-25: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
deploy/systemd-docker/flow-evm-gateway.service (2)
1-5: Consider adding additional unit metadata.The Unit section could benefit from additional metadata fields for better documentation and maintainability.
[Unit] Description=EVM Gateway running with Docker Requires=docker.service After=network-online.target docker.service +Documentation=https://github.com/onflow/flow-evm-gateway +PartOf=docker.service
6-9: Consider using multi-user.target instead of default.target.For system services, it's more appropriate to use multi-user.target as it ensures the service starts automatically in multi-user mode.
[Install] Alias=evm-gateway.service -WantedBy=default.target +WantedBy=multi-user.target
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
deploy/systemd-docker/flow-evm-gateway.service(1 hunks)deploy/systemd-docker/runtime-conf.env(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- deploy/systemd-docker/runtime-conf.env
🔇 Additional comments (1)
deploy/systemd-docker/flow-evm-gateway.service (1)
19-21: Document required environment variables.
The service relies on several environment variables but lacks documentation about their requirements and valid values.
Consider adding a template environment file with comments:
+# Add template file: /etc/flow/runtime-conf.env.template
+# Access node configuration
+ACCESS_NODE_GRPC_HOST=
+ACCESS_NODE_SPORK_HOSTS=
+
+# Network configuration
+FLOW_NETWORK_ID=
+INIT_CADENCE_HEIGHT=
+
+# Authentication
+COINBASE=
+COA_ADDRESS=
+COA_KEY=
+
+# Version
+VERSION=
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
Makefile (1)
Line range hint
1-216: Consider separating development and production configurationsThe Makefile currently mixes development and production configurations, which could lead to confusion and potential security risks. Consider:
- Splitting into separate Makefiles (e.g., Makefile.dev, Makefile.prod)
- Using different container registries for development and production
- Implementing stricter validation for production environment variables
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Makefile(3 hunks)deploy/systemd-docker/flow-evm-gateway.service(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- deploy/systemd-docker/flow-evm-gateway.service
🧰 Additional context used
🪛 Gitleaks (8.21.2)
Makefile
14-14: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
25-25: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (2)
Makefile (2)
83-83: LGTM: Build command properly uses IMAGE_TAG for versioning
The build command correctly incorporates the IMAGE_TAG into the binary version through ldflags.
12-14:
Critical: Move sensitive values to environment variables
The hardcoded values for EMULATOR_COINBASE, EMULATOR_COA_ADDRESS, and EMULATOR_COA_KEY pose a security risk. These should be configured through environment variables.
Apply this fix:
-EMULATOR_COINBASE := FACF71692421039876a5BB4F10EF7A439D8ef61E
-EMULATOR_COA_ADDRESS := f8d6e0586b0a20c7
-EMULATOR_COA_KEY := 2619878f0e2ff438d17835c2a4561cb87b4d24d72d12ec34569acd0dd4af7c21
+EMULATOR_COINBASE ?= ${FLOW_EMULATOR_COINBASE}
+EMULATOR_COA_ADDRESS ?= ${FLOW_EMULATOR_COA_ADDRESS}
+EMULATOR_COA_KEY ?= ${FLOW_EMULATOR_COA_KEY}Likely invalid or redundant comment.
🧰 Tools
🪛 Gitleaks (8.21.2)
14-14: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
This is the basic starting point for more docker automation and is enough to get someone started with just some EVN variables and the make commands
Summary by CodeRabbit
Release Notes
New Features
flow.jsonconfiguration file for managing network and account settings.RegisterStoragefor managing register state changes temporarily.RegisterDeltafunctionality for managing register state changes without immediate persistence.Bug Fixes
Documentation
Chores
.gitignoreto exclude specific directories and files from version control.dev/run.shscript as part of streamlining the setup process.