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

gui: Implement GUI version of consolidateunspent (coin control part) #2111

Merged

Conversation

jamescowens
Copy link
Member

@jamescowens jamescowens commented Apr 22, 2021

This PR extends coin control to

  1. Change the (un)select all button to a true select all/select none filp-flop.
  2. Implement an input filter function to allow the selection of inputs either less than or equal to or greater than or equal to a provided input value, with an automatically imposed limit of 200 inputs (to prevent transaction failures).
  3. Implementation of a "Consolidate" button which (re)filters the inputs and then presents a pick list to select the destination address for the consolidation, and then automatically fills in the send transaction for convenience.

What is left is the implementation of a wizard for the consolidate unspent which overlays on top of this machinery. I would like to do that in a separate PR.

Closes #1984.

@jamescowens
Copy link
Member Author

Here are some screenshots of a testnet node using this PR's features...

Start with the send tab (coin control turned on).
image

Push the "Inputs..." button.
image

Select the desired inputs to be subject to consolidation. Note that you can just select one of the first level nodes of the tree, which corresponds to the inputs on a particular address. (Cf. the single address mode of the rpc command.) You can also use the "(un)select all" button to select all of the inputs - this is similar to the sweep from all addresses boolean in the rpc command.
image

Notice that there are 1497 inputs selected. Next fill in the less than x GRC field to direct the dialog to select a subset of the UTXO's already selected that are equal to or less than the provided value (in this case 1100 GRC).
image

Push the "Consolidate Outputs" button.
image

The modal consolidate outputs dialog comes up and it provides a table for you to select the destination address for the consolidation. If over 200 inputs met the selection criteria, they will be ordered in increasing value and only the first 200 selected, and then a warning will appear letting you know that the selection scope has been limited to 200 to prevent transaction failure due to too many inputs.

After you select the address and press "ok". You return to the coin control dialog.
image

You notice that the number of outputs has been limited to 200, and expanding one of the branches of the tree, you can see that the actual selection value stopped at about 900 instead of 1100 because the number of inputs limit was reached.

Another branch... same thing...
image

Finally, after pressing "ok" on the coin control dialog box, you are returned to the send tab. Notice that the destination address, label, and amount are already pre-filled in along with the same address in the custom change address (in case there is a slight amount of change due to the uncertainty in the fee calculation).
image

The consolidation transaction is ready to send by pressing the "send" button.

@jamescowens
Copy link
Member Author

jamescowens commented Apr 22, 2021

Note that this integrated with coin control approach takes longer to do than the rpc command, but it offers more flexibility, which is appropriate for the GUI. It also (not shown above) allows things that are not supported by the current rpc consolidateunspent function, such as fine tuning the inputs selected on the coin control after the consolidate selection filter is applied but before sending, or selecting the inputs for consolidation from one address, but using a destination address that is different. (That is not currently supported by the rpc command.)

I know this needs some polishing, but it is good enough to review I think.

@presciencia
Copy link

It looks like better for the wizard. I will need to see this instruction each time I use this 😄

@jamescowens
Copy link
Member Author

It is not as hard as you think once you try it. And for you to use consolidateunspent, you have to know what a UTXO is, which means you should also be familiar with coin control and the inputs button. We can turn the Consolidate Outputs into a bonafide wizard if we want. Maybe we should also just say Consolidate, because people get confused by the outputs and inputs thing too.

@jamescowens jamescowens force-pushed the ui_consolidate_unspent branch 3 times, most recently from 9c1caba to 8f88de4 Compare April 24, 2021 00:07
@jamescowens jamescowens marked this pull request as ready for review April 24, 2021 00:14
@cyrossignol
Copy link
Member

cyrossignol commented Apr 24, 2021

I think it could be really effective in a wizard-like flow. It feels a bit unnatural to start this process from within the coin picker because the UI controls will have a dual meaning. Though it doesn't seem hard to figure out from my perspective, it's not immediately obvious from the dialog how to use the feature and that the outputs need to be selected first.

The folks who may benefit most from this tool are probably not experts with UTXOs and coin control. These are concepts that some struggle to grasp, so a targeted UI version of the consolidator could have a much greater impact on the people without the experience—those who know how to do coin control can already create these transactions using the existing GUI and RPC tools without trouble (I usually just check-off an address in the selector and address book, copy/paste amount, and press send). The widgets are already done here—it doesn't seem hard to reorganize them into a wizard.

Maybe we should also just say Consolidate, because people get confused by the outputs and inputs thing too.

I like that better. Maybe "Filter" instead?

I wonder... could we reuse the amount filter for regular coin selection as well? It seems like a nice feature that we'd get for free along with this.

@jamescowens
Copy link
Member Author

The wizard could simply use this machinery (underlying), much like I am actually driving the underlying senddialog. And filter is a better word. The only things specific about the consolidation filter are the limits on inputs and the selection of a destination address. Seems like a good idea to have it as a generalized feature for regular coin selection.

@jamescowens
Copy link
Member Author

jamescowens commented Apr 24, 2021

I still think we have no business presenting the wizard for UTXO consolidation unless advanced coin control is enabled. For you to ask the question as to whether you need to consolidate means that you have to understand what an input/output is. If you don't then the concept of consolidate UTXO's doesn't mean anything - unless we want the wizard to truly be a tutorial.

BTW this is, as I feared when starting it, turning into a larger project than I hoped, because we will descend into arguments on how it should look and function.

I do NOT like the idea of simply "wizardizing" the parameters in the RPC function, because people that don't know those parameters are going to be clueless, even in a wizard, on what to put in there.

@RoboticMind
Copy link
Contributor

Partly due to the existing clutter in the coin control interface, I think some of the layout may be a bit hard to follow. I had a harder time seeing how things worked and I was aware of what UTXOs are and all.

Maybe something like one of those below would be better? Ignore the weird spacing/weird text since I made this quickly

image

or like

image

or something else.

Have a couple other suggestions like greying out the consolidate button if nothing is selected and making the (un)select all just switch between select all and unselect all since it took me a minute to realize that it meant that it would either select or unselect depending on whether you had selecting things or not

No idea on the amount of work required for this, so let me know if this is too much

@jamescowens
Copy link
Member Author

I have separated out the filter into a separate function from the GUI button to facilitate more generic use.

@vincehusky
Copy link

BTW this is, as I feared when starting it, turning into a larger project than I hoped, because we will descend into arguments on how it should look and function.

Look and function...if this is not the whole essence of UI design, what else is there?

Apologies. I could not resist being a smart---; no disrespect intended. In fact I am thankful for your efforts. As I am a UI designer, I have started watching the Gridcoin UI updates. Liking the progress!

At risk of being a nuisance for you all, just want to add to the discusion since I am confused by the presented functionality. I do not frequently use the coin control so perhaps I just don't "get it." I am at a point in my knowledge where I get utxos and consolidating them for staking, but it's not carrying over to what it means above. Some others said "not obvious" so the question could be what can make it obvious. A good UI can make complex ideas simple and clear. I think I fall into the group of users who would benefit from some coddling.

I am no good for code but I wish I could help contribute to this great project. I hope this is some food for thought. If there are UI related tasks that I can assist with please give a shout.

@jamescowens
Copy link
Member Author

Ok. @cyrossignol and folks. Take a look at this in a partially completed state. I have not done the consolidate button over as a wizard yet, so ignore that one, but I changed the behavior and wording on the select all/select none button to be a true toggle. I have also rearranged the filter to be more logical. It is now generalized, so you can select the outputs either less than or equal to or greater than or equal to the filled in GRC value.
image

@jamescowens jamescowens force-pushed the ui_consolidate_unspent branch 2 times, most recently from f6d137b to 734c19d Compare April 25, 2021 04:01
@cyrossignol
Copy link
Member

@jamescowens Hehe, that's one big button 🙂

I think the filter looks great. How does the "greater than or equal to" button work? Does it cycle through the options with each click? We may want to use the >=, etc., symbols instead of text to avoid the work for translators.

@jamescowens
Copy link
Member Author

Haha. Ignore the appearance of the buttons. I will try and put spacers in there to make it more reasonable. It is a flip flop. It flips from less than (the default) to greater than and back as you repeatedly press it.

@jamescowens
Copy link
Member Author

I was thinking about using those symbols too, but will everyone know what <= or >= mean. (Using the same thought that this area has been too wonky....)

@cyrossignol
Copy link
Member

Yep. I like it.

@cyrossignol
Copy link
Member

I was thinking about using those symbols too, but will everyone know what <= or >= mean.

Perhaps some won't. I think the symbols are common enough for most people to grasp, though. At least for the time being, these should cover more ground until translations catch up.

@jamescowens jamescowens force-pushed the ui_consolidate_unspent branch 2 times, most recently from 4ccadb4 to ade13d1 Compare April 25, 2021 18:15
@jamescowens
Copy link
Member Author

jamescowens commented Apr 25, 2021

Ok. I think this is now pretty straightforward and logical. This wraps up the coin control part. I will do a wizard next.

Here is the basic flow. You should actually compile this and use it to try it out for yourself. There are good explanatory tooltips for the buttons now.

Start with coin control...
image

Select desired coins... (in this case I pressed "Select all").
image

Filter (if desired). In this case I put in <= 1000 GRC. You don't HAVE to filter separately, because the consolidate will RE filter, but you can use the filter without consolidate if desired for other purposes.
image

Here is the consolidate destination address pick list. Note it does NOT show a 200 input limit warning, because the filter already limited the number of inputs to 200.
image

If you had gone directly from the select all to the consolidate, or modified the selections after the filter to add more inputs manually than 200. you would get this...
image

After you press ok to acknowledge coin control, the send tab shows the ready to send transaction with everything already filled in. All you have to do is press the send button.
image

@jamescowens
Copy link
Member Author

So I think this is really good for the coin control version. Next up is a wizard, which I intend to reuse all of this machinery under the covers...

@jamescowens jamescowens changed the title gui: Implement GUI version of consolidateunspent gui: Implement GUI version of consolidateunspent (coin control part) Apr 25, 2021
@jamescowens
Copy link
Member Author

@RoboticMind ... something like
image

This now shows a green "ready to send" indicator with a tooltip.

@jamescowens jamescowens force-pushed the ui_consolidate_unspent branch 2 times, most recently from e02b7f5 to 04afbf6 Compare April 26, 2021 04:00
@RoboticMind
Copy link
Contributor

Might make the text a little brighter on the dark theme and maybe use the "ready to consolidate" wording instead, but I think something like that would work. Overall I think it's looking good

@jamescowens
Copy link
Member Author

I have used a different green on the light vs dark stylesheet...
image
image

This should be ready to go now.

Copy link
Contributor

@RoboticMind RoboticMind left a comment

Choose a reason for hiding this comment

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

Think it looks good now

Copy link
Member

@cyrossignol cyrossignol left a comment

Choose a reason for hiding this comment

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

Could you please remind me about why we chose 200 outputs for the consolidation limit? Can't remember the discussion... been too long...

src/qt/coincontroldialog.cpp Outdated Show resolved Hide resolved
src/qt/coincontroldialog.cpp Outdated Show resolved Hide resolved
src/qt/coincontroldialog.cpp Outdated Show resolved Hide resolved
src/qt/coincontroldialog.cpp Show resolved Hide resolved
src/qt/coincontroldialog.cpp Outdated Show resolved Hide resolved
src/qt/coincontroldialog.cpp Outdated Show resolved Hide resolved
src/qt/coincontroldialog.cpp Outdated Show resolved Hide resolved
src/qt/coincontroldialog.cpp Outdated Show resolved Hide resolved
src/qt/coincontroldialog.h Outdated Show resolved Hide resolved
src/qt/forms/coincontroldialog.ui Outdated Show resolved Hide resolved
@jamescowens jamescowens force-pushed the ui_consolidate_unspent branch 3 times, most recently from 0b33c22 to 9611c87 Compare April 30, 2021 00:53
1. Changes the (un)select all button to a true select all/select none filp-flop.

2. Implements an input filter function to allow the selection of inputs either less than
or equal to or greater than or equal to a provided input value. This can be used for more
than just consolidations.

3. Implements a "Consolidate" button which limits the selected inputs to 600 to ensure
the consolidation transaction will be successful, presents a pick list to select the destination
address for the consolidation, and then automatically fills in the send transaction for
convenience.
Copy link
Member

@cyrossignol cyrossignol left a comment

Choose a reason for hiding this comment

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

Looks good.

@jamescowens jamescowens merged commit 2d8f583 into gridcoin-community:development Apr 30, 2021
@vincehusky
Copy link

👏

@jamescowens jamescowens deleted the ui_consolidate_unspent branch May 23, 2021 00:23
jamescowens added a commit to jamescowens/Gridcoin-Research that referenced this pull request Aug 1, 2021
Added
 - util, rpc. gui: Changes for snapshotdownload and add feature sync from zero gridcoin-community#2093 (@iFoggz)
 - gui: Implement GUI version of consolidateunspent (coin control part) gridcoin-community#2111 (@jamescowens)
 - gui: Implement consolidateunspent wizard gridcoin-community#2125 (@jamescowens)
 - qt: Add antialiasing to traffic graph widget gridcoin-community#2150 (@barton2526)
 - util: Port of ArgsManager and a significant subset of src/util gridcoin-community#2146 (@jamescowens)
 - doc: add issue templates for bug reports and feature requests gridcoin-community#2147 (@Pythonix)
 - gui, rpc: Implement dynamic stakesplitting control, settings changes via rpc, and dynamic changes to sidestaking via rpc gridcoin-community#2164 (@jamescowens)
 - rpc: Implement getblocksbatch gridcoin-community#2205 (@jamescowens)
 - voting, rpc, gui: Implement demand loading of historical poll by poll id and AVW calculation gridcoin-community#2210 (@jamescowens)
 - gui: Show GUI error dialog if command line parsing fails gridcoin-community#2218 (@jamescowens)
 - gui: Implement close confirmation. gridcoin-community#2216 (@denravonska)
 - build: Use -fstack-reuse=none gridcoin-community#2232 (@barton2526)

Changed
 - doc: Update build doc gridcoin-community#2078 (@iFoggz)
 - gui: Normalize button and input control appearance gridcoin-community#2096 (@cyrossignol)
 - consensus: Implement GetMinimumRequiredConnectionsForStaking gridcoin-community#2097 (@jamescowens)
 - refactor: move CTransaction to primitives gridcoin-community#2006 (@div72)
 - consensus, refactor, test: Merkle gridcoin-community#2094 (@div72)
 - gui: Update diagnostics gridcoin-community#2095 (@jamescowens)
 - gui: Refresh UI styles and sidebar/statusbar design gridcoin-community#2102 (@cyrossignol)
 - gui: Set standard base Qt style on Windows and macOS gridcoin-community#2114 (@cyrossignol)
 - build, refactor: bump to C++17 gridcoin-community#2113 (@div72)
 - util, rpc, gui: Implement GetMaxInputsForConsolidationTxn() gridcoin-community#2119 (@jamescowens)
 - gui: Refresh overview page design gridcoin-community#2117 (@cyrossignol)
 - depends: change boost mirror gridcoin-community#2122 (@div72)
 - refactor: small cleanup gridcoin-community#2123 (@div72)
 - build: Update depends Qt recipe to version 5.12.10 gridcoin-community#2129 (@cyrossignol)
 - build: Bump Codespell to 2.0.0 gridcoin-community#2135 (@barton2526)
 - gui: Refresh "send coins" page design gridcoin-community#2126 (@cyrossignol)
 - gui: Optimize locks to improve responsiveness gridcoin-community#2137 (@cyrossignol)
 - gui: Refresh "receive payment" page design gridcoin-community#2138 (@cyrossignol)
 - gui: Add empty placeholder to recent transactions list gridcoin-community#2140 (@cyrossignol)
 - gui: Refresh transaction history page design gridcoin-community#2143 (@cyrossignol)
 - gui: Refresh address book page design gridcoin-community#2145 (@cyrossignol)
 - doc: Update http to https where possible gridcoin-community#2148 (@barton2526)
 - depends: Update dependencies gridcoin-community#2153 (@barton2526)
 - depends: Bump python to 3.6 gridcoin-community#2159 (@barton2526)
 - test: Update cppcheck linter to c++17 gridcoin-community#2157 (@barton2526)
 - test: Drop Travis specific workarounds, Mention commit id in error, Fix typos, Update spellcheck ignore words gridcoin-community#2158 (@barton2526)
 - gui: Overhaul the voting UI gridcoin-community#2151 (@cyrossignol)
 - wallet: simplify nTimeSmart calculation gridcoin-community#2144 (@div72)
 - gui: Refresh checkbox and radio button styles gridcoin-community#2170 (@cyrossignol)
 - build: Bump libevent to 2.1.11 gridcoin-community#2172 (@barton2526)
 - build: Update native_mac_alias, Remove big sur patch file in qt recipe gridcoin-community#2173 (@barton2526)
 - docs: Misc Grammar gridcoin-community#2176 (@barton2526)
 - build: miniupnpc 2.2.2 gridcoin-community#2179 (@barton2526)
 - rpc: Refresh rainbymagnitude gridcoin-community#2163 (@jamescowens)
 - util: optimize HexStr gridcoin-community#2185 (@div72)
 - refactor: misc style changes gridcoin-community#2177 (@div72)
 - rpc: consolidatemsunspent changes. gridcoin-community#2136 (@iFoggz)
 - refactor: Replace "GlobalStatus" state management gridcoin-community#2183 (@cyrossignol)
 - rpc, util: Remove use of ArgsManager::NETWORK_ONLY for now gridcoin-community#2190 (@jamescowens)
 - doc: Replace hidden service with onion service, Capitalize "Tor" gridcoin-community#2193 (@barton2526)
 - gui: Update Qt Linguist localization files gridcoin-community#2192 (@cyrossignol)
 - script: Shell script cleanups gridcoin-community#2195 (@barton2526)
 - build: set minimum required Boost to 1.58.0 gridcoin-community#2194 (@barton2526)
 - build, util: Prevent execution for Windows versions less than Windows 7 gridcoin-community#2203 (@jamescowens)
 - build: Tweak NSIS Windows installer gridcoin-community#2204 (@jamescowens)
 - build: Add bison in depends gridcoin-community#2206 (@iFoggz)
 - build: macOS toolchain bump gridcoin-community#2207 (@div72)
 - doc: Update build-unix.md gridcoin-community#2212 (@springfielddatarecovery)
 - build: Bump minimum python version to 3.6, Remove python2 references gridcoin-community#2219 (@barton2526)
 - depends: Change openSSL source path to Github gridcoin-community#2237 (@barton2526)
 - doc: Fix typo in bug report template gridcoin-community#2243 (@jamescowens)
 - ci: fold depends output gridcoin-community#2244 (@div72)

Removed
 - wallet: remove dead hardcoded addnodes gridcoin-community#2116 (@sweede-se)
 - rpc: Remove readconfig gridcoin-community#2248 (@jamescowens)
 - rpc: Remove obsolete comparesnapshotaccrual RPC function gridcoin-community#2100 (@jamescowens)
 - rpc: Remove memorypool RPC Command gridcoin-community#2214 (@RoboticMind)
 - rpc: Remove deprecated RPC commands gridcoin-community#2101 (@jamescowens)
 - Remove CCT from README, add Discord gridcoin-community#2134 (@barton2526)
 - refactor: Remove obsolete pubsub method definitions gridcoin-community#2191 (@barton2526)
 - refactor: Remove msMiningErrorsIncluded & msMiningErrorsExcluded gridcoin-community#2215 (@RoboticMind)
 - qt: Remove obsolete topLevelWidget(), Remove obsolete QRegExpValidator gridcoin-community#2198 (@barton2526)
 - net: Drop support of the insecure miniUPnPc versions gridcoin-community#2178 (@barton2526)
 - log: remove deprecated db log category gridcoin-community#2201 (@barton2526)
 - doc: Remove CCT from README and release process docs gridcoin-community#2175 (@barton2526)
 - build: Remove travis references gridcoin-community#2156 (@barton2526)

Fixed
 - gui: Fix macOS and designer font sizes gridcoin-community#2098 (@cyrossignol)
 - gui: Have the TrafficGraphWidget respect the selected stylesheet. gridcoin-community#2107 (@jamescowens)
 - gui: Fix macOS display inconsistencies gridcoin-community#2106 (@cyrossignol)
 - gui: Fix RPC console auto-complete background color gridcoin-community#2108 (@cyrossignol)
 - gui: Avoid reloading redundant stylesheets gridcoin-community#2109 (@cyrossignol)
 - gui: Fix "no active beacon" status gridcoin-community#2110 (@cyrossignol)
 - gui: Fix dark theme link text color visibility gridcoin-community#2115 (@cyrossignol)
 - scraper, util, qt: Fix several deprecations and warnings gridcoin-community#2131 (@jamescowens)
 - gui: Fix duplicate time in GUIUtil::dateTimeStr() gridcoin-community#2139 (@cyrossignol)
 - gui: Fix debug console traffic graph legend colors gridcoin-community#2142 (@cyrossignol)
 - gui: Fix nomenclature gridcoin-community#2104 (@jamescowens)
 - doc: Fix Typos gridcoin-community#2149 (@barton2526)
 - doc: Fix "master" branch build status badge in readme gridcoin-community#2167 (@cyrossignol)
 - gui: Fix Inter font rendering on Windows with FreeType gridcoin-community#2169 (@cyrossignol)
 - gui: Fix assert on non-existent data directory and GUI datadir chooser corner case issues gridcoin-community#2174 (@jamescowens)
 - gui: Fix display artifact in poll loading indicator gridcoin-community#2180 (@cyrossignol)
 - rpc, logging: Minor fixes for sidestake logging gridcoin-community#2187 (@jamescowens)
 - gui: Fix fractional scaling for dialog sizes gridcoin-community#2189 (@cyrossignol)
 - doc: Random fixes gridcoin-community#2197 (@barton2526)
 - doc: getbalance should say GRC not "btc" gridcoin-community#2199 (@barton2526)
 - net: Add missing verification of IPv6 address in CNetAddr::GetIn6Addr¦ gridcoin-community#2200 (@barton2526)
 - doc: remove duplicate line from .gitignore gridcoin-community#2202 (@Pythonix)
 - util: Tweak exception handling in MilliTimer class to eliminate compiler warnings gridcoin-community#2233 (@jamescowens)
 - depends: patch missing include in qt gridcoin-community#2234 (@div72)
 - wallet, rpc: Check each input for IsMine() in GetAddressGroupings gridcoin-community#2242 (@jamescowens)
 - util, qt: Fix snapshot download gridcoin-community#2246 (@jamescowens)
 - gui: Fix Column Widths in RPC Console. Elide long strings in their center. Indent user agent. gridcoin-community#2241 (@barton2526)
 - qt: Fix crash during download snapshot on macOS gridcoin-community#2250 (@jamescowens)
 - qt: Don't allow to open the debug window during splashscreen & verification state gridcoin-community#2245 (@barton2526)
 - gui: Fix address book selected model record when editing gridcoin-community#2253 (@cyrossignol)
 - researcher: Check wallet status before beacon renewal gridcoin-community#2254 (@cyrossignol)
 - qt: Prevent pasting (no label) as label in consolidation transaction gridcoin-community#2255 (@jamescowens)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create a GUI for the consolidateunspent Command
5 participants