Relay hints: nprofile parsing, hint routing, NIP-10 author pubkeys#1234
Relay hints: nprofile parsing, hint routing, NIP-10 author pubkeys#1234alltheseas wants to merge 3 commits into
Conversation
|
I'm working on an outbox pool which will solve alot of the same things, but in a more unified way. I think there is some code that could be useful here. Let's revisit after i finish |
📝 WalkthroughWalkthroughAdds nprofile (NIP-19) parsing with relay-hint extraction, exposes ParsedNprofile, and routes unknown-ID lookups to hinted relays when available; also augments reply and quote tag construction to include author pubkey references. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/Publisher
participant API as OneshotApi
participant Parser as ParsedNprofile Parser
participant Pool as RelayPool
participant HR as Hint Relays
participant DR as Default Relays
rect rgba(100, 200, 150, 0.5)
Note over User,DR: Existing flow (unhinted)
User->>API: oneshot(filters)
API->>Pool: RelayUrlPkgs(selected_read_relays)
Pool->>DR: send filters
DR-->>User: responses
end
rect rgba(150, 150, 200, 0.5)
Note over User,HR: New flow (hinted)
User->>API: unknown_id_send(ids)
API->>Parser: parse ids → ParsedNprofile (pubkey + relays)
Parser-->>API: relay hints (optional)
alt Hinted IDs
API->>Pool: oneshot_to_relays(filters, hint_relays)
Pool->>HR: send filters
HR-->>User: responses
end
alt Unhinted IDs
API->>Pool: oneshot(filters)
Pool->>DR: send filters
DR-->>User: responses
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
crates/enostr/src/relay/pool.rs (2)
184-190: Redundant normalization in relay_status inner loop.The pool URLs are already canonicalized via
add_url, butrelay_statusre-canonicalizes each pool relay URL on every lookup. This creates O(n) allocations per call.🔎 Proposed optimization
pub fn relay_status(&self, url: &str) -> Option<RelayStatus> { let normalized = Self::canonicalize_url(url.to_string()); self.relays .iter() - .find(|r| Self::canonicalize_url(r.url().to_string()) == normalized) + .find(|r| r.url() == normalized) .map(|r| r.status()) }
325-335: Consider logging when hint relays are skipped.When
subscribe_tosilently skips relays not in the pool, debugging hint routing issues becomes difficult. A trace log would help operators understand why hints aren't being used.🔎 Proposed enhancement
for relay in &mut self.relays { // Pool URLs are already canonicalized via add_url if !urls.contains(relay.url()) { + tracing::trace!("subscribe_to: skipping relay {} (not in target set)", relay.url()); continue; }crates/notedeck_columns/src/post.rs (1)
1373-1439: Good test coverage for NIP-10 relay hints.The tests properly verify the e-tag and q-tag structure including relay hints and author pubkeys. The test at line 1548 documents that passing
Noneresults in an empty string relay hint.Consider adding a test case for replies within an existing thread (where a root tag already exists) to verify the root e-tag relay hint extraction works correctly.
crates/notedeck/src/unknowns.rs (1)
173-199: Consider avoiding clone in the hot path.Line 183 clones the
HashSet<RelayUrl>for each ready ID. While this works correctly, if there are many IDs with large hint sets, this could cause allocation overhead.🔎 Suggested optimization using drain or swap
pub fn check_grace_period_timeouts(&mut self) -> Vec<(UnknownId, HashSet<RelayUrl>)> { if self.grace_period_ids.is_empty() { return Vec::new(); } let now = Instant::now(); - let ready: Vec<_> = self - .grace_period_ids - .iter() - .filter(|(_, (_, added_at))| now.duration_since(*added_at) >= GRACE_PERIOD_TIMEOUT) - .map(|(id, (hints, _))| (*id, hints.clone())) - .collect(); - - // Remove ready IDs from grace period tracking - for (id, _) in &ready { - self.grace_period_ids.remove(id); - } + let ready_keys: Vec<_> = self + .grace_period_ids + .iter() + .filter(|(_, (_, added_at))| now.duration_since(*added_at) >= GRACE_PERIOD_TIMEOUT) + .map(|(id, _)| *id) + .collect(); + + let ready: Vec<_> = ready_keys + .into_iter() + .filter_map(|id| self.grace_period_ids.remove(&id).map(|(hints, _)| (id, hints))) + .collect(); if !ready.is_empty() {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
CHANGELOG.mdcrates/enostr/src/lib.rscrates/enostr/src/pubkey.rscrates/enostr/src/relay/pool.rscrates/notedeck/src/app.rscrates/notedeck/src/unknowns.rscrates/notedeck_columns/src/post.rscrates/notedeck_columns/src/repost.rscrates/notedeck_columns/src/ui/note/post.rs
🧰 Additional context used
📓 Path-based instructions (2)
crates/notedeck*/src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
crates/notedeck*/src/**/*.rs: UseAppContextto access mutable handles to services (database, caches, relay pool, account state, localization, settings, wallet) rather than accessing global state
Never use Mutexes in UI paths; the render loop must never block. UseRc<RefCell<>>for single-threaded interior mutability,Arc<tokio::sync::RwLock<>>for cross-thread sharing, andpoll_promise::Promisefor async results
Wrap async work inpoll_promise::Promise, check withpromise.ready()orpromise.ready_mut()each frame—never block the render loop
Usetokio::sync::RwLockinstead ofArc<Mutex<>>for cross-thread sharing in Notedeck
Persist state viaTimedSerializer::try_saveto avoid blocking the frame; batch mutations withSettingsHandler::update_batch
Wrap user-facing strings withtr!ortr_plural!macros for localization and runpython3 scripts/export_source_strings.pyafter changing strings
UseJobPoolfor CPU-bound work and return results viatokio::sync::oneshotwrapped in Promises; usetokio::spawn()for network I/O and relay sync
Mark performance-critical functions with#[profiling::function]for visibility in the puffin profiler
Usetracingmacros for structured logging andprofilingscopes where hot paths exist
Prefer early returns and guard clauses over deeply nested conditionals; simplify control flow by exiting early instead of wrapping logic in multiple layers ofifstatements (Nevernesting principle)
Global variables are not allowed in this codebase, even if they are thread local; state should be managed in a struct that is passed in as reference
Ensure docstring coverage for any code added or modified
Avoid Mutexes in Notedeck code; preferpoll_promise::Promisefor async results,Rc<RefCell<>>for single-threaded interior mutability, ortokio::sync::RwLockwhen cross-thread sharing is truly necessary
Never block the render loop; usePromise::ready()for non-blocking result checks. Offload CPU-heavy work to...
Files:
crates/notedeck_columns/src/ui/note/post.rscrates/notedeck/src/app.rscrates/notedeck_columns/src/post.rscrates/notedeck_columns/src/repost.rscrates/notedeck/src/unknowns.rs
crates/notedeck*/src/app.rs
📄 CodeRabbit inference engine (AGENTS.md)
Implement the
Apptrait withupdate(&mut self, &mut AppContext, &mut egui::Ui) -> AppResponseto drive egui rendering and signal high-level actions
Files:
crates/notedeck/src/app.rs
🧠 Learnings (9)
📚 Learning: 2026-01-05T20:25:35.921Z
Learnt from: CR
Repo: damus-io/notedeck PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T20:25:35.921Z
Learning: Applies to crates/notedeck*/src/**/*.rs : Mark performance-critical functions with `#[profiling::function]` for visibility in the puffin profiler
Applied to files:
crates/enostr/src/lib.rs
📚 Learning: 2026-01-05T20:25:35.921Z
Learnt from: CR
Repo: damus-io/notedeck PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T20:25:35.921Z
Learning: Applies to crates/notedeck*/src/**/*.rs : Persist state via `TimedSerializer::try_save` to avoid blocking the frame; batch mutations with `SettingsHandler::update_batch`
Applied to files:
crates/notedeck/src/app.rscrates/notedeck/src/unknowns.rs
📚 Learning: 2026-01-05T20:25:35.921Z
Learnt from: CR
Repo: damus-io/notedeck PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T20:25:35.921Z
Learning: Applies to crates/notedeck*/src/**/*.rs : Use `JobPool` for CPU-bound work and return results via `tokio::sync::oneshot` wrapped in Promises; use `tokio::spawn()` for network I/O and relay sync
Applied to files:
crates/notedeck/src/app.rs
📚 Learning: 2026-01-05T20:25:35.921Z
Learnt from: CR
Repo: damus-io/notedeck PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T20:25:35.921Z
Learning: Applies to crates/notedeck*/src/**/*.rs : Use `AppContext` to access mutable handles to services (database, caches, relay pool, account state, localization, settings, wallet) rather than accessing global state
Applied to files:
crates/notedeck/src/app.rs
📚 Learning: 2026-01-05T20:25:35.921Z
Learnt from: CR
Repo: damus-io/notedeck PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T20:25:35.921Z
Learning: Applies to crates/notedeck*/src/**/*.rs : Avoid Mutexes in Notedeck code; prefer `poll_promise::Promise` for async results, `Rc<RefCell<>>` for single-threaded interior mutability, or `tokio::sync::RwLock` when cross-thread sharing is truly necessary
Applied to files:
crates/notedeck/src/app.rs
📚 Learning: 2026-01-05T20:25:35.921Z
Learnt from: CR
Repo: damus-io/notedeck PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T20:25:35.921Z
Learning: Applies to crates/notedeck*/src/lib.rs : Use strict Rust 2021 edition; ensure edition-lints are strict and clippy `disallowed_methods` is denied at crate root to enforce API hygiene
Applied to files:
crates/notedeck/src/app.rs
📚 Learning: 2026-01-05T20:25:35.921Z
Learnt from: CR
Repo: damus-io/notedeck PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T20:25:35.921Z
Learning: Applies to crates/notedeck*/src/**/*.rs : Never block the render loop; use `Promise::ready()` for non-blocking result checks. Offload CPU-heavy work to `JobPool` or `tokio::spawn()`, returning results via channels or Promises
Applied to files:
crates/notedeck/src/app.rs
📚 Learning: 2026-01-05T20:25:35.921Z
Learnt from: CR
Repo: damus-io/notedeck PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T20:25:35.921Z
Learning: Applies to crates/notedeck*/src/**/*.rs : Wrap async work in `poll_promise::Promise`, check with `promise.ready()` or `promise.ready_mut()` each frame—never block the render loop
Applied to files:
crates/notedeck/src/app.rs
📚 Learning: 2026-01-05T20:25:35.921Z
Learnt from: CR
Repo: damus-io/notedeck PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T20:25:35.921Z
Learning: Applies to crates/notedeck*/src/**/*.rs : Tests should live alongside modules (e.g., in test submodules), often using `#[tokio::test]` when async behavior is involved
Applied to files:
crates/enostr/src/relay/pool.rs
🧬 Code graph analysis (4)
crates/notedeck/src/app.rs (1)
crates/notedeck/src/unknowns.rs (3)
send_grace_period_ids(710-765)new(27-29)new(52-54)
crates/enostr/src/relay/pool.rs (1)
crates/enostr/src/client/message.rs (1)
req(41-43)
crates/notedeck_columns/src/post.rs (1)
crates/enostr/src/pubkey.rs (2)
hex(63-65)hex(87-89)
crates/notedeck/src/unknowns.rs (1)
crates/enostr/src/relay/pool.rs (3)
url(48-53)canonicalize_url(408-413)relay_status(184-190)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Test (macOS) / run
- GitHub Check: Test (Linux) / run
- GitHub Check: Test (Windows) / run
- GitHub Check: Check (android)
🔇 Additional comments (22)
crates/enostr/src/relay/pool.rs (1)
474-521: Good test coverage for URL normalization.The tests verify trailing slash normalization, various URL formats, and invalid URL handling. The test for
subscribe_toURL normalization correctly validates the URL set construction logic.crates/enostr/src/pubkey.rs (5)
15-40: Well-designed ParsedNprofile struct with clear constructors.The struct cleanly encapsulates the pubkey and relay hints with good documentation. The
without_relaysconstructor avoids allocation when no hints are available.
164-183: Solid fallback strategy for nprofile parsing.The two-phase approach (nostr crate → manual TLV) provides graceful degradation when relay URLs are malformed, ensuring the pubkey can still be extracted.
201-212: TLV length field is u8, limiting entry size to 255 bytes.This is correct per the NIP-19 TLV format where the length is a single byte. The bounds check at line 207 correctly prevents out-of-bounds reads.
223-232: Relay URL validation is appropriately lenient.The case-insensitive scheme check correctly handles variations like
WSS://orWss://. Invalid URLs are silently skipped, which aligns with graceful degradation goals.
322-404: Comprehensive test coverage for nprofile parsing.Tests verify:
- Parsing with relay hints
- Fallback for different formats (hex, npub)
- Manual TLV extraction
- Case-insensitive scheme handling
- Constructor behavior
crates/enostr/src/lib.rs (1)
18-18: Public API correctly exports ParsedNprofile.The new type is properly re-exported alongside existing pubkey types, maintaining API consistency.
crates/notedeck_columns/src/ui/note/post.rs (2)
81-86: Relay hint extraction follows NIP-10 correctly.Using the first relay where the note was seen as the hint is appropriate. The
relays(txn).next()pattern returnsOption<&str>, which matches the expectedrelay_hintparameter type.
88-93: Quote relay hint handling mirrors reply logic.Consistent approach for both replies and quotes, using the same relay hint extraction pattern.
crates/notedeck/src/app.rs (1)
473-483: Non-blocking hint routing lifecycle integration.The new branches correctly:
- Check preconditions before expensive operations (
has_grace_period_ids(),has_pending_hints())- Create Transaction only when needed (inside conditional)
- Follow the codebase pattern of using
AppContextfor service accessThe Transaction creation is fast (just opens a read view) and doesn't block the render loop. As per coding guidelines, this correctly avoids Mutexes and uses proper service access patterns.
CHANGELOG.md (1)
1-13: Well-structured changelog entries.The entries accurately document the relay hint routing features and fixes. Format is consistent with existing entries.
crates/notedeck_columns/src/repost.rs (2)
28-38: Relay hint derivation correctly prioritizes seen relay.The logic properly implements NIP-10 by preferring the relay where the note was actually seen, with a sensible fallback to the first connected relay. The ownership chain (
map(|s| s.to_owned())) is necessary sincerelays()returns borrowed strings.
44-57: Repost event structure follows NIP-18.The e-tag correctly includes the relay hint, and the p-tag properly references the original author's pubkey. The note builder chain is clear and maintainable.
crates/notedeck_columns/src/post.rs (3)
87-97: LGTM!The method signature update with
relay_hint: Option<&str>is well-designed. The docstring clearly explains the NIP-10 relay hint purpose.
103-134: Implementation correctly follows NIP-10 with documented limitation.The e-tag construction follows NIP-10 format
["e", <event-id>, <relay-url>, <marker>, <pubkey>]. The asymmetry where the reply e-tag includes the author pubkey (line 120) but the root e-tag doesn't is correctly documented with a TODO referencing nostrdb#113.One minor observation: when
relay_hintisNone, an empty string is written to the tag (line 99, 118, 130). This is valid per NIP-10, but consider whether omitting the relay field entirely would be preferable for cleaner tags when no hint is available.
175-207: LGTM!The q-tag construction correctly includes the relay hint and author pubkey in the expected positions. The implementation is consistent with the to_reply changes.
crates/notedeck/src/unknowns.rs (6)
82-104: LGTM!The timeout constants are reasonable: 3 seconds allows sufficient time for relay responses before falling back to broadcast, and 300ms grace period balances waiting for slower relays without degrading UX. The HashMap-based tracking avoids Mutex usage per coding guidelines.
201-263: LGTM!The
check_hint_timeoutsmethod correctly verifies IDs against ndb before re-queuing, avoiding redundant broadcast requests for already-resolved IDs. The separation ofrequeued_countandresolved_countprovides useful debug visibility.
417-459: LGTM!Good enhancement to extract relay hints from both e-tags and source relays. The heuristic that reactions likely came from the same relay as the reacted note is sound and improves hint routing effectiveness.
710-765: LGTM!The
send_grace_period_idsfunction correctly handles IDs that waited for the grace period. The re-filtering by pool membership (lines 722-728) handles the edge case where relays disconnected during the wait, with a clear comment explaining the fallback to hint timeout.
628-651: Thehas_connecting_relaysmethod is properly implemented onRelayPool(crates/enostr/src/relay/pool.rs:196) and correctly used here. No issues.
687-692: Subscription ID collision is possible but has limited impact due to timeout logic.The subscription ID
format!("unknownids-{}", relay_url.as_str())can indeed be replaced ifunknown_id_sendis called multiple times to the same relay. A race condition exists:
ready_to_send()waits 2 seconds before allowing a sendHINT_RETRY_TIMEOUTis 3 seconds- If new IDs are discovered within 2 seconds of a previous send, the new subscription will replace the old one before a response arrives (~1 second collision window)
- The old subscription's in-flight request is lost
However, the impact is mitigated because:
- Unresolved IDs are re-queued via
check_hint_timeouts()after 3 seconds- IDs will eventually be sent again (potentially to a different relay or in broadcast)
- The window is narrow and only occurs when IDs are discovered frequently
This is a real issue but not critical. Consider either:
- Using unique subscription IDs per batch (e.g., include a timestamp or sequence number)
- Closing the old subscription before creating a new one for the same relay
- Documenting this as accepted eventual-retry semantics
5d9acf1 to
4f283cb
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/enostr/src/pubkey.rs (1)
167-178: Consider logging invalid relay URLs for debugging.When a relay URL fails UTF-8 decoding (line 192) or has an invalid scheme, it's silently skipped. While this is correct behavior, a debug-level log could help diagnose issues with malformed nprofiles.
💡 Optional: Add debug logging for skipped relays
0x01 => { if let Ok(url) = std::str::from_utf8(value) { let lower = url.to_lowercase(); if lower.starts_with("ws://") || lower.starts_with("wss://") { relays.push(url.to_string()); + } else { + debug!("nprofile TLV: skipping relay with invalid scheme: {url}"); } + } else { + debug!("nprofile TLV: relay URL is not valid UTF-8"); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/enostr/src/pubkey.rs` around lines 167 - 178, The parser currently silently skips relay entries when UTF-8 decoding or scheme validation fails; update the nprofile TLV parsing logic (the loop handling variables typ, len, pos over data in pubkey.rs) to emit debug-level logs whenever a relay URL fails to decode from bytes or is rejected for an invalid scheme, including contextual info (the offending byte slice or attempted string, pos, and the specific error or reason) so malformed relays can be diagnosed while preserving the existing skip behavior.crates/notedeck/src/unknowns.rs (1)
398-428: Note: All hinted IDs are broadcast to all accumulated hint relays.The current implementation collects all hint relays into a single set and sends all hinted IDs to all of them. This means if ID₁ has hint
wss://relay-a.comand ID₂ has hintwss://relay-b.com, both IDs will be requested from both relays.This is a reasonable simplification that trades some extra network requests for implementation simplicity. Given the PR comments mention an upcoming outbox pool that will address routing more comprehensively, this approach is acceptable for now.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/notedeck/src/unknowns.rs` around lines 398 - 428, The loop collects all relays into hint_relays and broadcasts all hinted_ids to every relay (so IDs with relay-a and relay-b are both sent to a and b); update the code by adding a clear inline comment above the hinted_ids/hint_relays block stating this broadcasting behavior and the deliberate tradeoff, reference the symbols hinted_ids, hint_relays, get_unknown_ids_filter, and oneshot.oneshot_to_relays, and add a TODO mentioning the forthcoming outbox pool (or an issue/PR number) to revisit per-ID routing in the future.crates/notedeck/src/oneshot_api.rs (1)
28-36: Consider adding test coverage foroneshot_to_relays.The existing test
oneshot_uses_selected_account_read_relaysverifies thatoneshot()routes to account read relays. A similar test foroneshot_to_relays()would verify that explicit relay URLs are correctly routed.🧪 Example test structure
#[test] fn oneshot_to_relays_uses_provided_urls() { let (_tmp, accounts) = test_accounts_with_forced_relay("wss://relay-read.example.com"); let mut pool = OutboxPool::default(); let filter = Filter::new().kinds(vec![1]).limit(1).build(); let mut explicit_relays: HashSet<NormRelayUrl> = HashSet::new(); explicit_relays.insert(NormRelayUrl::new("wss://hint-relay.example.com").unwrap()); { let mut outbox = OutboxSessionHandler::new(&mut pool, EguiWakeup::new(egui::Context::default())); let mut oneshot = OneshotApi::new(&mut outbox, &accounts); oneshot.oneshot_to_relays(vec![filter.clone()], explicit_relays.clone()); } let request_id = OutboxSubId(0); let status = pool.status(&request_id); let status_relays: hashbrown::HashSet<NormRelayUrl> = status.keys().map(|url| (*url).clone()).collect(); assert_eq!(status_relays, explicit_relays); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/notedeck/src/oneshot_api.rs` around lines 28 - 36, Add a unit test that mirrors the existing oneshot_uses_selected_account_read_relays test but calls OneshotApi::oneshot_to_relays with a HashSet of explicit NormRelayUrl values and asserts the OutboxPool recorded those exact relay URLs; specifically, instantiate OutboxPool::default(), create a Filter, build a HashSet with a test NormRelayUrl, create an OutboxSessionHandler and OneshotApi, call oneshot_to_relays(filters, explicit_relays.clone()), then fetch pool.status(&OutboxSubId(0)) and compare the returned relay keys (as NormRelayUrl set) to the explicit_relays to ensure they match.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/enostr/src/pubkey.rs`:
- Around line 167-178: The parser currently silently skips relay entries when
UTF-8 decoding or scheme validation fails; update the nprofile TLV parsing logic
(the loop handling variables typ, len, pos over data in pubkey.rs) to emit
debug-level logs whenever a relay URL fails to decode from bytes or is rejected
for an invalid scheme, including contextual info (the offending byte slice or
attempted string, pos, and the specific error or reason) so malformed relays can
be diagnosed while preserving the existing skip behavior.
In `@crates/notedeck/src/oneshot_api.rs`:
- Around line 28-36: Add a unit test that mirrors the existing
oneshot_uses_selected_account_read_relays test but calls
OneshotApi::oneshot_to_relays with a HashSet of explicit NormRelayUrl values and
asserts the OutboxPool recorded those exact relay URLs; specifically,
instantiate OutboxPool::default(), create a Filter, build a HashSet with a test
NormRelayUrl, create an OutboxSessionHandler and OneshotApi, call
oneshot_to_relays(filters, explicit_relays.clone()), then fetch
pool.status(&OutboxSubId(0)) and compare the returned relay keys (as
NormRelayUrl set) to the explicit_relays to ensure they match.
In `@crates/notedeck/src/unknowns.rs`:
- Around line 398-428: The loop collects all relays into hint_relays and
broadcasts all hinted_ids to every relay (so IDs with relay-a and relay-b are
both sent to a and b); update the code by adding a clear inline comment above
the hinted_ids/hint_relays block stating this broadcasting behavior and the
deliberate tradeoff, reference the symbols hinted_ids, hint_relays,
get_unknown_ids_filter, and oneshot.oneshot_to_relays, and add a TODO mentioning
the forthcoming outbox pool (or an issue/PR number) to revisit per-ID routing in
the future.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CHANGELOG.mdcrates/enostr/src/lib.rscrates/enostr/src/pubkey.rscrates/notedeck/src/oneshot_api.rscrates/notedeck/src/unknowns.rscrates/notedeck_columns/src/post.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/enostr/src/lib.rs
- CHANGELOG.md
- crates/notedeck_columns/src/post.rs
Add ParsedNprofile struct that preserves relay hints from NIP-19 nprofile bech32 strings, unlike the existing from_nprofile_bech() which discards them. - ParsedNprofile: pubkey + relay hints vec - try_from_nprofile_string(): nostr crate first, manual TLV fallback - try_from_nprofile_manual(): graceful TLV parsing, ws:// validation - parse_with_relays(): hex -> npub -> nprofile chain with relay hints - Export ParsedNprofile from enostr crate Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The unknown ID system already collects relay hints from nprofile/nevent mentions but discards them when sending requests. Use them: - Add OneshotApi::oneshot_to_relays() for sending requests to specific relay URLs instead of the account's read relay set - Rewrite unknown_id_send() to partition IDs into hinted vs unhinted, routing hinted IDs to their hint relays via oneshot_to_relays() and unhinted IDs to account read relays via oneshot() - No new state or retry logic; existing 2s debouncer handles retries Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Per NIP-10, e-tags support an optional 5th element for the author's pubkey: ["e", <id>, <relay>, <marker>, <pubkey>]. This helps clients using the outbox model locate referenced events. - Add author pubkey to root and reply e-tags in to_reply() - Add empty relay hint and author pubkey to q-tags in to_quote() Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
4f283cb to
90d47b8
Compare
|
@kernelkind I re-based/modified on top of your latest notedeck outbox work that was merged |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/notedeck_columns/src/post.rs (1)
95-100:⚠️ Potential issue | 🟠 MajorRoot
etag is using the parent author pubkey, not the root author.When
nip10.root()exists, the tag referencesroot.idbut appendsreplying_to.pubkey(). That pubkey is for the parent note, so root metadata can be incorrect. If root author is unavailable, prefer omitting the 5th element for the root tag rather than writing the parent pubkey.Suggested fix
builder .start_tag() .tag_str("e") .tag_str(&hex::encode(root.id)) .tag_str("") .tag_str("root") - .tag_str(&hex::encode(replying_to.pubkey())) .start_tag() .tag_str("e") .tag_str(&hex::encode(replying_to.id()))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/notedeck_columns/src/post.rs` around lines 95 - 100, The root "e" tag is currently using replying_to.pubkey() as the 5th element which is the parent author, not the root author; update the tag construction in the code that builds the root tag (the chain that calls .tag_str("e") .tag_str(&hex::encode(root.id)) ...) to use the actual root author pubkey when nip10.root() provides it, and if the root author pubkey is unavailable then omit the 5th element (do not use replying_to.pubkey()) — i.e., prefer including the root author's pubkey from the root object (if present) or drop the extra .tag_str(...) for the author slot for the root tag.
🧹 Nitpick comments (3)
crates/notedeck/src/unknowns.rs (2)
388-442: Add rustdoc forunknown_id_send’s new routing semantics.This function now contains non-trivial hinted/unhinted routing behavior and relay normalization; a concise doc comment would make expectations and fallback behavior clearer.
As per coding guidelines "Ensure docstring coverage for any code added or modified".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/notedeck/src/unknowns.rs` around lines 388 - 442, Add a rustdoc comment above the function unknown_id_send describing its routing semantics: explain that it accepts a mutable UnknownIds and OneshotApi, partitions UnknownId entries into "hinted" (have relays that normalize successfully via NormRelayUrl::new) and "unhinted" (no relays or none normalize), that hinted ids are aggregated and sent with get_unknown_ids_filter -> oneshot_to_relays to the union of normalized hint_relays while unhinted ids fall back to get_unknown_ids_filter -> oneshot to account read relays, and note side effects (calls to oneshot_to_relays/oneshot and that unknown_ids.clear() is called) plus any error/empty-filter behavior to set expectations for callers.
398-429: Hinted IDs are still broadcast to unrelated hint relays.Because
hint_relaysis a union, each hinted relay can receive IDs that were never hinted for it. Consider grouping IDs by normalized relay (or relay-set) and sending per-group to keep routing truly targeted.Possible refactor
- let mut hinted_ids: Vec<&UnknownId> = Vec::new(); - let mut hint_relays: hashbrown::HashSet<NormRelayUrl> = hashbrown::HashSet::new(); + let mut hinted_by_relay: hashbrown::HashMap<NormRelayUrl, Vec<&UnknownId>> = + hashbrown::HashMap::new(); let mut unhinted_ids: Vec<&UnknownId> = Vec::new(); for (id, relays) in &unknown_ids.ids { if relays.is_empty() { unhinted_ids.push(id); continue; } - let normed: Vec<NormRelayUrl> = relays + let normed: Vec<NormRelayUrl> = relays .iter() .filter_map(|r| NormRelayUrl::new(r.as_str()).ok()) .collect(); - if normed.is_empty() { + if normed.is_empty() { unhinted_ids.push(id); } else { - hinted_ids.push(id); - hint_relays.extend(normed); + for relay in normed { + hinted_by_relay.entry(relay).or_default().push(id); + } } } - if !hinted_ids.is_empty() { - if let Some(filters) = get_unknown_ids_filter(&hinted_ids) { - oneshot.oneshot_to_relays(filters, hint_relays); - } - } + for (relay, ids_for_relay) in hinted_by_relay { + if let Some(filters) = get_unknown_ids_filter(&ids_for_relay) { + let mut relays = hashbrown::HashSet::new(); + relays.insert(relay); + oneshot.oneshot_to_relays(filters, relays); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/notedeck/src/unknowns.rs` around lines 398 - 429, The current logic builds a unioned hint_relays set and sends all hinted_ids to that union, which causes relays to receive IDs not intended for them; change the flow in the loop over unknown_ids.ids to group IDs by their normalized relay(s) (e.g. build a HashMap<NormRelayUrl, Vec<&UnknownId>> or HashMap<HashSet<NormRelayUrl>, Vec<&UnknownId>>), then for each group call get_unknown_ids_filter on the group's IDs and call oneshot.oneshot_to_relays with only that group's relay(s) instead of the global hint_relays; update references to hinted_ids and hint_relays to use the per-group collections and keep the existing get_unknown_ids_filter and oneshot.oneshot_to_relays calls.crates/enostr/src/pubkey.rs (1)
285-377: Add direct regression coverage for the manual TLV fallback path.Current tests create nprofile strings via
Nip19Profile::to_bech32(), which exercises the primaryNip19Profile::from_bech32()parser. Add at least one test that directly invokestry_from_nprofile_manual()or constructs a malformed nprofile that forces the fallback, including mixed-case relay schemes (e.g.,Wss://,WS://) to verify the case-insensitive URL scheme matching.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/enostr/src/pubkey.rs` around lines 285 - 377, Tests only exercise Nip19Profile::from_bech32 path; add a unit test that directly exercises the TLV/manual fallback by calling Pubkey::try_from_nprofile_manual (or constructing a malformed nprofile string that fails standard parsing and falls back) and assert the parsed relays include entries when given mixed-case schemes like "Wss://relay.damus.io" and "WS://nos.lol" to verify case-insensitive scheme matching; place the test in the existing tests module and mirror assertions used in parse_nprofile_multiple_relays (check result.pubkey.hex() == JB55_HEX and result.relays.len() and that relays contain the host substrings).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@crates/notedeck_columns/src/post.rs`:
- Around line 95-100: The root "e" tag is currently using replying_to.pubkey()
as the 5th element which is the parent author, not the root author; update the
tag construction in the code that builds the root tag (the chain that calls
.tag_str("e") .tag_str(&hex::encode(root.id)) ...) to use the actual root author
pubkey when nip10.root() provides it, and if the root author pubkey is
unavailable then omit the 5th element (do not use replying_to.pubkey()) — i.e.,
prefer including the root author's pubkey from the root object (if present) or
drop the extra .tag_str(...) for the author slot for the root tag.
---
Nitpick comments:
In `@crates/enostr/src/pubkey.rs`:
- Around line 285-377: Tests only exercise Nip19Profile::from_bech32 path; add a
unit test that directly exercises the TLV/manual fallback by calling
Pubkey::try_from_nprofile_manual (or constructing a malformed nprofile string
that fails standard parsing and falls back) and assert the parsed relays include
entries when given mixed-case schemes like "Wss://relay.damus.io" and
"WS://nos.lol" to verify case-insensitive scheme matching; place the test in the
existing tests module and mirror assertions used in
parse_nprofile_multiple_relays (check result.pubkey.hex() == JB55_HEX and
result.relays.len() and that relays contain the host substrings).
In `@crates/notedeck/src/unknowns.rs`:
- Around line 388-442: Add a rustdoc comment above the function unknown_id_send
describing its routing semantics: explain that it accepts a mutable UnknownIds
and OneshotApi, partitions UnknownId entries into "hinted" (have relays that
normalize successfully via NormRelayUrl::new) and "unhinted" (no relays or none
normalize), that hinted ids are aggregated and sent with get_unknown_ids_filter
-> oneshot_to_relays to the union of normalized hint_relays while unhinted ids
fall back to get_unknown_ids_filter -> oneshot to account read relays, and note
side effects (calls to oneshot_to_relays/oneshot and that unknown_ids.clear() is
called) plus any error/empty-filter behavior to set expectations for callers.
- Around line 398-429: The current logic builds a unioned hint_relays set and
sends all hinted_ids to that union, which causes relays to receive IDs not
intended for them; change the flow in the loop over unknown_ids.ids to group IDs
by their normalized relay(s) (e.g. build a HashMap<NormRelayUrl,
Vec<&UnknownId>> or HashMap<HashSet<NormRelayUrl>, Vec<&UnknownId>>), then for
each group call get_unknown_ids_filter on the group's IDs and call
oneshot.oneshot_to_relays with only that group's relay(s) instead of the global
hint_relays; update references to hinted_ids and hint_relays to use the
per-group collections and keep the existing get_unknown_ids_filter and
oneshot.oneshot_to_relays calls.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CHANGELOG.mdcrates/enostr/src/lib.rscrates/enostr/src/pubkey.rscrates/notedeck/src/oneshot_api.rscrates/notedeck/src/unknowns.rscrates/notedeck_columns/src/post.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/notedeck/src/oneshot_api.rs
Summary
Rebased and simplified relay hints against the post-outbox master. Down from ~1,030 lines across 9 files to ~270 lines across 6 files by dropping dead
RelayPoolcode and leveraging the existing outbox infrastructure.ParsedNprofilestruct preserves relay hints from NIP-19 nprofile bech32 strings (nostr crate first, manual TLV fallback for robustness)unknown_id_send()partitions IDs into hinted vs unhinted, routing hinted IDs to their hint relays viaoneshot_to_relays()and unhinted to account read relays. No new state or retry logic; existing 2s debouncer handles retries naturally.Commits
enostr: add ParsedNprofile and nprofile parsing with relay hints—pubkey.rs,lib.rsnotedeck: route unknown ID lookups to relay hints—oneshot_api.rs,unknowns.rspost: add NIP-10 author pubkey to reply and quote tags—post.rs,CHANGELOG.mdEach commit compiles independently.
What changed from the original PR
RelayPool::subscribe_to/ relay_status / canonicalizeapp.rshook pointspost.rsrelay_hint parameter into_quote()Known limitations
""because nostrdb doesn't expose which relay delivered a note. A nostrdb change (storing source relay per note) would enable this.replying_to.pubkey()rather than root author's pubkey, since root author isn't available without an extra DB lookup.Test plan
cargo test -p enostr— ParsedNprofile + nprofile parsing tests passcargo test -p notedeck— oneshot_api + unknowns tests passcargo test -p notedeck_columns— post.rs tests passcargo clippy -D warnings— clean across all 3 cratescargo build— full build compiles clean