Skip to content

chore: remove stale FIXME comments on snapshot URL - #276

Open
teyrebaz33 wants to merge 1 commit into
circlefin:mainfrom
teyrebaz33:chore/remove-stale-snapshot-url-fixme
Open

chore: remove stale FIXME comments on snapshot URL#276
teyrebaz33 wants to merge 1 commit into
circlefin:mainfrom
teyrebaz33:chore/remove-stale-snapshot-url-fixme

Conversation

@teyrebaz33

Copy link
Copy Markdown

Fixes #275

Both FIXMEs in crates/execution-config/src/defaults.rs read 'Update this to the actual snapshot URL', but the URLs are already the real, working snapshot endpoints — confirmed via crates/snapshots/src/download.rs using the same host without any such disclaimer, and docs/running-an-arc-node.md documenting it directly to users with no caveat.

Per @osr21's review on #275: verified GET https://snapshots.arc.network/5042002/latest.txt returns 200 with a live testnet snapshot pointer, confirming the base URL is correct and object keys (not the bare prefix) are what resolve.

Replaced both FIXMEs with a note on the base-prefix/latest.txt pattern, so a future curl-the-bare-URL-get-a-404 doesn't lead someone to re-add a FIXME.

Left the (testnet)/(devnet) label-in-URL-string nit on available_snapshots untouched — it's display-only today per the review, and splitting it out felt like a separate, unrelated change to bundle into a stale-comment cleanup.

Both FIXMEs read 'Update this to the actual snapshot URL', but the
URLs are already the real, working snapshot endpoints — confirmed via
crates/snapshots/src/download.rs using the same host without any such
disclaimer, and docs/running-an-arc-node.md documenting it directly to
users with no caveat.

Replaced with a note on the base-prefix/latest.txt object-store
pattern per @osr21's review on circlefin#275, to pre-empt someone re-adding a
FIXME after curling the bare base URL and getting a 404.

Fixes circlefin#275
@teyrebaz33

Copy link
Copy Markdown
Author

Looked into splitting the label per your nit — turns out there's no clean way to do it. Traced available_snapshots's only consumer in reth (DownloadDefaults::long_help() at the pinned d6324d6 rev): it's pushed straight into --help text as a plain string, nothing parses or fetches these as URLs anywhere in reth's own code. The field type itself is Vec<Cow<'static, str>> — reth doesn't expose a structured (url, label) pairing to split into, so 'splitting the label' would mean either changing reth's own API (out of scope here) or just reformatting the string, which wouldn't actually remove the 'looks like a URL but isn't' property either way. Given the risk is conditional on a consumer that doesn't currently exist, leaving it as-is for now rather than reformatting something that isn't broken.

@osr21

osr21 commented Aug 21, 2026

Copy link
Copy Markdown

LGTM — this matches the #275 recommendation precisely.

Change is correct and complete:

  • Both stale FIXMEs replaced with the base-prefix/latest.txt note (L26 and L39). ✅
  • DEFAULT_DOWNLOAD_URL (L28) and the available_snapshots entries left intact — the URLs were never the problem, only the comments. ✅
  • These are // line comments, not /// doc comments, so there's zero rustdoc/cargo doc impact — nothing for the incoming docs: clarify RPC behavior and validate rustdoc #273 rust-doc job to choke on. ✅
  • Scoping call is right: leaving the (testnet)/ (devnet) label-in-URL nit out keeps a stale-comment cleanup focused. It's display-only today, so bundling an unrelated refactor in here would only muddy the diff. Good judgment.

Merge status (head af6f6ff): mergeable=true (no conflicts) but mergeable_state=blocked. No check is failing — the check-runs are success/skipped (nothing red), and the legacy combined status is empty (total_count=0), so any "pending" shown there is just the no-statuses default, same artifact you saw on #274. One thing to note for whoever merges: the heavier Rust jobs (build/clippy/doc) show as skipped rather than green on this commit, so this .rs change didn't actually exercise the full suite. Risk is negligible since it's comment-only, but a maintainer may want to confirm the build suite runs (or is intentionally path-filtered off) before merge. Net: the block is maintainer-side (required review / branch protection), not anything in the PR.

Ultra-minor, non-blocking: the two replacement comments are worded slightly differently — L26 "Base prefix for the object store; the resolvable pointer is <base>/latest.txt." vs L39 "Base prefix; the resolvable pointer is <base>/latest.txt." Unify if you care; harmless either way.

Nice, clean fix — resolves #275 as intended.

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.

chore: stale FIXME comments on snapshot URL in execution-config defaults (already the real URL)

2 participants