HRN: Emit new event type when Offer has been received and validated

[tnull]

Currently, when using the ChannelManager::pay_for_offer_from_human_readable_name API, the user gets no access to the offer data retrieved. It would be great to add a new event type that is emitted when the offer has been retrieved and validated, e.g. here:

rust-lightning/lightning/src/ln/channelmanager.rs

Line 12790 in b543afe

if let Some((completed_requests, mut offer)) = offer_opt {

[TheBlueMatt]

Hmm, is there a reason you need the Offer directly or can you just access the offer fields via the Bolt12Invoice when the payment ultimately completes (which I believe was added upstream but is not in 0.1)?

[tnull]

Hmm, is there a reason you need the Offer directly or can you just access the offer fields via the Bolt12Invoice when the payment ultimately completes (which I believe was added upstream but is not in 0.1)?

Hmm, IIUC, PaymentSent would only give us the Bolt12Invoice, which doesn't even expose OfferId, for instance. But more generally: the payment might very well fail and we'd still like to be able to fill in some of the metadata fields and maybe even retry using the Offer, which would save us multiple RTTs for resolving/retrieving the offer once more.

[chuksys]

I'd like to pick this up.

[tnull]

I'd like to pick this up.

Sure, go for it!

[TheBlueMatt]

Hmm, IIUC, PaymentSent would only give us the Bolt12Invoice, which doesn't even expose OfferId, for instance.

that's easy to fix, though, and likely it should :).

But more generally: the payment might very well fail and we'd still like to be able to fill in some of the metadata fields

Hmm? Like what? I'm not really sure there's much to fill in from an Offer derived from an HRN - its really just a blinded path to request an Invoice?

maybe even retry using the Offer, which would save us multiple RTTs for resolving/retrieving the offer once more.

That's one RTT, but I guess I'm not clear on the point of that? Like, DNS should move pretty quick, hopefully, and if an Offer request times out its probably already retried a few times and we've waited a while. Adding one RTT on top of the retry seems perfectly fine?

[tnull]

That's one RTT, but I guess I'm not clear on the point of that? Like, DNS should move pretty quick, hopefully, and if an Offer request times out its probably already retried a few times and we've waited a while. Adding one RTT on top of the retry seems perfectly fine?

The point is that we already retrieved the offer and still would have an entirely blank entry in the payment store even though we could provide the user additional metadata.

[TheBlueMatt]

Right, I guess I'm missing what that metadata is? Like, we can give the user an OfferId, but I'm not really sure if they care? They certainly care about OfferIds when they sent an offer and can use it to track the payment, but here they can't (they have to use the HRN - multiple HRNs can even resolve to the same Offer!). The Offer won't have an amount or much in it, or certainly shouldn't, so there shouldn't be any fields they care about in it.

[tnull]

Right, I guess I'm missing what that metadata is?

Well, in LDK it would be all the fields of PaymentKind::Bolt12Offer, plus the actual Offer storage, which we're about to add support for currently.

Like, we can give the user an OfferId, but I'm not really sure if they care? They certainly care about OfferIds when they sent an offer and can use it to track the payment, but here they can't (they have to use the HRN - multiple HRNs can even resolve to the same Offer!). The Offer won't have an amount or much in it, or certainly shouldn't, so there shouldn't be any fields they care about in it.

Honestly, I'm not quite following why this is controversial at all? LDK retrieves the offer from DNS, why can't we give the user a way to access what we retrieved? FWIW, pay_for_offer_from_human_readable_name currently returns Result<(), ()> which gives the user 0 semantic feedback about what happend, what immediately succeeded/failed, what the OfferId is, etc. Of course, all of the protocol steps are happening async in the background, so we can only provide more information by emitting an event.

[TheBlueMatt]

its a question of what the API should look like so that there's a consistent view of what's going on - paying a BIP 353 isn't strictly just paying an offer, and to some extent I regret having a "pay for offer from BIP 353" method because a BIP 353 can also be on-chain but could also be silent payments or cashu or...

Having an API where someone pastes in a human readable name and then expects to get back an OfferId or really any additional offer metadata seems really wrong to me - its just a payment, for an amount, with a fee. There may be some additional metadata, but tying it to offers specifically just feels totally wrong.

I do kinda wonder if ldk-node shouldn't instead by using https://docs.rs/bitcoin-payment-instructions/ to parse these (which hopefully post-LDK-0.2 will support doing the DNS lookup over onion messages for privacy).

[tnull]

Having an API where someone pastes in a human readable name and then expects to get back an OfferId or really any additional offer metadata seems really wrong to me - its just a payment, for an amount, with a fee. There may be some additional metadata, but tying it to offers specifically just feels totally wrong.

Well, I'd argue it's the other way around: we def. need to track the concrete payment, including all metadata the kind of payment supports (including things like Txid, OfferId, payment hash, etc., depending on what we use). So yes, assuming an HRN always resolves to an offer is wrong, but in LDK Node we'd track the concrete payment and attach the HRN metadata if the payment happens to be the result of a HRN resolution.

I do kinda wonder if ldk-node shouldn't instead by using https://docs.rs/bitcoin-payment-instructions/ to parse these (which hopefully post-LDK-0.2 will support doing the DNS lookup over onion messages for privacy).

Hmm, that might indeed make more sense, at least if we're then also committed to drop the pay_for_offer_from_hrn API from core LDK? And how would we deal with it in the meantime?

[ouchennnn]

can I take this ?

[chuksys]

can I take this ?

Appreciate you looking into this! I've been monitoring this issue too, but I'm currently waiting for the maintainers to wrap up their discussion before starting any work. Happy to coordinate once they give the green light!

[tnull]

can I take this ?

Thanks for your interest, but as @chuksys mentioned the discussion is just pending a final resolution, at which point it's already assigned to him. So feel free to look around for another open issue.