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

Make onion fields optional on lsp client hook handler #544

Merged
merged 4 commits into from
Nov 25, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
59 changes: 40 additions & 19 deletions libs/gl-plugin/src/lsp.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@ use serde_json::Value;
struct Onion {
payload: tlv::SerializedTlvStream,
short_channel_id: Option<ShortChannelId>,
forward_msat: Amount,
outgoing_cltv_value: u32,
forward_msat: Option<Amount>,
outgoing_cltv_value: Option<u32>,
#[serde(deserialize_with = "from_hex")]
shared_secret: Vec<u8>,
#[serde(deserialize_with = "from_hex")]
Expand Down Expand Up @@ -54,12 +54,35 @@ struct HtlcAcceptedResponse {
const TLV_FORWARD_AMT: u64 = 2;
const TLV_PAYMENT_SECRET: u64 = 8;

/// A macro to break out of the current hook flow and return a `continue`
/// signal to core-lightning. This is to be used when we don't know how to
/// handle a given payload or as a shortcut in case we could identify that the
/// incoming htlc is not part of a LSP jit channel opening.
macro_rules! unwrap_or_continue {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if this couldn't have been a trait on the Result type instead? That would retain the operation.unwrap_or_continue() order and allow better chaining, rather than having to wrap like this unwrap_or_continue!(operation).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is what i considered to do first, but I fail to see how this is possible. How can we return an outer function from an inner function without calling panic (like unwrap does)? I also explored the possibility of using a ControlFlow or a custom implementation of the Try trait, using the ?-operator, but that also seemed a bit overly complicated given that we only use this in one place for now (the try trait is experimental as well).
I would suggest to leave it as is for now and If we feel that we could profit from this pattern at other places as well, we can move it upstream to the cln-plugin repo and may come up with a solution that is a bit more aligned with the chaining approach that rust follows.

($res:expr) => {
match $res {
Ok(x) => x,
Err(e) => {
log::debug!("Lsp-plugin continue, reason: {}", e.to_string());
return Ok(serde_json::to_value(HtlcAcceptedResponse {
result: "continue".to_string(),
..Default::default()
})
.expect("Could not serialize json value"));
}
}
};
}

pub async fn on_htlc_accepted(plugin: Plugin, v: Value) -> Result<Value, anyhow::Error> {
let req: HtlcAcceptedRequest = serde_json::from_value(v).unwrap();
let req: HtlcAcceptedRequest = unwrap_or_continue!(serde_json::from_value(v));
log::debug!("Decoded {:?}", &req);

let htlc_amt = req.htlc.amount_msat;
let onion_amt = req.onion.forward_msat;
let onion_amt = unwrap_or_continue!(req.onion.forward_msat.ok_or(format!(
"payload={} is missing forward_msat",
&req.onion.payload
)));

let res = if htlc_amt.msat() < onion_amt.msat() {
log::info!(
Expand All @@ -70,7 +93,10 @@ pub async fn on_htlc_accepted(plugin: Plugin, v: Value) -> Result<Value, anyhow:

let mut payload = req.onion.payload.clone();
payload.set_tu64(TLV_FORWARD_AMT, htlc_amt.msat());
let payment_secret = payload.get(TLV_PAYMENT_SECRET).unwrap();
let payment_secret = unwrap_or_continue!(payload.get(TLV_PAYMENT_SECRET).ok_or(format!(
"payload={} is missing payment_secret",
&payload.to_string()
)));

let mut rpc = cln_rpc::ClnRpc::new(plugin.configuration().rpc_file).await?;
let res: cln_rpc::model::responses::ListinvoicesResponse = rpc
Expand All @@ -84,19 +110,14 @@ pub async fn on_htlc_accepted(plugin: Plugin, v: Value) -> Result<Value, anyhow:
limit: None,
})
.await?;
if res.invoices.len() != 1 {
log::warn!(
"No invoice matching incoming HTLC payment_hash={} found, continuing",
hex::encode(&req.htlc.payment_hash)
);
return Ok(serde_json::to_value(HtlcAcceptedResponse {
result: "continue".to_string(),
..Default::default()
})
.unwrap());
}

let total_msat = res.invoices.iter().next().unwrap().amount_msat.unwrap();
let invoice = unwrap_or_continue!(res.invoices.first().ok_or(format!(
"no invoice matching incoming HTLC payment_hash={} found",
hex::encode(&req.htlc.payment_hash),
)));
let total_msat = unwrap_or_continue!(invoice
.amount_msat
.ok_or("invoice has no total amount msat"));

let mut ps = bytes::BytesMut::new();
ps.put(&payment_secret.value[0..32]);
Expand Down Expand Up @@ -140,7 +161,7 @@ where
{
match buffer {
None => serializer.serialize_none(),
Some(buffer) => serializer.serialize_str(&hex::encode(&buffer.as_ref())),
Some(buffer) => serializer.serialize_str(&hex::encode(buffer)),
}
}

Expand All @@ -151,5 +172,5 @@ where
{
use serde::de::Error;
String::deserialize(deserializer)
.and_then(|string| Vec::from_hex(&string).map_err(|err| Error::custom(err.to_string())))
.and_then(|string| Vec::from_hex(string).map_err(|err| Error::custom(err.to_string())))
}
7 changes: 7 additions & 0 deletions libs/gl-plugin/src/tlv.rs
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,13 @@ impl<'de> Deserialize<'de> for SerializedTlvStream {
}
}

impl std::fmt::Display for SerializedTlvStream {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
let s = hex::encode(SerializedTlvStream::to_bytes(self.clone()));
write!(f, "{s}")
}
}

#[cfg(test)]
mod tests {
use super::*;
Expand Down
Loading
Loading