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-01: filters can query any tag #1147

Closed
wants to merge 1 commit into from

Conversation

alexgleason
Copy link
Member

Filters such as { "#proxy": ["https://example.com/123"] } are valid. It doesn't mean the relay will index them - this is never a guarantee (regardless of whether the tag is single-letter or not).

@arthurfranca
Copy link
Contributor

It seems to encourage a bad practice. Why filter by an unindexed tag, specially when client can't rely on broad relay support?

@alexgleason
Copy link
Member Author

NIP-01 doesn't say which tags should or shouldn't be indexed by relays, or guarantee that relays need to return anything at all.

matchFilters doesn't enforce any tag name.

It's not "encouraging a bad practice", it's just the truth.

@arthurfranca
Copy link
Contributor

NIP-01 doesn't say which tags should or shouldn't be indexed by relays

From NIP-01 text: "As a convention, all single-letter (only english alphabet letters: a-z, A-Z) key tags are expected to be indexed by relays"

This is the main detail to remember when picking tag names for a new event spec.

@alexgleason
Copy link
Member Author

Yes, I was the one who pushed for that: #541

The reason I opened this issue was because someone argued that { "#proxy": ["https://example.com/123"] } is not considered a valid Nostr filter according to the NIP-01 spec. Do you think matchFilters should throw if you try to match against that filter?

Like you said, the text already clarifies that only single-letter tags should be expected to be indexed. This is about what's considered a valid filter.

@arthurfranca
Copy link
Contributor

Most relays will simply ignore the #proxy part and use the rest of the filter keys.

In the context of a personal relay it could filter by that custom key though so I guess it could be a valid check for the matchFilters fn.

But imo shouldn't be included on NIP-01.

@alexgleason
Copy link
Member Author

Look at my change again. I am not suggesting that people filter with multi-letter tags. I am replacing an unnecessary restriction (simplifying) with something that is already defined in other places in the document.

@MerryOscar
Copy link
Contributor

I'm not sure if this is relevant but thought I'd share my experience with trying to reference podcasts in nostr events - #760

Although there is still no consensus on how to do this - the two options I came up with that seemed to fall within the existing nostr spec were either using r tags: [“r”, “podcast:guid:123”] or NIP-32 labels - ["l", "123", "podcast:guid"].

The reasoning behind this was so the tags would be indexed by relays - but it would be easier if the single tag filter requirement were relaxed in the way this PR proposes. Then the existing proxy tag ["proxy", "podcast:guid:123", "podcast"] would work and not all relays would have to index the tags - only the ones that cared about podcasts.

@mikedilger
Copy link
Contributor

As a datapoint, chorus skips these when deserializing from json (if there is more than one letter after the hash character). Which probably isn't the right place to do it, but that's an internal problem to chorus.

So no problems here.

@vitorpamplona
Copy link
Collaborator

+1

I kinda wish we had both .tags and .indexes and separate business logic from db-indexing labels. But it's too late for that.

Copy link
Collaborator

@Semisol Semisol left a comment

Choose a reason for hiding this comment

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

update NIP-11 with what tags for what kinds are indexed?

@alexgleason
Copy link
Member Author

update NIP-11 with what tags for what kinds are indexed?

That's a great idea @Semisol

@alexgleason
Copy link
Member Author

Closing because it's old and has merge conflicts, but it's still true.

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.

7 participants