Page MenuHomePhabricator

Removed unused confirmation target dropdown from the UI
ClosedPublic

Authored by jasonbcox on Mar 4 2019, 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

Screenshot from 2019-03-04 10-35-58.png (598×834 px, 124 KB)

Diff Detail

Repository
rABC Bitcoin ABC
Branch
uiconftarget
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 5129
Build 8321: Bitcoin ABC Buildbot (legacy)
Build 8320: arc lint + arc unit

Event Timeline

src/qt/sendcoinsdialog.cpp
734 ↗(On Diff #7571)

This gets cleaned up in D2614.

Fabien requested changes to this revision.Mar 4 2019, 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.Mar 4 2019, 20:01

Cleanup nSmartFeeSliderPosition settings

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.

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