-
Notifications
You must be signed in to change notification settings - Fork 590
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
Add Filter Utxo Filter Function #919
Conversation
e9ea499
to
2323d8c
Compare
wallet/wallet.go
Outdated
@@ -1259,6 +1261,7 @@ out: | |||
type txCreateOptions struct { | |||
changeKeyScope *waddrmgr.KeyScope | |||
selectUtxos []wire.OutPoint | |||
allowUtxo func(wtxmgr.Credit, int32) bool |
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.
I still haven't been able to convince myself why this needs to take block height as an argument.
157721b
to
6d5d1cf
Compare
wallet/createtx.go
Outdated
@@ -181,6 +183,13 @@ func (w *Wallet) txToOutputs(outputs []*wire.TxOut, | |||
) | |||
|
|||
for _, e := range eligible { | |||
// Restrict the selected utxos if a filter | |||
// function is provided. | |||
if allowUtxo != nil && |
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.
nowhere in the API docs do we state that a nil predicate has "always return true" semantics. I am not sure how and where to communicate this, but I may suggest that we either add it to the api docs or do away with it entirely and just pass in
always := func[A any](_ A) bool { return true }
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.
ahh you mean keeping as is but just add always
in tests where we would prior use nil
?
I think we need go 1.21 for this at least thats what my vs-code linter says:
Even if I do it like this:
// always defines a generic filter function which always returns true.
func always[A any](A) bool {
return true
}
and supply it to the function, it tells me:
implicitly instantiated function as argument requires go1.21 or latercompiler[UnsupportedFeature](https://pkg.go.dev/golang.org/x/tools/internal/typesinternal#UnsupportedFeature)
So used non-generic function for now:
alwaysAllowUtxo := func(utxo wtxmgr.Credit) bool { return true }
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.
Yeah the monomorphic one is fine by me. Also if you really think that leaving nil in place is preferred I can be convinced, but I do think that it means that we need to be consistent about the semantics of nil
predicates (functions from some A to bool). If we do this, we should commit to a convention that nil
predicates are equivalent to the always
I defined above.
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.
I like the design choice to only using nil
where we conventionally defined it otherwise it's not good practice. So for sure will stay with the always
function approach.
wallet/wallet.go
Outdated
@@ -1289,6 +1292,14 @@ func WithCustomSelectUtxos(utxos []wire.OutPoint) TxCreateOption { | |||
} | |||
} | |||
|
|||
// WithAllowUtxo is used to restrict the selection of the internal wallet inputs | |||
// by further external conditions. | |||
func WithAllowUtxo(allowUtxo func(utxo wtxmgr.Credit) bool) TxCreateOption { |
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.
suggestion: rename this to WithUtxoFilter
as it is fundamentally being used for a Filter operation
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.
I'm good to merge as is but would invite you to consider the suggestions
wallet/createtx.go
Outdated
|
||
continue | ||
} | ||
|
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.
Maybe we can move this check to findEligibleOutputs
function? Seems inline with the function definition and prevents this duplicity?
2a4a7d7
to
f7b2946
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.
LGTM! one nit
Allows to attach a utxo filter function when creating a transaction funded by the internal wallet.
f7b2946
to
2bc8246
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.
Nice, LGTM 🎉
Add Filter Utxo Filter Function
Add Filter Utxo Filter Function
Needed for the new design to fix lightningnetwork/lnd#8545