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

Deleting replaceable events #1263

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Deleting replaceable events #1263

wants to merge 1 commit into from

Conversation

fiatjaf
Copy link
Member

@fiatjaf fiatjaf commented May 27, 2024

No description provided.

@arthurfranca
Copy link
Contributor

From other discussions the idea was to delete the referenced replaceable up to the kind:5's .created_at (although it was never written in the NIP text).

Though your idea helps clients that don't fetch kind:5s. I think instead of introducing a new deleted tag, one can just set ["expiration", "<now>"] then empty .content and tags other than d.

@vitorpamplona
Copy link
Collaborator

vitorpamplona commented May 27, 2024

I don't like it because it still uses bandwidth to download the event with deleted tag. If you have a TON of deletions, the client will receive a ton of crap before useful events arrive.

Either way... Amethyst does both:
1 - Updates the event with empty tags and empty content
2 - ALSO send the deletion event with the e AND a tags.

1 is needed for relays that don't support replaceables (yes, those are out there) and 2 is needed to reduce bandwidth on those who do implement it.

Full deletions are extremely helpful on Generic Drafts #1124, since things are being deleted all the time. There is usually more deletions than drafts.

@staab
Copy link
Member

staab commented May 27, 2024

I prefer (and implement) a tag deletions, where the referenced event is deleted only if its created_at is before the delete's created_at. This makes deletes soft by default (unless Amethyst is out there stomping event content), which supports useful UX patterns like "undelete", where you would simply update the created_at of the replaceable event.

@fiatjaf
Copy link
Member Author

fiatjaf commented May 27, 2024

I don't like it because it still uses bandwidth to download the event with deleted tag.

Well, the idea is that relays will delete and not send these events to the client.

@fiatjaf
Copy link
Member Author

fiatjaf commented May 27, 2024

From other discussions the idea was to delete the referenced replaceable up to the kind:5's .created_at (although it was never written in the NIP text).

I prefer (and implement) a tag deletions, where the referenced event is deleted only if its created_at is before the delete's created_at.

This is better. I wish it had been written to the NIP.

@staab
Copy link
Member

staab commented May 27, 2024

This is better. I wish it had been written to the NIP.

I don't think it's too late, as far as I can tell relays don't seem to implement a tag deletions.

@fiatjaf
Copy link
Member Author

fiatjaf commented May 27, 2024

One advantage of my approach here of replacing the replaceable with an empty event would be that it would be backwards-compatible with relays that don't implement actual deletes.

If the relay deletes it deletes -- otherwise the relay will just serve an empty event (the content itself will be deleted).

This is in fact what Amethyst is already doing and what you should do if you want to erase the content from relays that don't implement deletion.

@staab
Copy link
Member

staab commented May 27, 2024

One advantage of my approach here of replacing the replaceable with an empty event would be that it would be backwards-compatible with relays that don't implement actual deletes.

I don't think this really solves anything because clients should be pulling deletes anyhow if they want a consistent UX. An empty event in contrast can look broken, and the client can't know why. This approach just leans into what's already bad about replaceables, in that they destroy information, making it harder for clients/users to know what happened.

@arthurfranca
Copy link
Contributor

One advantage of my approach here of replacing the replaceable with an empty event would be that it would be backwards-compatible with relays that don't implement actual deletes

I put my zero-weight vote on the ["expiration", "<now+10>"] approach instead of the ["deleted"] tag, as the expiration tag is already a hint for relays to delete events.

This goes in line with #236 vision.

@fiatjaf
Copy link
Member Author

fiatjaf commented May 27, 2024

I don't think this really solves anything because clients should be pulling deletes anyhow if they want a consistent UX.

I don't think they should be pulling deletes. It's enough for relays to delete things. Unless the client is storing events from others locally, in which case that makes sense.

Sure, you can pull in delete events, but that should not be a requirement.

An empty event in contrast can look broken, and the client can't know why.

Well, it accomplishes the goal of erasing the content. Is that worse than relying on or rendering an event with broken old invalid information that the creator had already expressed their desire to erase? Plus if it looks bad this client will have an easy time fixing it by just ignoring received events with the "deleted" tag.

This approach just leans into what's already bad about replaceables, in that they destroy information, making it harder for clients/users to know what happened.

This is true of every approach listed here. And it's a feature if you want that information destroyed.

@fiatjaf
Copy link
Member Author

fiatjaf commented May 27, 2024

This goes in line with #236 vision.

The only problem is that approach is bad, for the reasons I stated there.

I feel like the more "elegant" approaches are often not the best ones to choose in the context of Nostr.

@arthurfranca
Copy link
Contributor

@fiatjaf Well, the idea is that relays will delete and not send these events to the client.
[...] I don't think they [clients] should be pulling deletes. It's enough for relays to delete things. Unless the client is storing events from others locally, in which case that makes sense.

Oh, forgot about this detail. @staab has a strong argument. Even if client is storing just the logged-in user's own events, considering a multi-device/app scerario, a local DB of events from a currently offline app will need to know that the replaceable was deleted, when the app goes online.

The ["deleted"]/expiration strategy would only work in the above scenario if relay sent these events to clients when asked, atleast for a while. But then @vitorpamplona is right that it would waste bandwidth, for example, when client just wants unexpired events (for the feed, for example).

kind:5 with a tag deleting things up to its .created_at timestamp is the champion.

@staab
Copy link
Member

staab commented May 27, 2024

Well, it accomplishes the goal of erasing the content. Is that worse than relying on or rendering an event with broken old invalid information that the creator had already expressed their desire to erase? Plus if it looks bad this client will have an easy time fixing it by just ignoring received events with the "deleted" tag.

I suppose this would work. It just means that now there are two ways to delete something (three if you count e/a tags as different things).

@vitorpamplona
Copy link
Collaborator

vitorpamplona commented May 27, 2024

I prefer (and implement) a tag deletions, where the referenced event is deleted only if its created_at is before the delete's created_at.
This is better. I wish it had been written to the NIP.

Let's write and code this. It is by far the best solution.

I don't think they should be pulling deletes.

Agree, clients should not receive deletes at all.

@staab
Copy link
Member

staab commented May 27, 2024

Let's write and code this. It is by far the best solution.

Agree, clients should not receive deletes at all.

I think these positions are mutually exclusive. What if a client already has an event in its local relay and wants to find out if it's been deleted?

@vitorpamplona
Copy link
Collaborator

Between ["deleted"] and ["expiration", "<now+10>"], ["expiration", "<now+10>"] is better.

BUT, it is also common to have relays that do not implement expiration. So.. it's not foolproof. Deleting all tags and content is the only way to work correctly in all relays right now.

@vitorpamplona
Copy link
Collaborator

I think these positions are mutually exclusive. What if a client already has an event in its local relay and wants to find out if it's been deleted?

Deletion Events are generally returned if you do a #e filter by the event id. But they should not be returned in place of the real event in a common filter. If you do {ids:[$eventID]}, it should either return a valid, non-expired event or nothing. The deleted event should not be returned.

@fiatjaf
Copy link
Member Author

fiatjaf commented May 27, 2024

I suppose this would work. It just means that now there are two ways to delete something (three if you count e/a tags as different things).

To delete an e and an a are two completely different procedures.

And the solution:

a tag deletions where the referenced event is deleted only if its created_at is before the delete's created_at.

Is also very different (and much harder to code both on relay and on client side) than normal e deletions.

@mikedilger
Copy link
Contributor

As a data point, in chorus/pocket I coded what Staab describes in this comment above: /nostrability/nostrability/issues/38#issuecomment-2120827062

kind 5 events with 'a' tags delete only events before the deletion event.

@alexgleason
Copy link
Member

Oh, I didn't even realize my MR was competing with this one. But we should just merge: #1293

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.

6 participants