-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 coin input control #5142
Add coin input control #5142
Conversation
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.
Testing issue:
- Start with a BSQ wallet with no BTC balance
- BSQ selection popup shows when button is pressed
- Send BTC to BSQ receive address (remove leading "B")
- BTC balance section shows in BSQ wallet but neither "select inputs" button work.
core/src/main/java/bisq/core/btc/wallet/NonBsqCoinSelector.java
Outdated
Show resolved
Hide resolved
core/src/main/java/bisq/core/btc/wallet/BisqDefaultCoinSelector.java
Outdated
Show resolved
Hide resolved
desktop/src/main/java/bisq/desktop/main/dao/wallet/send/BsqSendView.java
Outdated
Show resolved
Hide resolved
// If we had some selection stored we need to update to already spent entries | ||
bsqUtxoCandidates = bsqUtxoCandidates.stream(). | ||
filter(e -> unspentTransactionOutputs.contains(e)). | ||
collect(Collectors.toSet()); |
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.
This would remove transactions that are no longer spendable BSQ outputs from the selected outputs. I think that could be confusing. Better to check if the bsqUtxoCandidates
are still all spendable, if they are then all is good, if not, then reset the selection to allow choosing between all unspentTransactionOutputs
again.
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.
To request the spendable via bsqWalletService.getSpendableBsqTransactionOutputs() and then check if the bsqUtxoCandidate is in that set does check if the bsqUtxoCandidate is spendable. We use the coinSelector for that and there is no API for checking one utxo if it is still spendable.
Or do I misunderstand what you mean?
btcUtxoCandidates = btcUtxoCandidates.stream(). | ||
filter(e -> unspentTransactionOutputs.contains(e)). | ||
collect(Collectors.toSet()); |
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.
Same comment as for the BSQ, could be confusing if the selection changes without the users knowing about it
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 you make a send tx and spend a previous selected utxo we need to remove that. We always show that spendable utxos and those are marked the user has selected. A previous selected which got spent need to be removed.
2fad9aa
to
b37dc15
Compare
There was a bug with the button handlers. Fixed in b37dc15 |
We use that to use only the selected utxos instead of all available.
…onOutputs methods. Add overridden getPreparedSendBsqTx and getPreparedSendBtcTx methods with utxoCandidates param. If utxoCandidates is not null we apply it to our coinSelector. As the coin selector is re-used we re-set it immediately after it was applied (inside coin selector select method). Set preferences in nonBsqCoinSelector
It is both for BSQ and non-BSQ (BTC) used.
b37dc15
to
9ae76b6
Compare
If text field is empty we apply ValidationResult(true), otherwise we apply the validate result with the text and the given validator.
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.
ACK
Adds input selection for BSQ send transactions. We use a utxo based model in contrast to the address based model in the Funds screen. If any dev wants to apply that to the funds screen as well would be good. Also it is better IMO to hide it in a popup as it is a bit technical and might confuse some users.