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

Support NIP-26 in matchFilter #323

Closed
wants to merge 4 commits into from

Conversation

mariano-perez-rodriguez
Copy link

matchFilter fails for delegated events, by returning false when an author field is set in the filter but a delegate sends the event instead of the author in question proper.

This PR fixes this by including the delegate's pubkey in matchFilter's checks if one such delegate is found.

Additionally:

  • updated yarn
  • fixed linting issues
  • added tests for the new behavior

@agustinkassis
Copy link
Contributor

Very useful PR, otherwise NIP26 is being blocked.

@fiatjaf
Copy link
Collaborator

fiatjaf commented Dec 13, 2023

Sorry about this, but I don't like NIP-26 and I think it should be expunged out of the earth. I didn't even know we still had NIP-26 support in this library. It should probably be removed.

@alexgleason
Copy link
Collaborator

NIP-46 is a better solution to the problem.

@mariano-perez-rodriguez
Copy link
Author

Sorry about this, but I don't like NIP-26 and I think it should be expunged out of the earth. I didn't even know we still had NIP-26 support in this library. It should probably be removed.

We currently use it to be able to do stuff on behalf of the user in a non-interactive setting.
Furthermore, it is still listed as an optional NIP in the official documentation.

We find the protocol-based solutions the provided link alludes to sub-optimal for a number of reasons:

  • NIP-41 exists solely as a PR, and would not allow us to do things on behalf of nobody,
  • NIP-46 deals with an interactive setting only,
  • NIP-07 likewise deals only with an interactive setting,

Although I wholeheartedly agree that NIP-26 should get a "facelift", de facto deprecating it would leave the offline setting without a clear path forward in using NOSTR.

@mariano-perez-rodriguez
Copy link
Author

NIP-46 is a better solution to the problem.

Insofar as it allows us to sign a delegation, sure; otherwise, it falls short on our offline setting :(

@alexgleason
Copy link
Collaborator

What is your project? Why does it need to be offline? It's hard to imagine, because in that scenario I assume you have access to the private key and can simply just sign events.

@fiatjaf
Copy link
Collaborator

fiatjaf commented Dec 13, 2023

Nostr doesn't have to support all the use cases possible and imaginable. Life is full of tradeoffs, unfortunately.

@mariano-perez-rodriguez
Copy link
Author

What is your project? Why does it need to be offline? It's hard to imagine, because in that scenario I assume you have access to the private key and can simply just sign events.

We are the developers of LaWallet, a NOSTR-backed custodial wallet targeted towards grassroots federations.
We support the usage of BoltCard-compatible NTAG-424 NFC cards, these cards are not capable of performing asymmetric crypto, but are capable of doing MAC and AES.
On the backend, we ask the user for a delegation so that when the card is correctly validated, we can move funds in the end user's name.
All of this is communicated via NOSTR in order to provide for publicly verifiable transactions and an audit trail.

Since the card itself has no power to sign, we need a delegation stored in the backend in order for the transaction to go through.

I'm sure there are other ways of doing the same thing, but this is the one we consider to be the most "transparent".

@mariano-perez-rodriguez
Copy link
Author

Nostr doesn't have to support all the use cases possible and imaginable. Life is full of tradeoffs, unfortunately.

I 100% agree in this regard, but we're not asking NOSTR to support all possible use cases.

NIP-26 is not only part of the standard, but is actually part of nostr-tools already.
What's the point of having a standard if we're not willing to adhere to it?

Not only that, NIP-26 is correctly handled in at least one relay (nostream returns events signed by a delegate when asking for events authored by the delegator), and is inconsistently thrown away by nostr-tools.

We're not expecting NOSTR to handle every possible use case, but NIP-26 was a great idea, albeit a bit simplistic, and could do with an extension, instead of an outright de facto deprecation.

@fiatjaf
Copy link
Collaborator

fiatjaf commented Dec 13, 2023

NIP-26 is not only part of the standard, but is actually part of nostr-tools already.

Done, not anymore: 867aa11

What's the point of having a standard if we're not willing to adhere to it?

NIP-26 was a mistake. I supported it at the time, I thought it would be a good idea. I was wrong, but we don't have to continue committing the same mistake after we find out that it was a mistake. I will open a PR to the NIPs repo to unrecommend it and eventually delete it if possible.

@fiatjaf
Copy link
Collaborator

fiatjaf commented Dec 13, 2023

By the way, since you are doing some custom command-and-control setup for moving user funds in your closed wallet system I don't think you should worry about adhering to the NIP-26 standards. You can pretty much come up with a custom solution and that may be much better.

For example, if I understood it correctly (I probably didn't), you can just create new "custodial" keypairs for each users in your database and associate those in the background with the sovereign user keys and be done with it.

@mariano-perez-rodriguez
Copy link
Author

NIP-26 was a mistake. I supported it at the time, I thought it would be a good idea. I was wrong, but we don't have to continue committing the same mistake after we find out that it was a mistake. I will open a PR to the NIPs repo to unrecommend it and eventually delete it if possible.

I don't think it was a mistake per se, I really see the usefulness of if, but the spec does require some love.

What alternative for NIP-26's use case is there? The ones mentioned above fall short for NIP'26's application area (ie. offline delegations).

For example, if I understood it correctly (I probably didn't), you can just create new "custodial" keypairs for each users in your database and associate those in the background with the sovereign user keys and be done with it.

You did understand correctly :)
We can indeed do that, but "reifying" it as a proper NOSTR protocol element that relays can understand is the superior option in our opinion.

@fiatjaf
Copy link
Collaborator

fiatjaf commented Dec 16, 2023

What alternative for NIP-26's use case is there? The ones mentioned above fall short for NIP'26's application area (ie. offline delegations).

There is NIP-46 and not much more. Offline is out-of-scope, I think. Must be done by ad-hoc solutions like the one I suggested above for your specific use case.

We can indeed do that, but "reifying" it as a proper NOSTR protocol element that relays can understand is the superior option in our opinion.

I think you gain nothing by using a standard if you can't interoperate.

@fiatjaf
Copy link
Collaborator

fiatjaf commented Dec 16, 2023

I'll close this because I've deleted NIP-26 from this library, so there is no point. But we can continue the discussion if you want (until you finally realize I am right -- haha).

@fiatjaf fiatjaf closed this Dec 16, 2023
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.

None yet

4 participants