[Wallet] If nautomintdenom is set, prioritize its use #908
Conversation
Rock-N-Troll
changed the title
codeofalltrades
added
Tag: Automint
Tag: Waiting For Code Review
Rock-N-Troll
changed the title
|
|
Rock-N-Troll
changed the title
|
Generally we like to see multiple commits rebased down to one single commit (git rebase -i HEAD~6 and then change the ones that you want to combine to 'fixup'), but in this case I would be okay with 2 commits; one for the prioritization, and one for the disabling of the UI.
After you run a |
Rock-N-Troll
force-pushed
the
automintdenom_fix
branch
from
ac06b03 to
4075e18
Compare
CaveSpectre11
added
the
Tag: Needs Release Notes
src/qt/veil.cpp
Outdated
| // Preferences check | ||
| // If the UI settings have a different value than the server args then use the UI settings. | ||
| // Automint denom | ||
| // If nautomintdenom is set, use it | ||
| // Else use saved UI settings |
There was a problem hiding this comment.
This change is a behavior change (changing the preference from the UI setting to the veil.conf setting) so definitely needs a statement in the release notes; as those that have both will see a change in behavior.
src/qt/veil.cpp
Outdated
| // If the UI settings have a different value than the server args then use the UI settings. | ||
| // Automint denom | ||
| // If nautomintdenom is set, use it | ||
| // Else use saved UI settings | ||
| QSettings* settings = getSettings(); |
There was a problem hiding this comment.
Since settings isn't used anywhere else, this should be moved into the if not IsArgSet context, so it doesn't waste the cycles if it's not going to be used.
There was a problem hiding this comment.
Yes I was thinking the same thing but left it as is for simplicity's sake. Will update
| @@ -177,35 +187,35 @@ void SettingsMinting::mintzerocoins() | |||
| } | |||
|
|
|||
| void SettingsMinting::onCheck10Clicked(bool res) { | |||
| if(res && nPreferredDenom != 10){ | |||
| if(res && !gArgs.IsArgSet("-nautomintdenom") && nPreferredDenom != 10){ | |||
There was a problem hiding this comment.
This (and others) shouldn't be necessary since the radio buttons would be disabled if nautomintdenom is set. No sense wasting the cycles for code that wouldn't be reached if it argument was set.
Rock-N-Troll
force-pushed
the
automintdenom_fix
branch
from
5e7dbce to
3ad26d3
Compare
|
It seems like this is partially working. The disabled setting screen when nautomintdenom is set works. |
I think that is an existing issue, but let me see what I can do |
if nautomintdenom is set, do not allow UI update of nPreferredDenom settings minting screen GUI checkbox save fix
Rock-N-Troll
force-pushed
the
automintdenom_fix
branch
from
3ad26d3 to
313772e
Compare
|
The latest commit will also properly apply the saved checkbox settings whereas previously and existing code wasn't working. I also squashed to 1 commit while I was on the branch. |
codeofalltrades
added
Tag: Waiting For QA
nPreferredDenom == DEFAULT_AUTOMINT_DENOM is actually false because:
nPreferredDenom is 10
DEFAULT_AUTOMINT_DENOM is 1000
This code change just wraps the 2 nPreferredDenom inits based on if nautomindenom is set or not in the config.
The real existing issue is in veil.cpp which occurs after init.cpp and overwrites the logic in veil.cpp for nautomintdenom anyway when it pulls from QT getSettings()
Easiest way to test is to compare this code vs. existing
use a fresh wallet or re-build/remake from code and config file nautomintdenom value is not used on initial startup
Release Note
[GUI] Users that have set their desired zerocoin automint denomination via the GUI and in the veil.conf file will now find the configuration file setting is used over the GUI setting. If the veil configuration file has an automint denom setting, the GUI controls are now disabled and the user informed.