Skip to content

feat: test(exit-certificate): add E2E harness, bridge/status scripts and CI workflow#1655

Open
joanestebanr wants to merge 5 commits into
feature/exit-certificate-toolfrom
feature/exit_certifcate_tool-kurtosis
Open

feat: test(exit-certificate): add E2E harness, bridge/status scripts and CI workflow#1655
joanestebanr wants to merge 5 commits into
feature/exit-certificate-toolfrom
feature/exit_certifcate_tool-kurtosis

Conversation

@joanestebanr

Copy link
Copy Markdown
Collaborator

🔄 Changes Summary

  • Add the exit-certificate E2E test harness under test/e2e-exit_certificate/: spins up a Kurtosis CDK network (run_network.sh), prepares a settled certificate (prepare_network.sh), and runs the exit_certificate tool (run_exit_tool.sh), orchestrated by run_e2e_test.sh (+ helper.sh, single_op_pessimistic_args.json, README.md).
  • Add supporting scripts under tools/exit_certificate/scripts/:
    • bridge_l2_to_l1.sh — L2→L1 bridge to produce a certificate.
    • agglayer_certificate_status.sh — poll/wait for certificate settlement.
    • export_kurtosis_env.sh — export Kurtosis env vars for the scripts.
    • agglayer_status/main.go — agglayer status helper.
    • Update bridge_l1_to_l2.sh.
  • Add CI workflow .github/workflows/test-e2e-exit_cenrtificate_tool.yml that runs the E2E test on PRs touching tools/exit_certificate/** (also workflow_dispatch).

⚠️ Breaking Changes

  • None.

📋 Config Updates

  • None.

✅ Testing

  • 🤖 Automatic: New GitHub Actions workflow runs test/e2e-exit_certificate/run_e2e_test.sh on PRs that modify tools/exit_certificate/** (Kurtosis CDK network → settled certificate → exit_certificate tool).
  • 🖱️ Manual: ./test/e2e-exit_certificate/run_e2e_test.sh locally (requires Docker, Kurtosis CLI, Foundry cast, Go).

🐞 Issues

  • N/A

🔗 Related PRs

📝 Notes

  • The E2E harness clones a pinned Kurtosis CDK commit and manages its own aggkit enclave; the workflow stops the enclave on completion.

… workflow

Add the exit-certificate E2E test harness under test/e2e-exit_certificate/
that spins up a Kurtosis CDK network, prepares a settled certificate and runs
the exit_certificate tool. Add supporting scripts (L2->L1 bridge, agglayer
certificate status, kurtosis env export, agglayer_status helper) and a CI
workflow that runs the E2E test on PRs touching tools/exit_certificate/**.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@claude

claude Bot commented Jun 16, 2026

Copy link
Copy Markdown

Claude finished @joanestebanr's task in 2m 32s —— View job


🔍 PR Review Complete

PR #1655: E2E test harness for exit-certificate tool


🚀 What's New

This PR introduces a comprehensive E2E test harness for the exit-certificate tool that:

  • Spins up a Kurtosis CDK network with a single optimistic L2 chain
  • Performs L1→L2 and L2→L1 bridges to produce and settle a certificate
  • Runs the exit_certificate tool against this prepared environment
  • Adds supporting scripts for bridge operations, certificate status polling, and environment variable export
  • Integrates the E2E test into CI via GitHub Actions

🐛 Bug Fixes

None.

📋 Config Updates

None.

⚠️ Breaking Changes

None.


Review Feedback

🔴 Required Issues

1. Typo in GitHub Actions workflow filename

File: .github/workflows/test-e2e-exit_cenrtificate_tool.yml

The workflow filename contains a typo: cenrtificate should be certificate. This typo appears in:

  • The filename itself
  • Line 12 of the workflow (in the paths filter)

Impact: While this doesn't break functionality, it affects discoverability and professionalism. The typo will persist in the repository history.

Recommendation: Rename the file to test-e2e-exit_certificate_tool.yml and update line 12 accordingly.

Fix typo in workflow filename →


2. Empty README.md file

File: test/e2e-exit_certificate/README.md

The README.md file is completely empty (0 bytes). This directory contains a complex E2E test setup that would greatly benefit from documentation explaining:

  • Purpose and scope of the E2E tests
  • Prerequisites (Docker, Kurtosis CLI, Foundry, Go)
  • How to run the tests locally
  • What each script does
  • Expected duration and resource requirements
  • Troubleshooting common issues

Impact: Developers (especially new contributors) won't understand how to use or maintain this test harness without reading all the shell scripts.

Recommendation: Add comprehensive documentation to the README.md. Based on the usage patterns in the scripts, it should document the orchestration flow and prerequisites.

Add README documentation →


3. Typo in variable name (consistency issue)

File: test/e2e-exit_certificate/prepare_network.sh:7 and run_exit_tool.sh:8

The variable EXIT_CETIFICATE_SCRIPT_DIR has a typo: CETIFICATE should be CERTIFICATE.

Impact: While this doesn't break functionality (the typo is consistent within the affected files), it's a spelling error that reduces code quality.

Recommendation: Rename to EXIT_CERTIFICATE_SCRIPT_DIR throughout.

Fix variable name typo →


🟡 Suggested Improvements

4. Go code: Missing error context in some paths

File: tools/exit_certificate/scripts/agglayer_status/main.go:141

In printNetworkInfo(), the error from GetNetworkInfo is logged to stderr but doesn't include which network ID failed:

fmt.Fprintf(os.Stderr, "WARN: get network info: %v\n", err)

Suggestion: Include the netID in the warning for better debugging:

fmt.Fprintf(os.Stderr, "WARN: get network info for network %d: %v\n", netID, err)

5. Shell scripts: Hardcoded Python dependency

Files: Multiple scripts use python3 for parsing JSON and arithmetic

The scripts assume python3 is available on the system but don't check for it. Examples:

  • bridge_l1_to_l2.sh:214-222 (parsing JSON)
  • bridge_l1_to_l2.sh:234 (hex to decimal conversion)
  • bridge_l1_to_l2.sh:286,292,299 (arithmetic comparisons)

Suggestion: Add python3 to the dependency checks (similar to how cast is checked). For example, in bridge_l1_to_l2.sh:123-129:

check_deps() {
    if ! command -v cast &>/dev/null; then
        log_error "Missing required tool: cast (foundry)"
        log_error "Install Foundry: https://getfoundry.sh"
        exit 1
    fi
    if ! command -v python3 &>/dev/null; then
        log_error "Missing required tool: python3"
        exit 1
    fi
}

6. Shell script: Potential race condition with enclave state

File: test/e2e-exit_certificate/run_network.sh:89-92

The start_environment() function checks if the enclave exists and warns that nothing will be done. However, there's a potential race where the enclave could be in a partially-failed state (exists but not healthy).

Suggestion: Consider adding a health check or providing a --force flag to recreate the enclave even if it exists. Alternatively, document this behavior more clearly in the warning message.


7. Test harness: No timeout protection for the full E2E flow

File: test/e2e-exit_certificate/run_e2e_test.sh

The top-level test orchestration script doesn't implement a timeout, so a hung network or claim wait could cause the CI job to run until the GitHub Actions timeout (likely hours).

Suggestion: Add a timeout wrapper around the entire test, or ensure each sub-command has appropriate timeouts. The GitHub Actions job could specify a timeout-minutes as a safeguard:

jobs:
  test-e2e-exit-certificate:
    name: Exit-certificate tool E2E
    runs-on: amd-runner-2204
    timeout-minutes: 30  # Add this

8. Hardcoded magic numbers in bridge scripts

Files: bridge_l1_to_l2.sh:72, bridge_l2_to_l1.sh:76

The default bridge amount 1234567890 wei (≈0.0000000012 ETH) seems arbitrary. While this works for testing, the value and rationale aren't documented.

Suggestion: Add a comment explaining why this specific value is chosen, or use a more readable value like 1000000000 (1 gwei).


9. Inconsistent error handling in wait_for_claim

Files: bridge_l1_to_l2.sh:191, bridge_l2_to_l1.sh:195

The wait_for_claim() function returns 1 on timeout but the return value isn't always checked by callers (depending on the --wait flag). When used with set -euo pipefail, this could cause unexpected exits.

Suggestion: Make the error handling more explicit, or document that timeouts are non-fatal.


🟢 Optional / Minor Observations

10. CI workflow uses custom runner

File: .github/workflows/test-e2e-exit_cenrtificate_tool.yml:25

The workflow uses amd-runner-2204 which appears to be a self-hosted runner. This is fine but limits portability.

Note: This is likely intentional for performance/resource reasons. No action needed unless you want to add a comment explaining the choice.


11. Verbose log output in helper.sh

File: test/e2e-exit_certificate/helper.sh:25-82

The run_quiet() function is quite sophisticated with ANSI cursor manipulation and live tail output. This is excellent for UX but adds complexity.

Note: The implementation looks solid and handles both TTY and non-TTY scenarios well. Well done!


12. Git clone uses temporary directory without cleanup trap

File: test/e2e-exit_certificate/run_network.sh:64-84

The clone_kurtosis_repo() function creates a temporary directory but doesn't set a trap to clean it up on failure. However, the directory is under /tmp so it will eventually be cleaned up by the OS.

Note: Current approach is acceptable for temporary test directories. If long-running tests are an issue, consider adding cleanup.


Summary

This is a well-structured E2E test harness with good separation of concerns. The shell scripts are generally well-written with proper error handling, dependency checks, and user-friendly logging. The Go helper code follows good practices and integrates cleanly with the agglayer client.

Key strengths:

  • ✅ Comprehensive test coverage (network setup → bridge → certificate settlement → tool execution)
  • ✅ Good use of run_quiet() for clean CI output while preserving error details
  • ✅ Environment variable export pattern makes scripts composable
  • ✅ Proper use of set -euo pipefail for robust error handling
  • ✅ Clear logging with color-coded messages

Must-fix before merge:

  • 🔴 Typo in workflow filename (cenrtificatecertificate)
  • 🔴 Empty README.md needs documentation
  • 🔴 Variable name typo (CETIFICATECERTIFICATE)

Recommended improvements:

  • 🟡 Add python3 to dependency checks
  • 🟡 Add timeout protection to CI job
  • 🟡 Better error context in Go code
  • 🟡 Document magic numbers and improve error handling clarity

Overall, this is a solid contribution that adds valuable E2E testing infrastructure. Once the required issues are addressed, this will be ready to merge!


Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@joanestebanr joanestebanr self-assigned this Jun 16, 2026
@joanestebanr joanestebanr added the exit_certificate_tool Tool to create a final exit certificate label Jun 16, 2026
@joanestebanr joanestebanr changed the title test(exit-certificate): add E2E harness, bridge/status scripts and CI workflow feat: test(exit-certificate): add E2E harness, bridge/status scripts and CI workflow Jun 16, 2026

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0e5274301b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +195 to +198
// No open pending certificate: whatever was submitted has settled.
fmt.Printf("No pending certificate — latest is settled. (%s elapsed)\n", elapsed)
printStatus(ctx, client, netID)
return nil

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Wait for the first certificate before stopping aggsender

When prepare_network.sh calls this helper immediately after the L2→L1 bridge, there is a window before aggsender has submitted any pending certificate. In that state GetNetworkInfo can return LatestPendingStatus == nil and no settled height/LER yet, but this branch still returns success; the next step stops aggkit-001, leaving the E2E with no settled certificate and causing the exit tool to build against a zero previous LER or fail the LER check. Keep polling until a certificate is actually observed as settled, e.g. require a settled height/LER or first observe a pending certificate before treating “no pending” as success.

Useful? React with 👍 / 👎.

joanestebanr and others added 3 commits June 16, 2026 16:07
…yer_status

Drop the GetNetworkInfo summary helper and its call sites; the certificate
header is the primary output and the settled/pending summary was redundant.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Cover latestHeader, printStatus, waitForSettled (polling, settled,
in-error, timeout, RPC failure) and the formatting helpers using the
existing AgglayerClientMock.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
run_network.sh stop now uses `kurtosis clean --all` instead of removing a
single named enclave. bridge_l1_to_l2.sh dumps the full `cast run`
execution trace before filtering for the revert reason.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@sonarqubecloud

Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
1 Security Hotspot
64.4% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

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

Labels

exit_certificate_tool Tool to create a final exit certificate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant