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

Validate Poll Attestation Signature Upon Submission #18

Merged
merged 4 commits into from
Apr 14, 2020

Conversation

apbendi
Copy link
Contributor

@apbendi apbendi commented Apr 13, 2020

Closes #5, with the exception of the bug documented in #17

apbendi and others added 3 commits April 13, 2020 12:02
* Add PollValidator Unit Tests for attestation signature
  validation
* Fix endpoint smoketests to work as expected with attestations
  now being included & validated
* Refactor signature recovery into a helper method that can eventually
  be re-used for vote attestations
@apbendi apbendi requested a review from mds1 April 13, 2020 16:09
@mds1
Copy link
Contributor

mds1 commented Apr 13, 2020

LGTM! One nit—line 43 of poll-validator-test.js spells attestation wrong, so doesn't actually delete the attestation key. But the thing is, deleting the key is unnecessary in multiple tests throughout this file, since the key is just overwritten anyway.

Like I said, super small thing, but figured I'd point it out and let you decide how to handle it.

Feel free to rebase and merge afterwards!

Copy link
Contributor

@mds1 mds1 left a comment

Choose a reason for hiding this comment

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

GitHub has an "Add your review" button on the PR page since I was requested to review this PR. So I'm just repeating my previous comment here to try out this feature.

Comment from before

LGTM! One nit—line 43 of poll-validator-test.js spells attestation wrong, so doesn't actually delete the attestation key. But the thing is, deleting the key is unnecessary in multiple tests throughout this file, since the key is just overwritten anyway.

Like I said, super small thing, but figured I'd point it out and let you decide how to handle it.

Feel free to rebase and merge afterwards!

Edit: Oh, I think if I were to comment on specific lines of the diff it would show here. Pretty cool

@apbendi apbendi merged commit 39ffc42 into master Apr 14, 2020
@mds1 mds1 deleted the backend-signature branch April 16, 2020 20:32
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.

Backend Poll Post Endpoint To Process Signed Message
2 participants