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

Improve ProxyFactory #462

Closed
rmeissner opened this issue Dec 16, 2022 · 4 comments · Fixed by #468
Closed

Improve ProxyFactory #462

rmeissner opened this issue Dec 16, 2022 · 4 comments · Fixed by #468
Assignees
Milestone

Comments

@rmeissner
Copy link
Member

rmeissner commented Dec 16, 2022

Following changes should be made to the proxy factory:

  • remove createProxy
    • This method uses create which is not counterfactual for a specific deployment. This cause user errors and potential lost/ stuck funds
  • Add check that mastercopy exists
  • If init data is provided it should be ensured that the mastercopy exists and that the init call was successful to avoid that a proxy is deployed uninitialized
  • Add createNetworkSpecificProxy
    • This proxy will use the chainId in the salt and therefore it will not be possible to create it on the same address on other networks
    • This method should enable the creation of proxies that should really exist only on 1 network at the same address (e.g. specific governance or admin accounts)
  • Remove calculateProxyAddress method because methods that use the revert approach to return data. It doesn't work well with all nodes (as they all return revert messages differently)
@rmeissner rmeissner added this to the Version 1.4.0 milestone Dec 16, 2022
@mmv08 mmv08 self-assigned this Dec 23, 2022
@mmv08
Copy link
Member

mmv08 commented Dec 25, 2022

btw @rmeissner what's our stance on ZkSync? Just raising this because our current proxy factory is not compatible with their compiler. I wonder if we should do something, perhaps, create a separate zk proof factory because I assume the problem we had with zksync is universal for all type 4 zkEVMs

For more context check matter-labs/hardhat-zksync#99

@mmv08 mmv08 linked a pull request Dec 25, 2022 that will close this issue
@rmeissner
Copy link
Member Author

hmmm not sure if we require this method 🤔 Personally I am happy to remove it, iirc @Uxio0 added it in the past, so not sure if we use it somewhere.

@mmv08
Copy link
Member

mmv08 commented Dec 28, 2022

Looks like we have it in the safe-eth-py package but I don't see see it used on the backend

@Uxio0
Copy link
Member

Uxio0 commented Jan 16, 2023

hmmm not sure if we require this method 🤔 Personally I am happy to remove it, iirc @Uxio0 added it in the past, so not sure if we use it somewhere.

We used it on the relay, as there was no way to get the deployment addresses for addresses that were not funded, as the deploy function reverted. We can remove it.

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 a pull request may close this issue.

3 participants