-
Notifications
You must be signed in to change notification settings - Fork 968
lightningd: add description field to offer related responces #8782
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
base: master
Are you sure you want to change the base?
lightningd: add description field to offer related responces #8782
Conversation
c67d35a to
60073f9
Compare
00f9b51 to
d13eef1
Compare
Changelog-Added: Expose decoded offer description in `offer` and `listoffers` RPC responses.
d13eef1 to
053694a
Compare
ShahanaFarooqui
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rustyrussell Currently, listsendpays and sql -k query="SELECT * FROM sendpays;" do not return the Bolt11/Bolt12 description. In contrast, listinvoices and sql -k query="SELECT * FROM invoices;", and listoffers and sql -k query="SELECT * FROM offers;" (after this PR), will also show the description. Should we extend the same functionality to sendpays as well? If so, I will create a separate issue for that and keep this PR focused on offers only.
lightningd/offer.c
Outdated
| json_add_bool(response, "active", offer_status_active(status)); | ||
| json_add_bool(response, "single_use", offer_status_single(status)); | ||
| json_add_string(response, "bolt12", b12); | ||
| if (description) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding description here causes it to appear in the offer RPC response as well, which results in a msggen error because that field already exists in the offer request. We can rather add description separately in listoffers, disableoffer and enableoffer responses.
lightningd/offer.c
Outdated
| json_add_bool(response, "single_use", offer_status_single(status)); | ||
| json_add_string(response, "bolt12", b12); | ||
| if (description) | ||
| json_add_string(response, "description", description); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A better approach is to use json_add_stringn, since it accepts an explicit length parameter and doesn’t require the string to be null-terminated. Using json_add_string will result in garbage data being appended after the description value.
lightningd/offer.c
Outdated
| if (!offer->offer_description) | ||
| return NULL; | ||
|
|
||
| return (const char *)offer->offer_description; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Type casting for description is not required.
lightningd/offer.c
Outdated
|
|
||
| response = json_stream_success(cmd); | ||
| json_populate_offer(response, offer_id, b12, label, status); | ||
| json_populate_offer(response, offer_id, b12, label, description, status); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whenever we add or remove an RPC, or modify any RPC fields, the documentation JSON schema must also be updated, as explained here, for Core Lightning to build successfully. It also auto-generates the new protos. In this case, we need to add the description field — along with its added version tag (v26.04) — to listoffers, enableoffer, and disableoffer.
lightningd/offer.c
Outdated
|
|
||
| response = json_stream_success(cmd); | ||
| json_populate_offer(response, offer_id, b12, label, status); | ||
| json_populate_offer(response, offer_id, b12, label, description, status); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Writing a quick test for these new additions would be a good way to verify that the changes are working as expected. Currently, tests/test_plugin.py::test_sql is failing because the offers table in our SQL plugin now also expects the description field.
Fixes #8554.
Add description field for offer related responces. Examples:
% lightning-cli --network=regtest --lightning-dir=/tmp/cln1 listoffers { "offers": [ { "offer_id": "41ebdcd9d7b72e7b6e4294ab6fec93c8e99c4fe1df7b3503386b64aeefc233f0", "active": true, "single_use": false, "bolt12": "lno1qgsqvgnwgcg35z6ee2h3yczraddm72xrfua9uve2rlrm9deu7xyfzrcgq9jq5rjrdanxvet9ypcxz7tdv4h8gysqzcss8zeszk6dq87kufv7nswc9l22kfsy3x9ckuxavvle8q27gf5k2j8u", "description": "Coffee payment", "used": false, "label": "myoffer" } ] }