refactor(contracts): smart contracts optimizations #919
Conversation
✅ Deploy Preview for maci-typedoc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
ctrlc03
force-pushed
the
refactor/contract-optimizations
branch
3 times, most recently
from
cbef032 to
63b5180
Compare
ctrlc03
force-pushed
the
refactor/contract-optimizations
branch
from
63b5180 to
195bdd6
Compare
| // The final commitment to the state and ballot roots | ||
| uint256 public sbCommitment; | ||
| uint256 public subsidyCommitment; | ||
|
|
||
| uint8 public constant treeArity = 5; | ||
| uint8 internal constant TREE_ARITY = 5; |
There was a problem hiding this comment.
Thinking about that TREE_ARITY is called so many times in different contracts, and they should keep the same. Should we put it in somewhere and import it in these contracts?
There was a problem hiding this comment.
mm maybe we could put it inside another contract shared by all of them though I think that would bloat things up a little bit 🤔 was looking and couldn't find a contract that was already in use by all of them (MACI, Poll, MP, Tally, etc) where we could add this constant..
There was a problem hiding this comment.
Is that a good idea to have a contract holding the constants? or we just keep status quo?
There was a problem hiding this comment.
mm we have SnarkConstants holding some constants, though I'm not sure if it's a widely used pattern.. to avoid bloating contracts even more I would just keep this one constant inside each contract, at least for now
There was a problem hiding this comment.
Then when it comes to deployment, if a user wants to alter the TREE_ARITY, they currently need to navigate through every contract file to modify the value. What if we convert it into an environmental variable for the sake of convenience during deployment?
There was a problem hiding this comment.
Fair point, though they would also need to change the type of AccQueue in use, which currently is hardcoded to a quinary tree everywhere, mm 🤔
There was a problem hiding this comment.
useful discussion here, there's definitely some more simplifying we can do given we always use quinary trees
There was a problem hiding this comment.
by converting to env variable, you mean that it should be a value passed in the constructor when deploying the contracts?
There was a problem hiding this comment.
yes... but not sure if this is a good idea, just wanna reduce the risk that if some users deploy it and miss to change one of them?
There was a problem hiding this comment.
let's write this down and discuss together at the next team meeting? from next week we'll have a smart contract developer to ask all SC questions to 😈
ctrlc03
force-pushed
the
refactor/contract-optimizations
branch
3 times, most recently
from
f409fe2 to
f1514c4
Compare
ctrlc03
force-pushed
the
refactor/contract-optimizations
branch
8 times, most recently
from
c171e65 to
97a4589
Compare
Unify usage of TREE_ARITY Use calldata vs memory for function params which are immutable Ensure we do not accept max messages + 1 in top up Use payable for constructors to reduce deployment costs
ctrlc03
force-pushed
the
refactor/contract-optimizations
branch
from
97a4589 to
b10ff70
Compare
Description
Related issue(s)
#843
Confirmation