walletrpc: add default values to listunspent #6190
Conversation
ErikEk
changed the title
ErikEk
force-pushed
the
listunspent-deafult-values
branch
from
361ec28 to
5f3f40d
Compare
ErikEk
force-pushed
the
listunspent-deafult-values
branch
from
5f3f40d to
3252c12
Compare
lnrpc/walletrpc/walletkit.proto
Outdated
|
|
||
| /* | ||
| When min_confs and max_confs are zero, setting false implicitly | ||
| overrides max_confs to be MaxInt32, otherwise max_confs remains |
There was a problem hiding this comment.
nit: I think there are some extra spaces here
There was a problem hiding this comment.
@positiveblue I'm not sure if I saw the problem. Hope this look better.
ErikEk
force-pushed
the
listunspent-deafult-values
branch
2 times, most recently
from
7f94691 to
cb8e4ab
Compare
There was a problem hiding this comment.
Thanks for adding these @ErikEk ! since this is kinda repeating what is done on the lncli side, i think we can remove the default values from lncli now and just pass through the already existing unconfirmed_only flag in the request. Just so that we dont have duplicated logic for this. wdyt?
lnrpc/walletrpc/walletkit.proto
Outdated
| @@ -11,8 +11,14 @@ option go_package = "github.com/lightningnetwork/lnd/lnrpc/walletrpc"; | |||
| // daemon's wallet. | |||
| service WalletKit { | |||
| /* | |||
| ListUnspent returns a list of all utxos spendable by the wallet with a | |||
| number of confirmations between the specified minimum and maximum. | |||
| ListUnspent lists for each spendable utxo currently in the wallet, | |||
There was a problem hiding this comment.
im not sure that this comment is clearer. I think the previous comment is clear enough and then maybe we can just add something like "by default, all utxos are listed. To list only the unconfirmed utxos, set the unconfirmed_only to true."
There was a problem hiding this comment.
@ellemouton I agree.
|
@ellemouton Thank you for your input. listunspent (lncli listunspent/GET /v1/utxos) is deprecated and it is suggested we use walletrpc.ListUnspent instead. Line 68 in 9a50cc9 As is, there is no rpc (only REST) endpoint for listunspent functionality in the walletrpc subserver, hence there will be no duplication of logic. I can add the rpc command lncli wallet listunspent if you want?
|
ErikEk
force-pushed
the
listunspent-deafult-values
branch
2 times, most recently
from
3c0c0f0 to
0ee6907
Compare
ErikEk
force-pushed
the
listunspent-deafult-values
branch
from
0ee6907 to
74db83b
Compare
|
@ErikEk, remember to re-request review from reviewers when ready |
| number of confirmations between the specified minimum and maximum. By | ||
| default, all utxos are listed. To list only the unconfirmed utxos, set | ||
| the unconfirmed_only to true. |
Indeed, you are correct. My bad 👍 |
|
We found out that this PR is actually a breaking change for users that use min and max both at zero. |
|
@joostjager Let me look into it. |
|
@joostjager We had to override max_confs to be a MaxInt32, in order to return all confirmed and unconfirmed utxos as a default response. Before the change the command didn't worked as excepted. |
|
I think this merge broke '/v1/utxos' but it was depreciated already. |
Yes, so this is exactly the breaking change. Previously the default behavior was to return only the unconfirmed utxos, and after the change it returns everything. Users relying on that can get into trouble. |
|
@joostjager At this moment I'm not sure the best decision would be to reverse it to the old behavior, since we get the same issue but reversed (since this has been in the client for some time). |
|
Yes, that's a dilemma indeed. Given that nobody complained yet beside me since the release of 0.15, it's probably fine to leave as is. Mainly wanted to flag it to perhaps give extra attention to this for future changes. |
Adds default values to walletrpc ListUnspent RPC call as requested in #6028.
Fixes #6028