multi: update listsweeps to allow sweeps that are not in ListTransactions #4762
Conversation
lnrpc/walletrpc/walletkit_server.go
Outdated
| return &ListSweepsResponse{ | ||
| Sweeps: &ListSweepsResponse_TransactionDetails{ | ||
| TransactionDetails: lnrpc.RPCTransactionDetails(transactions), | ||
| TransactionDetails: lnrpc.RPCTransactionDetails(sweepTxDetails), |
There was a problem hiding this comment.
I think both verbose and non-verbose response should be filtered, such that they are consistent (returning the same sweeps but with different level of detail). The linked bug seems to recommend the same. WDYT?
There was a problem hiding this comment.
Originally the point of having the non-verbose output was to save us the call to list all of our transactions from the wallet, which was my thinking in still returning the full list (incl rbf).
I also think there can be a use-case for knowing the txids of sweeps that never made it to chain (eg, the case where you have alerting on every tx your node broadcasts, you'd want to be able to check lnd's sweeps to whitelist the tx).
But I do agree it's confusing to have different output for verbose/not. Could add a IncludeDropped option which just returns all the sweeps when verbose is false, and does not work if verbose is true so that you can still get those rbf-ed txids if you need them? Unsure about it tbh.
There was a problem hiding this comment.
IMHO simplifying and only including sweeps that made it to the chain would make more sense. Having the IncludeDropped smells like something users probably ain't going to need (although a good generalization idea).
There was a problem hiding this comment.
Yeah SGTM, keep it consistent for now and then add the invisible/rbfed sweeps if anybody requires them. Will make a note in the proto that these sweeps aren't listed.
carlaKC
force-pushed
the
4748-sweeps
branch
2 times, most recently
from
d26a44e
to
a05189e
Compare
Previously, the verbose output of listsweeps would fail if we did not find some sweeps in our wallet's listtransactions output. This could be the case for sweeps that were rbf-ed, so the endpoint would fail. This commit also updates the listsweeps endpoint to always check against the wallet, so that we do not return these discarded sweeps that never confirmed.
Fixes #4748