feat: evm abci handlers #6
Conversation
Signed-off-by: Nikhil Sharma <[email protected]>
…empool for application Signed-off-by: Nikhil Sharma <[email protected]>
…odes Signed-off-by: Nikhil Sharma <[email protected]>
Signed-off-by: Nikhil Sharma <[email protected]>
…ze multi-node setup on docker containers Signed-off-by: Nikhil Sharma <[email protected]>
…anges for multi node setup Signed-off-by: Nikhil Sharma <[email protected]>
Signed-off-by: Nikhil Sharma <[email protected]>
…nges for creating abci handler Signed-off-by: Nikhil Sharma <[email protected]>
Signed-off-by: Nikhil Sharma <[email protected]>
Signed-off-by: Nikhil Sharma <[email protected]>
… logic for eth engine api Signed-off-by: Nikhil Sharma <[email protected]>
…ded PrepareProposal logic Signed-off-by: Nikhil Sharma <[email protected]>
…al and changes making sure client creation before abci calls Signed-off-by: Nikhil Sharma <[email protected]>
Signed-off-by: Nikhil Sharma <[email protected]>
Signed-off-by: Nikhil Sharma <[email protected]>
Signed-off-by: Nikhil Sharma <[email protected]>
Signed-off-by: Nikhil Sharma <[email protected]>
Signed-off-by: Nikhil Sharma <[email protected]>
WalkthroughThe changes introduce enhancements across the repository. Updates include modifications to the Changes
Sequence Diagram(s)sequenceDiagram
participant App as Scalerized App
participant EVM as EVMClient
participant ABCI as EVMABCIHandler
participant Engine as EngineAPI
participant RPC as RPCAPI
App->>EVM: NewEVMClient(ctx, config, logger)
EVM-->>App: Connection Started (async ready signal)
App->>ABCI: NewEVMABCIHandler(ctx, EVM)
ABCI->>EVM: PrepareProposal()
EVM->>Engine: Connect/Request latest block info
Engine-->>EVM: Return block header/number
ABCI->>EVM: ProcessProposal()/NewPayload()
EVM->>RPC: Dial RPC and process payload request
RPC-->>EVM: Return payload/execution result
Poem
🪧 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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (48)
app/params/flags.go (1)
3-11: Well-structured configuration flags for EVM integrationThe defined flags cover the essential configuration parameters needed for connecting to an Ethereum execution client, including connection details, authentication, and chain identification.
Consider adding default values for these parameters directly in this file or in the command implementation to improve developer experience. Also, adding brief comments for each flag would enhance maintainability.
var ( - FlagExecutionClientType = "execution-client-type" - FlagExecutionEngineURL = "engine-api-url" - FlagExecutionEngineJWTSecretPath = "engine-api-jwtsecret" - FlagRPCURL = "eth-rpc-url" - FlagRPCJWTRefreshInterval = "jwt-refresh-interval" - FlagRPCCheckInterval = "eth-rpc-check-interval" - FlagEthChainID = "eth-chain-id" + // FlagExecutionClientType defines the type of execution client to use + FlagExecutionClientType = "execution-client-type" + // FlagExecutionEngineURL defines the URL for the execution engine API + FlagExecutionEngineURL = "engine-api-url" + // FlagExecutionEngineJWTSecretPath defines the path to the JWT secret for engine API authentication + FlagExecutionEngineJWTSecretPath = "engine-api-jwtsecret" + // FlagRPCURL defines the URL for the Ethereum RPC endpoint + FlagRPCURL = "eth-rpc-url" + // FlagRPCJWTRefreshInterval defines how often to refresh JWT tokens + FlagRPCJWTRefreshInterval = "jwt-refresh-interval" + // FlagRPCCheckInterval defines how often to check the Ethereum RPC connection + FlagRPCCheckInterval = "eth-rpc-check-interval" + // FlagEthChainID defines the Ethereum chain ID to connect to + FlagEthChainID = "eth-chain-id" )execution/evm/utils.go (1)
18-32: Robust hexToAddress implementation with proper error handlingThe
hexToAddressfunction is well-implemented with appropriate error handling:
- It correctly handles the "0x" prefix in Ethereum addresses
- It properly validates the address length (20 bytes)
- It uses clear, descriptive error messages
One small improvement would be to validate the hex string format before attempting to decode it.
func hexToAddress(hexStr string) (common.Address, error) { var addr [20]byte + // Remove 0x prefix if present decoded, err := hex.DecodeString(strings.TrimPrefix(hexStr, "0x")) if err != nil { return addr, err } if len(decoded) != 20 { return addr, fmt.Errorf("invalid length: got %d, want 20", len(decoded)) } copy(addr[:], decoded) return addr, nil }execution/evm/config.go (2)
9-16: Default configurations look appropriate for developmentThe default configuration values appear reasonable for a development environment, with appropriate URLs and intervals.
Consider adding a comment to
DefaultEthChainID = "80087"explaining what chain this ID corresponds to, as it's not immediately obvious.
43-51: Consider adding input validationThe
NewEVMConfigfunction parses URLs and retrieves the JWT secret, but it doesn't validate other inputs like the durations.Add validation for time durations to ensure they are positive values:
func NewEVMConfig(ethChainID *big.Int, rpcJWTRefreshInterval, rpcCheckInterval time.Duration, engineAPIURL, rpcURL, jwtSecretPath string) (*EVMConfig, error) { + if rpcJWTRefreshInterval <= 0 { + return nil, fmt.Errorf("rpcJWTRefreshInterval must be positive") + } + if rpcCheckInterval <= 0 { + return nil, fmt.Errorf("rpcCheckInterval must be positive") + } + if ethChainID == nil { + return nil, fmt.Errorf("ethChainID cannot be nil") + } eu, err := url.Parse(engineAPIURL) if err != nil { return nil, err } // Rest of the function remains the sameexecution/evm/constants.go (1)
22-29: Function implementation looks good but lacks documentationThe
ScalerizeSupportedCapabilitiesfunction correctly returns a subset of the defined constants for capability exchange.Add a comment documenting the purpose of this function:
+// ScalerizeSupportedCapabilities returns a list of Engine API methods that this +// implementation supports for capability exchange with execution clients. func ScalerizeSupportedCapabilities() []string { return []string{ ForkchoiceUpdatedMethodV3, GetPayloadMethodV3, NewPayloadMethodV3, GetClientVersionV1, } }Makefile (1)
1-4: Consider using underscores in variable namesThe variable
STARTING-IP-ADDRuses a dash, which is unconventional for Makefile variables and can cause issues in certain contexts.Use underscores instead of dashes in Makefile variable names:
-STARTING-IP-ADDR := 172.20.0.2 +STARTING_IP_ADDR := 172.20.0.2And update references to this variable accordingly.
go.mod (1)
47-52: Beacon Kit dependencies need version consistencyMultiple Beacon Kit modules are included with different versions. This could lead to compatibility issues if the modules have interdependencies. Consider aligning all Beacon Kit module versions.
-github.com/berachain/beacon-kit/mod/chain-spec v0.0.0-20240705193247-d464364483df // indirect -github.com/berachain/beacon-kit/mod/engine-primitives v0.0.0-20240808194557-e72e74f58197 // indirect -github.com/berachain/beacon-kit/mod/errors v0.0.0-20240705193247-d464364483df // indirect -github.com/berachain/beacon-kit/mod/geth-primitives v0.0.0-20240806160829-cde2d1347e7e // indirect -github.com/berachain/beacon-kit/mod/primitives v0.0.0-20240911165923-82f71ec86570 // indirect +github.com/berachain/beacon-kit/mod/chain-spec v0.0.0-20240911165923-82f71ec86570 // indirect +github.com/berachain/beacon-kit/mod/engine-primitives v0.0.0-20240911165923-82f71ec86570 // indirect +github.com/berachain/beacon-kit/mod/errors v0.0.0-20240911165923-82f71ec86570 // indirect +github.com/berachain/beacon-kit/mod/geth-primitives v0.0.0-20240911165923-82f71ec86570 // indirect +github.com/berachain/beacon-kit/mod/primitives v0.0.0-20240911165923-82f71ec86570 // indirectstart-testnet-node.sh (1)
3-9: Add environment variable validationThe script uses environment variables ($ID, $ENGINE_API, $RPC_API) without validating if they exist. This could lead to silent failures or unexpected behavior if these variables are not properly set.
# !/bin/sh +# Validate required environment variables +if [ -z "$ID" ]; then + echo "ERROR: ID environment variable is not set" + exit 1 +fi + +if [ -z "$ENGINE_API" ]; then + echo "ERROR: ENGINE_API environment variable is not set" + exit 1 +fi + +if [ -z "$RPC_API" ]; then + echo "ERROR: RPC_API environment variable is not set" + exit 1 +fi + # Copy the folder based on ID environment variable cp -r ./example-testnet/node$ID/scalerized /root/.scalerized # Start the scalerized node echo "starting scalerize node $ID in background ..." scalerized start --execution-client-type=evm --engine-api-url=$ENGINE_API --eth-rpc-url=$RPC_API tail -f /dev/nullreth.Dockerfile (3)
1-1: Specify a fixed version tag instead of 'latest'Using 'latest' as a tag can lead to unexpected behavior when the upstream image changes. This might cause reproducibility issues with your build process.
-FROM ghcr.io/paradigmxyz/reth:latest +FROM ghcr.io/paradigmxyz/reth:v0.1.0-alpha.8 # Replace with a specific versionConsider pinning to a specific version for better reproducibility and version control.
3-6: Optimize apt-get installation to reduce image sizeThe current installation doesn't clean up the apt cache, which increases the image size.
RUN apt-get update && apt-get install -y \ curl \ jq \ - iputils-ping + iputils-ping \ + && rm -rf /var/lib/apt/lists/*
14-15: Combine chmod commands to reduce layersMultiple RUN statements create additional layers in the Docker image. Combining related commands can reduce the image size.
-RUN chmod +x ./start-testnet-reth-bootnode.sh -RUN chmod +x ./start-testnet-reth-miner-node.sh +RUN chmod +x ./start-testnet-reth-bootnode.sh ./start-testnet-reth-miner-node.shDockerfile (7)
1-1: Specify a fixed version tag for golang:alpineUsing golang:alpine without a specific version can lead to unexpected behavior when the base image changes.
-FROM golang:alpine AS build-env +FROM golang:1.21-alpine AS build-env # Use a specific version that matches your Go module requirements
4-4: Consolidate package installationThe environment variable PACKAGES doesn't include linux-headers, which is installed separately. This makes maintenance more difficult.
-ENV PACKAGES git build-base +ENV PACKAGES git build-base linux-headers
10-11: Consolidate RUN commands to reduce image layersMultiple RUN statements create additional layers in the Docker image. Combining related commands reduces the image size.
-RUN apk add --update $PACKAGES -RUN apk add linux-headers +RUN apk add --update $PACKAGESNote: This works with the previous suggestion to include linux-headers in the PACKAGES variable.
17-19: Add error checking in the build processThe build script does not verify that each command succeeds before proceeding to the next step.
-RUN make build -RUN make localtestnet-example-config -RUN echo "completed testnet config" +RUN make build && \ + make localtestnet-example-config && \ + echo "completed testnet config"This ensures that if one command fails, the Docker build will fail immediately.
22-22: Consider using a more recent Alpine versionAlpine 3.17.3 is somewhat outdated. A more recent version might include security patches and improvements.
-FROM alpine:3.17.3 +FROM alpine:3.19 # Or the latest version available
25-25: Optimize apk installation to reduce image sizeThe current installation doesn't clean up the apk cache, which increases the image size.
-RUN apk add --update ca-certificates jq bash curl +RUN apk add --update ca-certificates jq bash curl && \ + rm -rf /var/cache/apk/*
30-33: Use COPY with multiple source files to reduce image layersMultiple COPY statements create additional layers in the Docker image. Combining related commands reduces the image size.
-COPY --from=build-env /go/src/github.com/aerius-labs/scalerize/build/scalerized /usr/bin/scalerized -COPY --from=build-env /go/src/github.com/aerius-labs/scalerize/example-testnet example-testnet -COPY --from=build-env /go/src/github.com/aerius-labs/scalerize/start-testnet-node.sh start-testnet-node.sh -COPY --from=build-env /go/src/github.com/aerius-labs/scalerize/testing/files/jwt.hex jwt.hex +COPY --from=build-env /go/src/github.com/aerius-labs/scalerize/build/scalerized /usr/bin/scalerized +COPY --from=build-env /go/src/github.com/aerius-labs/scalerize/example-testnet example-testnet \ + /go/src/github.com/aerius-labs/scalerize/start-testnet-node.sh start-testnet-node.sh \ + /go/src/github.com/aerius-labs/scalerize/testing/files/jwt.hex jwt.hexNote: The multi-source COPY format may need adjustment based on Docker version compatibility requirements.
start-testnet-reth-miner-node.sh (2)
39-50: Verify directory existence before other operations.Line 39 creates the data directory, but there's no error handling if this operation fails. Consider adding error checking for critical operations.
-mkdir -p $ETH_DATA_DIR +if ! mkdir -p $ETH_DATA_DIR; then + echo "Error: Failed to create data directory $ETH_DATA_DIR" + exit 1 +fi
1-7: Add environment variable validation.The script relies on environment variables like BOOTNODE_IP and BOOTNODE_RPC_PORT but doesn't validate their existence before use. Add validation to prevent runtime errors.
ETH_DATA_DIR=.scalerized/eth JWT_PATH=testing/files/jwt.hex ETH_GENESIS_PATH=testing/files/eth-genesis.json retries=3 delay=3 enode_url="" +# Validate required environment variables +if [ -z "$BOOTNODE_IP" ] || [ -z "$BOOTNODE_RPC_PORT" ]; then + echo "Error: BOOTNODE_IP and BOOTNODE_RPC_PORT environment variables must be set" + exit 1 +fi🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
start-testnet-reth-bootnode.sh (2)
5-16: Consider better error handling for critical operations.The script uses
&&for command chaining, which will silently stop execution if any command fails. Consider adding explicit error handling for critical operations.-mkdir -p $ETH_DATA_DIR -reth init --datadir $ETH_DATA_DIR --chain $ETH_GENESIS_PATH && \ -reth node \ +if ! mkdir -p $ETH_DATA_DIR; then + echo "Error: Failed to create data directory $ETH_DATA_DIR" + exit 1 +fi + +if ! reth init --datadir $ETH_DATA_DIR --chain $ETH_GENESIS_PATH; then + echo "Error: Failed to initialize Reth node" + exit 1 +fi + +reth node \
1-4: Verify configuration files exist before use.The script uses JWT_PATH and ETH_GENESIS_PATH without checking if these files exist. Add validation to provide clearer error messages if files are missing.
ETH_DATA_DIR=.scalerized/eth JWT_PATH=testing/files/jwt.hex ETH_GENESIS_PATH=testing/files/eth-genesis.json +# Verify critical files exist +if [ ! -f "$JWT_PATH" ]; then + echo "Error: JWT file not found at $JWT_PATH" + exit 1 +fi + +if [ ! -f "$ETH_GENESIS_PATH" ]; then + echo "Error: Genesis file not found at $ETH_GENESIS_PATH" + exit 1 +fi🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
testing/files/eth-genesis.json (2)
2-24: Consider adding documentation for the chosen network parameters.The config section defines important parameters like chainId and fork blocks, but lacks documentation explaining why these specific values were chosen. Consider adding comments to explain the reasoning for test network configuration choices.
342-355: Document the purpose of contract deployments.The file includes three accounts with pre-deployed contract code, but doesn't document their purpose. Consider adding comments explaining the function of each contract to improve maintainability.
execution/evm/rpc.go (2)
24-28: Consider adding error context for failed RPC calls.While the error handling in
GetBlockByNumberis functional, consider wrapping the error with additional context to make debugging easier, especially since this is a critical RPC call.func (ec *EVMClient) GetBlockByNumber(num rpc.BlockNumber, withTxs bool) (*coretypes.Header, error) { result := &coretypes.Header{} err := ec.rpcClient.Client().CallContext(ec.ctx, result, BlockByNumberMethod, num, withTxs) - return result, err + if err != nil { + return nil, fmt.Errorf("failed to get block %d from EVM client: %w", num, err) + } + return result, nil }
3-8: Add missing import for the suggested change.If you implement the suggested error context enhancement, you'll need to add the fmt package to your imports.
import ( + "fmt" "strconv" coretypes "github.com/ethereum/go-ethereum/core/types" "github.com/ethereum/go-ethereum/rpc" )execution/evm/engine.go (3)
9-18: Validate response and naming clarity
Currently, the method name does not convey what "capabilities" are being exchanged or validated, and the returned slice is not checked for emptiness. Consider adding a brief comment clarifying the nature of these capabilities and confirm that an empty returned slice is acceptable.
20-34: Refine fork choice payload status checks
Relying on(result.PayloadStatus == PayloadStatus{})could be error-prone if the type’s zero value is valid under certain conditions. Consider verifying specific fields withinPayloadStatus, or enhance the error message to clarify why the status is considered invalid.
46-55: Verify returned payload status
While this code is valid, consider adding a sanity check or logging aroundresultto confirm it is not unexpectedly defaulting to an empty struct before returning.scripts/execution-client.mk (2)
6-22: Consider specifying container name and logs
When running Docker, naming containers (e.g.,--name reth_node) and optionally redirecting logs could simplify debugging when issues arise or if multiple similar containers run on the same host.
23-33: Handle local node operation
Running the node directly on the host is fine. You may want to run it in the background (e.g., with systemd or tmux) and manage logs for long-running tests.app/app.go (3)
117-121: Context usage
Creating a new background context is effective for now, but consider passing a propagated context from upstream (e.g., server-level) to allow more flexible cancellation or timeouts.
144-192: Execution client switch
The switch-based approach for selecting the client type is clean. Consider adding a safer type assertion or fallback if the flag is unset or an unexpected type. Proper logs upon invalid input can improve debugging.
172-192: Avoid panics in goroutine
Inside the goroutine that starts your EVM client, a panic on startup error could abruptly crash the entire application. A more graceful shutdown approach or returning an error from the main flow could be preferable.Apply this diff to handle the error more gracefully:
go func() { if err := evmClient.Start(ctx, ensureClientCreatedCh); err != nil { - panic(err) + app.Logger().Error("failed to start EVM client", "error", err) + ensureClientCreatedCh <- false return } }()execution/evm/jwt.go (2)
22-36: Consider handling repeated refresh failures more robustly.Currently, the
refreshJWTForRPCClientmethod logs an error but continues attempting to refresh indefinitely. If the remote service remains unreachable, the loop will spam logs. You might consider imposing a maximum number of retries or an exponential backoff to handle sustained connectivity failures more gracefully.
74-83: Add token expiration or other claims to enhance security.Currently, the JWT only includes an "issued-at" (
iat) claim. Consider adding an "expiration" (exp) claim to enforce a limited token lifetime and reduce the window of opportunity if tokens are compromised. Additional claims likesub(subject) oraud(audience) could improve clarity and security, depending on API needs.docker-testnet-compose.yml (3)
1-90: Enhance production-readiness with health checks and auto-restart policies.Although this Compose file is likely for local testnet development, consider adding Docker health checks and restart policies to automatically restart failed containers. Health checks can help detect containers stuck in an unresponsive state and improve overall reliability.
91-91: Fix trailing spaces in the file.Line 91 has trailing whitespace. This violates YAML lint rules. Remove the trailing spaces to comply with lint requirements.
- image: "reth/node" + image: "reth/node"🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 91-91: trailing spaces
(trailing-spaces)
164-164: Add a newline at the end of the file.Static analysis indicates there is no newline character at the end of the file. Adding a final newline ensures compliance with YAML best practices and lint rules.
- - subnet: 172.20.0.0/16 \ No newline at end of file + - subnet: 172.20.0.0/16🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 164-164: no new line character at the end of file
(new-line-at-end-of-file)
execution/evm/client.go (2)
41-50: Avoid infinite polling in the background without a fallback.The
Startmethod schedules repeated checks for engine and RPC availability. If the services are down for an extended period, the logs may become noisy. Consider a finite retry mechanism or backoff strategy to reduce noise and handle permanent failures more gracefully.
123-151: Consider unifying dial logic and logging for consistent error handling.The
dialRPCCLientfunction branches on the client type but duplicates some setup logic. You might factor out common steps (like logging or context handling) into shared utility code. This keeps the function concise and reduces repetitive error handling.execution/evm/abci_handler.go (2)
30-30: Implement retries for reliability.A TODO note indicates the need for retry logic on engine API calls. Would you like assistance implementing a simple retry or circuit-breaker approach, or shall I open a follow-up issue tracking this enhancement?
37-46: Use structured logging instead of fmt.Printf.Relying on fmt.Printf for production logs can make it harder to collect and parse logs. Consider switching to a structured logger (e.g., Cosmos SDK’s logger) to maintain consistency and improve observability.
Also applies to: 76-76, 83-84
execution/evm/primitives.go (1)
60-105: Consider consolidating manual required-field checks with gencodec.Your manual required-field checks are robust. However, you also use
gencodec:"required"in struct tags, which can similarly enforce those constraints. If desired, you could unify or streamline checks by relying on one approach for clarity and maintainability.Also applies to: 141-186, 188-283
client/testnet.go (4)
127-129: Use consistent binary name in example usage.
This code snippet references "evmosd" from what appears to be a leftover fork. Consider updating the example to the correct binary name (e.g., "scalerized") to avoid confusion.
176-176: Update example command reference.
Likewise, the "evmosd" reference here should be replaced with the correct binary name to maintain consistency.
260-263: Consider safer error handling approach.
Currently, if an error occurs, the entire output directory is removed. This could be destructive if the directory contains other data. Consider a selective cleanup strategy or verifying that the directory is exclusively used for testnet initialization.
430-447: Clean up or uncomment code for better clarity.
Large blocks of commented-out code related to gov, mint, or crisis modules can introduce confusion or code bloat. Consider removing them if not needed or uncommenting them if they are relevant.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (30)
.gitignore(1 hunks)Dockerfile(1 hunks)Makefile(2 hunks)abci/proposal.go(1 hunks)app/app.go(5 hunks)app/params/config.go(1 hunks)app/params/flags.go(1 hunks)client/testnet.go(1 hunks)cmd/scalerized/cmd/commands.go(3 hunks)docker-testnet-compose.yml(1 hunks)execution/client.go(1 hunks)execution/evm/abci_handler.go(1 hunks)execution/evm/client.go(1 hunks)execution/evm/config.go(1 hunks)execution/evm/constants.go(1 hunks)execution/evm/engine.go(1 hunks)execution/evm/jwt.go(1 hunks)execution/evm/primitives.go(1 hunks)execution/evm/rpc.go(1 hunks)execution/evm/utils.go(1 hunks)go.mod(3 hunks)reth.Dockerfile(1 hunks)reth.env(1 hunks)scripts/execution-client.mk(1 hunks)scripts/init.sh(1 hunks)start-testnet-node.sh(1 hunks)start-testnet-reth-bootnode.sh(1 hunks)start-testnet-reth-miner-node.sh(1 hunks)testing/files/eth-genesis.json(1 hunks)testing/files/jwt.hex(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
start-testnet-node.sh
[error] 1-1: Remove spaces between # and ! in the shebang.
(SC1115)
start-testnet-reth-bootnode.sh
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
start-testnet-reth-miner-node.sh
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
🪛 YAMLlint (1.35.1)
docker-testnet-compose.yml
[error] 91-91: trailing spaces
(trailing-spaces)
[error] 164-164: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (29)
.gitignore (1)
2-5: Appropriate gitignore entries for the new EVM functionalityThe added entries are well-chosen for excluding build artifacts, test networks, and logs that shouldn't be tracked in version control. These additions align properly with the PR objectives of developing a Cosmos application with EVM ABCI handlers.
scripts/init.sh (1)
27-27: Confirm mempool.type "nop" is appropriate for this applicationSetting the mempool type to "nop" and skipping validation means the mempool won't process transactions, which could be intentional for an EVM-based application where transaction handling might be delegated to the EVM execution client.
Consider documenting why this setting is necessary, perhaps with a comment explaining that transaction processing is handled by the EVM execution client instead of the standard Cosmos mempool.
execution/evm/utils.go (1)
12-16: Consider making generateRandao an unexported functionThe
generateRandaofunction appears to be correctly implemented but may need some refinement:
- Since this function uses
crypto/rand, it's important for cryptographic operations - consider documenting its purpose- As this function isn't exported (capitalized), verify it's only used internally within the package
The implementation itself is sound - it properly fills a hash with cryptographically secure random data.
abci/proposal.go (1)
7-16: Interface design looks solid, but commented methods need clarificationThe
ABCIHandlerinterface appropriately defines the essential handlers for proposal management in ABCI. The commented methods suggest planned future functionality.Are the commented methods (lines 12-15) planned for future implementation? If so, consider adding a TODO comment explaining when they'll be implemented.
execution/evm/config.go (1)
27-42:✅ Verification successful
Missing JWTSecret implementation
The
NewEVMConfigfunction callsgetJWTFromPath, but this function is not defined in the provided code.Implement the
getJWTFromPathfunction or ensure it's properly imported from another package. The code won't compile without this function.
🏁 Script executed:
#!/bin/bash # Check if getJWTFromPath is defined elsewhere in the codebase rg -t go "func getJWTFromPath" --no-ignoreLength of output: 119
JWTSecret implementation confirmed — No changes needed
The function
getJWTFromPathis implemented inexecution/evm/jwt.go(see the output from thergcommand). There is no missing implementation issue.execution/evm/constants.go (1)
3-20: Well-documented constantsThe constants are well-named and properly documented with comments explaining their purpose. They follow standard Ethereum JSON-RPC and Engine API naming conventions.
Makefile (2)
41-49: Build targets look well-structuredThe build targets are well-organized with proper dependencies and output directory configuration.
6-6:✅ Verification successful
Verify inclusion of external script
The Makefile includes an external script that isn't shown in the provided code.
Ensure that
scripts/execution-client.mkexists and is properly set up. The build will fail if this file is missing.
🏁 Script executed:
#!/bin/bash # Check if the execution-client.mk file exists if [ -f "scripts/execution-client.mk" ]; then echo "File exists" head -n 10 scripts/execution-client.mk else echo "File does not exist" fiLength of output: 397
External Script Inclusion Verified
Thescripts/execution-client.mkfile exists in the repository and includes the expected configuration (e.g., directory definitions and target for starting an ephemeralrethnode). No modifications are necessary.go.mod (3)
25-31: New Ethereum dependencies align with project objectivesThe addition of these key dependencies aligns with the PR objective of developing custom ABCI handlers to interact with an Ethereum node. The Ethereum packages and Beacon Kit integration will enable the implementation of the EVM execution client.
3-5:❓ Verification inconclusive
Go version upgrade requires attention
The Go version has been updated from 1.21 to 1.23.0, which represents a significant upgrade. This change should be accompanied by verification that all dependencies are compatible with Go 1.23, especially considering that Go 1.23 was released very recently.
🏁 Script executed:
#!/bin/bash # Check Go version compatibility for packages go version go env GOFLAGS go mod tidy -vLength of output: 14959
Verification: Go 1.23 Upgrade Verified Successfully
- The
go.modfile now declaresgo 1.23.0with a toolchain ofgo1.23.2and the system reports a runtime version of go1.23.3, which is consistent with expected minor patch differences.- The execution of
go mod tidy -vsuccessfully downloaded all dependencies without errors, indicating that dependency compatibility with Go 1.23 is intact.- No additional changes are required; please proceed knowing that the upgraded Go version has been verified across dependencies.
[resolve_review_comment]
31-31:✅ Verification successful
Verify JWT package version security
The JWT package is being used for authentication between services. Ensure this is the latest secure version.
🌐 Web query:
Is GitHub.com/golang-jwt/jwt/v5 v5.2.1 the latest version? Are there any known security vulnerabilities?💡 Result:
The latest version of
github.com/golang-jwt/jwt/v5is v5.2.1, as confirmed by multiple sources including its GitHub release history and Ubuntu package updates[1][7][10]. This version includes dependency upgrades and maintenance fixes but no breaking changes[7].Security Status
There are no known security vulnerabilities specifically reported for v5.2.1. However, earlier versions of the library had critical issues, such as:
- CVE-2024-5174: Affected v4.x versions (fixed in v4.5.1) due to improper error handling in
ParseWithClaims, which could allow invalid tokens to be accepted if errors weren't checked properly[5][6]. This vulnerability did not impact v5.x[5].- Earlier vulnerabilities (e.g., CVE-2020-26160 in older forks) were resolved when migrating to the
golang-jwt/jwtrepository[9][16].The project actively addresses vulnerabilities, as seen in recent updates to dependencies like
golang.org/x/netandgolang-jwt/jwt/v4in related ecosystems[6][9]. The v5 branch benefits from improved validation logic and stricter error handling by design[2][5][13].For safety, ensure:
- You use
jwt/v5imports (github.com/golang-jwt/jwt/v5)[2][13].- Avoid deprecated forks like
dgrijalva/jwt-go, which had unpatched vulnerabilities[16].- Follow the library's validation guidelines, especially for token algorithms and error checking[11][14].
Citations:
- 1: https://github.com/labstack/echo-jwt/releases
- 2: https://github.com/golang-jwt/jwt/releases
- 3: https://security.snyk.io/package/golang/github.com%2Fgolang-jwt%2Fjwt
- 4: https://github.com/golang-jwt/jwt
- 5: GHSA-29wx-vh33-7x7r
- 6: https://cert-manager.io/docs/releases/release-notes/release-notes-1.12/
- 7: https://launchpad.net/ubuntu/+source/golang-github-golang-jwt-jwt-v5/+changelog
- 8: https://pkg.go.dev/github.com/golang-jwt/jwt
- 9: https://www.getambassador.io/docs/edge-stack/latest/release-notes
- 10: https://lists.gnu.org/archive/html/guix-commits/2025-02/msg01006.html
- 11: decode jwt with rs256 algorithm in golang-jwt / jwt golang-jwt/jwt#292
- 12: https://www.nuget.org/packages/jose-jwt/
- 13: https://pkg.go.dev/github.com/golang-jwt/jwt/v5
- 14: How to validate custom claims with _arbitrary_ additional claims? golang-jwt/jwt#380
- 15: Is this project still maintained? dgrijalva/jwt-go#457
- 16: https://golang-jwt.github.io/jwt/
- 17: https://github.com/golang-jwt/jwt/security
- 18: Vulnerable dependency in jwt-go v3.2.0 (CVE-2020-26160) helm/helm#10320
- 19: Add go module support golang-jwt/jwt#5
- 20: https://docs.konghq.com/gateway/changelog/
JWT Package Version Verified – The currently used version (
github.com/golang-jwt/jwt/v5 v5.2.1) is confirmed to be the latest release with no known security vulnerabilities. Please continue using this version and ensure you follow the library’s usage guidelines for secure token handling.execution/client.go (1)
3-5: Well-designed minimal interfaceThe ExecutionClient interface is well-designed with a single method for now, following the interface segregation principle. This allows for different execution client implementations while maintaining a clear contract.
As the project evolves, consider adding documentation to explain what an ExecutionClient is supposed to do beyond providing a name.
reth.env (2)
1-4: Environment configuration for execution clientThe environment configuration correctly defines the necessary paths for the execution client. This aligns with the PR objective of adding configuration options for the Scalerize Cosmos application.
Consider adding a brief comment at the top of the file explaining its purpose for better maintainability.
3-3:✅ Verification successful
JWT file path consistency
The JWT path references the jwt.hex file reviewed earlier. Ensure this file exists in the specified path during deployment, or provide a mechanism to generate it as needed.
🏁 Script executed:
#!/bin/bash # Check if JWT file exists and has correct format JWT_FILE="testing/files/jwt.hex" if [ -f "$JWT_FILE" ]; then echo "JWT file exists" # Check if it's a valid hex string of correct length (64 characters for 32 bytes) if [[ $(cat "$JWT_FILE" | wc -c) -eq 64 ]] && [[ $(cat "$JWT_FILE" | grep -E "^[0-9a-fA-F]+$") ]]; then echo "JWT file contains valid hex string" else echo "JWT file does not contain a valid 32-byte hex string" fi else echo "JWT file not found at $JWT_FILE" fi # Check ETH_GENESIS_PATH if [ -f "testing/files/eth-genesis.json" ]; then echo "Genesis file exists" else echo "Genesis file not found" fiLength of output: 469
JWT and Genesis Files Verified
The verification confirms that:
- The JWT file at
testing/files/jwt.hexexists and contains a valid 32-byte hex string.- The Ethereum genesis file at
testing/files/eth-genesis.jsonis present.No further action is necessary based on the current deployment setup.
start-testnet-reth-miner-node.sh (2)
32-35: Good error handling implementation.The script properly checks the return value of the get_enode function and provides a clear error message before exiting when the enode URL cannot be retrieved.
8-30: Well-structured function for retrieving enode information.The get_enode function includes retry logic and proper error handling, with informative logging about the process. The regex-based IP replacement in line 20 is a good approach to ensure the enode URL has the correct bootnode IP.
testing/files/eth-genesis.json (1)
1-358: Well-structured Ethereum genesis configuration.The genesis file is properly formatted with appropriate values for an Ethereum testnet. It includes:
- All hard forks activated at block 0
- A configured chainId (80087)
- Pre-funded accounts for testing
- Special contract deployments for blockchain functionality
This is a good configuration for a testing environment.
execution/evm/rpc.go (2)
10-22: Well-implemented block number retrieval with proper error handling.The
GetLatestBlockNumbermethod correctly:
- Makes the RPC call with proper context
- Handles potential errors during the call
- Parses the string result to an integer with error checking
- Returns the correctly typed block number
1-29:✅ Verification successful
Request verification of undefined constants.
The code references constants
BlockNumberMethodandBlockByNumberMethodwhich aren't defined in this file. Ensure these are properly defined elsewhere in the package.
🏁 Script executed:
#!/bin/bash # Verify the constants are defined somewhere in the codebase echo "Searching for BlockNumberMethod and BlockByNumberMethod definitions..." # First, find all Go files in the execution/evm directory files=$(fd --extension go . execution/evm) # Then grep for the constants in those files grep -n "BlockNumberMethod\|BlockByNumberMethod" $files || echo "Constants not found in any Go files in execution/evm directory"Length of output: 1253
CONSTANTS ARE DEFINED CORRECTLY
After verifying the codebase, bothBlockNumberMethodandBlockByNumberMethodare defined inexecution/evm/constants.go(lines 12–15). The earlier concern regarding undefined constants is resolved.execution/evm/engine.go (1)
36-44: Looks good
The retrieval of the execution payload envelope and direct return of the RPC error appear correct. No major issues here.scripts/execution-client.mk (1)
1-5: No major issues
Variable definitions look straightforward and appropriate.app/app.go (3)
4-31: Imports and mempool configuration
Introducing the no-op mempool is valid for a custom execution flow. Ensure that external transaction handling is not inadvertently disabled if the application eventually needs normal Cosmos transactions.
42-42: Good addition of new params import
This importer is crucial for pulling in the flag definitions. No issues found.
194-205: ABCI handlers assignment
The configuration of proposal and block handlers looks correct. Revisit enabling the commented-out handlers (e.g., BeginBlocker) if they become relevant.execution/evm/jwt.go (1)
38-60: Clarify behavior for partial or zero-length JWT secrets.The code allows a secret to be shorter than 32 bytes, silently copying fewer bytes into the
JWTSecretstruct, which might be intended. However, consider validating the exact length or documenting why partial secrets are acceptable if that’s intended, to avoid confusion or future security issues.execution/evm/abci_handler.go (1)
71-81: Check for nil PayloadID to avoid panics.If the engine API could return a ForkchoiceResponse with a nil PayloadID, calling
GetPayload(*fcres.PayloadID)at line 78 will panic. Please confirm that the engine always provides a non-nil ID in successful scenarios. Otherwise, a nil check or fallback handling is advisable.cmd/scalerized/cmd/commands.go (1)
46-60: LGTM for new persistent flags and testnet command.client/testnet.go (2)
1-39: Implementation looks good.
No major concerns in the initial package definition and import statements.
551-596: No major issues.
ThestartTestnetfunction correctly configures and launches the in-process testnet, then waits for user input before cleanup. The approach appears consistent with typical Cosmos SDK patterns.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
execution/evm/jwt.go (1)
18-36:⚠️ Potential issueBe mindful of concurrent pointer updates in
refreshJWTForRPCClient.This function periodically calls
dialRPCCLientto refresh tokens. Updating shared pointers (e.g.,engineClient) from multiple goroutines without synchronization may introduce data races. This issue aligns with a previously flagged concurrency concern. Consider guarding shared pointers with a mutex or ensuring their updates occur only on a single goroutine.
🧹 Nitpick comments (11)
execution/evm/jwt.go (2)
38-60: Minor naming consideration.Inside
getJWTFromPath, the local variablebytescan overshadow thebytespackage name if imported. While not critical, consider using a more descriptive name (e.g.,decodedBytes) to avoid confusion.
74-83: Consider adding an expiration claim to the JWT.Currently, the token never expires. Adding an
expclaim improves security by limiting token validity.execution/evm/client.go (2)
47-50: Reconsider starting the refresh loop unconditionally.The JWT refresh loop is started in a deferred block before confirming successful connections. If the client fails to connect, this goroutine still runs, possibly creating unnecessary retries. Consider starting it only after establishing a valid connection.
55-66: Reduce duplicated logic in the two goroutines.The retry logic for engine and RPC connections is almost identical, differing only in URLs and log messages. Consider extracting the shared logic into a helper function to adhere to DRY principles.
Also applies to: 69-80
client/testnet.go (5)
127-129: Rename leftover references to "evmosd" for consistency.The documentation references "evmosd" even though this repository is named "scalerize." Consider updating these references for clarity and consistency:
- evmosd testnet init-files --v 4 --output-dir ./.testnets --starting-ip-address 192.168.10.2 + scalerized testnet init-files --v 4 --output-dir ./.testnets --starting-ip-address 192.168.10.2
201-202: Clarify or remove the commented-out JSON-RPC address flag.Lines 201-202 comment out the JSON-RPC address configuration, which can be confusing. If this flag is no longer needed, consider removing it. Otherwise, enable it with a default value or a comment explaining its purpose.
208-392: Break down the largeinitTestnetFilesfunction.Currently,
initTestnetFilesspans many lines and handles multiple responsibilities (e.g., creating node directories, generating keys, collecting genesis files). Consider splitting it into smaller, dedicated helper functions to improve readability, maintainability, and testability.
260-264: Avoid removing the entire output directory on errors.This block removes the full testnet output directory upon encountering an error, which can complicate debugging or partial reuse. Consider a more granular cleanup strategy or preserve logs and configuration files for diagnostic purposes.
315-318: Mask or clarify plaintext storage of seed words.Seed words are stored in plaintext JSON, which is acceptable for purely local, ephemeral testnets. However, it may pose a security risk if deployed more widely. Consider emphasizing that this is for development purposes only or adding a configurable encryption mechanism.
start-testnet-reth-miner-node.sh (2)
7-7: Environment Variable Check: Validate BOOTNODE_IP and BOOTNODE_RPC_PORTThe script uses
BOOTNODE_IPandBOOTNODE_RPC_PORTwithout verifying they’re set. For enhanced robustness, consider adding a check at the top of the script to ensure these required environment variables are defined.+if [ -z "$BOOTNODE_IP" ] || [ -z "$BOOTNODE_RPC_PORT" ]; then + echo "Error: BOOTNODE_IP and BOOTNODE_RPC_PORT environment variables must be set." + exit 1 +fi
51-52: Trailing Backslash: Remove Unnecessary Line ContinuationThe last argument (
--trusted-peers $enode_url) is followed by a trailing backslash. This may lead to a syntax error if there is no subsequent argument. Removing the backslash will properly terminate the multi-line command.- --trusted-peers $enode_url \ + --trusted-peers $enode_url
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (7)
client/testnet.go(1 hunks)execution/evm/client.go(1 hunks)execution/evm/config.go(1 hunks)execution/evm/jwt.go(1 hunks)start-testnet-node.sh(1 hunks)start-testnet-reth-bootnode.sh(1 hunks)start-testnet-reth-miner-node.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- start-testnet-node.sh
- start-testnet-reth-bootnode.sh
- execution/evm/config.go
🧰 Additional context used
🧬 Code Definitions (2)
start-testnet-reth-miner-node.sh (2)
app/params/config.go (1) (1)
init(34:37)app/app.go (1) (1)
init(84:91)
execution/evm/client.go (4)
execution/evm/config.go (7) (7)
EVMConfig(18:25)c(53:55)c(57:59)c(61:63)c(65:67)c(69:71)c(73:75)execution/evm/jwt.go (2) (2)
c(18:36)c(62:72)execution/evm/engine.go (4) (4)
c(9:18)c(20:34)c(36:44)c(46:55)execution/evm/constants.go (1) (1)
ExchangeCapabilities(17:17)
🔇 Additional comments (2)
execution/evm/jwt.go (1)
62-72: Logic appears consistent for building JWT header.No issues found with header creation and error handling.
client/testnet.go (1)
531-535: Overflow handling for last octet appears correct.The code checks for rollover when incrementing the final IP octet and returns an error on overflow. This resolves a known issue about exceeding 255. Great job!
execution/evm/client.go
Outdated
| func (c *EVMClient) Start(ctx context.Context, ensureClientCreatedCh chan bool) error { | ||
| var ( | ||
| wg sync.WaitGroup | ||
| ticker = time.NewTicker(c.config.RPCCheckInterval()) | ||
| ) | ||
|
|
||
| defer func() { | ||
| ticker.Stop() | ||
| go c.refreshJWTForRPCClient(ctx, engineClient) | ||
| }() | ||
|
|
||
| c.logger.Info("Connecting to the execution client") | ||
| wg.Add(2) | ||
|
|
||
| c.logger.Info("Initializing connection with Ethereum Engine API: " + c.config.EngineAPIURL().String()) | ||
| go func() { | ||
| defer wg.Done() | ||
| for range ticker.C { | ||
| c.logger.Info("Waiting for ethereum engine api to be available: " + c.config.EngineAPIURL().String()) | ||
| if err := c.connect(engineClient); err != nil { | ||
| c.logger.Error("failed to create connection to ethereum engine api") | ||
| continue | ||
| } | ||
|
|
||
| break | ||
| } | ||
| }() | ||
|
|
||
| c.logger.Info("Initializing connection with Ethereum RPC API: " + c.config.EngineAPIURL().String()) | ||
| go func() { | ||
| defer wg.Done() | ||
| for range ticker.C { | ||
| c.logger.Info("Waiting for ethereum rpc api to be available: " + c.config.RPCURL().String()) | ||
| if err := c.connect(rpcClient); err != nil { | ||
| c.logger.Error("failed to create connection to ethereum rpc api") | ||
| continue | ||
| } | ||
|
|
||
| break | ||
| } | ||
| }() | ||
|
|
||
| wg.Wait() | ||
| ensureClientCreatedCh <- true | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Synchronize pointer assignments to prevent data races.
Multiple goroutines in Start (lines 54+69) and refreshJWTForRPCClient (line 49 in the deferred call) may invoke dialRPCCLient concurrently, updating the same pointer fields (engineClient or rpcClient). Ensure concurrency safety (e.g., using a mutex) to prevent possible data races.
Also applies to: 123-151
There was a problem hiding this comment.
No changes are made in engineClient and rpcClient in Start method. And in refreshJWTForRPCClient only engineClient header is changed with updated jwt token.
There was a problem hiding this comment.
You are on the CodeRabbit Free Plan. In order to use the Chat feature, please upgrade to CodeRabbit Pro.
| cmd.Println("press the Enter Key to terminate") | ||
| _, err = fmt.Scanln() // wait for Enter Key | ||
| if err != nil { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Provide a non-blocking alternative in startTestnet.
The fmt.Scanln() blocks until user input is received, which can be problematic in automated scripts or CI pipelines. Consider adding a flag to skip interactive prompts or a timeout-based termination strategy.
NikhilSharmaWe
force-pushed
the
feat/evm-abci-handlers
branch
from
37dc293 to
7189625
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
start-testnet-node.sh (1)
3-4: Environment Variable Safety for Node IDThe script uses the environment variable
$IDin the command:cp -r ./example-testnet/node$ID/scalerized /root/.scalerizedEnsure that
$IDis always set or consider adding a check (and possibly a default value) to prevent unexpected failures if it’s missing.execution/evm/jwt.go (3)
18-36: Consider adding a retry backoff or bounding attempts in the refresh loop.Right now, this loop dials the RPC client indefinitely at fixed intervals. In scenarios involving transient failures or unreachable endpoints, logs may be spammed and resources wasted with no backoff or limit. Introducing a backoff strategy or capping the attempts can help stabilize retries.
38-60: Consider validating minimum secret length.Currently, an empty hex string is treated as a valid secret, which might not provide meaningful authentication for generating JWTs. If a non-zero length secret is expected, consider returning an error if the file is empty or insufficiently long.
74-83: Add an expiration claim to enhance security.Currently, the JWT includes only an issued-at time and lacks an expiration. Without an
exp, the token will remain valid indefinitely if the secret leaks. Consider adding an expiry window (e.g., a few minutes) to mitigate potential token misuse.client/testnet.go (3)
128-129: Rename references from "evmosd" to "scalerized" (or the correct binary) for consistency.
The usage example references "evmosd," which might be confusing to new users if the actual binary name isscalerized.- evmosd testnet init-files --v 4 --output-dir ./.testnets --starting-ip-address 192.168.10.2 + scalerized testnet init-files --v 4 --output-dir ./.testnets --starting-ip-address 192.168.10.2
242-251: Trim whitespace for IP addresses read from the flag.
If a user separates IP addresses with spaces (e.g.,"192.168.0.1, 192.168.0.2"), the extra whitespace could cause parsing errors. Consider trimming each split token.ipList := strings.Split(args.ipAddresses, ",") for i := range ipList { - ipList[i] = ipList[i] + ipList[i] = strings.TrimSpace(ipList[i]) }
315-318: Enhance security of private key seed storage.
Seeds are written in plain JSON with 0o600 permissions. This is acceptable for local development but consider encryption or password protection for stronger security in multi-user environments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (7)
client/testnet.go(1 hunks)execution/evm/client.go(1 hunks)execution/evm/config.go(1 hunks)execution/evm/jwt.go(1 hunks)start-testnet-node.sh(1 hunks)start-testnet-reth-bootnode.sh(1 hunks)start-testnet-reth-miner-node.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- start-testnet-reth-bootnode.sh
- start-testnet-reth-miner-node.sh
- execution/evm/config.go
🔇 Additional comments (14)
start-testnet-node.sh (3)
1-1: Shebang Line CorrectnessThe shebang line is correctly formatted as
#!/bin/shand aligns with best practices.
9-9: Container Process PersistenceUsing
tail -f /dev/nullto keep the container running is a standard and acceptable approach. Double-check that this fits with your container management strategy.
6-8:❓ Verification inconclusive
Clarify Background Process Execution
The echo message states that the scalerize node will start in the background:
echo "starting scalerize node $ID in background ..."However, the subsequent command:
scalerized start --execution-client-type=evm --engine-api-url=$ENGINE_API --eth-rpc-url=$RPC_APIdoes not include an ampersand (
&), which typically denotes background execution. Verify whetherscalerized startinternally daemonizes its process. If it does not, consider appending&to ensure it runs in the background, or update the echo message accordingly.
Below is a shell script to help verify whether the
scalerized startcommand runs in the background (by checking for an explicit ampersand) or if it internally daemonizes itself. You can run this script from the repository root to inspect the lines of interest in thestart-testnet-node.shfile:
Action Required: Confirm Background Process Execution
The echo message indicates that the node is being started in the background, yet the command does not use an explicit ampersand (
&). Please determine one of the following:
- If
scalerized startinternally handles daemonization: update the echo message to remove any misleading “in background” phrasing.- If not: modify the command to append an ampersand so that it truly runs in the background.
Review and update the code accordingly.
🏁 Script executed:
#!/bin/bash file="start-testnet-node.sh" echo "Evaluating execution mode in $file (lines 6-8):" sed -n '6,8p' "$file" echo -e "\nChecking for explicit background operator '&' in the scalerized start command..." if grep "scalerized start" "$file" | grep -q "&"; then echo "Found '&' in the command: the process is explicitly backgrounded." else echo "No '&' found. Please verify if 'scalerized start' internally daemonizes its process." fiLength of output: 938
Action Required: Confirm Scaling Process Background Execution
The echo message suggests the scalerized node will run in the background, but the code does not append an ampersand (
&). The verification shows no explicit background operator is present. Please confirm one of the following:
- If
scalerized startinternally daemonizes: Update the echo message to eliminate the misleading mention of background execution.- If it does not daemonize: Append an ampersand (
&) to the command so that the process truly runs in the background.Review and update accordingly.
execution/evm/jwt.go (1)
62-72: Implementation looks good.Building the "Authorization" header with the signed token is a standard approach, and error handling is straightforward.
execution/evm/client.go (6)
1-20: No issues found in package definitions and import statements.
29-35: No issues found in the constructor.
37-39: Getter method looks fine.
41-86: Concurrent assignments remain unsynchronized.The
Startmethod spawns goroutines that eventually calldialRPCCLient, updating shared pointers (e.g.,c.engineClient) without locks or other synchronization. This can trigger data races, as previously noted.
88-121: Concurrent pointer writes can cause data inconsistencies.The
connectmethod callsdialRPCCLient, which overwritesc.engineClientorc.rpcClient. When multiple goroutines invokeconnectconcurrently, it can lead to data races.
123-151: Synchronize client pointer assignments and verify resource cleanup.
- Unprotected writes to
c.engineClient/c.rpcClientrisk data races.- Overwriting these clients might leak resources unless the old client is gracefully closed.
client/testnet.go (4)
201-202: Consider re-adding a JSON-RPC address flag if needed.
The JSON-RPC flag is commented out. If you plan to expose or configure a JSON-RPC server, uncomment and finalize the corresponding logic to ensure consistent behavior with other flags.Do we have references elsewhere in the code that might rely on this flag?
261-280: Revisit removing the output directory on initialization errors.
Callingos.RemoveAll(args.outputDir)may remove valuable partial data if multiple processes or other tasks share that directory. Consider removing only the new node subdirectory instead of the entire testnet output, or prompt the user before cleanup.
530-535: IP address overflow handling looks good!
Your check correctly fails fast when exceeding the maximum octet value. This ensures robust IP calculations for multi-validator setups.
591-593: Offer a non-blocking alternative to user input.
This code blocks until the user presses Enter, which can cause issues in CI or scripted deployments. Consider adding a flag to skip interactive waiting, or a timeout approach.
Signed-off-by: Nikhil Sharma <[email protected]>
NikhilSharmaWe
force-pushed
the
feat/evm-abci-handlers
branch
from
7189625 to
119b6fc
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (12)
execution/evm/client.go (1)
111-111: Minor grammatical improvement in error message."chainID do not match" should be "chain IDs do not match" or "chain ID does not match."
Apply this diff:
-c.logger.Error("eth chain ID specified not equal to the actual chain ID") -return fmt.Errorf("chainID do not match for the eth client with what specified in scalerize config") +c.logger.Error("eth chain ID specified is not equal to the actual chain ID") +return fmt.Errorf("chain IDs do not match the ID specified in the Scalerize config")execution/evm/jwt.go (1)
18-36: Clarify naming & log output for the JWT refresh loop.
- The method name
refreshJWTForRPCClientis misleading if we are passing"engineClient"as theclientType.- Similarly, messages like "Failed to refresh engine auth token" (line 31) and "Refreshed JWT Token for ethereum engine API" (line 33) do not distinguish between engine vs. RPC usage when
clientTypeis"rpcClient".Consider a more generic name such as
refreshJWTForEVMClient, plus conditional log messaging based onclientType:-func (c *EVMClient) refreshJWTForRPCClient(ctx context.Context, clientType string) { - c.logger.Info("Starting JWT refresh loop 🔄") +func (c *EVMClient) refreshJWTForEVMClient(ctx context.Context, clientType string) { + c.logger.Infof("Starting JWT refresh loop for %s 🔄", clientType) ... switch clientType { case engineClient: c.logger.Info("Refreshed JWT Token for ethereum engine API") case rpcClient: c.logger.Info("Refreshed JWT Token for ethereum RPC API") default: ... } }Makefile (2)
41-44: Separate build targets for multiple environments.Providing a
build-linuxtarget is a good practice for cross-platform compatibility. Consider supporting additional architectures if needed (e.g., ARM).
48-50: Directory creation for build outputs.Using
mkdir -pis straightforward. Alternatively, you might considerinstall -dfor explicit directory creation semantics, but this is fine.client/testnet.go (5)
59-70:initArgsstruct for testnet initialization.Storing CLI arguments in a dedicated struct improves clarity. Consider adding doc comments on each field, especially to clarify IP address usage vs. the
ipAddresseslist.
72-84:startArgsstruct for in-process testnet.Similar to
initArgs, this approach is clean. Evaluate whether the two structs can be refactored into a shared type if large portions overlap.
166-204:testnetStartCmd: start multi-validator in-process testnet.
- Allows immediate local testing without Docker or separate processes. Good for quick iteration.
- The blocking prompt at the end might hinder CI usage. If you want a non-blocking approach, consider adding a
--no-waitor--headlessflag to skip the blocking prompt.Do you want me to propose a change to conditionally skip the blocking prompt for automated workflows?
513-538:getIPandcalculateIPIP increment logic.
- Properly checks for IPv4 overflow.
- The existence of fallback to
sdkserver.ExternalIP()is helpful when no IP is provided.Consider adding error logs whenever the fallback is used, so users are aware.
554-599:startTestnetfinal function.
- Leverages the
network.DefaultConfigpattern fromCosmos SDKintegration tests.- Blocks on
fmt.Scanln()(line 592), which can interfere with automated environments (previously noted).- Cleans up network resources on exit.
Would you like a PR to implement an optional non-blocking exit for CI runs?
execution/evm/config.go (3)
9-16: Consider converting DefaultEthChainID to a constant big.Int for consistencyThe
DefaultEthChainIDis defined as a string "80087", but it's used with a*big.Inttype in theEVMConfigstruct. Consider defining it as a constant*big.Intor providing a helper function to convert it consistently.-const ( +var ( DefaultEngineAPIURL = "http://localhost:8551" DefaultRPCURL = "http://localhost:8545" DefaultRPCJWTRefreshInterval = 20 * time.Second DefaultJWTSecretPath = "./jwt.hex" DefaultRPCCheckInterval = 3 * time.Second - DefaultEthChainID = "80087" + // DefaultEthChainID represents the default Ethereum chain ID + DefaultEthChainID = new(big.Int).SetString("80087", 10) )
18-25: Add documentation for the EVMConfig structAdd proper Go documentation for the exported
EVMConfigtype to improve code readability and help other developers understand its purpose and usage.+// EVMConfig holds configuration parameters for connecting to and interacting with +// an Ethereum execution client through Engine API and JSON-RPC. type EVMConfig struct { engineAPIURL *url.URL rpcURL *url.URL jwtSecret *JWTSecret rpcJWTRefreshInterval time.Duration rpcCheckInterval time.Duration ethChainID *big.Int }
27-51: Add documentation for NewEVMConfig and consider wrapping errors with contextThe
NewEVMConfigfunction lacks documentation. Also, consider wrapping returned errors with additional context to improve debugging.+// NewEVMConfig creates a new EVMConfig with the provided parameters. +// It parses the provided URLs and retrieves the JWT secret from the specified path. +// Returns an error if URL parsing or JWT retrieval fails. func NewEVMConfig(ethChainID *big.Int, rpcJWTRefreshInterval, rpcCheckInterval time.Duration, engineAPIURL, rpcURL, jwtSecretPath string) (*EVMConfig, error) { eu, err := url.Parse(engineAPIURL) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to parse engine API URL: %w", err) } ru, err := url.Parse(rpcURL) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to parse RPC URL: %w", err) } secret, err := getJWTFromPath(jwtSecretPath) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to get JWT from path %s: %w", jwtSecretPath, err) } return &EVMConfig{ engineAPIURL: eu, rpcURL: ru, jwtSecret: &secret, rpcJWTRefreshInterval: rpcJWTRefreshInterval, rpcCheckInterval: rpcCheckInterval, ethChainID: ethChainID, }, nil }Remember to add the
fmtimport:import ( "fmt" "math/big" "net/url" "time" )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (9)
Makefile(2 hunks)abci/proposal.go(1 hunks)client/testnet.go(1 hunks)execution/evm/client.go(1 hunks)execution/evm/config.go(1 hunks)execution/evm/jwt.go(1 hunks)start-testnet-node.sh(1 hunks)start-testnet-reth-bootnode.sh(1 hunks)start-testnet-reth-miner-node.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- start-testnet-node.sh
- start-testnet-reth-bootnode.sh
- start-testnet-reth-miner-node.sh
- abci/proposal.go
🔇 Additional comments (18)
execution/evm/client.go (1)
41-86: Revisit concurrency approach to avoid potential data races.Multiple goroutines in the
Startmethod (lines 56 and 70) and the separatego c.refreshJWTForRPCClient(...)call at line 49 all eventually calldialRPCCLient, which modifiesc.engineClientorc.rpcClient. Without synchronization (e.g., via a mutex or channel-based coordination), these pointer assignments can lead to data races.Makefile (8)
1-4: Define environment variables for testnet configuration.These variables allow customization of networking (
STARTING-IP-ADDR), the number of testnet nodes (NODES), and the binary path (SCALERIZED_BINARY_PATH). They seem appropriate for containerized deployment or local testing workflows.
6-7: Includescripts/execution-client.mk.This makefile snippet suggests you are modularizing execution client configurations in a separate script. Ensure the file exists and is properly version-controlled.
29-29: Use of.PHONYis correct.Marking these targets as phony helps ensure they run even if files with the same name exist.
37-40: Comment block for clarity.The comment block neatly indicates a new section. No concerns here.
45-47: Specified build dependencies.Requiring
go.sumand the$(BUILDDIR)/directory ensures a clean build environment. Good practice to keep your dependencies locked.
51-54: Comment block for "Misc" section.Keeping the Makefile organized into sections increases maintainability.
58-59: New targetlocaltestnet-example-config.This target initializes example testnet files with a straightforward command invocation. If you intend to run it inside a container context, ensure the path and environment are properly set up.
56-56:✅ Verification successful
Running
./scripts/init.sh.Ensure that the script file has executable permissions and the environment is prepared for any required dependencies or environment variables.
Please verify the script is executable:
🏁 Script executed:
#!/bin/bash ls -l ./scripts/init.shLength of output: 91
Script Executable Verification Confirmed
The
./scripts/init.shfile has proper executable permissions (-rwxr-xr-x), so no changes are required in this regard. Please ensure that all necessary dependencies and environment variables are correctly set up, as this verification only covers file permissions.client/testnet.go (7)
1-53: Package and imports overview.
- The imports cover CometBFT, Cosmos SDK modules, and standard libraries for file, network, and JSON operations. This looks aligned with a typical CLI for a Cosmos-based chain.
- The introduced flags (for node prefixes, IP addresses, etc.) appear relevant for testnet customization.
86-95:addTestnetFlagsToCmdfunction.This function centralizes flag definitions for initialization and startup. The usage of default values is consistent with typical CLI expectations.
97-112:NewTestnetCmdentry point.Creates a root command with subcommands for starting and initializing. Good separation of concerns.
114-164:testnetInitFilesCmd: multi-validator file initialization.
- Offers a robust approach for generating validator configurations, keyrings, etc.
- The usage text is descriptive and helps users understand how to create the testnet config.
394-466:initGenFilesfunction.
- Sets up balance and staking modules for each account, factoring in a base coin unit.
- Marshals the final app state into the genesis doc.
- Saves each validator’s genesis file with identical chain data for consistency.
No immediate concerns found.
468-511:collectGenFilesfunction.
- Gathers and merges generated transactions (gentxs) into each node’s genesis file.
- Uses a canonical timestamp for all validators so they remain in sync.
No major issues.
540-552:writeFileutility.Streamlined approach to creating and writing to a file. No issues.
execution/evm/config.go (2)
53-75: LGTM! Proper getter methods implementedThe getter methods are well-implemented for all unexported fields, addressing the previous review comment about accessibility.
38-41:✅ Verification successful
Check for JWTSecret forward declaration
The code calls
getJWTFromPathfunction which returns aJWTSecrettype, but neither seems to be defined in this file. Ensure these are properly defined elsewhere in the package.
🏁 Script executed:
#!/bin/bash # Check for the definition of JWTSecret type and getJWTFromPath function echo "Searching for JWTSecret type definition..." rg -A 2 "type JWTSecret" --type go echo "Searching for getJWTFromPath function definition..." rg -A 2 "func getJWTFromPath" --type goLength of output: 558
JWTSecret and getJWTFromPath are defined as expected
After verifying, both the
JWTSecrettype and thegetJWTFromPathfunction are declared inexecution/evm/jwt.go:
- JWTSecret is defined as
type JWTSecret [32]byte- getJWTFromPath is implemented to return a
JWTSecretalong with an errorSince these definitions exist elsewhere in the package, no forward declaration is needed in
execution/evm/config.go.
| return err | ||
| } | ||
|
|
||
| keyringAlgos, _ := kb.SupportedAlgorithms() | ||
| algo, err := keyring.NewSigningAlgoFromString(args.algo, keyringAlgos) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| addr, secret, err := testutil.GenerateSaveCoinKey(kb, nodeDirName, "", true, algo) | ||
| if err != nil { | ||
| _ = os.RemoveAll(args.outputDir) | ||
| return err | ||
| } | ||
|
|
||
| info := map[string]string{"secret": secret} | ||
|
|
||
| cliPrint, err := json.Marshal(info) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // save private key seed words | ||
| if err := writeFile(fmt.Sprintf("%v.json", "key_seed"), nodeDir, cliPrint); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| accStakingTokens := sdk.TokensFromConsensusPower(5000, PowerReduction) | ||
| coins := sdk.Coins{ | ||
| sdk.NewCoin(params.CoinUnit, accStakingTokens), | ||
| } | ||
|
|
||
| genBalances = append(genBalances, banktypes.Balance{Address: addr.String(), Coins: coins.Sort()}) | ||
| genAccounts = append(genAccounts, authtypes.NewBaseAccount(addr, nil, 0, 0)) | ||
|
|
||
| valStr, err := valAddrCodec.BytesToString(sdk.ValAddress(addr)) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| valTokens := sdk.TokensFromConsensusPower(100, PowerReduction) | ||
| createValMsg, err := stakingtypes.NewMsgCreateValidator( | ||
| valStr, | ||
| valPubKeys[i], | ||
| sdk.NewCoin(params.CoinUnit, valTokens), | ||
| stakingtypes.NewDescription(nodeDirName, "", "", "", ""), | ||
| stakingtypes.NewCommissionRates(math.LegacyOneDec(), math.LegacyOneDec(), math.LegacyOneDec()), | ||
| math.OneInt(), | ||
| ) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| txBuilder := clientCtx.TxConfig.NewTxBuilder() | ||
| if err := txBuilder.SetMsgs(createValMsg); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| txBuilder.SetMemo(memo) | ||
|
|
||
| txFactory := tx.Factory{} | ||
| txFactory = txFactory. | ||
| WithChainID(args.chainID). | ||
| WithMemo(memo). | ||
| WithKeybase(kb). | ||
| WithTxConfig(clientCtx.TxConfig) | ||
|
|
||
| if err := tx.Sign(cmd.Context(), txFactory, nodeDirName, txBuilder, true); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| txBz, err := clientCtx.TxConfig.TxJSONEncoder()(txBuilder.GetTx()) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if err := writeFile(fmt.Sprintf("%v.json", nodeDirName), gentxsDir, txBz); err != nil { | ||
| return err | ||
| } | ||
| // config.DefaultConfig() | ||
| // customAppTemplate, customAppConfig := config.DefaultConfig() | ||
| srvconfig.SetConfigTemplate(srvconfig.DefaultConfigTemplate) | ||
|
|
||
| srvconfig.WriteConfigFile(filepath.Join(nodeDir, "config/app.toml"), appConfig) | ||
| } | ||
|
|
||
| if err := initGenFiles(clientCtx, mbm, args.chainID, params.CoinUnit, genAccounts, genBalances, genFiles, args.numValidators); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| err := collectGenFiles( | ||
| clientCtx, nodeConfig, args.chainID, nodeIDs, valPubKeys, args.numValidators, | ||
| args.outputDir, args.nodeDirPrefix, args.nodeDaemonHome, genBalIterator, valAddrCodec) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| cmd.PrintErrf("Successfully initialized %d node directories\n", args.numValidators) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
initTestnetFiles function.
- Generates each validator’s private keys, node directories, and genesis transactions.
- Cleans up (removing all generated files) if an error occurs, which is convenient but may be surprising in a multi-step pipeline. Ideally, you might isolate partial cleanup or confirm with the user.
- Writes mnemonic seeds (line 316) in JSON files. For production usage, strongly consider secure storage or disclaim usage only for test environments.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
execution/evm/client.go (1)
55-81: Revisit concurrency safety for pointer assignments.Calls to
c.connect(...)in separate goroutines updatec.engineClientorc.rpcClient. Additionally,refreshJWTForRPCClientmight also manipulateengineClientconcurrently. Past reviews flagged potential data races. Even if you’re updating different fields, ensure no reads/writes occur on the same pointer at the same time, or protect assignments with a mutex if needed.
🧹 Nitpick comments (4)
execution/evm/client.go (4)
29-35: Validate the EVMConfig in the constructor.Currently,
NewEVMClientreturns anEVMClienteven if the providedcfgis nil or improperly configured. Consider validatingcfg(e.g., non-nil with valid URLs) to fail fast and avoid downstream errors.
41-49: Provide a way to stop repetitively polling when the node is unavailable.In
Start, the repeated ticker-based retries continue indefinitely if the engine or RPC node never becomes available. Consider introducing a timeout or max attempts to prevent potentially infinite waiting.
83-86: Handle the scenario where connections fail repeatedly.After waiting for the goroutines to succeed (
wg.Wait), if both are unsuccessful indefinitely,ensureClientCreatedCh <- truestill executes eventually. Consider adding logic to detect unrecoverable failures and return an error to prevent proceeding with an uninitialized client.
123-151: Add more descriptive error logging in dialRPCCLient.When dialing fails for the engine or RPC client, you currently return the raw error. For better troubleshooting, prepend a short description (e.g., “Failed dialing engine client: ...”) so logs are more user-friendly, especially if multiple dial operations can fail.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (1)
execution/evm/client.go(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
execution/evm/client.go (2)
execution/evm/config.go (1)
EVMConfig(18-25)execution/evm/constants.go (2)
ExchangeCapabilities(17-17)ScalerizeSupportedCapabilities(22-29)
NikhilSharmaWe
force-pushed
the
feat/evm-abci-handlers
branch
from
0d5d30e to
9d97c1b
Compare
…d rename refreshJWTForRPCClient method to refreshJWTForClient Signed-off-by: Nikhil Sharma <[email protected]>
NikhilSharmaWe
force-pushed
the
feat/evm-abci-handlers
branch
from
9d97c1b to
54d283f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
execution/evm/client.go (2)
123-151:⚠️ Potential issuePotential concurrency hazards when assigning
c.engineClientandc.rpcClient.The
dialRPCCLientmethod setsc.engineClientorc.rpcClient, yet the same method may be invoked concurrently from multiple goroutines. Coordinate access to these pointer fields to prevent data races.For instance, introduce a mutex in the
EVMClientstruct:type EVMClient struct { ctx context.Context config *EVMConfig engineClient *ethclient.Client rpcClient *ethclient.Client logger log.Logger + mu sync.Mutex } func (c *EVMClient) dialRPCCLient(clientType string) error { + c.mu.Lock() defer c.mu.Unlock() ... }
41-49:⚠️ Potential issueConcurrent pointer updates may cause data races.
Multiple goroutines (the
Startmethod's ticker-based loops and the call torefreshJWTForRPCClient) can simultaneously invokedialRPCCLient, which replacesc.engineClientorc.rpcClient. Without synchronization (e.g., a mutex), this concurrency can cause data races and undefined behavior.One approach is to guard the pointer assignments with a mutex or ensure these assignments happen in a single goroutine. For example:
func (c *EVMClient) Start(ctx context.Context, ensureClientCreatedCh chan bool) error { var ( wg sync.WaitGroup ticker = time.NewTicker(c.config.RPCCheckInterval()) + mu sync.Mutex ) defer func() { ticker.Stop() - go c.refreshJWTForRPCClient(ctx, engineClient) + go func() { + mu.Lock() + defer mu.Unlock() + c.refreshJWTForRPCClient(ctx, engineClient) + }() }() ... }
🧹 Nitpick comments (2)
execution/evm/client.go (1)
49-49: Mismatch function name:refreshJWTForRPCClientvs.refreshJWTForClient.The function invoked here is
refreshJWTForRPCClient, but the implementation injwt.gois namedrefreshJWTForClient. Align these names to improve clarity and avoid confusion.- go c.refreshJWTForRPCClient(ctx, engineClient) + go c.refreshJWTForClient(ctx, engineClient)execution/evm/jwt.go (1)
30-33: Log message does not reflect the correct client type.The log statement says "Refreshed JWT Token for ethereum engine API," but this function handles refresh for both
"engine-client"and"rpc-client". Use the passed-inclientTypein your log message for accuracy.c.logger.Info("Starting JWT refresh loop 🔄") ... c.logger.Info("Refreshed JWT Token for ethereum engine API") +// Modify to: +c.logger.Info(fmt.Sprintf("Refreshed JWT Token for %s", clientType))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (2)
execution/evm/client.go(1 hunks)execution/evm/jwt.go(1 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
execution/evm/jwt.go (1)
execution/evm/client.go (1)
EVMClient(21-27)
execution/evm/client.go (2)
execution/evm/config.go (1)
EVMConfig(18-25)execution/evm/constants.go (2)
ExchangeCapabilities(17-17)ScalerizeSupportedCapabilities(22-29)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
execution/evm/client.go (2)
21-27: Synchronize pointer assignments to prevent data racesWe've flagged concurrency safety for pointer fields in prior reviews, but those concerns remain valid. If multiple goroutines call
dialRPCCLientin parallel (e.g., fromStartand the deferred JWT refresh), they may overwritec.engineClientorc.rpcClientconcurrently. Use a mutex or ensure that pointer assignment is done in a single goroutine.
88-121: Concurrent pointer updates inconnectmay cause data racesWhen multiple goroutines call
connect(e.g., one forengineClientand another forrpcClient), the pointer assignments toc.engineClientorc.rpcClientoccur concurrently. This can result in inconsistent states or data races unless synchronization is enforced.execution/evm/jwt.go (1)
18-36: Avoid potential concurrent pointer assignment in the refresh loop
refreshJWTForClientcallsdialRPCCLient, which sets eitherc.engineClientorc.rpcClient. This may coincide with other goroutines performing the same assignment unless steps are taken to prevent overlap.
🧹 Nitpick comments (1)
execution/evm/client.go (1)
41-86: Consider refactoring the deferred JWT refresh initializationCurrently, the JWT refresh loop starts only after
Startreturns. If you'd like more granular or immediate control over the token refresh, consider launching the refresh JWT routine once the Engine connection is established, rather than deferring it until the method exits.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (2)
execution/evm/client.go(1 hunks)execution/evm/jwt.go(1 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
execution/evm/jwt.go (1)
execution/evm/client.go (1)
EVMClient(21-27)
execution/evm/client.go (2)
execution/evm/config.go (1)
EVMConfig(18-25)execution/evm/constants.go (2)
ExchangeCapabilities(17-17)ScalerizeSupportedCapabilities(22-29)
🔇 Additional comments (1)
execution/evm/client.go (1)
123-151: Enhance concurrency safety and improve naming fordialRPCCLient
- The function still updates
c.engineClientorc.rpcClientwithout concurrency safeguards. Please ensure pointer assignments are not updated in parallel by multiple goroutines.- For clarity, consider renaming
dialRPCCLient→dialRPCClient. This small rename aligns better with existing naming conventions and improves readability.
[duplicate_comment, suggest_nitpick]
| func buildSignedJWT(s *JWTSecret) (string, error) { | ||
| token := jwt.NewWithClaims(jwt.SigningMethodHS256, jwt.MapClaims{ | ||
| "iat": &jwt.NumericDate{Time: time.Now()}, | ||
| }) | ||
| str, err := token.SignedString(s[:]) | ||
| if err != nil { | ||
| return "", errors.Newf("failed to create JWT token: %w", err) | ||
| } | ||
| return str, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider including an expiration claim in the JWT
Currently, only an "iat" (issued-at) claim is set. Following best practices, adding an "exp" (expiration) claim can help ensure tokens remain short-lived, mitigating risks if a token is compromised.
Description
Summary by CodeRabbit
New Features
Chores