Skip to content

Update listsendpays documentation to explain when amount_msat is not present #8144

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

shashankxrm
Copy link

@shashankxrm shashankxrm commented Mar 6, 2025

Fixes #6909

Update the documentation of listsendpays to be more explicit about when the amount_msat field might not be present.

  • Update the description of the amount_msat field in doc/schemas/lightning-listsendpays.json to explain that it might not be present if the payment is still pending or if the amount delivered to the destination is not known.
  • Add a note in the response section to indicate that if the amount_msat field is not present, it means the payment is still pending or the amount delivered to the destination is not known.

For more details, open the Copilot Workspace session.

…present

Fixes ElementsProject#6909

Update the documentation of `listsendpays` to be more explicit about when the `amount_msat` field might not be present.

* Update the description of the `amount_msat` field in `doc/schemas/lightning-listsendpays.json` to explain that it might not be present if the payment is still pending or if the amount delivered to the destination is not known.
* Add a note in the `response` section to indicate that if the `amount_msat` field is not present, it means the payment is still pending or the amount delivered to the destination is not known.

---

For more details, open the [Copilot Workspace session](https://copilot-workspace.githubnext.com/ElementsProject/lightning/issues/6909?shareId=XXXX-XXXX-XXXX-XXXX).
Copy link
Collaborator

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

not able to approve your CI because looks like I do not have anymore the permission. However, probably there is an extra step to update the md files

@shashankxrm
Copy link
Author

not able to approve your CI because looks like I do not have anymore the permission. However, probably there is an extra tesp to update the md files

Can u brief me about that "extra test"

@vincenzopalazzo
Copy link
Collaborator

Sorry I made a typo, you need to update the md file too, there is an extra step that is running make doc-all, probably you did not clone the repo either if you modify the code with copilot workspace. The Ci will fail

@shashankxrm
Copy link
Author

Can you specify which md file

@ShahanaFarooqui ShahanaFarooqui added this to the v25.05 milestone Apr 15, 2025
Copy link
Contributor

@rustyrussell rustyrussell left a comment

Choose a reason for hiding this comment

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

Like the exposition, it's helpful, but the explanation is wrong.

@@ -144,7 +144,7 @@
"amount_msat": {
"type": "msat",
"description": [
"The amount delivered to destination (if known)."
"The amount delivered to destination (if known). This field might not be present if the payment is still pending or if the amount delivered to the destination is not known."
Copy link
Contributor

Choose a reason for hiding this comment

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

No, it means that the caller used "sendonion" so we don't know how much actually got delivered.

@@ -326,7 +326,8 @@
}
},
"pre_return_value_notes": [
"Note that the returned array is ordered by increasing *id*."
"Note that the returned array is ordered by increasing *id*.",
"If the `amount_msat` field is not present, it means the payment is still pending or the amount delivered to the destination is not known."
Copy link
Contributor

Choose a reason for hiding this comment

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

This one seems redundant?

@ShahanaFarooqui
Copy link
Collaborator

Hi @shashankxrm, while working on @rustyrussell's requested changes, could you please:

  • Re-base the PR on master and resolve conflicts.

  • Add a changelog entry in the commit/PR description following our guidelines.

  • As @vincenzopalazzo already suggested, rebuild the project after documentation changes to sync updates across markdown, man pages, and schema files like below:

./configure
make -j$(nproc)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Documentation of listsendpays
4 participants