Use selected Utxos while crafting txn #912
Conversation
There was a problem hiding this comment.
Thanks for the PR @Chinwendu20 !
I left some comments. I think we should definitely cover the new code in some test cases.
wallet/createtx.go
Outdated
| if err != nil { | ||
| return err | ||
|
|
||
| arrangedCoins := wrappedEligible |
There was a problem hiding this comment.
We're using the arrangedCoins later in inputSource := makeInputSource(arrangedCoins). I wonder what happens if we're using an output amount much small than the combined inputs, I think it would be expected to use all the provided input, but I'm not sure if makeInputSource will do that. I think some test coverage is definitely needed here.
There was a problem hiding this comment.
If I understand correctly, you are asking what would happen if a user wants to send an amount way smaller than the combined amount present in the select utxos?
I do not think that scenario is a cause for alarm or violates anything?
There was a problem hiding this comment.
What I'm trying to convey is that not the arrangedCoins, but the inputSource is the final instance for selecting the inputs. In the current approach if we have preselected 2 inputs[1btc, 1btc] and want to send to an output with 0.5btc we would only select the first input. I don't think this is the intention of this PR, as we want to use all the inputs we select.
See:
Line 25 in 1f3534b
btcwallet/wallet/txauthor/author.go
Line 103 in 1f3534b
There was a problem hiding this comment.
Ohh I must have missed this requirement, my understanding was that we needed to use the utxos provided by the users only to make a decision on what to send to an address. I did not realize we had to use all the inputs. I would have to close this PR then to use the psbt API on lnd side.
Signed-off-by: Ononiwu Maureen <[email protected]>
Signed-off-by: Ononiwu Maureen <[email protected]>
Signed-off-by: Ononiwu Maureen <[email protected]>
|
I think we can continue with this approach, more straightforward than the psbt API, having to fund, finalize and all that, this is more direct. Made some changes with respect to the requirement as well. |
wallet/createtx_test.go
Outdated
| @@ -399,3 +402,96 @@ func TestCreateSimpleCustomChange(t *testing.T) { | |||
| require.Equal(t, scriptType, txscript.WitnessV0PubKeyHashTy) | |||
| } | |||
| } | |||
|
|
|||
| // TestCreateSimpleCustomChange tests that it's possible to let the | |||
| // CreateSimpleTx use all coins for coin selection, but specify a custom scope | |||
Chinwendu20
force-pushed
the
sendcoin
branch
6 times, most recently
from
ed1195c to
508c3a0
Compare
wallet/createtx.go
Outdated
| } | ||
| } | ||
|
|
||
| arrangedCoins, err := coinSelectionStrategy. |
There was a problem hiding this comment.
Maybe we should just rename coinSelectionStrategy to simply strategy to avoid this weird folding. The type contains enough information to warrant this shorter name IMO.
There was a problem hiding this comment.
I would change the name to that. But I am curious in the case we are not able to change the name, what would be the best way to handle this situation?
There was a problem hiding this comment.
Then the way you formatted it is would have been correct. I just really personally don't like that style, so I always try to avoid it if possible. There are a couple of tricks you can use to achieve that: Shorten the name of the struct the method is called on, shorten the method name, extract the code into its own function/method to reduce indentation, reduce indentation by avoiding else blocks where it makes sense (e.g. return in the if, so the else block can continue un-indented) and so on.
Signed-off-by: Ononiwu Maureen <[email protected]>
Chinwendu20
force-pushed
the
sendcoin
branch
2 times, most recently
from
0964003 to
3a16e1f
Compare
wallet/wallet.go
Outdated
| coinSelectionStrategy, label, selectedUtxos...) | ||
| } | ||
|
|
||
| // sendOutputs creates and sends payment transactions.It returns the transaction |
There was a problem hiding this comment.
nit: missing space after full stop.
wallet/createtx.go
Outdated
| e, ok := eligibleByOutpoint[outpoint] | ||
|
|
||
| if !ok { | ||
| return fmt.Errorf("selected outpoint"+ |
There was a problem hiding this comment.
nit: missing space after word "outpoint".
Signed-off-by: Ononiwu Maureen <[email protected]>
Use selected Utxos while crafting txn
Use selected Utxos while crafting txn
Related issue: lightningnetwork/lnd#6949 (comment)
and it is needed for lightningnetwork/lnd#8516
This pull request enables
SendOutputin the wallet to use passed UTXOS as inputs for transactions. All passed UTXOS would be used as inputs.