Skip to content

feat: add support for consensys zkevm network (Linea)#1126

Closed
VGau wants to merge 3 commits intoMetaMask:mainfrom
VGau:feat/add-consensys-zkevm-support
Closed

feat: add support for consensys zkevm network (Linea)#1126
VGau wants to merge 3 commits intoMetaMask:mainfrom
VGau:feat/add-consensys-zkevm-support

Conversation

@VGau
Copy link
Copy Markdown
Contributor

@VGau VGau commented Mar 13, 2023 •
edited
Loading

PR Title
Added support for ConsenSys zkEVM test network (Linea).

Description
It was added ConsenSys zkEVM test network (Linea) support to our Network Controller.

  • CHANGED:
    • The network controller has been updated to support Consensys zkEVM (Linea)
    • New test cases have been added to use ConsenSys zkEVM network (Linea)

Checklist

  • Tests are included if applicable
  • Any added code is fully documented

@VGau VGau requested a review from a team as a code owner March 13, 2023 16:16
@VGau VGau mentioned this pull request Mar 13, 2023
Merged
3 tasks
@VGau VGau changed the title feat: add support for consensys zkevm network feat: add support for consensys zkevm network (Linea) Mar 14, 2023
@VGau VGau force-pushed the feat/add-consensys-zkevm-support branch from ce939fd to 2a32fdd Compare March 15, 2023 13:18
sethkfman
sethkfman previously approved these changes Mar 16, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@sethkfman sethkfman left a comment

Choose a reason for hiding this comment

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

LGTM

mcmire
mcmire reviewed Mar 16, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

This makes sense. Looks like there's one block of tests missing though. I think you'll want to copy the describe starting at line 2222 to a new one and change instances of "localhost" to "lineatestnet".

sethkfman
sethkfman previously approved these changes Mar 17, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@sethkfman sethkfman left a comment

Choose a reason for hiding this comment

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

Reviewed typo. LGTM

@sethkfman
Copy link
Copy Markdown
Contributor

sethkfman commented Mar 17, 2023

@mcmire Can you confirm?

mcmire
mcmire previously approved these changes Mar 17, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

Yes, LGTM!

@VGau VGau dismissed stale reviews from mcmire and sethkfman via 35f4930 March 18, 2023 15:06
@VGau VGau force-pushed the feat/add-consensys-zkevm-support branch from 7ed667d to 35f4930 Compare March 18, 2023 15:06
@mcmire
Copy link
Copy Markdown
Contributor

mcmire commented Mar 20, 2023

@Gudahtt I notice you left a comment in Slack about this PR, are you good with this?

@VGau
Copy link
Copy Markdown
Contributor Author

VGau commented Mar 20, 2023

@Gudahtt I notice you left a comment in Slack about this PR, are you good with this?

Yes I am good with this PR thanks 😄

@Gudahtt
Copy link
Copy Markdown
Member

Gudahtt commented Mar 23, 2023

Nope, no objections. But I will add a DO-NOT-MERGE label for now because since this was approved, the strategy used to support this network on mobile has changed. We need to revisit whether we need a change to the network controller at all, or whether this can be handled solely in mobile instead.

@mcmire
Copy link
Copy Markdown
Contributor

mcmire commented Jul 21, 2023 •
edited
Loading

Now that #1423 has been merged, should we close this?

@Gudahtt
Copy link
Copy Markdown
Member

Gudahtt commented Jun 24, 2024

I'm assuming we don't need this anymore; if it turns out we do, we can open a new PR.

@Gudahtt Gudahtt closed this Jun 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcmire mcmire mcmire approved these changes

+1 more reviewer

@sethkfman sethkfman sethkfman left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@VGau @sethkfman @mcmire @Gudahtt