-
Notifications
You must be signed in to change notification settings - Fork 190
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
feat: add ParentChain
and Chain
to support Orbit chains
#312
Conversation
src/lib/inbox/inbox.ts
Outdated
@@ -90,7 +90,13 @@ export class InboxTools { | |||
|
|||
// we take a long average block time of 14s | |||
// and always move at least 10 blocks | |||
const diffBlocks = Math.max(Math.ceil(diff / this.l1Network.blockTime), 10) | |||
let diffBlocks = 10 | |||
if ((this.l1Network as L1Network).blockTime) { |
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.
L2Network
does not have blockTime
. I'm not sure if we need to add blockTime
to every L2 network that is a parent chain to L3?
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.
see #319 for an attempted fix
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.
I didn't include this fix because it's out of scope of this PR and the bridge UI doesn't require it.
I can create a separate PR to add this.
@@ -28,10 +28,14 @@ export interface L1Network extends Network { | |||
} |
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.
isArbitrum
can now be true
(so boolean
) for L1Network (parent chain)
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.
It will be left as false
here because L1Network
doesn't include Arbitrum chains. If an Arbitrum chain is a parent chain, it will be part of the ParentChain
.
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.
#319 added a util method to detect if a chain is Arbitrum chain runtime
ParentChain
and Chain
to support Orbit chains
ParentChain
and Chain
to support Orbit chainsParentChain
and Chain
to support Orbit chains
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.
It would probably be useful to have couple of tests for adding/fetching chains with their parents
…o support-l2-parent-chains
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.
lgtm
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.
Works, but there are some concern about increased code debt (e.g. usage of e.g. partnerChainIDs
to infer L2 parent chain and newly exported type Chain/ParentChain) before we can refactoring everything from L1/L2 to Parent/Child chain.
Moving to draft as it's in beta, not merging right now |
…abs/arbitrum-sdk into support-l2-parent-chains
…o support-l2-parent-chains
Closed in favor of #379 |
ParentChains
consist of:partnerChainIDs
(are a parent to at least one Orbit chain)Chains
is the same asL2Networks
(for now), but they consist of L2 networks and Orbit chains.With
ParentChains
andChains
we can manage Orbit chains and their parent chains without any breaking changes to L1 and L2 networks.