Dialog for allowing the user to choose the change output when bumping a tx #700
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
Can you add a screenshot to the PR description (and one for the single-output case)? |
There was a problem hiding this comment.
Approach ACK
I think I'd prefer if the user had some more information about the outputs in the dialog. Maybe something like this, but worded better:
Maybe the actual change address isn't even needed, or could be revealed in a tooltip.
Also, if there's a single change output with sufficient funds for feebumping, then selecting "none" has basically no effect compared to selecting that output, as the wallet will always(?) deduct from that existing change output. Maybe this common case can be simplified by removing the "None" option?
|
Can we be a bit more bold and just assume that a change address is change, and not show the dialog in that case? Ideally we don't show such a dialog more often than necessary. |
|
That would prevent the case where you wanted to deduct the extra fee from the recipient, such as when you initially selected "Subtract fee from amount". Ideally, you'd only show the dialog if that were selected previously, but I don't think it's persisted anywhere. Eg - gui/src/qt/sendcoinsrecipient.h Line 42 in 30f553d I could be wrong, though. |
hebasto
changed the title
achow101
force-pushed
the
bumpfee-choose-reduce-output
branch
from
bf222ab
to
6547892
Compare
|
Concept ACK. I'd just show "Change" for the change, though. End users shouldn't need to know about change addresses. |
achow101
force-pushed
the
bumpfee-choose-reduce-output
branch
from
6547892
to
0b23871
Compare
achow101
force-pushed
the
bumpfee-choose-reduce-output
branch
from
0b23871
to
b3653bc
Compare
In order to correctly choose the change output when doing fee bumping in the GUI, we need to ask the user which output is change. We can make a guess using our ScriptIsChange heuristic, however the user may have chosen to have a custom change address or have otherwise labeled their change address which makes our change detection fail. By asking the user when fee bumping, we can avoid adding additional change outputs that are unnecessary.
achow101
force-pushed
the
bumpfee-choose-reduce-output
branch
from
b3653bc
to
c697167
Compare
There was a problem hiding this comment.
The first commit e1ea0ba fails to compile:
CXX qt/libbitcoinqt_a-walletmodel.o
qt/walletmodel.cpp: In member function ‘bool WalletModel::bumpFee(uint256, uint256&)’:
qt/walletmodel.cpp:486:41: error: no matching function for call to ‘interfaces::Wallet::createBumpTransaction(uint256&, wallet::CCoinControl&, std::vector<bilingual_str>&, CAmount&, CAmount&, CMutableTransaction&)’
486 | if (!m_wallet->createBumpTransaction(hash, coin_control, errors, old_fee, new_fee, mtx)) {
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from ./qt/walletmodel.h:16,
from qt/walletmodel.cpp:9:
./interfaces/wallet.h:166:18: note: candidate: ‘virtual bool interfaces::Wallet::createBumpTransaction(const uint256&, const wallet::CCoinControl&, std::vector<bilingual_str>&, CAmount&, CAmount&, CMutableTransaction&, std::optional<unsigned int>)’
166 | virtual bool createBumpTransaction(const uint256& txid,
| ^~~~~~~~~~~~~~~~~~~~~
./interfaces/wallet.h:166:18: note: candidate expects 7 arguments, 6 provided
make: *** [Makefile:13550: qt/libbitcoinqt_a-walletmodel.o] Error 1
| address_info = QString::fromStdString(address); | ||
| } | ||
| } | ||
| QString output_info = QString::number(i) + QString(": ") + BitcoinUnits::formatWithUnit(model->getOptionsModel()->getDisplayUnit(), txout.nValue) + tr(" to ") + address_info; |
There was a problem hiding this comment.
To make translators' life easier and to keep the context as large as possible, I suggest to use QString::arg. Also it will fix the lint-qt-translation.py warning.
|
🐙 This pull request conflicts with the target branch and needs rebase. |
Based on bitcoin/bitcoin#26467
Implements a GUI dialog for allowing the user to choose the output to reduce when bumping a transaction. This adds the functionality that was added to the RPC.