-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Simplify coin selection for sendcoins #8516
base: master
Are you sure you want to change the base?
Conversation
Important Review SkippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Hello @hieblmi here is the PR |
Modified it, no need for this |
Hello @sputn1ck, do you think I should create a different function instead of adding an arg to an exported one as per your comment here: btcsuite/btcwallet#912 (comment) |
tACK. Tested locally and worked as expected with one utxo, sweep, and multiple utxos. Would like to see this merged as I am signing PSBTs manually because my application requires 1 input --> 1 output transactions. |
Is there an update on this @Chinwendu20 ? |
Thanks I should push an update by the end of this week.. |
c800df5
to
3e6ab6a
Compare
7b3d4ac
to
6659a2e
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.
Great work once again, got a small Q regarding the added itest and a UX issue I found.
func fetchUtxosFromOutpoints(utxos []*lnwallet.Utxo, | ||
outpoints []wire.OutPoint) ([]*lnwallet.Utxo, error) { | ||
|
||
lookup := fn.SliceToMap(utxos, func(utxo *lnwallet.Utxo) wire.OutPoint { |
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.
nice
name: "sendCoins, amount specified, " + | ||
"no select outpoints", | ||
amt: 200_000, | ||
}, |
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.
Are you referring to this test "sendCoins with selected utxos, expect change"
?
I was thinking of something like
{
selectedCoins: []btcutil.Amount{
50_000,
20_000,
1_000,
},
amt: 70_100,
...............,
},
@@ -330,16 +331,28 @@ var sendCoinsCommand = cli.Command{ | |||
"scripts", | |||
}, | |||
coinSelectionStrategyFlag, | |||
cli.StringSliceFlag{ | |||
Name: "utxo", |
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.
If we pass in the same utxo twice the command isn't failing which I think should happen from a UX perspective. I tested(last utxo is a dupe): reg_zane sendcoins --addr bcrt1pztmhas8h2vg8xfvj4kr4s3kgx69d4usq6uxxmyh6lhslrgllwpns87snxq --utxo 8c787614dc1adfda9d09f5a908853d8e91cc5579bc00e43c3958c5cb7dcfaf46:0 --utxo a2a9f76c008f4fecdcfe34fc93da1a5f4f7e63a1f896dec9869272b101ac89a9:1 --utxo a2a9f76c008f4fecdcfe34fc93da1a5f4f7e63a1f896dec9869272b101ac89a9:1 --sweepall
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.
Nice catch! I would fix it
This PR is now dependent on #8750 and btcsuite/btcwallet#928 |
607c321
to
27d8cdf
Compare
There are still itests failing. |
I think the taproot test needs to create a new node and not use the general Alice node. Two tests can request sendCoins to sweep all coins at the same time, creating the same transaction same txid, the other test can get the txid first into the mempool making the sweepall sendcoins operation fail in the other. I do not know if that makes sense. If it does I can open a new PR for a fix. In the meantime, this needs rebasing but I need to know where this stands in approval so I make changes at once. |
Maybe all tests should create their own node to prevent cases like this? |
Not sure how this applies to the tests, in here I think we should fix the itests in a separate commit in this PR. |
When I checked the logs for Alice related to this test, I saw this
sendcoins is called sequentially in that test but other tests using the Alice node would call sendcoins in parallel, right? Maybe if we rerun this it would pass, seems like a case of unprecedented slow server response |
fyi I ran this test locally and it was successful. |
Signed-off-by: Ononiwu Maureen <59079323+Chinwendu20@users.noreply.github.com>
Signed-off-by: Ononiwu Maureen <59079323+Chinwendu20@users.noreply.github.com>
Signed-off-by: Ononiwu Maureen <59079323+Chinwendu20@users.noreply.github.com>
Signed-off-by: Ononiwu Maureen <59079323+Chinwendu20@users.noreply.github.com>
Signed-off-by: Ononiwu Maureen <59079323+Chinwendu20@users.noreply.github.com>
Signed-off-by: Ononiwu Maureen <59079323+Chinwendu20@users.noreply.github.com>
Signed-off-by: Ononiwu Maureen <59079323+Chinwendu20@users.noreply.github.com>
Signed-off-by: Ononiwu Maureen <59079323+Chinwendu20@users.noreply.github.com>
Signed-off-by: Ononiwu Maureen <59079323+Chinwendu20@users.noreply.github.com>
Change Description
This PR fixes this issue: #6949 (comment)
Depends on: btcsuite/btcwallet#912 and #8729
In this well detailed issue comment, the process of sending funds using utxos selected by the sender currently has over five steps in which one has to call various psbt APIs.
The goal of this PR is to improve user experience by including
utxos
to thesendcoins
command and corresponding rpc structs (SendCoinsRequest
andSendCoinsResponse
) which would enable the sender described to achieve the same aim with one command.This is done by adding a new field to rpc structs
SendCoinsRequest
andSendCoinsResponse
to enable users request for that functionality and enable the driving function (in which we modify its functionality as well) use these utxos when crafting the transaction for this functionality.Additionally, the
sweepall
field in the above mentioned request and response rpc structs when true now not only sweeps ALL funds in the wallet but also ALL funds in the selected utxos when used in conjunction with the new select utxos field.The lncli sendcoins command was updated to include the flag,
utxo
to enable this functionality on that end.There were also slight internal logic change, where a slice of utxos are now accepted by relevant functions as variadic argument, functional options to enable this functionality.
Steps to Test
Steps for reviewers to follow to test the change.
Pull Request Checklist
Testing
Code Style and Documentation
[skip ci]
in the commit message for small changes.📝 Please see our Contribution Guidelines for further guidance.