Skip to content
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

OPSM: Refactor to better support features with different inputs #11783

Closed
protolambda opened this issue Sep 6, 2024 · 3 comments · Fixed by #11833 or #11854
Closed

OPSM: Refactor to better support features with different inputs #11783

protolambda opened this issue Sep 6, 2024 · 3 comments · Fixed by #11833 or #11854
Assignees
Labels

Comments

@protolambda
Copy link
Contributor

Support the initialize call of the SystemConfigInterop that is deployed in DeployImplementationsInterop, by introducing an OPStackManagerInterop and figuring out how to approach an input-struct that is compatible between interop and the inherited stack-manager (e.g. through composition, or dropping the struct pattern).

@mds1
Copy link
Contributor

mds1 commented Sep 6, 2024

We have agreed that structs for inputs are going to be messy, and not scale well as feature develop continues changing inputs. Therefore, we should remove input structs and move to a setter approach like what we do with the output contracts. (The output contracts do also contain structs that we should consider removing as well, in case this simplifies cases where outputs change). That refactor will take a bit of time, however.

Therefore to avoid blocking interop we will first support the interop contracts using the current struct-based input approach. Instead of doing it the "proper" (messy) way with new input structs, we just assume the dependency manager role is the same as the proxyAdminOwner and therefore the inputs don't need to change. For example, see:

// TODO For now we assume that the dependency manager is the same as the proxy admin owner.
// This is currently undefined since it's not part of the standard config, so we may need
// to update where this value is pulled from in the future. If we want to support a
// different dependency manager in this contract without an invasive change of redefining
// the `Roles` struct, we can pull this from "hidden data". Solidity allows excess
// calldata to be appended, so while in development we could simply require an extra
// 32 bytes to be appended to the calldata and then do:
//
// address dependencyManager = abi.decode(msg.data[msg.data.length - 32:], (address))
//
// And you don't even have really need to ABI encode the address, and can just append 20 bytes:
//
// address dependencyManager = address(uint160(uint256(msg.data[msg.data.length - 20:])))
//
// However, once closer to production and preparing the release candidate we must properly
// update the Roles struct.
address dependencyManager = address(_input.roles.opChainProxyAdminOwner);

Afterwards, we'll refactor the deploy scripts and their corresponding IO contracts to remove structs and use a setter based approach

@mds1 mds1 changed the title OPSM: Support SystemConfigInterop OPSM: Refactor to better support features with different inputs Sep 6, 2024
@mds1
Copy link
Contributor

mds1 commented Sep 6, 2024

I've updated the title here, since interop is now unblocked by our workaround. A good way to verify this refactor is successful is to enable the interop dependencyManager to be set (we currently hardcode it), and ensure the diff is simple

@mds1
Copy link
Contributor

mds1 commented Sep 11, 2024

#11833 did not full resolve this, so reopening

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment