Skip to content
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

add listing options to listpays #7385

Merged
merged 2 commits into from
Nov 12, 2024

Conversation

daywalker90
Copy link
Contributor

Fixes #6338 and #6348?

First time writing copy-pasting c code btw

@daywalker90 daywalker90 force-pushed the listpays-index branch 2 times, most recently from 1a2bf46 to c3f185a Compare August 9, 2024 18:05
@ShahanaFarooqui ShahanaFarooqui added this to the v24.11 milestone Aug 9, 2024
@Lagrang3
Copy link
Collaborator

Hi @daywalker90. The way I read it it seems that the option index is a boolean not a string, cause you are using it as a flag, right?

@Lagrang3
Copy link
Collaborator

Lagrang3 commented Aug 10, 2024

Instead of 3 extra cmd line arguments you could have only 1. For example index_range:

example 1. I want to see a range of known indexes

lightning-cli -k listpays index_range="[1,10]"

example 2. I want to see the last 10 pays (wit pythonic indexing):

lightning-cli -k listpays index_range="[-10,:]"

I think it is very useful to be able to see the last N pays instead of the first N pays.

@daywalker90
Copy link
Contributor Author

I wanted it to be the same as index, start and limit in listinvoices and listforwards

@Lagrang3
Copy link
Collaborator

I wanted it to be the same as index, start and limit in listinvoices and listforwards

Cool. I didn't know about those.

@daywalker90 daywalker90 force-pushed the listpays-index branch 2 times, most recently from 3f5e4f6 to d5e56d6 Compare November 9, 2024 16:17
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.

This is great! Minor changes only...

plugins/pay.c Outdated Show resolved Hide resolved
doc/schemas/lightning-listpays.json Outdated Show resolved Hide resolved
doc/schemas/lightning-listpays.json Outdated Show resolved Hide resolved
plugins/pay.c Outdated Show resolved Hide resolved
plugins/pay.c Outdated Show resolved Hide resolved
@@ -491,6 +514,8 @@ static struct command_result *listsendpays_done(struct command *cmd,
pm->sortkey.payment_hash = pm->payment_hash;
pm->sortkey.groupid = groupid;
pm->success_at = UINT64_MAX;
pm->created_index = created_index;
pm->updated_index = updated_index;
Copy link
Contributor

Choose a reason for hiding this comment

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

OK, here's the problem the other side of this "if (!pm)" statement. If we have seen part of this payment before, what do we use the created and update indexes?

For updated_index, I think we have to use the greatest we've seen (if any part of a payment gets updated, it's been updated), and for created_index I think it's the smallest we've seen (that's stable: we might create a new part of an existing payment).

This makes sense if we were to implement "wait" for pays, too (that requires more infrastructure to allow that to work in plugins).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm sorry i don't have a clue what's going on here, i just put it there because i saw the other values there :/

For updated_index, I think we have to use the greatest we've seen (if any part of a payment gets updated, it's been updated), and for created_index I think it's the smallest we've seen (that's stable: we might create a new part of an existing payment).

Yes, but i don't know how to do it, sorry.

Copy link
Contributor

Choose a reason for hiding this comment

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

No problems, I can fix this up for you to review!

@rustyrussell
Copy link
Contributor

Re-requesting review!

@daywalker90
Copy link
Contributor Author

I've tested this with my plugin summars and on the cli. It works as intended, thanks rusty!

daywalker90 and others added 2 commits November 12, 2024 09:06
Changelog-Added: JSON-RPC: `listpays` has `index`, `start` and `limit` parameters for listing control.
1. It's called listpays not listpay.
2. "index" does NOT have a default value (it must be specified if limit or start are used)
3. Note that limit and start have effects on accuracy, since we combine records.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell
Copy link
Contributor

Trivial rebase for conflict in genfile.

@vincenzopalazzo vincenzopalazzo merged commit fcdbbd8 into ElementsProject:master Nov 12, 2024
27 of 39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] listpays, listinvoices: display most recent N
5 participants