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

Fix event verifying (by useing 1ma/secp256k1-nostr-php) #50

Closed github-tijlxyz closed 8 months ago

github-tijlxyz commented 9 months ago

Event verifying doesn't seem to work properly currently. Use this package fixes that, it is a php extension and does need to be installed manually tho.

Sebastix commented 9 months ago

Installing a PHP extension can be a challenge in some cases (like on a shared hosting environment where many php apps and sites life).

I'd rather see way to implement / load this as a PHP package so maybe we can figure out better why the current package is not working for verifying the events.

Sebastix commented 9 months ago

@1ma What's your take on this?

1ma commented 9 months ago

The lib's composer.json should not require uma/secp256k1-nostr directly because that's just the stubs for PHPStorm. It could be added in require-dev as a convenience for development time, and maybe listed as a suggestion in extension form (ext-secp256k1-nostr), but while secp256k1-nostr is not in available in PECL to make it easy to install it may be a bit confusing.

Code wise (new SchnorrSignature())->verify() should not be replaced with secp256k1_nostr_verify() either as that breaks swentel/nostr-php when the native extension is not installed. I'd do something like this to get a very nice speedup (and the correct result, in this case) if the extension is installed, and use the Schnorr PHP fallback if not:

if (extension_loaded('secp256k1-nostr')) {
    return secp256k1_nostr_verify($event->pubkey, $event->id, $event->sig);
}

return (new SchnorrSignature())->verify($event->pubkey, $event->sig, $event->id);

(Also mind that the parameters are not in the same order in both cases)

Besides all that, we need to validate if public-square/phpecc really has a bug and submit an issue there with reproducing steps if its the case. Adding a new requirement to an obscure native extension is not the proper way to deal with the bug.

Sebastix commented 8 months ago

The follow up and fixes are done in https://github.com/swentel/nostr-php/pull/51 so I will close this PR.