Track the user's relay list #380
Conversation
ksedgwic
force-pushed
the
2024-10-user-relay-list-redux
branch
from
67b5e1a
to
86e6a08
Compare
|
rebased on master |
| .expect("selected account"); | ||
|
|
||
| // NIP-65 | ||
| Filter::new() |
There was a problem hiding this comment.
you need to add tags for the relay urls as specified in nip 65
There was a problem hiding this comment.
I'm not sure what you mean here.
Do you mean something in the subscription?
Or discrimination in the handling? We definitely want to discriminate read and write relays but that all needs to be added at the relay management layer as well?
| .collect() | ||
| } | ||
|
|
||
| fn handle_nip65_relays(ndb: &Ndb, txn: &Transaction, nks: &Vec<NoteKey>) -> Vec<String> { |
|
|
||
| fn set_advertised_relays(pool: &mut RelayPool, relays: Vec<String>) { | ||
| let wakeup = move || { | ||
| // FIXME - how do we repaint? |
There was a problem hiding this comment.
from deleted app::relay_setup:
let ctx = ctx.clone();
let wakeup = move || {
ctx.request_repaint();
};
There was a problem hiding this comment.
There is not a ctx around in this code. It appears to repaint in the next frame anyway? I'm not sure why this is necessary
| pub async fn track_user_relays(damus: &mut Damus) { | ||
| debug!("track_user_relays starting"); | ||
|
|
||
| let filter = user_relay_filter(damus); |
There was a problem hiding this comment.
we touched on this but we may not want to only track the current user's relays
| pub fn spawn_track_user_relays(damus: &mut Damus) { | ||
| // This is only safe because we are absolutely single threaded ... | ||
| let damus_ptr = &mut *damus as *mut Damus; | ||
| spawn_sendable(async move { |
There was a problem hiding this comment.
I think this implementation for adding relays can be simplified into a synchronous implementation that runs every frame in app::update_damus.
In the case where the user is actively using the app, the time between frames is going to be so low that it won't matter that we'll have to wait until
the next frame to check ndb for a new relay list. In the case where the user isn't using the app, the update function still runs, about once per second
it seems so if the user is just staring at their relay list without any interaction and an update to their list occurs, they will see it, albeit with
about a second delay.
Do you see a reason that I don't for the increased complexity of this async impl?
There was a problem hiding this comment.
I think polling for activity doesn't scale well to apps with lots of things to poll for. So it might be fine to poll for user relay lists, but when we add many many other things it will be less great
Pick a few fixes from #380 Link: #380 Ken Sedgwick (5): build: Cargo.lock updates to mitigate num_enum_derive problem add .rustfmt.toml to specify edition Fix parsing of subscription id for RelayMessage::Event Skip adding relays that are already in the pool canonicalize relay urls to avoid false duplicates
There was a problem hiding this comment.
I merged a few of these commits into master, up until the point that needs changes
commit 702f8ffca390eb77977556754daceab1e351f9de (HEAD -> master, github/master, test)
Merge: 877a30d2f666 570d64c3cdca
Author: William Casarin <jb55@jb55.com>
Date: Mon Nov 11 11:16:48 2024 -0800
Merge a few relay fixes from ken
Pick a few fixes from #380
Link: https://github.com/damus-io/notedeck/pull/380
Ken Sedgwick (5):
build: Cargo.lock updates to mitigate num_enum_derive problem
add .rustfmt.toml to specify edition
Fix parsing of subscription id for RelayMessage::Event
Skip adding relays that are already in the pool
canonicalize relay urls to avoid false duplicates
enostr/src/relay/pool.rs
Outdated
| fn canonicalize_url(url: String) -> String { | ||
| match Url::parse(&url) { | ||
| Ok(parsed_url) => parsed_url.to_string(), | ||
| Err(_) => url, // If parsing fails, return the original URL. | ||
| } | ||
| } |
There was a problem hiding this comment.
This is fine, but in damus-ios we have the RelayURL for this purpose which fixes equality and ensures that only ws/wss urls result in proper construction. see:
https://github.com/damus-io/damus/blob/master/damus/Nostr/RelayURL.swift
we can defer this to a future PR though
| // Add all of the existing subscriptions to the new relay | ||
| for (subid, filters) in &self.subs { | ||
| pool_relay.relay.subscribe(subid.clone(), filters.clone()); | ||
| } |
There was a problem hiding this comment.
I don't think this is correct at this abstraction level. this is application logic, not enostr/pool logic
ksedgwic
force-pushed
the
2024-10-user-relay-list-redux
branch
from
784732b
to
483846d
Compare
This is an alternative approach to #346
Instead of adding sync to convince rust that the operations are safe we assert they are safe (using
unsafe) because we are single threaded.The last commit in the stack is the interesting part ...