Skip to content

Conversation

akshay-ap
Copy link
Member

@akshay-ap akshay-ap commented Jul 16, 2024

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).

  • 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.

Changes in PR

  • 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.

@akshay-ap akshay-ap self-assigned this Jul 16, 2024
@akshay-ap akshay-ap marked this pull request as draft July 16, 2024 08:37
@coveralls
Copy link

coveralls commented Jul 16, 2024

Pull Request Test Coverage Report for Build 10010745656

Details

  • 20 of 20 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.5%) to 93.478%

Totals Coverage Status
Change from base Build 9780230154: 0.5%
Covered Lines: 386
Relevant Lines: 398

💛 - Coveralls

@akshay-ap akshay-ap changed the title Migration contract Generic migration contract Jul 16, 2024
@akshay-ap akshay-ap marked this pull request as ready for review July 16, 2024 13:40
@akshay-ap akshay-ap requested review from nlordell, mmv08 and remedcu July 16, 2024 13:42
Co-authored-by: Nicholas Rodrigues Lordello <n@lordello.net>
Copy link
Collaborator

@nlordell nlordell left a 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: Mikhail <16622558+mmv08@users.noreply.github.com>
Copy link
Collaborator

@nlordell nlordell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

akshay-ap and others added 4 commits July 17, 2024 16:17
Co-authored-by: Nicholas Rodrigues Lordello <n@lordello.net>
Use BigInt instead of ethers.toBigInt
akshay-ap and others added 4 commits July 19, 2024 10:18
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>
Copy link
Member

@remedcu remedcu left a 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 👍🏾

@akshay-ap akshay-ap merged commit b6692f9 into main Jul 19, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Jul 19, 2024
@akshay-ap akshay-ap deleted the feature-787-migration-contract branch July 19, 2024 16:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generalize Migration Contract
6 participants