Skip to content
This repository was archived by the owner on Jan 6, 2025. It is now read-only.

Documentation for lsps1 #77

Merged
merged 3 commits into from
Jan 10, 2024
Merged

Documentation for lsps1 #77

merged 3 commits into from
Jan 10, 2024

Conversation

Srg213
Copy link
Contributor

@Srg213 Srg213 commented Dec 26, 2023

Closes #67.

Copy link
Collaborator

@tnull tnull left a comment

Choose a reason for hiding this comment

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

Thanks! Did a first pass, here are a few questions.

It also unfortunately seems that our CI doc checks were not working correctly with cfg(lsps1), which is now fixed on main (see #78/#79). Could you rebase on main and make sure CI passes?

id: u128,
/// TODO
/// An identifier to track messages received.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need to expose this request id in the events? What do we expect the user to do with it?

Copy link
Contributor Author

@Srg213 Srg213 Jan 8, 2024

Choose a reason for hiding this comment

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

Right. It should not be exposed to user. Removed.

@@ -254,7 +255,12 @@ where
Ok(())
}

fn send_invoice_for_order(
/// Used by LSP to send invoice containing details regarding the channel fees and payment information.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm confused: this doesn't actually send and invoice (which seems more like the LSPS2-flow anyways), but simply a CreateOrderResponse. What am I missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This simply sends a CreateOrderResponse. Will rename the functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed to send_payment_details.

Copy link
Collaborator

@tnull tnull left a comment

Choose a reason for hiding this comment

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

Generally LGTM, mod one nit and the CI failing.

@Srg213
Copy link
Contributor Author

Srg213 commented Jan 9, 2024

Added commits only related to docs. I will create a new PR for api fixes.
Please approve this PR.

///
/// `counterparty_node_id` is the node_id of the LSP you would like to use.
///
/// 'channel_id' is the id used to uniquely identify the channel with counterparty node.
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Wrong ticks used here:

Suggested change
/// 'channel_id' is the id used to uniquely identify the channel with counterparty node.
/// `channel_id` is the id used to uniquely identify the channel with counterparty node.

But happy to fix this in a follow up sometime.

@tnull tnull merged commit 832b0b3 into lightningdevkit:main Jan 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LSPS1: Add missing docs
2 participants