Skip to content
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

Add Support for [Nostr] Followers #9870

Merged
merged 16 commits into from
Jan 11, 2024
Merged

Conversation

sepehr-safari
Copy link
Contributor

Added first and simplest interaction with Nostr (https://github.com/nostr-protocol/nostr).

Using APIs from https://api.nostr.band/ to keep things fast and simple.

Some other useful stats will be added further.

Copy link
Contributor

github-actions bot commented Jan 2, 2024

Messages
📖 ✨ Thanks for your contribution to Shields, @sepehr-safari!

Generated by 🚫 dangerJS against 4e63e9f

@chris48s chris48s added the service-badge New or updated service badge label Jan 4, 2024
@chris48s chris48s changed the title Add Support for Nostr Followers Add Support for [Nostr] Followers Jan 4, 2024
@sepehr-safari
Copy link
Contributor Author

ready to merge I guess?

@chris48s
Copy link
Member

chris48s commented Jan 6, 2024

Before I do a code review on this, a couple of things:

  1. As I understand it, nostr is a decentralised protocol. https://nostr.band/ is a stats service, but it is basically a stats service someone has set up - it is not "official". Someone else could come along and set up another "stats about nostr" service which is no more or less valid as a source of information than nostr.band. Is that a reasonable assessment of what nostr.band is?

  2. Your service tests are failing. Can you have a look at getting the build passing before we review.

Thanks

@sepehr-safari
Copy link
Contributor Author

nostr is a decentralised protocol.

Yes.

https://nostr.band/ is a stats service.

Yes.

https://nostr.band/ is not "official".

Yes.

Someone else could come along and set up another "stats about nostr" service.

Yes.

Your service tests are failing.

FIXED! (3e896e4)

so here is the conclusion:

  1. there is NOTHING "official" about Nostr. not even a little thing. this is the first rule of "decentralize".
  2. we can simply rely on a well-stablished service such as nostr.band.

ready to code review and merge.
thank you.

@chris48s
Copy link
Member

chris48s commented Jan 7, 2024

So I think I'd say the "service" here isn't really "nostr". It is nostr.band.

I guess what I'm getting at here is: If someone else sets up a competing service offering stats about the nostr network, why did stats from nostr.band get to be /nostr and not them? Seems like no good reason, right?

I'd suggest we make the route base for this /nostr-band.

A somewhat analogous situation would be: We have badges that show stats about the NPM ecosystem from https://npms.io/ and https://npm-stat.com/ but they live under /npm-stat and /npms-io.

@sepehr-safari
Copy link
Contributor Author

you are right. I've changed the service from nostr to nostr-band.
is there anything else that should be considered for this pr?

I would also suggest creating a separate service for nostr where users can freely choose their desired relay and fetch stats directly from that relay over websocket.
that would be the real decentralized nostr flow, not forcing users to use a pre-defined API service.
is there any documentation to use websocket inside your services?

@sepehr-safari sepehr-safari changed the title Add Support for [Nostr] Followers Add Support for [Nostr.band] Followers Jan 8, 2024
@sepehr-safari sepehr-safari changed the title Add Support for [Nostr.band] Followers Add Support for [Nostr] Followers Jan 8, 2024
Copy link
Contributor

github-actions bot commented Jan 8, 2024

🚀 Updated review app: https://pr-9870-badges-shields.fly.dev

Copy link
Member

@chris48s chris48s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks - have looked over the code and left some comments

@chris48s
Copy link
Member

chris48s commented Jan 8, 2024

I would also suggest creating a separate service for nostr where users can freely choose their desired relay and fetch stats directly from that relay over websocket.
that would be the real decentralized nostr flow, not forcing users to use a pre-defined API service.
is there any documentation to use websocket inside your services?

I suggest moving the discussion of this to a badge request issue. Lets not scope creep here as it feels like what you've got here is pretty close.

We don't have any other services that use websockets, so if you wanted to contribute that it would have to be a new base class. If you wanted to implement that, I'd suggest looking over the existing base classes we have, for example:

but its probably useful to nail down the details in an issue a bit first given it would be quite an involved contribution.

@sepehr-safari
Copy link
Contributor Author

thanks for your detailed review. think I've successfully addressed all your specs.

@sepehr-safari
Copy link
Contributor Author

I suggest moving the discussion of this to a badge request issue.

good idea. I will open an issue for implementing websocket and the rest.

Copy link
Contributor

🚀 Updated review app: https://pr-9870-badges-shields.fly.dev

Copy link
Member

@chris48s chris48s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one. Merging. Btw, I really appreciate the way you replied to each comment with a link to the commit addressing that comment - makes the second pass of review super easy. Thanks

@chris48s chris48s added this pull request to the merge queue Jan 11, 2024
Merged via the queue into badges:master with commit 4a55c3e Jan 11, 2024
@sepehr-safari
Copy link
Contributor Author

I really appreciate the way you replied to each comment with a link to the commit addressing that comment - makes the second pass of review super easy.

glad it was helpful. thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
service-badge New or updated service badge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants