gl-plugin: make lsp_invoice RPC call store additional invoice metadata - #742
gl-plugin: make lsp_invoice RPC call store additional invoice metadata#742Nazarevsky wants to merge 2 commits into
Conversation
In order to get an invoice, lsp_invoice RPC call is used, however, no additional information about the requested invoice is stored. That is unfortunate, since we may need this data to detect whether the incomming payment actually used for opening JIT channel or not. Besides, we don't store any information about the original invoice (e.g. the original amount), so this structure includes that information.
When calling lsp_invoice RPC call, two outcomes may be produced: - the client has enough liquidity in channels to receive a payment - a common bolt11 invoice is requested; - the client has not enough liquidity in channels to receive a payment - a JIT channel requested from an LSP; For both variants we use datastore in order to store additional meta information for the invoice. It is worth to state that the key by which the invoice meta distinguished is invoice's label. My first intention was to use payment hash, however, we cannot rely on invoice's payment hashes because invoice payment hashes for an LSP (with original amount) and for a client (with reduced amount) are the same in case of unspecified amount.
nepet
left a comment
nepet
left a comment
There was a problem hiding this comment.
Thank you @Nazarevsky for putting this together.
The approach makes sense and the doc comment on LspInvoiceMeta does a good job explaining why this workaround is needed, which will save the next person a lot of time.
One general note to keep in mind, not a blocker: nothing cleans these entries up on invoice expiry or payment so far, so they accumulate indefinitely. May not be a problem though, if we only store "lsp_invoices" and if we don't expect them to be created indefinitely.
| let meta = LspInvoiceMeta { | ||
| label: req.label.clone(), | ||
| payment_hash: res.payment_hash.to_string(), | ||
| requested_amount_msat: req.amount_msat, | ||
| bolt11: res.bolt11.clone(), | ||
| }; | ||
|
|
||
| write_lsp_invoice_meta(rpc, meta).await | ||
| .map_err(|e| Status::new(Code::Internal, e.to_string()))?; | ||
|
|
There was a problem hiding this comment.
I don't think we need to store the invoice meta data for a regular invoice. It will just double what we already have. Storing it for a jit-channel invoice is enough to identify it as such: If it's in the datastore, it automatically is a jit-channel invoice.
| pub label: String, | ||
| pub payment_hash: String, | ||
| pub requested_amount_msat: u64, | ||
| pub bolt11: String, |
There was a problem hiding this comment.
nit: I think we could also store the peer_id and the expected_amount_msat. But feel free to leave it out if it's not required.
| write_lsp_invoice_meta(rpc, meta).await | ||
| .map_err(|e| Status::new(Code::Internal, e.to_string()))?; |
There was a problem hiding this comment.
Do we really want to fail the whole RPC call in the (unexpected) case that we can't write the meta data? We already negotiated a jit-channel invoice and stored it on CLN. I think it's enough to just log::warn and continue. We won't loose much.
|
|
||
| /// Writes `LspInvoiceMeta` using datastore request. `LspInvoiceMeta` is useful for defining | ||
| /// some additional information regarding invoice being requested trough Greenlight. | ||
| async fn write_lsp_invoice_meta(mut rpc: tokio::sync::MutexGuard<'_, ClnRpc>, meta: LspInvoiceMeta) |
There was a problem hiding this comment.
nit: The signature is a bit weird, compared to the other callers of ClnRpc in this file. This function takes ownership of the lock, which means that the caller can't use rpc afterwards. Every other helper in this file takes &mut rpc:
async write_lsp_invoice_meta(rpc: &mut ClnRpc, meta: LspInvoiceMeta)
| let record_serialized = serde_json::to_string(&meta) | ||
| .map_err(|e| Status::new(Code::Internal, e.to_string()))?; |
There was a problem hiding this comment.
nit: This function returns an anyhow::Result. The extra serialization into a tonic::Status seems a bit useless. It's coerced into anyhow::Error and the caller converts it back into a tonic::Status.
| let record_serialized = serde_json::to_string(&meta) | |
| .map_err(|e| Status::new(Code::Internal, e.to_string()))?; | |
| let record_serialized = | |
| serde_json::to_string(&meta).context("failed to serialize LspInvoiceMeta")?; |
| let datastore_req = cln_rpc::model::requests::DatastoreRequest { | ||
| key: vec![ | ||
| "gl".to_string(), | ||
| "jit_channels".to_string(), | ||
| meta.label, | ||
| ], |
There was a problem hiding this comment.
Depending on what info the reading part actually has, I'd rather use the payment_hash as the preferred key instead of the label - which by the way can be an ugly (maybe even unbounded) string.
| let datastore_req = cln_rpc::model::requests::DatastoreRequest { | |
| key: vec![ | |
| "gl".to_string(), | |
| "jit_channels".to_string(), | |
| meta.label, | |
| ], | |
| let datastore_req = cln_rpc::model::requests::DatastoreRequest { | |
| key: vec![ | |
| "gl".to_string(), | |
| "lsp_invoices".to_string(), | |
| meta.payment_hash, | |
| ], |
2 participants
Problem
Currently, when a client requests for lsp invoice (JIT channel opening), no information about the original invoice is stored. Basically, the client hands out the requested invoice to a payer but receives only reduced amount. We'd like to have a way of understanding whether the requested invoice was for channel opening or a simple payment.
Solution
Introduced a structure
LspInvoiceMetawhich contains all information that we need to store (specifically, bolt11 invoice). Use datastore requests in order to store metadata.LspInvoiceMetais stored in both cases - if the invoice was requested from the LSP or not.Changes
LspInvoiceMetastructure for storing metaLspInvoiceMeta