Restrict drain_to usage#625
Conversation
There was a problem hiding this comment.
Tested ACK.
I prepared the following table to check if all the possible execution paths
including the new conditions in create_tx are tested:
x = doesn't care conditions
drain_to.is_some() |
drain_wallet |
!utxos.is_empty() |
drain_val.is_dust(...) |
Test |
|---|---|---|---|---|
| False | x | x | x | test_create_tx_empty_recipients |
| True | False | False | False | test_create_tx_drain_to_no_drain_wallet_no_utxos (NEW TEST) |
| True | False | True | False | test_create_tx_drain_to_and_utxos (NEW TEST) - test_bump_fee_reduce_single_recipient |
| True | True | x | False | test_create_tx_drain_wallet_and_drain_to |
| True | True | x | True | test_create_tx_absolute_high_fee - test_create_tx_drain_to_dust_amount |
| True | False | True | True | ? |
I don't see much gain in including a test for the last case in terms of tested
functionality. However, it may worth the effort to document the possible ways
create_tx can be called.
Any test for RBF with a too high fee rate should do
the trick.
rajarshimaitra
left a comment
rajarshimaitra
left a comment
There was a problem hiding this comment.
tACK 098c448
Code changes looks good to me. Just one comment.
|
I think this needs a rebase to pickup the "Blockchain test-rpc-legacy" CI test, then we can merge it. |
danielabrozzoni
force-pushed
the
620_drain_to_fix
branch
from
098c448 to
6a9eb60
Compare
|
Looks like Github Action got updated and now CI is broken 🔥 |
Before this commit, you could create a transaction with `drain_to` set without specifying recipients, nor `drain_wallet`, nor `utxos`. What would happen is that BDK would pick one input from the wallet and send that one to `drain_to`, which is quite weird. This PR restricts the usage of `drain_to`: if you want to use it as a change output, you need to set recipients as well. If you want to send a specific utxo to the `drain_to` address, you specify it through `add_utxos`. If you want to drain the whole wallet, you set `drain_wallet`. In any other case, if `drain_to` is set, we return a `NoRecipients` error. Fixes bitcoindevkit#620
danielabrozzoni
force-pushed
the
620_drain_to_fix
branch
from
5c682f9 to
a34a257
Compare
|
Rebased :) |
danielabrozzoni
force-pushed
the
620_drain_to_fix
branch
from
a34a257 to
6a15036
Compare
6 participants
Description
Before this commit, you could create a transaction with
drain_tosetwithout specifying recipients, nor
drain_wallet, norutxos. What wouldhappen is that BDK would pick one input from the wallet and send
that one to
drain_to, which is quite weird.This PR restricts the usage of
drain_to: if you want to use it as achange output, you need to set recipients as well. If you want to send
a specific utxo to the
drain_toaddress, you specify it throughadd_utxos. If you want to drain the whole wallet, you setdrain_wallet. In any other case, ifdrain_tois set, we return aNoRecipientserror.Fixes #620
Checklists
All Submissions:
cargo fmtandcargo clippybefore committingBugfixes: