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

lncli: add deletepayments command #5699

Merged
merged 5 commits into from
Sep 27, 2021

Conversation

guggero
Copy link
Collaborator

@guggero guggero commented Sep 7, 2021

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.

@guggero guggero added payments Related to invoices/payments lncli labels Sep 7, 2021
@guggero guggero added this to the v0.14.0 milestone Sep 7, 2021
@orijbot
Copy link

orijbot commented Sep 7, 2021

Copy link
Collaborator

@carlaKC carlaKC left a 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?

cmd/lncli/cmd_payments.go Outdated Show resolved Hide resolved
cmd/lncli/cmd_payments.go Outdated Show resolved Hide resolved
@guggero guggero force-pushed the lncli-deletepayment branch 2 times, most recently from 61c0a6f to 34a293a Compare September 7, 2021 14:35
@guggero
Copy link
Collaborator Author

guggero commented Sep 7, 2021

whether we want to have an "are you sure + this might take a while"

I think making it an explicit flag (--include_non_failed) is probably enough? Added the "this might take a while" output though.

Copy link
Member

@yyforyongyu yyforyongyu left a 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.

cmd/lncli/cmd_payments.go Outdated Show resolved Hide resolved
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.
@Roasbeef
Copy link
Member

Lol I totally missed this when I made #5778

cmd/lncli/cmd_payments.go Show resolved Hide resolved
Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

LGTM 🌂

@Roasbeef
Copy link
Member

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.
@guggero guggero merged commit f60a1ba into lightningnetwork:master Sep 27, 2021
@guggero guggero deleted the lncli-deletepayment branch September 27, 2021 11:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lncli payments Related to invoices/payments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants