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: remove all references to L1Network #453

Merged
merged 54 commits into from
Apr 18, 2024
Merged

feat: remove all references to L1Network #453

merged 54 commits into from
Apr 18, 2024

Conversation

spsjvc
Copy link
Member

@spsjvc spsjvc commented Apr 17, 2024

Closes FS-451.

This PR:

  • Removes the Network and L1Network types, leaving only ArbitrumNetwork (also removes all references to L1Network)
  • Removes isArbitrum from ArbitrumNetwork
  • Updates addCustomNetwork so only the ArbitrumNetwork is necessary

Base automatically changed from feat-networks-change-2.1 to v4 April 17, 2024 19:31
await SignerProviderUtils.checkNetworkMatches(sop, this.parentChain.chainID)
await SignerProviderUtils.checkNetworkMatches(
sop,
this.childChain.parentChainId
Copy link
Member Author

Choose a reason for hiding this comment

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

Removed the parent chain reference, but we only need the chain id anyway

@@ -443,10 +443,10 @@ export const skipIfMainnet = (() => {
let chainId: number
return async (testContext: Mocha.Context) => {
if (!chainId) {
const { parentChain } = await testSetup()
chainId = parentChain.chainID
const { childChain } = await testSetup()
Copy link
Member Author

Choose a reason for hiding this comment

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

The only usage of parentChain from testSetup was to detect if it was mainnet, so we check the child

if (!l1Network) {
throw new Error(`Unrecognised parent chain id: ${l2Network.parentChainId}`)
}
const l2Network = await getL2Network(l2NetworkID)
Copy link
Member Author

Choose a reason for hiding this comment

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

Got rid of l1Network, and validate the l2Network by calling getL2Network

@@ -36,17 +40,18 @@ function getLocalNetworksFromContainer(which: 'l1l2' | 'l2l3'): any {
* we can remove this patchwork
*/
async function patchNetworks(
Copy link
Member Author

Choose a reason for hiding this comment

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

Adjusted this function so instead of mutating in-place it returns new objects

@spsjvc spsjvc changed the title feat: clean up networks 3 feat: remove L1Network Apr 18, 2024
@spsjvc spsjvc requested a review from douglance April 18, 2024 08:15
@spsjvc spsjvc changed the title feat: remove L1Network feat: remove all references to L1Network Apr 18, 2024
@spsjvc spsjvc marked this pull request as ready for review April 18, 2024 08:21
Copy link
Contributor

@douglance douglance left a comment

Choose a reason for hiding this comment

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

amazing.

I checked the teleporter PR and we should be good with these changes when we merge that one up into v4.

only change I'd suggest is moving the Prettify type into utls since it could be used elsewhere.

isArbitrum: false
}
// https://twitter.com/mattpocockuk/status/1622730173446557697
export type Prettify<T> = {
Copy link
Contributor

Choose a reason for hiding this comment

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

niiice matt's awesome. I think there might be a better spot for this in /utils?

Copy link
Member Author

Choose a reason for hiding this comment

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

Moved in 2f080da

/**
* Id of the chain.
*/
chainID: number
Copy link
Contributor

Choose a reason for hiding this comment

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

it'd be nice to standardize capitalization of ID before publishing v4

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes ser, it's part of #458

}
if (chainId === 1) {
if (chainId === 42161 || chainId === 42170) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this list won't be exhaustive when we start testing L2 orbit chains

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we're good for now, those are the only 2 Arbitrum chains on mainnet (assuming no custom networks registered)

@spsjvc spsjvc merged commit 33df534 into v4 Apr 18, 2024
24 checks passed
@spsjvc spsjvc deleted the feat-networks-change-3 branch April 18, 2024 13:00
This pull request was closed.
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.

2 participants