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

Add RPC eth_chainId for querying the current blockchain chain ID #6329

Merged
merged 12 commits into from Sep 26, 2017

Conversation

@sorpaas
Copy link
Member

commented Aug 18, 2017

Currently although we can use net_version RPC call to get the
current network ID, there's no RPC for querying the chain ID. This
makes it impossible to determine the current actual blockchain using
the RPC. An ETH/ETC client can accidentally connect to an ETC/ETH RPC
endpoint without knowing it unless it tries to sign a transaction or
it fetch a transaction that is known to have signed with a chain
ID. This has since caused trouble for application developers, such as
MetaMask, to add multi-chain support.

The same RPC endpoint is also about to be merged for ETC's
go-ethereum: ethereumproject/go-ethereum#336

Add RPC eth_chainId for querying the current blockchain chain ID
Currently although we can use `net_version` RPC call to get the
current network ID, there's no RPC for querying the chain ID. This
makes it impossible to determine the current actual blockchain using
the RPC. An ETH/ETC client can accidentally connect to an ETC/ETH RPC
endpoint without knowing it unless it tries to sign a transaction or
it fetch a transaction that is known to have signed with a chain
ID. This has since caused trouble for application developers, such as
MetaMask, to add multi-chain support.

The same RPC endpoint is also about to be merged for ETC's
go-ethereum: ethereumproject/go-ethereum#336
@parity-cla-bot

This comment has been minimized.

Copy link

commented Aug 18, 2017

It looks like @sorpaas hasn'signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence.
Wikipedia

You can read and sign our full Contributor License Agreement at the following URL: https://cla.parity.io

Once you've signed, plesae reply to this thread with [clabot:check] to prove it.

Many thanks,

Parity Technologies CLA Bot

@sorpaas sorpaas force-pushed the sorpaas:rpc/eth-chainId branch from 67ef961 to 7f8f215 Aug 18, 2017

@sorpaas

This comment has been minimized.

Copy link
Member Author

commented Aug 18, 2017

[clabot:check]

@parity-cla-bot

This comment has been minimized.

Copy link

commented Aug 18, 2017

It looks like @sorpaas signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

@tcz001

This comment has been minimized.

Copy link

commented Aug 21, 2017

+1 on this PR, I faced with this problem when using web3.js which is using the getId by default

@tcz001

This comment has been minimized.

Copy link

commented Aug 21, 2017

Just created a EIP694

@tomusdrw
Copy link
Contributor

left a comment

A small grumble regarding types.

Also this pollutes eth_* namespace and it has been agreed that those namespaces are reserved only for cross-client compatbile RPC methods that preferrable has gone through the EIP process.

@@ -284,6 +284,11 @@ impl<C, SN: ?Sized, S: ?Sized, M, EM> Eth for EthClient<C, SN, S, M, EM> where
Ok(format!("{}", version))
}

fn chain_id(&self) -> Result<String, Error> {

This comment has been minimized.

Copy link
@tomusdrw

tomusdrw Aug 21, 2017

Contributor

Chain id in the transactions is serialized as Option<u64>, would be best to keep it constistent.
IMHO using a generic String response is a bit misleading. If we want to return a hex-encoded value then Option<U64> would be a right choice.

Note that using Option indicates that in case the chain id is missing null will be returned.

@rphmeier

This comment has been minimized.

Copy link
Member

commented Aug 22, 2017

I also disagree with polluting the eth namespace just yet, but am definitely not opposed to the idea in general.

@tcz001

This comment has been minimized.

Copy link

commented Aug 22, 2017

Agree, an int in string type would be better for extension and compatibility here, how about changing it in EIP694 @sorpaas & @whilei, any thoughts? In this case we need to modify the etc geth implementation accordingly.

@whilei

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2017

@tcz001 I've created an EIP-rpc_chain_id as a PR (since it should be a PR rather than an issue) which is almost exactly like your issue EIP694 -- maybe we can switch to using the PR as a reference?

I used a bigint in hexadecimal because that's in line with the rest of the eth_-namespaced returnables, eg. https://github.com/ethereumproject/go-ethereum/wiki/JSON-RPC#eth_blocknumber.

@tcz001

This comment has been minimized.

Copy link

commented Aug 22, 2017

Thx, will close that issue

@tomusdrw

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2017

Build failure seems unrelated. Will be happy to merge under parity_chainId. After the method is accepted via EIP we can rename it to eth_chainId.

@gavofyork

This comment has been minimized.

Copy link
Member

commented Sep 16, 2017

Tests fail with RPC_API_Test...error: An unknown error occurred.

sorpaas added 3 commits Sep 17, 2017
Use U256 to return chainId and fix for master
u64 returns decimal integer, and there seems to be no type called
U64. So here I use U256 to return the hex integer.
Fix chainID test
Before EIP155 fork number, chainID should be null.
@@ -200,6 +200,10 @@ impl<C, M, U> Parity for ParityClient<C, M, U> where
Ok(self.settings.chain.clone())
}

fn chain_id(&self) -> Result<Option<U256>, Error> {

This comment has been minimized.

Copy link
@tomusdrw

tomusdrw Sep 18, 2017

Contributor
  1. minor: U64 would be enough
  2. As mentioned earlier chain_id is already returned as decimal (u64) in transactions and I'd prefer consistency (either all U64 (hex) or all u64 (decimal)).

This comment has been minimized.

Copy link
@sorpaas

sorpaas Sep 18, 2017

Author Member

@tomusdrw Where's the U64 struct defined?

This comment has been minimized.

Copy link
@tomusdrw

tomusdrw Sep 18, 2017

Contributor

Thought we already have it, but seems we only have U128 and U256. You can create U64 in types/uint.rs.

Change both parity_chainId and transaction::chainId to use U64
This makes it consistent that all chain ids returned are hex string.

impl serde::Serialize for U64 {
fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error> where S: serde::Serializer {
serializer.serialize_str(&format!("0x{}", self.0))

This comment has been minimized.

Copy link
@rphmeier

rphmeier Sep 18, 2017

Member

i don't think this will actually format as hex. maybe {:h}? @tomusdrw should it be left-padded with zeros?

@sorpaas

This comment has been minimized.

Copy link
Member Author

commented Sep 18, 2017

gitlab-test-rust-stable was failed due to a connection timeout. Glad if someone can help to restart it.

@gavofyork

This comment has been minimized.

Copy link
Member

commented Sep 26, 2017

@tomusdrw @rphmeier please verify all good.

@gavofyork gavofyork merged commit d8af9f4 into paritytech:master Sep 26, 2017

2 checks passed

continuous-integration/gitlab-js-test Build stage: test; status: success
Details
continuous-integration/gitlab-test-rust-stable Build stage: test; status: success
Details
@axic axic referenced this pull request Jan 26, 2018
@hackmod hackmod referenced this pull request Oct 19, 2018
hackmod added a commit to EthersocialNetwork/parity-ethereum that referenced this pull request Oct 19, 2018
hackmod added a commit to EthersocialNetwork/parity-ethereum that referenced this pull request Oct 19, 2018
hackmod added a commit to EthersocialNetwork/parity-ethereum that referenced this pull request Oct 19, 2018
sorpaas added a commit that referenced this pull request Oct 20, 2018
Support eth_chainId RPC method (#9783)
* Support eth_chainId RPC method

 * ethereum/EIPs#695
 * Original PR is #6329

* rpc: remove parity_chainId
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.