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

feat: make voting easier with delegate modal #1027

Merged
merged 42 commits into from Mar 1, 2019

Conversation

@dated
Copy link
Contributor

commented Jan 26, 2019

Proposed changes

#803

vote

Types of changes

  • New feature (non-breaking change which adds functionality)

Checklist

  • I have read the CONTRIBUTING documentation
  • Lint and unit tests pass locally with my changes

dated added some commits Jan 26, 2019

@dated

This comment has been minimized.

Copy link
Contributor Author

commented Jan 27, 2019

This PR depends on #1023 and won't work as intended without those changes.

dated added some commits Jan 29, 2019

@dated

This comment has been minimized.

Copy link
Contributor Author

commented Jan 29, 2019

There is one edge case which currently isn't handled correctly, i.e. when the the vote/unvote transaction gets broadcast and confirmed before the broadcast is finished. The vote details won't be loaded correctly in that case. If you want to try it out in the meantime @alexbarnsley 😉

@alexbarnsley

This comment has been minimized.

Copy link
Member

commented Jan 29, 2019

🤔 yeh will have a test either today or tomorrow 👌

j-a-m-l and others added some commits Jan 30, 2019

@dated dated changed the title [WIP] feat: make voting easier with delegate modal feat: make voting easier with delegate modal Jan 30, 2019

@dated

This comment has been minimized.

Copy link
Contributor Author

commented Jan 30, 2019

the above edge case is handled correctly now.

@dated dated changed the title feat: make voting easier with delegate modal [WIP] feat: make voting easier with delegate modal Jan 31, 2019

@dated dated force-pushed the dated:wallet-vote branch from 2bb09ed to f618591 Jan 31, 2019

@dated dated changed the title [WIP] feat: make voting easier with delegate modal feat: make voting easier with delegate modal Jan 31, 2019

dated added some commits Jan 31, 2019

@j-a-m-l

This comment has been minimized.

Copy link
Contributor

commented Feb 5, 2019

@dated is this already finished?

@dated

This comment has been minimized.

Copy link
Contributor Author

commented Feb 5, 2019

If you can give it a try that'd be great. I'll have another look at it to see if somethings missing.

@j-a-m-l
Copy link
Contributor

left a comment

I love that the bar at the bottom displays a message when the vote is being loaded, etc, but the processing status isn't changed even when the transaction has been well confirmed.

src/renderer/components/Input/InputDelegate.vue Outdated Show resolved Hide resolved
@ArkEcosystemBot

This comment has been minimized.

Copy link
Member

commented Feb 5, 2019

@dated Your pull request needs some changes. Please wait for a comment from one of our developers for more information.

@dated

This comment has been minimized.

Copy link
Contributor Author

commented Feb 5, 2019

I'm unable to reproduce what you are saying unfortunately, the wallet shows as processing after the vote has been sent successfully until the actually confirmed vote transaction has been received.

@alexbarnsley

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

all looking good! just a message I think at the bottom of the delegate modal when already voting elsewhere, which will be replaced by the "switch votes" option when you get round to it 👌

@dated dated changed the title feat: make voting easier with delegate modal [WIP] feat: make voting easier with delegate modal Feb 28, 2019

@dated dated changed the title [WIP] feat: make voting easier with delegate modal feat: make voting easier with delegate modal Feb 28, 2019

@dated dated force-pushed the dated:wallet-vote branch from 25ffb3a to 2c642e2 Feb 28, 2019

@alexbarnsley

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

I hate to say it @dated 😩 but I'm getting issues when i hit the "unvote" button in the footer. I'd screenshot but my clipboard is messed up. It's like it can't find the delegate. The modal pops up to unvote but all it says is "Unvote Delegate arkx" 🤔 will try a restart in the morning and do a screenshot then if you still need one. Double check as it may be my local data, but I believe it was working earlier

@dated

This comment has been minimized.

Copy link
Contributor Author

commented Feb 28, 2019

I broke that in one of the recent commits, it's fixed now @alexbarnsley.

@dated

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2019

There is a little issue still that's most likely caused by the api cache and the way fetchWalletVote works: after voting the outdated information is loaded. A proper fix for this will be possible once local storage of the transactions is implemented.

@dated

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2019

Alternatively one could leverage the fact, that the wallet vote is stored on the wallet itself, which would eliminate the need to touch the transactions at all.

@alexbarnsley

This comment has been minimized.

Copy link
Member

commented Mar 1, 2019

🤔 well we have access to the delegate the wallet is voting for don't we. as you said in your last comment (which may be saying what i'm saying), the wallet API stores the delegate that wallet is voting for vote: "publickey"

@dated

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2019

yea, we are talking about the same

@alexbarnsley

This comment has been minimized.

Copy link
Member

commented Mar 1, 2019

Okay, great. Are you happy with this PR? If so, I'll give it a test

@dated

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2019

If you are happy with me changing the vote fetching in a future iteration, then yes 😄 the API call was previously fired everytime isAwaitingConfirmation changed, so when a vote transaction was received in under 8 seconds it fetched the cached response, which did not include the last vote. If i'm not mistaken, right now it should fail (= show outdated information) only when all of the following happens in a timeframe of 8 seconds: open wallet -> select delegate -> input passphrase & fee -> send/receive transaction

@ArkEcosystemBot

This comment has been minimized.

Copy link
Member

commented Mar 1, 2019

@dated A member has approved this PR. A maintainer will merge this PR shortly. If it shouldn't be merged yet, please leave a comment saying so and we'll wait.

Thank you for your contribution!

@alexbarnsley alexbarnsley merged commit 26da980 into ArkEcosystem:develop Mar 1, 2019

1 check passed

ci/circleci: test-node-11 Your tests passed on CircleCI!
Details
@ArkEcosystemBot

This comment has been minimized.

Copy link
Member

commented Mar 1, 2019

@dated Your pull request has been merged and marked as tier 1. It will earn you $100 USD.

@dated dated deleted the dated:wallet-vote branch Mar 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.