Ethereum Classic Support #1476

sorpaas · 2017-05-23T09:50:18Z

For ETC users who want to test this out, here are the builds. Most of the function works except address/transaction explorer links and buy-eth link.

To fix the address/transaction links, however, there's some complications:

Ethereum and Ethereum Classic uses the same networkId = 1
Ethereum doesn't have a chainId. Ethereum Classic has chainId = 0x3d.
The current transaction signing process in metamask confuses networkId with chainId. chainId is set to always equal to networkId, if I get this correct.
UI links dispatch based on the networkId.

So I think there's some small refactoring needed...

frankiebee · 2017-05-23T14:13:20Z

ui/app/config.js

+    case 'classic':
+      title = 'Current Network'
+      value = 'Ethereum Classic Network'
+


missing break

frankiebee · 2017-05-23T14:26:05Z

Thanks for the PR I appreciate it how ever this will require some discussion with the rest of the MetaMask team.

ProphetDaniel · 2017-05-23T14:52:47Z

One extra functionality I would like to see is: Depending on selected network if Ethereum Classic network is set, Instead of ETH, ETC is displayed. And instead of using Ethereum block explorer, an ETC one would rather be used. I suggest gastracker.io for this task.

danfinlay

Hi there, thanks for your interest. We'd love to support a multi-chain future, and so making these changes with a long-sighted strategy is important to us.

MetaMask uses Infura for all our providers, who we work closely with, and so we trust. We've never added a third party provider before, and adding your provider as a default to our provider menu would not make sense for our current trust model.

We've always been compatible with custom RPC providers, so that is currently the path that we recommend for you. In the future, to avoid rejected work, we recommend opening an issue to discuss code changes before implementing them.

While we're not adding an ETC provider to our provider menu at this time, there are two things we're leaving open for you:

The possibility of adding better ETC support when MetaMask is connected to it. This will need to be implemented in a DRY way, dictated by a config file, not using custom ternary operators throughout the codebase like the current PR does.
The possibility of adding a future method for Dapps, which will allow them to request the user logs in with a custom network, wherein the dapp could define the network they request the user connects to. This concept is still a work in progress, but for a truly multi-chain world, the provider menu will become less and less sustainable, and an increasingly novice user base will be less and less tolerant of the extra configuration per site. For this reason, once Be signed out per domain until signing in #714 is implemented, we will be interested in allowing sites propose the network their user signs in with, which should improve UX dramatically for all chain developers.

I've added a few line-notes, but the basic points are:

No default menu item.
UI conditionals should be chain-agnostic, and easily support RSK or other non-ETH EVM chains as well.

danfinlay · 2017-05-23T16:42:43Z

app/scripts/config.js

@@ -2,6 +2,7 @@ const MAINET_RPC_URL = 'https://mainnet.infura.io/metamask'
 const ROPSTEN_RPC_URL = 'https://ropsten.infura.io/metamask'
 const KOVAN_RPC_URL = 'https://kovan.infura.io/metamask'
 const RINKEBY_RPC_URL = 'https://rinkeby.infura.io/metamask'
+const CLASSIC_RPC_URL = 'http://metamask.epool.io:9999/'


RPC providers should use https for privacy.

danfinlay · 2017-05-23T16:46:34Z

ui/app/actions.js

@@ -871,7 +871,8 @@ function pairUpdate (coin) {
 }

 function shapeShiftSubview (network) {
-  var pair = 'btc_eth'
+  var pair
+  network === 'classic' ? pair = 'btc_etc' : pair = 'btc_eth'


I'm not a fan of having these ternary classic checks all over the place. ETC isn't the only non-ETH EVM blockchain that wants to be MetaMask compatible (Rootstock is another, for example).

Rather than doing this custom coding by hand, it would be nicer to use a general network config file for interpreting chain data into app behavior.

sjmackenzie · 2017-05-23T18:59:16Z

Hi MetaMask devs, sorry if our community ambassador's medium post caused you guys to worry about a fishing attack in MetaMask's name, this is not our intention, we're trying our best to support the ETC community. Thanks so much for writing this great software.

danfinlay · 2017-05-23T19:09:21Z

The concern is not that your medium post is itself phishing, but that you're distributing MetaMask-branded software through an insecure, unsigned channel. This habituates your users to installing software from unknown sources. This is seriously dangerous, and not only should you be concerned about your users getting used to this pattern, but we need to protect our own reputation when you distribute software branded with our name and logo in this insecure manner.

I'm getting some advice now on what the right path forward is, but I have a feeling that you shouldn't be distributing software with our brand that is modified, through insecure channels. It endangers both our user bases.

If you don't want to negotiate a merge into our main branch, please fork, rebrand, & redistribute under your own brand.

sjmackenzie · 2017-05-23T19:56:32Z

@FlySwatter Yeah I know you called your lawyers and now are not willing to merge this simple pull request.

We also included a *Not Officially Endorsed by Metamask.io*.

Secondly you yourself are distributing via insecure channels https://medium.com/metamask/how-to-install-metamask-on-firefox-today-670e03aa42d6 .

I quote:

Shields Down
Once you’re running Developer Edition, you’re going to need to visit the config page at about:config. This will give you a big scary warning that this may void your warranty, and it may! So heed it!
You then want to find the setting called xpinstall.signatures.required and double click it to say false.

Go Download MetaMask!
Our link on the Firefox store is here, and you can now use it!
Normal Firefox users will be able to use that link once we’ve been approved & signed by Mozilla.

As I said before we'd love for MetaMask to support us and the only reasonable request is the TLS support. The other stuff is beyond the scope of the PR and can easily be done in a separate PR.

The whole point of this Pull Request is to "habituate" users to use official channels.

Please advise.

kumavis · 2017-05-23T21:06:42Z

@FlySwatter Yeah I know you called your lawyers and now are not willing to merge this simple pull request.

@sjmackenzie we're actively reviewing this pull request. Our users depend on the integrity of the software-- we don't merge in code without careful consideration. Merging this would be the start of a longer relationship with the ETC dev community (managing support and compatibility). So far the experience has been a little confusing with compliments on one hand and urges to merge hastily on the other.

kumavis · 2017-05-23T21:33:00Z

There seems to be two components to this PR:

support for ETC networkId and chainId (EIP155 support) as well as etc currency conversion lookup
adding a new trusted provider to our network list

support for ETC networkId and chainId

This initially seems simple enough. It would be our first non-ETH addition, and so it deserves some more thought on how to organize this and support a different coin. We've been looking forward to multichain support, and ETC is a nice close-to-home place to start. Additionally, it would be nice if there was an RPC method to query for the chainId instead of having to hard code for this value.

adding a new trusted provider to our network list

The Ethereum JSON RPC is a trusted communication layer between the MetaMask UI and the network. We're not comfortable adding a new trusted third party that we have no close relationship with. We collaborate with Infura very closely in order to offer security, compatibility, and liveliness. Who runs ePool? Do we have permission to point thousands of users at their server? What are the side effects of a rush event like an ICO reducing availability of this server? Would it impact the mining pool's availability? Theres a lot to consider here.

We're open to merging the first part of that immediately, but the second part requires much more serious consideration. Ideally the PR would allow users to point at an ETC node until we have a more sustainable resource available.

ghost · 2017-05-23T22:16:20Z

Hi guys. I was asked to provide the API for this. Currently it's not using https. However I do provide the API for Mew which is https cloudflare protected. I would much rather that be used.

ghost · 2017-05-23T22:45:45Z

https://mewapi.epool.io

ghost · 2017-05-23T22:56:52Z

Sorry. To answer a couple other the questions. The node is totally independent from any of the pool servers. It is currently being used by Mew and is considered trusted to handle ETC MeW transactions which are quite large and sustained. There's never been a reported issue with the API node and it has been running for many months. the back-end uses nginx load balancing and can be scaled horizontally if the need presents itself. Hope answers all of the questions. Thanks

sorpaas · 2017-05-24T02:48:55Z

@FlySwatter @kumavis sorry about that incident. I was requested by an ETC user to add support for MetaMask, so I thought some curious users might just want to try out the build. Hence I included a link. Until this is settled, I removed the insecure build with a README file telling users that they need to build it themselves (see here).

For the RPC endpoint issue, are there other providers besides Infura you're trusting?

kumavis · 2017-05-24T03:07:43Z

@EtherNinja thanks for the info! tayvano from mew has vouched for the availability of the node. curious about your privacy and data retention policy for the rpc logs.

ghost · 2017-05-24T04:32:24Z

I do not store or maintain information beyond what would be required for standard troubleshooting. The Nginx logs are removed daily. I can make adjustments if necessity but storing nginx logs beyond 24 hours is not practical in my opinion.

The Parity logs are rotated daily and purged weekly. However, I don't believe there is any sensitive information contained there as they are set to the standard log level. Correct me if I'm wrong.

kumavis · 2017-05-24T05:19:04Z

@sorpaas thanks for your understanding and quick action removing the unofficial build. Instructions for building from source is a good interim solution.

As for the technical requirements of adding ETC support, first need to understand networkId vs chainId. I agree we are conflating the language for networkId and chainId, but in ETH mainnet and testnets these always line up. I'll define these terms as MetaMask experiences them:

networkId: the return value of net_version (despite legacy behavior)
chainId: The appropriate parameter for use in EIP155 transactions

Ethereum doesn't have a chainId. Ethereum Classic has chainId = 0x3d.

Ethereum Mainnet's chainId is 0x01 as can be confirmed via EIP155 transaction support. Odd that ETC chainId !== networkId but my guess is that was the most graceful minimal change.

My primary desire would be for ETC to change net_version to return 0x3d. This is still sort of pretending ETC is another ETH network (doesn't solve currency conversion or ticker symbol display).

To properly treat it as a separate coin/project will require more design and thinking. Right now we are assuming RPC is compliant across ETH and ETC. This may not be fully true now or in the future. Likewise with the web3 API. Going further, other chains may want a completely different API to be injected into pages.

Not trying to tie this up with analysis paralysis, but just trying to walk through the idea and identify that this endeavor is no one-and-done PR.

minimal fix metamask fix: add an option to disable EIP155 signatures

a DIY fix: put a reverse proxy in front of the rpc node that intercepts net_version and returns the chainId

kumavis · 2017-05-24T05:21:43Z

@EtherNinja thanks
mostly concerned about metadata like "address X is a MEW user".
and more so "address X has IP address Y".
glad to hear you clear logs regularly.

This commit also removes all other conditional code specific to classic

…into feature/classic

sorpaas · 2017-05-24T07:06:22Z

@kumavis I think before the refactoring is on, it is good enough right now to treat ETC as another ETH chain. So I used the net_version interception approach, and I think this is the version that has the minimal maintenance cost from your side.

Specifically, I removed all conditional for classic and added a new config item networkIdOverwrites. If the network controller finds the provider type has a value in networkIdOverwrites, it will use that value instead of what it got from net_version RPC call.

sorpaas · 2017-05-25T02:50:48Z

@FlySwatter @kumavis It would be great if you can review the current version of this PR soon. Are there other changes I need to make to get this PR merged?

danfinlay · 2017-05-25T20:51:05Z

@sorpaas We're a little backlogged at the moment, and will come back to this soon.

sorpaas · 2017-06-03T12:51:40Z

@FlySwatter @kumavis Any news on this? I would really want to get this PR merged, so please let me know if there's any other code clean-ups or things to be addressed. 😄 However, if the PR is staled for too long, I might need to consider forking and rebranding for the benefits of the ETC community.

Thanks for you help!

kumavis · 2017-06-03T19:27:26Z

There's some eslint errors

https://circleci.com/gh/MetaMask/metamask-plugin/2153?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

CI doesnt run on outside contributer's forks right now, which is really annoying

@sorpaas be sure to run npm test locally

…into feature/classic

sorpaas · 2017-06-03T19:48:27Z

@kumavis Just fixed. npm test is passing for me now.

jhansikiraani · 2017-06-06T20:47:50Z

Hi, I need help with my Mist wallet and the ETCs. Not sure it this is the right place to post this.

I recently sent some ETHs to my account on Poloniex and forgot about the split.

How can I recover the ETC from the wallet.

Please help!

kumavis · 2017-06-07T17:54:16Z

@jhansikiraani this is not the right place for support, ask in Mist or an ETC channel

Dexaran · 2017-07-11T21:23:25Z

Hi there,
I'm wondering how things are with the addition of ETC support in MetaMask. What are current problems and what prevents this PR from being merged now?

@kumavis @FlySwatter

kumavis · 2017-07-19T15:26:33Z

This is at the architectural design phase - will require significant refactor to cleanly handle multiple chains. Prioritizing core stability and network scalability first.

2-am-zzz · 2017-07-26T15:41:44Z

Encapsulated in #1820 until we can dedicate more time to this. Please redirect all concerns to that issue.

classic - reinit support

1bc7c1a

frankiebee added the type-discussion label May 23, 2017

frankiebee suggested changes May 23, 2017

View reviewed changes

ui/app/config.js Outdated

case 'classic':

title = 'Current Network'

value = 'Ethereum Classic Network'

Copy link

Contributor

frankiebee May 23, 2017

Choose a reason for hiding this comment

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

missing break

missing break in config for classic

b398a56

danfinlay requested changes May 23, 2017

View reviewed changes

danfinlay added the DO-NOT-MERGE Pull requests that should not be merged label May 23, 2017

sorpaas added 5 commits May 24, 2017 14:34

config: allow arbitrary network id overwrite not specific to classic

782f476

This commit also removes all other conditional code specific to classic

classic: networkId overwrites return string by default

2398ea5

classic: add buy-eth-url, account-link and explorer-link

7b8eb42

classic: make the transaction linkable

a6d8ce8

Merge branch 'master' of https://github.com/MetaMask/metamask-plugin …

e2d8ec3

…into feature/classic

sorpaas changed the title ~~[WIP] Ethereum Classic Support~~ Ethereum Classic Support May 24, 2017

config-manager: remove usued getNetworkIdOverwrite fn

6483d81

sorpaas added 2 commits June 4, 2017 03:35

Merge branch 'master' of https://github.com/MetaMask/metamask-plugin …

b0364b8

…into feature/classic

Fix lint warnings and errors

0ffd152

kumavis mentioned this pull request Jun 8, 2017

Adding expanse network #1555

Closed

danfinlay mentioned this pull request Jul 25, 2017

Multi-Chain Support #1820

Closed

2 tasks

2-am-zzz closed this Jul 26, 2017

danfinlay mentioned this pull request Jan 25, 2018

add POA.network support #2815

Closed

This comment has been minimized.

Sign in to view

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ethereum Classic Support #1476

Ethereum Classic Support #1476

sorpaas commented May 23, 2017 •

edited

Loading

frankiebee May 23, 2017

frankiebee commented May 23, 2017 •

edited

Loading

ProphetDaniel commented May 23, 2017

danfinlay left a comment

danfinlay May 23, 2017

danfinlay May 23, 2017

sjmackenzie commented May 23, 2017

danfinlay commented May 23, 2017

sjmackenzie commented May 23, 2017 •

edited

Loading

kumavis commented May 23, 2017

kumavis commented May 23, 2017

ghost commented May 23, 2017

ghost commented May 23, 2017

ghost commented May 23, 2017

sorpaas commented May 24, 2017

kumavis commented May 24, 2017

ghost commented May 24, 2017

kumavis commented May 24, 2017 •

edited

Loading

kumavis commented May 24, 2017

sorpaas commented May 24, 2017

sorpaas commented May 25, 2017

danfinlay commented May 25, 2017

sorpaas commented Jun 3, 2017

kumavis commented Jun 3, 2017

sorpaas commented Jun 3, 2017

jhansikiraani commented Jun 6, 2017

kumavis commented Jun 7, 2017

Dexaran commented Jul 11, 2017 •

edited

Loading

kumavis commented Jul 19, 2017

2-am-zzz commented Jul 26, 2017

This comment has been minimized.

Ethereum Classic Support #1476

Ethereum Classic Support #1476

Conversation

sorpaas commented May 23, 2017 • edited Loading

frankiebee May 23, 2017

Choose a reason for hiding this comment

frankiebee commented May 23, 2017 • edited Loading

ProphetDaniel commented May 23, 2017

danfinlay left a comment

Choose a reason for hiding this comment

danfinlay May 23, 2017

Choose a reason for hiding this comment

danfinlay May 23, 2017

Choose a reason for hiding this comment

sjmackenzie commented May 23, 2017

danfinlay commented May 23, 2017

sjmackenzie commented May 23, 2017 • edited Loading

kumavis commented May 23, 2017

kumavis commented May 23, 2017

support for ETC networkId and chainId

adding a new trusted provider to our network list

ghost commented May 23, 2017

ghost commented May 23, 2017

ghost commented May 23, 2017

sorpaas commented May 24, 2017

kumavis commented May 24, 2017

ghost commented May 24, 2017

kumavis commented May 24, 2017 • edited Loading

kumavis commented May 24, 2017

sorpaas commented May 24, 2017

sorpaas commented May 25, 2017

danfinlay commented May 25, 2017

sorpaas commented Jun 3, 2017

kumavis commented Jun 3, 2017

sorpaas commented Jun 3, 2017

jhansikiraani commented Jun 6, 2017

kumavis commented Jun 7, 2017

Dexaran commented Jul 11, 2017 • edited Loading

kumavis commented Jul 19, 2017

2-am-zzz commented Jul 26, 2017

This comment has been minimized.

sorpaas commented May 23, 2017 •

edited

Loading

frankiebee commented May 23, 2017 •

edited

Loading

sjmackenzie commented May 23, 2017 •

edited

Loading

kumavis commented May 24, 2017 •

edited

Loading

Dexaran commented Jul 11, 2017 •

edited

Loading