Conversation
matheusd
left a comment
matheusd
left a comment
There was a problem hiding this comment.
Did a first pass review. TBH, I'm not super sure this should live in the wallet package, since you can effectively do this selection by calling the existing ListUnspent, then using whatever logic you want to select desired outputs.
Do you have a specific reasoning for why this needs to be in the wallet itself?
wallet/wallet.go
Outdated
|
|
||
| // SelectUnspent returns a slice of objects representing the unspent wallet | ||
| // transactions for the given criteria that are enough to pay the target amount. | ||
| // The transaction amount and confirmations will be greater than the amount |
There was a problem hiding this comment.
"The output amount [...] will be greater than the minAmount"
There was a problem hiding this comment.
minAmount refers to the minimum amount output value of transaction output should have before it is considered
wallet/wallet.go
Outdated
| case stake.TxTypeSStx: | ||
| // Ticket commitment, only spendable after ticket maturity. | ||
| if output.Index == 0 { | ||
| if !ticketMatured(w.chainParams, details.Height(), tipHeight) { |
There was a problem hiding this comment.
Hmm, if you're going to use the new selectunspent api to build regular txs, using this output without client-side filtering will break your code (because you can't arbitrarily spend a ticket submission - only votes and revocations can).
Any thoughts on how to handle this (whether clients need to be warned to verify this or have some flag to toggle inclusion of ticket submission outputs)?
There was a problem hiding this comment.
Ticket submissions will be removed by default, I don't think this API will be used to build votes or revocation transactions.
| if !coinbaseMatured(w.chainParams, details.Height(), tipHeight) { | ||
| continue | ||
| } | ||
| } |
There was a problem hiding this comment.
Need to also check if it's a tspend. OP_GEN outputs also need to pass maturity before being spent.
wallet/wallet.go
Outdated
| acct, err := w.manager.AddrAccount( | ||
| addrmgrNs, addrs[0]) | ||
| if err == nil { | ||
| s, err := w.manager.AccountName( | ||
| addrmgrNs, acct) | ||
| if err == nil { | ||
| acctName = s | ||
| } | ||
| } |
There was a problem hiding this comment.
Silencing errors in general is not a great idea. Seems you want to specifically handle the case of the address not existing (which as you noted in the comment earlier, shouldn't happen), so you could check for errors.NotExist directly:
acct, err := w.manager.AddrAccount(addrmgrNs, addrs[0])
if err == nil {
var s string
s, err = w.manager.AccountName(addrmgrNs, acct)
if err == nil {
acctName = s
}
}
if err != nil && !errors.Is(err, errors.NotExist) {
return err
}There was a problem hiding this comment.
Should I also make this change here https://github.com/decred/dcrwallet/blob/master/wallet/wallet.go#L3633-L3641
|
@matheusd the |
- Shuffle transactions using wallet package's shuffle() and remove redundant sort. - Change log level to trace
2 participants
This adds the SelectUnspent method to the JSON-RPC server. It is used for fetching unspent wallet transactions that are enough to pay a specified amount. The returned transactions can be filtered using the
minAmount,minconf&accountNameparameters. ThetargetAmountparameter specifies the minimum total amount that should be returned and it'll be ignored if thespendAllparameter is true.Inputs can be selected using the different methods specified by the
InputSelectionMethodparameter. These options consist ofRandomInputSelectionthat indicates any random utxo can be selected.RandomAddressInputSelectionthat indicates only utxos matching a randomly selected address should be selected.OneUTXOInputSelectionindicates only one utxo should be selected(and should be enough to pay targetAmount). As well asUniqueTxInputSelectionthat indicates only utxos with a unique address and hash should be selected.