-
Notifications
You must be signed in to change notification settings - Fork 23
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
interop: add Dependency Set Manager & Shared Lockbox design doc #84
interop: add Dependency Set Manager & Shared Lockbox design doc #84
Conversation
Co-authored-by: ng <[email protected]>
From a product perspective - the ETH shared lockbox design solves a lot of user pain, by maintaining the invariant that "any amount of ETH can always be withdrawn from L2 to L1 safely", which many protocols have built upon. I think the only concern at this point is the time and resources required to actually ship this securely. Is there some rough scoping of the amount of time this adds to the engineering timeline for interoperable ETH? Who would we need review/support from to feel confident in the security of this upgrade? It's certainly a major change and would want to make sure we get a diverse set of reviews/eyes on this + put together a very thorough Failure Mode Analysis |
Also a stream of work will be making sure that external tooling that relies on the OptimismPortal to calculate ETH stored in a chain is updated as well (e.g. update L2Beat, Defillama, etc.) |
This is a very interesting consideration. Actually, when making ETH interoperable, a shift in the interpretation of liquidity distribution is needed. To show the actual TVL, |
I have updated the doc with substantial changes:
|
|
One consideration now that we have a contract for managing the dependency set - should we make the full dependency set legible from within each L2? This would prevent the need to send to chains that do not have the local chain in their dependency set We could assume this by observing the dependency set locally when sending a message through the |
We need to align on whether or not we want to be supporting non-complete graphs in the dependency set. We can make assumptions that it must be a full mesh. Right now we do not make this assumption |
Lets not worry about this as it adds scope and we can revisit in the future |
protocol/eth-shared-lockbox.md
Outdated
|
||
### Managing op-governed dependency set through `SuperchainConfig` | ||
|
||
The `SuperchainConfig` contract will serve as the single point for managing the dependency set of a cluster and will be managed by the admin as occurs in other L1 OP contracts. This contract assumes the role of `dependencyManager` for every `SystemConfig` contract involved. Since the dependency graph follows a simple dependency, it only stores a mapping (or array) of chains added, e.g. given a `chainId`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could simplify and remove the concept of dependencyManager
and instead just hardcode it to be the SuperchainConfig
. Unclear what the tradeoffs are here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change involves hardcoding the current L1 SuperchainConfig
address in the SystemConfigInterop
contract and removing dependencyManager
as an initialize
parameter.
Pros:
- Reduces code complexity.
- Simplifies the migration process by minimizing steps, avoiding the need to reset the
initialized
slot.
Cons:
- Reduces flexibility.
- New chains deployed through the OPCM will share the same
SuperchainConfig
, which might not be desirable for clusters outside the Superchain.
About cons, we always tend to think about future clusters but we are not sure if we should do it and we will like to know your vision on this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets enshrine the SuperchainConfig
rather than having a dependencyManager
, this will enforce the assumptions in ethereum-optimism/specs#460 and make the core product always be a highly connected graph. It makes no sense having another topology until transitive deps don't need to be ran to fully validate the network. This also lets us remove a role from the SystemConfig
, we may need to move it to the SuperchainConfig
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Depending on the design of OPCM, it may mean that there is 1 OPCM per cluster, if the SuperchainConfig
is immutable
|
||
```solidity | ||
// Mapping from chainId to SystemConfig address | ||
mapping(uint256 _chainId => ISystemConfig) public systemConfigs; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keeping a mapping of chain id to system configs is related to standardizing on how L2s put their chain configs on chain, as mentioned at eth interop forum. My guess is that we will need to use the interface we are defining now to populate a standardized interface in the future
|
||
1. The `updater` calls the function with `chainId` and `SystemConfig`. | ||
2. An external call is made to each `SystemConfig` from already added chains with the new `chainId`. | ||
3. The new `SystemConfig` is called to add all previously existing `chainId` from `SuperchainConfig` to itself. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would we need to enforce via social that the dependency set is empty before addChain
is called? Perhaps we keep dependency set size in the SystemConfig
for this reason, so you can require(config.depSetSize() == 0)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We would need to track the dependency set size for each chain in SystemConfig
, updating it by adding or subtracting in the addDependency
and removeDependency
functions. The size check will apply only to the new chain being added at addChain
in the SuperchainConfig
. Does this approach make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes this makes sense, its double storage on L1 since the set is stored in both sys config + superchain config under this model. Not the end of the world but we generally need a way to make sure that the dependency set size is 0 before the chain joins
In favor of this as its designed. Nice work! There is definitely going to be some integration work with OP Stack Contracts Manager and the upgrade process, but nothing sticks out as being blocking based on the latest OPCM + upgrades designs |
Regarding #84 (comment), there is some unscoped work around guaranteeing that there is the same respected game type set for all chains, but that can be ignored for now and scoped as part of the proof design. |
This is now good to move on to the spec phase |
Co-authored-by: ng [email protected], agusduha [email protected]
Description
The following design doc cover a previous discussed issue on ETH withdrawals introduced by interop. The Shared lockbox is presented as the most convenient solution to solve this problem. However, it is known that the design also is the interest for various aspects of the Superchain design, such as chain management, core interop, and others, which need to be carefully discussed.
Additional context
This doesn't address the case for L1 bridged tokens.