-
Notifications
You must be signed in to change notification settings - Fork 197
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
feat: add LNURL processor #202
Conversation
Pull Request Test Coverage Report for Build 4164668858
💛 - Coveralls |
@cameri can you please review this :D |
Thanks for contributing. It's on my todo list. |
@im-adithya could you please rebase and fix the conflicts? |
76fe003
to
6bd859f
Compare
Done! |
@im-adithya do you have a relay instance running with this where you and I can test this code? |
Sorry, I don't :( |
const response = await this.httpClient.get(`${this.settings().paymentsProcessors?.lnurl?.invoiceURL}/callback?amount=${amountMsats}&comment=${requestId}`) | ||
|
||
const result = { | ||
id: randomUUID(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
although this works, I'm concerned we won't be able to look up this invoice on your LNURL provider. Should we instead use UUID v5 which uses namespaces and can be used to generate deterministic UUIDs? That way we can pass the invoice description and always get the same UUID back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We didn't have a uuid string coming from the response: https://getalby.com/lnurlp/adithya/callback?amount=10000
So we thought of using a random UUID.
instead use UUID v5 which uses namespaces and can be used to generate deterministic UUIDs?
The problem is that we don't return the description (and because of that I changed the getInvoice as well to take the whole invoice instead of just invoice.id)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well, this could be a future improvement, not a blocker. But we definitely want to give relay operators the ability to go to their LNURL provider to lookup invoices. If they can at least see descriptions which should include the kind of fee and pubkey then it's all good for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! I'll then change the below endpoint to have request.description instead of requestId
src/services/payments-service.ts
Outdated
const fullInvoice = await this.invoiceRepository.findById(invoice.id) | ||
await this.invoiceRepository.upsert({ | ||
id: invoice.id, | ||
pubkey: invoice.pubkey, | ||
bolt11: invoice.bolt11, | ||
amountRequested: invoice.amountRequested, | ||
description: invoice.description, | ||
unit: invoice.unit, | ||
...fullInvoice, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's revert this change. upsert
already updates some invoice properties if one already exists in the database.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it? I had issues with it iirc
Idk if it's because of not including verify_url here? https://github.com/Cameri/nostream/pull/202/files/6bd859fd43f44f4db0cd3336ca5f7771cd6e5d29#diff-a3487de3d1ff9ff4f5025e4073c92c4f2d51c7b664a818b0fcd38fd5469f5674R127
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Testing again)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It both updates and inserts. If it's failing with verify_url then it needs to be accounted for.
But I prefer explicitly mapping stuff as opposed to spreading an object because it's unclear what is being mapped.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops sorry, it's actually because we only have the information to update i.e. status (and id) coming from the getInvoice in the lnurl processor as we don't have access to the whole invoice by id from the server side (since we use a randomUUID)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, please! We might need a corresponding repository function too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! I'm using the upsert by fetching the fullInvoice using Id as before
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cameri just a gentle reminder on this one!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You leaked your private keys. Please don't type private keys in the code again and always use environment variables to inject them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah had to remove them :') luckily didn't use them much
always use environment variables to inject them.
Sure!
# [1.23.0](v1.22.6...v1.23.0) (2023-05-12) ### Bug Fixes * add SECRET as env variable ([#298](#298)) ([58a1254](58a1254)) * invoice auto marked as paid ([be6d6f1](be6d6f1)) * issues with invoices ([#271](#271)) ([e1561e7](e1561e7)) ### Features * add LNURL processor ([#202](#202)) ([f237400](f237400)) * allow lightning zap receipts on paid relays ([#303](#303)) ([14bc96f](14bc96f))
🎉 This PR is included in version 1.23.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
# [1.23.0](v1.22.6...v1.23.0) (2023-05-12) ### Bug Fixes * add SECRET as env variable ([#298](#298)) ([58a1254](58a1254)) * invoice auto marked as paid ([be6d6f1](be6d6f1)) * issues with invoices ([#271](#271)) ([e1561e7](e1561e7)) ### Features * add LNURL processor ([#202](#202)) ([f237400](f237400)) * allow lightning zap receipts on paid relays ([#303](#303)) ([14bc96f](14bc96f))
* chore: hide powered by zebedee if payment processor is not * chore: add nodeless as payments processor to settings * fix: bad content type on zebedee callback req handler * chore(release): 1.23.0 [skip ci] # [1.23.0](v1.22.6...v1.23.0) (2023-05-12) ### Bug Fixes * add SECRET as env variable ([#298](#298)) ([58a1254](58a1254)) * invoice auto marked as paid ([be6d6f1](be6d6f1)) * issues with invoices ([#271](#271)) ([e1561e7](e1561e7)) ### Features * add LNURL processor ([#202](#202)) ([f237400](f237400)) * allow lightning zap receipts on paid relays ([#303](#303)) ([14bc96f](14bc96f)) * feat: implement nodeless payments processor * docs: add accepting payments section * chore: validate nodeless webhook secret * chore: hide powered-by-zebedee for non-zebedee processors --------- Co-authored-by: semantic-release-bot <semantic-release-bot@martynus.net>
Description
Adds an lnurl processor for payments. We used the LNURL verify spec: lnurl/luds#182
This is made in such a way that any provider can implement, all they have to do is return a verify link in their lnurlp callback.
For example:
https://getalby.com/lnurlp/adithya/callback?amount=10000
And a call to the verify URL returns the status:
To support this, we added a
verify_url
field in the database.Related Issue
Motivation and Context
Mitigates the need of adding processors for each and every provider separately.
How Has This Been Tested?
Tested it locally.
Screenshots (if appropriate):
Types of changes
Checklist: