-
-
Notifications
You must be signed in to change notification settings - Fork 113
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
Nostr crossposting all item types #779
Nostr crossposting all item types #779
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the late review and thanks for the time you invested in this!
I think it would be great if SN is more visible on nostr and crossposting is a good way to achieve this. Also, I would like to interact more with nostr but all my "social media time" already goes into SN. So crossposting more stuff also seems to be a good way to "build up engagement" on nostr (which so far was lacking for me). Now to the review:
So far, I only read the code (see bullet point about testing) and left some comments. I have four things I want to mention here in a dedicated way:
1. Crossposting of polls
No clients seem to support nip41 poll notes yet. Though we are still able to link out to the note on nostr.com
Using a NIP that wasn't merged yet, doesn't look like it's going to get merged and looks like there are no clients that support makes me wonder if we should use it.
I have seen this though:
-- https://nitter.cz/lopp/status/1645514530368126978
Since it doesn't exactly match our poll system since it uses sats for votes (and not unique votes per stacker), it's also not a good match probably. But I like the idea. But I haven't found the NIP for this. And me liking this idea is probably totally unrelated off-topic for this PR. Just thought I'd mention that polls seems to exist on nostr in some way.
2. Crossposting of (short text) notes vs edit timer
Short text notes are not replaceable using the d
tag. Wouldn't the code as-is create a new short text note for each edit of link posts and polls?
Also, would be nice if we could use kind:30403
as specified in NIP-99 for bounties which are still editable. But this is related to my comment on crossposting link posts where I wondered if we should even crosspost anything which is still editable.
You mentioned this:
Since both poll and link crossposts are kind 1 they cannot be edited after crossposting. We can make these paramaterized replaceable events to enable editing I just wanted to stick close to the spec at first.
Using parameterized replaceable events might be an option but maybe we could avoid this whole edit problem all together by only crossposting when the timer ran out? As mentioned in my comment regarding link posts, this would require quite some architectural changes but might be worth it? Not sure, let me know what you think :)
3. Stackers should be able to override default relays to give them more control
Currently the user relays (if they have any set) are added to the default relays when crossposting. Do we want it to only post to the user's relays? Is the current copy here misleading in how it explains that?
Currently, I don't think it's clear that user relays are only added to the default relays. See comment for more info.
4. Testing
Is there an easy way to test this code without hitting public relays? How did you test this?
components/adv-post-form.js
Outdated
<Checkbox | ||
label={ | ||
<div className='d-flex align-items-center'>crosspost to nostr | ||
<Info> | ||
<ul className='fw-bold'> | ||
<li>crosspost this discussion item to nostr</li> | ||
<li>crosspost this item to nostr</li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As someone more interested in the tech, I would like to see what event kinds are used per post type.
But not sure how to tread the fine line between not confusing "normal users" and providing enough information for more "techy persons" to make it less confusing how nostr crossposting works; especially since we had support requests in the past regarding it.
But maybe it's not too confusing for anyone to simply mention here which event kind is used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mhh, I would make it context aware. Also, would use NIP-01
instead of nip01
to be consistent with the other lines that already mention NIPs. So if you're writing a discussion post, it would show
crosspost this discussion as a NIP-23 event
or while writing a link
crosspost this link as a NIP-01 event
etc.
(btw, polls use NIP-41 events, not NIP-44)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh good idea, this is what I have now.
https://github.com/stackernews/stacker.news/blob/fc8518050b6086f734caa6066f2b064cae16492a/components/adv-post-form.js
First line is dynamic and describes the nip being used for the item type:
function renderCrosspostDetails(itemType) {
switch (itemType) {
case 'discussion':
return <li>crosspost this discussion as a NIP-23 event</li>;
case 'link':
return <li>crosspost this link as a NIP-01 event</li>;
case 'bounty':
return <li>crosspost this bounty as a NIP-99 event</li>;
case 'poll':
return <li>crosspost this poll as a NIP-41 event</li>;
default:
return null;
}
}
The rest is static:
{me && itemType &&
<Checkbox
label={
<div className='d-flex align-items-center'>crosspost to nostr
<Info>
<ul className='fw-bold'>
{renderCrosspostDetails(itemType)}
<li>requires NIP-07 extension for signing</li>
<li>we use your NIP-05 relays if set</li>
<li>we use these relays by default:</li>
<ul>
{DEFAULT_CROSSPOSTING_RELAYS.map((relay, i) => (
<li key={i}>{relay}</li>
))}
</ul>
</ul>
</Info>
</div>
}
name='crosspost'
/>}
Hey @ekzyis thanks for the thorough review! I'll start by addressing these high level points since it might take some back and forth and then I'll go down the line for these smaller nits. 1:In regards to the poll nip not being merged it seems there's four options:
I like the lopp poll example a lot but like you I couldn't find the nip and I even couldn't find his original note to see what kind it was. Do you know what client that screenshot was from? 2.You are correct non paramaterized replaceable events if edited will then be crossposted a second time instead of being replaced in the current state. Currently the non replaceable crosspost items are polls/links. From what I can tell there is no such thing as a parametrized replaceable short note so the only option here is to make them kind 30023 long form replaceable events and allow editing. I don't think there is any way we could post the note after edit window without having the user pre-sign the note in which case if there was an edit we wouldn't be able to add the changes to the note without invalidating the sig. 3.Yeah I can update it to use ONLY the users relays if they have them set.
Given these considerations should we maybe add an option or something in settings for the user to choose whether crossposting will only use their set relays or use a combination of user + default relays? 4.For testing only in the early part of development I have the bucket relay and booger relay locally that I bounced between. Once I got further along I started testing with real relays using some fake profiles I generated (Alice, Bob, etc.) |
I think the frontend could presign an event for every edit and send these presigned events to the backend where we do the replacement? And when the edit timer ran out, the worker should be able to publish only the most recent edit (the final state) on the user behalf since they are presigned afaict. Let me know what you think, I just came up with this idea here so I probably didn't think this through. will respond to the other stuff later today hopefully, I just wanted to mention this already |
Great idea! I think this is a good middleground approach. Relying on paramterized replaceable events seems a bit fickle. It works well for nip23 long-form events but even then clients are taking a long time to implement it and I know a lot of relay runners don't like/support replaceable events. Since currently a user already has to resign on an edit of a crossposted item the UX should be the same if we have worker broadcast it after edit window. One thing we would want to consider is what to show in item-info for a crossposted item DURING the edit window. Currently it will either show |
I prefer option 2. I think that's also the easiest option. However, this might just be my personal bias on using something which isn't standardized yet since it's antithetical to standardization in the first place. Also, imo, we don't have to be ones who move fast when it comes to implementation of NIPs. But don't see this as diminishing your efforts! I was completely unaware of the status of nostr polls so thanks to your work, I am less unaware now :) I would also like to see polls crossposted but if nostr hasn't figured out how polls should work yet, I think efforts are better spent on contributing to getting a standard for polls out there. I subscribed to the PR now.
I also haven't found the NIP or the original note. Judging from the color theme, it might be a screenshot from Snort.
Your considerations make sense! Mhh, need to think about the UX more but maybe a checkbox which determines if the user relays overwrite the default relays or just append might be the way. The problem as you also nicely described is basically how much we can trust the users with control about nostr details:
This also raises the question if it should be possible to crosspost something to new relays if new relays were set but it's probably too early to think about details like this. Need to get this into the hands of users to play with first :)
Oh, interesting 👀 Do you know why? Isn't it just replacing old events in the database via an
I don't understand. There would be no nostr note during the edit window since nothing was crossposted yet 🤔
Ah, I see, you use this: https://github.com/coracle-social/bucket Didn't know about this relay, thanks, will check it out! Wanted to setup a local nostr relay for NWC in #749 anyway. Being able to test nostr stuff locally will only get more important moving forward. |
I talked to @huumn earlier today and he doesn't mind about the polls NIP and one note per edit. So we can ignore my comments about 1. and 2. -- I was probably thinking too far (scope creep). Regarding 3. (user relays override or append), we can probably do this in another PR if required. For now, they append and thinking more about this, I think your intuition is right. Most stackers probably don't care and just want to see their notes published reliably. Going to take another look at the code now but this is probably good to go then. Sorry for our split brains haha |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Damn, I accidentally used my real nostr key to test this haha
But looks good, just a few changes required:
- fetch policy
- toaster API
- comments in
crosspostItem
function
components/adv-post-form.js
Outdated
<Checkbox | ||
label={ | ||
<div className='d-flex align-items-center'>crosspost to nostr | ||
<Info> | ||
<ul className='fw-bold'> | ||
<li>crosspost this discussion item to nostr</li> | ||
<li>crosspost this item to nostr</li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mhh, I would make it context aware. Also, would use NIP-01
instead of nip01
to be consistent with the other lines that already mention NIPs. So if you're writing a discussion post, it would show
crosspost this discussion as a NIP-23 event
or while writing a link
crosspost this link as a NIP-01 event
etc.
(btw, polls use NIP-41 events, not NIP-44)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, found two things.
The missing crosspost checkbox during edits is a nitpick and can be fixed later imo.
But the missing case handling should be dealt with in some way.
If you want, I can take it from here, just let me know :)
Co-authored-by: ekzyis <27162016+ekzyis@users.noreply.github.com>
…e isOp to mine, remove unused noteId params
…str-tools, bounty to follw nip-99 spec
…finish removing job crosspostr code
Co-authored-by: ekzyis <ek@stacker.news>
…d mutiny strfry relay to defaults
…ure crosspostItem to use await with try catch
…ding outside of do ... while loop
3e031af
to
f92f333
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Played around with the changes again and works great now :)
Just noticed another thing that I didn't see before: the default value for the crosspost checkbox was using outdated code. Fixed that in 116edb4.
This PR enables nostr crossposting for all item types (excluding jobs) as well as providing a few fixes, tweaks, and a less intrusive implementation into the item forms.
High level summary:
User facing changes:
Internal code changes:
other notes:
stacker.news/components/adv-post-form.js
Lines 102 to 103 in c3df24b