Skip to content

Conversation

@GreatSock
Copy link
Contributor

@GreatSock GreatSock 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 2 commits May 19, 2018 14:15
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.
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.
Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK. Will test.

}

if (valid) {
if (val < min_amount) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@GreatSock
Copy link
Contributor Author

GreatSock 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
Copy link
Contributor

Concept ACK

@promag
Copy link
Contributor

promag commented Jun 1, 2018

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

Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


bool allow_empty;
CAmount min_amount;
CAmount max_amount;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@DrahtBot
Copy link
Contributor

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
@maflcko maflcko added this to the 0.18.0 milestone Jul 29, 2018
@maflcko
Copy link
Member

maflcko commented Jul 29, 2018

@GreatSock Are you still working on this?

@DrahtBot
Copy link
Contributor

Needs rebase

@Sjors
Copy link
Member

Sjors 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
Copy link
Member

Sjors 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
Copy link
Member

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
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
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants