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

Add publish to relay method #13

Closed eko2one closed 1 year ago

eko2one commented 1 year ago

I have added publish as a first relay interaction method. Additionally, I have made some changes to the terminology, as explained in the comments within RelayTest. Please let me know what you think. It seems reasonable to me to stick with implementations in other languages.

swentel commented 1 year ago

Looks good! The test worries me a bit though: it makes an actual post to a relay. It is of course the best way to test :)

But, my initial gut feeling is that I would create a MockRelay (or TestRelay - naming, horrible!) implementing an interface which Relay can use too then, with publish being the first method. While I'm thinking out loud, I have this idea of maybe introducing a RelayResponse class which is returned in publish() and has some methods like isSuccess or isFail ? Or am I overthinking this?

// Some pseudo code as the RelayResponse class doesn't exist yet.
$relay = new MockRelay($relay);
$response = $relay->publish($event);
if ($response->isSuccess($response)) {
  print 'wicked'
}
else {
  print $response->getReason();
}

class TestRelay implements RelayInterface {

  public function publish($event) {
    if ($event['content'] == 'return_success_response') {
    // Pass a 'success' response
     return new RelayResponse(['OK', $event['id'], TRUE, '']]);
  }
   // and so on.
  }

}

Class RelayResponse {

  protected $success = FALSE;
  protected $reason = '';

  public function __construct(array $response) {
    if ($response[0] == 'OK' && $response[2]) {
     $this->success = TRUE;
   }
   else {
     $this->reason = !empty($resonse[3]) ?: 'Failed with no reason';
   }
  }

  public function isSuccess() {
    return $this->success === TRUE;
  }

  public function reason() {
    return $this->reason;
  }

}

Regarding evenlope / generateEvent() method: I agree that this might become very confusing fast :) Ironically, while reading NIP-01, especially https://github.com/nostr-protocol/nips/blob/master/01.md#from-client-to-relay-sending-events-and-creating-subscriptions it does make a bit of sense, especially in the publish context. But I would take this to https://github.com/swentel/nostr-php/issues/14

eko2one commented 1 year ago

@swentel Thank you for your suggestions/code examples. I have additionally added a condition to check for duplicates while generating the RelayResponse. Also I added the event_id to the response since I think it could be useful.

Please let me know if there is anything to add in this PR. And yes you are of course right, message is the correct terminology. Sorry for the confusion!

swentel commented 1 year ago

Very cool! I see improvements to document the methods, but I'm not going to hold that up for merging this one.

14 will become the big overhaul anyway, and after that one settles, I'm going to look at the documentation for it.

Merging now.

Sebastix commented 1 year ago

@eko2one @swentel

Have you looked at NIP-20 (I just did for the first time)? https://github.com/nostr-protocol/nips/blob/master/20.md

This NIP describes how relays should respond / sent a result back to a client.

eko2one commented 1 year ago

@Sebastix yep. The RelayResponse abstracts the relay response for easier handling. Its method isSuccess() will return true in case that the event has been saved to the relay and is not a dupilicate.