offers: add used_count to listoffers#9162
Conversation
85af710 to
e3719b3
Compare
This adds a new 'used_count' field to the 'listoffers' RPC command output to see the exact number of times a BOLT12 offer has been paid by counting associated settled invoices. Changelog-Added: add a new 'used_count' field to the 'listoffers' RPC command Closes ElementsProject#9146
e3719b3 to
5370032
Compare
nGoline
left a comment
There was a problem hiding this comment.
Great addition. This fills a real gap and the implementation is straightforward, the core idea is solid and it's close to being mergeable.
A few things to fix before we can land this.
| json_populate_offer(response, | ||
| &id, b12, | ||
| label, status, force_paths); | ||
| u64 used_count = wallet_offer_count_payments(wallet, offer_id); |
There was a problem hiding this comment.
You're passing offer_id instead of &id here. Since we're inside the else block (meaning offer_id was not provided), this would pass NULL to a function annotated with NON_NULL_ARGS(1,2), so every offer in the list-all path would get a count of 0 (or worse).
Should be &id.
| u64 wallet_offer_count_payments(struct wallet *w, | ||
| const struct sha256 *offer_id); | ||
| NON_NULL_ARGS(1,2); |
There was a problem hiding this comment.
You should remove the ; between the function declaration and NON_NULL_ARGS.
| u64 wallet_offer_count_payments(struct wallet *w, | |
| const struct sha256 *offer_id); | |
| NON_NULL_ARGS(1,2); | |
| u64 wallet_offer_count_payments(struct wallet *w, | |
| const struct sha256 *offer_id) | |
| NON_NULL_ARGS(1,2); |
| " AND state = 1;")); | ||
|
|
||
| db_bind_sha256(stmt, offer_id); |
There was a problem hiding this comment.
The hardcoded 1 is fragile and doesn't self-document intent. We have a enum constant for PAID.
| " AND state = 1;")); | |
| db_bind_sha256(stmt, offer_id); | |
| " AND state = ?;")); | |
| db_bind_sha256(stmt, offer_id); | |
| db_bind_int(stmt, invoice_status_in_db(PAID)); |
| &id, b12, | ||
| label, status, force_paths); | ||
| u64 used_count = wallet_offer_count_payments(wallet, offer_id); | ||
| json_add_u64(response, "used_count", used_count); |
There was a problem hiding this comment.
The indentation here is inconsistent with a mix of spaces and tabs. Please align it with the surrounding code (tabs only).
| "deprecated": null | ||
| }, | ||
| "ListOffers.offers[].used_count": { | ||
| "added": "pre-v0.10.1", |
There was a problem hiding this comment.
pre-v0.10.1 is reserved for fields that existed before versioning was introduced. This is a new field, so it needs the correct release tag. There's no time to include it in v26.06, so let's target the next one: v26.09.
| "required": [ | ||
| "offer_id", | ||
| "active", | ||
| "single_use", | ||
| "force_paths", | ||
| "bolt12", | ||
| "used" | ||
| ], |
There was a problem hiding this comment.
used_count is always computed and returned unconditionally, so it should be listed in the required array of the items object in this schema.
| stmt = db_prepare_v2(w->db, SQL("SELECT COUNT(*)" | ||
| " FROM invoices" | ||
| " WHERE local_offer_id = ?" | ||
| " AND state = 1;")); |
There was a problem hiding this comment.
This executes one SELECT COUNT(*) per offer, which means a node with 1000 offers will fire 1001 queries on every listoffers call. Consider computing all counts in a single query with GROUP BY local_offer_id, then looking up each offer's count from the result.
| stmt = db_prepare_v2(w->db, SQL("SELECT COUNT(*)" | ||
| " FROM invoices" | ||
| " WHERE local_offer_id = ?" | ||
| " AND state = 1;")); |
There was a problem hiding this comment.
Trailing whitespace: make check-source will catch this. Please remove the space after ));..
| &id, b12, | ||
| label, status, force_paths); | ||
| u64 used_count = wallet_offer_count_payments(wallet, offer_id); | ||
| json_add_u64(response, "used_count", used_count); |
There was a problem hiding this comment.
You're adding this field between json_populate_offer() output and description. Consider moving it after description so the JSON output is consistent between versions.
| assert 'used_count' in offers[0], "Error: used_count field is missing from listoffers response!" | ||
| assert offers[0]['used_count'] == 0 |
There was a problem hiding this comment.
Let's drop the custom message string. The in check is redundant since the == 0 assertion on the next line would already produce a KeyError if the key were missing. Plain assert is the project convention here.
| assert 'used_count' in offers[0], "Error: used_count field is missing from listoffers response!" | |
| assert offers[0]['used_count'] == 0 | |
| assert offers[0]['used_count'] == 0 |
|
Please rebase onto |
Also, when addressing the review feedback, please use |
This adds a new 'used_count' field to the 'listoffers' RPC command output to see the exact number of times a BOLT12 offer has been paid by counting associated settled invoices.
Changelog-Added: add a new 'used_count' field to the 'listoffers' RPC command
Closes #9146