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

Initial multi-chain support (aka. optional chainid fix) #4932

Closed

Conversation

hackmod
Copy link
Contributor

@hackmod hackmod commented Aug 1, 2018

from #4516

Description

The customRpc feature support by MetaMask somewhat buggy.
Currently, metamask get the chainId using net_version rpc call, but net_version does not return the real chainId. for these cases, we need to know real chainId to support EIP155 replay protection correctly.

more over, In most case of ethereum forks and variants, chainId != networkId.

This PR has been incubated at #4516 (about two months) and simplified. It is almost minimal fix to support the following.

  • user configurable customRPCs with their chainIds to support EIP155 signing for networkId != chainId cases.
  • Ethereum Classic support added (a pre-defined custom RPC, indeed)

above two features share their code, ETC support is implemented as a predefined customRPC.
that's why I do not separate this fix into two PRs.

$ git diff 4f02726fd9a |diffstat
 app/_locales/en/messages.json                                  |    9
 app/images/etc_logo.svg                                        |  468 ++++++++++
...
 50 files changed, 1146 insertions(+), 172 deletions(-)

net changes are only +678, -172 lines

InfuraProvider() for ethereum, _configureStandardProvider() for customRPC are not much configurable.
this customRPC implementation is a prof-of-concept quick hack.
not much nice looking but ready to be merged, work nicely and small enough for incremental development.

Done

  • fixed custom Rpc form to support optional chainid.
    • both old-ui and new-ui
  • ETC support added
    • basically, this is a predefined rpcUrl + chainId
  • show ticker properly (6/21)
  • block explorer urls (6/21)
  • support shapeshift exchange for deposit(6/22)
  • support testnet faucet (buyUrl) (6/23)
  • [x] fixed frequentRpcList bug.
  • [x] increase frequentRpcList length by 1
  • pass circleci (Support etc optional chainid hackmod/metamask-extension#1) (8/2)

changelog

  • 8/16 use ETC cooperative's api server for ETC.
  • 8/16 update ETC logo.
  • 8/21 simplified, remove some logs.

References

See also

Screenshot

image

user configured custom RPCs (upto 3)

image

example. upto 3 customRpc supported (previously, 2 supported)
image

(old-ui)
image

@hackmod hackmod force-pushed the support-etc-optional-chainid branch 3 times, most recently from 55a30bb to d7d3ecf Compare August 2, 2018 08:11
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

@hackmod I think Metamask will avoid adding bunch of providers and chains, instead we need optional chain fix only without ethereum classic support

@hackmod hackmod force-pushed the support-etc-optional-chainid branch from d7d3ecf to 470a4c2 Compare August 16, 2018 15:37
@hackmod
Copy link
Contributor Author

hackmod commented Aug 16, 2018

rebased, resolved conflict, changed ETC logo/rpcUrl

$ git diff 955ec2dca62 |diffstat
 app/_locales/en/messages.json                                  |    9
 app/images/etc_logo.svg                                        |   20 ++
...
 51 files changed, 695 insertions(+), 172 deletions(-)

now net changes are +675, -172 lines (svg file changes are not counted)

@hackmod hackmod force-pushed the support-etc-optional-chainid branch from 470a4c2 to 3c3e96c Compare August 16, 2018 15:47
@hackmod
Copy link
Contributor Author

hackmod commented Aug 16, 2018

@akx20000a

I think Metamask will avoid adding bunch of providers and chains...

what bunch of providers and chains?? this PR only include ETC chain, no other providers added at all.

@hackmod hackmod force-pushed the support-etc-optional-chainid branch 2 times, most recently from 1ad9043 to 9cbb649 Compare August 21, 2018 04:58
@chrisfranko
Copy link

I think its pretty apparent that after 2 years of various networks trying to get integrated into metamask its just not going to happen. Why not just strip the etc stuff from this PR and submit it again with just the optional chain_id for added networks? I know its not as glamorous but it would be nice to move this along.

@bdresser
Copy link
Contributor

bdresser commented Aug 23, 2018

hey @hackmod, sorry for the slow response here.

@chrisfranko is right - we’re not currently interested in adding Classic to our network menu, as discussed here: https://medium.com/metamask/metamasks-vision-for-multiple-network-support-4ffbee9ec64d

We are open to adding support for the chainId parameter, though, which would allow users to easily opt into Classic support if they want to. Looks like @chrisfranko opened a separate PR with that feature set (#5130) - I'll comment there about a couple lingering concerns from the team (specifically around changes to the block explorer and price API).

Long term, we are interested in ways of supporting different chains without having to impose the politics of a supported network list, but right now, we believe the addition of an extra chain named “Ethereum” would only serve to confuse our already very vulnerable userbase. I hope that makes sense!

 * fixed custom Rpc form to support optional chainid
 * experimental ETC support added

[fixed conflict 8/16]
 * fixup for testnet faucet
@hackmod hackmod force-pushed the support-etc-optional-chainid branch 3 times, most recently from 159a695 to b81c4e3 Compare August 24, 2018 08:27
@hackmod
Copy link
Contributor Author

hackmod commented Aug 24, 2018

  • use MultiChain option added. (FIXME)

image

@hackmod hackmod force-pushed the support-etc-optional-chainid branch from a453eb4 to 50b1e66 Compare August 24, 2018 08:44
@ghost
Copy link

ghost commented Aug 24, 2018

@hackmod Can you please split your PR one for ETC support and one for optional chainid??

Your PR changes price api and lots of stuffs that Metamask will avoid merging this, please split your PR

@hackmod
Copy link
Contributor Author

hackmod commented Aug 24, 2018

@chrisfranko @bdresser @eosclassicteam // OK. Ive made new PR to separate all other stuff except optional chainId. Please see PR #5134

@bdresser
Copy link
Contributor

bdresser commented Oct 2, 2018

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants