Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

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
Collaborator

@sorpaas sorpaas 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

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
Copy link

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

@5chdn 5chdn added A0-pleasereview 🤓 Pull request needs code review. M6-rpcapi 📣 RPC API. labels Aug 18, 2017
@sorpaas
Copy link
Collaborator Author

sorpaas commented Aug 18, 2017

[clabot:check]

@parity-cla-bot
Copy link

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

Many thanks,

Parity Technologies CLA Bot

@tcz001
Copy link

tcz001 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
Copy link

tcz001 commented Aug 21, 2017

Just created a EIP694

Copy link
Collaborator

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

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> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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
Copy link
Contributor

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

@tcz001
Copy link

tcz001 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
Copy link
Contributor

whilei 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
Copy link

tcz001 commented Aug 22, 2017

Thx, will close that issue

@tomusdrw
Copy link
Collaborator

tomusdrw 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.

@tomusdrw tomusdrw added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Sep 4, 2017
@gavofyork
Copy link
Contributor

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

u64 returns decimal integer, and there seems to be no type called
U64. So here I use U256 to return the hex integer.
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> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

  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)).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@tomusdrw Where's the U64 struct defined?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

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))
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@sorpaas
Copy link
Collaborator Author

sorpaas commented Sep 18, 2017

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

@gavofyork gavofyork added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Sep 26, 2017
@gavofyork
Copy link
Contributor

@tomusdrw @rphmeier please verify all good.

@tomusdrw tomusdrw added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Sep 26, 2017
@gavofyork gavofyork merged commit d8af9f4 into openethereum:master Sep 26, 2017
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 pushed a commit that referenced this pull request Oct 20, 2018
* Support eth_chainId RPC method

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

* rpc: remove parity_chainId
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M6-rpcapi 📣 RPC API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants