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

Support the the filtering option by status in the listpays and listsendpays #4595

Merged

Conversation

vincenzopalazzo
Copy link
Collaborator

@vincenzopalazzo vincenzopalazzo commented Jun 11, 2021

This is a working in progress feature and it is a proposal to give the user to see the possibility to see the payment filtered by status.

Fixes #4588

  • Support new propriety in the last command that is status
  • Sanity check in the status propriety
  • Adding a test to make sure that the filter option works
  • Update docs

Signed-off-by: Vincenzo Palazzo [email protected]

@vincenzopalazzo vincenzopalazzo force-pushed the last_payment_failure branch 3 times, most recently from bb02f7d to fe74e5c Compare June 14, 2021 15:21
@vincenzopalazzo
Copy link
Collaborator Author

The GitHub action failure seems unrelated https://github.com/ElementsProject/lightning/runs/2821322348?check_suite_focus=true#step:5:2397

It looks like related to #4551.

I retry to be lucky with GitHub actions, lets see 💺

@vincenzopalazzo vincenzopalazzo force-pushed the last_payment_failure branch 3 times, most recently from c3a4459 to ff0b93e Compare June 16, 2021 09:53
@vincenzopalazzo
Copy link
Collaborator Author

I move it as a draft, we can wait for it the release is around the corner.

@vincenzopalazzo vincenzopalazzo marked this pull request as draft June 16, 2021 09:57
@vincenzopalazzo vincenzopalazzo force-pushed the last_payment_failure branch 2 times, most recently from 77c48d1 to aa33880 Compare June 23, 2021 11:06
@vincenzopalazzo vincenzopalazzo force-pushed the last_payment_failure branch 4 times, most recently from a4d0df4 to 5d1e590 Compare June 29, 2021 12:27
@vincenzopalazzo vincenzopalazzo force-pushed the last_payment_failure branch 4 times, most recently from e27cc17 to baf1f1d Compare July 20, 2021 21:17
@vincenzopalazzo vincenzopalazzo marked this pull request as ready for review July 20, 2021 21:29
@vincenzopalazzo vincenzopalazzo force-pushed the last_payment_failure branch 4 times, most recently from 4166813 to d2bf904 Compare July 22, 2021 07:24
@vincenzopalazzo
Copy link
Collaborator Author

The failure in CI seems to be unrelated.

What do you think about the point refactoring in the wallet.c to make the if-else smaller and more readable @rustyrussell do your think that is a good refactoring in the wallet.c file?

I want to make an append operation from a default string before making the query. in this way, we have a smaller if-else, but I don't know if it is an antipattern in C

@vincenzopalazzo vincenzopalazzo force-pushed the last_payment_failure branch 2 times, most recently from e143c06 to 7a6817c Compare August 13, 2021 10:35
@vincenzopalazzo vincenzopalazzo force-pushed the last_payment_failure branch 2 times, most recently from 2276cf8 to 5187fac Compare September 10, 2021 15:18
@vincenzopalazzo vincenzopalazzo force-pushed the last_payment_failure branch 3 times, most recently from b074fd0 to 6038609 Compare September 14, 2021 08:01
@vincenzopalazzo
Copy link
Collaborator Author

Rebased on top of master, and add missing fix around the code, like formatting fix and doc fixes

Copy link
Member

@cdecker cdecker left a comment

Choose a reason for hiding this comment

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

The last commit needs to be removed, since at the raw DB layer we don't have the required context on the schema to even know if the field is nullable in the first place. There are several updates to the lnprototest submodule which I think are unwranted, could you filter those out?

Otherwise excellent proposal and rework 👍

wallet/wallet.c Outdated Show resolved Hide resolved
wallet/db.c Outdated Show resolved Hide resolved
wallet/wallet.c Outdated Show resolved Hide resolved
Copy link
Collaborator Author

@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.

There are several updates to the lnprototest submodule which I think are unwranted, could you filter those out?

@cdecker I think this message was referring to an old commit, I check the actual file change history and there are no lnprototest changes here, but I remember that I have a bad life some day ago with the lnprototest update.

There is other stuff there I need to do regarding to lnprototest?

@vincenzopalazzo vincenzopalazzo force-pushed the last_payment_failure branch 3 times, most recently from 8b96161 to 0e7e2bf Compare September 15, 2021 19:06
wallet/db.c Outdated Show resolved Hide resolved
@vincenzopalazzo vincenzopalazzo force-pushed the last_payment_failure branch 4 times, most recently from 0065464 to fb1cef4 Compare September 20, 2021 14:53
Changelog-Added: Support to listpays the status parameter to filter the payments by status.

Signed-off-by: Vincenzo Palazzo <[email protected]>
Signed-off-by: Vincenzo Palazzo <[email protected]>

Rebase

Signed-off-by: Vincenzo Palazzo <[email protected]>
Suggested by @cdecker

P.S: Also this include an API refactoring from my previous solution, also this it is suggested by @cdecker.

Signed-off-by: Vincenzo Palazzo <[email protected]>
@rustyrussell
Copy link
Contributor

Ack 72c13d7

@rustyrussell rustyrussell merged commit eb103c1 into ElementsProject:master Sep 22, 2021
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 request: list of recently-failed payments
3 participants