-
Notifications
You must be signed in to change notification settings - Fork 42
feat: add BOLT-12 offers #1242
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
base: master
Are you sure you want to change the base?
feat: add BOLT-12 offers #1242
Conversation
@vincenzopalazzo would be great if you can try this out and give some feedback! |
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.
I did not tested but I review the code and left some comments!
lnclient/ldk/ldk.go
Outdated
@@ -456,6 +456,18 @@ func getMaxTotalRoutingFeeLimit(amountMsat uint64) ldk_node.MaxTotalRoutingFeeLi | |||
} | |||
} | |||
|
|||
func (ls *LDKService) GenerateOfferSync(ctx context.Context, description string) (string, error) { | |||
expiry := uint32(86400000) |
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.
Based on the details you provided in the PR description, it states: ‘Setting expiry to 86400 fails, but 86400000 works (contacted the LDK team to investigate this).’
Could you clarify what specific error you are encountering?
That said, this behavior doesn’t seem logical for the type of offer you are using. An ‘any amount’ invoice is typically intended for periodic events (e.g., Ocean payouts). Such an invoice should have a long expiration time; otherwise, it defeats the purpose of having an offer for these kinds of services.
Wouldn’t it make sense for this value to default to None
?
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.
Cuncurrent event :) 79ae577
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.
Based on the details you provided in the PR description, it states: ‘Setting expiry to 86400 fails, but 86400000 works (contacted the LDK team to investigate this)
Atleast locally, if I set the expiry to 1 day (86400), it shows the same expiry on decoding the offer as expected, but if I try to pay it, it gives me InvoiceRequestExpired
error. But if I do the same with 1000 days, it works fine...
I think it has something to do with the path info in the offer... (see below decoded offers)
This is with 86400
$ lightning-cli decode lno1qgs0v8hw8d368q9yw7sx8tejk2aujlyll8cp7tzzyh5h8xyppqqqqqq2qdkx7mqwq3nlp7x9zzmsqqye7qqqqqgqqyppexejqvz945nl3gnucd0m43t706mpgq4jva9wl8njc40m6pd49sgzq02zlx4fu57wfyq4pr2w0m9tlgry60mu98cg60dcxewuc05pssjhuqq6qdpd779caw4fw4vj9h6qv56zct2npk98d4xrtwhepcpsqu4nae3tg0rxrmkusfk8m5v87s0zm8wzgjergm3u76w6p7ta05sq9jf4djz4cvs77jhh5vcsxt3lvhwme52y3zemtd5fl9xyqkwzx7chaqu9rzccw0pr4rhmwvupzcssyh3yfs9xq89r0s7ttq9x36uwh6gfzkx5fw7cjte0unc4avre5vy0
{
"type": "bolt12 offer",
"offer_id": "883c03820a0a904c0841914bbc76261ca08762e928333cb05e16246a76a17429",
"offer_chains": [
"f61eee3b63a380a477a063af32b2bbc97c9ff9f01f2c4225e973988108000000"
],
"offer_description": "lol",
"offer_absolute_expiry": 1743845573,
"offer_paths": [
{
"first_scid": "39408x1x1",
"first_scid_dir": 0,
"first_path_key": "021c9b3203045ad27f8a27cc35fbac57e7eb61402b2674aef9e72c55fbd05b52c1",
"path": [
{
"blinded_node_id": "03d42f9aa9e53ce4901508d4e7ecabfa064d3f7c29f08d3db8365dcc3e8184257e",
"encrypted_recipient_data": "0342df78b8ebaa9755922df4065342c2d530d8a76d4c35baf90e"
},
{
"blinded_node_id": "030072b3ee62b43c661eedc826c7dd187f41e2d9dc244b2346e3cf69da0f97d7d2",
"encrypted_recipient_data": "9356c855c321ef4af7a331032e3f65ddbcd14488b3b5b689f94c4059c237b17e838518b1873c23a8efb73381"
}
]
}
],
"offer_issuer_id": "025e244c0a601ca37c3cb580a68eb8ebe909158d44bbd892f2fe4f15eb079a308f",
"valid": true
}
And this is with 86400000
:
$ lightning-cli decode lno1qgs0v8hw8d368q9yw7sx8tejk2aujlyll8cp7tzzyh5h8xyppqqqqqq2q4kx7mrpwv8qgmgkqcc3p6qzge0dt0jn6p8auekfgx8lzjjlyfnhywqszakfyy4hytj59hq6lvds9s5gm3ltgt3epvxlh52u8z63vkzrgfa49p0enrv57cdmnrp3dq8yqgpe5c3xj6qf4hdq62qga6k4gmsw7pm037wxc9vdkg6rxp6rtrgpu7gqxvatz9w687acfm73lltkadl3nngy55av33qx9szr65uhu0m6ujluh8jcq9qrtxglkwa82e7ac308uw9a7hds8qh6qld5zsyrt6lmwmaxs0j9hfk0uk2ql2kakxvq3nhnrq9dpvq5qqkqh26zghstcczu36rv82kqejuzggt24zq4t9xww5k0w2smv6ff8p6epr3qnjwrekfnchkzvvtzzqc2y00xujfqr5ywugyng5uqjsa3ljh8a3jhlkulfee5vyg0rhc67c
{
"type": "bolt12 offer",
"offer_id": "d7b8b2301ffc5679f9f6570c0f210d5bf3651ff18267c3383e716ed0dc4eff65",
"offer_chains": [
"f61eee3b63a380a477a063af32b2bbc97c9ff9f01f2c4225e973988108000000"
],
"offer_description": "lolas",
"offer_absolute_expiry": 1830159921,
"offer_paths": [
{
"first_node_id": "02465ed5be53d04fde66c9418ff14a5f2267723810176c9212b722e542dc1afb1b",
"first_path_key": "02c288dc7eb42e390b0dfbd15c38b5165843427b5285f998d94f61bb98c31680e4",
"path": [
{
"blinded_node_id": "039a622696809adda0d2808eead546e0ef076f8f9c6c158db23433074358d01e79",
"encrypted_recipient_data": "3ab115da3fbb84efd1ffd76eb7f19cd04a53ac8c4062c043d5397e3f7ae4bfcb9e58014035991fb3ba7567ddc45e7e38bdf5db"
},
{
"blinded_node_id": "0382fa07db4140835ebfb76fa683e45ba6cfe5940faaddb19808cef3180ad0b014",
"encrypted_recipient_data": "0bab4245e0bc605c8e86c3aac0ccb824216aa8815594ce752cf72a1b669293875908e209c9c3cd933c5ec263"
}
]
}
],
"offer_issuer_id": "030a23de6e49201d08ee209345380943b1fcae7ec657fdb9f4e7346110f1df1af6",
"valid": true
}
The only thing I changed is the expiry between these two, no new channels/peers nothing, turning the expiry back to 1 day fails immediately again. Really weird that the routing info is being affected by the expiry field for some reason.
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.
probably more easy to write a test in ldk-node itself! let me try to do it RN
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.
Ok, I wrote the code to check this case in here lightningdevkit/ldk-node#518, and I probably am not able to get the failure.
I am not sure that the path is related tho, it is just peaking different nodes if I am not mistaking
While reviewing the code in [1], an interesting issue was discussed related to the expiry of the offer. The reported problem is as follows: > At least locally, if I set the expiry to 1 day (86400), it shows > the same expiry on decoding the offer as expected, but if I try to > pay it, it gives me an InvoiceRequestExpired error. However, if I > do the same with 1000 days, it works fine... This commit adds a test to ensure that we can pay an offer with an expiry of 1 day without encountering an InvoiceRequestExpired error. [1] getAlby/hub#1242 Signed-off-by: Vincenzo Palazzo <[email protected]>
While reviewing the code in [1], an interesting issue was discussed related to the expiry of the offer. The reported problem is as follows: > At least locally, if I set the expiry to 1 day (86400), it shows > the same expiry on decoding the offer as expected, but if I try to > pay it, it gives me an InvoiceRequestExpired error. However, if I > do the same with 1000 days, it works fine... This commit adds a test to ensure that we can pay an offer with an expiry of 1 day without encountering an InvoiceRequestExpired error. [1] getAlby/hub#1242 Signed-off-by: Vincenzo Palazzo <[email protected]>
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.
Copilot reviewed 15 out of 15 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
tests/mock_ln_client.go:216
- The mock implementation of GenerateOfferSync always returns 'not supported', which may cause tests expecting a valid offer to fail. Consider providing a stub or configuration option to simulate a successful offer generation for tests that rely on this method.
func (mln *MockLn) GenerateOfferSync(ctx context.Context, description string) (string, error) {
@@ -539,3 +539,7 @@ func (svc *PhoenixService) GetCustomNodeCommandDefinitions() []lnclient.CustomNo | |||
func (svc *PhoenixService) ExecuteCustomNodeCommand(ctx context.Context, command *lnclient.CustomNodeCommandRequest) (*lnclient.CustomNodeCommandResponse, error) { | |||
return nil, nil | |||
} | |||
|
|||
func (svc *PhoenixService) GenerateOfferSync(ctx context.Context, description string) (string, error) { | |||
return "", errors.New("not supported") |
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.
just checked how phoenixd does bolt12 offers.
they do not accept any params like descriptions:
https://phoenix.acinq.co/server/api#get-bolt12-offer
when paying a bolt12 offer a message can be provided: https://phoenix.acinq.co/server/api#pay-bolt12-offer
Note: we'll need to update error handling once #1249 is merged |
<div className="grid gap-5"> | ||
<Alert> | ||
<AlertTriangleIcon className="h-4 w-4" /> | ||
<AlertTitle>BOLT-12 is in beta phase</AlertTitle> |
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.
Not sure this is the right wording. Maybe the title and description can be reviewed
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.
Changed a bit, can you check now?
paymentHash = *bolt12PaymentKind.Hash | ||
} | ||
if bolt12PaymentKind.PayerNote != nil { | ||
description = *bolt12PaymentKind.PayerNote |
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.
description and payer note are two different things, right?
description: set on the offer at creation by the recipient
payer note: set by the payer at the time of payment.
can you re-review this and see how it applies to how we save data in the transaction (e.g. in the DB)? should the payer note be added in the metadata?
description = *bolt12PaymentKind.PayerNote | ||
} | ||
|
||
metadata["offer_id"] = bolt12PaymentKind.OfferId |
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.
is it ok to store this as metadata or would it be better as a proper column?
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.
I was considering these a bit similar to keysend, and there we store the tlv records in the metadata too so I followed the same here. I'd say let's store it in the metadata for now, wdyt?
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.
I'm not sure these can be compared, they are two completely different things. How difficult will it be to migrate later?
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 but bolt 12 is not much different from bolt11 apart from the communication part, it's still using the same bolt11 invoices right?
I'll see if the migration from metadata["offer_id"] to a offer_id column is simple enough, else we can add a column.
@im-adithya we have some code to delete old LDK payments. I'll update it in the LDK-node 0.5 PR (coming soon) to only delete BOLT-11 invoices there and add a note that this needs to be reviewed at some point (otherwise too many BOLT12 payments could get stored in VSS and slowdown startup) |
TODOs
86400
fails but86400000
works (@rolznz contactedLDK team to figure this out)Users who don't have alby account connected don't directly see a button leading them to BOLT-12 offer creation pageDone but needs some copy changesDescription
Fixes #1204
Adds option to generate BOLT12 Offers. And displays incoming payments in the transaction list (with
description
i.e.payerNote
andoffer_id
)Also adds
payBOLT12Offer
node command to pay BOLT12 offers from debug toolsScreenshots