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.

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

Optional Chain_id w/o Native ETC Support #5130

Closed
wants to merge 15 commits into from
Closed

Optional Chain_id w/o Native ETC Support #5130

wants to merge 15 commits into from

Conversation

chrisfranko
Copy link

No description provided.

hackmod and others added 10 commits August 21, 2018 13:11
@hackmod
support chainid
8dcf860 
 * fixed custom Rpc form to support optional chainid
 * experimental ETC support added

[fixed conflict 8/16]
@hackmod
setup settings and use correct ticker / blockexploer URLs
110b65b 
 * etc logo added
 * price info from https://min-api.cryptocompare.com/
@hackmod
support shapeshift exchange for deposit
c603f13 
 * fixup for testnet faucet
@hackmod
update rpcList correctly
11a918e 
 * rename frequentRpcList to frequentRpcListDetail to prevent compatible issue
 * increase frequentRpcList length by 1
@hackmod
use ETC Cooperative's api server for ETC
9cbb649
@hackmod
ui: fixed css style
5825556
@chrisfranko
Merge branch 'support-etc-optional-chainid' of https://github.com/hac…
f9f4414 
…kmod/metamask-extension into develop
@chrisfranko
Remove ETC additions
089e84e
@chrisfranko
Merge pull request #2 from chrisfranko/develop
8fa8eaf 
Optional Chain Id
@chrisfranko
Merge branch 'develop' of https://github.com/MetaMask/metamask-extension
2d06784 
 into develop
@bdresser bdresser mentioned this pull request Aug 23, 2018
Closed
8 tasks
@bdresser
Copy link
Contributor

rather than change all currency conversion from Infura to cryptocompare, perhaps we could default to Infura (for standard networks) and only hit cryptocompare if the user has entered a custom chainId.

@danfinlay what do you think? any other concerns about the changes to the block explorer?

@hackmod
Copy link
Contributor

hackmod commented Aug 23, 2018

with predefined ETC network, I can make PR #4932 (formerly known PR #4516) support optional chainId + blockExplorer + symbol + currency conversion using cryptocompare.
I don't think its a big deal to get ETC network merged into trunk. its a routine thing. so that the MetaMask support multi chain properly as already described at the MetaMask blog (multiple network support )
image before ethereum/EIPs#1102 is implemented correctly.

and this PR only remove some part of ETC menu only, but it can be done in the settings menu like as following:
image

@chrisfranko
Copy link
Author

Why dont we just fork and manage our own version with the changes we want? I know it would be ideal to have them in MetaMask but they have made up their mind already. -shrug-

@danfinlay
Copy link
Contributor

Thanks for the PR, I hope to have some time next week to review it.

ghost
ghost approved these changes Aug 24, 2018
View reviewed changes
Copy link

@ghost ghost left a comment •
edited by ghost
Loading

Choose a reason for hiding this comment

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

ACK for the PR, can you please squash your commit into one?

@hackmod hackmod mentioned this pull request Aug 24, 2018
Merged
6 tasks
@hackmod
Copy link
Contributor

hackmod commented Aug 24, 2018

Ive made new PR to separate chain_id stuff correctly at PR #5134.

@bdresser
Copy link
Contributor

bdresser commented Oct 2, 2018

thanks @chrisfranko - closing in favor of #5134

@bdresser bdresser closed this Oct 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alextsg alextsg Awaiting requested review from alextsg

@danjm danjm Awaiting requested review from danjm

@whymarrh whymarrh Awaiting requested review from whymarrh

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
@chrisfranko @bdresser @hackmod @danfinlay