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

All Vote Validation (Except Signature) #47

Merged
merged 8 commits into from
Apr 29, 2020
Merged

Conversation

apbendi
Copy link
Contributor

@apbendi apbendi commented Apr 29, 2020

  • Validate token ownership
  • Validate each token is qualified
  • Validate each token has not already voted
  • Validate all qualifying tokens are voting
  • Validate each account can only vote once

Run npm test. Should not require any migrations. Run npm run dev Should work with seamlessly with the UI w/o modifications.

Closes #9

@apbendi apbendi requested a review from mds1 April 29, 2020 17:44
@mds1
Copy link
Contributor

mds1 commented Apr 29, 2020

All tests pass, but I did get an issue when using it on the frontend. Submitting a vote failed with the error message Tokens not held by voting account <tokenId>. It seems the problem is that !accountTokenIds.includes(tokenId) compares the wrong values:

  • accountTokenIds contains the unique IDs of the NFTs
  • tokenId is the token's event ID, which is passed from the frontend as part of the vote data

Let me know how you want to resolve this, as it seems there are a few different options here.


Also one small thing is that some of the error messages are a bit confusing. Two examples:

  • "Token not held by voting account 1234" would be more clear as "Token 1234 is not held by voting account"
  • "Token not qualified to vote in this poll 8019" would be more clear as "Token 8019 is not qualified to vote in this poll"

Similarly, with "Account or token cannot vote twice" perhaps we should specify the offending token ID?

@apbendi
Copy link
Contributor Author

apbendi commented Apr 29, 2020

tokenId is the token's event ID, which is passed from the frontend as part of the vote data

Unless I've gotten myself confused and I'm thinking of it wrong, I think this has to be the ID of the NFT coming from the front end. The user might, for example, own two tokens from the same event. And we would want to identify the tokens by their NFT Id to ensure no specific token has ever voted twice. Does that make sense? Apologies if I ever communicated that wrong to you in the past. Entirely possible I did.

Should be a pretty easy fix on the frontend, right? The poap API hands back both the token & event Ids. Mind pushing a commit that updates it?

Also one small thing is that some of the error messages are a bit confusing.

Good call, I will update this.

Similarly, with "Account or token cannot vote twice" perhaps we should specify the offending token ID?

Yes, this would be better, but because of the way I've structured this check (using one big query) it would take some logic changes that I don't think are worth it for now.

@mds1
Copy link
Contributor

mds1 commented Apr 29, 2020

Unless I've gotten myself confused and I'm thinking of it wrong, I think this has to be the ID of the NFT coming from the front end. The user might, for example, own two tokens from the same event. And we would want to identify the tokens by their NFT Id to ensure no specific token has ever voted twice. Does that make sense? Apologies if I ever communicated that wrong to you in the past. Entirely possible I did.

Should be a pretty easy fix on the frontend, right? The poap API hands back both the token & event Ids. Mind pushing a commit that updates it?

Ah, didn't realize you could have (or would want to count) multiple tokens from one event. I was using event ID on the front end to match it up with qualifying events, but since it's not necessarily one token per event then this would undercount votes.

Will push an update in a few minutes where voteData.token_ids is the unique ID instead

@apbendi
Copy link
Contributor Author

apbendi commented Apr 29, 2020

Will push an update in a few minutes where voteData.token_ids is the unique ID instead

Great, thanks! Just pushed a commit with better error messages. If all looks good and the frontend is working, feel free to merge 👍

@mds1 mds1 merged commit 03f8dd6 into master Apr 29, 2020
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.

Non-Signature Servside Validations & Derivations
2 participants