-
-
Notifications
You must be signed in to change notification settings - Fork 137
Add NIP-C7 support #1067
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
base: master
Are you sure you want to change the base?
Add NIP-C7 support #1067
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.
Also update crates/nostr/CHANGELOG.md
Also I think you should parse the content using Or maybe add it if it doesn't contains it? the @yukibtc what do you think? |
Okay @TheAwiteb, I updated the changelog and code. I am new to Rust and Nostr so thanks for the help. I think it should automatically add the event to the context if it's not there already. I played around with NostrParser a bit but I think something like this could be simpler:
I don't think we need NostrParser here since all we need to do is check the start of a string. Using NostrParser would be very similar to this with the only difference being that instead of checking the start of a string we parse the whole string, get the first element, and check if its the correct nevent. So let me know what you think, maybe I have overlooked something. |
Apologies for the delay in responding! You're absolutely welcome, happy to help.
We do need the |
Also you can write this content_string = format!("nostr:{}\n{}", nevent, content_string); like this content_string = format!("nostr:{nevent}\n{content_string}"); in your new change, it's more readable. |
Okay so something like this would work, right?
It basically just parses the content, gets the first token, checks if it is an nevent and if it has the same event id as the reply_to event, and then it adds the nostr uri to the content based on those conditions. What should I do with the relays in Nip19Event though? Should I leave them empty, add only the optional relay_url (if it is not None), or add a third parameter (a vec of RelayUrls) for it? Thanks! |
Instead of an empty Also remove the And don't just check the first token, the And don't Thank you for your contribution. |
Yeah, I think make sense to automatically add it in the content if it doesn't exist. |
@SubatomicPlanets, I would rewrite it like this, for better readability: impl EventBuilder {
// ...
/// Chat message reply
///
/// <https://github.com/nostr-protocol/nips/blob/master/C7.md>
pub fn chat_message_reply<S>(
content: S,
reply_to: &Event,
relay_url: Option<RelayUrl>,
) -> Result<Self, Error>
where
S: Into<String>,
{
let mut content: String = content.into();
// Check if the content starts with a `nostr:` event uri
if !starts_with_nostr_event_uri(&content, &reply_to.id)? {
let nevent = Nip19Event {
event_id: reply_to.id,
author: Some(reply_to.pubkey),
kind: Some(reply_to.kind),
relays: relay_url.clone().into_iter().collect(),
};
let nevent_uri: String = nevent.to_nostr_uri()?;
content = format!("{nevent_uri}\n{content}");
}
Ok(
Self::new(Kind::ChatMessage, content).tag(Tag::from_standardized_without_cell(
TagStandard::Quote {
event_id: reply_to.id,
relay_url,
public_key: Some(reply_to.pubkey),
},
)),
)
}
}
fn starts_with_nostr_event_uri(content: &str, event_id: &EventId) -> Result<bool, Error> {
let token: Option<Token> = NostrParser::new().parse(content).next();
if let Some(Token::Nostr(nip21)) = token {
if let Some(id) = nip21.event_id() {
return if &id == event_id {
Ok(true)
} else {
Err(Error::) // Add an error to indicate that the event in the content is not right
};
}
}
Ok(false)
} @SubatomicPlanets @TheAwiteb, what do you think? |
It's cool, but I don't like assuming that the quote are in the binging, it can be in the end. my previous review:
|
Ah, I've missed it. Otherwise, what if we specify in doc that the content must be just the message without the |
I think adding it if it's not exist is better, because maybe I want add a custom relays to it or the kind and the public key, or maybe not :). We need to make it clear that if the content doesn't contains the |
@SubatomicPlanets If you need any help let me know |
So, instead of the fn has_nostr_event_uri(content: &str, event_id: &EventId) -> bool {
const OPTS: NostrParserOptions = NostrParserOptions::disable_all().nostr_uris(true);
let parser = NostrParser::new().parse(content).opts(OPTS);
for token in parser.into_iter() {
if let Token::Nostr(nip21) = token {
if let Some(id) = nip21.event_id() {
if &id == event_id {
return true;
}
}
}
}
false
} |
Yes, but this version is much simpler fn has_nostr_event_uri(content: &str, event_id: &EventId) -> bool {
const OPTS: NostrParserOptions = NostrParserOptions::disable_all().nostr_uris(true);
NostrParser::new().parse(content).opts(OPTS).any(
|token| matches!(token, Token::Nostr(uri) if uri.event_id().as_ref() == Some(event_id)),
)
} |
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.
LGTM. Just this minor changes
/// Chat message reply | ||
/// | ||
/// <https://github.com/nostr-protocol/nips/blob/master/C7.md> | ||
#[inline] |
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.
@yukibtc Do you think the #[inline]
should be removed? Because it's not a small function anymore
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.
Waiting for @yukibtc about removing the #[inline]
from chat_message_reply
function, it's not small anymore.
nostr: add NIP-C7 support
Added a new ChatMessage kind and 2 new builders: chat_message and chat_message_reply.
The code is structured similarly to text_note and text_note_reply.