Bolt12 support #1727

riccardobl · 2024-12-14T19:54:55Z

Description

This PR adds support to pay, estimate fees and decode bolt12 invoices and a bolt12 wallet attachment.
The codebase uses the bolt11 nomenclature pretty much everywhere. After this PR, some areas will be generalized to also account for the possibility of a bolt12 invoice. However, to avoid weighing this PR down with unrelated, mostly graphical or naming changes, I’ve decided to leave those changes for a follow-up PR.

Closes #1317

Screenshots

Checklist

Are your changes backwards compatible? Please answer below:
yes

On a scale of 1-10 how well and how have you QA'd this change and any features it might affect? Please answer below:

For frontend changes: Tested on mobile, light and dark mode? Please answer below:

Did you introduce any new environment variables? If so, call them out explicitly here:
yes

LNDK_SOCKET
LNDK_CERT and LNDK_MACAROON : can be the same values used for LND (see https://github.com/lndk-org/lndk/blob/master/docs/cli_commands.md )

Testing

How to get a bolt12 offer string:

wait for eclair channels to be confirmed
- bash sndev cli eclair channels should return at least 1 channel marked as NORMAL
get the tipjar offer string: bash sndev cli eclair tipjarshowoffer
see channels balance with bash sndev cli eclair usablebalances

Tests

Regression tests

riccardobl · 2024-12-18T13:17:18Z

api/lnd/index.js

@@ -10,6 +11,11 @@ const lnd = global.lnd || authenticatedLndGrpc({
  macaroon: process.env.LND_MACAROON,
  socket: process.env.LND_SOCKET
 }).lnd
+installLNDK(lnd, {


since lndk is just adding few apis to lnd, we are going to need them both to do most of the api work, by appending the lndk client to the lnd object we reflect its nature as an extension and we avoid passing another field to every context

Please don't mutate objects from an external library in any way for any reason.

I know it's highly unlikely that this is an issue now (and probably also not in the future), but if this ever becomes an issue, it's going to be extremely hard to debug because it will break in ways we cannot anticipate.

I also don't want to even have to ask myself if this might break. I see this like the Sword of Damocles. Just don't do it, the risk/reward ratio just isn't worth it. I suggest to just pass lndk in the context like we pass lnd around.

You can then also remove all these checks:

const lndk = lnd?.lndk if (!lndk) throw new Error('lndk not installed, please use installLNDK')

riccardobl · 2024-12-18T16:23:52Z

lib/bolt11-tags.js

+export function bolt11Tags (bolt11) {
+  if (!isBolt11(bolt11)) throw new Error('not a bolt11 invoice')
+  return decode(bolt11).tagsObject
+}


i've renamed @/lib/bolt11 to @/lib/bolt11-tags

Why not put everything bolt11 related into lib/bolt11.js? Same for bolt12 stuff?

because ln-service uses 'fs' to load protobuf files and it breaks webpack builds for the browser, iirc there are also a bunch of other node specific imports that would require a polyfill

Mhhh, the files in lib/ should be importable by the client and server afaik.

Is there a way to fix this by dynamically importing server stuff if a server function is called without significant downsides?

yes, it is possible, but last time dynamic imports caused a lot of issue in production.
Are they supposed to work reliably after your patch @huumn ?

riccardobl · 2024-12-18T16:27:55Z

lib/bolt12-info.js

+  return invoice.startsWith('lni1') || invoice.startsWith('lno1')
+}
+
+export function bolt12Info (bolt12) {


This is the bolt12 equivalent of the bolt11Tags helper.
Similar to the bolt11 counterpart, i've used a light parser just for the few tags we need in the ui, since the lndk parser needs access to the grpc endpoint.

I couldn't find an established pure js bolt12 parser, so i wrote a small tlv parser (@/lib/tlv) following the specs.

riccardobl · 2024-12-18T16:36:59Z

lib/lndk.js

+    }
+  }
+
+  const out = {


This function converts the output from lndk to an object with a layout compatible with the output from ln-service.

I've followed the doc so it should match the output very closely, but honestly i don't like the fact that ln-service returns so many values that mean the same thing with different types (eg, tokens, mtokens, safe_tokens...), i've implemented this conversion because it makes our code simpler with fewer changes, since we can handle both bolt11 and bolt12 invoices in the same way, but imo we should provide our own abstraction at some point, using only bigint msats and things that make more sense for our codebase.

I agree, ln-service tries to make the grpc interface nicer, but it isn't as well documented as the grpc interface so it ends up just being confusing most of the time.

riccardobl · 2024-12-18T16:40:30Z

lib/lndk.js

+      response_invoice_timeout: timeout
+    }, async (error, response) => {
+      if (error) return reject(error)
+      const bech32invoice = 'lni1' + bech32b12.encode(Buffer.from(response.invoice_hex_str, 'hex'))


Bolt12 invoices are not supposed to be presented to the user, so the spec doesn't define an human readable representation, instead it is left to apps and nodes to handle the representation how they see more fit.
LNDK uses an hex string, while CLN use bech32.
For consistency with bolt11, i've decided to convert them to bech32 like in CLN.

ekzyis

Tested normal and automated withdrawals and they work well!

But I noticed the followings things I mentioned in comments that I'd like to see discussed before approval:

lib/ was meant to be files that the server and client can import. Can we use dynamic imports so we don't need to split bolt files by server and client and can have just one file per bolt?
how did you test max_fee?
duplicate checks via if and schema
function signature of parseBolt12Request
error handling via async callback
add similar function for bolt tags like for payInvoice, parseInvoice, estimateFees?
Showing BOLT12 invoices to the user: I think ideally they would always only see the BOLT12 offer. That's also what other wallets do. Is that possible in this PR or should we keep it for another PR?
mutating the lnd object instead of passing lndk separately in context
LND custom options

I read in the LNDK docs that we need to run LND with custom options:

[protocol]
protocol.custom-message=513
protocol.custom-nodeann=39
protocol.custom-init=39

You've added these options in #1702.

Afaict, this means @huumn needs to configure LND differently in prod. You should have mentioned this since I think this wouldn't have worked as-is in prod. 👀

other small stuff (see comments)

ekzyis · 2024-12-24T14:39:17Z

wallets/bolt12/index.js

+    placeholder: 'lno....',
+    clear: true,
+    serverOnly: true,
+    validate: string()


If I enter asdf, I get this error as a toast:

failed to create test invoice: 3 INVALID_ARGUMENT: The provided offer was invalid. Please provide a valid offer in bech32 format, i.e. starting with 'lno'. Error: Bech32(MissingSeparator)

Validation should make sure the input actually makes sense

ekzyis · 2024-12-24T14:40:35Z

wallets/bolt12/index.js

+export const card = {
+  title: 'Bolt12',
+  subtitle: 'receive payments to a bolt12 offer',
+  image: { src: '/wallets/bolt12.svg' }


nitpick: I am not a fan of this (unofficial?) logo. I would prefer to just use text for the protocols but no strong opinion. I let you or @huumn decide.

wallets/bolt12/index.js

ekzyis · 2024-12-24T15:02:34Z

prisma/migrations/20241212160430_bolt12_attachment/migration.sql

+
+
+-- Update wallet json
+CREATE TRIGGER wallet_blink_as_jsonb


I didn't know that triggers are scoped to the tables, so this worked because the table WalletBolt12 does not have a trigger with this name yet.

So maybe we should rename all triggers to use a name like wallet_jsonb_trigger so there are less unnecessary changes between wallet tables? 🤔

But I think for now, in this PR, this should use the consistent name wallet_bolt12_as_jsonb.

ekzyis · 2024-12-24T18:46:54Z

wallets/bolt12/server.js

+
+export async function testCreateInvoice ({ offer }, { lnd }) {
+  const invoice = await fetchBolt12InvoiceFromOffer({ lnd, offer, msats: 1000, description: 'test' })
+  if (!isBolt12Invoice(invoice)) throw new Error('not a bolt12 invoice')


Can fetchBolt12InvoiceFromOffer ever return someting that is not a bolt12 invoice?

ekzyis · 2024-12-25T00:15:34Z

wallets/server.js

      }

+      checkInvoice(invoice, { msats }, { lnd, logger })


await missing 👀

ekzyis · 2024-12-25T00:15:59Z

wallets/server.js

+  logger.info(`created invoice for ${formatSats(msatsToSats(parsedInvoice.mtokens))}`, {
+    bolt11: invoice
+  })


I think this logger call should be in createInvoice immediately after walletCreateInvoice like it was before

ekzyis · 2024-12-25T00:19:08Z

wallets/server.js

  // get the wallets in order of priority
  const wallets = await getInvoiceableWallets(userId, { predecessorId, models })

  msats = toPositiveNumber(msats)

  for (const { def, wallet } of wallets) {
    const logger = walletLogger({ wallet, models })
+    if (def.isBolt12OnlyWallet && !supportBolt12) continue


Do you still need this? You also check below for supportBolt12 after the invoice was fetched

yes, this early check is to exclude bolt12-only wallets when WE cannot use them, logging an error for a perfectly fine wallet just because someone is asking an invoice via lnurlp, would be confusing for the user

ekzyis · 2024-12-25T00:28:54Z

lib/lndk.js

+export async function estimateBolt12RouteFee ({ lnd, destination, tokens, mtokens, request, timeout }) {
+  const lndk = lnd?.lndk
+  if (!lndk) throw new Error('lndk not installed, please use installLNDK')
+  const parsedInvoice = request ? await parseBolt12Request({ lnd, request }) : {}
+
+  if (!tokens && mtokens) tokens = toPositiveNumber(msatsToSats(mtokens))
+  tokens ??= toPositiveNumber(parsedInvoice.tokens)
+  destination ??= parsedInvoice.destination
+
+  if (!destination) throw new Error('no destination provided')
+  if (!tokens) throw new Error('no tokens provided')
+
+  return await new Promise((resolve, reject) => {
+    lnd.router.estimateRouteFee({
+      dest: Buffer.from(destination, 'hex'),
+      amt_sat: tokens,
+      timeout
+    }, (err, res) => {
+      if (err) {
+        if (res?.failure_reason) {
+          reject(new Error(`unable to estimate route: ${res.failure_reason}`))
+        } else {
+          reject(err)
+        }
+        return
+      }
+
+      if (res.routing_fee_msat < 0 || res.time_lock_delay <= 0) {
+        reject(new Error('unable to estimate route, excessive values: ' + JSON.stringify(res)))
+        return
+      }
+
+      resolve({
+        routingFeeMsat: toPositiveNumber(res.routing_fee_msat),
+        timeLockDelay: toPositiveNumber(res.time_lock_delay)
+      })
+    })
+  })
+}


I think this is the same as estimateRouteFee for bolt11 except it needs to parse the bolt12 request before. This should afterwards call estimateRouteFee imo.

ekzyis · 2024-12-25T00:30:21Z

worker/index.js

+  installLNDK(lnd, {
+    cert: process.env.LNDK_CERT,
+    macaroon: process.env.LNDK_MACAROON,
+    socket: process.env.LNDK_SOCKET
+  })


see other comment

Co-authored-by: ekzyis <[email protected]>

riccardobl force-pushed the bolt12a branch from 15d17e0 to 10512b0 Compare December 14, 2024 19:58

riccardobl added 4 commits December 17, 2024 01:12

bolt12 attachment

e803efe

don't support direct payments to bolt12

332d1e1

code cleanup, add bolt12info (bolt11 tags equivalent)

f927fc5

add withdraw to bolt12, improve checks and naming

494061c

riccardobl force-pushed the bolt12a branch from 10512b0 to 494061c Compare December 17, 2024 19:17

riccardobl added 9 commits December 17, 2024 20:43

Add create invoice test

8fbf5c2

Add bolt12 logo

f68b69d

improve labels

7ff3e1b

download from sn fork

95246bd

resolve bolt12 invoice inside attachment

d326322

Merge branch 'master' into bolt12a

bae01b3

rebase

20c3e58

add support for max_fee_mtokens in bolt12 interface

90f2c9c

revert some unrelated changes

b6cc65f

riccardobl commented Dec 18, 2024

View reviewed changes

riccardobl changed the title ~~Bolt12 wallet attachment~~ Bolt12 support Dec 18, 2024

Merge branch 'master' into bolt12a

6c863d8

riccardobl marked this pull request as ready for review December 18, 2024 16:45

deduplicate code

0e56bc8

riccardobl added wallets feature new product features that weren't there before labels Dec 20, 2024

Merge branch 'master' into bolt12a

cc993ff

ekzyis reviewed Dec 25, 2024

View reviewed changes

riccardobl and others added 6 commits December 25, 2024 01:56

Update lib/validate.js

efcd9a2

Co-authored-by: ekzyis <[email protected]>

Update wallets/bolt12/index.js

2411e99

Co-authored-by: ekzyis <[email protected]>

fix trigger name

28c24d5

fix typo

f735d68

use String() to cast strings

86a36ae

catch errors in async callback

aae6de9

riccardobl added 5 commits December 25, 2024 02:27

readd trim removed by mistake

4191919

removed unused default, rename lndSocket to lndkSocket

fa9ede4

improve feature bit mapping

63013c0

add missing await

406d3aa

permalink to repo

501d272

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bolt12 support #1727

Bolt12 support #1727

riccardobl commented Dec 14, 2024 •

edited

Loading

riccardobl Dec 18, 2024

ekzyis Dec 24, 2024

ekzyis Dec 24, 2024

riccardobl Dec 18, 2024

ekzyis Dec 24, 2024

riccardobl Dec 24, 2024 •

edited

Loading

ekzyis Dec 24, 2024

riccardobl Dec 25, 2024

riccardobl Dec 18, 2024

riccardobl Dec 18, 2024

huumn Dec 20, 2024

riccardobl Dec 18, 2024

ekzyis left a comment

ekzyis Dec 24, 2024

ekzyis Dec 24, 2024

ekzyis Dec 24, 2024

ekzyis Dec 24, 2024

ekzyis Dec 25, 2024

ekzyis Dec 25, 2024

ekzyis Dec 25, 2024

riccardobl Dec 25, 2024

ekzyis Dec 25, 2024

ekzyis Dec 25, 2024

Bolt12 support #1727

Are you sure you want to change the base?

Bolt12 support #1727

Conversation

riccardobl commented Dec 14, 2024 • edited Loading

Description

Screenshots

Checklist

Testing

Tests

Regression tests

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riccardobl Dec 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekzyis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riccardobl commented Dec 14, 2024 •

edited

Loading

riccardobl Dec 24, 2024 •

edited

Loading