Page MenuHomePhabricator

Removed unused confirmation target dropdown from the UI
ClosedPublic

Authored by jasonbcox on Mon, Mar 4, 18:40.

Details

Summary

Confirmation targets do not affect tx fees, so having this on the UI is useless at best and confusing at worst. I'm also removing confirmation targets from txmempool::estimateFee's API in D2614, so there is no plan to use this in the future.

Test Plan

ninja check
test_runner.py
bitcoin-qt

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

jasonbcox created this revision.Mon, Mar 4, 18:40
Herald added a reviewer: Restricted Project. · View Herald TranscriptMon, Mar 4, 18:40
Herald added a subscriber: schancel. · View Herald Transcript
jasonbcox edited the test plan for this revision. (Show Details)Mon, Mar 4, 18:41
jasonbcox added inline comments.Mon, Mar 4, 18:42
src/qt/sendcoinsdialog.cpp
734 ↗(On Diff #7571)

This gets cleaned up in D2614.

Fabien requested changes to this revision.Mon, Mar 4, 20:01
Fabien added inline comments.
src/qt/sendcoinsdialog.cpp
258 ↗(On Diff #7571)

This block of code is no longer needed.
If remove() deletes the entry from the configuration file, this could be also applied to nConfTarget in order to clean up the file and avoid confusion.
Maybe something like this:

// nSmartFeeSliderPosition and nConfTarget are obsolete.
// Remove them from the settings if they exist
if (settings.value("nSmartFeeSliderPosition").toInt() != 0) {
    settings.remove("nSmartFeeSliderPosition");
}
if (settings.value("nConfTarget").toInt() != 0) {
    settings.remove("nConfTarget");
}
This revision now requires changes to proceed.Mon, Mar 4, 20:01
jasonbcox updated this revision to Diff 7576.Mon, Mar 4, 20:33

Cleanup nSmartFeeSliderPosition settings

jasonbcox added inline comments.Mon, Mar 4, 20:34
src/qt/sendcoinsdialog.cpp
258 ↗(On Diff #7571)

I agree. This should also be cleaned up in the next major version, so I've noted that in the comment.

Fabien accepted this revision.Mon, Mar 4, 21:21
This revision is now accepted and ready to land.Mon, Mar 4, 21:21
This revision was automatically updated to reflect the committed changes.