Docker updates to build and run #696
Conversation
Ensure ldlags VERSION pass through
Add previous spork host
franklywatson
requested review from
janezpodhostnik,
peterargue,
m-Peter and
zhangchiqing
as code owners
WalkthroughThe changes in this pull request involve updates to the Dockerfile, Makefile, README.md, GitHub Actions workflow, and other related files. Key modifications include the introduction of new build arguments for versioning and architecture, enhancements to the Makefile for improved Git integration and Docker functionalities, and extensive improvements to the README for better clarity. Additionally, a new configuration file Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
Dockerfile (1)
Line range hint
22-22: Consider using a specific Debian version instead of 'latest'.Using
debian:latestcan lead to reproducibility issues and potential security vulnerabilities. Consider pinning to a specific version.-FROM debian:latest +FROM debian:bullseye-slim
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
Dockerfile(1 hunks)Makefile(3 hunks)README.md(1 hunks)
🧰 Additional context used
🪛 Gitleaks (8.21.2)
README.md
98-99: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (2)
Dockerfile (1)
15-16: LGTM! Version argument properly configured.
The VERSION argument is correctly defined and used in the build process.
Makefile (1)
109-113: LGTM! Docker build target properly configured.
The docker-build target correctly uses VERSION and includes appropriate metadata labels.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
README.md(1 hunks)
🧰 Additional context used
🪛 Gitleaks (8.21.2)
README.md
98-99: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (1)
README.md (1)
94-109:
Add security warning and best practices for environment variables
Environment variables contain sensitive information (private keys). Add security warnings and best practices.
Apply this diff to add security warnings:
-Set the following ENV variables
+# Environment Variables Configuration
+
+## Security Warning ⚠️
+# These environment variables contain sensitive information.
+# - Never commit actual values to version control
+# - Use environment-specific configuration management
+# - Consider using secrets management solutions in production
+# - Rotate keys regularly following security best practices
+
+Set the following ENV variables in a secure manner:
```bash
+# Store these in a secure .env file (add .env to .gitignore)
FLOW_NETWORK_ID=[flow-testnet | flow-mainnet]
ACCESS_NODE_GRPC_HOST=
INIT_CADENCE_HEIGHT=211176670
COA_ADDRESS=
COA_KEY=
COINBASE=+For production deployments, refer to our security guidelines in the Deployment section.
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 Gitleaks (8.21.2)</summary>
98-99: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
</details>
</details>
</details>
</details>
</details>
<!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (2)
Makefile (2)
1-7: Add error handling for Git commandsThe Git commands could fail silently if not in a Git repository or if Git is not installed. Consider adding error handling.
-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')
9-10: Consider making ACCESS_NODE_SPORK_HOSTS configurableThe spork host is hardcoded to devnet. Consider making it configurable via environment variable with the current value as default.
-ACCESS_NODE_SPORK_HOSTS := access-001.devnet51.nodes.onflow.org:9000 +ACCESS_NODE_SPORK_HOSTS ?= access-001.devnet51.nodes.onflow.org:9000
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
.github/workflows/build.yml(1 hunks)Makefile(3 hunks)
🔇 Additional comments (1)
Makefile (1)
123-128: LGTM! Well-structured docker build target
The target correctly passes version information and adds useful Git metadata as Docker labels.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
Makefile (2)
9-10: Consider parameterizing ACCESS_NODE_SPORK_HOSTSThe spork hosts are currently hardcoded. Consider making this configurable through an environment variable to support different environments and networks.
-ACCESS_NODE_SPORK_HOSTS := access-001.devnet51.nodes.onflow.org:9000 +ACCESS_NODE_SPORK_HOSTS ?= $(FLOW_SPORK_HOSTS)
38-40: Consider making container registry configurableThe container registry is hardcoded to a specific GCP project. Consider making it configurable to support different environments and CI/CD pipelines.
-export CONTAINER_REGISTRY := us-west1-docker.pkg.dev/dl-flow-devex-production/development +export CONTAINER_REGISTRY ?= us-west1-docker.pkg.dev/dl-flow-devex-production/development
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
Makefile(3 hunks)
🔇 Additional comments (6)
Makefile (6)
12-21: LGTM! Well-structured helper functions
The helper functions are well-implemented with proper error handling and make's eval feature.
23-36: Simplify version handling logic
The version handling logic remains complex with potential circular dependency between VERSION and IMAGE_TAG.
123-128: LGTM! Well-implemented Docker build target
The docker-build target follows best practices with proper version handling and Git metadata labels.
145-149: Remove duplicate append_spork_hosts call
The append_spork_hosts function is called twice, which would duplicate the spork host arguments.
161-161:
Remove duplicate MOUNT_ARG in docker run command
MOUNT_ARG is added both through CMD_ARGS and directly in the docker run command.
- docker run $(MODE) "$(CONTAINER_REGISTRY)/evm-gateway:$(IMAGE_TAG)" $(CMD_ARGS) $(MOUNT_ARG)
+ docker run $(MODE) "$(CONTAINER_REGISTRY)/evm-gateway:$(IMAGE_TAG)" $(CMD_ARGS)Likely invalid or redundant comment.
151-155:
Fix malformed if-else block for DOCKER_MOUNT
The if-else block has syntax issues and only echoes without setting MOUNT_ARG properly.
- @if [ -z "$(DOCKER_MOUNT)" ]; then \
- echo \
- else \
- MOUNT_ARG="-v $(DOCKER_MOUNT):/flow-evm-gateway"; \
- fi
+ @if [ ! -z "$(DOCKER_MOUNT)" ]; then \
+ MOUNT_ARG="-v $(DOCKER_MOUNT):/flow-evm-gateway"; \
+ fiLikely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
Makefile (2)
9-10: Consider parameterizing ACCESS_NODE_SPORK_HOSTSThe spork hosts are currently hardcoded. Consider making this configurable through an environment variable to support different environments and networks.
-ACCESS_NODE_SPORK_HOSTS := access-001.devnet51.nodes.onflow.org:9000 +ACCESS_NODE_SPORK_HOSTS ?= $(FLOW_SPORK_HOSTS)
123-128: Consider adding more metadata labelsWhile the current labels are good, consider adding more metadata labels for better container management:
docker build --build-arg VERSION="$(VERSION)" -f Dockerfile -t "$(CONTAINER_REGISTRY)/evm-gateway:$(IMAGE_TAG)" \ - --label "git_commit=$(COMMIT)" --label "git_tag=$(IMAGE_TAG)" . + --label "git_commit=$(COMMIT)" \ + --label "git_tag=$(IMAGE_TAG)" \ + --label "git_branch=$(BRANCH_NAME)" \ + --label "build_date=$(shell date -u +'%Y-%m-%dT%H:%M:%SZ')" \ + --label "version=$(VERSION)" .
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
Makefile(3 hunks)
🔇 Additional comments (5)
Makefile (5)
12-21: LGTM! Well-structured helper functions
The helper functions are well-designed:
check_and_appendproperly validates required variablesappend_spork_hostscleanly handles multiple host entries
23-36: Simplify version handling logic
The current version handling has a potential circular dependency between VERSION and IMAGE_TAG. Consider simplifying the logic.
-# Image tag: if image tag is not set, set it with version (or short commit if empty)
-ifeq (${IMAGE_TAG},)
-IMAGE_TAG := ${VERSION}
-endif
-
-ifeq (${IMAGE_TAG},)
-IMAGE_TAG := ${SHORT_COMMIT}
-endif
-
-VERSION ?= ${IMAGE_TAG}
-
-ifeq ($(origin VERSION),command line)
-VERSION = $(VERSION)
-endif
+# Set VERSION from command line, environment, or default to SHORT_COMMIT
+VERSION ?= ${SHORT_COMMIT}
+
+# Set IMAGE_TAG from VERSION if not explicitly set
+IMAGE_TAG ?= ${VERSION}64-64: CGO can be disabled for better portability
The codebase shows no direct CGO dependencies. You can safely disable it by setting CGO_ENABLED=0 for better binary portability.
145-149: Remove duplicate append_spork_hosts call
The append_spork_hosts function is called twice, which would duplicate the spork host arguments.
$(call append_spork_hosts)
- $(call append_spork_hosts)132-159: 🛠️ Refactor suggestion
Add environment variable validation for docker-run target
The docker-run target depends on several environment variables but doesn't provide clear error messages when they're missing.
Add validation at the start of the target:
.PHONY: docker-run
docker-run:
+ @echo "Validating environment variables..."
+ @if [ -z "$(ACCESS_NODE_GRPC_HOST)" ]; then \
+ echo "ERROR: ACCESS_NODE_GRPC_HOST is required"; \
+ exit 1; \
+ fi
+ @if [ -z "$(FLOW_NETWORK_ID)" ]; then \
+ echo "ERROR: FLOW_NETWORK_ID is required"; \
+ exit 1; \
+ fi
+ @if [ -z "$(INIT_CADENCE_HEIGHT)" ]; then \
+ echo "ERROR: INIT_CADENCE_HEIGHT is required"; \
+ exit 1; \
+ fiLikely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
Makefile (3)
9-10: Consider making ACCESS_NODE_SPORK_HOSTS configurable via environmentThe spork hosts are hardcoded. Consider allowing override via environment variable for flexibility across different environments.
-ACCESS_NODE_SPORK_HOSTS := access-001.devnet51.nodes.onflow.org:9000 +ACCESS_NODE_SPORK_HOSTS ?= access-001.devnet51.nodes.onflow.org:9000
38-40: Make container registry configurableThe container registry is hardcoded to a specific environment. Consider making it configurable for different deployment scenarios.
-export CONTAINER_REGISTRY := us-west1-docker.pkg.dev/dl-flow-devex-production/development +export CONTAINER_REGISTRY ?= us-west1-docker.pkg.dev/dl-flow-devex-production/development
155-155: Make port binding configurableThe port binding is hardcoded to 8545. Consider making it configurable via environment variable.
- docker run $(MODE) -p 127.0.0.1:8545:8545 -w /flow-evm-gateway $(MOUNT) "$(CONTAINER_REGISTRY)/evm-gateway:$(IMAGE_TAG)" $(CMD_ARGS) + $(eval PORT ?= 8545) + docker run $(MODE) -p 127.0.0.1:$(PORT):$(PORT) -w /flow-evm-gateway $(MOUNT) "$(CONTAINER_REGISTRY)/evm-gateway:$(IMAGE_TAG)" $(CMD_ARGS)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
Makefile(3 hunks)
🔇 Additional comments (6)
Makefile (6)
12-21: LGTM! Well-structured helper functions
The helper functions are well-implemented with proper error handling and clear functionality.
123-127: LGTM! Well-implemented Docker build target
The Docker build target is well-implemented with proper version handling and git metadata labels for traceability.
151-153: Simplify mount handling logic
The mount handling could be simplified by directly adding to CMD_ARGS.
-ifdef DOCKER_MOUNT
- $(eval MOUNT=--mount type=bind,src="$(DOCKER_MOUNT)",target=/flow-evm-gateway)
-endif
+ifdef DOCKER_MOUNT
+ $(eval CMD_ARGS += --mount type=bind,src="$(DOCKER_MOUNT)",target=/flow-evm-gateway)
+endif23-36: 🛠️ Refactor suggestion
Simplify version handling logic
The version handling logic is overly complex with potential circular dependency between VERSION and IMAGE_TAG.
-# Image tag: if image tag is not set, set it with version (or short commit if empty)
-ifeq (${IMAGE_TAG},)
-IMAGE_TAG := ${VERSION}
-endif
-
-ifeq (${IMAGE_TAG},)
-IMAGE_TAG := ${SHORT_COMMIT}
-endif
-
-VERSION ?= ${IMAGE_TAG}
-
-ifeq ($(origin VERSION),command line)
-VERSION = $(VERSION)
-endif
+# Set VERSION from command line, environment, or default to SHORT_COMMIT
+VERSION ?= ${SHORT_COMMIT}
+
+# Set IMAGE_TAG from VERSION if not explicitly set
+IMAGE_TAG ?= ${VERSION}64-64: 🛠️ Refactor suggestion
CGO can be disabled for better portability
The codebase shows no direct CGO dependencies. Consider disabling it for better binary portability.
- CGO_ENABLED=1 go build -o flow-evm-gateway -ldflags="-X github.com/onflow/flow-evm-gateway/api.Version=$(IMAGE_TAG)" cmd/main.go
+ CGO_ENABLED=0 go build -o flow-evm-gateway -ldflags="-X github.com/onflow/flow-evm-gateway/api.Version=$(IMAGE_TAG)" cmd/main.go145-149:
Remove duplicate append_spork_hosts call
The append_spork_hosts function is called twice, which would duplicate the spork host arguments.
$(call append_spork_hosts)
- $(call append_spork_hosts)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
Makefile (3)
79-79: Make version package path configurableThe version package path is hardcoded in ldflags. Consider making it a variable for better maintainability.
+VERSION_PACKAGE := github.com/onflow/flow-evm-gateway/api - CGO_ENABLED=1 go build -o flow-evm-gateway -ldflags="-X github.com/onflow/flow-evm-gateway/api.Version=$(IMAGE_TAG)" cmd/main.go + CGO_ENABLED=1 go build -o flow-evm-gateway -ldflags="-X $(VERSION_PACKAGE).Version=$(IMAGE_TAG)" cmd/main.go
Line range hint
111-119: Extract common configuration to avoid duplicationThe configuration is duplicated between
start-localandstart-local-bintargets. Consider extracting it to a variable.+# Common emulator configuration +define EMULATOR_FLAGS + --flow-network-id=flow-emulator \ + --coinbase=$(EMULATOR_COINBASE) \ + --coa-address=$(EMULATOR_COA_ADDRESS) \ + --coa-key=$(EMULATOR_COA_KEY) \ + --wallet-api-key=$(WALLET_API_KEY) \ + --coa-resource-create=true \ + --gas-price=0 \ + --log-writer=console \ + --profiler-enabled=true \ + --profiler-port=6060 +endef + +# Configurable wallet API key with default value +WALLET_API_KEY ?= 2619878f0e2ff438d17835c2a4561cb87b4d24d72d12ec34569acd0dd4af7c21 + .PHONY: start-local start-local: rm -rf db/ rm -rf metrics/data/ - go run cmd/main.go run \ - --flow-network-id=flow-emulator \ - --coinbase=$(EMULATOR_COINBASE) \ - --coa-address=$(EMULATOR_COA_ADDRESS) \ - --coa-key=$(EMULATOR_COA_KEY) \ - --wallet-api-key=2619878f0e2ff438d17835c2a4561cb87b4d24d72d12ec34569acd0dd4af7c21 \ - --coa-resource-create=true \ - --gas-price=0 \ - --log-writer=console \ - --profiler-enabled=true \ - --profiler-port=6060 + go run cmd/main.go run $(EMULATOR_FLAGS) .PHONY: start-local-bin start-local-bin: rm -rf db/ rm -rf metrics/data/ - ./flow-evm-gateway run \ - --flow-network-id=flow-emulator \ - --coinbase=$(EMULATOR_COINBASE) \ - --coa-address=$(EMULATOR_COA_ADDRESS) \ - --coa-key=$(EMULATOR_COA_KEY) \ - --wallet-api-key=2619878f0e2ff438d17835c2a4561cb87b4d24d72d12ec34569acd0dd4af7c21 \ - --coa-resource-create=true \ - --gas-price=0 \ - --log-writer=console \ - --profiler-enabled=true \ - --profiler-port=6060 + ./flow-evm-gateway run $(EMULATOR_FLAGS)Also applies to: 128-136
🧰 Tools
🪛 Gitleaks (8.21.2)
114-114: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
193-222: Add validation for HOST_PORT and MODE valuesThe docker-run target should validate the HOST_PORT range and MODE values for better error handling.
.PHONY: docker-run docker-run: $(eval CMD_ARGS :=) ifdef DOCKER_RUN_DETACHED $(eval MODE=-d) +else + $(eval MODE=) endif 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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
Makefile(4 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)
131-131: 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: 0
🧹 Outside diff range and nitpick comments (1)
Makefile (1)
206-209: Simplify mount handling.The current mount handling could be simplified.
Apply this diff:
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)) + $(eval CMD_ARGS += --database-dir=$(DATADIR)) endif
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
Makefile(4 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)
131-131: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (7)
Makefile (7)
2-7: Add error handling for Git commands.
The Git commands might fail in CI/CD environments where Git metadata isn't available.
Apply this diff to add 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")14-50: LGTM!
The OS and architecture detection is well-implemented with proper error handling for unsupported systems.
52-54: Make container registry configurable.
The container registry is hardcoded to a specific GCP registry.
Apply this diff to make it configurable:
-export CONTAINER_REGISTRY := us-west1-docker.pkg.dev/dl-flow-devex-production/development
+export CONTAINER_REGISTRY ?= us-west1-docker.pkg.dev/dl-flow-devex-production/development79-79: CGO can be disabled for better portability.
The codebase shows no direct CGO dependencies.
Apply this diff:
- CGO_ENABLED=1 go build -o flow-evm-gateway -ldflags="-X github.com/onflow/flow-evm-gateway/api.Version=$(IMAGE_TAG)" cmd/main.go
+ CGO_ENABLED=0 go build -o flow-evm-gateway -ldflags="-X github.com/onflow/flow-evm-gateway/api.Version=$(IMAGE_TAG)" cmd/main.go111-113: Fix typo in COA_KEY variable.
There's a typo in the COA_KEY variable name (KEYE).
Apply this diff:
- --coa-key=$(EMULATOR_COA_KEYE) \
+ --coa-key=$(EMULATOR_COA_KEY) \Also applies to: 128-130
138-223: LGTM! Well-structured Docker configuration.
The Docker targets are well-implemented with:
- Proper environment variable validation
- Flexible configuration options
- Clear documentation
11-13:
Remove hardcoded sensitive data.
The emulator configuration contains hardcoded API keys and addresses which should be configurable.
Apply this diff to make the values configurable:
-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}🧰 Tools
🪛 Gitleaks (8.21.2)
13-13: 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: 0
🧹 Outside diff range and nitpick comments (2)
Makefile (2)
41-50: Enhance OS/Architecture detectionThe current implementation has limitations:
- No Windows support
- No fallback for unsupported architectures
- Hard error on unsupported OS
Consider this more robust implementation:
ifeq ($(UNAME_S),Darwin) OS := macos ARCH := arm64 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)) + $(warning Unsupported operating system: $(UNAME_S), defaulting to linux/amd64) + OS := linux + ARCH := amd64 endif
193-222: Add validation for Docker environmentThe docker-run target could benefit from additional safety checks:
- Validate IMAGE_TAG is set
- Verify Docker daemon is running
- Check if required ports are available
Add these checks at the beginning of the target:
.PHONY: docker-run docker-run: + @if [ -z "$(IMAGE_TAG)" ]; then \ + echo "ERROR: IMAGE_TAG is not set"; \ + exit 1; \ + fi + @if ! docker info > /dev/null 2>&1; then \ + echo "ERROR: Docker daemon is not running"; \ + exit 1; \ + fi + @if [ -n "$(DOCKER_HOST_PORT)" ] && lsof -i :$(DOCKER_HOST_PORT) > /dev/null 2>&1; then \ + echo "ERROR: Port $(DOCKER_HOST_PORT) is already in use"; \ + exit 1; \ + fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
Makefile(4 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)
131-131: 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: Remove hardcoded sensitive data
The emulator configuration contains hardcoded API keys and addresses which should be configurable.
🧰 Tools
🪛 Gitleaks (8.21.2)
13-13: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
52-54: Make container registry configurable
The container registry is hardcoded to a specific GCP registry.
79-79: CGO can be disabled for better portability
111-113: Fix typo in COA_KEY variable and reduce duplication
Also applies to: 128-130
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Tidy up compiler args and account for Mac specifics Fix docker-run-local with host node docker access
…fixes # Conflicts: # Makefile
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
Makefile (2)
18-18: Consider disabling CGO for better portability.CGO is enabled by default, but if there are no direct C dependencies, disabling it would improve binary portability.
Apply this diff:
-COMPILER_FLAGS := CGO_ENABLED=1 +COMPILER_FLAGS := CGO_ENABLED=0
194-197: Simplify mount handling.The current mount handling could be simplified while maintaining the same functionality.
Apply this diff:
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 (3)
Dockerfile(1 hunks)Makefile(3 hunks)dev/Dockerfile(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Dockerfile
- 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 (6)
Makefile (6)
2-7: Add error handling for Git commands.
The Git commands might fail in CI/CD environments where Git metadata isn't available.
Apply this diff to add 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")60-62: Make container registry configurable.
The container registry is hardcoded to a specific GCP registry.
Apply this diff:
-export CONTAINER_REGISTRY := us-west1-docker.pkg.dev/dl-flow-devex-production/development
+export CONTAINER_REGISTRY ?= us-west1-docker.pkg.dev/dl-flow-devex-production/development87-87: LGTM!
The build command correctly uses IMAGE_TAG for versioning.
111-125: LGTM!
The local development commands are consistent in their use of EMULATOR_ARGS.
11-13:
Remove hardcoded sensitive data.
The emulator configuration contains hardcoded API keys and addresses which should be configurable.
Apply this diff to make the values configurable:
-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}🧰 Tools
🪛 Gitleaks (8.21.2)
13-13: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
44-48: 🛠️ Refactor suggestion
Remove duplicated version handling logic.
The version handling logic is duplicated from lines 30-34.
Remove these duplicated lines:
-# Set VERSION from command line, environment, or default to SHORT_COMMIT
-VERSION ?= ${SHORT_COMMIT}
-
-# Set IMAGE_TAG from VERSION if not explicitly set
-IMAGE_TAG ?= ${VERSION}Likely invalid or redundant comment.
|
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 |
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.
| ifdef GOARCH | ||
| $(eval ARCH=$(GOARCH)) | ||
| endif | ||
| docker build --build-arg VERSION="$(VERSION)" --build-arg ARCH=$(ARCH) -f Dockerfile -t "$(CONTAINER_REGISTRY)/evm-gateway:$(IMAGE_TAG)" \ |
There was a problem hiding this comment.
@vishalchangrani - let me know what should be done for tags - i copied what we do for other Flow nodes, but not sure that is what you need
… 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))
endifCo-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)
| | `coa-address` | `""` | Flow address holding COA account for submitting transactions | | ||
| | `coa-key` | `""` | Private key for the COA address used for transactions | |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add security warnings for sensitive configuration flags.
The coa-key and related flags handle sensitive information and need security warnings.
| `coa-key` | `""` | Private key for the COA address used for transactions |
+| `coa-key-file` | `""` | Path to a JSON file of COA keys for key-rotation (⚠️ Secure access required) |Also, add a security notice section above the configuration table:
> ⚠️ **Security Notice**: Several configuration flags handle sensitive information:
> - `coa-key`: Never expose this in logs or configuration files
> - `coa-key-file`: Ensure proper file permissions and secure storage
> - `wallet-api-key`: Only use in development environments
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.Bug Fixes
Documentation
Chores
.gitignoreto exclude specific directories and files from version control.dev/run.shscript as part of streamlining the setup process.