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

interop: add Dependency Set Manager & Shared Lockbox design doc #84

Merged
merged 10 commits into from
Nov 21, 2024

Conversation

Joxess
Copy link
Contributor

@Joxess Joxess commented Sep 18, 2024

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.

@K-Ho
Copy link

K-Ho commented Sep 20, 2024

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

@K-Ho
Copy link

K-Ho commented Sep 20, 2024

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

@Joxess
Copy link
Contributor Author

Joxess commented Sep 20, 2024

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, OptimismPortal isn't the only data to reference anymore; it is also necessary to consider the new contributions whenever ETH enters or exits in the form of SuperchainWETH. Probably, similar reasoning as in the case of a migration (deposit mapping + SuperchainWETH net flows) will need to be followed.

protocol/eth-shared-lockbox.md Outdated Show resolved Hide resolved
protocol/eth-shared-lockbox.md Outdated Show resolved Hide resolved
protocol/eth-shared-lockbox.md Outdated Show resolved Hide resolved
protocol/eth-shared-lockbox.md Show resolved Hide resolved
protocol/eth-shared-lockbox.md Show resolved Hide resolved
@Joxess Joxess changed the title interop: add ETH Shared Lockbox design doc interop: add Dependency Set Manager & Shared Lockbox design doc Oct 30, 2024
@Joxess
Copy link
Contributor Author

Joxess commented Oct 30, 2024

I have updated the doc with substantial changes:

  • Updates to the Considerations section
  • Introduction of the DependencySetManager contract as a replacement for the old "Joining the Cluster" section
  • Proposed changes to SystemConfigInterop
  • Modifications to the upgrade process
  • Removal of the "Opt-out of the Cluster" section and liquidity trackability features

protocol/eth-shared-lockbox.md Outdated Show resolved Hide resolved
protocol/eth-shared-lockbox.md Outdated Show resolved Hide resolved
protocol/eth-shared-lockbox.md Outdated Show resolved Hide resolved
protocol/eth-shared-lockbox.md Outdated Show resolved Hide resolved
protocol/eth-shared-lockbox.md Outdated Show resolved Hide resolved
protocol/eth-shared-lockbox.md Outdated Show resolved Hide resolved
protocol/eth-shared-lockbox.md Outdated Show resolved Hide resolved
protocol/eth-shared-lockbox.md Outdated Show resolved Hide resolved
protocol/eth-shared-lockbox.md Outdated Show resolved Hide resolved
@zainbacchus
Copy link

@tynes
Copy link
Contributor

tynes commented Nov 4, 2024

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 L2ToL2CrossDomainMessenger, but this would have implications about the topology about all possible chain clusters. I do not love the idea of enforcing that all possible clusters MUST be a mesh

@tynes
Copy link
Contributor

tynes commented Nov 4, 2024

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

@tynes
Copy link
Contributor

tynes commented Nov 6, 2024

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 L2ToL2CrossDomainMessenger, but this would have implications about the topology about all possible chain clusters. I do not love the idea of enforcing that all possible clusters MUST be a mesh

Lets not worry about this as it adds scope and we can revisit in the future


### 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`.
Copy link
Contributor

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

Copy link
Contributor

@agusduha agusduha Nov 19, 2024

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

Copy link
Contributor

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

Copy link
Contributor

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;
Copy link
Contributor

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.
Copy link
Contributor

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)

Copy link
Contributor

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?

Copy link
Contributor

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

@tynes
Copy link
Contributor

tynes commented Nov 18, 2024

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

@tynes
Copy link
Contributor

tynes commented Nov 19, 2024

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.

@tynes
Copy link
Contributor

tynes commented Nov 21, 2024

This is now good to move on to the spec phase

@tynes tynes merged commit 3e8fa01 into ethereum-optimism:main Nov 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants