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

Staking reward setup GUI #605

Merged
merged 23 commits into from
Oct 8, 2019

Conversation

aguycalled
Copy link
Member

@aguycalled aguycalled commented Sep 20, 2019

This PR adds a new GUI to set up different ways to split the staking rewards. Setup can be done generally for every address or individually per staking address.

Works for both hot and cold staking methods.

Includes the following hardcoded addresses:

{"NN5QSSMAdtRU35BffLZUw9vChnhHKKMeuL", "Alex aguycalled - Core Dev"},
{"NRXfZ1egFxMSUsc4Ufpi4Lm7DdXStYmeBG", "mxaddict - Core Dev"},
{"3HnzbJ4TR9", "Community Fund"},
{"XVPMwBdNU9ou3a3TnwaVgAgEecbdsEVZHbVmeY4TMAHbY6BdtY8xW6m1Q1rkb", "Core Dev Bounty"},
{"XAznGHuQ35hvgSGsVWi5Nu2Y6n3rT4cycE3yfZWCfnNjycCGdGAEnta2G24Mi", "Web Dev Bounty"}

Please comment in this pull request to add other community member addresses.

image

image

@chasingkirkjufell
Copy link
Contributor

built and tested locally on ubuntu 18.04. it worked as expected. made a couple suggestions on handling the default settings.
the receiving stakes will display 2 NAV no mater how ever much the % is if the staking address and the receiving addresses both belong to the same wallet.dat. not likely to be encountered and the actual amount is accurate despite showing 2 NAV.

@chasingkirkjufell
Copy link
Contributor

image

the new drop down is empty and nothing happens after clicking.

@aguycalled
Copy link
Member Author

should be fixed now @chasingkirkjufell

@chasingkirkjufell
Copy link
Contributor

chasingkirkjufell commented Sep 24, 2019

in cold-staking mode, staking node stake redirection did not work when choosing the cold-staking address from the drop down list and then point to an address that belongs to staking node. it only worked when the redirection address was set under "default" rather than "cold-staking address".

Edit: also doesn't work if pointed to a 3rd unrelated wallet address. so basically choosing the cold-staking address from the drop down list and then set a redirection address does not work.

@aguycalled
Copy link
Member Author

this happens because the wallet looks for the configuration for the staking address of the cold staking address which owns the coins that are staking. i've updated the gui so it only shows the staking address instead of the long cold staking one. does it sound like a good solution?

@chasingkirkjufell
Copy link
Contributor

should be fine if the amount it's cold-staking is also shown so users don't have to figure out which address they used. the questions though will be what will it show if somehow someone send coins to the staking address, will it show the cold-staking amount or the amount in the address? will test it tomorrow.

@aguycalled
Copy link
Member Author

it will show both

Copy link
Contributor

@mxaddict mxaddict left a comment

Choose a reason for hiding this comment

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

utACK, building now

@chasingkirkjufell
Copy link
Contributor

when receiving re-directed hot-staking or cold-staking, the transaction history shows receiving 0 NAV instead of the redirected amount on the re-directed address.
image

image

coin control shows the correct amount.

image

@aguycalled
Copy link
Member Author

how is it shown in listtransactions?

@chasingkirkjufell
Copy link
Contributor

0

image

@aguycalled
Copy link
Member Author

should be fixed now

@chasingkirkjufell
Copy link
Contributor

setting window width increases with the amount of addresses set and cannot be decreased.

image

image

http://giphygifs.s3.amazonaws.com/media/njYrp176NQsHS/giphy.gif :P

@aguycalled
Copy link
Member Author

fixed
image

@chasingkirkjufell
Copy link
Contributor

users are able to set 0% contribution to an address, which is fine, but the receiving address will actually see it. could it be used to bloat the blockchain if someone just set infinite numbers of addresses getting 0%?

image

image

@chasingkirkjufell
Copy link
Contributor

when setting both default redirection address and individual address redirection address, redirection will stop after setting individual address and then removing it while it shouldn't since the default should still redirect it to the set address.

a uncommon situation but when cold-staking, the spending wallet redirection drop down will show the staking address as well as the spending address tied to it as separate addresses. uncommon since if someone is cold-staking, they won't need to use redirection on the spending wallet.

@chasingkirkjufell
Copy link
Contributor

wait, i might have made a mistake

@chasingkirkjufell
Copy link
Contributor

my bad, cold-staking redirecting to another cold-staking address works just fine. i opened the wrong wallet and redirected to myself that was why i couldn't see any contribution difference. and a hot-staking node can redirect stakes to a cold-staking address normally. so the irregularity is only when a node is both hot-staking and cold-staking.

@chasingkirkjufell
Copy link
Contributor

okay, narrowed down. if a node is cold-staking and hot staking, hot staking stake redirection works normally unless the redirected address is the long cold-staking address which the wallet owns the staking address, then the incorrect amount will be shown. the correct amount is shown if hot-stakes redirected to just the staking address of the cold-staking address. only doesn't work when redirected to the long cold-staking address the wallet is staking with.

image

@aguycalled
Copy link
Member Author

could you share a screenshot of your rewards setup?

@chasingkirkjufell
Copy link
Contributor

image

image

image

image

@chasingkirkjufell
Copy link
Contributor

Yeah the spending address in my situation belongs to another wallet. The problem comes when a normal address of the staking node and belongs to the staking node wallet finds a block and redirects it to a long cold staking address of which the staking node only owns the staking address and not the spending address

@chasingkirkjufell
Copy link
Contributor

Kind of proud that I could find that problem tbh :P

@mxaddict
Copy link
Contributor

mxaddict commented Sep 26, 2019 via email

@aguycalled
Copy link
Member Author

@chasingkirkjufell i wanted to send you some navs from the dev bounty for helping testing if you could share a nav address 😉

@chasingkirkjufell
Copy link
Contributor

sure. really appreciate it. :)
NL9jPW75P4kNMdvQmFBpYVkZ3sABQtXfYY

@mxaddict
Copy link
Contributor

mxaddict commented Sep 26, 2019 via email

@aguycalled
Copy link
Member Author

i think i got it fixed now

@chasingkirkjufell
Copy link
Contributor

tested and worked well. all redirection possibilities worked as expected. the only imperfection is what i mentioned before but it doesn't affect functionality. good job!

#605 (comment)

@mxaddict
Copy link
Contributor

@aguycalled can you add my address to this list? :)

NRXfZ1egFxMSUsc4Ufpi4Lm7DdXStYmeBG

@@ -24,6 +24,7 @@
#include <QVBoxLayout>

static QMap<QString, QString> teamAddresses = {{"NN5QSSMAdtRU35BffLZUw9vChnhHKKMeuL", "Alex aguycalled - Core Dev"},
{"NRXfZ1egFxMSUsc4Ufpi4Lm7DdXStYmeBG", "mxaddict - Core Dev"},
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

@aguycalled
Copy link
Member Author

Paid 2,800NAV to NL9jPW75P4kNMdvQmFBpYVkZ3sABQtXfYY
300 (minor ux issue) + 600 (major ux issue) + 600 (major ux issue) + 600 (major ux issue) + 100 (visual glitch) + 300 (minor ux issue) + 300 (minor ux issue)
339db52e68ca2937bc36fe38addca5d6e15333cbaf54de4045d76400d0e05aed

@chasingkirkjufell
Copy link
Contributor

thanks a lot!! :D

@proletesseract
Copy link
Member

Compiles & Runs on OSX 10.14.5. Seems like there's been some pretty rigorous testing on this one so far. My only real feedback is adding a button to the staking section on the overview page which also launches the staking setup window to drive people to consider using it more than just hidden away in the menu. I've committed the change adding the button. If you don't think we should have the button there we can roll it back.

I've also added a donation address for me :)

Screen Shot 2019-10-06 at 5 08 24 PM

@@ -61,11 +62,11 @@ public Q_SLOTS:
void updateStakeReportNow();
void updateStakeReportbalanceChanged(CAmount, CAmount, CAmount, CAmount, CAmount, CAmount, CAmount);
void setVotingStatus(QString text);

void on_pushButton_clicked();
Copy link
Member Author

Choose a reason for hiding this comment

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

@proletesseract could we maybe use another name for this method? it reads very generic and does not refer to its real meaning

Copy link
Member

Choose a reason for hiding this comment

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

Sure, that was just what was autogenerated by QT Creator. I'll commit the change when i get a chance.

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.

None yet

4 participants