fix: stop appending endpoint path to checkpoint sync URL

[MegaRedHand]

Summary

• --checkpoint-sync-url now expects the full URL (e.g., http://peer:5052/lean/v0/states/finalized) instead of just the base URL with /lean/v0/states/finalized appended automatically
• Decouples the CLI from a specific API path convention, so it works with any checkpoint provider regardless of endpoint layout

Test plan

• cargo check passes
• Verify checkpoint sync works end-to-end with the full URL in a devnet

[github-actions]

🤖 Kimi Code Review

Review Summary

The PR makes checkpoint sync URL handling more flexible by removing the hardcoded /lean/v0/states/finalized suffix and requiring users to provide the full URL. This is a good change for flexibility, but introduces some potential issues.

Issues Found

1. Breaking Change Without Backward Compatibility (checkpoint_sync.rs:68-75)

The removal of automatic path suffixing breaks existing deployments that expect to provide just the base URL. Consider:

• Adding a deprecation warning for URLs without the expected suffix
• Or keeping backward compatibility by checking if the URL already ends with /lean/v0/states/finalized

2. Missing URL Validation (checkpoint_sync.rs:68)

The function now accepts any URL without validating it contains the expected endpoint. Add validation:

if !url.ends_with("/lean/v0/states/finalized") {
    return Err(CheckpointSyncError::InvalidUrl(
        "URL must point to /lean/v0/states/finalized endpoint".to_string()
    ));
}

3. Documentation Inconsistency (docs/checkpoint_sync.md:29)

The documentation states "Any running node that serves the finalized state as SSZ can be used" but then immediately specifies the ethlambda endpoint. This could be clearer - explicitly state that non-ethlambda nodes must expose their finalized state at the same endpoint path.

4. Potential Security Issue (checkpoint_sync.rs:68)

With full URL control, users could accidentally point to:

• HTTP instead of HTTPS endpoints
• Malicious servers hosting invalid states
  Consider adding a warning log or requiring HTTPS for production use.

5. CLI Help Text Could Be More Explicit (main.rs:50)

The help text mentions the ethlambda endpoint but doesn't clearly state this is required for ethlambda peers. Consider:

/// URL to download checkpoint state from (e.g., http://peer:5052/lean/v0/states/finalized)
/// Must serve SSZ-encoded finalized state at this exact path.

Minor Suggestions

• The parameter name change from base_url to url is good - more accurate
• The removal of trim_end_matches('/') is acceptable since users now provide the full URL

---

Automated review by Kimi (Moonshot AI) · custom prompt

[github-actions]

🤖 Codex Code Review

Overall looks fine; the change is small and aligns docs/help with behavior. One correctness/UX concern to consider.

Findings:

1. Potential backward-compat break without validation. fetch_checkpoint_state now expects a full endpoint URL and no longer appends /lean/v0/states/finalized. If users still pass a base URL (per previous behavior), this will hit the base URL and likely 404 without a clear error. Consider either:

• Accepting both: detect missing path and append, or
• Validating the URL path and returning a targeted error message.
  This is in bin/ethlambda/src/checkpoint_sync.rs:65-79 and CLI help updated in bin/ethlambda/src/main.rs:50-53.

No other correctness, security, or performance issues spotted in this diff.

---

Automated review by OpenAI Codex · custom prompt

[greptile-apps]

Greptile Summary

This PR refactors the checkpoint sync URL handling to require the full endpoint URL instead of automatically appending /lean/v0/states/finalized. This decouples the CLI from a specific API path convention, making it more flexible for different checkpoint providers.

Changes:

• Removed automatic path construction in fetch_checkpoint_state function
• Updated CLI help text to show full URL example
• Updated documentation to clarify endpoint flexibility

Impact:

• This is a breaking change for existing users who pass base URLs like http://peer:5052 - they must now include the full path http://peer:5052/lean/v0/states/finalized
• The change improves flexibility by supporting any checkpoint provider regardless of their endpoint structure

Confidence Score: 5/5

• This PR is safe to merge with no risk
• Clean refactoring with clear documentation updates. The logic change is minimal (removed 2 lines of path construction), all documentation is updated consistently, and the change improves API flexibility without affecting correctness or security
• No files require special attention

Important Files Changed

Filename	Overview
bin/ethlambda/src/checkpoint_sync.rs	Simplified fetch_checkpoint_state to accept full URL instead of base URL, removing path construction logic
bin/ethlambda/src/main.rs	Updated CLI documentation to clarify that full URL is required including the endpoint path
docs/checkpoint_sync.md	Updated documentation to reflect that any SSZ-serving node can be used, with endpoint path specified by the provider

_{Last reviewed commit: edf5722}

[github-actions]

🤖 Claude Code Review

Now I have a complete picture of the PR. Here is my review:

---

PR Review: refactor: stop appending endpoint path to checkpoint sync URL

Summary

A small, focused refactor. The implementation is clean and correct. My notes below, roughly ordered by importance.

---

Breaking Change — Not Flagged

This is a breaking CLI change. Any deployment or script using the old format:

--checkpoint-sync-url http://peer:5052

will now silently make a GET to http://peer:5052 (no path), likely receiving a 404 or HTML error that fails with an HTTP error or SSZ decode failure — but not a clear "you passed the wrong format" message.

The PR description calls this out, but there's no migration hint in the docs or CLI help. It's worth adding a note to the docs/checkpoint_sync.md under a ## Migration section or in the change log, so operators know what to update.

---

Trailing-Slash Regression

The old code had trim_end_matches('/'). With the full-URL approach, if an operator passes:

http://peer:5052/lean/v0/states/finalized/

they get a 404 from the server instead of a graceful error. The fix is trivial if desired:

let url = url.trim_end_matches('/');

Not a blocker, but a usability regression worth noting.

---

URL Validity Error Quality

There's no validation that the string is a parseable URL before making the request. If someone passes a typo like htp://peer:5052/..., they get a reqwest error that bubbles up as CheckpointSyncError::Http. That's adequate — reqwest errors are descriptive — but it's worth being aware this is the failure mode.

Counter to the Kimi review: Adding if !url.ends_with("/lean/v0/states/finalized") validation would contradict the PR's stated goal of supporting arbitrary checkpoint providers. Don't add it.

---

Code Quality

The actual code changes are clean:

• base_url → url: correct rename, more accurate.
• .get(&url) → .get(url): both compile (reqwest accepts &str or String); the new form is slightly more idiomatic.
• Removing trim_end_matches('/') and the format!() call is clean.

---

Docs

docs/checkpoint_sync.md:29 — the updated text is good and accurate. One minor improvement: the usage section (line 22) says Where <URL> is the address of a checkpoint source but doesn't explicitly say it must be the full endpoint path. A one-word addition would close the ambiguity:

Where `<URL>` is the **full endpoint URL** of a checkpoint source (e.g., `http://peer:5052/lean/v0/states/finalized`).

---

Test Coverage

The test plan has [ ] Verify checkpoint sync works end-to-end with the full URL in a devnet. That test is still unchecked. The unit tests in checkpoint_sync.rs test verify_checkpoint_state only — they don't exercise fetch_checkpoint_state or the URL construction path — so the end-to-end gap is real and should be completed before merge.

---

Bottom Line

The refactor logic is correct and the motivation is valid. The two things worth resolving before merge:

• Complete the devnet end-to-end test.
• Either add the trailing-slash trim back or document that trailing slashes are not supported.

Everything else is minor.

---

Automated review by Claude (Anthropic) · custom prompt