-
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
refactor(contracts): optimize code #1008
Conversation
✅ Deploy Preview for maci-typedoc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
bf8415f
to
9a83eba
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.
@ctrlc03 thanks, just one question. Do we need to support the same state for deployed smart contracts? As far as I know, if someone tries to redeploy these contracts using proxies, it won't work because state if not the same.
In case if we don't care about it, I'm good with these changes.
Could you please expand on that? do you mean because of immutable parameters? if so, there's libraries for proxy clones with immutable args: https://github.com/wighawag/clones-with-immutable-args |
I had some issues after adding immutable which causes storage layout change. But if it's not a case, let's leave it. |
issues when upgrading? I've not tried these contracts with any proxy pattern before tbf |
Yes, it's related to upgrades |
oh that makes sense then! I don't think MACI as a protocol should use upgradable proxies, to enhance transparency. On the other hand, at some point we should definitely consider using clone proxies (or another efficient method) to deploy instances. What do you think? |
@ctrlc03 I think yes, at least we need to know community about that because someone else can use it with proxies and it can be a problem after upgrade with storage layout. |
Definitely agree! Yes being able to use already deployed contracts and just point a proxy to it would be fantastic. Let's merge this after #949 ? |
@ctrlc03 sure, thanks! |
…emoving redundant code
9a83eba
to
d39d384
Compare
Description
Optimize code by using immutable variables are removing redundant code.
Confirmation