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

support optional chainId #4516

Closed
wants to merge 6 commits into from

Conversation

hackmod
Copy link
Contributor

@hackmod hackmod commented Jun 5, 2018

This is an experimental work (WIP)

Basically this PR is based on the net_version hack (by @turbobits)

Currently, MetaMask get the chainId using net_version rpc call but net_version does not return the real chainId. there are no net_chainid nor eth_chainid rpc call exist. ethereum/go-ethereum#15002

  • In most case of ethereum variants, chainId != networkId.
  • This is a quick hack to support optional chainid in the custom Rpc.
  • experimental ETC support added

Done

  • fixed custom Rpc form to support optional chainid.
    • support both old-ui and new-ui
  • experimental ETC support added
    • basically this is a predefined rpcUrl + chainId
  • ticker (6/21)
  • block explorer urls (6/21)
  • support shapeshift exchange for deposit(6/22)
  • support testnet faucet (6/23)
    • [x] MORDEN testnet added
  • more configurable settings like as "blockExplorerUrl"
    • rpcUrl, chainId, blockExplorerUrl, ticker symbol
  • increased customRpcList size from 2 to 3

ToDo

  • QA test / fix possible bugs
  • sanity check rpcUrl
  • [x] FIXME morden api server url

need some feed back!

References

Screenshot

image

image

image

image

image

image

@shd101wyy
Copy link

This pull request is awesome. Thanks a lot. I also have the demand to set the chainId.
Hope the maintainer can review your pull request soon. Good job.

@@ -18,6 +18,7 @@ module.exports = function (address, network) {
link = `https://kovan.etherscan.io/address/${address}`
break
default:
// FIXME
Copy link
Contributor

Choose a reason for hiding this comment

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

Does GasTracker.io work for this purpose?

Copy link

Choose a reason for hiding this comment

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

yes, would be https://gastracker.io/addr/${address}

@wjmelements wjmelements changed the base branch from master to develop June 8, 2018 22:38
@wjmelements
Copy link
Contributor

Infura doesn't support ETC, so you would need to provide a default RPC provider or enforce a custom RPC URL for non-Infura networks.

@splix
Copy link

splix commented Jun 9, 2018

Does Infura provides something specific for metamask, or standard web3 JSON RPC would work? If later works, there is one at https://web3.gastracker.io

@wjmelements
Copy link
Contributor

@splix I believe they would only have to support the RPC methods listed at https://github.com/MetaMask/provider-engine#current-rpc-method-support

Based on this article they may support them.

case 'classic':
title = 'Current Network'
value = 'Ethereum Classic Network'
break
Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly, you could add the ETC Testnet, Morden.

@wjmelements
Copy link
Contributor

I'm surprised information about a network is not already centralized in an object. That might be a good refactor, though perhaps outside the scope of this pull request.

@kumavis
Copy link
Member

kumavis commented Jun 14, 2018

Another issue here is showing conversion rate
we're still working on breaking out networks for first class treatment which will enable non-evm blockchains as well, but longer time line

'blockExplorerTX': 'https://gastracker.io/tx/[[txHash]]',
'blockExplorerAddr': 'https://gastracker.io/addr/[[address]]',
'service': 'epool.io',
'rpcUrl': 'https://mew.epool.io',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

like as myetherwallet's app/scripts/nodes.js, the networks.js is the predefined rpcUrl + chainId and more.

@hackmod
Copy link
Contributor Author

hackmod commented Jun 21, 2018

screenshot updated, etc.

@hackmod hackmod force-pushed the chainid-fix branch 4 times, most recently from c3d16ec to 722802f Compare June 22, 2018 04:29
@pyskell
Copy link

pyskell commented Jun 22, 2018

BUG: Doesn't support sending transaction data.

Tried sending a transaction with: 0x657463206f6e206d6574616d61736b20697320776f726b696e6721
image

Result:
image

@hackmod
Copy link
Contributor Author

hackmod commented Jun 22, 2018

@hackmod
Copy link
Contributor Author

hackmod commented Jun 22, 2018

@hackmod hackmod force-pushed the chainid-fix branch 2 times, most recently from 0a401b8 to 4d02967 Compare June 27, 2018 05:27
'blockExplorerAddr': 'https://gastracker.io/addr/[[address]]',
'blockExplorerToken': 'https://gastracker.io/token/[[tokenAddress]]/[[address]]',
'service': 'Ethereum Commonwealth',
'rpcUrl': 'https://etc-geth.0xinfra.com',
Copy link
Contributor Author

@hackmod hackmod Jun 27, 2018

Choose a reason for hiding this comment

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

where can I find api server for the modern testnet?

https://web3.gastracker.io/morden not correctly work...

'blockExplorerAddr': 'https://gastracker.io/addr/[[address]]',
'blockExplorerToken': 'https://gastracker.io/token/[[tokenAddress]]/[[address]]',
'service': 'Gastracker.io',
'rpcUrl': 'https://web3.gastracker.io/morden',
Copy link
Contributor Author

@hackmod hackmod Jun 27, 2018

Choose a reason for hiding this comment

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

https://web3.gastracker.io/morden is not work correctly with MetaMask.
@pyskell // is there any official api-server/block explorer exist for the Morden testnet ?

http://mordenexplorer.ethertrack.io looks good to me.

@hackmod hackmod force-pushed the chainid-fix branch 3 times, most recently from 1a3c090 to 8d26a69 Compare July 3, 2018 00:50
'blockExplorerAddr': 'http://mordenexplorer.ethertrack.io/addr/[[address]]',
'blockExplorerToken': 'http://mordenexplorer.ethertrack.io/token/[[tokenAddress]]/[[address]]',
'service': 'Gastracker.io',
'rpcUrl': 'https://etc-geth.0xinfra.com',
Copy link
Contributor Author

@hackmod hackmod Jul 9, 2018

Choose a reason for hiding this comment

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

FIXME is there any official json rpc api-server/block explorer exist for the Morden testnet ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe contact the ETC rpc providers about adding support for the testnet. If there isn't a reliable rpc provider for the ETC testnet, don't list it among the supported networks.

Copy link

Choose a reason for hiding this comment

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

@hackmod Remove modern testnet, no one is using ethereum classic testnet for testing purpose. Why do we need the option??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. as you already know, this is a quick hack and I'm not much familiar with a react program.
we need more testing to find possible bugs. that's the reason the morden testnet added.

Copy link

@ghost ghost Jul 17, 2018

Choose a reason for hiding this comment

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

@hackmod I've opened a public rpc endpoint for morden testnet

https://morden.eos-classic.io

Hope we can go forward 👍

Copy link

Choose a reason for hiding this comment

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

And my personal opinion is, we don't have to suppot additional api endpoint for chain-id.

It is needed however we can just hardcode for this case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

temporary added morden api server.
but the api server provided by other coin project seems improper to me.

Copy link

Choose a reason for hiding this comment

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

@hackmod Then remove useless morden testnet menu, we've added the endpoint only for this PR merge if you don't need it we will close then

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed and rebased

@hackmod
Copy link
Contributor Author

hackmod commented Jul 18, 2018

current screenshot

image

image

image

image

@hackmod
Copy link
Contributor Author

hackmod commented Jul 18, 2018

more configurable options for other ethereum networks or private network

TODO

  • error handle

image

@hackmod
Copy link
Contributor Author

hackmod commented Jul 20, 2018

  • increased customRpcList size from 2 to 3
    image

@MetaMask MetaMask deleted a comment Jul 24, 2018
@MetaMask MetaMask deleted a comment Jul 24, 2018
@MetaMask MetaMask deleted a comment from wjmelements Jul 24, 2018
@PeterTheOne
Copy link

I've tried it with a custom RPC and ticker symbol, seems to work fine, didn't encounter any bugs. keep up the good work!

@danfinlay
Copy link
Contributor

This is nice work, but we are not ready to add a bunch of new networks and providers to our menu. If all this PR did was add an optional chainId parameter, we would merge it much more easily, but it instead adds many other things as well, which we are simply not ready to do at this time.

You can read more about our roadmap and vision for supporting multiple networks here:
https://medium.com/metamask/metamasks-vision-for-multiple-network-support-4ffbee9ec64d

@danfinlay danfinlay closed this Jul 25, 2018
@hackmod
Copy link
Contributor Author

hackmod commented Jul 28, 2018

bunch of new networks and providers to our menu

@danfinlay
I guess you miss understood this PR

the following screen shot is mere an example. all question mark prefixed items are configured by user by means of settings menu (Custom RPCs).
image

image

image

and the only newly added menu item is ETC.

optional chain_id feature and pre-configured menu(ETC) shares their code, that's why this PR named as "optional chain Id"

anyway, I will separate this issue into several PRs, for example,

  • optional chain id
  • more configurable settings for custom Rpc
  • ETC support

@34r7h
Copy link

34r7h commented Oct 13, 2018

Hey guys and gals. Everything looks good. Anything we still need to help out with to get it shipped with ETC cupport?

@bdresser
Copy link
Contributor

@irthos we've moved to #5134 which we'll review this week once we get #4510 merged!

@34r7h
Copy link

34r7h commented Oct 15, 2018 via email

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.

10 participants