Skip to content

[FIX] - Allow Optimism & Optimism Testnet to be added via wallet_addEthereumChain #910

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Sep 22, 2022
Merged

[FIX] - Allow Optimism & Optimism Testnet to be added via wallet_addEthereumChain #910

merged 4 commits into from
Sep 22, 2022
+8 −12

Conversation

sethkfman
Copy link
Contributor

@sethkfman sethkfman commented Sep 14, 2022 •
edited
Loading

PR Title

Allow Optimism & Testnet to be added via wallet_addEthereumChain

Description

  • Because optimism was added in the default network list the wallet_addEthereumChain method fails when adding Optimism or the Optimism Testnet. These networks were initially added to identify them as custom network but through further testing the Optimism network is a custom network since it gets added using the setupStandardProvider method.

  • FIXED:

    • This fixes the issue of adding Optimism using the wallet_addEthereumChain method.

  • REMOVED:

    • Remove Optimism and the Optimism Testnet

Checklist

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

Issue

Resolves 4492

Sorry, something went wrong.

@sethkfman sethkfman requested a review from a team as a code owner September 14, 2022 21:06
@sethkfman sethkfman changed the title [FIX] - Allow Optimism & Test Net to be added via wallet_addEthereumChain [FIX] - Allow Optimism & Testnet to be added via wallet_addEthereumChain Sep 14, 2022
blackdevelopa
blackdevelopa previously approved these changes Sep 15, 2022
View reviewed changes
Copy link
Contributor

@blackdevelopa blackdevelopa left a comment

Choose a reason for hiding this comment

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

LGTM. Removes Optimism and its testnet to be added via wallet_addEthereumChain.

Test seem to be failing here. src/transaction/TransactionController.test.ts:139:32 - error TS2339: Property 'optimism' does not exist on type 'typeof NetworksChainId'.

@sethkfman sethkfman changed the title [FIX] - Allow Optimism & Testnet to be added via wallet_addEthereumChain [FIX] - Allow Optimism & Optimism Testnet to be added via wallet_addEthereumChain Sep 15, 2022
mcmire
mcmire reviewed Sep 19, 2022
View reviewed changes
Copy link
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 to me. That said, the type on line 46 of src/constants.ts doesn't seem right now, as the object doesn't include all of the properties of NetworkType, only a subset. I think we should fix that.

@Gudahtt
Copy link
Member

Gudahtt commented Sep 21, 2022 •
edited
Loading

@mcmire could you elaborate? It looks like NetworkType is an enum union of strings, so it doesn't have any properties. Every network type appears to be present in that constant as well. Maybe I'm missing something.

@mcmire
Merge branch 'main' into fix/remove-optimisim-networks-default-list

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
GPG key ID: 4AEE18F83AFDEB23
Expired
Verified
Learn about vigilant mode
cee3189
@mcmire
Copy link
Contributor

mcmire commented Sep 22, 2022

@Gudahtt Oops, sorry, I might have been looking at something else. That looks right to me after all.

mcmire
mcmire approved these changes Sep 22, 2022
View reviewed changes
Copy link
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.

LGTM!

@sethkfman
Merge branch 'main' into fix/remove-optimisim-networks-default-list

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
GPG key ID: 4AEE18F83AFDEB23
Expired
Verified
Learn about vigilant mode
2476203
@sethkfman sethkfman merged commit b4fa756 into main Sep 22, 2022
@sethkfman sethkfman deleted the fix/remove-optimisim-networks-default-list branch September 22, 2022 21:36
gantunesr pushed a commit that referenced this pull request Dec 8, 2022
@sethkfman @mcmire @gantunesr
[FIX] - Allow Optimism & Optimism Testnet to be added via wallet_addE…
2e71571 
…thereumChain (#910)

* removed the explict addition of optimism to default networks and updated test cases

* updated test case mocks

Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>
MajorLift pushed a commit that referenced this pull request Oct 11, 2023
@sethkfman @mcmire @MajorLift
[FIX] - Allow Optimism & Optimism Testnet to be added via wallet_addE…

Verified

This commit was signed with the committer’s verified signature.
MajorLift Jongsun Suh
GPG key ID: 06207DE49641BC5E
Verified
Learn about vigilant mode
bed8103 
…thereumChain (#910)

* removed the explict addition of optimism to default networks and updated test cases

* updated test case mocks

Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>
MajorLift pushed a commit that referenced this pull request Oct 11, 2023
@sethkfman @mcmire @MajorLift
[FIX] - Allow Optimism & Optimism Testnet to be added via wallet_addE…

Verified

This commit was signed with the committer’s verified signature.
MajorLift Jongsun Suh
GPG key ID: 06207DE49641BC5E
Verified
Learn about vigilant mode
0b9ac9f 
…thereumChain (#910)

* removed the explict addition of optimism to default networks and updated test cases

* updated test case mocks

Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcmire mcmire

@Gudahtt Gudahtt

@blackdevelopa blackdevelopa

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Can't add Optimism network on MetaMask mobile
4 participants
@sethkfman @Gudahtt @mcmire @blackdevelopa