-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
shuffle new relays #41
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution!!
However, I can't entirely agree with parts of these changes. Here are the reasons
- This requires a level of interaction from the user. This means it's going to be harder to auto-provision the server
- There should be a strong preference for a set of nostr nodes that work well.
I think the best course of action here is for the user to be able to provide the node they want to connect to. And in the read me we can have a example start command that uses the nostr.watch API.
Lmk what you think
I made this available via argument parsing:
I don't know if exiting right after running the --show command is good, maybe I should just let the --add option and the user has to find new relays by himself. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the slow review. This looks good. I do have a open question
src/utils/nostr_utils.py
Outdated
def add_relays(): | ||
def show_relays(): | ||
r = (requests.get(NOSTR_WATCH, {})).json() | ||
new_relays = [x for x in r[:20] if 'damus' not in x and 'nostr-pub.wellorder' not in x] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not show damus
and the nostr-pub.wellorder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain why r[:20]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that just to skip protocol portion of the url?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- First question: that’s because I’ve let the original relays in the NOSTR_RELAY global list, so I’m excluding them here to avoid potential duplicates
- Second question: sorry for the magic number, should be corrected, that’s an arbitrary number bc the list of active relays is quite long, originally I was asking the user if he wanted to reshuffle so that it shows 20 other relays and so on
The relay selection could be improved based on relay location or other informations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this make the service dependent on Nostr Watch? If that service goes down would that be a single point of failure? We may be able to achieve the same thing with an async websocket relay pool unless there are other advantages to using Nostr Watch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah the show option relies on the nostr_watch API , the advantage to me is that it's dynamically updated which could be desirable given how relays are prone to instability.
Moreover it seems pretty canonical.
If by websocket relay pool you mean a statically maintained pool, it could lead to extra continuous work to maintain if but if you know how to get relays that are online and dynamically update the pool, it would indeed be a better idea. I could peep the nostr_watch repo to see how they get the relays under the hood.
Lmk what you think
Added a function to use more active relays via the nostr watch API