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

LUD-18 Service Support #518

Merged
merged 17 commits into from
Oct 3, 2023
Merged

LUD-18 Service Support #518

merged 17 commits into from
Oct 3, 2023

Conversation

SatsAllDay
Copy link
Contributor

@SatsAllDay SatsAllDay commented Sep 24, 2023

This PR implements LUD-181, namely incorporating payer data in payRequest invoices.

I have made all identification methods optional (mandatory: false), this can change over time if needed.

Footnotes

  1. LUD-18 Spec

@SatsAllDay SatsAllDay marked this pull request as ready for review September 25, 2023 16:03
Copy link
Contributor Author

@SatsAllDay SatsAllDay left a comment

Choose a reason for hiding this comment

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

It feels weird not doing anything with the payer data, but I guess that's not really part of the spec. Just hashing it into the invoice, if provided. Let me know what you think!

@@ -62,17 +66,15 @@ module.exports = withPlausibleProxy()({
{
source: '/.well-known/:slug*',
headers: [
...corsHeaders
...corsHeaders,
noCacheHeader
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 set no caching here since we include a k1 in the response payload that's unique for each request.

@@ -83,7 +85,8 @@ module.exports = withPlausibleProxy()({
{
source: '/api/lnurlp/:slug*',
headers: [
...corsHeaders
...corsHeaders,
noCacheHeader
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above

Comment on lines 19 to 18
k1Cache.set(k1, username)
// Invalidate the k1 after 10 minutes, if unused
setTimeout(() => {
k1Cache.delete(k1)
}, 1000 * 60 * 10)

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 went with an in-memory cache because it felt like overkill to store this in the DB. However, I am not certain of SN's deployment architecture (i.e. are there several server instances behind a load balancer?), so if we need to push the k1 cache to the DB to share across instances, I can make those changes. Just let me know.

Copy link
Member

Choose a reason for hiding this comment

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

Yes we have serveral servers behind a load balancer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I'll move it to the DB, then. Thank you for the confirmation.

@SatsAllDay SatsAllDay changed the title WIP: LUD-18 Support LUD-18 Support Sep 25, 2023
@huumn
Copy link
Member

huumn commented Sep 26, 2023

It feels weird not doing anything with the payer data

Is the intent to extend this to do more? Because I agree. I'm not sure we're gaining anything here yet

@SatsAllDay
Copy link
Contributor Author

It feels weird not doing anything with the payer data

Is the intent to extend this to do more? Because I agree. I'm not sure we're gaining anything here yet

I didn't have any specific plans. I think what we gain with this is spec compliance, so the question may really be, what does the spec give us? My understanding is that there's value is hashing payer identifying data into the invoice so that you can provide proof of payment, should you need to (meaning, alice payed this invoice, not bob).

We could store the payer data as a string in the invoice table and show it to the payee on the invoice instance page, tucked in an accordion or something. But I'm not sure we should, because I'm not sure wallets indicate whether this information is exposed to the payee like they do with LUD-12 comments. See Open Bitcoin Wallet for an example of this, I found this on DarthCoin's substack. Alby doesn't seem to make this distinction in their UX, so it's hard to say...

@huumn
Copy link
Member

huumn commented Sep 27, 2023

I could see this being more useful in the other direction: when someone goes to pay SN, we give them a photo and stats and stuff so they can verify it's going to the right place ... or when someone is paying out of SN, we give them a photo and stuff about the person they're sending to.

@SatsAllDay
Copy link
Contributor Author

I could see this being more useful in the other direction: when someone goes to pay SN, we give them a photo and stats and stuff so they can verify it's going to the right place ... or when someone is paying out of SN, we give them a photo and stuff about the person they're sending to.

I need to think about this some more, but this sounds more like recipient (of the sats) validation vs sender (of the sats) validation, which, AFAICT, isn't really what LUD-18 is about? I might just need to re-read the above a few times for it to click...

@huumn
Copy link
Member

huumn commented Sep 27, 2023

Yeah it's probably another spec item.

@Darth-Coin
Copy link

Darth-Coin commented Sep 27, 2023

Here you can test LUD-18 very nice.
https://chat.blixtwallet.com/
Is a very cool and useful feature for a wallet or service.
Blixt wallet have it also implemented.
Hampus (Blixt dev) is actually the one that create this LUD-18

@SatsAllDay
Copy link
Contributor Author

SatsAllDay commented Sep 27, 2023

Here you can test LUD-18 very nice. https://chat.blixtwallet.com/ Is a very cool and useful feature for a wallet or service. Blixt wallet have it also implemented. Hampus (Blixt dev) is actually the one that create this LUD-18

Thank you Darth, that is a neat place to experiment. I used Alby to send and I can see my LUD-12 message (comment) and also the name I sent along (LUD-18).

So from the SN perspective, perhaps we could store the identification info in our invoice DB, and extract any human-readable identification methods and include those in invoice notifications and listings? I was just concerned about privacy, exposing payer data to the payee, but I guess that's the point?

Let me know what you think, @huumn , I can update the PR to reflect those changes to make this more useful!

@huumn
Copy link
Member

huumn commented Sep 27, 2023

Privacy was my concern as well, but if the payer is opting in I guess it's what they want.

we could store the identification info in our invoice DB, and extract any human-readable identification methods and include those in invoice notifications and listings

Let's do it

@SatsAllDay SatsAllDay marked this pull request as draft September 27, 2023 15:13
@hsjoberg
Copy link

hsjoberg commented Sep 27, 2023

what does the spec give us?

@SatsAllDay So I think for stacker.news, it could be nice to have the payerdata visible in the transaction log, as you guys have already talked about. Specifically, name and identifier(meaning Lightning Address) has some support across the ecosystem with OBW, Blixt, etc supporting name and Alby, BitBanana (and probably others) supporting identifier too.

Another neat thing would being a consumer of LUD-18.
Meaning that when you pay out to a Lightning Address or LNURL-pay code, you could provide the @stacker.news Lightning Address (at user's discretion of course) to the payer.
As far as I understand, this current pull request covers being a LUD-18 service (in LNURL terms). you can also be a wallet. Just a friendly suggestion for the future.

As @Darth-Coin mentioned, supporting LUD-18 name as a wallet could be tried on https://chat.blixtwallet.com, paying via Lightning Address [email protected].
Hmm I should actually look into supporting identifier & email too there!

I could see this being more useful in the other direction: when someone goes to pay SN, we give them a photo and stats and stuff so they can verify it's going to the right place ... or when someone is paying out of SN, we give them a photo and stuff about the person they're sending to.

@huumn If I understand you correctly, these things can be provided by the normal LUD-06 LNURL-pay protocol.
Specifically an image/jpeg or image/png entry can be provided in the metadata prop.

@huumn
Copy link
Member

huumn commented Sep 27, 2023

@hsjoberg you rock!

@Darth-Coin
Copy link

@hsjoberg you rock!

LNURL Mafia Boss is here!

@SatsAllDay
Copy link
Contributor Author

SatsAllDay commented Sep 27, 2023

So I think for stacker.news, it could be nice to have the payerdata visible in the transaction log, as you guys have already talked about. Specifically, name and identifier(meaning Lightning Address) has some support across the ecosystem with OBW, Blixt, etc supporting name and Alby, BitBanana (and probably others) supporting identifier too.

👍

Another neat thing would being a consumer of LUD-18.
Meaning that when you pay out to a Lightning Address or LNURL-pay code, you could provide the @stacker.news Lightning Address (at user's discretion of course) to the payer.
As far as I understand, this current pull request covers being a LUD-18 service (in LNURL terms). you can also be a wallet. Just a friendly suggestion for the future.

Yea, that would be a good enhancement. You're correct that this PR is really just covering the service aspect. I think to cover the wallet aspect, we probably need to revamp our UX in that area before adding more options, like these. The biggest thing I think we should do is allow a user to enter the lightning address recipient, then we go and query capabilities, then update the UI form accordingly, based on what's accepted by that provider. Right now we just show all fields all the time, and let the request fail if e.g. the user sends a comment but the recipient doesn't accept them. It would be nice to have the form be dynamic based on what's allowed for the specified recipient.

@SatsAllDay SatsAllDay changed the title LUD-18 Support LUD-18 Service Support Sep 28, 2023
* don't cache the well-known response, since it includes randomly generated single use values

* validate k1 from well-known response to pay URL

* only keep k1's for 10 minutes if they go unused

* fix validation logic to make auth object optional
* move k1 cache to database

* store payer data in invoice db table

* show payer data in invoices on satistics page

* show comments and payer data on invoice page
@SatsAllDay
Copy link
Contributor Author

Some updated screenshots...
LUD18 data now reflected in:

  1. notifications for invoices deposited
  2. wallet history
  3. Invoice page (LUD-12 comments, too!)
Screen Shot 2023-09-28 at 8 41 45 PM Screen Shot 2023-09-28 at 8 41 27 PM Screen Shot 2023-09-28 at 8 41 07 PM

@SatsAllDay SatsAllDay marked this pull request as ready for review September 29, 2023 00:54
@SatsAllDay
Copy link
Contributor Author

I think this is ready for review now. I would like to do the wallet side in a separate PR, based on the above discussion.

@SatsAllDay
Copy link
Contributor Author

For those following along, I've worked up the LUD-18 wallet implementation in #531

@huumn
Copy link
Member

huumn commented Oct 3, 2023

What's the purpose of our use of the challenge k1? The only thing it seems to do right now is verify the wallet hit the .../lnurlp/[username]/index.js endpoint first within the last 10 minutes, right?

I guess we could eventually use it to auth the sender but without a way to verify with the wallet couldn't they still send arbitrary payer data? Is there another LUD that would help us ask the wallet if the payer data is legit?

@SatsAllDay
Copy link
Contributor Author

SatsAllDay commented Oct 3, 2023

What's the purpose of our use of the challenge k1? The only thing it seems to do right now is verify the wallet hit the .../lnurlp/[username]/index.js endpoint first within the last 10 minutes, right?

It's used here to validate the supplied signature. It is only used if you supply the auth payer data entry. None of the other payer data fields make use of it.

I guess we could eventually use it to auth the sender but without a way to verify with the wallet couldn't they still send arbitrary payer data? Is there another LUD that would help us ask the wallet if the payer data is legit?

AFAIK any payer data aside from auth can be "spoofed". A successful auth entry can give you proof of ownership of a public key, which is useful if the key is associated to an account. At least, that's my understanding. I'm not aware of any other LUDs to ensure other payer data is legit.

@huumn
Copy link
Member

huumn commented Oct 3, 2023

A successful auth entry can give you proof of ownership of a public key

I believe it's ownership of a linking key (which is derived from an arbitrary pubkey but is irreversibly hidden from us when generating the linking key) and cannot be independently verified.

Just trying to figure out if we should use auth if we can't really use it to authenticate payer data.

@SatsAllDay
Copy link
Contributor Author

Yes I think you are right.

We certainly could remove auth for now and reconsider including it later on. It would considerably simplify things in this PR. I have yet to encounter another service that supports it. I added it because it was in the spec ¯\_(ツ)_/¯

@huumn
Copy link
Member

huumn commented Oct 3, 2023

I'm going to work on removing it fyi. I already have it going locally.

@huumn
Copy link
Member

huumn commented Oct 3, 2023

Nice work! Other than removing auth, the only other significant change was using changing the payer data to be stored as JSONB. I regretted not doing this for nostr zaps.

@huumn huumn merged commit 3acaee3 into stackernews:master Oct 3, 2023
1 check passed
@SatsAllDay
Copy link
Contributor Author

Awesome, thanks!

@SatsAllDay SatsAllDay deleted the lud-18 branch October 4, 2023 19:48
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.

4 participants