Skip to content
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

[qt] Removed "Pay only the required fee" checkbox #13280

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
8 participants
@GreatSock
Copy link
Contributor

commented May 19, 2018

This removes the "Pay only the required fee" checkbox from the custom transaction fee section in the SendCoinsDialog. Instead, a minimum value will be enforced on the custom fee input box.

Before, a user could enter a fee below the minimum required fee but upon pressing send the resulting transaction would still pay the minimum fee. Now, if a user enters a value below the minimum fee the custom fee input box will be set to the minimum fee.

Where the checkbox used to be, there is now a more general warning about low fees. The tooltip is still the same as before.

image

GreatSock added some commits May 19, 2018

[qt] AmountField min/max value, allow empty option
This adds functions for specifing a min/max value for a BitcoinAmountField. These options only affect user input, so it's still possible to use setValue to set values outside of the min/max range. The existing value will not be changed when calling these functions even if it's out of range. The min/max range will be reinforced when the field loses focus.
This also adds an allow empty function which specifies if the field is allowed to be left empty by the user. If set to false the field will be set to the minimum allowed value if it's empty when focus is lost.
[qt] Removed pay only required fee checkbox
Removes the pay only required fee checkbox from the custom transaction fee section in the SendCoinsDialog. It's replaced by a label giving a more general warning about low fees.
The custom fee input box now has a minimum value equal to the minimum required fee. Before a value below the minimum fee could be entered which was confusing since the minimum fee would still be paid even though a lower amount was entered.
@promag
Copy link
Member

left a comment

Concept ACK. Will test.

}

if (valid) {
if (val < min_amount) {

This comment has been minimized.

void stepBy(int steps)
{
bool valid = false;
CAmount val = value(&valid);
val = val + steps * singleStep;
val = qMin(qMax(val, CAmount(0)), BitcoinUnits::maxMoney());
val = qMin(qMax(val, CAmount(min_amount)), max_amount);

This comment has been minimized.

@GreatSock

This comment has been minimized.

Copy link
Contributor Author

commented May 19, 2018

Any idea why the Travis build failed after changing to qBound?

Edit: Nevermind! Seems to have passed now

@laanwj laanwj added the GUI label May 19, 2018

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented May 25, 2018

Concept ACK

@promag

This comment has been minimized.

Copy link
Member

commented Jun 1, 2018

It would be nice to see a previous screenshot. I forgot to test this 🙄

@MarcoFalke
Copy link
Member

left a comment

utACK. Please adjust the code to the developer notes (see two comments)

Also, might want to squash the third commit into one of the earlier ones.

Finally, add empty new lines after the git commit subject line. Otherwise, the commit subject can not be parsed properly.

singleStep(100000), // satoshis
allow_empty(true),
min_amount(0),
max_amount(BitcoinUnits::maxMoney())

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Jul 22, 2018

Member

Please initialize non-static class members directly where they are declared.

@@ -129,6 +156,10 @@ class AmountSpinBox: public QAbstractSpinBox
CAmount singleStep;
mutable QSize cachedMinimumSizeHint;

bool allow_empty;
CAmount min_amount;
CAmount max_amount;

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Jul 22, 2018

Member

Please prefix class members with m_, according to the developer notes.

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Jul 22, 2018

The last travis run for this pull request was 64 days ago and is thus outdated. To trigger a fresh travis build, this pull request should be closed and re-opened.

@DrahtBot DrahtBot closed this Jul 22, 2018

@DrahtBot DrahtBot reopened this Jul 22, 2018

@MarcoFalke MarcoFalke added this to the 0.18.0 milestone Jul 29, 2018

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Jul 29, 2018

@GreatSock Are you still working on this?

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2018

Needs rebase
@Sjors

This comment has been minimized.

Copy link
Member

commented Sep 7, 2018

Concept ACK, less UI clutter is great, and it gets rid of a duplicate tooltip.

Also thanks for switching to qBound.

To clarify the minimum fee a bit, as well as make way for a potential future decrease, maybe change the tooltip from:

Paying only the minimum fee is just fine as long as there is less transaction volume than space in the blocks. But be aware that this can end up in a never confirming transaction once there is more demand for bitcoin transactions than the network can process.

To:

When there is less transaction volume than space in the blocks, miners as well as relaying nodes may enforce a minimum fee. Paying only this minimum fee is just fine, but be aware that this can result in a never confirming transaction once there is more demand for bitcoin transactions than the network can process.

Removing the checkbox breaks horizontal alignment. The text needs be aligned with "Confirmation time target":

schermafbeelding 2018-09-07 om 21 58 00

@GreatSock if you don't have time for this in the next month or so, I can take it over.

@Sjors

This comment has been minimized.

Copy link
Member

commented Oct 30, 2018

@hebasto said on IRC:

provoostenator: hi! have you took #13280 over or it is still up for grabs?

Not yet, feel free to and I can review it.

@fanquake

This comment has been minimized.

Copy link
Member

commented Oct 30, 2018

I'll close this then. hebasto can cherry-pick and open a new PR when ready.

@fanquake fanquake closed this Oct 30, 2018

jonasschnelli added a commit that referenced this pull request Nov 13, 2018

Merge #14608: qt: Remove the "Pay only required fee..." checkbox
a16f44c qt: Remove "Pay only required fee" checkbox (Hennadii Stepanov)
8711cc0 qt: Improve BitcoinAmountField class (Hennadii Stepanov)

Pull request description:

  Ref #13280
  This PR removes the "Pay only the required fee..." checkbox from the custom transaction fee section in the "Send" tab. Instead, a minimum value will be enforced on the custom fee input box.

  All comments from #13280 are addressed.

  Before:
  ![screenshot from 2018-10-30 16-42-18](https://user-images.githubusercontent.com/32963518/47726622-866d8e80-dc63-11e8-8670-3f97ff0fa5da.png)

  After:
  ![screenshot from 2018-10-30 16-40-37](https://user-images.githubusercontent.com/32963518/47726633-8f5e6000-dc63-11e8-82cf-5b9ff4aae91d.png)

  cc: @promag @MarcoFalke @Sjors

Tree-SHA512: 073577d38d8353b10e8f36fb52e3c6e81dd45d25d84df9b9e4f78f452ff0bdbff3e225bdd6122b5a03839ffdcc2a2a08175f81c2541cf2d12918536abbfa3fd1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.