nostrver-se / nostr-php

PHP helper library for Nostr https://nostr-php.dev
https://nostr-php.dev
MIT License
53 stars 15 forks source link

Introduce Event, Message and CommandResult classes #14

Closed eko2one closed 1 year ago

eko2one commented 1 year ago

What do you think, is it too early to refactor event? It might be more difficult to refactor later. On the other hand the code complexity could increase now a bit.

I am not sure what makes more sense at this time: class, interface or trait? A nice thing would be to cover the different event types that result from the spec.

swentel commented 1 year ago

Very good question. Not an easy answer :)

https://github.com/swentel/nostr-php/pull/13 shows that we need to starting thinking about this now already as there's a huge possibility we are going to confuse ourselves, especially with terminology.

I do think an interface will probably help us making sense of all the event / kinds - just brainstorming here a bit:

$note = new PublicEvent(); // implements EventInterface, extends EventBase;
$note->setContent('Hello I am public '); // Base method on EventBase
$note->addTag(k, v);

$dm = new DirectMessageEvent(); // implements EventIterface, extends EventBase or PublicMessage?)
$dm->setContent('hello, I will be encrypted'); // Overrides to encrypt

$zap = new ZapEvent(); // implements eventInterface and ZapInterface
$zap->addBollt1('value'); // on the zapInterface, calls addTag internally.

Re: the generateEvent() method which now is on the Sign class: that doesn't belong there, but it doesn't belong on the Event class either. My personal opinion at this moment is a Message class, taking inspiration from https://github.com/nostr-protocol/nips/blob/master/01.md#from-client-to-relay-sending-events-and-creating-subscriptions

Eventually, sending something to the network could become something like this, which kind of makes sense to me, at this moment at least :)


$event = new PublicEvent();
$event->setContent('Well hello');
// Or
$event = new DirectMessageEvent();
// Or
$event = new IHaveMyOwnRelayWhichAcceptsUnicornEvent();

// Signing, only needed for an EventMessage
// Note, maybe signEvent() just becomes sign() and doesn't return either, it's an object now.
// We can also add a 'isSigned()' method on the event base class which can be used
// in the EventMessage() class 
$signer = new Sign();
$event = $signer->signEvent($event, $private_key);

// The messages classes probably don't need a base class as the contructor will be different, 
// e.g. Req and Close would set the subscription id?
// or pass that one along in the constructor?
// Also this could throw an exception in case the event is not signed
$message = new EventMessage($event); // wraps into [EVENT, ...]; eventually
// Or
$message = new ReqMessage(); // wraps INTO [REQ] implements MessageInterface and ReqInterface
$message->addFilter(); // from the ReqInterface
// Or
$message = new CloseMessage(); // wraps INTO [CLOSE] eventually.
// Or
$message new WhateverSomeoneElseThinksOfClass(); // Let them wrap themselve

$relay = new Relay($relay);
// not the publish() method, but simply send().
// in this method we call $message->getContent() (or getData())
$relay->send($message);

Feedback very much welcome!

swentel commented 1 year ago

Additionally, we can also introduce a socketClientInterface so developers can swap the default socket client we ship with, because well, you never know :)

swentel commented 1 year ago

Renamed the title to clarify better what we really want - at least in my head. I think we should start simple and not introduce everything. So as far as i can see this would be fine:

Naming is not written in stone, I'm still letting event and kind wander around in my head, that's the most intriguing one for me.

I think it might be good to look at other projects how they are naming things - I've scanned https://github.com/nbd-wtf/nostr-tools a few times now already which makes me lean to settle on EventInterface. It doesn't have the concept of Message though, that is covered by the relay publish method, which, in a way makes sense too.

swentel commented 1 year ago

Note: currently working on a proof of concept

eko2one commented 1 year ago

I have read through NIPs and also dived a bit into the current issues/discussions. I think in order to be as flexible but also as adequate with the specs, we have to align to the following minimum structure:

Client sends three types of messages to Relay: Message of type Event Message of type Request Message of type Close

The Message Event can have one of three Event types: Regular Event Ephemeral Event Replaceable Event (static or parametrized)

To satisfy this structure I would confirm most of your suggestions and specifically prefer an Implementation as follows:

For messages: MessageInterface EventMessage RequestMessage CloseMessage

For Event Messages: EventInterface RegularEvent EphemeralEvent ReplacableEvent

I am omitting BaseEvent and TextNote here since I think we can decide later on about the details.

Please also lets keep in mind that: There is a possible backwards compatible spec clean-up. (https://github.com/nostr-protocol/nips/issues/236) There are problems with Direct Messages and they seem not ready for a proper implementation (https://github.com/nostr-protocol/nips/issues/107).

Hope I did not make more confusion with this :) Sorry for late response - doing my best to get back on track after a quick flu. Let me know what you think.

swentel commented 1 year ago

I got a little confused about the 'For Event Messages' label, but I have a feeling you mean 'For Event Types' there right?

We agree on the Message class, that's a good thing, I don't have to change anything then locally (it's currently ridiculously easy anyway, having 1 method being 'generate' which wraps an EventInterface into the format that is suitable to send to a relay).

Regarding the event types: Interesting! I didn't know about NIP-16 which defines those. After reading that nip, in combination with https://github.com/nostr-protocol/nips/blob/master/README.md#event-kinds, it made me realize I miss something currently. To give you a sense, this is what I'm able to code with what I have locally right now:

$note = new Note();
$note->setContent('Hello world');
$signer = new Sign();
$signer->signEvent($note, $private_key);
$eventMessage = new EventMessage($note);
$relay = new Relay($websocket, $eventMessage);
$response = $relay->send();

But there's a flaw:

Which brings me to the suggestion then of RegularEvent, etc. The only difference, unless I'm wrong, is basically the value of the kind property. Which is why my gut feeling at the moment tells me that they don't fit in - yet. But I might be missing something completely. e.g. imagine that RegularEvent would throw an exception in case the kind property is 0, you still can't send a text note (Kind 1), right?

Thanks for thinking along, this is super important, and I think we're close - I'm going to let it sink in a bit more, and especially, have good night of sleeping too :)

Unrelated to the naming: setContent() now only accepts a string, but kind 0 should probably allow accepting an array too, so it can be encoded to JSON - although one can argue about this one: if someone wants tot send kind 0, they are responsible of serializing it themselves first, otoh, having a dedicated MetadataEvent class would allow us to verify the incoming array having the proper keys - but I'm drifting off on a detail now haha.

(Note: I have some other unrelated changes too locally, like a RelayInterface and ResponseInterface, but those are easy haha)

Sebastix commented 1 year ago

Let me make an attempt in terms of thinking along, I'm very interested how all things are formed (I'm learning a lot here for (re)improving my raw PHP skills while reading your comments).

What about changing the Note class to Message class, because for what I'm understanding now while still reading all the NIPS...everything is basicly a message which are sent from client to relay and vica versa. On the Message class, you can call the setKind(). By setting a kind to a message, the message object would transform to class which implements the NIP. So maybe there is a way working with NIPInterfaces? For example a NoteInterface class (which extends the MessageInterface class) which is related to NIP-1. Note: multiple kinds can refer to the same NIP. 🤔

Does that make any sense?

swentel commented 1 year ago

Hmm, my brain raised a red flag immediately on using Message there, I'm pretty sure that's the wrong one, referring to https://github.com/nostr-protocol/nips/blob/master/README.md#message-types for that. Yes, what is sent to a relay is a message, but the message contains either an event, filters or nothing.

Also, NIP-01 clearly states: The only object type that exists is the event

It is clear though that everyone is struggling with Event, Event Type and Kind :)

https://github.com/nostr-protocol/nips/blob/master/README.md#event-kinds gives the impression that they seem to settle on Event Kind? My mind always seems to translate that to Event type, but I think that's my Drupal background banging on the door all the time.

Now, after some thinking, I should probably drop my current EventBase class, rename it to Event, and not make it abstract. That, way, everyone can simply do this:

$event = new Event(); $event->setKind(12345)->setContent('This is my special kind content')->addTag("s", "my special tag");

Specific event (kinds) can be added, extending on Event: e.g. new Zap() with a zapInterface having a setBolt11() method.

My gut feeling can live with that to be fair. As always, feedback welcome :)

Sebastix commented 1 year ago

Agree, that makes much more sense!

How are other existing Nostr libs are solving this? Maybe we can get some inspiration from them. Like https://github.com/nbd-wtf/nostr-tools and I also came across https://github.com/ethicnology/dart-nostr.

swentel commented 1 year ago

Ok, current local code - note that I've made everything 'singular' (e.g. Keys -> Key)

Real live integration in the CLI client I'm creating (see #15)

$event = new Event();
$event->setContent($content);
$event->setKind(1);
$signer = new Sign();
$signer->signEvent($event, $private_key);
$eventMessage = new EventMessage($event);
$relay = new Relay($socket, $eventMessage);
$response = $relay->send();
if ($response->isSuccess())
{
    print "Send to Nostr!\n";
}
else
{
    print "Something went wrong: " . $response->message() . "\n";
}

I think this is a good start no? :)

One last thing that kind of diverges from Nostr terminology currently is our RelayResponse. In NIP-20, they introduce Command Results - https://github.com/nostr-protocol/nips/blob/master/20.md If we would follow that pattern, which in a way makes sense, RelayResponse would change to CommandResult. It feels a bit weird now, but I can get used to it, but, more importantly, we don't invent something.

So if everyone can agree with the current code + the change to CommandResult, then I'll commit!

eko2one commented 1 year ago

Looks all good to me! Thanks

swentel commented 1 year ago

Alright, committed, will test and commit the #15 script now too.

Thanks everyone!

Sebastix commented 1 year ago

Nice work!! Just discovered this tool: https://play.phpsandbox.io/swentel/nostr-php to experiment / test with.