Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Changes from 9 commits
77c818f
7a89e33
258f7de
bb46e30
4c49463
887b022
6424b99
b9ecd91
300d053
d2e4a64
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 theSuperchainConfig
. Unclear what the tradeoffs are hereThere 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 theSystemConfigInterop
contract and removingdependencyManager
as aninitialize
parameter.Pros:
initialized
slot.Cons:
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 adependencyManager
, 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 theSystemConfig
, we may need to move it to theSuperchainConfig
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
isimmutable
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
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 theSystemConfig
for this reason, so you canrequire(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 theaddDependency
andremoveDependency
functions. The size check will apply only to the new chain being added ataddChain
in theSuperchainConfig
. 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