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

feat: add LNURL processor #202

Merged
merged 10 commits into from
Mar 6, 2023
Merged

Conversation

im-adithya
Copy link
Contributor

@im-adithya im-adithya commented Feb 13, 2023

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

{
    "status": "OK",
    "verify": "https://getalby.com/lnurlp/adithya/verify/HLzzSn7vzrAwgrsmTJ9LjQSH",  // <--
    "routes": [],
    "pr": <bolt11 invoice>
}

And a call to the verify URL returns the status:

{
    "status": "OK",
    "settled": false, // <--
    "preimage": null,
    "pr": <bolt11 invoice>
}

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):

Screenshot 2023-02-17 at 2 31 48 PM

Types of changes

  • Non-functional change (docs, style, minor refactor)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my code changes.
  • All new and existing tests passed.

@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 4164668858

  • 9 of 41 (21.95%) changed or added relevant lines in 7 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.6%) to 58.41%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/app/maintenance-worker.ts 0 1 0.0%
src/payments-processors/payments-procesor.ts 0 1 0.0%
src/payments-processors/zebedee-payments-processor.ts 0 3 0.0%
src/services/payments-service.ts 0 3 0.0%
src/factories/payments-processor-factory.ts 3 10 30.0%
src/payments-processors/lnurl-payments-processor.ts 5 22 22.73%
Files with Coverage Reduction New Missed Lines %
src/services/payments-service.ts 1 7.03%
Totals Coverage Status
Change from base Build 4148256062: -0.6%
Covered Lines: 1174
Relevant Lines: 1983

💛 - Coveralls

@im-adithya
Copy link
Contributor Author

@cameri can you please review this :D

@cameri
Copy link
Owner

cameri commented Feb 17, 2023

@cameri can you please review this :D

Thanks for contributing. It's on my todo list.

@cameri
Copy link
Owner

cameri commented Feb 24, 2023

@im-adithya could you please rebase and fix the conflicts?

@im-adithya
Copy link
Contributor Author

Done!

@cameri
Copy link
Owner

cameri commented Feb 24, 2023

@im-adithya do you have a relay instance running with this where you and I can test this code?

@im-adithya
Copy link
Contributor Author

Sorry, I don't :(
I just tested it locally

const response = await this.httpClient.get(`${this.settings().paymentsProcessors?.lnurl?.invoiceURL}/callback?amount=${amountMsats}&comment=${requestId}`)

const result = {
id: randomUUID(),
Copy link
Owner

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.

Copy link
Contributor Author

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)

Copy link
Owner

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.

Copy link
Contributor Author

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

Comment on lines 116 to 118
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,
Copy link
Owner

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(Testing again)

Copy link
Owner

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.

Copy link
Contributor Author

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)

Copy link
Owner

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?

Copy link
Contributor Author

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

Copy link
Contributor Author

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!

Copy link
Owner

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.

Copy link
Contributor Author

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!

@cameri cameri merged commit f237400 into cameri:main Mar 6, 2023
github-actions bot pushed a commit that referenced this pull request May 12, 2023
# [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))
@github-actions
Copy link

🎉 This PR is included in version 1.23.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

cameri pushed a commit that referenced this pull request May 15, 2023
# [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))
cameri added a commit that referenced this pull request May 15, 2023
* 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>
github-actions bot pushed a commit that referenced this pull request May 15, 2023
# [1.24.0](v1.23.0...v1.24.0) (2023-05-15)

### Features

* implement nodeless payments processor ([#305](#305)) ([52aac39](52aac39)), closes [#298](#298) [#271](#271) [#202](#202) [#303](#303)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants