-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
lncli: add deletepayments command #5699
Conversation
Visit https://dashboard.github.orijtech.com?back=0&pr=5699&remote=true&repo=guggero%2Flnd to see benchmark details. |
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.
tACK for the last 4 commits, nice refactoring in this as well!
Only question I have here is whether we want to have an "are you sure + this might take a while" prompt for when we delete all payments?
61c0a6f
to
34a293a
Compare
I think making it an explicit flag ( |
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.
LGMT, just a typo and pending a release note.
34a293a
to
a04cf31
Compare
a04cf31
to
def463e
Compare
With this commit we move all commands that can be found within the "Payments" category into its own file called cmd_payments.go. The only exception are the Mission Control related commands which we are going to move to their own category and file in the next commit.
To make the "Payments" category a bit less overloaded and to move the Mission Control configuration commands away from the "root" category, we create the new "Mission Control" category that we move the commands into. We do this in a single commit so the next one where we move them into the same file can be a pure code move without any additional changes.
Having one file per sub command seems a bit too excessive and isn't really implemented for any of the other commands. So we just group the Mission Control commands into their own file for now.
def463e
to
48ca55f
Compare
Lol I totally missed this when I made #5778 |
48ca55f
to
49ef29f
Compare
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.
LGTM 🌂
Just needs the release notes, and this is g2g |
To give the CLI user the option to delete a single or multiple payments in one command, we expose the DeletePayment and DeleteAllPayments RPCs to the command line as well. Due to the similarity of the two RPCs we combine them into a single command with multiple flags. To make the command a bit more safe to run without arguments, we consciously switch the logic of the RPC flag "failed_payments_only" which is false by default into a "--include_non_failed" in the CLI which is false by thefault. So a user running the command without knowing what they are doing are only deleting failed payments by default, not all of the payments.
2219812
to
57d203c
Compare
Depends on #5660.
To give the CLI user the option to delete a single or multiple payments
in one command, we expose the DeletePayment and DeleteAllPayments
RPCs to the command line as well. Due to the similarity of the two RPCs
we combine them into a single command with multiple flags.
To make the command a bit more safe to run without arguments, we
consciously switch the logic of the RPC flag "failed_payments_only"
which is false by default into a "--include_non_failed" in the CLI which
is false by thefault. So a user running the command without knowing what
they are doing are only deleting failed payments by default, not all of
the payments.
We also do some re-grouping and code movement to consolidate all payment
related commands into the same file.