Expose BOLT 12 refunds over gRPC - #262
Conversation
|
👋 Thanks for assigning @jkczyz as a reviewer! |
| let invoice = context.node.bolt12_payment().request_refund_payment(&refund)?; | ||
| let payment_id = invoice.payment_hash().to_string(); | ||
|
|
||
| Ok(Bolt12RequestRefundResponse { payment_id }) |
There was a problem hiding this comment.
Hmmm... we can't do this unfortunately as LDK Node was updated to decouple PaymentId from PaymentHash in lightningdevkit/ldk-node#948. Even more unfortunately, we rely on LDK for generating the PaymentId when the payment is received, so LDK Node will only know it when it processes the PaymentClaimable event.
We'll likely want to model Bolt11ReceiveResponse, which returns the PaymentHash, since they are both inbound payments. So in practice we just need to rename the field in the response and update the docs.
There was a problem hiding this comment.
Made Bolt12ReceiveRefundResponse return payment_hash
| let route_parameters = build_route_parameters_config_from_proto(request.route_parameters)?; | ||
| let refund = context.node.bolt12_payment().initiate_refund( | ||
| request.amount_msat, | ||
| request.expiry_secs, |
There was a problem hiding this comment.
Should we reject a 0-expiry? The CLI will give a more reasonable default if left unset, but other clients may forget to set it.
| |------------------------|-------------------------------------------------------------------------| | ||
| | `Bolt12Receive` | Create a BOLT12 offer (fixed or variable amount) | | ||
| | `Bolt12Send` | Pay a BOLT12 offer (with optional quantity, payer note, routing config) | | ||
| | `Bolt12InitiateRefund` | Create a BOLT12 refund | |
| message Bolt12InitiateRefundResponse { | ||
|
|
||
| // A BOLT12 refund that the recipient can use to request the refund payment. | ||
| string refund = 1; | ||
| } |
There was a problem hiding this comment.
Since this is equivalent to an outbound payment, we should have a payment_id here, but LDK Node doesn't return it. We'll want to do so upstream, IIUC, so we can return it here.
There was a problem hiding this comment.
Yeah, seems more systemic than this PR tho, can defer for now?
| // Return a BOLT12 refund. | ||
| rpc Bolt12InitiateRefund(Bolt12InitiateRefundRequest) returns (Bolt12InitiateRefundResponse); | ||
| // Request payment for a BOLT12 refund. | ||
| rpc Bolt12RequestRefund(Bolt12RequestRefundRequest) returns (Bolt12RequestRefundResponse); |
There was a problem hiding this comment.
We may want to break with the LDK Node naming convention and call these Bolt12SendRefund and Bolt12ReceiveRefund, though I don't have a strong opinion. The double "request" is just a bit icky.
There was a problem hiding this comment.
yeah this is better, fixed
| // Return a BOLT12 refund. | ||
| rpc Bolt12InitiateRefund(Bolt12InitiateRefundRequest) returns (Bolt12InitiateRefundResponse); | ||
| // Request payment for a BOLT12 refund. | ||
| rpc Bolt12RequestRefund(Bolt12RequestRefundRequest) returns (Bolt12RequestRefundResponse); |
There was a problem hiding this comment.
Also need MCP handlers for these.
f6eb35b to
5d630e7
Compare
| "type": "integer", | ||
| "minimum": 1, | ||
| "description": "Amount in millisatoshis to refund" |
There was a problem hiding this comment.
Should we do this elsewhere, too, for consistency?
There was a problem hiding this comment.
removed so we're consistent wit the rest for now
| if request.expiry_secs == 0 { | ||
| request.expiry_secs = DEFAULT_EXPIRY_SECS; | ||
| } |
There was a problem hiding this comment.
The schema as "minimum": 1,, so this will never be hit? Probably better to remove the minimum from the schema.
| // Refund expiry time in seconds. | ||
| uint32 expiry_secs = 2; |
There was a problem hiding this comment.
Should note that zero is rejected.
| assert_eq!(refund.payer_note().unwrap().to_string(), "test refund"); | ||
|
|
||
| let output = run_cli(&server_a, &["bolt12-receive-refund", refund_str]); | ||
| assert!(!output["payment_hash"].as_str().unwrap().is_empty()); |
There was a problem hiding this comment.
Could we check the payment hash now?
There was a problem hiding this comment.
added, also added mcp tests too
5d630e7 to
dd43c20
Compare
| fn tool_result_json(response: &Value) -> Value { | ||
| let text = response["result"]["content"][0]["text"].as_str().unwrap(); | ||
| serde_json::from_str(text).unwrap() | ||
| } |
There was a problem hiding this comment.
This could be used in a few other pre-existing places. Consider adding another commit to do so.
| async fn wait_for_event(events: &mut EventStream, pred: impl Fn(&Event) -> bool) -> EventEnvelope { | ||
| tokio::time::timeout(EVENT_TIMEOUT, async { | ||
| while let Some(Ok(event)) = events.next_message().await { | ||
| if event.event.as_ref().is_some_and(&pred) { | ||
| return event; | ||
| } | ||
| } | ||
| panic!("Event stream ended without matching event"); | ||
| }) | ||
| .await | ||
| .expect("Timed out waiting for event") | ||
| } |
There was a problem hiding this comment.
This is copied from e2e.rs. Could you refactor them into e2e-tests/src/lib.rs?
| }, | ||
| "expiry_secs": { | ||
| "type": "integer", | ||
| "description": "Refund expiry time in seconds" |
There was a problem hiding this comment.
Could you add "(defaults to 86400 if omitted or 0)" like a similar description in bolt11_receive_schema?
Share event waiting across test suites and centralize MCP tool response parsing so existing and future tests use consistent behavior. Created with Codex, an AI coding assistant.
Clients need both parts of the BOLT 12 refund flow. Add RPCs for creating a refund and requesting its payment, and expose them through the Rust client and CLI. Add end-to-end coverage for the reverse payment flow and document the new endpoints. This change was developed with OpenAI Codex assistance.
dd43c20 to
b661d5f
Compare
3 participants
Clients need both parts of the BOLT 12 refund flow. Add RPCs for creating a refund and requesting its payment, and expose them through the Rust client and CLI.
Add end-to-end coverage for the reverse payment flow and document the new endpoints.