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: add reason when voting #511

Merged
merged 39 commits into from
Aug 7, 2024
Merged

feat: add reason when voting #511

merged 39 commits into from
Aug 7, 2024

Conversation

wa0x6e
Copy link
Contributor

@wa0x6e wa0x6e commented Jul 20, 2024

Summary

Re-submission of the vote reason modal PR (#427), which was reverted

Towards: #386

This PR moves the vote UI inside a vote modal, similar to v1, and allow attaching a reason to a vote.

Features

  • new vote modal: voting for a choice will open a Vote modal, where user can see a summary of its vote and voting power, before submitting the vote. This step will add an extra guard layer, where we can prevent users without sufficient voting power from voting.
  • vote with reason: a new reason field has been added in the vote modal, where the voter can attach a reason to his vote.
  • refactoring the getVotingPower logic into a store and composable, similar to how we handle Space, enabling caching and avoid code duplicate

How to test

  1. Try to vote from inside a proposal page, or a proposal list (space proposals/home feed)
  2. It should open the vote modal if logged in, else will open the account modal
  3. The vote modal should show your selected choice, as well as your voting power for the current proposal
  4. The CONFIRM button should be disabled if you don't have sufficient voting power to vote
  5. You can attach an optional reason to your vote
  6. Vote is limited to 1000 chars, and should disable the form on error
  7. Clicking on the CONFIRM should vote like before if no reason, and will add a metadataUri (onchain) or reason (offchain) field if set
  8. It should show a warning message and disable the submit button when the vp fetching has failed or on insufficient vp
  9. On the create proposal page and vote modal, it should show a RETRY button to refetch the voting power, in case of fetching error
  10. Voting power should be cached, you should not see the loading indicator when getting the same vp again on same space and block, and should show the vp immediately.
  11. Voting power should refresh on wallet change/logout

TODO

  • Confirm reason works offchain
  • Confirm reason works on EVM testnet (waiting for thegraph fix on the sepolia graph)
  • Confirm reason works on EVM mainnet (insufficient fund on all L2 networks)
  • Confirm reason works on Starknet testnet
  • Confirm reason works on Starknet mainnet
  • Fix/Update tests

Future

@wa0x6e wa0x6e marked this pull request as ready for review July 20, 2024 05:46
@wa0x6e
Copy link
Contributor Author

wa0x6e commented Jul 20, 2024

Issue about voting power fixed.

Tested and confirmed the reason submission is working on starknet and offchain, but still not able to test on evm, as eth-sepolia not working, and other l2 networks have insufficient funds.

@wa0x6e wa0x6e requested review from ChaituVR and Sekhmet and removed request for ChaituVR July 25, 2024 10:27
@wa0x6e
Copy link
Contributor Author

wa0x6e commented Jul 25, 2024

Finally tested on our own new sepolia graph, it's working well.

Copy link
Member

@Sekhmet Sekhmet left a comment

Choose a reason for hiding this comment

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

Removing close trigger for #386 because this PR is not covering entire issue (progress, share etc.)

apps/ui/src/networks/types.ts Outdated Show resolved Hide resolved
apps/ui/src/stores/votingPowers.ts Outdated Show resolved Hide resolved
apps/ui/src/stores/votingPowers.ts Show resolved Hide resolved
apps/ui/src/components/IndicatorVotingPower.vue Outdated Show resolved Hide resolved
apps/ui/src/views/Proposal.vue Outdated Show resolved Hide resolved
apps/ui/src/components/ProposalsListItem.vue Outdated Show resolved Hide resolved
apps/ui/src/components/Modal/VotingPower.vue Outdated Show resolved Hide resolved
apps/ui/src/components/MessageVotingPower.vue Outdated Show resolved Hide resolved
availableChoices: string[],
choice: number | number[] | Record<string, number>
) {
export function getChoiceText(availableChoices: string[], choice: Choice) {
Copy link
Member

Choose a reason for hiding this comment

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

Not sure why those changes were introduced in this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We use getChoiceText() to preview the user's choice in the vote modal.

The previous function does not return anything when the choice is invalid or blank (in the case of weighted choice)

@bonustrack
Copy link
Member

bonustrack commented Jul 30, 2024

For some reason, when I create a proposal with this PR, I stay on the editor instead of being redirected to the proposals page. I also can change the voting type while it shouldn't be the case on the space I've enforced "basic" voting type, this space: http://localhost:8080/#/s:fabien.eth In the main branch I don't see these issues. Otherwise the modal looks good.

@wa0x6e
Copy link
Contributor Author

wa0x6e commented Aug 5, 2024

Just noticed that this PR is currently only checking that voting power is > 0 to vote, and does not take into consideration when a space has set a custom minimum voting power score.

Will work on that

@wa0x6e wa0x6e requested a review from Sekhmet August 5, 2024 12:58
@wa0x6e
Copy link
Contributor Author

wa0x6e commented Aug 5, 2024

Just noticed that this PR is currently only checking that voting power is > 0 to vote, and does not take into consideration when a space has set a custom minimum voting power score.

Will work on that

After some progress, may be better to have it in another PR, as this PR is already complex enough. Seems like it's not a matter of adding 2 more lines

Copy link
Member

@Sekhmet Sekhmet left a comment

Choose a reason for hiding this comment

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

tACK, other than one small comment, needs rebase as well.

apps/ui/src/views/Space/Proposals.vue Outdated Show resolved Hide resolved
@Sekhmet Sekhmet merged commit 9e4e6ea into master Aug 7, 2024
3 checks passed
@Sekhmet Sekhmet deleted the feat-vote-reason-modal branch August 7, 2024 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants