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

Add NostrEventBuilding protocol to enable flexible composition of shared patterns across different kinds of NostrEvents #175

Merged
merged 2 commits into from
Sep 4, 2024

Conversation

tyiu
Copy link
Contributor

@tyiu tyiu commented Aug 9, 2024

#170

This PR uses a combination of different patterns (builder, inheritance, protocols, generics) to enable what I think is a flexible, powerful, easy-to-use framework to build NostrEvents.

It allows maintainers of the SDK to compose shared logic / components together, e.g. NIPs that are used in multiple event kinds. Duplicate code no longer needs to be copied into multiple EventCreating functions.

It allows developers using the SDK to build NostrEvents of any event kind, even if it is not yet supported in the SDK. They can make additions on top of what the SDK provides, or construct their own custom event. They can also not be constrained to what I believe in my opinion is a rigid EventCreating protocol API, where they need to pass in too many parameters into one single function just to be able to create the NostrEvent that they want.

I'm proposing we eventually deprecate the EventCreating protocol in favor of this NostrEventBuilding protocol.

Note: for full transparency, I tried to integrate Apple's result builder pattern unsuccessfully. Although it's more idiomatic to the Swift language, I actually found it to be extremely difficult to implement for this use case. It's too rigid to allow for subclassing and composability. It also constrains you to build your object within a single block. Lastly, error handling was quite difficult. You have to do some really weird things to bubble errors up the stack to the caller.

@@ -39,6 +39,14 @@ public class CustomEmoji: CustomEmojiValidating, Equatable {
}
}

public protocol CustomEmojiBuilding: NostrEventBuilding {}
public extension CustomEmojiBuilding {
func customEmojis(_ customEmojis: [CustomEmoji]) -> Self {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This example illustrates why using a builder pattern is powerful for something like Nostr. There are all kinds of shared event creation patterns across different Nostr event kinds. Each kind-specific NostrEvent.Builder subclass can easily integrate with the protocol and get the implementation for free. In the case for custom emojis, it can be used across three different event kinds -- I've only integrated with two of them (kind 0 metadata and kind 1 text notes).

@@ -131,66 +136,99 @@ public extension EventCreating {
///
/// See [NIP-01](https://github.com/nostr-protocol/nips/blob/master/01.md)
/// See [NIP-10 - On "e" and "p" tags in Text Events (kind 1)](https://github.com/nostr-protocol/nips/blob/master/10.md)
@available(*, deprecated, message: "Deprecated in favor of TextNote.Builder.")
func textNote(withContent content: String, replyingTo repliedEvent: TextNoteEvent? = nil, mentionedEventTags: [EventTag]? = nil, subject: String? = nil, customEmojis: [CustomEmoji]? = nil, signedBy keypair: Keypair) throws -> TextNoteEvent {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As a maintainer, there's no need to worry about adding to a trainwreck of a function if we need to add new functionality.

As a developer using the SDK, I can now customize the specific event kinds in my client with the NostrEvent.Builder, especially if the SDK hasn't caught up to implementing the latest NIP updates. It gives me flexibility to create my own event, or combine my own schema modifications to what the SDK provides.

@tyiu tyiu force-pushed the tyiu/builders branch 5 times, most recently from 46eee0d to 9030599 Compare August 11, 2024 18:55
@@ -51,7 +51,7 @@ public class NostrEvent: Codable, Equatable, Hashable {
case content
case signature = "sig"
}

init(id: String, pubkey: String, createdAt: Int64, kind: EventKind, tags: [Tag], content: String, signature: String?) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I sort of want to eventually get rid of this initializer. We shouldn't make it easy to handcraft your own event by also allowing you to set id and signature so freely. They should be computed.

@tyiu tyiu force-pushed the tyiu/builders branch 2 times, most recently from 94d1fdf to ef842a8 Compare August 11, 2024 19:04
@tyiu tyiu marked this pull request as ready for review August 11, 2024 19:09
@tyiu tyiu requested a review from bryanmontz August 11, 2024 19:09
…red patterns across different kinds of NostrEvents
@tyiu tyiu force-pushed the tyiu/builders branch 2 times, most recently from 04d2f54 to b50b7fe Compare August 12, 2024 13:36
@@ -96,6 +96,16 @@ public struct EventTag: RelayProviding, RelayURLValidating, Equatable {
return EventTagMarker(rawValue: tag.otherParameters[1])
}

/// The pubkey of the author of the referenced event.
public var pubkey: String? {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

pubkey was added a few months ago to text note event tags.
nostr-protocol/nips@d688998

@tyiu tyiu merged commit 4e65dfa into main Sep 4, 2024
8 checks passed
@tyiu tyiu deleted the tyiu/builders branch September 4, 2024 16:07
@tyiu
Copy link
Contributor Author

tyiu commented Sep 4, 2024

I've merged without code review because I'm currently the only person actively working on the SDK at the moment.

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.

1 participant