add DeletePayment that allows to delete a specific payment or its failed HTLCs

[bjarnemagnussen]

Description

This PR adds a RPC DeletePayment that allows to delete a payment by specifying its payment hash. It also has the option to only delete failed HTLCs of a payment by setting the failed_htlcs_only option.

Similar to DeleteAllPayments payments that have status InFlight are not touched as they cannot be removed safely. Instead an error is returned.

[bjarnemagnussen]

I am not sure of the linting error. It seems to make a wrong suggestion?

[guggero]

I am not sure of the linting error. It seems to make a wrong suggestion?

Yeah, that's a bit annoying. You need to add the // nolint:interfacer comment to get rid of the suggestion.

[guggero]

Great PR and very useful feature, thank you very much!
Looks pretty good to me, mostly nits or test related suggestions.

[guggero]

nit: use require library for new code, here and below.

[orijbot]

Visit https://dashboard.github.orijtech.com?back=0&pr=5660&remote=true&repo=LN-Zap%2Flnd to see benchmark details.

[bjarnemagnussen]

Thanks @guggero for your review! I have squashed in the requested changes. Please let me know if there is anything else, or if I didn't fully address your issues.

[guggero]

tACK, LGTM 🎉

[carlaKC]

Thanks for the PR! Really useful feature :)

Just some nits from me.

[carlaKC]

s/attmpet/attempt

[carlaKC]

Could this be a helper function rather than a closure? It could also just return a bool indicating whether the payment was found then we require.True(t, found)

[bjarnemagnussen]

Just to be sure, do you prefer the function assertPayments to live outside together with some of the other assert helper function, and take the payments structure directly as argument instead of the indices (which could be of type []*MPPayment instead)?

[carlaKC]

Yeah, was thinking a separate function rather than inlining it, can still use indexes:

func assertPaymentsPresent(t * testing.T, [] payment) {
    dbPayments, err := db.FetchPayments()
    ...
}

Just cuts the need for the Outer loop, and can potentially be reused one day.

[bjarnemagnussen]

I have refactored the testing by creating a helper function assertPayments and also a helper function createTestPayments that registers those payments and populates the data into the payments slice: 1e613b6bbcf9d8b19c0a5a8d7e3a725486296242

Since these function can also directly be used the the test case for DeleteAllPayments (from which I took a lot of inspiration for the testing code!), I also refactored it to make use of the helper functions.

I did create its own commit without squashing for easier code review. Please let me know if there is something you feel is missing or you have another idea for. Thanks!

[carlaKC]

very nice! Thanks for taking the time on this :D Makes for very nice, readable tests.

[bjarnemagnussen]

Cool! I will then squash it and also address your nits!

[carlaKC]

extra-credit: make a note that you can't delete in-flight payments?

[bjarnemagnussen]

I have force-pushed the new changes and hope to have addressed all issues.

[guggero]

Thanks for the updates, @bjarnemagnussen! The unit tests seem to fail now. But everything else looks good, so we can merge once that's fixed.

[bjarnemagnussen]

Oh, I see what happened! PR #5635 changed the DB structure and I didn't rebase when running the unit tests locally. I have made the changes with the new db structure and it passes now.

[guggero]

Thanks for the fix. The remaining kvdb unit test seems to have been introduced by another recently merged PR, going to fix asap.