Explicit methods for "#e" and "#p" Filter attrs; Filter test suite

[kdmukai]

Explicit methods for "#e" and "#p" tag filters

• event_refs: the "#e" attr; list of event ids referenced in an "e" tag
• pubkey_refs: the "#p" attr; list of pubkeys referenced in a "p" tag

Rationale: users shouldn't have to know the protocol-level formatting details. Much more human-friendly to have explicit methods that describe exactly what the additional filter does.

Example usage

# Get replies to a specific note
Filter(event_refs=[note_id], kinds=[EventKind.TEXT_NOTE])

# Get DMs sent to target pubkey
Filter(pubkey_refs=[pubkey], kinds=[EventKind.ENCRYPTED_DIRECT_MESSAGE])

Preserves NIP-12 arbitrary single-letter tag filters

• add_arbitrary_tag will accept any single-letter tag and list of values

filter.add_arbitrary_tag('x', ["apples", "oranges"])

# json output will include:
{
    "#x": ["apples", "oranges"],
}

PEP-8 compliance on None tests

PEP-8 says that testing against None should use is or is not.

Opinionated minor refactor

Filter.IDs -> Filter.event_ids

---

Tests for Filter and Filters added

Fairly thorough tests of the updated approach to 'e' and 'p' tag filters, plus overall basic usage.

Grouped relevant tests together into their own classes primarily for organizational purposes. Also allows you to run just that one class' tests:

pytest test/test_filter.py::TestFilters

Note: the "module"-level fixtures could really just be globals, but fixtures are widely preferred over globals. I don't love that the fixture approach added a little more repetitive boilerplate (including them as input args in every test method). But I opted to follow the best practice, even though I don't really see what it gained us here!

---

Further Work

• I recommend that Filter be refactored into a dataclass (as I've done elsewhere); the unnecessary boilerplate in __init__ can be completely avoided since no real work is being done there.
• I recommend that Filters be renamed to FilterList. It's more explicit and IMO more pythonic. Reduces (the admittedly minor) confusion with the Filter class.

[jeffthibault]

Correct me if I'm misunderstanding, but I think these changes will break how we handle NIP-12

[kdmukai]

Ah, I see @jeffthibault. I didn't realize before that the expected usage was something like:

Filter(
    tags={"#e": [<event_id>, ...], "#p": [<pubkey>, ...]}
)

To me that requires too much direct knowledge of the Nostr spec (as evidenced by my own gaps here!). I had been trying:

# wrong format, no "#"
Filter(tags=[['e', <event_id>, ...], ['p', <pubkey>, ...]])

Or something along those lines because I was expecting it to match the format of the Event.tags.

I think for both Filter and for Event it would be nice to have explicit methods to add things like 'p' pubkey refs that abstract away the actual format details. But still leave the option open to manually add any arbitrary tags the user might need. And if other tag types gain more prominence, they could also gain explicit method support:

# For now:
Filter(tags={"#t": [list]})

# If promoted
Filter(hashtags=[list])

I'll rework this PR and see if I can strike the right balance.

[kdmukai]

Updated. Preserved the explicit methods for 'e' and 'p' but added add_arbitrary_tag.

Breaking change: Filter input arg ids renamed to be more explicit: event_ids.

Bugfix in RelayManager test suite (Event.id was being set before created_at was changed).

[armstrys]

Ahh good catch on the event ID. I’m not sure how I missed that. Can we move the compute is line below setting the created_at time? I think that change was introduced in my pull request here #24

[jeffthibault]

re: event id bug, I noticed that bug as well after I merged #24. Please see c68ebe5, where I fixed it.

[kdmukai]

(I made a mess of my attempts to restore #24 in this branch. Fixed it all now and squashed those commits)

[jeffthibault]

Thanks @kdmukai, this is awesome! I think it makes sense to abstract away as much of the spec as possible and this PR does that.

Here's one question I have, let's say I want to get events with the delegation tag. By restricting the Filter arbitrary tags to only one character, does that prevent me from being able to make that request?

[kdmukai]

Will address other arbitrary tags like delegation.

I think my filter match logic on tags is wrong. I got the Event.tag format confused with the filter format.

# Event referencing multiple people
"tags": [
  ['p', <pubkey>, <recommended relay URL>],
  ['p', <pubkey>, <recommended relay URL>],
]

# ...versus an AND filter that requires both people to be referenced
"#p": [<pubkey>, <pubkey>]

I'm iterating over a single Event 'p' tag when searching for the two pubkeys in my filter. I should be looking across all Event 'p' tags. I'll revert that part to how it was originally (which looks correct) but with a nod toward the possible inclusion of delegate and other tags that aren't prefixed with "#".

Working on a fix and more robust test cases that better reflect real-world Events.

[armstrys]

This isn't a critical change, but might help make the code more clear. I am trying to track down instances where mutable objects are used as default args as that seems to be causing unexpected behavior. Which lead me down a rabbit hole here...

It turns out to not be an issue for the Filters argument initlist because UserList recreates the list object upon init. In the UserList class the default initlist is None as opposed to []. It might make sense to also default to None in Filters just to make this behavior clear.

[kdmukai]

I'll revert that part to how it was originally (which looks correct) but with a nod toward the possible inclusion of delegate and other tags that aren't prefixed with "#".

Looks like the original code has two bugs.

1. It's not matching up the f_tag with the e_tag in the final check. So if the event has an additional tag that isn't in the filter, it fails the match.
2. Even within the same tag type, if the event has an additional entry (e.g. a second p reference) where the value is not in the filter, it fails the match.

from nostr.event import Event
from nostr.filter import Filter

event1 = Event(
    public_key="somepubkey",
    content="Anyone know what 2+2 = ?",
)
event2 = Event(
    public_key="someotherpubkey",
    content="It's 4!",
    tags=[
        ['e', event1.id],      # Replies reference which note they're directly responding to
        ['p', event1.public_key],  # Replies reference who they're responding to
    ],
)

filter = Filter(
    tags={'#p': [event1.public_key]}
)
assert filter.matches(event2)  # fails due to 'e' tag

event2 = Event(
    public_key="someotherpubkey",
    content="It's 4!",
    tags=[
        ['p', event1.public_key],  # Replies reference who they're responding to
    ],
)
assert filter.matches(event2) # succeeds

event2 = Event(
    public_key="someotherpubkey",
    content="It's 4!",
    tags=[
        ['p', event1.public_key],  # Replies reference who they're responding to
        ['p', "some_other_pubkey"],
    ],
)
assert filter.matches(event2) # fails due to second 'p' tag

[jeffthibault]

Yup, thanks for the clear explanation. I'm not surprised there were bugs. I have always thought that handling the tags was kinda tricky.

[kdmukai]

Updated!

Tag filtering should now:

• address the above bugs in the prior implementation (explicitly tested in the test suite).
• Allow for arbitrary tags that are not single-letter NIP-12 filters (e.g. delegation)

Tests updated with much better organization and much better readability. Ditched the pytest.fixture approach.

[jeffthibault]

Thanks @kdmukai! This is fantastic and fixes up the filter classes nicely.

Tested ACK.