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

Featured voting #37

Merged
merged 29 commits into from
Jun 29, 2023
Merged

Featured voting #37

merged 29 commits into from
Jun 29, 2023

Conversation

jkbktl
Copy link
Collaborator

@jkbktl jkbktl commented May 15, 2023

  • load initial SNT amount from proposal vote (contract?)
  • load cooldown period for communities which were featured and cannot be voted for again (contract?)
  • verification
  • testing

@vercel
Copy link

vercel bot commented May 15, 2023

@jkbktl is attempting to deploy a commit to the Status Team on Vercel.

A member of the Team first needs to authorize it.

@jkbktl jkbktl changed the title [wip] featured voting [wip] Featured voting May 15, 2023
<ConfirmBtn
onClick={() => {
setShowModal(false)
history.go(0)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added reload of the page, so user can see newly proposed votings.

@@ -0,0 +1,12 @@
import React from 'react'
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Newly created page, so we can show featured communities.

@@ -25,7 +25,7 @@ export function App() {
<Page>
<GlobalStyle />
{mobileVersion ? <MobileRouter /> : <DesktopRouter />}
<NotificationsList />
{/* <NotificationsList /> */}
Copy link
Collaborator Author

@jkbktl jkbktl May 15, 2023

Choose a reason for hiding this comment

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

This is commented out, because of some issue which was breaking whole page. Needs to be debugged.

@jkbktl jkbktl changed the title [wip] Featured voting Featured voting May 25, 2023
@@ -14,6 +14,7 @@ export function NotificationsList() {
{notifications.map((notification) => {
if ('receipt' in notification) {
return notification.receipt.logs.map((log) => {
// this needs to be updated so it takes into account also interface of featuredVotingContract
const parsedLog = votingContract.interface.parseLog(log)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here is the issue with NotificationList, it accept list of notifications which can be emmited by both contracts (voting and featuredVoting), but there is currently no way how to differentiate notification item and therefore parse it by parser with correct interface. If notification item is parsed by wrong parser it throws an exception and crashes whole page.

@vercel
Copy link

vercel bot commented May 25, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
community-dapp ❌ Failed (Inspect) Mar 19, 2024 3:21pm
1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
community-dapp-dapp ⬜️ Ignored (Inspect) Visit Preview Mar 19, 2024 3:21pm

@jkbktl jkbktl marked this pull request as ready for review May 25, 2023 19:38
@jkbktl
Copy link
Collaborator Author

jkbktl commented May 25, 2023

Guys, it's ready for review, please check it out when you have a bit of time. I cannot add you as reviewers for some reason. cc @osmaczko @prichodko

@osmaczko
Copy link
Collaborator

Guys, it's ready for review, please check it out when you have a bit of time. I cannot add you as reviewers for some reason. cc @osmaczko @prichodko

Cool! I am on it. Could you please check tests as they are failing on CI?

Copy link
Collaborator

@osmaczko osmaczko left a comment

Choose a reason for hiding this comment

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

Overall looks good, thanks for implementation. However, I did spot a few minor issues:

  1. Once the directory vote is finalized the view is not updated
dapp-1-2023-05-31_12.08.58.mp4
  1. It should not be possible to vote more than once for featured community from the same account
dapp-2-2023-05-31_12.09.39.mp4

Copy link
Collaborator

@osmaczko osmaczko left a comment

Choose a reason for hiding this comment

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

Thanks. It mostly works as expected, I identified few issues:

  1. (blocker) Peers should be able to vote for more than one community
dapp-go-through-2023-06-28_09.05.05-00.08.45.568-00.09.22.560-seg1.mp4
  1. (blocker) "Verify Weekly featured" doesn't work for consecutive votings (works only for first voting)
dapp-go-through-2023-06-28_09.05.05-00.17.05.311-00.18.00.475-seg2.mp4
  1. (minor - will create a separate issue for that) directory page refreshes itself after each vote

@@ -62,7 +62,7 @@ contract FeaturedVotingContract {
uint256 public cooldownPeriod;

Voting[] public votings;
mapping(uint256 => Vote[]) private votesByVotingID;
mapping(uint256 => Vote[]) public votesByVotingID;
Copy link
Collaborator

Choose a reason for hiding this comment

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

leftover

@jkbktl
Copy link
Collaborator Author

jkbktl commented Jun 28, 2023

Peers can vote for more than one community, but they can't vote multiple times for one community?

@osmaczko
Copy link
Collaborator

Peers can vote for more than one community, but they can't vote multiple times for one community?

Yes, exactly.

@jkbktl jkbktl requested a review from osmaczko June 28, 2023 23:16
Copy link
Collaborator

@osmaczko osmaczko left a comment

Choose a reason for hiding this comment

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

Tested, core functionality works fine 💪

I'll create separate issues for minor stuff as followup.

@osmaczko
Copy link
Collaborator

osmaczko commented Jun 29, 2023

I gave it a second try and identified one critical issue, although I don't want to block this PR any longer. We should address it in followup.

dapp-go-through-2023-06-29_12.29.34-cut-merged-1688043261398.mp4
  1. The featured communities list is wrong, "From contract for portal" should not make it into the list, as it has the lowest amount of SNT. What I think happened is either castVotes didn't propagate all the votes to the contract or the topN calculation is done wrongly by the contract itself. I think it is rather the former because I just double checked topN calculation in contract, also it is covered with tests.

What I did is:
voted 100SNTs for communityA with account1
voted 100SNTs for communityB with account1
voted 100SNTs for communityC with account1
voted 1000SNTs for communityA with account2
voted 1000SNTs for communityB with account2
voted 1000SNTs for communityD with account2

where communityC is "From contract for portal", and communityD is "Contributors' test community"

  1. The button says "Finalize Weekly featured" instead of "Verify Weekly featured", this seems to be state update issue.

@@ -34,7 +34,7 @@
"chai": "^4.2.0",
"eslint": "^8.36.0",
"ethers": "5.4.1",
"hardhat": "^2.13.0",
"hardhat": "^2.16.1",
Copy link
Collaborator

Choose a reason for hiding this comment

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

@jkbktl
Copy link
Collaborator Author

jkbktl commented Jun 29, 2023

@osmaczko let's merge it then (I don't have permissions)

@osmaczko osmaczko merged commit 420c9b2 into status-im:master Jun 29, 2023
3 checks passed
@osmaczko osmaczko linked an issue Jun 30, 2023 that may be closed by this pull request
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.

Update UI to respect new featured communities verification mechanism
3 participants