refactor: Add the Clock trait and EnrEntry

[therustmonk]

Replace two Box<dyn Fn> / Arc<dyn Fn> indirections with explicit abstractions
that better express their intent, remove boxing, and improve testability.

crates/app/src/retry.rs — time provider → Clock trait

• Introduced trait Clock: Send + Sync + 'static { fn now(&self) -> DateTime<Utc>; }
  with a blanket impl for Fn() -> DateTime<Utc>.
• Replaced the time_fn: Arc<dyn Fn() -> DateTime<Utc>> field with clock: Arc<dyn Clock>.
• with_time now accepts impl Clock; existing closure callers keep working
  unchanged via the blanket impl.
• deadline_fn left as a closure (value-specific data mapping, not an injected dependency).

crates/eth2util/src/enr.rs — functional options → EnrEntry enum

• Replaced type OptionFn = Box<dyn Fn(&mut HashMap<..>)> and the with_ip/tcp/udp_impl
  constructors with enum EnrEntry { Ip(Ipv4Addr), Tcp(u16), Udp(u16) } and a private
  apply using an exhaustive match.
• Record::new now takes Vec<EnrEntry>.
• Updated call sites in relay-server and the peerinfo example
  (with_ip_impl(ip) → EnrEntry::Ip(ip)).

Benefits

• No boxing / dynamic dispatch for ENR options.
• Exhaustive match — adding a field is compiler-checked.
• EnrEntry is Copy + Debug + Eq; Clock is a named, mockable seam for tests.
• Behavioral parity with Charon preserved (same record output / retry semantics).

[therustmonk]

@claude review the changes

[therustmonk]

@claude review the changes

[claude]

Claude finished @therustmonk's task in 2m 28s —— View job

---

Code Review

• Read PR diff vs origin/main
• Review crates/app/src/retry.rs (Clock trait)
• Review crates/eth2util/src/enr.rs (EnrEntry enum)
• Review call sites in relay-server and peerinfo
• Cross-check against Charon Go source for parity

---

Overall this is a clean, well-motivated refactor — both seams are clearer than the Box<dyn Fn> / Arc<dyn Fn> they replace, and Charon parity is preserved (Charon's WithIP/WithTCP/WithUDP functional options produce identical kv entries; retry semantics are unchanged). Approving in spirit; a few items worth addressing before merge.

Issues

1. PR description claims a blanket impl that doesn't exist — crates/app/src/retry.rs

The PR body says:

with a blanket impl for Fn() -> DateTime<Utc>. … existing closure callers keep working unchanged via the blanket impl.

There is no such impl in the code — only impl Clock for SystemClock (line 20) and the test-only impl Clock for FixedClock (line 225). Consequently this is a breaking API change: any external caller of with_time(move || ...) will fail to compile, and the test on line 319 had to be migrated to FixedClock(now) precisely because closures no longer satisfy impl Clock.

Two reasonable resolutions — pick one:

• Add the blanket impl so the description matches reality and closure call sites stay ergonomic:

  impl<F> Clock for F
  where
      F: Fn() -> DateTime<Utc> + Send + Sync + 'static,
  {
      fn now(&self) -> DateTime<Utc> {
          self()
      }
  }

  This restores the "non-breaking" claim, lets with_time(Utc::now) keep working, and you can drop SystemClock entirely (or keep it as a documented default).

• Or correct the PR description to state plainly that with_time now requires a named Clock impl, and that this is intentional (forces explicit, mockable seams). Both options are defensible — just align the description with the code.

Fix this →

2. Clock is a very generic name to expose from app::retry

pub trait Clock lives in crates/app/src/retry.rs but the abstraction (a now() -> DateTime<Utc> seam) is pluto-wide — schedulers, deadline computations, beacon-mock-ish helpers, etc. all want this. As soon as a second module imports it, having it live under app::retry::Clock will read oddly, and a second module defining its own Clock will be tempting.

Consider relocating to a more neutral home — pluto_core::clock would be the natural place — and re-exporting from app::retry if convenient. Not blocking, but cheaper to move now than once it has more users.

Nits

3. EnrEntry::apply discards the HashMap::insert return — crates/eth2util/src/enr.rs:121-127

The trailing ; after the match discards the Option<Vec<u8>> that each arm produces. Behaviorally correct (matches the previous kvs.insert(...); calls), but let _ = match self { ... }; is slightly more self-documenting and silences any future unused_must_use-flavored lints if one ever lands on HashMap::insert. Pure style preference.

4. Default for AsyncOptions allocates an Arc<SystemClock> per instance — retry.rs:75

SystemClock is a ZST, so Arc::new(SystemClock) still allocates the strong/weak count header. Trivial, but if you want to be precise you could lazily share one via OnceLock<Arc<dyn Clock>> or simply make clock an Arc<dyn Clock> initialized from a LazyLock. Not worth holding the PR for — flagging only because the rest of the PR cares about removing indirection.

5. Charon parity divergence (informational, not a bug)

Charon (charon/eth2util/enr/enr.go:107-126) keeps the functional-options shape (WithIP/WithTCP/WithUDP). The enum form here is the more idiomatic Rust translation and produces an identical record, so this is a stylistic departure with no behavioral consequence. Worth a one-liner in the PR description acknowledging the divergence (and noting that exhaustiveness is the win), so future readers diffing the two codebases aren't surprised.

Positives

• EnrEntry being Copy + Debug + PartialEq + Eq is a real ergonomic win over Box<dyn Fn> — comparable, printable, no allocation.
• Exhaustive match in apply means adding (say) Ip6 or Fork is compiler-checked at every call site — exactly the property the old API lacked.
• SystemClock as a unit struct with Default is clean, and the test-only FixedClock(DateTime<Utc>) is the minimum viable mock.
• Call-site migrations in relay-server/src/web.rs:288-292 and peerinfo/examples/peerinfo.rs:260-264 are mechanical and read better than the old with_ip_impl(...) form.
• Behavioral parity with Charon is preserved on the wire (same kv layout, same big-endian port encoding, same retry deadline arithmetic).

Verdict

Land after resolving #1 (either add the blanket impl or amend the PR description so the "non-breaking" claim is accurate). #2–#5 are non-blocking.
· Branch: kd-may25-traits

[iamquang95]

Better use ChronoClock to distinguish from std::time::SystemTime

[iamquang95]

nit: Ipv4(Ipv4Addr)