Add Migration.sol action upgrade contract #15
Conversation
src/espresso-migration/Migration.sol
Outdated
| import "@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol"; | ||
| import "@openzeppelin/contracts/utils/Address.sol"; | ||
|
|
||
| contract{ |
There was a problem hiding this comment.
I think you are missing the name of the contract here
…esso mode at the end of migration.
src/espresso-migration/Migration.sol
Outdated
| address currentOspEntry | ||
| ) public{ | ||
| //Handle assertions in the perform functoin as we shouldn't be storing local state for delegated calls. | ||
| require(newWasmModuleRoot != bytes32(0), "_newWasmModuleRoot cannot be empty"); |
There was a problem hiding this comment.
nit: I think we should use custom errors to save gas as we have quite a lot of require's here
There was a problem hiding this comment.
I agree that this would be rather helpful to reduce the gas used.
There was a problem hiding this comment.
I think for these contracts the gas usage is not of much concern because each chain only does the upgrade once, and we can just go for whatever is simplest to write.
| import "../precompiles/ArbOwner.sol"; | ||
|
|
||
| contract SetEspressoChainConfig { | ||
| function perform(string calldata serialiazedEspressoConfig) public { |
There was a problem hiding this comment.
can anyone call this function or should we add some access control here?
There was a problem hiding this comment.
The setChainConfig function has built in access control, but I think it could be beneficial to add some additional access control to this function as well.
I think a good approach would be to add a custom error and a revert based on the ArbOwner.isChainOwner function.
src/espresso-migration/Migration.sol
Outdated
| import "@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol"; | ||
| import "@openzeppelin/contracts/utils/Address.sol"; | ||
|
|
||
| contract Migration{ |
There was a problem hiding this comment.
Should this be named something like ChallengeManagerUpgrader or can it upgrade other contract as well?
There was a problem hiding this comment.
I agree that the contract should be renamed. Likely in line with README.md from this repo
…duleRoot` in the migration contract
…by the ArbOS upgrade
…he migration to espresso.
|
|
||
| import "../precompiles/ArbOwner.sol"; | ||
|
|
||
| contract ScheduleEspressoUpgrade { |
There was a problem hiding this comment.
Have you considered re-using what's here: https://github.com/OffchainLabs/orbit-actions/tree/main/scripts/foundry/arbos-upgrades/at-timestamp? I wonder if it would make sense to fork the orbit-actions repo instead of putting the contracts into this repo.
There was a problem hiding this comment.
I think it might be beneficial to have a fork of the orbit-actions repo for the migration. I don't believe we need to include them in the integration repo like I had originally expected.
If we do decide to move these contracts to a fork of the orbit-action repo we can just close this PR correct?
There was a problem hiding this comment.
I made a fork here: https://github.com/EspressoSystems/orbit-actions i think you can make a PR in that repo and reference this PR in the new one.
| import "@openzeppelin/contracts/utils/Address.sol"; | ||
|
|
||
| contract EspressoMigration{ | ||
| function perform( |
There was a problem hiding this comment.
There are some "constants" that are set in the constructor instead of having everything passed to perform. I think this makes sense because someone can then deploy then deploy a correctly configured action contract (setting the correct wasm root and OSP implementation) and the the rollup owner only needs to pass what is custom to the rollup.
There was a problem hiding this comment.
I agree that we should put as much into the constructor as possible. I suppose the question then becomes what constants we can set in the constructor? To me it seems like we could set the new OSP implememtation address in the constructor, and I'm tempted to say the wasmModuleRoot.
However, there is a case that has me concerned. What if an orbit chain has some pre-existing changes to their STF that they want to merge with ours? If we declare an immutable wasmModuleRoot in the constructor, suddenly we can't use the deployed contract to help upgrade this hypothetical orbit chain as the wasmModuleRoot might be different when merging their changes with ours.
There was a problem hiding this comment.
That rollup would then have to deploy their own upgrade action where they set the root as needed.
| import "@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol"; | ||
| import "@openzeppelin/contracts/utils/Address.sol"; | ||
|
|
||
| contract EspressoMigration{ |
There was a problem hiding this comment.
I think we should also follow the upstream governance guidelines as much as possible.
This PR:
This PR adds an initial upgrade action contract that will point a rollups
ChallengeManagerat a newOSPEntryto allow it to function with the espresso network. Additionally theEspressoMigration.solcontract will point the rollups e to the new value needed after the migration.This PR does not:
Add a test for the scheduled upgrade as that needs to be tested outside of solidity.