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

NIP-22 - Comment #1233

Merged
merged 24 commits into from
Nov 7, 2024
Merged

NIP-22 - Comment #1233

merged 24 commits into from
Nov 7, 2024

Conversation

arthurfranca
Copy link
Contributor

@arthurfranca arthurfranca commented May 12, 2024

Please read here

@vitorpamplona
Copy link
Collaborator

vitorpamplona commented May 12, 2024

+1.

Thread roots and replies should always be different kinds. This "Comment" kind could be for replies only. The thread root is usually the main content kind in each NIP.

Then it would be similar to reactions. They can react to anything but they never start anything by themselves.

I also think we should migrate Kind 1 to use this kind as reply in the future. Kind 1 should be for root posts only.

@arthurfranca
Copy link
Contributor Author

I also think we should migrate Kind 1 to use this kind as reply in the future. Kind 1 should be for root posts only.

It would be great if clients start posting kind:1111s along with kind:1 replies when publishing new replies.

@arthurfranca arthurfranca marked this pull request as ready for review May 13, 2024 18:19
Copy link
Contributor

@dluvian dluvian left a comment

Choose a reason for hiding this comment

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

+1
We should deprecate nip10 root and reply markers and refer to this nip, so people are aware that kind 1 shouldn't be used for replies

I also think we should migrate Kind 1 to use this kind as reply in the future. Kind 1 should be for root posts only.

100% agree. Kind 1 being root and reply is extremely inefficient

22.md Outdated Show resolved Hide resolved
22.md Outdated Show resolved Hide resolved
@mikedilger
Copy link
Contributor

Juggernauts, mavericks, all of you. This is a seriously breaking change breaking tons of existing software.

@vitorpamplona
Copy link
Collaborator

Juggernauts, mavericks, all of you. This is a seriously breaking change breaking tons of existing software.

But is it better or worse?

@dluvian
Copy link
Contributor

dluvian commented May 16, 2024

This is a seriously breaking change breaking tons of existing software.

Strong words for something that existing clients can just completely ignore. If devs feel like their clients don't show enough content they can opt-in to querying this kind, like they do with every other event kind.

We also have to be realistic. Kind 1 sucks because it is overloaded. There have been multiple PRs that address this issue. At some point client devs will just use a different kind for replies, because it's easier to work with and is more bandwidth efficient, which is an important point since the majority of potential nostr users will use mobile clients that have bandwidth constraints.
Having these replies standardized in this repo does not matter, but would be preferable. If the overlords here disagree with a sensible proposal clients will just define their own standards like jb55 did.

@arthurfranca arthurfranca mentioned this pull request May 16, 2024
@staab
Copy link
Member

staab commented May 16, 2024

@dluvian the the bar for adding something to the nips repo has never been "will the overlords allow it", but "do two clients implement it". So if you can point to two clients that have added this, we can merge it. But it's also fine to have the conversation here and debate whether it's a good idea. I'm against it until we have a system for managing breaking changes. I think there are things we can do, but let's solve that before we break the experience in all existing clients.

22.md Outdated Show resolved Hide resolved
22.md Outdated Show resolved Hide resolved
@mikedilger
Copy link
Contributor

mikedilger commented May 16, 2024

I made a mistake. This it isn't what I initially thought. Reading it again after a good nights sleep and I understand better what this is.

If you need to comment on a patch, or on a bid, or on a relay list, this event makes it possible.

My initial read was that all kind-1 replies would be replaced with these comments, and only kind-1 thread roots would remain kind-1, which is why I made the previous comment. But that's not the case.

EDIT: And I got that impression from comments earlier in this thread, which were I'd say "aspirational" and not reflecting the NIP as it is proposed.

@mikedilger
Copy link
Contributor

But please explain why kind-1 notes cannot already be used to do this, and how a new kind improves things. I fail to see how it improves things versus kind-1.

@vitorpamplona
Copy link
Collaborator

But please explain why kind-1 notes cannot already be used to do this, and how a new kind improves things. I fail to see how it improves things versus kind-1.

It is currently impossible to make a filter that only brings kind1 root posts. You have to download all new posts and replies and then filter the replies out. It's extremely wasteful.

@arthurfranca
Copy link
Contributor Author

@mikedilger kind:1 could in theory retrofit into a generic comment. One caveat is that to download root posts only, we'de have to convince existing clients to add something hacky like an empty k tag to be able to filter by { #k: [""], kinds: [1] } (and set k=referenced-event-kind for non-root ones). Separating comments from root posts felt more appropriate than the empty string.

Also, this thread you are in is a good example of the current situation: diverse apps are indiscriminately using kind:1 to comment on things that twitter-like clients don't recognize. Kind:1 clients currently have no way to opt-out.

If we do nothing, more and more bogus events will get downloaded only to be discarded right away. A bandwidth waste to wsay the least.

@mikedilger
Copy link
Contributor

I agree root posts being of a different kind would be better. But it seems like a change that is too late to make.

I'm adding it to mikedilger/nostr-next#31

@dluvian
Copy link
Contributor

dluvian commented May 17, 2024

But it seems like a change that is too late to make.

But why? Not improving the protocol in fear of a migration process feels wrong. Especially since nostr is still relatively small, we should just bite the bullet if it's worth it.

Consider that right now we can't:

  • Query only roots
  • Query only replies without setting ids
  • Lazy load a thread starting from the root (because of nip10 root e-tags)
  • Know if a reply is to another kind 1 or some other event

@vitorpamplona
Copy link
Collaborator

vitorpamplona commented May 17, 2024

Talking about the specifics here, I am failing to see how the new role-based tag structures (o and r) are an improvement over NIP-10 markers. The parser of the first value is wild. If we keep adding other types to it (hashtag, geolocation roots?), the parser itself could become an issue.

Is this,

  • ["o", "i:<event-kind>:<event-pubkey>:<event-id>", "<relay-url, optional>"]
  • ["o", "a:<event-kind>:<event-pubkey>:<d-tag, optional>", "<relay-url, optional>"]
  • ["o", "https://abc.com/articles/1/"]
  • ["r", "i:<event-kind>:<event-pubkey>:<event-id>", "<relay-url, optional>"]
  • ["r", "a:<event-kind>:<event-pubkey>:<d-tag, optional>", "<relay-url, optional>"]
  • ["r", "https://abc.com/articles/1/"]

better than this?

  • ["e", "<event-id>", "<relay-url, optional>", "root"]
  • ["a", "<event-kind>:<event-pubkey>:<d-tag, optional>", "<relay-url, optional>", "root"]
  • ["r", "https://abc.com/articles/1/", "root"]
  • ["e", "<event-id>", "<relay-url, optional>", "reply"]
  • ["a", "<event-kind>:<event-pubkey>:<d-tag, optional>", "<relay-url, optional>", "reply"]
  • ["r", "https://abc.com/articles/1/", "reply"]

o and r tags feel worse because now we have a little evolving (do we need versions for this?) algorithm to parse the first value. Historically the algorithm has been defined by each tag and because it had a fixed scope, it didn't need to change over time. Putting the marker as the first tag means that you have to evolve the algo to support any new owners and reply structures that might arise.

@vitorpamplona
Copy link
Collaborator

Maybe what's missing is the clarification of the "schema" in value 1. We could specify the chars before the first : define a schema for the rest of the value.

  • ["o", "i:<event-kind>:<event-pubkey>:<event-id>", "<relay-url, optional>"]
  • ["o", "a:<event-kind>:<event-pubkey>:<d-tag, optional>", "<relay-url, optional>"]
  • ["o", "r:https://abc.com/articles/1/"]
  • ["o", "t:<hashtag>"]
  • ["o", "g:<geohash>"]

Parse the schema first and then send the rest of the value to the appropriate decoder if you support that.

Do the same structure for replies.

Then over time these letters can be expanded to words to represent more objects that can be root or reply.

@arthurfranca
Copy link
Contributor Author

failing to see how the new role-based tag structures (o and r) are an improvement over NIP-10 markers

As you know, relays won't filter by marker. That's why the marker meaning became the indexable tag. "root" became "o", "reply" became "r" (mention became "q" - "The q tag ensures quote reposts are not pulled and included as replies in threads"). There's further explantion in the NIP at the threads section (there are two "This tag is useful to...")

We could specify the chars before the first : define a schema for the rest of the value

I'm not sure about t: and g:. It is unusual treating the hashtag as the subject (OP) considering this can already be achived by picking kind:1 and adding one t tag.

r: prefix is a good idea, good to have prefixes if we need to extend them (though I think "i:", "a:" for nostr - also "s:" for event sets - and "r:" for all old web things would cover everything).
People could add r tag too (like t and g), but twitter-like clients would be polluted by kind:1's used as blog comments, followers don't expect to see random blog comments of their follows.

@vitorpamplona
Copy link
Collaborator

I'm not sure about t: and g:. It is unusual treating the hashtag as the subject (OP) considering this can already be achived by picking kind:1 and adding one t tag.

The hashtag root comment will only show in UIs that follow the hashtag itself. Meaning, you can post into #bitcoin without notifying your followers on the regular kind 1 feed. Only people following #bitcoin would see your post. That is a great new feature to boost the use of hashtag-exclusive content.

Same for geolocation as in #1170

r: prefix is a good idea

I think these prefixes that define how the rest of the value is encoded are required.

@arthurfranca
Copy link
Contributor Author

Makes total sense. Some time ago before this PR I also thought of location-based public chats.

22.md Outdated
Comment on lines 22 to 30
**It must have a `k` tag** pointing to the kind of the subject being commented on.
Its value has a prefix: `"n:"` for nostr subjects and other prefixes (listed in a section ahead)
for things that aren't nostr events.

For example, when commenting on a `kind:30023`, the `k` tag is set to "n:30023".
When replying to another `kind:1111`, the `k` tag is set to "n:1111".

The `k` tag is useful to fetch top-level comments about events of specific types, for example,
by filtering with `{ "#k": ["n:30023"], "kinds": [1111] }`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I do not see the point of trying to get top level replies only, and we should just keep a tag that specifies the top level note's kind.

Could we not implement non-existence filters and check for the existence for r for this arguably nonexistent use case if we really need to?

Copy link
Contributor Author

@arthurfranca arthurfranca May 17, 2024

Choose a reason for hiding this comment

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

Unfortunatelly, non-existence filters are a dream that will never come true, according to previous 3 or so discussions about it on this repo.

On the lowercase k tag, check previous response. Can also see it being used to hook up on { "#k": ["#:t"] } filter to check for trending hashtags, cause the uppercase version would bring many more events. More on the hashtag use case for context.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Unfortunatelly, non-existence filters are a dream that will never come true, according to previous 3 or so discussions about it on this repo.

Which ones?

They can happen without much performance penalty.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

I honestly wouldn't dismiss it based off of those opinions. You don't actually need a tag exists/doesn't exist index, as 99.99% of tag nonexistence queries come attached with other indexable fields (kind, other tags, author).

I will make a PR soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe @mikedilger said the same thing at #683.

The caveat is relay may end up returning less events than the requested (even if there were more on DB) - of course considering client didn't request with a bizarre limit like {limit: 10000} to start with.

Clients would lose the ability of figuring out if there are more events to fetch depending on the number of returned events. Like, if client wants chunks of 5 events, it requests 6. If relay returns 6, client knows it should ask for more.

@fiatjaf
Copy link
Member

fiatjaf commented May 18, 2024

I think this is probably a good idea.

@fiatjaf
Copy link
Member

fiatjaf commented May 18, 2024

But please explain why kind-1 notes cannot already be used to do this, and how a new kind improves things. I fail to see how it improves things versus kind-1.

@mikedilger multiple NIPs have already been defining special kinds for contextual replies because people are very wary of using kind 1 -- as they should be, because kind 1 will show up in all microblogging clients without any context.

So having a single generic comment kind solves that.

Or we could create dozens of different comment kinds, all with the same format, but using a kind number for each situation.

I think there are pros and cons on both sides.

@vitorpamplona
Copy link
Collaborator

@pablof7z should we use this kind to reply Wikifreedia posts? :)

@arthurfranca arthurfranca changed the title NIP-73 - Generic Comment NIP-22 - Generic Comment Sep 25, 2024
@arthurfranca arthurfranca changed the title NIP-22 - Generic Comment NIP-22 - Comment Sep 25, 2024

It uses `kind:1111` with plaintext `.content` (no HTML, Markdown, or other formatting).

Comments MUST point to the root scope using uppercase tag names (e.g. `K`, `E`, `A` or `I`)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should be a SHOULD instead of a MUST. Clients don't always have access to the root event.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If everyone follows the spec from start, the first level thread comments will have root references, then the next level of comments would just copy these references from their parent comment, even if the root event is missing.

Copy link
Contributor

Choose a reason for hiding this comment

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

This only works when commenting on events that use this spec and you would have to trust the parent author to set the correct root data.

A comment only needs its parent data. Adding root data might be helpful for clients implementing full threads but is burdensome for clients that just want to create a comment. It should be encouraged but not be a requirement.

Copy link
Contributor

Choose a reason for hiding this comment

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

The root data is not even that useful. Right now you would get the whole thread by querying E because most threads are tiny. When threads get bigger you would hit relay restrictions and only receive a subset of the comments many of which will be n-level instead top-level replies, which are not useful for building the thread from the root.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm with you. Going the easy route of loading the full thread (relying on the uppercase E/A/I tags) in contrast to lazy loading is bad cause it loads too much events.

The uppercase K tag being the root item's kind though is very useful to filter by { authors: [someone], kinds: [1111], "#K": ["30023", ...others kinds this client cares about] }

Copy link
Contributor

Choose a reason for hiding this comment

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

What should be set when the parent does not set an E tag (we shouldn't rely on other people complying) and you also don't know the root?
Having the root scope as a MUST requirement doesn't make sense in this case. It should be SHOULD.

I personally don't really care about it as I'm going to disrespect the root scope the same way I disrespect nip10, but when a library dev sees a MUST he will implement it that way and I would like to use a library function instead of building the event myself.

@SilberWitch
Copy link
Contributor

Glad you decided against overwriting NIP-73.

22.md Outdated Show resolved Hide resolved
@staab
Copy link
Member

staab commented Oct 1, 2024

Can we clarify how great-grand-child replies should work? E.g.:

"kind": 1111,
"tags": [
  ["K", "1"],
  ["E", "<event-id>"],
  ["k", "1111"],
  ["e", "<direct-reply-id>"],
  ["k", "1111"],
  ["e", "<grandchild-reply-id>"],
]

I think it's important to specify that position matters here (if it matters, maybe there is a better way, since we ultimately deprecated positional tags in nip 10). The earlier k/e/a/i tags should indicate more distant ancestors, later ones should indicate more proximate ancestors. In other words, there's no equivalent to NIP 10's mention marker. This created the problem that it becomes hard to discern what the reply's immediate parent is.

@arthurfranca
Copy link
Contributor Author

The spec (for now atleast) says you should just reference the root and the parent, like this:

"kind": 1111,
"tags": [
  ["K", "1"],
  ["E", "<event-id>"],
  ["k", "1111"],
  ["e", "<grandchild-reply-id>"],
]

NIP-10 mention would be q tag in this PR.

@staab
Copy link
Member

staab commented Oct 1, 2024

The NIP says "q tags MAY be used when citing events in the .content with NIP-21." which aligns with NIP 18, in that q should be used for quotes, not ancestors. Even if that weren't the case though, the reply hierarchy ambiguity would remain for great-great-grandchild replies.

@arthurfranca
Copy link
Contributor Author

I think I didn't understand you. Among other things, you talked about the lack of a NIP-10 marker equivalent. NIP-10 says "Those marked with 'mention' denote a quoted or reposted event id". The marker has nothing to do with events in the reply tree (ancestors) as a mentioned event could be, for example, another root kind:1 external to the thread?

@staab
Copy link
Member

staab commented Oct 1, 2024

There are two separate issues, quotes and non-root ancestors. NIP 10 puts both in the mention category, and this NIP (ostensibly) puts both in q tags. In both cases, it's impossible to tell what is an ancestor, and what is a quote (unless you parse the content, and even then it's questionable).

Apart from quotes and ancestors being combined, if there are 3+ ancestors (other than root), it's impossible to tell what the hierarchy is.

I realize now that we've never had this, so maybe it's ok. Maybe immediate reply and root are enough, and other ancestors SHOULD NOT be included. I'm not sure though.

@vitorpamplona
Copy link
Collaborator

vitorpamplona commented Oct 1, 2024

Yes, we have never had this. But it would be nice if we did. Loading a long thread reply by reply is extremely annoying for users.

The issue is that mid level events cannot use neither E nor e, without relying on position

@vitorpamplona
Copy link
Collaborator

vitorpamplona commented Oct 30, 2024

So, I am trying to implement this for geohashes and found an interesting problem: Geohashes need multiple roots to support querying in multiple resolutions.

{
  kind: 1111,
  content: "Where is the party tonight?",
  tags: [
    ["I", "geo:9q9hv"],
    ["I", "geo:9q9h"],
    ["I", "geo:9q9"],
    ["I", "geo:9q"],
    ["I", "geo:9"],
    ["K", "geo"],

    ["i", "geo:9q9hv"],
    ["i", "geo:9q9h"],
    ["i", "geo:9q9"],
    ["i", "geo:9q"],
    ["i", "geo:9"],
    ["k", "geo"]
  ]
}

How do people feel about this?

@arthurfranca
Copy link
Contributor Author

that is the right way to deal with geohashes

@mikedilger
Copy link
Contributor

I'm OK with this PR. Are there clients doing it?

@staab
Copy link
Member

staab commented Nov 4, 2024

Flotilla uses this to reply to chat messages and threads (new kinds which I've made up)

@mikedilger
Copy link
Contributor

I've been subscribing to these now and haven't seen any in the wild yet.

It seems the migration path would be (1) subscribe to these on as many clients as possible, then (2) start creating these after most people can already see them.

@mikedilger
Copy link
Contributor

Can we clarify how great-grand-child replies should work? E.g.:

"kind": 1111,
"tags": [
  ["K", "1"],
  ["E", "<event-id>"],
  ["k", "1111"],
  ["e", "<direct-reply-id>"],
  ["k", "1111"],
  ["e", "<grandchild-reply-id>"],
]

I think it's important to specify that position matters here (if it matters, maybe there is a better way, since we ultimately deprecated positional tags in nip 10). The earlier k/e/a/i tags should indicate more distant ancestors, later ones should indicate more proximate ancestors. In other words, there's no equivalent to NIP 10's mention marker. This created the problem that it becomes hard to discern what the reply's immediate parent is.

IMHO events should only reference their direct parent with 'e' / 'a' tags (and the root with 'E' / 'A' tags), and non-root non-parent ancestors should not be referenced (unless you are explicitly quoting them with 'q').

@dluvian
Copy link
Contributor

dluvian commented Nov 5, 2024

I'm OK with this PR. Are there clients doing it?

Voyage subscribes to this and uses it to reply to other nip22 comments and nip88 polls and when creating single character replies. Users can also opt in to always use it.

I still think having a root tag should be optional and not a MUST (https://github.com/nostr-protocol/nips/pull/1233/files#r1779356113)

@arthurfranca
Copy link
Contributor Author

How people few about making this event editable?

If we don't add this feature to the spec before merging, in the future we will end up shoehorning #1090 like is happening with kind:1 events.

@vitorpamplona
Copy link
Collaborator

I am not opposed to extending kind:1010 to this PR.

#1090 specifically targets kind 1 to simplify the debate around edits.

That being said, I don't think kind:1010 should be applied to all events, like reactions do. We need to slowly test new kinds as needed because each kind brings it's own challenges to an editable .content.

@fiatjaf fiatjaf merged commit ec8eb9a into nostr-protocol:master Nov 7, 2024
@fiatjaf
Copy link
Member

fiatjaf commented Nov 7, 2024

This has been a long due and there are 3~4 clients using it already.

@arthurfranca arthurfranca deleted the comment branch November 7, 2024 14:32
@Darecjo
Copy link

Darecjo commented Nov 9, 2024

Which clients are using this spec? Is there a list of it? And is this required to implement atm ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.