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] Improved copy for RBF checkbox and tooltip #11556

Merged
merged 1 commit into from Dec 5, 2017

Conversation

Sjors
Copy link
Member

@Sjors Sjors commented Oct 25, 2017

Fixes #11344 and replaces #11428.

Before:
before

After:
after

@Sjors
Copy link
Member Author

Sjors commented Oct 25, 2017

I didn't replace bitcoin_en.ts because apparently it's generated by a script. If that's the case though, shouldn't it be removed from the repo?

@fanquake fanquake added the GUI label Oct 25, 2017
@meshcollider
Copy link
Contributor

This makes the Replace-by-fee aspect of it a bit too subtle IMO. Increasing the transaction fee for confirmations is not the only benefit, the fee increase is just the mechanism by which the transaction can be replaced. But still perhaps an improvement, the everyday user might not need to be bothered by the details of RBF. utACK on the tooltip change at least

@Sjors
Copy link
Member Author

Sjors commented Oct 25, 2017

@meshcollider note that the counterpart to this feature #9697 also only refers to the fee bumping aspect of RBF.

@Sjors
Copy link
Member Author

Sjors commented Oct 25, 2017

The segwit.py tests failed in one of the Travis builds, presumably unrelated? Might be related to #10869?

@meshcollider
Copy link
Contributor

@Sjors

The segwit.py tests failed in one of the Travis builds, presumably unrelated? Might be related to #10869?

Indeed unrelated, I've restarted it for you.

@meshcollider note that the counterpart to this feature #9697 also only refers to the fee bumping aspect of RBF.

That is true, fair enough. I guess users of the GUI probably shouldn't be concerned, so utACK 115739a

@@ -1108,10 +1108,10 @@
<item>
<widget class="QCheckBox" name="optInRBF">
<property name="text">
<string>Request Replace-By-Fee</string>
<string>Allow Fee Increase</string>
Copy link
Contributor

Choose a reason for hiding this comment

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

not exactly sure how it works in English, but shouldn't "fee increase" be lowercased? Capitalizing made sense for "Replace-By-Fee" since it is more of a name, but now we're using generic nouns

Copy link
Member Author

Choose a reason for hiding this comment

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

The UI uses a mix of "Upper lower case" and "All Upper Case". For example "Custom change address", "Subtract fee from amount" vs. "Pay To", "Coin Control Features". In addition some sentences end with a period, others don't.

I'm happy to change it either way, but it might be better to have a single pull request to implement (and document) a consistent style across the wallet.

Copy link
Member

Choose a reason for hiding this comment

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

👍 lower case.

</property>
<property name="toolTip">
<string>Indicates that the sender may wish to replace this transaction with a new one paying higher fees (prior to being confirmed).</string>
<string>If the transaction takes a long time to confirm, this allows you to increase the fee later ("Replace-By-Fee", BIP 125). The recommended fee will be lower.</string>
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe shorten it to "If the transaction takes too long to confirm" ?

Copy link
Member

Choose a reason for hiding this comment

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

This is one of the possible use cases. How about cancelling? And there is also the confirmations detail that concerns the recipient. There are too many details in the BIP to make this copy simple.

Copy link
Member

Choose a reason for hiding this comment

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

I'm more fond of the previous tooltip as it doesn't mention confirmation time. Maybe just add BIP 125 reference?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm more fond of the previous tooltip as it doesn't mention confirmation time

Agree, and would at least shift the emphasis away if this needs to be kept, changing from "If the transaction takes a long time to confirm, this allows you to increase the fee later" to "This allows you to increase the fee later if the transaction takes a long time to confirm."

It might also be clearer to change "The recommended fee will be lower" to ""This will cause the recommended fee to be lower."

@Sjors
Copy link
Member Author

Sjors commented Oct 25, 2017

@promag cancelling a transaction sounds like a very useful feature. My suggestion though would be to introduce a Cancel button in a future PR, and then modify the copy of this checkbox. If we change it to "Allow Fee Increase, Cancellation and More" people will expect those features to exist in the wallet.

@flack
Copy link
Contributor

flack commented Oct 25, 2017

Another option would be to have the label read "Mark as replaceable" (Electrum uses similar wording I think). This would be more future-proof for a potential cancellation feature, because then you'd only have to adapt the tooltip, and it's still an improvement over "Request Replace-By-Fee"

@promag
Copy link
Member

promag commented Oct 25, 2017

Sometimes I think this option shouldn't be in the fee section. From the user point of view, either make the transaction final/locked or allow it to be replaced/bumped. Something like:

  • Lock transaction

@flack
Copy link
Contributor

flack commented Oct 25, 2017

@promag that is also a nice idea, but I would prefer a wording like "mark transaction as final" (or immutable or something to that effect). For not-so-technical users, "lock" has a security implication (like those little green locks you see on https pages, or lock symbols that indicate protected files), and I don't think we'd want users to think rbf transactions are somehow less secure

@promag
Copy link
Member

promag commented Oct 25, 2017

@flack right, my main point is to not mention fee as it can confuse the user as in what's the deal with fee and replace transaction.... For the curious/technical user the BIP reference can be enough?

Edit: for sure out of scope!

"Allow Fee Increase" is also out of context, that is not the purpose of the option, as in why would I want to be able to increase the fee?.

As it is, NACK.

@flack
Copy link
Contributor

flack commented Oct 25, 2017

@promag like I said, I like your suggestion to move the checkbox somewhere else, esp. when more rbf functionality gets added to the GUI. But (as mentioned above), the only thing the GUI currently lets you do with the rbf checkbox is to bump the transaction fee later on. So the idea of this PR is to help the user make the connection between the rbf checkbox and the bumpfee command.

If it's likely that more rbf functionality gets added to the GUI in the forseeable future, I'd say a relatively generic label (like "mark as replaceable", "mark as final") would be best, but for now I'd mention fees either way, since it's the only visible effect the checkbox currently has (i.e enables bumpfee, and gives you a lower estimate)

@Sjors
Copy link
Member Author

Sjors commented Oct 25, 2017

@petertodd any thoughts on this?

@promag
Copy link
Member

promag commented Oct 25, 2017

@flack honestly I wouldn't change the UI if there is consensus in adding such RBF functionality in an foreseeable future.

@Sjors
Copy link
Member Author

Sjors commented Oct 26, 2017

An alternative pull request could be one where we turn on RBF by default. In that case there's no need to explain what it's for or persuade users why they should tick this box. That would allow moving the button elsewhere in the send UI and just calling it "Disable Replace-By-Fee". If such a PR would be uncontentious, it would have my preference over this one, but I'm not sufficiently aware of the downsides.

@jonasschnelli
Copy link
Contributor

Concept ACK.
The new text is better understandable, though, it's not as prices as the current text (okay to me).
"Allow fee increase" does not include that one could allow to add an output... but IMO neglect able.

I would also prefer @promag approach of flipping the default (PRs welcome).

Not sure if we should flip the bitcoind -walletrbf or if we should set -walletrbf=1 in GUI mode.

@promag
Copy link
Member

promag commented Oct 26, 2017

@jonasschnelli I wasn't suggesting to change the defaults. I was suggesting to think from the user perspective. I believe the use case is to choose whether the transaction should be final or not. As such that option shouldn't be in the "Transaction Fee" section.

Anyway, RBF enabled by default sounds good to me too.

@Sjors
Copy link
Member Author

Sjors commented Oct 27, 2017

@promag I don't think unexperienced users think that way. They already assume a transaction is final, because that's what they're used to from the legacy fiat world. Only when they send a transaction and find out it's stuck, they start looking for ways to fix that acute problem.

The way I see it (subjective of course) is that bumping fees is a feature, while the fact that this feature uses RBF is an implementation detail. If in the future we discover a different way to bump fees without RBF, the button could stay the same.

I'm trying to understand your NACK though (as opposed to just a preference). Do you believe something would break? Are you worried that users won't tick the box if it doesn't show the full range of potential uses? Or that it's unmaintainable (to me it seems trivial to change this again later).

It sounds like it's worth trying to flip the default first; if that PR doesn't raise objections then we can just close this one. If it does, we can continue the discussion.

Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

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

utACK 115739acbe477e52716382e79732e38d34890d4c. Code change seems fine, and I think this is an improvement over the status quo, though also I agree with the concerns above. FWIW, I like the "Mark as replaceable" suggestion, and would also suggest "Allow fee adjustment" or "Allow increasing fees" as alternatives to "Allow Fee Increase" because "Fee Increase" sounds to me like something that could happen outside of your control.

</property>
<property name="toolTip">
<string>Indicates that the sender may wish to replace this transaction with a new one paying higher fees (prior to being confirmed).</string>
<string>If the transaction takes a long time to confirm, this allows you to increase the fee later ("Replace-By-Fee", BIP 125). The recommended fee will be lower.</string>
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm more fond of the previous tooltip as it doesn't mention confirmation time

Agree, and would at least shift the emphasis away if this needs to be kept, changing from "If the transaction takes a long time to confirm, this allows you to increase the fee later" to "This allows you to increase the fee later if the transaction takes a long time to confirm."

It might also be clearer to change "The recommended fee will be lower" to ""This will cause the recommended fee to be lower."

@ryanofsky
Copy link
Contributor

Whatever text you decide to use for the checkbox, it would probably be good to repeat it in the confirmation dialog here, so it is clear that they are referring to the same option:

questionString.append(tr("This transaction signals replaceability (optin-RBF)."));

@Sjors
Copy link
Member Author

Sjors commented Nov 4, 2017

Rebased and tweaked the text based on feedback.

I also removed the confirmation dialog @ryanofsky mentioned. It doesn't make sense to me to show a warning, if we don't explain what the risk is. Alternatively, the dialog could explain that some merchants may not like RBF. In that case, that should also go in the tooltip.

schermafbeelding 2017-11-04 om 12 48 58

I should also have a PR that enables RBF by default soon.

@Sjors
Copy link
Member Author

Sjors commented Nov 4, 2017

Oh wait, I thought that was a separate "are you sure?" kind of confirmation dialog. But it's just a line in the larger confirmation window. I'll put it back with some improvement.

@Sjors
Copy link
Member Author

Sjors commented Nov 4, 2017

Screenshot of updated confirmation screen:
confirmation

@flack
Copy link
Contributor

flack commented Nov 4, 2017

maybe out of scope for this PR, but it might make sense to move the rbf line in the confirmation dialog next to where the fee is displayed (and then show it in smaller print, like the mBTC line)

@Sjors
Copy link
Member Author

Sjors commented Nov 4, 2017

PR to turn RBF on by default: #11605

Opt-in RBF checkbox uses less technical jargon and emphasises
the fee bump functionality (at the expense of not mentioning
other uses of RBF).

The transaction confirmation screen uses copy consistent with this.
@Sjors
Copy link
Member Author

Sjors commented Nov 18, 2017

Rebased. Not sure why Travis is unhappy.

#11605 is causing even more discussion than this PR. Although I haven't given up on it, it might take a while. In the mean time I suggest we consider merging this one.

@maflcko maflcko added the Docs label Nov 27, 2017
@petertodd
Copy link
Contributor

utACK

@TheBlueMatt
Copy link
Contributor

utACK db0b737

@jonasschnelli
Copy link
Contributor

ACK db0b737

@jonasschnelli jonasschnelli merged commit db0b737 into bitcoin:master Dec 5, 2017
jonasschnelli added a commit that referenced this pull request Dec 5, 2017
db0b737 [Qt] Improved copy: RBF checkbox, tooltip and confirmation screen (Sjors Provoost)

Pull request description:

  Fixes #11344 and replaces #11428.

  **Before**:
  <img width="588" alt="before" src="https://user-images.githubusercontent.com/10217/31984211-3299e81a-b993-11e7-94e9-bf63d2fed4bd.png">

  **After**:
  <img width="578" alt="after" src="https://user-images.githubusercontent.com/10217/31984404-11f839da-b994-11e7-86ad-4c17a7d44b86.png">

Tree-SHA512: 04876b2f2eab53c8d4fd4279e8384fd4869af7e15de7648b2689092f800b6ae9c890c01c26c2f7deffe79a1d70c6440d702cbe420e44fe3ded25c5b83d44ecfa
@Sjors Sjors deleted the rbf-ui-text branch December 5, 2017 08:15
laanwj added a commit that referenced this pull request Dec 22, 2017
5cbbbd7 [Wallet] Use RBF by default in QT only (Sjors Provoost)

Pull request description:

  ~If there are no objections, this would supersede #11556.~

  Enabling RBF by default avoids the need to explain all possible use cases of RBF.

  This PR does not change the default RPC wallet behavior, as this could break implementations that depend on it and it's not clear what happens when automated services suddenly switch on RBF on a large scale.

  After trying various approaches, we settled on just having QT ignore `-walletrbf`.

  Send screen:
  <img width="388" alt="send" src="https://user-images.githubusercontent.com/10217/34251097-329c8dee-e63f-11e7-9e14-d7f55d2b52cc.png">

  Confirmation screen by default (with RBF):
  <img width="429" alt="rbf yes" src="https://user-images.githubusercontent.com/10217/32442799-f50d54aa-c2fc-11e7-9392-96339d0f1f74.png">

  Confirmation screen without RBF:
  <img width="431" alt="rf no" src="https://user-images.githubusercontent.com/10217/32442793-ef30bc34-c2fc-11e7-8ca2-e86a97175278.png">

Tree-SHA512: 53efb5d277144478143e69dcae8112c1b9c2beb981fdd0fe778592e5f7d5bf838f73d48052ead874586a75b944e8af469b25e5f376c135cf48cc3598e77f5891
@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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Better text for sending transaction option "Request Replace-By-Fee"
10 participants