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] New Governance Graphical User Interface #2406

Merged
merged 59 commits into from
Nov 4, 2021

Conversation

furszy
Copy link

@furszy furszy commented May 31, 2021

Adding visual interface support for the governance system.

Work in conjunction with @Neoperol ☕ who made the wireframes, layouts organization and theme styles.

Implementing the following points:

  • New governance screen: laying out the network proposals (name, url, votes, status) and the budget distribution (allocated and available) with filtering and sorting capabilities.
  • New proposal creation wizard making the proposal generation fairly simple with automatic proposal broadcast support.
  • New proposal voting visual flow, MN controller wallets will be able to vote for proposals using pivx-qt, with multiple MN/s voters selection and change vote capabilities.
  • Styled for both themes, light and dark.

Demo videos:

1) Proposals view, budget distribution and create proposal wizard.

(Two videos due github's 10mb maximum size restriction)

create_proposals_step_1.mp4
create_proposal_step_2.mp4

2) Proposal voting flow:

proposal_voting_flow.mp4

3) Proposals grid view responsiveness.

proposal_grid_responsiveness.mp4

Important note:

This work is still in-progress, wording and visuals can change a bit moving on :), and there are some issues that i'm currently working on.
Plus, the backend tier two synchronization process requires improvements that I'm planning to do in another PR.. all of the data forced cleanups + re-synchronizations (budget proposals, votes and MNs) are notorious now that we have visual support for it.

To do:

  • X- Create Proposal, add autocomplete dropdown for the address field.
  • Add tooltip for proposal card status, disabled mn voter, forum URL copy.
  • Complete dark theme. missing checkbox disabled icon and fix for proposal card status border, mnselection list, among others (double check prototype).
  • Fix DAO nav icon location.
  • Add "sync not finished, proposal and budget data may be outdated".
  • Budget signal to trigger visual refresh (currently only refreshing in the screen's show event).
  • Lay out and connect proposals sort action.
  • Create proposal, validate name and url max length.
  • Continue testing..

@furszy furszy self-assigned this Jun 1, 2021
@furszy furszy added the GUI label Jun 1, 2021
@furszy furszy added this to the 6.0.0 milestone Jun 1, 2021
@furszy furszy force-pushed the 2021_GUI_governance branch 5 times, most recently from 92d40df to 1e1e808 Compare June 1, 2021 20:15
@furszy furszy force-pushed the 2021_GUI_governance branch 2 times, most recently from 4302a99 to fc81411 Compare June 14, 2021 18:01
@furszy furszy force-pushed the 2021_GUI_governance branch 2 times, most recently from cc607ce to a3e5946 Compare June 28, 2021 19:16
@furszy furszy force-pushed the 2021_GUI_governance branch 4 times, most recently from 4fa167d to b0f17dc Compare July 7, 2021 14:20
@furszy furszy changed the title [WIP][GUI] New Governance Graphical User Interface [GUI] New Governance Graphical User Interface Jul 12, 2021
@furszy furszy force-pushed the 2021_GUI_governance branch 3 times, most recently from d981abc to bf6adc6 Compare July 22, 2021 14:03
@furszy furszy force-pushed the 2021_GUI_governance branch 2 times, most recently from a88662e to d60b8dc Compare July 25, 2021 21:01
Copy link

@random-zebra random-zebra left a comment

Choose a reason for hiding this comment

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

I like this very much.
Awesome job 🍻

Left some first impressions going through the code, will do more accurate reviewing/testing later.

One general point of discussion would be to remove the dependence of GovernanceModel on WalletModel (ofc for the future, as no-wallet builds are broken atm).
The proposal-creation could be its own module, depending on GovernanceModel and WalletModel, which would be disabled when the wallet support is not compiled (and, for example,. the "Create Proposal" button would be hidden).

Ideally a node built without wallet would not be able to create new proposals (as it couldn't send the fee tx), but should still be able to inspect the existing budgets.

src/qt/pivx/governancemodel.cpp Outdated Show resolved Hide resolved
src/qt/pivx/governancemodel.h Outdated Show resolved Hide resolved
src/qt/pivx/governancewidget.cpp Outdated Show resolved Hide resolved
@@ -19,6 +19,7 @@ class OperationResult
OperationResult(bool _res) : m_res(_res) { }

std::string getError() const { return (m_error ? *m_error : ""); }
bool getRes() const { return m_res; }

Choose a reason for hiding this comment

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

(b6b638d)

Nit: As already discussed in #2453, we can remove getRes() and just explicitly cast to bool.

src/qt/pivx/governancemodel.cpp Outdated Show resolved Hide resolved
Comment on lines 384 to 386
pwallet->GetKey(mnKeyID, mnKey);
}
if (mnKey.IsValid()) {

Choose a reason for hiding this comment

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

(b6dbb02)

Better to check the return value of GetKey directly, rather than ignoring it and then checking if the key is valid.

Also, this loops over ALL masternodes in the list (if no filter is given) to see whther we have any of the voting keys... probably keeping the lock outside the loop (as it was) is better, performance-wise, than thousands of LOCK/UNLOCK operations for a single function call.

Copy link
Author

Choose a reason for hiding this comment

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

better to improve this in another focused PR and remove the for-loop entirely. The node/wallet can know and maintain an indicator and/or a cached list of the enabled DMNs for which it can vote for.

Choose a reason for hiding this comment

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

Sure, but in the meantime we can (and imo should) keep it as it was: LOCK outside of the loop, and return value of GetKey checked.

src/rpc/budget.cpp Outdated Show resolved Hide resolved
src/budget/budgetproposal.h Outdated Show resolved Hide resolved
src/qt/pivx/forms/createproposaldialog.ui Outdated Show resolved Hide resolved
src/qt/pivx/forms/createproposaldialog.ui Outdated Show resolved Hide resolved
@furszy
Copy link
Author

furszy commented Aug 1, 2021

great that you liked it zebra 😄, lot of work 🍺 .
Will be working on all the comments and rebasing this work 👍 .

@furszy furszy force-pushed the 2021_GUI_governance branch 3 times, most recently from 4dbad6a to deb8f3d Compare August 4, 2021 00:27
@ambassador000
Copy link

ambassador000 commented Aug 4, 2021

Some new feedback on this PR:

(1)
When typing Amount bigger than the maximum possible amount, box is Red, but it still allows user to proceed with the NEXT button:
issue1

issue2

However, when trying to press NEXT button afterwards, it will show an error that the amount is too high.
Suggestion: NEXT button should not be accepted in the first place if the amount entered in the box is too high.


(2)
ENTER button is not working when trying to go NEXT using the keyboard instead of the mouse click.


(3)
There is some visual inconsistency around the button in the last step of the proposal creation (another smaller box within the NEXT button box):
issue3


(4)
I think I saw a commit to fix this earlier, but cannot find it anymore. This is still being showed during the sync process, which would look ugly on mainnet especially when sync is fast.
issue4


(5)
It is better now because there is a limit in title length, but if you type proposal name such as "-/-0'90'00909''0--*-", it will not recognize some signs as a letters and again it will allow the too long proposal name to pass, which crashes the wallet again:
issue5

NOTE: Crash is bad enough so that it corrupts the wallet.dat file so users would lose funds if they don't have earlier backup of their wallet.dat file. Also, wallet cannot be started again without deleting the wallet.dat file or using the non-corrupted one:
issue6

Another NOTE: Unfortunately, I don't have MNs on testnet to be able to test voting on the proposals, but proposals were created and live for the next 6 cycles if someone wants to vote and give it a try.


(6)
There is something wrong with the NEXT and BACK buttons code, because after the Error shows up and you still click NEXT multiple times, it gets "stuck" and you BACK button is not accepted anymore (sometimes it does get accepted after clicking 5-10 times after few seconds), so user have to click X button in the top right corner and start with the proposal creation process all over again.

For example: If user has only 1 UTXO of 1000 PIVs for example, creates 1 proposal which deducts 50 PIV but makes the user's available balance at 0 (because the entire wallet's balance is in unconfirmed state), if user tries to create new proposal, he will get an Error that there is not enough funds or similar. However, after waiting the couple minutes (few block confirmations) and the amount gets available again, user will NOT be able to click the SEND button and proceed with the proposal creation although there are enough funds now, so he will need to press X and go with the proposal creation process all over again.

issue7


(7)
Take this bug report with a grain of salt, but I think that's the issue or bug as well. After I crashed the wallet.dat file in the issue 5 mentioned above, I deleted the wallet.dat file and everything in the wallets folder in the data directory (including database, .walletlock, etc.). After that, I started the PIVX Core again, and after it synced 100%, it shows the proposal that I created from the PREVIOUS corrupted wallet.dat file that got deleted and it is not used anymore! (fresh wallet.dat file was created after opening the PIVX Core after crash):
issue8


(8)
I think we discussed this few months back already, but just reporting again that 2 proposals with the same names, amounts and URLs are allowed to be created:
issue9


(9)
User is allowed to type the incorrect URL as long as "https://" or "http://" is mentioned, even if URL is entered with spaces and without dot (.) anywhere:
issue10

Will give it another testing round after few more days.

@ambassador000
Copy link

ambassador000 commented Aug 4, 2021

Same thing applies to URL length and special characters used:

issue11

issue12


If user has a DAO tab opened in the wallet while wallet is syncing after being turned on, listed proposals are being updated only if user is changing tabs, for example from DAO to CONTACTS and back to DAO. Although proposals are not something that changes too often, but maybe it would be cool if the DAO tab refreshes automatically every few minutes without needing to switch tabs, for those that are addicted to following the MNO yes and no votes for each and every proposal in real time (not actually in real time, but every minute or few minutes depending on the code if it can be done).

Copy link
Collaborator

@Fuzzbawls Fuzzbawls left a comment

Choose a reason for hiding this comment

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

Lots to dive into here, overall a great addition and presentation! Without duplicating @NoobieDev12 's feedback, i've restricted my comments mostly to translation issues and commenting on previous feedbacks. There is one bug that i commented on WRT to allocated/available amount being displayed in the UI.

Note: generally speaking, unless a particular UI element's name isn't self-descriptive enough, any text value set in the .ui file that is always going to be programmatically set in the .cpp file would be better to just use the generic <string notr="true">N/A</string> line in the .ui file

src/qt/pivx/forms/createproposaldialog.ui Outdated Show resolved Hide resolved
src/qt/pivx/forms/createproposaldialog.ui Outdated Show resolved Hide resolved
src/qt/pivx/forms/createproposaldialog.ui Outdated Show resolved Hide resolved
src/qt/pivx/forms/createproposaldialog.ui Outdated Show resolved Hide resolved
src/qt/pivx/forms/createproposaldialog.ui Outdated Show resolved Hide resolved
src/qt/pivx/proposalcard.cpp Outdated Show resolved Hide resolved
src/qt/pivx/votedialog.cpp Outdated Show resolved Hide resolved
src/qt/pivx/votedialog.cpp Outdated Show resolved Hide resolved
src/qt/walletmodel.cpp Outdated Show resolved Hide resolved
src/qt/pivx/proposalcard.cpp Outdated Show resolved Hide resolved
Plus fixed several warnings for QT duplicate widget container names.
…tion call.

* Connect governanceModel stop().
* And not update next superblock height visually until the wallet passed IBD.
Plus whitespace styling corrections.
This fixes a conversion issue for the QString::toInt() previous usage.
And don't try to create a proposal if the wallet has not enough balance to pay for the proposal fee.
… to vote until the minimum vote time passes.
Copy link
Collaborator

@Fuzzbawls Fuzzbawls left a comment

Choose a reason for hiding this comment

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

ACK fccb4cc

Lets get this merged sooner rather than later so translation source strings can be updated. any further functional or stylistic changes can be done in smaller follow-up PRs.

Copy link

@random-zebra random-zebra left a comment

Choose a reason for hiding this comment

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

ACK fccb4cc

Just few minor things that could also be tackled in a follow-up PR.

src/qt/pivx/governancemodel.cpp Outdated Show resolved Hide resolved
CBudgetProposal proposal;
ss >> proposal;
if (!g_budgetman.HaveProposal(proposal.GetHash()) &&
proposal.GetBlockStart() < clientModel->getNumBlocks()) {

Choose a reason for hiding this comment

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

(c952196)

Suggested change
proposal.GetBlockStart() < clientModel->getNumBlocks()) {
proposal.GetBlockStart() > clientModel->getNumBlocks()) {

Copy link
Author

Choose a reason for hiding this comment

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

great catch 👍

Comment on lines +61 to +65
enum VoteDirection {
ABSTAIN=0,
YES=1,
NO=2
};

Choose a reason for hiding this comment

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

why are we duplicating this enum in the VoteInfo struct, instead of using CBudgetVote::VoteDirection directly?
It would be easier to maintain.

Copy link
Author

Choose a reason for hiding this comment

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

the goal was to continue preparing the ground for the removal of the backend objects/functions dependencies on the GUI elements, to only have them in the cpp of the interface layer.

If I would had added CBudgetVote::VoteDirection here, as it's a header file, then every GUI element that has the governancemodel.h dependency, will have access to budgetvote.h, univalue.h and to the transaction.h primitive for example.

will think on a different way to remove the duplication and not end up having all this unneeded dependencies.

@furszy
Copy link
Author

furszy commented Nov 4, 2021

k great.
Only minor things, will merge it so we can start the strings translations process and tackle them in follow-up PR.

@furszy furszy merged commit b525146 into PIVX-Project:master Nov 4, 2021
furszy added a commit that referenced this pull request Nov 6, 2021
…ock has not passed yet

c33f0e5 Bugfix GUI: proposal broadcast should be scheduled only if the block start has not passed yet (furszy)

Pull request description:

  Small follow-up to #2406, solving zebra's comment: #2406 (comment).

  Fixing the not schedule of waiting proposals for broadcast at startup. Which caused that if the wallet was restarted in the middle of the broadcast process, the waiting proposals will not enter in the broadcast queue.

ACKs for top commit:
  random-zebra:
    utACK c33f0e5
  Fuzzbawls:
    utACK c33f0e5

Tree-SHA512: d0fd17276c24971571385828001a9cf95cfcf6e472b4e0dce0dcc054e2e633856833ebd39d0058fa81e5d7482483b7f04d163df1aec474c53f30ac8ff7cf564d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants