-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Generic migration contract #793
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
Conversation
Pull Request Test Coverage Report for Build 10010745656Details
💛 - Coveralls |
…ntract tests, fix tests in Safe150Migration.spec.ts
Co-authored-by: Nicholas Rodrigues Lordello <n@lordello.net>
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.
Looks great! Just some minor nits, but everything is looking Gucci.
Co-authored-by: Nicholas Rodrigues Lordello <n@lordello.net>
Co-authored-by: Mikhail <16622558+mmv08@users.noreply.github.com>
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.
Co-authored-by: Nicholas Rodrigues Lordello <n@lordello.net>
Co-authored-by: Nicholas Rodrigues Lordello <n@lordello.net>
Co-authored-by: Nicholas Rodrigues Lordello <n@lordello.net>
Co-authored-by: Nicholas Rodrigues Lordello <n@lordello.net>
Co-authored-by: Mikhail <16622558+mmv08@users.noreply.github.com>
Co-authored-by: Mikhail <16622558+mmv08@users.noreply.github.com>
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.
Small nit as Nick suggested, rest LGTM 👍🏾
Fixes: #787
Summary
This PR adds a general migration contract that takes address of the Safe, SafeL2 and fallback handler contracts during deployment. The contract allows Safe to update the Singleton at
address(0)
.As of now tests cover below migration paths:
See
SafeMigration.spec.ts
to see how tests are organised. Do share if you any thoughts to better run same tests on different migration paths.The migration contract stores address of the Safe singletons and fallback handler rather than using code hash and requiring the user to provide singleton address as described in the issue. The reason being as follows:
Checking codehash of the target singleton means user has to provide the address of the target singleton. Also, checking code hash has higher gas costs.
The only argument for using code hash for upgrades is that it also allows unofficial singletons to be used for migration using official migration contract. But, users/projects can also deploy their own version of migration contract by providing singleton addresses in the constructor and have similar security guarantees as the official migration contract.
Changes in PR
Unlike
Safe150Migration.sol
, this new contract does not check if slot(0) of the contract stores an address having some non-empty code. I think this check is not need because this contract is not intended to be used in general by other proxy contracts and checking slot(0) value is only a partially correct way. Would like to know thought of others.