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

Allow creation of reusable lnurl-pay endpoints #78

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions migrations/20201101170804_lnurlpay.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
exports.up = async db => {
await db.schema.createTable('lnurlpay_endpoint', t => {
t.string('id').primary()
t.string('metadata').notNullable().defaultTo('{}')
t.integer('min').notNullable()
fiatjaf marked this conversation as resolved.
Show resolved Hide resolved
t.integer('max').notNullable()
t.string('currency').nullable()
Copy link
Contributor

Choose a reason for hiding this comment

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

This does not appear to be in use, was the intention to allow denominating the min/max values in fiat currencies?

Copy link
Author

Choose a reason for hiding this comment

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

Yes. I will implement that.

t.string('text').notNullable()
t.string('image').nullable()
t.string('success_text').nullable()
t.string('success_secret').nullable()
t.string('success_url').nullable()
t.integer('comment').notNullable().defaultTo(0)
fiatjaf marked this conversation as resolved.
Show resolved Hide resolved
t.string('webhook').nullable()
})
await db.schema.table('invoice', t => {
t.string('lnurlpay_endpoint').nullable()
})
}

exports.down = async db => {
await db.schema.dropTable('lnurlpay_endpoint')
await db.schema.table('invoice', t => {
t.dropColumn('lnurlpay_endpoint')
})
}
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
"dependencies": {
"@babel/polyfill": "^7.10.1",
"basic-auth": "^2.0.1",
"bech32": "^1.1.4",
"big.js": "^5.2.2",
"body-parser": "^1.19.0",
"clightning-client": "^0.1.2",
Expand Down
1 change: 1 addition & 0 deletions src/app.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ const lnPath = process.env.LN_PATH || join(require('os').homedir(), '.lightnin

require('./invoicing')(app, payListen, model, auth, lnconf)
require('./checkout')(app, payListen)
require('./lnurl')(app, payListen, model, auth)

require('./sse')(app, payListen, auth)
require('./webhook')(app, payListen, model, auth)
Expand Down
116 changes: 116 additions & 0 deletions src/lnurl.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
import bech32 from 'bech32'
import wrap from './lib/promise-wrap'

const debug = require('debug')('lightning-charge')

module.exports = (app, payListen, model, auth) => {
const {
newInvoice, listInvoicesByLnurlPayEndpoint
, getLnurlPayEndpoint, listLnurlPayEndpoints
, setLnurlPayEndpoint, delLnurlPayEndpoint
} = model

app.get('/endpoints', auth, wrap(async (req, res) =>
res.status(200).send(
(await listLnurlPayEndpoints())
.map(lnurlpay => addBech32Lnurl(req, lnurlpay))
)))

app.post('/endpoint', auth, wrap(async (req, res) =>
res.status(201).send(
addBech32Lnurl(req, await setLnurlPayEndpoint(null, req.body))
)))

app.put('/endpoint/:id', auth, wrap(async (req, res) =>
res.status(200).send(
addBech32Lnurl(req, await setLnurlPayEndpoint(req.params.id, req.body))
)))

app.delete('/endpoint/:id', auth, wrap(async (req, res) =>
res.status(200).send(await delLnurlPayEndpoint(req.params.id))))
fiatjaf marked this conversation as resolved.
Show resolved Hide resolved

app.get('/endpoint/:id', auth, wrap(async (req, res) =>
res.status(200).send(
addBech32Lnurl(req, await getLnurlPayEndpoint(req.params.id))
)))

app.get('/endpoint/:id/invoices', auth, wrap(async (req, res) =>
res.send(await listInvoicesByLnurlPayEndpoint(req.params.id))))

// this is the actual endpoint users will hit
app.get('/lnurl/:id', wrap(async (req, res) => {
const lnurlpay = await getLnurlPayEndpoint(req.params.id)
fiatjaf marked this conversation as resolved.
Show resolved Hide resolved

res.status(200).send({
tag: 'payRequest'
, minSendable: lnurlpay.min
, maxSendable: lnurlpay.max
, metadata: makeMetadata(lnurlpay)
, commentAllowed: lnurlpay.comment
, callback: `https://${req.hostname}/lnurl/${lnurlpay.id}/callback`
})
}))

app.get('/lnurl/:id/callback', wrap(async (req, res) => {
const lnurlpay = await getLnurlPayEndpoint(req.params.id)

if (req.query.amount > lnurlpay.max)
fiatjaf marked this conversation as resolved.
Show resolved Hide resolved
return res.send({status: 'ERROR', reason: 'amount too large'})
if (req.query.amount < lnurlpay.min)
return res.send({status: 'ERROR', reason: 'amount too small'})

let invoiceMetadata = {...req.query}
delete invoiceMetadata.amount
delete invoiceMetadata.fromnodes
delete invoiceMetadata.nonce
Copy link
Contributor

Choose a reason for hiding this comment

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

I would read just the comment query parameter instead of keeping all of them minus amount/fromnodes/nonce. Also, this needs to enforce the length limit.

So maybe something like if (lnurlpay.comment && req.query.comment) invoiceMetadata.lnurlpay_comment = (''+req.query.comment).substr(0, lnurlpay.comment)

Copy link
Author

Choose a reason for hiding this comment

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

The rationale for keeping the other query parameters is the following:

Imagine a shop like https://bitclouds.sh/. Every VM you buy there gets its own lnurlpay you can use to recharge it (not really true today but it's the main use case I have in mind and one that I expect to adopt this charge-lnurlpay feature once their refactor is ready).

So instead of asking Charge to create a new lnurlpay endpoint every time someone creates a VM, Bitclouds can pregenerate a single lnurlpay endpoint and modify its URL by adding an identifier for each VM.

If the base URL is https://charge.bitclouds.sh/lnurl/recharge it can then assign https://charge.bitclouds.sh/lnurl/recharge?vm=nusakan58 to the owner of a VM called nusakan58. Then every time a payment is received at that Bitclouds will get a webhook containing {"vm": "nusakan58"} and know what to do with it.

Copy link
Author

@fiatjaf fiatjaf Nov 24, 2020

Choose a reason for hiding this comment

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

But your comment about the comment is very commentive good, I will integrate it (although I prefer to keep the field named as comment instead of lnurlpay_comment, why not?).

invoiceMetadata = {...lnurlpay.metadata, ...invoiceMetadata}
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you see as the use-cases for the endpoint metadata fields? I wonder if its worth keeping a copy of it alongside every invoice, or just a reference to the endpoint id. Duplicating the metadata might be worth it if the endpoint metadata fields are likely to change and its important to have a copy of the original fields at the time of payment.

Copy link
Author

@fiatjaf fiatjaf Nov 24, 2020

Choose a reason for hiding this comment

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

The use case I had in mind was an app storing stuff on an lnurlpay endpoint that is sent back to it on every webhook for payments made from that lnurlpay endpoint -- so the app can identify where the payment is coming from, basically, and have other data there to make it easier to reason.

I think it is not worth duplicating, it's better to fetch these again at webhook time.

But then I have to change the webhook dispatching logic in weird ways. Should I do it?

Copy link
Author

Choose a reason for hiding this comment

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

Oh, there is also the websocket and the SSE interfaces, so it won't make sense to change the webhook code. Instead I must change the src/lib/payment-listener.js code to make it fetch metadata from the lnurlpay table for each received invoice. Do you think that is good?

I think it is simpler to duplicate the metadata on each invoice, as ideally this metadata field shouldn't contain many things anyway, and in many cases I have in mind it is likely to be empty.


const invoice = await newInvoice({
descriptionHash: require('crypto')
.createHash('sha256')
.update(makeMetadata(lnurlpay))
.digest('hex')
, msatoshi: req.query.amount
, metadata: invoiceMetadata
, webhook: lnurlpay.webhook
, lnurlpay_endpoint: lnurlpay.id
})

let successAction
if (lnurlpay.success_url) {
successAction = {
tag: 'url'
, url: lnurlpay.success_url
, description: lnurlpay.success_text || ''
}
} else if (lnurlpay.success_value) {
fiatjaf marked this conversation as resolved.
Show resolved Hide resolved
// not implemented yet
} else if (lnurlpay.success_text) {
successAction = {tag: 'message', message: lnurlpay.success_text}
}

res.status(200).send({
pr: invoice.payreq
, successAction
, routes: []
, disposable: false
})
}))
}

function makeMetadata (lnurlpay) {
const text = lnurlpay.text

const meta = [['text/plain', text]]
.concat(lnurlpay.image ? ['image/png;base64', lnurlpay.image] : [])
fiatjaf marked this conversation as resolved.
Show resolved Hide resolved

return JSON.stringify(meta)
}

function addBech32Lnurl (req, lnurlpay) {
const hostname = req.hostname || req.params.hostname
const url = `https://${hostname}/lnurl/${lnurlpay.id}`
Copy link
Contributor

Choose a reason for hiding this comment

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

This assumes that the protocol is https and that charge is served from the root / path, which may not be true. This also uses the user-provided hostname (sent via the Host header), which may not be reliable.

This should use the URL environment variable, which is already used by the built-in checkout page as the root url of where the charge instance is publicly accessible to payers. This option is not documented in the CLI help text though, would be cool if you could add it :)

If the URL option is not provided, this could fall back to guessing the URL. But I am bit wary about using the Host header, need to give this some more thought and make sure that its not potentially exploitable somehow.

Copy link
Author

@fiatjaf fiatjaf Nov 24, 2020

Choose a reason for hiding this comment

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

Right, I will use that, but I thought using req.hostname was fine since you have app.set('trust proxy', process.env.PROXIED || 'loopback') there in place, which means you gave this some thought.

If I remember correctly, req.hostname will use the value from the Host header, but if X-Forwarded-Host is present it will use that instead as long as you have set app.set('trust proxy', true).

So maybe we should use process.env.URL and fallback to https://${req.hostname}? (Usage of https is required according to the LNURL spec so I think it is fine to hardcode that if no process.env.URL is found.)

(That req.params.hostname was arbitrarily inserted there to aid myself and others on a development environment, but it is probably doing more harm than good.)

const words = bech32.toWords(Buffer.from(url))
lnurlpay.bech32 = bech32.encode('lnurl', words, 2500).toUpperCase()
return lnurlpay
}
72 changes: 68 additions & 4 deletions src/model.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { toMsat } from './lib/exchange-rate'
const debug = require('debug')('lightning-charge')
, status = inv => inv.pay_index ? 'paid' : inv.expires_at > now() ? 'unpaid' : 'expired'
, format = inv => ({ ...inv, status: status(inv), msatoshi: (inv.msatoshi || null), metadata: JSON.parse(inv.metadata) })
, formatLnurlpay = lnurlpay => ({...lnurlpay, metadata: JSON.parse(lnurlpay.metadata)})
, now = _ => Date.now() / 1000 | 0

// @XXX invoices that accept any amount are stored as msatoshi='' (empty string)
Expand All @@ -16,19 +17,21 @@ const defaultDesc = process.env.INVOICE_DESC_DEFAULT || 'Lightning Charge Invoic

module.exports = (db, ln) => {
const newInvoice = async props => {
const { currency, amount, expiry, description, metadata, webhook } = props
const { currency, amount, expiry, metadata, webhook, lnurlpay_endpoint } = props

const id = nanoid()
, msatoshi = props.msatoshi ? ''+props.msatoshi : currency ? await toMsat(currency, amount) : ''
, desc = props.description ? ''+props.description : defaultDesc
, lninv = await ln.invoice(msatoshi || 'any', id, desc, expiry)
, desc = props.descriptionHash || (props.description ? ''+props.description : defaultDesc)
fiatjaf marked this conversation as resolved.
Show resolved Hide resolved
, method = props.descriptionHash ? 'invoicewithdescriptionhash' : 'invoice'
Copy link
Author

Choose a reason for hiding this comment

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

By the way, let me note here that none of this will work on c-lightning unless you have the method invoicewithdescriptionhash, which is provided by this plugin.

Should I check at startup that this method exists by calling help and print something to the console if not? Or disable the lnurl-pay thing entirely?

Copy link
Contributor

@shesek shesek Nov 4, 2020

Choose a reason for hiding this comment

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

I would disable it. The way its implemented makes that pretty easy, the lnurlpay express setup is already self-contained and decoupled from the main app.

While at it, maybe also make it optional via an NO_LNURLPAY option? (on by default)

I wouldn't bother with disabling the new migrations if lnurlpay is not enabled, just let it create an unused table/column.

As an aside, lnurl-pay could theoretically specify that invoices may use the hash as an hex-encoded string descriptor and that wallets should check for both options, which would make compatibility easier.

Copy link
Author

Choose a reason for hiding this comment

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

Awesome, thanks!

Yes, I agree with that last suggestion, but I don't know if it's too late for that by now.

Copy link
Author

Choose a reason for hiding this comment

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

What is the rationale for having a NO_LNURLPAY option? Users who don't want lnurlpay won't notice anything, the lnurlpay API routes will never be called and the lnurlpay tables will be forever empty and that's it.

On the other hand, if someone wants to use the lnurlpay feature they will very likely miss the fact that there is a NO_LNURLPAY option that is on by default and have a lot of frustration.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, I did disable the lnurlpay routes when the invoicewithdescriptionhash method isn't found, but I think that is also confusing. What if a middleware was defined inside src/lnurl.js that returned a more informative error about the missing plugin (and printed something to the console?) only when lnurlpay routes were called?

, lninv = await ln.call(method, [msatoshi || 'any', id, desc, expiry])

const invoice = {
id, msatoshi, description: desc
, quoted_currency: currency, quoted_amount: amount
, rhash: lninv.payment_hash, payreq: lninv.bolt11
, expires_at: lninv.expires_at, created_at: now()
, metadata: JSON.stringify(metadata || null)
, lnurlpay_endpoint
}

debug('saving invoice:', invoice)
Expand All @@ -50,6 +53,66 @@ module.exports = (db, ln) => {
await db('invoice').where({ id }).del()
}

const listLnurlPayEndpoints = _ =>
db('lnurlpay_endpoint')
.then(rows => rows.map(formatLnurlpay))

const listInvoicesByLnurlPayEndpoint = lnurlpayId =>
db('invoice')
.where({ lnurlpay_endpoint: lnurlpayId })
.then(rows => rows.map(format))

const getLnurlPayEndpoint = async id => {
let lnurlpay = await db('lnurlpay_endpoint').where({ id }).first()
return formatLnurlpay(lnurlpay)
fiatjaf marked this conversation as resolved.
Show resolved Hide resolved
}

const setLnurlPayEndpoint = async (id, props) => {
let lnurlpay
if (id) {
lnurlpay = await db('lnurlpay_endpoint').where({ id }).first()
lnurlpay = { ...lnurlpay, ...props }
} else
fiatjaf marked this conversation as resolved.
Show resolved Hide resolved
lnurlpay = { ...props, id: nanoid() }

if (typeof props.metadata != 'undefined') {
let metadata = JSON.stringify(props.metadata || {})
if (metadata[0] != '{')
metadata = '{}'

lnurlpay.metadata = metadata
}
Copy link
Contributor

@shesek shesek Nov 4, 2020

Choose a reason for hiding this comment

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

The metadata field should always contain a valid JSON string even if no metadata was provided, or it'll fail parsing later when reading it. Simply removing the if metadata check should do the trick.

Also, one easy way to check if metadata is an object with some properties is checking for Object.keys(props.metadata).length>0, instead of finding out after serializing it. So you could do something like lnurlpay.metadata = JSON.stringify(props.metadata != null && Object.keys(props.metadata).length ? props.metadata : {}).

Copy link
Contributor

Choose a reason for hiding this comment

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

Never mind the first part -- I just noticed the default metadata value set via knex.

Copy link
Author

Choose a reason for hiding this comment

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

But Object.keys([1, 2, 3]) will evaluate to ['0', '1', '2'], so that will falsely identify an array as an object, which we do not want!


if (props.amount) {
lnurlpay.min = props.amount
lnurlpay.max = props.amount
} else if (props.min <= props.max) {
lnurlpay.min = props.min
lnurlpay.max = props.max
} else if (props.min > props.max) {
// silently correct a user error
lnurlpay.min = props.max
lnurlpay.max = props.min
}

if (lnurlpay.min && !lnurlpay.max)
lnurlpay.max = lnurlpay.min

if (lnurlpay.max && !lnurlpay.min)
lnurlpay.min = lnurlpay.max

await db('lnurlpay_endpoint')
.insert(lnurlpay)
.onConflict('id')
.merge()

return formatLnurlpay(lnurlpay)
}

const delLnurlPayEndpoint = async id => {
await db('lnurlpay_endpoint').where({ id }).del()
}

const markPaid = (id, pay_index, paid_at, msatoshi_received) =>
db('invoice').where({ id, pay_index: null })
.update({ pay_index, paid_at, msatoshi_received })
Expand Down Expand Up @@ -85,7 +148,8 @@ module.exports = (db, ln) => {
: { requested_at: now(), success: false, resp_error: err })

return { newInvoice, listInvoices, fetchInvoice, delInvoice
, listInvoicesByLnurlPayEndpoint, listLnurlPayEndpoints
, getLnurlPayEndpoint, setLnurlPayEndpoint, delLnurlPayEndpoint
, getLastPaid, markPaid, delExpired
, addHook, getHooks, logHook }
}