diff --git a/src/fetch/client.rs b/src/fetch/client.rs index bf981e7..730aa29 100644 --- a/src/fetch/client.rs +++ b/src/fetch/client.rs @@ -7,18 +7,14 @@ use tokio::sync::OnceCell; /// Timeout for both the relay connection attempt and each fetch query. const RELAY_TIMEOUT: Duration = Duration::from_secs(10); -/// 002 FR-014's "more than a couple of seconds" progress-indicator trigger (T130 -/// evidence): measured against the real default relay `wss://relay.mostro.network` with -/// a known real pubkey, 3 runs of a full connect+fetch cycle timed at 2.06s/1.96s/1.69s -/// wall time — normal single-relay operation sits right around 2 seconds. 3 seconds is +/// Measured against the real default relay `wss://relay.mostro.network` with a known +/// real pubkey: 3 runs of a full connect+fetch cycle timed at 2.06s/1.96s/1.69s wall time +/// -- normal single-relay operation sits right around 2 seconds. 3 seconds is /// comfortably above that normal variance while still low enough to catch a genuinely /// slow fetch. pub const PROGRESS_INDICATOR_THRESHOLD: Duration = Duration::from_secs(3); -/// PR 2 (T066-T069): the result of attempting to connect to every configured relay. -/// Principle VI's graceful-degradation rule and the Technical Context constraint ("one -/// failed relay among several that succeeded is a warning, not a failure; exit code 3 -/// requires all relays to fail") both read off this struct: `run()` treats +/// The result of attempting to connect to every configured relay. `run()` treats /// `connected_count == 0` as fatal (`AppError::RelaysUnreachable`) and a non-empty /// `failed` with `connected_count > 0` as warnings to print before continuing. #[derive(Debug, Clone, PartialEq, Eq)] @@ -26,13 +22,12 @@ pub struct RelayConnectionOutcome { pub connected_count: usize, pub connected_urls: Vec, pub failed: Vec, - /// PR 7a (002 FR-003): one entry per configured relay, in the user's originally - /// configured `--relays` order — `connected_urls` and `failed` alone cannot - /// reconstruct this once combined, since each is independently populated from - /// `Output`'s unordered success/failure sets and merging two separately-sorted - /// lists still groups every success before every failure regardless of where each - /// relay actually sat in `--relays`. `report::model`'s `fetch` section maps this - /// field directly into `RelaySummary`, one to one, with no merge logic of its own. + /// One entry per configured relay, in the user's originally configured `--relays` + /// order -- `connected_urls` and `failed` alone cannot reconstruct this once + /// combined, since each is independently populated from `Output`'s unordered + /// success/failure sets and merging two separately-sorted lists still groups every + /// success before every failure regardless of where each relay actually sat in + /// `--relays`. pub ordered: Vec, } @@ -51,11 +46,11 @@ pub struct RelayOutcome { } /// Pure interpretation of `Client::try_connect`'s per-relay `Output`, kept separate from -/// the real network call so it stays unit-testable without a socket (Testing Strategy: -/// "No network in tests"). `connected_urls`/`failed` are alphabetical here (`Output`'s -/// own sets/maps have no meaningful order of their own); `RelayEventSource::connect()` -/// separately builds `ordered` in the user's configured order once it has the original -/// `--relays` list to consult. +/// the real network call so it stays unit-testable without a socket. +/// `connected_urls`/`failed` are alphabetical here (`Output`'s own sets/maps have no +/// meaningful order of their own); `RelayEventSource::connect()` separately builds +/// `ordered` in the user's configured order once it has the original `--relays` list to +/// consult. fn interpret_connect_output(output: &Output<()>) -> RelayConnectionOutcome { let mut connected_urls: Vec = output.success.iter().map(|url| url.to_string()).collect(); @@ -79,13 +74,13 @@ fn interpret_connect_output(output: &Output<()>) -> RelayConnectionOutcome { } } -/// 002 FR-003: builds `RelayConnectionOutcome::ordered` — one `RelayOutcome` per entry -/// in `configured`, in that exact order. Every configured relay ends up in exactly one -/// of `connected_urls` or `failed` (registration failures are added to `failed` for -/// every configured relay that never even reached `try_connect`), so the `None` arm +/// Builds `RelayConnectionOutcome::ordered` -- one `RelayOutcome` per entry in +/// `configured`, in that exact order. Every configured relay ends up in exactly one of +/// `connected_urls` or `failed` (registration failures are added to `failed` for every +/// configured relay that never even reached `try_connect`), so the final `else` arm /// below is unreachable in practice; it reports an unknown-outcome relay rather than -/// panicking or silently dropping it, per Principle VI, in case that invariant is ever -/// violated by a future change. +/// panicking or silently dropping it, in case that invariant is ever violated by a +/// future change. fn build_ordered_outcomes( configured: &[String], connected_urls: &[String], @@ -119,58 +114,48 @@ fn build_ordered_outcomes( .collect() } -/// Seam introduced by PR 1 Step 0: the event-fetching surface `run()` depends on, so -/// production code and tests can supply different implementations (real relays vs. a -/// fixture replaying a captured event set). A generic bound, not `&dyn EventSource`, -/// since only two implementations exist and stable async-fn-in-traits needs no boxing -/// for static dispatch. `async fn` in a public trait is a deliberate, documented choice -/// (plan.md's Step 0 rationale): with only two call sites in this crate, the `Send` -/// bound the lint suggests adds nothing. +/// The event-fetching surface `run()` depends on, so production code and tests can +/// supply different implementations (real relays vs. a fixture replaying a captured +/// event set). A generic bound, not `&dyn EventSource`, since only two implementations +/// exist and stable async-fn-in-traits needs no boxing for static dispatch. /// -/// Two methods, not one: the original (pre-PR1) `main()` prints "Connected to relays" -/// only after relay setup (`add_relay`, which can fail) succeeds, and strictly before -/// issuing any fetch filters. A single `fetch()` call collapsing both phases -/// would either print that message too early (before a malformed relay's `add_relay` -/// failure surfaces) or require `run()` to reach into connection internals it has no -/// business owning. `connect()` isolates exactly the fallible relay-setup step so -/// `run()` can print its status line at the same logical point the original code did; -/// `fetch()` issues its filters afterward (PR 1's original two, expanded to PR 3's -/// four kind-scoped filters). +/// Two methods, not one: `run()` prints "Connected to relays" only after relay setup +/// (`add_relay`, which can fail) succeeds, and strictly before issuing any fetch +/// filters. A single `fetch()` call collapsing both phases would either print that +/// message too early (before a malformed relay's `add_relay` failure surfaces) or +/// require `run()` to reach into connection internals it has no business owning. +/// `connect()` isolates exactly the fallible relay-setup step; `fetch()` issues its +/// filters afterward. #[allow(async_fn_in_trait)] pub trait EventSource { - /// Establishes whatever connection this source needs before any event is fetched, - /// reporting per-relay success/failure (PR 2, T067) so `run()` can distinguish a - /// total outage from a partial one. A source with no real connection (e.g. a fixture - /// replaying canned events for a test) reports every configured relay as connected. + /// Reports per-relay success/failure so `run()` can distinguish a total outage from + /// a partial one. A source with no real connection (e.g. a fixture replaying canned + /// events for a test) reports every configured relay as connected. async fn connect(&self) -> Result; async fn fetch(&self, public_key: PublicKey) -> Result>; } /// Production `EventSource`: connects to the configured relays and issues the four -/// kind-scoped filters from `filters_summary.rs` (PR 3), chaining every result set into -/// one `Vec`. The connected `Client` is cached in `connect()` and reused by -/// `fetch()`, since the original code builds the client and relay connection once, then -/// queries against that same connection. +/// kind-scoped filters from `filters_summary.rs`, chaining every result set into one +/// `Vec`. The connected `Client` is cached in `connect()` and reused by `fetch()`. /// /// Generic over `R: ProgressReporter` (defaulting to `NoOpProgressReporter`) rather than /// depending on `report::progress::TerminalProgressReporter` directly: `fetch` depends -/// only on `models` and `error` per the constitution's dependency direction, so the -/// concrete terminal reporter is bound here only from the library/binary wiring root -/// (`main.rs`), never from within this module. +/// only on `models` and `error`, so the concrete terminal reporter is bound here only +/// from the library/binary wiring root (`main.rs`), never from within this module. pub struct RelayEventSource { pub relays: Vec, client: OnceCell, progress_reporter: R, - /// T137's test-only seam: when set, `fetch()` substitutes this controllable async - /// delay for the real relay-client call entirely, so `tests/metrics_end_to_end.rs` - /// can drive the real `fetch()` method's progress-reporter-racing logic under + /// Test-only seam: when set, `fetch()` substitutes this controllable async delay for + /// the real relay-client call entirely, so `tests/metrics_end_to_end.rs` can drive + /// the real `fetch()` method's progress-reporter-racing logic under /// `tokio::time::pause`/`advance` without a real relay connection or any real sleep. - /// Not `#[cfg(test)]`-gated: `cfg(test)` only applies when this crate itself is - /// compiled in test mode, which does not cover `tests/`'s separate compilation of - /// this crate as an ordinary dependency, so a literal `#[cfg(test)]` field would be - /// invisible there. No production call site (`main.rs`) ever sets this, so real runs - /// are structurally unaffected. + /// Not `#[cfg(test)]`-gated: that only applies when this crate itself is compiled in + /// test mode, which does not cover `tests/`'s separate compilation of this crate as + /// an ordinary dependency, so a literal `#[cfg(test)]` field would be invisible + /// there. No production call site (`main.rs`) ever sets this. test_fetch_delay: Option, } @@ -202,8 +187,7 @@ impl RelayEventSource { /// Races `task` against `PROGRESS_INDICATOR_THRESHOLD`, invoking the bound /// `ProgressReporter` at most once if the threshold elapses before `task` resolves, /// then continuing to await `task` itself. Shared by both the real relay-client path - /// and the test-only simulated-delay path, so the exact same racing logic is what - /// `tests/metrics_end_to_end.rs` exercises against the real `EventSource`. + /// and the test-only simulated-delay path. async fn await_with_progress( &self, mut task: tokio::task::JoinHandle>, @@ -237,16 +221,16 @@ impl RelayEventSource { .ok_or("RelayEventSource::fetch called before connect()")? .clone(); - // Spawned in its own task, same as before this PR: `nostr-relay-pool` 0.43.1's - // `fetch_events` constructs an internal `mpsc::channel` that Tokio panics on when - // every targeted relay's stream setup fails, and `tokio::spawn` isolates that - // panic into a catchable `JoinError` instead of unwinding past it. + // Spawned in its own task: `nostr-relay-pool` 0.43.1's `fetch_events` constructs + // an internal `mpsc::channel` that Tokio panics on when every targeted relay's + // stream setup fails, and `tokio::spawn` isolates that panic into a catchable + // `JoinError` instead of unwinding past it. let task = tokio::spawn(async move { client.fetch_events(filter, RELAY_TIMEOUT).await }); let fetched = self.await_with_progress(task).await?; Ok(fetched.into_iter().collect()) } - /// T137's simulated path: substitutes `tokio::time::sleep(delay)` for the real + /// Simulated path: substitutes `tokio::time::sleep(delay)` for the real /// `client.fetch_events(...)` call entirely, so the test needs no `Client` at all /// (and therefore no `connect()`, no real network) while still exercising the real /// `await_with_progress` racing logic above. @@ -263,16 +247,14 @@ impl EventSource for RelayEventSource { async fn connect(&self) -> Result { let client = Client::new(Keys::generate()); - // `add_relay` parses its argument into a canonical `RelayUrl` internally (via - // `TryIntoUrl`) before ever touching the pool, so `output.success`/`output.failed` - // (and therefore `connected_urls`/`failed`) always report relays in that - // canonical form (e.g. normalized trailing slash), not the user's raw `--relays` - // string. Canonicalizing here too, once, up front, keeps every later string - // comparison against `connected_urls`/`failed` correct even when a relay's raw - // and canonical forms differ syntactically but name the same relay — a URL that - // fails to parse at all (and so can never appear in the pool's output either) - // falls back to its raw string, which is exactly what `add_relay` itself would - // have failed on too. + // `add_relay` parses its argument into a canonical `RelayUrl` internally before + // ever touching the pool, so `output.success`/`output.failed` always report + // relays in that canonical form (e.g. normalized trailing slash), not the user's + // raw `--relays` string. Canonicalizing here too, once, up front, keeps every + // later string comparison against `connected_urls`/`failed` correct even when a + // relay's raw and canonical forms differ syntactically but name the same relay + // -- a URL that fails to parse at all falls back to its raw string, which is + // exactly what `add_relay` itself would have failed on too. let configured_relays: Vec = self .relays .iter() @@ -283,10 +265,11 @@ impl EventSource for RelayEventSource { }) .collect(); - // A relay URL that fails to register (e.g. malformed) is a connection failure like - // any other, not a distinct error class: it must feed the same graceful-degradation - // classification as a relay that registers but fails to connect, so that "all relays - // failed, for whatever reason" still maps to `RelaysUnreachable`, not `Other`. + // A relay URL that fails to register (e.g. malformed) is a connection failure + // like any other, not a distinct error class: it must feed the same + // classification as a relay that registers but fails to connect, so that "all + // relays failed, for whatever reason" still maps to `RelaysUnreachable`, not + // `Other`. let mut registration_failures: Vec = Vec::new(); for relay in &configured_relays { if let Err(error) = client.add_relay(relay.as_str()).await { @@ -311,10 +294,7 @@ impl EventSource for RelayEventSource { } async fn fetch(&self, public_key: PublicKey) -> Result> { - // PR 3 (T097/T098): the four kind-scoped filters per 001 FR-015 — dev-fee - // (8383), order (38383), instance-status (38385), and dispute (38386) — - // replacing PR 1's original two-filter query. T135-138 (002 FR-014): each - // filter's fetch races against `PROGRESS_INDICATOR_THRESHOLD` in + // Each filter's fetch races against `PROGRESS_INDICATOR_THRESHOLD` in // `await_with_progress`, invoking the bound `ProgressReporter` at most once if a // fetch runs past it. `test_fetch_delay`, when set, substitutes a controllable // delay for the real relay-client call entirely (see its field doc). @@ -347,10 +327,6 @@ mod tests { assert_eq!(canonical, "wss://relay.example"); } - /// 002 FR-003: `fetch.relays[]` preserves the user's originally configured - /// `--relays` order, not an alphabetical one — success and failure entries are - /// interleaved in this test specifically to prove the reorder doesn't just group by - /// outcome first. #[test] fn build_ordered_outcomes_matches_the_configured_relays_order_across_success_and_failure() { // Interleaved on purpose: success, failure, success — proves the merge follows @@ -390,9 +366,8 @@ mod tests { assert!(ordered[2].succeeded); } - /// The plan's constraint ("`unwrap`/`expect` are permitted only in tests") rules out - /// panicking when `fetch()` is called before `connect()` — an internal misuse that must - /// still surface as an ordinary `AppError`, not abort the process. + /// Calling `fetch()` before `connect()` is an internal misuse that must still + /// surface as an ordinary `AppError`, not abort the process. #[tokio::test] async fn fetch_before_connect_returns_an_error_instead_of_panicking() { let source = RelayEventSource::new(vec!["wss://relay.example".to_string()]); diff --git a/src/fetch/filters_summary.rs b/src/fetch/filters_summary.rs index c6675b6..456e5f6 100644 --- a/src/fetch/filters_summary.rs +++ b/src/fetch/filters_summary.rs @@ -1,11 +1,10 @@ -//! Per-kind Nostr filters for a node's four scoped event kinds (001 FR-015), and -//! `RelayFetchOutcome` — the fetch-result summary those four kinds produce (002 FR-003). -//! Each `RelayFetchOutcome` field follows its own semantic rule rather than a single -//! generic id-dedup helper: `dev_fee_events`/`order_events` are raw event-id dedup -//! counts, `unique_orders` applies `models::order`'s full qualifying-order procedure, -//! `dispute_events` applies `models::dispute`'s dedup-by-`d`-tag classification, and -//! `instance_status_found` reflects `models::instance_status`'s actual valid-instance -//! selection. +//! Per-kind Nostr filters for a node's four scoped event kinds, and `RelayFetchOutcome` +//! -- the fetch-result summary those four kinds produce. Each `RelayFetchOutcome` field +//! follows its own semantic rule rather than a single generic id-dedup helper: +//! `dev_fee_events`/`order_events` are raw event-id dedup counts, `unique_orders` +//! applies `models::order`'s full qualifying-order procedure, `dispute_events` applies +//! `models::dispute`'s dedup-by-`d`-tag classification, and `instance_status_found` +//! reflects `models::instance_status`'s actual valid-instance selection. use crate::models::core::scope_events_to_node; use crate::models::dispute::DisputeAggregate; @@ -32,9 +31,9 @@ fn scoped_filter(public_key: PublicKey, kind: u16, expected_z: &str) -> Filter { .custom_tag(SingleLetterTag::lowercase(Alphabet::Y), "mostro") } -/// FR-015: the four kind-scoped filters a relay fetch issues for one node's report — -/// dev-fee (`8383`), order (`38383`), instance-status (`38385`), and dispute (`38386`), -/// each scoped to the node's own pubkey as author, its kind's expected `z` value, and +/// The four kind-scoped filters a relay fetch issues for one node's report -- dev-fee +/// (`8383`), order (`38383`), instance-status (`38385`), and dispute (`38386`), each +/// scoped to the node's own pubkey as author, its kind's expected `z` value, and /// `y=mostro`. pub fn build_scoped_filters(public_key: PublicKey) -> [Filter; 4] { [ @@ -46,8 +45,8 @@ pub fn build_scoped_filters(public_key: PublicKey) -> [Filter; 4] { } /// A node's raw fetched events (all four scoped kinds, concatenated), routed into -/// per-kind buckets and scoped to the node's own author/z/y per FR-015. Events of any -/// other kind, or that fail scoping for their kind, are silently excluded (FR-013). +/// per-kind buckets and scoped to the node's own author/z/y. Events of any other kind, +/// or that fail scoping for their kind, are silently excluded. #[derive(Debug, Clone, Default, PartialEq, Eq)] pub struct PartitionedEvents { pub dev_fee_events: Vec, @@ -80,20 +79,19 @@ pub fn partition_scoped_events(events: Vec, node_pubkey: &PublicKey) -> P } } -/// 002 FR-003's per-kind fetch-result summary. Each field follows its own semantic rule, -/// not one generic id-dedup helper: +/// Per-kind fetch-result summary. Each field follows its own semantic rule, not one +/// generic id-dedup helper: /// - `dev_fee_events`/`order_events`: raw event-id dedup count, guarding against the same /// relay-delivered event being double-counted when multiple relays return it. -/// - `unique_orders`: `models::order`'s full qualifying-order procedure (001 FR-002). +/// - `unique_orders`: `models::order`'s full qualifying-order procedure. /// - `has_valid_orders`: whether at least one deduplicated order carries a recognized /// `s` status, independent of whether that status is `success` — an order missing its -/// `d` tag, or missing/carrying an unrecognized `s` value entirely, is malformed -/// (FR-013) and must not count as usable data on its own, even though it still -/// increments the raw `order_events` count above. -/// - `dispute_events`: `models::dispute`'s dedup-by-`d`-tag count (001 FR-006). +/// `d` tag, or missing/carrying an unrecognized `s` value entirely, is malformed and +/// must not count as usable data on its own, even though it still increments the raw +/// `order_events` count above. +/// - `dispute_events`: `models::dispute`'s dedup-by-`d`-tag count. /// - `instance_status_found`: whether `models::instance_status`'s actual valid-instance -/// selection succeeded (001 FR-012), not merely whether a kind-`38385` event was -/// fetched. +/// selection succeeded, not merely whether a kind-`38385` event was fetched. #[derive(Debug, Clone, PartialEq, Eq)] pub struct RelayFetchOutcome { pub dev_fee_events: usize, @@ -105,11 +103,11 @@ pub struct RelayFetchOutcome { } impl RelayFetchOutcome { - /// 002 FR-019 exit code `4`'s gate: true only when none of the four scoped kinds - /// yielded any usable data at all. Orders are gated on `has_valid_orders`, not the - /// raw `order_events` count: an order fetched but missing its `d` tag is discarded - /// entirely by `models::order`'s qualifying-order procedure and must not, on its - /// own, count as "this node has usable order data." + /// True only when none of the four scoped kinds yielded any usable data at all. + /// Orders are gated on `has_valid_orders`, not the raw `order_events` count: an + /// order fetched but missing its `d` tag is discarded entirely by `models::order`'s + /// qualifying-order procedure and must not, on its own, count as "this node has + /// usable order data." pub fn has_no_usable_events(&self) -> bool { self.dev_fee_events == 0 && !self.has_valid_orders @@ -129,10 +127,9 @@ pub(crate) fn dedup_by_event_id_count(events: &[Event]) -> usize { /// Computes a `RelayFetchOutcome` from a node's already-partitioned, already-scoped /// event buckets. Takes already-computed dev-fee/order counts, the `OrderAggregate`, /// the `InstanceStatusAggregate`, and the `DisputeAggregate` rather than raw event -/// vectors for those: `run()` needs the deduplicated dev-fee count, the full order -/// aggregate, the full instance-status aggregate (PR 6's bond policy), and the full -/// dispute aggregate for its own reporting regardless (stats included, PR 5/PR 6), so -/// recomputing any of them here would rescan the same event set for no new information. +/// vectors for those: `run()` needs each of these aggregates for its own reporting +/// regardless, so recomputing any of them here would rescan the same event set for no +/// new information. pub fn compute_relay_fetch_outcome( dev_fee_event_count: usize, order_event_count: usize, @@ -191,9 +188,9 @@ mod tests { assert_eq!(filters.len(), 4); } - /// Regression: a filter-count assertion alone would still pass for four wrong or - /// duplicate filters. Assert each filter's actual kind, author, and expected `z` - /// value, per FR-015. + /// A filter-count assertion alone would still pass for four wrong or duplicate + /// filters, so this asserts each filter's actual kind, author, and expected `z` + /// value. #[test] fn build_scoped_filters_each_filter_has_the_expected_kind_author_and_z_value() { let public_key = Keys::generate().public_key(); @@ -281,9 +278,9 @@ mod tests { assert!(outcome.has_valid_orders); } - /// Regression: an order missing its `d` tag is discarded entirely by the qualifying- - /// order procedure (FR-013), so it must not count as usable order data even though it - /// still increments the raw `order_events` count. + /// An order missing its `d` tag is discarded entirely by the qualifying-order + /// procedure, so it must not count as usable order data even though it still + /// increments the raw `order_events` count. #[test] fn compute_relay_fetch_outcome_has_valid_orders_is_false_for_a_d_tagless_order() { let node_pubkey = Keys::generate().public_key(); @@ -303,10 +300,10 @@ mod tests { assert!(outcome.has_no_usable_events()); } - /// Regression: an order with a `d` tag but no `s` tag at all survives `d`-tag dedup - /// (it has a valid dedup key), but real Mostro order events always publish a status - /// — a missing one is malformed data (FR-013), not evidence of order history, and - /// must not count toward `has_valid_orders`. + /// An order with a `d` tag but no `s` tag at all survives `d`-tag dedup (it has a + /// valid dedup key), but real Mostro order events always publish a status -- a + /// missing one is malformed data, not evidence of order history, and must not count + /// toward `has_valid_orders`. #[test] fn compute_relay_fetch_outcome_has_valid_orders_is_false_for_an_order_missing_its_s_tag() { let node_pubkey = Keys::generate().public_key();