-
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
feat(state-tree-depth) - ensure stateTreeDepth is not hardcoded #777
Conversation
@@ -129,6 +131,7 @@ contract MACI is IMACI, DomainObjs, Params, SnarkCommon, Ownable { | |||
pollFactory = _pollFactory; | |||
signUpGatekeeper = _signUpGatekeeper; | |||
initialVoiceCreditProxy = _initialVoiceCreditProxy; | |||
stateTreeDepth = _stateTreeDepth; |
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.
Is there any danger in this value now being an arbitrary size? i.e. should there be a check against some max value?
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.
Yes and no. If they put a value that does not match the circuit, they would basically need to start over (there might be some workarounds though, still not ideal).
This is a uint8 so max value would be 255, having said that, a tree depth of 255 would be incredibly big, considering 10 allows for a little bit more than 1M total signups.
My view here is the following:
- if we put a max value, what should this be?
- In the past, we asked the users to change a constant variable rather than passing it as a constructor parameter:
- I consider these options kinda the same, we never really validated that this constant variable was > than a max value
- people running MACI should be (and are to some extent) informed that this parameter should match the one in the circuits, hence it is their responsibility of setting it up correctly.
- even if this value was < than a max value we decide on, it could still be different than what they put in the circuit and in the other components of MACI -> end result would still not work
To summarize, I think the best we can do here in terms of "validation" is to properly inform that this value should match whichever value is in the Circom circuits and how many signups each value supports.
contracts/ts/__tests__/MACI.test.ts
Outdated
|
||
describe('Deployment', () => { | ||
beforeAll(async () => { | ||
signer = await getDefaultSigner() | ||
const r = await deployTestContracts( | ||
initialVoiceCreditBalance, | ||
STATE_TREE_DEPTH, | ||
true |
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.
Why pass true
here?
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 basically those utils functions like deployTestContract or deployContract have an optional parameter called "quiet", passing true so it doesn't print unnecessary logs while running the tests
7c1580d
to
a755b18
Compare
To confirm, we don't have to follow the steps described in this doc anymore? |
6d562ea
to
30ec3d9
Compare
correct, I would say we probably need to improve documentation on this change - for now I updated the scripts in the contracts folder, as well as the README. What do you think, should we updated the cli docs too to ensure the new flag ("-s") is more evident? would be curious to hear your opinion after you review this PR and how it worked for you testing it |
str2BigInt, | ||
getSignal | ||
} | ||
export const STATE_TREE_DEPTH = 10 |
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.
Is there a specific reason this is here? Perhaps it could fit well in constants.ts
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.
totally
It’s an essential value, so it should be as visible as possible. However, people often don’t read the documentation thoroughly..so we might need to seek a somewhat better way to highlight it. Although a good idea doesn’t come to mind immediately. |
Just a reminder that there is a merge conflict that needs to be addressed. Once that's resolved, it looks like we're all set to merge this PR into the dev branch. Great work! |
…e smart contracts and can be passed around as an argument
30ec3d9
to
b01eabc
Compare
merge conflicts sorted! thanks for the review |
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.
LGTM
We do not want to hardcode the stateTreeDepth in the smart contracts, but instead we want to pass them as arguments.
Changes:
compileSol
takes an additional parameter so that we can compile the trees contracts with the correct params