-
Notifications
You must be signed in to change notification settings - Fork 153
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
tests(key-change): add tests and docs related to MACI's key change #885
Conversation
✅ Deploy Preview for maci-typedoc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Super cool. It's much needed. |
dea1c25
to
ccef119
Compare
ccef119
to
b75a462
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! Just left a nit
VOTE_OPTION_TREE_DEPTH, | ||
coordinatorPubKey, | ||
); | ||
stateIndex = parseInt(await signup(keypair1.pubKey.serialize())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stateIndex = parseInt(await signup(keypair1.pubKey.serialize())); | |
stateIndex = await signup(keypair1.pubKey.serialize()); |
nit: it's better to return number
type from signup()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@baumstern Can you please dismiss your change request as this is something not related to this PR? will amend in another PR
cli/tests/keyChange.test.ts
Outdated
const pollId = 0; | ||
let stateIndex = 0; | ||
const expectedTally = initialVoteAmount - 1; | ||
const expectedPerVOteOptionTally = (initialVoteAmount - 1) ** 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanna ask why is expectedPerVOteOptionTally
except for expectedPerVoteOptionTally
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please rephrase that? I'm afraid I didn't get it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh just wanna ask why the character 'o' is capitalized XD
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ohh it's a typo 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed :)
b75a462
to
852510c
Compare
852510c
to
ce394e2
Compare
Currently, it is not clear how the key change mechanism works (or doesn't), hence this PR aims to provide some clarification on this front, by adding docs and test cases.
This is in reference to this: #717