Feature: 1.5.0 Migration #652
Conversation
mmv08
requested review from
a team,
rmeissner,
Uxio0 and
akshay-ap
and removed request for
a team
|
As the 1.5.0 is not finalized I created a follow-up issue: #653 |
mmv08
force-pushed
the
feature/1.5.0-migration
branch
from
f16b3b3
to
01ab5e8
Compare
Pull Request Test Coverage Report for Build 6274960393
💛 - Coveralls |
| * @param guard The address of the new guard contract. | ||
| */ | ||
| function migrateL2WithSetGuard(address guard) public { | ||
| require(address(this) != MIGRATION_SINGLETON, "Migration should only be called via delegatecall"); |
There was a problem hiding this comment.
Do I assume correctly that you do not use migrateL2Singleton() to avoid the call to checkGuard();
| import { AddressZero } from "@ethersproject/constants"; | ||
| import { getSafeWithSingleton, migrationContractTo150, getSafeSingletonAt, getMock } from "../utils/setup"; | ||
| import deploymentData from "../json/safeDeployment.json"; | ||
| import safeRuntimeBytecode from "../json/safeRuntimeBytecode.json"; |
There was a problem hiding this comment.
We use a separate file for the bytecode (instead of the build artefacts) to make the tests work also for future versions?
There was a problem hiding this comment.
Hmm, I was torn because there are no guarantees that the next version will be compatible with the upgrade contract
| import safeRuntimeBytecode from "../json/safeRuntimeBytecode.json"; | ||
| import { executeContractCallWithSigners } from "../../src/utils/execution"; | ||
|
|
||
| const SAFE_SINGLETON_150_ADDRESS = "0x88627c8904eCd9DF96A572Ef32A7ff13b199Ed8D"; |
There was a problem hiding this comment.
Shouldn't we be able to calculate this based on the runtime bytecode?
There was a problem hiding this comment.
We can, but I'm not sure why, since after 1.5.0 is released and the addresses known this doesn't have to be adjusted any more
| const invalidGuardMock = await getMock(); | ||
| await invalidGuardMock.givenCalldataReturnBool(guardEip165Calldata, false); | ||
|
|
||
| const migration = await (await migrationContractTo150()).deploy(); |
There was a problem hiding this comment.
Why don't we deploy the migration contract as part of the deployment scripts and then reused the deterministic instance of it?
There was a problem hiding this comment.
I thought about doing that, but I couldn't come up with a neat way to do this because it depends on the singleton addresses, which either have to be computed depending on the runtime bytecode (doesn't work for the future contract updates well) or hardcoding the addresses (they're not known yet, because the 1.5.0 contract is not released). I plan on adding it to the deploy scripts with hardcoded addresses alongside the 1.5.0 release.
|
|
||
| const migration = await (await migrationContractTo150()).deploy(); | ||
| return { | ||
| safe130: await getSafeWithSingleton(singleton130, [user1.address]), |
There was a problem hiding this comment.
Should we have a test that uses a different type of proxy?
There was a problem hiding this comment.
I added tests that use an eip-1967 proxy by openzeppelin and a modifier to every migrate function that verifies that the address stored at slot 0 is a contract. This also allowed the removal of an address check to require the library to be delegatecalled.
mmv08
force-pushed
the
feature/1.5.0-migration
branch
2 times, most recently
from
88c0d8a
to
f84f71d
Compare
…ture/1.5.0-migration
mmv08
force-pushed
the
feature/1.5.0-migration
branch
from
f84f71d
to
11e9837
Compare
|
Question: Should we revisit the 1.4.1 migration contract and add the same checks as here that the address stored in the slot 0 is a contract? |
This PR focuses on adding a new migration contract for Safe Upgrade to version 1.5.0. The notable changes include:
Safe150Migration.solcontract for Safe Upgrade to 1.5.0migrationContractTo150function insetup.tsSafe150MigrationinSafe150Migration.spec.tsThe functions available in Safe150Migration.sol are:
constructor(): Initializes the migrationSingleton with the contract's own address.migrateSingleton(): Migrates to Safe 1.5.0 Singleton (L1) atSAFE_150_SINGLETON.migrateWithFallbackHandler(): Migrates and sets the fallback handler to Safe 1.5.0 Compatibility Fallback Handler.migrateWithSetGuard(address guard): Migrates and sets the guard to the specified address.migrateWithSetGuardAndFallbackHandler(address guard): Migrates, sets the guard to the specified address, and sets the fallback handler to Safe 1.5.0 Compatibility Fallback Handler.migrateL2Singleton(): Migrates to Safe 1.5.0 Singleton (L2) atSAFE_150_SINGLETON_L2.migrateL2WithFallbackHandler(): Migrates to Safe 1.5.0 Singleton (L2) and sets the fallback handler to Safe 1.5.0 Compatibility Fallback Handler.migrateL2WithSetGuard(address guard): Migrates to Safe 1.5.0 Singleton (L2) and sets the guard to the specified address.migrateL2WithSetGuardAndFallbackHandler(address guard): Migrates to Safe 1.5.0 Singleton (L2), sets the guard to the specified address, and sets the fallback handler to Safe 1.5.0 Compatibility Fallback Handler.getGuard(): Gets the address of the current guard.isContract(address account): Checks whether an Ethereum address corresponds to a contract or an externally owned account (EOA).