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

feat: add ParentChain and Chain to support Orbit chains #312

Closed
wants to merge 44 commits into from

Conversation

brtkx
Copy link
Contributor

@brtkx brtkx commented Aug 2, 2023

ParentChains consist of:

  • all L1 networks
  • L2 networks that have at least one chain in partnerChainIDs (are a parent to at least one Orbit chain)

Chains is the same as L2Networks (for now), but they consist of L2 networks and Orbit chains.

With ParentChains and Chains we can manage Orbit chains and their parent chains without any breaking changes to L1 and L2 networks.

@cla-bot cla-bot bot added the cla-signed label Aug 2, 2023
@@ -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) {
Copy link
Contributor Author

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?

Copy link
Member

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

Copy link
Contributor Author

@brtkx brtkx Aug 23, 2023

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.

@brtkx brtkx marked this pull request as ready for review August 4, 2023 07:42
src/lib/dataEntities/networks.ts Outdated Show resolved Hide resolved
src/lib/dataEntities/networks.ts Outdated Show resolved Hide resolved
src/lib/dataEntities/networks.ts Outdated Show resolved Hide resolved
src/lib/dataEntities/networks.ts Outdated Show resolved Hide resolved
src/lib/dataEntities/networks.ts Outdated Show resolved Hide resolved
@@ -28,10 +28,14 @@ export interface L1Network extends Network {
}
Copy link
Member

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)

Copy link
Contributor Author

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.

Copy link
Member

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

@brtkx brtkx changed the title support L2 parent chains chore: add ParentChain and Chain to support Orbit chains Aug 11, 2023
@brtkx brtkx changed the title chore: add ParentChain and Chain to support Orbit chains feat: add ParentChain and Chain to support Orbit chains Aug 17, 2023
Copy link

@gvladika gvladika left a 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

src/lib/dataEntities/networks.ts Show resolved Hide resolved
src/lib/dataEntities/networks.ts Show resolved Hide resolved
src/lib/dataEntities/networks.ts Outdated Show resolved Hide resolved
src/lib/dataEntities/networks.ts Show resolved Hide resolved
Copy link

@gvladika gvladika left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Member

@gzeoneth gzeoneth left a 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.

@brtkx brtkx marked this pull request as draft August 28, 2023 12:12
@brtkx
Copy link
Contributor Author

brtkx commented Aug 28, 2023

Moving to draft as it's in beta, not merging right now

@yahgwai yahgwai removed their request for review October 5, 2023 09:01
@douglance
Copy link
Contributor

Closed in favor of #379

@douglance douglance closed this Jan 10, 2024
@spsjvc spsjvc deleted the support-l2-parent-chains branch February 7, 2024 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants