1.4.1 Feature: Backport migration contracts to 1.4.1 #795
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fixes: #787 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)`. - Uses error strings rather than error types because Solidity version 0.7.6 doesn't support it. As of now tests cover below migration paths: - 1.3.0 to 1.5.0 - 1.3.0 to 1.4.1 - 14.1 to 1.5.0 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. - Create a general migration contract which is not tightly bound to any specific Safe version - Update tests - Remove other migration contract as this PR supersedes it 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. --------- Co-authored-by: Nicholas Rodrigues Lordello <[email protected]> Co-authored-by: Shebin John <[email protected]> Co-authored-by: Mikhail <[email protected]>
* Add contract to migrate a Safe from not L2 to L2 * Related to safe-global/safe-transaction-service#1703
Pull Request Test Coverage Report for Build 10039158260Details
💛 - Coveralls |
|
Nit: can we rename the branch to |
nlordell
reviewed
Jul 22, 2024
| /** | ||
| * @notice Address of this contract | ||
| */ | ||
| address public immutable MIGRATION_SINGLETON; |
There was a problem hiding this comment.
These lints are annoying - can we fix the linter settings?
There was a problem hiding this comment.
I think it's fixed by upgrading the linter to the latest version, I would ignore it (like human ignore) and not add anything to keep the diff to a minimum.
nlordell
approved these changes
Jul 22, 2024
oops, I absolutely missed it - but I think a follow up PR is better anyway |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR implements #781.
It migrates the migration contracts from:
I had to refactor tests quite a bit because a lot has changed since the release of 1.4.1 like we added typechain types, migrated to ethers v6, etc