Skip to content

Conversation

@MegaRedHand
Copy link
Collaborator

This PR updates the devnet setup we have in our repo:

@github-actions
Copy link

🤖 Codex Code Review

Changes here are documentation/Makefile only; no Rust, fork choice, attestation, STF, XMSS, or SSZ code touched in this PR, so consensus‑layer correctness isn’t affected.

Findings

  • Doc drift: CLAUDE.md still documents a metrics/ directory that this PR deletes; it now points contributors to a non‑existent path. Update or remove that entry to reflect the move to lean‑quickstart. CLAUDE.md:20
  • UX/diagnostics: run-devnet now redirects all output to devnet.log, which hides failures unless users know to inspect the file. Consider echoing the log path on error or tail -f guidance so failures aren’t silent. Makefile:40-50

Notes

  • The sed replacement rewrites lean-quickstart/client-cmds/ethlambda-cmd.sh in place; if the script format changes and the pattern doesn’t match, the tag won’t update but the command won’t clearly warn. Consider adding a check to fail loudly when no replacement occurs. Makefile:45-47

If you want, I can propose exact text updates for CLAUDE.md and the run-devnet logging behavior.


Automated review by OpenAI Codex · custom prompt

@github-actions
Copy link

🤖 Claude Code Review

Perfect! Now I have a complete understanding of the PR. Let me compile my review:


PR Review: feat: simplify devnet setup (#109)

Summary

This PR simplifies the devnet setup by removing local metrics stack configuration (which was integrated into lean-quickstart) and improves the run-devnet Makefile target to use locally built Docker images. Overall, this is a positive cleanup that reduces maintenance burden and improves the developer experience.

Changes Overview

Removed files: Local metrics stack (docker-compose, Grafana dashboards, Prometheus configs)
Updated documentation: README.md, docs/metrics.md
Enhanced Makefile: Added fmt target, improved run-devnet workflow
No Rust code changes: Infrastructure-only PR


Review Findings

Good Changes

  1. Makefile improvements (Makefile:6-7, 26, 40-50)

    • Adding make fmt target aligns with the documented workflow in CLAUDE.md
    • The run-devnet target now dynamically injects the locally built Docker tag into lean-quickstart's config
    • Proper use of temp file for sed operation ensures macOS/GNU compatibility
    • Clear user feedback with @echo statements
  2. Documentation updates (README.md, docs/metrics.md)

    • README now clearly documents common Makefile targets
    • Properly redirects users to lean-quickstart for metrics stack
    • Removes outdated instructions about local metrics configuration
  3. Cleanup rationale


⚠️ Issues & Concerns

Critical: Output redirection may hide errors (Makefile:50)

&& NETWORK_DIR=local-devnet ./spin-node.sh --node all --generateGenesis --metrics > devnet.log 2>&1

Problem: Redirecting both stdout and stderr to devnet.log means:

  • Users won't see real-time progress or error messages
  • If the devnet fails to start, there's no immediate feedback
  • The message "Press Ctrl+C to stop all nodes" appears BEFORE the devnet actually starts, which could be confusing if startup fails

Impact: Medium — degrades developer experience, especially for debugging startup issues

Recommendation: Consider one of these options:

  1. Use tee to show output and log simultaneously:
    && NETWORK_DIR=local-devnet ./spin-node.sh --node all --generateGenesis --metrics 2>&1 | tee devnet.log
  2. Run in foreground without redirection (current behavior shown to user)
  3. Add a follow-up message after the command explaining where to find logs

Minor: Potential race condition with sed/mv (Makefile:46-47)

@sed 's|ghcr.io/lambdaclass/ethlambda:[^ ]*|ghcr.io/lambdaclass/ethlambda:$(DOCKER_TAG)|' lean-quickstart/client-cmds/ethlambda-cmd.sh > lean-quickstart/client-cmds/ethlambda-cmd.sh.tmp \
    && mv lean-quickstart/client-cmds/ethlambda-cmd.sh.tmp lean-quickstart/client-cmds/ethlambda-cmd.sh

Observation: This modifies a file in the cloned lean-quickstart directory. If users run multiple devnets in parallel or the repository is shared, there could be conflicts.

Impact: Low — unlikely in typical single-developer scenarios

Optional improvement: Consider using sed -i.bak (though this has portability issues you're trying to avoid) or document that only one devnet should run at a time per lean-quickstart clone.


Minor: Missing .PHONY declaration (Makefile:1, 30, 37)

.PHONY: help fmt lint docker-build run-devnet test

Issue: The targets leanSpec and lean-quickstart are not declared as .PHONY, but they create directories with the same name. This is actually correct behavior — they should NOT be .PHONY since they're file-based targets.

Status: No issue — current implementation is correct ✅


Documentation clarity (README.md:34)

For custom devnet configurations, go to `lean-quickstart/local-devnet/genesis/validator-config.yaml`

Minor suggestion: This instruction assumes the user has already cloned lean-quickstart. It might be clearer to add:

After running `make run-devnet` once, you can customize the devnet by editing `lean-quickstart/local-devnet/genesis/validator-config.yaml` before subsequent runs.

Impact: Very low — current wording is acceptable


Security Considerations

  • No security vulnerabilities introduced
  • No credential handling or sensitive data exposure
  • Dockerfile tag injection uses properly scoped Makefile variables

Best Practices Check

  • ✅ No Rust code changes — reduces risk
  • ✅ Follows project philosophy of minimalism
  • ✅ Removes unused code rather than commenting it out
  • ✅ Uses idiomatic Make patterns
  • ✅ Platform compatibility (macOS/Linux) considered

Recommendation

Approve with minor suggestions

The PR successfully achieves its goals and is well-executed. The only significant concern is the output redirection in run-devnet that may hide startup errors. Consider addressing this to improve developer experience.

Suggested changes (optional):

  1. High priority: Replace > devnet.log 2>&1 with 2>&1 | tee devnet.log to show real-time output
  2. Low priority: Clarify documentation about when to edit validator config

Testing Verification

Since this is an infrastructure PR, verify:

  • make fmt works correctly
  • make run-devnet successfully builds and starts the devnet with the correct Docker tag
  • Logs appear in devnet.log as expected
  • The dynamically injected Docker tag is correctly substituted in ethlambda-cmd.sh

Automated review by Claude (Anthropic) · custom prompt

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Feb 10, 2026

Greptile Overview

Greptile Summary

This PR simplifies local devnet setup by removing the in-repo Grafana/Prometheus metrics stack, adding a make fmt wrapper around cargo fmt, and updating make run-devnet to build a Docker image and patch lean-quickstart’s ethlambda client script to use the freshly built DOCKER_TAG. Documentation is updated to enumerate common Makefile targets and to point metrics-stack users to lean-quickstart instead of the removed metrics/ directory.

Confidence Score: 3/5

  • Mostly safe to merge, but the updated devnet target has a couple of behavior mismatches that should be corrected to avoid confusing/incorrect runs.
  • Large portion of the PR is deletions (metrics stack). The main risk is the Makefile run-devnet behavior: log path messaging is currently incorrect, and the sed-based tag rewrite is broad enough to update the wrong occurrence if the upstream script changes.
  • Makefile

Important Files Changed

Filename Overview
Makefile Adds fmt target and rewrites run-devnet to patch lean-quickstart script with DOCKER_TAG and log to devnet.log.
README.md Updates getting started to list common make targets and adjusts devnet description/config guidance.
docs/metrics.md Removes in-repo metrics stack reference; points users to lean-quickstart for Grafana/Prometheus dashboards.
metrics/README.md Deletes metrics stack README as part of removing in-repo metrics tooling.
metrics/docker-compose-metrics.yaml Deletes docker compose file for Grafana/Prometheus metrics stack.
metrics/grafana/dashboards/client-dashboard.json Removes bundled Grafana client dashboard JSON.
metrics/grafana/dashboards/client-interop-dashboard.json Removes bundled Grafana interop dashboard JSON.
metrics/grafana/provisioning/dashboards/dashboards.yml Deletes Grafana provisioning for dashboards.
metrics/grafana/provisioning/datasources/prometheus.yml Deletes Grafana provisioning for Prometheus datasource.
metrics/prometheus/prometheus.yml Deletes Prometheus scrape configuration for local metrics stack.

Sequence Diagram

sequenceDiagram
  participant Dev as Developer
  participant Make as Makefile
  participant Docker as docker
  participant LQS as lean-quickstart repo
  participant Sh as spin-node.sh
  participant Client as ethlambda (docker image)

  Dev->>Make: make run-devnet (DOCKER_TAG=...)
  Make->>Docker: make docker-build (tag ghcr.io/lambdaclass/ethlambda:${DOCKER_TAG})
  Make->>LQS: git clone lean-quickstart (if missing)
  Make->>LQS: sed replace image tag in client-cmds/ethlambda-cmd.sh
  Make->>Sh: cd lean-quickstart && NETWORK_DIR=local-devnet ./spin-node.sh --node all --generateGenesis --metrics
  Sh->>Client: start nodes using updated image tag
  Sh-->>Make: stream logs (redirected to devnet.log)
  Make-->>Dev: prints instructions / exits
Loading

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

10 files reviewed, 3 comments

Edit Code Review Agent Settings | Greptile

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant